[PATCH] HID: i2c-hid: add retry in set power for fixing weida's issue

[PATCH] HID: i2c-hid: add retry in set power for fixing weida's issue

[hn.chen]

From: HungNien Chen <hn.chen@weidahitech.com>

Just modify the set_power function to send the command twice.
It should be ok for other contorllers since it will jump out the loop after
the first command send out. If this is not a proper modification,
please tell me a proper way to fix this kind of issue.

Signed-off-by: HungNien Chen <hn.chen@weidahitech.com>
---
 drivers/hid/i2c-hid/i2c-hid.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
index b3ec4f2..d7423d9 100644
--- a/drivers/hid/i2c-hid/i2c-hid.c
+++ b/drivers/hid/i2c-hid/i2c-hid.c
@@ -49,6 +49,8 @@
 #define I2C_HID_PWR_ON		0x00
 #define I2C_HID_PWR_SLEEP	0x01
 
+#define	SET_PWR_RETRIES		2
+
 /* debug option */
 static bool debug;
 module_param(debug, bool, 0444);
@@ -343,14 +345,26 @@ static int i2c_hid_set_power(struct i2c_client *client, int power_state)
 {
 	struct i2c_hid *ihid = i2c_get_clientdata(client);
 	int ret;
+	int retry;
 
 	i2c_hid_dbg(ihid, "%s\n", __func__);
 
-	ret = __i2c_hid_command(client, &hid_set_power_cmd, power_state,
-		0, NULL, 0, NULL, 0);
-	if (ret)
-		dev_err(&client->dev, "failed to change power setting.\n");
+       /*
+	* Some Weida's controllers require Set_Power twice on resume.
+	* The 1st cmd wakeup the controller and the 2nd cmd will be executed.
+	* It should be safe to controllers of other vendors.
+	*/
+	for (retry = 0; retry < SET_PWR_RETRIES; retry++) {
+		ret = __i2c_hid_command(client, &hid_set_power_cmd,
+			power_state, 0, NULL, 0, NULL, 0);
+
+		if (!ret)
+			goto set_power_exit;
+	}
+
+	dev_err(&client->dev, "failed to change power setting.\n");
 
+set_power_exit:
 	return ret;
 }
 
-- 
1.9.1

Re: [PATCH] HID: i2c-hid: add retry in set power for fixing weida's issue

[Jiri Kosina]

On Mon, 7 Nov 2016, hn.chen@weidahitech.com wrote:

