Skip to content

i2c_hid: ELAN touchpad bugs fix #663

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 6 commits into from
Closed

i2c_hid: ELAN touchpad bugs fix #663

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e3d16ad
ELAN touchpad i2c_hid bugs fix
Mar 19, 2019
d7eb86d
i2c-hid: Requested changes
Mar 20, 2019
b7a1b00
i2c-hid: revert BOGUS_IRQ check
Mar 20, 2019
14a6666
i2c-hid: define and quirk sections moved
Mar 20, 2019
844193c
i2c-hid: extra newline fix
Mar 20, 2019
f7ee5a1
i2c-hid: Benjamin Tissoires changes applied
Mar 20, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions drivers/hid/hid-ids.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -385,6 +385,8 @@
#define USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_73F7 0x73f7
#define USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_A001 0xa001

#define I2C_PRODUCT_ID_ELAN_TOUCHPAD 0x303e
Copy link
Contributor

@andy-shev andy-shev Mar 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm... I'm not sure you need to put this into header file.
And the name should start with I2C_DEVICE_ID_ of course.

Copy link
Contributor

@jwrdegoede jwrdegoede Mar 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For hid devices we have the convention to gather all vendor / product id defines in this single header.

We also have the convention to simply prefix all IDS with USB_DEVICE_ID_ even if it is an i2c device, the only exception to this is when the vendor uses a different VENDOR_ID for usb vs i2c, which they usually do not.

Copy link
Author

@dalechyn dalechyn Mar 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For hid devices we have the convention to gather all vendor / product id defines in this single header.

We also have the convention to simply prefix all IDS with USB_DEVICE_ID_ even if it is an i2c device, the only exception to this is when the vendor uses a different VENDOR_ID for usb vs i2c, which they usually do not.

Then I will rename it to USB_DEVICE_ID


#define USB_VENDOR_ID_ELAN 0x04f3
#define USB_DEVICE_ID_TOSHIBA_CLICK_L9W 0x0401
#define USB_DEVICE_ID_HP_X2 0x074d
Expand Down
22 changes: 9 additions & 13 deletions drivers/hid/i2c-hid/i2c-hid-core.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@
#define I2C_HID_QUIRK_NO_IRQ_AFTER_RESET BIT(1)
#define I2C_HID_QUIRK_NO_RUNTIME_PM BIT(2)
#define I2C_HID_QUIRK_DELAY_AFTER_SLEEP BIT(3)
#define I2C_HID_QUIRK_BOGUS_IRQ BIT(4)
Copy link
Contributor

@andy-shev andy-shev Mar 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are not supposed to touch existing quirks. Just add a new one on top.

Copy link
Contributor

@jwrdegoede jwrdegoede Mar 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, so your quirk becomes:

#define I2C_HID_QUIRK_FORCE_TRIGGER_FALLING BIT(5)
and then put your device specific quirk before the match using ANY_ID so that it will be checked first, you device specific quirk should look something like this:

{ USB_VENDOR_ID_ELAN, I2C_PRODUCT_ID_ELAN_TOUCHPAD,
  I2C_HID_QUIRK_BOGUS_IRQ | I2C_HID_QUIRK_FORCE_TRIGGER_FALLING },

And this needs to be above:

	{ USB_VENDOR_ID_ELAN, HID_ANY_ID,
		 I2C_HID_QUIRK_BOGUS_IRQ },

Otherwise the HID_ANY_ID match will match first and the code looking up the quirks will stop there.

After fixing this please submit a new version of the patch to the mailinglist. Please Cc:
Hans de Goede [email protected] when submitting a new version.

Copy link
Author

@dalechyn dalechyn Mar 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, I will fix that.

#define I2C_HID_QUIRK_FORCE_TRIGGER_FALLING BIT(4)

/* flags */
#define I2C_HID_STARTED 0
Expand Down Expand Up @@ -182,8 +182,8 @@ static const struct i2c_hid_quirks {
I2C_HID_QUIRK_NO_RUNTIME_PM },
{ I2C_VENDOR_ID_GOODIX, I2C_DEVICE_ID_GOODIX_01F0,
I2C_HID_QUIRK_NO_RUNTIME_PM },
{ USB_VENDOR_ID_ELAN, HID_ANY_ID,
I2C_HID_QUIRK_BOGUS_IRQ },
{ USB_VENDOR_ID_ELAN, I2C_PRODUCT_ID_ELAN_TOUCHPAD,
Copy link
Contributor

@andy-shev andy-shev Mar 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should use I2C_VENDOR_ID_ELAN, but since there is an example of USB_VENDOR_ID_ELAN in use, it's okay for now.

I2C_HID_QUIRK_FORCE_TRIGGER_FALLING },
{ 0, 0 }
};

Expand Down Expand Up @@ -508,12 +508,6 @@ static void i2c_hid_get_input(struct i2c_hid *ihid)
return;
}

if (ihid->quirks & I2C_HID_QUIRK_BOGUS_IRQ && ret_size == 0xffff) {
dev_warn_once(&ihid->client->dev, "%s: IRQ triggered but "
"there's no data\n", __func__);
return;
}

if ((ret_size > size) || (ret_size < 2)) {
dev_err(&ihid->client->dev, "%s: incomplete report (%d/%d)\n",
__func__, size, ret_size);
Expand Down Expand Up @@ -854,6 +848,8 @@ static int i2c_hid_init_irq(struct i2c_client *client)

if (!irq_get_trigger_type(client->irq))
irqflags = IRQF_TRIGGER_LOW;
if (ihid->quirks & I2C_HID_QUIRK_FORCE_TRIGGER_FALLING)
irqflags = IRQF_TRIGGER_FALLING;

ret = request_threaded_irq(client->irq, NULL, i2c_hid_irq,
irqflags | IRQF_ONESHOT, client->name, ihid);
Expand Down Expand Up @@ -1123,10 +1119,6 @@ static int i2c_hid_probe(struct i2c_client *client,
if (ret < 0)
goto err_pm;

ret = i2c_hid_init_irq(client);
if (ret < 0)
goto err_pm;

hid = hid_allocate_device();
if (IS_ERR(hid)) {
ret = PTR_ERR(hid);
Expand All @@ -1149,6 +1141,10 @@ static int i2c_hid_probe(struct i2c_client *client,

ihid->quirks = i2c_hid_lookup_quirk(hid->vendor, hid->product);

ret = i2c_hid_init_irq(client);
if (ret < 0)
goto err_pm;

ret = hid_add_device(hid);
if (ret) {
if (ret != -ENODEV)
Expand Down