Re: [PATCH] ELAN touchpad i2c_hid bugs fix

[Kai-Heng Feng]

at 19:42, Hans de Goede  wrote:


Hi,

On 15-04-19 13:36, hotwater...@tutanota.com wrote:

Sorry for the delay.
By applying this patch I get next results:
Five finger tap and two finger scroll issues disappear, but after  
suspend touchpad dies. Restarting module doesn't help.


So bascally the same results as with the edge-triggered interrupt  
patch/hack,

right?

Are you still using the edge-triggered interrupt patch, or just the new
patch Kai-Heng Feng provided.

To me it sounds like the patch Kai-Heng Feng provided at least removes
the need for the edge-triggered interrupt patch/hack and what remains to
be solved is the suspend/resume issues.


Great! I’ll send a patch to address this issue.

Kai-Heng



Regards,

Hans




Here's the log:
Apr 15 14:35:54 parrot sudo[3473]: h0tw4t3r : TTY=pts/1 ;  
PWD=/home/h0tw4t3r ; USER=root ; COMMAND=/sbin/rmmod i2c_hid
Apr 15 14:35:54 parrot sudo[3478]: h0tw4t3r : TTY=pts/1 ;  
PWD=/home/h0tw4t3r ; USER=root ; COMMAND=/sbin/modprobe i2c_hid
Apr 15 14:35:54 parrot kernel: i2c_hid i2c-ELAN1200:00: i2c-ELAN1200:00  
supply vdd not found, using dummy regulator
Apr 15 14:35:54 parrot kernel: i2c_hid i2c-ELAN1200:00: i2c-ELAN1200:00  
supply vddl not found, using dummy regulator

Could you please explain what this patch does?
Regards,
Vladislav.
Apr 13, 2019, 11:42 AM by kai.heng.f...@canonical.com:
at 16:40, mailto:hotwater...@tutanota.com>> 
mailto:hotwater...@tutanota.com>> wrote:
Hi.
I've applied this patch, but still getting incomplete report messages.
Does the patch fix the other two issues:
- 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.
Kai-Heng
Regards,
Vladislav
Apr 11, 2019, 7:17 PM by kai.heng.f...@canonical.com 
:
Hi,
at 05:18, mailto:hotwater...@tutanota.com>> 
mailto:hotwater...@tutanota.com>> wrote:
Hi.
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.
I've tested that, main diffs are 30, 24, 16 (the most frequent), 2 (the 
least frequent).
I was using 4.19.13 kernel, because I use ParrotOS (which happens to be 
Debian distribution).
But I've installed experimental 5.0.0 kernel and I can't say right now 
if suspend problem is resolved (i have to rebuild latest kernel with patch).
Can you try below fix?
This can solve what commit 1475af255e18 ("HID: i2c-hid: Ignore input 
report if there's no data present on Elan touchpanels”) tries to workaround.
diff --git a/drivers/pinctrl/intel/pinctrl-intel.c 
b/drivers/pinctrl/intel/pinctrl-intel.c
index c19a4c45f7bb..30e3664f1ae5 100644
--- a/drivers/pinctrl/intel/pinctrl-intel.c
+++ b/drivers/pinctrl/intel/pinctrl-intel.c
@@ -957,6 +957,10 @@ static void intel_gpio_irq_mask_unmask(struct 
irq_data *d, bool mask)
reg = community->regs + community->ie_offset + gpp * 4;
raw_spin_lock_irqsave(>lock, flags);
+
+ if (!mask)
+ writel(BIT(gpp_offset), community->regs + community->is_offset + gpp 
* 4);
+
value = readl(reg);
if (mask)
value &= ~BIT(gpp_offset);
Regards,
Vladislav.
Apr 3, 2019, 2:18 PM by hdego...@redhat.com 
:
Hi,
On 31-03-19 11:50, hotwater...@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 

Re: [PATCH] ELAN touchpad i2c_hid bugs fix

[Hans de Goede]

Hi,

On 15-04-19 13:36, hotwater...@tutanota.com wrote:

Sorry for the delay.
By applying this patch I get next results:
Five finger tap and two finger scroll issues disappear, but after suspend 
touchpad dies. Restarting module doesn't help.


So bascally the same results as with the edge-triggered interrupt patch/hack,
right?

Are you still using the edge-triggered interrupt patch, or just the new
patch Kai-Heng Feng provided.

To me it sounds like the patch Kai-Heng Feng provided at least removes
the need for the edge-triggered interrupt patch/hack and what remains to
be solved is the suspend/resume issues.

Regards,

Hans




Here's the log:

Apr 15 14:35:54 parrot sudo[3473]: h0tw4t3r : TTY=pts/1 ; PWD=/home/h0tw4t3r ; 
USER=root ; COMMAND=/sbin/rmmod i2c_hid
Apr 15 14:35:54 parrot sudo[3478]: h0tw4t3r : TTY=pts/1 ; PWD=/home/h0tw4t3r ; 
USER=root ; COMMAND=/sbin/modprobe i2c_hid
Apr 15 14:35:54 parrot kernel: i2c_hid i2c-ELAN1200:00: i2c-ELAN1200:00 supply 
vdd not found, using dummy regulator
Apr 15 14:35:54 parrot kernel: i2c_hid i2c-ELAN1200:00: i2c-ELAN1200:00 supply 
vddl not found, using dummy regulator

Could you please explain what this patch does?

Regards,
Vladislav.

Apr 13, 2019, 11:42 AM by kai.heng.f...@canonical.com:

at 16:40, mailto:hotwater...@tutanota.com>> 
mailto:hotwater...@tutanota.com>> wrote:

Hi.

I've applied this patch, but still getting incomplete report messages.


Does the patch fix the other two issues:
- 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.

Kai-Heng


Regards,
Vladislav

Apr 11, 2019, 7:17 PM by kai.heng.f...@canonical.com 
:
Hi,

at 05:18, mailto:hotwater...@tutanota.com>> 
mailto:hotwater...@tutanota.com>> wrote:
Hi.

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.
I've tested that, main diffs are 30, 24, 16 (the most frequent), 2 (the 
least frequent).

