[PATCH] HID:i2c-hid: add a simple quirk to fix device defects

[PATCH] HID:i2c-hid: add a simple quirk to fix device defects

[hn.chen]

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

Weida's device can get a quirk value by the quirk function.
Base on the quirk value, set_power function will send a command
to wake up the device before send the PWR_ON command.

Signed-off-by: HungNien Chen <hn.chen@weidahitech.com>
---
 drivers/hid/hid-ids.h         |  5 +++++
 drivers/hid/i2c-hid/i2c-hid.c | 32 ++++++++++++++++++++++++++++++++
 2 files changed, 37 insertions(+)

diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index 6cfb5ca..787afdf 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -1033,6 +1033,11 @@
 #define USB_DEVICE_ID_WALTOP_MEDIA_TABLET_14_1_INCH	0x0500
 #define USB_DEVICE_ID_WALTOP_SIRIUS_BATTERY_FREE_TABLET	0x0502
 
+#define	USB_VENDOR_ID_WEIDA		0x2575
+#define	USB_DEVICE_ID_WEIDA_8756	0x8756
+#define	USB_DEVICE_ID_WEIDA_8752	0xC300
+#define	USB_DEVICE_ID_WEIDA_8755	0xC301
+
 #define USB_VENDOR_ID_WISEGROUP		0x0925
 #define USB_DEVICE_ID_SMARTJOY_PLUS	0x0005
 #define USB_DEVICE_ID_SUPER_JOY_BOX_3	0x8888
diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
index b3ec4f2..7a9b100 100644
--- a/drivers/hid/i2c-hid/i2c-hid.c
+++ b/drivers/hid/i2c-hid/i2c-hid.c
@@ -41,6 +41,11 @@
 
 #include <linux/i2c/i2c-hid.h>
 
+#include "../hid-ids.h"
+
+/* quirks to control the device */
+#define I2C_HID_QUIRK_SET_PWR_WAKEUP_DEV	(1 << 0)
+
 /* flags */
 #define I2C_HID_STARTED		0
 #define I2C_HID_RESET_PENDING	1
