Hi Hans, > On Aug 11, 2020, at 20:59, Hans de Goede <hdegoede@xxxxxxxxxx> wrote: > > 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. Thanks for your patch. The subject is still "1ms" instead of "60ms". Kai-Heng > > Cc: stable@xxxxxxxxxxxxxxx > BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=208247 > Reported-by: Kai-Heng Feng <kai.heng.feng@xxxxxxxxxxxxx> > Reported-and-tested-by: Andrea Borgia <andrea@xxxxxxxxxxxx> > Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx> > --- > Changes in v2: > - Add Kai-Heng's case, with Goodix touchpads needing a delay after PWR_ON too, > to the commit message > - Add a Reported-by tag for Kai-Heng > - Increase the delay to 60ms > --- > 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); > -- > 2.28.0 >
