[PATCH] Input: ALPS - Fix TrackStick support for SS5 hardware

[PATCH] Input: ALPS - Fix TrackStick support for SS5 hardware

[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 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 <linux-kernel@PaulSD.com>

diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
index 6d7de9b..2c2e55b 100644
--- a/drivers/input/mouse/alps.c
+++ b/drivers/input/mouse/alps.c
@@ -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);
diff --git a/drivers/input/mouse/alps.h b/drivers/input/mouse/alps.h
index b9417e2..1dc09b5 100644
--- a/drivers/input/mouse/alps.h
+++ b/drivers/input/mouse/alps.h
@@ -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;

Re: [PATCH] Input: ALPS - Fix TrackStick support for SS5 hardware

[Pali Rohár]

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 <linux-kernel@PaulSD.com>
> 
> diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
> index 6d7de9b..2c2e55b 100644
> --- a/drivers/input/mouse/alps.c
> +++ b/drivers/input/mouse/alps.c
> @@ -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);
> diff --git a/drivers/input/mouse/alps.h b/drivers/input/mouse/alps.h
> index b9417e2..1dc09b5 100644
> --- a/drivers/input/mouse/alps.h
> +++ b/drivers/input/mouse/alps.h
> @@ -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;
> 

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?

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

Re: [PATCH] Input: ALPS - Fix TrackStick support for SS5 hardware

[Paul Donohue]

On Tue, Nov 01, 2016 at 10:35:07AM +0100, Pali Rohár wrote:
> 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 <linux-kernel@PaulSD.com>
> > 
> > diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
> > index 6d7de9b..2c2e55b 100644
> > --- a/drivers/input/mouse/alps.c
> > +++ b/drivers/input/mouse/alps.c
> > @@ -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);
> > diff --git a/drivers/input/mouse/alps.h b/drivers/input/mouse/alps.h
> > index b9417e2..1dc09b5 100644
> > --- a/drivers/input/mouse/alps.h
> > +++ b/drivers/input/mouse/alps.h
> > @@ -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;
> > 
> 
> 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.

[PATCH v2] Input: ALPS - Fix TrackStick support for SS5 hardware

[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 <linux-kernel@PaulSD.com>

diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
index 6d7de9b..1d0302b 100644
--- a/drivers/input/mouse/alps.c
+++ b/drivers/input/mouse/alps.c
@@ -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);

Re: [PATCH v2] Input: ALPS - Fix TrackStick support for SS5 hardware

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 3153 bytes --]

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 <linux-kernel@PaulSD.com>
> 
> diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
> index 6d7de9b..1d0302b 100644
> --- a/drivers/input/mouse/alps.c
> +++ b/drivers/input/mouse/alps.c
> @@ -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) {

Ideally adds comment that this is test for SS4_PACKET_ID_STICK.

> +               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);
> 

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

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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH v2] Input: ALPS - Fix TrackStick support for SS5 hardware

[Paul Donohue]

On Sun, Nov 06, 2016 at 08:59:34PM +0100, Pali Rohár wrote:
> 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

Re: [PATCH] Input: ALPS - Fix TrackStick support for SS5 hardware

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 432 bytes --]

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

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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

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

[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 <linux-kernel@PaulSD.com>

diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
index 6d7de9b..b93fe83 100644
--- a/drivers/input/mouse/alps.c
+++ b/drivers/input/mouse/alps.c
@@ -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)

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

[Paul Donohue]

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 <linux-kernel@PaulSD.com>

diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
index b93fe83..12376d2 100644
--- a/drivers/input/mouse/alps.c
+++ b/drivers/input/mouse/alps.c
@@ -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;
        }
 

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

[Paul Donohue]

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

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

[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 <linux-kernel@PaulSD.com>

diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
index 6d7de9b..b93fe83 100644
--- a/drivers/input/mouse/alps.c
+++ b/drivers/input/mouse/alps.c
@@ -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)

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

[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 <linux-kernel@PaulSD.com>

diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
index b93fe83..12376d2 100644
--- a/drivers/input/mouse/alps.c
+++ b/drivers/input/mouse/alps.c
@@ -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;
 	}
 

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

[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 <linux-kernel@PaulSD.com>

diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
index 12376d2..6a2fb69 100644
--- a/drivers/input/mouse/alps.c
+++ b/drivers/input/mouse/alps.c
@@ -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);
 }
 
