[RFC PATCH] Input: elan_i2c_smbus - resend ENABLE_TP on wake from sleep (NOT TESTED, NEEDS TESTER!)

[RFC PATCH] Input: elan_i2c_smbus - resend ENABLE_TP on wake from sleep (NOT TESTED, NEEDS TESTER!)

[R. Larocque]

elan_smbus_sleep_control() sends ETP_SMBUS_SLEEP_CMD to put the
controller to sleep, but does nothing on wake, leaving a long-standing
XXX comment questionning wether ETP_SMBUS_ENABLE_TP should be re sent.

elan_initialize() already treats sleep_control(client, false) as a
genuine "wake device up" primitive (see the ASUS special-firmware
branch and its accompanying dev_err() message), so this makes the
non-ASUS/default path resend ETP_SMBUS_ENABLE_TP symetrically with
elan_smbus_initialize() instead of being a no-op.

Signed-off-by: Raphaël Larocque <rlarocque@disroot.org>
---
RFC: I dont have the Elan SMBus protocol datasheet and consequently 
havent been able to test this on any real hardware where the previous no-op
path was actually relied upon. Sending this for testing by
someone with access to affected devices before it is considered for
merge. A bad SMBus write to a sleeping controller could leave it
in a weird state on the wrong firmware revision if unlucky.

 drivers/input/mouse/elan_i2c_smbus.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/input/mouse/elan_i2c_smbus.c b/drivers/input/mouse/elan_i2c_smbus.c
index 0287441cda46..95f5951e9549 100644
--- a/drivers/input/mouse/elan_i2c_smbus.c
+++ b/drivers/input/mouse/elan_i2c_smbus.c
@@ -93,10 +93,25 @@ static int elan_smbus_set_mode(struct i2c_client *client, u8 mode)
 
 static int elan_smbus_sleep_control(struct i2c_client *client, bool sleep)
 {
+	int error;
+
 	if (sleep)
 		return i2c_smbus_write_byte(client, ETP_SMBUS_SLEEP_CMD);
-	else
-		return 0; /* XXX should we send ETP_SMBUS_ENABLE_TP here? */
+
+	/*
+	 * The controller stops touch reporting while asleep; resend
+	 * ETP_SMBUS_ENABLE_TP on wake, mirroring what is done during
+	 * elan_smbus_initialize(), so that reporting actually resumes.
+	 */
+	error = i2c_smbus_write_byte(client, ETP_SMBUS_ENABLE_TP);
+	if (error) {
+		dev_err(&client->dev,
+			"failed to re-enable touchpad after wake: %d\n",
+			error);
+		return error;
+	}
+
+	return 0;
 }
 
 static int elan_smbus_power_control(struct i2c_client *client, bool enable)
-- 
2.55.0

Re: [RFC PATCH] Input: elan_i2c_smbus - resend ENABLE_TP on wake from sleep (NOT TESTED, NEEDS TESTER!)

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] input: redundant ETP_SMBUS_ENABLE_TP command during initialization
- [Low] input: explicit failure path violation in elan_smbus_sleep_control()
--

commit a7e8e038348366c26f0b3d1a3a7f9e778cbce51a
Author: R. Larocque <rlarocque@disroot.org>

Input: elan_i2c_smbus - resend ENABLE_TP on wake from sleep (NOT TESTED, NEEDS TESTER!)

This commit updates the Elan SMBus sleep control to resend the enable command
when waking up the touchpad. This mirrors the initialization path to ensure
touch reporting resumes.

> diff --git a/drivers/input/mouse/elan_i2c_smbus.c b/drivers/input/mouse/elan_i2c_smbus.c
> index 0287441cda46d..dd86bd1739c55 100644
> --- a/drivers/input/mouse/elan_i2c_smbus.c
> +++ b/drivers/input/mouse/elan_i2c_smbus.c
> @@ -93,10 +93,25 @@ static int elan_smbus_set_mode(struct i2c_client *client, u8 mode)
>  
>  static int elan_smbus_sleep_control(struct i2c_client *client, bool sleep)
>  {
> +	int error;
> +
>  	if (sleep)
>  		return i2c_smbus_write_byte(client, ETP_SMBUS_SLEEP_CMD);

[Severity: Low]
Does this direct return bypass the preferred explicit failure path style?

The input subsystem maintainer style guidelines suggest expanding direct
returns into explicit error checks when a function has multiple failure
points. Should this be updated to assign the return value to error and return
explicitly?

> -	else
> -		return 0; /* XXX should we send ETP_SMBUS_ENABLE_TP here? */
> +
> +	/*
> +	 * The controller stops touch reporting while asleep; resend
> +	 * ETP_SMBUS_ENABLE_TP on wake, mirroring what is done during
> +	 * elan_smbus_initialize(), so that reporting actually resumes.
> +	 */
> +	error = i2c_smbus_write_byte(client, ETP_SMBUS_ENABLE_TP);

[Severity: Medium]
Will this cause a redundant enable command during initialization that
might confuse the hardware?

During device probe, the sequence calls the initialization routine and then
the sleep control routine to wake it up:

__elan_initialize()
   data->ops->initialize(client)
      -> elan_smbus_initialize() sends ETP_SMBUS_ENABLE_TP
   data->ops->sleep_control(client, false)
      -> elan_smbus_sleep_control() now sends ETP_SMBUS_ENABLE_TP again

Since this patch is explicitly untested on real hardware, is there a risk
that this double-command could cause unexpected states or fail the probe
sequence?

> +	if (error) {
> +		dev_err(&client->dev,
> +			"failed to re-enable touchpad after wake: %d\n",
> +			error);
> +		return error;
> +	}
> +
> +	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260922232707.8949-1-rlarocque@disroot.org?part=1