I was using 4.19.13 kernel, because I use ParrotOS (which happens to be 
Debian distribution).
But I've installed experimental 5.0.0 kernel and I can't say right now 
if suspend problem is resolved (i have to rebuild latest kernel with patch).

Can you try below fix?

This can solve what commit 1475af255e18 ("HID: i2c-hid: Ignore input 
report if there's no data present on Elan touchpanels”) tries to workaround.

diff --git a/drivers/pinctrl/intel/pinctrl-intel.c 
b/drivers/pinctrl/intel/pinctrl-intel.c
index c19a4c45f7bb..30e3664f1ae5 100644
--- a/drivers/pinctrl/intel/pinctrl-intel.c
+++ b/drivers/pinctrl/intel/pinctrl-intel.c
@@ -957,6 +957,10 @@ static void intel_gpio_irq_mask_unmask(struct 
irq_data *d, bool mask)
reg = community->regs + community->ie_offset + gpp * 4;

raw_spin_lock_irqsave(>lock, flags);
+
+ if (!mask)
+ writel(BIT(gpp_offset), community->regs + community->is_offset + gpp 
* 4);
+
value = readl(reg);
if (mask)
value &= ~BIT(gpp_offset);


Regards,
Vladislav.

Apr 3, 2019, 2:18 PM by hdego...@redhat.com 
:
Hi,

On 31-03-19 11:50, hotwater...@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 

Re: [PATCH] ELAN touchpad i2c_hid bugs fix

[Kai-Heng Feng]

at 16:40,   wrote:


Hi.

I've applied this patch, but still getting incomplete report messages.


Does the patch fix the other two issues:
- 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.

Kai-Heng



Regards,
Vladislav

Apr 11, 2019, 7:17 PM by kai.heng.f...@canonical.com:
Hi,

at 05:18,   wrote:
Hi.

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.
I've tested that, main diffs are 30, 24, 16 (the most frequent), 2 (the  
least frequent).


I was using 4.19.13 kernel, because I use ParrotOS (which happens to be  
Debian distribution).
But I've installed experimental 5.0.0 kernel and I can't say right now if  
suspend problem is resolved (i have to rebuild latest kernel with patch).


Can you try below fix?

