[PATCH] pxa/hx4700: Avoid unbalanced irq wakeup enables/disables

[PATCH] pxa/hx4700: Avoid unbalanced irq wakeup enables/disables

[Paul Parsons]

Resuming a suspended hx4700 results in Unbalanced IRQ warnings:

WARNING: at kernel/irq/manage.c:507 irq_set_irq_wake+0xe0/0xec()
Unbalanced IRQ 246 wake disable
..

Likewise for IRQ 241 and IRQ 243. The 3 ASIC3 GPIO buttons - RECORD/CALENDAR/HOME - each fail in a call to enable_irq_wake() in gpio_keys_suspend() because of an absent irq_set_wake() handler in the asic3_gpio_irq_chip structure. Matching calls to disable_irq_wake() in gpio_keys_resume() then print the warnings. Since we should never need to resume via the 3 ASIC3 GPIO buttons, nor 2 of the 3 builtin GPIO buttons - MAIL/CONTACTS, the simplest remedy for the warnings is to enable irq wakeup on the POWER button only.

Signed-off-by: Paul Parsons <lost.distance@yahoo.com>
---
--- clean-2.6.39-rc6/arch/arm/mach-pxa/hx4700.c	2011-05-04 12:09:06.298511475 +0100
+++ linux-2.6.39-rc6/arch/arm/mach-pxa/hx4700.c	2011-05-05 00:07:57.034493981 +0100
@@ -191,7 +191,7 @@ static struct pxaficp_platform_data ficp
 		.active_low = _active_low,		\
 		.desc       = _desc,			\
 		.type       = EV_KEY,			\
-		.wakeup     = 1,			\
+		.wakeup     = KEY_##_code == KEY_POWER,	\
 	}
 
 static struct gpio_keys_button gpio_keys_buttons[] = {

[PATCH] pxa/hx4700: Avoid unbalanced irq wakeup enables/disables

[Dmitry Artamonow]

On 01:10 Thu 05 May     , Paul Parsons wrote:
> Resuming a suspended hx4700 results in Unbalanced IRQ warnings:
> 
> WARNING: at kernel/irq/manage.c:507 irq_set_irq_wake+0xe0/0xec()
> Unbalanced IRQ 246 wake disable
> ..
> 
> Likewise for IRQ 241 and IRQ 243. The 3 ASIC3 GPIO buttons - RECORD/CALENDAR/HOME - each fail in a call to enable_irq_wake() in gpio_keys_suspend() because of an absent irq_set_wake() handler in the asic3_gpio_irq_chip structure. Matching calls to disable_irq_wake() in gpio_keys_resume() then print the warnings. Since we should never need to resume via the 3 ASIC3 GPIO buttons, nor 2 of the 3 builtin GPIO buttons - MAIL/CONTACTS, the simplest remedy for the warnings is to enable irq wakeup on the POWER button only.

Not sure if it's right to allow only POWER button to wake the device.
In both Windows CE and handhelds.org 2.6.21 kernel any button can wake
device, and even start corresponding action (i.e. run some app) after wakeup.
I think we should just disable wakeup on ASIC3 button for now, and/or think
about how to implement .set_wake in ASIC3.

For first part I mean something like this (not sure if it still applies,
but you should get the idea):

Date: Sat, 31 Jul 2010 11:55:25 +0400
Subject: [PATCH 17/47] pxa/hx4700: don't wake on ASIC3 buttons

Currently ASIC3 driver doesn't support set_wake on its GPIOs,
which leads to "Unbalanced IRQ ### wake disable" warnings on wakeup.
Workaround this by simply not using buttons connected to ASIC3
gpios as wakeup sources.

Signed-off-by: Dmitry Artamonow <mad_soft@inbox.ru>
---
 arch/arm/mach-pxa/hx4700.c |   16 ++++++++--------
 1 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/arch/arm/mach-pxa/hx4700.c b/arch/arm/mach-pxa/hx4700.c
index fa33fef..6c78f93 100644
--- a/arch/arm/mach-pxa/hx4700.c
+++ b/arch/arm/mach-pxa/hx4700.c
@@ -202,23 +202,23 @@ static struct pxaficp_platform_data ficp_info = {
  * GPIO Keys
  */
 
-#define INIT_KEY(_code, _gpio, _active_low, _desc)	\
+#define INIT_KEY(_code, _gpio, _active_low, _wake, _desc)	\
 	{						\
 		.code       = KEY_##_code,		\
 		.gpio       = _gpio,			\
 		.active_low = _active_low,		\
 		.desc       = _desc,			\
 		.type       = EV_KEY,			\
-		.wakeup     = 1,			\
+		.wakeup     = _wake,			\
 	}
 
 static struct gpio_keys_button gpio_keys_buttons[] = {
-	INIT_KEY(POWER,       GPIO0_HX4700_nKEY_POWER,   1, "Power button"),
-	INIT_KEY(MAIL,        GPIO94_HX4700_KEY_MAIL,    0, "Mail button"),
-	INIT_KEY(ADDRESSBOOK, GPIO99_HX4700_KEY_CONTACTS,0, "Contacts button"),
-	INIT_KEY(RECORD,      GPIOD6_nKEY_RECORD,        1, "Record button"),
-	INIT_KEY(CALENDAR,    GPIOD1_nKEY_CALENDAR,      1, "Calendar button"),
-	INIT_KEY(HOMEPAGE,    GPIOD3_nKEY_HOME,          1, "Home button"),
+	INIT_KEY(POWER,       GPIO0_HX4700_nKEY_POWER,   1, 1, "Power button"),
+	INIT_KEY(MAIL,        GPIO94_HX4700_KEY_MAIL,    0, 1, "Mail button"),
+	INIT_KEY(ADDRESSBOOK, GPIO99_HX4700_KEY_CONTACTS,0, 1, "Contacts button"),
+	INIT_KEY(RECORD,      GPIOD6_nKEY_RECORD,        1, 0, "Record button"),
+	INIT_KEY(CALENDAR,    GPIOD1_nKEY_CALENDAR,      1, 0, "Calendar button"),
+	INIT_KEY(HOMEPAGE,    GPIOD3_nKEY_HOME,          1, 0, "Home button"),
 };
 
 static struct gpio_keys_platform_data gpio_keys_data = {

-- 
Best regards,
Dmitry "MAD" Artamonow

[PATCH] pxa/hx4700: Avoid unbalanced irq wakeup enables/disables

[Paul Parsons]

Hi Dmitry,

I tried your patch and found that the MAIL and CONTACTS buttons did not resume the unit. The same is true of the handhelds.org 2.6.21-hh20 kernel. I don't have WinCE any more so am unable to test that.

The reason appears to be that neither GPIO94 (MAIL) nor GPIO99 (CONTACTS) can be enabled as a wake-up source in the PWER register.

Regards,
Paul

--- On Fri, 13/5/11, Dmitry Artamonow <mad_soft@inbox.ru> wrote:

> From: Dmitry Artamonow <mad_soft@inbox.ru>
> Subject: Re: [PATCH] pxa/hx4700: Avoid unbalanced irq wakeup enables/disables
> To: "Paul Parsons" <lost.distance@yahoo.com>
> Cc: linux-arm-kernel at lists.infradead.org, eric.y.miao at gmail.com, philipp.zabel at gmail.com
> Date: Friday, 13 May, 2011, 11:13
> On 01:10 Thu 05 May?
> ???, Paul Parsons wrote:
> > Resuming a suspended hx4700 results in Unbalanced IRQ
> warnings:
> > 
> > WARNING: at kernel/irq/manage.c:507
> irq_set_irq_wake+0xe0/0xec()
> > Unbalanced IRQ 246 wake disable
> > ..
> > 
> > Likewise for IRQ 241 and IRQ 243. The 3 ASIC3 GPIO
> buttons - RECORD/CALENDAR/HOME - each fail in a call to
> enable_irq_wake() in gpio_keys_suspend() because of an
> absent irq_set_wake() handler in the asic3_gpio_irq_chip
> structure. Matching calls to disable_irq_wake() in
> gpio_keys_resume() then print the warnings. Since we should
> never need to resume via the 3 ASIC3 GPIO buttons, nor 2 of
> the 3 builtin GPIO buttons - MAIL/CONTACTS, the simplest
> remedy for the warnings is to enable irq wakeup on the POWER
> button only.
> 
> Not sure if it's right to allow only POWER button to wake
> the device.
> In both Windows CE and handhelds.org 2.6.21 kernel any
> button can wake
> device, and even start corresponding action (i.e. run some
> app) after wakeup.
> I think we should just disable wakeup on ASIC3 button for
> now, and/or think
> about how to implement .set_wake in ASIC3.
> 
> For first part I mean something like this (not sure if it
> still applies,
> but you should get the idea):
> 
> Date: Sat, 31 Jul 2010 11:55:25 +0400
> Subject: [PATCH 17/47] pxa/hx4700: don't wake on ASIC3
> buttons
> 
> Currently ASIC3 driver doesn't support set_wake on its
> GPIOs,
> which leads to "Unbalanced IRQ ### wake disable" warnings
> on wakeup.
> Workaround this by simply not using buttons connected to
> ASIC3
> gpios as wakeup sources.
> 
> Signed-off-by: Dmitry Artamonow <mad_soft@inbox.ru>
> ---
>  arch/arm/mach-pxa/hx4700.c |???16
> ++++++++--------
>  1 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/arm/mach-pxa/hx4700.c
> b/arch/arm/mach-pxa/hx4700.c
> index fa33fef..6c78f93 100644
> --- a/arch/arm/mach-pxa/hx4700.c
> +++ b/arch/arm/mach-pxa/hx4700.c
> @@ -202,23 +202,23 @@ static struct pxaficp_platform_data
> ficp_info = {
> ? * GPIO Keys
> ? */
>  
> -#define INIT_KEY(_code, _gpio, _active_low,
> _desc)??? \
> +#define INIT_KEY(_code, _gpio, _active_low, _wake,
> _desc)??? \
>  ??? {??? ???
> ??? ??? ???
> ??? \
>  ??? ??? .code? ?
> ???= KEY_##_code,???
> ??? \
>  ??? ??? .gpio? ?
> ???= _gpio,???
> ??? ??? \
>  ??? ??? .active_low =
> _active_low,??? ??? \
>  ??? ??? .desc? ?
> ???= _desc,???
> ??? ??? \
>  ??? ??? .type? ?
> ???= EV_KEY,???
> ??? ??? \
> -??? ??? .wakeup?
> ???= 1,??? ???
> ??? \
> +??? ??? .wakeup?
> ???= _wake,???
> ??? ??? \
>  ??? }
>  
>  static struct gpio_keys_button gpio_keys_buttons[] = {
> -??? INIT_KEY(POWER,? ?
> ???GPIO0_HX4700_nKEY_POWER,???1,
> "Power button"),
> -??? INIT_KEY(MAIL,? ? ?
> ? GPIO94_HX4700_KEY_MAIL,? ? 0, "Mail
> button"),
> -??? INIT_KEY(ADDRESSBOOK,
> GPIO99_HX4700_KEY_CONTACTS,0, "Contacts button"),
> -??? INIT_KEY(RECORD,? ? ?
> GPIOD6_nKEY_RECORD,? ? ? ? 1, "Record
> button"),
> -??? INIT_KEY(CALENDAR,? ?
> GPIOD1_nKEY_CALENDAR,? ? ? 1, "Calendar
> button"),
> -??? INIT_KEY(HOMEPAGE,? ?
> GPIOD3_nKEY_HOME,? ? ? ? ? 1, "Home
> button"),
> +??? INIT_KEY(POWER,? ?
> ???GPIO0_HX4700_nKEY_POWER,???1,
> 1, "Power button"),
> +??? INIT_KEY(MAIL,? ? ?
> ? GPIO94_HX4700_KEY_MAIL,? ? 0, 1, "Mail
> button"),
> +??? INIT_KEY(ADDRESSBOOK,
> GPIO99_HX4700_KEY_CONTACTS,0, 1, "Contacts button"),
> +??? INIT_KEY(RECORD,? ? ?
> GPIOD6_nKEY_RECORD,? ? ? ? 1, 0, "Record
> button"),
> +??? INIT_KEY(CALENDAR,? ?
> GPIOD1_nKEY_CALENDAR,? ? ? 1, 0, "Calendar
> button"),
> +??? INIT_KEY(HOMEPAGE,? ?
> GPIOD3_nKEY_HOME,? ? ? ? ? 1, 0,
> "Home button"),
>  };
>  
>  static struct gpio_keys_platform_data gpio_keys_data = {
> 
> -- 
> Best regards,
> Dmitry "MAD" Artamonow
> 

[PATCH] pxa/hx4700: Avoid unbalanced irq wakeup enables/disables

[Russell King - ARM Linux]

On Fri, May 13, 2011 at 02:13:49PM +0400, Dmitry Artamonow wrote:
> Not sure if it's right to allow only POWER button to wake the device.
> In both Windows CE and handhelds.org 2.6.21 kernel any button can wake
> device, and even start corresponding action (i.e. run some app) after wakeup.

Ah, I'm glad you mentioned that.

This 'feature' is really annoying, as it keeps resulting in my iPAQ
battery being drained completely.  My iPAQ now lives plugged into its
charger because that's the only way I can ensure that the programs and
data on it stay there.  If I put it anywhere else, a button will get
pressed unnoticed and then I'll have to reload everything back on to it.

So really, whether a non-power button wakes up or not should be ultimately
end-user configurable.  Wish it was in wince.

[PATCH] pxa/hx4700: Avoid unbalanced irq wakeup enables/disables

[Dmitry Artamonow]

On 11:53 Fri 13 May     , Paul Parsons wrote:
> Hi Dmitry,
> 
> I tried your patch and found that the MAIL and CONTACTS buttons did not resume the unit. The same is true of the handhelds.org 2.6.21-hh20 kernel. I don't have WinCE any more so am unable to test that.
> 
> The reason appears to be that neither GPIO94 (MAIL) nor GPIO99 (CONTACTS) can be enabled as a wake-up source in the PWER register.

Well, you right here - GPIO94 and GPIO99 can't be enabled as wake-up
sources in PWER on PXA270. But they can be enabled in PKWR, according to
section 3.8.1.15 of PXA27x Developer's Manual. And there's a proof: WinCE
happily wakes on MAIL/CONTACTS presses. Though probably these pins will
need some configuration in MFP table to work as wake-up sources...

Anyway, I have no strong opinion about whether to disable just ASIC3
buttons, or to leave only POWER as wakeup source. So I think your patch is
fine as the solution to unbalanced irq wakeup problem and may go in, unless
Philipp or Eric have objections.

-- 
Best regards,
Dmitry "MAD" Artamonow

[PATCH] pxa/hx4700: Avoid unbalanced irq wakeup enables/disables

[Paul Parsons]

Hi Dmitry,

> Well, you right here - GPIO94 and GPIO99 can't be enabled
> as wake-up
> sources in PWER on PXA270. But they can be enabled in PKWR,
> according to
> section 3.8.1.15 of PXA27x Developer's Manual. And there's
> a proof: WinCE
> happily wakes on MAIL/CONTACTS presses. Though probably
> these pins will
> need some configuration in MFP table to work as wake-up
> sources...

Ah, another wake-up enable register; I missed that! Yes, that should work via tweaks to the MFP table.

> Anyway, I have no strong opinion about whether to disable
> just ASIC3
> buttons, or to leave only POWER as wakeup source. So I
> think your patch is
> fine as the solution to unbalanced irq wakeup problem and
> may go in, unless
> Philipp or Eric have objections.

My feeling is that it should be all or nothing; we either get all other 5 buttons working as wake-ups or we disable them. Perhaps Russell's comments are the clincher.

While on the subject, it turns out that the reset button (= GPIO reset) also acts as a wake-up on my unit. But the PXA27x manual says this:

"In standby, sleep, and deep-sleep modes, GPIO reset is first treated as a wake-up event."

Does this mean that both the SMR and GPR bits the RCSR register are set? And if so, does the bootloader then decide whether to wake-up the unit or whether to reset it?

Regards,
Paul

[PATCH] pxa/hx4700: Avoid unbalanced irq wakeup enables/disables

[Eric Miao]

On Sun, May 15, 2011 at 7:20 AM, Paul Parsons <lost.distance@yahoo.com> wrote:
> Hi Dmitry,
>
>> Well, you right here - GPIO94 and GPIO99 can't be enabled
>> as wake-up
>> sources in PWER on PXA270. But they can be enabled in PKWR,
>> according to
>> section 3.8.1.15 of PXA27x Developer's Manual. And there's
>> a proof: WinCE
>> happily wakes on MAIL/CONTACTS presses. Though probably
>> these pins will
>> need some configuration in MFP table to work as wake-up
>> sources...
>
> Ah, another wake-up enable register; I missed that! Yes, that should work via tweaks to the MFP table.
>
>> Anyway, I have no strong opinion about whether to disable
>> just ASIC3
>> buttons, or to leave only POWER as wakeup source. So I
>> think your patch is
>> fine as the solution to unbalanced irq wakeup problem and
>> may go in, unless
>> Philipp or Eric have objections.
>
> My feeling is that it should be all or nothing; we either get all other 5 buttons working as wake-ups or we disable them. Perhaps Russell's comments are the clincher.
>

To use GPIOs in PKWR, you need to do two things:

1. WAKEUP_ON_LEVEL_HIGH or'ed with the pin configuration (see mfp-pxa2xx.h)
2. enable_irq_wakeup() will eventually invoke gpio_set_wake() in
mfp-pxa2xx.c, and that function will handle the PKWR setting.

> While on the subject, it turns out that the reset button (= GPIO reset) also acts as a wake-up on my unit. But the PXA27x manual says this:
>
> "In standby, sleep, and deep-sleep modes, GPIO reset is first treated as a wake-up event."
>
> Does this mean that both the SMR and GPR bits the RCSR register are set? And if so, does the bootloader then decide whether to wake-up the unit or whether to reset it?
>

The full statement is:

In standby, sleep, and deep-sleep modes, GPIO reset is first treated
as a wake-up event. It is then
recognized as a GPIO reset if held asserted for more than 1 ms after
all power supplies are stable.

And my understanding of this is - an assertion on GPIO<1> will firstly be
treated as a wake-up event and cause the sleep-exit. SMR in RCSR will
be set. A normal resume process begins from software perspective. If
the assertion is a pulse less than 1ms, no reset will happen. However,
if the assertion is more than 1ms, then GPIO reset will happen wherever
the software resuming process is. SMR will most likely be cleared during
this reset process.

In other word, the whole process is transparent to software including
the bootloader.

> Regards,
> Paul
>

[PATCH] pxa/hx4700: Avoid unbalanced irq wakeup enables/disables

[Philipp Zabel]

Am Donnerstag, den 05.05.2011, 01:10 +0000 schrieb Paul Parsons:
> Resuming a suspended hx4700 results in Unbalanced IRQ warnings:
> 
> WARNING: at kernel/irq/manage.c:507 irq_set_irq_wake+0xe0/0xec()
> Unbalanced IRQ 246 wake disable
> ..
> 
> Likewise for IRQ 241 and IRQ 243. The 3 ASIC3 GPIO buttons - RECORD/CALENDAR/HOME - each fail in a call to enable_irq_wake() in gpio_keys_suspend() because of an absent irq_set_wake() handler in the asic3_gpio_irq_chip structure. Matching calls to disable_irq_wake() in gpio_keys_resume() then print the warnings. Since we should never need to resume via the 3 ASIC3 GPIO buttons, nor 2 of the 3 builtin GPIO buttons - MAIL/CONTACTS, the simplest remedy for the warnings is to enable irq wakeup on the POWER button only.
> 
> Signed-off-by: Paul Parsons <lost.distance@yahoo.com>
> ---
> --- clean-2.6.39-rc6/arch/arm/mach-pxa/hx4700.c	2011-05-04 12:09:06.298511475 +0100
> +++ linux-2.6.39-rc6/arch/arm/mach-pxa/hx4700.c	2011-05-05 00:07:57.034493981 +0100
> @@ -191,7 +191,7 @@ static struct pxaficp_platform_data ficp
>  		.active_low = _active_low,		\
>  		.desc       = _desc,			\
>  		.type       = EV_KEY,			\
> -		.wakeup     = 1,			\
> +		.wakeup     = KEY_##_code == KEY_POWER,	\
>  	}
>  
>  static struct gpio_keys_button gpio_keys_buttons[] = {
> 

On my hx4700 iPAQ running Windows Mobile 5, gpio button wakeup is
configurable under "Start -> Settings -> Personal -> Buttons -> Lock".
There is a checkbox "Disable all buttons except power button" which
enables/disables the keypad and ASIC3 GPIO buttons as wakeup sources.

To achieve the same functionality on linux, we could split the power
button and the other buttons into separate gpio_keys devices and let
userspace disable the other buttons as wakeup sources via the device's
pm_wakeup interface. For that we need 

Given that ASIC3 doesn't support irq_set_wake for now,
Acked-By: Philipp Zabel <philipp.zabel@gmail.com>

regards
Philipp

[PATCH] pxa/hx4700: Avoid unbalanced irq wakeup enables/disables

[Haojian Zhuang]

On Sun, Feb 26, 2012 at 6:48 PM, Philipp Zabel <philipp.zabel@gmail.com> wrote:
> Am Donnerstag, den 05.05.2011, 01:10 +0000 schrieb Paul Parsons:
>> Resuming a suspended hx4700 results in Unbalanced IRQ warnings:
>>
>> WARNING: at kernel/irq/manage.c:507 irq_set_irq_wake+0xe0/0xec()
>> Unbalanced IRQ 246 wake disable
>> ..
>>
>> Likewise for IRQ 241 and IRQ 243. The 3 ASIC3 GPIO buttons - RECORD/CALENDAR/HOME - each fail in a call to enable_irq_wake() in gpio_keys_suspend() because of an absent irq_set_wake() handler in the asic3_gpio_irq_chip structure. Matching calls to disable_irq_wake() in gpio_keys_resume() then print the warnings. Since we should never need to resume via the 3 ASIC3 GPIO buttons, nor 2 of the 3 builtin GPIO buttons - MAIL/CONTACTS, the simplest remedy for the warnings is to enable irq wakeup on the POWER button only.
>>
>> Signed-off-by: Paul Parsons <lost.distance@yahoo.com>
>> ---
>> --- clean-2.6.39-rc6/arch/arm/mach-pxa/hx4700.c ? ? ? 2011-05-04 12:09:06.298511475 +0100
>> +++ linux-2.6.39-rc6/arch/arm/mach-pxa/hx4700.c ? ? ? 2011-05-05 00:07:57.034493981 +0100
>> @@ -191,7 +191,7 @@ static struct pxaficp_platform_data ficp
>> ? ? ? ? ? ? ? .active_low = _active_low, ? ? ? ? ? ? ?\
>> ? ? ? ? ? ? ? .desc ? ? ? = _desc, ? ? ? ? ? ? ? ? ? ?\
>> ? ? ? ? ? ? ? .type ? ? ? = EV_KEY, ? ? ? ? ? ? ? ? ? \
>> - ? ? ? ? ? ? .wakeup ? ? = 1, ? ? ? ? ? ? ? ? ? ? ? ?\
>> + ? ? ? ? ? ? .wakeup ? ? = KEY_##_code == KEY_POWER, \
>> ? ? ? }
>>
>> ?static struct gpio_keys_button gpio_keys_buttons[] = {
>>
>
> On my hx4700 iPAQ running Windows Mobile 5, gpio button wakeup is
> configurable under "Start -> Settings -> Personal -> Buttons -> Lock".
> There is a checkbox "Disable all buttons except power button" which
> enables/disables the keypad and ASIC3 GPIO buttons as wakeup sources.
>
> To achieve the same functionality on linux, we could split the power
> button and the other buttons into separate gpio_keys devices and let
> userspace disable the other buttons as wakeup sources via the device's
> pm_wakeup interface. For that we need
>
> Given that ASIC3 doesn't support irq_set_wake for now,
> Acked-By: Philipp Zabel <philipp.zabel@gmail.com>
>
It seems that this patch is only enable powerup key as wakeup source.
Other gpio keys could be configured as wakeup source in user space. So could
you help to figure out the interface of configuring gpio key as wakeup source
in user space?

Thanks
Haojian

[PATCH] pxa/hx4700: Avoid unbalanced irq wakeup enables/disables

[Philipp Zabel]

Am Montag, den 27.02.2012, 10:00 +0800 schrieb Haojian Zhuang:
> On Sun, Feb 26, 2012 at 6:48 PM, Philipp Zabel <philipp.zabel@gmail.com> wrote:
> > On my hx4700 iPAQ running Windows Mobile 5, gpio button wakeup is
> > configurable under "Start -> Settings -> Personal -> Buttons -> Lock".
> > There is a checkbox "Disable all buttons except power button" which
> > enables/disables the keypad and ASIC3 GPIO buttons as wakeup sources.
> >
> > To achieve the same functionality on linux, we could split the power
> > button and the other buttons into separate gpio_keys devices and let
> > userspace disable the other buttons as wakeup sources via the device's
> > pm_wakeup interface. 
>
> It seems that this patch is only enable powerup key as wakeup source.

Yes. This is temporary until ASIC3 GPIO wakeup support is in place.

> Other gpio keys could be configured as wakeup source in user space. So could
> you help to figure out the interface of configuring gpio key as wakeup source
> in user space?

I was thinking of the /sys/devices/.../power/wakeup interface, as
documented in Documentation/ABI/testing/sysfs-devices-power.

Put the power button into its own gpio-keys device:

diff --git a/arch/arm/mach-pxa/hx4700.c b/arch/arm/mach-pxa/hx4700.c
index 06ec926..21d26a3 100644
--- a/arch/arm/mach-pxa/hx4700.c
+++ b/arch/arm/mach-pxa/hx4700.c
@@ -166,11 +166,26 @@ static struct pxaficp_platform_data ficp_info = {
 		.active_low = _active_low,		\
 		.desc       = _desc,			\
 		.type       = EV_KEY,			\
-		.wakeup     = KEY_##_code == KEY_POWER,	\
+		.wakeup     = 1,			\
 	}
 
+static struct gpio_keys_button gpio_keys_power_button =
+	INIT_KEY(POWER,       GPIO0_HX4700_nKEY_POWER,   1, "Power button");
+
+static struct gpio_keys_platform_data gpio_keys_power_data = {
+	.buttons = gpio_keys_power_button,
+	.nbuttons = 1,
+};
+
+static struct platform_device gpio_keys_power = {
+	.name = "gpio-keys",
+	.dev  = {
+		.platform_data = &gpio_keys_power_data,
+	},
+	.id   = 0,
+};
+
 static struct gpio_keys_button gpio_keys_buttons[] = {
-	INIT_KEY(POWER,       GPIO0_HX4700_nKEY_POWER,   1, "Power button"),
 	INIT_KEY(MAIL,        GPIO94_HX4700_KEY_MAIL,    0, "Mail button"),
 	INIT_KEY(ADDRESSBOOK, GPIO99_HX4700_KEY_CONTACTS,0, "Contacts button"),
 	INIT_KEY(RECORD,      GPIOD6_nKEY_RECORD,        1, "Record button"),
@@ -188,7 +203,7 @@ static struct platform_device gpio_keys = {
 	.dev  = {
 		.platform_data = &gpio_keys_data,
 	},
-	.id   = -1,
+	.id   = 1,
 };
 
 /*
@@ -836,6 +851,7 @@ static struct platform_device audio = {
 
 static struct platform_device *devices[] __initdata = {
 	&asic3,
+	&gpio_keys_power,
 	&gpio_keys,
 	&navpoint,
 	&backlight,

And then let userspace disable wakeup for the other keys via
echo disabled > /sys/devices/platform/gpio-keys.1/power/wakeup

The gpio-keys driver will check this setting with device_may_wakeup()
during gpio_keys_suspend() and decide whether to call enable_irq_wake()
for its button GPIOs.

regards
Philipp

[PATCH] pxa/hx4700: Avoid unbalanced irq wakeup enables/disables

[Haojian Zhuang]

On Mon, Feb 27, 2012 at 3:42 PM, Philipp Zabel <philipp.zabel@gmail.com> wrote:
> Am Montag, den 27.02.2012, 10:00 +0800 schrieb Haojian Zhuang:
>> On Sun, Feb 26, 2012 at 6:48 PM, Philipp Zabel <philipp.zabel@gmail.com> wrote:
>> > On my hx4700 iPAQ running Windows Mobile 5, gpio button wakeup is
>> > configurable under "Start -> Settings -> Personal -> Buttons -> Lock".
>> > There is a checkbox "Disable all buttons except power button" which
>> > enables/disables the keypad and ASIC3 GPIO buttons as wakeup sources.
>> >
>> > To achieve the same functionality on linux, we could split the power
>> > button and the other buttons into separate gpio_keys devices and let
>> > userspace disable the other buttons as wakeup sources via the device's
>> > pm_wakeup interface.
>>
>> It seems that this patch is only enable powerup key as wakeup source.
>
> Yes. This is temporary until ASIC3 GPIO wakeup support is in place.
>
>> Other gpio keys could be configured as wakeup source in user space. So could
>> you help to figure out the interface of configuring gpio key as wakeup source
>> in user space?
>
> I was thinking of the /sys/devices/.../power/wakeup interface, as
> documented in Documentation/ABI/testing/sysfs-devices-power.
>
> Put the power button into its own gpio-keys device:
>
> diff --git a/arch/arm/mach-pxa/hx4700.c b/arch/arm/mach-pxa/hx4700.c
> index 06ec926..21d26a3 100644
> --- a/arch/arm/mach-pxa/hx4700.c
> +++ b/arch/arm/mach-pxa/hx4700.c
> @@ -166,11 +166,26 @@ static struct pxaficp_platform_data ficp_info = {
> ? ? ? ? ? ? ? ?.active_low = _active_low, ? ? ? ? ? ? ?\
> ? ? ? ? ? ? ? ?.desc ? ? ? = _desc, ? ? ? ? ? ? ? ? ? ?\
> ? ? ? ? ? ? ? ?.type ? ? ? = EV_KEY, ? ? ? ? ? ? ? ? ? \
> - ? ? ? ? ? ? ? .wakeup ? ? = KEY_##_code == KEY_POWER, \
> + ? ? ? ? ? ? ? .wakeup ? ? = 1, ? ? ? ? ? ? ? ? ? ? ? ?\
> ? ? ? ?}
>
> +static struct gpio_keys_button gpio_keys_power_button =
> + ? ? ? INIT_KEY(POWER, ? ? ? GPIO0_HX4700_nKEY_POWER, ? 1, "Power button");
> +
> +static struct gpio_keys_platform_data gpio_keys_power_data = {
> + ? ? ? .buttons = gpio_keys_power_button,
> + ? ? ? .nbuttons = 1,
> +};
> +
> +static struct platform_device gpio_keys_power = {
> + ? ? ? .name = "gpio-keys",
> + ? ? ? .dev ?= {
> + ? ? ? ? ? ? ? .platform_data = &gpio_keys_power_data,
> + ? ? ? },
> + ? ? ? .id ? = 0,
> +};
> +
> ?static struct gpio_keys_button gpio_keys_buttons[] = {
> - ? ? ? INIT_KEY(POWER, ? ? ? GPIO0_HX4700_nKEY_POWER, ? 1, "Power button"),
> ? ? ? ?INIT_KEY(MAIL, ? ? ? ?GPIO94_HX4700_KEY_MAIL, ? ?0, "Mail button"),
> ? ? ? ?INIT_KEY(ADDRESSBOOK, GPIO99_HX4700_KEY_CONTACTS,0, "Contacts button"),
> ? ? ? ?INIT_KEY(RECORD, ? ? ?GPIOD6_nKEY_RECORD, ? ? ? ?1, "Record button"),
> @@ -188,7 +203,7 @@ static struct platform_device gpio_keys = {
> ? ? ? ?.dev ?= {
> ? ? ? ? ? ? ? ?.platform_data = &gpio_keys_data,
> ? ? ? ?},
> - ? ? ? .id ? = -1,
> + ? ? ? .id ? = 1,
> ?};
>
> ?/*
> @@ -836,6 +851,7 @@ static struct platform_device audio = {
>
> ?static struct platform_device *devices[] __initdata = {
> ? ? ? ?&asic3,
> + ? ? ? &gpio_keys_power,
> ? ? ? ?&gpio_keys,
> ? ? ? ?&navpoint,
> ? ? ? ?&backlight,
>
> And then let userspace disable wakeup for the other keys via
> echo disabled > /sys/devices/platform/gpio-keys.1/power/wakeup
>
> The gpio-keys driver will check this setting with device_may_wakeup()
> during gpio_keys_suspend() and decide whether to call enable_irq_wake()
> for its button GPIOs.
>
> regards
> Philipp
>
>
If one of gpio keys wakeup is enabled, every gpio keys in this device can
wakeup system. So your patch is better than original one that only keeping
power key as wakeup source.

You overwrite Paul's patch. I think that you can format a new patch that is
based on current code base not Paul's patch.

Thanks
Haojian

[PATCH] pxa/hx4700: Avoid unbalanced irq wakeup enables/disables

[Paul Parsons]

> >> > On my hx4700 iPAQ running Windows Mobile 5,
> gpio button wakeup is
> >> > configurable under "Start -> Settings ->
> Personal -> Buttons -> Lock".
> >> > There is a checkbox "Disable all buttons
> except power button" which
> >> > enables/disables the keypad and ASIC3 GPIO
> buttons as wakeup sources.
> >> >
> >> > To achieve the same functionality on linux, we
> could split the power
> >> > button and the other buttons into separate
> gpio_keys devices and let
> >> > userspace disable the other buttons as wakeup
> sources via the device's
> >> > pm_wakeup interface.
> >>
> >> It seems that this patch is only enable powerup key
> as wakeup source.
> >
> > Yes. This is temporary until ASIC3 GPIO wakeup support
> is in place.
> >
> >> Other gpio keys could be configured as wakeup
> source in user space. So could
> >> you help to figure out the interface of configuring
> gpio key as wakeup source
> >> in user space?
> >
> > I was thinking of the /sys/devices/.../power/wakeup
> interface, as
> > documented in
> Documentation/ABI/testing/sysfs-devices-power.
> >
> > Put the power button into its own gpio-keys device:
> >
> > diff --git a/arch/arm/mach-pxa/hx4700.c
> b/arch/arm/mach-pxa/hx4700.c
> > index 06ec926..21d26a3 100644
> > --- a/arch/arm/mach-pxa/hx4700.c
> > +++ b/arch/arm/mach-pxa/hx4700.c
> > @@ -166,11 +166,26 @@ static struct
> pxaficp_platform_data ficp_info = {
> > ? ? ? ? ? ? ? ?.active_low = _active_low, ? ?
> ? ? ? ? ?\
> > ? ? ? ? ? ? ? ?.desc ? ? ? = _desc, ? ? ?
> ? ? ? ? ? ? ?\
> > ? ? ? ? ? ? ? ?.type ? ? ? = EV_KEY, ? ?
> ? ? ? ? ? ? ? \
> > - ? ? ? ? ? ? ? .wakeup ? ? = KEY_##_code ==
> KEY_POWER, \
> > + ? ? ? ? ? ? ? .wakeup ? ? = 1, ? ? ? ?
> ? ? ? ? ? ? ? ?\
> > ? ? ? ?}
> >
> > +static struct gpio_keys_button gpio_keys_power_button
> =
> > + ? ? ? INIT_KEY(POWER, ? ? ?
> GPIO0_HX4700_nKEY_POWER, ? 1, "Power button");
> > +
> > +static struct gpio_keys_platform_data
> gpio_keys_power_data = {
> > + ? ? ? .buttons = gpio_keys_power_button,
> > + ? ? ? .nbuttons = 1,
> > +};
> > +
> > +static struct platform_device gpio_keys_power = {
> > + ? ? ? .name = "gpio-keys",
> > + ? ? ? .dev ?= {
> > + ? ? ? ? ? ? ? .platform_data =
> &gpio_keys_power_data,
> > + ? ? ? },
> > + ? ? ? .id ? = 0,
> > +};
> > +
> > ?static struct gpio_keys_button gpio_keys_buttons[] =
> {
> > - ? ? ? INIT_KEY(POWER, ? ? ?
> GPIO0_HX4700_nKEY_POWER, ? 1, "Power button"),
> > ? ? ? ?INIT_KEY(MAIL, ? ? ?
> ?GPIO94_HX4700_KEY_MAIL, ? ?0, "Mail button"),
> > ? ? ? ?INIT_KEY(ADDRESSBOOK,
> GPIO99_HX4700_KEY_CONTACTS,0, "Contacts button"),
> > ? ? ? ?INIT_KEY(RECORD, ? ? ?GPIOD6_nKEY_RECORD,
> ? ? ? ?1, "Record button"),
> > @@ -188,7 +203,7 @@ static struct platform_device
> gpio_keys = {
> > ? ? ? ?.dev ?= {
> > ? ? ? ? ? ? ? ?.platform_data =
> &gpio_keys_data,
> > ? ? ? ?},
> > - ? ? ? .id ? = -1,
> > + ? ? ? .id ? = 1,
> > ?};
> >
> > ?/*
> > @@ -836,6 +851,7 @@ static struct platform_device audio
> = {
> >
> > ?static struct platform_device *devices[] __initdata =
> {
> > ? ? ? ?&asic3,
> > + ? ? ? &gpio_keys_power,
> > ? ? ? ?&gpio_keys,
> > ? ? ? ?&navpoint,
> > ? ? ? ?&backlight,
> >
> > And then let userspace disable wakeup for the other
> keys via
> > echo disabled >
> /sys/devices/platform/gpio-keys.1/power/wakeup
> >
> > The gpio-keys driver will check this setting with
> device_may_wakeup()
> > during gpio_keys_suspend() and decide whether to call
> enable_irq_wake()
> > for its button GPIOs.
> >
> > regards
> > Philipp
> >
> >
> If one of gpio keys wakeup is enabled, every gpio keys in
> this device can
> wakeup system. So your patch is better than original one
> that only keeping
> power key as wakeup source.
> 
> You overwrite Paul's patch. I think that you can format a
> new patch that is
> based on current code base not Paul's patch.

I'm not sure if allowing user space software to define which buttons can
be used as wakeup sources is a really problem which needs to be solved.

Windows Mobile 5 may provide this functionality, but is there any Linux
mobile software which attempts to do this?