ALPS v4 Semi-mt Support

ALPS v4 Semi-mt Support

[George Pantalos]

Hello,

I have created a crude patch to add Semi-MT support for v4 ALPS touchpads 
based on the work by Seth Forshee. [1]

This patch works ok for Single-Touch Events however the Left Half side of the 
touchpad is far less accurate than the Right Half for Semi-mt events 
(synclient -m 10 shows f alternating constantly between 1 and 2 fingers with 
two fingers touching it and one finger touch erroneously reports as 2 finger 
touch sometimes.)

I hope this is useful and maybe it could be improved and approved :)

Sorry for the code quality, I am not a programmer.

[1]  http://people.canonical.com/~sforshee/alps-touchpad/psmouse-
alps-0.10/

ALPS v4 Semi-mt Support

[George Pantalos]

--- drivers/input/mouse/alps.c	2012-03-19 01:15:34.000000000 +0200
+++ drivers/input/mouse/alps.c.new	2012-04-09 17:14:25.193621808 +0300
@@ -604,11 +604,59 @@
 
 static void alps_process_packet_v4(struct psmouse *psmouse)
 {
+        struct alps_data *priv = psmouse->private;
 	unsigned char *packet = psmouse->packet;
 	struct input_dev *dev = psmouse->dev;
 	int x, y, z;
 	int left, right;
+	int x1 = 0, y1 = 0, x2 = 0, y2 = 0;
+	int fingers = 0, bmap_fingers;
+	unsigned int x_bitmap, y_bitmap;
+
+	if (priv->multi_packet) {
+          if (packet[6] != 0x00) { 
+		priv->multi_data_v4[2] = packet[6];
+		priv->multi_data_v4[3] = packet[7];
+		priv->multi_packet = 1;
+		if (priv->last_fingers > 1)
+		return;
+          } else {
+		priv->multi_data_v4[4] = packet[6];
+		priv->multi_data_v4[5] = packet[7];
+
+		fingers = 0;
+		priv->multi_packet = 0;
+		x_bitmap = ((priv->multi_data_v4[2] & 0x1f) << 10) | /*0x1f*/
+			   ((priv->multi_data_v4[3] & 0x60) << 3) |
+			   ((priv->multi_data_v4[0] & 0x3f) << 2) |
+			   ((priv->multi_data_v4[1] & 0x60) >> 5);
+		y_bitmap = ((priv->multi_data_v4[5] & 0x01) << 10) |
+			   ((priv->multi_data_v4[3] & 0x1f) << 5) |
+			    (priv->multi_data_v4[1] & 0x1f);
+	  
+		bmap_fingers = alps_process_bitmap(x_bitmap, y_bitmap,
+						  &x1, &y1, &x2, &y2);
+		if (bmap_fingers > 1) {
+			fingers = bmap_fingers;
+		}
+	  
+		priv->last_fingers = fingers;
 
+          /*packet = priv->multi_data;*/
+          /*packet = packet;*/
+	  }
+	}
+
+	if (!priv->multi_packet && (packet[6] & 0x40)) {
+		priv->multi_packet = 1;
+		priv->multi_data_v4[0] = packet[6];
+		priv->multi_data_v4[1] = packet[7];
+		if (priv->last_fingers > 1)
+		return;
+	}
+	
+        /*priv->multi_packet = 0;*/
+        
 	left = packet[4] & 0x01;
 	right = packet[4] & 0x02;
 
@@ -617,21 +665,33 @@
 	y = ((packet[2] & 0x7f) << 4) | (packet[3] & 0x0f);
 	z = packet[5] & 0x7f;
 
+	if (!fingers) {
+		x1 = x;
+		y1 = y;
+		fingers = z > 0 ? 1 : 0;
+	}
+
 	if (z >= 64)
 		input_report_key(dev, BTN_TOUCH, 1);
 	else
 		input_report_key(dev, BTN_TOUCH, 0);
 
+	alps_report_semi_mt_data(dev, fingers, x1, y1, x2, y2);
+
+	input_report_key(dev, BTN_TOOL_FINGER, fingers == 1);
+	input_report_key(dev, BTN_TOOL_DOUBLETAP, fingers == 2);
+	input_report_key(dev, BTN_TOOL_TRIPLETAP, fingers == 3);
+	input_report_key(dev, BTN_TOOL_QUADTAP, fingers == 4);
+
+	input_report_key(dev, BTN_LEFT, left);
+	input_report_key(dev, BTN_RIGHT, right);
+
 	if (z > 0) {
 		input_report_abs(dev, ABS_X, x);
 		input_report_abs(dev, ABS_Y, y);
 	}
 	input_report_abs(dev, ABS_PRESSURE, z);
 
-	input_report_key(dev, BTN_TOOL_FINGER, z > 0);
-	input_report_key(dev, BTN_LEFT, left);
-	input_report_key(dev, BTN_RIGHT, right);
-
 	input_sync(dev);
 }
 
@@ -1567,8 +1627,18 @@
 		set_bit(BTN_TOOL_QUADTAP, dev1->keybit);
 		/* fall through */
 	case ALPS_PROTO_V4:
-		input_set_abs_params(dev1, ABS_X, 0, ALPS_V3_X_MAX, 0, 0);
-		input_set_abs_params(dev1, ABS_Y, 0, ALPS_V3_Y_MAX, 0, 0);
+		set_bit(INPUT_PROP_SEMI_MT, dev1->propbit);
+		input_mt_init_slots(dev1, 2);
+                input_set_abs_params(dev1, ABS_X, 0, ALPS_V3_X_MAX, 0, 0);
+                input_set_abs_params(dev1, ABS_Y, 0, ALPS_V3_Y_MAX, 0, 0);
+
+		input_set_abs_params(dev1, ABS_MT_POSITION_X, 0, 
ALPS_V3_X_MAX, 0, 0);
+		input_set_abs_params(dev1, ABS_MT_POSITION_Y, 0, 
ALPS_V3_Y_MAX, 0, 0);
+
+		set_bit(BTN_TOOL_DOUBLETAP, dev1->keybit);
+		set_bit(BTN_TOOL_TRIPLETAP, dev1->keybit);
+		set_bit(BTN_TOOL_QUADTAP, dev1->keybit);
+		/* fall through */
 		break;
 	}
 
--- drivers/input/mouse/alps.h	2012-03-19 01:15:34.000000000 +0200
+++ drivers/input/mouse/alps.h.new	2012-04-09 17:17:32.733618703 +0300
@@ -39,6 +39,8 @@
 	int prev_fin;			/* Finger bit from previous packet */
 	int multi_packet;		/* Multi-packet data in progress */
 	unsigned char multi_data[6];	/* Saved multi-packet data */
+	unsigned char multi_data_v4[6];
+	int last_fingers;
 	u8 quirks;
 	struct timer_list timer;
 };

Re: ALPS v4 Semi-mt Support

[Seth Forshee]

Hi George,

Thanks for the patch. I've got some specific comments inline below, but
here are some general comments.

The reason I've never followed through with the v4 semi-MT support is
that I lost access to the hardware before I could complete it. When I
had the hardware the thing I struggled with was how to handle receiving
only one full set of MT data for every 3 sets of ST data. It looks like
you've opted to only report every third ST data point (if the previous
MT packet showed more than 1 touch, more on that below), when you also
have a complete set of MT data. That's one possible approach, but I'm
not sure that it's the best. Hopefully some of the input gurus on the
list can comment. As I recall the main difficulty with reporting all ST
points was not knowing the finger count until the third packet of the
sequence, so I was leaning towards accumulating the data and processing
it all in one go after the third packet in the sequence arrived.

I definitely think your state processing could be improved. My
suggestion would be to treat multi_packet as a counter. Reset it to 0
when you see the sync bit is set, and increment it for each packet until
you have a full set of MT data. That way you know for sure which section
of the MT data you're working with for any given packet. But be prepared
to handle an incomplete packet sequence as well (I'm pretty sure I saw
some of these when I was working with a v4 touchpad).