diff --git a/drivers/input/mouse/alps.h b/drivers/input/mouse/alps.h
index b9417e2..e221dab 100644
--- a/drivers/input/mouse/alps.h
+++ b/drivers/input/mouse/alps.h
@@ -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 */

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

[Paul Donohue]

On Tue, Nov 08, 2016 at 10:17:10AM -0500, Paul Donohue wrote:
> +#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.

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

[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 <linux-kernel@PaulSD.com>

diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
index 12376d2..6a2fb69 100644
--- a/drivers/input/mouse/alps.c
+++ b/drivers/input/mouse/alps.c
@@ -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);
 }
 
diff --git a/drivers/input/mouse/alps.h b/drivers/input/mouse/alps.h
index b9417e2..96e1d1d 100644
--- a/drivers/input/mouse/alps.h
+++ b/drivers/input/mouse/alps.h
@@ -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 */

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

[Pali Rohár]

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 <linux-kernel@PaulSD.com>
> 
> diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
> index b93fe83..12376d2 100644
> --- a/drivers/input/mouse/alps.c
> +++ b/drivers/input/mouse/alps.c
> @@ -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);

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.

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

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

[Paul Donohue]

On Wed, Nov 09, 2016 at 01:14:43PM +0100, Pali Rohár wrote:
> On Tuesday 08 November 2016 10:16:21 Paul Donohue wrote:
> > --- a/drivers/input/mouse/alps.c
> > +++ b/drivers/input/mouse/alps.c
> > @@ -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.
> 
> 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.

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

[Pali Rohár]

On Thursday 10 November 2016 09:27:39 Paul Donohue wrote:
> On Wed, Nov 09, 2016 at 01:14:43PM +0100, Pali Rohár wrote:
> > On Tuesday 08 November 2016 10:16:21 Paul Donohue wrote:
> > > --- a/drivers/input/mouse/alps.c
> > > +++ b/drivers/input/mouse/alps.c
> > > @@ -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.
> > 
> > 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.

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!

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

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

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

[Paul Donohue]

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 <pali.rohar@gmail.com>)

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 <pali.rohar@gmail.com>)

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 <pali.rohar@gmail.com>)

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

-- 
2.7.4

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

[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 <linux-kernel@PaulSD.com>
---
 drivers/input/mouse/alps.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
index 6d7de9b..b93fe83 100644
--- a/drivers/input/mouse/alps.c
+++ b/drivers/input/mouse/alps.c
@@ -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)
-- 
2.7.4

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

[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 <linux-kernel@PaulSD.com>
---
 drivers/input/mouse/alps.c | 42 +++++++++++++++++++++++++-----------------
 1 file changed, 25 insertions(+), 17 deletions(-)

diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
index b93fe83..61d61cc 100644
--- a/drivers/input/mouse/alps.c
+++ b/drivers/input/mouse/alps.c
@@ -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;
 	}
 
-- 
2.7.4

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

[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 <linux-kernel@PaulSD.com>
---
 drivers/input/mouse/alps.c | 24 ++++++++----------------
 drivers/input/mouse/alps.h | 22 +++++++++++++++++++++-
 2 files changed, 29 insertions(+), 17 deletions(-)

diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
index 61d61cc..c984b8c 100644
--- a/drivers/input/mouse/alps.c
+++ b/drivers/input/mouse/alps.c
@@ -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);
 }
 
diff --git a/drivers/input/mouse/alps.h b/drivers/input/mouse/alps.h
index b9417e2..96e1d1d 100644
--- a/drivers/input/mouse/alps.h
+++ b/drivers/input/mouse/alps.h
@@ -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 */
-- 
2.7.4

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

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 1377 bytes --]

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 <pali.rohar@gmail.com>)
> 
> 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 <pali.rohar@gmail.com>)
> 
> 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 <pali.rohar@gmail.com>)
> 
> 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(-)

Whole patch series looks fine.
You can add my Reviewed-by: Pali Rohár <pali.rohar@gmail.com>

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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

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

[Dmitry Torokhov]

