| Message ID | 1438202726-5100-2-git-send-email-cpaul@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Jul 29, 2015 at 04:45:26PM -0400, cpaul@redhat.com wrote: > From: Stephen Chandler Paul <cpaul@redhat.com> > > The data concerning which buttons on the touchpad are held down or not > are in the fourth packet we receive from the mouse, not the first. > > Signed-off-by: Stephen Chandler Paul <cpaul@redhat.com> Let's also make sure it hoes not break Hans' (de Bruin) touchpad. > --- > drivers/input/mouse/alps.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c > index 113d6f1..e2f9b25 100644 > --- a/drivers/input/mouse/alps.c > +++ b/drivers/input/mouse/alps.c > @@ -254,9 +254,9 @@ static void alps_process_packet_v1_v2(struct psmouse *psmouse) > /* Non interleaved V2 dualpoint has separate stick button bits */ > if (priv->proto_version == ALPS_PROTO_V2 && > priv->flags == (ALPS_PASS | ALPS_DUALPOINT)) { > - left |= packet[0] & 1; > - right |= packet[0] & 2; > - middle |= packet[0] & 4; > + left |= packet[3] & 1; > + right |= packet[3] & 2; > + middle |= packet[3] & 4; > } > > alps_report_buttons(dev, dev2, left, right, middle); > -- > 2.4.3 >
On Wednesday 29 July 2015 16:45:26 cpaul@redhat.com wrote: > From: Stephen Chandler Paul <cpaul@redhat.com> > > The data concerning which buttons on the touchpad are held down or not > are in the fourth packet we receive from the mouse, not the first. > > Signed-off-by: Stephen Chandler Paul <cpaul@redhat.com> > --- > drivers/input/mouse/alps.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c > index 113d6f1..e2f9b25 100644 > --- a/drivers/input/mouse/alps.c > +++ b/drivers/input/mouse/alps.c > @@ -254,9 +254,9 @@ static void alps_process_packet_v1_v2(struct psmouse *psmouse) > /* Non interleaved V2 dualpoint has separate stick button bits */ > if (priv->proto_version == ALPS_PROTO_V2 && > priv->flags == (ALPS_PASS | ALPS_DUALPOINT)) { > - left |= packet[0] & 1; > - right |= packet[0] & 2; > - middle |= packet[0] & 4; > + left |= packet[3] & 1; > + right |= packet[3] & 2; > + middle |= packet[3] & 4; > } > > alps_report_buttons(dev, dev2, left, right, middle); This patch will break other ALPS devices... So we cannot accept it :-( Looks like some proto v2 devices report button clicks in packet[0] and some in packet[3]. Any idea how to fix it correctly? Do you have other laptops with proto V2 ALPS devices, to do more tests and dumps and distinguish buttons?
Hi Chandler, On 29-07-15 22:45, cpaul@redhat.com wrote: > From: Stephen Chandler Paul <cpaul@redhat.com> > > The data concerning which buttons on the touchpad are held down or not > are in the fourth packet we receive from the mouse, not the first. > > Signed-off-by: Stephen Chandler Paul <cpaul@redhat.com> > --- > drivers/input/mouse/alps.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c > index 113d6f1..e2f9b25 100644 > --- a/drivers/input/mouse/alps.c > +++ b/drivers/input/mouse/alps.c > @@ -254,9 +254,9 @@ static void alps_process_packet_v1_v2(struct psmouse *psmouse) > /* Non interleaved V2 dualpoint has separate stick button bits */ > if (priv->proto_version == ALPS_PROTO_V2 && > priv->flags == (ALPS_PASS | ALPS_DUALPOINT)) { > - left |= packet[0] & 1; > - right |= packet[0] & 2; > - middle |= packet[0] & 4; > + left |= packet[3] & 1; > + right |= packet[3] & 2; > + middle |= packet[3] & 4; > } > > alps_report_buttons(dev, dev2, left, right, middle); Thanks for taking a look at the recordings, but the above patch is wrong, if you look slightly higher in the lps_process_packet_v1_v2() function there is this: if (priv->proto_version == ALPS_PROTO_V1) { ... } else { left = packet[3] & 1; right = packet[3] & 2; middle = packet[3] & 4; } So with your patch for the devices in question the entire code flow becomes: left = packet[3] & 1; right = packet[3] & 2; middle = packet[3] & 4; left |= packet[3] & 1; right |= packet[3] & 2; middle |= packet[3] & 4; Which is not really helpful for the devices for which I added commit 92bac83dd: https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/input/mouse/alps.c?id=92bac83dd79e60e65c475222e41a992a70434beb and will cause these devices to regress. Since Hans de Bruin's laptop is a Dell Latitude D430 and I saw the same problem and tested my patch on a Dell Latitude D630, it seems the use of the low bits of packet[0] to report the trackpoint buttons separately when the touchpad is active is a Dell specific thing, so I believe that a patch to only activate this code block on Dell's is the right solution for the regression Douglas is seeing. I'll write such a patch and post it shortly. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thursday 30 July 2015 15:51:25 Hans de Goede wrote: > Hi Chandler, > > On 29-07-15 22:45, cpaul@redhat.com wrote: > >From: Stephen Chandler Paul <cpaul@redhat.com> > > > >The data concerning which buttons on the touchpad are held down or not > >are in the fourth packet we receive from the mouse, not the first. > > > >Signed-off-by: Stephen Chandler Paul <cpaul@redhat.com> > >--- > > drivers/input/mouse/alps.c | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > >diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c > >index 113d6f1..e2f9b25 100644 > >--- a/drivers/input/mouse/alps.c > >+++ b/drivers/input/mouse/alps.c > >@@ -254,9 +254,9 @@ static void alps_process_packet_v1_v2(struct psmouse *psmouse) > > /* Non interleaved V2 dualpoint has separate stick button bits */ > > if (priv->proto_version == ALPS_PROTO_V2 && > > priv->flags == (ALPS_PASS | ALPS_DUALPOINT)) { > >- left |= packet[0] & 1; > >- right |= packet[0] & 2; > >- middle |= packet[0] & 4; > >+ left |= packet[3] & 1; > >+ right |= packet[3] & 2; > >+ middle |= packet[3] & 4; > > } > > > > alps_report_buttons(dev, dev2, left, right, middle); > > Thanks for taking a look at the recordings, but the above patch is wrong, > if you look slightly higher in the lps_process_packet_v1_v2() function there > is this: > > if (priv->proto_version == ALPS_PROTO_V1) { > ... > } else { > left = packet[3] & 1; > right = packet[3] & 2; > middle = packet[3] & 4; > } > > So with your patch for the devices in question the entire code flow > becomes: > > left = packet[3] & 1; > right = packet[3] & 2; > middle = packet[3] & 4; > left |= packet[3] & 1; > right |= packet[3] & 2; > middle |= packet[3] & 4; > > Which is not really helpful for the devices for which I added > commit 92bac83dd: > > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/input/mouse/alps.c?id=92bac83dd79e60e65c475222e41a992a70434beb > > and will cause these devices to regress. > > Since Hans de Bruin's laptop is a Dell Latitude D430 and I saw > the same problem and tested my patch on a Dell Latitude D630, > it seems the use of the low bits of packet[0] to report the > trackpoint buttons separately when the touchpad is active is > a Dell specific thing, so I believe that a patch to only > activate this code block on Dell's is the right solution for > the regression Douglas is seeing. > > I'll write such a patch and post it shortly. > > Regards, > > Hans > > > Hans, can you check ec and e7 registers if are same or if they differs?
Hi, On 30-07-15 16:11, Pali Rohár wrote: > On Thursday 30 July 2015 15:51:25 Hans de Goede wrote: >> Hi Chandler, >> >> On 29-07-15 22:45, cpaul@redhat.com wrote: >>> From: Stephen Chandler Paul <cpaul@redhat.com> >>> >>> The data concerning which buttons on the touchpad are held down or not >>> are in the fourth packet we receive from the mouse, not the first. >>> >>> Signed-off-by: Stephen Chandler Paul <cpaul@redhat.com> >>> --- >>> drivers/input/mouse/alps.c | 6 +++--- >>> 1 file changed, 3 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c >>> index 113d6f1..e2f9b25 100644 >>> --- a/drivers/input/mouse/alps.c >>> +++ b/drivers/input/mouse/alps.c >>> @@ -254,9 +254,9 @@ static void alps_process_packet_v1_v2(struct psmouse *psmouse) >>> /* Non interleaved V2 dualpoint has separate stick button bits */ >>> if (priv->proto_version == ALPS_PROTO_V2 && >>> priv->flags == (ALPS_PASS | ALPS_DUALPOINT)) { >>> - left |= packet[0] & 1; >>> - right |= packet[0] & 2; >>> - middle |= packet[0] & 4; >>> + left |= packet[3] & 1; >>> + right |= packet[3] & 2; >>> + middle |= packet[3] & 4; >>> } >>> >>> alps_report_buttons(dev, dev2, left, right, middle); >> >> Thanks for taking a look at the recordings, but the above patch is wrong, >> if you look slightly higher in the lps_process_packet_v1_v2() function there >> is this: >> >> if (priv->proto_version == ALPS_PROTO_V1) { >> ... >> } else { >> left = packet[3] & 1; >> right = packet[3] & 2; >> middle = packet[3] & 4; >> } >> >> So with your patch for the devices in question the entire code flow >> becomes: >> >> left = packet[3] & 1; >> right = packet[3] & 2; >> middle = packet[3] & 4; >> left |= packet[3] & 1; >> right |= packet[3] & 2; >> middle |= packet[3] & 4; >> >> Which is not really helpful for the devices for which I added >> commit 92bac83dd: >> >> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/input/mouse/alps.c?id=92bac83dd79e60e65c475222e41a992a70434beb >> >> and will cause these devices to regress. >> >> Since Hans de Bruin's laptop is a Dell Latitude D430 and I saw >> the same problem and tested my patch on a Dell Latitude D630, >> it seems the use of the low bits of packet[0] to report the >> trackpoint buttons separately when the touchpad is active is >> a Dell specific thing, so I believe that a patch to only >> activate this code block on Dell's is the right solution for >> the regression Douglas is seeing. >> >> I'll write such a patch and post it shortly. >> >> Regards, >> >> Hans >> >> >> > > Hans, can you check ec and e7 registers if are same or if they differs? As Benjamin already pointed out Douglas' touchpad matches this line in alps.c : { { 0x22, 0x02, 0x14 }, 0x00, { ALPS_PROTO_V2, 0xff, 0xff, ALPS_PASS | ALPS_DUALPOINT } }, /* Dell Latitude D600 */ This is a full match, and note the "Dell" in the comment after the line, so it seems that going by the registers is not good enough here, whereas doing a vendor check is actually quite easy, so the patch I just send. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thursday 30 July 2015 16:18:47 Hans de Goede wrote: > Hi, > > On 30-07-15 16:11, Pali Rohár wrote: > >On Thursday 30 July 2015 15:51:25 Hans de Goede wrote: > >>Hi Chandler, > >> > >>On 29-07-15 22:45, cpaul@redhat.com wrote: > >>>From: Stephen Chandler Paul <cpaul@redhat.com> > >>> > >>>The data concerning which buttons on the touchpad are held down or not > >>>are in the fourth packet we receive from the mouse, not the first. > >>> > >>>Signed-off-by: Stephen Chandler Paul <cpaul@redhat.com> > >>>--- > >>> drivers/input/mouse/alps.c | 6 +++--- > >>> 1 file changed, 3 insertions(+), 3 deletions(-) > >>> > >>>diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c > >>>index 113d6f1..e2f9b25 100644 > >>>--- a/drivers/input/mouse/alps.c > >>>+++ b/drivers/input/mouse/alps.c > >>>@@ -254,9 +254,9 @@ static void alps_process_packet_v1_v2(struct psmouse *psmouse) > >>> /* Non interleaved V2 dualpoint has separate stick button bits */ > >>> if (priv->proto_version == ALPS_PROTO_V2 && > >>> priv->flags == (ALPS_PASS | ALPS_DUALPOINT)) { > >>>- left |= packet[0] & 1; > >>>- right |= packet[0] & 2; > >>>- middle |= packet[0] & 4; > >>>+ left |= packet[3] & 1; > >>>+ right |= packet[3] & 2; > >>>+ middle |= packet[3] & 4; > >>> } > >>> > >>> alps_report_buttons(dev, dev2, left, right, middle); > >> > >>Thanks for taking a look at the recordings, but the above patch is wrong, > >>if you look slightly higher in the lps_process_packet_v1_v2() function there > >>is this: > >> > >>if (priv->proto_version == ALPS_PROTO_V1) { > >>... > >>} else { > >> left = packet[3] & 1; > >> right = packet[3] & 2; > >> middle = packet[3] & 4; > >>} > >> > >>So with your patch for the devices in question the entire code flow > >>becomes: > >> > >> left = packet[3] & 1; > >> right = packet[3] & 2; > >> middle = packet[3] & 4; > >> left |= packet[3] & 1; > >> right |= packet[3] & 2; > >> middle |= packet[3] & 4; > >> > >>Which is not really helpful for the devices for which I added > >>commit 92bac83dd: > >> > >>https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/input/mouse/alps.c?id=92bac83dd79e60e65c475222e41a992a70434beb > >> > >>and will cause these devices to regress. > >> > >>Since Hans de Bruin's laptop is a Dell Latitude D430 and I saw > >>the same problem and tested my patch on a Dell Latitude D630, > >>it seems the use of the low bits of packet[0] to report the > >>trackpoint buttons separately when the touchpad is active is > >>a Dell specific thing, so I believe that a patch to only > >>activate this code block on Dell's is the right solution for > >>the regression Douglas is seeing. > >> > >>I'll write such a patch and post it shortly. > >> > >>Regards, > >> > >>Hans > >> > >> > >> > > > >Hans, can you check ec and e7 registers if are same or if they differs? > > As Benjamin already pointed out Douglas' touchpad matches this > line in alps.c : > > { { 0x22, 0x02, 0x14 }, 0x00, { ALPS_PROTO_V2, 0xff, 0xff, ALPS_PASS | ALPS_DUALPOINT } }, /* Dell Latitude D600 */ > > This is a full match No, this is not full match. It just check e7, not ec. This is reason why I asked for both ec and e7 registers of all affected machines. > and note the "Dell" in the comment after the line, > so it seems that going by the registers is not good enough here, whereas > doing a vendor check is actually quite easy, so the patch I just > send. > If registers are different, then I think it is good idea to check for them. I do not like vendor checks in such device driver unless we know that there is some special vendor code/bug in firmware which cause it... > Regards, > > Hans > >
On Thursday 30 July 2015 16:28:39 Pali Rohár wrote: > On Thursday 30 July 2015 16:18:47 Hans de Goede wrote: > > Hi, > > > > On 30-07-15 16:11, Pali Rohár wrote: > > >On Thursday 30 July 2015 15:51:25 Hans de Goede wrote: > > >>Hi Chandler, > > >> > > >>On 29-07-15 22:45, cpaul@redhat.com wrote: > > >>>From: Stephen Chandler Paul <cpaul@redhat.com> > > >>> > > >>>The data concerning which buttons on the touchpad are held down or not > > >>>are in the fourth packet we receive from the mouse, not the first. > > >>> > > >>>Signed-off-by: Stephen Chandler Paul <cpaul@redhat.com> > > >>>--- > > >>> drivers/input/mouse/alps.c | 6 +++--- > > >>> 1 file changed, 3 insertions(+), 3 deletions(-) > > >>> > > >>>diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c > > >>>index 113d6f1..e2f9b25 100644 > > >>>--- a/drivers/input/mouse/alps.c > > >>>+++ b/drivers/input/mouse/alps.c > > >>>@@ -254,9 +254,9 @@ static void alps_process_packet_v1_v2(struct psmouse *psmouse) > > >>> /* Non interleaved V2 dualpoint has separate stick button bits */ > > >>> if (priv->proto_version == ALPS_PROTO_V2 && > > >>> priv->flags == (ALPS_PASS | ALPS_DUALPOINT)) { > > >>>- left |= packet[0] & 1; > > >>>- right |= packet[0] & 2; > > >>>- middle |= packet[0] & 4; > > >>>+ left |= packet[3] & 1; > > >>>+ right |= packet[3] & 2; > > >>>+ middle |= packet[3] & 4; > > >>> } > > >>> > > >>> alps_report_buttons(dev, dev2, left, right, middle); > > >> > > >>Thanks for taking a look at the recordings, but the above patch is wrong, > > >>if you look slightly higher in the lps_process_packet_v1_v2() function there > > >>is this: > > >> > > >>if (priv->proto_version == ALPS_PROTO_V1) { > > >>... > > >>} else { > > >> left = packet[3] & 1; > > >> right = packet[3] & 2; > > >> middle = packet[3] & 4; > > >>} > > >> > > >>So with your patch for the devices in question the entire code flow > > >>becomes: > > >> > > >> left = packet[3] & 1; > > >> right = packet[3] & 2; > > >> middle = packet[3] & 4; > > >> left |= packet[3] & 1; > > >> right |= packet[3] & 2; > > >> middle |= packet[3] & 4; > > >> > > >>Which is not really helpful for the devices for which I added > > >>commit 92bac83dd: > > >> > > >>https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/input/mouse/alps.c?id=92bac83dd79e60e65c475222e41a992a70434beb > > >> > > >>and will cause these devices to regress. > > >> > > >>Since Hans de Bruin's laptop is a Dell Latitude D430 and I saw > > >>the same problem and tested my patch on a Dell Latitude D630, > > >>it seems the use of the low bits of packet[0] to report the > > >>trackpoint buttons separately when the touchpad is active is > > >>a Dell specific thing, so I believe that a patch to only > > >>activate this code block on Dell's is the right solution for > > >>the regression Douglas is seeing. > > >> > > >>I'll write such a patch and post it shortly. > > >> > > >>Regards, > > >> > > >>Hans > > >> > > >> > > >> > > > > > >Hans, can you check ec and e7 registers if are same or if they differs? > > > > As Benjamin already pointed out Douglas' touchpad matches this > > line in alps.c : > > > > { { 0x22, 0x02, 0x14 }, 0x00, { ALPS_PROTO_V2, 0xff, 0xff, ALPS_PASS | ALPS_DUALPOINT } }, /* Dell Latitude D600 */ > > > > This is a full match > > No, this is not full match. It just check e7, not ec. This is reason why > I asked for both ec and e7 registers of all affected machines. > Douglas already sent e7 and ec regs: [ 6.617471] psmouse serio1: alps: E7 report: 22 02 14 [ 6.656974] psmouse serio1: alps: EC report: 10 00 64 Can you post ec regs from your tested Dell machine? > > and note the "Dell" in the comment after the line, > > so it seems that going by the registers is not good enough here, whereas > > doing a vendor check is actually quite easy, so the patch I just > > send. > > > > If registers are different, then I think it is good idea to check for > them. I do not like vendor checks in such device driver unless we know > that there is some special vendor code/bug in firmware which cause it... > > > Regards, > > > > Hans > > > > >
Hi, On 30-07-15 16:32, Pali Rohár wrote: > On Thursday 30 July 2015 16:28:39 Pali Rohár wrote: >> On Thursday 30 July 2015 16:18:47 Hans de Goede wrote: >>> Hi, >>> >>> On 30-07-15 16:11, Pali Rohár wrote: >>>> On Thursday 30 July 2015 15:51:25 Hans de Goede wrote: >>>>> Hi Chandler, >>>>> >>>>> On 29-07-15 22:45, cpaul@redhat.com wrote: >>>>>> From: Stephen Chandler Paul <cpaul@redhat.com> >>>>>> >>>>>> The data concerning which buttons on the touchpad are held down or not >>>>>> are in the fourth packet we receive from the mouse, not the first. >>>>>> >>>>>> Signed-off-by: Stephen Chandler Paul <cpaul@redhat.com> >>>>>> --- >>>>>> drivers/input/mouse/alps.c | 6 +++--- >>>>>> 1 file changed, 3 insertions(+), 3 deletions(-) >>>>>> >>>>>> diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c >>>>>> index 113d6f1..e2f9b25 100644 >>>>>> --- a/drivers/input/mouse/alps.c >>>>>> +++ b/drivers/input/mouse/alps.c >>>>>> @@ -254,9 +254,9 @@ static void alps_process_packet_v1_v2(struct psmouse *psmouse) >>>>>> /* Non interleaved V2 dualpoint has separate stick button bits */ >>>>>> if (priv->proto_version == ALPS_PROTO_V2 && >>>>>> priv->flags == (ALPS_PASS | ALPS_DUALPOINT)) { >>>>>> - left |= packet[0] & 1; >>>>>> - right |= packet[0] & 2; >>>>>> - middle |= packet[0] & 4; >>>>>> + left |= packet[3] & 1; >>>>>> + right |= packet[3] & 2; >>>>>> + middle |= packet[3] & 4; >>>>>> } >>>>>> >>>>>> alps_report_buttons(dev, dev2, left, right, middle); >>>>> >>>>> Thanks for taking a look at the recordings, but the above patch is wrong, >>>>> if you look slightly higher in the lps_process_packet_v1_v2() function there >>>>> is this: >>>>> >>>>> if (priv->proto_version == ALPS_PROTO_V1) { >>>>> ... >>>>> } else { >>>>> left = packet[3] & 1; >>>>> right = packet[3] & 2; >>>>> middle = packet[3] & 4; >>>>> } >>>>> >>>>> So with your patch for the devices in question the entire code flow >>>>> becomes: >>>>> >>>>> left = packet[3] & 1; >>>>> right = packet[3] & 2; >>>>> middle = packet[3] & 4; >>>>> left |= packet[3] & 1; >>>>> right |= packet[3] & 2; >>>>> middle |= packet[3] & 4; >>>>> >>>>> Which is not really helpful for the devices for which I added >>>>> commit 92bac83dd: >>>>> >>>>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/input/mouse/alps.c?id=92bac83dd79e60e65c475222e41a992a70434beb >>>>> >>>>> and will cause these devices to regress. >>>>> >>>>> Since Hans de Bruin's laptop is a Dell Latitude D430 and I saw >>>>> the same problem and tested my patch on a Dell Latitude D630, >>>>> it seems the use of the low bits of packet[0] to report the >>>>> trackpoint buttons separately when the touchpad is active is >>>>> a Dell specific thing, so I believe that a patch to only >>>>> activate this code block on Dell's is the right solution for >>>>> the regression Douglas is seeing. >>>>> >>>>> I'll write such a patch and post it shortly. >>>>> >>>>> Regards, >>>>> >>>>> Hans >>>>> >>>>> >>>>> >>>> >>>> Hans, can you check ec and e7 registers if are same or if they differs? >>> >>> As Benjamin already pointed out Douglas' touchpad matches this >>> line in alps.c : >>> >>> { { 0x22, 0x02, 0x14 }, 0x00, { ALPS_PROTO_V2, 0xff, 0xff, ALPS_PASS | ALPS_DUALPOINT } }, /* Dell Latitude D600 */ >>> >>> This is a full match >> >> No, this is not full match. It just check e7, not ec. This is reason why >> I asked for both ec and e7 registers of all affected machines. >> > > Douglas already sent e7 and ec regs: > [ 6.617471] psmouse serio1: alps: E7 report: 22 02 14 > [ 6.656974] psmouse serio1: alps: EC report: 10 00 64 > > Can you post ec regs from your tested Dell machine? [ 1.906031] psmouse serio1: alps: EC report: 10 00 64 Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thursday 30 July 2015 16:38:47 Hans de Goede wrote: > Hi, > > On 30-07-15 16:32, Pali Rohár wrote: > >On Thursday 30 July 2015 16:28:39 Pali Rohár wrote: > >>On Thursday 30 July 2015 16:18:47 Hans de Goede wrote: > >>>Hi, > >>> > >>>On 30-07-15 16:11, Pali Rohár wrote: > >>>>On Thursday 30 July 2015 15:51:25 Hans de Goede wrote: > >>>>>Hi Chandler, > >>>>> > >>>>>On 29-07-15 22:45, cpaul@redhat.com wrote: > >>>>>>From: Stephen Chandler Paul <cpaul@redhat.com> > >>>>>> > >>>>>>The data concerning which buttons on the touchpad are held down or not > >>>>>>are in the fourth packet we receive from the mouse, not the first. > >>>>>> > >>>>>>Signed-off-by: Stephen Chandler Paul <cpaul@redhat.com> > >>>>>>--- > >>>>>> drivers/input/mouse/alps.c | 6 +++--- > >>>>>> 1 file changed, 3 insertions(+), 3 deletions(-) > >>>>>> > >>>>>>diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c > >>>>>>index 113d6f1..e2f9b25 100644 > >>>>>>--- a/drivers/input/mouse/alps.c > >>>>>>+++ b/drivers/input/mouse/alps.c > >>>>>>@@ -254,9 +254,9 @@ static void alps_process_packet_v1_v2(struct psmouse *psmouse) > >>>>>> /* Non interleaved V2 dualpoint has separate stick button bits */ > >>>>>> if (priv->proto_version == ALPS_PROTO_V2 && > >>>>>> priv->flags == (ALPS_PASS | ALPS_DUALPOINT)) { > >>>>>>- left |= packet[0] & 1; > >>>>>>- right |= packet[0] & 2; > >>>>>>- middle |= packet[0] & 4; > >>>>>>+ left |= packet[3] & 1; > >>>>>>+ right |= packet[3] & 2; > >>>>>>+ middle |= packet[3] & 4; > >>>>>> } > >>>>>> > >>>>>> alps_report_buttons(dev, dev2, left, right, middle); > >>>>> > >>>>>Thanks for taking a look at the recordings, but the above patch is wrong, > >>>>>if you look slightly higher in the lps_process_packet_v1_v2() function there > >>>>>is this: > >>>>> > >>>>>if (priv->proto_version == ALPS_PROTO_V1) { > >>>>>... > >>>>>} else { > >>>>> left = packet[3] & 1; > >>>>> right = packet[3] & 2; > >>>>> middle = packet[3] & 4; > >>>>>} > >>>>> > >>>>>So with your patch for the devices in question the entire code flow > >>>>>becomes: > >>>>> > >>>>> left = packet[3] & 1; > >>>>> right = packet[3] & 2; > >>>>> middle = packet[3] & 4; > >>>>> left |= packet[3] & 1; > >>>>> right |= packet[3] & 2; > >>>>> middle |= packet[3] & 4; > >>>>> > >>>>>Which is not really helpful for the devices for which I added > >>>>>commit 92bac83dd: > >>>>> > >>>>>https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/input/mouse/alps.c?id=92bac83dd79e60e65c475222e41a992a70434beb > >>>>> > >>>>>and will cause these devices to regress. > >>>>> > >>>>>Since Hans de Bruin's laptop is a Dell Latitude D430 and I saw > >>>>>the same problem and tested my patch on a Dell Latitude D630, > >>>>>it seems the use of the low bits of packet[0] to report the > >>>>>trackpoint buttons separately when the touchpad is active is > >>>>>a Dell specific thing, so I believe that a patch to only > >>>>>activate this code block on Dell's is the right solution for > >>>>>the regression Douglas is seeing. > >>>>> > >>>>>I'll write such a patch and post it shortly. > >>>>> > >>>>>Regards, > >>>>> > >>>>>Hans > >>>>> > >>>>> > >>>>> > >>>> > >>>>Hans, can you check ec and e7 registers if are same or if they differs? > >>> > >>>As Benjamin already pointed out Douglas' touchpad matches this > >>>line in alps.c : > >>> > >>>{ { 0x22, 0x02, 0x14 }, 0x00, { ALPS_PROTO_V2, 0xff, 0xff, ALPS_PASS | ALPS_DUALPOINT } }, /* Dell Latitude D600 */ > >>> > >>>This is a full match > >> > >>No, this is not full match. It just check e7, not ec. This is reason why > >>I asked for both ec and e7 registers of all affected machines. > >> > > > >Douglas already sent e7 and ec regs: > >[ 6.617471] psmouse serio1: alps: E7 report: 22 02 14 > >[ 6.656974] psmouse serio1: alps: EC report: 10 00 64 > > > >Can you post ec regs from your tested Dell machine? > > [ 1.906031] psmouse serio1: alps: EC report: 10 00 64 > > Regards, > > Hans Ok, in this case DMI based check is maybe only possible solution.
diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c index 113d6f1..e2f9b25 100644 --- a/drivers/input/mouse/alps.c +++ b/drivers/input/mouse/alps.c @@ -254,9 +254,9 @@ static void alps_process_packet_v1_v2(struct psmouse *psmouse) /* Non interleaved V2 dualpoint has separate stick button bits */ if (priv->proto_version == ALPS_PROTO_V2 && priv->flags == (ALPS_PASS | ALPS_DUALPOINT)) { - left |= packet[0] & 1; - right |= packet[0] & 2; - middle |= packet[0] & 4; + left |= packet[3] & 1; + right |= packet[3] & 2; + middle |= packet[3] & 4; } alps_report_buttons(dev, dev2, left, right, middle);