[PATCH 0/2] HID: i2c-hid: Unify device wake-up logic

[PATCH 0/2] HID: i2c-hid: Unify device wake-up logic

[Kenny Levinsen]

Third time was not the charm[0]. After Dmitry's comment[1], and after
looking some more at the I2C drivers, I have given up on removing the
smbus probe for now. We can always revisit this later if the situation
improves enough, but there are more important things to fix.

Instead, go all in on the address probe with retry and use it for both
initial probe and resume, replacing the previous retry on power on
commands. This gives us consistency and a single place to update and
document.

[0]: https://lore.kernel.org/all/20240426225739.2166-1-kl@kl.wtf/
[1]: https://lore.kernel.org/all/ZixvUNooESC02cJK@google.com/

[PATCH 1/2] HID: i2c-hid: Retry address probe after delay

[Kenny Levinsen]

Some STM microcontrollers need 400µs after rising clock edge in order to
come out of their deep sleep state. This in turn means that our address
probe will fail as the device is not ready to service it.

Retry the probe once after a delay to see if the device came alive,
otherwise treat the device as missing.

Link: https://lore.kernel.org/all/20240405102436.3479210-1-lma@chromium.org/#t
Co-developed-by: Radoslaw Biernacki <rad@chromium.org>
Co-developed-by: Lukasz Majczak <lma@chromium.org>
Signed-off-by: Kenny Levinsen <kl@kl.wtf>
---
 drivers/hid/i2c-hid/i2c-hid-core.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
index d965382196c6..a40489bb7643 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -163,6 +163,24 @@ static u32 i2c_hid_lookup_quirk(const u16 idVendor, const u16 idProduct)
 	return quirks;
 }
 
+static int i2c_hid_probe_address(struct i2c_hid *ihid)
+{
+	int ret;
+
+	/*
+	 * Some STM-based devices need 400µs after a rising clock edge to wake
+	 * from deep sleep, in which case the first read will fail. Try after a
+	 * short sleep to see if the device came alive on the bus. Certain
+	 * Weida Tech devices also need this.
+	 */
+	ret = i2c_smbus_read_byte(ihid->client);
+	if (ret < 0) {
+		usleep_range(400, 500);
+		ret = i2c_smbus_read_byte(ihid->client);
+	}
+	return ret < 0 ? ret : 0;
+}
+
 static int i2c_hid_xfer(struct i2c_hid *ihid,
 			u8 *send_buf, int send_len, u8 *recv_buf, int recv_len)
 {
@@ -992,8 +1010,7 @@ 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);
+	ret = i2c_hid_probe_address(ihid);
 	if (ret < 0) {
 		i2c_hid_dbg(ihid, "nothing at this address: %d\n", ret);
 		return -ENXIO;
-- 
2.44.0

[PATCH 2/2] HID: i2c-hid: Use address probe to wake on resume

[Kenny Levinsen]

Certain devices, both from STM and Weida Tech, need to be woken up after
having entered a deeper sleep state. The relevant places to wake up such
device is during our initial HID probe, and after resuming.

A retry for power commands was previously added to i2c_hid_set_power to
wake up Weida Tech devices, but lacked sufficient sleep for STM devices.
Replace the power command retry with the same address probe we using
during our initial HID probe.

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

diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
index a40489bb7643..3e3885ae6ce2 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -402,19 +402,6 @@ static int i2c_hid_set_power(struct i2c_hid *ihid, int power_state)
 
 	i2c_hid_dbg(ihid, "%s\n", __func__);
 
-	/*
-	 * Some devices require to send a command to wakeup before power on.
-	 * 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, 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,
@@ -977,6 +964,14 @@ static int i2c_hid_core_resume(struct i2c_hid *ihid)
 
 	enable_irq(client->irq);
 
+	/* Make sure the device is awake on the bus */
+	ret = i2c_hid_probe_address(ihid);
+	if (ret < 0) {
+		dev_err(&client->dev, "nothing at address after resume: %d\n",
+			ret);
+		return -ENXIO;
+	}
+
 	/* Instead of resetting device, simply powers the device on. This
 	 * solves "incomplete reports" on Raydium devices 2386:3118 and
 	 * 2386:4B33 and fixes various SIS touchscreens no longer sending
-- 
2.44.0

Re: [PATCH 0/2] HID: i2c-hid: Unify device wake-up logic

[Jiri Kosina]

On Tue, 30 Apr 2024, Kenny Levinsen wrote:

> Third time was not the charm[0]. After Dmitry's comment[1], and after
> looking some more at the I2C drivers, I have given up on removing the
> smbus probe for now. We can always revisit this later if the situation
> improves enough, but there are more important things to fix.
> 
> Instead, go all in on the address probe with retry and use it for both
> initial probe and resume, replacing the previous retry on power on
> commands. This gives us consistency and a single place to update and
> document.
> 
> [0]: https://lore.kernel.org/all/20240426225739.2166-1-kl@kl.wtf/
> [1]: https://lore.kernel.org/all/ZixvUNooESC02cJK@google.com/

I really apologize for belated response to all this, I've been completely 
swamped for the past two weeks.

This is now queued in hid.git#for-6.10/i2c-hid.

Thanks,

-- 
Jiri Kosina
SUSE Labs