On Tue, Apr 10, 2012 at 05:02:41PM +0300, George Pantalos wrote:
> --- drivers/input/mouse/alps.c	2012-03-19 01:15:34.000000000 +0200
> +++ drivers/input/mouse/alps.c.new	2012-04-09 17:14:25.193621808 +0300
> @@ -604,11 +604,59 @@
>  
>  static void alps_process_packet_v4(struct psmouse *psmouse)
>  {
> +        struct alps_data *priv = psmouse->private;

Should be a tab instead of spaces. You've got spaces for indentation at
some other places as well. Try running checkpatch.pl (in the scripts
directory of the kernel source tree) to catch these types of problems.

>  	unsigned char *packet = psmouse->packet;
>  	struct input_dev *dev = psmouse->dev;
>  	int x, y, z;
>  	int left, right;
> +	int x1 = 0, y1 = 0, x2 = 0, y2 = 0;
> +	int fingers = 0, bmap_fingers;
> +	unsigned int x_bitmap, y_bitmap;
> +
> +	if (priv->multi_packet) {
> +          if (packet[6] != 0x00) { 

This condition may not work all the time. Based on the documentaiton I
wrote at least it looks like byte 6 in the second packet of the sequence
could be zero, but I can't remember for sure.

> +		priv->multi_data_v4[2] = packet[6];
> +		priv->multi_data_v4[3] = packet[7];
> +		priv->multi_packet = 1;
> +		if (priv->last_fingers > 1)
> +		return;

The body of if statements needs to be indented.

> +          } else {
> +		priv->multi_data_v4[4] = packet[6];
> +		priv->multi_data_v4[5] = packet[7];
> +
> +		fingers = 0;
> +		priv->multi_packet = 0;
> +		x_bitmap = ((priv->multi_data_v4[2] & 0x1f) << 10) | /*0x1f*/
> +			   ((priv->multi_data_v4[3] & 0x60) << 3) |
> +			   ((priv->multi_data_v4[0] & 0x3f) << 2) |
> +			   ((priv->multi_data_v4[1] & 0x60) >> 5);
> +		y_bitmap = ((priv->multi_data_v4[5] & 0x01) << 10) |
> +			   ((priv->multi_data_v4[3] & 0x1f) << 5) |
> +			    (priv->multi_data_v4[1] & 0x1f);
> +	  
> +		bmap_fingers = alps_process_bitmap(x_bitmap, y_bitmap,
> +						  &x1, &y1, &x2, &y2);
> +		if (bmap_fingers > 1) {
> +			fingers = bmap_fingers;
> +		}

Braces aren't needed when there's only a single line in the body of an
if statement.

> +	  
> +		priv->last_fingers = fingers;
>  
> +          /*packet = priv->multi_data;*/
> +          /*packet = packet;*/

Delete the commented out lines if they aren't needed.

> +	  }
> +	}
> +
> +	if (!priv->multi_packet && (packet[6] & 0x40)) {
> +		priv->multi_packet = 1;
> +		priv->multi_data_v4[0] = packet[6];
> +		priv->multi_data_v4[1] = packet[7];
> +		if (priv->last_fingers > 1)
> +		return;

So you're deciding how to handle the current packet sequence based on
the number of touch points in the previous sequence. You lose a couple
of ST points then each time you transition from multiple touches to a
single touch. That might be okay, depending on what kind of feedback is
received on how to handle the packet sequences.

> +	}
> +	
> +        /*priv->multi_packet = 0;*/

Again, if this isn't needed just delete it.

> +        
>  	left = packet[4] & 0x01;
>  	right = packet[4] & 0x02;
>  
> @@ -617,21 +665,33 @@
>  	y = ((packet[2] & 0x7f) << 4) | (packet[3] & 0x0f);
>  	z = packet[5] & 0x7f;
>  
> +	if (!fingers) {
> +		x1 = x;
> +		y1 = y;
> +		fingers = z > 0 ? 1 : 0;
> +	}
> +
>  	if (z >= 64)
>  		input_report_key(dev, BTN_TOUCH, 1);
>  	else
>  		input_report_key(dev, BTN_TOUCH, 0);
>  
> +	alps_report_semi_mt_data(dev, fingers, x1, y1, x2, y2);
> +
> +	input_report_key(dev, BTN_TOOL_FINGER, fingers == 1);
> +	input_report_key(dev, BTN_TOOL_DOUBLETAP, fingers == 2);
> +	input_report_key(dev, BTN_TOOL_TRIPLETAP, fingers == 3);
> +	input_report_key(dev, BTN_TOOL_QUADTAP, fingers == 4);
> +
> +	input_report_key(dev, BTN_LEFT, left);
> +	input_report_key(dev, BTN_RIGHT, right);
> +
>  	if (z > 0) {
>  		input_report_abs(dev, ABS_X, x);
>  		input_report_abs(dev, ABS_Y, y);
>  	}
>  	input_report_abs(dev, ABS_PRESSURE, z);
>  
> -	input_report_key(dev, BTN_TOOL_FINGER, z > 0);
> -	input_report_key(dev, BTN_LEFT, left);
> -	input_report_key(dev, BTN_RIGHT, right);
> -
>  	input_sync(dev);
>  }
>  
> @@ -1567,8 +1627,18 @@
>  		set_bit(BTN_TOOL_QUADTAP, dev1->keybit);
>  		/* fall through */
>  	case ALPS_PROTO_V4:
> -		input_set_abs_params(dev1, ABS_X, 0, ALPS_V3_X_MAX, 0, 0);
> -		input_set_abs_params(dev1, ABS_Y, 0, ALPS_V3_Y_MAX, 0, 0);
> +		set_bit(INPUT_PROP_SEMI_MT, dev1->propbit);
> +		input_mt_init_slots(dev1, 2);
> +                input_set_abs_params(dev1, ABS_X, 0, ALPS_V3_X_MAX, 0, 0);
> +                input_set_abs_params(dev1, ABS_Y, 0, ALPS_V3_Y_MAX, 0, 0);
> +
> +		input_set_abs_params(dev1, ABS_MT_POSITION_X, 0, 
> ALPS_V3_X_MAX, 0, 0);
> +		input_set_abs_params(dev1, ABS_MT_POSITION_Y, 0, 
> ALPS_V3_Y_MAX, 0, 0);
> +
> +		set_bit(BTN_TOOL_DOUBLETAP, dev1->keybit);
> +		set_bit(BTN_TOOL_TRIPLETAP, dev1->keybit);
> +		set_bit(BTN_TOOL_QUADTAP, dev1->keybit);
> +		/* fall through */
>  		break;
>  	}
>  
> --- drivers/input/mouse/alps.h	2012-03-19 01:15:34.000000000 +0200
> +++ drivers/input/mouse/alps.h.new	2012-04-09 17:17:32.733618703 +0300
> @@ -39,6 +39,8 @@
>  	int prev_fin;			/* Finger bit from previous packet */
>  	int multi_packet;		/* Multi-packet data in progress */
>  	unsigned char multi_data[6];	/* Saved multi-packet data */
> +	unsigned char multi_data_v4[6];

Why do you need multi_data_v4? You should be able to use multi_data.

> +	int last_fingers;
>  	u8 quirks;
>  	struct timer_list timer;
>  };
> 
> --
> 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

Re: ALPS v4 Semi-mt Support

[Seth Forshee]

On Tue, Apr 10, 2012 at 10:21:13AM -0500, Seth Forshee wrote:
> I definitely think your state processing could be improved. My
> suggestion would be to treat multi_packet as a counter. Reset it to 0
> when you see the sync bit is set, and increment it for each packet until
> you have a full set of MT data. That way you know for sure which section
> of the MT data you're working with for any given packet. But be prepared
> to handle an incomplete packet sequence as well (I'm pretty sure I saw
> some of these when I was working with a v4 touchpad).

I found an old patch I had from working on the semi-MT support
previously that demonstrates the multi_packet-as-counter approach. I
ported the relevant code on top of 3.4-rc2, cleaned it up, compile
tested it, and pasted the resulting patch below.

Note that this is ignoring the ST coordinates except from the final
packet of the MT sequence, which isn't ideal. A better approach might be
to stash the ST data from each packet in alps_data, then when you have
all three packets process the bitmaps and report all three ST points
with the same set of MT data.

Cheers,
Seth


diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
index 4c6a72d..8cc0dbf 100644
--- a/drivers/input/mouse/alps.c
+++ b/drivers/input/mouse/alps.c
@@ -604,35 +604,88 @@ static void alps_process_packet_v3(struct psmouse *psmouse)
 
 static void alps_process_packet_v4(struct psmouse *psmouse)
 {
+	struct alps_data *priv = psmouse->private;
 	unsigned char *packet = psmouse->packet;
 	struct input_dev *dev = psmouse->dev;
+	int offset;
 	int x, y, z;
 	int left, right;
+	int x1, y1, x2, y2;
+	int fingers = 0;
+	unsigned int x_bitmap, y_bitmap;
 
-	left = packet[4] & 0x01;
-	right = packet[4] & 0x02;
+	/*
+	 * v4 has a 6-byte encoding for bitmap data, but this data is
+	 * broken up between 3 normal packets. Use priv->multi_packet to
+	 * track our position in the bitmap packet.
+	 */
+	if (packet[6] & 0x40) {
+		/* sync, reset position */
+		priv->multi_packet = 0;
+	}
 
-	x = ((packet[1] & 0x7f) << 4) | ((packet[3] & 0x30) >> 2) |
-	    ((packet[0] & 0x30) >> 4);
-	y = ((packet[2] & 0x7f) << 4) | (packet[3] & 0x0f);
-	z = packet[5] & 0x7f;
+	if (WARN_ON_ONCE(priv->multi_packet > 2))
+		return;
 
-	if (z >= 64)
-		input_report_key(dev, BTN_TOUCH, 1);
-	else
-		input_report_key(dev, BTN_TOUCH, 0);
+	offset = 2 * priv->multi_packet;
+	priv->multi_data[offset] = packet[6];
+	priv->multi_data[offset + 1] = packet[7];
 
-	if (z > 0) {
-		input_report_abs(dev, ABS_X, x);
-		input_report_abs(dev, ABS_Y, y);
-	}
-	input_report_abs(dev, ABS_PRESSURE, z);
+	if (++priv->multi_packet > 2) {
+		priv->multi_packet = 0;
 
-	input_report_key(dev, BTN_TOOL_FINGER, z > 0);
-	input_report_key(dev, BTN_LEFT, left);
-	input_report_key(dev, BTN_RIGHT, right);
+		left = packet[4] & 0x01;
+		right = packet[4] & 0x02;
 
-	input_sync(dev);
+		x = ((packet[1] & 0x7f) << 4) | ((packet[3] & 0x30) >> 2) |
+		    ((packet[0] & 0x30) >> 4);
+		y = ((packet[2] & 0x7f) << 4) | (packet[3] & 0x0f);
+		z = packet[5] & 0x7f;
+
+		x_bitmap = ((priv->multi_data[2] & 0x1f) << 10) |
+			   ((priv->multi_data[3] & 0x60) << 3) |
+			   ((priv->multi_data[0] & 0x3f) << 2) | 
+			   ((priv->multi_data[1] & 0x60) >> 5);
+		y_bitmap = ((priv->multi_data[5] & 0x01) << 10) |
+			   ((priv->multi_data[3] & 0x1f) << 5) |
+			   (priv->multi_data[1] & 0x1f);
+
+		fingers = alps_process_bitmap(x_bitmap, y_bitmap,
+					      &x1, &y1, &x2, &y2);
+
+		/*
+		 * If there were no contacts in the bitmap, use ST
+		 * points in MT reports.
+		 */
+		if (fingers == 0) {
+			x1 = x;
+			y1 = y;
+			fingers = 1;
+		}
+
+		if (z >= 64)
+			input_report_key(dev, BTN_TOUCH, 1);
+		else
+			input_report_key(dev, BTN_TOUCH, 0);
+
+		alps_report_semi_mt_data(dev, fingers, x1, y1, x2, y2);
+
+		input_report_key(dev, BTN_TOOL_FINGER, fingers == 1);
+		input_report_key(dev, BTN_TOOL_DOUBLETAP, fingers == 2);
+		input_report_key(dev, BTN_TOOL_TRIPLETAP, fingers == 3);
+		input_report_key(dev, BTN_TOOL_QUADTAP, fingers == 4);
+
+		input_report_key(dev, BTN_LEFT, left);
+		input_report_key(dev, BTN_RIGHT, right);
+
+		if (z > 0) {
+			input_report_abs(dev, ABS_X, x);
+			input_report_abs(dev, ABS_Y, y);
+		}
+		input_report_abs(dev, ABS_PRESSURE, z);
+
+		input_sync(dev);
+	}
 }
 
 static void alps_process_packet(struct psmouse *psmouse)
@@ -1557,6 +1610,7 @@ int alps_init(struct psmouse *psmouse)
 		input_set_abs_params(dev1, ABS_Y, 0, 767, 0, 0);
 		break;
 	case ALPS_PROTO_V3:
+	case ALPS_PROTO_V4:
 		set_bit(INPUT_PROP_SEMI_MT, dev1->propbit);
 		input_mt_init_slots(dev1, 2);
 		input_set_abs_params(dev1, ABS_MT_POSITION_X, 0, ALPS_V3_X_MAX, 0, 0);
@@ -1565,8 +1619,7 @@ int alps_init(struct psmouse *psmouse)
 		set_bit(BTN_TOOL_DOUBLETAP, dev1->keybit);
 		set_bit(BTN_TOOL_TRIPLETAP, dev1->keybit);
 		set_bit(BTN_TOOL_QUADTAP, dev1->keybit);
-		/* fall through */
-	case ALPS_PROTO_V4:
+
 		input_set_abs_params(dev1, ABS_X, 0, ALPS_V3_X_MAX, 0, 0);
 		input_set_abs_params(dev1, ABS_Y, 0, ALPS_V3_Y_MAX, 0, 0);
 		break;

Re: ALPS v4 Semi-mt Support

[George Pantalos]

