Skip to contentSkip to conversation

Input: ALPS - Fix TrackStick support for SS5 hardware

PATCH Superseded Paul Donohue 3 participants 36 messages Oct 24, 2016 – Nov 24, 2016

Subsystems (3)
HID Input drivers Alps ps/2 touchpad driver
lore
Share & tools
lore

Paul Donohue

author
#0 lore raw

PATCH 1/1 Input: ALPS - Fix TrackStick support for SS5 hardware

The current Alps SS5 (SS4 v2) code generates bogus TouchPad events when TrackStick packets are processed.

This causes the xorg synaptics driver to print
"unable to find touch point 0" and
"BUG: triggered 'if (priv->num_active_touches > priv->num_slots)'" messages. It also causes unexpected TouchPad button release and reclick event sequences if the TrackStick is moved while holding a TouchPad button.

This commit corrects the problem by setting a flag for the relevant packet type in alps_decode_ss4_v2(), then using that flag in alps_process_packet_ss4_v2() to send only TrackStick reports when processing TrackStick packets.

Signed-off-by Paul Donohue <[email protected]>
drivers/input/mouse/alps.c +13 -9
index 6d7de9b..2c2e55b 100644
@@ -1279,6 +1279,8 @@ static int alps_decode_ss4_v2(struct alps_fields *f,
                        input_report_rel(priv->dev2, REL_Y, -y);
                        input_report_abs(priv->dev2, ABS_PRESSURE, pressure);
                }
+               f->first_mp = 0;
+               f->is_mp = 0;
                break;
 
        case SS4_PACKET_ID_IDLE:

