[PATCH v2 0/2] Improve the performance of alps v5-protocol's touchpad

[PATCH v2 0/2] Improve the performance of alps v5-protocol's touchpad

[Yunkang Tang]

Hi all,

Here is the v2 of improving ALPS v5 protocol device.

Change since v1:
- fix the issue that previous patches were broken by mail system.
- split the modification to 2 patches.
- Change dev2's name to "ALPS PS/2 Device"

Best Regards,
Tommy

[PATCH v2 1/2] Add an additional argument for decode routine, change secondary device name

[Yunkang Tang]

From: Yunkang Tang <yunkang.tang@cn.alps.com>

- Add an addition argument for decode routine, new devices need this info.
- Change the dev2's name from "PS/2 Mouse" to "ALPS PS/2 Device".

Signed-off-by: Yunkang Tang <yunkang.tang@cn.alps.com>
---
 drivers/input/mouse/alps.c | 17 ++++++++++-------
 drivers/input/mouse/alps.h |  5 +++--
 2 files changed, 13 insertions(+), 9 deletions(-)

diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
index 7c5d72a..7e8a4fb 100644
--- a/drivers/input/mouse/alps.c
+++ b/drivers/input/mouse/alps.c
@@ -461,7 +461,8 @@ static void alps_decode_buttons_v3(struct alps_fields *f, unsigned char *p)
 	f->ts_middle = !!(p[3] & 0x40);
 }
 
-static void alps_decode_pinnacle(struct alps_fields *f, unsigned char *p)
+static void alps_decode_pinnacle(struct alps_fields *f, unsigned char *p,
+				 struct psmouse *psmouse)
 {
 	f->first_mp = !!(p[4] & 0x40);
 	f->is_mp = !!(p[0] & 0x40);
@@ -482,15 +483,17 @@ static void alps_decode_pinnacle(struct alps_fields *f, unsigned char *p)
 	alps_decode_buttons_v3(f, p);
 }
 
-static void alps_decode_rushmore(struct alps_fields *f, unsigned char *p)
+static void alps_decode_rushmore(struct alps_fields *f, unsigned char *p,
+				 struct psmouse *psmouse)
 {
-	alps_decode_pinnacle(f, p);
+	alps_decode_pinnacle(f, p, psmouse);
 
 	f->x_map |= (p[5] & 0x10) << 11;
 	f->y_map |= (p[5] & 0x20) << 6;
 }
 
-static void alps_decode_dolphin(struct alps_fields *f, unsigned char *p)
+static void alps_decode_dolphin(struct alps_fields *f, unsigned char *p,
+				struct psmouse *psmouse)
 {
 	f->first_mp = !!(p[0] & 0x02);
 	f->is_mp = !!(p[0] & 0x20);
@@ -523,7 +526,7 @@ static void alps_process_touchpad_packet_v3(struct psmouse *psmouse)
 	int fingers = 0, bmap_fingers;
 	struct alps_fields f;
 
-	priv->decode_fields(&f, packet);
+	priv->decode_fields(&f, packet, psmouse);
 
 	/*
 	 * There's no single feature of touchpad position and bitmap packets
@@ -552,7 +555,7 @@ static void alps_process_touchpad_packet_v3(struct psmouse *psmouse)
 				fingers = bmap_fingers;
 
 			/* Now process position packet */
-			priv->decode_fields(&f, priv->multi_data);
+			priv->decode_fields(&f, priv->multi_data, psmouse);
 		} else {
 			priv->multi_packet = 0;
 		}
@@ -1792,7 +1795,7 @@ int alps_init(struct psmouse *psmouse)
 	snprintf(priv->phys, sizeof(priv->phys), "%s/input1", psmouse->ps2dev.serio->phys);
 	dev2->phys = priv->phys;
 	dev2->name = (priv->flags & ALPS_DUALPOINT) ?
-		     "DualPoint Stick" : "PS/2 Mouse";
+		     "DualPoint Stick" : "ALPS PS/2 Device";
 	dev2->id.bustype = BUS_I8042;
 	dev2->id.vendor  = 0x0002;
 	dev2->id.product = PSMOUSE_ALPS;
