[PATCH 0/3] HID: i2c-hid: Probe and wake device with HID descriptor fetch

[PATCH 0/3] HID: i2c-hid: Probe and wake device with HID descriptor fetch

[Kenny Levinsen]

This is in response to
https://lore.kernel.org/all/Zh0qKeI-YPDE-NVT@hovoldconsulting.com/

Instead of extending the existing smbus probe to include a retry loop,
this patch takes the same approach as i2c_hid_set_power() by retrying on
EREMOTEIO.

This maintains the "silent" error in case no device is present without
having to send any dummy commands. Tested with a disconnected touchpad
on a Dell XPS 13.

I left out the particular sleep. If one is needed, it should also be
added to i2c_hid_set_power() which is where we'd wake the device after
resuming from suspend.

Lukasz and Radoslaw, can you please test if this still does the trick?

[PATCH 1/3] HID: i2c-hid: Rely on HID descriptor fetch to probe

[Kenny Levinsen]

To avoid error messages when a device is not present, b3a81b6c4fc6 added
an initial bus probe using a dummy i2c_smbus_read_byte() call.

Without this probe, i2c_hid_fetch_hid_descriptor() will fail with
EREMOTEIO. Propagate the error up so the caller can handle EREMOTEIO
gracefully, and remove the probe as it is no longer necessary.

Signed-off-by: Kenny Levinsen <kl@kl.wtf>
---
 drivers/hid/i2c-hid/i2c-hid-core.c | 20 ++++++--------------
 1 file changed, 6 insertions(+), 14 deletions(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
index 2df1ab3c31cc..515a80dbf6c7 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -894,12 +894,8 @@ static int i2c_hid_fetch_hid_descriptor(struct i2c_hid *ihid)
 					      ihid->wHIDDescRegister,
 					      &ihid->hdesc,
 					      sizeof(ihid->hdesc));
-		if (error) {
-			dev_err(&ihid->client->dev,
-				"failed to fetch HID descriptor: %d\n",
-				error);
-			return -ENODEV;
-		}
+		if (error)
+			return error;
 	}
 
 	/* Validate the length of HID descriptor, the 4 first bytes:
@@ -1014,17 +1010,13 @@ static int __i2c_hid_core_probe(struct i2c_hid *ihid)
 	struct hid_device *hid = ihid->hid;
 	int ret;
 
-	/* Make sure there is something at this address */
-	ret = i2c_smbus_read_byte(client);
-	if (ret < 0) {
+	ret = i2c_hid_fetch_hid_descriptor(ihid);
+	if (ret == -EREMOTEIO) {
 		i2c_hid_dbg(ihid, "nothing at this address: %d\n", ret);
 		return -ENXIO;
-	}
-
-	ret = i2c_hid_fetch_hid_descriptor(ihid);
-	if (ret < 0) {
+	} else if (ret < 0) {
 		dev_err(&client->dev,
-			"Failed to fetch the HID Descriptor\n");
+			"failed to fetch HID descriptor: %d\n", ret);
 		return ret;
 	}
 
-- 
2.44.0

[PATCH 2/3] HID: i2c-hid: Retry HID descriptor read to wake up STM devices

[Kenny Levinsen]

Some STM microcontrollers need some time after rising clock edge in
order to come out of their deep sleep state. This in turn means that the
first command send to them timeout and fail with EREMOTEIO.

Retry once on EREMOTEIO to see if the device came alive, otherwise treat
the error as if no device was present like before.

Link: https://lore.kernel.org/all/20240405102436.3479210-1-lma@chromium.org/#t
Signed-off-by: Kenny Levinsen <kl@kl.wtf>
---
 drivers/hid/i2c-hid/i2c-hid-core.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
index 515a80dbf6c7..ac661199d2c8 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -1010,7 +1010,15 @@ static int __i2c_hid_core_probe(struct i2c_hid *ihid)
 	struct hid_device *hid = ihid->hid;
 	int ret;
 
+	/*
+	 * Some STM-based devices need some time after a rising clock edge to
+	 * wake from deep sleep, which in turn means that our first command
+	 * will fail EREMOTEIO. Retry the command in this case.
+	 */
 	ret = i2c_hid_fetch_hid_descriptor(ihid);
+	if (ret == -EREMOTEIO)
+		ret = i2c_hid_fetch_hid_descriptor(ihid);
+
 	if (ret == -EREMOTEIO) {
 		i2c_hid_dbg(ihid, "nothing at this address: %d\n", ret);
 		return -ENXIO;
-- 
2.44.0

[PATCH 3/3] HID: i2c-hid: Align i2c_hid_set_power() retry with HID descriptor read

[Kenny Levinsen]

Instead of retrying on any error, just retry on EREMOTEIO and simplify
the retry handling a bit.

Signed-off-by: Kenny Levinsen <kl@kl.wtf>
---
 drivers/hid/i2c-hid/i2c-hid-core.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
index ac661199d2c8..998e7aa140d7 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -395,21 +395,14 @@ static int i2c_hid_set_power(struct i2c_hid *ihid, int power_state)
 	 * The call will get a return value (EREMOTEIO) but device will be
 	 * triggered and activated. After that, it goes like a normal device.
 	 */
-	if (power_state == I2C_HID_PWR_ON) {
+	ret = i2c_hid_set_power_command(ihid, power_state);
+	if (ret == -EREMOTEIO && power_state == I2C_HID_PWR_ON)
 		ret = i2c_hid_set_power_command(ihid, I2C_HID_PWR_ON);
 
-		/* Device was already activated */
-		if (!ret)
-			goto set_pwr_exit;
-	}
-
-	ret = i2c_hid_set_power_command(ihid, power_state);
 	if (ret)
 		dev_err(&ihid->client->dev,
 			"failed to change power setting.\n");
 
-set_pwr_exit:
-
 	/*
 	 * The HID over I2C specification states that if a DEVICE needs time
 	 * after the PWR_ON request, it should utilise CLOCK stretching.
-- 
2.44.0