> On Tue, Apr 10, 2012 at 10:21:13AM -0500, Seth Forshee wrote:
> > I definitely think your state processing could be improved. My
> > suggestion would be to treat multi_packet as a counter. Reset it to 0
> > when you see the sync bit is set, and increment it for each packet until
> > you have a full set of MT data. That way you know for sure which section
> > of the MT data you're working with for any given packet. But be prepared
> > to handle an incomplete packet sequence as well (I'm pretty sure I saw
> > some of these when I was working with a v4 touchpad).
> 
> I found an old patch I had from working on the semi-MT support
> previously that demonstrates the multi_packet-as-counter approach. I
> ported the relevant code on top of 3.4-rc2, cleaned it up, compile
> tested it, and pasted the resulting patch below.
> 
> Note that this is ignoring the ST coordinates except from the final
> packet of the MT sequence, which isn't ideal. A better approach might be
> to stash the ST data from each packet in alps_data, then when you have
> all three packets process the bitmaps and report all three ST points
> with the same set of MT data.

Hello Seth,

First of all, sorry for answering so late, I had no internet access for the 
past week.

Thank you for your comments and for posting your patch. You are of course 
correct in your remarks.

 I decided to try and improve upon your patch which has superior state 
processing. I followed your proposal initially and stashed the first and 
second packet ST data in alps_data and if bitmap fingers < 2 report all the ST 
data.
The problem with this approach is that it introduces a noticeable delay/lag in 
mouse pointer movement.

I then tried using the last_fingers approach.  This way we report ST data as 
they come but only if the last MT report had 1 finger present and we always 
(as far as I can tell) must have calculated bitmap finger count before 
reporting ST events. I have posted this patch below. I also observed jumpy 
behavior with the xf86-input-multitouch driver when the MT report had 1 
finger, so if bitmap fingers are 1 , it uses x,y instead of x1,y1.
This approach has the benefit of producing smooth pointer movement and 
accurately reporting MT events like the other approach.

To handle inconsistent data I used your patch to dump raw packets.  I noticed 
,as your documentation also states, that the condition (packet[6] & 0x40) 
could also be triggered by the second MT packet and then reset 
multi_packet to 0.
So I used the fact that byte 5 of the MT data, priv->multi_data [4] must 
always be 0x00. A kind of second sync byte to ensure we have correct MT 
reports. Unless the MT data is consistent we report nothing.

Thanks for your help and guidance.

--- linux-3.3/drivers/input/mouse/alps.c.orig	2012-04-15 21:45:18.083826446 
+0300
+++ linux-3.3/drivers/input/mouse/alps.c	2012-04-16 16:33:11.412181964 
+0300
@@ -604,10 +604,39 @@
 
 static void alps_process_packet_v4(struct psmouse *psmouse)
 {
+	struct alps_data *priv = psmouse->private;
 	unsigned char *packet = psmouse->packet;
 	struct input_dev *dev = psmouse->dev;
+	int offset;
 	int x, y, z;
 	int left, right;
+	int x1, y1, x2, y2;
+	int fingers = 0;
+	unsigned int x_bitmap, y_bitmap;
+
+	/*
+	 * v4 has a 6-byte encoding for bitmap data, but this data is
+	 * broken up between 3 normal packets. Use priv->multi_packet to
+	 * track our position in the bitmap packet.
+	 */
+	if ((packet[6] & 0x40) && (priv->multi_data[4] == 0x00)) {
+		/* sync, reset position */
+		priv->multi_packet = 0;
+	}
+
+	if (WARN_ON_ONCE(priv->multi_packet > 2))
+		return;
+
+	offset = 2 * priv->multi_packet;
+	priv->multi_data[offset] = packet[6];
+	priv->multi_data[offset + 1] = packet[7];
+
+	/*
+	 * The 5th byte of the MT data must always be 0x00.
+	 * Try to re-sync our position if MT data is inconsistent.
+	 */
+	if (priv->multi_data[4] != 0x00)
+		return;
 
 	left = packet[4] & 0x01;
 	right = packet[4] & 0x02;
@@ -617,22 +646,78 @@
 	y = ((packet[2] & 0x7f) << 4) | (packet[3] & 0x0f);
 	z = packet[5] & 0x7f;
 
-	if (z >= 64)
-		input_report_key(dev, BTN_TOUCH, 1);
-	else
-		input_report_key(dev, BTN_TOUCH, 0);
 
-	if (z > 0) {
-		input_report_abs(dev, ABS_X, x);
-		input_report_abs(dev, ABS_Y, y);
-	}
-	input_report_abs(dev, ABS_PRESSURE, z);
+	if (++priv->multi_packet > 2) {
+		priv->multi_packet = 0;
+
+		x_bitmap = ((priv->multi_data[2] & 0x1f) << 10) |
+			   ((priv->multi_data[3] & 0x60) << 3) |
+			   ((priv->multi_data[0] & 0x3f) << 2) |
+			   ((priv->multi_data[1] & 0x60) >> 5);
+		y_bitmap = ((priv->multi_data[5] & 0x01) << 10) |
+			   ((priv->multi_data[3] & 0x1f) << 5) |
+			   (priv->multi_data[1] & 0x1f);
+
+		fingers = alps_process_bitmap(x_bitmap, y_bitmap,
+					      &x1, &y1, &x2, &y2);
 
-	input_report_key(dev, BTN_TOOL_FINGER, z > 0);
-	input_report_key(dev, BTN_LEFT, left);
-	input_report_key(dev, BTN_RIGHT, right);
+		/*
+		 * If there were no contacts in the bitmap, use ST
+		 * points in MT reports.
+		 */
+		if (fingers < 2) {
+			x1 = x;
+			y1 = y;
+			fingers = z > 0 ? 1 : 0;
+		}
+
+		priv->last_fingers = fingers;
+
+		if (z >= 64)
+			input_report_key(dev, BTN_TOUCH, 1);
+		else
+			input_report_key(dev, BTN_TOUCH, 0);
+
+		alps_report_semi_mt_data(dev, fingers, x1, y1, x2, y2);
+
+		input_report_key(dev, BTN_TOOL_FINGER, fingers == 1);
+		input_report_key(dev, BTN_TOOL_DOUBLETAP, fingers == 2);
+		input_report_key(dev, BTN_TOOL_TRIPLETAP, fingers == 3);
+		input_report_key(dev, BTN_TOOL_QUADTAP, fingers == 4);
+
+		input_report_key(dev, BTN_LEFT, left);
+		input_report_key(dev, BTN_RIGHT, right);
+
+		if (z > 0) {
+			input_report_abs(dev, ABS_X, x);
+			input_report_abs(dev, ABS_Y, y);
+		}
+		input_report_abs(dev, ABS_PRESSURE, z);
+
+		input_sync(dev);
+	} else {
+		if (priv->last_fingers == 1) {
+			if (z >= 64)
+				input_report_key(dev, BTN_TOUCH, 1);
+			else
+				input_report_key(dev, BTN_TOUCH, 0);
+
+			alps_report_semi_mt_data(dev, 1, x, y, 0, 0);
+
+			input_report_key(dev, BTN_TOOL_FINGER, z > 0);
+
+			input_report_key(dev, BTN_LEFT, left);
+			input_report_key(dev, BTN_RIGHT, right);
+
+			if (z > 0) {
+				input_report_abs(dev, ABS_X, x);
+				input_report_abs(dev, ABS_Y, y);
+			}
+			input_report_abs(dev, ABS_PRESSURE, z);
 
-	input_sync(dev);
+			input_sync(dev);
+		}
+	}
 }
 
 static void alps_process_packet(struct psmouse *psmouse)
