Re: [PATCH 1/1] Input - alps: Fix button reporting on the V2 Alps protocol

["Pali Rohár" <pali.rohar@gmail.com>]

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

-- 
Pali Rohár
pali.rohar@gmail.com
--
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