This can solve what commit 1475af255e18 ("HID: i2c-hid: Ignore input  
report if there's no data present on Elan touchpanels”) tries to  
workaround.


diff --git a/drivers/pinctrl/intel/pinctrl-intel.c  
b/drivers/pinctrl/intel/pinctrl-intel.c

index c19a4c45f7bb..30e3664f1ae5 100644
--- a/drivers/pinctrl/intel/pinctrl-intel.c
+++ b/drivers/pinctrl/intel/pinctrl-intel.c
@@ -957,6 +957,10 @@ static void intel_gpio_irq_mask_unmask(struct  
irq_data *d, bool mask)

reg = community->regs + community->ie_offset + gpp * 4;

raw_spin_lock_irqsave(>lock, flags);
+
+ if (!mask)
+ writel(BIT(gpp_offset), community->regs + community->is_offset + gpp *  
4);

+
value = readl(reg);
if (mask)
value &= ~BIT(gpp_offset);


Regards,
Vladislav.

Apr 3, 2019, 2:18 PM by hdego...@redhat.com:
Hi,

On 31-03-19 11:50, hotwater...@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 13+ 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:

Re: [PATCH] ELAN touchpad i2c_hid bugs fix

[Kai-Heng Feng]

Hi,

at 05:18,   wrote:


Hi.

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.
I've tested that, main diffs are 30, 24, 16 (the most frequent), 2 (the  
least frequent).


I was using 4.19.13 kernel, because I use ParrotOS (which happens to be  
Debian distribution).
But I've installed experimental 5.0.0 kernel and I can't say right now if  
suspend problem is resolved (i have to rebuild latest kernel with patch).


Can you try below fix?

This can solve what commit 1475af255e18 ("HID: i2c-hid: Ignore input report  
if there's no data present on Elan touchpanels”) tries to workaround.


diff --git a/drivers/pinctrl/intel/pinctrl-intel.c  
b/drivers/pinctrl/intel/pinctrl-intel.c

index c19a4c45f7bb..30e3664f1ae5 100644
--- a/drivers/pinctrl/intel/pinctrl-intel.c
+++ b/drivers/pinctrl/intel/pinctrl-intel.c
@@ -957,6 +957,10 @@ static void intel_gpio_irq_mask_unmask(struct irq_data  
*d, bool mask)

reg = community->regs + community->ie_offset + gpp * 4;

raw_spin_lock_irqsave(>lock, flags);
+
+   if (!mask)
+   writel(BIT(gpp_offset), community->regs +  
community->is_offset + gpp * 4);

+
value = readl(reg);
if (mask)
value &= ~BIT(gpp_offset);




Regards,
Vladislav.

Apr 3, 2019, 2:18 PM by hdego...@redhat.com:
Hi,

On 31-03-19 11:50, hotwater...@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 13+ 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  

Re: [PATCH] ELAN touchpad i2c_hid bugs fix

[Hans de Goede]

Hi,

On 31-03-19 11:50, hotwater...@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 13+ 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

Re: [PATCH] ELAN touchpad i2c_hid bugs fix

[Kai-Heng Feng]

at 22:08,   wrote:


-Original Message-
From: Kai Heng Feng 
Sent: Monday, April 1, 2019 11:18 PM
To: Limonciello, Mario
Cc: Andy Shevchenko; hdego...@redhat.com; benjamin.tissoi...@redhat.com;
hotwater...@tutanota.com; ji...@kernel.org; swb...@chromium.org;
bige...@linutronix.de; d...@chromium.org; linux-in...@vger.kernel.org;  
linux-

ker...@vger.kernel.org
Subject: Re: [PATCH] ELAN touchpad i2c_hid bugs fix


[EXTERNAL EMAIL]




On Apr 2, 2019, at 5:37 AM, 
 wrote:


-Original Message-
From: Andy Shevchenko 
Sent: Thursday, March 21, 2019 4:48 AM
To: Kai-Heng Feng; Limonciello, Mario
Cc: Hans de Goede; Benjamin Tissoires; hotwater...@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
 wrote:

at 01:18, Andy Shevchenko  wrote:


On Wed, Mar 20, 2019 at 6:55 PM Kai-Heng Feng
 wrote:

at 23:39, Hans de Goede  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, 0x,
"\\_SB.GPO1", 0x00, ResourceConsumer, ,
)
{   // Pin list
0x0012
}
})
Name (SBFI, ResourceTemplate ()
{
Interrupt (ResourceConsumer, Level, ActiveLow, 
ExclusiveAndWake, ,, )
{
0x003C,
}
})
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