@@ -1557,6 +1642,7 @@
 		input_set_abs_params(dev1, ABS_Y, 0, 767, 0, 0);
 		break;
 	case ALPS_PROTO_V3:
+	case ALPS_PROTO_V4:
 		set_bit(INPUT_PROP_SEMI_MT, dev1->propbit);
 		input_mt_init_slots(dev1, 2);
 		input_set_abs_params(dev1, ABS_MT_POSITION_X, 0, 
ALPS_V3_X_MAX, 0, 0);
@@ -1565,8 +1651,7 @@
 		set_bit(BTN_TOOL_DOUBLETAP, dev1->keybit);
 		set_bit(BTN_TOOL_TRIPLETAP, dev1->keybit);
 		set_bit(BTN_TOOL_QUADTAP, dev1->keybit);
-		/* fall through */
-	case ALPS_PROTO_V4:
+
 		input_set_abs_params(dev1, ABS_X, 0, ALPS_V3_X_MAX, 0, 0);
 		input_set_abs_params(dev1, ABS_Y, 0, ALPS_V3_Y_MAX, 0, 0);
 		break;
--- linux-3.3/drivers/input/mouse/alps.h.orig	2012-04-16 
16:39:49.948823942 +0300
+++ linux-3.3/drivers/input/mouse/alps.h	2012-04-16 16:41:28.228817760 
+0300
@@ -39,6 +39,7 @@
 	int prev_fin;			/* Finger bit from previous packet */
 	int multi_packet;		/* Multi-packet data in progress */
 	unsigned char multi_data[6];	/* Saved multi-packet data */
+	int last_fingers;		/* Number of fingers from MT report*/
 	u8 quirks;
 	struct timer_list timer;
 };

Re: ALPS v4 Semi-mt Support

[Seth Forshee]

On Mon, Apr 16, 2012 at 05:24:55PM +0300, George Pantalos wrote:
>  I decided to try and improve upon your patch which has superior state 
> processing. I followed your proposal initially and stashed the first and 
> second packet ST data in alps_data and if bitmap fingers < 2 report all the ST 
> data.
> The problem with this approach is that it introduces a noticeable delay/lag in 
> mouse pointer movement.
> 
> I then tried using the last_fingers approach.  This way we report ST data as 
> they come but only if the last MT report had 1 finger present and we always 
> (as far as I can tell) must have calculated bitmap finger count before 
> reporting ST events. I have posted this patch below. I also observed jumpy 
> behavior with the xf86-input-multitouch driver when the MT report had 1 
> finger, so if bitmap fingers are 1 , it uses x,y instead of x1,y1.
> This approach has the benefit of producing smooth pointer movement and 
> accurately reporting MT events like the other approach.

If the latency really is noticible when you stash the ST points, here's
what I'd suggest trying instead. Stash away the last set of MT data you
saw and repeat it with each of the next two ST coordinates. I suspect
that will probably work well enough, and will allow every ST point to
still be reported. And it should significantly simplify the code as
well.

> To handle inconsistent data I used your patch to dump raw packets.  I noticed 
> ,as your documentation also states, that the condition (packet[6] & 0x40) 
> could also be triggered by the second MT packet and then reset 
> multi_packet to 0.
> So I used the fact that byte 5 of the MT data, priv->multi_data [4] must 
> always be 0x00. A kind of second sync byte to ensure we have correct MT 
> reports. Unless the MT data is consistent we report nothing.

You should keep in mind that the documentation is based purely on my
observations. I observed that the 4th byte of the assembled MT data
packet is alwyas 0, so that's how I documented it. That doesn't
necessarily mean that it really is always 0, just that I could never get
it to assume any other value.

> +	if ((packet[6] & 0x40) && (priv->multi_data[4] == 0x00)) {
> +		/* sync, reset position */
> +		priv->multi_packet = 0;
> +	}

If you see the sync bit set, it's _always_ the first fragment of the MT
data, so you shoule _always_ reset the position. Why should past data
have any effect on this decision?

> +	/*
> +	 * The 5th byte of the MT data must always be 0x00.
> +	 * Try to re-sync our position if MT data is inconsistent.
> +	 */
> +	if (priv->multi_data[4] != 0x00)
> +		return;

This doesn't really re-sync the position, and the sync bit is sufficient
for this purpose anyway. I'd propose that if you really think checking
multi_data[4] is beneficial, use it only for validating the MT packet
before parsing it.

> +	} else {
> +		if (priv->last_fingers == 1) {
> +			if (z >= 64)
> +				input_report_key(dev, BTN_TOUCH, 1);
> +			else
> +				input_report_key(dev, BTN_TOUCH, 0);
> +
> +			alps_report_semi_mt_data(dev, 1, x, y, 0, 0);
> +
> +			input_report_key(dev, BTN_TOOL_FINGER, z > 0);

Even if you use a separate case here you need to update the other
BTN_TOOL keys. The 1 to 0 transition is needed for userspace to know
that the situation has changed. Failing to report any value is not the
same as reporting a value of 0.

Re: ALPS v4 Semi-mt Support

[George Pantalos]

On Monday 16 of April 2012 16:24:07 Seth Forshee wrote:
> If the latency really is noticible when you stash the ST points, here's
> what I'd suggest trying instead. Stash away the last set of MT data you
> saw and repeat it with each of the next two ST coordinates. I suspect
> that will probably work well enough, and will allow every ST point to
> still be reported. And it should significantly simplify the code as
> well.

I will try to do that, thanks.

> If you see the sync bit set, it's _always_ the first fragment of the MT
> data, so you shoule _always_ reset the position. Why should past data
> have any effect on this decision?

I have noticed that the sync bit can at times be set in the second packet of 
the sequence. Couldn't this reset the position to priv->multi_packet=0 when 
in fact we are in priv->multi_packet=1  position?
I have also thought about "if((packet[6] & 0x40) && (priv->multi_packet == 
0))" so that sync is not lost. 
 
> This doesn't really re-sync the position, and the sync bit is sufficient
> for this purpose anyway. I'd propose that if you really think checking
> multi_data[4] is beneficial, use it only for validating the MT packet
> before parsing it.

OK, I have tried that before, thanks for the suggestion.

> Even if you use a separate case here you need to update the other
> BTN_TOOL keys. The 1 to 0 transition is needed for userspace to know
> that the situation has changed. Failing to report any value is not the
> same as reporting a value of 0.

Re: ALPS v4 Semi-mt Support

[Seth Forshee]

On Tue, Apr 17, 2012 at 01:21:45AM +0300, George Pantalos wrote:
> I have noticed that the sync bit can at times be set in the second packet of 
> the sequence. Couldn't this reset the position to priv->multi_packet=0 when 
> in fact we are in priv->multi_packet=1  position?
> I have also thought about "if((packet[6] & 0x40) && (priv->multi_packet == 
> 0))" so that sync is not lost. 

I think there are two possibilities for when you see the sync bit set in
the second packet.

The first is that the touchpad aborted the previous sequence and started
a new one. In this case you really do want to reset priv->multi_packet.

The other option is that it really isn't a sync bit, and that it has
some other meaning. In that case you'll need to find some other reliable
method for assembling the MT data in the correct order.

The only way you'll be able to tell is by inspecting the packets after
the one with the unexpected sync. I suspect you'll either see one of two
sequences. This one:

  sync -> sync -> non-sync -> non-sync -> sync -> ...

Which would appear to be an aborted sequence followed by the start of a
new one. Or this one:

  sync -> sync -> non-sync -> sync -> ...

Which would appear to indicate that what we're calling the sync bit
isn't really a sync after all.

Seth

Re: ALPS v4 Semi-mt Support

[George Pantalos]

On Monday 16 of April 2012 16:24:07 Seth Forshee wrote:
> If the latency really is noticible when you stash the ST points, here's
> what I'd suggest trying instead. Stash away the last set of MT data you
> saw and repeat it with each of the next two ST coordinates. I suspect
> that will probably work well enough, and will allow every ST point to
> still be reported. And it should significantly simplify the code as
> well.

I tried your suggestion and I have to say that it works great. 
Thank you, Seth.
I hope my execution is ok. 

