diff mbox

Input: ALPS - Fix TrackStick support for SS5 hardware

Message ID 20161024210122.GA2919@TopQuark.net (mailing list archive)
State Superseded
Headers show

Commit Message

Paul Donohue Oct. 24, 2016, 9:01 p.m. UTC 
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>


--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Pali Rohár Nov. 1, 2016, 9:35 a.m. UTC | #1 
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?
Paul Donohue Nov. 5, 2016, 4:30 p.m. UTC | #2 
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pali Rohár Nov. 6, 2016, 7:57 p.m. UTC | #3 
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...
diff mbox

Patch

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;