RE: [PATCH] ELAN touchpad i2c_hid bugs fix

[Mario.Limonciello]

> -Original Message-
> From: Kai Heng Feng 
> Sent: Monday, April 1, 2019 11:18 PM
> To: Limonciello, Mario
> Cc: Andy Shevchenko; hdego...@redhat.com; benjamin.tissoi...@redhat.com;
> hotwater...@tutanota.com; ji...@kernel.org; swb...@chromium.org;
> bige...@linutronix.de; d...@chromium.org; linux-in...@vger.kernel.org; linux-
> ker...@vger.kernel.org
> Subject: Re: [PATCH] ELAN touchpad i2c_hid bugs fix
> 
> 
> [EXTERNAL EMAIL]
> 
> 
> 
> > On Apr 2, 2019, at 5:37 AM, 
> >  wrote:
> >
> >> -Original Message-
> >> From: Andy Shevchenko 
> >> Sent: Thursday, March 21, 2019 4:48 AM
> >> To: Kai-Heng Feng; Limonciello, Mario
> >> Cc: Hans de Goede; Benjamin Tissoires; hotwater...@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
> >>  wrote:
> >>>
> >>> at 01:18, Andy Shevchenko  wrote:
> >>>
> >>>> On Wed, Mar 20, 2019 at 6:55 PM Kai-Heng Feng
> >>>>  wrote:
> >>>>> at 23:39, Hans de Goede  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 ()
> >>> {
> >>> 

Re: [PATCH] ELAN touchpad i2c_hid bugs fix

[Kai Heng Feng]

On Apr 2, 2019, at 5:37 AM,   
 wrote:



-Original Message-
From: Andy Shevchenko 
Sent: Thursday, March 21, 2019 4:48 AM
To: Kai-Heng Feng; Limonciello, Mario
Cc: Hans de Goede; Benjamin Tissoires; hotwater...@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
 wrote:


at 01:18, Andy Shevchenko  wrote:


On Wed, Mar 20, 2019 at 6:55 PM Kai-Heng Feng
 wrote:

at 23:39, Hans de Goede  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, 0x,
"\\_SB.GPO1", 0x00, ResourceConsumer, ,
)
{   // Pin list
0x0012
}
})
Name (SBFI, ResourceTemplate ()
{
Interrupt (ResourceConsumer, Level, ActiveLow, 
ExclusiveAndWake, ,, )
{
0x003C,
}
})
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

RE: [PATCH] ELAN touchpad i2c_hid bugs fix

[Mario.Limonciello]

> -Original Message-
> From: Andy Shevchenko 
> Sent: Thursday, March 21, 2019 4:48 AM
> To: Kai-Heng Feng; Limonciello, Mario
> Cc: Hans de Goede; Benjamin Tissoires; hotwater...@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
>  wrote:
> >
> > at 01:18, Andy Shevchenko  wrote:
> >
> > > On Wed, Mar 20, 2019 at 6:55 PM Kai-Heng Feng
> > >  wrote:
> > >> at 23:39, Hans de Goede  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, 
> > 0x,
> >  "\\_SB.GPO1", 0x00, ResourceConsumer, ,
> >  )
> >  {   // Pin list
> >  0x0012
> >  }
> >  })
> >  Name (SBFI, ResourceTemplate ()
> >  {
> >  Interrupt (ResourceConsumer, Level, ActiveLow, 
> > ExclusiveAndWake, ,, )
> >  {
> >  0x003C,
> >  }
> >  })
> >  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 
&

RE: [PATCH] ELAN touchpad i2c_hid bugs fix

[廖崇榮]

Hi Dmitry,

-Original Message-
From: Dmitry Torokhov [mailto:d...@chromium.org] 
Sent: Saturday, March 30, 2019 2:24 AM
To: Hans de Goede; 廖崇榮
Cc: Vladislav Dalechyn; Benjamin Tissoires; Jiri Kosina; 
kai.heng.f...@canonical.com; swb...@chromium.org; bige...@linutronix.de; open 
list:HID CORE LAYER; lkml; hotwater...@tutanota.com
Subject: Re: [PATCH] ELAN touchpad i2c_hid bugs fix

On Fri, Mar 29, 2019 at 5:18 AM Hans de Goede  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  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 
> >>>  wrote:
> >>>>
> >>>> From: Vladislav Dalechyn 
> >>>>
> >>>> 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 t

Re: [PATCH] ELAN touchpad i2c_hid bugs fix

[Dmitry Torokhov]

On Fri, Mar 29, 2019 at 5:18 AM Hans de Goede  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  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
> >>>  wrote:
> 
>  From: Vladislav Dalechyn 
> 
>  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)

>
> 
>
> 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.

>
> 
>
> > 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 

Re: [PATCH] ELAN touchpad i2c_hid bugs fix

[Hans de Goede]

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  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
 wrote:


From: Vladislav Dalechyn 

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.



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.




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 

Re: [PATCH] ELAN touchpad i2c_hid bugs fix

[Dmitry Torokhov]

On Mon, Mar 25, 2019 at 11:23 AM  wrote:
>
> Hi.
>
> Mar 25, 2019, 6:56 PM by d...@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

Re: [PATCH] ELAN touchpad i2c_hid bugs fix

[Dmitry Torokhov]

Hi Hans,

On Mon, Mar 25, 2019 at 9:38 AM Hans de Goede  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
> >  wrote:
> >>
> >> From: Vladislav Dalechyn 
> >>
> >> 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

Re: [PATCH] ELAN touchpad i2c_hid bugs fix

[Hans de Goede]

Hi Dmitry,

On 25-03-19 17:02, Dmitry Torokhov wrote:

Hi Vladislav,

On Mon, Mar 25, 2019 at 5:57 AM Vladislav Dalechyn
 wrote:


From: Vladislav Dalechyn 

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

Re: [PATCH] ELAN touchpad i2c_hid bugs fix

[Dmitry Torokhov]

Hi Vladislav,

On Mon, Mar 25, 2019 at 5:57 AM Vladislav Dalechyn
 wrote:
>
> From: Vladislav Dalechyn 
>
> 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

[PATCH] ELAN touchpad i2c_hid bugs fix

[Vladislav Dalechyn]

From: Vladislav Dalechyn 

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 
Signed-off-by: Vladislav Dalechyn 
---
 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_L9W0x0401
 #define USB_DEVICE_ID_HP_X20x074d
 #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_PMBIT(2)
 #define I2C_HID_QUIRK_DELAY_AFTER_SLEEPBIT(3)
 #define I2C_HID_QUIRK_BOGUS_IRQBIT(4)
+#define I2C_HID_QUIRK_FORCE_TRIGGER_FALLINGBIT(5)
 
 /* flags */
 #define I2C_HID_STARTED0
@@ -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(>dev);
pm_runtime_disable(>dev);
-- 
2.20.1

Re: [PATCH] ELAN touchpad i2c_hid bugs fix

[Benjamin Tissoires]

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
 wrote:
>
> From: h0tw4t3r 

checkpatch.pl complains here:
WARNING: Missing Signed-off-by: line by nominal patch author 'h0tw4t3r
'

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 

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_L9W0x0401
>  #define USB_DEVICE_ID_HP_X20x074d
>  #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_PMBIT(2)
>  #define I2C_HID_QUIRK_DELAY_AFTER_SLEEPBIT(3)
>  #define I2C_HID_QUIRK_BOGUS_IRQBIT(4)
> +#define I2C_HID_QUIRK_FORCE_TRIGGER_FALLINGBIT(5)
>
>  /* flags */
>  #define I2C_HID_STARTED0
> @@ -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 

[PATCH] ELAN touchpad i2c_hid bugs fix

[Vladislav Dalechyn]

From: h0tw4t3r 

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 
---
 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_L9W0x0401
 #define USB_DEVICE_ID_HP_X20x074d
 #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_PMBIT(2)
 #define I2C_HID_QUIRK_DELAY_AFTER_SLEEPBIT(3)
 #define I2C_HID_QUIRK_BOGUS_IRQBIT(4)
+#define I2C_HID_QUIRK_FORCE_TRIGGER_FALLINGBIT(5)
 
 /* flags */
 #define I2C_HID_STARTED0