--- linux-3.3/drivers/input/mouse/alps.c.orig	2012-04-15 21:45:18.083826446 
+0300
+++ linux-3.3/drivers/input/mouse/alps.c	2012-04-17 03:18:51.806595048 
+0300
@@ -604,10 +604,56 @@
 
 static void alps_process_packet_v4(struct psmouse *psmouse)
 {
+	struct alps_data *priv = psmouse->private;
 	unsigned char *packet = psmouse->packet;
 	struct input_dev *dev = psmouse->dev;
+	int offset;
 	int x, y, z;
 	int left, right;
+	int x1, y1, x2, y2;
+	int fingers = 0;
+	unsigned int x_bitmap, y_bitmap;
+
+	/*
+	 * v4 has a 6-byte encoding for bitmap data, but this data is
+	 * broken up between 3 normal packets. Use priv->multi_packet to
+	 * track our position in the bitmap packet.
+	 */
+	if (packet[6] & 0x40) {
+		/* sync, reset position */
+		priv->multi_packet = 0;
+	}
+
+	if (WARN_ON_ONCE(priv->multi_packet > 2))
+		return;
+
+	offset = 2 * priv->multi_packet;
+	priv->multi_data[offset] = packet[6];
+	priv->multi_data[offset + 1] = packet[7];
+
+	if (++priv->multi_packet > 2) {
+		priv->multi_packet = 0;
+
+		x_bitmap = ((priv->multi_data[2] & 0x1f) << 10) |
+			   ((priv->multi_data[3] & 0x60) << 3) |
+			   ((priv->multi_data[0] & 0x3f) << 2) |
+			   ((priv->multi_data[1] & 0x60) >> 5);
+		y_bitmap = ((priv->multi_data[5] & 0x01) << 10) |
+			   ((priv->multi_data[3] & 0x1f) << 5) |
+			   (priv->multi_data[1] & 0x1f);
+
+		fingers = alps_process_bitmap(x_bitmap, y_bitmap,
+					      &x1, &y1, &x2, &y2);
+
+		/*
+		 * Store MT data.
+		 */
+		priv->fingers = fingers;
+		priv->x1 = x1;
+		priv->x2 = x2;
+		priv->y1 = y1;
+		priv->y2 = y2;
+	}
 
 	left = packet[4] & 0x01;
 	right = packet[4] & 0x02;
@@ -617,21 +663,44 @@
 	y = ((packet[2] & 0x7f) << 4) | (packet[3] & 0x0f);
 	z = packet[5] & 0x7f;
 
+	/*
+	 * If there were no contacts in the bitmap, use ST
+	 * points in MT reports.
+	 * If there were two contacts or more, report MT data.
+	 */
+	if (priv->fingers < 2) {
+		x1 = x;
+		y1 = y;
+		fingers = z > 0 ? 1 : 0;
+	} else {
+		fingers = priv->fingers;
+		x1 = priv->x1;
+		x2 = priv->x2;
+		y1 = priv->y1;
+		y2 = priv->y2;
+	}
+
 	if (z >= 64)
 		input_report_key(dev, BTN_TOUCH, 1);
 	else
 		input_report_key(dev, BTN_TOUCH, 0);
 
+	alps_report_semi_mt_data(dev, fingers, x1, y1, x2, y2);
+
+	input_report_key(dev, BTN_TOOL_FINGER, fingers == 1);
+	input_report_key(dev, BTN_TOOL_DOUBLETAP, fingers == 2);
+	input_report_key(dev, BTN_TOOL_TRIPLETAP, fingers == 3);
+	input_report_key(dev, BTN_TOOL_QUADTAP, fingers == 4);
+
+	input_report_key(dev, BTN_LEFT, left);
+	input_report_key(dev, BTN_RIGHT, right);
+
 	if (z > 0) {
 		input_report_abs(dev, ABS_X, x);
 		input_report_abs(dev, ABS_Y, y);
 	}
 	input_report_abs(dev, ABS_PRESSURE, z);
 
-	input_report_key(dev, BTN_TOOL_FINGER, z > 0);
-	input_report_key(dev, BTN_LEFT, left);
-	input_report_key(dev, BTN_RIGHT, right);
-
 	input_sync(dev);
 }
 
@@ -1557,6 +1626,7 @@
 		input_set_abs_params(dev1, ABS_Y, 0, 767, 0, 0);
 		break;
 	case ALPS_PROTO_V3:
+	case ALPS_PROTO_V4:
 		set_bit(INPUT_PROP_SEMI_MT, dev1->propbit);
 		input_mt_init_slots(dev1, 2);
 		input_set_abs_params(dev1, ABS_MT_POSITION_X, 0, 
ALPS_V3_X_MAX, 0, 0);
@@ -1565,8 +1635,7 @@
 		set_bit(BTN_TOOL_DOUBLETAP, dev1->keybit);
 		set_bit(BTN_TOOL_TRIPLETAP, dev1->keybit);
 		set_bit(BTN_TOOL_QUADTAP, dev1->keybit);
-		/* fall through */
-	case ALPS_PROTO_V4:
+
 		input_set_abs_params(dev1, ABS_X, 0, ALPS_V3_X_MAX, 0, 0);
 		input_set_abs_params(dev1, ABS_Y, 0, ALPS_V3_Y_MAX, 0, 0);
 		break;
--- linux-3.3/drivers/input/mouse/alps.h.orig	2012-04-16 
16:39:49.948823942 +0300
+++ linux-3.3/drivers/input/mouse/alps.h	2012-04-17 03:20:03.656594368 
+0300
@@ -39,6 +39,8 @@
 	int prev_fin;			/* Finger bit from previous packet */
 	int multi_packet;		/* Multi-packet data in progress */
 	unsigned char multi_data[6];	/* Saved multi-packet data */
+	int x1, x2, y1, y2;		/* Coordinates from last MT report */
+	int fingers;			/* Number of fingers from MT report */
 	u8 quirks;
 	struct timer_list timer;
 };

Re: ALPS v4 Semi-mt Support

[Seth Forshee]

On Tue, Apr 17, 2012 at 03:52:34AM +0300, George Pantalos wrote:
> On Monday 16 of April 2012 16:24:07 Seth Forshee wrote:
> > If the latency really is noticible when you stash the ST points, here's
> > what I'd suggest trying instead. Stash away the last set of MT data you
> > saw and repeat it with each of the next two ST coordinates. I suspect
> > that will probably work well enough, and will allow every ST point to
> > still be reported. And it should significantly simplify the code as
> > well.
> 
> I tried your suggestion and I have to say that it works great. 
> Thank you, Seth.
> I hope my execution is ok. 

The implementation looks good to me. If you're confident that the
interpretation of the sync is correct, submit the patch according to the
guidelines in Documentation/SubmittingPatches and I'll be happy to give
my ack.

Seth