[PATCH 1/1] input: bbnsm_pwrkey: Fix missed key press after suspend

[PATCH 1/1] input: bbnsm_pwrkey: Fix missed key press after suspend

[Frank Li]

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.
+	 */
+	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,
-- 
2.34.1

Re: [PATCH 1/1] input: bbnsm_pwrkey: Fix missed key press after suspend

[Dmitry Torokhov]

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.

-- 
Dmitry

Re: [PATCH 1/1] input: bbnsm_pwrkey: Fix missed key press after suspend

[Frank Li]

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

Re: [PATCH 1/1] input: bbnsm_pwrkey: Fix missed key press after suspend

[Dmitry Torokhov]

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.

-- 
Dmitry