@@ -1289,12 +1291,14 @@ static int alps_decode_ss4_v2(struct alps_fields *f,
 
        /* handle buttons */
        if (pkt_id == SS4_PACKET_ID_STICK) {
+               f->is_ts = 1;
                f->ts_left = !!(SS4_BTN_V2(p) & 0x01);
                if (!(priv->flags & ALPS_BUTTONPAD)) {
                        f->ts_right = !!(SS4_BTN_V2(p) & 0x02);
                        f->ts_middle = !!(SS4_BTN_V2(p) & 0x04);
                }
        } else {
+               f->is_ts = 0;
                f->left = !!(SS4_BTN_V2(p) & 0x01);
                if (!(priv->flags & ALPS_BUTTONPAD)) {
                        f->right = !!(SS4_BTN_V2(p) & 0x02);

@@ -1346,18 +1350,18 @@ static void alps_process_packet_ss4_v2(struct psmouse *psmouse)
 
        priv->multi_packet = 0;
 
-       alps_report_mt_data(psmouse, (f->fingers <= 4) ? f->fingers : 4);
-
-       input_mt_report_finger_count(dev, f->fingers);
+       if (!f->is_ts) {
+               alps_report_mt_data(psmouse, (f->fingers <= 4) ? f->fingers : 4);
 
-       input_report_key(dev, BTN_LEFT, f->left);
-       input_report_key(dev, BTN_RIGHT, f->right);
-       input_report_key(dev, BTN_MIDDLE, f->middle);
+               input_mt_report_finger_count(dev, f->fingers);
 
-       input_report_abs(dev, ABS_PRESSURE, f->pressure);
-       input_sync(dev);
+               input_report_key(dev, BTN_LEFT, f->left);
+               input_report_key(dev, BTN_RIGHT, f->right);
+               input_report_key(dev, BTN_MIDDLE, f->middle);
 
-       if (priv->flags & ALPS_DUALPOINT) {
+               input_report_abs(dev, ABS_PRESSURE, f->pressure);
+               input_sync(dev);
+       } else {
                input_report_key(dev2, BTN_LEFT, f->ts_left);
                input_report_key(dev2, BTN_RIGHT, f->ts_right);
                input_report_key(dev2, BTN_MIDDLE, f->ts_middle);
drivers/input/mouse/alps.h +2 -0
index b9417e2..1dc09b5 100644
@@ -192,6 +192,7 @@ struct alps_bitmap_point {
  * @left: Left touchpad button is active.
  * @right: Right touchpad button is active.
  * @middle: Middle touchpad button is active.
+ * @is_ts: Packet is for the trackstick, not the touchpad.
  * @ts_left: Left trackstick button is active.
  * @ts_right: Right trackstick button is active.
  * @ts_middle: Middle trackstick button is active.

@@ -212,6 +213,7 @@ struct alps_fields {
        unsigned int right:1;
        unsigned int middle:1;
 
+       unsigned int is_ts:1;
        unsigned int ts_left:1;
        unsigned int ts_right:1;
        unsigned int ts_middle:1;
#1 lore raw
Quoted 87 lines · On Monday 24 October 2016 17:01:22 Paul Donohue wrote:

The current Alps SS5 (SS4 v2) code generates bogus TouchPad events when TrackStick packets are processed.

This causes the xorg synaptics driver to print
"unable to find touch point 0" and
"BUG: triggered 'if (priv->num_active_touches > priv->num_slots)'" messages. It also causes unexpected TouchPad button release and reclick event sequences if the TrackStick is moved while holding a TouchPad button.

This commit corrects the problem by setting a flag for the relevant packet type in alps_decode_ss4_v2(), then using that flag in alps_process_packet_ss4_v2() to send only TrackStick reports when processing TrackStick packets.

Signed-off-by Paul Donohue <[email protected]>
drivers/input/mouse/alps.c +13 -9
index 6d7de9b..2c2e55b 100644
@@ -1279,6 +1279,8 @@ static int alps_decode_ss4_v2(struct alps_fields *f,
                        input_report_rel(priv->dev2, REL_Y, -y);
                        input_report_abs(priv->dev2, ABS_PRESSURE, pressure);
                }
+               f->first_mp = 0;
+               f->is_mp = 0;
                break;
 
        case SS4_PACKET_ID_IDLE:

@@ -1289,12 +1291,14 @@ static int alps_decode_ss4_v2(struct alps_fields *f,
 
        /* handle buttons */
        if (pkt_id == SS4_PACKET_ID_STICK) {
+               f->is_ts = 1;
                f->ts_left = !!(SS4_BTN_V2(p) & 0x01);
                if (!(priv->flags & ALPS_BUTTONPAD)) {
                        f->ts_right = !!(SS4_BTN_V2(p) & 0x02);
                        f->ts_middle = !!(SS4_BTN_V2(p) & 0x04);
                }
        } else {
+               f->is_ts = 0;
                f->left = !!(SS4_BTN_V2(p) & 0x01);
                if (!(priv->flags & ALPS_BUTTONPAD)) {
                        f->right = !!(SS4_BTN_V2(p) & 0x02);

@@ -1346,18 +1350,18 @@ static void alps_process_packet_ss4_v2(struct psmouse *psmouse)
 
        priv->multi_packet = 0;
 
-       alps_report_mt_data(psmouse, (f->fingers <= 4) ? f->fingers : 4);
-
-       input_mt_report_finger_count(dev, f->fingers);
+       if (!f->is_ts) {
+               alps_report_mt_data(psmouse, (f->fingers <= 4) ? f->fingers : 4);
 
-       input_report_key(dev, BTN_LEFT, f->left);
-       input_report_key(dev, BTN_RIGHT, f->right);
-       input_report_key(dev, BTN_MIDDLE, f->middle);
+               input_mt_report_finger_count(dev, f->fingers);
 
-       input_report_abs(dev, ABS_PRESSURE, f->pressure);
-       input_sync(dev);
+               input_report_key(dev, BTN_LEFT, f->left);
+               input_report_key(dev, BTN_RIGHT, f->right);
+               input_report_key(dev, BTN_MIDDLE, f->middle);
 
-       if (priv->flags & ALPS_DUALPOINT) {
+               input_report_abs(dev, ABS_PRESSURE, f->pressure);
+               input_sync(dev);
+       } else {
                input_report_key(dev2, BTN_LEFT, f->ts_left);
                input_report_key(dev2, BTN_RIGHT, f->ts_right);
                input_report_key(dev2, BTN_MIDDLE, f->ts_middle);
drivers/input/mouse/alps.h +1 -0
index b9417e2..1dc09b5 100644
@@ -192,6 +192,7 @@ struct alps_bitmap_point {
  * @left: Left touchpad button is active.
  * @right: Right touchpad button is active.
  * @middle: Middle touchpad button is active.
+ * @is_ts: Packet is for the trackstick, not the touchpad.
  * @ts_left: Left trackstick button is active.
  * @ts_right: Right trackstick button is active.
  * @ts_middle: Middle trackstick button is active.

drivers/input/mouse/alps.h struct alps_fields { · line 219

Quoted 7 lines
drivers/input/mouse/alps.h +1 -0
@@ -212,6 +213,7 @@ struct alps_fields {
        unsigned int right:1;
        unsigned int middle:1;
 
+       unsigned int is_ts:1;
        unsigned int ts_left:1;
        unsigned int ts_right:1;
        unsigned int ts_middle:1;

Hi! What looks suspicious on this patch is that you need to extend structure in alps.h for marking packet as it comes from trackstick... and only for sse_v2 devices. It is not possible to do detection in process code similarly like for other devices?

Signature · 2 lines
Pali Rohár
[email protected]
author
#2 lore raw
Quoted 90 lines · Pali Rohár
Quoted 89 lines · On Monday 24 October 2016 17:01:22 Paul Donohue wrote:

The current Alps SS5 (SS4 v2) code generates bogus TouchPad events when TrackStick packets are processed.

This causes the xorg synaptics driver to print
"unable to find touch point 0" and
"BUG: triggered 'if (priv->num_active_touches > priv->num_slots)'" messages. It also causes unexpected TouchPad button release and reclick event sequences if the TrackStick is moved while holding a TouchPad button.

This commit corrects the problem by setting a flag for the relevant packet type in alps_decode_ss4_v2(), then using that flag in alps_process_packet_ss4_v2() to send only TrackStick reports when processing TrackStick packets.

Signed-off-by Paul Donohue <[email protected]>
drivers/input/mouse/alps.c +13 -9
index 6d7de9b..2c2e55b 100644
@@ -1279,6 +1279,8 @@ static int alps_decode_ss4_v2(struct alps_fields *f,
                        input_report_rel(priv->dev2, REL_Y, -y);
                        input_report_abs(priv->dev2, ABS_PRESSURE, pressure);
                }
+               f->first_mp = 0;
+               f->is_mp = 0;
                break;
 
        case SS4_PACKET_ID_IDLE:

@@ -1289,12 +1291,14 @@ static int alps_decode_ss4_v2(struct alps_fields *f,
 
        /* handle buttons */
        if (pkt_id == SS4_PACKET_ID_STICK) {
+               f->is_ts = 1;
                f->ts_left = !!(SS4_BTN_V2(p) & 0x01);
                if (!(priv->flags & ALPS_BUTTONPAD)) {
                        f->ts_right = !!(SS4_BTN_V2(p) & 0x02);
                        f->ts_middle = !!(SS4_BTN_V2(p) & 0x04);
                }
        } else {
+               f->is_ts = 0;
                f->left = !!(SS4_BTN_V2(p) & 0x01);
                if (!(priv->flags & ALPS_BUTTONPAD)) {
                        f->right = !!(SS4_BTN_V2(p) & 0x02);

@@ -1346,18 +1350,18 @@ static void alps_process_packet_ss4_v2(struct psmouse *psmouse)
 
        priv->multi_packet = 0;
 
-       alps_report_mt_data(psmouse, (f->fingers <= 4) ? f->fingers : 4);
-
-       input_mt_report_finger_count(dev, f->fingers);
+       if (!f->is_ts) {
+               alps_report_mt_data(psmouse, (f->fingers <= 4) ? f->fingers : 4);
 
-       input_report_key(dev, BTN_LEFT, f->left);
-       input_report_key(dev, BTN_RIGHT, f->right);
-       input_report_key(dev, BTN_MIDDLE, f->middle);
+               input_mt_report_finger_count(dev, f->fingers);
 
-       input_report_abs(dev, ABS_PRESSURE, f->pressure);
-       input_sync(dev);
+               input_report_key(dev, BTN_LEFT, f->left);
+               input_report_key(dev, BTN_RIGHT, f->right);
+               input_report_key(dev, BTN_MIDDLE, f->middle);
 
-       if (priv->flags & ALPS_DUALPOINT) {
+               input_report_abs(dev, ABS_PRESSURE, f->pressure);
+               input_sync(dev);
+       } else {
                input_report_key(dev2, BTN_LEFT, f->ts_left);
                input_report_key(dev2, BTN_RIGHT, f->ts_right);
                input_report_key(dev2, BTN_MIDDLE, f->ts_middle);
drivers/input/mouse/alps.h +1 -0
index b9417e2..1dc09b5 100644
@@ -192,6 +192,7 @@ struct alps_bitmap_point {
  * @left: Left touchpad button is active.
  * @right: Right touchpad button is active.
  * @middle: Middle touchpad button is active.
+ * @is_ts: Packet is for the trackstick, not the touchpad.
  * @ts_left: Left trackstick button is active.
  * @ts_right: Right trackstick button is active.
  * @ts_middle: Middle trackstick button is active.

@@ -212,6 +213,7 @@ struct alps_fields {
        unsigned int right:1;
        unsigned int middle:1;
Quoted 10 lines

drivers/input/mouse/alps.h struct alps_fields { · line 219

Quoted 5 lines
drivers/input/mouse/alps.h +1 -0
+       unsigned int is_ts:1;
        unsigned int ts_left:1;
        unsigned int ts_right:1;
        unsigned int ts_middle:1;

Hi! What looks suspicious on this patch is that you need to extend structure in alps.h for marking packet as it comes from trackstick... and only for sse_v2 devices. It is not possible to do detection in process code similarly like for other devices?

I was trying to avoid duplicating the detection logic (that is already implemented in alps_get_pkt_id_ss4_v2(), but yes, it is possible to implement the detection the same way as the other devices.

author
#3 lore raw

PATCH v2 1/1 Input: ALPS - Fix TrackStick support for SS5 hardware

The current Alps SS5 (SS4 v2) code generates bogus TouchPad events when TrackStick packets are processed.

This causes the xorg synaptics driver to print
"unable to find touch point 0" and
"BUG: triggered 'if (priv->num_active_touches > priv->num_slots)'" messages. It also causes unexpected TouchPad button release and reclick event sequences if the TrackStick is moved while holding a TouchPad button.

This commit corrects the problem by adjusting alps_process_packet_ss4_v2() so that it only sends TrackStick reports when processing TrackStick packets.

Signed-off-by Paul Donohue <[email protected]>
drivers/input/mouse/alps.c +11 -9
index 6d7de9b..1d0302b 100644
@@ -1279,6 +1279,8 @@ static int alps_decode_ss4_v2(struct alps_fields *f,
                        input_report_rel(priv->dev2, REL_Y, -y);
                        input_report_abs(priv->dev2, ABS_PRESSURE, pressure);
                }
+               f->first_mp = 0;
+               f->is_mp = 0;
                break;
 
        case SS4_PACKET_ID_IDLE:

@@ -1346,18 +1348,18 @@ static void alps_process_packet_ss4_v2(struct psmouse *psmouse)
 
        priv->multi_packet = 0;
 
-       alps_report_mt_data(psmouse, (f->fingers <= 4) ? f->fingers : 4);
+       if ((packet[3] & 0x30) != 0x20) {
+               alps_report_mt_data(psmouse, (f->fingers <= 4) ? f->fingers : 4);
 
-       input_mt_report_finger_count(dev, f->fingers);
+               input_mt_report_finger_count(dev, f->fingers);
 
-       input_report_key(dev, BTN_LEFT, f->left);
-       input_report_key(dev, BTN_RIGHT, f->right);
-       input_report_key(dev, BTN_MIDDLE, f->middle);
+               input_report_key(dev, BTN_LEFT, f->left);
+               input_report_key(dev, BTN_RIGHT, f->right);
+               input_report_key(dev, BTN_MIDDLE, f->middle);
 
-       input_report_abs(dev, ABS_PRESSURE, f->pressure);
-       input_sync(dev);
-
-       if (priv->flags & ALPS_DUALPOINT) {
+               input_report_abs(dev, ABS_PRESSURE, f->pressure);
+               input_sync(dev);
+       } else if (priv->flags & ALPS_DUALPOINT) {
                input_report_key(dev2, BTN_LEFT, f->ts_left);
                input_report_key(dev2, BTN_RIGHT, f->ts_right);
                input_report_key(dev2, BTN_MIDDLE, f->ts_middle);
#4 lore raw
Quoted 30 lines · On Saturday 05 November 2016 17:44:58 Paul Donohue wrote:

The current Alps SS5 (SS4 v2) code generates bogus TouchPad events when TrackStick packets are processed.

This causes the xorg synaptics driver to print
"unable to find touch point 0" and
"BUG: triggered 'if (priv->num_active_touches > priv->num_slots)'" messages. It also causes unexpected TouchPad button release and reclick event sequences if the TrackStick is moved while holding a TouchPad button.

This commit corrects the problem by adjusting alps_process_packet_ss4_v2() so that it only sends TrackStick reports when processing TrackStick packets.

Signed-off-by Paul Donohue <[email protected]>
drivers/input/mouse/alps.c +2 -0
index 6d7de9b..1d0302b 100644
@@ -1279,6 +1279,8 @@ static int alps_decode_ss4_v2(struct alps_fields *f,
                        input_report_rel(priv->dev2, REL_Y, -y);
                        input_report_abs(priv->dev2, ABS_PRESSURE, pressure);
                }
+               f->first_mp = 0;
+               f->is_mp = 0;
                break;
 
        case SS4_PACKET_ID_IDLE:

drivers/input/mouse/alps.c static void alps_process_packet_ss4_v2(struct psmouse *psmouse) · line 1351

Quoted 5 lines
drivers/input/mouse/alps.c +1 -1
@@ -1346,18 +1348,18 @@ static void alps_process_packet_ss4_v2(struct psmouse *psmouse)
 
        priv->multi_packet = 0;
 
-       alps_report_mt_data(psmouse, (f->fingers <= 4) ? f->fingers : 4);
+       if ((packet[3] & 0x30) != 0x20) {

Ideally adds comment that this is test for SS4_PACKET_ID_STICK.

Quoted 13 lines
drivers/input/mouse/alps.c +5 -4
+               alps_report_mt_data(psmouse, (f->fingers <= 4) ? f->fingers : 4);
 
-       input_mt_report_finger_count(dev, f->fingers);
+               input_mt_report_finger_count(dev, f->fingers);
 
-       input_report_key(dev, BTN_LEFT, f->left);
-       input_report_key(dev, BTN_RIGHT, f->right);
-       input_report_key(dev, BTN_MIDDLE, f->middle);
+               input_report_key(dev, BTN_LEFT, f->left);
+               input_report_key(dev, BTN_RIGHT, f->right);
+               input_report_key(dev, BTN_MIDDLE, f->middle);

drivers/input/mouse/alps.c static void alps_process_packet_ss4_v2(struct psmouse *psmouse) · line 1365

Quoted 10 lines
drivers/input/mouse/alps.c +3 -4
-       input_report_abs(dev, ABS_PRESSURE, f->pressure);
-       input_sync(dev);
-
-       if (priv->flags & ALPS_DUALPOINT) {
+               input_report_abs(dev, ABS_PRESSURE, f->pressure);
+               input_sync(dev);
+       } else if (priv->flags & ALPS_DUALPOINT) {
                input_report_key(dev2, BTN_LEFT, f->ts_left);
                input_report_key(dev2, BTN_RIGHT, f->ts_right);
                input_report_key(dev2, BTN_MIDDLE, f->ts_middle);

Otherwise this patch looks OK.

Anyway, I looked into alps.c source code and I see there another problem with ss4 devices. Function alps_decode_ss4_v2() calls input_report_*() functions without input_sync(). But all those input_report_*() should be in alps_process_packet_ss4_v2().

Would you like to fix this problem?

I think due to this problem, original code contains that BUG reported by you...

Signature · 2 lines
Pali Rohár
[email protected]
author
#5 lore raw
Quoted 6 lines · Pali Rohár

Anyway, I looked into alps.c source code and I see there another problem with ss4 devices. Function alps_decode_ss4_v2() calls input_report_*() functions without input_sync(). But all those input_report_*() should be in alps_process_packet_ss4_v2().

Would you like to fix this problem?

I think this part actually functions properly as-is because alps_process_packet_ss4_v2() always calls input_sync() after alps_decode_ss4_v2() calls input_report_*().

But I definitely agree this code path is confusing and inconsistent with the rest of alps.c. I'll take another stab at cleaning this up.

Thanks,
-Paul

#6 lore raw
In reply to Paul Donohue
Quoted 4 lines · On Saturday 05 November 2016 17:30:47 Paul Donohue wrote:

I was trying to avoid duplicating the detection logic (that is already implemented in alps_get_pkt_id_ss4_v2(), but yes, it is possible to implement the detection the same way as the other devices.

Ah... I see. alps_get_pkt_id_ss4_v2() should returns enum SS4_PACKET_ID but for some reasons it is unsigned char...

Signature · 2 lines
Pali Rohár
[email protected]
author
#7 lore raw

PATCH v3 1/3 Input: ALPS - Fix TrackStick support for SS5 hardware

In reply to Paul Donohue

Input: ALPS - Fix TrackStick support for SS5 hardware

The current Alps SS5 (SS4 v2) code generates bogus TouchPad events when TrackStick packets are processed.

This causes the xorg synaptics driver to print
"unable to find touch point 0" and
"BUG: triggered 'if (priv->num_active_touches > priv->num_slots)'" messages. It also causes unexpected TouchPad button release and reclick event sequences if the TrackStick is moved while holding a TouchPad button.

This commit corrects the problem by adjusting alps_process_packet_ss4_v2() so that it only sends TrackStick reports when processing TrackStick packets.

Signed-off-by Paul Donohue <[email protected]>
drivers/input/mouse/alps.c +12 -7
index 6d7de9b..b93fe83 100644
@@ -1346,6 +1346,18 @@ static void alps_process_packet_ss4_v2(struct psmouse *psmouse)
 
        priv->multi_packet = 0;
 
+       /* Report trackstick */
+       if (alps_get_pkt_id_ss4_v2(packet) == SS4_PACKET_ID_STICK) {
+               if (priv->flags & ALPS_DUALPOINT) {
+                       input_report_key(dev2, BTN_LEFT, f->ts_left);
+                       input_report_key(dev2, BTN_RIGHT, f->ts_right);
+                       input_report_key(dev2, BTN_MIDDLE, f->ts_middle);
+                       input_sync(dev2);
+               }
+               return;
+       }
+
+       /* Report touchpad */
        alps_report_mt_data(psmouse, (f->fingers <= 4) ? f->fingers : 4);
 
        input_mt_report_finger_count(dev, f->fingers);

@@ -1356,13 +1368,6 @@ static void alps_process_packet_ss4_v2(struct psmouse *psmouse)
 
        input_report_abs(dev, ABS_PRESSURE, f->pressure);
        input_sync(dev);
-
-       if (priv->flags & ALPS_DUALPOINT) {
-               input_report_key(dev2, BTN_LEFT, f->ts_left);
-               input_report_key(dev2, BTN_RIGHT, f->ts_right);
-               input_report_key(dev2, BTN_MIDDLE, f->ts_middle);
-               input_sync(dev2);
-       }
 }
 
 static bool alps_is_valid_package_ss4_v2(struct psmouse *psmouse)
author
#8 lore raw

PATCH v3 2/3 Input: ALPS - Fix TrackStick support for SS5 hardware

Input: ALPS - Clean up TrackStick handling for SS5 hardware

For consistency and clarity, the input_report_*() functions should be called by alps_process_packet_ss4_v2() instead of by alps_decode_ss4_v2().

Signed-off-by Paul Donohue <[email protected]>
drivers/input/mouse/alps.c +19 -17
index b93fe83..12376d2 100644
@@ -1267,18 +1267,11 @@ static int alps_decode_ss4_v2(struct alps_fields *f,
                break;
 
        case SS4_PACKET_ID_STICK:
-               if (!(priv->flags & ALPS_DUALPOINT)) {
-                       psmouse_warn(psmouse,
-                                    "Rejected trackstick packet from non DualPoint device");
-               } else {
-                       int x = (s8)(((p[0] & 1) << 7) | (p[1] & 0x7f));
-                       int y = (s8)(((p[3] & 1) << 7) | (p[2] & 0x7f));
-                       int pressure = (s8)(p[4] & 0x7f);
-
-                       input_report_rel(priv->dev2, REL_X, x);
-                       input_report_rel(priv->dev2, REL_Y, -y);
-                       input_report_abs(priv->dev2, ABS_PRESSURE, pressure);
-               }
+               f->st.x = (s8)(((p[0] & 1) << 7) | (p[1] & 0x7f));
+               f->st.y = -(s8)(((p[3] & 1) << 7) | (p[2] & 0x7f));
+               f->pressure = (s8)(p[4] & 0x7f);
+               f->first_mp = 0;
+               f->is_mp = 0;
                break;
 
        case SS4_PACKET_ID_IDLE:

@@ -1348,12 +1341,21 @@ static void alps_process_packet_ss4_v2(struct psmouse *psmouse)
 
        /* Report trackstick */
        if (alps_get_pkt_id_ss4_v2(packet) == SS4_PACKET_ID_STICK) {
-               if (priv->flags & ALPS_DUALPOINT) {
-                       input_report_key(dev2, BTN_LEFT, f->ts_left);
-                       input_report_key(dev2, BTN_RIGHT, f->ts_right);
-                       input_report_key(dev2, BTN_MIDDLE, f->ts_middle);
-                       input_sync(dev2);
+               if (!(priv->flags & ALPS_DUALPOINT)) {
+                       psmouse_warn(psmouse,
+                                    "Rejected trackstick packet from non DualPoint device");
+                       return;
                }
+
+               input_report_rel(priv->dev2, REL_X, f->st.x);
+               input_report_rel(priv->dev2, REL_Y, f->st.y);
+               input_report_abs(priv->dev2, ABS_PRESSURE, f->pressure);
+
+               input_report_key(dev2, BTN_LEFT, f->ts_left);
+               input_report_key(dev2, BTN_RIGHT, f->ts_right);
+               input_report_key(dev2, BTN_MIDDLE, f->ts_middle);
+
+               input_sync(dev2);
                return;
        }
author
#9 lore raw

Ugh. My mailer isn't formatting these patches properly ... let me try this again.

author
#10 lore raw

PATCH v4 1/3 Input: ALPS - Fix TrackStick support for SS5 hardware

In reply to Paul Donohue

The current Alps SS5 (SS4 v2) code generates bogus TouchPad events when TrackStick packets are processed.

This causes the xorg synaptics driver to print
"unable to find touch point 0" and
"BUG: triggered 'if (priv->num_active_touches > priv->num_slots)'" messages. It also causes unexpected TouchPad button release and reclick event sequences if the TrackStick is moved while holding a TouchPad button.

This commit corrects the problem by adjusting alps_process_packet_ss4_v2() so that it only sends TrackStick reports when processing TrackStick packets.

Signed-off-by Paul Donohue <[email protected]>
drivers/input/mouse/alps.c +12 -7
index 6d7de9b..b93fe83 100644
@@ -1346,6 +1346,18 @@ static void alps_process_packet_ss4_v2(struct psmouse *psmouse)
 
 	priv->multi_packet = 0;
 
+	/* Report trackstick */
+	if (alps_get_pkt_id_ss4_v2(packet) == SS4_PACKET_ID_STICK) {
+		if (priv->flags & ALPS_DUALPOINT) {
+			input_report_key(dev2, BTN_LEFT, f->ts_left);
+			input_report_key(dev2, BTN_RIGHT, f->ts_right);
+			input_report_key(dev2, BTN_MIDDLE, f->ts_middle);
+			input_sync(dev2);
+		}
+		return;
+	}
+
+	/* Report touchpad */
 	alps_report_mt_data(psmouse, (f->fingers <= 4) ? f->fingers : 4);
 
 	input_mt_report_finger_count(dev, f->fingers);

@@ -1356,13 +1368,6 @@ static void alps_process_packet_ss4_v2(struct psmouse *psmouse)
 
 	input_report_abs(dev, ABS_PRESSURE, f->pressure);
 	input_sync(dev);
-
-	if (priv->flags & ALPS_DUALPOINT) {
-		input_report_key(dev2, BTN_LEFT, f->ts_left);
-		input_report_key(dev2, BTN_RIGHT, f->ts_right);
-		input_report_key(dev2, BTN_MIDDLE, f->ts_middle);
-		input_sync(dev2);
-	}
 }
 
 static bool alps_is_valid_package_ss4_v2(struct psmouse *psmouse)
author
#11 lore raw

PATCH v4 2/3 Input: ALPS - Clean up TrackStick handling for SS5 hardware

For consistency and clarity, the input_report_*() functions should be called by alps_process_packet_ss4_v2() instead of by alps_decode_ss4_v2().

Signed-off-by Paul Donohue <[email protected]>
drivers/input/mouse/alps.c +19 -17
index b93fe83..12376d2 100644
@@ -1267,18 +1267,11 @@ static int alps_decode_ss4_v2(struct alps_fields *f,
 		break;
 
 	case SS4_PACKET_ID_STICK:
-		if (!(priv->flags & ALPS_DUALPOINT)) {
-			psmouse_warn(psmouse,
-				     "Rejected trackstick packet from non DualPoint device");
-		} else {
-			int x = (s8)(((p[0] & 1) << 7) | (p[1] & 0x7f));
-			int y = (s8)(((p[3] & 1) << 7) | (p[2] & 0x7f));
-			int pressure = (s8)(p[4] & 0x7f);
-
-			input_report_rel(priv->dev2, REL_X, x);
-			input_report_rel(priv->dev2, REL_Y, -y);
-			input_report_abs(priv->dev2, ABS_PRESSURE, pressure);
-		}
+		f->st.x = (s8)(((p[0] & 1) << 7) | (p[1] & 0x7f));
+		f->st.y = -(s8)(((p[3] & 1) << 7) | (p[2] & 0x7f));
+		f->pressure = (s8)(p[4] & 0x7f);
+		f->first_mp = 0;
+		f->is_mp = 0;
 		break;
 
 	case SS4_PACKET_ID_IDLE:

@@ -1348,12 +1341,21 @@ static void alps_process_packet_ss4_v2(struct psmouse *psmouse)
 
 	/* Report trackstick */
 	if (alps_get_pkt_id_ss4_v2(packet) == SS4_PACKET_ID_STICK) {
-		if (priv->flags & ALPS_DUALPOINT) {
-			input_report_key(dev2, BTN_LEFT, f->ts_left);
-			input_report_key(dev2, BTN_RIGHT, f->ts_right);
-			input_report_key(dev2, BTN_MIDDLE, f->ts_middle);
-			input_sync(dev2);
+		if (!(priv->flags & ALPS_DUALPOINT)) {
+			psmouse_warn(psmouse,
+				     "Rejected trackstick packet from non DualPoint device");
+			return;
 		}
+
+		input_report_rel(priv->dev2, REL_X, f->st.x);
+		input_report_rel(priv->dev2, REL_Y, f->st.y);
+		input_report_abs(priv->dev2, ABS_PRESSURE, f->pressure);
+
+		input_report_key(dev2, BTN_LEFT, f->ts_left);
+		input_report_key(dev2, BTN_RIGHT, f->ts_right);
+		input_report_key(dev2, BTN_MIDDLE, f->ts_middle);
+
+		input_sync(dev2);
 		return;
 	}
author
#12 lore raw

PATCH v4 3/3 Input: ALPS - Clean up code for SS5 hardware

The return value of alps_get_pkt_id_ss4_v2() should really be "enum SS4_PACKET_ID", not "unsigned char". Correct this.

Also, most of the Alps SS5 (SS4 v2) packet byte parsing code is implemented using macros, but there are a few places where bytes are directly manipulated in alps.c. For consistency, migrate the rest of these to macros.

Signed-off-by Paul Donohue <[email protected]>
drivers/input/mouse/alps.c +8 -11
index 12376d2..6a2fb69 100644
@@ -1153,15 +1153,13 @@ static void alps_process_packet_v7(struct psmouse *psmouse)
 		alps_process_touchpad_packet_v7(psmouse);
 }
 
-static unsigned char alps_get_pkt_id_ss4_v2(unsigned char *byte)
+static enum SS4_PACKET_ID alps_get_pkt_id_ss4_v2(unsigned char *byte)
 {
-	unsigned char pkt_id = SS4_PACKET_ID_IDLE;
+	enum SS4_PACKET_ID pkt_id = SS4_PACKET_ID_IDLE;
 
 	switch (byte[3] & 0x30) {
 	case 0x00:
-		if (byte[0] == 0x18 && byte[1] == 0x10 && byte[2] == 0x00 &&
-		    (byte[3] & 0x88) == 0x08 && byte[4] == 0x10 &&
-		    byte[5] == 0x00) {
+		if (SS4_IS_IDLE_V2(byte)) {
 			pkt_id = SS4_PACKET_ID_IDLE;
 		} else {
 			pkt_id = SS4_PACKET_ID_ONE;

@@ -1188,7 +1186,7 @@ static int alps_decode_ss4_v2(struct alps_fields *f,
 			      unsigned char *p, struct psmouse *psmouse)
 {
 	struct alps_data *priv = psmouse->private;
-	unsigned char pkt_id;
+	enum SS4_PACKET_ID pkt_id;
 	unsigned int no_data_x, no_data_y;
 
 	pkt_id = alps_get_pkt_id_ss4_v2(p);

@@ -1267,9 +1265,9 @@ static int alps_decode_ss4_v2(struct alps_fields *f,
 		break;
 
 	case SS4_PACKET_ID_STICK:
-		f->st.x = (s8)(((p[0] & 1) << 7) | (p[1] & 0x7f));
-		f->st.y = -(s8)(((p[3] & 1) << 7) | (p[2] & 0x7f));
-		f->pressure = (s8)(p[4] & 0x7f);
+		f->st.x = SS4_TS_X_V2(p);
+		f->st.y = SS4_TS_Y_V2(p);
+		f->pressure = SS4_TS_Z_V2(p);
 		f->first_mp = 0;
 		f->is_mp = 0;
 		break;

@@ -1361,14 +1359,13 @@ static void alps_process_packet_ss4_v2(struct psmouse *psmouse)
 
 	/* Report touchpad */
 	alps_report_mt_data(psmouse, (f->fingers <= 4) ? f->fingers : 4);
-
 	input_mt_report_finger_count(dev, f->fingers);
+	input_report_abs(dev, ABS_PRESSURE, f->pressure);
 
 	input_report_key(dev, BTN_LEFT, f->left);
 	input_report_key(dev, BTN_RIGHT, f->right);
 	input_report_key(dev, BTN_MIDDLE, f->middle);
 
-	input_report_abs(dev, ABS_PRESSURE, f->pressure);
 	input_sync(dev);
 }
drivers/input/mouse/alps.h +19 -1
index b9417e2..e221dab 100644
@@ -54,7 +54,15 @@ enum SS4_PACKET_ID {
 
 #define SS4_MASK_NORMAL_BUTTONS		0x07
 
-#define SS4_1F_X_V2(_b)		((_b[0] & 0x0007) |		\
+#define SS4_IS_IDLE_V2(_b)	(((_b[0]) == 0x18) &&		\
+				 ((_b[1]) == 0x10) &&		\
+				 ((_b[2]) == 0x00) &&		\
+				 ((_b[3] & 0x88) == 0x08) &&	\
+				 ((_b[4]) == 0x10) &&		\
+				 ((_b[5]) == 0x00)		\
+				)
+
+#define SS4_1F_X_V2(_b)		(((_b[0]) & 0x0007) |		\
 				 ((_b[1] << 3) & 0x0078) |	\
 				 ((_b[1] << 2) & 0x0380) |	\
 				 ((_b[2] << 5) & 0x1C00)	\

@@ -101,6 +109,16 @@ enum SS4_PACKET_ID {
 #define SS4_IS_MF_CONTINUE(_b)	((_b[2] & 0x10) == 0x10)
 #define SS4_IS_5F_DETECTED(_b)	((_b[2] & 0x10) == 0x10)
 
+#define SS4_TS_X_V2(_b)		(((_b[0] & 0x01) << 7) |	\
+				 ((_b[1]) & 0x7F)		\
+				)
+
+#define SS4_TS_Y_V2(_b)		-(((_b[3] & 0x01) << 7) |	\
+				  ((_b[2]) & 0x7F)		\
+				 )
+
+#define SS4_TS_Z_V2(_b)		(_b[4] & 0x7F)
+
 
 #define SS4_MFPACKET_NO_AX	8160	/* X-Coordinate value */
 #define SS4_MFPACKET_NO_AY	4080	/* Y-Coordinate value */
author
#13 lore raw

Re: [PATCH v4 3/3] Input: ALPS - Clean up code for SS5 hardware

drivers/input/mouse/alps.h enum SS4_PACKET_ID { · line 120

Quoted 9 lines · Paul Donohue
drivers/input/mouse/alps.h +9 -0
+#define SS4_TS_X_V2(_b)		(((_b[0] & 0x01) << 7) |	\
+				 ((_b[1]) & 0x7F)		\
+				)
+
+#define SS4_TS_Y_V2(_b)		-(((_b[3] & 0x01) << 7) |	\
+				  ((_b[2]) & 0x7F)		\
+				 )
+
+#define SS4_TS_Z_V2(_b)		(_b[4] & 0x7F)

This isn't working quite right. I'm going to try this patch again.

author
#14 lore raw

PATCH v4 3/3 Input: ALPS - Clean up code for SS5 hardware

The return value of alps_get_pkt_id_ss4_v2() should really be "enum SS4_PACKET_ID", not "unsigned char". Correct this.

Also, most of the Alps SS5 (SS4 v2) packet byte parsing code is implemented using macros, but there are a few places where bytes are directly manipulated in alps.c. For consistency, migrate the rest of these to macros.

Signed-off-by Paul Donohue <[email protected]>
drivers/input/mouse/alps.c +8 -11
index 12376d2..6a2fb69 100644
@@ -1153,15 +1153,13 @@ static void alps_process_packet_v7(struct psmouse *psmouse)
 		alps_process_touchpad_packet_v7(psmouse);
 }
 
-static unsigned char alps_get_pkt_id_ss4_v2(unsigned char *byte)
+static enum SS4_PACKET_ID alps_get_pkt_id_ss4_v2(unsigned char *byte)
 {
-	unsigned char pkt_id = SS4_PACKET_ID_IDLE;
+	enum SS4_PACKET_ID pkt_id = SS4_PACKET_ID_IDLE;
 
 	switch (byte[3] & 0x30) {
 	case 0x00:
-		if (byte[0] == 0x18 && byte[1] == 0x10 && byte[2] == 0x00 &&
-		    (byte[3] & 0x88) == 0x08 && byte[4] == 0x10 &&
-		    byte[5] == 0x00) {
+		if (SS4_IS_IDLE_V2(byte)) {
 			pkt_id = SS4_PACKET_ID_IDLE;
 		} else {
 			pkt_id = SS4_PACKET_ID_ONE;

@@ -1188,7 +1186,7 @@ static int alps_decode_ss4_v2(struct alps_fields *f,
 			      unsigned char *p, struct psmouse *psmouse)
 {
 	struct alps_data *priv = psmouse->private;
-	unsigned char pkt_id;
+	enum SS4_PACKET_ID pkt_id;
 	unsigned int no_data_x, no_data_y;
 
 	pkt_id = alps_get_pkt_id_ss4_v2(p);

@@ -1267,9 +1265,9 @@ static int alps_decode_ss4_v2(struct alps_fields *f,
 		break;
 
 	case SS4_PACKET_ID_STICK:
-		f->st.x = (s8)(((p[0] & 1) << 7) | (p[1] & 0x7f));
-		f->st.y = -(s8)(((p[3] & 1) << 7) | (p[2] & 0x7f));
-		f->pressure = (s8)(p[4] & 0x7f);
+		f->st.x = SS4_TS_X_V2(p);
+		f->st.y = SS4_TS_Y_V2(p);
+		f->pressure = SS4_TS_Z_V2(p);
 		f->first_mp = 0;
 		f->is_mp = 0;
 		break;

@@ -1361,14 +1359,13 @@ static void alps_process_packet_ss4_v2(struct psmouse *psmouse)
 
 	/* Report touchpad */
 	alps_report_mt_data(psmouse, (f->fingers <= 4) ? f->fingers : 4);
-
 	input_mt_report_finger_count(dev, f->fingers);
+	input_report_abs(dev, ABS_PRESSURE, f->pressure);
 
 	input_report_key(dev, BTN_LEFT, f->left);
 	input_report_key(dev, BTN_RIGHT, f->right);
 	input_report_key(dev, BTN_MIDDLE, f->middle);
 
-	input_report_abs(dev, ABS_PRESSURE, f->pressure);
 	input_sync(dev);
 }
drivers/input/mouse/alps.h +21 -1
index b9417e2..96e1d1d 100644
@@ -54,7 +54,15 @@ enum SS4_PACKET_ID {
 
 #define SS4_MASK_NORMAL_BUTTONS		0x07
 
-#define SS4_1F_X_V2(_b)		((_b[0] & 0x0007) |		\
+#define SS4_IS_IDLE_V2(_b)	(((_b[0]) == 0x18) &&		\
+				 ((_b[1]) == 0x10) &&		\
+				 ((_b[2]) == 0x00) &&		\
+				 ((_b[3] & 0x88) == 0x08) &&	\
+				 ((_b[4]) == 0x10) &&		\
+				 ((_b[5]) == 0x00)		\
+				)
+
+#define SS4_1F_X_V2(_b)		(((_b[0]) & 0x0007) |		\
 				 ((_b[1] << 3) & 0x0078) |	\
 				 ((_b[1] << 2) & 0x0380) |	\
 				 ((_b[2] << 5) & 0x1C00)	\

@@ -101,6 +109,18 @@ enum SS4_PACKET_ID {
 #define SS4_IS_MF_CONTINUE(_b)	((_b[2] & 0x10) == 0x10)
 #define SS4_IS_5F_DETECTED(_b)	((_b[2] & 0x10) == 0x10)
 
+#define SS4_TS_X_V2(_b)		(s8)(				\
+				 ((_b[0] & 0x01) << 7) |	\
+				 ((_b[1]) & 0x7F)		\
+				)
+
+#define SS4_TS_Y_V2(_b)		-(s8)(				\
+				 ((_b[3] & 0x01) << 7) |	\
+				 ((_b[2]) & 0x7F)		\
+				)
+
+#define SS4_TS_Z_V2(_b)		(s8)(_b[4] & 0x7F)
+
 
 #define SS4_MFPACKET_NO_AX	8160	/* X-Coordinate value */
 #define SS4_MFPACKET_NO_AY	4080	/* Y-Coordinate value */
#15 lore raw

Re: [PATCH v4 2/3] Input: ALPS - Clean up TrackStick handling for SS5 hardware

In reply to Paul Donohue
Quoted 19 lines · On Tuesday 08 November 2016 10:16:21 Paul Donohue wrote:

For consistency and clarity, the input_report_*() functions should be called by alps_process_packet_ss4_v2() instead of by alps_decode_ss4_v2().

Signed-off-by Paul Donohue <[email protected]>
drivers/input/mouse/alps.c +0 -5
index b93fe83..12376d2 100644
@@ -1267,18 +1267,11 @@ static int alps_decode_ss4_v2(struct alps_fields *f,
 		break;
 
 	case SS4_PACKET_ID_STICK:
-		if (!(priv->flags & ALPS_DUALPOINT)) {
-			psmouse_warn(psmouse,
-				     "Rejected trackstick packet from non DualPoint device");
-		} else {
-			int x = (s8)(((p[0] & 1) << 7) | (p[1] & 0x7f));

drivers/input/mouse/alps.c static int alps_decode_ss4_v2(struct alps_fields *f, · line 1272

Quoted 10 lines
drivers/input/mouse/alps.c +3 -7
-			int y = (s8)(((p[3] & 1) << 7) | (p[2] & 0x7f));
-			int pressure = (s8)(p[4] & 0x7f);
-
-			input_report_rel(priv->dev2, REL_X, x);
-			input_report_rel(priv->dev2, REL_Y, -y);
-			input_report_abs(priv->dev2, ABS_PRESSURE, pressure);
-		}
+		f->st.x = (s8)(((p[0] & 1) << 7) | (p[1] & 0x7f));
+		f->st.y = -(s8)(((p[3] & 1) << 7) | (p[2] & 0x7f));
+		f->pressure = (s8)(p[4] & 0x7f);

This is not correct. Those fields values are used for single touch events from touchpad -- not from trackstick.

Btw, you have access to packet also in process functions, so you can extract x, y and pressure in process function too.

Signature · 2 lines
Pali Rohár
[email protected]
author
#16 lore raw

Re: [PATCH v4 2/3] Input: ALPS - Clean up TrackStick handling for SS5 hardware

Quoted 12 lines · Pali Rohár

drivers/input/mouse/alps.c static int alps_decode_ss4_v2(struct alps_fields *f, · line 1270

Quoted 7 lines · On Tuesday 08 November 2016 10:16:21 Paul Donohue wrote:
drivers/input/mouse/alps.c +3 -0
@@ -1267,18 +1267,11 @@ static int alps_decode_ss4_v2(struct alps_fields *f,
 	case SS4_PACKET_ID_STICK:
+		f->st.x = (s8)(((p[0] & 1) << 7) | (p[1] & 0x7f));
+		f->st.y = -(s8)(((p[3] & 1) << 7) | (p[2] & 0x7f));
+		f->pressure = (s8)(p[4] & 0x7f);

This is not correct. Those fields values are used for single touch events from touchpad -- not from trackstick.

Quoted 2 lines

Btw, you have access to packet also in process functions, so you can extract x, y and pressure in process function too.

I was trying to keep all of the decoding logic in alps_decode_ss4_v2(). And since there aren't any fields specifically for the trackstick in alps_fields, I figured the single touch fields would be an appropriate place to stash those coordinates.

But if you would prefer to move some of the trackstick decoding logic to alps_process_packet_ss4_v2(), I can do that.

#17 lore raw

Re: [PATCH v4 2/3] Input: ALPS - Clean up TrackStick handling for SS5 hardware

Quoted 16 lines · On Thursday 10 November 2016 09:27:39 Paul Donohue wrote:
Quoted 12 lines · Pali Rohár

drivers/input/mouse/alps.c static int alps_decode_ss4_v2(struct alps_fields *f, · line 1270

Quoted 7 lines · On Tuesday 08 November 2016 10:16:21 Paul Donohue wrote:
drivers/input/mouse/alps.c +3 -0
@@ -1267,18 +1267,11 @@ static int alps_decode_ss4_v2(struct alps_fields *f,
 	case SS4_PACKET_ID_STICK:
+		f->st.x = (s8)(((p[0] & 1) << 7) | (p[1] & 0x7f));
+		f->st.y = -(s8)(((p[3] & 1) << 7) | (p[2] & 0x7f));
+		f->pressure = (s8)(p[4] & 0x7f);

This is not correct. Those fields values are used for single touch events from touchpad -- not from trackstick.

Quoted 2 lines

Btw, you have access to packet also in process functions, so you can extract x, y and pressure in process function too.

Quoted 7 lines

I was trying to keep all of the decoding logic in alps_decode_ss4_v2(). And since there aren't any fields specifically for the trackstick in alps_fields, I figured the single touch fields would be an appropriate place to stash those coordinates.

But if you would prefer to move some of the trackstick decoding logic to alps_process_packet_ss4_v2(), I can do that.

I see that e.g. alps_process_packet_v1_v2() or alps_process_packet_v6() or alps_process_trackstick_packet_v3() having decode logic... So for me it make sense to have it same also in ss4. But if some other developers prefer something different, let us know!

Quoted 1 line

From current code I understand that decode functions are there to fill

"struct alps_fields" and process functions to process and send events.

I would suggest to read code for other alps protocol versions and have similar behaviour for ss4 as for other protocol versions. At least this produce code which will be consistent...

Signature · 2 lines
Pali Rohár
[email protected]
author
#18 lore raw

v5 cover Input: ALPS - Bug fix and cleanup for SS5 hardware

This series includes a bug fix and assorted cleanup for the Alps SS5 (SS4 v2) code.

Changes in v5:
* Moved TrackStick decoding logic to alps_process_packet_ss4_v2() to make the code more consistent with other protocol versions (Suggested by Pali Rohár <[email protected]>)

Changes in v4:
* Fixed patch formatting issues
* Correct casting issues in macros added in v3

Changes in v3:
* Added additional code cleanup to make the code easier to understand (Suggested by Pali Rohár <[email protected]>)

Changes in v2:
* For consistency with other Alps functions, check packet bytes for packet type rather than adding a flag to alps_fields for packet type. (Suggested by Pali Rohár <[email protected]>)

Paul Donohue (3):
Input: ALPS - Fix TrackStick support for SS5 hardware
Input: ALPS - Clean up TrackStick handling for SS5 hardware
Input: ALPS - Clean up code for SS5 hardware

 drivers/input/mouse/alps.c | 59 +++++++++++++++++++++++++---------------------
 drivers/input/mouse/alps.h | 22 ++++++++++++++++-
 2 files changed, 53 insertions(+), 28 deletions(-)
Signature · 1 line
2.7.4
author
#19 lore raw

PATCH v5 1/3 Input: ALPS - Fix TrackStick support for SS5 hardware

The current Alps SS5 (SS4 v2) code generates bogus TouchPad events when TrackStick packets are processed.

This causes the xorg synaptics driver to print
"unable to find touch point 0" and
"BUG: triggered 'if (priv->num_active_touches > priv->num_slots)'" messages. It also causes unexpected TouchPad button release and reclick event sequences if the TrackStick is moved while holding a TouchPad button.

This commit corrects the problem by adjusting alps_process_packet_ss4_v2() so that it only sends TrackStick reports when processing TrackStick packets.

Signed-off-by Paul Donohue <[email protected]>
---
 drivers/input/mouse/alps.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)
drivers/input/mouse/alps.c +12 -7
index 6d7de9b..b93fe83 100644
@@ -1346,6 +1346,18 @@ static void alps_process_packet_ss4_v2(struct psmouse *psmouse)
 
 	priv->multi_packet = 0;
 
+	/* Report trackstick */
+	if (alps_get_pkt_id_ss4_v2(packet) == SS4_PACKET_ID_STICK) {
+		if (priv->flags & ALPS_DUALPOINT) {
+			input_report_key(dev2, BTN_LEFT, f->ts_left);
+			input_report_key(dev2, BTN_RIGHT, f->ts_right);
+			input_report_key(dev2, BTN_MIDDLE, f->ts_middle);
+			input_sync(dev2);
+		}
+		return;
+	}
+
+	/* Report touchpad */
 	alps_report_mt_data(psmouse, (f->fingers <= 4) ? f->fingers : 4);
 
 	input_mt_report_finger_count(dev, f->fingers);

@@ -1356,13 +1368,6 @@ static void alps_process_packet_ss4_v2(struct psmouse *psmouse)
 
 	input_report_abs(dev, ABS_PRESSURE, f->pressure);
 	input_sync(dev);
-
-	if (priv->flags & ALPS_DUALPOINT) {
-		input_report_key(dev2, BTN_LEFT, f->ts_left);
-		input_report_key(dev2, BTN_RIGHT, f->ts_right);
-		input_report_key(dev2, BTN_MIDDLE, f->ts_middle);
-		input_sync(dev2);
-	}
 }
 
 static bool alps_is_valid_package_ss4_v2(struct psmouse *psmouse)
Signature · 1 line
2.7.4
author
#20 lore raw

PATCH v5 2/3 Input: ALPS - Clean up TrackStick handling for SS5 hardware

In reply to Paul Donohue

For consistency and clarity, the input_report_*() functions should be called by alps_process_packet_ss4_v2() instead of by alps_decode_ss4_v2().

Signed-off-by Paul Donohue <[email protected]>
---
 drivers/input/mouse/alps.c | 42 +++++++++++++++++++++++++-----------------
 1 file changed, 25 insertions(+), 17 deletions(-)
drivers/input/mouse/alps.c +25 -17
index b93fe83..61d61cc 100644
@@ -1267,18 +1267,12 @@ static int alps_decode_ss4_v2(struct alps_fields *f,
 		break;
 
 	case SS4_PACKET_ID_STICK:
-		if (!(priv->flags & ALPS_DUALPOINT)) {
-			psmouse_warn(psmouse,
-				     "Rejected trackstick packet from non DualPoint device");
-		} else {
-			int x = (s8)(((p[0] & 1) << 7) | (p[1] & 0x7f));
-			int y = (s8)(((p[3] & 1) << 7) | (p[2] & 0x7f));
-			int pressure = (s8)(p[4] & 0x7f);
-
-			input_report_rel(priv->dev2, REL_X, x);
-			input_report_rel(priv->dev2, REL_Y, -y);
-			input_report_abs(priv->dev2, ABS_PRESSURE, pressure);
-		}
+		/*
+		 * x, y, and pressure are decoded in
+		 * alps_process_packet_ss4_v2()
+		 */
+		f->first_mp = 0;
+		f->is_mp = 0;
 		break;
 
 	case SS4_PACKET_ID_IDLE:

@@ -1312,6 +1306,7 @@ static void alps_process_packet_ss4_v2(struct psmouse *psmouse)
 	struct input_dev *dev = psmouse->dev;
 	struct input_dev *dev2 = priv->dev2;
 	struct alps_fields *f = &priv->f;
+	int x, y, pressure;
 
 	memset(f, 0, sizeof(struct alps_fields));
 	priv->decode_fields(f, packet, psmouse);

@@ -1348,12 +1343,25 @@ static void alps_process_packet_ss4_v2(struct psmouse *psmouse)
 
 	/* Report trackstick */
 	if (alps_get_pkt_id_ss4_v2(packet) == SS4_PACKET_ID_STICK) {
-		if (priv->flags & ALPS_DUALPOINT) {
-			input_report_key(dev2, BTN_LEFT, f->ts_left);
-			input_report_key(dev2, BTN_RIGHT, f->ts_right);
-			input_report_key(dev2, BTN_MIDDLE, f->ts_middle);
-			input_sync(dev2);
+		if (!(priv->flags & ALPS_DUALPOINT)) {
+			psmouse_warn(psmouse,
+				     "Rejected trackstick packet from non DualPoint device");
+			return;
 		}
+
+		x = (s8)(((packet[0] & 1) << 7) | (packet[1] & 0x7f));
+		y = (s8)(((packet[3] & 1) << 7) | (packet[2] & 0x7f));
+		pressure = (s8)(packet[4] & 0x7f);
+
+		input_report_rel(dev2, REL_X, x);
+		input_report_rel(dev2, REL_Y, -y);
+		input_report_abs(dev2, ABS_PRESSURE, pressure);
+
+		input_report_key(dev2, BTN_LEFT, f->ts_left);
+		input_report_key(dev2, BTN_RIGHT, f->ts_right);
+		input_report_key(dev2, BTN_MIDDLE, f->ts_middle);
+
+		input_sync(dev2);
 		return;
 	}
Signature · 1 line
2.7.4
author
#21 lore raw

PATCH v5 3/3 Input: ALPS - Clean up code for SS5 hardware

In reply to Paul Donohue

The return value of alps_get_pkt_id_ss4_v2() should really be "enum SS4_PACKET_ID", not "unsigned char". Correct this.

Also, most of the Alps SS5 (SS4 v2) packet byte parsing code is implemented using macros, but there are a few places where bytes are directly manipulated in alps.c. For consistency, migrate the rest of these to macros.

Signed-off-by Paul Donohue <[email protected]>
---
 drivers/input/mouse/alps.c | 24 ++++++++----------------
 drivers/input/mouse/alps.h | 22 +++++++++++++++++++++-
 2 files changed, 29 insertions(+), 17 deletions(-)
drivers/input/mouse/alps.c +8 -16
index 61d61cc..c984b8c 100644
@@ -1153,15 +1153,13 @@ static void alps_process_packet_v7(struct psmouse *psmouse)
 		alps_process_touchpad_packet_v7(psmouse);
 }
 
-static unsigned char alps_get_pkt_id_ss4_v2(unsigned char *byte)
+static enum SS4_PACKET_ID alps_get_pkt_id_ss4_v2(unsigned char *byte)
 {
-	unsigned char pkt_id = SS4_PACKET_ID_IDLE;
+	enum SS4_PACKET_ID pkt_id = SS4_PACKET_ID_IDLE;
 
 	switch (byte[3] & 0x30) {
 	case 0x00:
-		if (byte[0] == 0x18 && byte[1] == 0x10 && byte[2] == 0x00 &&
-		    (byte[3] & 0x88) == 0x08 && byte[4] == 0x10 &&
-		    byte[5] == 0x00) {
+		if (SS4_IS_IDLE_V2(byte)) {
 			pkt_id = SS4_PACKET_ID_IDLE;
 		} else {
 			pkt_id = SS4_PACKET_ID_ONE;

@@ -1188,7 +1186,7 @@ static int alps_decode_ss4_v2(struct alps_fields *f,
 			      unsigned char *p, struct psmouse *psmouse)
 {
 	struct alps_data *priv = psmouse->private;
-	unsigned char pkt_id;
+	enum SS4_PACKET_ID pkt_id;
 	unsigned int no_data_x, no_data_y;
 
 	pkt_id = alps_get_pkt_id_ss4_v2(p);

@@ -1306,7 +1304,6 @@ static void alps_process_packet_ss4_v2(struct psmouse *psmouse)
 	struct input_dev *dev = psmouse->dev;
 	struct input_dev *dev2 = priv->dev2;
 	struct alps_fields *f = &priv->f;
-	int x, y, pressure;
 
 	memset(f, 0, sizeof(struct alps_fields));
 	priv->decode_fields(f, packet, psmouse);

@@ -1349,13 +1346,9 @@ static void alps_process_packet_ss4_v2(struct psmouse *psmouse)
 			return;
 		}
 
-		x = (s8)(((packet[0] & 1) << 7) | (packet[1] & 0x7f));
-		y = (s8)(((packet[3] & 1) << 7) | (packet[2] & 0x7f));
-		pressure = (s8)(packet[4] & 0x7f);
-
-		input_report_rel(dev2, REL_X, x);
-		input_report_rel(dev2, REL_Y, -y);
-		input_report_abs(dev2, ABS_PRESSURE, pressure);
+		input_report_rel(dev2, REL_X, SS4_TS_X_V2(packet));
+		input_report_rel(dev2, REL_Y, SS4_TS_Y_V2(packet));
+		input_report_abs(dev2, ABS_PRESSURE, SS4_TS_Z_V2(packet));
 
 		input_report_key(dev2, BTN_LEFT, f->ts_left);
 		input_report_key(dev2, BTN_RIGHT, f->ts_right);

@@ -1367,14 +1360,13 @@ static void alps_process_packet_ss4_v2(struct psmouse *psmouse)
 
 	/* Report touchpad */
 	alps_report_mt_data(psmouse, (f->fingers <= 4) ? f->fingers : 4);
-
 	input_mt_report_finger_count(dev, f->fingers);
+	input_report_abs(dev, ABS_PRESSURE, f->pressure);
 
 	input_report_key(dev, BTN_LEFT, f->left);
 	input_report_key(dev, BTN_RIGHT, f->right);
 	input_report_key(dev, BTN_MIDDLE, f->middle);
 
-	input_report_abs(dev, ABS_PRESSURE, f->pressure);
 	input_sync(dev);
 }
drivers/input/mouse/alps.h +21 -1
index b9417e2..96e1d1d 100644
@@ -54,7 +54,15 @@ enum SS4_PACKET_ID {
 
 #define SS4_MASK_NORMAL_BUTTONS		0x07
 
-#define SS4_1F_X_V2(_b)		((_b[0] & 0x0007) |		\
+#define SS4_IS_IDLE_V2(_b)	(((_b[0]) == 0x18) &&		\
+				 ((_b[1]) == 0x10) &&		\
+				 ((_b[2]) == 0x00) &&		\
+				 ((_b[3] & 0x88) == 0x08) &&	\
+				 ((_b[4]) == 0x10) &&		\
+				 ((_b[5]) == 0x00)		\
+				)
+
+#define SS4_1F_X_V2(_b)		(((_b[0]) & 0x0007) |		\
 				 ((_b[1] << 3) & 0x0078) |	\
 				 ((_b[1] << 2) & 0x0380) |	\
 				 ((_b[2] << 5) & 0x1C00)	\

@@ -101,6 +109,18 @@ enum SS4_PACKET_ID {
 #define SS4_IS_MF_CONTINUE(_b)	((_b[2] & 0x10) == 0x10)
 #define SS4_IS_5F_DETECTED(_b)	((_b[2] & 0x10) == 0x10)
 
+#define SS4_TS_X_V2(_b)		(s8)(				\
+				 ((_b[0] & 0x01) << 7) |	\
+				 ((_b[1]) & 0x7F)		\
+				)
+
+#define SS4_TS_Y_V2(_b)		-(s8)(				\
+				 ((_b[3] & 0x01) << 7) |	\
+				 ((_b[2]) & 0x7F)		\
+				)
+
+#define SS4_TS_Z_V2(_b)		(s8)(_b[4] & 0x7F)
+
 
 #define SS4_MFPACKET_NO_AX	8160	/* X-Coordinate value */
 #define SS4_MFPACKET_NO_AY	4080	/* Y-Coordinate value */
Signature · 1 line
2.7.4
#22 lore raw

Re: [PATCH v5 0/3] Input: ALPS - Bug fix and cleanup for SS5 hardware

In reply to Paul Donohue
Quoted 26 lines · On Thursday 10 November 2016 17:44:43 Paul Donohue wrote:

This series includes a bug fix and assorted cleanup for the Alps SS5 (SS4 v2) code.

Changes in v5:
* Moved TrackStick decoding logic to alps_process_packet_ss4_v2() to make the code more consistent with other protocol versions (Suggested by Pali Rohár <[email protected]>)

Changes in v4:
* Fixed patch formatting issues
* Correct casting issues in macros added in v3

Changes in v3:
* Added additional code cleanup to make the code easier to understand (Suggested by Pali Rohár <[email protected]>)

Changes in v2:
* For consistency with other Alps functions, check packet bytes for packet type rather than adding a flag to alps_fields for packet
type. (Suggested by Pali Rohár <[email protected]>)

Paul Donohue (3):
Input: ALPS - Fix TrackStick support for SS5 hardware
Input: ALPS - Clean up TrackStick handling for SS5 hardware
Input: ALPS - Clean up code for SS5 hardware

Quoted 4 lines
 drivers/input/mouse/alps.c | 59
+++++++++++++++++++++++++---------------------
drivers/input/mouse/alps.h | 22 ++++++++++++++++-
 2 files changed, 53 insertions(+), 28 deletions(-)

Whole patch series looks fine.
You can add my Reviewed-by: Pali Rohár <[email protected]>

Signature · 2 lines
Pali Rohár
[email protected]
maintainer
#23 lore raw

Re: [PATCH v5 0/3] Input: ALPS - Bug fix and cleanup for SS5 hardware

Quoted 27 lines · Pali Rohár
Quoted 26 lines · On Thursday 10 November 2016 17:44:43 Paul Donohue wrote:

This series includes a bug fix and assorted cleanup for the Alps SS5 (SS4 v2) code.

Changes in v5:
* Moved TrackStick decoding logic to alps_process_packet_ss4_v2() to make the code more consistent with other protocol versions (Suggested by Pali Rohár <[email protected]>)

Changes in v4:
* Fixed patch formatting issues
* Correct casting issues in macros added in v3

Changes in v3:
* Added additional code cleanup to make the code easier to understand (Suggested by Pali Rohár <[email protected]>)

Changes in v2:
* For consistency with other Alps functions, check packet bytes for packet type rather than adding a flag to alps_fields for packet
type. (Suggested by Pali Rohár <[email protected]>)

Paul Donohue (3):
Input: ALPS - Fix TrackStick support for SS5 hardware
Input: ALPS - Clean up TrackStick handling for SS5 hardware
Input: ALPS - Clean up code for SS5 hardware

Quoted 7 lines
Quoted 4 lines
 drivers/input/mouse/alps.c | 59
+++++++++++++++++++++++++---------------------
drivers/input/mouse/alps.h | 22 ++++++++++++++++-
 2 files changed, 53 insertions(+), 28 deletions(-)

Whole patch series looks fine.
You can add my Reviewed-by: Pali Rohár <[email protected]>

FYI: I do not see a dingle email from Paul, only your replies.

Thanks.

Signature · 1 line
Dmitry
#24 lore raw

Re: [PATCH v5 0/3] Input: ALPS - Bug fix and cleanup for SS5 hardware

Quoted 40 lines · On Saturday 12 November 2016 01:56:21 Dmitry Torokhov wrote:
Quoted 39 lines · Pali Rohár
Quoted 38 lines · On Thursday 10 November 2016 17:44:43 Paul Donohue wrote:

This series includes a bug fix and assorted cleanup for the Alps SS5 (SS4 v2) code.

Changes in v5:
* Moved TrackStick decoding logic to alps_process_packet_ss4_v2() to

make the code more consistent with other protocol versions (Suggested by Pali Rohár <[email protected]>)

Changes in v4:
* Fixed patch formatting issues
* Correct casting issues in macros added in v3

Changes in v3:
* Added additional code cleanup to make the code easier to understand

(Suggested by Pali Rohár <[email protected]>)

Changes in v2:
* For consistency with other Alps functions, check packet bytes for

packet type rather than adding a flag to alps_fields for packet

type. (Suggested by Pali Rohár <[email protected]>)

Paul Donohue (3):
Input: ALPS - Fix TrackStick support for SS5 hardware
Input: ALPS - Clean up TrackStick handling for SS5 hardware
Input: ALPS - Clean up code for SS5 hardware

 drivers/input/mouse/alps.c | 59

+++++++++++++++++++++++++---------------------
drivers/input/mouse/alps.h | 22 ++++++++++++++++-

Quoted 6 lines
Quoted 4 lines
Quoted 1 line
 2 files changed, 53 insertions(+), 28 deletions(-)

Whole patch series looks fine.
You can add my Reviewed-by: Pali Rohár <[email protected]>

FYI: I do not see a dingle email from Paul, only your replies.

Patches were sent to [email protected] ML. Maybe some spam filter ate emails? Btw I added you to receivers of my last sent email.

Signature · 2 lines
Pali Rohár
[email protected]
maintainer
#25 lore raw

Re: [PATCH v5 0/3] Input: ALPS - Bug fix and cleanup for SS5 hardware

Quoted 43 lines · Pali Rohár
Quoted 42 lines · On Saturday 12 November 2016 01:56:21 Dmitry Torokhov wrote:
Quoted 41 lines · Pali Rohár
Quoted 39 lines · On Thursday 10 November 2016 17:44:43 Paul Donohue wrote:

This series includes a bug fix and assorted cleanup for the Alps SS5 (SS4 v2) code.

Changes in v5:
* Moved TrackStick decoding logic to alps_process_packet_ss4_v2() to

make the code more consistent with other protocol versions (Suggested by Pali Rohár <[email protected]>)

Changes in v4:
* Fixed patch formatting issues
* Correct casting issues in macros added in v3

Changes in v3:
* Added additional code cleanup to make the code easier to understand

(Suggested by Pali Rohár <[email protected]>)

Changes in v2:
* For consistency with other Alps functions, check packet bytes for

packet type rather than adding a flag to alps_fields for packet

type. (Suggested by Pali Rohár <[email protected]>)

Paul Donohue (3):
Input: ALPS - Fix TrackStick support for SS5 hardware
Input: ALPS - Clean up TrackStick handling for SS5 hardware
Input: ALPS - Clean up code for SS5 hardware

 drivers/input/mouse/alps.c | 59

+++++++++++++++++++++++++---------------------
drivers/input/mouse/alps.h | 22 ++++++++++++++++-

 2 files changed, 53 insertions(+), 28 deletions(-)
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines

Whole patch series looks fine.
You can add my Reviewed-by: Pali Rohár <[email protected]>

FYI: I do not see a dingle email from Paul, only your replies.

Patches were sent to [email protected] ML. Maybe some spam filter ate emails? Btw I added you to receivers of my last sent email.

Yes, I looked there as well and did not see anything but you replies.

Thanks.

Signature · 1 line
Dmitry
#26 lore raw

Re: [PATCH v5 0/3] Input: ALPS - Bug fix and cleanup for SS5 hardware

Quoted 50 lines · On Saturday 12 November 2016 02:06:40 Dmitry Torokhov wrote:
Quoted 49 lines · Pali Rohár
Quoted 47 lines · On Saturday 12 November 2016 01:56:21 Dmitry Torokhov wrote:
Quoted 37 lines · Pali Rohár
Quoted 36 lines · On Thursday 10 November 2016 17:44:43 Paul Donohue wrote:

This series includes a bug fix and assorted cleanup for the Alps SS5 (SS4 v2) code.

Changes in v5:
* Moved TrackStick decoding logic to
alps_process_packet_ss4_v2() to

make the code more consistent with other protocol versions (Suggested by Pali Rohár <[email protected]>)

Changes in v4:
* Fixed patch formatting issues
* Correct casting issues in macros added in v3

Changes in v3:
* Added additional code cleanup to make the code easier to understand

(Suggested by Pali Rohár <[email protected]>)

Changes in v2:
* For consistency with other Alps functions, check packet bytes for

packet type rather than adding a flag to alps_fields for packet

type. (Suggested by Pali Rohár <[email protected]>)

Paul Donohue (3):
Input: ALPS - Fix TrackStick support for SS5 hardware
Input: ALPS - Clean up TrackStick handling for SS5 hardware
Input: ALPS - Clean up code for SS5 hardware

 drivers/input/mouse/alps.c | 59
Quoted 7 lines
Quoted 4 lines

+++++++++++++++++++++++++---------------------
drivers/input/mouse/alps.h | 22 ++++++++++++++++-

 2 files changed, 53 insertions(+), 28 deletions(-)

Whole patch series looks fine.
You can add my Reviewed-by: Pali Rohár <[email protected]>

FYI: I do not see a dingle email from Paul, only your replies.

Quoted 7 lines
Quoted 3 lines

Patches were sent to [email protected] ML. Maybe some spam filter ate emails? Btw I added you to receivers of my last sent email.

Yes, I looked there as well and did not see anything but you replies.

Thanks.

Do you need to resend patches? Or have you already found them?

Signature · 2 lines
Pali Rohár
[email protected]
maintainer
#27 lore raw

Re: [PATCH v5 0/3] Input: ALPS - Bug fix and cleanup for SS5 hardware

Quoted 55 lines · Pali Rohár
Quoted 54 lines · On Saturday 12 November 2016 02:06:40 Dmitry Torokhov wrote:
Quoted 52 lines · Pali Rohár
Quoted 41 lines · On Saturday 12 November 2016 01:56:21 Dmitry Torokhov wrote:
Quoted 40 lines · Pali Rohár
Quoted 39 lines · On Thursday 10 November 2016 17:44:43 Paul Donohue wrote:

This series includes a bug fix and assorted cleanup for the Alps SS5 (SS4 v2) code.

Changes in v5:
* Moved TrackStick decoding logic to
alps_process_packet_ss4_v2() to

make the code more consistent with other protocol versions (Suggested by Pali Rohár <[email protected]>)

Changes in v4:
* Fixed patch formatting issues
* Correct casting issues in macros added in v3

Changes in v3:
* Added additional code cleanup to make the code easier to understand

(Suggested by Pali Rohár <[email protected]>)

Changes in v2:
* For consistency with other Alps functions, check packet bytes for

packet type rather than adding a flag to alps_fields for packet

type. (Suggested by Pali Rohár <[email protected]>)

Paul Donohue (3):
Input: ALPS - Fix TrackStick support for SS5 hardware
Input: ALPS - Clean up TrackStick handling for SS5 hardware
Input: ALPS - Clean up code for SS5 hardware

 drivers/input/mouse/alps.c | 59

+++++++++++++++++++++++++---------------------
drivers/input/mouse/alps.h | 22 ++++++++++++++++-

Quoted 6 lines
Quoted 4 lines
Quoted 1 line
 2 files changed, 53 insertions(+), 28 deletions(-)

Whole patch series looks fine.
You can add my Reviewed-by: Pali Rohár <[email protected]>

FYI: I do not see a dingle email from Paul, only your replies.

Patches were sent to [email protected] ML. Maybe some spam filter ate emails? Btw I added you to receivers of my last sent email.

Quoted 6 lines
Quoted 3 lines

Yes, I looked there as well and did not see anything but you replies.

Thanks.

Do you need to resend patches? Or have you already found them?

Hmm, was the latest v5? I see https://patchwork.kernel.org/patch/9421473/

It looks like Paul is running his own mailserver on what looks like Verizon IP pool, which in this day and age likely makes gmail drop his mails.

Thanks.

Signature · 1 line
Dmitry
#28 lore raw

Re: [PATCH v5 0/3] Input: ALPS - Bug fix and cleanup for SS5 hardware

Quoted 62 lines · On Sunday 13 November 2016 07:43:35 Dmitry Torokhov wrote:
Quoted 60 lines · Pali Rohár
Quoted 59 lines · On Saturday 12 November 2016 02:06:40 Dmitry Torokhov wrote:
Quoted 49 lines · Pali Rohár
Quoted 48 lines · On Saturday 12 November 2016 01:56:21 Dmitry Torokhov wrote:
Quoted 46 lines · Pali Rohár
Quoted 35 lines · On Thursday 10 November 2016 17:44:43 Paul Donohue wrote:

This series includes a bug fix and assorted cleanup for the Alps SS5 (SS4 v2) code.

Changes in v5:
* Moved TrackStick decoding logic to
alps_process_packet_ss4_v2() to

make the code more consistent with other protocol versions (Suggested by Pali Rohár <[email protected]>)

Changes in v4:
* Fixed patch formatting issues
* Correct casting issues in macros added in v3

Changes in v3:
* Added additional code cleanup to make the code easier to understand

(Suggested by Pali Rohár <[email protected]>)

Changes in v2:
* For consistency with other Alps functions, check packet bytes for

packet type rather than adding a flag to alps_fields for packet

type. (Suggested by Pali Rohár <[email protected]>)

Paul Donohue (3):
Input: ALPS - Fix TrackStick support for SS5 hardware
Input: ALPS - Clean up TrackStick handling for SS5 hardware Input: ALPS - Clean up code for SS5 hardware

Quoted 6 lines
 drivers/input/mouse/alps.c | 59

+++++++++++++++++++++++++---------------------
drivers/input/mouse/alps.h | 22 ++++++++++++++++-

 2 files changed, 53 insertions(+), 28 deletions(-)

Whole patch series looks fine.
You can add my Reviewed-by: Pali Rohár
<[email protected]>

Quoted 5 lines
Quoted 1 line

FYI: I do not see a dingle email from Paul, only your replies.

Patches were sent to [email protected] ML. Maybe some spam filter ate emails? Btw I added you to receivers of my last sent email.

Yes, I looked there as well and did not see anything but you replies.

Quoted 6 lines
Quoted 3 lines
Quoted 1 line

Thanks.

Do you need to resend patches? Or have you already found them?

Hmm, was the latest v5? I see
https://patchwork.kernel.org/patch/9421473/

Yes, v5 is the last.

Quoted 3 lines

It looks like Paul is running his own mailserver on what looks like Verizon IP pool, which in this day and age likely makes gmail drop his mails.

Hm... looks weird... because at least I received his emails on my gmail account...

Signature · 2 lines
Pali Rohár
[email protected]
author
#29 lore raw

Re: [PATCH v5 0/3] Input: ALPS - Bug fix and cleanup for SS5 hardware

Quoted 11 lines · Pali Rohár
Quoted 8 lines · On Sunday 13 November 2016 07:43:35 Dmitry Torokhov wrote:
Quoted 8 lines
Quoted 8 lines
Quoted 5 lines
Quoted 1 line

FYI: I do not see a dingle email from Paul, only your replies.

Patches were sent to [email protected] ML. Maybe some spam filter ate emails? Btw I added you to receivers of my last sent email.

Yes, I looked there as well and did not see anything but you replies.

Quoted 6 lines
Quoted 3 lines

It looks like Paul is running his own mailserver on what looks like Verizon IP pool, which in this day and age likely makes gmail drop his mails.

Hm... looks weird... because at least I received his emails on my gmail account...

Very weird. Yes, I run my own mail servers ... incoming mail goes to a Verizon IP, but outgoing mail is sent from a US-based web / cloud hosting provider (specifically for this reason - to avoid spam filters).

Either way, the mailing list itself is clearly getting my messages (since they show up in the web archive), so spam filtering must be happening after the message has been forwarded by the mailing list.

Do the messages show up in your spam folder on gmail? If not, we might have to ask the admins for vger.kernel.org to check their logs so we can figure out where the messages are being lost.

author
#30 lore raw

v6 cover Input: ALPS - Bug fix and cleanup for SS5 hardware

In reply to Pali Rohár

This series includes a bug fix and assorted cleanup for the Alps SS5 (SS4 v2) code.

Changes in v6:
* Added Reviewed-by: Pali Rohár <[email protected]>
* No code changes

Changes in v5:
* Moved TrackStick decoding logic to alps_process_packet_ss4_v2() to make the code more consistent with other protocol versions (Suggested by Pali Rohár <[email protected]>)

Changes in v4:
* Fixed patch formatting issues
* Correct casting issues in macros added in v3

Changes in v3:
* Added additional code cleanup to make the code easier to understand (Suggested by Pali Rohár <[email protected]>)

Changes in v2:
* For consistency with other Alps functions, check packet bytes for packet type rather than adding a flag to alps_fields for packet type. (Suggested by Pali Rohár <[email protected]>)

Paul Donohue (3):
Input: ALPS - Fix TrackStick support for SS5 hardware
Input: ALPS - Clean up TrackStick handling for SS5 hardware
Input: ALPS - Clean up code for SS5 hardware

 drivers/input/mouse/alps.c | 59 +++++++++++++++++++++++++---------------------
 drivers/input/mouse/alps.h | 22 ++++++++++++++++-
 2 files changed, 53 insertions(+), 28 deletions(-)
Signature · 1 line
2.7.4
author
#31 lore raw

PATCH v6 1/3 Input: ALPS - Fix TrackStick support for SS5 hardware

The current Alps SS5 (SS4 v2) code generates bogus TouchPad events when TrackStick packets are processed.

This causes the xorg synaptics driver to print
"unable to find touch point 0" and
"BUG: triggered 'if (priv->num_active_touches > priv->num_slots)'" messages. It also causes unexpected TouchPad button release and reclick event sequences if the TrackStick is moved while holding a TouchPad button.

This commit corrects the problem by adjusting alps_process_packet_ss4_v2() so that it only sends TrackStick reports when processing TrackStick packets.

Reviewed-by Pali Rohár <[email protected]>
Signed-off-by Paul Donohue <[email protected]>
---
 drivers/input/mouse/alps.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)
drivers/input/mouse/alps.c +12 -7
index 6d7de9b..b93fe83 100644
@@ -1346,6 +1346,18 @@ static void alps_process_packet_ss4_v2(struct psmouse *psmouse)
 
 	priv->multi_packet = 0;
 
+	/* Report trackstick */
+	if (alps_get_pkt_id_ss4_v2(packet) == SS4_PACKET_ID_STICK) {
+		if (priv->flags & ALPS_DUALPOINT) {
+			input_report_key(dev2, BTN_LEFT, f->ts_left);
+			input_report_key(dev2, BTN_RIGHT, f->ts_right);
+			input_report_key(dev2, BTN_MIDDLE, f->ts_middle);
+			input_sync(dev2);
+		}
+		return;
+	}
+
+	/* Report touchpad */
 	alps_report_mt_data(psmouse, (f->fingers <= 4) ? f->fingers : 4);
 
 	input_mt_report_finger_count(dev, f->fingers);

@@ -1356,13 +1368,6 @@ static void alps_process_packet_ss4_v2(struct psmouse *psmouse)
 
 	input_report_abs(dev, ABS_PRESSURE, f->pressure);
 	input_sync(dev);
-
-	if (priv->flags & ALPS_DUALPOINT) {
-		input_report_key(dev2, BTN_LEFT, f->ts_left);
-		input_report_key(dev2, BTN_RIGHT, f->ts_right);
-		input_report_key(dev2, BTN_MIDDLE, f->ts_middle);
-		input_sync(dev2);
-	}
 }
 
 static bool alps_is_valid_package_ss4_v2(struct psmouse *psmouse)
Signature · 1 line
2.7.4
author
#32 lore raw

PATCH v6 2/3 Input: ALPS - Clean up TrackStick handling for SS5 hardware

In reply to Paul Donohue

For consistency and clarity, the input_report_*() functions should be called by alps_process_packet_ss4_v2() instead of by alps_decode_ss4_v2().

Reviewed-by Pali Rohár <[email protected]>
Signed-off-by Paul Donohue <[email protected]>
---
 drivers/input/mouse/alps.c | 42 +++++++++++++++++++++++++-----------------
 1 file changed, 25 insertions(+), 17 deletions(-)
drivers/input/mouse/alps.c +25 -17
index b93fe83..61d61cc 100644
@@ -1267,18 +1267,12 @@ static int alps_decode_ss4_v2(struct alps_fields *f,
 		break;
 
 	case SS4_PACKET_ID_STICK:
-		if (!(priv->flags & ALPS_DUALPOINT)) {
-			psmouse_warn(psmouse,
-				     "Rejected trackstick packet from non DualPoint device");
-		} else {
-			int x = (s8)(((p[0] & 1) << 7) | (p[1] & 0x7f));
-			int y = (s8)(((p[3] & 1) << 7) | (p[2] & 0x7f));
-			int pressure = (s8)(p[4] & 0x7f);
-
-			input_report_rel(priv->dev2, REL_X, x);
-			input_report_rel(priv->dev2, REL_Y, -y);
-			input_report_abs(priv->dev2, ABS_PRESSURE, pressure);
-		}
+		/*
+		 * x, y, and pressure are decoded in
+		 * alps_process_packet_ss4_v2()
+		 */
+		f->first_mp = 0;
+		f->is_mp = 0;
 		break;
 
 	case SS4_PACKET_ID_IDLE:

@@ -1312,6 +1306,7 @@ static void alps_process_packet_ss4_v2(struct psmouse *psmouse)
 	struct input_dev *dev = psmouse->dev;
 	struct input_dev *dev2 = priv->dev2;
 	struct alps_fields *f = &priv->f;
+	int x, y, pressure;
 
 	memset(f, 0, sizeof(struct alps_fields));
 	priv->decode_fields(f, packet, psmouse);

@@ -1348,12 +1343,25 @@ static void alps_process_packet_ss4_v2(struct psmouse *psmouse)
 
 	/* Report trackstick */
 	if (alps_get_pkt_id_ss4_v2(packet) == SS4_PACKET_ID_STICK) {
-		if (priv->flags & ALPS_DUALPOINT) {
-			input_report_key(dev2, BTN_LEFT, f->ts_left);
-			input_report_key(dev2, BTN_RIGHT, f->ts_right);
-			input_report_key(dev2, BTN_MIDDLE, f->ts_middle);
-			input_sync(dev2);
+		if (!(priv->flags & ALPS_DUALPOINT)) {
+			psmouse_warn(psmouse,
+				     "Rejected trackstick packet from non DualPoint device");
+			return;
 		}
+
+		x = (s8)(((packet[0] & 1) << 7) | (packet[1] & 0x7f));
+		y = (s8)(((packet[3] & 1) << 7) | (packet[2] & 0x7f));
+		pressure = (s8)(packet[4] & 0x7f);
+
+		input_report_rel(dev2, REL_X, x);
+		input_report_rel(dev2, REL_Y, -y);
+		input_report_abs(dev2, ABS_PRESSURE, pressure);
+
+		input_report_key(dev2, BTN_LEFT, f->ts_left);
+		input_report_key(dev2, BTN_RIGHT, f->ts_right);
+		input_report_key(dev2, BTN_MIDDLE, f->ts_middle);
+
+		input_sync(dev2);
 		return;
 	}
Signature · 1 line
2.7.4
author
#33 lore raw

PATCH v6 3/3 Input: ALPS - Clean up code for SS5 hardware

In reply to Paul Donohue

The return value of alps_get_pkt_id_ss4_v2() should really be "enum SS4_PACKET_ID", not "unsigned char". Correct this.

Also, most of the Alps SS5 (SS4 v2) packet byte parsing code is implemented using macros, but there are a few places where bytes are directly manipulated in alps.c. For consistency, migrate the rest of these to macros.

Reviewed-by Pali Rohár <[email protected]>
Signed-off-by Paul Donohue <[email protected]>
---
 drivers/input/mouse/alps.c | 24 ++++++++----------------
 drivers/input/mouse/alps.h | 22 +++++++++++++++++++++-
 2 files changed, 29 insertions(+), 17 deletions(-)
drivers/input/mouse/alps.c +8 -16
index 61d61cc..c984b8c 100644
@@ -1153,15 +1153,13 @@ static void alps_process_packet_v7(struct psmouse *psmouse)
 		alps_process_touchpad_packet_v7(psmouse);
 }
 
-static unsigned char alps_get_pkt_id_ss4_v2(unsigned char *byte)
+static enum SS4_PACKET_ID alps_get_pkt_id_ss4_v2(unsigned char *byte)
 {
-	unsigned char pkt_id = SS4_PACKET_ID_IDLE;
+	enum SS4_PACKET_ID pkt_id = SS4_PACKET_ID_IDLE;
 
 	switch (byte[3] & 0x30) {
 	case 0x00:
-		if (byte[0] == 0x18 && byte[1] == 0x10 && byte[2] == 0x00 &&
-		    (byte[3] & 0x88) == 0x08 && byte[4] == 0x10 &&
-		    byte[5] == 0x00) {
+		if (SS4_IS_IDLE_V2(byte)) {
 			pkt_id = SS4_PACKET_ID_IDLE;
 		} else {
 			pkt_id = SS4_PACKET_ID_ONE;

@@ -1188,7 +1186,7 @@ static int alps_decode_ss4_v2(struct alps_fields *f,
 			      unsigned char *p, struct psmouse *psmouse)
 {
 	struct alps_data *priv = psmouse->private;
-	unsigned char pkt_id;
+	enum SS4_PACKET_ID pkt_id;
 	unsigned int no_data_x, no_data_y;
 
 	pkt_id = alps_get_pkt_id_ss4_v2(p);

@@ -1306,7 +1304,6 @@ static void alps_process_packet_ss4_v2(struct psmouse *psmouse)
 	struct input_dev *dev = psmouse->dev;
 	struct input_dev *dev2 = priv->dev2;
 	struct alps_fields *f = &priv->f;
-	int x, y, pressure;
 
 	memset(f, 0, sizeof(struct alps_fields));
 	priv->decode_fields(f, packet, psmouse);

@@ -1349,13 +1346,9 @@ static void alps_process_packet_ss4_v2(struct psmouse *psmouse)
 			return;
 		}
 
-		x = (s8)(((packet[0] & 1) << 7) | (packet[1] & 0x7f));
-		y = (s8)(((packet[3] & 1) << 7) | (packet[2] & 0x7f));
-		pressure = (s8)(packet[4] & 0x7f);
-
-		input_report_rel(dev2, REL_X, x);
-		input_report_rel(dev2, REL_Y, -y);
-		input_report_abs(dev2, ABS_PRESSURE, pressure);
+		input_report_rel(dev2, REL_X, SS4_TS_X_V2(packet));
+		input_report_rel(dev2, REL_Y, SS4_TS_Y_V2(packet));
+		input_report_abs(dev2, ABS_PRESSURE, SS4_TS_Z_V2(packet));
 
 		input_report_key(dev2, BTN_LEFT, f->ts_left);
 		input_report_key(dev2, BTN_RIGHT, f->ts_right);

@@ -1367,14 +1360,13 @@ static void alps_process_packet_ss4_v2(struct psmouse *psmouse)
 
 	/* Report touchpad */
 	alps_report_mt_data(psmouse, (f->fingers <= 4) ? f->fingers : 4);
-
 	input_mt_report_finger_count(dev, f->fingers);
+	input_report_abs(dev, ABS_PRESSURE, f->pressure);
 
 	input_report_key(dev, BTN_LEFT, f->left);
 	input_report_key(dev, BTN_RIGHT, f->right);
 	input_report_key(dev, BTN_MIDDLE, f->middle);
 
-	input_report_abs(dev, ABS_PRESSURE, f->pressure);
 	input_sync(dev);
 }
drivers/input/mouse/alps.h +21 -1
index b9417e2..96e1d1d 100644
@@ -54,7 +54,15 @@ enum SS4_PACKET_ID {
 
 #define SS4_MASK_NORMAL_BUTTONS		0x07
 
-#define SS4_1F_X_V2(_b)		((_b[0] & 0x0007) |		\
+#define SS4_IS_IDLE_V2(_b)	(((_b[0]) == 0x18) &&		\
+				 ((_b[1]) == 0x10) &&		\
+				 ((_b[2]) == 0x00) &&		\
+				 ((_b[3] & 0x88) == 0x08) &&	\
+				 ((_b[4]) == 0x10) &&		\
+				 ((_b[5]) == 0x00)		\
+				)
+
+#define SS4_1F_X_V2(_b)		(((_b[0]) & 0x0007) |		\
 				 ((_b[1] << 3) & 0x0078) |	\
 				 ((_b[1] << 2) & 0x0380) |	\
 				 ((_b[2] << 5) & 0x1C00)	\

@@ -101,6 +109,18 @@ enum SS4_PACKET_ID {
 #define SS4_IS_MF_CONTINUE(_b)	((_b[2] & 0x10) == 0x10)
 #define SS4_IS_5F_DETECTED(_b)	((_b[2] & 0x10) == 0x10)
 
+#define SS4_TS_X_V2(_b)		(s8)(				\
+				 ((_b[0] & 0x01) << 7) |	\
+				 ((_b[1]) & 0x7F)		\
+				)
+
+#define SS4_TS_Y_V2(_b)		-(s8)(				\
+				 ((_b[3] & 0x01) << 7) |	\
+				 ((_b[2]) & 0x7F)		\
+				)
+
+#define SS4_TS_Z_V2(_b)		(s8)(_b[4] & 0x7F)
+
 
 #define SS4_MFPACKET_NO_AX	8160	/* X-Coordinate value */
 #define SS4_MFPACKET_NO_AY	4080	/* Y-Coordinate value */
Signature · 1 line
2.7.4
#34 lore raw
In reply to Paul Donohue
Quoted 21 lines · On Tuesday 08 November 2016 10:14:30 Paul Donohue wrote:

The current Alps SS5 (SS4 v2) code generates bogus TouchPad events when TrackStick packets are processed.

This causes the xorg synaptics driver to print
"unable to find touch point 0" and
"BUG: triggered 'if (priv->num_active_touches > priv->num_slots)'" messages. It also causes unexpected TouchPad button release and reclick event sequences if the TrackStick is moved while holding a TouchPad button.

This commit corrects the problem by adjusting alps_process_packet_ss4_v2() so that it only sends TrackStick reports when processing TrackStick packets.

Signed-off-by Paul Donohue <[email protected]>
drivers/input/mouse/alps.c +0 -0
index 6d7de9b..b93fe83 100644

drivers/input/mouse/alps.c static void alps_process_packet_ss4_v2(struct psmouse *psmouse) · line 1350

Quoted 5 lines
drivers/input/mouse/alps.c +2 -0
@@ -1346,6 +1346,18 @@ static void alps_process_packet_ss4_v2(struct psmouse *psmouse)
 
 	priv->multi_packet = 0;
 
+	/* Report trackstick */
+	if (alps_get_pkt_id_ss4_v2(packet) == SS4_PACKET_ID_STICK) {

I would propose to show warning when trackstick packet is received on device marked as non-trackstick. It would help to debug possible problems in future...

if (!(priv->flags & ALPS_DUALPOINT))

        psmouse_warn(psmouse, "Rejected trackstick packet from non DualPoint device");
Quoted 8 lines
drivers/input/mouse/alps.c +8 -0
+		if (priv->flags & ALPS_DUALPOINT) {
+			input_report_key(dev2, BTN_LEFT, f->ts_left);
+			input_report_key(dev2, BTN_RIGHT, f->ts_right);
+			input_report_key(dev2, BTN_MIDDLE, f->ts_middle);
+			input_sync(dev2);
+		}
+		return;
+	}
Signature · 2 lines
Pali Rohár
[email protected]
author
#35 lore raw
Quoted 14 lines · Pali Rohár

drivers/input/mouse/alps.c static void alps_process_packet_ss4_v2(struct psmouse *psmouse) · line 1350

Quoted 8 lines · On Tuesday 08 November 2016 10:14:30 Paul Donohue wrote:
drivers/input/mouse/alps.c +2 -0
@@ -1346,6 +1346,18 @@ static void alps_process_packet_ss4_v2(struct psmouse *psmouse)
 
 	priv->multi_packet = 0;
 
+	/* Report trackstick */
+	if (alps_get_pkt_id_ss4_v2(packet) == SS4_PACKET_ID_STICK) {

I would propose to show warning when trackstick packet is received on device marked as non-trackstick. It would help to debug possible problems in future...

Quoted 2 lines

if (!(priv->flags & ALPS_DUALPOINT))

        psmouse_warn(psmouse, "Rejected trackstick packet from non DualPoint device");

For patch 1/3, the original alps_decode_ss4_v2() prints this warning, so I don't think alps_process_packet_ss4_v2() needs to print it again. Since this patch is a bug fix, I was trying to avoid any unnecessary changes, and change just enough to fix the actual bug, so it is clear to future readers what the bug was and how it was fixed.

In patch 2/3, I moved the warning from alps_decode_ss4_v2() to alps_process_packet_ss4_v2(), since I agree it probably makes more sense to print the warning in alps_process_packet_ss4_v2(). I also removed the if statement from alps_decode_ss4_v2() entirely because alps_decode_ss4_v2() decodes the trackstick buttons even in cases where the trackstick packet is going to be rejected, and I thought it was a bit confusing that alps_decode_ss4_v2() does not also decode the trackstick movements even in cases where the trackstick packet is going to be rejected (either they should both be decoded, or neither should be decoded, and decoding both rather than skipping both requires less logic).