Re: [PATCH v2 4/4] Input: snvs_pwrkey - report press event in interrupt handler

[Joy Zou <joy.zou@oss.nxp.com>]

On Fri, Jun 05, 2026 at 02:59:13PM +0800, Bough Chen wrote:
> On Thu, Jun 04, 2026 at 02:56:24PM +0800, Joy Zou wrote:
> > The driver implements debounce protection using a timer-based mechanism:
> > when a key interrupt occurs, a timer is scheduled to verify the key state
> > after DEBOUNCE_TIME before reporting the event. This works well during
> > normal operation.
> > 
> > However, key press events can be lost during system resume on platforms
> > like i.MX8MQ-EVK because:
> > 1. During the no_irq resume phase, PCIe driver restoration can take up to
> > 200ms with IRQs disabled.
> > 2. The power key interrupt remains pending during the no_irq phase.
> > 3. If the key is released before IRQs are re-enabled, the timer eventually
> > runs but sees the key as released and skips reporting the event.
> > 
> > Report key press events directly in interrupt handler to prevent event
> > loss during system suspend. This is safe because:
> > 
> > 1. Only one event is reported per suspend cycle.
> > 2. Normal operation retains the existing timer-based debounce mechanism.
> > 
> > Signed-off-by: Joy Zou <joy.zou@nxp.com>
> > ---
> > Changes for v2:
> > 1. Add a boolean variable suspended and PM callback functions to replace
> >    the use of the is_suspended field per AI review comments.
> > 2. Move event report handle to else branch in suspended state, since the
> >    pdata->minor_rev == 0 branch has no debounce detection per AI review
> >    comments.
> > 3. Modify the commit message.
> > ---
> >  drivers/input/keyboard/snvs_pwrkey.c | 35 +++++++++++++++++++++++++++++++++++
> >  1 file changed, 35 insertions(+)
> > 
> > diff --git a/drivers/input/keyboard/snvs_pwrkey.c b/drivers/input/keyboard/snvs_pwrkey.c
> > index 4a1d04898482669894e9978014b62e4e9774b4e4..f212a6b26185d13e1af62728e7b2add5010adc5a 100644
> > --- a/drivers/input/keyboard/snvs_pwrkey.c
> > +++ b/drivers/input/keyboard/snvs_pwrkey.c
> > @@ -39,6 +39,7 @@ struct pwrkey_drv_data {
> >  	int keycode;
> >  	int keystate;  /* 1:pressed */
> >  	int wakeup;
> > +	bool suspended; /* Track suspend state */
> >  	struct timer_list check_timer;
> >  	struct input_dev *input;
> >  	u8 minor_rev;
> > @@ -92,6 +93,15 @@ static irqreturn_t imx_snvs_pwrkey_interrupt(int irq, void *dev_id)
> >  			input_sync(input);
> >  			pm_relax(input->dev.parent);
> >  		} else {
> > +			/*
> > +			 * Report key press events directly in interrupt handler to prevent event
> > +			 * loss during system suspend.
> > +			 */
> > +			if (pdata->suspended) {
> 
> Here, pdata->suspended should not be safe for SMP system, maybe to use atomic_read() to make the code more stronger.
Thanks for your comments! You're right that pdata->suspended needs protection.
However, AI review found there are the pdata->keystate that also need protection.
Instead of using atomic operations for individual fields, I think it would be
more maintainable to use a single lock to protect all related fields consistently.
> 
> > +				pdata->keystate = 1;
> > +				input_report_key(input, pdata->keycode, 1);
> > +				input_sync(input);
> > +			}
> >  			mod_timer(&pdata->check_timer,
> >  			          jiffies + msecs_to_jiffies(DEBOUNCE_TIME));
> 
> Here already report the key when pdata->suspend == true, seems do not need to trigger the timer again.
> Is it more reasonable to change like this?
> if (atomic_read(&pdata->suspended)) {
> 	...
> } else {
> 	mode_timer()
> }
Have checked the interrupt handler only report directly press event, still need
to report release event in timer callback. So can't use the way.
BR
Joy Zou
> 
> >  		}
> > @@ -219,6 +229,30 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
> >  	return 0;
> >  }
> >  
> > +static int __maybe_unused imx_snvs_pwrkey_suspend(struct device *dev)
> > +{
> > +	struct platform_device *pdev = to_platform_device(dev);
> > +	struct pwrkey_drv_data *pdata = platform_get_drvdata(pdev);
> > +
> > +	pdata->suspended = true;
> > +
> 
> atomic_set(&pdata->suspended, 1) 
> 
> > +	return 0;
> > +}
> > +
> > +static int __maybe_unused imx_snvs_pwrkey_resume(struct device *dev)
> > +{
> > +	struct platform_device *pdev = to_platform_device(dev);
> > +	struct pwrkey_drv_data *pdata = platform_get_drvdata(pdev);
> > +
> > +	pdata->suspended = false;
> > +
> 
> atomic_set(&pdata->suspended, 0);
> 
> Regards
> Haibo Chen
> > +	return 0;
> > +}
> > +
> > +static SIMPLE_DEV_PM_OPS(imx_snvs_pwrkey_pm_ops,
> > +			 imx_snvs_pwrkey_suspend,
> > +			 imx_snvs_pwrkey_resume);
> > +
> >  static const struct of_device_id imx_snvs_pwrkey_ids[] = {
> >  	{ .compatible = "fsl,sec-v4.0-pwrkey" },
> >  	{ /* sentinel */ }
> > @@ -229,6 +263,7 @@ static struct platform_driver imx_snvs_pwrkey_driver = {
> >  	.driver = {
> >  		.name = "snvs_pwrkey",
> >  		.of_match_table = imx_snvs_pwrkey_ids,
> > +		.pm   = &imx_snvs_pwrkey_pm_ops,
> >  	},
> >  	.probe = imx_snvs_pwrkey_probe,
> >  };
> > 
> > -- 
> > 2.50.1
> >