| Message ID | 20200818084309.7114-2-kai.heng.feng@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fix non-working Goodix touchpad after system sleep | expand |
On 18.08.20 10:43, Kai-Heng Feng wrote: > From: Hans de Goede <hdegoede@redhat.com> > > BugLink: https://bugs.launchpad.net/bugs/1891998 > > Before this commit i2c_hid_parse() consists of the following steps: > > 1. Send power on cmd > 2. usleep_range(1000, 5000) > 3. Send reset cmd > 4. Wait for reset to complete (device interrupt, or msleep(100)) > 5. Send power on cmd > 6. Try to read HID descriptor > > Notice how there is an usleep_range(1000, 5000) after the first power-on > command, but not after the second power-on command. > > Testing has shown that at least on the BMAX Y13 laptop's i2c-hid touchpad, > not having a delay after the second power-on command causes the HID > descriptor to read as all zeros. > > In case we hit this on other devices too, the descriptor being all zeros > can be recognized by the following message being logged many, many times: > > hid-generic 0018:0911:5288.0002: unknown main item tag 0x0 > > At the same time as the BMAX Y13's touchpad issue was debugged, > Kai-Heng was working on debugging some issues with Goodix i2c-hid > touchpads. It turns out that these need a delay after a PWR_ON command > too, otherwise they stop working after a suspend/resume cycle. > According to Goodix a delay of minimal 60ms is needed. > > Having multiple cases where we need a delay after sending the power-on > command, seems to indicate that we should always sleep after the power-on > command. > > This commit fixes the mentioned issues by moving the existing 1ms sleep to > the i2c_hid_set_power() function and changing it to a 60ms sleep. > > Cc: stable@vger.kernel.org > BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=208247 > Reported-by: Kai-Heng Feng <kai.heng.feng@canonical.com> > Reported-and-tested-by: Andrea Borgia <andrea@borgia.bo.it> > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > Signed-off-by: Jiri Kosina <jkosina@suse.cz> > (cherry picked from commit eef4016243e94c438f177ca8226876eb873b9c75 linux-next) > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> Acked-by: Stefan Bader <stefan.bader@canonical.com> > --- I guess the regression potential would be I2C attached devices acting oddly after power on / suspend/resume. Plus the estimate you already had. > drivers/hid/i2c-hid/i2c-hid-core.c | 22 +++++++++++++--------- > 1 file changed, 13 insertions(+), 9 deletions(-) > > diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c > index 294c84e136d7..dbd04492825d 100644 > --- a/drivers/hid/i2c-hid/i2c-hid-core.c > +++ b/drivers/hid/i2c-hid/i2c-hid-core.c > @@ -420,6 +420,19 @@ static int i2c_hid_set_power(struct i2c_client *client, int power_state) > dev_err(&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. > + * However, it has been observered that the Windows driver provides a > + * 1ms sleep between the PWR_ON and RESET requests. > + * According to Goodix Windows even waits 60 ms after (other?) > + * PWR_ON requests. Testing has confirmed that several devices > + * will not work properly without a delay after a PWR_ON request. > + */ > + if (!ret && power_state == I2C_HID_PWR_ON) > + msleep(60); > + > return ret; > } > > @@ -441,15 +454,6 @@ static int i2c_hid_hwreset(struct i2c_client *client) > if (ret) > goto out_unlock; > > - /* > - * The HID over I2C specification states that if a DEVICE needs time > - * after the PWR_ON request, it should utilise CLOCK stretching. > - * However, it has been observered that the Windows driver provides a > - * 1ms sleep between the PWR_ON and RESET requests and that some devices > - * rely on this. > - */ > - usleep_range(1000, 5000); > - > i2c_hid_dbg(ihid, "resetting...\n"); > > ret = i2c_hid_command(client, &hid_reset_cmd, NULL, 0); >
diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c index 294c84e136d7..dbd04492825d 100644 --- a/drivers/hid/i2c-hid/i2c-hid-core.c +++ b/drivers/hid/i2c-hid/i2c-hid-core.c @@ -420,6 +420,19 @@ static int i2c_hid_set_power(struct i2c_client *client, int power_state) dev_err(&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. + * However, it has been observered that the Windows driver provides a + * 1ms sleep between the PWR_ON and RESET requests. + * According to Goodix Windows even waits 60 ms after (other?) + * PWR_ON requests. Testing has confirmed that several devices + * will not work properly without a delay after a PWR_ON request. + */ + if (!ret && power_state == I2C_HID_PWR_ON) + msleep(60); + return ret; } @@ -441,15 +454,6 @@ static int i2c_hid_hwreset(struct i2c_client *client) if (ret) goto out_unlock; - /* - * The HID over I2C specification states that if a DEVICE needs time - * after the PWR_ON request, it should utilise CLOCK stretching. - * However, it has been observered that the Windows driver provides a - * 1ms sleep between the PWR_ON and RESET requests and that some devices - * rely on this. - */ - usleep_range(1000, 5000); - i2c_hid_dbg(ihid, "resetting...\n"); ret = i2c_hid_command(client, &hid_reset_cmd, NULL, 0);