| Message ID | 1478515138-1922-1-git-send-email-hn.chen@weidahitech.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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?
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 -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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).
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
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; }