Re: [PATCHv6 1/2] drivers: spi: Add qspi flash controller

[Felipe Balbi <balbi@ti.com>]

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

Hi,

On Mon, Jul 29, 2013 at 04:45:02PM +0530, Sourav Poddar wrote:
> >>>>>>+	irq = platform_get_irq(pdev, 0);
> >>>>>>+	if (irq<    0) {
> >>>>>>+		dev_err(&pdev->dev, "no irq resource?\n");
> >>>>>>+		return irq;
> >>>>>>+	}
> >>>>>>+
> >>>>>>+	spin_lock_init(&qspi->lock);
> >>>>>>+
> >>>>>>+	qspi->base = devm_ioremap_resource(&pdev->dev, r);
> >>>>>>+	if (IS_ERR(qspi->base)) {
> >>>>>>+		ret = PTR_ERR(qspi->base);
> >>>>>>+		goto free_master;
> >>>>>>+	}
> >>>>>>+
> >>>>>>+	ret = devm_request_threaded_irq(&pdev->dev, irq, ti_qspi_isr,
> >>>>>>+			ti_qspi_threaded_isr, IRQF_NO_SUSPEND | IRQF_ONESHOT,
> >>>>>why do you need IRQF_NO_SUSPEND ?
> >>>>>
> >>>>I should get away with this.
> >>>why ? Do you need or do you *not* need it ? And in either case, why ?
> >>>
> >>I was thinking, this will keep the irqs up even when we are
> >>hitting suspend, we will not be prepared to handle it. ?
> >won't be prepared in what way ?
> >
> Our driver will be down, so the irq might go un-serviced.

only if you wrote the driver that way. IRQ subsystem doesn't know the
state of the device, all it knows is that in case an IRQ fires, it needs
to call the registered IRQ handler.

Now the thing is:

Initially you had the flag setup, so I asked why you needed it. I
expected you to tell me why you think QSPI's IRQ shouldn't be disabled
during suspend and the implications of disabling it.

Instead you just changed your mind and decided to remove the flag.

Because you changed your mind with no explanation, that tells me you
haven't fully grasped how that flag works and what it means to set (or
not set) it.

My question now is simply: why don't you need that flag ? What are the
implications of setting that flag ? How would your driver behave if an
IRQ fired while your driver was suspended ? Unless you understand what
it does, how can you understand the behavior or the driver ?

cheers

-- 
balbi

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