Input: gpio_keys: Make sure wake-up buttons work.

PATCH In review NeilBrown 3 participants 5 messages Nov 7, 2014 – Nov 9, 2014

Subsystems (2)

HID Input drivers

lore

Share & tools

lore

NeilBrown

author

2014-11-07 23:37 UTC #0 lore raw

PATCH 1/1 Input: gpio_keys: Make sure wake-up buttons work.

If a key is to be used for wake-up, we must not disable the interrupt during suspend.

Signed-off-by NeilBrown <[email protected]>

drivers/input/keyboard/gpio_keys.c +2 -0

index 8c98e97f8e41..0b5e54ae343e 100644
@@ -526,6 +526,8 @@ 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 = devm_request_any_context_irq(&pdev->dev, bdata->irq,
 					     isr, irqflags, desc, bdata);

Dmitry Torokhov

maintainer

2014-11-07 23:45 UTC #1 lore raw

Hi Neil,

Quoted 12 lines · NeilBrown

If a key is to be used for wake-up, we must not disable the interrupt during suspend.
Signed-off-by NeilBrown <[email protected]>
drivers/input/keyboard/gpio_keys.c +0 -0
index 8c98e97f8e41..0b5e54ae343e 100644

drivers/input/keyboard/gpio_keys.c static int gpio_keys_setup_key(struct platform_device *pdev, · line 530

Quoted 5 lines

drivers/input/keyboard/gpio_keys.c +2 -0

@@ -526,6 +526,8 @@ static int gpio_keys_setup_key(struct platform_device *pdev,
 	 */
 	if (!button->can_disable)
 		irqflags |= IRQF_SHARED;
+	if (button->wakeup)
+		irqflags |= IRQF_NO_SUSPEND;

No, enable_irq_wake() should be enough. I believe Rafael has fixed that in the core, right?

drivers/input/keyboard/gpio_keys.c static int gpio_keys_setup_key(struct platform_device *pdev, · line 533

Quoted 3 lines

drivers/input/keyboard/gpio_keys.c +0 -0

 	error = devm_request_any_context_irq(&pdev->dev, bdata->irq,
 					     isr, irqflags, desc, bdata);

Thanks.

Signature · 1 line

Dmitry

NeilBrown

author

2014-11-08 00:01 UTC #2 lore raw

Quoted 10 lines · On Fri, 7 Nov 2014 15:45:07 -0800 Dmitry Torokhov <[email protected]>…

Hi Neil,
Quoted 7 lines · NeilBrown
If a key is to be used for wake-up, we must not disable the interrupt during suspend.
Signed-off-by NeilBrown <[email protected]>

Quoted 14 lines

drivers/input/keyboard/gpio_keys.c static int gpio_keys_setup_key(struct platform_device *pdev, · line 530
Quoted 10 lines
drivers/input/keyboard/gpio_keys.c +2 -0
index 8c98e97f8e41..0b5e54ae343e 100644
@@ -526,6 +526,8 @@ static int gpio_keys_setup_key(struct platform_device *pdev,
 	 */
 	if (!button->can_disable)
 		irqflags |= IRQF_SHARED;
+	if (button->wakeup)
+		irqflags |= IRQF_NO_SUSPEND;
No, enable_irq_wake() should be enough. I believe Rafael has fixed that in the core, right?

Interesting... you seem to be right, but I was having wakeup problems until I added that patch.
I didn't test exhaustively, but the first time my device entered suspend, the gpio-key didn't wake it up. Subsequent suspends did...

After I applied this patch, it would reliably wake up even on the first suspend.

So there seems to be something wrong, but maybe it is more subtle.

Is there a good reason why enable_irq_wake() is only called just as the device is being suspended, and why disable_irq_wake() is called on resume? To me it would make more sense to just enable it once (if required) and leave it enabled....

I'll see what I can find.

Thanks,
NeilBrown

Rafael J. Wysocki

2014-11-08 00:28 UTC #3 lore raw

Quoted 28 lines · NeilBrown

Quoted 10 lines · On Fri, 7 Nov 2014 15:45:07 -0800 Dmitry Torokhov <[email protected]>…
Hi Neil,
Quoted 7 lines · NeilBrown
If a key is to be used for wake-up, we must not disable the interrupt during suspend.
Signed-off-by NeilBrown <[email protected]>
Quoted 14 lines
drivers/input/keyboard/gpio_keys.c static int gpio_keys_setup_key(struct platform_device *pdev, · line 530
Quoted 10 lines
drivers/input/keyboard/gpio_keys.c +2 -0
index 8c98e97f8e41..0b5e54ae343e 100644
@@ -526,6 +526,8 @@ static int gpio_keys_setup_key(struct platform_device *pdev,
 	 */
 	if (!button->can_disable)
 		irqflags |= IRQF_SHARED;
+	if (button->wakeup)
+		irqflags |= IRQF_NO_SUSPEND;
No, enable_irq_wake() should be enough. I believe Rafael has fixed that in the core, right?

Quoted 2 lines

Interesting... you seem to be right, but I was having wakeup problems until I added that patch.

This was a fairly recent change made in 3.18-rc1.

Quoted 6 lines

I didn't test exhaustively, but the first time my device entered suspend, the gpio-key didn't wake it up. Subsequent suspends did...
After I applied this patch, it would reliably wake up even on the first suspend.

Quoted 6 lines

So there seems to be something wrong, but maybe it is more subtle.
Is there a good reason why enable_irq_wake() is only called just as the device is being suspended, and why disable_irq_wake() is called on resume? To me it would make more sense to just enable it once (if required) and leave it enabled....

On some platforms it actually changes the configuration of interrupt controllers in to a "suspend mode" which is not appropriate for run time AFAICS.

Quoted 1 line

I'll see what I can find.

Yes, please.

Rafael

NeilBrown

author

2014-11-09 21:56 UTC #4 lore raw

Quoted 47 lines · On Sat, 08 Nov 2014 01:49:06 +0100 "Rafael J. Wysocki" <[email protected]>…

Quoted 28 lines · NeilBrown
Quoted 10 lines · On Fri, 7 Nov 2014 15:45:07 -0800 Dmitry Torokhov <[email protected]>…
Hi Neil,
Quoted 7 lines · NeilBrown
If a key is to be used for wake-up, we must not disable the interrupt during suspend.
Signed-off-by NeilBrown <[email protected]>
Quoted 14 lines
drivers/input/keyboard/gpio_keys.c static int gpio_keys_setup_key(struct platform_device *pdev, · line 530
Quoted 10 lines
drivers/input/keyboard/gpio_keys.c +2 -0
index 8c98e97f8e41..0b5e54ae343e 100644
@@ -526,6 +526,8 @@ static int gpio_keys_setup_key(struct platform_device *pdev,
 	 */
 	if (!button->can_disable)
 		irqflags |= IRQF_SHARED;
+	if (button->wakeup)
+		irqflags |= IRQF_NO_SUSPEND;
No, enable_irq_wake() should be enough. I believe Rafael has fixed that in the core, right?
Quoted 2 lines
Interesting... you seem to be right, but I was having wakeup problems until I added that patch.
This was a fairly recent change made in 3.18-rc1.
Quoted 6 lines
I didn't test exhaustively, but the first time my device entered suspend, the gpio-key didn't wake it up. Subsequent suspends did...
After I applied this patch, it would reliably wake up even on the first suspend.
Quoted 6 lines
So there seems to be something wrong, but maybe it is more subtle.
Is there a good reason why enable_irq_wake() is only called just as the device is being suspended, and why disable_irq_wake() is called on resume? To me it would make more sense to just enable it once (if required) and leave it enabled....

Quoted 7 lines

On some platforms it actually changes the configuration of interrupt controllers in to a "suspend mode" which is not appropriate for run time AFAICS.
Quoted 1 line
I'll see what I can find.
Yes, please.

Hmmm.. that's frustrating. I cannot reproduce the problem at all now. I wonder what changed..
And I see there are lots of changes in 3.18.

I'll discard my patch and wait to see if the problem recurs.

Thanks for your time,
NeilBrown