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

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

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