[PATCH v3 0/4] snvs_pwrkey - code improvements and add report event

[PATCH v3 0/4] snvs_pwrkey - code improvements and add report event

[joy.zou]

This patch series improves the snvs_pwrkey driver with better code quality
and add report press event.

The main improvements include:
1. Clean up the code by using local device pointers and dev_err_probe() for
better readability and easier debugging.

2. Fix potential event loss during system suspend by reporting key press events
directly in the interrupt handler.

Signed-off-by: Joy Zou <joy.zou@oss.nxp.com>
---
Changes in v3:
- Add spinlock for pdata->keystate and pdata->suspended per AI review comments.
- Replace hardcode value 1 with local variable keystate in input_report_key()
  under suspended.
- Link to v2: https://lore.kernel.org/r/20260604-pwrkey-cleanup-v2-0-59fa0ddf0d7c@nxp.com

Changes in v2:
- Drop dev_err_probe() change for platform_get_irq() per AI review comments.
- Add new patch #2 replace hardcoding -EINVAL with original error code per
  AI review comments.
- Use dev instead of &pdev->dev for devm_input_allocate_device(), which was
  missed in patch v1 per AI review comments.
- Add a boolean variable suspended and PM callback functions to replace
  the use of the is_suspended field per AI review comments.
- 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.
- Modify patch #3 and #4 commit message.
- Add Reviewed-by tag for patch #1.
- Link to v1: https://lore.kernel.org/r/20260326-pwrkey-cleanup-v1-0-d85d7c0bf275@nxp.com

To: Dmitry Torokhov <dmitry.torokhov@gmail.com>
To: Frank Li <Frank.Li@nxp.com>
To: Bough Chen <haibo.chen@nxp.com>
To: Peng Fan <peng.fan@nxp.com>
To: Jacky Bai <ping.bai@nxp.com>
To: Ye Li <ye.li@nxp.com>
Cc: imx@lists.linux.dev
Cc: linux-input@vger.kernel.org
Cc: linux-kernel@vger.kernel.org

---
Joy Zou (4):
      Input: snvs_pwrkey - make use of dev_err_probe()
      Input: snvs_pwrkey - replace hardcoding -EINVAL with original error code
      Input: snvs_pwrkey - use local device pointer to simple code
      Input: snvs_pwrkey - report press event in interrupt handler

 drivers/input/keyboard/snvs_pwrkey.c | 127 ++++++++++++++++++++++++-----------
 1 file changed, 86 insertions(+), 41 deletions(-)
---
base-commit: f7af91adc230aa99e23330ecf85bc9badd9780ad
change-id: 20260615-b4-pwrkey-a60b2043cd29

Best regards,
--  
Joy Zou <joy.zou@oss.nxp.com>

[PATCH v3 1/4] Input: snvs_pwrkey - make use of dev_err_probe()

[joy.zou]

From: Joy Zou <joy.zou@nxp.com>

Add dev_err_probe() at return path of probe() to support users to
identify issues easier.

Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Joy Zou <joy.zou@nxp.com>
---
Changes in v2:
1. Drop dev_err_probe() change for platform_get_irq() per AI review comments.
2. Add Reviewed-by tag.
---
 drivers/input/keyboard/snvs_pwrkey.c | 38 +++++++++++++-----------------------
 1 file changed, 14 insertions(+), 24 deletions(-)

diff --git a/drivers/input/keyboard/snvs_pwrkey.c b/drivers/input/keyboard/snvs_pwrkey.c
index 954055aaf6e2..748196fcce75 100644
--- a/drivers/input/keyboard/snvs_pwrkey.c
+++ b/drivers/input/keyboard/snvs_pwrkey.c
@@ -124,17 +124,15 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
 	/* Get SNVS register Page */
 	np = pdev->dev.of_node;
 	if (!np)
-		return -ENODEV;
+		return dev_err_probe(&pdev->dev, -ENODEV, "Device tree node not found\n");
 
 	pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
 	if (!pdata)
 		return -ENOMEM;
 
 	pdata->snvs = syscon_regmap_lookup_by_phandle(np, "regmap");
-	if (IS_ERR(pdata->snvs)) {
-		dev_err(&pdev->dev, "Can't get snvs syscon\n");
-		return PTR_ERR(pdata->snvs);
-	}
+	if (IS_ERR(pdata->snvs))
+		return dev_err_probe(&pdev->dev, PTR_ERR(pdata->snvs), "Can't get snvs syscon\n");
 
 	if (of_property_read_u32(np, "linux,keycode", &pdata->keycode)) {
 		pdata->keycode = KEY_POWER;
@@ -142,10 +140,9 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
 	}
 
 	clk = devm_clk_get_optional_enabled(&pdev->dev, NULL);
