From a2d7cbd6781954902803cc94ff0f8076f8df1420 Mon Sep 17 00:00:00 2001 From: Hans de Goede Date: Sat, 2 Dec 2023 23:46:08 +0100 Subject: [PATCH 01/12] HID: i2c-hid: Fold i2c_hid_execute_reset() into i2c_hid_hwreset() mainline inclusion from mainline-v6.8-rc1 category: bugfix i2c_hid_hwreset() is the only caller of i2c_hid_execute_reset(), fold the latter into the former. This is a preparation patch for removing the need for I2C_HID_QUIRK_NO_IRQ_AFTER_RESET by making i2c-hid behave more like Windows. No functional changes intended. Reviewed-by: Douglas Anderson Signed-off-by: Hans de Goede Signed-off-by: Jiri Kosina (cherry picked from commit f023605d1de6f150d9266747a2630f029956041f) --- drivers/hid/i2c-hid/i2c-hid-core.c | 73 ++++++++++++------------------ 1 file changed, 30 insertions(+), 43 deletions(-) diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c index 045db6f0fb4c4..fd371d6757dfd 100644 --- a/drivers/hid/i2c-hid/i2c-hid-core.c +++ b/drivers/hid/i2c-hid/i2c-hid-core.c @@ -419,12 +419,23 @@ static int i2c_hid_set_power(struct i2c_hid *ihid, int power_state) return ret; } -static int i2c_hid_execute_reset(struct i2c_hid *ihid) +static int i2c_hid_hwreset(struct i2c_hid *ihid) { size_t length = 0; int ret; - i2c_hid_dbg(ihid, "resetting...\n"); + i2c_hid_dbg(ihid, "%s\n", __func__); + + /* + * This prevents sending feature reports while the device is + * being reset. Otherwise we may lose the reset complete + * interrupt. + */ + mutex_lock(&ihid->reset_lock); + + ret = i2c_hid_set_power(ihid, I2C_HID_PWR_ON); + if (ret) + goto err_unlock; /* Prepare reset command. Command register goes first. */ *(__le16 *)ihid->cmdbuf = ihid->hdesc.wCommandRegister; @@ -437,59 +448,35 @@ static int i2c_hid_execute_reset(struct i2c_hid *ihid) ret = i2c_hid_xfer(ihid, ihid->cmdbuf, length, NULL, 0); if (ret) { - dev_err(&ihid->client->dev, "failed to reset device.\n"); - goto out; + dev_err(&ihid->client->dev, + "failed to reset device: %d\n", ret); + goto err_clear_reset; } + i2c_hid_dbg(ihid, "%s: waiting...\n", __func__); + if (ihid->quirks & I2C_HID_QUIRK_NO_IRQ_AFTER_RESET) { msleep(100); - goto out; - } - - i2c_hid_dbg(ihid, "%s: waiting...\n", __func__); - if (!wait_event_timeout(ihid->wait, - !test_bit(I2C_HID_RESET_PENDING, &ihid->flags), - msecs_to_jiffies(5000))) { + clear_bit(I2C_HID_RESET_PENDING, &ihid->flags); + } else if (!wait_event_timeout(ihid->wait, + !test_bit(I2C_HID_RESET_PENDING, &ihid->flags), + msecs_to_jiffies(5000))) { ret = -ENODATA; - goto out; + goto err_clear_reset; } i2c_hid_dbg(ihid, "%s: finished.\n", __func__); -out: - clear_bit(I2C_HID_RESET_PENDING, &ihid->flags); - return ret; -} - -static int i2c_hid_hwreset(struct i2c_hid *ihid) -{ - int ret; - - i2c_hid_dbg(ihid, "%s\n", __func__); - - /* - * This prevents sending feature reports while the device is - * being reset. Otherwise we may lose the reset complete - * interrupt. - */ - mutex_lock(&ihid->reset_lock); - - ret = i2c_hid_set_power(ihid, I2C_HID_PWR_ON); - if (ret) - goto out_unlock; - - ret = i2c_hid_execute_reset(ihid); - if (ret) { - dev_err(&ihid->client->dev, - "failed to reset device: %d\n", ret); - i2c_hid_set_power(ihid, I2C_HID_PWR_SLEEP); - goto out_unlock; - } - /* At least some SIS devices need this after reset */ if (!(ihid->quirks & I2C_HID_QUIRK_NO_WAKEUP_AFTER_RESET)) ret = i2c_hid_set_power(ihid, I2C_HID_PWR_ON); -out_unlock: + mutex_unlock(&ihid->reset_lock); + return ret; + +err_clear_reset: + clear_bit(I2C_HID_RESET_PENDING, &ihid->flags); + i2c_hid_set_power(ihid, I2C_HID_PWR_SLEEP); +err_unlock: mutex_unlock(&ihid->reset_lock); return ret; } From f764704f35d6ca464e1cae2cc40b2cad2f0c9eb3 Mon Sep 17 00:00:00 2001 From: Hans de Goede Date: Sat, 2 Dec 2023 23:46:09 +0100 Subject: [PATCH 02/12] HID: i2c-hid: Split i2c_hid_hwreset() in start() and finish() functions mainline inclusion from mainline-v6.8-rc1 category: bugfix Split i2c_hid_hwreset() into: i2c_hid_start_hwreset() which sends the PWR_ON and reset commands; and i2c_hid_finish_hwreset() which actually waits for the reset to complete. This is a preparation patch for removing the need for I2C_HID_QUIRK_NO_IRQ_AFTER_RESET by making i2c-hid behave more like Windows. No functional changes intended. Reviewed-by: Douglas Anderson Signed-off-by: Hans de Goede Signed-off-by: Jiri Kosina (cherry picked from commit 96d3098db835d58649b73a5788898bd7672a319b) --- drivers/hid/i2c-hid/i2c-hid-core.c | 38 ++++++++++++++++++++++-------- 1 file changed, 28 insertions(+), 10 deletions(-) diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c index fd371d6757dfd..1c52fd43459ce 100644 --- a/drivers/hid/i2c-hid/i2c-hid-core.c +++ b/drivers/hid/i2c-hid/i2c-hid-core.c @@ -419,7 +419,7 @@ static int i2c_hid_set_power(struct i2c_hid *ihid, int power_state) return ret; } -static int i2c_hid_hwreset(struct i2c_hid *ihid) +static int i2c_hid_start_hwreset(struct i2c_hid *ihid) { size_t length = 0; int ret; @@ -431,11 +431,11 @@ static int i2c_hid_hwreset(struct i2c_hid *ihid) * being reset. Otherwise we may lose the reset complete * interrupt. */ - mutex_lock(&ihid->reset_lock); + lockdep_assert_held(&ihid->reset_lock); ret = i2c_hid_set_power(ihid, I2C_HID_PWR_ON); if (ret) - goto err_unlock; + return ret; /* Prepare reset command. Command register goes first. */ *(__le16 *)ihid->cmdbuf = ihid->hdesc.wCommandRegister; @@ -453,6 +453,18 @@ static int i2c_hid_hwreset(struct i2c_hid *ihid) goto err_clear_reset; } + return 0; + +err_clear_reset: + clear_bit(I2C_HID_RESET_PENDING, &ihid->flags); + i2c_hid_set_power(ihid, I2C_HID_PWR_SLEEP); + return ret; +} + +static int i2c_hid_finish_hwreset(struct i2c_hid *ihid) +{ + int ret = 0; + i2c_hid_dbg(ihid, "%s: waiting...\n", __func__); if (ihid->quirks & I2C_HID_QUIRK_NO_IRQ_AFTER_RESET) { @@ -470,14 +482,11 @@ static int i2c_hid_hwreset(struct i2c_hid *ihid) if (!(ihid->quirks & I2C_HID_QUIRK_NO_WAKEUP_AFTER_RESET)) ret = i2c_hid_set_power(ihid, I2C_HID_PWR_ON); - mutex_unlock(&ihid->reset_lock); return ret; err_clear_reset: clear_bit(I2C_HID_RESET_PENDING, &ihid->flags); i2c_hid_set_power(ihid, I2C_HID_PWR_SLEEP); -err_unlock: - mutex_unlock(&ihid->reset_lock); return ret; } @@ -721,7 +730,11 @@ static int i2c_hid_parse(struct hid_device *hid) } do { - ret = i2c_hid_hwreset(ihid); + mutex_lock(&ihid->reset_lock); + ret = i2c_hid_start_hwreset(ihid); + if (ret == 0) + ret = i2c_hid_finish_hwreset(ihid); + mutex_unlock(&ihid->reset_lock); if (ret) msleep(1000); } while (tries-- > 0 && ret); @@ -965,10 +978,15 @@ static int i2c_hid_core_resume(struct i2c_hid *ihid) * However some ALPS touchpads generate IRQ storm without reset, so * let's still reset them here. */ - if (ihid->quirks & I2C_HID_QUIRK_RESET_ON_RESUME) - ret = i2c_hid_hwreset(ihid); - else + if (ihid->quirks & I2C_HID_QUIRK_RESET_ON_RESUME) { + mutex_lock(&ihid->reset_lock); + ret = i2c_hid_start_hwreset(ihid); + if (ret == 0) + ret = i2c_hid_finish_hwreset(ihid); + mutex_unlock(&ihid->reset_lock); + } else { ret = i2c_hid_set_power(ihid, I2C_HID_PWR_ON); + } if (ret) return ret; From d88fd35eba10192719248d7a08ed96e6fbf98925 Mon Sep 17 00:00:00 2001 From: Hans de Goede Date: Sat, 2 Dec 2023 23:46:10 +0100 Subject: [PATCH 03/12] HID: i2c-hid: Switch i2c_hid_parse() to goto style error handling mainline inclusion from mainline-v6.8-rc1 category: bugfix Switch i2c_hid_parse() to goto style error handling. This is a preparation patch for removing the need for I2C_HID_QUIRK_NO_IRQ_AFTER_RESET by making i2c-hid behave more like Windows. Note this changes the descriptor read error path to propagate the actual i2c_hid_read_register() error code (which is always negative) instead of hardcoding a -EIO return. Reviewed-by: Douglas Anderson Signed-off-by: Hans de Goede Signed-off-by: Jiri Kosina (cherry picked from commit aa69d6974185e9f7a552ba982540a38e34f69690) --- drivers/hid/i2c-hid/i2c-hid-core.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c index 1c52fd43459ce..7a11b033fe441 100644 --- a/drivers/hid/i2c-hid/i2c-hid-core.c +++ b/drivers/hid/i2c-hid/i2c-hid-core.c @@ -763,23 +763,21 @@ static int i2c_hid_parse(struct hid_device *hid) rdesc, rsize); if (ret) { hid_err(hid, "reading report descriptor failed\n"); - kfree(rdesc); - return -EIO; + goto out; } } i2c_hid_dbg(ihid, "Report Descriptor: %*ph\n", rsize, rdesc); ret = hid_parse_report(hid, rdesc, rsize); + if (ret) + dbg_hid("parsing report descriptor failed\n"); + +out: if (!use_override) kfree(rdesc); - if (ret) { - dbg_hid("parsing report descriptor failed\n"); - return ret; - } - - return 0; + return ret; } static int i2c_hid_start(struct hid_device *hid) From cf9930d340701477064f2ef8bcdd9e5643030f0c Mon Sep 17 00:00:00 2001 From: Hans de Goede Date: Sat, 2 Dec 2023 23:46:11 +0100 Subject: [PATCH 04/12] HID: i2c-hid: Move i2c_hid_finish_hwreset() to after reading the report-descriptor MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit mainline inclusion from mainline-v6.8-rc1 category: bugfix A recent bug made me look at Microsoft's i2c-hid docs again and I noticed the following: """ 4. Issue a RESET (Host Initiated Reset) to the Device. 5. Retrieve report descriptor from the device. Note: Steps 4 and 5 may be done in parallel to optimize for time on I²C. Since report descriptors are (a) static and (b) quite long, Windows 8 may issue a request for 5 while it is waiting for a response from the device on 4. """ Which made me think that maybe on some touchpads the reset ack is delayed till after the report descriptor is read ? Testing a T-BAO Tbook Air 12.5 with a 0911:5288 (SIPODEV SP1064?) touchpad, for which the I2C_HID_QUIRK_NO_IRQ_AFTER_RESET quirk was first introduced, shows that reading the report descriptor before waiting for the reset helps with the missing reset IRQ. Now the reset does get acked properly, but the ack sometimes still does not happen unfortunately. Still moving the wait for ack to after reading the report-descriptor, is probably a good idea, both to make i2c-hid's behavior closer to Windows as well as to speed up probing i2c-hid devices. While at it drop the dbg_hid() for a malloc failure, malloc failures already get logged extensively by malloc itself. Link: https://bugzilla.redhat.com/show_bug.cgi?id=2247751 Signed-off-by: Hans de Goede Reviewed-by: Douglas Anderson Signed-off-by: Jiri Kosina (cherry picked from commit af93a167eda90192563bce64c4bb989836afac1f) --- drivers/hid/i2c-hid/i2c-hid-core.c | 29 +++++++++++++++++++---------- 1 file changed, 19 insertions(+), 10 deletions(-) diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c index 7a11b033fe441..c9c547b8d3bfc 100644 --- a/drivers/hid/i2c-hid/i2c-hid-core.c +++ b/drivers/hid/i2c-hid/i2c-hid-core.c @@ -715,11 +715,10 @@ static int i2c_hid_parse(struct hid_device *hid) struct i2c_client *client = hid->driver_data; struct i2c_hid *ihid = i2c_get_clientdata(client); struct i2c_hid_desc *hdesc = &ihid->hdesc; + char *rdesc = NULL, *use_override = NULL; unsigned int rsize; - char *rdesc; int ret; int tries = 3; - char *use_override; i2c_hid_dbg(ihid, "entering %s\n", __func__); @@ -729,18 +728,15 @@ static int i2c_hid_parse(struct hid_device *hid) return -EINVAL; } + mutex_lock(&ihid->reset_lock); do { - mutex_lock(&ihid->reset_lock); ret = i2c_hid_start_hwreset(ihid); - if (ret == 0) - ret = i2c_hid_finish_hwreset(ihid); - mutex_unlock(&ihid->reset_lock); if (ret) msleep(1000); } while (tries-- > 0 && ret); if (ret) - return ret; + goto abort_reset; use_override = i2c_hid_get_dmi_hid_report_desc_override(client->name, &rsize); @@ -752,8 +748,8 @@ static int i2c_hid_parse(struct hid_device *hid) rdesc = kzalloc(rsize, GFP_KERNEL); if (!rdesc) { - dbg_hid("couldn't allocate rdesc memory\n"); - return -ENOMEM; + ret = -ENOMEM; + goto abort_reset; } i2c_hid_dbg(ihid, "asking HID report descriptor\n"); @@ -763,10 +759,23 @@ static int i2c_hid_parse(struct hid_device *hid) rdesc, rsize); if (ret) { hid_err(hid, "reading report descriptor failed\n"); - goto out; + goto abort_reset; } } + /* + * Windows directly reads the report-descriptor after sending reset + * and then waits for resets completion afterwards. Some touchpads + * actually wait for the report-descriptor to be read before signalling + * reset completion. + */ + ret = i2c_hid_finish_hwreset(ihid); +abort_reset: + clear_bit(I2C_HID_RESET_PENDING, &ihid->flags); + mutex_unlock(&ihid->reset_lock); + if (ret) + goto out; + i2c_hid_dbg(ihid, "Report Descriptor: %*ph\n", rsize, rdesc); ret = hid_parse_report(hid, rdesc, rsize); From 7af378e14b4db01c94635e58d5a0ab7facc791ad Mon Sep 17 00:00:00 2001 From: Hans de Goede Date: Sat, 2 Dec 2023 23:46:12 +0100 Subject: [PATCH 05/12] HID: i2c-hid: Turn missing reset ack into a warning mainline inclusion from mainline-v6.8-rc1 category: bugfix On all i2c-hid devices seen sofar the reset-ack either works, or the hw is somehow buggy and does not (always) ack the reset properly, yet it still works fine. Lower the very long reset timeout to 1 second which should be plenty and change the reset not getting acked from an error into a warning. This results in a bit cleaner code and avoids the need to add more I2C_HID_QUIRK_NO_IRQ_AFTER_RESET quirks in the future. Reviewed-by: Douglas Anderson Signed-off-by: Hans de Goede Signed-off-by: Jiri Kosina (cherry picked from commit 7bcf9ebb50f2afc84b33b06edfd20d47218af758) --- drivers/hid/i2c-hid/i2c-hid-core.c | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c index c9c547b8d3bfc..1c86c97688e93 100644 --- a/drivers/hid/i2c-hid/i2c-hid-core.c +++ b/drivers/hid/i2c-hid/i2c-hid-core.c @@ -472,9 +472,9 @@ static int i2c_hid_finish_hwreset(struct i2c_hid *ihid) clear_bit(I2C_HID_RESET_PENDING, &ihid->flags); } else if (!wait_event_timeout(ihid->wait, !test_bit(I2C_HID_RESET_PENDING, &ihid->flags), - msecs_to_jiffies(5000))) { - ret = -ENODATA; - goto err_clear_reset; + msecs_to_jiffies(1000))) { + dev_warn(&ihid->client->dev, "device did not ack reset within 1000 ms\n"); + clear_bit(I2C_HID_RESET_PENDING, &ihid->flags); } i2c_hid_dbg(ihid, "%s: finished.\n", __func__); @@ -483,11 +483,6 @@ static int i2c_hid_finish_hwreset(struct i2c_hid *ihid) ret = i2c_hid_set_power(ihid, I2C_HID_PWR_ON); return ret; - -err_clear_reset: - clear_bit(I2C_HID_RESET_PENDING, &ihid->flags); - i2c_hid_set_power(ihid, I2C_HID_PWR_SLEEP); - return ret; } static void i2c_hid_get_input(struct i2c_hid *ihid) From 4247c616686cc957d7ba0a64fa6c87a4f0ee0d70 Mon Sep 17 00:00:00 2001 From: Kenny Levinsen Date: Tue, 2 Apr 2024 13:10:04 +0200 Subject: [PATCH 06/12] HID: i2c-hid: Revert to await reset ACK before reading report descriptor mainline inclusion from mainline-v6.9-rc6 category: bugfix In af93a167eda9, i2c_hid_parse was changed to continue with reading the report descriptor before waiting for reset to be acknowledged. This has lead to two regressions: 1. We fail to handle reset acknowledgment if it happens while reading the report descriptor. The transfer sets I2C_HID_READ_PENDING, which causes the IRQ handler to return without doing anything. This affects both a Wacom touchscreen and a Sensel touchpad. 2. On a Sensel touchpad, reading the report descriptor this quickly after reset results in all zeroes or partial zeroes. The issues were observed on the Lenovo Thinkpad Z16 Gen 2. The change in question was made based on a Microsoft article[0] stating that Windows 8 *may* read the report descriptor in parallel with awaiting reset acknowledgment, intended as a slight reset performance optimization. Perhaps they only do this if reset is not completing quickly enough for their tastes? As the code is not currently ready to read registers in parallel with a pending reset acknowledgment, and as reading quickly breaks the report descriptor on the Sensel touchpad, revert to waiting for reset acknowledgment before proceeding to read the report descriptor. [0]: https://learn.microsoft.com/en-us/windows-hardware/drivers/hid/plug-and-play-support-and-power-management Fixes: af93a167eda9 ("HID: i2c-hid: Move i2c_hid_finish_hwreset() to after reading the report-descriptor") Closes: https://bugzilla.redhat.com/show_bug.cgi?id=2271136 Cc: stable@vger.kernel.org Signed-off-by: Kenny Levinsen Link: https://lore.kernel.org/r/20240331182440.14477-1-kl@kl.wtf [hdegoede@redhat.com Drop no longer necessary abort_reset error exit path] Signed-off-by: Hans de Goede Tested-by: Mark Pearson Signed-off-by: Jiri Kosina (cherry picked from commit ea36bf1827462e4a52365bf8e3f7d1712c5d9600) --- drivers/hid/i2c-hid/i2c-hid-core.c | 29 ++++++++--------------------- 1 file changed, 8 insertions(+), 21 deletions(-) diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c index 1c86c97688e93..d965382196c69 100644 --- a/drivers/hid/i2c-hid/i2c-hid-core.c +++ b/drivers/hid/i2c-hid/i2c-hid-core.c @@ -726,12 +726,15 @@ static int i2c_hid_parse(struct hid_device *hid) mutex_lock(&ihid->reset_lock); do { ret = i2c_hid_start_hwreset(ihid); - if (ret) + if (ret == 0) + ret = i2c_hid_finish_hwreset(ihid); + else msleep(1000); } while (tries-- > 0 && ret); + mutex_unlock(&ihid->reset_lock); if (ret) - goto abort_reset; + return ret; use_override = i2c_hid_get_dmi_hid_report_desc_override(client->name, &rsize); @@ -741,11 +744,8 @@ static int i2c_hid_parse(struct hid_device *hid) i2c_hid_dbg(ihid, "Using a HID report descriptor override\n"); } else { rdesc = kzalloc(rsize, GFP_KERNEL); - - if (!rdesc) { - ret = -ENOMEM; - goto abort_reset; - } + if (!rdesc) + return -ENOMEM; i2c_hid_dbg(ihid, "asking HID report descriptor\n"); @@ -754,23 +754,10 @@ static int i2c_hid_parse(struct hid_device *hid) rdesc, rsize); if (ret) { hid_err(hid, "reading report descriptor failed\n"); - goto abort_reset; + goto out; } } - /* - * Windows directly reads the report-descriptor after sending reset - * and then waits for resets completion afterwards. Some touchpads - * actually wait for the report-descriptor to be read before signalling - * reset completion. - */ - ret = i2c_hid_finish_hwreset(ihid); -abort_reset: - clear_bit(I2C_HID_RESET_PENDING, &ihid->flags); - mutex_unlock(&ihid->reset_lock); - if (ret) - goto out; - i2c_hid_dbg(ihid, "Report Descriptor: %*ph\n", rsize, rdesc); ret = hid_parse_report(hid, rdesc, rsize); From 6628a5a5ab435371db29af2a65bfda8f439db0ab Mon Sep 17 00:00:00 2001 From: Kenny Levinsen Date: Tue, 30 Apr 2024 01:33:14 +0200 Subject: [PATCH 07/12] HID: i2c-hid: Retry address probe after delay MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit mainline inclusion from mainline-v6.10-rc1 category: bugfix 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 Co-developed-by: Lukasz Majczak Signed-off-by: Kenny Levinsen Signed-off-by: Jiri Kosina (cherry picked from commit ab5ec06a7070840bb64a125fe6e5b0ddcb36346c) --- 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 d965382196c69..a40489bb76432 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; From 15d06b6452fe24babe390f263db2a85ce5c37d05 Mon Sep 17 00:00:00 2001 From: Kenny Levinsen Date: Tue, 30 Apr 2024 01:33:15 +0200 Subject: [PATCH 08/12] HID: i2c-hid: Use address probe to wake on resume mainline inlcusion from mainline-v6.10-rc1 category: bugfix 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 Signed-off-by: Jiri Kosina (cherry picked from commit 7d6f065de37c31c37e56611efd41260c66c868ca) --- 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 a40489bb76432..3e3885ae6ce2d 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 From 81b5ffd4d6fd12530232697b5924a3d4460ac898 Mon Sep 17 00:00:00 2001 From: Kenny Levinsen Date: Tue, 7 May 2024 08:36:56 +0200 Subject: [PATCH 09/12] HID: i2c-hid: Remove unused label in i2c_hid_set_power mainline inclusion from mainline-v6.10-rc1 category: bugfix This label was left behind when the wake-up logic was moved from i2c_hid_set_power to i2c_hid_probe_address. Clean it up as it causes warnings-as-errors builds to fail. Fixes: bb1033c8a3ea ("HID: i2c-hid: Use address probe to wake on resume") Reported-by: Stephen Rothwell Signed-off-by: Kenny Levinsen Signed-off-by: Jiri Kosina (cherry picked from commit d2b34fa81445193532e7012106f60426de3b3718) --- drivers/hid/i2c-hid/i2c-hid-core.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c index 3e3885ae6ce2d..632eaf9e11a6b 100644 --- a/drivers/hid/i2c-hid/i2c-hid-core.c +++ b/drivers/hid/i2c-hid/i2c-hid-core.c @@ -407,8 +407,6 @@ static int i2c_hid_set_power(struct i2c_hid *ihid, int power_state) 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. From 705afe24eba0e0d129c005d90bb73d8b33eb1733 Mon Sep 17 00:00:00 2001 From: Dmitry Torokhov Date: Mon, 9 Sep 2024 13:37:40 -0700 Subject: [PATCH 10/12] HID: i2c-hid: ensure various commands do not interfere with each other mainline inclusion from mainline-v6.12-rc1 category: bugfix i2c-hid uses 2 shared buffers: command and "raw" input buffer for sending requests to peripherals and read data from peripherals when executing variety of commands. Such commands include reading of HID registers, requesting particular power mode, getting and setting reports and so on. Because all such requests use the same 2 buffers they should not execute simultaneously. Fix this by introducing "cmd_lock" mutex and acquire it whenever we needs to access ihid->cmdbuf or idid->rawbuf. Signed-off-by: Dmitry Torokhov Reviewed-by: Hans de Goede Signed-off-by: Jiri Kosina (cherry picked from commit b4ed18a3d56eabd18cfd9841ff05111e3cfbe8f9) --- drivers/hid/i2c-hid/i2c-hid-core.c | 42 +++++++++++++++++++----------- 1 file changed, 27 insertions(+), 15 deletions(-) diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c index 632eaf9e11a6b..2f8a9d3f1e861 100644 --- a/drivers/hid/i2c-hid/i2c-hid-core.c +++ b/drivers/hid/i2c-hid/i2c-hid-core.c @@ -105,6 +105,7 @@ struct i2c_hid { wait_queue_head_t wait; /* For waiting the interrupt */ + struct mutex cmd_lock; /* protects cmdbuf and rawbuf */ struct mutex reset_lock; struct i2chid_ops *ops; @@ -220,6 +221,8 @@ static int i2c_hid_xfer(struct i2c_hid *ihid, static int i2c_hid_read_register(struct i2c_hid *ihid, __le16 reg, void *buf, size_t len) { + guard(mutex)(&ihid->cmd_lock); + *(__le16 *)ihid->cmdbuf = reg; return i2c_hid_xfer(ihid, ihid->cmdbuf, sizeof(__le16), buf, len); @@ -252,6 +255,8 @@ static int i2c_hid_get_report(struct i2c_hid *ihid, i2c_hid_dbg(ihid, "%s\n", __func__); + guard(mutex)(&ihid->cmd_lock); + /* Command register goes first */ *(__le16 *)ihid->cmdbuf = ihid->hdesc.wCommandRegister; length += sizeof(__le16); @@ -342,6 +347,8 @@ static int i2c_hid_set_or_send_report(struct i2c_hid *ihid, if (!do_set && le16_to_cpu(ihid->hdesc.wMaxOutputLength) == 0) return -ENOSYS; + guard(mutex)(&ihid->cmd_lock); + if (do_set) { /* Command register goes first */ *(__le16 *)ihid->cmdbuf = ihid->hdesc.wCommandRegister; @@ -384,6 +391,8 @@ static int i2c_hid_set_power_command(struct i2c_hid *ihid, int power_state) { size_t length; + guard(mutex)(&ihid->cmd_lock); + /* SET_POWER uses command register */ *(__le16 *)ihid->cmdbuf = ihid->hdesc.wCommandRegister; length = sizeof(__le16); @@ -440,25 +449,27 @@ static int i2c_hid_start_hwreset(struct i2c_hid *ihid) if (ret) return ret; - /* Prepare reset command. Command register goes first. */ - *(__le16 *)ihid->cmdbuf = ihid->hdesc.wCommandRegister; - length += sizeof(__le16); - /* Next is RESET command itself */ - length += i2c_hid_encode_command(ihid->cmdbuf + length, - I2C_HID_OPCODE_RESET, 0, 0); + scoped_guard(mutex, &ihid->cmd_lock) { + /* Prepare reset command. Command register goes first. */ + *(__le16 *)ihid->cmdbuf = ihid->hdesc.wCommandRegister; + length += sizeof(__le16); + /* Next is RESET command itself */ + length += i2c_hid_encode_command(ihid->cmdbuf + length, + I2C_HID_OPCODE_RESET, 0, 0); - set_bit(I2C_HID_RESET_PENDING, &ihid->flags); + set_bit(I2C_HID_RESET_PENDING, &ihid->flags); - ret = i2c_hid_xfer(ihid, ihid->cmdbuf, length, NULL, 0); - if (ret) { - dev_err(&ihid->client->dev, - "failed to reset device: %d\n", ret); - goto err_clear_reset; - } + ret = i2c_hid_xfer(ihid, ihid->cmdbuf, length, NULL, 0); + if (ret) { + dev_err(&ihid->client->dev, + "failed to reset device: %d\n", ret); + break; + } - return 0; + return 0; + } -err_clear_reset: + /* Clean up if sending reset command failed */ clear_bit(I2C_HID_RESET_PENDING, &ihid->flags); i2c_hid_set_power(ihid, I2C_HID_PWR_SLEEP); return ret; @@ -1200,6 +1211,7 @@ int i2c_hid_core_probe(struct i2c_client *client, struct i2chid_ops *ops, ihid->is_panel_follower = drm_is_panel_follower(&client->dev); init_waitqueue_head(&ihid->wait); + mutex_init(&ihid->cmd_lock); mutex_init(&ihid->reset_lock); INIT_WORK(&ihid->panel_follower_prepare_work, ihid_core_panel_prepare_work); From 9e48f97c5885439663ecf4da592e5681b68c334b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Mary=C5=84czak?= Date: Tue, 8 Oct 2024 00:25:42 +0200 Subject: [PATCH 11/12] HID: i2c-hid: Delayed i2c resume wakeup for 0x0d42 Goodix touchpad MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit mainline inclusion from mainline-v6.12-rc5 category: bugfix Patch for Goodix 27c6:0d42 touchpads found in Inspiron 5515 laptops. After resume from suspend, one can communicate with this device just fine. We can read data from it or request a reset, but for some reason the interrupt line will not go up when new events are available. (it can correctly respond to a reset with an interrupt tho) The only way I found to wake this device up is to send anything to it after ~1.5s mark, for example a simple read request, or power mode change. In this patch, I simply delay the resume steps with msleep, this will cause the set_power request to happen after the ~1.5s barrier causing the device to resume its event interrupts. Sleep was used rather than delayed_work to make this workaround as non-invasive as possible. [jkosina@suse.com: shortlog update] Signed-off-by: Bartłomiej Maryńczak Signed-off-by: Jiri Kosina (cherry picked from commit 293c485cbac2607595fdaae2b1fb390fc7b2d014) --- drivers/hid/hid-ids.h | 1 + drivers/hid/i2c-hid/i2c-hid-core.c | 10 ++++++++++ 2 files changed, 11 insertions(+) diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h index 622cb71f474ab..376f3ae74d473 100644 --- a/drivers/hid/hid-ids.h +++ b/drivers/hid/hid-ids.h @@ -509,6 +509,7 @@ #define I2C_DEVICE_ID_GOODIX_01E8 0x01e8 #define I2C_DEVICE_ID_GOODIX_01E9 0x01e9 #define I2C_DEVICE_ID_GOODIX_01F0 0x01f0 +#define I2C_DEVICE_ID_GOODIX_0D42 0x0d42 #define USB_VENDOR_ID_GOODTOUCH 0x1aad #define USB_DEVICE_ID_GOODTOUCH_000f 0x000f diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c index 2f8a9d3f1e861..8914c7db94718 100644 --- a/drivers/hid/i2c-hid/i2c-hid-core.c +++ b/drivers/hid/i2c-hid/i2c-hid-core.c @@ -50,6 +50,7 @@ #define I2C_HID_QUIRK_BAD_INPUT_SIZE BIT(3) #define I2C_HID_QUIRK_NO_WAKEUP_AFTER_RESET BIT(4) #define I2C_HID_QUIRK_NO_SLEEP_ON_SUSPEND BIT(5) +#define I2C_HID_QUIRK_DELAY_WAKEUP_AFTER_RESUME BIT(6) /* Command opcodes */ #define I2C_HID_OPCODE_RESET 0x01 @@ -140,6 +141,8 @@ static const struct i2c_hid_quirks { { USB_VENDOR_ID_ELAN, HID_ANY_ID, I2C_HID_QUIRK_NO_WAKEUP_AFTER_RESET | I2C_HID_QUIRK_BOGUS_IRQ }, + { I2C_VENDOR_ID_GOODIX, I2C_DEVICE_ID_GOODIX_0D42, + I2C_HID_QUIRK_DELAY_WAKEUP_AFTER_RESUME }, { 0, 0 } }; @@ -981,6 +984,13 @@ static int i2c_hid_core_resume(struct i2c_hid *ihid) return -ENXIO; } + /* On Goodix 27c6:0d42 wait extra time before device wakeup. + * It's not clear why but if we send wakeup too early, the device will + * never trigger input interrupts. + */ + if (ihid->quirks & I2C_HID_QUIRK_DELAY_WAKEUP_AFTER_RESUME) + msleep(1500); + /* 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 From b3d59ca50e5af362d1341252471bed89de99807f Mon Sep 17 00:00:00 2001 From: Kenny Levinsen Date: Wed, 20 Nov 2024 00:53:17 +0100 Subject: [PATCH 12/12] HID: i2c-hid: Revert to using power commands to wake on resume mainline inclusion from mainline-v6.13-rc2 category: bugfix commit 7d6f065de37c ("HID: i2c-hid: Use address probe to wake on resume") replaced the retry of power commands with the dummy read "bus probe" we use on boot which accounts for a necessary delay before retry. This made at least one Weida device (2575:0910 in an ASUS Vivobook S14) very unhappy, as the bus probe despite being successful somehow lead to the following power command failing so hard that the device never lets go of the bus. This means that even retries of the power command would fail on a timeout as the bus remains busy. Remove the bus probe on resume and instead reintroduce retry of the power command for wake-up purposes while respecting the newly established wake-up retry timings. Fixes: 7d6f065de37c ("HID: i2c-hid: Use address probe to wake on resume") Cc: stable@vger.kernel.org Reported-by: Michael Closes: https://bugzilla.kernel.org/show_bug.cgi?id=219440 Link: https://lore.kernel.org/r/d5acb485-7377-4139-826d-4df04d21b5ed@leemhuis.info/ Signed-off-by: Kenny Levinsen Link: https://patch.msgid.link/20241119235615.23902-1-kl@kl.wtf Signed-off-by: Benjamin Tissoires (cherry picked from commit 34851431ceca1bf457d85895bd38a4e7967e2055) --- drivers/hid/i2c-hid/i2c-hid-core.c | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c index 8914c7db94718..53411762d7ce4 100644 --- a/drivers/hid/i2c-hid/i2c-hid-core.c +++ b/drivers/hid/i2c-hid/i2c-hid-core.c @@ -414,7 +414,19 @@ static int i2c_hid_set_power(struct i2c_hid *ihid, int power_state) i2c_hid_dbg(ihid, "%s\n", __func__); + /* + * Some STM-based devices need 400µs after a rising clock edge to wake + * from deep sleep, in which case the first request will fail due to + * the address not being acknowledged. Try after a short sleep to see + * if the device came alive on the bus. Certain Weida Tech devices also + * need this. + */ ret = i2c_hid_set_power_command(ihid, power_state); + if (ret && power_state == I2C_HID_PWR_ON) { + usleep_range(400, 500); + ret = i2c_hid_set_power_command(ihid, I2C_HID_PWR_ON); + } + if (ret) dev_err(&ihid->client->dev, "failed to change power setting.\n"); @@ -976,14 +988,6 @@ 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; - } - /* On Goodix 27c6:0d42 wait extra time before device wakeup. * It's not clear why but if we send wakeup too early, the device will * never trigger input interrupts.