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

[Sourav Poddar <sourav.poddar@ti.com>]

Hi Felipe,
On Monday 29 July 2013 06:05 PM, Felipe Balbi wrote:
> 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
>
We dont need IRQF_NO_SUSPEND flag in our qspi controller.

Qspi driver need to be prevented from receiving interrupts during system 
wide suspend/hibernation.
"suspend_device_irqs" in kernel/irq/pm.c marks interrupt line in use and 
sets IRQS_SUSPENDED
for them.
If we use "IRQF_NO_SUSPEND", we will bypass setting this IRQS_SUSPENDED 
flag, which is not.
desired


For this, interrupt lines need to
  and this function is provided for this purpose.
  * It marks all interrupt lines in use, except for the timer ones, as 
disabled
  * and sets the IRQS_SUSPENDED flag for each of them.

This flag gets used in __disable_irq api(kernel/irq/manage.c) which