| Message ID | 20240712224352.82190-1-Frank.Li@nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/1] input: bbnsm_pwrkey: Fix missed key press after suspend | expand |
Hi Frank, On Fri, Jul 12, 2024 at 06:43:51PM -0400, Frank Li wrote: > From: Jacky Bai <ping.bai@nxp.com> > > Report input event directly on wakeup to ensure no press event is missed > when resuming from suspend. > > Signed-off-by: Jacky Bai <ping.bai@nxp.com> > Reviewed-by: Peng Fan <peng.fan@nxp.com> > Acked-by: Jason Liu <jason.hui.liu@nxp.com> > Signed-off-by: Frank Li <Frank.Li@nxp.com> > --- > drivers/input/misc/nxp-bbnsm-pwrkey.c | 36 +++++++++++++++++++++++++++ > 1 file changed, 36 insertions(+) > > diff --git a/drivers/input/misc/nxp-bbnsm-pwrkey.c b/drivers/input/misc/nxp-bbnsm-pwrkey.c > index 1d99206dd3a8b..9675717ecbdfe 100644 > --- a/drivers/input/misc/nxp-bbnsm-pwrkey.c > +++ b/drivers/input/misc/nxp-bbnsm-pwrkey.c > @@ -38,6 +38,7 @@ struct bbnsm_pwrkey { > int irq; > int keycode; > int keystate; /* 1:pressed */ > + bool suspended; > struct timer_list check_timer; > struct input_dev *input; > }; > @@ -70,6 +71,7 @@ static irqreturn_t bbnsm_pwrkey_interrupt(int irq, void *dev_id) > { > struct platform_device *pdev = dev_id; > struct bbnsm_pwrkey *bbnsm = platform_get_drvdata(pdev); > + struct input_dev *input = bbnsm->input; > u32 event; > > regmap_read(bbnsm->regmap, BBNSM_EVENTS, &event); > @@ -81,6 +83,16 @@ static irqreturn_t bbnsm_pwrkey_interrupt(int irq, void *dev_id) > mod_timer(&bbnsm->check_timer, > jiffies + msecs_to_jiffies(DEBOUNCE_TIME)); > > + /* > + * Directly report key event after resume to make sure key press > + * event is never missed. > + */ How do you know that wakeup was caused by the key press on this device? As far as I can see the driver requests the interrupt as shared, so we could end up in bbnsm_pwrkey_interrupt() even if button was not pressed. Thanks.
On Fri, Jul 12, 2024 at 04:04:28PM -0700, Dmitry Torokhov wrote: > Hi Frank, > > On Fri, Jul 12, 2024 at 06:43:51PM -0400, Frank Li wrote: > > From: Jacky Bai <ping.bai@nxp.com> > > > > Report input event directly on wakeup to ensure no press event is missed > > when resuming from suspend. > > > > Signed-off-by: Jacky Bai <ping.bai@nxp.com> > > Reviewed-by: Peng Fan <peng.fan@nxp.com> > > Acked-by: Jason Liu <jason.hui.liu@nxp.com> > > Signed-off-by: Frank Li <Frank.Li@nxp.com> > > --- > > drivers/input/misc/nxp-bbnsm-pwrkey.c | 36 +++++++++++++++++++++++++++ > > 1 file changed, 36 insertions(+) > > > > diff --git a/drivers/input/misc/nxp-bbnsm-pwrkey.c b/drivers/input/misc/nxp-bbnsm-pwrkey.c > > index 1d99206dd3a8b..9675717ecbdfe 100644 > > --- a/drivers/input/misc/nxp-bbnsm-pwrkey.c > > +++ b/drivers/input/misc/nxp-bbnsm-pwrkey.c > > @@ -38,6 +38,7 @@ struct bbnsm_pwrkey { > > int irq; > > int keycode; > > int keystate; /* 1:pressed */ > > + bool suspended; > > struct timer_list check_timer; > > struct input_dev *input; > > }; > > @@ -70,6 +71,7 @@ static irqreturn_t bbnsm_pwrkey_interrupt(int irq, void *dev_id) > > { > > struct platform_device *pdev = dev_id; > > struct bbnsm_pwrkey *bbnsm = platform_get_drvdata(pdev); > > + struct input_dev *input = bbnsm->input; > > u32 event; > > > > regmap_read(bbnsm->regmap, BBNSM_EVENTS, &event); > > @@ -81,6 +83,16 @@ static irqreturn_t bbnsm_pwrkey_interrupt(int irq, void *dev_id) > > mod_timer(&bbnsm->check_timer, > > jiffies + msecs_to_jiffies(DEBOUNCE_TIME)); > > > > + /* > > + * Directly report key event after resume to make sure key press > > + * event is never missed. > > + */ > > How do you know that wakeup was caused by the key press on this device? > As far as I can see the driver requests the interrupt as shared, so we > could end up in bbnsm_pwrkey_interrupt() even if button was not pressed. > In bbnsm_pwrky_interrupt() { ... regmap_read(bbnsm->regmap, BBNSM_EVENTS, &event); if (!(event & BBNSM_BTN_OFF)) return IRQ_NONE; ... } If wakeup was not caused by pwr key, irq will do nothing, code will not reach to here. Frank > Thanks. > > -- > Dmitry
On Fri, Jul 12, 2024 at 07:17:11PM -0400, Frank Li wrote: > On Fri, Jul 12, 2024 at 04:04:28PM -0700, Dmitry Torokhov wrote: > > Hi Frank, > > > > On Fri, Jul 12, 2024 at 06:43:51PM -0400, Frank Li wrote: > > > From: Jacky Bai <ping.bai@nxp.com> > > > > > > Report input event directly on wakeup to ensure no press event is missed > > > when resuming from suspend. > > > > > > Signed-off-by: Jacky Bai <ping.bai@nxp.com> > > > Reviewed-by: Peng Fan <peng.fan@nxp.com> > > > Acked-by: Jason Liu <jason.hui.liu@nxp.com> > > > Signed-off-by: Frank Li <Frank.Li@nxp.com> > > > --- > > > drivers/input/misc/nxp-bbnsm-pwrkey.c | 36 +++++++++++++++++++++++++++ > > > 1 file changed, 36 insertions(+) > > > > > > diff --git a/drivers/input/misc/nxp-bbnsm-pwrkey.c b/drivers/input/misc/nxp-bbnsm-pwrkey.c > > > index 1d99206dd3a8b..9675717ecbdfe 100644 > > > --- a/drivers/input/misc/nxp-bbnsm-pwrkey.c > > > +++ b/drivers/input/misc/nxp-bbnsm-pwrkey.c > > > @@ -38,6 +38,7 @@ struct bbnsm_pwrkey { > > > int irq; > > > int keycode; > > > int keystate; /* 1:pressed */ > > > + bool suspended; > > > struct timer_list check_timer; > > > struct input_dev *input; > > > }; > > > @@ -70,6 +71,7 @@ static irqreturn_t bbnsm_pwrkey_interrupt(int irq, void *dev_id) > > > { > > > struct platform_device *pdev = dev_id; > > > struct bbnsm_pwrkey *bbnsm = platform_get_drvdata(pdev); > > > + struct input_dev *input = bbnsm->input; > > > u32 event; > > > > > > regmap_read(bbnsm->regmap, BBNSM_EVENTS, &event); > > > @@ -81,6 +83,16 @@ static irqreturn_t bbnsm_pwrkey_interrupt(int irq, void *dev_id) > > > mod_timer(&bbnsm->check_timer, > > > jiffies + msecs_to_jiffies(DEBOUNCE_TIME)); > > > > > > + /* > > > + * Directly report key event after resume to make sure key press > > > + * event is never missed. > > > + */ > > > > How do you know that wakeup was caused by the key press on this device? > > As far as I can see the driver requests the interrupt as shared, so we > > could end up in bbnsm_pwrkey_interrupt() even if button was not pressed. > > > > In bbnsm_pwrky_interrupt() > { ... > regmap_read(bbnsm->regmap, BBNSM_EVENTS, &event); > if (!(event & BBNSM_BTN_OFF)) > return IRQ_NONE; > > ... > } > > If wakeup was not caused by pwr key, irq will do nothing, code will not > reach to here. OK, so the case you are trying to fix is when interrupt is delayed so the key is either already released or it will be released by the time the normal debouncing interval passes? I think you want to reset ->suspended in the interrupt handler so that this block fires at most once per suspend/resume cycle. And maybe move it before calling mod_timer(). Thanks.
diff --git a/drivers/input/misc/nxp-bbnsm-pwrkey.c b/drivers/input/misc/nxp-bbnsm-pwrkey.c index 1d99206dd3a8b..9675717ecbdfe 100644 --- a/drivers/input/misc/nxp-bbnsm-pwrkey.c +++ b/drivers/input/misc/nxp-bbnsm-pwrkey.c @@ -38,6 +38,7 @@ struct bbnsm_pwrkey { int irq; int keycode; int keystate; /* 1:pressed */ + bool suspended; struct timer_list check_timer; struct input_dev *input; }; @@ -70,6 +71,7 @@ static irqreturn_t bbnsm_pwrkey_interrupt(int irq, void *dev_id) { struct platform_device *pdev = dev_id; struct bbnsm_pwrkey *bbnsm = platform_get_drvdata(pdev); + struct input_dev *input = bbnsm->input; u32 event; regmap_read(bbnsm->regmap, BBNSM_EVENTS, &event); @@ -81,6 +83,16 @@ static irqreturn_t bbnsm_pwrkey_interrupt(int irq, void *dev_id) mod_timer(&bbnsm->check_timer, jiffies + msecs_to_jiffies(DEBOUNCE_TIME)); + /* + * Directly report key event after resume to make sure key press + * event is never missed. + */ + if (bbnsm->suspended) { + bbnsm->keystate = 1; + input_event(input, EV_KEY, bbnsm->keycode, 1); + input_sync(input); + } + /* clear PWR OFF */ regmap_write(bbnsm->regmap, BBNSM_EVENTS, BBNSM_BTN_OFF); @@ -173,6 +185,29 @@ static int bbnsm_pwrkey_probe(struct platform_device *pdev) return 0; } +static int __maybe_unused bbnsm_pwrkey_suspend(struct device *dev) +{ + struct platform_device *pdev = to_platform_device(dev); + struct bbnsm_pwrkey *bbnsm = platform_get_drvdata(pdev); + + bbnsm->suspended = true; + + return 0; +} + +static int __maybe_unused bbnsm_pwrkey_resume(struct device *dev) +{ + struct platform_device *pdev = to_platform_device(dev); + struct bbnsm_pwrkey *bbnsm = platform_get_drvdata(pdev); + + bbnsm->suspended = false; + + return 0; +} + +static SIMPLE_DEV_PM_OPS(bbnsm_pwrkey_pm_ops, bbnsm_pwrkey_suspend, + bbnsm_pwrkey_resume); + static const struct of_device_id bbnsm_pwrkey_ids[] = { { .compatible = "nxp,imx93-bbnsm-pwrkey" }, { /* sentinel */ } @@ -182,6 +217,7 @@ MODULE_DEVICE_TABLE(of, bbnsm_pwrkey_ids); static struct platform_driver bbnsm_pwrkey_driver = { .driver = { .name = "bbnsm_pwrkey", + .pm = &bbnsm_pwrkey_pm_ops, .of_match_table = bbnsm_pwrkey_ids, }, .probe = bbnsm_pwrkey_probe,