Hi Mark,
On Thursday 18 July 2013 04:12 PM, Mark Brown wrote:
> On Thu, Jul 18, 2013 at 03:31:26PM +0530, Sourav Poddar wrote:
>
>> QSPI is a kind of spi module that allows single,
>> dual and quad read access to external spi devices. The module
>> has a memory mapped interface which provide direct interface
>> for accessing data form external spi devices.
> Have you seen the ongoing thread about SPI buses with extra data lines?
> How does this driver fit in with that?
>
>> --- a/drivers/spi/Makefile
>> +++ b/drivers/spi/Makefile
>> @@ -46,6 +46,7 @@ obj-$(CONFIG_SPI_OCTEON) += spi-octeon.o
>> obj-$(CONFIG_SPI_OMAP_UWIRE) += spi-omap-uwire.o
>> obj-$(CONFIG_SPI_OMAP_100K) += spi-omap-100k.o
>> obj-$(CONFIG_SPI_OMAP24XX) += spi-omap2-mcspi.o
>> +obj-$(CONFIG_QSPI_DRA7xxx) += spi-ti-qspi.o
>> obj-$(CONFIG_SPI_ORION) += spi-orion.o
>> obj-$(CONFIG_SPI_PL022) += spi-pl022.o
>> obj-$(CONFIG_SPI_PPC4xx) += spi-ppc4xx.o
> Please use SPI_ like the other drivers.
>
>> +static int ti_qspi_prepare_xfer(struct spi_master *master)
>> +{
>> + struct ti_qspi *qspi = spi_master_get_devdata(master);
>> + int ret;
>> +
>> + ret = pm_runtime_get_sync(qspi->dev);
>> + if (ret< 0) {
>> + dev_err(qspi->dev, "pm_runtime_get_sync() failed\n");
>> + return ret;
>> + }
>> +
>> + return 0;
>> +}
> This is a very common pattern, it should probably be factored out into
> the core, probably not even as ops but rather as an actual feature.
>
I see, every other driver doing it this way right now.
Is it ok, if I take your above idea as an seperate excercise after this
patch.?
>> + list_for_each_entry(t,&m->transfers, transfer_list) {
>> + qspi->cmd |= QSPI_WLEN(t->bits_per_word);
>> + qspi->cmd |= QSPI_WC_CMD_INT_EN;
>> +
>> + ret = qspi_transfer_msg(qspi, t);
>> + if (ret) {
>> + dev_dbg(qspi->dev, "transfer message failed\n");
>> + return -EINVAL;
>> + }
>> +
>> + m->actual_length += t->len;
>> +
>> + if (list_is_last(&t->transfer_list,&m->transfers))
>> + goto out;
>> + }
> The use of list_is_last() here is *realy* strange - what's going on
> there?
>
>> +static irqreturn_t ti_qspi_isr(int irq, void *dev_id)
>> +{
>> + struct ti_qspi *qspi = dev_id;
>> + u16 mask, stat;
>> +
>> + irqreturn_t ret = IRQ_HANDLED;
>> +
>> + spin_lock(&qspi->lock);
>> +
>> + stat = ti_qspi_readl(qspi, QSPI_SPI_STATUS_REG);
>> + mask = ti_qspi_readl(qspi, QSPI_INTR_ENABLE_SET_REG);
>> +
>> + if (stat&& mask)
>> + ret = IRQ_WAKE_THREAD;
>> +
>> + spin_unlock(&qspi->lock);
>> +
>> + return ret;
> According to the above code we might interrupt for masked events...
> that's a bit worrying isn't it?
>
>> + ret = devm_request_threaded_irq(&pdev->dev, irq, ti_qspi_isr,
>> + ti_qspi_threaded_isr, IRQF_NO_SUSPEND | IRQF_ONESHOT,
>> + dev_name(&pdev->dev), qspi);
>> + if (ret< 0) {
>> + dev_err(&pdev->dev, "Failed to register ISR for IRQ %d\n",
>> + irq);
>> + goto free_master;
>> + }
> Standard question about devm_request_threaded_irq() - how can we be
> certain it's safe to use during removal?
------------------------------------------------------------------------------
See everything from the browser to the database with AppDynamics
Get end-to-end visibility with application monitoring from AppDynamics
Isolate bottlenecks and diagnose root cause in seconds.
Start your free trial of AppDynamics Pro today!
http://pubads.g.doubleclick.net/gampad/clk?id=48808831&iu=/4140/ostg.clktrk
_______________________________________________
spi-devel-general mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/spi-devel-general
