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

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

[Kenny Levinsen]

This revises my previous patch[0] to add the sleep STM chips seem to
require as per discussion on the original patch from Lukasz and
Radoslaw[1]. I had initially tried without as it had not previously been
needed in the similar logic in our resume path, but it would appear that
this was simply luck as the affected device was woken up in that case by
"noise" from other sources.

To reiterate, the idea is to add the retry that Lukasz and Radoslaw
discovered was necessary, but do away with the dummy smbus probe and
instead just let HID descriptor fetch retry as needed, aligning more
with the existing retry logic used after resume while saving some noise
on the bus and speeding up initialization a tiny bit.

I added Co-developed-by tags, I hope that's appropriate. We should await
an ACK from Lukasz on it fixing their hardware quirk.

[0]: https://lore.kernel.org/all/20240415170517.18780-1-kl@kl.wtf/
[1]: https://lore.kernel.org/all/CAE5UKNqPA4SnnXyaB7Hwk0kcKMMQ_DUuxogDphnnvSGP8g1nAQ@mail.gmail.com/

[PATCH v2 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 v2 2/3] HID: i2c-hid: Retry HID descriptor read to wake up STM devices

[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 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
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 | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
index 515a80dbf6c7..252ccb3b71d1 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -1010,7 +1010,17 @@ static int __i2c_hid_core_probe(struct i2c_hid *ihid)
 	struct hid_device *hid = ihid->hid;
 	int ret;
 
+	/*
+	 * Some STM-based devices need 400µs 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) {
+		usleep_range(400, 500);
+		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 v2 3/3] HID: i2c-hid: Align i2c_hid_set_power() retry with HID descriptor read

[Kenny Levinsen]

The retry for HID descriptor and for power commands deals with the same
device quirk, so align the two.

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

diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
index 252ccb3b71d1..749c0c036adb 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -391,25 +391,21 @@ 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.
+	 * Some STM-based devices need 400µs after a rising clock edge to wake
+	 * from deep sleep, which in turn means that our first command will
+	 * fail EREMOTEIO. Certain Weida Tech devices also need this wake-up.
+	 * Retry the command in this case.
 	 */
-	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) {
+		usleep_range(400, 500);
 		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

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

[Łukasz Majczak]

On Tue, Apr 23, 2024 at 2:26 PM Kenny Levinsen <kl@kl.wtf> wrote:
>
> This revises my previous patch[0] to add the sleep STM chips seem to
> require as per discussion on the original patch from Lukasz and
> Radoslaw[1]. I had initially tried without as it had not previously been
> needed in the similar logic in our resume path, but it would appear that
> this was simply luck as the affected device was woken up in that case by
> "noise" from other sources.
>
> To reiterate, the idea is to add the retry that Lukasz and Radoslaw
> discovered was necessary, but do away with the dummy smbus probe and
> instead just let HID descriptor fetch retry as needed, aligning more
> with the existing retry logic used after resume while saving some noise
> on the bus and speeding up initialization a tiny bit.
>
> I added Co-developed-by tags, I hope that's appropriate. We should await
> an ACK from Lukasz on it fixing their hardware quirk.
>
> [0]: https://lore.kernel.org/all/20240415170517.18780-1-kl@kl.wtf/
> [1]: https://lore.kernel.org/all/CAE5UKNqPA4SnnXyaB7Hwk0kcKMMQ_DUuxogDphnnvSGP8g1nAQ@mail.gmail.com/
>
Hi Kenny,

Your solution works as it should - I have tested it on my Eve with
enabled debugs and
the retries works as expected with power-on, reboot and suspend/resume paths.

I have also disabled cros_ec_i2c driver to be 100% sure it doesn't do
any i2c transactions on the bus
and again the touchpad initialized successfully (with a retry) on all paths.

So you can add:
Tested-by: Lukasz Majczak <lma@chromium.org>
Reviewed-by: Lukasz Majczak <lma@chromium.org>

Thank you Kenny for your work :)

Best regards,
Lukasz

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

[Doug Anderson]

Hi,

On Tue, Apr 23, 2024 at 5:26 AM Kenny Levinsen <kl@kl.wtf> wrote:
>
> 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) {

I worry a little bit about keying just off of -EREMOTEIO. If I'm
skimming the code properly it's up to the different i2c bus controller
to decide which error code to return here. Looking at, for instance,
"i2c-qcom-geni.c", I see:

[NACK] = {-ENXIO, "NACK: slv unresponsive, check its power/reset-ln"},

Maybe we should just use dev_dbg() in all cases here when we fail to
fetch the descriptor? ...otherwise I think some boards will start
getting a noisy error message.

...and confirmed that my skim seemed to be accurate. i put your
patches on my system and then changed the system to think my
hid-over-i2c device was at 0x11 instead of the normal 0x10. Now, I
get:

[    5.973417] i2c_hid_of 4-0011: failed to fetch HID descriptor: -6
[    5.979701] i2c_hid_of 4-0011: Power on failed: -6


-Doug

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

[Kenny Levinsen]

On 4/24/24 7:00 PM, Doug Anderson wrote:
> I worry a little bit about keying just off of -EREMOTEIO. If I'm
> skimming the code properly it's up to the different i2c bus controller
> to decide which error code to return here. Looking at, for instance,
> "i2c-qcom-geni.c", I see:
>
> [NACK] = {-ENXIO, "NACK: slv unresponsive, check its power/reset-ln"},


Hmm, good point. I decided to go through the drivers and check their 
behavior on NACK, and based on my quick glance I found (insert accuracy 
disclaimer):

- 52 drivers emitting ENXIO
- 14 drivers emitting EREMOTEIO
- 11 driver emitting EIO
- 5 drivers emitting ETIMEDOUT
- 1 driver emitting EAGAIN
- 1 driver emitting I2C_ERR_BERR (???)

So just EREMOTEIO is definitely not good enough. Looking at the drivers, 
it seems like the majority of drivers emitting generic error codes could 
just as well emit ENXIO on NACK. Room for improvement.

> Maybe we should just use dev_dbg() in all cases here when we fail to
> fetch the descriptor? ...otherwise I think some boards will start
> getting a noisy error message.

I'm okay with that. I don't like hiding a useful error message, but the 
smbus probe would also have hidden bus errors.

I'll send a v3 with just dev_dbg, then if I (or someone else) end up 
aligning more i2c drivers on their NACK error we can go to the stricter 
check and incentivize the drivers to give meaningful error values...