ACK/Cmnt: [PATCH 1/1] HID: i2c-hid: Always sleep 60ms after I2C_HID_PWR_ON commands

[Stefan Bader]

On 18.08.20 10:43, Kai-Heng Feng wrote:
> From: Hans de Goede <hdegoede at 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 at vger.kernel.org
> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=208247
> Reported-by: Kai-Heng Feng <kai.heng.feng at canonical.com>
> Reported-and-tested-by: Andrea Borgia <andrea at borgia.bo.it>
> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
> Signed-off-by: Jiri Kosina <jkosina at suse.cz>
> (cherry picked from commit eef4016243e94c438f177ca8226876eb873b9c75 linux-next)
> Signed-off-by: Kai-Heng Feng <kai.heng.feng at canonical.com>
Acked-by: Stefan Bader <stefan.bader at 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);
> 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.ubuntu.com/archives/kernel-team/attachments/20200819/b090ef73/attachment.sig>