| Message ID | 20141108103707.45bcc7e3@notabene.brown (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <neilb@suse.de> > > 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.
On Fri, 7 Nov 2014 15:45:07 -0800 Dmitry Torokhov <dmitry.torokhov@gmail.com> 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 <neilb@suse.de> > > > > 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 Saturday, November 08, 2014 11:00:58 AM NeilBrown wrote: > On Fri, 7 Nov 2014 15:45:07 -0800 Dmitry Torokhov <dmitry.torokhov@gmail.com> > 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 <neilb@suse.de> > > > > > > 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 Sat, 08 Nov 2014 01:49:06 +0100 "Rafael J. Wysocki" <rjw@rjwysocki.net> wrote: > On Saturday, November 08, 2014 11:00:58 AM NeilBrown wrote: > > On Fri, 7 Nov 2014 15:45:07 -0800 Dmitry Torokhov <dmitry.torokhov@gmail.com> > > 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 <neilb@suse.de> > > > > > > > > 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
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);
If a key is to be used for wake-up, we must not disable the interrupt during suspend. Signed-off-by: NeilBrown <neilb@suse.de>