diff --git a/drivers/input/mouse/alps.h b/drivers/input/mouse/alps.h
index eee5985..cdc10f3 100644
--- a/drivers/input/mouse/alps.h
+++ b/drivers/input/mouse/alps.h
@@ -69,7 +69,7 @@ struct alps_nibble_commands {
  * @y: Y position for ST.
  * @z: Z position for ST.
  * @first_mp: Packet is the first of a multi-packet report.
- * @is_mp: Packet is part of a multi-packet report.
+ * @is_mp: Packet is the last of a multi-packet report.
  * @left: Left touchpad button is active.
  * @right: Right touchpad button is active.
  * @middle: Middle touchpad button is active.
@@ -145,7 +145,8 @@ struct alps_data {
 
 	int (*hw_init)(struct psmouse *psmouse);
 	void (*process_packet)(struct psmouse *psmouse);
-	void (*decode_fields)(struct alps_fields *f, unsigned char *p);
+	void (*decode_fields)(struct alps_fields *f, unsigned char *p,
+			      struct psmouse *psmouse);
 	void (*set_abs_params)(struct alps_data *priv, struct input_dev *dev1);
 
 	int prev_fin;
-- 
1.8.1.2

[PATCH v2 2/2] Improve the performance of alps v5-protocol's touchpad

[Yunkang Tang]

From: Yunkang Tang <yunkang.tang@cn.alps.com>

- Calculate the device's dimension in alps_identify().
- Change the logic of packet decoding.
- Add the new data process logic.

Signed-off-by: Yunkang Tang <yunkang.tang@cn.alps.com>
---
 drivers/input/mouse/alps.c | 278 ++++++++++++++++++++++++++++++++++++++++++---
 drivers/input/mouse/alps.h |   4 +
 2 files changed, 264 insertions(+), 18 deletions(-)

diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
index 7e8a4fb..86c956a 100644
--- a/drivers/input/mouse/alps.c
+++ b/drivers/input/mouse/alps.c
@@ -257,6 +257,75 @@ static void alps_process_packet_v1_v2(struct psmouse *psmouse)
 }
 
 /*
+ * Process bitmap data for V5 protocols. Return value is null.
+ *
+ * The bitmaps don't have enough data to track fingers, so this function
+ * only generates points representing a bounding box of at most two contacts.
+ * These two points are returned in x1, y1, x2, and y2.
+ */
+static void alps_process_bitmap_dolphin(struct alps_data *priv,
+					struct alps_fields *fields,
+					int *x1, int *y1, int *x2, int *y2)
+{
+	struct alps_palm_bitmap {
+		int start_bit;
+		int end_bit;
+	};
+
+	int i;
+	int box_middle_x, box_middle_y;
+	unsigned int x_map, y_map;
+	struct alps_palm_bitmap x_bitmap = {0}, y_bitmap = {0};
+
+	x_map = fields->x_map;
+	y_map = fields->y_map;
+
+	if (!x_map || !y_map)
+		return;
+
+	*x1 = *y1 = *x2 = *y2 = 0;
+
+	if (fields->fingers > 1) {
+		for (i = 0;  i < 32; i++) {
+			if (x_map & (1 << i)) {
+				x_bitmap.end_bit = priv->x_bits - i;
+				break;
+			}
+		}
+
+		for (i = 31; i >= 0; i--) {
+			if (x_map & (1 << i)) {
+				x_bitmap.start_bit = priv->x_bits - i;
+				break;
+			}
+		}
+
+		for (i = 0;  i < 32; i++) {
+			if (y_map & (1 << i)) {
+				y_bitmap.start_bit = i;
+				break;
+			}
+		}
+
+		for (i = 31; i >= 0; i--) {
+			if (y_map & (1 << i)) {
+				y_bitmap.end_bit = i;
+				break;
+			}
+		}
+
+		box_middle_x = (priv->x_max * (x_bitmap.start_bit + x_bitmap.end_bit)) /
+				(2 * (priv->x_bits - 1));
+		box_middle_y = (priv->y_max * (y_bitmap.start_bit + y_bitmap.end_bit)) /
+				(2 * (priv->y_bits - 1));
+		*x1 = fields->x;
+		*y1 = fields->y;
+		*x2 = 2 * box_middle_x - *x1;
+		*y2 = 2 * box_middle_y - *y1;
+	}
+}
+
+/*
  * Process bitmap data from v3 and v4 protocols. Returns the number of
  * fingers detected. A return value of 0 means at least one of the
  * bitmaps was empty.
@@ -495,25 +564,55 @@ static void alps_decode_rushmore(struct alps_fields *f, unsigned char *p,
 static void alps_decode_dolphin(struct alps_fields *f, unsigned char *p,
 				struct psmouse *psmouse)
 {
+	unsigned int palm_high = 0, palm_low = 0, palm_mask = 0;
+	int i, remain_xbits;
+	struct alps_data *priv = psmouse->private;
+
 	f->first_mp = !!(p[0] & 0x02);
 	f->is_mp = !!(p[0] & 0x20);
 
-	f->fingers = ((p[0] & 0x6) >> 1 |
+	if (!f->is_mp) {
+		f->x = ((p[1] & 0x7f) | ((p[4] & 0x0f) << 7));
+		f->y = ((p[2] & 0x7f) | ((p[4] & 0xf0) << 3));
+		f->z = (p[0] & 4) ? 0 : p[5] & 0x7f;
+		alps_decode_buttons_v3(f, p);
+	} else {
+		f->fingers = ((p[0] & 0x6) >> 1 |
 		     (p[0] & 0x10) >> 2);
-	f->x_map = ((p[2] & 0x60) >> 5) |
-		   ((p[4] & 0x7f) << 2) |
-		   ((p[5] & 0x7f) << 9) |
-		   ((p[3] & 0x07) << 16) |
-		   ((p[3] & 0x70) << 15) |
-		   ((p[0] & 0x01) << 22);
-	f->y_map = (p[1] & 0x7f) |
-		   ((p[2] & 0x1f) << 7);
-
-	f->x = ((p[1] & 0x7f) | ((p[4] & 0x0f) << 7));
-	f->y = ((p[2] & 0x7f) | ((p[4] & 0xf0) << 3));
-	f->z = (p[0] & 4) ? 0 : p[5] & 0x7f;
 
-	alps_decode_buttons_v3(f, p);
+		palm_low = (p[1] & 0x7f) |
+			   ((p[2] & 0x7f) << 7) |
+			   ((p[4] & 0x7f) << 14) |
+			   ((p[5] & 0x7f) << 21) |
+			   ((p[3] & 0x07) << 28) | ((p[3] & 0x10) << 27);
+		palm_high = ((p[3] & 0x60) >> 5) | ((p[0] & 0x01) << 2);
+
+		for (i = 0; i < priv->y_bits; i++)
+			palm_mask |= 1 << i;
+
+		/* Y-profile is stored in P(0) to p(n), n = y_bits; */
+		f->y_map = palm_low & palm_mask;
+
+		/* X-profile is stored in p(n) to p(n+x_bits). */
+		palm_mask = 0;
+		for (i = priv->y_bits; i < 32 && i < priv->y_bits + priv->x_bits; i++)
+			palm_mask |= 1 << i;
+
+		f->x_map = ((palm_low & (palm_mask)) >> priv->y_bits);
+
+		/*
+		 * In some cases, palm_low's 32bit is not enough to save
+		 * all X&Y-profiles, we need to use palm_high.
+		 */
+		remain_xbits = priv->x_bits + priv->y_bits - 32;
+		if (remain_xbits > 0) {
+			palm_mask = 0;
+			for (i = 0; i < remain_xbits; i++)
+				palm_mask |= 1 << i;
+
+			f->x_map |= ((palm_high & palm_mask) << (32 - priv->y_bits));
+		}
+	}
 }
 
 static void alps_process_touchpad_packet_v3(struct psmouse *psmouse)
