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

[Hans de Goede <hdegoede@redhat.com>]

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