On Fri, Nov 11, 2016 at 02:59:09PM +0100, Pali Rohár wrote:
> 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 <pali.rohar@gmail.com>)
> > 
> > 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 <pali.rohar@gmail.com>)
> > 
> > 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 <pali.rohar@gmail.com>)
> > 
> > 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(-)
> 
> Whole patch series looks fine.
> You can add my Reviewed-by: Pali Rohár <pali.rohar@gmail.com>

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

Thanks.


-- 
Dmitry

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

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 1924 bytes --]

On Saturday 12 November 2016 01:56:21 Dmitry Torokhov wrote:
> On Fri, Nov 11, 2016 at 02:59:09PM +0100, Pali Rohár wrote:
> > 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 <pali.rohar@gmail.com>)
> > > 
> > > 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 <pali.rohar@gmail.com>)
> > > 
> > > 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 <pali.rohar@gmail.com>)
> > > 
> > > 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(-)
> > 
> > Whole patch series looks fine.
> > You can add my Reviewed-by: Pali Rohár <pali.rohar@gmail.com>
> 
> FYI: I do not see a dingle email from Paul, only your replies.

Patches were sent to linux-input@vger.kernel.org ML. Maybe some spam 
filter ate emails? Btw I added you to receivers of my last sent email.

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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

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

[Dmitry Torokhov]

On Sat, Nov 12, 2016 at 02:03:42AM +0100, Pali Rohár wrote:
> On Saturday 12 November 2016 01:56:21 Dmitry Torokhov wrote:
> > On Fri, Nov 11, 2016 at 02:59:09PM +0100, Pali Rohár wrote:
> > > 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 <pali.rohar@gmail.com>)
> > > > 
> > > > 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 <pali.rohar@gmail.com>)
> > > > 
> > > > 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 <pali.rohar@gmail.com>)
> > > > 
> > > > 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(-)
> > > 
> > > Whole patch series looks fine.
> > > You can add my Reviewed-by: Pali Rohár <pali.rohar@gmail.com>
> > 
> > FYI: I do not see a dingle email from Paul, only your replies.
> 
> Patches were sent to linux-input@vger.kernel.org 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.

-- 
Dmitry

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

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 2425 bytes --]

On Saturday 12 November 2016 02:06:40 Dmitry Torokhov wrote:
> On Sat, Nov 12, 2016 at 02:03:42AM +0100, Pali Rohár wrote:
> > On Saturday 12 November 2016 01:56:21 Dmitry Torokhov wrote:
> > > On Fri, Nov 11, 2016 at 02:59:09PM +0100, Pali Rohár wrote:
> > > > 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 <pali.rohar@gmail.com>)
> > > > > 
> > > > > 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 <pali.rohar@gmail.com>)
> > > > > 
> > > > > 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 <pali.rohar@gmail.com>)
> > > > > 
> > > > > 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(-)
> > > > 
> > > > Whole patch series looks fine.
> > > > You can add my Reviewed-by: Pali Rohár <pali.rohar@gmail.com>
> > > 
> > > FYI: I do not see a dingle email from Paul, only your replies.
> > 
> > Patches were sent to linux-input@vger.kernel.org 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?

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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

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

[Dmitry Torokhov]

On Sat, Nov 12, 2016 at 4:29 AM, Pali Rohár <pali.rohar@gmail.com> wrote:
> On Saturday 12 November 2016 02:06:40 Dmitry Torokhov wrote:
>> On Sat, Nov 12, 2016 at 02:03:42AM +0100, Pali Rohár wrote:
>> > On Saturday 12 November 2016 01:56:21 Dmitry Torokhov wrote:
>> > > On Fri, Nov 11, 2016 at 02:59:09PM +0100, Pali Rohár wrote:
>> > > > 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 <pali.rohar@gmail.com>)
>> > > > >
>> > > > > 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 <pali.rohar@gmail.com>)
>> > > > >
>> > > > > 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 <pali.rohar@gmail.com>)
>> > > > >
>> > > > > 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(-)
>> > > >
>> > > > Whole patch series looks fine.
>> > > > You can add my Reviewed-by: Pali Rohár <pali.rohar@gmail.com>
>> > >
>> > > FYI: I do not see a dingle email from Paul, only your replies.
>> >
>> > Patches were sent to linux-input@vger.kernel.org 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?
>

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.

-- 
Dmitry

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

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 3136 bytes --]

