Hi Pali,
Do we need to check firmware id? Is not check for "any alps trackpoint"
enough? If in future there would be more alps trackpoint-only devices it probably have different firmware id.
-> Yes ,we need the firmware version to check if the device is trackpoint-only.
This method fit all the current alps trackpoint device.
Calling that trackpoint check two times is useless and just increase detection time of PS/2 devices.
-> what the twice means?
-> Do you means ps2_command(&psmouse->ps2dev, param, MAKE_PS2_CMD(0, 2, TP_READ_ID)) used in the alps.c and trackpoint.c or in function alps_detect twice?(because the alps_identify was called twice.)
-> we must use this command in alps.c to filter the trackpoint-only device.
-> We can move it into alps_detect function to reduce calls. How about this?
Best Regards
Shona
-----邮件原件-----
发件人: Pali Rohár <pali.rohar@xxxxxxxxx>
发送时间: Wednesday, June 12, 2019 3:38 PM
收件人: Hui Wang <hui.wang@xxxxxxxxxxxxx>
抄送: linux-input@xxxxxxxxxxxxxxx; dmitry.torokhov@xxxxxxxxx; 劉 曉曉 Xiaoxiao Liu <xiaoxiao.liu-1@xxxxxxxxxxx>; sliuuxiaonxiao@xxxxxxxxx; 曹 曉建 Xiaojian Cao <xiaojian.cao@xxxxxxxxxxx>; 斉藤 直樹 Naoki Saito <naoki.saito@xxxxxxxxxxxxxx>; 川瀬 英夫 Hideo Kawase <hideo.kawase@xxxxxxxxxxxxxx>
主题: Re: [PATCH] Input: alps - Don't handle ALPS cs19 trackpoint-only device
On Wednesday 12 June 2019 15:05:17 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@xxxxxxxxx>
> Signed-off-by: Hui Wang <hui.wang@xxxxxxxxxxxxx>
> ---
> drivers/input/mouse/alps.c | 28 ++++++++++++++++++++++++++++
> 1 file changed, 28 insertions(+)
>
> diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
> index 0a6f7ca883e7..ff522cd980a0 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,24 @@ 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 };
> + int error;
> +
> + error = ps2_command(&psmouse->ps2dev,
> + param, MAKE_PS2_CMD(0, 2, TP_READ_ID));
> + if (error)
> + return false;
> +
> + if (param[0] == TP_VARIANT_ALPS && param[1] & 0x20) {
Hi!
Do we need to check firmware id? Is not check for "any alps trackpoint"
enough? If in future there would be more alps trackpoint-only devices it probably have different firmware id.
Also you need to put param[1] & 0x20 into parenthesis due to priority of & and && operators.
Also, what about making trackpoint_start_protocol() function non-static and use it in alps_is_c19_trackpoint implementation? It is doing exactly same thing.
> + psmouse_dbg(psmouse, "It is an ALPS trackpoint-only device (CS19)\n");
> + return true;
> + }
> +
> + return false;
> +}
> +
> static int alps_identify(struct psmouse *psmouse, struct alps_data
> *priv) {
> const struct alps_protocol_info *protocol; @@ -2883,6 +2902,15 @@
> static int alps_identify(struct psmouse *psmouse, struct alps_data *priv)
> if ((e6[0] & 0xf8) != 0 || e6[1] != 0 || (e6[2] != 10 && e6[2] != 100))
> return -EINVAL;
>
> + /*
> + * 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 drive this device.
> + */
> + if (alps_is_cs19_trackpoint(psmouse))
> + return -EINVAL;
> +
This change is not ideal as this function would be called two times, see alps_detect(). I would suggest to think more about detection and come up with better solution so above trackpoint check would called only once during PS/2 device detection.
Calling that trackpoint check two times is useless and just increase detection time of PS/2 devices.
> /*
> * Now get the "E7" and "EC" reports. These will uniquely identify
> * most ALPS touchpads.
--
Pali Rohár
pali.rohar@xxxxxxxxx
