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@xxxxxxxxxx] Sent: Wednesday, November 09, 2016 4:25 PM To: Hn Chen Cc: jkosina@xxxxxxx; dmitry.torokhov@xxxxxxxxx; linux-input@xxxxxxxxxxxxxxx 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@xxxxxxxxxx] > Sent: Tuesday, November 08, 2016 5:38 PM > To: Hn Chen > Cc: jkosina@xxxxxxx; dmitry.torokhov@xxxxxxxxx; > linux-input@xxxxxxxxxxxxxxx > Subject: Re: [PATCH] HID:i2c-hid: add a simple quirk to fix device > defects > > On Nov 08 2016 or thereabouts, hn.chen@xxxxxxxxxxxxxxx wrote: > > From: HungNien Chen <hn.chen@xxxxxxxxxxxxxxx> > > > > 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@xxxxxxxxxxxxxxx> > > --- > > 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 > ��.n��������+%������w��{.n�����{��)��^n�r������&��z�ޗ�zf���h���~����������_��+v���)ߣ�
