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

[Felipe Balbi <balbi@ti.com>]

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

Hi,

On Tue, Jul 30, 2013 at 01:04:28PM +0530, Sourav Poddar wrote:
> 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

some formatting issues in the mail, but good that you looked at the
code. Cheers

-- 
balbi

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