@@ -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(>dev);
pm_runtime_disable(>dev);
-- 
2.20.1

Re: [PATCH] ELAN touchpad i2c_hid bugs fix

[Hans de Goede]

Hi,

On 24-03-19 18:15, hotwater...@tutanota.com wrote:

Hi,

Sorry, I forgot to put the description.

goto err_foo stuff changes were requested by Benjamin. Please tell if you think 
something is wrong.

Regards, Vladislav.

Here's the final patch:

 From 80576dd7ac193548ba747d287b5ab5606f642d00 Mon Sep 17 00:00:00 2001
From: h0tw4t3r mailto:hotwater...@tutanota.com>>
Date: Wed, 24 Mar 2019 19:14:22 +0200
Subject: [PATCH] ELAN touchpad i2c_hid bugs fix

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.


Some lines in your commit messages are longer then 75 chars, please
add an enter after aprox. 75 chars so that you have no lines longer
then 75 chars.



Signed-off-by: Vladislav Dalechyn mailto:hotwater...@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;


The patch has been completely mangled by your mail client, please
use "git send-email" to send a new version of the patch without
it getting mangled by your email client.

Regards,

Hans

Re: [PATCH] ELAN touchpad i2c_hid bugs fix

[Hans de Goede]

Hi,

On 23-03-19 12:18, hotwater...@tutanota.com wrote:

Guys, I am a little bit confused. Do you mean that it is not kernel related 
issue, and it's the issue of ASUS BIOS?


Technically this is a BIOS bug, the BIOS should properly declare the IRQ as
edge-falling and then everything would just work.

But BIOS bugs is what quirks are for (unfortunately)


And why do you think that triggering edge-falling quirk for this exactly this 
touchpad and exactly ELAN vendor can somehow break other systems?


I'm not seeing anything about breaking other systems in the discussion
(but I may have missed this) what we were discussing is also applying
the quirk to other systems with an Elan touchpad to see if it also
helps with issues on other systems.

Summarizing: Your patch is fine (minus the style issues and the goto error_foo 
stuff)
please fix those issues and submit a new version.

Regards,

Hans






Regards,
Vladislav

Mar 21, 2019, 12:38 PM by hotwater...@tutanota.com:

Hello! Thank you for an answer and for helping me with the patch. It is my 
very first contribution.

About some indent problems: I am sorry, it was mainly caused by my .vimrc, 
and I will edit these errors and send a new patch. Though, there is one typo:

+#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.

If you take a look at original file, you can notice extra space there.

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.

I don't have any.

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.

I tried to understand this as hard as I could, but there may be some 
errors, so please review my latest patch:

 From 80576dd7ac193548ba747d287b5ab5606f642d00 Mon Sep 17 00:00:00 2001
From: h0tw4t3r mailto:hotwater...@tutanota.com>>
Date: Wed, 20 Mar 2019 00:41:22 +0200
Subject: [PATCH] ELAN touchpad i2c_hid bugs fix

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 mailto:hotwater...@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

Re: [PATCH] ELAN touchpad i2c_hid bugs fix

[Andy Shevchenko]

+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
 wrote:
>
> at 01:18, Andy Shevchenko  wrote:
>
> > On Wed, Mar 20, 2019 at 6:55 PM Kai-Heng Feng
> >  wrote:
> >> at 23:39, Hans de Goede  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, 0x,
>  "\\_SB.GPO1", 0x00, ResourceConsumer, ,
>  )
>  {   // Pin list
>  0x0012
>  }
>  })
>  Name (SBFI, ResourceTemplate ()
>  {
>  Interrupt (ResourceConsumer, Level, ActiveLow, 
> ExclusiveAndWake, ,, )
>  {
>  0x003C,
>  }
>  })
>  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
>  // 

Re: [PATCH] ELAN touchpad i2c_hid bugs fix

[Kai Heng Feng]

> On Mar 21, 2019, at 4:55 PM, Andy Shevchenko  
> wrote:
> 
> On Thu, Mar 21, 2019 at 6:08 AM Kai-Heng Feng
>  wrote:
>> at 01:18, Andy Shevchenko  wrote:
>>> On Wed, Mar 20, 2019 at 6:55 PM Kai-Heng Feng
>>>  wrote:
 at 23:39, Hans de Goede  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, 0x,