@@ -745,6 +844,112 @@ static void alps_process_packet_v4(struct psmouse *psmouse)
 	input_sync(dev);
 }
 
+
+static void alps_process_touchpad_packet_v5(struct psmouse *psmouse)
+{
+	struct alps_data *priv = psmouse->private;
+	unsigned char *packet = psmouse->packet;
+	struct input_dev *dev = psmouse->dev;
+	int x1 = 0, y1 = 0, x2 = 0, y2 = 0;
+	int fingers = 0;
+	struct alps_fields f;
+
+	priv->decode_fields(&f, packet, psmouse);
+
+	/*
+	 * There's no single feature of touchpad position and bitmap packets
+	 * that can be used to distinguish between them. We rely on the fact
+	 * that a bitmap packet should always follow a position packet with
+	 * bit 6 of packet[4] set.
+	 */
+	if (priv->multi_packet) {
+		/*
+		 * Sometimes a position packet will indicate a multi-packet
+		 * sequence, but then what follows is another position
+		 * packet. Check for this, and when it happens process the
+		 * position packet as usual.
+		 */
+		if (f.is_mp) {
+			fingers = f.fingers;
+			priv->decode_fields(&f, priv->multi_data, psmouse);
+			alps_process_bitmap_dolphin(priv, &f, &x1, &y1,
+						    &x2, &y2);
+		} else {
+			priv->multi_packet = 0;
+		}
+	}
+
+	/*
+	 * Bit 6 of byte 0 is not usually set in position packets. The only
+	 * times it seems to be set is in situations where the data is
+	 * suspect anyway, e.g. a palm resting flat on the touchpad. Given
+	 * this combined with the fact that this bit is useful for filtering
+	 * out misidentified bitmap packets, we reject anything with this
+	 * bit set.
+	 */
+	if (f.is_mp)
+		return;
+
+	if (!priv->multi_packet && f.first_mp) {
+		priv->multi_packet = 1;
+		memcpy(priv->multi_data, packet, sizeof(priv->multi_data));
+		return;
+	}
+
+	priv->multi_packet = 0;
+
+	/*
+	 * Sometimes the hardware sends a single packet with z = 0
+	 * in the middle of a stream. Real releases generate packets
+	 * with x, y, and z all zero, so these seem to be flukes.
+	 * Ignore them.
+	 */
+	if (f.x && f.y && !f.z)
+		return;
+
+	/*
+	 * If we don't have MT data or the bitmaps were empty, we have
+	 * to rely on ST data.
+	 */
+	if (!fingers) {
+		x1 = f.x;
+		y1 = f.y;
+		fingers = f.z > 0 ? 1 : 0;
+	}
+
+	if (f.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_mt_report_finger_count(dev, 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);
+
+	if (f.z > 0) {
+		input_report_abs(dev, ABS_X, f.x);
+		input_report_abs(dev, ABS_Y, f.y);
+	}
+	input_report_abs(dev, ABS_PRESSURE, f.z);
+
+	input_sync(dev);
+}
+
+static void alps_process_packet_v5(struct psmouse *psmouse)
+{
+	unsigned char *packet = psmouse->packet;
+
+	/* Ignore stick point data */
+	if (packet[0] == 0xD8)
+		return;
+
+	alps_process_touchpad_packet_v5(psmouse);
+}
+
 static void alps_report_bare_ps2_packet(struct psmouse *psmouse,
 					unsigned char packet[],
 					bool report_buttons)
@@ -1522,6 +1727,41 @@ error:
 	return -1;
 }
 