On Sunday 13 November 2016 07:43:35 Dmitry Torokhov wrote:
> On Sat, Nov 12, 2016 at 4:29 AM, Pali Rohár <pali.rohar@gmail.com>
> wrote:
> > On Saturday 12 November 2016 02:06:40 Dmitry Torokhov wrote:
> >> On Sat, Nov 12, 2016 at 02:03:42AM +0100, Pali Rohár wrote:
> >> > On Saturday 12 November 2016 01:56:21 Dmitry Torokhov wrote:
> >> > > On Fri, Nov 11, 2016 at 02:59:09PM +0100, Pali Rohár wrote:
> >> > > > 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
> >> > > > >   <pali.rohar@gmail.com>)
> >> > > > > 
> >> > > > > 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 <pali.rohar@gmail.com>)
> >> > > > > 
> >> > > > > 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 <pali.rohar@gmail.com>)
> >> > > > > 
> >> > > > > 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(-)
> >> > > > 
> >> > > > Whole patch series looks fine.
> >> > > > You can add my Reviewed-by: Pali Rohár
> >> > > > <pali.rohar@gmail.com>
> >> > > 
> >> > > FYI: I do not see a dingle email from Paul, only your replies.
> >> > 
> >> > Patches were sent to linux-input@vger.kernel.org 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?
> 
> Hmm, was the latest v5? I see
> https://patchwork.kernel.org/patch/9421473/

Yes, v5 is the last.

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

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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

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

[Paul Donohue]

On Sun, Nov 13, 2016 at 11:31:21AM +0100, Pali Rohár wrote:
> On Sunday 13 November 2016 07:43:35 Dmitry Torokhov wrote:

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

> > >> > Patches were sent to linux-input@vger.kernel.org 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.

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

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

[Paul Donohue]

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 <pali.rohar@gmail.com>
* 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 <pali.rohar@gmail.com>)

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 <pali.rohar@gmail.com>)

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 <pali.rohar@gmail.com>)

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

-- 
2.7.4

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

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

Reviewed-by: Pali Rohár <pali.rohar@gmail.com>
Signed-off-by: Paul Donohue <linux-kernel@PaulSD.com>
---
 drivers/input/mouse/alps.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
index 6d7de9b..b93fe83 100644
--- a/drivers/input/mouse/alps.c
+++ b/drivers/input/mouse/alps.c
@@ -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)
-- 
2.7.4

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

[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 <pali.rohar@gmail.com>
Signed-off-by: Paul Donohue <linux-kernel@PaulSD.com>
---
 drivers/input/mouse/alps.c | 42 +++++++++++++++++++++++++-----------------
 1 file changed, 25 insertions(+), 17 deletions(-)

diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
index b93fe83..61d61cc 100644
--- a/drivers/input/mouse/alps.c
+++ b/drivers/input/mouse/alps.c
@@ -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;
 	}
 
-- 
2.7.4

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

[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 <pali.rohar@gmail.com>
Signed-off-by: Paul Donohue <linux-kernel@PaulSD.com>
---
 drivers/input/mouse/alps.c | 24 ++++++++----------------
 drivers/input/mouse/alps.h | 22 +++++++++++++++++++++-
 2 files changed, 29 insertions(+), 17 deletions(-)

diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
index 61d61cc..c984b8c 100644
--- a/drivers/input/mouse/alps.c
+++ b/drivers/input/mouse/alps.c
@@ -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);
 }
 
diff --git a/drivers/input/mouse/alps.h b/drivers/input/mouse/alps.h
index b9417e2..96e1d1d 100644
--- a/drivers/input/mouse/alps.h
+++ b/drivers/input/mouse/alps.h
@@ -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 */
-- 
2.7.4

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

[Pali Rohár]

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 <linux-kernel@PaulSD.com>
> 
> diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
> index 6d7de9b..b93fe83 100644
> --- a/drivers/input/mouse/alps.c
> +++ b/drivers/input/mouse/alps.c
> @@ -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");

> +		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;
> +	}

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

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

[Paul Donohue]

On Wed, Nov 09, 2016 at 01:12:31PM +0100, Pali Rohár wrote:
> On Tuesday 08 November 2016 10:14:30 Paul Donohue wrote:
> > --- a/drivers/input/mouse/alps.c
> > +++ b/drivers/input/mouse/alps.c
> > @@ -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");
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).