[PATCH] HID: i2c-hid: Add quirk for sleep before reset

[PATCH] HID: i2c-hid: Add quirk for sleep before reset

[Brendan McGrath]

Support for the Asus Touchpad was recently added. It turns out this
device can fail initialisation (and become unusable) when the RESET
command is sent too soon after the POWER ON command.

Unfortunately the i2c-hid specification does not specify the need for
a delay between these two commands. But it was discovered the Windows
driver has a 1ms delay.

As a result, this patch adds a new QUIRK to the i2c-hid module that
allows a device to specify a specific sleep inbetween the POWER ON and
RESET commands.

See https://github.com/vlasenko/hid-asus-dkms/issues/24 for further
details.

Signed-off-by: Brendan McGrath <redmcg@redmandi.dyndns.org>
---
I considered three approaches for this patch:
a) add a hardcoded sleep that would affect all devices;
b) add a quirk with a hardcoded sleep value; or
c) add a quirk with a configurable sleep value

Each was a trade off between flexibility and the amount of code/complexity required.

In the end I chose c) as this allowed for the most flexibility; but would
be happy to resubmit the patch using one of the other options (or any other
alternative).

 drivers/hid/i2c-hid/i2c-hid.c | 30 ++++++++++++++++++++++++++----
 1 file changed, 26 insertions(+), 4 deletions(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
index 8d53efe..fb1ebfa 100644
--- a/drivers/hid/i2c-hid/i2c-hid.c
+++ b/drivers/hid/i2c-hid/i2c-hid.c
@@ -45,6 +45,7 @@
 
 /* quirks to control the device */
 #define I2C_HID_QUIRK_SET_PWR_WAKEUP_DEV	BIT(0)
+#define I2C_HID_QUIRK_SLEEP_BEFORE_RESET	BIT(1)
 
 /* flags */
 #define I2C_HID_STARTED		0
@@ -156,17 +157,26 @@ struct i2c_hid {
 
 	bool			irq_wake_enabled;
 	struct mutex		reset_lock;
+
+	__u32			reset_usleep_low;
+	__u32			reset_usleep_high;
 };
 
 static const struct i2c_hid_quirks {
 	__u16 idVendor;
 	__u16 idProduct;
 	__u32 quirks;
+	__u32 reset_usleep_low;
+	__u32 reset_usleep_high;
 } i2c_hid_quirks[] = {
 	{ USB_VENDOR_ID_WEIDA, USB_DEVICE_ID_WEIDA_8752,
 		I2C_HID_QUIRK_SET_PWR_WAKEUP_DEV },
 	{ USB_VENDOR_ID_WEIDA, USB_DEVICE_ID_WEIDA_8755,
 		I2C_HID_QUIRK_SET_PWR_WAKEUP_DEV },
+	{ USB_VENDOR_ID_ASUSTEK, USB_DEVICE_ID_ASUSTEK_TOUCHPAD,
+		I2C_HID_QUIRK_SLEEP_BEFORE_RESET,
+		 .reset_usleep_low = 750,
+		 .reset_usleep_high = 5000 },
 	{ 0, 0 }
 };
 
@@ -177,16 +187,16 @@ static const struct i2c_hid_quirks {
  *
  * Returns: a u32 quirks value.
  */
-static u32 i2c_hid_lookup_quirk(const u16 idVendor, const u16 idProduct)
+static const struct i2c_hid_quirks* i2c_hid_lookup_quirk(const u16 idVendor, const u16 idProduct)
 {
-	u32 quirks = 0;
+	const struct i2c_hid_quirks *quirks = NULL;
 	int n;
 
 	for (n = 0; i2c_hid_quirks[n].idVendor; n++)
 		if (i2c_hid_quirks[n].idVendor == idVendor &&
 		    (i2c_hid_quirks[n].idProduct == (__u16)HID_ANY_ID ||
 		     i2c_hid_quirks[n].idProduct == idProduct))
-			quirks = i2c_hid_quirks[n].quirks;
+			quirks = &i2c_hid_quirks[n];
 
 	return quirks;
 }
@@ -425,6 +435,9 @@ static int i2c_hid_hwreset(struct i2c_client *client)
 	if (ret)
 		goto out_unlock;
 
+	if (ihid->quirks & I2C_HID_QUIRK_SLEEP_BEFORE_RESET)
+		usleep_range(ihid->reset_usleep_low, ihid->reset_usleep_high);
+
 	i2c_hid_dbg(ihid, "resetting...\n");
 
 	ret = i2c_hid_command(client, &hid_reset_cmd, NULL, 0);
@@ -1005,6 +1018,7 @@ static int i2c_hid_probe(struct i2c_client *client,
 	struct i2c_hid *ihid;
 	struct hid_device *hid;
 	__u16 hidRegister;
+	const struct i2c_hid_quirks *quirks;
 	struct i2c_hid_platform_data *platform_data = client->dev.platform_data;
 
 	dbg_hid("HID probe called for i2c 0x%02x\n", client->addr);
@@ -1091,7 +1105,15 @@ static int i2c_hid_probe(struct i2c_client *client,
 		 client->name, hid->vendor, hid->product);
 	strlcpy(hid->phys, dev_name(&client->dev), sizeof(hid->phys));
 
-	ihid->quirks = i2c_hid_lookup_quirk(hid->vendor, hid->product);
+	quirks = i2c_hid_lookup_quirk(hid->vendor, hid->product);
+
+	if (quirks)
+		ihid->quirks = quirks->quirks;
+
+	if (ihid->quirks & I2C_HID_QUIRK_SLEEP_BEFORE_RESET) {
+		ihid->reset_usleep_low = quirks->reset_usleep_low;
+		ihid->reset_usleep_high = quirks->reset_usleep_high;
+	}
 
 	ret = hid_add_device(hid);
 	if (ret) {
-- 
2.7.4

Re: [PATCH] HID: i2c-hid: Add quirk for sleep before reset

[Benjamin Tissoires]

On Jan 05 2017 or thereabouts, Brendan McGrath wrote:
> Support for the Asus Touchpad was recently added. It turns out this
> device can fail initialisation (and become unusable) when the RESET
> command is sent too soon after the POWER ON command.
> 
> Unfortunately the i2c-hid specification does not specify the need for
> a delay between these two commands. But it was discovered the Windows
> driver has a 1ms delay.
> 
> As a result, this patch adds a new QUIRK to the i2c-hid module that
> allows a device to specify a specific sleep inbetween the POWER ON and
> RESET commands.
> 
> See https://github.com/vlasenko/hid-asus-dkms/issues/24 for further
> details.
> 
> Signed-off-by: Brendan McGrath <redmcg@redmandi.dyndns.org>
> ---
> I considered three approaches for this patch:
> a) add a hardcoded sleep that would affect all devices;
> b) add a quirk with a hardcoded sleep value; or
> c) add a quirk with a configurable sleep value
> 
> Each was a trade off between flexibility and the amount of code/complexity required.

I am not a big fan of having to configure the sleep value in each quirk.
I think I'd actually prefer the solution a: no quirk and a small wait
(you wait less than 5ms, I would think this as acceptable). I wouldn't
be surprised if other devices would require such timing issues, so I
wouldn't mind waiting a tiny bit for those to be working without any
quirks.

If anyone prefer not having those timeouts, we can add some quirks, but
I would then prefer solution b.

Cheers,
Benjamin

> 
> In the end I chose c) as this allowed for the most flexibility; but would
> be happy to resubmit the patch using one of the other options (or any other
> alternative).
> 
>  drivers/hid/i2c-hid/i2c-hid.c | 30 ++++++++++++++++++++++++++----
>  1 file changed, 26 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
> index 8d53efe..fb1ebfa 100644
> --- a/drivers/hid/i2c-hid/i2c-hid.c
> +++ b/drivers/hid/i2c-hid/i2c-hid.c
> @@ -45,6 +45,7 @@
>  
>  /* quirks to control the device */
>  #define I2C_HID_QUIRK_SET_PWR_WAKEUP_DEV	BIT(0)
> +#define I2C_HID_QUIRK_SLEEP_BEFORE_RESET	BIT(1)
>  
>  /* flags */
>  #define I2C_HID_STARTED		0
> @@ -156,17 +157,26 @@ struct i2c_hid {
>  
>  	bool			irq_wake_enabled;
>  	struct mutex		reset_lock;
> +
> +	__u32			reset_usleep_low;
> +	__u32			reset_usleep_high;
>  };
>  
>  static const struct i2c_hid_quirks {
>  	__u16 idVendor;
>  	__u16 idProduct;
>  	__u32 quirks;
> +	__u32 reset_usleep_low;
> +	__u32 reset_usleep_high;
>  } i2c_hid_quirks[] = {
>  	{ USB_VENDOR_ID_WEIDA, USB_DEVICE_ID_WEIDA_8752,
>  		I2C_HID_QUIRK_SET_PWR_WAKEUP_DEV },
>  	{ USB_VENDOR_ID_WEIDA, USB_DEVICE_ID_WEIDA_8755,
>  		I2C_HID_QUIRK_SET_PWR_WAKEUP_DEV },
> +	{ USB_VENDOR_ID_ASUSTEK, USB_DEVICE_ID_ASUSTEK_TOUCHPAD,
> +		I2C_HID_QUIRK_SLEEP_BEFORE_RESET,
> +		 .reset_usleep_low = 750,
> +		 .reset_usleep_high = 5000 },
>  	{ 0, 0 }
>  };
>  
> @@ -177,16 +187,16 @@ static const struct i2c_hid_quirks {
>   *
>   * Returns: a u32 quirks value.
>   */
> -static u32 i2c_hid_lookup_quirk(const u16 idVendor, const u16 idProduct)
> +static const struct i2c_hid_quirks* i2c_hid_lookup_quirk(const u16 idVendor, const u16 idProduct)
>  {
> -	u32 quirks = 0;
> +	const struct i2c_hid_quirks *quirks = NULL;
>  	int n;
>  
>  	for (n = 0; i2c_hid_quirks[n].idVendor; n++)
>  		if (i2c_hid_quirks[n].idVendor == idVendor &&
>  		    (i2c_hid_quirks[n].idProduct == (__u16)HID_ANY_ID ||
>  		     i2c_hid_quirks[n].idProduct == idProduct))
> -			quirks = i2c_hid_quirks[n].quirks;
> +			quirks = &i2c_hid_quirks[n];
>  
>  	return quirks;
>  }
> @@ -425,6 +435,9 @@ static int i2c_hid_hwreset(struct i2c_client *client)
>  	if (ret)
>  		goto out_unlock;
>  
> +	if (ihid->quirks & I2C_HID_QUIRK_SLEEP_BEFORE_RESET)
> +		usleep_range(ihid->reset_usleep_low, ihid->reset_usleep_high);
> +
>  	i2c_hid_dbg(ihid, "resetting...\n");
>  
>  	ret = i2c_hid_command(client, &hid_reset_cmd, NULL, 0);
> @@ -1005,6 +1018,7 @@ static int i2c_hid_probe(struct i2c_client *client,
>  	struct i2c_hid *ihid;
>  	struct hid_device *hid;
>  	__u16 hidRegister;
> +	const struct i2c_hid_quirks *quirks;
>  	struct i2c_hid_platform_data *platform_data = client->dev.platform_data;
>  
>  	dbg_hid("HID probe called for i2c 0x%02x\n", client->addr);
> @@ -1091,7 +1105,15 @@ static int i2c_hid_probe(struct i2c_client *client,
>  		 client->name, hid->vendor, hid->product);
>  	strlcpy(hid->phys, dev_name(&client->dev), sizeof(hid->phys));
>  
> -	ihid->quirks = i2c_hid_lookup_quirk(hid->vendor, hid->product);
> +	quirks = i2c_hid_lookup_quirk(hid->vendor, hid->product);
> +
> +	if (quirks)
> +		ihid->quirks = quirks->quirks;
> +
> +	if (ihid->quirks & I2C_HID_QUIRK_SLEEP_BEFORE_RESET) {
> +		ihid->reset_usleep_low = quirks->reset_usleep_low;
> +		ihid->reset_usleep_high = quirks->reset_usleep_high;
> +	}
>  
>  	ret = hid_add_device(hid);
>  	if (ret) {
> -- 
> 2.7.4
> 

[PATCHv2] HID: i2c-hid: Add sleep between POWER ON and RESET

[Brendan McGrath]

Support for the Asus Touchpad was recently added. It turns out this
device can fail initialisation (and become unusable) when the RESET
command is sent too soon after the POWER ON command.

Unfortunately the i2c-hid specification does not specify the need for
a delay between these two commands. But it was discovered the Windows
driver has a 1ms delay.

As a result, this patch modifies the i2c-hid module to add a sleep
inbetween the POWER ON and RESET commands which lasts between 1ms and 5ms.

See https://github.com/vlasenko/hid-asus-dkms/issues/24 for further
details.

Signed-off-by: Brendan McGrath <redmcg@redmandi.dyndns.org>
---
Patch has been changed to use option a)

I upped the minimum sleep to 1ms to match Microsoft. I figure this is a
safer value.

 drivers/hid/i2c-hid/i2c-hid.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
index 844662c..61ff6a6 100644
--- a/drivers/hid/i2c-hid/i2c-hid.c
+++ b/drivers/hid/i2c-hid/i2c-hid.c
@@ -426,6 +426,7 @@ static int i2c_hid_hwreset(struct i2c_client *client)
 	if (ret)
 		goto out_unlock;
 
+	usleep_range(1000, 5000);
 	i2c_hid_dbg(ihid, "resetting...\n");
 
 	ret = i2c_hid_command(client, &hid_reset_cmd, NULL, 0);
-- 
2.7.4

Re: [PATCHv2] HID: i2c-hid: Add sleep between POWER ON and RESET

[Benjamin Tissoires]

On Jan 06 2017 or thereabouts, Brendan McGrath wrote:
> Support for the Asus Touchpad was recently added. It turns out this
> device can fail initialisation (and become unusable) when the RESET
> command is sent too soon after the POWER ON command.
> 
> Unfortunately the i2c-hid specification does not specify the need for
> a delay between these two commands. But it was discovered the Windows
> driver has a 1ms delay.
> 
> As a result, this patch modifies the i2c-hid module to add a sleep
> inbetween the POWER ON and RESET commands which lasts between 1ms and 5ms.
> 
> See https://github.com/vlasenko/hid-asus-dkms/issues/24 for further
> details.
> 
> Signed-off-by: Brendan McGrath <redmcg@redmandi.dyndns.org>
> ---
> Patch has been changed to use option a)
> 
> I upped the minimum sleep to 1ms to match Microsoft. I figure this is a
> safer value.
> 
>  drivers/hid/i2c-hid/i2c-hid.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
> index 844662c..61ff6a6 100644
> --- a/drivers/hid/i2c-hid/i2c-hid.c
> +++ b/drivers/hid/i2c-hid/i2c-hid.c
> @@ -426,6 +426,7 @@ static int i2c_hid_hwreset(struct i2c_client *client)
>  	if (ret)
>  		goto out_unlock;
>  

Sorry to request a new version, but I think we should have a comment
here explaining the sleep. It should be noted here that it's not in the
spec but it mimics the behavior of the Windows driver, thus hardware
makers might rely on this small timeout instead of following the spec.

Cheers,
Benjamin

> +	usleep_range(1000, 5000);
>  	i2c_hid_dbg(ihid, "resetting...\n");
>  
>  	ret = i2c_hid_command(client, &hid_reset_cmd, NULL, 0);
> -- 
> 2.7.4
> 

[PATCHv3] HID: i2c-hid: Add sleep between POWER ON and RESET

[Brendan McGrath]

Support for the Asus Touchpad was recently added. It turns out this
device can fail initialisation (and become unusable) when the RESET
command is sent too soon after the POWER ON command.

Unfortunately the i2c-hid specification does not specify the need for
a delay between these two commands. But it was discovered the Windows
driver has a 1ms delay.

As a result, this patch modifies the i2c-hid module to add a sleep
inbetween the POWER ON and RESET commands which lasts between 1ms and 5ms.

See https://github.com/vlasenko/hid-asus-dkms/issues/24 for further
details.

Signed-off-by: Brendan McGrath <redmcg@redmandi.dyndns.org>
---
Changes since v2:
  - added comments to explain the reason for the sleep

 drivers/hid/i2c-hid/i2c-hid.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
index 844662c..d5288f3 100644
--- a/drivers/hid/i2c-hid/i2c-hid.c
+++ b/drivers/hid/i2c-hid/i2c-hid.c
@@ -426,6 +426,15 @@ 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.7.4

Re: [PATCHv3] HID: i2c-hid: Add sleep between POWER ON and RESET

[Benjamin Tissoires]

On Jan 07 2017 or thereabouts, Brendan McGrath wrote:
> Support for the Asus Touchpad was recently added. It turns out this
> device can fail initialisation (and become unusable) when the RESET
> command is sent too soon after the POWER ON command.
> 
> Unfortunately the i2c-hid specification does not specify the need for
> a delay between these two commands. But it was discovered the Windows
> driver has a 1ms delay.
> 
> As a result, this patch modifies the i2c-hid module to add a sleep
> inbetween the POWER ON and RESET commands which lasts between 1ms and 5ms.
> 
> See https://github.com/vlasenko/hid-asus-dkms/issues/24 for further
> details.
> 
> Signed-off-by: Brendan McGrath <redmcg@redmandi.dyndns.org>
> ---
> Changes since v2:
>   - added comments to explain the reason for the sleep
> 

Looks good now, thanks!

Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

Cheers,
Benjamin

>  drivers/hid/i2c-hid/i2c-hid.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
> index 844662c..d5288f3 100644
> --- a/drivers/hid/i2c-hid/i2c-hid.c
> +++ b/drivers/hid/i2c-hid/i2c-hid.c
> @@ -426,6 +426,15 @@ 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.7.4
> 

Re: [PATCHv3] HID: i2c-hid: Add sleep between POWER ON and RESET

[Jiri Kosina]

On Sat, 7 Jan 2017, Brendan McGrath wrote:

> Support for the Asus Touchpad was recently added. It turns out this
> device can fail initialisation (and become unusable) when the RESET
> command is sent too soon after the POWER ON command.
> 
> Unfortunately the i2c-hid specification does not specify the need for
> a delay between these two commands. But it was discovered the Windows
> driver has a 1ms delay.
> 
> As a result, this patch modifies the i2c-hid module to add a sleep
> inbetween the POWER ON and RESET commands which lasts between 1ms and 5ms.
> 
> See https://github.com/vlasenko/hid-asus-dkms/issues/24 for further
> details.
> 
> Signed-off-by: Brendan McGrath <redmcg@redmandi.dyndns.org>
> ---
> Changes since v2:
>   - added comments to explain the reason for the sleep

Applied to for-4.10/upstream-fixes, thanks.

-- 
Jiri Kosina
SUSE Labs