| Message ID | 20190617120555.8750-1-hui.wang@canonical.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v4] Input: alps - Don't handle ALPS cs19 trackpoint-only device | expand |
On Monday 17 June 2019 20:05:55 Hui Wang wrote: > On a latest Lenovo laptop, the trackpoint and 3 buttons below it > don't work at all, when we move the trackpoint or press those 3 > buttons, the kernel will print out: > "Rejected trackstick packet from non DualPoint device" > > This device is identified as alps touchpad but the packet has > trackpoint format, so the alps.c drops the packet and prints out > the message above. > > According to XiaoXiao's explanation, this device is named cs19 and > is trackpoint-only device, its firmware is only for trackpoint, it > is independent of touchpad and is a completely different device from > DualPoint ones. > > To drive this device with mininal changes to the existing driver, we > just let the alps driver not handle this device, then the trackpoint.c > will be the driver of this device. > > With the trackpoint.c, this trackpoint and 3 buttons all work well, > they have all features that the trackpoint should have, like > scrolling-screen, drag-and-drop and frame-selection. > > Signed-off-by: XiaoXiao Liu <sliuuxiaonxiao@gmail.com> > Signed-off-by: Hui Wang <hui.wang@canonical.com> > --- > Sorry, forgot to add "param[1] & 0x20" checking in the v3, please > ignore the v3 patch. > drivers/input/mouse/alps.c | 29 +++++++++++++++++++++++++++++ > 1 file changed, 29 insertions(+) > > diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c > index 0a6f7ca883e7..6bed9eb16c2c 100644 > --- a/drivers/input/mouse/alps.c > +++ b/drivers/input/mouse/alps.c > @@ -24,6 +24,7 @@ > > #include "psmouse.h" > #include "alps.h" > +#include "trackpoint.h" > > /* > * Definitions for ALPS version 3 and 4 command mode protocol > @@ -2864,6 +2865,23 @@ static const struct alps_protocol_info *alps_match_table(unsigned char *e7, > return NULL; > } > > +static bool alps_is_cs19_trackpoint(struct psmouse *psmouse) > +{ > + u8 param[2] = { 0 }; > + > + if (ps2_command(&psmouse->ps2dev, > + param, MAKE_PS2_CMD(0, 2, TP_READ_ID))) > + return false; > + > + if ((param[0] == TP_VARIANT_ALPS) && (param[1] & 0x20)) { You should describe what does magic (param[1] & 0x20) check is doing. Reading trakcpoint.c gives us assumption that param[1] is firmware id, but why mask 0x20 is not explained. > + psmouse_warn(psmouse, > + "It is an ALPS trackpoint-only device (CS19), make sure the MOUSE_PS2_TRACKPOINT is enabled to drive it\n"); This will throw a warning for every CS19 device independently of kernel configuration. You should not throw warning and spam users who compiled kernel with trackpoint support. Rather use something like this: if (param[0] ...) { if (IS_ENABLED(CONFIG_MOUSE_PS2_TRACKPOINT)) psmouse_dbg(psmouse, "ALPS CS19 trackpoint-only device detected, not using ALPS touchpad driver\n"); else psmouse_warn(psmouse, "ALPS CS19 trackpoint-only device detected, but CONFIG_MOUSE_PS2_TRACKPOINT is not enabled, fallback to bare PS/2 mouse\n"); return true; } Just rephrase messages to not be too long and useful for user. > + return true; > + } > + > + return false; > +} > + > static int alps_identify(struct psmouse *psmouse, struct alps_data *priv) > { > const struct alps_protocol_info *protocol; > @@ -3164,6 +3182,17 @@ int alps_detect(struct psmouse *psmouse, bool set_properties) > if (error) > return error; > > + /* > + * ALPS cs19 is a trackpoint-only device, it is completely independent > + * of touchpad. So it is a different device from DualPoint ones, if it > + * is identified as a cs19 trackpoint device, we return -EINVAL here and > + * let trackpoint.c to drive this device. > + * If ps2_command() fails here, we depend on the immediate followed > + * psmouse_reset() to reset the device to normal state. > + */ > + if (alps_is_cs19_trackpoint(psmouse)) > + return -EINVAL; > + > /* > * Reset the device to make sure it is fully operational: > * on some laptops, like certain Dell Latitudes, we may
On 2019/6/18 下午4:27, Pali Rohár wrote: > On Monday 17 June 2019 20:05:55 Hui Wang wrote: >> On a latest Lenovo laptop, the trackpoint and 3 buttons below it >> don't work at all, when we move the trackpoint or press those 3 >> buttons, the kernel will print out: >> "Rejected trackstick packet from non DualPoint device" >> >> This device is identified as alps touchpad but the packet has >> trackpoint format, so the alps.c drops the packet and prints out >> the message above. >> >> According to XiaoXiao's explanation, this device is named cs19 and >> is trackpoint-only device, its firmware is only for trackpoint, it >> is independent of touchpad and is a completely different device from >> DualPoint ones. >> >> To drive this device with mininal changes to the existing driver, we >> just let the alps driver not handle this device, then the trackpoint.c >> will be the driver of this device. >> >> With the trackpoint.c, this trackpoint and 3 buttons all work well, >> they have all features that the trackpoint should have, like >> scrolling-screen, drag-and-drop and frame-selection. >> >> Signed-off-by: XiaoXiao Liu <sliuuxiaonxiao@gmail.com> >> Signed-off-by: Hui Wang <hui.wang@canonical.com> >> --- >> Sorry, forgot to add "param[1] & 0x20" checking in the v3, please >> ignore the v3 patch. >> drivers/input/mouse/alps.c | 29 +++++++++++++++++++++++++++++ >> 1 file changed, 29 insertions(+) >> >> diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c >> index 0a6f7ca883e7..6bed9eb16c2c 100644 >> --- a/drivers/input/mouse/alps.c >> +++ b/drivers/input/mouse/alps.c >> @@ -24,6 +24,7 @@ >> >> #include "psmouse.h" >> #include "alps.h" >> +#include "trackpoint.h" >> >> /* >> * Definitions for ALPS version 3 and 4 command mode protocol >> @@ -2864,6 +2865,23 @@ static const struct alps_protocol_info *alps_match_table(unsigned char *e7, >> return NULL; >> } >> >> +static bool alps_is_cs19_trackpoint(struct psmouse *psmouse) >> +{ >> + u8 param[2] = { 0 }; >> + >> + if (ps2_command(&psmouse->ps2dev, >> + param, MAKE_PS2_CMD(0, 2, TP_READ_ID))) >> + return false; >> + >> + if ((param[0] == TP_VARIANT_ALPS) && (param[1] & 0x20)) { > You should describe what does magic (param[1] & 0x20) check is doing. > Reading trakcpoint.c gives us assumption that param[1] is firmware id, > but why mask 0x20 is not explained. Hi XiaoXiao, what explanation should we add for (param[1] & 0x20)? > >> + psmouse_warn(psmouse, >> + "It is an ALPS trackpoint-only device (CS19), make sure the MOUSE_PS2_TRACKPOINT is enabled to drive it\n"); > This will throw a warning for every CS19 device independently of kernel > configuration. You should not throw warning and spam users who compiled > kernel with trackpoint support. > > Rather use something like this: > > if (param[0] ...) { > if (IS_ENABLED(CONFIG_MOUSE_PS2_TRACKPOINT)) > psmouse_dbg(psmouse, "ALPS CS19 trackpoint-only device detected, not using ALPS touchpad driver\n"); > else > psmouse_warn(psmouse, "ALPS CS19 trackpoint-only device detected, but CONFIG_MOUSE_PS2_TRACKPOINT is not enabled, fallback to bare PS/2 mouse\n"); > return true; > } > > Just rephrase messages to not be too long and useful for user. OK, will change it. Thanks > >> + return true; >> + } >> + >> + return false; >> +} >> + >> static int alps_identify(struct psmouse *psmouse, struct alps_data *priv) >> { >> const struct alps_protocol_info *protocol; >> @@ -3164,6 +3182,17 @@ int alps_detect(struct psmouse *psmouse, bool set_properties) >> if (error) >> return error; >> >> + /* >> + * ALPS cs19 is a trackpoint-only device, it is completely independent >> + * of touchpad. So it is a different device from DualPoint ones, if it >> + * is identified as a cs19 trackpoint device, we return -EINVAL here and >> + * let trackpoint.c to drive this device. >> + * If ps2_command() fails here, we depend on the immediate followed >> + * psmouse_reset() to reset the device to normal state. >> + */ >> + if (alps_is_cs19_trackpoint(psmouse)) >> + return -EINVAL; >> + >> /* >> * Reset the device to make sure it is fully operational: >> * on some laptops, like certain Dell Latitudes, we may
diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c index 0a6f7ca883e7..6bed9eb16c2c 100644 --- a/drivers/input/mouse/alps.c +++ b/drivers/input/mouse/alps.c @@ -24,6 +24,7 @@ #include "psmouse.h" #include "alps.h" +#include "trackpoint.h" /* * Definitions for ALPS version 3 and 4 command mode protocol @@ -2864,6 +2865,23 @@ static const struct alps_protocol_info *alps_match_table(unsigned char *e7, return NULL; } +static bool alps_is_cs19_trackpoint(struct psmouse *psmouse) +{ + u8 param[2] = { 0 }; + + if (ps2_command(&psmouse->ps2dev, + param, MAKE_PS2_CMD(0, 2, TP_READ_ID))) + return false; + + if ((param[0] == TP_VARIANT_ALPS) && (param[1] & 0x20)) { + psmouse_warn(psmouse, + "It is an ALPS trackpoint-only device (CS19), make sure the MOUSE_PS2_TRACKPOINT is enabled to drive it\n"); + return true; + } + + return false; +} + static int alps_identify(struct psmouse *psmouse, struct alps_data *priv) { const struct alps_protocol_info *protocol; @@ -3164,6 +3182,17 @@ int alps_detect(struct psmouse *psmouse, bool set_properties) if (error) return error; + /* + * ALPS cs19 is a trackpoint-only device, it is completely independent + * of touchpad. So it is a different device from DualPoint ones, if it + * is identified as a cs19 trackpoint device, we return -EINVAL here and + * let trackpoint.c to drive this device. + * If ps2_command() fails here, we depend on the immediate followed + * psmouse_reset() to reset the device to normal state. + */ + if (alps_is_cs19_trackpoint(psmouse)) + return -EINVAL; + /* * Reset the device to make sure it is fully operational: * on some laptops, like certain Dell Latitudes, we may