>> "\\_SB.GPO1", 0x00, ResourceConsumer, ,
>> )
>> {   // Pin list
>> 0x0012
>> }
>> })
>> Name (SBFI, ResourceTemplate ()
>> {
>> Interrupt (ResourceConsumer, Level, ActiveLow, 
>> ExclusiveAndWake, ,, )
>> {
>> 0x003C,
>> }
>> })
> 
>> 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

Re: [PATCH] ELAN touchpad i2c_hid bugs fix

[Hans de Goede]

Hi,

On 21-03-19 05:08, Kai-Heng Feng wrote:

at 01:18, Andy Shevchenko  wrote:


On Wed, Mar 20, 2019 at 6:55 PM Kai-Heng Feng
 wrote:

at 23:39, Hans de Goede  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, 0x,
     "\\_SB.GPO1", 0x00, ResourceConsumer, ,
     )
     {   // Pin list
     0x0012
     }
     })
     Name (SBFI, ResourceTemplate ()
     {
     Interrupt (ResourceConsumer, Level, ActiveLow, 
ExclusiveAndWake, ,, )
     {
     0x003C,
     }
     })


OK, so both interrupt definitions declare the interrupt as Level, ActiveLow, so 
forcing
falling-edge here *might* help too.

Regards,

Hans

Re: [PATCH] ELAN touchpad i2c_hid bugs fix

[Andy Shevchenko]

On Thu, Mar 21, 2019 at 6:08 AM Kai-Heng Feng
 wrote:
> at 01:18, Andy Shevchenko  wrote:
> > On Wed, Mar 20, 2019 at 6:55 PM Kai-Heng Feng
> >  wrote:
> >> at 23:39, Hans de Goede  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, 0x,
>  "\\_SB.GPO1", 0x00, ResourceConsumer, ,
>  )
>  {   // Pin list
>  0x0012
>  }
>  })
>  Name (SBFI, ResourceTemplate ()
>  {
>  Interrupt (ResourceConsumer, Level, ActiveLow, 
> ExclusiveAndWake, ,, )
>  {
>  0x003C,
>  }
>  })

>  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

Re: [PATCH] ELAN touchpad i2c_hid bugs fix

[Kai-Heng Feng]

at 01:18, Andy Shevchenko  wrote:


On Wed, Mar 20, 2019 at 6:55 PM Kai-Heng Feng
 wrote:

at 23:39, Hans de Goede  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, 0x,
"\\_SB.GPO1", 0x00, ResourceConsumer, ,
)
{   // Pin list
0x0012
}
})
Name (SBFI, ResourceTemplate ()
{
Interrupt (ResourceConsumer, Level, ActiveLow, 
ExclusiveAndWake, ,, )
{
0x003C,
}
})
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   
  // .
})
}
}

   

Re: [PATCH] ELAN touchpad i2c_hid bugs fix

[Andy Shevchenko]

On Wed, Mar 20, 2019 at 6:55 PM Kai-Heng Feng
 wrote:
> at 23:39, Hans de Goede  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

Re: [PATCH] ELAN touchpad i2c_hid bugs fix

[Andy Shevchenko]

On Wed, Mar 20, 2019 at 4:38 PM Benjamin Tissoires
 wrote:
> On Wed, Mar 20, 2019 at 2:28 PM  wrote:

> > Patch is created by help of ELANTECH and RedHat guys (Hans De Goede 
> > , Benjamin Tissoires , Andy 
> > Shevchenko ) and me (Vladislav Dalechyn 
> > .
>
> 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

Re: [PATCH] ELAN touchpad i2c_hid bugs fix

[Kai-Heng Feng]

Hi Hans,

at 23:39, Hans de Goede  wrote:


H,

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




The reason why i2c_hid_init_irq was moved is told by Hans De Goede
:
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

Re: [PATCH] ELAN touchpad i2c_hid bugs fix

[Hans de Goede]

H,

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




The reason why i2c_hid_init_irq was moved is told by Hans De Goede
:
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

Re: [PATCH] ELAN touchpad i2c_hid bugs fix

[Benjamin Tissoires]

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  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
> :
> 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 
> , Benjamin Tissoires , Andy 
> Shevchenko ) and me (Vladislav Dalechyn 
> .

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,