Regression since commit 92bac83

Regression since commit 92bac83

[Larry Finger]

Hi,

I recently upgraded the kernel in a Dell Latitude D600 and found that the 
touchpad clicks failed. The problem was bisected to commit 
92bac83dd79e60e65c475222e41a992a70434beb ("Input: alps - non interleaved V2 
dualpoint has separate stick button bits"). The laptop has a combination 
touchpad and control stick. For this device, the following values are found:

priv->protoversion is 0x200 (ALPS_PROTO_V2)
priv->flags is 0x6 (ALPS_DUALPOINT | ALPS_PASS)

As a result, the new code added in this patch is executed, and left, right, and 
middle are updated. Once this code is introduced, a left click causes some event 
as it will wake a sleeping screen, but not select any windows or do anything useful.

Please advise on what information would be needed to help debug this problem.

Thanks,

Larry

Re: Regression since commit 92bac83

[Hans de Goede]

Hi,

On 19-10-15 01:59, Larry Finger wrote:
> Hi,
>
> I recently upgraded the kernel in a Dell Latitude D600 and found that the touchpad clicks failed. The problem was bisected to commit 92bac83dd79e60e65c475222e41a992a70434beb ("Input: alps - non interleaved V2 dualpoint has separate stick button bits"). The laptop has a combination touchpad and control stick. For this device, the following values are found:
>
> priv->protoversion is 0x200 (ALPS_PROTO_V2)
> priv->flags is 0x6 (ALPS_DUALPOINT | ALPS_PASS)
>
> As a result, the new code added in this patch is executed, and left, right, and middle are updated. Once this code is introduced, a left click causes some event as it will wake a sleeping screen, but not select any windows or do anything useful.
>
> Please advise on what information would be needed to help debug this problem.

Can you build a recent upstream kernel from source, and when building it
comment out these lines in drivers/input/mouse/alps.c, around lines 2555 - 2556

         if (dmi_name_in_vendors("Dell"))
                 priv->flags |= ALPS_DELL;

That should fix things, if that fixes things we need to rename the flag
and move to a list of dmi-matched models (rather then vendor) where the new behavior
introduced by the patch causing you problems is actually necessary.

Step 1 is confirming that not setting the flag fixes things for you,
if you can get back to us confirming that, then I'll whip up a patch
to switch to model matching (which is not ideal, but seems to be
necessary).

Regards,

Hans

Re: Regression since commit 92bac83

[Larry Finger]

On 10/19/2015 03:08 AM, Hans de Goede wrote:
> Hi,
>
> On 19-10-15 01:59, Larry Finger wrote:
>> Hi,
>>
>> I recently upgraded the kernel in a Dell Latitude D600 and found that the
>> touchpad clicks failed. The problem was bisected to commit
>> 92bac83dd79e60e65c475222e41a992a70434beb ("Input: alps - non interleaved V2
>> dualpoint has separate stick button bits"). The laptop has a combination
>> touchpad and control stick. For this device, the following values are found:
>>
>> priv->protoversion is 0x200 (ALPS_PROTO_V2)
>> priv->flags is 0x6 (ALPS_DUALPOINT | ALPS_PASS)
>>
>> As a result, the new code added in this patch is executed, and left, right,
>> and middle are updated. Once this code is introduced, a left click causes some
>> event as it will wake a sleeping screen, but not select any windows or do
>> anything useful.
>>
>> Please advise on what information would be needed to help debug this problem.
>
> Can you build a recent upstream kernel from source, and when building it
> comment out these lines in drivers/input/mouse/alps.c, around lines 2555 - 2556
>
>          if (dmi_name_in_vendors("Dell"))
>                  priv->flags |= ALPS_DELL;
>
> That should fix things, if that fixes things we need to rename the flag
> and move to a list of dmi-matched models (rather then vendor) where the new
> behavior
> introduced by the patch causing you problems is actually necessary.
>
> Step 1 is confirming that not setting the flag fixes things for you,
> if you can get back to us confirming that, then I'll whip up a patch
> to switch to model matching (which is not ideal, but seems to be
> necessary).

Thanks for the quick response. Removing the two lines mentioned above restored 
correct touchpad operation with kernel 4.2.0. It seems that the Latitude D600 is 
different than other Dell models.

Larry

Re: Regression since commit 92bac83

[Dmitry Torokhov]

On Mon, Oct 19, 2015 at 10:55:20AM -0500, Larry Finger wrote:
> On 10/19/2015 03:08 AM, Hans de Goede wrote:
> >Hi,
> >
> >On 19-10-15 01:59, Larry Finger wrote:
> >>Hi,
> >>
> >>I recently upgraded the kernel in a Dell Latitude D600 and found that the
> >>touchpad clicks failed. The problem was bisected to commit
> >>92bac83dd79e60e65c475222e41a992a70434beb ("Input: alps - non interleaved V2
> >>dualpoint has separate stick button bits"). The laptop has a combination
> >>touchpad and control stick. For this device, the following values are found:
> >>
> >>priv->protoversion is 0x200 (ALPS_PROTO_V2)
> >>priv->flags is 0x6 (ALPS_DUALPOINT | ALPS_PASS)
> >>
> >>As a result, the new code added in this patch is executed, and left, right,
> >>and middle are updated. Once this code is introduced, a left click causes some
> >>event as it will wake a sleeping screen, but not select any windows or do
> >>anything useful.
> >>
> >>Please advise on what information would be needed to help debug this problem.
> >
> >Can you build a recent upstream kernel from source, and when building it
> >comment out these lines in drivers/input/mouse/alps.c, around lines 2555 - 2556
> >
> >         if (dmi_name_in_vendors("Dell"))
> >                 priv->flags |= ALPS_DELL;
> >
> >That should fix things, if that fixes things we need to rename the flag
> >and move to a list of dmi-matched models (rather then vendor) where the new
> >behavior
> >introduced by the patch causing you problems is actually necessary.
> >
> >Step 1 is confirming that not setting the flag fixes things for you,
> >if you can get back to us confirming that, then I'll whip up a patch
> >to switch to model matching (which is not ideal, but seems to be
> >necessary).
> 
> Thanks for the quick response. Removing the two lines mentioned
> above restored correct touchpad operation with kernel 4.2.0. It
> seems that the Latitude D600 is different than other Dell models.

I wonder if we should not revert all these patches splitting what once
was one relative input device into separate trackstick/external mouse.
They seem to cause a lot of troubles for little benefit. Pali?

Thanks.

-- 
Dmitry

Re: Regression since commit 92bac83

[Hans de Goede]

Hi,

On 19-10-15 18:51, Dmitry Torokhov wrote:
> On Mon, Oct 19, 2015 at 10:55:20AM -0500, Larry Finger wrote:
>> On 10/19/2015 03:08 AM, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 19-10-15 01:59, Larry Finger wrote:
>>>> Hi,
>>>>
>>>> I recently upgraded the kernel in a Dell Latitude D600 and found that the
>>>> touchpad clicks failed. The problem was bisected to commit
>>>> 92bac83dd79e60e65c475222e41a992a70434beb ("Input: alps - non interleaved V2
>>>> dualpoint has separate stick button bits"). The laptop has a combination
>>>> touchpad and control stick. For this device, the following values are found:
>>>>
>>>> priv->protoversion is 0x200 (ALPS_PROTO_V2)
>>>> priv->flags is 0x6 (ALPS_DUALPOINT | ALPS_PASS)
>>>>
>>>> As a result, the new code added in this patch is executed, and left, right,
>>>> and middle are updated. Once this code is introduced, a left click causes some
>>>> event as it will wake a sleeping screen, but not select any windows or do
>>>> anything useful.
>>>>
>>>> Please advise on what information would be needed to help debug this problem.
>>>
>>> Can you build a recent upstream kernel from source, and when building it
>>> comment out these lines in drivers/input/mouse/alps.c, around lines 2555 - 2556
>>>
>>>          if (dmi_name_in_vendors("Dell"))
>>>                  priv->flags |= ALPS_DELL;
>>>
>>> That should fix things, if that fixes things we need to rename the flag
>>> and move to a list of dmi-matched models (rather then vendor) where the new
>>> behavior
>>> introduced by the patch causing you problems is actually necessary.
>>>
>>> Step 1 is confirming that not setting the flag fixes things for you,
>>> if you can get back to us confirming that, then I'll whip up a patch
>>> to switch to model matching (which is not ideal, but seems to be
>>> necessary).
>>
>> Thanks for the quick response. Removing the two lines mentioned
>> above restored correct touchpad operation with kernel 4.2.0. It
>> seems that the Latitude D600 is different than other Dell models.
>
> I wonder if we should not revert all these patches splitting what once
> was one relative input device into separate trackstick/external mouse.
> They seem to cause a lot of troubles for little benefit. Pali?

This is unrelated to this splitting, on v2 devices (which we are talking
about here) we always combine all button status, and report the buttons
on the device from which we are actually getting packets atm (only one
sends packets at a time on these specific laptops) and then do some
magic to make sure we send the release on the right device, this all
works well.

The problem is that when using the touchpad (so the touchpad and not
the stick is generating packets) combined with the trackpoint buttons,
the buttons get reported in a different place in the touchpad packet
then when using the touchpad buttons themselves, which is what
commit 92bac83 fixes.

AFAICT we've had the bug which commit 92bac83 fixes since more or less
day 1 of the alps driver, but no one noticed and/or reported the problem
since reproducing the problem is somewhat hard.

Another reason why this problem was likely not noticed until recently
is because it seems it affects only a limited number of models. At first
I thought it would affect all non-interleaved dualpoint v2 devices, then
when we got bug reports of commit 92bac83 causing issues for some non
Dell laptops, I added the DELL flag and limited the fix to only Dell
laptops, and now it seems the separate reporting of the trackpoint buttons
in the touchpad packets only happens on some model Dells.

TL;DR: this is not caused by Pali's patches, if you want to blame anyone
it would be me. In hindsight the fix commit 92bac83 introduces should
have been behind a quirk flag which is only set on specific models.

I will write a patch this week actually changing the fix to use such
a quirk flag.

Regards,

Hans

Re: Regression since commit 92bac83

[Pali Rohár]

On Monday 19 October 2015 09:51:28 Dmitry Torokhov wrote:
> On Mon, Oct 19, 2015 at 10:55:20AM -0500, Larry Finger wrote:
> > On 10/19/2015 03:08 AM, Hans de Goede wrote:
> > >Hi,
> > >
> > >On 19-10-15 01:59, Larry Finger wrote:
> > >>Hi,
> > >>
> > >>I recently upgraded the kernel in a Dell Latitude D600 and found that the
> > >>touchpad clicks failed. The problem was bisected to commit
> > >>92bac83dd79e60e65c475222e41a992a70434beb ("Input: alps - non interleaved V2
> > >>dualpoint has separate stick button bits"). The laptop has a combination
> > >>touchpad and control stick. For this device, the following values are found:
> > >>
> > >>priv->protoversion is 0x200 (ALPS_PROTO_V2)
> > >>priv->flags is 0x6 (ALPS_DUALPOINT | ALPS_PASS)
> > >>
> > >>As a result, the new code added in this patch is executed, and left, right,
> > >>and middle are updated. Once this code is introduced, a left click causes some
> > >>event as it will wake a sleeping screen, but not select any windows or do
> > >>anything useful.
> > >>
> > >>Please advise on what information would be needed to help debug this problem.
> > >
> > >Can you build a recent upstream kernel from source, and when building it
> > >comment out these lines in drivers/input/mouse/alps.c, around lines 2555 - 2556
> > >
> > >         if (dmi_name_in_vendors("Dell"))
> > >                 priv->flags |= ALPS_DELL;
> > >
> > >That should fix things, if that fixes things we need to rename the flag
> > >and move to a list of dmi-matched models (rather then vendor) where the new
> > >behavior
> > >introduced by the patch causing you problems is actually necessary.
> > >
> > >Step 1 is confirming that not setting the flag fixes things for you,
> > >if you can get back to us confirming that, then I'll whip up a patch
> > >to switch to model matching (which is not ideal, but seems to be
> > >necessary).
> > 
> > Thanks for the quick response. Removing the two lines mentioned
> > above restored correct touchpad operation with kernel 4.2.0. It
> > seems that the Latitude D600 is different than other Dell models.
> 
> I wonder if we should not revert all these patches splitting what once
> was one relative input device into separate trackstick/external mouse.
> They seem to cause a lot of troubles for little benefit. Pali?
> 
> Thanks.
> 

Hi Dmitry! I think that this bug is not related to my separation
patches, but rather to approach which is trying to fix some obscure bug
on some Dell machines...

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

Re: Regression since commit 92bac83

[Hans de Goede]

[-- Attachment #1: Type: text/plain, Size: 2085 bytes --]

Hi,

On 19-10-15 17:55, Larry Finger wrote:
> On 10/19/2015 03:08 AM, Hans de Goede wrote:
>> Hi,
>>
>> On 19-10-15 01:59, Larry Finger wrote:
>>> Hi,
>>>
>>> I recently upgraded the kernel in a Dell Latitude D600 and found that the
>>> touchpad clicks failed. The problem was bisected to commit
>>> 92bac83dd79e60e65c475222e41a992a70434beb ("Input: alps - non interleaved V2
>>> dualpoint has separate stick button bits"). The laptop has a combination
>>> touchpad and control stick. For this device, the following values are found:
>>>
>>> priv->protoversion is 0x200 (ALPS_PROTO_V2)
>>> priv->flags is 0x6 (ALPS_DUALPOINT | ALPS_PASS)
>>>
>>> As a result, the new code added in this patch is executed, and left, right,
>>> and middle are updated. Once this code is introduced, a left click causes some
>>> event as it will wake a sleeping screen, but not select any windows or do
>>> anything useful.
>>>
>>> Please advise on what information would be needed to help debug this problem.
>>
>> Can you build a recent upstream kernel from source, and when building it
>> comment out these lines in drivers/input/mouse/alps.c, around lines 2555 - 2556
>>
>>          if (dmi_name_in_vendors("Dell"))
>>                  priv->flags |= ALPS_DELL;
>>
>> That should fix things, if that fixes things we need to rename the flag
>> and move to a list of dmi-matched models (rather then vendor) where the new
>> behavior
>> introduced by the patch causing you problems is actually necessary.
>>
>> Step 1 is confirming that not setting the flag fixes things for you,
>> if you can get back to us confirming that, then I'll whip up a patch
>> to switch to model matching (which is not ideal, but seems to be
>> necessary).
>
> Thanks for the quick response. Removing the two lines mentioned above restored correct touchpad operation with kernel 4.2.0. It seems that the Latitude D600 is different than other Dell models.

Thanks, can you undo the commenting of those 2 lines, apply the attached
patch, and then build, install and test, and see if this fixes things ?

Thanks & Regards,

Hans

[-- Attachment #2: 0001-alps-Only-the-Dell-Latitude-D420-430-620-630-have-se.patch --]
[-- Type: text/x-patch, Size: 4108 bytes --]

>From 5d21a8004260c3e6287bde81c2a9e8f80144e77c Mon Sep 17 00:00:00 2001
From: Hans de Goede <hdegoede@redhat.com>
Date: Tue, 20 Oct 2015 11:12:07 +0200
Subject: [PATCH] alps: Only the Dell Latitude D420/430/620/630 have separate
 stick button bits

commit 92bac83dd79e ("Input: alps - non interleaved V2 dualpoint has
separate stick button bits") assumes that all alps v2 non-interleaved
dual point setups have the separate stick button bits.

Later we limited this to Dell laptops only because of reports that this
broke things on non Dell laptops. Now it turns out that this breaks things
on the Dell Latitude D600 too. So it seems that only the Dell Latitude
D420/430/620/630, which all share the same touchpad / stick combo,
have these separate bits.

This patch limits the checking of the separate bits to only these models
fixing regressions with other models.

Reported-and-tested-by: Larry Finger <Larry.Finger@lwfinger.net>
Cc: stable@vger.kernel.org
Tested-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/input/mouse/alps.c | 48 ++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 42 insertions(+), 6 deletions(-)

diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
index 4d24686..41e6cb5 100644
--- a/drivers/input/mouse/alps.c
+++ b/drivers/input/mouse/alps.c
@@ -100,7 +100,7 @@ static const struct alps_nibble_commands alps_v6_nibble_commands[] = {
 #define ALPS_FOUR_BUTTONS	0x40	/* 4 direction button present */
 #define ALPS_PS2_INTERLEAVED	0x80	/* 3-byte PS/2 packet interleaved with
 					   6-byte ALPS packet */
-#define ALPS_DELL		0x100	/* device is a Dell laptop */
+#define ALPS_STICK_BITS		0x100	/* separate stick button bits */
 #define ALPS_BUTTONPAD		0x200	/* device is a clickpad */
 
 static const struct alps_model_info alps_model_data[] = {
@@ -159,6 +159,43 @@ static const struct alps_protocol_info alps_v8_protocol_data = {
 	ALPS_PROTO_V8, 0x18, 0x18, 0
 };
 
+/*
+ * Some v2 models report the stick buttons in separate bits
+ */
+static const struct dmi_system_id alps_dmi_has_separate_stick_buttons[] = {
+#if defined(CONFIG_DMI) && defined(CONFIG_X86)
+	{
+		/* Extrapolated from other entries */
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude D420"),
+		},
+	},
+	{
+		/* Reported-by: Hans de Bruin <jmdebruin@xmsnet.nl> */
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude D430"),
+		},
+	},
+	{
+		/* Reported-by: Hans de Goede <hdegoede@redhat.com> */
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude D620"),
+		},
+	},
+	{
+		/* Extrapolated from other entries */
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude D630"),
+		},
+	},
+#endif
+	{ }
+};
+
 static void alps_set_abs_params_st(struct alps_data *priv,
 				   struct input_dev *dev1);
 static void alps_set_abs_params_semi_mt(struct alps_data *priv,
@@ -253,9 +290,8 @@ static void alps_process_packet_v1_v2(struct psmouse *psmouse)
 		return;
 	}
 
-	/* Dell non interleaved V2 dualpoint has separate stick button bits */
-	if (priv->proto_version == ALPS_PROTO_V2 &&
-	    priv->flags == (ALPS_DELL | ALPS_PASS | ALPS_DUALPOINT)) {
+	/* Some models have separate stick button bits */
+	if (priv->flags & ALPS_STICK_BITS) {
 		left |= packet[0] & 1;
 		right |= packet[0] & 2;
 		middle |= packet[0] & 4;
@@ -2552,8 +2588,6 @@ static int alps_set_protocol(struct psmouse *psmouse,
 	priv->byte0 = protocol->byte0;
 	priv->mask0 = protocol->mask0;
 	priv->flags = protocol->flags;
-	if (dmi_name_in_vendors("Dell"))
-		priv->flags |= ALPS_DELL;
 
 	priv->x_max = 2000;
 	priv->y_max = 1400;
@@ -2568,6 +2602,8 @@ static int alps_set_protocol(struct psmouse *psmouse,
 		priv->set_abs_params = alps_set_abs_params_st;
 		priv->x_max = 1023;
 		priv->y_max = 767;
+		if (dmi_check_system(alps_dmi_has_separate_stick_buttons))
+			priv->flags |= ALPS_STICK_BITS;
 		break;
 
 	case ALPS_PROTO_V3:
-- 
2.5.0

Re: Regression since commit 92bac83

[Larry Finger]

On 10/20/2015 06:39 AM, Hans de Goede wrote:
> Hi,
>
> On 19-10-15 17:55, Larry Finger wrote:
>> On 10/19/2015 03:08 AM, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 19-10-15 01:59, Larry Finger wrote:
>>>> Hi,
>>>>
>>>> I recently upgraded the kernel in a Dell Latitude D600 and found that the
>>>> touchpad clicks failed. The problem was bisected to commit
>>>> 92bac83dd79e60e65c475222e41a992a70434beb ("Input: alps - non interleaved V2
>>>> dualpoint has separate stick button bits"). The laptop has a combination
>>>> touchpad and control stick. For this device, the following values are found:
>>>>
>>>> priv->protoversion is 0x200 (ALPS_PROTO_V2)
>>>> priv->flags is 0x6 (ALPS_DUALPOINT | ALPS_PASS)
>>>>
>>>> As a result, the new code added in this patch is executed, and left, right,
>>>> and middle are updated. Once this code is introduced, a left click causes some
>>>> event as it will wake a sleeping screen, but not select any windows or do
>>>> anything useful.
>>>>
>>>> Please advise on what information would be needed to help debug this problem.
>>>
>>> Can you build a recent upstream kernel from source, and when building it
>>> comment out these lines in drivers/input/mouse/alps.c, around lines 2555 - 2556
>>>
>>>          if (dmi_name_in_vendors("Dell"))
>>>                  priv->flags |= ALPS_DELL;
>>>
>>> That should fix things, if that fixes things we need to rename the flag
>>> and move to a list of dmi-matched models (rather then vendor) where the new
>>> behavior
>>> introduced by the patch causing you problems is actually necessary.
>>>
>>> Step 1 is confirming that not setting the flag fixes things for you,
>>> if you can get back to us confirming that, then I'll whip up a patch
>>> to switch to model matching (which is not ideal, but seems to be
>>> necessary).
>>
>> Thanks for the quick response. Removing the two lines mentioned above restored
>> correct touchpad operation with kernel 4.2.0. It seems that the Latitude D600
>> is different than other Dell models.
>
> Thanks, can you undo the commenting of those 2 lines, apply the attached
> patch, and then build, install and test, and see if this fixes things ?

Hans,

The patch fixes my problem. Hopefully only the D420, D430, D620 and D630 are the 
only models that need the ALPS_STICK_BITS flag.

Thanks again for the prompt attention.

Larry

Re: Regression since commit 92bac83

[Pali Rohár]

On Tuesday 20 October 2015 13:39:05 Hans de Goede wrote:
> From 5d21a8004260c3e6287bde81c2a9e8f80144e77c Mon Sep 17 00:00:00 2001
> From: Hans de Goede <hdegoede@redhat.com>
> Date: Tue, 20 Oct 2015 11:12:07 +0200
> Subject: [PATCH] alps: Only the Dell Latitude D420/430/620/630 have separate
>  stick button bits
> 
> commit 92bac83dd79e ("Input: alps - non interleaved V2 dualpoint has
> separate stick button bits") assumes that all alps v2 non-interleaved
> dual point setups have the separate stick button bits.
> 
> Later we limited this to Dell laptops only because of reports that this
> broke things on non Dell laptops. Now it turns out that this breaks things
> on the Dell Latitude D600 too. So it seems that only the Dell Latitude
> D420/430/620/630, which all share the same touchpad / stick combo,
> have these separate bits.
> 
> This patch limits the checking of the separate bits to only these models
> fixing regressions with other models.
> 
> Reported-and-tested-by: Larry Finger <Larry.Finger@lwfinger.net>
> Cc: stable@vger.kernel.org
> Tested-by: Hans de Goede <hdegoede@redhat.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/input/mouse/alps.c | 48 ++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 42 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
> index 4d24686..41e6cb5 100644
> --- a/drivers/input/mouse/alps.c
> +++ b/drivers/input/mouse/alps.c
> @@ -100,7 +100,7 @@ static const struct alps_nibble_commands alps_v6_nibble_commands[] = {
>  #define ALPS_FOUR_BUTTONS	0x40	/* 4 direction button present */
>  #define ALPS_PS2_INTERLEAVED	0x80	/* 3-byte PS/2 packet interleaved with
>  					   6-byte ALPS packet */
> -#define ALPS_DELL		0x100	/* device is a Dell laptop */
> +#define ALPS_STICK_BITS		0x100	/* separate stick button bits */
>  #define ALPS_BUTTONPAD		0x200	/* device is a clickpad */
>  
>  static const struct alps_model_info alps_model_data[] = {
> @@ -159,6 +159,43 @@ static const struct alps_protocol_info alps_v8_protocol_data = {
>  	ALPS_PROTO_V8, 0x18, 0x18, 0
>  };
>  
> +/*
> + * Some v2 models report the stick buttons in separate bits
> + */
> +static const struct dmi_system_id alps_dmi_has_separate_stick_buttons[] = {
> +#if defined(CONFIG_DMI) && defined(CONFIG_X86)
> +	{
> +		/* Extrapolated from other entries */
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude D420"),
> +		},
> +	},
> +	{
> +		/* Reported-by: Hans de Bruin <jmdebruin@xmsnet.nl> */
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude D430"),
> +		},
> +	},
> +	{
> +		/* Reported-by: Hans de Goede <hdegoede@redhat.com> */
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude D620"),
> +		},
> +	},
> +	{
> +		/* Extrapolated from other entries */
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude D630"),
> +		},
> +	},
> +#endif
> +	{ }
> +};
> +

Hi! Are you sure that above machines do not have variants without
ALPS_DUALPOINT or without ALPS_PASS? Because if yes, then we could see
another break.

>  static void alps_set_abs_params_st(struct alps_data *priv,
>  				   struct input_dev *dev1);
>  static void alps_set_abs_params_semi_mt(struct alps_data *priv,
> @@ -253,9 +290,8 @@ static void alps_process_packet_v1_v2(struct psmouse *psmouse)
>  		return;
>  	}
>  
> -	/* Dell non interleaved V2 dualpoint has separate stick button bits */
> -	if (priv->proto_version == ALPS_PROTO_V2 &&
> -	    priv->flags == (ALPS_DELL | ALPS_PASS | ALPS_DUALPOINT)) {
> +	/* Some models have separate stick button bits */
> +	if (priv->flags & ALPS_STICK_BITS) {

Previous code checked at this place if device has also flags ALPS_PASS
and ALPS_DUALPOINT. Now ALPS_STICK_BITS is defined only if ALPS_PROTO_V2
and DMI has specific Dell machine.

>  		left |= packet[0] & 1;
>  		right |= packet[0] & 2;
>  		middle |= packet[0] & 4;
> @@ -2552,8 +2588,6 @@ static int alps_set_protocol(struct psmouse *psmouse,
>  	priv->byte0 = protocol->byte0;
>  	priv->mask0 = protocol->mask0;
>  	priv->flags = protocol->flags;
> -	if (dmi_name_in_vendors("Dell"))
> -		priv->flags |= ALPS_DELL;
>  
>  	priv->x_max = 2000;
>  	priv->y_max = 1400;
> @@ -2568,6 +2602,8 @@ static int alps_set_protocol(struct psmouse *psmouse,
>  		priv->set_abs_params = alps_set_abs_params_st;
>  		priv->x_max = 1023;
>  		priv->y_max = 767;
> +		if (dmi_check_system(alps_dmi_has_separate_stick_buttons))
> +			priv->flags |= ALPS_STICK_BITS;
>  		break;
>  
>  	case ALPS_PROTO_V3:

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

Re: Regression since commit 92bac83

[Hans de Goede]

Hi,

On 21-10-15 10:19, Pali Rohár wrote:
> On Tuesday 20 October 2015 13:39:05 Hans de Goede wrote:
>>  From 5d21a8004260c3e6287bde81c2a9e8f80144e77c Mon Sep 17 00:00:00 2001
>> From: Hans de Goede <hdegoede@redhat.com>
>> Date: Tue, 20 Oct 2015 11:12:07 +0200
>> Subject: [PATCH] alps: Only the Dell Latitude D420/430/620/630 have separate
>>   stick button bits
>>
>> commit 92bac83dd79e ("Input: alps - non interleaved V2 dualpoint has
>> separate stick button bits") assumes that all alps v2 non-interleaved
>> dual point setups have the separate stick button bits.
>>
>> Later we limited this to Dell laptops only because of reports that this
>> broke things on non Dell laptops. Now it turns out that this breaks things
>> on the Dell Latitude D600 too. So it seems that only the Dell Latitude
>> D420/430/620/630, which all share the same touchpad / stick combo,
>> have these separate bits.
>>
>> This patch limits the checking of the separate bits to only these models
>> fixing regressions with other models.
>>
>> Reported-and-tested-by: Larry Finger <Larry.Finger@lwfinger.net>
>> Cc: stable@vger.kernel.org
>> Tested-by: Hans de Goede <hdegoede@redhat.com>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   drivers/input/mouse/alps.c | 48 ++++++++++++++++++++++++++++++++++++++++------
>>   1 file changed, 42 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
>> index 4d24686..41e6cb5 100644
>> --- a/drivers/input/mouse/alps.c
>> +++ b/drivers/input/mouse/alps.c
>> @@ -100,7 +100,7 @@ static const struct alps_nibble_commands alps_v6_nibble_commands[] = {
>>   #define ALPS_FOUR_BUTTONS	0x40	/* 4 direction button present */
>>   #define ALPS_PS2_INTERLEAVED	0x80	/* 3-byte PS/2 packet interleaved with
>>   					   6-byte ALPS packet */
>> -#define ALPS_DELL		0x100	/* device is a Dell laptop */
>> +#define ALPS_STICK_BITS		0x100	/* separate stick button bits */
>>   #define ALPS_BUTTONPAD		0x200	/* device is a clickpad */
>>
>>   static const struct alps_model_info alps_model_data[] = {
>> @@ -159,6 +159,43 @@ static const struct alps_protocol_info alps_v8_protocol_data = {
>>   	ALPS_PROTO_V8, 0x18, 0x18, 0
>>   };
>>
>> +/*
>> + * Some v2 models report the stick buttons in separate bits
>> + */
>> +static const struct dmi_system_id alps_dmi_has_separate_stick_buttons[] = {
>> +#if defined(CONFIG_DMI) && defined(CONFIG_X86)
>> +	{
>> +		/* Extrapolated from other entries */
>> +		.matches = {
>> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
>> +			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude D420"),
>> +		},
>> +	},
>> +	{
>> +		/* Reported-by: Hans de Bruin <jmdebruin@xmsnet.nl> */
>> +		.matches = {
>> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
>> +			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude D430"),
>> +		},
>> +	},
>> +	{
>> +		/* Reported-by: Hans de Goede <hdegoede@redhat.com> */
>> +		.matches = {
>> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
>> +			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude D620"),
>> +		},
>> +	},
>> +	{
>> +		/* Extrapolated from other entries */
>> +		.matches = {
>> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
>> +			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude D630"),
>> +		},
>> +	},
>> +#endif
>> +	{ }
>> +};
>> +
>
> Hi! Are you sure that above machines do not have variants without
> ALPS_DUALPOINT or without ALPS_PASS? Because if yes, then we could see
> another break.

Yes I'm sure that these machines always come with a dualpoint setup
with pass-through support. The 20/30 models are in essence the same
with just a newer version core2duo dropped in (they changed the model
now since the newer core2duo-s where the first x86_64 cpu-s from Intel,
and the 4x0 / 6x0 models have the exact same dualpoint setup, which is
why both Hans de Bruin and me were seeing the same bug.

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

Re: Regression since commit 92bac83

[Pali Rohár]

On Wednesday 21 October 2015 10:30:47 Hans de Goede wrote:
> >Hi! Are you sure that above machines do not have variants without
> >ALPS_DUALPOINT or without ALPS_PASS? Because if yes, then we could see
> >another break.
> 
> Yes I'm sure that these machines always come with a dualpoint setup
> with pass-through support.

Ok, in this case, I'm fine with this patch. Hopefully no other obscure
problem will be reported...

So you can add my Acked-By: Pali Rohár <pali.rohar@gmail.com>

-- 
Pali Rohár
pali.rohar@gmail.com