@@ -143,6 +148,7 @@ struct i2c_hid {
 	char			*argsbuf;	/* Command arguments buffer */
 
 	unsigned long		flags;		/* device flags */
+	unsigned long		quirks;		/* Various quirks */
 
 	wait_queue_head_t	wait;		/* For waiting the interrupt */
 	struct gpio_desc	*desc;
@@ -154,6 +160,25 @@ struct i2c_hid {
 	struct mutex		reset_lock;
 };
 
+/*
+ * i2c_hid_lookup_quirk: return any quirks associated with a I2C HID device
+ * @idVendor: the 16-bit vendor ID
+ * @idProduct: the 16-bit product ID
+ *
+ * Returns: a u32 quirks value.
+ */
+static u32 i2c_hid_lookup_quirk(const u16 idVendor, const u16 idProduct)
+{
+	u32 quirks = 0;
+
+	/* Weida devices check here */
+	if (idVendor == USB_VENDOR_ID_WEIDA &&
+	    idProduct >= USB_DEVICE_ID_WEIDA_8752)
+		return I2C_HID_QUIRK_SET_PWR_WAKEUP_DEV;
+
+	return quirks;
+}
+
 static int __i2c_hid_command(struct i2c_client *client,
 		const struct i2c_hid_cmd *command, u8 reportID,
 		u8 reportType, u8 *args, int args_len,
@@ -346,6 +371,11 @@ static int i2c_hid_set_power(struct i2c_client *client, int power_state)
 
 	i2c_hid_dbg(ihid, "%s\n", __func__);
 
+	/* Some devices require to send a command to wakeup first */
+	if (power_state == I2C_HID_PWR_ON &&
+	    ihid->quirks & I2C_HID_QUIRK_SET_PWR_WAKEUP_DEV)
+		i2c_hid_command(client, &hid_set_power_cmd, NULL, 0);
+
 	ret = __i2c_hid_command(client, &hid_set_power_cmd, power_state,
 		0, NULL, 0, NULL, 0);
 	if (ret)
@@ -661,6 +691,8 @@ static int i2c_hid_parse(struct hid_device *hid)
 
 	i2c_hid_dbg(ihid, "entering %s\n", __func__);
 
+	ihid->quirks = i2c_hid_lookup_quirk(hid->vendor, hid->product);
+
 	rsize = le16_to_cpu(hdesc->wReportDescLength);
 	if (!rsize || rsize > HID_MAX_DESCRIPTOR_SIZE) {
 		dbg_hid("weird size of report descriptor (%u)\n", rsize);
-- 
1.9.1

Re: [PATCH] HID:i2c-hid: add a simple quirk to fix device defects

[Benjamin Tissoires]

On Nov 08 2016 or thereabouts, hn.chen@weidahitech.com wrote:
> From: HungNien Chen <hn.chen@weidahitech.com>
> 
> Weida's device can get a quirk value by the quirk function.
> Base on the quirk value, set_power function will send a command
> to wake up the device before send the PWR_ON command.
> 
> Signed-off-by: HungNien Chen <hn.chen@weidahitech.com>
> ---
>  drivers/hid/hid-ids.h         |  5 +++++
>  drivers/hid/i2c-hid/i2c-hid.c | 32 ++++++++++++++++++++++++++++++++
>  2 files changed, 37 insertions(+)
> 
> diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
> index 6cfb5ca..787afdf 100644
> --- a/drivers/hid/hid-ids.h
> +++ b/drivers/hid/hid-ids.h
> @@ -1033,6 +1033,11 @@
>  #define USB_DEVICE_ID_WALTOP_MEDIA_TABLET_14_1_INCH	0x0500
>  #define USB_DEVICE_ID_WALTOP_SIRIUS_BATTERY_FREE_TABLET	0x0502
>  
> +#define	USB_VENDOR_ID_WEIDA		0x2575
> +#define	USB_DEVICE_ID_WEIDA_8756	0x8756
> +#define	USB_DEVICE_ID_WEIDA_8752	0xC300
> +#define	USB_DEVICE_ID_WEIDA_8755	0xC301
> +
>  #define USB_VENDOR_ID_WISEGROUP		0x0925
>  #define USB_DEVICE_ID_SMARTJOY_PLUS	0x0005
>  #define USB_DEVICE_ID_SUPER_JOY_BOX_3	0x8888
> diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
> index b3ec4f2..7a9b100 100644
> --- a/drivers/hid/i2c-hid/i2c-hid.c
> +++ b/drivers/hid/i2c-hid/i2c-hid.c
> @@ -41,6 +41,11 @@
>  
>  #include <linux/i2c/i2c-hid.h>
>  
> +#include "../hid-ids.h"
> +
> +/* quirks to control the device */
> +#define I2C_HID_QUIRK_SET_PWR_WAKEUP_DEV	(1 << 0)
> +
>  /* flags */
>  #define I2C_HID_STARTED		0
>  #define I2C_HID_RESET_PENDING	1
> @@ -143,6 +148,7 @@ struct i2c_hid {
>  	char			*argsbuf;	/* Command arguments buffer */
>  
>  	unsigned long		flags;		/* device flags */
> +	unsigned long		quirks;		/* Various quirks */
>  
>  	wait_queue_head_t	wait;		/* For waiting the interrupt */
>  	struct gpio_desc	*desc;
> @@ -154,6 +160,25 @@ struct i2c_hid {
>  	struct mutex		reset_lock;
>  };
>  
> +/*
> + * i2c_hid_lookup_quirk: return any quirks associated with a I2C HID device
> + * @idVendor: the 16-bit vendor ID
> + * @idProduct: the 16-bit product ID
> + *
> + * Returns: a u32 quirks value.
> + */
> +static u32 i2c_hid_lookup_quirk(const u16 idVendor, const u16 idProduct)
> +{
> +	u32 quirks = 0;
> +
> +	/* Weida devices check here */
> +	if (idVendor == USB_VENDOR_ID_WEIDA &&
> +	    idProduct >= USB_DEVICE_ID_WEIDA_8752)

Wouldn't it make more sense to have a static table of the affected
products, instead of this test?

> +		return I2C_HID_QUIRK_SET_PWR_WAKEUP_DEV;
> +
> +	return quirks;
> +}
> +
>  static int __i2c_hid_command(struct i2c_client *client,
>  		const struct i2c_hid_cmd *command, u8 reportID,
>  		u8 reportType, u8 *args, int args_len,
> @@ -346,6 +371,11 @@ static int i2c_hid_set_power(struct i2c_client *client, int power_state)
>  
>  	i2c_hid_dbg(ihid, "%s\n", __func__);
>  
> +	/* Some devices require to send a command to wakeup first */
> +	if (power_state == I2C_HID_PWR_ON &&
> +	    ihid->quirks & I2C_HID_QUIRK_SET_PWR_WAKEUP_DEV)
> +		i2c_hid_command(client, &hid_set_power_cmd, NULL, 0);

Isn't there a need to check the return value here?

> +
>  	ret = __i2c_hid_command(client, &hid_set_power_cmd, power_state,
>  		0, NULL, 0, NULL, 0);
>  	if (ret)
> @@ -661,6 +691,8 @@ static int i2c_hid_parse(struct hid_device *hid)
>  
>  	i2c_hid_dbg(ihid, "entering %s\n", __func__);
>  
> +	ihid->quirks = i2c_hid_lookup_quirk(hid->vendor, hid->product);

Please lookup for the quirks in i2c_hid_probe(), right after we set
version, vendor and product

> +
>  	rsize = le16_to_cpu(hdesc->wReportDescLength);
>  	if (!rsize || rsize > HID_MAX_DESCRIPTOR_SIZE) {
>  		dbg_hid("weird size of report descriptor (%u)\n", rsize);
> -- 
> 1.9.1
> 

Cheers,
Benjamin

RE: [PATCH] HID:i2c-hid: add a simple quirk to fix device defects

[Hn Chen]

Hi Benjamin,

Ok, I will add a static quirk table and lookup for the quirks in i2c_hid_probe().

About the return value after send the PWR_ON command, it should be failed in weida's case.
The oscillator of the controller will be gated in the deep sleep mode and 
the controller will be active after the first command but there is no any feedback.
So should I check the failed return value here ? Or I should check if it is ok and then just return ?

Hn.chen

-----Original Message-----
From: Benjamin Tissoires [mailto:benjamin.tissoires@redhat.com] 
Sent: Tuesday, November 08, 2016 5:38 PM
To: Hn Chen
Cc: jkosina@suse.cz; dmitry.torokhov@gmail.com; linux-input@vger.kernel.org
Subject: Re: [PATCH] HID:i2c-hid: add a simple quirk to fix device defects

On Nov 08 2016 or thereabouts, hn.chen@weidahitech.com wrote:
> From: HungNien Chen <hn.chen@weidahitech.com>
> 
> Weida's device can get a quirk value by the quirk function.
> Base on the quirk value, set_power function will send a command to 
> wake up the device before send the PWR_ON command.
> 
> Signed-off-by: HungNien Chen <hn.chen@weidahitech.com>
> ---
>  drivers/hid/hid-ids.h         |  5 +++++
>  drivers/hid/i2c-hid/i2c-hid.c | 32 ++++++++++++++++++++++++++++++++
>  2 files changed, 37 insertions(+)
> 
> diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h index 
> 6cfb5ca..787afdf 100644
> --- a/drivers/hid/hid-ids.h
> +++ b/drivers/hid/hid-ids.h
> @@ -1033,6 +1033,11 @@
>  #define USB_DEVICE_ID_WALTOP_MEDIA_TABLET_14_1_INCH	0x0500
>  #define USB_DEVICE_ID_WALTOP_SIRIUS_BATTERY_FREE_TABLET	0x0502
>  
> +#define	USB_VENDOR_ID_WEIDA		0x2575
> +#define	USB_DEVICE_ID_WEIDA_8756	0x8756
> +#define	USB_DEVICE_ID_WEIDA_8752	0xC300
> +#define	USB_DEVICE_ID_WEIDA_8755	0xC301
> +
>  #define USB_VENDOR_ID_WISEGROUP		0x0925
>  #define USB_DEVICE_ID_SMARTJOY_PLUS	0x0005
>  #define USB_DEVICE_ID_SUPER_JOY_BOX_3	0x8888
> diff --git a/drivers/hid/i2c-hid/i2c-hid.c 
> b/drivers/hid/i2c-hid/i2c-hid.c index b3ec4f2..7a9b100 100644
> --- a/drivers/hid/i2c-hid/i2c-hid.c
> +++ b/drivers/hid/i2c-hid/i2c-hid.c
> @@ -41,6 +41,11 @@
>  
>  #include <linux/i2c/i2c-hid.h>
>  
> +#include "../hid-ids.h"
> +
> +/* quirks to control the device */
> +#define I2C_HID_QUIRK_SET_PWR_WAKEUP_DEV	(1 << 0)
> +
>  /* flags */
>  #define I2C_HID_STARTED		0
>  #define I2C_HID_RESET_PENDING	1
> @@ -143,6 +148,7 @@ struct i2c_hid {
>  	char			*argsbuf;	/* Command arguments buffer */
>  
>  	unsigned long		flags;		/* device flags */
> +	unsigned long		quirks;		/* Various quirks */
>  
>  	wait_queue_head_t	wait;		/* For waiting the interrupt */
>  	struct gpio_desc	*desc;
> @@ -154,6 +160,25 @@ struct i2c_hid {
>  	struct mutex		reset_lock;
>  };
>  
> +/*
> + * i2c_hid_lookup_quirk: return any quirks associated with a I2C HID 
> +device
> + * @idVendor: the 16-bit vendor ID
> + * @idProduct: the 16-bit product ID
> + *
> + * Returns: a u32 quirks value.
> + */
> +static u32 i2c_hid_lookup_quirk(const u16 idVendor, const u16 
> +idProduct) {
> +	u32 quirks = 0;
> +
> +	/* Weida devices check here */
> +	if (idVendor == USB_VENDOR_ID_WEIDA &&
> +	    idProduct >= USB_DEVICE_ID_WEIDA_8752)

Wouldn't it make more sense to have a static table of the affected products, instead of this test?

> +		return I2C_HID_QUIRK_SET_PWR_WAKEUP_DEV;
> +
> +	return quirks;
> +}
> +
>  static int __i2c_hid_command(struct i2c_client *client,
>  		const struct i2c_hid_cmd *command, u8 reportID,
>  		u8 reportType, u8 *args, int args_len, @@ -346,6 +371,11 @@ static 
> int i2c_hid_set_power(struct i2c_client *client, int power_state)
>  
>  	i2c_hid_dbg(ihid, "%s\n", __func__);
>  
> +	/* Some devices require to send a command to wakeup first */
> +	if (power_state == I2C_HID_PWR_ON &&
> +	    ihid->quirks & I2C_HID_QUIRK_SET_PWR_WAKEUP_DEV)
> +		i2c_hid_command(client, &hid_set_power_cmd, NULL, 0);

Isn't there a need to check the return value here?

> +
>  	ret = __i2c_hid_command(client, &hid_set_power_cmd, power_state,
>  		0, NULL, 0, NULL, 0);
>  	if (ret)
> @@ -661,6 +691,8 @@ static int i2c_hid_parse(struct hid_device *hid)
>  
>  	i2c_hid_dbg(ihid, "entering %s\n", __func__);
>  
> +	ihid->quirks = i2c_hid_lookup_quirk(hid->vendor, hid->product);

Please lookup for the quirks in i2c_hid_probe(), right after we set version, vendor and product

> +
>  	rsize = le16_to_cpu(hdesc->wReportDescLength);
>  	if (!rsize || rsize > HID_MAX_DESCRIPTOR_SIZE) {
>  		dbg_hid("weird size of report descriptor (%u)\n", rsize);
> --
> 1.9.1
> 

Cheers,
Benjamin

Re: [PATCH] HID:i2c-hid: add a simple quirk to fix device defects

[Benjamin Tissoires]

On Nov 09 2016 or thereabouts, Hn Chen wrote:
> Hi Benjamin,
> 
> Ok, I will add a static quirk table and lookup for the quirks in i2c_hid_probe().
> 
> About the return value after send the PWR_ON command, it should be failed in weida's case.
> The oscillator of the controller will be gated in the deep sleep mode and 
> the controller will be active after the first command but there is no any feedback.
> So should I check the failed return value here ? Or I should check if it is ok and then just return ?

Maybe just add a comment above saying that this is expected to fail on
some devices, so there is no point in checking the return value here.

Cheers,
Benjamin

> 
> Hn.chen
> 
> -----Original Message-----
> From: Benjamin Tissoires [mailto:benjamin.tissoires@redhat.com] 
> Sent: Tuesday, November 08, 2016 5:38 PM
> To: Hn Chen
> Cc: jkosina@suse.cz; dmitry.torokhov@gmail.com; linux-input@vger.kernel.org
> Subject: Re: [PATCH] HID:i2c-hid: add a simple quirk to fix device defects
> 
> On Nov 08 2016 or thereabouts, hn.chen@weidahitech.com wrote:
> > From: HungNien Chen <hn.chen@weidahitech.com>
> > 
> > Weida's device can get a quirk value by the quirk function.
> > Base on the quirk value, set_power function will send a command to 
> > wake up the device before send the PWR_ON command.
> > 
> > Signed-off-by: HungNien Chen <hn.chen@weidahitech.com>
> > ---
> >  drivers/hid/hid-ids.h         |  5 +++++
> >  drivers/hid/i2c-hid/i2c-hid.c | 32 ++++++++++++++++++++++++++++++++
> >  2 files changed, 37 insertions(+)
> > 
> > diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h index 
> > 6cfb5ca..787afdf 100644
> > --- a/drivers/hid/hid-ids.h
> > +++ b/drivers/hid/hid-ids.h
> > @@ -1033,6 +1033,11 @@
> >  #define USB_DEVICE_ID_WALTOP_MEDIA_TABLET_14_1_INCH	0x0500
> >  #define USB_DEVICE_ID_WALTOP_SIRIUS_BATTERY_FREE_TABLET	0x0502
> >  
> > +#define	USB_VENDOR_ID_WEIDA		0x2575
> > +#define	USB_DEVICE_ID_WEIDA_8756	0x8756
> > +#define	USB_DEVICE_ID_WEIDA_8752	0xC300
> > +#define	USB_DEVICE_ID_WEIDA_8755	0xC301
> > +
> >  #define USB_VENDOR_ID_WISEGROUP		0x0925
> >  #define USB_DEVICE_ID_SMARTJOY_PLUS	0x0005
> >  #define USB_DEVICE_ID_SUPER_JOY_BOX_3	0x8888
> > diff --git a/drivers/hid/i2c-hid/i2c-hid.c 
> > b/drivers/hid/i2c-hid/i2c-hid.c index b3ec4f2..7a9b100 100644
> > --- a/drivers/hid/i2c-hid/i2c-hid.c
> > +++ b/drivers/hid/i2c-hid/i2c-hid.c
> > @@ -41,6 +41,11 @@
> >  
> >  #include <linux/i2c/i2c-hid.h>
> >  
> > +#include "../hid-ids.h"
> > +
> > +/* quirks to control the device */
> > +#define I2C_HID_QUIRK_SET_PWR_WAKEUP_DEV	(1 << 0)
> > +
> >  /* flags */
> >  #define I2C_HID_STARTED		0
> >  #define I2C_HID_RESET_PENDING	1
> > @@ -143,6 +148,7 @@ struct i2c_hid {
> >  	char			*argsbuf;	/* Command arguments buffer */
> >  
> >  	unsigned long		flags;		/* device flags */
> > +	unsigned long		quirks;		/* Various quirks */
> >  
> >  	wait_queue_head_t	wait;		/* For waiting the interrupt */
> >  	struct gpio_desc	*desc;
> > @@ -154,6 +160,25 @@ struct i2c_hid {
> >  	struct mutex		reset_lock;
> >  };
> >  
> > +/*
> > + * i2c_hid_lookup_quirk: return any quirks associated with a I2C HID 
> > +device
> > + * @idVendor: the 16-bit vendor ID
> > + * @idProduct: the 16-bit product ID
> > + *
> > + * Returns: a u32 quirks value.
> > + */
> > +static u32 i2c_hid_lookup_quirk(const u16 idVendor, const u16 
> > +idProduct) {
> > +	u32 quirks = 0;
> > +
> > +	/* Weida devices check here */
> > +	if (idVendor == USB_VENDOR_ID_WEIDA &&
> > +	    idProduct >= USB_DEVICE_ID_WEIDA_8752)
> 
> Wouldn't it make more sense to have a static table of the affected products, instead of this test?
> 
> > +		return I2C_HID_QUIRK_SET_PWR_WAKEUP_DEV;
> > +
> > +	return quirks;
> > +}
> > +
> >  static int __i2c_hid_command(struct i2c_client *client,
> >  		const struct i2c_hid_cmd *command, u8 reportID,
> >  		u8 reportType, u8 *args, int args_len, @@ -346,6 +371,11 @@ static 
> > int i2c_hid_set_power(struct i2c_client *client, int power_state)
> >  
> >  	i2c_hid_dbg(ihid, "%s\n", __func__);
> >  
> > +	/* Some devices require to send a command to wakeup first */
> > +	if (power_state == I2C_HID_PWR_ON &&
> > +	    ihid->quirks & I2C_HID_QUIRK_SET_PWR_WAKEUP_DEV)
> > +		i2c_hid_command(client, &hid_set_power_cmd, NULL, 0);
> 
> Isn't there a need to check the return value here?
> 
> > +
> >  	ret = __i2c_hid_command(client, &hid_set_power_cmd, power_state,
> >  		0, NULL, 0, NULL, 0);
> >  	if (ret)
> > @@ -661,6 +691,8 @@ static int i2c_hid_parse(struct hid_device *hid)
> >  
> >  	i2c_hid_dbg(ihid, "entering %s\n", __func__);
> >  
> > +	ihid->quirks = i2c_hid_lookup_quirk(hid->vendor, hid->product);
> 
> Please lookup for the quirks in i2c_hid_probe(), right after we set version, vendor and product
> 
> > +
> >  	rsize = le16_to_cpu(hdesc->wReportDescLength);
> >  	if (!rsize || rsize > HID_MAX_DESCRIPTOR_SIZE) {
> >  		dbg_hid("weird size of report descriptor (%u)\n", rsize);
> > --
> > 1.9.1
> > 
> 
> Cheers,
> Benjamin
> 

RE: [PATCH] HID:i2c-hid: add a simple quirk to fix device defects

[Hn Chen]

Hi Benjamin,

Just submit patch v2 with comments added and the return value check.

About the return value check, not every time we send command with POWER_ON,
the device was always in the Sleep mode. If the POWER_ON command is executed well(like system boot up),
then just let it leave i2c_hid_set_power().

Hn.chen.

-----Original Message-----
From: Benjamin Tissoires [mailto:benjamin.tissoires@redhat.com] 
Sent: Wednesday, November 09, 2016 4:25 PM
To: Hn Chen
Cc: jkosina@suse.cz; dmitry.torokhov@gmail.com; linux-input@vger.kernel.org
Subject: Re: [PATCH] HID:i2c-hid: add a simple quirk to fix device defects

On Nov 09 2016 or thereabouts, Hn Chen wrote:
> Hi Benjamin,
> 
> Ok, I will add a static quirk table and lookup for the quirks in i2c_hid_probe().
> 
> About the return value after send the PWR_ON command, it should be failed in weida's case.
> The oscillator of the controller will be gated in the deep sleep mode 
> and the controller will be active after the first command but there is no any feedback.
> So should I check the failed return value here ? Or I should check if it is ok and then just return ?

Maybe just add a comment above saying that this is expected to fail on some devices, so there is no point in checking the return value here.

Cheers,
Benjamin

> 
> Hn.chen
> 
> -----Original Message-----
> From: Benjamin Tissoires [mailto:benjamin.tissoires@redhat.com]
> Sent: Tuesday, November 08, 2016 5:38 PM
> To: Hn Chen
> Cc: jkosina@suse.cz; dmitry.torokhov@gmail.com; 
> linux-input@vger.kernel.org
> Subject: Re: [PATCH] HID:i2c-hid: add a simple quirk to fix device 
> defects
> 
> On Nov 08 2016 or thereabouts, hn.chen@weidahitech.com wrote:
> > From: HungNien Chen <hn.chen@weidahitech.com>
> > 
> > Weida's device can get a quirk value by the quirk function.
> > Base on the quirk value, set_power function will send a command to 
> > wake up the device before send the PWR_ON command.
> > 
> > Signed-off-by: HungNien Chen <hn.chen@weidahitech.com>
> > ---
> >  drivers/hid/hid-ids.h         |  5 +++++
> >  drivers/hid/i2c-hid/i2c-hid.c | 32 ++++++++++++++++++++++++++++++++
> >  2 files changed, 37 insertions(+)
> > 
> > diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h index 
> > 6cfb5ca..787afdf 100644
> > --- a/drivers/hid/hid-ids.h
> > +++ b/drivers/hid/hid-ids.h
> > @@ -1033,6 +1033,11 @@
> >  #define USB_DEVICE_ID_WALTOP_MEDIA_TABLET_14_1_INCH	0x0500
> >  #define USB_DEVICE_ID_WALTOP_SIRIUS_BATTERY_FREE_TABLET	0x0502
> >  
> > +#define	USB_VENDOR_ID_WEIDA		0x2575
> > +#define	USB_DEVICE_ID_WEIDA_8756	0x8756
> > +#define	USB_DEVICE_ID_WEIDA_8752	0xC300
> > +#define	USB_DEVICE_ID_WEIDA_8755	0xC301
> > +
> >  #define USB_VENDOR_ID_WISEGROUP		0x0925
> >  #define USB_DEVICE_ID_SMARTJOY_PLUS	0x0005
> >  #define USB_DEVICE_ID_SUPER_JOY_BOX_3	0x8888
> > diff --git a/drivers/hid/i2c-hid/i2c-hid.c 
> > b/drivers/hid/i2c-hid/i2c-hid.c index b3ec4f2..7a9b100 100644
> > --- a/drivers/hid/i2c-hid/i2c-hid.c
> > +++ b/drivers/hid/i2c-hid/i2c-hid.c
> > @@ -41,6 +41,11 @@
> >  
> >  #include <linux/i2c/i2c-hid.h>
> >  
> > +#include "../hid-ids.h"
> > +
> > +/* quirks to control the device */
> > +#define I2C_HID_QUIRK_SET_PWR_WAKEUP_DEV	(1 << 0)
> > +
> >  /* flags */
> >  #define I2C_HID_STARTED		0
> >  #define I2C_HID_RESET_PENDING	1
> > @@ -143,6 +148,7 @@ struct i2c_hid {
> >  	char			*argsbuf;	/* Command arguments buffer */
> >  
> >  	unsigned long		flags;		/* device flags */
> > +	unsigned long		quirks;		/* Various quirks */
> >  
> >  	wait_queue_head_t	wait;		/* For waiting the interrupt */
> >  	struct gpio_desc	*desc;
> > @@ -154,6 +160,25 @@ struct i2c_hid {
> >  	struct mutex		reset_lock;
> >  };
> >  
> > +/*
> > + * i2c_hid_lookup_quirk: return any quirks associated with a I2C 
> > +HID device
> > + * @idVendor: the 16-bit vendor ID
> > + * @idProduct: the 16-bit product ID
> > + *
> > + * Returns: a u32 quirks value.
> > + */
> > +static u32 i2c_hid_lookup_quirk(const u16 idVendor, const u16
> > +idProduct) {
> > +	u32 quirks = 0;
> > +
> > +	/* Weida devices check here */
> > +	if (idVendor == USB_VENDOR_ID_WEIDA &&
> > +	    idProduct >= USB_DEVICE_ID_WEIDA_8752)
> 
> Wouldn't it make more sense to have a static table of the affected products, instead of this test?
> 
> > +		return I2C_HID_QUIRK_SET_PWR_WAKEUP_DEV;
> > +
> > +	return quirks;
> > +}
> > +
> >  static int __i2c_hid_command(struct i2c_client *client,
> >  		const struct i2c_hid_cmd *command, u8 reportID,
> >  		u8 reportType, u8 *args, int args_len, @@ -346,6 +371,11 @@ 
> > static int i2c_hid_set_power(struct i2c_client *client, int 
> > power_state)
> >  
> >  	i2c_hid_dbg(ihid, "%s\n", __func__);
> >  
> > +	/* Some devices require to send a command to wakeup first */
> > +	if (power_state == I2C_HID_PWR_ON &&
> > +	    ihid->quirks & I2C_HID_QUIRK_SET_PWR_WAKEUP_DEV)
> > +		i2c_hid_command(client, &hid_set_power_cmd, NULL, 0);
> 
> Isn't there a need to check the return value here?
> 
> > +
> >  	ret = __i2c_hid_command(client, &hid_set_power_cmd, power_state,
> >  		0, NULL, 0, NULL, 0);
> >  	if (ret)
> > @@ -661,6 +691,8 @@ static int i2c_hid_parse(struct hid_device *hid)
> >  
> >  	i2c_hid_dbg(ihid, "entering %s\n", __func__);
> >  
> > +	ihid->quirks = i2c_hid_lookup_quirk(hid->vendor, hid->product);
> 
> Please lookup for the quirks in i2c_hid_probe(), right after we set 
> version, vendor and product
> 
> > +
> >  	rsize = le16_to_cpu(hdesc->wReportDescLength);
> >  	if (!rsize || rsize > HID_MAX_DESCRIPTOR_SIZE) {
> >  		dbg_hid("weird size of report descriptor (%u)\n", rsize);
> > --
> > 1.9.1
> > 
> 
> Cheers,
> Benjamin
>