Re: [PATCHv4 2/3] drivers: spi: Add qspi flash controller

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2703 bytes --]

On Thu, Jul 18, 2013 at 05:15:45PM +0530, Sourav Poddar wrote:
> On Thursday 18 July 2013 04:12 PM, Mark Brown wrote:

> >>+	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?

> We are checking if there is any transfer left, if no we are signalling the
> flash device about the end of transfer.

Please be more specific.  How will you ever reach the end of the
transfer list in a way which wouldn't cause the for loop to terminate?

> >>+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?

> Yes, there is WC interrupt enable bit which enables the interrupt.
> This interrupt
> gets disabled by writing to the CLEAR reg in the threaded irq.

So why do we report that we handled the interrupt then?  Shouldn't we at
least warn if we're getting spurious IRQs?

> >>+	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?
> I am not sure about the exact flow. If we see the api description,
> it says about irq getting
> freed automatically.
> Practically, I will check that on removing the driver, cat
> /proc/interrupts  should not show
> the required interrupt getting registered.
> Though, I see an api also existing "devm_free_irq", which explicitly
> un allocate your irq requested
> through devm_* variants.

You're missing the point here - how can you be sure that the interrupt
can't fire?

> 
> 
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]