Re: [PATCH] ELAN touchpad i2c_hid bugs fix

[Hans de Goede]

H,

On 3/20/19 3:37 PM, Benjamin Tissoires wrote:

<snip>

> > The reason why i2c_hid_init_irq was moved is told by Hans De Goede
> > <hdegoede@xxxxxxxxxx>:
> > i2c_hid_init_irq now checks for a quirk, so we must setup the quirks before we init the irq, and we cannot setup the quirk earlier, so we must init the irq later.
>
> I am pretty sure you can paraphrase Hans in your commit message
> without having to formally quote him (Hans, please shout if you
> disagree).

Right, it is fine to explain why the irq init is moved without
quoting or even referring to me.

> > --- a/drivers/hid/hid-ids.h
> > +++ b/drivers/hid/hid-ids.h
> > @@ -389,6 +389,7 @@
> > #define USB_DEVICE_ID_TOSHIBA_CLICK_L9W 0x0401
> > #define USB_DEVICE_ID_HP_X2 0x074d
> > #define USB_DEVICE_ID_HP_X2_10_COVER 0x0755
> > +#define USB_DEVICE_ID_ELAN_TOUCHPAD 0x303e
>
> I know Hans wanted you to use USB here, but I'd think we should have a
> I2C_DEVICE_ID_*
> There is no guarantees that this same PID will be used in a different
> chip over USB.
> We tend to not car about USB/I2C for the vendor IDs: the vendors
> decided to reuse their USB VID, which makes our life easier.

Ok, no objection from me to use an I2C_DEVICE_ID prefix, here it
is after all an I2C device.

> > #define USB_VENDOR_ID_ELECOM 0x056e
> > #define USB_DEVICE_ID_ELECOM_BM084 0x0061
> > diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
> > index 90164fed08d3..16b55c45e2e8 100644
> > --- a/drivers/hid/i2c-hid/i2c-hid-core.c
> > +++ b/drivers/hid/i2c-hid/i2c-hid-core.c
> > @@ -50,7 +50,8 @@
> > #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)
> > +#define I2C_HID_QUIRK_BOGUS_IRQ BIT(4)
> > +#define I2C_HID_QUIRK_FORCE_TRIGGER_FALLING BIT(5)
> >
> > /* flags */
> > #define I2C_HID_STARTED 0
> > @@ -182,8 +183,10 @@ 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, USB_DEVICE_ID_ELAN_TOUCHPAD,
> > + I2C_HID_QUIRK_BOGUS_IRQ | I2C_HID_QUIRK_FORCE_TRIGGER_FALLING },
> > { USB_VENDOR_ID_ELAN, HID_ANY_ID,
> > - I2C_HID_QUIRK_BOGUS_IRQ },
> > + I2C_HID_QUIRK_BOGUS_IRQ },

Benjamin, what I find interesting here is that the BOGUS_IRQ quirk
is also used on Elan devices, I suspect that these Elan devices
likely also need the I2C_HID_QUIRK_FORCE_TRIGGER_FALLING quirk
and then they probably will no longer need the bogus IRQ flag,
if you know about bugreports with an acpidump for any of the devices
needing the bogus IRQ quirk, then I (or you) can check how the IRQ is
declared there, I suspect it will be declared as level-low, just like
with the laptop this patch was written for. And it probably need to
be edge-falling instead of level-low just like this case.

Regards,

Hans