[PATCH] Alps dualpoint touchpads losing sync [buttons fixed too]

[PATCH] Alps dualpoint touchpads losing sync [buttons fixed too]

[Sebastian Kapfer]

Fix Alps dualpoint touchpads (at the very least Dell E6x00 series)
losing sync when touchpad and trackpoint are used at the same time.
Hardware sends a different packet format than when either is used alone.
This format was not recognised by the existing Alps driver.

This is slightly changed from the first patch, hanging buttons issue has
been ironed out.

Has been tested by a number of people so far with good results.

---
 drivers/input/mouse/alps.c    |  107 +++++++++++++++++++++++++++++++++++++----
 drivers/input/mouse/psmouse.h |    2 +-
 2 files changed, 99 insertions(+), 10 deletions(-)

diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
index f361106..aaab238 100644
--- a/drivers/input/mouse/alps.c
+++ b/drivers/input/mouse/alps.c
@@ -1,14 +1,20 @@
-/*
+/* vim:noet:sw=8:ts=8
  * ALPS touchpad PS/2 mouse driver
  *
  * Copyright (c) 2003 Neil Brown <neilb@cse.unsw.edu.au>
  * Copyright (c) 2003-2005 Peter Osterlund <petero2@telia.com>
  * Copyright (c) 2004 Dmitry Torokhov <dtor@mail.ru>
  * Copyright (c) 2005 Vojtech Pavlik <vojtech@suse.cz>
+ * Copyright (c) 2009 Sebastian Kapfer <sebastian_kapfer@gmx.net>
  *
  * ALPS detection, tap switching and status querying info is taken from
  * tpconfig utility (by C. Scott Ananian and Bruce Kall).
  *
+ * Inspiration for parts of the multitouch codepath from a patch by
+ * Matthew Chapman.  (See http://lkml.org/lkml/2008/12/8/182 )
+ * Additional research by David Kubicek and Erik Osterholm
+ * (https://bugs.launchpad.net/ubuntu/+source/linux/+bug/296610 )
+ *
  * This program is free software; you can redistribute it and/or modify it
  * under the terms of the GNU General Public License version 2 as published by
  * the Free Software Foundation.
@@ -35,6 +41,15 @@
 #define ALPS_OLDPROTO	0x10
 #define ALPS_PASS	0x20
 #define ALPS_FW_BK_2	0x40
+/* capable of sending 9-byte packets.  these packets are recognized by having
+ * LMR buttons set.  if there were a dualpoint device with three mouse buttons,
+ * we could misrecognize, so an additional flag.  if your dualpoint device
+ * often loses sync, try adding ALPS_DUALPOINT9.
+ * these devices also have the property that they don't keep button state
+ * separate for the touchpad and stick device. */
+#define ALPS_DUALPOINT9	0x80
+
+#define DELL_STYLE_DUALPOINT (ALPS_DUALPOINT|ALPS_DUALPOINT9|ALPS_PASS)
 
 static const struct alps_model_info alps_model_data[] = {
 	{ { 0x32, 0x02, 0x14 },	0xf8, 0xf8, ALPS_PASS | ALPS_DUALPOINT }, /* Toshiba Salellite Pro M10 */
@@ -55,7 +70,8 @@ static const struct alps_model_info alps_model_data[] = {
 	{ { 0x20, 0x02, 0x0e },	0xf8, 0xf8, ALPS_PASS | ALPS_DUALPOINT }, /* XXX */
 	{ { 0x22, 0x02, 0x0a },	0xf8, 0xf8, ALPS_PASS | ALPS_DUALPOINT },
 	{ { 0x22, 0x02, 0x14 }, 0xff, 0xff, ALPS_PASS | ALPS_DUALPOINT }, /* Dell Latitude D600 */
-	{ { 0x62, 0x02, 0x14 }, 0xcf, 0xcf, ALPS_PASS | ALPS_DUALPOINT }, /* Dell Latitude E6500 */
+	/* Dell Latitude E6400, E6500, Precision M4400 */
+	{ { 0x62, 0x02, 0x14 }, 0xcf, 0xcf, DELL_STYLE_DUALPOINT },
 	{ { 0x73, 0x02, 0x50 }, 0xcf, 0xcf, ALPS_FW_BK_1 },		  /* Dell Vostro 1400 */
 };
 
@@ -65,8 +81,13 @@ static const struct alps_model_info alps_model_data[] = {
  * isn't valid per PS/2 spec.
  */
 
-/*
- * ALPS abolute Mode - new format
+/* PS/2 packet format
+ *
+ * byte 0: YOFL XOFL YSGN XSGN  1    M    R    L
+ * byte 1: X7   X6   X5   X4   X3   X2   X1   X0
+ * byte 2: Y7   Y6   Y5   Y4   Y3   Y2   Y1   Y0
+ *
+ * ALPS absolute Mode - new format
  *
  * byte 0:  1    ?    ?    ?    1    ?    ?    ?
  * byte 1:  0   x6   x5   x4   x3   x2   x1   x0
@@ -75,6 +96,20 @@ static const struct alps_model_info alps_model_data[] = {
  * byte 4:  0   y6   y5   y4   y3   y2   y1   y0
  * byte 5:  0   z6   z5   z4   z3   z2   z1   z0
  *
+ * Dualpoint device -- 9-byte packet format
+ *
+ * byte 0:    1    1    0    0    1    1    1    1
+ * byte 1:    0   x6   x5   x4   x3   x2   x1   x0
+ * byte 2:    0  x10   x9   x8   x7    0  fin  ges
+ * byte 3: YOFL XOFL YSGN XSGN    1    1    1    1
+ * byte 4:   X7   X6   X5   X4   X3   X2   X1   X0
+ * byte 5:   Y7   Y6   Y5   Y4   Y3   Y2   Y1   Y0
+ * byte 6:    0   y9   y8   y7    1    m    r    l
+ * byte 7:    0   y6   y5   y4   y3   y2   y1   y0
+ * byte 8:    0   z6   z5   z4   z3   z2   z1   z0
+ *
+ * CAPITALS = stick, miniscules = touchpad
+ *
  * ?'s can have different meanings on different models,
  * such as wheel rotation, extra buttons, stick buttons
  * on a dualpoint, etc.
@@ -98,9 +133,30 @@ static void alps_process_packet(struct psmouse *psmouse)
 		input_report_rel(dev2, REL_Y,
 			packet[2] ? ((packet[0] << 3) & 0x100) - packet[2] : 0);
 		input_sync(dev2);
+		if ((priv->i->flags & ALPS_DUALPOINT9) == 0)
+			return;
+		/* copy state to other input layer device, since
+		 * the hardware does not keep them separate. */
+		input_report_key(dev,  BTN_LEFT,   packet[0] & 1);
+		input_report_key(dev,  BTN_RIGHT,  packet[0] & 2);
+		input_report_key(dev,  BTN_MIDDLE, packet[0] & 4);
+		input_sync(dev);
 		return;
 	}
 
+	/* handle trackpoint part of a 9-byte packet,
+	   pass the rest on. */
+	if (priv->i->flags & ALPS_DUALPOINT9 && (packet[3] & 0xf) == 0xf) {
+		input_report_rel(dev2, REL_X,
+			packet[4] ? packet[4] - ((packet[3] << 4) & 0x100) : 0);
+		input_report_rel(dev2, REL_Y,
+			packet[5] ? ((packet[3] << 3) & 0x100) - packet[5] : 0);
+		/* touchpad data and buttons are handled below */
+		packet[3] = packet[6];
+		packet[4] = packet[7];
+		packet[5] = packet[8];
+	}
+
 	if (priv->i->flags & ALPS_OLDPROTO) {
 		left = packet[2] & 0x10;
 		right = packet[2] & 0x08;
@@ -148,6 +204,14 @@ static void alps_process_packet(struct psmouse *psmouse)
 	input_report_key(dev, BTN_LEFT, left);
 	input_report_key(dev, BTN_RIGHT, right);
 	input_report_key(dev, BTN_MIDDLE, middle);
+	/* copy state to other input layer device, since
+	 * the hardware does not keep them separate. */
+	if (priv->i->flags & ALPS_DUALPOINT9) {
+		input_report_key(dev2, BTN_LEFT,   left);
+		input_report_key(dev2, BTN_RIGHT,  right);
+		input_report_key(dev2, BTN_MIDDLE, middle);
+		input_sync(dev2);
+	}
 
 	/* Convert hardware tap to a reasonable Z value */
 	if (ges && !fin) z = 40;
@@ -191,6 +255,7 @@ static void alps_process_packet(struct psmouse *psmouse)
 static psmouse_ret_t alps_process_byte(struct psmouse *psmouse)
 {
 	struct alps_data *priv = psmouse->private;
+	int length_full_packet = 6;
 
 	if ((psmouse->packet[0] & 0xc8) == 0x08) { /* PS/2 packet */
 		if (psmouse->pktcnt == 3) {
@@ -200,15 +265,39 @@ static psmouse_ret_t alps_process_byte(struct psmouse *psmouse)
 		return PSMOUSE_GOOD_DATA;
 	}
 
-	if ((psmouse->packet[0] & priv->i->mask0) != priv->i->byte0)
+	/* must have been a non-PS/2 packet to even get here. */
+
+	if ((psmouse->packet[0] & priv->i->mask0) != priv->i->byte0) {
+		dbg("don't like packet[0] = %x (mask0 = %x, byte0 = %x\n",
+		    (int)psmouse->packet[0],
+		    (int)priv->i->mask0,
+		    (int)priv->i->byte0);
 		return PSMOUSE_BAD_DATA;
+	}
 
-	/* Bytes 2 - 6 should have 0 in the highest bit */
-	if (psmouse->pktcnt >= 2 && psmouse->pktcnt <= 6 &&
-	    (psmouse->packet[psmouse->pktcnt - 1] & 0x80))
+	/* 9-byte packet format by dualpoint units */
+	if (priv->i->flags & ALPS_DUALPOINT9) {
+		/* is marked by a packet with LMR set */
+		if ((psmouse->pktcnt >= 4)
+		    && ((psmouse->packet[3] & 0xf) == 0xf)) {
+			length_full_packet = 9;
+			/* wave stick bytes through */
+			if (psmouse->pktcnt <= 6)
+				return PSMOUSE_GOOD_DATA;
+		}
+	}
+
+	/* Bytes 2 - 6 should have 0 in the highest bit for 6-byte packet */
+	/* Bytes 2, 3, and 7 through 9 should have for 9-byte packet      */
+	if (psmouse->pktcnt >= 2 &&
+	    (psmouse->packet[psmouse->pktcnt - 1] & 0x80)) {
+		dbg("don't like packet[%i] = %x\n",
+		    psmouse->pktcnt - 1,
+		    (int)psmouse->packet[psmouse->pktcnt - 1]);
 		return PSMOUSE_BAD_DATA;
+	}
 
-	if (psmouse->pktcnt == 6) {
+	if (psmouse->pktcnt == length_full_packet) {
 		alps_process_packet(psmouse);
 		return PSMOUSE_FULL_PACKET;
 	}
diff --git a/drivers/input/mouse/psmouse.h b/drivers/input/mouse/psmouse.h
index e053bdd..d4772fe 100644
--- a/drivers/input/mouse/psmouse.h
+++ b/drivers/input/mouse/psmouse.h
@@ -42,7 +42,7 @@ struct psmouse {
 	struct delayed_work resync_work;
 	char *vendor;
 	char *name;
-	unsigned char packet[8];
+	unsigned char packet[9];
 	unsigned char badbyte;
 	unsigned char pktcnt;
 	unsigned char pktsize;
-- 
1.6.3.3

Re: [PATCH] Alps dualpoint touchpads losing sync [buttons fixed too]

[Dmitry Torokhov]

Hi Sebastian,

On Sun, Nov 15, 2009 at 08:42:38PM +0100, Sebastian Kapfer wrote:
> Fix Alps dualpoint touchpads (at the very least Dell E6x00 series)
> losing sync when touchpad and trackpoint are used at the same time.
> Hardware sends a different packet format than when either is used alone.
> This format was not recognised by the existing Alps driver.
> 
> This is slightly changed from the first patch, hanging buttons issue has
> been ironed out.
> 
> Has been tested by a number of people so far with good results.
> 

Thank you very much for your patch. I took the liberty of rearranging it a
bit, could you please trythe patch below and check that I did not screw
it up. It shoudl apply to the 'next' branch of my tree:

	git://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git

I also need a "Signed-off-by:" line from you so I can get it into 2.6.33
queue. Thanks!

-- 
Dmitry

Input: ALPS - add interleaved protocol support (Dell E6x00 series)

From: Sebastian Kapfer <sebastian_kapfer@gmx.net>

Properly handle version of the protocol where standard PS/2 packets
from trackpoint are stuffed into middle (byte 3-6) of the standard
ALPS packets when both the touchpad and trackpoint are used together.

The patch is based on work done by Matthew Chapman and additional
research done by David Kubicek and Erik Osterholm:

	https://bugs.launchpad.net/ubuntu/+source/linux/+bug/296610

Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
---

 drivers/input/mouse/alps.c |   87 ++++++++++++++++++++++++++++++++++----------
 1 files changed, 68 insertions(+), 19 deletions(-)


diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
index a3f492a..9c91b0b 100644
--- a/drivers/input/mouse/alps.c
+++ b/drivers/input/mouse/alps.c
@@ -5,6 +5,7 @@
  * Copyright (c) 2003-2005 Peter Osterlund <petero2@telia.com>
  * Copyright (c) 2004 Dmitry Torokhov <dtor@mail.ru>
  * Copyright (c) 2005 Vojtech Pavlik <vojtech@suse.cz>
+ * Copyright (c) 2009 Sebastian Kapfer <sebastian_kapfer@gmx.net>
  *
  * ALPS detection, tap switching and status querying info is taken from
  * tpconfig utility (by C. Scott Ananian and Bruce Kall).
@@ -28,7 +29,6 @@
 #define dbg(format, arg...) do {} while (0)
 #endif
 
-
 #define ALPS_OLDPROTO		0x01	/* old style input */
 #define ALPS_DUALPOINT		0x02	/* touchpad has trackstick */
 #define ALPS_PASS		0x04	/* device has a pass-through port */
@@ -37,7 +37,8 @@
 #define ALPS_FW_BK_1		0x10	/* front & back buttons present */
 #define ALPS_FW_BK_2		0x20	/* front & back buttons present */
 #define ALPS_FOUR_BUTTONS	0x40	/* 4 direction button present */
-
+#define ALPS_PS2_INTERLEAVED	0x80	/* 3-byte PS/2 packet interleaved with
+					   6-byte ALPS packet */
 
 static const struct alps_model_info alps_model_data[] = {
 	{ { 0x32, 0x02, 0x14 },	0xf8, 0xf8, ALPS_PASS | ALPS_DUALPOINT }, /* Toshiba Salellite Pro M10 */
@@ -58,7 +59,9 @@ static const struct alps_model_info alps_model_data[] = {
 	{ { 0x20, 0x02, 0x0e },	0xf8, 0xf8, ALPS_PASS | ALPS_DUALPOINT }, /* XXX */
 	{ { 0x22, 0x02, 0x0a },	0xf8, 0xf8, ALPS_PASS | ALPS_DUALPOINT },
 	{ { 0x22, 0x02, 0x14 }, 0xff, 0xff, ALPS_PASS | ALPS_DUALPOINT }, /* Dell Latitude D600 */
-	{ { 0x62, 0x02, 0x14 }, 0xcf, 0xcf, ALPS_PASS | ALPS_DUALPOINT }, /* Dell Latitude E6500 */
+	/* Dell Latitude E6400, E6500, Precision M4400 */
+	{ { 0x62, 0x02, 0x14 }, 0xcf, 0xcf,
+		ALPS_PASS | ALPS_DUALPOINT | ALPS_PS2_INTERLEAVED },
 	{ { 0x73, 0x02, 0x50 }, 0xcf, 0xcf, ALPS_FOUR_BUTTONS },	  /* Dell Vostro 1400 */
 };
 
@@ -69,7 +72,13 @@ static const struct alps_model_info alps_model_data[] = {
  */
 
 /*
- * ALPS abolute Mode - new format
+ * PS/2 packet format
+ *
+ * byte 0: YOFL XOFL YSGN XSGN  1    M    R    L
+ * byte 1: X7   X6   X5   X4   X3   X2   X1   X0
+ * byte 2: Y7   Y6   Y5   Y4   Y3   Y2   Y1   Y0
+ *
+ * ALPS absolute Mode - new format
  *
  * byte 0:  1    ?    ?    ?    1    ?    ?    ?
  * byte 1:  0   x6   x5   x4   x3   x2   x1   x0
@@ -78,6 +87,20 @@ static const struct alps_model_info alps_model_data[] = {
  * byte 4:  0   y6   y5   y4   y3   y2   y1   y0
  * byte 5:  0   z6   z5   z4   z3   z2   z1   z0
  *
+ * Dualpoint device -- interleaved packet format
+ *
+ * byte 0:    1    1    0    0    1    1    1    1
+ * byte 1:    0   x6   x5   x4   x3   x2   x1   x0
+ * byte 2:    0  x10   x9   x8   x7    0  fin  ges
+ * byte 3: YOFL XOFL YSGN XSGN    1    1    1    1
+ * byte 4:   X7   X6   X5   X4   X3   X2   X1   X0
+ * byte 5:   Y7   Y6   Y5   Y4   Y3   Y2   Y1   Y0
+ * byte 6:    0   y9   y8   y7    1    m    r    l
+ * byte 7:    0   y6   y5   y4   y3   y2   y1   y0
+ * byte 8:    0   z6   z5   z4   z3   z2   z1   z0
+ *
+ * CAPITALS = stick, miniscules = touchpad
+ *
  * ?'s can have different meanings on different models,
  * such as wheel rotation, extra buttons, stick buttons
  * on a dualpoint, etc.
@@ -93,18 +116,6 @@ static void alps_process_packet(struct psmouse *psmouse)
 	int x, y, z, ges, fin, left, right, middle;
 	int back = 0, forward = 0;
 
-	if ((packet[0] & 0xc8) == 0x08) {   /* 3-byte PS/2 packet */
-		input_report_key(dev2, BTN_LEFT,   packet[0] & 1);
-		input_report_key(dev2, BTN_RIGHT,  packet[0] & 2);
-		input_report_key(dev2, BTN_MIDDLE, packet[0] & 4);
-		input_report_rel(dev2, REL_X,
-			packet[1] ? packet[1] - ((packet[0] << 4) & 0x100) : 0);
-		input_report_rel(dev2, REL_Y,
-			packet[2] ? ((packet[0] << 3) & 0x100) - packet[2] : 0);
-		input_sync(dev2);
-		return;
-	}
-
 	if (model->flags & ALPS_OLDPROTO) {
 		left = packet[2] & 0x10;
 		right = packet[2] & 0x08;
@@ -202,25 +213,63 @@ static void alps_process_packet(struct psmouse *psmouse)
 	input_sync(dev);
 }
 
+static void alps_report_bare_ps2_packet(unsigned char packet[],
+					struct input_dev *dev)
+{
+	input_report_key(dev, BTN_LEFT,   packet[0] & 1);
+	input_report_key(dev, BTN_RIGHT,  packet[0] & 2);
+	input_report_key(dev, BTN_MIDDLE, packet[0] & 4);
+	input_report_rel(dev, REL_X,
+		packet[1] ? packet[1] - ((packet[0] << 4) & 0x100) : 0);
+	input_report_rel(dev, REL_Y,
+		packet[2] ? ((packet[0] << 3) & 0x100) - packet[2] : 0);
+	input_sync(dev);
+}
+
 static psmouse_ret_t alps_process_byte(struct psmouse *psmouse)
 {
 	struct alps_data *priv = psmouse->private;
+	const struct alps_model_info *model = priv->i;
 
 	if ((psmouse->packet[0] & 0xc8) == 0x08) { /* PS/2 packet */
 		if (psmouse->pktcnt == 3) {
-			alps_process_packet(psmouse);
+			alps_report_bare_ps2_packet(psmouse->packet,
+						    priv->dev2);
 			return PSMOUSE_FULL_PACKET;
 		}
 		return PSMOUSE_GOOD_DATA;
 	}
 
-	if ((psmouse->packet[0] & priv->i->mask0) != priv->i->byte0)
+	/* Check for PS/2 packet stuffed in the middle of ALPS packet. */
+
+	if ((model->flags & ALPS_PS2_INTERLEAVED) &&
+	    psmouse->pktcnt >= 4 && (psmouse->packet[3] & 0x0f) == 0x0f) {
+		if (psmouse->pktcnt < 7)
+			return PSMOUSE_GOOD_DATA;
+
+		/* Get the real button data */
+		psmouse->packet[3] &= psmouse->packet[6] & 0x0f;
+		alps_report_bare_ps2_packet(&psmouse->packet[3],
+					    priv->dev2);
+
+		/* Continue with the standard ALPS protocol handling */
+		psmouse->packet[3] = psmouse->packet[6];
+		psmouse->pktcnt = 4;
+	}
+
+	if ((psmouse->packet[0] & model->mask0) != model->byte0) {
+		dbg("refusing packet[0] = %x (mask0 = %x, byte0 = %x)\n",
+		    psmouse->packet[0], model->mask0, model->byte0);
 		return PSMOUSE_BAD_DATA;
+	}
 
 	/* Bytes 2 - 6 should have 0 in the highest bit */
 	if (psmouse->pktcnt >= 2 && psmouse->pktcnt <= 6 &&
-	    (psmouse->packet[psmouse->pktcnt - 1] & 0x80))
+	    (psmouse->packet[psmouse->pktcnt - 1] & 0x80)) {
+		dbg("refusing packet[%i] = %x\n",
+		    psmouse->pktcnt - 1, psmouse->packet[psmouse->pktcnt - 1]);
 		return PSMOUSE_BAD_DATA;
+	}
 
 	if (psmouse->pktcnt == 6) {
 		alps_process_packet(psmouse);

[PATCH] 9-byte Alps, revisited

[Sebastian Kapfer]

Hi Dmitry,

thank you for your reply.  It is much cleaner now!  I had to make a few
changes though:

1. As posted, the rearranged patch doesn't work properly.  One has to
retain the sign bits of the PS/2 subpacket.

2. I've also pulled the checking of byte0 before the demuxing of the
PS/2 subpacket.  I think it's safer to desync early if the data is bad.

3. The hardware is very broken:  Touchpad and trackpoint share button
state.  This means that you can trigger this sequence of events:

	<user presses button on trackpoint>
	3byte:  left down  --> reported to "dev2"
	<user moves pointer with touchpad>
	6byte:  left down  --> reported to "dev"
	<user releases button on trackpoint>
	3byte:  left up    --> reported to "dev2"

The result is that dev has a stuck mouse button, and in X11 the mouse
button stays down.  That's why I explicitly cloned button events to both
devices in my first patch. 

However, this created a different problem:  If the user hammered at the
mouse button very quickly, the events would be processed out of order,
e.g.

	touch_press touch_release stick_press stick_release

instead of

	touch_press stick_press touch_release stick_release

As a result of this, a double click was detected in X11.

I have added logic that assigns each button down event to only one of
the devices, and also avoids hanging buttons.  This is activated by a
new flag.

(I'm pretty sure the shared button state is true for most if not all
Alps dualpoints, but I made a separate flag out of it for now).

4. I've noticed the applied patch for the 4-button Alps device. Is it
really intended that one of the buttons fires both a BTN_x event and a
BTN_MIDDLE event?  I don't think so, even tough they share the same bit
in the packet.  BTN_MIDDLE should never be emitted from a device with
the ALPS_FOUR_BUTTONS flag IMHO.  I haven't changed this, never having
seen such a unit.

5. There remains a slight conceptual problem.  The distinction between
6-byte and 9-byte packets is not possible only looking at the first 6
bytes.  (This is a protocoll issue.  We have scrutinized every bit now,
the protocol just seems to be broken in this regard.)

This means that if the user triggers a 6-byte message while holding all
three buttons down (e.g. hold buttons and move pointer via touchpad), we
run into de-sync.

We can't solve this without knowing the message size in the driver.  I
have no idea if we can somehow get this info out of the PS/2 layer.
Dmitry, do you have any insight into this?

I still vote strongly for applying the patch, since this is a mostly
cosmetic problem that never occurs in practical work whereas in the
current state my touchpad is unusable.

Best wishes,
  Sebastian.


Patch following:


Implement protocol for certain Alps dualpoint touchpads sending 9-byte packets,
common in Dell laptops, e.g. E6x00.

Contains pieces of work from Sebastian Kapfer, David Kubicek, Erik Osterholm,
and Dmitry Torokhov.

Signed-off-by: Sebastian Kapfer <sebastian_kapfer@gmx.net>

---
 drivers/input/mouse/alps.c |  140 +++++++++++++++++++++++++++++++++++--------
 drivers/input/mouse/alps.h |    3 +-
 2 files changed, 116 insertions(+), 27 deletions(-)

diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
index a3f492a..aa2498e 100644
--- a/drivers/input/mouse/alps.c
+++ b/drivers/input/mouse/alps.c
@@ -5,10 +5,14 @@
  * Copyright (c) 2003-2005 Peter Osterlund <petero2@telia.com>
  * Copyright (c) 2004 Dmitry Torokhov <dtor@mail.ru>
  * Copyright (c) 2005 Vojtech Pavlik <vojtech@suse.cz>
+ * Copyright (c) 2009 Sebastian Kapfer <sebastian_kapfer@gmx.net>
  *
  * ALPS detection, tap switching and status querying info is taken from
  * tpconfig utility (by C. Scott Ananian and Bruce Kall).
  *
+ * Additional research by David Kubicek and Erik Osterholm
+ * (https://bugs.launchpad.net/ubuntu/+source/linux/+bug/296610 )
+ *
  * This program is free software; you can redistribute it and/or modify it
  * under the terms of the GNU General Public License version 2 as published by
  * the Free Software Foundation.
@@ -28,7 +32,6 @@
 #define dbg(format, arg...) do {} while (0)
 #endif
 
-
 #define ALPS_OLDPROTO		0x01	/* old style input */
 #define ALPS_DUALPOINT		0x02	/* touchpad has trackstick */
 #define ALPS_PASS		0x04	/* device has a pass-through port */
@@ -37,7 +40,9 @@
 #define ALPS_FW_BK_1		0x10	/* front & back buttons present */
 #define ALPS_FW_BK_2		0x20	/* front & back buttons present */
 #define ALPS_FOUR_BUTTONS	0x40	/* 4 direction button present */
-
+#define ALPS_PS2_INTERLEAVED	0x80	/* 3-byte PS/2 packet interleaved with
+					   6-byte ALPS packet */
+#define ALPS_SHARED_BTNSTATE	0x100	/* PS/2 and touchpad share button st. */
 
 static const struct alps_model_info alps_model_data[] = {
 	{ { 0x32, 0x02, 0x14 },	0xf8, 0xf8, ALPS_PASS | ALPS_DUALPOINT }, /* Toshiba Salellite Pro M10 */
@@ -58,7 +63,9 @@ static const struct alps_model_info alps_model_data[] = {
 	{ { 0x20, 0x02, 0x0e },	0xf8, 0xf8, ALPS_PASS | ALPS_DUALPOINT }, /* XXX */
 	{ { 0x22, 0x02, 0x0a },	0xf8, 0xf8, ALPS_PASS | ALPS_DUALPOINT },
 	{ { 0x22, 0x02, 0x14 }, 0xff, 0xff, ALPS_PASS | ALPS_DUALPOINT }, /* Dell Latitude D600 */
-	{ { 0x62, 0x02, 0x14 }, 0xcf, 0xcf, ALPS_PASS | ALPS_DUALPOINT }, /* Dell Latitude E6500 */
+	/* Dell Latitude E6400, E6500, Precision M4400 */
+	{ { 0x62, 0x02, 0x14 }, 0xcf, 0xcf,
+		ALPS_PASS | ALPS_DUALPOINT | ALPS_PS2_INTERLEAVED | ALPS_SHARED_BTNSTATE },
 	{ { 0x73, 0x02, 0x50 }, 0xcf, 0xcf, ALPS_FOUR_BUTTONS },	  /* Dell Vostro 1400 */
 };
 
@@ -69,7 +76,13 @@ static const struct alps_model_info alps_model_data[] = {
  */
 
 /*
- * ALPS abolute Mode - new format
+ * PS/2 packet format
+ *
+ * byte 0: YOFL XOFL YSGN XSGN  1    M    R    L
+ * byte 1: X7   X6   X5   X4   X3   X2   X1   X0
+ * byte 2: Y7   Y6   Y5   Y4   Y3   Y2   Y1   Y0
+ *
+ * ALPS absolute Mode - new format
  *
  * byte 0:  1    ?    ?    ?    1    ?    ?    ?
  * byte 1:  0   x6   x5   x4   x3   x2   x1   x0
@@ -78,11 +91,59 @@ static const struct alps_model_info alps_model_data[] = {
  * byte 4:  0   y6   y5   y4   y3   y2   y1   y0
  * byte 5:  0   z6   z5   z4   z3   z2   z1   z0
  *
+ * Dualpoint device -- interleaved packet format
+ *
+ * byte 0:    1    1    0    0    1    1    1    1
+ * byte 1:    0   x6   x5   x4   x3   x2   x1   x0
+ * byte 2:    0  x10   x9   x8   x7    0  fin  ges
+ * byte 3: YOFL XOFL YSGN XSGN    1    1    1    1
+ * byte 4:   X7   X6   X5   X4   X3   X2   X1   X0
+ * byte 5:   Y7   Y6   Y5   Y4   Y3   Y2   Y1   Y0
+ * byte 6:    0   y9   y8   y7    1    m    r    l
+ * byte 7:    0   y6   y5   y4   y3   y2   y1   y0
+ * byte 8:    0   z6   z5   z4   z3   z2   z1   z0
+ *
+ * CAPITALS = stick, miniscules = touchpad
+ *
  * ?'s can have different meanings on different models,
  * such as wheel rotation, extra buttons, stick buttons
  * on a dualpoint, etc.
  */
 
+
+/* for Alps units where PS/2 and touchpad share a common button state, try our
+ * best to disentangle them.  If we don't do this, we either get hanging
+ * buttons (when the button-up occurs on the 'wrong' of the two input_dev's) or
+ * duplicated events (if we just broadcast button state to both input_dev's).
+ * the latter can arrive in X11 as doubleclicks if the user clicked quickly.
+ * This also has the advantage that users can assign different actions to
+ * the buttons.
+ */
+static void alps_report_button(struct psmouse *psmouse,
+                               int whichbtn, int state, int mask,
+                               struct input_dev *preferred_dev)
+{
+	struct alps_data *priv = psmouse->private;
+	if (priv->i->flags & ALPS_SHARED_BTNSTATE) {
+		if (state) {
+			if (priv->btn_state & mask)
+				/* already communicated, drop */
+				return;
+			priv->btn_state |= mask;
+		} else {
+			/* release the button on the other device */
+			struct input_dev *other = psmouse->dev;
+			if (preferred_dev == other)
+				other = priv->dev2;
+			priv->btn_state &= 7 ^ mask;
+			input_report_key(other, whichbtn, 0);
+			input_sync(other);
+		}
+	}
+
+	input_report_key(preferred_dev, whichbtn, state);
+}
+
 static void alps_process_packet(struct psmouse *psmouse)
 {
 	struct alps_data *priv = psmouse->private;
@@ -93,18 +154,6 @@ static void alps_process_packet(struct psmouse *psmouse)
 	int x, y, z, ges, fin, left, right, middle;
 	int back = 0, forward = 0;
 
-	if ((packet[0] & 0xc8) == 0x08) {   /* 3-byte PS/2 packet */
-		input_report_key(dev2, BTN_LEFT,   packet[0] & 1);
-		input_report_key(dev2, BTN_RIGHT,  packet[0] & 2);
-		input_report_key(dev2, BTN_MIDDLE, packet[0] & 4);
-		input_report_rel(dev2, REL_X,
-			packet[1] ? packet[1] - ((packet[0] << 4) & 0x100) : 0);
-		input_report_rel(dev2, REL_Y,
-			packet[2] ? ((packet[0] << 3) & 0x100) - packet[2] : 0);
-		input_sync(dev2);
-		return;
-	}
-
 	if (model->flags & ALPS_OLDPROTO) {
 		left = packet[2] & 0x10;
 		right = packet[2] & 0x08;
@@ -140,18 +189,17 @@ static void alps_process_packet(struct psmouse *psmouse)
 		input_report_rel(dev2, REL_X,  (x > 383 ? (x - 768) : x));
 		input_report_rel(dev2, REL_Y, -(y > 255 ? (y - 512) : y));
 
-		input_report_key(dev2, BTN_LEFT, left);
-		input_report_key(dev2, BTN_RIGHT, right);
-		input_report_key(dev2, BTN_MIDDLE, middle);
+		alps_report_button(psmouse, BTN_LEFT, left, 1, dev2);
+		alps_report_button(psmouse, BTN_RIGHT, right, 2, dev2);
+		alps_report_button(psmouse, BTN_MIDDLE, middle, 4, dev2);
 
-		input_sync(dev);
 		input_sync(dev2);
 		return;
 	}
 
-	input_report_key(dev, BTN_LEFT, left);
-	input_report_key(dev, BTN_RIGHT, right);
-	input_report_key(dev, BTN_MIDDLE, middle);
+	alps_report_button(psmouse, BTN_LEFT, left, 1, dev);
+	alps_report_button(psmouse, BTN_RIGHT, right, 2, dev);
+	alps_report_button(psmouse, BTN_MIDDLE, middle, 4, dev);
 
 	/* Convert hardware tap to a reasonable Z value */
 	if (ges && !fin)
@@ -202,25 +250,65 @@ static void alps_process_packet(struct psmouse *psmouse)
 	input_sync(dev);
 }
 
+static void alps_report_bare_ps2_packet(unsigned char packet[],
+					struct psmouse *psmouse)
+{
+	struct alps_data *priv = psmouse->private;
+	struct input_dev *dev2 = priv->dev2;
+
+	alps_report_button(psmouse, BTN_LEFT, packet[0] & 1, 1, dev2);
+	alps_report_button(psmouse, BTN_RIGHT, packet[0] & 2, 2, dev2);
+	alps_report_button(psmouse, BTN_MIDDLE, packet[0] & 4, 4, dev2);
+	input_report_rel(dev2, REL_X,
+		packet[1] ? packet[1] - ((packet[0] << 4) & 0x100) : 0);
+	input_report_rel(dev2, REL_Y,
+		packet[2] ? ((packet[0] << 3) & 0x100) - packet[2] : 0);
+	input_sync(dev2);
+}
+
 static psmouse_ret_t alps_process_byte(struct psmouse *psmouse)
 {
 	struct alps_data *priv = psmouse->private;
+	const struct alps_model_info *model = priv->i;
 
 	if ((psmouse->packet[0] & 0xc8) == 0x08) { /* PS/2 packet */
 		if (psmouse->pktcnt == 3) {
-			alps_process_packet(psmouse);
+			alps_report_bare_ps2_packet(psmouse->packet,
+						    psmouse);
 			return PSMOUSE_FULL_PACKET;
 		}
 		return PSMOUSE_GOOD_DATA;
 	}
 
-	if ((psmouse->packet[0] & priv->i->mask0) != priv->i->byte0)
+	if ((psmouse->packet[0] & model->mask0) != model->byte0) {
+		dbg("refusing packet[0] = %x (mask0 = %x, byte0 = %x)\n",
+		    psmouse->packet[0], model->mask0, model->byte0);
 		return PSMOUSE_BAD_DATA;
+	}
+
+	/* Check for PS/2 packet stuffed in the middle of ALPS packet. */
+	if ((model->flags & ALPS_PS2_INTERLEAVED) &&
+	    psmouse->pktcnt >= 4 && (psmouse->packet[3] & 0x0f) == 0x0f) {
+		if (psmouse->pktcnt < 7)
+			return PSMOUSE_GOOD_DATA;
+
+		/* Get the real button data */
+		psmouse->packet[3] &= 0xf0 | (psmouse->packet[6] & 0x0f);
+		alps_report_bare_ps2_packet(&psmouse->packet[3],
+					    psmouse);
+
+		/* Continue with the standard ALPS protocol handling */
+		psmouse->packet[3] = psmouse->packet[6];
+		psmouse->pktcnt = 4;
+	}
 
 	/* Bytes 2 - 6 should have 0 in the highest bit */
 	if (psmouse->pktcnt >= 2 && psmouse->pktcnt <= 6 &&
-	    (psmouse->packet[psmouse->pktcnt - 1] & 0x80))
+	    (psmouse->packet[psmouse->pktcnt - 1] & 0x80)) {
+		dbg("refusing packet[%i] = %x\n",
+		    psmouse->pktcnt - 1, psmouse->packet[psmouse->pktcnt - 1]);
 		return PSMOUSE_BAD_DATA;
+	}
 
 	if (psmouse->pktcnt == 6) {
 		alps_process_packet(psmouse);
diff --git a/drivers/input/mouse/alps.h b/drivers/input/mouse/alps.h
index bc87936..af0f370 100644
--- a/drivers/input/mouse/alps.h
+++ b/drivers/input/mouse/alps.h
@@ -15,7 +15,7 @@
 struct alps_model_info {
         unsigned char signature[3];
         unsigned char byte0, mask0;
-        unsigned char flags;
+        unsigned int flags;
 };
 
 struct alps_data {
@@ -23,6 +23,7 @@ struct alps_data {
 	char phys[32];			/* Phys */
 	const struct alps_model_info *i;/* Info */
 	int prev_fin;			/* Finger bit from previous packet */
+	int btn_state;			/* Shared state of the buttons */
 };
 
 #ifdef CONFIG_MOUSE_PS2_ALPS
-- 
1.6.3.3

Re: [PATCH] 9-byte Alps, revisited

[Dmitry Torokhov]

Hi Sebastian,

On Fri, Nov 20, 2009 at 01:07:49AM +0100, Sebastian Kapfer wrote:
> 
> Hi Dmitry,
> 
> thank you for your reply.  It is much cleaner now!  I had to make a few
> changes though:
> 
> 1. As posted, the rearranged patch doesn't work properly.

Kinda expected that ;) That's why I asked to try it out.

>  One has to
> retain the sign bits of the PS/2 subpacket.
> 

Yes, indeed.

> 2. I've also pulled the checking of byte0 before the demuxing of the
> PS/2 subpacket.  I think it's safer to desync early if the data is bad.

It does not matter - byte0 does not change when you are receiving bytes 1
through 5 - if you already checked then it is still good.

> 
> 3. The hardware is very broken:  Touchpad and trackpoint share button
> state.  This means that you can trigger this sequence of events:
> 
> 	<user presses button on trackpoint>
> 	3byte:  left down  --> reported to "dev2"
> 	<user moves pointer with touchpad>
> 	6byte:  left down  --> reported to "dev"
> 	<user releases button on trackpoint>
> 	3byte:  left up    --> reported to "dev2"
> 
> The result is that dev has a stuck mouse button, and in X11 the mouse
> button stays down.  That's why I explicitly cloned button events to both
> devices in my first patch. 
> 
> However, this created a different problem:  If the user hammered at the
> mouse button very quickly, the events would be processed out of order,
> e.g.
> 
> 	touch_press touch_release stick_press stick_release
> 
> instead of
> 
> 	touch_press stick_press touch_release stick_release
> 
> As a result of this, a double click was detected in X11.
> 
> I have added logic that assigns each button down event to only one of
> the devices, and also avoids hanging buttons.  This is activated by a
> new flag.
> 

How about we just don't report button presses on the device representing
the stick? This is how Synaptics touchpads with pass-through ports work
(all buttons belong to the touchpad) and it seems to be working very well.

> (I'm pretty sure the shared button state is true for most if not all
> Alps dualpoints, but I made a separate flag out of it for now).
> 
> 4. I've noticed the applied patch for the 4-button Alps device. Is it
> really intended that one of the buttons fires both a BTN_x event and a
> BTN_MIDDLE event?  I don't think so, even tough they share the same bit
> in the packet.  BTN_MIDDLE should never be emitted from a device with
> the ALPS_FOUR_BUTTONS flag IMHO.  I haven't changed this, never having
> seen such a unit.

There is another patch that clears BTN_MIDDLE on the ones that have 4
directional button so input core will deliver either one or the other to
the clients.

> 
> 5. There remains a slight conceptual problem.  The distinction between
> 6-byte and 9-byte packets is not possible only looking at the first 6
> bytes.  (This is a protocoll issue.  We have scrutinized every bit now,
> the protocol just seems to be broken in this regard.)
> 
> This means that if the user triggers a 6-byte message while holding all
> three buttons down (e.g. hold buttons and move pointer via touchpad), we
> run into de-sync.
> 
> We can't solve this without knowing the message size in the driver.  I
> have no idea if we can somehow get this info out of the PS/2 layer.
> Dmitry, do you have any insight into this?

I had another version of the patch that looked at the 7th byte before
deciding if it was standard or interleaved packet instead of using
ALPS_PS2_INTERLEAVD flag but I decided it was too complex. If the device
in E6x00 indeed has 3 buttons then I can ressurect it.

> 
> I still vote strongly for applying the patch, since this is a mostly
> cosmetic problem that never occurs in practical work whereas in the
> current state my touchpad is unusable.
>

Hmm, it is too late for .32 but maybe we can respin it for stable oncde
we hammer out the functionality.

-- 
Dmitry

Re: [PATCH] 9-byte Alps, revisited

[Sebastian Kapfer]

Hi Dmitry!

> > thank you for your reply.  It is much cleaner now!  I had to make a few
> > changes though:
> > 
> > 1. As posted, the rearranged patch doesn't work properly.
> 
> Kinda expected that ;) That's why I asked to try it out.

Well, how should you test it :-)

> > 3. The hardware is very broken:  Touchpad and trackpoint share button
> > state.  This means that you can trigger this sequence of events:
> > 
> > 	<user presses button on trackpoint>
> > 	3byte:  left down  --> reported to "dev2"
> > 	<user moves pointer with touchpad>
> > 	6byte:  left down  --> reported to "dev"
> > 	<user releases button on trackpoint>
> > 	3byte:  left up    --> reported to "dev2"
> > 
> > The result is that dev has a stuck mouse button, and in X11 the mouse
> > button stays down.  That's why I explicitly cloned button events to both
> > devices in my first patch. 
> > 
> > However, this created a different problem:  If the user hammered at the
> > mouse button very quickly, the events would be processed out of order,
> > e.g.
> > 
> > 	touch_press touch_release stick_press stick_release
> > 
> > instead of
> > 
> > 	touch_press stick_press touch_release stick_release
> > 
> > As a result of this, a double click was detected in X11.
> > 
> > I have added logic that assigns each button down event to only one of
> > the devices, and also avoids hanging buttons.  This is activated by a
> > new flag.
> > 
> 
> How about we just don't report button presses on the device representing
> the stick? This is how Synaptics touchpads with pass-through ports work
> (all buttons belong to the touchpad) and it seems to be working very well.

We could do that -- however, one of my testers said he liked it that on
his Synaptics, he could assign some different function to the Trackpoint
middle button than to the touchpad middle button (no idea, really).

Obviously, it's not 100% possible to tell the two apart, but for all
practical purposes, it should be OK.  The button is always reported on
the device where the button is first pressed.

> There is another patch that clears BTN_MIDDLE on the ones that have 4
> directional button so input core will deliver either one or the other to
> the clients.

Ah ok.  So these things have a four-direction button instead of the
middle mouse button then.

> > 5. There remains a slight conceptual problem.  The distinction between
> > 6-byte and 9-byte packets is not possible only looking at the first 6
> > bytes.  (This is a protocoll issue.  We have scrutinized every bit now,
> > the protocol just seems to be broken in this regard.)
> > 
> > This means that if the user triggers a 6-byte message while holding all
> > three buttons down (e.g. hold buttons and move pointer via touchpad), we
> > run into de-sync.
> > 
> > We can't solve this without knowing the message size in the driver.  I
> > have no idea if we can somehow get this info out of the PS/2 layer.
> > Dmitry, do you have any insight into this?
> 
> I had another version of the patch that looked at the 7th byte before
> deciding if it was standard or interleaved packet instead of using
> ALPS_PS2_INTERLEAVD flag but I decided it was too complex. If the device
> in E6x00 indeed has 3 buttons then I can ressurect it.

Even with the seventh byte, we can't be totally sure.  The protocol is
just nuts.  And waiting for the seventh byte causes the code in
psmouse-base.c to force desync.  So this is not really an option, too.

I have no clue how PS/2 works internally and if there is some sort of
message separator mechanism.  I would hope that Alps' original Windows
driver does not resort to timing for telling packets apart...

> > 
> > I still vote strongly for applying the patch, since this is a mostly
> > cosmetic problem that never occurs in practical work whereas in the
> > current state my touchpad is unusable.
> >
> 
> Hmm, it is too late for .32 but maybe we can respin it for stable oncde
> we hammer out the functionality.

I'd volunteer for any changes required, although I'm not really sure
what they are.  The patch should almost completely apply to the stable
kernel, too?

-- 
Best Regards,  | Hi! I'm a .signature virus. Copy me into
 Sebastian     | your ~/.signature to help me spread!