Re: [patch]GPIO button is supposed to wake the system up if the wakeup attribute is set

["Li, Aubrey" <aubrey.li@linux.intel.com>]

On 2014/4/21 3:39, Dmitry Torokhov wrote:
> On Fri, Apr 18, 2014 at 01:23:11PM +0800, Li, Aubrey wrote:
>> On 2014/4/18 7:54, Dmitry Torokhov wrote:
>>> Hi Aubrey,
>>>
>>> On Fri, Apr 18, 2014 at 12:42:24AM +0800, Li, Aubrey wrote:
>>>> On 2014/4/16 20:35, Laxman Dewangan wrote:
>>>>> On Tuesday 15 April 2014 09:48 PM, Li, Aubrey wrote:
>>>>>> On 2014/4/15 20:38, Laxman Dewangan wrote:
>>>>>>> On Monday 14 April 2014 09:12 PM, Li, Aubrey wrote:
>>>>>>>> ping...
>>>>>>>>
>>>>>>>> On 2014/4/10 18:48, One Thousand Gnomes wrote:
>>>>>>>>>
>>>>>>> I think when we say irq_wake_enable() then based on underlying HW, it
>>>>>>> should not turn off the irq if it is require for the wakeup. I mean it
>>>>>>> need to be handle in the hw specific callbacks to keep enabling the
>>>>>>> wakeup irq on suspend also.
>>>>>> I failed to see why this can't be generic to all of the GPIO buttons for
>>>>>> suspend wakeup. Do you see any cases broken by this proposal?
>>>>>
>>>>> My point here is that if underlying HW needs to have irq enabled for
>>>>> wakup then it need to handle in centralized location i.e. the driver
>>>>> which is implementing it for the irq callbacks.
>>>>> Otherwise, we need to change this on multiple places who needs wakeups
>>>>> which is vast in nature like sd driver for sdcard insert/remove etc.
>>>>> almost all drivers which need wakeups through GPIOs.
>>>>
>>>> I think we have to handle this driver by driver. I didn't see how can we
>>>> make it in a centralized location. Looking forward to see your proposal.
>>>>
>>>>>
>>>>>>> For me, I have key which is interrupt based from PMIC, not based on GPIO
>>>>>>> and on that if I set it to IRQF_EARLY_RESUME then it works fine.
>>>>>>>
>>>>>> IRQF_NO_SUSPEND - Do not disable this IRQ during suspend
>>>>>> IRQF_EARLY_RESUME - Resume IRQ early during syscore instead of at device
>>>>>> resume time.
>>>>>>
>>>>>> IRQF_NO_SUSPEND is exactly what I want, instead of IRQF_EARLY_RESUME.
>>>>>> Can you please send your proposal/code to help me understand why this
>>>>>> has to hw specific and why IRQF_EARLY_RESUME is better than
>>>>>> IRQF_NO_SUSPEND?
>>>>>
>>>>> IRQF_EARLY_RESUME helps to re-enable mask or irq before parent interrupt
>>>>> resume and so parent isr handler sees the irq flag enabled when it try
>>>>> to scan source of interrupt. Otherwise parent isr handler treat this as
>>>>> spurious interrupt and does not call handler as irq flag disabled for that.
>>>>>
>>>>> This only happen when on resume, parent inettrupt enabled before the
>>>>> child interrupt on irq resume. Because as soon as parent isr re-enabled
>>>>> on resume, its hadnler get called before actually child interrupt
>>>>> enabled. This is what I observed mainly on PMIC and its sub irq. Not
>>>>> observed on SoC level of interrupts.
>>>>>
>>>>
>>>> This is expected behavior. I think I still need IRQF_NO_SUSPEND here.
>>>> What I want is, this IRQ is able to generate pm wakeup event to wake the
>>>> system up. It's enough for my case.
>>>
>>> The driver does call enable_irq_wake() in its suspend routine to prepare
>>> the interrupt in question to be used as a wakeup source. Why isn't it
>>> enough? It seems to me that your platform code should properly handle
>>> this case instead of relying on the driver to modify IRQ flags.
>>
>> Yes, gpio_keys_suspend() does call enable_irq_wake() to enable the irq
>> of the button. So when the button is pressed, hardware interrupt from
>> this irq does occur.
>>
>> However, after gpio_keys_suspend(), irq_desc of this irq will be
>> disabled if there is no IRQF_NO_SUSPEND flag. So when the hardware
>> interrupt occurs, the irq handler won't call the action of the irq desc.
>> That is, for this case, even if the driver call enable_irq_wake() during
>> suspend, the irq handler in this driver won't be called because it's an
>> action handler, not a irq handler.
> 
> Right, so what I am saying is that enable_irq_wake() should really be
> taking care of that and ensuring that if device is marked as wakeup
> source it should prepare irq handler code to run all necessary parts
> instead of sprinkling random flags all over individual drivers.
> 

If the IRQ is shared, how to handle the case of the one is marked as
wakeup source and the other is not?

Thanks,
-Aubrey