+static int alps_dolphin_get_device_area(struct psmouse *psmouse,
+					struct alps_data *priv)
+{
+	struct ps2dev *ps2dev = &psmouse->ps2dev;
+	unsigned char param[4] = {0};
+	int num_x_electrode, num_y_electrode;
+
+	if (alps_enter_command_mode(psmouse))
+		return -1;
+
+	if (ps2_command(ps2dev, NULL, 0x00EC) ||
+	    ps2_command(ps2dev, NULL, 0x00F0) ||
+	    ps2_command(ps2dev, NULL, 0x00F0) ||
+	    ps2_command(ps2dev, NULL, 0x00F3) ||
+	    ps2_command(ps2dev, NULL, 0x000A) ||
+	    ps2_command(ps2dev, NULL, 0x00F3) ||
+	    ps2_command(ps2dev, NULL, 0x000A))
+		return -1;
+
+	if (ps2_command(ps2dev, param, PSMOUSE_CMD_GETINFO))
+		return -1;
+
+	num_x_electrode = DOLPHIN_PROFILE_XOFFSET + (param[2] & 0x0F);
+	num_y_electrode = DOLPHIN_PROFILE_YOFFSET + ((param[2] >> 4) & 0x0F);
+	priv->x_bits = num_x_electrode;
+	priv->y_bits = num_y_electrode;
+	priv->x_max = (num_x_electrode - 1) * DOLPHIN_COUNT_PER_ELECTRODE;
+	priv->y_max = (num_y_electrode - 1) * DOLPHIN_COUNT_PER_ELECTRODE;
+
+	if (alps_exit_command_mode(psmouse))
+		return -1;
+
+	return 0;
+}
+
 static int alps_hw_init_dolphin_v1(struct psmouse *psmouse)
 {
 	struct ps2dev *ps2dev = &psmouse->ps2dev;
@@ -1574,7 +1814,7 @@ static void alps_set_defaults(struct alps_data *priv)
 		break;
 	case ALPS_PROTO_V5:
 		priv->hw_init = alps_hw_init_dolphin_v1;
-		priv->process_packet = alps_process_packet_v3;
+		priv->process_packet = alps_process_packet_v5;
 		priv->decode_fields = alps_decode_dolphin;
 		priv->set_abs_params = alps_set_abs_params_mt;
 		priv->nibble_commands = alps_v3_nibble_commands;
@@ -1648,11 +1888,13 @@ static int alps_identify(struct psmouse *psmouse, struct alps_data *priv)
 	if (alps_match_table(psmouse, priv, e7, ec) == 0) {
 		return 0;
 	} else if (e7[0] == 0x73 && e7[1] == 0x03 && e7[2] == 0x50 &&
-		   ec[0] == 0x73 && ec[1] == 0x01) {
+		   ec[0] == 0x73 && (ec[1] == 0x01 || ec[1] == 0x02)) {
 		priv->proto_version = ALPS_PROTO_V5;
 		alps_set_defaults(priv);
-
-		return 0;
+		if (alps_dolphin_get_device_area(psmouse, priv))
+			return -EIO;
+		else
+			return 0;
 	} else if (ec[0] == 0x88 && ec[1] == 0x08) {
 		priv->proto_version = ALPS_PROTO_V3;
 		alps_set_defaults(priv);
diff --git a/drivers/input/mouse/alps.h b/drivers/input/mouse/alps.h
index cdc10f3..43d89fc7 100644
--- a/drivers/input/mouse/alps.h
+++ b/drivers/input/mouse/alps.h
@@ -18,6 +18,10 @@
 #define ALPS_PROTO_V4	4
 #define ALPS_PROTO_V5	5
 
+#define DOLPHIN_COUNT_PER_ELECTRODE	64
+#define DOLPHIN_PROFILE_XOFFSET		8	/* x-electrode offset */
+#define DOLPHIN_PROFILE_YOFFSET		1	/* y-electrode offset */
+
 /**
  * struct alps_model_info - touchpad ID table
  * @signature: E7 response string to match.
-- 
1.8.1.2

Re: [PATCH v2 1/2] Add an additional argument for decode routine, change secondary device name

[Kevin Cernekee]

> [PATCH v2 2/2] Improve the performance of alps v5-protocol's touchpad
>
> From: Yunkang Tang <yunkang.tang@xxxxxxxxxxx>
>
> - Calculate the device's dimension in alps_identify().
> - Change the logic of packet decoding.
> - Add the new data process logic.
>
> Signed-off-by: Yunkang Tang <yunkang.tang@xxxxxxxxxxx>

Hmm, somehow the [2/2] patch made it into the linux-input list
archives, but linux-input did not forward it to my inbox so I missed
it the first time around.

But thanks for sending these changes.  There has been a great deal of
interest in enhancing ALPS touchpad support, especially for the
Dolphin models.

Some review comments follow:

> ---
>  drivers/input/mouse/alps.c | 278 ++++++++++++++++++++++++++++++++++++++++++---
>  drivers/input/mouse/alps.h |   4 +
>  2 files changed, 264 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
> index 7e8a4fb..86c956a 100644
> --- a/drivers/input/mouse/alps.c
> +++ b/drivers/input/mouse/alps.c
> @@ -257,6 +257,75 @@ static void alps_process_packet_v1_v2(struct psmouse *psmouse)
>  }
>
>  /*
> + * Process bitmap data for V5 protocols. Return value is null.
> + *
> + * The bitmaps don't have enough data to track fingers, so this function
> + * only generates points representing a bounding box of at most two contacts.
> + * These two points are returned in x1, y1, x2, and y2.
> + */
> +static void alps_process_bitmap_dolphin(struct alps_data *priv,
> +                    struct alps_fields *fields,
> +                    int *x1, int *y1, int *x2, int *y2)
> +{
> +    struct alps_palm_bitmap {
> +        int start_bit;
> +        int end_bit;
> +    };
> +
> +    int i;
> +    int box_middle_x, box_middle_y;
> +    unsigned int x_map, y_map;
> +    struct alps_palm_bitmap x_bitmap = {0}, y_bitmap = {0};
> +
> +    x_map = fields->x_map;
> +    y_map = fields->y_map;
> +
> +    if (!x_map || !y_map)
> +        return;
> +
> +    *x1 = *y1 = *x2 = *y2 = 0;
> +
> +    if (fields->fingers > 1) {
> +        for (i = 0;  i < 32; i++) {

Extra space after "i = 0;"

> +            if (x_map & (1 << i)) {
> +                x_bitmap.end_bit = priv->x_bits - i;
> +                break;
> +            }
> +        }
> +
> +        for (i = 31; i >= 0; i--) {
> +            if (x_map & (1 << i)) {
> +                x_bitmap.start_bit = priv->x_bits - i;
> +                break;
> +            }
> +        }
> +
> +        for (i = 0;  i < 32; i++) {

Same here

> +            if (y_map & (1 << i)) {
> +                y_bitmap.start_bit = i;
> +                break;
> +            }
> +        }

So, assuming x_bits == y_bits == 8, and both x_map and y_map are in
the range 0x00..0xff:

y_bitmap.* lies in the range 0..7
x_bitmap.* lies in the range 1..8

Is that correct, or is x_bitmap off by one?

> +        for (i = 31; i >= 0; i--) {
> +            if (y_map & (1 << i)) {
> +                y_bitmap.end_bit = i;
> +                break;
> +            }
> +        }

I suspect that this whole function could be simplified a bit.

Would it be possible to use ffs() and fls() instead of loops?

Just a personal preference, but if you calculated X first and then Y,
it may require fewer temporary variables and the alps_palm_bitmap
struct could be removed.

> +
> +        box_middle_x = (priv->x_max * (x_bitmap.start_bit + x_bitmap.end_bit)) /
> +                (2 * (priv->x_bits - 1));
> +        box_middle_y = (priv->y_max * (y_bitmap.start_bit + y_bitmap.end_bit)) /
> +                (2 * (priv->y_bits - 1));

These lines look a bit long - suggest running through checkpatch.pl

I assume start_bit and end_bit can never be negative, because the code
that calculates e.g. x_map will populate at most bits 0..(x_bits-1)?

> +        *x1 = fields->x;
> +        *y1 = fields->y;
> +        *x2 = 2 * box_middle_x - *x1;
> +        *y2 = 2 * box_middle_y - *y1;
> +    }
> +}
> +
> +/*
>   * Process bitmap data from v3 and v4 protocols. Returns the number of
>   * fingers detected. A return value of 0 means at least one of the
>   * bitmaps was empty.
> @@ -495,25 +564,55 @@ static void alps_decode_rushmore(struct alps_fields *f, unsigned char *p,
>  static void alps_decode_dolphin(struct alps_fields *f, unsigned char *p,
>                  struct psmouse *psmouse)
>  {
> +    unsigned int palm_high = 0, palm_low = 0, palm_mask = 0;
> +    int i, remain_xbits;
> +    struct alps_data *priv = psmouse->private;
> +
>      f->first_mp = !!(p[0] & 0x02);
>      f->is_mp = !!(p[0] & 0x20);
>
> -    f->fingers = ((p[0] & 0x6) >> 1 |
> +    if (!f->is_mp) {
> +        f->x = ((p[1] & 0x7f) | ((p[4] & 0x0f) << 7));
> +        f->y = ((p[2] & 0x7f) | ((p[4] & 0xf0) << 3));
> +        f->z = (p[0] & 4) ? 0 : p[5] & 0x7f;
> +        alps_decode_buttons_v3(f, p);
> +    } else {
> +        f->fingers = ((p[0] & 0x6) >> 1 |
>               (p[0] & 0x10) >> 2);
> -    f->x_map = ((p[2] & 0x60) >> 5) |
> -           ((p[4] & 0x7f) << 2) |
> -           ((p[5] & 0x7f) << 9) |
> -           ((p[3] & 0x07) << 16) |
> -           ((p[3] & 0x70) << 15) |
> -           ((p[0] & 0x01) << 22);
> -    f->y_map = (p[1] & 0x7f) |
> -           ((p[2] & 0x1f) << 7);
> -
> -    f->x = ((p[1] & 0x7f) | ((p[4] & 0x0f) << 7));
> -    f->y = ((p[2] & 0x7f) | ((p[4] & 0xf0) << 3));
> -    f->z = (p[0] & 4) ? 0 : p[5] & 0x7f;
>
> -    alps_decode_buttons_v3(f, p);
> +        palm_low = (p[1] & 0x7f) |
> +               ((p[2] & 0x7f) << 7) |
> +               ((p[4] & 0x7f) << 14) |
> +               ((p[5] & 0x7f) << 21) |
> +               ((p[3] & 0x07) << 28) | ((p[3] & 0x10) << 27);
> +        palm_high = ((p[3] & 0x60) >> 5) | ((p[0] & 0x01) << 2);
> +
> +        for (i = 0; i < priv->y_bits; i++)
> +            palm_mask |= 1 << i;
> +
> +        /* Y-profile is stored in P(0) to p(n), n = y_bits; */
> +        f->y_map = palm_low & palm_mask;

Suggest something like:

f->y_map = palm_low & (BIT(priv->y_bits) - 1);

> +
> +        /* X-profile is stored in p(n) to p(n+x_bits). */
> +        palm_mask = 0;
> +        for (i = priv->y_bits; i < 32 && i < priv->y_bits + priv->x_bits; i++)
> +            palm_mask |= 1 << i;
> +
> +        f->x_map = ((palm_low & (palm_mask)) >> priv->y_bits);

Likewise, maybe use:

f->x_map = (palm_low >> priv->y_bits) & (BIT(priv->x_bits) - 1);

(but double-check my math first)

> +
> +        /*
> +         * In some cases, palm_low's 32bit is not enough to save
> +         * all X&Y-profiles, we need to use palm_high.
> +         */
> +        remain_xbits = priv->x_bits + priv->y_bits - 32;
> +        if (remain_xbits > 0) {
> +            palm_mask = 0;
> +            for (i = 0; i < remain_xbits; i++)
> +                palm_mask |= 1 << i;
> +
> +            f->x_map |= ((palm_high & palm_mask) << (32 - priv->y_bits));

Would it be cleaner to just use a u64 for the palm data, instead of
splitting into low/high?  (If so you'll probably want to compile-test
a 32-bit kernel.)

> +        }
> +    }
>  }
>
>  static void alps_process_touchpad_packet_v3(struct psmouse *psmouse)
> @@ -745,6 +844,112 @@ static void alps_process_packet_v4(struct psmouse *psmouse)
>      input_sync(dev);
>  }
>
> +
> +static void alps_process_touchpad_packet_v5(struct psmouse *psmouse)
> +{
> +    struct alps_data *priv = psmouse->private;
> +    unsigned char *packet = psmouse->packet;
> +    struct input_dev *dev = psmouse->dev;
> +    int x1 = 0, y1 = 0, x2 = 0, y2 = 0;
> +    int fingers = 0;
> +    struct alps_fields f;
> +
> +    priv->decode_fields(&f, packet, psmouse);
> +
> +    /*
> +     * There's no single feature of touchpad position and bitmap packets
> +     * that can be used to distinguish between them. We rely on the fact
> +     * that a bitmap packet should always follow a position packet with
> +     * bit 6 of packet[4] set.
> +     */
> +    if (priv->multi_packet) {
> +        /*
> +         * Sometimes a position packet will indicate a multi-packet
> +         * sequence, but then what follows is another position
> +         * packet. Check for this, and when it happens process the
> +         * position packet as usual.
> +         */
> +        if (f.is_mp) {
> +            fingers = f.fingers;
> +            priv->decode_fields(&f, priv->multi_data, psmouse);
> +            alps_process_bitmap_dolphin(priv, &f, &x1, &y1,
> +                            &x2, &y2);

Aside from this clause, alps_process_touchpad_packet_v5() is virtually
identical to alps_process_touchpad_packet_v3().  It would be good to
find a way to reuse more of the code.

> +        } else {
> +            priv->multi_packet = 0;
> +        }
> +    }
> +
> +    /*
> +     * Bit 6 of byte 0 is not usually set in position packets. The only
> +     * times it seems to be set is in situations where the data is
> +     * suspect anyway, e.g. a palm resting flat on the touchpad. Given
> +     * this combined with the fact that this bit is useful for filtering
> +     * out misidentified bitmap packets, we reject anything with this
> +     * bit set.
> +     */
> +    if (f.is_mp)
> +        return;
> +
> +    if (!priv->multi_packet && f.first_mp) {
> +        priv->multi_packet = 1;
> +        memcpy(priv->multi_data, packet, sizeof(priv->multi_data));
> +        return;
> +    }
> +
> +    priv->multi_packet = 0;
> +
> +    /*
> +     * Sometimes the hardware sends a single packet with z = 0
> +     * in the middle of a stream. Real releases generate packets
> +     * with x, y, and z all zero, so these seem to be flukes.
> +     * Ignore them.
> +     */
> +    if (f.x && f.y && !f.z)
> +        return;
> +
> +    /*
> +     * If we don't have MT data or the bitmaps were empty, we have
> +     * to rely on ST data.
> +     */
> +    if (!fingers) {
> +        x1 = f.x;
> +        y1 = f.y;
> +        fingers = f.z > 0 ? 1 : 0;
> +    }
> +
> +    if (f.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_mt_report_finger_count(dev, 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);
> +
> +    if (f.z > 0) {
> +        input_report_abs(dev, ABS_X, f.x);
> +        input_report_abs(dev, ABS_Y, f.y);
> +    }
> +    input_report_abs(dev, ABS_PRESSURE, f.z);
> +
> +    input_sync(dev);
> +}
> +
> +static void alps_process_packet_v5(struct psmouse *psmouse)
> +{
> +    unsigned char *packet = psmouse->packet;
> +
> +    /* Ignore stick point data */
> +    if (packet[0] == 0xD8)
> +        return;
> +
> +    alps_process_touchpad_packet_v5(psmouse);
> +}
> +
>  static void alps_report_bare_ps2_packet(struct psmouse *psmouse,
>                      unsigned char packet[],
>                      bool report_buttons)
> @@ -1522,6 +1727,41 @@ error:
>      return -1;
>  }
>
> +static int alps_dolphin_get_device_area(struct psmouse *psmouse,
> +                    struct alps_data *priv)
> +{
> +    struct ps2dev *ps2dev = &psmouse->ps2dev;
> +    unsigned char param[4] = {0};
> +    int num_x_electrode, num_y_electrode;
> +
> +    if (alps_enter_command_mode(psmouse))
> +        return -1;
> +
> +    if (ps2_command(ps2dev, NULL, 0x00EC) ||
> +        ps2_command(ps2dev, NULL, 0x00F0) ||
> +        ps2_command(ps2dev, NULL, 0x00F0) ||
> +        ps2_command(ps2dev, NULL, 0x00F3) ||
> +        ps2_command(ps2dev, NULL, 0x000A) ||
> +        ps2_command(ps2dev, NULL, 0x00F3) ||
> +        ps2_command(ps2dev, NULL, 0x000A))
> +        return -1;
> +
> +    if (ps2_command(ps2dev, param, PSMOUSE_CMD_GETINFO))
> +        return -1;
> +
> +    num_x_electrode = DOLPHIN_PROFILE_XOFFSET + (param[2] & 0x0F);
> +    num_y_electrode = DOLPHIN_PROFILE_YOFFSET + ((param[2] >> 4) & 0x0F);

This is strictly optional, but adding a comment indicating typical
values for num_x_electrode and num_y_electrode could aid in
understanding the math.

> +    priv->x_bits = num_x_electrode;
> +    priv->y_bits = num_y_electrode;
> +    priv->x_max = (num_x_electrode - 1) * DOLPHIN_COUNT_PER_ELECTRODE;
> +    priv->y_max = (num_y_electrode - 1) * DOLPHIN_COUNT_PER_ELECTRODE;
> +
> +    if (alps_exit_command_mode(psmouse))
> +        return -1;
> +
> +    return 0;
> +}
> +
>  static int alps_hw_init_dolphin_v1(struct psmouse *psmouse)
>  {
>      struct ps2dev *ps2dev = &psmouse->ps2dev;
> @@ -1574,7 +1814,7 @@ static void alps_set_defaults(struct alps_data *priv)
>          break;
>      case ALPS_PROTO_V5:
>          priv->hw_init = alps_hw_init_dolphin_v1;
> -        priv->process_packet = alps_process_packet_v3;
> +        priv->process_packet = alps_process_packet_v5;
>          priv->decode_fields = alps_decode_dolphin;
>          priv->set_abs_params = alps_set_abs_params_mt;
>          priv->nibble_commands = alps_v3_nibble_commands;
> @@ -1648,11 +1888,13 @@ static int alps_identify(struct psmouse *psmouse, struct alps_data *priv)
>      if (alps_match_table(psmouse, priv, e7, ec) == 0) {
>          return 0;
>      } else if (e7[0] == 0x73 && e7[1] == 0x03 && e7[2] == 0x50 &&
> -           ec[0] == 0x73 && ec[1] == 0x01) {
> +           ec[0] == 0x73 && (ec[1] == 0x01 || ec[1] == 0x02)) {
>          priv->proto_version = ALPS_PROTO_V5;
>          alps_set_defaults(priv);
> -
> -        return 0;
> +        if (alps_dolphin_get_device_area(psmouse, priv))
> +            return -EIO;
> +        else
> +            return 0;
>      } else if (ec[0] == 0x88 && ec[1] == 0x08) {
>          priv->proto_version = ALPS_PROTO_V3;
>          alps_set_defaults(priv);
> diff --git a/drivers/input/mouse/alps.h b/drivers/input/mouse/alps.h
> index cdc10f3..43d89fc7 100644
> --- a/drivers/input/mouse/alps.h
> +++ b/drivers/input/mouse/alps.h
> @@ -18,6 +18,10 @@
>  #define ALPS_PROTO_V4    4
>  #define ALPS_PROTO_V5    5
>
> +#define DOLPHIN_COUNT_PER_ELECTRODE    64
> +#define DOLPHIN_PROFILE_XOFFSET        8    /* x-electrode offset */
> +#define DOLPHIN_PROFILE_YOFFSET        1    /* y-electrode offset */
> +
>  /**
>   * struct alps_model_info - touchpad ID table
>   * @signature: E7 response string to match.

Re: [PATCH v2 1/2] Add an additional argument for decode routine, change secondary device name

[Will Tommy]

Hi Kevin,

Thanks for your carefully review. It's amazing !

> So, assuming x_bits == y_bits == 8, and both x_map and y_map are in
> the range 0x00..0xff:
>
> y_bitmap.* lies in the range 0..7
> x_bitmap.* lies in the range 1..8
>
> Is that correct, or is x_bitmap off by one?

Hmm, it seems a bug of algorithm. Both x_bitmay&y_bitmap's range should be 0..7.
I'll fix it.

> I suspect that this whole function could be simplified a bit.
>
> Would it be possible to use ffs() and fls() instead of loops?
>
> Just a personal preference, but if you calculated X first and then Y,
> it may require fewer temporary variables and the alps_palm_bitmap
> struct could be removed.

Thanks for your proposal. I'll re-write this function in next submission.

> > +        box_middle_x = (priv->x_max * (x_bitmap.start_bit + x_bitmap.end_bit)) /
> > +                (2 * (priv->x_bits - 1));
> > +        box_middle_y = (priv->y_max * (y_bitmap.start_bit + y_bitmap.end_bit)) /
> > +                (2 * (priv->y_bits - 1));
> These lines look a bit long - suggest running through checkpatch.pl
>

Yes, they're longer than 80. However, in order not to let code hard to
be understood,
I decided not to split them. Anyway, since this function would be
re-written, there
would be no problem.

> I assume start_bit and end_bit can never be negative, because the code
> that calculates e.g. x_map will populate at most bits 0..(x_bits-1)?

Do you mean it's not good to define start_bit & end_bit as "int" since they have
no chance to be negative ? Actually, "unsigned char" is much better.

> Suggest something like:
>
> f->y_map = palm_low & (BIT(priv->y_bits) - 1);

> Likewise, maybe use:
>
> f->x_map = (palm_low >> priv->y_bits) & (BIT(priv->x_bits) - 1);
>
> (but double-check my math first)

> Would it be cleaner to just use a u64 for the palm data, instead of
> splitting into low/high?  (If so you'll probably want to compile-test
> a 32-bit kernel.)

> Aside from this clause, alps_process_touchpad_packet_v5() is virtually
> identical to alps_process_touchpad_packet_v3().  It would be good to
> find a way to reuse more of the code.

Okay, I'll take your advice and check the behavior on both 32-bit and 64-bit.

> This is strictly optional, but adding a comment indicating typical
> values for num_x_electrode and num_y_electrode could aid in
> understanding the math.

That's a good idea! I'll add them.

Tommy

Re: [PATCH v2 1/2] Add an additional argument for decode routine, change secondary device name

[Kevin Cernekee]

On Sun, Oct 13, 2013 at 10:50 PM, Will Tommy <tommywill2011@gmail.com> wrote:
>> I assume start_bit and end_bit can never be negative, because the code
>> that calculates e.g. x_map will populate at most bits 0..(x_bits-1)?
>
> Do you mean it's not good to define start_bit & end_bit as "int" since they have
> no chance to be negative ? Actually, "unsigned char" is much better.

No type change needed - I was just checking to make sure it wasn't a
risk, since getting a negative number in there would really throw off
the math...

Re: [PATCH v2 1/2] Add an additional argument for decode routine, change secondary device name

[Tommy Will]

2013/10/14 Kevin Cernekee <cernekee@gmail.com>:
> On Sun, Oct 13, 2013 at 10:50 PM, Will Tommy <tommywill2011@gmail.com> wrote:
>
> No type change needed - I was just checking to make sure it wasn't a
> risk, since getting a negative number in there would really throw off
> the math...

Got that. Thanks : )

Re: [PATCH v2 1/2] Add an additional argument for decode routine, change secondary device name

[Dmitry Torokhov]

Hi Yunkang,

On Fri, Sep 13, 2013 at 12:03:29PM +0800, Yunkang Tang wrote:
> From: Yunkang Tang <yunkang.tang@cn.alps.com>
> 
> - Add an addition argument for decode routine, new devices need this info.
> - Change the dev2's name from "PS/2 Mouse" to "ALPS PS/2 Device".
> 

I carved the name change device out of this patch and applied to my
3.13 queue.

Thanks.

-- 
Dmitry

Re: [PATCH v2 0/2] Improve the performance of alps v5-protocol's touchpad

[Justin Clift]

On 13/09/2013, at 5:03 AM, Yunkang Tang wrote:
> Hi all,
> 
> Here is the v2 of improving ALPS v5 protocol device.
> 
> Change since v1:
> - fix the issue that previous patches were broken by mail system.
> - split the modification to 2 patches.
> - Change dev2's name to "ALPS PS/2 Device"

Thanks Tommy.  We've been trying to get recent ALPS touchpads
working through Red Hat Bugzilla issue 953211.

  https://bugzilla.redhat.com/show_bug.cgi?id=953211

Niels de Vos, CC'd, was kind enough to build a Fedora 19 kernel
with your patch added.  We've been testing it since yesterday.

Your patch works well for several models of laptop (confirmed
success):

 * Dell Inspiron 17R SE / Inspiron 7720
 * Dell Vostro 3360
 * Fujitsu Lifebook AH532

With your patches, the touchpad:

* is recognised as a touchpad
* both KDE and Gnome can control it
* 2 finger scrolling works

That being said, we have conflicting reports about 1 finger
vertical scrolling on the Inspiron 17R SE / Inspiron 7720.

* 1 finger scrolling works on my laptop (Inspiron 17R SE),
  if 2 finger scrolling is NOT enabled in Gnome3 mouse &
  touchpad settings. (seems mutually exclusive)
* 1 finger scrolling does NOT work in KDE (Inspiron 7720)
  * I'm following this up with the reporter, as it may
    turn out to be a KDE settings issue

From my point of view, I'm be happy to see these patches
be applied, and you're welcome to use:

  Tested-by: Justin Clift <jclift@redhat.com>

Side note - Thanks to the people who've tested this and
reported back. :)

* Stanislav Datskevich
* Özgür Gündoğan
* Pentarh Udi
* Arnaud Lacombe
* Gaspard Jankowiak
* myself ;>

Regards and best wishes,

Justin Clift


> Best Regards,
> Tommy

--
Open Source and Standards @ Red Hat

twitter.com/realjustinclift

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