If a key is to be used for wake-up, we must not disable
the interrupt during suspend.
Signed-off-by: NeilBrown <[email protected]>
diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
index 8c98e97f8e41..0b5e54ae343e 100644
--- a/drivers/input/keyboard/gpio_keys.c
+++ b/drivers/input/keyboard/gpio_keys.c
@@ -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);
Hi Neil,
On Sat, Nov 08, 2014 at 10:37:07AM +1100, NeilBrown wrote:
>
>
> If a key is to be used for wake-up, we must not disable
> the interrupt during suspend.
>
> Signed-off-by: NeilBrown <[email protected]>
>
> diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
> index 8c98e97f8e41..0b5e54ae343e 100644
> --- a/drivers/input/keyboard/gpio_keys.c
> +++ b/drivers/input/keyboard/gpio_keys.c
> @@ -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?
>
> error = devm_request_any_context_irq(&pdev->dev, bdata->irq,
> isr, irqflags, desc, bdata);
Thanks.
--
Dmitry
On Saturday, November 08, 2014 11:00:58 AM NeilBrown wrote:
> On Fri, 7 Nov 2014 15:45:07 -0800 Dmitry Torokhov <[email protected]>
> wrote:
>
> > Hi Neil,
> >
> > On Sat, Nov 08, 2014 at 10:37:07AM +1100, NeilBrown wrote:
> > >
> > >
> > > If a key is to be used for wake-up, we must not disable
> > > the interrupt during suspend.
> > >
> > > Signed-off-by: NeilBrown <[email protected]>
> > >
> > > diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
> > > index 8c98e97f8e41..0b5e54ae343e 100644
> > > --- a/drivers/input/keyboard/gpio_keys.c
> > > +++ b/drivers/input/keyboard/gpio_keys.c
> > > @@ -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.
This was a fairly recent change made in 3.18-rc1.
> 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....
On some platforms it actually changes the configuration of interrupt
controllers in to a "suspend mode" which is not appropriate for run time
AFAICS.
> I'll see what I can find.
Yes, please.
Rafael
On Fri, 7 Nov 2014 15:45:07 -0800 Dmitry Torokhov <[email protected]>
wrote:
> Hi Neil,
>
> On Sat, Nov 08, 2014 at 10:37:07AM +1100, NeilBrown wrote:
> >
> >
> > If a key is to be used for wake-up, we must not disable
> > the interrupt during suspend.
> >
> > Signed-off-by: NeilBrown <[email protected]>
> >
> > diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
> > index 8c98e97f8e41..0b5e54ae343e 100644
> > --- a/drivers/input/keyboard/gpio_keys.c
> > +++ b/drivers/input/keyboard/gpio_keys.c
> > @@ -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
On Sat, 08 Nov 2014 01:49:06 +0100 "Rafael J. Wysocki" <[email protected]>
wrote:
> On Saturday, November 08, 2014 11:00:58 AM NeilBrown wrote:
> > On Fri, 7 Nov 2014 15:45:07 -0800 Dmitry Torokhov <[email protected]>
> > wrote:
> >
> > > Hi Neil,
> > >
> > > On Sat, Nov 08, 2014 at 10:37:07AM +1100, NeilBrown wrote:
> > > >
> > > >
> > > > If a key is to be used for wake-up, we must not disable
> > > > the interrupt during suspend.
> > > >
> > > > Signed-off-by: NeilBrown <[email protected]>
> > > >
> > > > diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
> > > > index 8c98e97f8e41..0b5e54ae343e 100644
> > > > --- a/drivers/input/keyboard/gpio_keys.c
> > > > +++ b/drivers/input/keyboard/gpio_keys.c
> > > > @@ -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.
>
> This was a fairly recent change made in 3.18-rc1.
>
> > 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....
>
> On some platforms it actually changes the configuration of interrupt
> controllers in to a "suspend mode" which is not appropriate for run time
> AFAICS.
>
> > 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