> From: HungNien Chen <hn.chen@weidahitech.com>
> 
> Just modify the set_power function to send the command twice.
> It should be ok for other contorllers since it will jump out the loop after
> the first command send out. If this is not a proper modification,
> please tell me a proper way to fix this kind of issue.
> 
> Signed-off-by: HungNien Chen <hn.chen@weidahitech.com>
> ---
>  drivers/hid/i2c-hid/i2c-hid.c | 22 ++++++++++++++++++----
>  1 file changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
> index b3ec4f2..d7423d9 100644
> --- a/drivers/hid/i2c-hid/i2c-hid.c
> +++ b/drivers/hid/i2c-hid/i2c-hid.c
> @@ -49,6 +49,8 @@
>  #define I2C_HID_PWR_ON		0x00
>  #define I2C_HID_PWR_SLEEP	0x01
>  
> +#define	SET_PWR_RETRIES		2
> +
>  /* debug option */
>  static bool debug;
>  module_param(debug, bool, 0444);
> @@ -343,14 +345,26 @@ static int i2c_hid_set_power(struct i2c_client *client, int power_state)
>  {
>  	struct i2c_hid *ihid = i2c_get_clientdata(client);
>  	int ret;
> +	int retry;
>  
>  	i2c_hid_dbg(ihid, "%s\n", __func__);
>  
> -	ret = __i2c_hid_command(client, &hid_set_power_cmd, power_state,
> -		0, NULL, 0, NULL, 0);
> -	if (ret)
> -		dev_err(&client->dev, "failed to change power setting.\n");
> +       /*
> +	* Some Weida's controllers require Set_Power twice on resume.
> +	* The 1st cmd wakeup the controller and the 2nd cmd will be executed.
> +	* It should be safe to controllers of other vendors.
> +	*/
> +	for (retry = 0; retry < SET_PWR_RETRIES; retry++) {
> +		ret = __i2c_hid_command(client, &hid_set_power_cmd,
> +			power_state, 0, NULL, 0, NULL, 0);
> +
> +		if (!ret)
> +			goto set_power_exit;

This is ugly, and we have no idea what this might do to any other 
(potential) devices. Can't you make this a quirk that'd be applied only to 
this specific device?

-- 
Jiri Kosina
SUSE Labs

RE: [PATCH] HID: i2c-hid: add retry in set power for fixing weida's issue

[Hn Chen]

Hi Jiri,

Thanks for the tip.

Since I don't have any experience to add a quirk, may I have a sample code for reference ?
It should be a HID quirk, right ?

Hn.chen.

-----Original Message-----
From: Jiri Kosina [mailto:jikos@kernel.org] 
Sent: Monday, November 07, 2016 6:49 PM
To: Hn Chen
Cc: benjamin.tissoires@redhat.com; dmitry.torokhov@gmail.com; linux-input@vger.kernel.org
Subject: Re: [PATCH] HID: i2c-hid: add retry in set power for fixing weida's issue

On Mon, 7 Nov 2016, hn.chen@weidahitech.com wrote:

> From: HungNien Chen <hn.chen@weidahitech.com>
> 
> Just modify the set_power function to send the command twice.
> It should be ok for other contorllers since it will jump out the loop 
> after the first command send out. If this is not a proper 
> modification, please tell me a proper way to fix this kind of issue.
> 
> Signed-off-by: HungNien Chen <hn.chen@weidahitech.com>
> ---
>  drivers/hid/i2c-hid/i2c-hid.c | 22 ++++++++++++++++++----
>  1 file changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/hid/i2c-hid/i2c-hid.c 
> b/drivers/hid/i2c-hid/i2c-hid.c index b3ec4f2..d7423d9 100644
> --- a/drivers/hid/i2c-hid/i2c-hid.c
> +++ b/drivers/hid/i2c-hid/i2c-hid.c
> @@ -49,6 +49,8 @@
>  #define I2C_HID_PWR_ON		0x00
>  #define I2C_HID_PWR_SLEEP	0x01
>  
> +#define	SET_PWR_RETRIES		2
> +
>  /* debug option */
>  static bool debug;
>  module_param(debug, bool, 0444);
> @@ -343,14 +345,26 @@ static int i2c_hid_set_power(struct i2c_client 
> *client, int power_state)  {
>  	struct i2c_hid *ihid = i2c_get_clientdata(client);
>  	int ret;
> +	int retry;
>  
>  	i2c_hid_dbg(ihid, "%s\n", __func__);
>  
> -	ret = __i2c_hid_command(client, &hid_set_power_cmd, power_state,
> -		0, NULL, 0, NULL, 0);
> -	if (ret)
> -		dev_err(&client->dev, "failed to change power setting.\n");
> +       /*
> +	* Some Weida's controllers require Set_Power twice on resume.
> +	* The 1st cmd wakeup the controller and the 2nd cmd will be executed.
> +	* It should be safe to controllers of other vendors.
> +	*/
> +	for (retry = 0; retry < SET_PWR_RETRIES; retry++) {
> +		ret = __i2c_hid_command(client, &hid_set_power_cmd,
> +			power_state, 0, NULL, 0, NULL, 0);
> +
> +		if (!ret)
> +			goto set_power_exit;

This is ugly, and we have no idea what this might do to any other
(potential) devices. Can't you make this a quirk that'd be applied only to this specific device?

--
Jiri Kosina
SUSE Labs

RE: [PATCH] HID: i2c-hid: add retry in set power for fixing weida's issue

[Jiri Kosina]

On Mon, 7 Nov 2016, Hn Chen wrote:

> Since I don't have any experience to add a quirk, may I have a sample 
> code for reference ? It should be a HID quirk, right ?

Generic HID quirk might be over-stretching it, as i2c_hid_set_power() is 
being called from various places, so generalizing this requirement out 
into HID core wouldn't be clean either.

You can special-case this behavior by looking at wVendorID / wProductID / 
wVersionID from the report descriptor, can't you?

There is not a quirk mechanism for i2c-hid yet. If things start exploding 
like in usb-hid space, we'd definitely want to have a generic quirk 
mechanism one day. But if this has been a single case over the existence 
of the hid-over-i2c, I wouldn't bother (yet).

-- 
Jiri Kosina
SUSE Labs

RE: [PATCH] HID: i2c-hid: add retry in set power for fixing weida's issue

[Hn Chen]

HI Jiri,

Ok. I see.  
I will do simple vid/pid filter to see if it's weida's controller.
Send the extra command if the answer is true.

hn.chen
________________________________________
From: Jiri Kosina [jikos@kernel.org]
Sent: Monday, November 07, 2016 7:59 PM
To: Hn Chen
Cc: benjamin.tissoires@redhat.com; dmitry.torokhov@gmail.com; linux-input@vger.kernel.org
Subject: RE: [PATCH] HID: i2c-hid: add retry in set power for fixing weida's issue

On Mon, 7 Nov 2016, Hn Chen wrote:

> Since I don't have any experience to add a quirk, may I have a sample
> code for reference ? It should be a HID quirk, right ?

Generic HID quirk might be over-stretching it, as i2c_hid_set_power() is
being called from various places, so generalizing this requirement out
into HID core wouldn't be clean either.

You can special-case this behavior by looking at wVendorID / wProductID /
wVersionID from the report descriptor, can't you?

There is not a quirk mechanism for i2c-hid yet. If things start exploding
like in usb-hid space, we'd definitely want to have a generic quirk
mechanism one day. But if this has been a single case over the existence
of the hid-over-i2c, I wouldn't bother (yet).

--
Jiri Kosina
SUSE Labs

RE: [PATCH] HID: i2c-hid: add retry in set power for fixing weida's issue

[Hn Chen]

Hi all,

I am sorry for the format mess up.
Some messages are missing and just have an additional description here.

Weida's controllers(WDT8752/WDT8755) require 2 steps to wakeup from SLEEP mode. 
The first command will wakeup the controller but will be ignored. 
The second command will be executed when the controller was active.

Hn.chen.

-----Original Message-----
From: hn.chen@weidahitech.com [mailto:hn.chen@weidahitech.com] 
Sent: Monday, November 07, 2016 6:39 PM
To: jkosina@suse.cz
Cc: benjamin.tissoires@redhat.com; dmitry.torokhov@gmail.com; linux-input@vger.kernel.org; Hn Chen
Subject: [PATCH] HID: i2c-hid: add retry in set power for fixing weida's issue

From: HungNien Chen <hn.chen@weidahitech.com>

Just modify the set_power function to send the command twice.
It should be ok for other contorllers since it will jump out the loop after the first command send out. If this is not a proper modification, please tell me a proper way to fix this kind of issue.

Signed-off-by: HungNien Chen <hn.chen@weidahitech.com>
---
 drivers/hid/i2c-hid/i2c-hid.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c index b3ec4f2..d7423d9 100644
--- a/drivers/hid/i2c-hid/i2c-hid.c
+++ b/drivers/hid/i2c-hid/i2c-hid.c
@@ -49,6 +49,8 @@
 #define I2C_HID_PWR_ON		0x00
 #define I2C_HID_PWR_SLEEP	0x01
 
+#define	SET_PWR_RETRIES		2
+
 /* debug option */
 static bool debug;
 module_param(debug, bool, 0444);
@@ -343,14 +345,26 @@ static int i2c_hid_set_power(struct i2c_client *client, int power_state)  {
 	struct i2c_hid *ihid = i2c_get_clientdata(client);
 	int ret;
+	int retry;
 
 	i2c_hid_dbg(ihid, "%s\n", __func__);
 
-	ret = __i2c_hid_command(client, &hid_set_power_cmd, power_state,
-		0, NULL, 0, NULL, 0);
-	if (ret)
-		dev_err(&client->dev, "failed to change power setting.\n");
+       /*
+	* Some Weida's controllers require Set_Power twice on resume.
+	* The 1st cmd wakeup the controller and the 2nd cmd will be executed.
+	* It should be safe to controllers of other vendors.
+	*/
+	for (retry = 0; retry < SET_PWR_RETRIES; retry++) {
+		ret = __i2c_hid_command(client, &hid_set_power_cmd,
+			power_state, 0, NULL, 0, NULL, 0);
+
+		if (!ret)
+			goto set_power_exit;
+	}
+
+	dev_err(&client->dev, "failed to change power setting.\n");
 
+set_power_exit:
 	return ret;
 }
 
--
1.9.1