Message ID | 55BA3C28.3080506@redhat.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On Thursday 30 July 2015 17:00:56 Hans de Goede wrote: > Hi, > > On 30-07-15 16:46, Pali Rohár wrote: > >What about introducing new flag ALPS_<something> instead calling > >dmi_name_in_vendors() function every time when we need to process > >packet? > > That is a good idea. Douglas can you test the attached version > instead of the previous one please ? > > Thanks & Regards, > > Hans > @@ -251,9 +253,9 @@ static void alps_process_packet_v1_v2(struct psmouse *psmouse) > return; > } > > - /* Non interleaved V2 dualpoint has separate stick button bits */ > + /* Dell non interleaved V2 dualpoint has separate stick button bits */ > if (priv->proto_version == ALPS_PROTO_V2 && > - priv->flags == (ALPS_PASS | ALPS_DUALPOINT)) { > + priv->flags == (ALPS_DELL | ALPS_PASS | ALPS_DUALPOINT)) { Hi again. Now I'm trying to understand what this condition means and you probably wanted to write... priv->flags is field and so == comparator is hard to decode and understood. Now it means that priv->flags must have set ALPS_DELL, ALPS_PASS and ALPS_DUALPOINT and must not set ALPS_WHEEL, ALPS_FW_BK_1, ALPS_FW_BK_2, ALPS_FOUR_BUTTONS, ALPS_PS2_INTERLEAVED, ALPS_BUTTONPAD and all other future flags! With future flags this code is fragile and can be easy broken in future (by introducing new flags). Because of "Non interleaved" in description you probably wanted something like this? if (priv->proto_version == ALPS_PROTO_V2 && (priv->flags & (ALPS_DELL | ALPS_PASS | ALPS_DUALPOINT)) && !(priv->flags & ALPS_PS2_INTERLEAVED)) (flags must contains ALPS_DELL, ALPS_PASS, ALPS_DUALPOINT and must not ALPS_PS2_INTERLEAVED)
Hi, On 30-07-15 17:49, Pali Rohár wrote: > On Thursday 30 July 2015 17:00:56 Hans de Goede wrote: >> Hi, >> >> On 30-07-15 16:46, Pali Rohár wrote: >>> What about introducing new flag ALPS_<something> instead calling >>> dmi_name_in_vendors() function every time when we need to process >>> packet? >> >> That is a good idea. Douglas can you test the attached version >> instead of the previous one please ? >> >> Thanks & Regards, >> >> Hans > >> @@ -251,9 +253,9 @@ static void alps_process_packet_v1_v2(struct psmouse *psmouse) >> return; >> } >> >> - /* Non interleaved V2 dualpoint has separate stick button bits */ >> + /* Dell non interleaved V2 dualpoint has separate stick button bits */ >> if (priv->proto_version == ALPS_PROTO_V2 && >> - priv->flags == (ALPS_PASS | ALPS_DUALPOINT)) { >> + priv->flags == (ALPS_DELL | ALPS_PASS | ALPS_DUALPOINT)) { > > Hi again. Now I'm trying to understand what this condition means and you > probably wanted to write... priv->flags is field and so == comparator is > hard to decode and understood. The == operator was already being used before this patch, and it does the job just fine IMHO. Regards, Hans Now it means that priv->flags must have > set ALPS_DELL, ALPS_PASS and ALPS_DUALPOINT and must not set ALPS_WHEEL, > ALPS_FW_BK_1, ALPS_FW_BK_2, ALPS_FOUR_BUTTONS, ALPS_PS2_INTERLEAVED, > ALPS_BUTTONPAD and all other future flags! With future flags this code > is fragile and can be easy broken in future (by introducing new flags). > Because of "Non interleaved" in description you probably wanted > something like this? > > if (priv->proto_version == ALPS_PROTO_V2 && > (priv->flags & (ALPS_DELL | ALPS_PASS | ALPS_DUALPOINT)) && > !(priv->flags & ALPS_PS2_INTERLEAVED)) > > (flags must contains ALPS_DELL, ALPS_PASS, ALPS_DUALPOINT and must not > ALPS_PS2_INTERLEAVED) > -- 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 Thu, Jul 30, 2015 at 11:00 AM, Hans de Goede <hdegoede@redhat.com> wrote: > Hi, > > On 30-07-15 16:46, Pali Rohár wrote: >> What about introducing new flag ALPS_<something> instead calling >> dmi_name_in_vendors() function every time when we need to process >> packet? > > That is a good idea. Douglas can you test the attached version > instead of the previous one please ? > > Thanks & Regards, > > Hans This patch works for me. Doug -- 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 Friday 31 July 2015 23:12:56 Douglas Christman wrote: > On Thu, Jul 30, 2015 at 11:00 AM, Hans de Goede <hdegoede@redhat.com> > wrote: > > Hi, > > > > On 30-07-15 16:46, Pali Rohár wrote: > >> What about introducing new flag ALPS_<something> instead calling > >> dmi_name_in_vendors() function every time when we need to process > >> packet? > > > > That is a good idea. Douglas can you test the attached version > > instead of the previous one please ? > > > > Thanks & Regards, > > > > Hans > > This patch works for me. > > Doug Douglas: Thanks for testing! Hans: Can you update also documentation in Documentation/input/alps.txt to match with this your change?
Hi, On 31-07-15 23:17, Pali Rohár wrote: > On Friday 31 July 2015 23:12:56 Douglas Christman wrote: >> On Thu, Jul 30, 2015 at 11:00 AM, Hans de Goede <hdegoede@redhat.com> >> wrote: >>> Hi, >>> >>> On 30-07-15 16:46, Pali Rohár wrote: >>>> What about introducing new flag ALPS_<something> instead calling >>>> dmi_name_in_vendors() function every time when we need to process >>>> packet? >>> >>> That is a good idea. Douglas can you test the attached version >>> instead of the previous one please ? >>> >>> Thanks & Regards, >>> >>> Hans >> >> This patch works for me. >> >> Doug > > Douglas: Thanks for testing! Ack, thanks for testing :) > Hans: Can you update also documentation in Documentation/input/alps.txt > to match with this your change? Good point, I've fixed this up and submitted the patch fixing this upstream. 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
From 93a1852f32ab37b2cbcdb7e797f1ad59eb6bbef4 Mon Sep 17 00:00:00 2001 From: Hans de Goede <hdegoede@redhat.com> Date: Thu, 30 Jul 2015 15:49:16 +0200 Subject: [PATCH] alps: Only Dell laptops have separate button bits for v2 dualpoint sticks It turns out that only Dell laptops have the separate button bits for v2 dualpoint sticks and that commit 92bac83dd79e ("Input: alps - non interleaved V2 dualpoint has separate stick button bits") causes regressions on Toshiba laptops. This commit adds a check for Dell laptops to the code for handling these extra button bits, fixing this regression. This patch has been tested on a Dell Latitude D620 to make sure that it does not reintroduce the original problem. Reported-by: Douglas Christman <douglaschristman@gmail.com> Cc: stable@vger.kernel.org Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/input/mouse/alps.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c index 113d6f1..4d24686 100644 --- a/drivers/input/mouse/alps.c +++ b/drivers/input/mouse/alps.c @@ -20,6 +20,7 @@ #include <linux/input/mt.h> #include <linux/serio.h> #include <linux/libps2.h> +#include <linux/dmi.h> #include "psmouse.h" #include "alps.h" @@ -99,6 +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_BUTTONPAD 0x200 /* device is a clickpad */ static const struct alps_model_info alps_model_data[] = { @@ -251,9 +253,9 @@ static void alps_process_packet_v1_v2(struct psmouse *psmouse) return; } - /* Non interleaved V2 dualpoint has separate stick button bits */ + /* Dell non interleaved V2 dualpoint has separate stick button bits */ if (priv->proto_version == ALPS_PROTO_V2 && - priv->flags == (ALPS_PASS | ALPS_DUALPOINT)) { + priv->flags == (ALPS_DELL | ALPS_PASS | ALPS_DUALPOINT)) { left |= packet[0] & 1; right |= packet[0] & 2; middle |= packet[0] & 4; @@ -2550,6 +2552,8 @@ 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; -- 2.4.3