diff mbox

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

Message ID 1438202726-5100-2-git-send-email-cpaul@redhat.com
State New, archived
Headers show

Commit Message

cpaul@redhat.com July 29, 2015, 8:45 p.m. UTC
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(-)

Comments

Dmitry Torokhov July 29, 2015, 9:01 p.m. UTC | #1
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
>
Pali Rohár July 30, 2015, 7:52 a.m. UTC | #2
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?
Hans de Goede July 30, 2015, 1:51 p.m. UTC | #3
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
Pali Rohár July 30, 2015, 2:11 p.m. UTC | #4
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?
Hans de Goede July 30, 2015, 2:18 p.m. UTC | #5
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
Pali Rohár July 30, 2015, 2:28 p.m. UTC | #6
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 July 30, 2015, 2:32 p.m. UTC | #7
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
> > 
> > 
>
Hans de Goede July 30, 2015, 2:38 p.m. UTC | #8
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
Pali Rohár July 30, 2015, 2:45 p.m. UTC | #9
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 mbox

Patch

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