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

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

[Li, Aubrey]

When the wakeup attribute is set, GPIO button is supposed to set
irqflag - IRQF_NO_SUSPEND to request irq. So when the system enters
the suspend sleep mode, the GPIO irq keeps enabled and is able to
wake the system up.

The affected/tested machines include Dell Venue 11 Pro and Asus T100TA.

Signed-off-by: Aubrey Li <aubrey.li@linux.intel.com>
---
 drivers/input/keyboard/gpio_keys.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
index 2db1324..40f3963 100644
--- a/drivers/input/keyboard/gpio_keys.c
+++ b/drivers/input/keyboard/gpio_keys.c
@@ -503,6 +503,9 @@ static int gpio_keys_setup_key(struct platform_device *pdev,
 	if (!button->can_disable)
 		irqflags |= IRQF_SHARED;
 
+	if (button->wakeup)
+		irqflags |= IRQF_NO_SUSPEND;
+
 	error = request_any_context_irq(bdata->irq, isr, irqflags, desc, bdata);
 	if (error < 0) {
 		dev_err(dev, "Unable to claim irq %d; error %d\n",
-- 
1.7.10.4

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

[One Thousand Gnomes]

On Thu, 10 Apr 2014 10:11:09 +0800
"Li, Aubrey" <aubrey.li@linux.intel.com> wrote:

> When the wakeup attribute is set, GPIO button is supposed to set
> irqflag - IRQF_NO_SUSPEND to request irq. So when the system enters
> the suspend sleep mode, the GPIO irq keeps enabled and is able to
> wake the system up.
> 
> The affected/tested machines include Dell Venue 11 Pro and Asus T100TA.
> 
> Signed-off-by: Aubrey Li <aubrey.li@linux.intel.com>

Tested-by: Alan Cox <alan@linux.intel.com>

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

[Li, Aubrey]

ping...

On 2014/4/10 18:48, One Thousand Gnomes wrote:
> On Thu, 10 Apr 2014 10:11:09 +0800
> "Li, Aubrey" <aubrey.li@linux.intel.com> wrote:
> 
>> When the wakeup attribute is set, GPIO button is supposed to set
>> irqflag - IRQF_NO_SUSPEND to request irq. So when the system enters
>> the suspend sleep mode, the GPIO irq keeps enabled and is able to
>> wake the system up.
>>
>> The affected/tested machines include Dell Venue 11 Pro and Asus T100TA.
>>
>> Signed-off-by: Aubrey Li <aubrey.li@linux.intel.com>
> 
> Tested-by: Alan Cox <alan@linux.intel.com>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 

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

[Laxman Dewangan]

On Monday 14 April 2014 09:12 PM, Li, Aubrey wrote:
> ping...
>
> On 2014/4/10 18:48, One Thousand Gnomes wrote:
>> On Thu, 10 Apr 2014 10:11:09 +0800
>> "Li, Aubrey" <aubrey.li@linux.intel.com> wrote:
>>
>>> When the wakeup attribute is set, GPIO button is supposed to set
>>> irqflag - IRQF_NO_SUSPEND to request irq. So when the system enters
>>> the suspend sleep mode, the GPIO irq keeps enabled and is able to
>>> wake the system up.
>>>

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.
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.



-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information.  Any unauthorized review, use, disclosure or distribution
is prohibited.  If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------

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

[Li, Aubrey]

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:
>>> On Thu, 10 Apr 2014 10:11:09 +0800
>>> "Li, Aubrey" <aubrey.li@linux.intel.com> wrote:
>>>
>>>> When the wakeup attribute is set, GPIO button is supposed to set
>>>> irqflag - IRQF_NO_SUSPEND to request irq. So when the system enters
>>>> the suspend sleep mode, the GPIO irq keeps enabled and is able to
>>>> wake the system up.
>>>>
> 
> 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?

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

Thanks,
-Aubrey


> 
> 
> -----------------------------------------------------------------------------------
> 
> This email message is for the sole use of the intended recipient(s) and
> may contain
> confidential information.  Any unauthorized review, use, disclosure or
> distribution
> is prohibited.  If you are not the intended recipient, please contact
> the sender by
> reply email and destroy all copies of the original message.
> -----------------------------------------------------------------------------------
> 
> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 

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

[Laxman Dewangan]

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.

>> 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 email message is for the sole use of the intended recipient(s) and may contain
confidential information.  Any unauthorized review, use, disclosure or distribution
is prohibited.  If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------

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

[Li, Aubrey]

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.

Did you see a failing case of my patch?

Thanks,
-Aubrey

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

[Laxman Dewangan]

On Thursday 17 April 2014 10:12 PM, 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 Tegra SoC, we have implemented this such that we keep re-enabe 
interrupts when going to suspend. That's why my point is.
May be your SoC ha implemented on different way and hence it is require 
NO_SUSPEND.

I do not have any negative remark here, I jut kept my point here.

> 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.
>
> Did you see a failing case of my patch?

Nop, I have not tested the patch and I think it will not break anything 
for me with your patch.



-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information.  Any unauthorized review, use, disclosure or distribution
is prohibited.  If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------

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

[Li, Aubrey]

On 2014/4/18 1:18, Laxman Dewangan wrote:
> On Thursday 17 April 2014 10:12 PM, 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 Tegra SoC, we have implemented this such that we keep re-enabe
> interrupts when going to suspend. That's why my point is.
> May be your SoC ha implemented on different way and hence it is require
> NO_SUSPEND.
> 
> I do not have any negative remark here, I jut kept my point here.

I see. thanks for your point.

> 
>> 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.
>>
>> Did you see a failing case of my patch?
> 
> Nop, I have not tested the patch and I think it will not break anything
> for me with your patch.

Good to hear it.

Thanks,
-Aubrey

> 
> 
> 
> -----------------------------------------------------------------------------------
> 
> This email message is for the sole use of the intended recipient(s) and
> may contain
> confidential information.  Any unauthorized review, use, disclosure or
> distribution
> is prohibited.  If you are not the intended recipient, please contact
> the sender by
> reply email and destroy all copies of the original message.
> -----------------------------------------------------------------------------------
> 
> 
> 

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

[Dmitry Torokhov]

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.

Thanks.
 
-- 
Dmitry

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

[Li, Aubrey]

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.

Does this make sense?

Thanks,
-Aubrey

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

[Dmitry Torokhov]

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.

Thanks.

-- 
Dmitry

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

[Li, Aubrey]

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

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

[Dmitry Torokhov]

On Mon, Apr 21, 2014 at 09:34:20AM +0800, Li, Aubrey wrote:
> 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?

How do we handle this now? You can either make enable_irq_wake() fail
or, like with other shared interrupts, have it runi both handlers and
let drivers sort it out.

Thanks.

-- 
Dmitry

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

[Li, Aubrey]

On 2014/4/21 23:55, Dmitry Torokhov wrote:
> On Mon, Apr 21, 2014 at 09:34:20AM +0800, Li, Aubrey wrote:
>> 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?
> 
> How do we handle this now? You can either make enable_irq_wake() fail
> or, like with other shared interrupts, have it runi both handlers and
> let drivers sort it out.
> 
Hi Dmitry,

The current implementation is, setup the irq with the desired/correct
flags, so the irq and the corresponding irq_desc handler will be enabled
or disabled properly during suspend and resume.

My patch is exactly following this way and the impact is gpio key driver
only. What you suggest seems not to be a small change and need a widely
testing because the change is supposed to cover all of the drivers.

I'm looking forward to seeing your patch or if you want me to do this
change, please let me know.

Thanks,
-Aubrey

> Thanks.
> 

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

[Dmitry Torokhov]

On Tue, Apr 22, 2014 at 12:16:52AM +0800, Li, Aubrey wrote:
> On 2014/4/21 23:55, Dmitry Torokhov wrote:
> > On Mon, Apr 21, 2014 at 09:34:20AM +0800, Li, Aubrey wrote:
> >> 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?
> > 
> > How do we handle this now? You can either make enable_irq_wake() fail
> > or, like with other shared interrupts, have it runi both handlers and
> > let drivers sort it out.
> > 
> Hi Dmitry,
> 
> The current implementation is, setup the irq with the desired/correct
> flags, so the irq and the corresponding irq_desc handler will be enabled
> or disabled properly during suspend and resume.
> 
> My patch is exactly following this way and the impact is gpio key driver
> only. What you suggest seems not to be a small change and need a widely
> testing because the change is supposed to cover all of the drivers.

Right, you are working around issue in IRQ core by adjusting the driver.
Once the core is fixed you (and others) won't be needing to change
drivers at all.

> 
> I'm looking forward to seeing your patch or if you want me to do this
> change, please let me know.

Yes please ;)

Thanks.

-- 
Dmitry