-	if (IS_ERR(clk)) {
-		dev_err(&pdev->dev, "Failed to get snvs clock (%pe)\n", clk);
-		return PTR_ERR(clk);
-	}
+	if (IS_ERR(clk))
+		return dev_err_probe(&pdev->dev, PTR_ERR(clk),
+				     "Failed to get snvs clock (%pe)\n", clk);
 
 	pdata->wakeup = of_property_read_bool(np, "wakeup-source");
 
@@ -165,9 +162,8 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
 			bpt = (val / 5) - 1;
 			break;
 		default:
-			dev_err(&pdev->dev,
-				"power-off-time-sec %d out of range\n", val);
-			return -EINVAL;
+			return dev_err_probe(&pdev->dev, -EINVAL,
+					     "power-off-time-sec %d out of range\n", val);
 		}
 
 		regmap_update_bits(pdata->snvs, SNVS_LPCR_REG, SNVS_LPCR_BPT_MASK,
@@ -198,10 +194,8 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
 
 	/* input customer action to cancel release timer */
 	error = devm_add_action(&pdev->dev, imx_snvs_pwrkey_act, pdata);
-	if (error) {
-		dev_err(&pdev->dev, "failed to register remove action\n");
-		return error;
-	}
+	if (error)
+		return dev_err_probe(&pdev->dev, error, "failed to register remove action\n");
 
 	pdata->input = input;
 	platform_set_drvdata(pdev, pdata);
@@ -209,16 +203,12 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
 	error = devm_request_irq(&pdev->dev, pdata->irq,
 			       imx_snvs_pwrkey_interrupt,
 			       0, pdev->name, pdev);
-	if (error) {
-		dev_err(&pdev->dev, "interrupt not available.\n");
-		return error;
-	}
+	if (error)
+		return dev_err_probe(&pdev->dev, error, "interrupt not available.\n");
 
 	error = input_register_device(input);
-	if (error < 0) {
-		dev_err(&pdev->dev, "failed to register input device\n");
-		return error;
-	}
+	if (error < 0)
+		return dev_err_probe(&pdev->dev, error, "failed to register input device\n");
 
 	device_init_wakeup(&pdev->dev, pdata->wakeup);
 	error = dev_pm_set_wake_irq(&pdev->dev, pdata->irq);

-- 
2.34.1

Re: [PATCH v3 1/4] Input: snvs_pwrkey - make use of dev_err_probe()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [Medium] Missing `.remove` callback causes a leak of `struct wake_irq` and leaves dangling wakeup configuration upon driver unbind.
--

commit 07567ee0ff85afeb83d7fcfa00735130f4ec77e0
Author: Joy Zou <joy.zou@nxp.com>
Input: snvs_pwrkey - make use of dev_err_probe()

This commit updates the imx_snvs_pwrkey_probe function to use dev_err_probe
for its error paths. This helps simplify error logging and properly handles
probe deferral conditions.

> diff --git a/drivers/input/keyboard/snvs_pwrkey.c b/drivers/input/keyboard/snvs_pwrkey.c
> --- a/drivers/input/keyboard/snvs_pwrkey.c
> +++ b/drivers/input/keyboard/snvs_pwrkey.c
[ ... ]
> @@ -209,16 +203,12 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
>  	error = devm_request_irq(&pdev->dev, pdata->irq,
>  			       imx_snvs_pwrkey_interrupt,
>  			       0, pdev->name, pdev);
> -	if (error) {
> -		dev_err(&pdev->dev, "interrupt not available.\n");
> -		return error;
> -	}
> +	if (error)
> +		return dev_err_probe(&pdev->dev, error, "interrupt not available.\n");
>  
>  	error = input_register_device(input);
> -	if (error < 0) {
> -		dev_err(&pdev->dev, "failed to register input device\n");
> -		return error;
> -	}
> +	if (error < 0)
> +		return dev_err_probe(&pdev->dev, error, "failed to register input device\n");
>  
>  	device_init_wakeup(&pdev->dev, pdata->wakeup);
>  	error = dev_pm_set_wake_irq(&pdev->dev, pdata->irq);

[Severity: Medium]
This is a pre-existing issue, but I noticed there might be a resource cleanup
issue here in imx_snvs_pwrkey_probe().

Does this code leak the wakeup resources when the driver is unbound?

It appears that device_init_wakeup() and dev_pm_set_wake_irq() allocate
unmanaged resources and attach them to the device. Because there is no remove
callback or managed action to clean them up via dev_pm_clear_wake_irq() and
device_init_wakeup(dev, false), these resources might be leaked on unbind.

If the driver is later re-bound, could this also cause probe to fail since
dev_pm_attach_wake_irq() returns -EEXIST when the wakeirq is already
initialized?

Would it be appropriate to switch these to devm_device_init_wakeup() and
devm_pm_set_wake_irq() to handle the cleanup automatically?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260615-b4-pwrkey-v3-0-9510b1173f6e@oss.nxp.com?part=1

[PATCH v3 2/4] Input: snvs_pwrkey - replace hardcoding -EINVAL with original error code

[joy.zou]

From: Joy Zou <joy.zou@nxp.com>

Hardcoding -EINVAL discards the actual error code, which breaks probe
deferral (-EPROBE_DEFER) and loses critical diagnostic information
needed for proper kernel error handling.

Signed-off-by: Joy Zou <joy.zou@nxp.com>
---
 drivers/input/keyboard/snvs_pwrkey.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/input/keyboard/snvs_pwrkey.c b/drivers/input/keyboard/snvs_pwrkey.c
index 748196fcce75..a291812e6d22 100644
--- a/drivers/input/keyboard/snvs_pwrkey.c
+++ b/drivers/input/keyboard/snvs_pwrkey.c
@@ -148,7 +148,7 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
 
 	pdata->irq = platform_get_irq(pdev, 0);
 	if (pdata->irq < 0)
-		return -EINVAL;
+		return pdata->irq;
 
 	error = of_property_read_u32(np, "power-off-time-sec", &val);
 	if (!error) {

-- 
2.34.1

Re: [PATCH v3 2/4] Input: snvs_pwrkey - replace hardcoding -EINVAL with original error code

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing cleanup for dev_pm_set_wake_irq() causes memory leak and state corruption on driver unbind.
- [High] Systematic failure to check regmap_read() return values leads to consuming uninitialized stack memory, risking spurious system shutdown.
--

commit d2ee00e9f90457779e6f0d87a01d54e5af524f82
Author: Joy Zou <joy.zou@nxp.com>

Input: snvs_pwrkey - replace hardcoding -EINVAL with original error code

This commit replaces a hardcoded -EINVAL with the actual error code returned
by platform_get_irq(). This preserves valuable diagnostic information and
allows probe deferral (-EPROBE_DEFER) to work correctly.

> diff --git a/drivers/input/keyboard/snvs_pwrkey.c b/drivers/input/keyboard/snvs_pwrkey.c
> index 748196fcce75b..a291812e6d22e 100644
> --- a/drivers/input/keyboard/snvs_pwrkey.c
> +++ b/drivers/input/keyboard/snvs_pwrkey.c
> @@ -148,7 +148,7 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
>  
>  	pdata->irq = platform_get_irq(pdev, 0);
>  	if (pdata->irq < 0)
> -		return -EINVAL;
> +		return pdata->irq;
>  
>  	error = of_property_read_u32(np, "power-off-time-sec", &val);
>  	if (!error) {

[ ... ]

[Severity: High]
This is a pre-existing issue, but I noticed a potential lifecycle regression
further down in imx_snvs_pwrkey_probe():

drivers/input/keyboard/snvs_pwrkey.c:imx_snvs_pwrkey_probe() {
    ...
	error = dev_pm_set_wake_irq(dev, pdata->irq);
	if (error)
		dev_err(dev, "irq wake enable failed.\n");
    ...
}

The driver configures a wake IRQ using dev_pm_set_wake_irq(), which
allocates a wake_irq structure and attaches it to dev->power.wakeirq.
However, there doesn't appear to be a paired remove callback to call
dev_pm_clear_wake_irq() during driver unbind.

Does this permanently leak memory if the driver is unbound via sysfs?

Additionally, since the underlying IRQ is managed by devm_request_irq(), it
will be freed on unbind while dev->power.wakeirq remains configured. If the
system later attempts to suspend, could the PM core evaluate
device_may_wakeup() (which remains true since device_init_wakeup() isn't
reverted) and call enable_irq_wake() on a freed IRQ?

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but I also noticed that
regmap_read() return values are not checked in several places before
consuming the variables passed by reference:

drivers/input/keyboard/snvs_pwrkey.c:imx_snvs_pwrkey_interrupt() {
    ...
	u32 lp_status;

	pm_wakeup_event(input->dev.parent, 0);

	regmap_read(pdata->snvs, SNVS_LPSR_REG, &lp_status);
	if (lp_status & SNVS_LPSR_SPO) {
    ...
}

It also happens in the timer handler:

drivers/input/keyboard/snvs_pwrkey.c:imx_imx_snvs_check_for_events() {
    ...
	u32 state;

	regmap_read(pdata->snvs, SNVS_HPSR_REG, &state);
	state = state & SNVS_HPSR_BTN ? 1 : 0;
    ...
}

If regmap_read() fails (e.g., due to an underlying clock enable failure or
MMIO bus error), it returns an error code and leaves the target stack
variables (lp_status, state) uninitialized.

Since these uninitialized stack variables are then used in logic checks,
could this lead to unpredictable behavior? For example, in the interrupt
and timer handlers, if the uninitialized stack memory randomly has the
SNVS_LPSR_SPO or SNVS_HPSR_BTN bits set, would this trigger a spurious
KEY_POWER event and an unintended system shutdown?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260615-b4-pwrkey-v3-0-9510b1173f6e@oss.nxp.com?part=2

Re: [PATCH v3 2/4] Input: snvs_pwrkey - replace hardcoding -EINVAL with original error code

[Frank Li]

On Mon, Jun 15, 2026 at 03:52:15PM +0800, joy.zou@oss.nxp.com wrote:

Nit: subject

Input: snvs_pwrkey: propagate error code of platform_get_irq()

>
> Hardcoding -EINVAL discards the actual error code, which breaks probe
> deferral (-EPROBE_DEFER) and loses critical diagnostic information
> needed for proper kernel error handling.
>
> Signed-off-by: Joy Zou <joy.zou@nxp.com>
> ---

Reviewed-by: Frank Li <Frank.Li@nxp.com>

>  drivers/input/keyboard/snvs_pwrkey.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/input/keyboard/snvs_pwrkey.c b/drivers/input/keyboard/snvs_pwrkey.c
> index 748196fcce75..a291812e6d22 100644
> --- a/drivers/input/keyboard/snvs_pwrkey.c
> +++ b/drivers/input/keyboard/snvs_pwrkey.c
> @@ -148,7 +148,7 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
>
>         pdata->irq = platform_get_irq(pdev, 0);
>         if (pdata->irq < 0)
> -               return -EINVAL;
> +               return pdata->irq;
>
>         error = of_property_read_u32(np, "power-off-time-sec", &val);
>         if (!error) {
>
> --
> 2.34.1
>
>

[PATCH v3 3/4] Input: snvs_pwrkey - use local device pointer to simple code

[joy.zou]

From: Joy Zou <joy.zou@nxp.com>

Use local struct device pointer to avoid reference the platform_device
pointer every time.

No functional change.

Signed-off-by: Joy Zou <joy.zou@nxp.com>
---
Changes for v2:
1. Use dev instead of &pdev->dev for devm_input_allocate_device(),
   which was missed in patch v1 per AI review comments.
2. Modify commit message.
---
 drivers/input/keyboard/snvs_pwrkey.c | 41 ++++++++++++++++++------------------
 1 file changed, 21 insertions(+), 20 deletions(-)

diff --git a/drivers/input/keyboard/snvs_pwrkey.c b/drivers/input/keyboard/snvs_pwrkey.c
index a291812e6d22..4a1d04898482 100644
--- a/drivers/input/keyboard/snvs_pwrkey.c
+++ b/drivers/input/keyboard/snvs_pwrkey.c
@@ -112,6 +112,7 @@ static void imx_snvs_pwrkey_act(void *pdata)
 
 static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
 {
+	struct device *dev = &pdev->dev;
 	struct pwrkey_drv_data *pdata;
 	struct input_dev *input;
 	struct device_node *np;
@@ -122,26 +123,26 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
 	u32 vid;
 
 	/* Get SNVS register Page */
-	np = pdev->dev.of_node;
+	np = dev->of_node;
 	if (!np)
-		return dev_err_probe(&pdev->dev, -ENODEV, "Device tree node not found\n");
+		return dev_err_probe(dev, -ENODEV, "Device tree node not found\n");
 
-	pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
+	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
 	if (!pdata)
 		return -ENOMEM;
 
 	pdata->snvs = syscon_regmap_lookup_by_phandle(np, "regmap");
 	if (IS_ERR(pdata->snvs))
-		return dev_err_probe(&pdev->dev, PTR_ERR(pdata->snvs), "Can't get snvs syscon\n");
+		return dev_err_probe(dev, PTR_ERR(pdata->snvs), "Can't get snvs syscon\n");
 
 	if (of_property_read_u32(np, "linux,keycode", &pdata->keycode)) {
 		pdata->keycode = KEY_POWER;
-		dev_warn(&pdev->dev, "KEY_POWER without setting in dts\n");
+		dev_warn(dev, "KEY_POWER without setting in dts\n");
 	}
 
-	clk = devm_clk_get_optional_enabled(&pdev->dev, NULL);
+	clk = devm_clk_get_optional_enabled(dev, NULL);
 	if (IS_ERR(clk))
-		return dev_err_probe(&pdev->dev, PTR_ERR(clk),
+		return dev_err_probe(dev, PTR_ERR(clk),
 				     "Failed to get snvs clock (%pe)\n", clk);
 
 	pdata->wakeup = of_property_read_bool(np, "wakeup-source");
@@ -162,7 +163,7 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
 			bpt = (val / 5) - 1;
 			break;
 		default:
-			return dev_err_probe(&pdev->dev, -EINVAL,
+			return dev_err_probe(dev, -EINVAL,
 					     "power-off-time-sec %d out of range\n", val);
 		}
 
@@ -180,9 +181,9 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
 
 	timer_setup(&pdata->check_timer, imx_imx_snvs_check_for_events, 0);
 
-	input = devm_input_allocate_device(&pdev->dev);
+	input = devm_input_allocate_device(dev);
 	if (!input) {
-		dev_err(&pdev->dev, "failed to allocate the input device\n");
+		dev_err(dev, "failed to allocate the input device\n");
 		return -ENOMEM;
 	}
 
@@ -193,27 +194,27 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
 	input_set_capability(input, EV_KEY, pdata->keycode);
 
 	/* input customer action to cancel release timer */
-	error = devm_add_action(&pdev->dev, imx_snvs_pwrkey_act, pdata);
+	error = devm_add_action(dev, imx_snvs_pwrkey_act, pdata);
 	if (error)
-		return dev_err_probe(&pdev->dev, error, "failed to register remove action\n");
+		return dev_err_probe(dev, error, "failed to register remove action\n");
 
 	pdata->input = input;
 	platform_set_drvdata(pdev, pdata);
 
-	error = devm_request_irq(&pdev->dev, pdata->irq,
-			       imx_snvs_pwrkey_interrupt,
-			       0, pdev->name, pdev);
+	error = devm_request_irq(dev, pdata->irq,
+				 imx_snvs_pwrkey_interrupt,
+				 0, pdev->name, pdev);
 	if (error)
-		return dev_err_probe(&pdev->dev, error, "interrupt not available.\n");
+		return dev_err_probe(dev, error, "interrupt not available.\n");
 
 	error = input_register_device(input);
 	if (error < 0)
-		return dev_err_probe(&pdev->dev, error, "failed to register input device\n");
+		return dev_err_probe(dev, error, "failed to register input device\n");
 
-	device_init_wakeup(&pdev->dev, pdata->wakeup);
-	error = dev_pm_set_wake_irq(&pdev->dev, pdata->irq);
+	device_init_wakeup(dev, pdata->wakeup);
+	error = dev_pm_set_wake_irq(dev, pdata->irq);
 	if (error)
-		dev_err(&pdev->dev, "irq wake enable failed.\n");
+		dev_err(dev, "irq wake enable failed.\n");
 
 	return 0;
 }

-- 
2.34.1

Re: [PATCH v3 3/4] Input: snvs_pwrkey - use local device pointer to simple code

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] The driver calls `dev_pm_set_wake_irq()` but does not use the `devm_` variant or provide a `.remove` function, leading to a resource leak and potentially broken system suspend on driver unbind.
--

commit d7d5ed812213c739d1459c472b4e230aac0a6e8d
Author: Joy Zou <joy.zou@nxp.com>

Input: snvs_pwrkey - use local device pointer to simple code

This commit introduces a local struct device pointer in the probe function
to reduce boilerplate code when referencing the platform device. It is
intended to be a non-functional change.

> diff --git a/drivers/input/keyboard/snvs_pwrkey.c b/drivers/input/keyboard/snvs_pwrkey.c
> --- a/drivers/input/keyboard/snvs_pwrkey.c
> +++ b/drivers/input/keyboard/snvs_pwrkey.c
[ ... ]
> @@ -193,27 +194,27 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
[ ... ]
> -	device_init_wakeup(&pdev->dev, pdata->wakeup);
> -	error = dev_pm_set_wake_irq(&pdev->dev, pdata->irq);
> +	device_init_wakeup(dev, pdata->wakeup);
> +	error = dev_pm_set_wake_irq(dev, pdata->irq);

[Severity: High]
This is a pre-existing issue, but does this leak the wake IRQ on driver 
unbind?

Looking at imx_snvs_pwrkey_probe() in drivers/input/keyboard/snvs_pwrkey.c,
the driver configures the wake IRQ using dev_pm_set_wake_irq() but it does
not use the devm_ variant, nor does it provide a .remove callback to call
dev_pm_clear_wake_irq().

When the driver unbinds, devres automatically frees the IRQ, but the memory
for the wake IRQ structure remains allocated and attached to
dev->power.wakeirq. 

Will a subsequent system suspend then attempt to call enable_irq_wake() on a
freed IRQ number? Also, if the driver is later re-bound, would it trigger a
warning in dev_pm_attach_wake_irq() because dev->power.wakeirq is already
initialized?

Should this be updated to use devm_pm_set_wake_irq() instead?

>  	if (error)
> -		dev_err(&pdev->dev, "irq wake enable failed.\n");
> +		dev_err(dev, "irq wake enable failed.\n");
>  
>  	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260615-b4-pwrkey-v3-0-9510b1173f6e@oss.nxp.com?part=3

Re: [PATCH v3 3/4] Input: snvs_pwrkey - use local device pointer to simple code

[Frank Li]

On Mon, Jun 15, 2026 at 03:52:16PM +0800, joy.zou@oss.nxp.com wrote:
>
> Use local struct device pointer to avoid reference the platform_device
> pointer every time.
>
> No functional change.
>
> Signed-off-by: Joy Zou <joy.zou@nxp.com>
> ---

Reviewed-by: Frank Li <Frank.Li@nxp.com>

> Changes for v2:
> 1. Use dev instead of &pdev->dev for devm_input_allocate_device(),
>    which was missed in patch v1 per AI review comments.
> 2. Modify commit message.
> ---
>  drivers/input/keyboard/snvs_pwrkey.c | 41 ++++++++++++++++++------------------
>  1 file changed, 21 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/input/keyboard/snvs_pwrkey.c b/drivers/input/keyboard/snvs_pwrkey.c
> index a291812e6d22..4a1d04898482 100644
> --- a/drivers/input/keyboard/snvs_pwrkey.c
> +++ b/drivers/input/keyboard/snvs_pwrkey.c
> @@ -112,6 +112,7 @@ static void imx_snvs_pwrkey_act(void *pdata)
>
>  static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
>  {
> +       struct device *dev = &pdev->dev;
>         struct pwrkey_drv_data *pdata;
>         struct input_dev *input;
>         struct device_node *np;
> @@ -122,26 +123,26 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
>         u32 vid;
>
>         /* Get SNVS register Page */
> -       np = pdev->dev.of_node;
> +       np = dev->of_node;
>         if (!np)
> -               return dev_err_probe(&pdev->dev, -ENODEV, "Device tree node not found\n");
> +               return dev_err_probe(dev, -ENODEV, "Device tree node not found\n");
>
> -       pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
> +       pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
>         if (!pdata)
>                 return -ENOMEM;
>
>         pdata->snvs = syscon_regmap_lookup_by_phandle(np, "regmap");
>         if (IS_ERR(pdata->snvs))
> -               return dev_err_probe(&pdev->dev, PTR_ERR(pdata->snvs), "Can't get snvs syscon\n");
> +               return dev_err_probe(dev, PTR_ERR(pdata->snvs), "Can't get snvs syscon\n");
>
>         if (of_property_read_u32(np, "linux,keycode", &pdata->keycode)) {
>                 pdata->keycode = KEY_POWER;
> -               dev_warn(&pdev->dev, "KEY_POWER without setting in dts\n");
> +               dev_warn(dev, "KEY_POWER without setting in dts\n");
>         }
>
> -       clk = devm_clk_get_optional_enabled(&pdev->dev, NULL);
> +       clk = devm_clk_get_optional_enabled(dev, NULL);
>         if (IS_ERR(clk))
> -               return dev_err_probe(&pdev->dev, PTR_ERR(clk),
> +               return dev_err_probe(dev, PTR_ERR(clk),
>                                      "Failed to get snvs clock (%pe)\n", clk);
>
>         pdata->wakeup = of_property_read_bool(np, "wakeup-source");
> @@ -162,7 +163,7 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
>                         bpt = (val / 5) - 1;
>                         break;
>                 default:
> -                       return dev_err_probe(&pdev->dev, -EINVAL,
> +                       return dev_err_probe(dev, -EINVAL,
>                                              "power-off-time-sec %d out of range\n", val);
>                 }
>
> @@ -180,9 +181,9 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
>
>         timer_setup(&pdata->check_timer, imx_imx_snvs_check_for_events, 0);
>
> -       input = devm_input_allocate_device(&pdev->dev);
> +       input = devm_input_allocate_device(dev);
>         if (!input) {
> -               dev_err(&pdev->dev, "failed to allocate the input device\n");
> +               dev_err(dev, "failed to allocate the input device\n");
>                 return -ENOMEM;
>         }
>
> @@ -193,27 +194,27 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
>         input_set_capability(input, EV_KEY, pdata->keycode);
>
>         /* input customer action to cancel release timer */
> -       error = devm_add_action(&pdev->dev, imx_snvs_pwrkey_act, pdata);
> +       error = devm_add_action(dev, imx_snvs_pwrkey_act, pdata);
>         if (error)
> -               return dev_err_probe(&pdev->dev, error, "failed to register remove action\n");
> +               return dev_err_probe(dev, error, "failed to register remove action\n");
>
>         pdata->input = input;
>         platform_set_drvdata(pdev, pdata);
>
> -       error = devm_request_irq(&pdev->dev, pdata->irq,
> -                              imx_snvs_pwrkey_interrupt,
> -                              0, pdev->name, pdev);
> +       error = devm_request_irq(dev, pdata->irq,
> +                                imx_snvs_pwrkey_interrupt,
> +                                0, pdev->name, pdev);
>         if (error)
> -               return dev_err_probe(&pdev->dev, error, "interrupt not available.\n");
> +               return dev_err_probe(dev, error, "interrupt not available.\n");
>
>         error = input_register_device(input);
>         if (error < 0)
> -               return dev_err_probe(&pdev->dev, error, "failed to register input device\n");
> +               return dev_err_probe(dev, error, "failed to register input device\n");
>
> -       device_init_wakeup(&pdev->dev, pdata->wakeup);
> -       error = dev_pm_set_wake_irq(&pdev->dev, pdata->irq);
> +       device_init_wakeup(dev, pdata->wakeup);
> +       error = dev_pm_set_wake_irq(dev, pdata->irq);
>         if (error)
> -               dev_err(&pdev->dev, "irq wake enable failed.\n");
> +               dev_err(dev, "irq wake enable failed.\n");
>
>         return 0;
>  }
>
> --
> 2.34.1
>
>

Re: [PATCH v3 3/4] Input: snvs_pwrkey - use local device pointer to simple code

[Frank Li]

On Mon, Jun 15, 2026 at 03:52:16PM +0800, joy.zou@oss.nxp.com wrote:
> [You don't often get email from joy.zou@oss.nxp.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> From: Joy Zou <joy.zou@nxp.com>
>
> Use local struct device pointer to avoid reference the platform_device
> pointer every time.
>
> No functional change.
>
> Signed-off-by: Joy Zou <joy.zou@nxp.com>
> ---
> Changes for v2:
> 1. Use dev instead of &pdev->dev for devm_input_allocate_device(),
>    which was missed in patch v1 per AI review comments.
> 2. Modify commit message.
> ---
>  drivers/input/keyboard/snvs_pwrkey.c | 41 ++++++++++++++++++------------------
>  1 file changed, 21 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/input/keyboard/snvs_pwrkey.c b/drivers/input/keyboard/snvs_pwrkey.c
> index a291812e6d22..4a1d04898482 100644
> --- a/drivers/input/keyboard/snvs_pwrkey.c
> +++ b/drivers/input/keyboard/snvs_pwrkey.c
> @@ -112,6 +112,7 @@ static void imx_snvs_pwrkey_act(void *pdata)
>
>  static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
>  {
> +       struct device *dev = &pdev->dev;
>         struct pwrkey_drv_data *pdata;
>         struct input_dev *input;
>         struct device_node *np;
> @@ -122,26 +123,26 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
>         u32 vid;
>
>         /* Get SNVS register Page */
> -       np = pdev->dev.of_node;
> +       np = dev->of_node;
>         if (!np)
> -               return dev_err_probe(&pdev->dev, -ENODEV, "Device tree node not found\n");
> +               return dev_err_probe(dev, -ENODEV, "Device tree node not found\n");
>
> -       pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
> +       pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
>         if (!pdata)
>                 return -ENOMEM;
>
>         pdata->snvs = syscon_regmap_lookup_by_phandle(np, "regmap");
>         if (IS_ERR(pdata->snvs))
> -               return dev_err_probe(&pdev->dev, PTR_ERR(pdata->snvs), "Can't get snvs syscon\n");
> +               return dev_err_probe(dev, PTR_ERR(pdata->snvs), "Can't get snvs syscon\n");
>
>         if (of_property_read_u32(np, "linux,keycode", &pdata->keycode)) {
>                 pdata->keycode = KEY_POWER;
> -               dev_warn(&pdev->dev, "KEY_POWER without setting in dts\n");
> +               dev_warn(dev, "KEY_POWER without setting in dts\n");
>         }
>
> -       clk = devm_clk_get_optional_enabled(&pdev->dev, NULL);
> +       clk = devm_clk_get_optional_enabled(dev, NULL);
>         if (IS_ERR(clk))
> -               return dev_err_probe(&pdev->dev, PTR_ERR(clk),
> +               return dev_err_probe(dev, PTR_ERR(clk),
>                                      "Failed to get snvs clock (%pe)\n", clk);
>
>         pdata->wakeup = of_property_read_bool(np, "wakeup-source");
> @@ -162,7 +163,7 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
>                         bpt = (val / 5) - 1;
>                         break;
>                 default:
> -                       return dev_err_probe(&pdev->dev, -EINVAL,
> +                       return dev_err_probe(dev, -EINVAL,
>                                              "power-off-time-sec %d out of range\n", val);
>                 }
>
> @@ -180,9 +181,9 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
>
>         timer_setup(&pdata->check_timer, imx_imx_snvs_check_for_events, 0);
>
> -       input = devm_input_allocate_device(&pdev->dev);
> +       input = devm_input_allocate_device(dev);
>         if (!input) {
> -               dev_err(&pdev->dev, "failed to allocate the input device\n");
> +               dev_err(dev, "failed to allocate the input device\n");
>                 return -ENOMEM;

look like you missed this one changeing to dev_err_probe() at patch 1.

Frank

>         }
>
> @@ -193,27 +194,27 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
>         input_set_capability(input, EV_KEY, pdata->keycode);
>
>         /* input customer action to cancel release timer */
> -       error = devm_add_action(&pdev->dev, imx_snvs_pwrkey_act, pdata);
> +       error = devm_add_action(dev, imx_snvs_pwrkey_act, pdata);
>         if (error)
> -               return dev_err_probe(&pdev->dev, error, "failed to register remove action\n");
> +               return dev_err_probe(dev, error, "failed to register remove action\n");
>
>         pdata->input = input;
>         platform_set_drvdata(pdev, pdata);
>
> -       error = devm_request_irq(&pdev->dev, pdata->irq,
> -                              imx_snvs_pwrkey_interrupt,
> -                              0, pdev->name, pdev);
> +       error = devm_request_irq(dev, pdata->irq,
> +                                imx_snvs_pwrkey_interrupt,
> +                                0, pdev->name, pdev);
>         if (error)
> -               return dev_err_probe(&pdev->dev, error, "interrupt not available.\n");
> +               return dev_err_probe(dev, error, "interrupt not available.\n");
>
>         error = input_register_device(input);
>         if (error < 0)
> -               return dev_err_probe(&pdev->dev, error, "failed to register input device\n");
> +               return dev_err_probe(dev, error, "failed to register input device\n");
>
> -       device_init_wakeup(&pdev->dev, pdata->wakeup);
> -       error = dev_pm_set_wake_irq(&pdev->dev, pdata->irq);
> +       device_init_wakeup(dev, pdata->wakeup);
> +       error = dev_pm_set_wake_irq(dev, pdata->irq);
>         if (error)
> -               dev_err(&pdev->dev, "irq wake enable failed.\n");
> +               dev_err(dev, "irq wake enable failed.\n");
>
>         return 0;
>  }
>
> --
> 2.34.1
>
>

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

[joy.zou]

From: Joy Zou <joy.zou@nxp.com>

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 v3:
1. Add spinlock for pdata->keystate and pdata->suspended per AI review
   comments.
2. Replace hardcode value 1 with local variable keystate in input_report_key()
   under suspended.

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 | 60 ++++++++++++++++++++++++++++++++++--
 1 file changed, 57 insertions(+), 3 deletions(-)

diff --git a/drivers/input/keyboard/snvs_pwrkey.c b/drivers/input/keyboard/snvs_pwrkey.c
index 4a1d04898482..d6ea3c2d87c6 100644
--- a/drivers/input/keyboard/snvs_pwrkey.c
+++ b/drivers/input/keyboard/snvs_pwrkey.c
@@ -39,6 +39,8 @@ struct pwrkey_drv_data {
 	int keycode;
 	int keystate;  /* 1:pressed */
 	int wakeup;
+	bool suspended; /* Track suspend state */
+	spinlock_t lock; /* Protects keystate and suspended */
 	struct timer_list check_timer;
 	struct input_dev *input;
 	u8 minor_rev;
@@ -49,14 +51,21 @@ static void imx_imx_snvs_check_for_events(struct timer_list *t)
 	struct pwrkey_drv_data *pdata = timer_container_of(pdata, t,
 							   check_timer);
 	struct input_dev *input = pdata->input;
+	bool state_changed = false;
 	u32 state;
 
 	regmap_read(pdata->snvs, SNVS_HPSR_REG, &state);
 	state = state & SNVS_HPSR_BTN ? 1 : 0;
 
-	/* only report new event if status changed */
-	if (state ^ pdata->keystate) {
-		pdata->keystate = state;
+	scoped_guard(spinlock_irqsave, &pdata->lock) {
+		/* only report new event if status changed */
+		if (state ^ pdata->keystate) {
+			pdata->keystate = state;
+			state_changed = true;
+		}
+	}
+
+	if (state_changed) {
 		input_event(input, EV_KEY, pdata->keycode, state);
 		input_sync(input);
 		pm_relax(pdata->input->dev.parent);
@@ -74,6 +83,8 @@ static irqreturn_t imx_snvs_pwrkey_interrupt(int irq, void *dev_id)
 	struct platform_device *pdev = dev_id;
 	struct pwrkey_drv_data *pdata = platform_get_drvdata(pdev);
 	struct input_dev *input = pdata->input;
+	bool suspended;
+	int keystate;
 	u32 lp_status;
 
 	pm_wakeup_event(input->dev.parent, 0);
@@ -92,6 +103,21 @@ 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.
+			 */
+			scoped_guard(spinlock_irqsave, &pdata->lock) {
+				suspended = pdata->suspended;
+				if (suspended) {
+					pdata->keystate = 1;
+					keystate = pdata->keystate;
+				}
+			}
+			if (suspended) {
+				input_report_key(input, pdata->keycode, keystate);
+				input_sync(input);
+			}
 			mod_timer(&pdata->check_timer,
 			          jiffies + msecs_to_jiffies(DEBOUNCE_TIME));
 		}
@@ -151,6 +177,7 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
 	if (pdata->irq < 0)
 		return pdata->irq;
 
+	spin_lock_init(&pdata->lock);
 	error = of_property_read_u32(np, "power-off-time-sec", &val);
 	if (!error) {
 		switch (val) {
@@ -219,6 +246,32 @@ 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);
+
+	scoped_guard(spinlock_irqsave, &pdata->lock)
+		pdata->suspended = true;
+
+	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);
+
+	scoped_guard(spinlock_irqsave, &pdata->lock)
+		pdata->suspended = false;
+
+	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 +282,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.34.1