* Re: [PATCH] ELAN touchpad i2c_hid bugs fix
[not found] <LaQHUFs--3-1@tutanota.com>
@ 2019-03-20 14:37 ` Benjamin Tissoires
2019-03-20 15:39 ` Hans de Goede
2019-03-20 17:11 ` Andy Shevchenko
[not found] ` <LaUpAlT--3-1@tutanota.com>
1 sibling, 2 replies; 34+ messages in thread
From: Benjamin Tissoires @ 2019-03-20 14:37 UTC (permalink / raw)
To: hotwater438
Cc: Jiri Kosina, Hans de Goede, Kai Heng Feng, Stephen Boyd, bigeasy,
Dmitry Torokhov, open list:HID CORE LAYER, lkml
Hi Vladislav,
I really appreciate the contribution, there are a few issues with the
formatting of the patch, thanks for taking care of those (I'll inline
the problems).
On Wed, Mar 20, 2019 at 2:28 PM <hotwater438@tutanota.com> wrote:
>
> Desciption: This patch add's additional quirk to force
> IRQ_TRIGGER_FALLING flag which resolves issues with ELAN1200:04F3:303E
> touchpad. Issues are the next:
> - Journalctl floods with next report:
> i2c_hid i2c-ELAN1200:00: i2c_hid_get_input: incomplete report (16/65535)
> - Five finger tap kills i2c_hid
> - When you scroll anything, and release one finger, driver thought you are still scrolling
You don't need to be that "formal" in the commit message.
The "description" part describes too much what the patch is (while we
can just read the code), the important part is the issues you are
fixing.
I'd like you to rewrite this in a way that doesn't feel like you are
writing a bug report.
For instance, you could write (but you can change this too):
---
The ELAN1200:04F3:303E touchpad exposes several issues, all caused by
an error setting the correct IRQ_TRIGGER flag:
- ...
- ...
- ...
Fix all of these with a new quirk that corrects the trigger flag
announced by the ACPI tables.
---
I know writing good commit message is a tricky part :)
>
> The reason why i2c_hid_init_irq was moved is told by Hans De Goede
> <hdegoede@redhat.com>:
> 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).
But this is a definitively good point to raise: your patch touches a
general path, so it might have unexpected issues if not carefully
reviewed by us.
>
>
> Patch is created by help of ELANTECH and RedHat guys (Hans De Goede <hdegoede@redhat.com>, Benjamin Tissoires <btissoir@redhat.com>, Andy Shevchenko <andy.shevchenko@gmail.com>) and me (Vladislav Dalechyn <hotwater438@tutanota.com>.
Don't take it wrong, but that's not how we give credit in a patch.
In your case, if you do not have any Elantech engineer who agreed to
sign off the patch, you can definitively mention them this way.
But the other guys (Hans, Andy and myself) are well known kernel
developers in the input and HID subsystems, so we usually take the
fame by adding our Reviewed-by or Signed-off-by tag at the end of the
commit message. (Also note that Andy is working for Intel ;-P )
If you feel that these persons have an equal participation in the
creation of the patch, you should ask for their Signed-off-by in the
patch. My gut feelings tell me they would gladly give a Reviewed-by
because they helped you on the patch, but they are not the "author",
so the Signed-off-by should be you only.
And yes, you should sign your work here saying that you wrote this
code in accordance to the Developer's Certificate of Origin 1.1
(https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst)
Then, either Jiri or myself will have a special role, and we will add
our own signed-of-by as the maintainers of the HID tree certifying
that this patch has been accepted by one of us.
>
> ---
> drivers/hid/hid-ids.h | 1 +
> drivers/hid/i2c-hid/i2c-hid-core.c | 17 +++++++++++------
> 2 files changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
> index b6d93f4ad037..dc44b5661669 100644
> --- 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.
>
> #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)
your mail client is playing with tabs and spaces, but I don't expect
the line above to be changed at all.
> +#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 },
same here, this line should not be changed.
> { 0, 0 }
> };
>
> @@ -854,6 +857,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;
again, indentation problems, tabs are missing
>
> ret = request_threaded_irq(client->irq, NULL, i2c_hid_irq,
> irqflags | IRQF_ONESHOT, client->name, ihid);
> @@ -1123,10 +1128,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);
the next call is "goto err_irq", which should be changed to "goto err_pm"
> @@ -1149,6 +1150,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;
this should be "err_mem_free"
> +
> ret = hid_add_device(hid);
> if (ret) {
> if (ret != -ENODEV)
next one should be "err_irq"
and the err_irq and err_mem_free should be inverted at the end of probe.
Cheers,
Benjamin
> --
> 2.20.1
>
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH] ELAN touchpad i2c_hid bugs fix
2019-03-20 14:37 ` [PATCH] ELAN touchpad i2c_hid bugs fix Benjamin Tissoires
@ 2019-03-20 15:39 ` Hans de Goede
2019-03-20 16:53 ` Kai-Heng Feng
2019-03-20 17:11 ` Andy Shevchenko
1 sibling, 1 reply; 34+ messages in thread
From: Hans de Goede @ 2019-03-20 15:39 UTC (permalink / raw)
To: Benjamin Tissoires, hotwater438
Cc: Jiri Kosina, Kai Heng Feng, Stephen Boyd, bigeasy,
Dmitry Torokhov, open list:HID CORE LAYER, lkml
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@redhat.com>:
>> 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
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH] ELAN touchpad i2c_hid bugs fix
2019-03-20 15:39 ` Hans de Goede
@ 2019-03-20 16:53 ` Kai-Heng Feng
2019-03-20 17:18 ` Andy Shevchenko
0 siblings, 1 reply; 34+ messages in thread
From: Kai-Heng Feng @ 2019-03-20 16:53 UTC (permalink / raw)
To: Hans de Goede
Cc: Benjamin Tissoires, hotwater438, Jiri Kosina, Stephen Boyd,
bigeasy, Dmitry Torokhov, open list:HID CORE LAYER, lkml
Hi Hans,
at 23:39, Hans de Goede <hdegoede@redhat.com> wrote:
> 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@redhat.com>:
>>> 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.
First, I’ve already tried using IRQF_TRIGGER_FALLING, unfortunately it
doesn’t solve the issue for me.
I talked to Elan once, and they confirm the correct IRQ trigger is level
low. So forcing falling trigger may break other platforms.
Recently we found that Elan touchpad doesn’t like GpioInt() from its _CRS.
Once the Interrupt() is used instead, the issue goes away.
But I am not sure how to patch its DSDT/SSDT in i2c-hid.
Kai-Heng
>
> Regards,
>
> Hans
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH] ELAN touchpad i2c_hid bugs fix
2019-03-20 16:53 ` Kai-Heng Feng
@ 2019-03-20 17:18 ` Andy Shevchenko
2019-03-21 4:08 ` Kai-Heng Feng
0 siblings, 1 reply; 34+ messages in thread
From: Andy Shevchenko @ 2019-03-20 17:18 UTC (permalink / raw)
To: Kai-Heng Feng
Cc: Hans de Goede, Benjamin Tissoires, hotwater438, Jiri Kosina,
Stephen Boyd, Sebastian Andrzej Siewior, Dmitry Torokhov,
open list:HID CORE LAYER, lkml
On Wed, Mar 20, 2019 at 6:55 PM Kai-Heng Feng
<kai.heng.feng@canonical.com> wrote:
> at 23:39, Hans de Goede <hdegoede@redhat.com> wrote:
> > On 3/20/19 3:37 PM, Benjamin Tissoires wrote:
> > 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.
>
> First, I’ve already tried using IRQF_TRIGGER_FALLING, unfortunately it
> doesn’t solve the issue for me.
>
> I talked to Elan once, and they confirm the correct IRQ trigger is level
> low. So forcing falling trigger may break other platforms.
As far as I understood Vladislav the quirk he got from Elan as well.
> Recently we found that Elan touchpad doesn’t like GpioInt() from its _CRS.
> Once the Interrupt() is used instead, the issue goes away.
IIRC i2c core tries to get interrupt from Interrupt() resource and
then falls back to GpioInt().
See i2c_acpi_get_info() and i2c_device_probe().
> But I am not sure how to patch its DSDT/SSDT in i2c-hid.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH] ELAN touchpad i2c_hid bugs fix
2019-03-20 17:18 ` Andy Shevchenko
@ 2019-03-21 4:08 ` Kai-Heng Feng
2019-03-21 8:55 ` Andy Shevchenko
` (2 more replies)
0 siblings, 3 replies; 34+ messages in thread
From: Kai-Heng Feng @ 2019-03-21 4:08 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Hans de Goede, Benjamin Tissoires, hotwater438, Jiri Kosina,
Stephen Boyd, Sebastian Andrzej Siewior, Dmitry Torokhov,
open list:HID CORE LAYER, lkml
at 01:18, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> On Wed, Mar 20, 2019 at 6:55 PM Kai-Heng Feng
> <kai.heng.feng@canonical.com> wrote:
>> at 23:39, Hans de Goede <hdegoede@redhat.com> wrote:
>>> On 3/20/19 3:37 PM, Benjamin Tissoires wrote:
>
>>> 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.
>>
>> First, I’ve already tried using IRQF_TRIGGER_FALLING, unfortunately it
>> doesn’t solve the issue for me.
>>
>> I talked to Elan once, and they confirm the correct IRQ trigger is level
>> low. So forcing falling trigger may break other platforms.
>
> As far as I understood Vladislav the quirk he got from Elan as well.
Ok, then this is really weird.
>
>> Recently we found that Elan touchpad doesn’t like GpioInt() from its _CRS.
>> Once the Interrupt() is used instead, the issue goes away.
>
> IIRC i2c core tries to get interrupt from Interrupt() resource and
> then falls back to GpioInt().
> See i2c_acpi_get_info() and i2c_device_probe().
Here’s its ASL:
Scope (\_SB.PCI0.I2C4)
{
Device (TPD0)
{
Name (_ADR, One) // _ADR: Address
Name (_HID, "DELL08AE") // _HID: Hardware ID
Name (_CID, "PNP0C50" /* HID Protocol Device (I2C bus) */) // _CID: Compatible ID
Name (_UID, One) // _UID: Unique ID
Name (_S0W, 0x04) // _S0W: S0 Device Wake State
Name (SBFB, ResourceTemplate ()
{
I2cSerialBusV2 (0x002C, ControllerInitiated, 0x00061A80,
AddressingMode7Bit, "\\_SB.PCI0.I2C4",
0x00, ResourceConsumer, , Exclusive,
)
})
Name (SBFG, ResourceTemplate ()
{
GpioInt (Level, ActiveLow, ExclusiveAndWake, PullUp, 0x0000,
"\\_SB.GPO1", 0x00, ResourceConsumer, ,
)
{ // Pin list
0x0012
}
})
Name (SBFI, ResourceTemplate ()
{
Interrupt (ResourceConsumer, Level, ActiveLow, ExclusiveAndWake, ,, )
{
0x0000003C,
}
})
Method (_INI, 0, NotSerialized) // _INI: Initialize
{
}
Method (_STA, 0, NotSerialized) // _STA: Status
{
If ((TCPD == One))
{
Return (0x0F)
}
Return (Zero)
}
Method (_CRS, 0, NotSerialized) // _CRS: Current Resource Settings
{
If ((OSYS < 0x07DC))
{
Return (SBFI) /* \_SB_.PCI0.I2C4.TPD0.SBFI */
}
Return (ConcatenateResTemplate (SBFB, SBFG))
}
Method (_DSM, 4, NotSerialized) // _DSM: Device-Specific Method
{
If ((Arg0 == ToUUID ("3cdff6f7-4267-4555-ad05-b30a3d8938de") /* HID I2C Device */))
{
If ((Arg2 == Zero))
{
If ((Arg1 == One))
{
Return (Buffer (One)
{
0x03 // .
})
}
Else
{
Return (Buffer (One)
{
0x00 // .
})
}
}
ElseIf ((Arg2 == One))
{
Return (0x20)
}
Else
{
Return (Buffer (One)
{
0x00 // .
})
}
}
ElseIf ((Arg0 == ToUUID ("ef87eb82-f951-46da-84ec-14871ac6f84b")))
{
If ((Arg2 == Zero))
{
If ((Arg1 == One))
{
Return (Buffer (One)
{
0x03 // .
})
}
}
If ((Arg2 == One))
{
Return (ConcatenateResTemplate (SBFB, SBFG))
}
Return (Buffer (One)
{
0x00 // .
})
}
}
Else
{
Return (Buffer (One)
{
0x00 // .
})
}
}
}
}
Change SBFG to SBFI in its _CRS can workaround the issue.
Is ASL in this form possible to do the flow you described?
Kai-Heng
>
>> But I am not sure how to patch its DSDT/SSDT in i2c-hid.
>
> --
> With Best Regards,
> Andy Shevchenko
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH] ELAN touchpad i2c_hid bugs fix
2019-03-21 4:08 ` Kai-Heng Feng
@ 2019-03-21 8:55 ` Andy Shevchenko
2019-03-21 9:28 ` Kai Heng Feng
2019-03-21 8:57 ` Hans de Goede
2019-03-21 9:48 ` Andy Shevchenko
2 siblings, 1 reply; 34+ messages in thread
From: Andy Shevchenko @ 2019-03-21 8:55 UTC (permalink / raw)
To: Kai-Heng Feng
Cc: Hans de Goede, Benjamin Tissoires, hotwater438, Jiri Kosina,
Stephen Boyd, Sebastian Andrzej Siewior, Dmitry Torokhov,
open list:HID CORE LAYER, lkml
On Thu, Mar 21, 2019 at 6:08 AM Kai-Heng Feng
<kai.heng.feng@canonical.com> wrote:
> at 01:18, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > On Wed, Mar 20, 2019 at 6:55 PM Kai-Heng Feng
> > <kai.heng.feng@canonical.com> wrote:
> >> at 23:39, Hans de Goede <hdegoede@redhat.com> wrote:
> >>> On 3/20/19 3:37 PM, Benjamin Tissoires wrote:
> >> Recently we found that Elan touchpad doesn’t like GpioInt() from its _CRS.
> >> Once the Interrupt() is used instead, the issue goes away.
> >
> > IIRC i2c core tries to get interrupt from Interrupt() resource and
> > then falls back to GpioInt().
> > See i2c_acpi_get_info() and i2c_device_probe().
>
> Here’s its ASL:
> Name (SBFB, ResourceTemplate ()
> {
> I2cSerialBusV2 (0x002C, ControllerInitiated, 0x00061A80,
> AddressingMode7Bit, "\\_SB.PCI0.I2C4",
> 0x00, ResourceConsumer, , Exclusive,
> )
> })
> Name (SBFG, ResourceTemplate ()
> {
> GpioInt (Level, ActiveLow, ExclusiveAndWake, PullUp, 0x0000,
> "\\_SB.GPO1", 0x00, ResourceConsumer, ,
> )
> { // Pin list
> 0x0012
> }
> })
> Name (SBFI, ResourceTemplate ()
> {
> Interrupt (ResourceConsumer, Level, ActiveLow, ExclusiveAndWake, ,, )
> {
> 0x0000003C,
> }
> })
> Method (_CRS, 0, NotSerialized) // _CRS: Current Resource Settings
> {
> If ((OSYS < 0x07DC))
> {
> Return (SBFI) /* \_SB_.PCI0.I2C4.TPD0.SBFI */
> }
This will return only Interrupt() resource
>
> Return (ConcatenateResTemplate (SBFB, SBFG))
This one I2cSerialBus() and GpioInt().
> }
> }
> }
>
> Change SBFG to SBFI in its _CRS can workaround the issue.
> Is ASL in this form possible to do the flow you described?
Since it's enumerated in Linux as I2C device, it means it gets I2C and
GPIO resources.
So, no, it's not possible.
What are you describing might tell us about one of the following:
- touchpad should be switched to PS/2 mode in order to get working
- GPIO resource is not correct / bug in GPIO driver
I don't believe the first one is a case here.
If GPIO resource is not correct and main OS has some quirks, we need
to do similar in Linux.
Otherwise, debugging GPIO driver, starting from what exactly (pin
number, pin settings, etc) gets i2c-hid module.
Note, ACPICA and related stuff is done in order to be Windows compatible.
If you have settings in BIOS that defines OS to boot, it should be
chosen Windows.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH] ELAN touchpad i2c_hid bugs fix
2019-03-21 8:55 ` Andy Shevchenko
@ 2019-03-21 9:28 ` Kai Heng Feng
0 siblings, 0 replies; 34+ messages in thread
From: Kai Heng Feng @ 2019-03-21 9:28 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Hans de Goede, Benjamin Tissoires, hotwater438, Jiri Kosina,
Stephen Boyd, Sebastian Andrzej Siewior, Dmitry Torokhov,
open list:HID CORE LAYER, lkml
> On Mar 21, 2019, at 4:55 PM, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>
> On Thu, Mar 21, 2019 at 6:08 AM Kai-Heng Feng
> <kai.heng.feng@canonical.com> wrote:
>> at 01:18, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>>> On Wed, Mar 20, 2019 at 6:55 PM Kai-Heng Feng
>>> <kai.heng.feng@canonical.com> wrote:
>>>> at 23:39, Hans de Goede <hdegoede@redhat.com> wrote:
>>>>> On 3/20/19 3:37 PM, Benjamin Tissoires wrote:
>
>>>> Recently we found that Elan touchpad doesn’t like GpioInt() from its _CRS.
>>>> Once the Interrupt() is used instead, the issue goes away.
>>>
>>> IIRC i2c core tries to get interrupt from Interrupt() resource and
>>> then falls back to GpioInt().
>>> See i2c_acpi_get_info() and i2c_device_probe().
>>
>> Here’s its ASL:
>
>> Name (SBFB, ResourceTemplate ()
>> {
>> I2cSerialBusV2 (0x002C, ControllerInitiated, 0x00061A80,
>> AddressingMode7Bit, "\\_SB.PCI0.I2C4",
>> 0x00, ResourceConsumer, , Exclusive,
>> )
>> })
>> Name (SBFG, ResourceTemplate ()
>> {
>> GpioInt (Level, ActiveLow, ExclusiveAndWake, PullUp, 0x0000,
>> "\\_SB.GPO1", 0x00, ResourceConsumer, ,
>> )
>> { // Pin list
>> 0x0012
>> }
>> })
>> Name (SBFI, ResourceTemplate ()
>> {
>> Interrupt (ResourceConsumer, Level, ActiveLow, ExclusiveAndWake, ,, )
>> {
>> 0x0000003C,
>> }
>> })
>
>> Method (_CRS, 0, NotSerialized) // _CRS: Current Resource Settings
>> {
>
>> If ((OSYS < 0x07DC))
>> {
>> Return (SBFI) /* \_SB_.PCI0.I2C4.TPD0.SBFI */
>> }
>
> This will return only Interrupt() resource
>
>>
>> Return (ConcatenateResTemplate (SBFB, SBFG))
>
> This one I2cSerialBus() and GpioInt().
>
>> }
>
>
>> }
>> }
>>
>> Change SBFG to SBFI in its _CRS can workaround the issue.
>> Is ASL in this form possible to do the flow you described?
>
> Since it's enumerated in Linux as I2C device, it means it gets I2C and
> GPIO resources.
> So, no, it's not possible.
>
> What are you describing might tell us about one of the following:
> - touchpad should be switched to PS/2 mode in order to get working
> - GPIO resource is not correct / bug in GPIO driver
>
> I don't believe the first one is a case here.
> If GPIO resource is not correct and main OS has some quirks, we need
> to do similar in Linux.
How do I check quirks on Windows?
> Otherwise, debugging GPIO driver, starting from what exactly (pin
> number, pin settings, etc) gets i2c-hid module.
Ok, please let me know where should I start looking into.
>
> Note, ACPICA and related stuff is done in order to be Windows compatible.
> If you have settings in BIOS that defines OS to boot, it should be
> chosen Windows.
Yes, it’s Windows.
Kai-Heng
>
> --
> With Best Regards,
> Andy Shevchenko
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH] ELAN touchpad i2c_hid bugs fix
2019-03-21 4:08 ` Kai-Heng Feng
2019-03-21 8:55 ` Andy Shevchenko
@ 2019-03-21 8:57 ` Hans de Goede
2019-03-21 9:48 ` Andy Shevchenko
2 siblings, 0 replies; 34+ messages in thread
From: Hans de Goede @ 2019-03-21 8:57 UTC (permalink / raw)
To: Kai-Heng Feng, Andy Shevchenko
Cc: Benjamin Tissoires, hotwater438, Jiri Kosina, Stephen Boyd,
Sebastian Andrzej Siewior, Dmitry Torokhov,
open list:HID CORE LAYER, lkml
Hi,
On 21-03-19 05:08, Kai-Heng Feng wrote:
> at 01:18, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>
>> On Wed, Mar 20, 2019 at 6:55 PM Kai-Heng Feng
>> <kai.heng.feng@canonical.com> wrote:
>>> at 23:39, Hans de Goede <hdegoede@redhat.com> wrote:
>>>> On 3/20/19 3:37 PM, Benjamin Tissoires wrote:
>>
>>>> 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.
>>>
>>> First, I’ve already tried using IRQF_TRIGGER_FALLING, unfortunately it
>>> doesn’t solve the issue for me.
>>>
>>> I talked to Elan once, and they confirm the correct IRQ trigger is level
>>> low. So forcing falling trigger may break other platforms.
>>
>> As far as I understood Vladislav the quirk he got from Elan as well.
>
> Ok, then this is really weird.
>
>>
>>> Recently we found that Elan touchpad doesn’t like GpioInt() from its _CRS.
>>> Once the Interrupt() is used instead, the issue goes away.
>>
>> IIRC i2c core tries to get interrupt from Interrupt() resource and
>> then falls back to GpioInt().
>> See i2c_acpi_get_info() and i2c_device_probe().
>
> Here’s its ASL:
>
> Scope (\_SB.PCI0.I2C4)
> {
> Device (TPD0)
> {
> Name (_ADR, One) // _ADR: Address
> Name (_HID, "DELL08AE") // _HID: Hardware ID
> Name (_CID, "PNP0C50" /* HID Protocol Device (I2C bus) */) // _CID: Compatible ID
> Name (_UID, One) // _UID: Unique ID
> Name (_S0W, 0x04) // _S0W: S0 Device Wake State
> Name (SBFB, ResourceTemplate ()
> {
> I2cSerialBusV2 (0x002C, ControllerInitiated, 0x00061A80,
> AddressingMode7Bit, "\\_SB.PCI0.I2C4",
> 0x00, ResourceConsumer, , Exclusive,
> )
> })
> Name (SBFG, ResourceTemplate ()
> {
> GpioInt (Level, ActiveLow, ExclusiveAndWake, PullUp, 0x0000,
> "\\_SB.GPO1", 0x00, ResourceConsumer, ,
> )
> { // Pin list
> 0x0012
> }
> })
> Name (SBFI, ResourceTemplate ()
> {
> Interrupt (ResourceConsumer, Level, ActiveLow, ExclusiveAndWake, ,, )
> {
> 0x0000003C,
> }
> })
OK, so both interrupt definitions declare the interrupt as Level, ActiveLow, so forcing
falling-edge here *might* help too.
Regards,
Hans
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH] ELAN touchpad i2c_hid bugs fix
2019-03-21 4:08 ` Kai-Heng Feng
2019-03-21 8:55 ` Andy Shevchenko
2019-03-21 8:57 ` Hans de Goede
@ 2019-03-21 9:48 ` Andy Shevchenko
2019-04-01 21:37 ` Mario.Limonciello
2 siblings, 1 reply; 34+ messages in thread
From: Andy Shevchenko @ 2019-03-21 9:48 UTC (permalink / raw)
To: Kai-Heng Feng, Mario Limonciello
Cc: Hans de Goede, Benjamin Tissoires, hotwater438, Jiri Kosina,
Stephen Boyd, Sebastian Andrzej Siewior, Dmitry Torokhov,
open list:HID CORE LAYER, lkml
+Cc: Mario
Mario, do you have any insights about the issue with touchpad on Dell
system described below?
On Thu, Mar 21, 2019 at 6:08 AM Kai-Heng Feng
<kai.heng.feng@canonical.com> wrote:
>
> at 01:18, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>
> > On Wed, Mar 20, 2019 at 6:55 PM Kai-Heng Feng
> > <kai.heng.feng@canonical.com> wrote:
> >> at 23:39, Hans de Goede <hdegoede@redhat.com> wrote:
> >>> On 3/20/19 3:37 PM, Benjamin Tissoires wrote:
> >
> >>> 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.
> >>
> >> First, I’ve already tried using IRQF_TRIGGER_FALLING, unfortunately it
> >> doesn’t solve the issue for me.
> >>
> >> I talked to Elan once, and they confirm the correct IRQ trigger is level
> >> low. So forcing falling trigger may break other platforms.
> >
> > As far as I understood Vladislav the quirk he got from Elan as well.
>
> Ok, then this is really weird.
>
> >
> >> Recently we found that Elan touchpad doesn’t like GpioInt() from its _CRS.
> >> Once the Interrupt() is used instead, the issue goes away.
> >
> > IIRC i2c core tries to get interrupt from Interrupt() resource and
> > then falls back to GpioInt().
> > See i2c_acpi_get_info() and i2c_device_probe().
>
> Here’s its ASL:
>
> Scope (\_SB.PCI0.I2C4)
> {
> Device (TPD0)
> {
> Name (_ADR, One) // _ADR: Address
> Name (_HID, "DELL08AE") // _HID: Hardware ID
> Name (_CID, "PNP0C50" /* HID Protocol Device (I2C bus) */) // _CID: Compatible ID
> Name (_UID, One) // _UID: Unique ID
> Name (_S0W, 0x04) // _S0W: S0 Device Wake State
> Name (SBFB, ResourceTemplate ()
> {
> I2cSerialBusV2 (0x002C, ControllerInitiated, 0x00061A80,
> AddressingMode7Bit, "\\_SB.PCI0.I2C4",
> 0x00, ResourceConsumer, , Exclusive,
> )
> })
> Name (SBFG, ResourceTemplate ()
> {
> GpioInt (Level, ActiveLow, ExclusiveAndWake, PullUp, 0x0000,
> "\\_SB.GPO1", 0x00, ResourceConsumer, ,
> )
> { // Pin list
> 0x0012
> }
> })
> Name (SBFI, ResourceTemplate ()
> {
> Interrupt (ResourceConsumer, Level, ActiveLow, ExclusiveAndWake, ,, )
> {
> 0x0000003C,
> }
> })
> Method (_INI, 0, NotSerialized) // _INI: Initialize
> {
> }
> Method (_STA, 0, NotSerialized) // _STA: Status
> {
> If ((TCPD == One))
> {
> Return (0x0F)
> }
>
> Return (Zero)
> }
> Method (_CRS, 0, NotSerialized) // _CRS: Current Resource Settings
> {
> If ((OSYS < 0x07DC))
> {
> Return (SBFI) /* \_SB_.PCI0.I2C4.TPD0.SBFI */
> }
>
> Return (ConcatenateResTemplate (SBFB, SBFG))
> }
> Method (_DSM, 4, NotSerialized) // _DSM: Device-Specific Method
> {
> If ((Arg0 == ToUUID ("3cdff6f7-4267-4555-ad05-b30a3d8938de") /* HID I2C Device */))
> {
> If ((Arg2 == Zero))
> {
> If ((Arg1 == One))
> {
> Return (Buffer (One)
> {
> 0x03 // .
> })
> }
> Else
> {
> Return (Buffer (One)
> {
> 0x00 // .
> })
> }
> }
> ElseIf ((Arg2 == One))
> {
> Return (0x20)
> }
> Else
> {
> Return (Buffer (One)
> {
> 0x00 // .
> })
> }
> }
> ElseIf ((Arg0 == ToUUID ("ef87eb82-f951-46da-84ec-14871ac6f84b")))
> {
> If ((Arg2 == Zero))
> {
> If ((Arg1 == One))
> {
> Return (Buffer (One)
> {
> 0x03 // .
> })
> }
> }
>
> If ((Arg2 == One))
> {
> Return (ConcatenateResTemplate (SBFB, SBFG))
> }
>
> Return (Buffer (One)
> {
> 0x00 // .
> })
> }
> }
> Else
> {
> Return (Buffer (One)
> {
> 0x00 // .
> })
> }
> }
> }
> }
>
> Change SBFG to SBFI in its _CRS can workaround the issue.
> Is ASL in this form possible to do the flow you described?
>
> Kai-Heng
>
> >
> >> But I am not sure how to patch its DSDT/SSDT in i2c-hid.
> >
> > --
> > With Best Regards,
> > Andy Shevchenko
>
>
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 34+ messages in thread* RE: [PATCH] ELAN touchpad i2c_hid bugs fix
2019-03-21 9:48 ` Andy Shevchenko
@ 2019-04-01 21:37 ` Mario.Limonciello
0 siblings, 0 replies; 34+ messages in thread
From: Mario.Limonciello @ 2019-04-01 21:37 UTC (permalink / raw)
To: andy.shevchenko, kai.heng.feng
Cc: hdegoede, benjamin.tissoires, hotwater438, jikos, swboyd,
bigeasy, dtor, linux-input, linux-kernel
> -----Original Message-----
> From: Andy Shevchenko <andy.shevchenko@gmail.com>
> Sent: Thursday, March 21, 2019 4:48 AM
> To: Kai-Heng Feng; Limonciello, Mario
> Cc: Hans de Goede; Benjamin Tissoires; hotwater438@tutanota.com; Jiri Kosina;
> Stephen Boyd; Sebastian Andrzej Siewior; Dmitry Torokhov; open list:HID CORE
> LAYER; lkml
> Subject: Re: [PATCH] ELAN touchpad i2c_hid bugs fix
>
>
> [EXTERNAL EMAIL]
>
> +Cc: Mario
>
> Mario, do you have any insights about the issue with touchpad on Dell
> system described below?
My apologies, this got lost while I was on vacation.
>
> On Thu, Mar 21, 2019 at 6:08 AM Kai-Heng Feng
> <kai.heng.feng@canonical.com> wrote:
> >
> > at 01:18, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> >
> > > On Wed, Mar 20, 2019 at 6:55 PM Kai-Heng Feng
> > > <kai.heng.feng@canonical.com> wrote:
> > >> at 23:39, Hans de Goede <hdegoede@redhat.com> wrote:
> > >>> On 3/20/19 3:37 PM, Benjamin Tissoires wrote:
> > >
> > >>> 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.
> > >>
> > >> First, I’ve already tried using IRQF_TRIGGER_FALLING, unfortunately it
> > >> doesn’t solve the issue for me.
> > >>
> > >> I talked to Elan once, and they confirm the correct IRQ trigger is level
> > >> low. So forcing falling trigger may break other platforms.
> > >
> > > As far as I understood Vladislav the quirk he got from Elan as well.
> >
> > Ok, then this is really weird.
> >
> > >
> > >> Recently we found that Elan touchpad doesn’t like GpioInt() from its _CRS.
> > >> Once the Interrupt() is used instead, the issue goes away.
> > >
> > > IIRC i2c core tries to get interrupt from Interrupt() resource and
> > > then falls back to GpioInt().
> > > See i2c_acpi_get_info() and i2c_device_probe().
> >
> > Here’s its ASL:
> >
> > Scope (\_SB.PCI0.I2C4)
> > {
> > Device (TPD0)
> > {
> > Name (_ADR, One) // _ADR: Address
> > Name (_HID, "DELL08AE") // _HID: Hardware ID
> > Name (_CID, "PNP0C50" /* HID Protocol Device (I2C bus) */) // _CID:
> Compatible ID
> > Name (_UID, One) // _UID: Unique ID
> > Name (_S0W, 0x04) // _S0W: S0 Device Wake State
> > Name (SBFB, ResourceTemplate ()
> > {
> > I2cSerialBusV2 (0x002C, ControllerInitiated, 0x00061A80,
> > AddressingMode7Bit, "\\_SB.PCI0.I2C4",
> > 0x00, ResourceConsumer, , Exclusive,
> > )
> > })
> > Name (SBFG, ResourceTemplate ()
> > {
> > GpioInt (Level, ActiveLow, ExclusiveAndWake, PullUp, 0x0000,
> > "\\_SB.GPO1", 0x00, ResourceConsumer, ,
> > )
> > { // Pin list
> > 0x0012
> > }
> > })
> > Name (SBFI, ResourceTemplate ()
> > {
> > Interrupt (ResourceConsumer, Level, ActiveLow, ExclusiveAndWake, ,, )
> > {
> > 0x0000003C,
> > }
> > })
> > Method (_INI, 0, NotSerialized) // _INI: Initialize
> > {
> > }
> > Method (_STA, 0, NotSerialized) // _STA: Status
> > {
> > If ((TCPD == One))
> > {
> > Return (0x0F)
> > }
> >
> > Return (Zero)
> > }
> > Method (_CRS, 0, NotSerialized) // _CRS: Current Resource Settings
> > {
> > If ((OSYS < 0x07DC))
> > {
> > Return (SBFI) /* \_SB_.PCI0.I2C4.TPD0.SBFI */
> > }
> >
> > Return (ConcatenateResTemplate (SBFB, SBFG))
> > }
> > Method (_DSM, 4, NotSerialized) // _DSM: Device-Specific Method
> > {
> > If ((Arg0 == ToUUID ("3cdff6f7-4267-4555-ad05-b30a3d8938de") /* HID
> I2C Device */))
> > {
> > If ((Arg2 == Zero))
> > {
> > If ((Arg1 == One))
> > {
> > Return (Buffer (One)
> > {
> > 0x03 // .
> > })
> > }
> > Else
> > {
> > Return (Buffer (One)
> > {
> > 0x00 // .
> > })
> > }
> > }
> > ElseIf ((Arg2 == One))
> > {
> > Return (0x20)
> > }
> > Else
> > {
> > Return (Buffer (One)
> > {
> > 0x00 // .
> > })
> > }
> > }
> > ElseIf ((Arg0 == ToUUID ("ef87eb82-f951-46da-84ec-14871ac6f84b")))
> > {
> > If ((Arg2 == Zero))
> > {
> > If ((Arg1 == One))
> > {
> > Return (Buffer (One)
> > {
> > 0x03 // .
> > })
> > }
> > }
> >
> > If ((Arg2 == One))
> > {
> > Return (ConcatenateResTemplate (SBFB, SBFG))
> > }
> >
> > Return (Buffer (One)
> > {
> > 0x00 // .
> > })
> > }
> > }
> > Else
> > {
> > Return (Buffer (One)
> > {
> > 0x00 // .
> > })
> > }
> > }
> > }
> > }
> >
> > Change SBFG to SBFI in its _CRS can workaround the issue.
> > Is ASL in this form possible to do the flow you described?
> >
> > Kai-Heng
> >
> > >
> > >> But I am not sure how to patch its DSDT/SSDT in i2c-hid.
Is this pre-production HW? If so, maybe this is a case that we should talk
about custom OSI string to run the ASL differently.
The other option would be to create a new ASL method in FW and from Linux
side a quirk that calls this new ASL method.
^ permalink raw reply [flat|nested] 34+ messages in thread* RE: [PATCH] ELAN touchpad i2c_hid bugs fix
@ 2019-04-01 21:37 ` Mario.Limonciello
0 siblings, 0 replies; 34+ messages in thread
From: Mario.Limonciello @ 2019-04-01 21:37 UTC (permalink / raw)
To: andy.shevchenko, kai.heng.feng
Cc: hdegoede, benjamin.tissoires, hotwater438, jikos, swboyd,
bigeasy, dtor, linux-input, linux-kernel
> -----Original Message-----
> From: Andy Shevchenko <andy.shevchenko@gmail.com>
> Sent: Thursday, March 21, 2019 4:48 AM
> To: Kai-Heng Feng; Limonciello, Mario
> Cc: Hans de Goede; Benjamin Tissoires; hotwater438@tutanota.com; Jiri Kosina;
> Stephen Boyd; Sebastian Andrzej Siewior; Dmitry Torokhov; open list:HID CORE
> LAYER; lkml
> Subject: Re: [PATCH] ELAN touchpad i2c_hid bugs fix
>
>
> [EXTERNAL EMAIL]
>
> +Cc: Mario
>
> Mario, do you have any insights about the issue with touchpad on Dell
> system described below?
My apologies, this got lost while I was on vacation.
>
> On Thu, Mar 21, 2019 at 6:08 AM Kai-Heng Feng
> <kai.heng.feng@canonical.com> wrote:
> >
> > at 01:18, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> >
> > > On Wed, Mar 20, 2019 at 6:55 PM Kai-Heng Feng
> > > <kai.heng.feng@canonical.com> wrote:
> > >> at 23:39, Hans de Goede <hdegoede@redhat.com> wrote:
> > >>> On 3/20/19 3:37 PM, Benjamin Tissoires wrote:
> > >
> > >>> 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.
> > >>
> > >> First, I’ve already tried using IRQF_TRIGGER_FALLING, unfortunately it
> > >> doesn’t solve the issue for me.
> > >>
> > >> I talked to Elan once, and they confirm the correct IRQ trigger is level
> > >> low. So forcing falling trigger may break other platforms.
> > >
> > > As far as I understood Vladislav the quirk he got from Elan as well.
> >
> > Ok, then this is really weird.
> >
> > >
> > >> Recently we found that Elan touchpad doesn’t like GpioInt() from its _CRS.
> > >> Once the Interrupt() is used instead, the issue goes away.
> > >
> > > IIRC i2c core tries to get interrupt from Interrupt() resource and
> > > then falls back to GpioInt().
> > > See i2c_acpi_get_info() and i2c_device_probe().
> >
> > Here’s its ASL:
> >
> > Scope (\_SB.PCI0.I2C4)
> > {
> > Device (TPD0)
> > {
> > Name (_ADR, One) // _ADR: Address
> > Name (_HID, "DELL08AE") // _HID: Hardware ID
> > Name (_CID, "PNP0C50" /* HID Protocol Device (I2C bus) */) // _CID:
> Compatible ID
> > Name (_UID, One) // _UID: Unique ID
> > Name (_S0W, 0x04) // _S0W: S0 Device Wake State
> > Name (SBFB, ResourceTemplate ()
> > {
> > I2cSerialBusV2 (0x002C, ControllerInitiated, 0x00061A80,
> > AddressingMode7Bit, "\\_SB.PCI0.I2C4",
> > 0x00, ResourceConsumer, , Exclusive,
> > )
> > })
> > Name (SBFG, ResourceTemplate ()
> > {
> > GpioInt (Level, ActiveLow, ExclusiveAndWake, PullUp, 0x0000,
> > "\\_SB.GPO1", 0x00, ResourceConsumer, ,
> > )
> > { // Pin list
> > 0x0012
> > }
> > })
> > Name (SBFI, ResourceTemplate ()
> > {
> > Interrupt (ResourceConsumer, Level, ActiveLow, ExclusiveAndWake, ,, )
> > {
> > 0x0000003C,
> > }
> > })
> > Method (_INI, 0, NotSerialized) // _INI: Initialize
> > {
> > }
> > Method (_STA, 0, NotSerialized) // _STA: Status
> > {
> > If ((TCPD == One))
> > {
> > Return (0x0F)
> > }
> >
> > Return (Zero)
> > }
> > Method (_CRS, 0, NotSerialized) // _CRS: Current Resource Settings
> > {
> > If ((OSYS < 0x07DC))
> > {
> > Return (SBFI) /* \_SB_.PCI0.I2C4.TPD0.SBFI */
> > }
> >
> > Return (ConcatenateResTemplate (SBFB, SBFG))
> > }
> > Method (_DSM, 4, NotSerialized) // _DSM: Device-Specific Method
> > {
> > If ((Arg0 == ToUUID ("3cdff6f7-4267-4555-ad05-b30a3d8938de") /* HID
> I2C Device */))
> > {
> > If ((Arg2 == Zero))
> > {
> > If ((Arg1 == One))
> > {
> > Return (Buffer (One)
> > {
> > 0x03 // .
> > })
> > }
> > Else
> > {
> > Return (Buffer (One)
> > {
> > 0x00 // .
> > })
> > }
> > }
> > ElseIf ((Arg2 == One))
> > {
> > Return (0x20)
> > }
> > Else
> > {
> > Return (Buffer (One)
> > {
> > 0x00 // .
> > })
> > }
> > }
> > ElseIf ((Arg0 == ToUUID ("ef87eb82-f951-46da-84ec-14871ac6f84b")))
> > {
> > If ((Arg2 == Zero))
> > {
> > If ((Arg1 == One))
> > {
> > Return (Buffer (One)
> > {
> > 0x03 // .
> > })
> > }
> > }
> >
> > If ((Arg2 == One))
> > {
> > Return (ConcatenateResTemplate (SBFB, SBFG))
> > }
> >
> > Return (Buffer (One)
> > {
> > 0x00 // .
> > })
> > }
> > }
> > Else
> > {
> > Return (Buffer (One)
> > {
> > 0x00 // .
> > })
> > }
> > }
> > }
> > }
> >
> > Change SBFG to SBFI in its _CRS can workaround the issue.
> > Is ASL in this form possible to do the flow you described?
> >
> > Kai-Heng
> >
> > >
> > >> But I am not sure how to patch its DSDT/SSDT in i2c-hid.
Is this pre-production HW? If so, maybe this is a case that we should talk
about custom OSI string to run the ASL differently.
The other option would be to create a new ASL method in FW and from Linux
side a quirk that calls this new ASL method.
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH] ELAN touchpad i2c_hid bugs fix
2019-04-01 21:37 ` Mario.Limonciello
(?)
@ 2019-04-02 4:18 ` Kai Heng Feng
2019-04-02 14:08 ` Mario.Limonciello
-1 siblings, 1 reply; 34+ messages in thread
From: Kai Heng Feng @ 2019-04-02 4:18 UTC (permalink / raw)
To: Mario.Limonciello
Cc: Andy Shevchenko, hdegoede, benjamin.tissoires, hotwater438,
jikos, swboyd, bigeasy, dtor, linux-input, linux-kernel
> On Apr 2, 2019, at 5:37 AM, <Mario.Limonciello@dell.com>
> <Mario.Limonciello@dell.com> wrote:
>
>> -----Original Message-----
>> From: Andy Shevchenko <andy.shevchenko@gmail.com>
>> Sent: Thursday, March 21, 2019 4:48 AM
>> To: Kai-Heng Feng; Limonciello, Mario
>> Cc: Hans de Goede; Benjamin Tissoires; hotwater438@tutanota.com; Jiri
>> Kosina;
>> Stephen Boyd; Sebastian Andrzej Siewior; Dmitry Torokhov; open list:HID
>> CORE
>> LAYER; lkml
>> Subject: Re: [PATCH] ELAN touchpad i2c_hid bugs fix
>>
>>
>> [EXTERNAL EMAIL]
>>
>> +Cc: Mario
>>
>> Mario, do you have any insights about the issue with touchpad on Dell
>> system described below?
>
> My apologies, this got lost while I was on vacation.
>
>>
>> On Thu, Mar 21, 2019 at 6:08 AM Kai-Heng Feng
>> <kai.heng.feng@canonical.com> wrote:
>>>
>>> at 01:18, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>>>
>>>> On Wed, Mar 20, 2019 at 6:55 PM Kai-Heng Feng
>>>> <kai.heng.feng@canonical.com> wrote:
>>>>> at 23:39, Hans de Goede <hdegoede@redhat.com> wrote:
>>>>>> On 3/20/19 3:37 PM, Benjamin Tissoires wrote:
>>>>
>>>>>> 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.
>>>>>
>>>>> First, I’ve already tried using IRQF_TRIGGER_FALLING, unfortunately it
>>>>> doesn’t solve the issue for me.
>>>>>
>>>>> I talked to Elan once, and they confirm the correct IRQ trigger is
>>>>> level
>>>>> low. So forcing falling trigger may break other platforms.
>>>>
>>>> As far as I understood Vladislav the quirk he got from Elan as well.
>>>
>>> Ok, then this is really weird.
>>>
>>>>
>>>>> Recently we found that Elan touchpad doesn’t like GpioInt() from its
>>>>> _CRS.
>>>>> Once the Interrupt() is used instead, the issue goes away.
>>>>
>>>> IIRC i2c core tries to get interrupt from Interrupt() resource and
>>>> then falls back to GpioInt().
>>>> See i2c_acpi_get_info() and i2c_device_probe().
>>>
>>> Here’s its ASL:
>>>
>>> Scope (\_SB.PCI0.I2C4)
>>> {
>>> Device (TPD0)
>>> {
>>> Name (_ADR, One) // _ADR: Address
>>> Name (_HID, "DELL08AE") // _HID: Hardware ID
>>> Name (_CID, "PNP0C50" /* HID Protocol Device (I2C bus) */) // _CID:
>> Compatible ID
>>> Name (_UID, One) // _UID: Unique ID
>>> Name (_S0W, 0x04) // _S0W: S0 Device Wake State
>>> Name (SBFB, ResourceTemplate ()
>>> {
>>> I2cSerialBusV2 (0x002C, ControllerInitiated, 0x00061A80,
>>> AddressingMode7Bit, "\\_SB.PCI0.I2C4",
>>> 0x00, ResourceConsumer, , Exclusive,
>>> )
>>> })
>>> Name (SBFG, ResourceTemplate ()
>>> {
>>> GpioInt (Level, ActiveLow, ExclusiveAndWake, PullUp, 0x0000,
>>> "\\_SB.GPO1", 0x00, ResourceConsumer, ,
>>> )
>>> { // Pin list
>>> 0x0012
>>> }
>>> })
>>> Name (SBFI, ResourceTemplate ()
>>> {
>>> Interrupt (ResourceConsumer, Level, ActiveLow, ExclusiveAndWake, ,, )
>>> {
>>> 0x0000003C,
>>> }
>>> })
>>> Method (_INI, 0, NotSerialized) // _INI: Initialize
>>> {
>>> }
>>> Method (_STA, 0, NotSerialized) // _STA: Status
>>> {
>>> If ((TCPD == One))
>>> {
>>> Return (0x0F)
>>> }
>>>
>>> Return (Zero)
>>> }
>>> Method (_CRS, 0, NotSerialized) // _CRS: Current Resource Settings
>>> {
>>> If ((OSYS < 0x07DC))
>>> {
>>> Return (SBFI) /* \_SB_.PCI0.I2C4.TPD0.SBFI */
>>> }
>>>
>>> Return (ConcatenateResTemplate (SBFB, SBFG))
>>> }
>>> Method (_DSM, 4, NotSerialized) // _DSM: Device-Specific Method
>>> {
>>> If ((Arg0 == ToUUID ("3cdff6f7-4267-4555-ad05-b30a3d8938de") /* HID
>> I2C Device */))
>>> {
>>> If ((Arg2 == Zero))
>>> {
>>> If ((Arg1 == One))
>>> {
>>> Return (Buffer (One)
>>> {
>>> 0x03 // .
>>> })
>>> }
>>> Else
>>> {
>>> Return (Buffer (One)
>>> {
>>> 0x00 // .
>>> })
>>> }
>>> }
>>> ElseIf ((Arg2 == One))
>>> {
>>> Return (0x20)
>>> }
>>> Else
>>> {
>>> Return (Buffer (One)
>>> {
>>> 0x00 // .
>>> })
>>> }
>>> }
>>> ElseIf ((Arg0 == ToUUID ("ef87eb82-f951-46da-84ec-14871ac6f84b")))
>>> {
>>> If ((Arg2 == Zero))
>>> {
>>> If ((Arg1 == One))
>>> {
>>> Return (Buffer (One)
>>> {
>>> 0x03 // .
>>> })
>>> }
>>> }
>>>
>>> If ((Arg2 == One))
>>> {
>>> Return (ConcatenateResTemplate (SBFB, SBFG))
>>> }
>>>
>>> Return (Buffer (One)
>>> {
>>> 0x00 // .
>>> })
>>> }
>>> }
>>> Else
>>> {
>>> Return (Buffer (One)
>>> {
>>> 0x00 // .
>>> })
>>> }
>>> }
>>> }
>>> }
>>>
>>> Change SBFG to SBFI in its _CRS can workaround the issue.
>>> Is ASL in this form possible to do the flow you described?
>>>
>>> Kai-Heng
>>>
>>>>
>>>>> But I am not sure how to patch its DSDT/SSDT in i2c-hid.
>
> Is this pre-production HW? If so, maybe this is a case that we should talk
> about custom OSI string to run the ASL differently.
Some are already shipped.
>
> The other option would be to create a new ASL method in FW and from Linux
> side a quirk that calls this new ASL method.
>
Since this patch is for ASUS Laptop, I think a more generic solution from
driver layer is preferred.
Kai-Heng
^ permalink raw reply [flat|nested] 34+ messages in thread* RE: [PATCH] ELAN touchpad i2c_hid bugs fix
2019-04-02 4:18 ` Kai Heng Feng
@ 2019-04-02 14:08 ` Mario.Limonciello
0 siblings, 0 replies; 34+ messages in thread
From: Mario.Limonciello @ 2019-04-02 14:08 UTC (permalink / raw)
To: kai.heng.feng
Cc: andy.shevchenko, hdegoede, benjamin.tissoires, hotwater438,
jikos, swboyd, bigeasy, dtor, linux-input, linux-kernel
> -----Original Message-----
> From: Kai Heng Feng <kai.heng.feng@canonical.com>
> Sent: Monday, April 1, 2019 11:18 PM
> To: Limonciello, Mario
> Cc: Andy Shevchenko; hdegoede@redhat.com; benjamin.tissoires@redhat.com;
> hotwater438@tutanota.com; jikos@kernel.org; swboyd@chromium.org;
> bigeasy@linutronix.de; dtor@chromium.org; linux-input@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH] ELAN touchpad i2c_hid bugs fix
>
>
> [EXTERNAL EMAIL]
>
>
>
> > On Apr 2, 2019, at 5:37 AM, <Mario.Limonciello@dell.com>
> > <Mario.Limonciello@dell.com> wrote:
> >
> >> -----Original Message-----
> >> From: Andy Shevchenko <andy.shevchenko@gmail.com>
> >> Sent: Thursday, March 21, 2019 4:48 AM
> >> To: Kai-Heng Feng; Limonciello, Mario
> >> Cc: Hans de Goede; Benjamin Tissoires; hotwater438@tutanota.com; Jiri
> >> Kosina;
> >> Stephen Boyd; Sebastian Andrzej Siewior; Dmitry Torokhov; open list:HID
> >> CORE
> >> LAYER; lkml
> >> Subject: Re: [PATCH] ELAN touchpad i2c_hid bugs fix
> >>
> >>
> >> [EXTERNAL EMAIL]
> >>
> >> +Cc: Mario
> >>
> >> Mario, do you have any insights about the issue with touchpad on Dell
> >> system described below?
> >
> > My apologies, this got lost while I was on vacation.
> >
> >>
> >> On Thu, Mar 21, 2019 at 6:08 AM Kai-Heng Feng
> >> <kai.heng.feng@canonical.com> wrote:
> >>>
> >>> at 01:18, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> >>>
> >>>> On Wed, Mar 20, 2019 at 6:55 PM Kai-Heng Feng
> >>>> <kai.heng.feng@canonical.com> wrote:
> >>>>> at 23:39, Hans de Goede <hdegoede@redhat.com> wrote:
> >>>>>> On 3/20/19 3:37 PM, Benjamin Tissoires wrote:
> >>>>
> >>>>>> 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.
> >>>>>
> >>>>> First, I’ve already tried using IRQF_TRIGGER_FALLING, unfortunately it
> >>>>> doesn’t solve the issue for me.
> >>>>>
> >>>>> I talked to Elan once, and they confirm the correct IRQ trigger is
> >>>>> level
> >>>>> low. So forcing falling trigger may break other platforms.
> >>>>
> >>>> As far as I understood Vladislav the quirk he got from Elan as well.
> >>>
> >>> Ok, then this is really weird.
> >>>
> >>>>
> >>>>> Recently we found that Elan touchpad doesn’t like GpioInt() from its
> >>>>> _CRS.
> >>>>> Once the Interrupt() is used instead, the issue goes away.
> >>>>
> >>>> IIRC i2c core tries to get interrupt from Interrupt() resource and
> >>>> then falls back to GpioInt().
> >>>> See i2c_acpi_get_info() and i2c_device_probe().
> >>>
> >>> Here’s its ASL:
> >>>
> >>> Scope (\_SB.PCI0.I2C4)
> >>> {
> >>> Device (TPD0)
> >>> {
> >>> Name (_ADR, One) // _ADR: Address
> >>> Name (_HID, "DELL08AE") // _HID: Hardware ID
> >>> Name (_CID, "PNP0C50" /* HID Protocol Device (I2C bus) */) // _CID:
> >> Compatible ID
> >>> Name (_UID, One) // _UID: Unique ID
> >>> Name (_S0W, 0x04) // _S0W: S0 Device Wake State
> >>> Name (SBFB, ResourceTemplate ()
> >>> {
> >>> I2cSerialBusV2 (0x002C, ControllerInitiated, 0x00061A80,
> >>> AddressingMode7Bit, "\\_SB.PCI0.I2C4",
> >>> 0x00, ResourceConsumer, , Exclusive,
> >>> )
> >>> })
> >>> Name (SBFG, ResourceTemplate ()
> >>> {
> >>> GpioInt (Level, ActiveLow, ExclusiveAndWake, PullUp, 0x0000,
> >>> "\\_SB.GPO1", 0x00, ResourceConsumer, ,
> >>> )
> >>> { // Pin list
> >>> 0x0012
> >>> }
> >>> })
> >>> Name (SBFI, ResourceTemplate ()
> >>> {
> >>> Interrupt (ResourceConsumer, Level, ActiveLow, ExclusiveAndWake, ,, )
> >>> {
> >>> 0x0000003C,
> >>> }
> >>> })
> >>> Method (_INI, 0, NotSerialized) // _INI: Initialize
> >>> {
> >>> }
> >>> Method (_STA, 0, NotSerialized) // _STA: Status
> >>> {
> >>> If ((TCPD == One))
> >>> {
> >>> Return (0x0F)
> >>> }
> >>>
> >>> Return (Zero)
> >>> }
> >>> Method (_CRS, 0, NotSerialized) // _CRS: Current Resource Settings
> >>> {
> >>> If ((OSYS < 0x07DC))
> >>> {
> >>> Return (SBFI) /* \_SB_.PCI0.I2C4.TPD0.SBFI */
> >>> }
> >>>
> >>> Return (ConcatenateResTemplate (SBFB, SBFG))
> >>> }
> >>> Method (_DSM, 4, NotSerialized) // _DSM: Device-Specific Method
> >>> {
> >>> If ((Arg0 == ToUUID ("3cdff6f7-4267-4555-ad05-b30a3d8938de") /*
> HID
> >> I2C Device */))
> >>> {
> >>> If ((Arg2 == Zero))
> >>> {
> >>> If ((Arg1 == One))
> >>> {
> >>> Return (Buffer (One)
> >>> {
> >>> 0x03 // .
> >>> })
> >>> }
> >>> Else
> >>> {
> >>> Return (Buffer (One)
> >>> {
> >>> 0x00 // .
> >>> })
> >>> }
> >>> }
> >>> ElseIf ((Arg2 == One))
> >>> {
> >>> Return (0x20)
> >>> }
> >>> Else
> >>> {
> >>> Return (Buffer (One)
> >>> {
> >>> 0x00 // .
> >>> })
> >>> }
> >>> }
> >>> ElseIf ((Arg0 == ToUUID ("ef87eb82-f951-46da-84ec-14871ac6f84b")))
> >>> {
> >>> If ((Arg2 == Zero))
> >>> {
> >>> If ((Arg1 == One))
> >>> {
> >>> Return (Buffer (One)
> >>> {
> >>> 0x03 // .
> >>> })
> >>> }
> >>> }
> >>>
> >>> If ((Arg2 == One))
> >>> {
> >>> Return (ConcatenateResTemplate (SBFB, SBFG))
> >>> }
> >>>
> >>> Return (Buffer (One)
> >>> {
> >>> 0x00 // .
> >>> })
> >>> }
> >>> }
> >>> Else
> >>> {
> >>> Return (Buffer (One)
> >>> {
> >>> 0x00 // .
> >>> })
> >>> }
> >>> }
> >>> }
> >>> }
> >>>
> >>> Change SBFG to SBFI in its _CRS can workaround the issue.
> >>> Is ASL in this form possible to do the flow you described?
> >>>
> >>> Kai-Heng
> >>>
> >>>>
> >>>>> But I am not sure how to patch its DSDT/SSDT in i2c-hid.
> >
> > Is this pre-production HW? If so, maybe this is a case that we should talk
> > about custom OSI string to run the ASL differently.
>
> Some are already shipped.
>
>
> >
> > The other option would be to create a new ASL method in FW and from Linux
> > side a quirk that calls this new ASL method.
> >
>
> Since this patch is for ASUS Laptop, I think a more generic solution from
> driver layer is preferred.
>
I thought this ASL was from Dell laptop. The HID make it looks like it at least.
> >>> Name (_HID, "DELL08AE") // _HID: Hardware ID
In general more generic solution from driver layer is preferred I agree. You might
need to check with ACPI guys to see if they have some ideas on situations that
GpioInt fail.
^ permalink raw reply [flat|nested] 34+ messages in thread* RE: [PATCH] ELAN touchpad i2c_hid bugs fix
@ 2019-04-02 14:08 ` Mario.Limonciello
0 siblings, 0 replies; 34+ messages in thread
From: Mario.Limonciello @ 2019-04-02 14:08 UTC (permalink / raw)
To: kai.heng.feng
Cc: andy.shevchenko, hdegoede, benjamin.tissoires, hotwater438,
jikos, swboyd, bigeasy, dtor, linux-input, linux-kernel
> -----Original Message-----
> From: Kai Heng Feng <kai.heng.feng@canonical.com>
> Sent: Monday, April 1, 2019 11:18 PM
> To: Limonciello, Mario
> Cc: Andy Shevchenko; hdegoede@redhat.com; benjamin.tissoires@redhat.com;
> hotwater438@tutanota.com; jikos@kernel.org; swboyd@chromium.org;
> bigeasy@linutronix.de; dtor@chromium.org; linux-input@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH] ELAN touchpad i2c_hid bugs fix
>
>
> [EXTERNAL EMAIL]
>
>
>
> > On Apr 2, 2019, at 5:37 AM, <Mario.Limonciello@dell.com>
> > <Mario.Limonciello@dell.com> wrote:
> >
> >> -----Original Message-----
> >> From: Andy Shevchenko <andy.shevchenko@gmail.com>
> >> Sent: Thursday, March 21, 2019 4:48 AM
> >> To: Kai-Heng Feng; Limonciello, Mario
> >> Cc: Hans de Goede; Benjamin Tissoires; hotwater438@tutanota.com; Jiri
> >> Kosina;
> >> Stephen Boyd; Sebastian Andrzej Siewior; Dmitry Torokhov; open list:HID
> >> CORE
> >> LAYER; lkml
> >> Subject: Re: [PATCH] ELAN touchpad i2c_hid bugs fix
> >>
> >>
> >> [EXTERNAL EMAIL]
> >>
> >> +Cc: Mario
> >>
> >> Mario, do you have any insights about the issue with touchpad on Dell
> >> system described below?
> >
> > My apologies, this got lost while I was on vacation.
> >
> >>
> >> On Thu, Mar 21, 2019 at 6:08 AM Kai-Heng Feng
> >> <kai.heng.feng@canonical.com> wrote:
> >>>
> >>> at 01:18, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> >>>
> >>>> On Wed, Mar 20, 2019 at 6:55 PM Kai-Heng Feng
> >>>> <kai.heng.feng@canonical.com> wrote:
> >>>>> at 23:39, Hans de Goede <hdegoede@redhat.com> wrote:
> >>>>>> On 3/20/19 3:37 PM, Benjamin Tissoires wrote:
> >>>>
> >>>>>> 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.
> >>>>>
> >>>>> First, I’ve already tried using IRQF_TRIGGER_FALLING, unfortunately it
> >>>>> doesn’t solve the issue for me.
> >>>>>
> >>>>> I talked to Elan once, and they confirm the correct IRQ trigger is
> >>>>> level
> >>>>> low. So forcing falling trigger may break other platforms.
> >>>>
> >>>> As far as I understood Vladislav the quirk he got from Elan as well.
> >>>
> >>> Ok, then this is really weird.
> >>>
> >>>>
> >>>>> Recently we found that Elan touchpad doesn’t like GpioInt() from its
> >>>>> _CRS.
> >>>>> Once the Interrupt() is used instead, the issue goes away.
> >>>>
> >>>> IIRC i2c core tries to get interrupt from Interrupt() resource and
> >>>> then falls back to GpioInt().
> >>>> See i2c_acpi_get_info() and i2c_device_probe().
> >>>
> >>> Here’s its ASL:
> >>>
> >>> Scope (\_SB.PCI0.I2C4)
> >>> {
> >>> Device (TPD0)
> >>> {
> >>> Name (_ADR, One) // _ADR: Address
> >>> Name (_HID, "DELL08AE") // _HID: Hardware ID
> >>> Name (_CID, "PNP0C50" /* HID Protocol Device (I2C bus) */) // _CID:
> >> Compatible ID
> >>> Name (_UID, One) // _UID: Unique ID
> >>> Name (_S0W, 0x04) // _S0W: S0 Device Wake State
> >>> Name (SBFB, ResourceTemplate ()
> >>> {
> >>> I2cSerialBusV2 (0x002C, ControllerInitiated, 0x00061A80,
> >>> AddressingMode7Bit, "\\_SB.PCI0.I2C4",
> >>> 0x00, ResourceConsumer, , Exclusive,
> >>> )
> >>> })
> >>> Name (SBFG, ResourceTemplate ()
> >>> {
> >>> GpioInt (Level, ActiveLow, ExclusiveAndWake, PullUp, 0x0000,
> >>> "\\_SB.GPO1", 0x00, ResourceConsumer, ,
> >>> )
> >>> { // Pin list
> >>> 0x0012
> >>> }
> >>> })
> >>> Name (SBFI, ResourceTemplate ()
> >>> {
> >>> Interrupt (ResourceConsumer, Level, ActiveLow, ExclusiveAndWake, ,, )
> >>> {
> >>> 0x0000003C,
> >>> }
> >>> })
> >>> Method (_INI, 0, NotSerialized) // _INI: Initialize
> >>> {
> >>> }
> >>> Method (_STA, 0, NotSerialized) // _STA: Status
> >>> {
> >>> If ((TCPD == One))
> >>> {
> >>> Return (0x0F)
> >>> }
> >>>
> >>> Return (Zero)
> >>> }
> >>> Method (_CRS, 0, NotSerialized) // _CRS: Current Resource Settings
> >>> {
> >>> If ((OSYS < 0x07DC))
> >>> {
> >>> Return (SBFI) /* \_SB_.PCI0.I2C4.TPD0.SBFI */
> >>> }
> >>>
> >>> Return (ConcatenateResTemplate (SBFB, SBFG))
> >>> }
> >>> Method (_DSM, 4, NotSerialized) // _DSM: Device-Specific Method
> >>> {
> >>> If ((Arg0 == ToUUID ("3cdff6f7-4267-4555-ad05-b30a3d8938de") /*
> HID
> >> I2C Device */))
> >>> {
> >>> If ((Arg2 == Zero))
> >>> {
> >>> If ((Arg1 == One))
> >>> {
> >>> Return (Buffer (One)
> >>> {
> >>> 0x03 // .
> >>> })
> >>> }
> >>> Else
> >>> {
> >>> Return (Buffer (One)
> >>> {
> >>> 0x00 // .
> >>> })
> >>> }
> >>> }
> >>> ElseIf ((Arg2 == One))
> >>> {
> >>> Return (0x20)
> >>> }
> >>> Else
> >>> {
> >>> Return (Buffer (One)
> >>> {
> >>> 0x00 // .
> >>> })
> >>> }
> >>> }
> >>> ElseIf ((Arg0 == ToUUID ("ef87eb82-f951-46da-84ec-14871ac6f84b")))
> >>> {
> >>> If ((Arg2 == Zero))
> >>> {
> >>> If ((Arg1 == One))
> >>> {
> >>> Return (Buffer (One)
> >>> {
> >>> 0x03 // .
> >>> })
> >>> }
> >>> }
> >>>
> >>> If ((Arg2 == One))
> >>> {
> >>> Return (ConcatenateResTemplate (SBFB, SBFG))
> >>> }
> >>>
> >>> Return (Buffer (One)
> >>> {
> >>> 0x00 // .
> >>> })
> >>> }
> >>> }
> >>> Else
> >>> {
> >>> Return (Buffer (One)
> >>> {
> >>> 0x00 // .
> >>> })
> >>> }
> >>> }
> >>> }
> >>> }
> >>>
> >>> Change SBFG to SBFI in its _CRS can workaround the issue.
> >>> Is ASL in this form possible to do the flow you described?
> >>>
> >>> Kai-Heng
> >>>
> >>>>
> >>>>> But I am not sure how to patch its DSDT/SSDT in i2c-hid.
> >
> > Is this pre-production HW? If so, maybe this is a case that we should talk
> > about custom OSI string to run the ASL differently.
>
> Some are already shipped.
>
>
> >
> > The other option would be to create a new ASL method in FW and from Linux
> > side a quirk that calls this new ASL method.
> >
>
> Since this patch is for ASUS Laptop, I think a more generic solution from
> driver layer is preferred.
>
I thought this ASL was from Dell laptop. The HID make it looks like it at least.
> >>> Name (_HID, "DELL08AE") // _HID: Hardware ID
In general more generic solution from driver layer is preferred I agree. You might
need to check with ACPI guys to see if they have some ideas on situations that
GpioInt fail.
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH] ELAN touchpad i2c_hid bugs fix
2019-04-02 14:08 ` Mario.Limonciello
(?)
@ 2019-04-03 9:24 ` Kai-Heng Feng
-1 siblings, 0 replies; 34+ messages in thread
From: Kai-Heng Feng @ 2019-04-03 9:24 UTC (permalink / raw)
To: Mario.Limonciello
Cc: Andy Shevchenko, Hans de Goede, benjamin.tissoires, hotwater438,
jikos, swboyd, bigeasy, dtor, linux-input, linux-kernel
at 22:08, <Mario.Limonciello@dell.com> <Mario.Limonciello@dell.com> wrote:
>> -----Original Message-----
>> From: Kai Heng Feng <kai.heng.feng@canonical.com>
>> Sent: Monday, April 1, 2019 11:18 PM
>> To: Limonciello, Mario
>> Cc: Andy Shevchenko; hdegoede@redhat.com; benjamin.tissoires@redhat.com;
>> hotwater438@tutanota.com; jikos@kernel.org; swboyd@chromium.org;
>> bigeasy@linutronix.de; dtor@chromium.org; linux-input@vger.kernel.org;
>> linux-
>> kernel@vger.kernel.org
>> Subject: Re: [PATCH] ELAN touchpad i2c_hid bugs fix
>>
>>
>> [EXTERNAL EMAIL]
>>
>>
>>
>>> On Apr 2, 2019, at 5:37 AM, <Mario.Limonciello@dell.com>
>>> <Mario.Limonciello@dell.com> wrote:
>>>
>>>> -----Original Message-----
>>>> From: Andy Shevchenko <andy.shevchenko@gmail.com>
>>>> Sent: Thursday, March 21, 2019 4:48 AM
>>>> To: Kai-Heng Feng; Limonciello, Mario
>>>> Cc: Hans de Goede; Benjamin Tissoires; hotwater438@tutanota.com; Jiri
>>>> Kosina;
>>>> Stephen Boyd; Sebastian Andrzej Siewior; Dmitry Torokhov; open list:HID
>>>> CORE
>>>> LAYER; lkml
>>>> Subject: Re: [PATCH] ELAN touchpad i2c_hid bugs fix
>>>>
>>>>
>>>> [EXTERNAL EMAIL]
>>>>
>>>> +Cc: Mario
>>>>
>>>> Mario, do you have any insights about the issue with touchpad on Dell
>>>> system described below?
>>>
>>> My apologies, this got lost while I was on vacation.
>>>
>>>> On Thu, Mar 21, 2019 at 6:08 AM Kai-Heng Feng
>>>> <kai.heng.feng@canonical.com> wrote:
>>>>> at 01:18, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>>>>>
>>>>>> On Wed, Mar 20, 2019 at 6:55 PM Kai-Heng Feng
>>>>>> <kai.heng.feng@canonical.com> wrote:
>>>>>>> at 23:39, Hans de Goede <hdegoede@redhat.com> wrote:
>>>>>>>> On 3/20/19 3:37 PM, Benjamin Tissoires wrote:
>>>>>>
>>>>>>>> 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.
>>>>>>>
>>>>>>> First, I’ve already tried using IRQF_TRIGGER_FALLING, unfortunately
>>>>>>> it
>>>>>>> doesn’t solve the issue for me.
>>>>>>>
>>>>>>> I talked to Elan once, and they confirm the correct IRQ trigger is
>>>>>>> level
>>>>>>> low. So forcing falling trigger may break other platforms.
>>>>>>
>>>>>> As far as I understood Vladislav the quirk he got from Elan as well.
>>>>>
>>>>> Ok, then this is really weird.
>>>>>
>>>>>>> Recently we found that Elan touchpad doesn’t like GpioInt() from its
>>>>>>> _CRS.
>>>>>>> Once the Interrupt() is used instead, the issue goes away.
>>>>>>
>>>>>> IIRC i2c core tries to get interrupt from Interrupt() resource and
>>>>>> then falls back to GpioInt().
>>>>>> See i2c_acpi_get_info() and i2c_device_probe().
>>>>>
>>>>> Here’s its ASL:
>>>>>
>>>>> Scope (\_SB.PCI0.I2C4)
>>>>> {
>>>>> Device (TPD0)
>>>>> {
>>>>> Name (_ADR, One) // _ADR: Address
>>>>> Name (_HID, "DELL08AE") // _HID: Hardware ID
>>>>> Name (_CID, "PNP0C50" /* HID Protocol Device (I2C bus) */) // _CID:
>>>> Compatible ID
>>>>> Name (_UID, One) // _UID: Unique ID
>>>>> Name (_S0W, 0x04) // _S0W: S0 Device Wake State
>>>>> Name (SBFB, ResourceTemplate ()
>>>>> {
>>>>> I2cSerialBusV2 (0x002C, ControllerInitiated, 0x00061A80,
>>>>> AddressingMode7Bit, "\\_SB.PCI0.I2C4",
>>>>> 0x00, ResourceConsumer, , Exclusive,
>>>>> )
>>>>> })
>>>>> Name (SBFG, ResourceTemplate ()
>>>>> {
>>>>> GpioInt (Level, ActiveLow, ExclusiveAndWake, PullUp, 0x0000,
>>>>> "\\_SB.GPO1", 0x00, ResourceConsumer, ,
>>>>> )
>>>>> { // Pin list
>>>>> 0x0012
>>>>> }
>>>>> })
>>>>> Name (SBFI, ResourceTemplate ()
>>>>> {
>>>>> Interrupt (ResourceConsumer, Level, ActiveLow, ExclusiveAndWake, ,, )
>>>>> {
>>>>> 0x0000003C,
>>>>> }
>>>>> })
>>>>> Method (_INI, 0, NotSerialized) // _INI: Initialize
>>>>> {
>>>>> }
>>>>> Method (_STA, 0, NotSerialized) // _STA: Status
>>>>> {
>>>>> If ((TCPD == One))
>>>>> {
>>>>> Return (0x0F)
>>>>> }
>>>>>
>>>>> Return (Zero)
>>>>> }
>>>>> Method (_CRS, 0, NotSerialized) // _CRS: Current Resource Settings
>>>>> {
>>>>> If ((OSYS < 0x07DC))
>>>>> {
>>>>> Return (SBFI) /* \_SB_.PCI0.I2C4.TPD0.SBFI */
>>>>> }
>>>>>
>>>>> Return (ConcatenateResTemplate (SBFB, SBFG))
>>>>> }
>>>>> Method (_DSM, 4, NotSerialized) // _DSM: Device-Specific Method
>>>>> {
>>>>> If ((Arg0 == ToUUID ("3cdff6f7-4267-4555-ad05-b30a3d8938de") /*
>> HID
>>>> I2C Device */))
>>>>> {
>>>>> If ((Arg2 == Zero))
>>>>> {
>>>>> If ((Arg1 == One))
>>>>> {
>>>>> Return (Buffer (One)
>>>>> {
>>>>> 0x03 // .
>>>>> })
>>>>> }
>>>>> Else
>>>>> {
>>>>> Return (Buffer (One)
>>>>> {
>>>>> 0x00 // .
>>>>> })
>>>>> }
>>>>> }
>>>>> ElseIf ((Arg2 == One))
>>>>> {
>>>>> Return (0x20)
>>>>> }
>>>>> Else
>>>>> {
>>>>> Return (Buffer (One)
>>>>> {
>>>>> 0x00 // .
>>>>> })
>>>>> }
>>>>> }
>>>>> ElseIf ((Arg0 == ToUUID ("ef87eb82-f951-46da-84ec-14871ac6f84b")))
>>>>> {
>>>>> If ((Arg2 == Zero))
>>>>> {
>>>>> If ((Arg1 == One))
>>>>> {
>>>>> Return (Buffer (One)
>>>>> {
>>>>> 0x03 // .
>>>>> })
>>>>> }
>>>>> }
>>>>>
>>>>> If ((Arg2 == One))
>>>>> {
>>>>> Return (ConcatenateResTemplate (SBFB, SBFG))
>>>>> }
>>>>>
>>>>> Return (Buffer (One)
>>>>> {
>>>>> 0x00 // .
>>>>> })
>>>>> }
>>>>> }
>>>>> Else
>>>>> {
>>>>> Return (Buffer (One)
>>>>> {
>>>>> 0x00 // .
>>>>> })
>>>>> }
>>>>> }
>>>>> }
>>>>> }
>>>>>
>>>>> Change SBFG to SBFI in its _CRS can workaround the issue.
>>>>> Is ASL in this form possible to do the flow you described?
>>>>>
>>>>> Kai-Heng
>>>>>
>>>>>>> But I am not sure how to patch its DSDT/SSDT in i2c-hid.
>>>
>>> Is this pre-production HW? If so, maybe this is a case that we should
>>> talk
>>> about custom OSI string to run the ASL differently.
>>
>> Some are already shipped.
>>
>>
>>> The other option would be to create a new ASL method in FW and from Linux
>>> side a quirk that calls this new ASL method.
>>
>> Since this patch is for ASUS Laptop, I think a more generic solution from
>> driver layer is preferred.
>
> I thought this ASL was from Dell laptop. The HID make it looks like it
> at least.
Yes this ASL is from a Dell system.
>>>>> Name (_HID, "DELL08AE") // _HID: Hardware ID
>
> In general more generic solution from driver layer is preferred I agree.
> You might
> need to check with ACPI guys to see if they have some ideas on situations
> that
> GpioInt fail.
I’ve filed a bug here but we don’t find any proper solution:
https://bugzilla.kernel.org/show_bug.cgi?id=201311
Kai-Heng
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH] ELAN touchpad i2c_hid bugs fix
2019-03-20 14:37 ` [PATCH] ELAN touchpad i2c_hid bugs fix Benjamin Tissoires
2019-03-20 15:39 ` Hans de Goede
@ 2019-03-20 17:11 ` Andy Shevchenko
1 sibling, 0 replies; 34+ messages in thread
From: Andy Shevchenko @ 2019-03-20 17:11 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: hotwater438, Jiri Kosina, Hans de Goede, Kai Heng Feng,
Stephen Boyd, Sebastian Andrzej Siewior, Dmitry Torokhov,
open list:HID CORE LAYER, lkml
On Wed, Mar 20, 2019 at 4:38 PM Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
> On Wed, Mar 20, 2019 at 2:28 PM <hotwater438@tutanota.com> wrote:
> > Patch is created by help of ELANTECH and RedHat guys (Hans De Goede <hdegoede@redhat.com>, Benjamin Tissoires <btissoir@redhat.com>, Andy Shevchenko <andy.shevchenko@gmail.com>) and me (Vladislav Dalechyn <hotwater438@tutanota.com>.
>
> Don't take it wrong, but that's not how we give credit in a patch.
> In your case, if you do not have any Elantech engineer who agreed to
> sign off the patch, you can definitively mention them this way.
> But the other guys (Hans, Andy and myself) are well known kernel
> developers in the input and HID subsystems, so we usually take the
> fame by adding our Reviewed-by or Signed-off-by tag at the end of the
> commit message. (Also note that Andy is working for Intel ;-P )
>
> If you feel that these persons have an equal participation in the
> creation of the patch, you should ask for their Signed-off-by in the
> patch. My gut feelings tell me they would gladly give a Reviewed-by
> because they helped you on the patch, but they are not the "author",
> so the Signed-off-by should be you only.
Just in case, we have Co-developed-by: tag and for my opinion Hans'
name should go under this category.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 34+ messages in thread
[parent not found: <LaUpAlT--3-1@tutanota.com>]
* [PATCH] ELAN touchpad i2c_hid bugs fix
@ 2019-03-24 19:10 Vladislav Dalechyn
2019-03-25 9:23 ` Benjamin Tissoires
0 siblings, 1 reply; 34+ messages in thread
From: Vladislav Dalechyn @ 2019-03-24 19:10 UTC (permalink / raw)
To: jikos, benjamin.tissoires, kai.heng.feng, swboyd, bigeasy, dtor,
linux-input, linux-kernel
Cc: h0tw4t3r
From: h0tw4t3r <hotwater438@tutanota.com>
Description: The ELAN1200:04F3:303E touchpad exposes several issues, all
caused by an error setting the correct IRQ_TRIGGER flag:
- i2c_hid incoplete error flood in journalctl;
- Five finger tap kill's module so you have to restart it;
- Two finger scoll is working incorrect and sometimes even when you
raised one of two finger still thinks that you are scrolling
Fix all of these with a new quirk that corrects the trigger flag
announced by the ACPI tables. (edge-falling).
Reason behind moving i2c_hid_init_irq section described below:
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.
Signed-off-by: Vladislav Dalechyn <hotwater438@tutanota.com>
---
drivers/hid/hid-ids.h | 1 +
drivers/hid/i2c-hid/i2c-hid-core.c | 25 +++++++++++++++----------
2 files changed, 16 insertions(+), 10 deletions(-)
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index b6d93f4ad037..660b4e0e912e 100644
--- 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 I2C_DEVICE_ID_ELAN_TOUCHPAD 0x303e
#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..9b417914411f 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -51,6 +51,7 @@
#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_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, I2C_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 },
{ 0, 0 }
};
@@ -854,6 +857,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);
@@ -1123,14 +1128,10 @@ 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);
- goto err_irq;
+ goto err_pm;
}
ihid->hid = hid;
@@ -1149,11 +1150,15 @@ 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_mem_free;
+
ret = hid_add_device(hid);
if (ret) {
if (ret != -ENODEV)
hid_err(client, "can't add hid device: %d\n", ret);
- goto err_mem_free;
+ goto err_irq;
}
if (!(ihid->quirks & I2C_HID_QUIRK_NO_RUNTIME_PM))
@@ -1161,12 +1166,12 @@ static int i2c_hid_probe(struct i2c_client *client,
return 0;
+err_irq:
+ free_irq(client->irq, ihid);
+
err_mem_free:
hid_destroy_device(hid);
-err_irq:
- free_irq(client->irq, ihid);
-
err_pm:
pm_runtime_put_noidle(&client->dev);
pm_runtime_disable(&client->dev);
--
2.20.1
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [PATCH] ELAN touchpad i2c_hid bugs fix
2019-03-24 19:10 Vladislav Dalechyn
@ 2019-03-25 9:23 ` Benjamin Tissoires
0 siblings, 0 replies; 34+ messages in thread
From: Benjamin Tissoires @ 2019-03-25 9:23 UTC (permalink / raw)
To: Vladislav Dalechyn
Cc: Jiri Kosina, Kai Heng Feng, Stephen Boyd, bigeasy,
Dmitry Torokhov, open list:HID CORE LAYER, lkml, h0tw4t3r
Hi Vladislav,
we are almost there.
When submitting/applying a patch, we do run ./script/checkpatch.pl on
the final patch.
This gives us here 2 warnings (inlined below)
Also, can you versionize your submissions (use `-v` in git
format-patch: `git format-patch -v3 HEAD`).
On Sun, Mar 24, 2019 at 8:10 PM Vladislav Dalechyn
<vlad.dalechin@gmail.com> wrote:
>
> From: h0tw4t3r <hotwater438@tutanota.com>
checkpatch.pl complains here:
WARNING: Missing Signed-off-by: line by nominal patch author 'h0tw4t3r
<hotwater438@tutanota.com>'
Either:
- change your git settings and reset the author (git commit --amend
--reset-author)
- just drop this "From:" from the patch from git format-patch
>
> Description: The ELAN1200:04F3:303E touchpad exposes several issues, all
> caused by an error setting the correct IRQ_TRIGGER flag:
> - i2c_hid incoplete error flood in journalctl;
> - Five finger tap kill's module so you have to restart it;
> - Two finger scoll is working incorrect and sometimes even when you
> raised one of two finger still thinks that you are scrolling
>
> Fix all of these with a new quirk that corrects the trigger flag
> announced by the ACPI tables. (edge-falling).
>
> Reason behind moving i2c_hid_init_irq section described below:
> 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.
>
> Signed-off-by: Vladislav Dalechyn <hotwater438@tutanota.com>
IIRC Andy said Co-developped-by: Hans de Goede <...> here. Does that stand Hans?
> ---
> drivers/hid/hid-ids.h | 1 +
> drivers/hid/i2c-hid/i2c-hid-core.c | 25 +++++++++++++++----------
> 2 files changed, 16 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
> index b6d93f4ad037..660b4e0e912e 100644
> --- 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 I2C_DEVICE_ID_ELAN_TOUCHPAD 0x303e
>
> #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..9b417914411f 100644
> --- a/drivers/hid/i2c-hid/i2c-hid-core.c
> +++ b/drivers/hid/i2c-hid/i2c-hid-core.c
> @@ -51,6 +51,7 @@
> #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_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, I2C_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 },
> { 0, 0 }
> };
>
> @@ -854,6 +857,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);
> @@ -1123,14 +1128,10 @@ 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);
> - goto err_irq;
> + goto err_pm;
> }
>
> ihid->hid = hid;
> @@ -1149,11 +1150,15 @@ 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_mem_free;
> +
> ret = hid_add_device(hid);
> if (ret) {
> if (ret != -ENODEV)
> hid_err(client, "can't add hid device: %d\n", ret);
> - goto err_mem_free;
> + goto err_irq;
> }
>
> if (!(ihid->quirks & I2C_HID_QUIRK_NO_RUNTIME_PM))
> @@ -1161,12 +1166,12 @@ static int i2c_hid_probe(struct i2c_client *client,
>
> return 0;
>
> +err_irq:
> + free_irq(client->irq, ihid);
> +
WARNING: please, no spaces at the start of a line
#112: FILE: drivers/hid/i2c-hid/i2c-hid-core.c:1170:
+ free_irq(client->irq, ihid);$
Thanks for fixing these 3 minor issues, and we will be able to merge it :)
Cheers,
Benjamin
> err_mem_free:
> hid_destroy_device(hid);
>
> -err_irq:
> - free_irq(client->irq, ihid);
> -
> err_pm:
> pm_runtime_put_noidle(&client->dev);
> pm_runtime_disable(&client->dev);
> --
> 2.20.1
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH] ELAN touchpad i2c_hid bugs fix
@ 2019-03-25 12:57 Vladislav Dalechyn
2019-03-25 16:02 ` Dmitry Torokhov
0 siblings, 1 reply; 34+ messages in thread
From: Vladislav Dalechyn @ 2019-03-25 12:57 UTC (permalink / raw)
To: benjamin.tissoires, jikos, kai.heng.feng, swboyd, bigeasy, dtor,
linux-input, linux-kernel, hotwater438
Cc: Hans De Goede
From: Vladislav Dalechyn <hotwater438@tutanota.com>
Description: The ELAN1200:04F3:303E touchpad exposes several issues, all
caused by an error setting the correct IRQ_TRIGGER flag:
- i2c_hid incoplete error flood in journalctl;
- Five finger tap kill's module so you have to restart it;
- Two finger scoll is working incorrect and sometimes even when you
raised one of two finger still thinks that you are scrolling.
Fix all of these with a new quirk that corrects the trigger flag
announced by the ACPI tables. (edge-falling).
Reason behind moving i2c_hid_init_irq section described below:
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.
Co-developed-by: Hans De Goede <hdegoede@redhat.com>
Signed-off-by: Vladislav Dalechyn <hotwater438@tutanota.com>
---
drivers/hid/hid-ids.h | 1 +
drivers/hid/i2c-hid/i2c-hid-core.c | 25 +++++++++++++++----------
2 files changed, 16 insertions(+), 10 deletions(-)
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index b6d93f4ad037..660b4e0e912e 100644
--- 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 I2C_DEVICE_ID_ELAN_TOUCHPAD 0x303e
#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..9b417914411f 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -51,6 +51,7 @@
#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_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, I2C_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 },
{ 0, 0 }
};
@@ -854,6 +857,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);
@@ -1123,14 +1128,10 @@ 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);
- goto err_irq;
+ goto err_pm;
}
ihid->hid = hid;
@@ -1149,11 +1150,15 @@ 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_mem_free;
+
ret = hid_add_device(hid);
if (ret) {
if (ret != -ENODEV)
hid_err(client, "can't add hid device: %d\n", ret);
- goto err_mem_free;
+ goto err_irq;
}
if (!(ihid->quirks & I2C_HID_QUIRK_NO_RUNTIME_PM))
@@ -1161,12 +1166,12 @@ static int i2c_hid_probe(struct i2c_client *client,
return 0;
+err_irq:
+ free_irq(client->irq, ihid);
+
err_mem_free:
hid_destroy_device(hid);
-err_irq:
- free_irq(client->irq, ihid);
-
err_pm:
pm_runtime_put_noidle(&client->dev);
pm_runtime_disable(&client->dev);
--
2.20.1
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [PATCH] ELAN touchpad i2c_hid bugs fix
2019-03-25 12:57 Vladislav Dalechyn
@ 2019-03-25 16:02 ` Dmitry Torokhov
2019-03-25 16:38 ` Hans de Goede
0 siblings, 1 reply; 34+ messages in thread
From: Dmitry Torokhov @ 2019-03-25 16:02 UTC (permalink / raw)
To: Vladislav Dalechyn
Cc: Benjamin Tissoires, Jiri Kosina, kai.heng.feng, swboyd, bigeasy,
open list:HID CORE LAYER, lkml, hotwater438, Hans De Goede
Hi Vladislav,
On Mon, Mar 25, 2019 at 5:57 AM Vladislav Dalechyn
<vlad.dalechin@gmail.com> wrote:
>
> From: Vladislav Dalechyn <hotwater438@tutanota.com>
>
> Description: The ELAN1200:04F3:303E touchpad exposes several issues, all
> caused by an error setting the correct IRQ_TRIGGER flag:
> - i2c_hid incoplete error flood in journalctl;
> - Five finger tap kill's module so you have to restart it;
> - Two finger scoll is working incorrect and sometimes even when you
> raised one of two finger still thinks that you are scrolling.
>
> Fix all of these with a new quirk that corrects the trigger flag
> announced by the ACPI tables. (edge-falling).
I do not believe this is right solution. The driver makes liberal use
of disable_irq() and enable_irq() which may lead to lost edges and
touchpad stopping working altogether.
Usually the "extra" report is caused by GPIO controller clearing
interrupt condition at the wrong time (too early), or in unsafe or
racy fashion. You need to look there instead of adding quirk to
i2c-hid.
Thanks.
--
Dmitry
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH] ELAN touchpad i2c_hid bugs fix
2019-03-25 16:02 ` Dmitry Torokhov
@ 2019-03-25 16:38 ` Hans de Goede
2019-03-25 16:56 ` Dmitry Torokhov
0 siblings, 1 reply; 34+ messages in thread
From: Hans de Goede @ 2019-03-25 16:38 UTC (permalink / raw)
To: Dmitry Torokhov, Vladislav Dalechyn
Cc: Benjamin Tissoires, Jiri Kosina, kai.heng.feng, swboyd, bigeasy,
open list:HID CORE LAYER, lkml, hotwater438
Hi Dmitry,
On 25-03-19 17:02, Dmitry Torokhov wrote:
> Hi Vladislav,
>
> On Mon, Mar 25, 2019 at 5:57 AM Vladislav Dalechyn
> <vlad.dalechin@gmail.com> wrote:
>>
>> From: Vladislav Dalechyn <hotwater438@tutanota.com>
>>
>> Description: The ELAN1200:04F3:303E touchpad exposes several issues, all
>> caused by an error setting the correct IRQ_TRIGGER flag:
>> - i2c_hid incoplete error flood in journalctl;
>> - Five finger tap kill's module so you have to restart it;
>> - Two finger scoll is working incorrect and sometimes even when you
>> raised one of two finger still thinks that you are scrolling.
>>
>> Fix all of these with a new quirk that corrects the trigger flag
>> announced by the ACPI tables. (edge-falling).
>
> I do not believe this is right solution. The driver makes liberal use
> of disable_irq() and enable_irq() which may lead to lost edges and
> touchpad stopping working altogether.
>
> Usually the "extra" report is caused by GPIO controller clearing
> interrupt condition at the wrong time (too early), or in unsafe or
> racy fashion. You need to look there instead of adding quirk to
> i2c-hid.
The falling-edge solution was proposed by Elan themselves.
Also if you look at: https://bugzilla.redhat.com/show_bug.cgi?id=1543769
And esp. the "cat /proc/interrupts" output there, then you will see
that the interrupt seems to be stuck at low level, which according
to the ACPI tables is its active level.
As for this being a GPIO chip driver problem, this is using standard
Intel pinctrl stuff, which is not showing this same issue with many
other i2c-hid touchpads.
Regards,
Hans
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH] ELAN touchpad i2c_hid bugs fix
2019-03-25 16:38 ` Hans de Goede
@ 2019-03-25 16:56 ` Dmitry Torokhov
[not found] ` <Laq4ykv--3-1@tutanota.com>
2019-03-29 12:18 ` Hans de Goede
0 siblings, 2 replies; 34+ messages in thread
From: Dmitry Torokhov @ 2019-03-25 16:56 UTC (permalink / raw)
To: Hans de Goede
Cc: Vladislav Dalechyn, Benjamin Tissoires, Jiri Kosina,
kai.heng.feng, swboyd, bigeasy, open list:HID CORE LAYER, lkml,
hotwater438
Hi Hans,
On Mon, Mar 25, 2019 at 9:38 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi Dmitry,
>
> On 25-03-19 17:02, Dmitry Torokhov wrote:
> > Hi Vladislav,
> >
> > On Mon, Mar 25, 2019 at 5:57 AM Vladislav Dalechyn
> > <vlad.dalechin@gmail.com> wrote:
> >>
> >> From: Vladislav Dalechyn <hotwater438@tutanota.com>
> >>
> >> Description: The ELAN1200:04F3:303E touchpad exposes several issues, all
> >> caused by an error setting the correct IRQ_TRIGGER flag:
> >> - i2c_hid incoplete error flood in journalctl;
> >> - Five finger tap kill's module so you have to restart it;
> >> - Two finger scoll is working incorrect and sometimes even when you
> >> raised one of two finger still thinks that you are scrolling.
> >>
> >> Fix all of these with a new quirk that corrects the trigger flag
> >> announced by the ACPI tables. (edge-falling).
> >
> > I do not believe this is right solution. The driver makes liberal use
> > of disable_irq() and enable_irq() which may lead to lost edges and
> > touchpad stopping working altogether.
> >
> > Usually the "extra" report is caused by GPIO controller clearing
> > interrupt condition at the wrong time (too early), or in unsafe or
> > racy fashion. You need to look there instead of adding quirk to
> > i2c-hid.
>
> The falling-edge solution was proposed by Elan themselves.
>
> Also if you look at: https://bugzilla.redhat.com/show_bug.cgi?id=1543769
>
> And esp. the "cat /proc/interrupts" output there, then you will see
> that the interrupt seems to be stuck at low level, which according
> to the ACPI tables is its active level.
So how does it generate a new edge if it is stuck at low?
Is it bad touchpad firmware that does not deassert interrupt quickly
enough? I scrolled through the bug but I do not see if it had been
confirmed that original windows installation actually uses edge (it
may very well be using it; Elan engineers pushed us to use edge in a
few cases, but they all boiled down to an issue with pin control/GPIO
implementation).
>
> As for this being a GPIO chip driver problem, this is using standard
> Intel pinctrl stuff, which is not showing this same issue with many
> other i2c-hid touchpads.
Well, there have been plenty of issues in intel drivers, coupled with
"interesting" things done by firmware and boards.
If you want to keep on using edge you need to make sure that i2c-hid
never loses edge, as replaying of previously disabled interrupts in
not at all reliable. So you need to "kick" the device after
enable_irq() by initiating read from it and be ready to not get any
data or get valid data and process accordingly.
Thanks.
--
Dmitry
^ permalink raw reply [flat|nested] 34+ messages in thread[parent not found: <Laq4ykv--3-1@tutanota.com>]
* Re: [PATCH] ELAN touchpad i2c_hid bugs fix
[not found] ` <Laq4ykv--3-1@tutanota.com>
@ 2019-03-25 18:30 ` Dmitry Torokhov
0 siblings, 0 replies; 34+ messages in thread
From: Dmitry Torokhov @ 2019-03-25 18:30 UTC (permalink / raw)
To: hotwater438
Cc: Hans de Goede, Vladislav Dalechyn, Benjamin Tissoires,
Jiri Kosina, Kai Heng Feng, Swboyd, Bigeasy,
open list:HID CORE LAYER, lkml
On Mon, Mar 25, 2019 at 11:23 AM <hotwater438@tutanota.com> wrote:
>
> Hi.
>
> Mar 25, 2019, 6:56 PM by dtor@chromium.org:
>
> If you want to keep on using edge you need to make sure that i2c-hid
> never loses edge, as replaying of previously disabled interrupts in
> not at all reliable. So you need to "kick" the device after
> enable_irq() by initiating read from it and be ready to not get any
> data or get valid data and process accordingly.
>
> I'm sorry, but how edge can be loosed? There is device ID and quirk's which are associated with that ID, and quirk will always trigger edge irq.
If edge arrives while interrupt is disabled and replay of the
interrupt does not work when it is reenabled (and I do not think it
ever works for GPIO, you can hunt for Thomas Gleixner emails to that
effect on LKML), then ISR in the driver will never be called, so
driver will never read from the device to reset the interrupt
condition and interrupt will never be reasserted again -> edge lost.
Thanks.
--
Dmitry
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH] ELAN touchpad i2c_hid bugs fix
2019-03-25 16:56 ` Dmitry Torokhov
[not found] ` <Laq4ykv--3-1@tutanota.com>
@ 2019-03-29 12:18 ` Hans de Goede
2019-03-29 18:23 ` Dmitry Torokhov
[not found] ` <LbI7kio--3-1@tutanota.com>
1 sibling, 2 replies; 34+ messages in thread
From: Hans de Goede @ 2019-03-29 12:18 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Vladislav Dalechyn, Benjamin Tissoires, Jiri Kosina,
kai.heng.feng, swboyd, bigeasy, open list:HID CORE LAYER, lkml,
hotwater438
Hi,
On 3/25/19 5:56 PM, Dmitry Torokhov wrote:
> Hi Hans,
>
> On Mon, Mar 25, 2019 at 9:38 AM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Hi Dmitry,
>>
>> On 25-03-19 17:02, Dmitry Torokhov wrote:
>>> Hi Vladislav,
>>>
>>> On Mon, Mar 25, 2019 at 5:57 AM Vladislav Dalechyn
>>> <vlad.dalechin@gmail.com> wrote:
>>>>
>>>> From: Vladislav Dalechyn <hotwater438@tutanota.com>
>>>>
>>>> Description: The ELAN1200:04F3:303E touchpad exposes several issues, all
>>>> caused by an error setting the correct IRQ_TRIGGER flag:
>>>> - i2c_hid incoplete error flood in journalctl;
>>>> - Five finger tap kill's module so you have to restart it;
>>>> - Two finger scoll is working incorrect and sometimes even when you
>>>> raised one of two finger still thinks that you are scrolling.
>>>>
>>>> Fix all of these with a new quirk that corrects the trigger flag
>>>> announced by the ACPI tables. (edge-falling).
>>>
>>> I do not believe this is right solution. The driver makes liberal use
>>> of disable_irq() and enable_irq() which may lead to lost edges and
>>> touchpad stopping working altogether.
>>>
>>> Usually the "extra" report is caused by GPIO controller clearing
>>> interrupt condition at the wrong time (too early), or in unsafe or
>>> racy fashion. You need to look there instead of adding quirk to
>>> i2c-hid.
>>
>> The falling-edge solution was proposed by Elan themselves.
>>
>> Also if you look at: https://bugzilla.redhat.com/show_bug.cgi?id=1543769
>>
>> And esp. the "cat /proc/interrupts" output there, then you will see
>> that the interrupt seems to be stuck at low level, which according
>> to the ACPI tables is its active level.
>
> So how does it generate a new edge if it is stuck at low?
>
> Is it bad touchpad firmware that does not deassert interrupt quickly
> enough?
I do not believe it is not de-asserting it quick enough (I believe
the amount of interrups is too high for that.
It seems to simply be low most of the time, or it is really really
slow with de-asserting.
Vladislav can you check the output of /cat/interrupts on a kernel
without the patch and while *not* using the touchpad; and check
if the amount of touchpads-interrupts still keeps increasing in this
case?
Also I believe that you had contact with Elan about this and they
told you to change the interrupt type to falling-edge as work-around,
right? Can you ask them why?
This is quite unususal, I've collected quite a few DSDTs over time
and I've just checked about 40 of them all with a PNP0C50 in
some form (and in many cases multiple such devices) and NONE of
them is using edged-interrupts in the ACPI config.
<speculation>
I think that the Elan touchpad firmware supports a mode to
work on devices which only support edge interrupts and that
this mode is accidentally enabled in this firmware.
I think that the interrupt line is simply low all the time
and gets pulsed high then low again when the touchpad detects
a finger. Hopefully it does this pulsing on every event and not
only when its event "fifo" is empty.
</speculation>
> I scrolled through the bug but I do not see if it had been
> confirmed that original windows installation actually uses edge (it
> may very well be using it; Elan engineers pushed us to use edge in a
> few cases, but they all boiled down to an issue with pin control/GPIO
> implementation).
This has not been checked on Windows AFAIK.
>> As for this being a GPIO chip driver problem, this is using standard
>> Intel pinctrl stuff, which is not showing this same issue with many
>> other i2c-hid touchpads.
>
> Well, there have been plenty of issues in intel drivers, coupled with
> "interesting" things done by firmware and boards.
>
> If you want to keep on using edge you need to make sure that i2c-hid
> never loses edge, as replaying of previously disabled interrupts in
> not at all reliable. So you need to "kick" the device after
> enable_irq() by initiating read from it and be ready to not get any
> data or get valid data and process accordingly.
That is a good point and I agree.
Vladislav, let me explain this a bit. Normally the touchpad
driver the IRQ line low when it has touch-data to respond, which means
that if touch-data is reported before the driver loads (or while
the driver has the irq disabled during e.g. suspend), it will immediately
see an interrupt. If we use edge mode then the IRQ will only trigger
when the IRQ line goes from high to low, if this happens when the driver
is not listening then we do not see the edge. And since we never read the
pending touch-data, the IRQ line never goes high again (which it does
when we have read all available data), so we will never see a negative-edge
and then things are stuck.
It would be good, if running a kernel with your patch, you can try
to trigger this by:
1) Suspending the machine by selecting suspend from a menu in your
desktop environment, or by briefly pressing the power-button, do
not close the lid
2) As soon as the system starts suspending and while it is suspended, move
your finger around the touchpad
3) Wake the system up with the powerbutton while moving your finger around
4) Check if the touchpad still works after this
Or by:
1) Using ctrl + alt + f3 to switch to a text console
2) Move finger around on touchpad, keep moving it around
3) Switch back to X11 with alt + f2 or alt + f7, while still moving the finger
4) Check if the touchpad still works after this
If neither causes the touchpad to stop working, then at least the problem Dmitry
fears is not easy to trigger, but we should probably still prepare to deal
with it; and we really should try to better understand the problem here, so
if you can answer my questions above, then that would be great.
Regards,
Hans
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH] ELAN touchpad i2c_hid bugs fix
2019-03-29 12:18 ` Hans de Goede
@ 2019-03-29 18:23 ` Dmitry Torokhov
2019-04-01 12:26 ` 廖崇榮
[not found] ` <LbI7kio--3-1@tutanota.com>
1 sibling, 1 reply; 34+ messages in thread
From: Dmitry Torokhov @ 2019-03-29 18:23 UTC (permalink / raw)
To: Hans de Goede, 廖崇榮
Cc: Vladislav Dalechyn, Benjamin Tissoires, Jiri Kosina,
kai.heng.feng, swboyd, bigeasy, open list:HID CORE LAYER, lkml,
hotwater438
On Fri, Mar 29, 2019 at 5:18 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 3/25/19 5:56 PM, Dmitry Torokhov wrote:
> > Hi Hans,
> >
> > On Mon, Mar 25, 2019 at 9:38 AM Hans de Goede <hdegoede@redhat.com> wrote:
> >>
> >> Hi Dmitry,
> >>
> >> On 25-03-19 17:02, Dmitry Torokhov wrote:
> >>> Hi Vladislav,
> >>>
> >>> On Mon, Mar 25, 2019 at 5:57 AM Vladislav Dalechyn
> >>> <vlad.dalechin@gmail.com> wrote:
> >>>>
> >>>> From: Vladislav Dalechyn <hotwater438@tutanota.com>
> >>>>
> >>>> Description: The ELAN1200:04F3:303E touchpad exposes several issues, all
> >>>> caused by an error setting the correct IRQ_TRIGGER flag:
> >>>> - i2c_hid incoplete error flood in journalctl;
> >>>> - Five finger tap kill's module so you have to restart it;
> >>>> - Two finger scoll is working incorrect and sometimes even when you
> >>>> raised one of two finger still thinks that you are scrolling.
> >>>>
> >>>> Fix all of these with a new quirk that corrects the trigger flag
> >>>> announced by the ACPI tables. (edge-falling).
> >>>
> >>> I do not believe this is right solution. The driver makes liberal use
> >>> of disable_irq() and enable_irq() which may lead to lost edges and
> >>> touchpad stopping working altogether.
> >>>
> >>> Usually the "extra" report is caused by GPIO controller clearing
> >>> interrupt condition at the wrong time (too early), or in unsafe or
> >>> racy fashion. You need to look there instead of adding quirk to
> >>> i2c-hid.
> >>
> >> The falling-edge solution was proposed by Elan themselves.
> >>
> >> Also if you look at: https://bugzilla.redhat.com/show_bug.cgi?id=1543769
> >>
> >> And esp. the "cat /proc/interrupts" output there, then you will see
> >> that the interrupt seems to be stuck at low level, which according
> >> to the ACPI tables is its active level.
> >
> > So how does it generate a new edge if it is stuck at low?
> >
> > Is it bad touchpad firmware that does not deassert interrupt quickly
> > enough?
>
> I do not believe it is not de-asserting it quick enough (I believe
> the amount of interrups is too high for that.
>
> It seems to simply be low most of the time, or it is really really
> slow with de-asserting.
>
> Vladislav can you check the output of /cat/interrupts on a kernel
> without the patch and while *not* using the touchpad; and check
> if the amount of touchpads-interrupts still keeps increasing in this
> case?
>
> Also I believe that you had contact with Elan about this and they
> told you to change the interrupt type to falling-edge as work-around,
> right? Can you ask them why?
KT, do you know anything about using edge interrupts with your
hid-over-i2c products?
>
> This is quite unususal, I've collected quite a few DSDTs over time
> and I've just checked about 40 of them all with a PNP0C50 in
> some form (and in many cases multiple such devices) and NONE of
> them is using edged-interrupts in the ACPI config.
That is because MS spec for HID over I2C requires level interrupts:
"7.4 Interrupt Line Management
DEVICE is responsible to assert the interrupt line (level trigger
interrupt required) when it has data.
DEVICE must keep the interrupt line asserted until the data has been
fully read by the HOST. After this point, the DEVICE must de-assert
the interrupt line if it has no more data to report to the HOST.
When the DEVICE has new data to report, it must repeat the process above."
See https://docs.microsoft.com/en-us/previous-versions/windows/hardware/design/dn642101(v=vs.85)
>
> <speculation>
>
> I think that the Elan touchpad firmware supports a mode to
> work on devices which only support edge interrupts and that
> this mode is accidentally enabled in this firmware.
>
> I think that the interrupt line is simply low all the time
> and gets pulsed high then low again when the touchpad detects
> a finger. Hopefully it does this pulsing on every event and not
> only when its event "fifo" is empty.
This behavior would violate HID-over-I2C spec though.
>
> </speculation>
>
> > I scrolled through the bug but I do not see if it had been
> > confirmed that original windows installation actually uses edge (it
> > may very well be using it; Elan engineers pushed us to use edge in a
> > few cases, but they all boiled down to an issue with pin control/GPIO
> > implementation).
>
> This has not been checked on Windows AFAIK.
>
> >> As for this being a GPIO chip driver problem, this is using standard
> >> Intel pinctrl stuff, which is not showing this same issue with many
> >> other i2c-hid touchpads.
> >
> > Well, there have been plenty of issues in intel drivers, coupled with
> > "interesting" things done by firmware and boards.
> >
> > If you want to keep on using edge you need to make sure that i2c-hid
> > never loses edge, as replaying of previously disabled interrupts in
> > not at all reliable. So you need to "kick" the device after
> > enable_irq() by initiating read from it and be ready to not get any
> > data or get valid data and process accordingly.
>
> That is a good point and I agree.
>
> Vladislav, let me explain this a bit. Normally the touchpad
> driver the IRQ line low when it has touch-data to respond, which means
> that if touch-data is reported before the driver loads (or while
> the driver has the irq disabled during e.g. suspend), it will immediately
> see an interrupt. If we use edge mode then the IRQ will only trigger
> when the IRQ line goes from high to low, if this happens when the driver
> is not listening then we do not see the edge. And since we never read the
> pending touch-data, the IRQ line never goes high again (which it does
> when we have read all available data), so we will never see a negative-edge
> and then things are stuck.
>
> It would be good, if running a kernel with your patch, you can try
> to trigger this by:
>
> 1) Suspending the machine by selecting suspend from a menu in your
> desktop environment, or by briefly pressing the power-button, do
> not close the lid
> 2) As soon as the system starts suspending and while it is suspended, move
> your finger around the touchpad
> 3) Wake the system up with the powerbutton while moving your finger around
> 4) Check if the touchpad still works after this
>
> Or by:
>
> 1) Using ctrl + alt + f3 to switch to a text console
> 2) Move finger around on touchpad, keep moving it around
> 3) Switch back to X11 with alt + f2 or alt + f7, while still moving the finger
> 4) Check if the touchpad still works after this
>
> If neither causes the touchpad to stop working, then at least the problem Dmitry
> fears is not easy to trigger, but we should probably still prepare to deal
> with it; and we really should try to better understand the problem here, so
> if you can answer my questions above, then that would be great.
Thanks.
--
Dmitry
^ permalink raw reply [flat|nested] 34+ messages in thread* RE: [PATCH] ELAN touchpad i2c_hid bugs fix
2019-03-29 18:23 ` Dmitry Torokhov
@ 2019-04-01 12:26 ` 廖崇榮
0 siblings, 0 replies; 34+ messages in thread
From: 廖崇榮 @ 2019-04-01 12:26 UTC (permalink / raw)
To: 'Dmitry Torokhov', 'Hans de Goede'
Cc: 'Vladislav Dalechyn', 'Benjamin Tissoires',
'Jiri Kosina',
kai.heng.feng, swboyd, bigeasy,
'open list:HID CORE LAYER', 'lkml',
hotwater438
Hi Dmitry,
-----Original Message-----
From: Dmitry Torokhov [mailto:dtor@chromium.org]
Sent: Saturday, March 30, 2019 2:24 AM
To: Hans de Goede; 廖崇榮
Cc: Vladislav Dalechyn; Benjamin Tissoires; Jiri Kosina; kai.heng.feng@canonical.com; swboyd@chromium.org; bigeasy@linutronix.de; open list:HID CORE LAYER; lkml; hotwater438@tutanota.com
Subject: Re: [PATCH] ELAN touchpad i2c_hid bugs fix
On Fri, Mar 29, 2019 at 5:18 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 3/25/19 5:56 PM, Dmitry Torokhov wrote:
> > Hi Hans,
> >
> > On Mon, Mar 25, 2019 at 9:38 AM Hans de Goede <hdegoede@redhat.com> wrote:
> >>
> >> Hi Dmitry,
> >>
> >> On 25-03-19 17:02, Dmitry Torokhov wrote:
> >>> Hi Vladislav,
> >>>
> >>> On Mon, Mar 25, 2019 at 5:57 AM Vladislav Dalechyn
> >>> <vlad.dalechin@gmail.com> wrote:
> >>>>
> >>>> From: Vladislav Dalechyn <hotwater438@tutanota.com>
> >>>>
> >>>> Description: The ELAN1200:04F3:303E touchpad exposes several
> >>>> issues, all caused by an error setting the correct IRQ_TRIGGER flag:
> >>>> - i2c_hid incoplete error flood in journalctl;
> >>>> - Five finger tap kill's module so you have to restart it;
> >>>> - Two finger scoll is working incorrect and sometimes even when
> >>>> you raised one of two finger still thinks that you are scrolling.
> >>>>
> >>>> Fix all of these with a new quirk that corrects the trigger flag
> >>>> announced by the ACPI tables. (edge-falling).
> >>>
> >>> I do not believe this is right solution. The driver makes liberal
> >>> use of disable_irq() and enable_irq() which may lead to lost edges
> >>> and touchpad stopping working altogether.
> >>>
> >>> Usually the "extra" report is caused by GPIO controller clearing
> >>> interrupt condition at the wrong time (too early), or in unsafe or
> >>> racy fashion. You need to look there instead of adding quirk to
> >>> i2c-hid.
> >>
> >> The falling-edge solution was proposed by Elan themselves.
> >>
> >> Also if you look at:
> >> https://bugzilla.redhat.com/show_bug.cgi?id=1543769
> >>
> >> And esp. the "cat /proc/interrupts" output there, then you will see
> >> that the interrupt seems to be stuck at low level, which according
> >> to the ACPI tables is its active level.
> >
> > So how does it generate a new edge if it is stuck at low?
> >
> > Is it bad touchpad firmware that does not deassert interrupt quickly
> > enough?
>
> I do not believe it is not de-asserting it quick enough (I believe the
> amount of interrups is too high for that.
>
> It seems to simply be low most of the time, or it is really really
> slow with de-asserting.
>
> Vladislav can you check the output of /cat/interrupts on a kernel
> without the patch and while *not* using the touchpad; and check if the
> amount of touchpads-interrupts still keeps increasing in this case?
>
> Also I believe that you had contact with Elan about this and they told
> you to change the interrupt type to falling-edge as work-around,
> right? Can you ask them why?
KT, do you know anything about using edge interrupts with your hid-over-i2c products?
Ya, I sent the patch to Vladislav for testing his 5 finger-tap issue.
The patch is created by Dell/Intel for debugging Elan PTP's incomplete report message.
Host tried to get report again while touchpad finish report transmission.
I excerpt from Dell's mail
===========Begin=========================
We have worked with Intel for a while to revise Linux kernel driver code to prevent Touch I2C error message show up,
And Intel dig out there’re 2 issues to cause this i2c error on Linux kernel 4.18.
Therefore Intel suggested to revised SW code into “trigger falling edge + pm_disabled + IRQF_SHARED | IRQF_COND_SUSPEND(i2c-designware-master.c)”
and then we can get positive result that i2c error message disappeared when Touch Panel is working.
Since this issue only happened on ELAN touch pad + Intel GLK platform (Linux) and other Intel reference platform (Whisky lake) won’t, would you help check the solution (as patch attached) and let us know if any question/concern for this modification?
===========End=========================
I don't know the root cause of 5-finger issue with level trigger so far.
I will check it once I get the touchpad with the same issue.
From previous issue list, it seems that some touchpad's crush issue will be fixed by edge trigger.
I have discussed with FW engineer, both level and edge trigger should be OK for our PTP(HID touchpad).
I summary the behavior of our HID touchpad's interrupt.
1. Assert for Every finger's report, which means 5 ISR for 5 finger operation.
2. We will de-assert INT pin after the last byte's NACK signal.
I checked LA scope, 2nd finger's assert will happen after 10us if two finger touch.
SYNAPTICS's touchpad will de-assert it after address byte sent.
I am not sure if SYNAPTICE's earlier de-asserting is a better way for level-trigger?
At least we never see issue happen on our PTP in Windows 10.
>
> This is quite unususal, I've collected quite a few DSDTs over time and
> I've just checked about 40 of them all with a PNP0C50 in some form
> (and in many cases multiple such devices) and NONE of them is using
> edged-interrupts in the ACPI config.
That is because MS spec for HID over I2C requires level interrupts:
"7.4 Interrupt Line Management
DEVICE is responsible to assert the interrupt line (level trigger interrupt required) when it has data.
DEVICE must keep the interrupt line asserted until the data has been fully read by the HOST. After this point, the DEVICE must de-assert the interrupt line if it has no more data to report to the HOST.
When the DEVICE has new data to report, it must repeat the process above."
See https://docs.microsoft.com/en-us/previous-versions/windows/hardware/design/dn642101(v=vs.85)
>
> <speculation>
>
> I think that the Elan touchpad firmware supports a mode to work on
> devices which only support edge interrupts and that this mode is
> accidentally enabled in this firmware.
>
> I think that the interrupt line is simply low all the time and gets
> pulsed high then low again when the touchpad detects a finger.
> Hopefully it does this pulsing on every event and not only when its
> event "fifo" is empty.
This behavior would violate HID-over-I2C spec though.
>
> </speculation>
>
> > I scrolled through the bug but I do not see if it had been confirmed
> > that original windows installation actually uses edge (it may very
> > well be using it; Elan engineers pushed us to use edge in a few
> > cases, but they all boiled down to an issue with pin control/GPIO
> > implementation).
>
> This has not been checked on Windows AFAIK.
>
> >> As for this being a GPIO chip driver problem, this is using
> >> standard Intel pinctrl stuff, which is not showing this same issue
> >> with many other i2c-hid touchpads.
> >
> > Well, there have been plenty of issues in intel drivers, coupled
> > with "interesting" things done by firmware and boards.
> >
> > If you want to keep on using edge you need to make sure that i2c-hid
> > never loses edge, as replaying of previously disabled interrupts in
> > not at all reliable. So you need to "kick" the device after
> > enable_irq() by initiating read from it and be ready to not get any
> > data or get valid data and process accordingly.
>
> That is a good point and I agree.
>
> Vladislav, let me explain this a bit. Normally the touchpad driver the
> IRQ line low when it has touch-data to respond, which means that if
> touch-data is reported before the driver loads (or while the driver
> has the irq disabled during e.g. suspend), it will immediately see an
> interrupt. If we use edge mode then the IRQ will only trigger when the
> IRQ line goes from high to low, if this happens when the driver is not
> listening then we do not see the edge. And since we never read the
> pending touch-data, the IRQ line never goes high again (which it does
> when we have read all available data), so we will never see a
> negative-edge and then things are stuck.
>
> It would be good, if running a kernel with your patch, you can try to
> trigger this by:
>
> 1) Suspending the machine by selecting suspend from a menu in your
> desktop environment, or by briefly pressing the power-button, do not
> close the lid
> 2) As soon as the system starts suspending and while it is suspended,
> move your finger around the touchpad
> 3) Wake the system up with the powerbutton while moving your finger
> around
> 4) Check if the touchpad still works after this
>
> Or by:
>
> 1) Using ctrl + alt + f3 to switch to a text console
> 2) Move finger around on touchpad, keep moving it around
> 3) Switch back to X11 with alt + f2 or alt + f7, while still moving
> the finger
> 4) Check if the touchpad still works after this
>
> If neither causes the touchpad to stop working, then at least the
> problem Dmitry fears is not easy to trigger, but we should probably
> still prepare to deal with it; and we really should try to better
> understand the problem here, so if you can answer my questions above, then that would be great.
Thanks.
--
Dmitry
^ permalink raw reply [flat|nested] 34+ messages in thread
* RE: [PATCH] ELAN touchpad i2c_hid bugs fix
@ 2019-04-01 12:26 ` 廖崇榮
0 siblings, 0 replies; 34+ messages in thread
From: 廖崇榮 @ 2019-04-01 12:26 UTC (permalink / raw)
To: 'Dmitry Torokhov', 'Hans de Goede'
Cc: 'Vladislav Dalechyn', 'Benjamin Tissoires',
'Jiri Kosina',
kai.heng.feng, swboyd, bigeasy,
'open list:HID CORE LAYER', 'lkml',
hotwater438
Hi Dmitry,
-----Original Message-----
From: Dmitry Torokhov [mailto:dtor@chromium.org]
Sent: Saturday, March 30, 2019 2:24 AM
To: Hans de Goede; 廖崇榮
Cc: Vladislav Dalechyn; Benjamin Tissoires; Jiri Kosina; kai.heng.feng@canonical.com; swboyd@chromium.org; bigeasy@linutronix.de; open list:HID CORE LAYER; lkml; hotwater438@tutanota.com
Subject: Re: [PATCH] ELAN touchpad i2c_hid bugs fix
On Fri, Mar 29, 2019 at 5:18 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 3/25/19 5:56 PM, Dmitry Torokhov wrote:
> > Hi Hans,
> >
> > On Mon, Mar 25, 2019 at 9:38 AM Hans de Goede <hdegoede@redhat.com> wrote:
> >>
> >> Hi Dmitry,
> >>
> >> On 25-03-19 17:02, Dmitry Torokhov wrote:
> >>> Hi Vladislav,
> >>>
> >>> On Mon, Mar 25, 2019 at 5:57 AM Vladislav Dalechyn
> >>> <vlad.dalechin@gmail.com> wrote:
> >>>>
> >>>> From: Vladislav Dalechyn <hotwater438@tutanota.com>
> >>>>
> >>>> Description: The ELAN1200:04F3:303E touchpad exposes several
> >>>> issues, all caused by an error setting the correct IRQ_TRIGGER flag:
> >>>> - i2c_hid incoplete error flood in journalctl;
> >>>> - Five finger tap kill's module so you have to restart it;
> >>>> - Two finger scoll is working incorrect and sometimes even when
> >>>> you raised one of two finger still thinks that you are scrolling.
> >>>>
> >>>> Fix all of these with a new quirk that corrects the trigger flag
> >>>> announced by the ACPI tables. (edge-falling).
> >>>
> >>> I do not believe this is right solution. The driver makes liberal
> >>> use of disable_irq() and enable_irq() which may lead to lost edges
> >>> and touchpad stopping working altogether.
> >>>
> >>> Usually the "extra" report is caused by GPIO controller clearing
> >>> interrupt condition at the wrong time (too early), or in unsafe or
> >>> racy fashion. You need to look there instead of adding quirk to
> >>> i2c-hid.
> >>
> >> The falling-edge solution was proposed by Elan themselves.
> >>
> >> Also if you look at:
> >> https://bugzilla.redhat.com/show_bug.cgi?id=1543769
> >>
> >> And esp. the "cat /proc/interrupts" output there, then you will see
> >> that the interrupt seems to be stuck at low level, which according
> >> to the ACPI tables is its active level.
> >
> > So how does it generate a new edge if it is stuck at low?
> >
> > Is it bad touchpad firmware that does not deassert interrupt quickly
> > enough?
>
> I do not believe it is not de-asserting it quick enough (I believe the
> amount of interrups is too high for that.
>
> It seems to simply be low most of the time, or it is really really
> slow with de-asserting.
>
> Vladislav can you check the output of /cat/interrupts on a kernel
> without the patch and while *not* using the touchpad; and check if the
> amount of touchpads-interrupts still keeps increasing in this case?
>
> Also I believe that you had contact with Elan about this and they told
> you to change the interrupt type to falling-edge as work-around,
> right? Can you ask them why?
KT, do you know anything about using edge interrupts with your hid-over-i2c products?
Ya, I sent the patch to Vladislav for testing his 5 finger-tap issue.
The patch is created by Dell/Intel for debugging Elan PTP's incomplete report message.
Host tried to get report again while touchpad finish report transmission.
I excerpt from Dell's mail
===========Begin=========================
We have worked with Intel for a while to revise Linux kernel driver code to prevent Touch I2C error message show up,
And Intel dig out there’re 2 issues to cause this i2c error on Linux kernel 4.18.
Therefore Intel suggested to revised SW code into “trigger falling edge + pm_disabled + IRQF_SHARED | IRQF_COND_SUSPEND(i2c-designware-master.c)”
and then we can get positive result that i2c error message disappeared when Touch Panel is working.
Since this issue only happened on ELAN touch pad + Intel GLK platform (Linux) and other Intel reference platform (Whisky lake) won’t, would you help check the solution (as patch attached) and let us know if any question/concern for this modification?
===========End=========================
I don't know the root cause of 5-finger issue with level trigger so far.
I will check it once I get the touchpad with the same issue.
>From previous issue list, it seems that some touchpad's crush issue will be fixed by edge trigger.
I have discussed with FW engineer, both level and edge trigger should be OK for our PTP(HID touchpad).
I summary the behavior of our HID touchpad's interrupt.
1. Assert for Every finger's report, which means 5 ISR for 5 finger operation.
2. We will de-assert INT pin after the last byte's NACK signal.
I checked LA scope, 2nd finger's assert will happen after 10us if two finger touch.
SYNAPTICS's touchpad will de-assert it after address byte sent.
I am not sure if SYNAPTICE's earlier de-asserting is a better way for level-trigger?
At least we never see issue happen on our PTP in Windows 10.
>
> This is quite unususal, I've collected quite a few DSDTs over time and
> I've just checked about 40 of them all with a PNP0C50 in some form
> (and in many cases multiple such devices) and NONE of them is using
> edged-interrupts in the ACPI config.
That is because MS spec for HID over I2C requires level interrupts:
"7.4 Interrupt Line Management
DEVICE is responsible to assert the interrupt line (level trigger interrupt required) when it has data.
DEVICE must keep the interrupt line asserted until the data has been fully read by the HOST. After this point, the DEVICE must de-assert the interrupt line if it has no more data to report to the HOST.
When the DEVICE has new data to report, it must repeat the process above."
See https://docs.microsoft.com/en-us/previous-versions/windows/hardware/design/dn642101(v=vs.85)
>
> <speculation>
>
> I think that the Elan touchpad firmware supports a mode to work on
> devices which only support edge interrupts and that this mode is
> accidentally enabled in this firmware.
>
> I think that the interrupt line is simply low all the time and gets
> pulsed high then low again when the touchpad detects a finger.
> Hopefully it does this pulsing on every event and not only when its
> event "fifo" is empty.
This behavior would violate HID-over-I2C spec though.
>
> </speculation>
>
> > I scrolled through the bug but I do not see if it had been confirmed
> > that original windows installation actually uses edge (it may very
> > well be using it; Elan engineers pushed us to use edge in a few
> > cases, but they all boiled down to an issue with pin control/GPIO
> > implementation).
>
> This has not been checked on Windows AFAIK.
>
> >> As for this being a GPIO chip driver problem, this is using
> >> standard Intel pinctrl stuff, which is not showing this same issue
> >> with many other i2c-hid touchpads.
> >
> > Well, there have been plenty of issues in intel drivers, coupled
> > with "interesting" things done by firmware and boards.
> >
> > If you want to keep on using edge you need to make sure that i2c-hid
> > never loses edge, as replaying of previously disabled interrupts in
> > not at all reliable. So you need to "kick" the device after
> > enable_irq() by initiating read from it and be ready to not get any
> > data or get valid data and process accordingly.
>
> That is a good point and I agree.
>
> Vladislav, let me explain this a bit. Normally the touchpad driver the
> IRQ line low when it has touch-data to respond, which means that if
> touch-data is reported before the driver loads (or while the driver
> has the irq disabled during e.g. suspend), it will immediately see an
> interrupt. If we use edge mode then the IRQ will only trigger when the
> IRQ line goes from high to low, if this happens when the driver is not
> listening then we do not see the edge. And since we never read the
> pending touch-data, the IRQ line never goes high again (which it does
> when we have read all available data), so we will never see a
> negative-edge and then things are stuck.
>
> It would be good, if running a kernel with your patch, you can try to
> trigger this by:
>
> 1) Suspending the machine by selecting suspend from a menu in your
> desktop environment, or by briefly pressing the power-button, do not
> close the lid
> 2) As soon as the system starts suspending and while it is suspended,
> move your finger around the touchpad
> 3) Wake the system up with the powerbutton while moving your finger
> around
> 4) Check if the touchpad still works after this
>
> Or by:
>
> 1) Using ctrl + alt + f3 to switch to a text console
> 2) Move finger around on touchpad, keep moving it around
> 3) Switch back to X11 with alt + f2 or alt + f7, while still moving
> the finger
> 4) Check if the touchpad still works after this
>
> If neither causes the touchpad to stop working, then at least the
> problem Dmitry fears is not easy to trigger, but we should probably
> still prepare to deal with it; and we really should try to better
> understand the problem here, so if you can answer my questions above, then that would be great.
Thanks.
--
Dmitry
^ permalink raw reply [flat|nested] 34+ messages in thread
[parent not found: <LbI7kio--3-1@tutanota.com>]
* Re: [PATCH] ELAN touchpad i2c_hid bugs fix
[not found] ` <LbI7kio--3-1@tutanota.com>
@ 2019-04-03 11:18 ` Hans de Goede
[not found] ` <LbZjy9p--3-1@tutanota.com>
0 siblings, 1 reply; 34+ messages in thread
From: Hans de Goede @ 2019-04-03 11:18 UTC (permalink / raw)
To: hotwater438
Cc: Dmitry Torokhov, Vladislav Dalechyn, Benjamin Tissoires,
Jiri Kosina, Kai Heng Feng, Swboyd, Bigeasy,
open list:HID CORE LAYER, lkml
Hi,
On 31-03-19 11:50, hotwater438@tutanota.com wrote:
> Hi. I've done everything you said, here are results:
>
> Vladislav can you check the output of /cat/interrupts on a kernel
> without the patch and while *not* using the touchpad; and check
> if the amount of touchpads-interrupts still keeps increasing in this
> case?
>
> IWI or IRQ work interrupts keep increasing with speed at least 3 interrupts/s.
I'm really only interested in the touchpad related IRQs, so e.g. the line
about "intel-gpio 129 ELAN1200:00", if you're seeing 3 interrupts/s on
some others that is fine, so I take it the ELAN1200:00 interrupt count
does not increase on an *unpatched* kernel, unless you use the touchpad?
> Also when I am moving touchpad IR-IO-APIC 14-fasteoi INT345D:00 get's triggered and increased.
That is the GPIO controller interrupt, so that one increasing is normal.
If I understand things correctly then this all means that the IRQ indeed
is a normal level IRQ and Dmitry is likely correct that there is an
pinctrl / gpiochip driver problem here.
Can you try the following with an *unpatched* kernel? :
1) Run "cat /proc/interrupts | grep ELAN" , note down the value
2) Very quickly/briefly touch the touchpad once
3) Run "cat /proc/interrupts | grep ELAN" , note down the value again
4) Subtract result from 1. from result from 3, this difference is
the value we are interested in. E.g. my testing got 254 and 257, so
a difference of 3.
The goal here is to get an as low as possible difference. Feel free
to repeat this a couple of times.
On an Apollo Lake laptop with an I2C hid mt touchpad I can get the
amount of interrupts triggered for a single touch down to 3,
given the huge interrupt counts of 130000+ reported in:
https://bugzilla.redhat.com/show_bug.cgi?id=1543769
I expect you to get a much bigger smallest possible difference
between 2 "cat /proc/interrupts | grep ELAN" commands, note a
difference of 0 means your touch did not register.
Assuming you indeed see much more interrupts for a very quick
touch + release, then we indeed have an interrupt handling problem
we need to investigate further.
> I don't know if it's important or not, but for some reason these interrupts keep popping only on CPU2 (i have 4cpu processor).
That does not matter.
> 1) Suspending the machine by selecting suspend from a menu in your
> desktop environment, or by briefly pressing the power-button, do
> not close the lid
> 2) As soon as the system starts suspending and while it is suspended, move
> your finger around the touchpad
> 3) Wake the system up with the powerbutton while moving your finger around
> 4) Check if the touchpad still works after this
>
> It works, but as it seems, looses edge. JournalCTL is being flooded with i2c_hid_get_input: incomplete report (16/65535)
That is probably a different issue. If you loose the edge IRQ, then the touchpad
would stop working without any messages. I believe that the suspend / resume
issue may be fixed by:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=52cf93e63ee672a92f349edc6ddad86ec8808fd8
Does your kernel have this commit? (please always use the latest kernel while
testing).
> Also a thing to notice, that after manually removing and modprobing i2c_hid module, it says next in journalctl:
>
> i2c_hid i2c-ELAN1200:00: i2c-ELAN1200:00 supply vdd not found, using dummy regulator
> i2c_hid i2c-ELAN1200:00: i2c-ELAN1200:00 supply vddl not found, using dummy regulator
Those messages can safely be ignored.
Regards,
Hans
^ permalink raw reply [flat|nested] 34+ messages in thread
end of thread, other threads:[~2019-04-16 3:59 UTC | newest]
Thread overview: 34+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <LaQHUFs--3-1@tutanota.com>
2019-03-20 14:37 ` [PATCH] ELAN touchpad i2c_hid bugs fix Benjamin Tissoires
2019-03-20 15:39 ` Hans de Goede
2019-03-20 16:53 ` Kai-Heng Feng
2019-03-20 17:18 ` Andy Shevchenko
2019-03-21 4:08 ` Kai-Heng Feng
2019-03-21 8:55 ` Andy Shevchenko
2019-03-21 9:28 ` Kai Heng Feng
2019-03-21 8:57 ` Hans de Goede
2019-03-21 9:48 ` Andy Shevchenko
2019-04-01 21:37 ` Mario.Limonciello
2019-04-01 21:37 ` Mario.Limonciello
2019-04-02 4:18 ` Kai Heng Feng
2019-04-02 14:08 ` Mario.Limonciello
2019-04-02 14:08 ` Mario.Limonciello
2019-04-03 9:24 ` Kai-Heng Feng
2019-03-20 17:11 ` Andy Shevchenko
[not found] ` <LaUpAlT--3-1@tutanota.com>
[not found] ` <LaeGPSe--3-1@tutanota.com>
2019-03-24 12:27 ` Hans de Goede
[not found] ` <LakgsCJ--3-1@tutanota.com>
2019-03-24 18:37 ` Hans de Goede
2019-03-24 19:10 Vladislav Dalechyn
2019-03-25 9:23 ` Benjamin Tissoires
2019-03-25 12:57 Vladislav Dalechyn
2019-03-25 16:02 ` Dmitry Torokhov
2019-03-25 16:38 ` Hans de Goede
2019-03-25 16:56 ` Dmitry Torokhov
[not found] ` <Laq4ykv--3-1@tutanota.com>
2019-03-25 18:30 ` Dmitry Torokhov
2019-03-29 12:18 ` Hans de Goede
2019-03-29 18:23 ` Dmitry Torokhov
2019-04-01 12:26 ` 廖崇榮
2019-04-01 12:26 ` 廖崇榮
[not found] ` <LbI7kio--3-1@tutanota.com>
2019-04-03 11:18 ` Hans de Goede
[not found] ` <LbZjy9p--3-1@tutanota.com>
2019-04-11 16:17 ` Kai-Heng Feng
[not found] ` <LcKqhgD--3-1@tutanota.com>
2019-04-13 8:42 ` Kai-Heng Feng
[not found] ` <LcVmBjG--3-1@tutanota.com>
2019-04-15 11:42 ` Hans de Goede
2019-04-16 3:59 ` Kai-Heng Feng
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.