[PATCH] Input: hgpk - support GlideSensor and PenTablet modes

[PATCH] Input: hgpk - support GlideSensor and PenTablet modes

[Daniel Drake]

Add a "hgpk_mode" sysfs attribute that allows selection between 3 options:
Mouse (the existing option), GlideSensor and PenTablet.

GlideSensor is an enhanced protocol for the regular touchpad mode that
additionally reports pressure and uses absolute coordinates. We suspect
that it may be more reliable than mouse mode in some environments.

PenTablet mode puts the touchpad into resistive mode, you must then use
a stylus as an input. We suspect this is the most reliable way to drive
the touchpad.

After changing the mode you must request a rescan for the change to take
effect:
	echo -n rescan > /sys/bus/serio/devices/serio1/drvctl

The GlideSensor and PenTablet devices expose themselves with the
intention of being combined with the synaptics X11 input driver.

Based on earlier work by Paul Fox.

Signed-off-by: Daniel Drake <dsd@laptop.org>
---
 drivers/input/mouse/hgpk.c |  315 +++++++++++++++++++++++++++++++++++++++++---
 drivers/input/mouse/hgpk.h |   11 ++
 2 files changed, 307 insertions(+), 19 deletions(-)

Dmitry: thanks for pointing out that the synaptics X11 driver is no longer
only for synaptics hardware, that was the solution I was missing.

I'll rebase and resubmit the other patches in the original series once
this one has passed review and merge.

diff --git a/drivers/input/mouse/hgpk.c b/drivers/input/mouse/hgpk.c
index 1d2205b..6115e0d 100644
--- a/drivers/input/mouse/hgpk.c
+++ b/drivers/input/mouse/hgpk.c
@@ -69,6 +69,13 @@ module_param(post_interrupt_delay, int, 0644);
 MODULE_PARM_DESC(post_interrupt_delay,
 	"delay (ms) before recal after recal interrupt detected");
 
+int hgpk_mode = HGPK_MODE_MOUSE;
+static const char * const mode_names[] = {
+	[HGPK_MODE_MOUSE] = "Mouse",
+	[HGPK_MODE_GLIDESENSOR] = "GlideSensor",
+	[HGPK_MODE_PENTABLET] = "PenTablet",
+};
+
 /*
  * When the touchpad gets ultra-sensitive, one can keep their finger 1/2"
  * above the pad and still have it send packets.  This causes a jump cursor
@@ -143,23 +150,137 @@ static void hgpk_spewing_hack(struct psmouse *psmouse,
  * swr/swl are the left/right buttons.
  * x-neg/y-neg are the x and y delta negative bits
  * x-over/y-over are the x and y overflow bits
+ *
+ * ---
+ *
+ * HGPK Advanced Mode - single-mode format
+ *
+ * byte 0(PT):  1    1    0    0    1    1     1     1
+ * byte 0(GS):  1    1    1    1    1    1     1     1
+ * byte 1:      0   x6   x5   x4   x3   x2    x1    x0
+ * byte 2(PT):  0    0   x9   x8   x7    ? pt-dsw    0
+ * byte 2(GS):  0  x10   x9   x8   x7    ? gs-dsw pt-dsw
+ * byte 3:      0   y9   y8   y7    1    0   swr   swl
+ * byte 4:      0   y6   y5   y4   y3   y2    y1    y0
+ * byte 5:      0   z6   z5   z4   z3   z2    z1    z0
+ *
+ * ?'s are not defined in the protocol spec, may vary between models.
+ *
+ * swr/swl are the left/right buttons.
+ *
+ * pt-dsw/gs-dsw indicate that the pt/gs sensor is detecting a
+ * pen/finger
  */
-static int hgpk_validate_byte(unsigned char *packet)
+static int hgpk_validate_byte(struct psmouse *psmouse, unsigned char *packet)
 {
-	return (packet[0] & 0x0C) != 0x08;
+	struct hgpk_data *priv = psmouse->private;
+	int pktcnt = psmouse->pktcnt;
+	int r = 0;
+
+	switch (priv->mode) {
+	case HGPK_MODE_MOUSE:
+		r = (packet[0] & 0x0C) != 0x08;
+		if (r)
+			hgpk_dbg(psmouse, "bad data (%d) %02x %02x %02x\n",
+				 psmouse->pktcnt, psmouse->packet[0],
+				 psmouse->packet[1], psmouse->packet[2]);
+		break;
+
+	case HGPK_MODE_GLIDESENSOR:
+	case HGPK_MODE_PENTABLET:
+		/* bytes 2 - 6 should have 0 in the highest bit */
+		if (pktcnt >= 2 && pktcnt <= 6 && (packet[pktcnt - 1] & 0x80))
+			r = -1;
+		if (priv->mode == HGPK_MODE_GLIDESENSOR && packet[0] != HGPK_GS)
+			r = -1;
+		if (priv->mode == HGPK_MODE_PENTABLET && packet[0] != HGPK_PT)
+			r = -1;
+		if (r)
+			hgpk_dbg(psmouse, "bad data, mode %d (%d) "
+				 "%02x %02x %02x %02x %02x %02x\n",
+				 priv->mode, psmouse->pktcnt,
+				 psmouse->packet[0], psmouse->packet[1],
+				 psmouse->packet[2], psmouse->packet[3],
+				 psmouse->packet[4], psmouse->packet[5]);
+		break;
+	}
+	return r;
 }
 
-static void hgpk_process_packet(struct psmouse *psmouse)
+static void hgpk_process_advanced_packet(struct psmouse *psmouse)
 {
-	struct input_dev *dev = psmouse->dev;
+	struct hgpk_data *priv = psmouse->private;
+	struct input_dev *idev = psmouse->dev;
 	unsigned char *packet = psmouse->packet;
-	int x, y, left, right;
+	int left = !!(packet[3] & 1);
+	int right = !!(packet[3] & 2);
+	int x = packet[1] | ((packet[2] & 0x78) << 4);
+	int y = packet[4] | ((packet[3] & 0x70) << 3);
+	int z = packet[5];
+	int down;
+
+	if (priv->mode == HGPK_MODE_GLIDESENSOR) {
+		int pt_down = !!(packet[2] & 1);
+		int finger_down = !!(packet[2] & 2);
+
+		BUG_ON(packet[0] == HGPK_PT);
+		input_report_abs(idev, ABS_PRESSURE, z);
+		down = finger_down;
+		if (tpdebug)
+			hgpk_dbg(psmouse, "pd=%d fd=%d ",
+				 pt_down, finger_down);
+	} else {
+		BUG_ON(packet[0] == HGPK_GS);
+		down = !!(packet[2] & 2);
+		if (tpdebug)
+			hgpk_dbg(psmouse, "pd=%d ", down);
+	}
 
-	left = packet[0] & 1;
-	right = (packet[0] >> 1) & 1;
+	if (tpdebug)
+		hgpk_dbg(psmouse, "l=%d r=%d x=%d y=%d z=%d\n",
+			 left, right, x, y, z);
 
-	x = packet[1] - ((packet[0] << 4) & 0x100);
-	y = ((packet[0] << 3) & 0x100) - packet[2];
+	input_report_key(idev, BTN_TOUCH, down);
+	input_report_key(idev, BTN_LEFT, left);
+	input_report_key(idev, BTN_RIGHT, right);
+
+	/*
+	 * if this packet says that the finger was removed, reset our position
+	 * tracking so that we don't erroneously detect a jump on next press.
+	 */
+	if (!down)
+		priv->abs_x = priv->abs_y = -1;
+
+	/* Report position if finger/pen is down, but weed out duplicate
+	 * packets (we get quite a few in this mode, and they mess up our
+	 * jump detection */
+	if (down && (x != priv->abs_x || y != priv->abs_y)) {
+
+		/* Don't apply hacks in PT mode, it seems reliable */
+		if (priv->mode != HGPK_MODE_PENTABLET && priv->abs_x != -1) {
+			hgpk_jumpy_hack(psmouse,
+					priv->abs_x - x, priv->abs_y - y);
+			hgpk_spewing_hack(psmouse, left, right,
+					  priv->abs_x - x, priv->abs_y - y);
+		}
+
+		input_report_abs(idev, ABS_X, x);
+		input_report_abs(idev, ABS_Y, y);
+		priv->abs_x = x;
+		priv->abs_y = y;
+	}
+
+	input_sync(idev);
+}
+
+static void hgpk_process_simple_packet(struct psmouse *psmouse)
+{
+	struct input_dev *dev = psmouse->dev;
+	unsigned char *packet = psmouse->packet;
+	int left = packet[0] & 1;
+	int right = (packet[0] >> 1) & 1;
+	int x = packet[1] - ((packet[0] << 4) & 0x100);
+	int y = ((packet[0] << 3) & 0x100) - packet[2];
 
 	hgpk_jumpy_hack(psmouse, x, y);
 	hgpk_spewing_hack(psmouse, left, right, x, y);
@@ -180,15 +301,14 @@ static psmouse_ret_t hgpk_process_byte(struct psmouse *psmouse)
 {
 	struct hgpk_data *priv = psmouse->private;
 
-	if (hgpk_validate_byte(psmouse->packet)) {
-		hgpk_dbg(psmouse, "%s: (%d) %02x %02x %02x\n",
-				__func__, psmouse->pktcnt, psmouse->packet[0],
-				psmouse->packet[1], psmouse->packet[2]);
+	if (hgpk_validate_byte(psmouse, psmouse->packet))
 		return PSMOUSE_BAD_DATA;
-	}
 
 	if (psmouse->pktcnt >= psmouse->pktsize) {
-		hgpk_process_packet(psmouse);
+		if (priv->mode == HGPK_MODE_MOUSE)
+			hgpk_process_simple_packet(psmouse);
+		else
+			hgpk_process_advanced_packet(psmouse);
 		return PSMOUSE_FULL_PACKET;
 	}
 
@@ -210,6 +330,59 @@ static psmouse_ret_t hgpk_process_byte(struct psmouse *psmouse)
 	return PSMOUSE_GOOD_DATA;
 }
 
+static int hgpk_select_mode(struct psmouse *psmouse)
+{
+	struct ps2dev *ps2dev = &psmouse->ps2dev;
+	struct hgpk_data *priv = psmouse->private;
+	int i;
+	int cmd;
+
+	/*
+	 * 4 disables to enable advanced mode
+	 * then 3 0xf2 bytes as the preamble for GS/PT selection
+	 */
+	const int advanced_init[] = {
+		PSMOUSE_CMD_DISABLE, PSMOUSE_CMD_DISABLE,
+		PSMOUSE_CMD_DISABLE, PSMOUSE_CMD_DISABLE,
+		0xf2, 0xf2, 0xf2,
+	};
+
+	switch (priv->mode) {
+	case HGPK_MODE_MOUSE:
+		psmouse->pktsize = 3;
+		break;
+
+	case HGPK_MODE_GLIDESENSOR:
+	case HGPK_MODE_PENTABLET:
+		psmouse->pktsize = 6;
+
+		/* Switch to 'Advanced mode.', four disables in a row. */
+		for (i = 0; i < ARRAY_SIZE(advanced_init); i++)
+			if (ps2_command(ps2dev, NULL, advanced_init[i]))
+				return -EIO;
+
+		/* select between GlideSensor (mouse) or PenTablet */
+		if (priv->mode == HGPK_MODE_GLIDESENSOR)
+			cmd = PSMOUSE_CMD_SETSCALE11;
+		else
+			cmd = PSMOUSE_CMD_SETSCALE21;
+
+		if (ps2_command(ps2dev, NULL, cmd))
+			return -EIO;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static void reset_hack_state(struct psmouse *psmouse)
+{
+	struct hgpk_data *priv = psmouse->private;
+	priv->abs_x = priv->abs_y = -1;
+}
+
 static int hgpk_force_recalibrate(struct psmouse *psmouse)
 {
 	struct ps2dev *ps2dev = &psmouse->ps2dev;
@@ -236,6 +409,13 @@ static int hgpk_force_recalibrate(struct psmouse *psmouse)
 	/* according to ALPS, 150mS is required for recalibration */
 	msleep(150);
 
+	if (hgpk_select_mode(psmouse)) {
+		hgpk_err(psmouse, "failed to select mode\n");
+		return -1;
+	}
+
+	reset_hack_state(psmouse);
+
 	/* XXX: If a finger is down during this delay, recalibration will
 	 * detect capacitance incorrectly.  This is a hardware bug, and
 	 * we don't have a good way to deal with it.  The 2s window stuff
@@ -290,6 +470,13 @@ static int hgpk_toggle_power(struct psmouse *psmouse, int enable)
 
 		psmouse_reset(psmouse);
 
+		if (hgpk_select_mode(psmouse)) {
+			hgpk_err(psmouse, "Failed to select mode!\n");
+			return -1;
+		}
+
+		reset_hack_state(psmouse);
+
 		/* should be all set, enable the touchpad */
 		ps2_command(&psmouse->ps2dev, NULL, PSMOUSE_CMD_ENABLE);
 		psmouse_set_state(psmouse, PSMOUSE_ACTIVATED);
@@ -328,7 +515,12 @@ static int hgpk_reconnect(struct psmouse *psmouse)
 			return 0;
 
 	psmouse_reset(psmouse);
+	if (hgpk_select_mode(psmouse)) {
+		hgpk_err(psmouse, "Failed to select mode!\n");
+		return -1;
+	}
 
+	reset_hack_state(psmouse);
 	return 0;
 }
 
@@ -366,6 +558,35 @@ static ssize_t hgpk_set_powered(struct psmouse *psmouse, void *data,
 __PSMOUSE_DEFINE_ATTR(powered, S_IWUSR | S_IRUGO, NULL,
 		      hgpk_show_powered, hgpk_set_powered, false);
 
+static ssize_t attr_show_mode(struct psmouse *psmouse, void *data, char *buf)
+{
+	return sprintf(buf, "%s\n", mode_names[hgpk_mode]);
+}
+
+static ssize_t attr_set_mode(struct psmouse *psmouse, void *data,
+			     const char *buf, size_t len)
+{
+	int i;
+	int new_mode = -1;
+
+	for (i = 0; i < ARRAY_SIZE(mode_names); i++) {
+		const char *name = mode_names[i];
+		if (strlen(name) == len && !strncasecmp(name, buf, len)) {
+			new_mode = i;
+			break;
+		}
+	}
+
+	if (new_mode == -1)
+		return -EINVAL;
+
+	hgpk_mode = new_mode;
+	return len;
+}
+
+__PSMOUSE_DEFINE_ATTR(hgpk_mode, S_IWUSR | S_IRUGO, NULL,
+		      attr_show_mode, attr_set_mode, 0);
+
 static ssize_t hgpk_trigger_recal_show(struct psmouse *psmouse,
 		void *data, char *buf)
 {
@@ -401,6 +622,8 @@ static void hgpk_disconnect(struct psmouse *psmouse)
 
 	device_remove_file(&psmouse->ps2dev.serio->dev,
 			   &psmouse_attr_powered.dattr);
+	device_remove_file(&psmouse->ps2dev.serio->dev,
+			   &psmouse_attr_hgpk_mode.dattr);
 
 	if (psmouse->model >= HGPK_MODEL_C)
 		device_remove_file(&psmouse->ps2dev.serio->dev,
@@ -424,6 +647,8 @@ static void hgpk_recalib_work(struct work_struct *work)
 
 static int hgpk_register(struct psmouse *psmouse)
 {
+	struct hgpk_data *priv = psmouse->private;
+	struct input_dev *idev = psmouse->dev;
 	int err;
 
 	/* register handlers */
@@ -431,13 +656,45 @@ static int hgpk_register(struct psmouse *psmouse)
 	psmouse->poll = hgpk_poll;
 	psmouse->disconnect = hgpk_disconnect;
 	psmouse->reconnect = hgpk_reconnect;
-	psmouse->pktsize = 3;
 
 	/* Disable the idle resync. */
 	psmouse->resync_time = 0;
 	/* Reset after a lot of bad bytes. */
 	psmouse->resetafter = 1024;
 
+	if (priv->mode != HGPK_MODE_MOUSE) {
+		__set_bit(EV_ABS, idev->evbit);
+		__set_bit(EV_KEY, idev->evbit);
+		__set_bit(BTN_TOUCH, idev->keybit);
+		__set_bit(BTN_TOOL_FINGER, idev->keybit);
+		__set_bit(BTN_LEFT, idev->keybit);
+		__set_bit(BTN_RIGHT, idev->keybit);
+		__clear_bit(EV_REL, idev->evbit);
+		__clear_bit(REL_X, idev->relbit);
+		__clear_bit(REL_Y, idev->relbit);
+	}
+
+	if (priv->mode == HGPK_MODE_GLIDESENSOR) {
+		/* GlideSensor has pressure sensor, PenTablet does not */
+		input_set_abs_params(idev, ABS_PRESSURE, 0, 15, 0, 0);
+
+		/* From device specs */
+		input_set_abs_params(idev, ABS_X, 0, 399, 0, 0);
+		input_set_abs_params(idev, ABS_Y, 0, 290, 0, 0);
+
+		/* Calculated by hand based on usable size (52mm x 38mm) */
+		input_abs_set_res(idev, ABS_X, 8);
+		input_abs_set_res(idev, ABS_Y, 8);
+	} else if (priv->mode == HGPK_MODE_PENTABLET) {
+		/* From device specs */
+		input_set_abs_params(idev, ABS_X, 0, 999, 0, 0);
+		input_set_abs_params(idev, ABS_Y, 5, 239, 0, 0);
+
+		/* Calculated by hand based on usable size (156mm x 38mm) */
+		input_abs_set_res(idev, ABS_X, 6);
+		input_abs_set_res(idev, ABS_Y, 8);
+	}
+
 	err = device_create_file(&psmouse->ps2dev.serio->dev,
 				 &psmouse_attr_powered.dattr);
 	if (err) {
@@ -445,6 +702,13 @@ static int hgpk_register(struct psmouse *psmouse)
 		return err;
 	}
 
+	err = device_create_file(&psmouse->ps2dev.serio->dev,
+				 &psmouse_attr_hgpk_mode.dattr);
+	if (err) {
+		hgpk_err(psmouse, "Failed creating 'hgpk_mode' sysfs node\n");
+		goto err_remove_powered;
+	}
+
 	/* C-series touchpads added the recalibrate command */
 	if (psmouse->model >= HGPK_MODEL_C) {
 		err = device_create_file(&psmouse->ps2dev.serio->dev,
@@ -452,13 +716,19 @@ static int hgpk_register(struct psmouse *psmouse)
 		if (err) {
 			hgpk_err(psmouse,
 				"Failed creating 'recalibrate' sysfs node\n");
-			device_remove_file(&psmouse->ps2dev.serio->dev,
-					&psmouse_attr_powered.dattr);
-			return err;
+			goto err_remove_mode;
 		}
 	}
 
 	return 0;
+
+err_remove_mode:
+	device_remove_file(&psmouse->ps2dev.serio->dev,
+			   &psmouse_attr_hgpk_mode.dattr);
+err_remove_powered:
+	device_remove_file(&psmouse->ps2dev.serio->dev,
+			   &psmouse_attr_powered.dattr);
+	return err;
 }
 
 int hgpk_init(struct psmouse *psmouse)
@@ -473,12 +743,19 @@ int hgpk_init(struct psmouse *psmouse)
 	psmouse->private = priv;
 	priv->psmouse = psmouse;
 	priv->powered = true;
+	priv->mode = hgpk_mode;
 	INIT_DELAYED_WORK(&priv->recalib_wq, hgpk_recalib_work);
 
 	err = psmouse_reset(psmouse);
 	if (err)
 		goto init_fail;
 
+	err = hgpk_select_mode(psmouse);
+	if (err)
+		goto init_fail;
+
+	reset_hack_state(psmouse);
+
 	err = hgpk_register(psmouse);
 	if (err)
 		goto init_fail;
diff --git a/drivers/input/mouse/hgpk.h b/drivers/input/mouse/hgpk.h
index d61cfd3..430f29f 100644
--- a/drivers/input/mouse/hgpk.h
+++ b/drivers/input/mouse/hgpk.h
@@ -5,6 +5,9 @@
 #ifndef _HGPK_H
 #define _HGPK_H
 
+#define HGPK_GS		0xff       /* The GlideSensor */
+#define HGPK_PT		0xcf       /* The PenTablet */
+
 enum hgpk_model_t {
 	HGPK_MODEL_PREA = 0x0a,	/* pre-B1s */
 	HGPK_MODEL_A = 0x14,	/* found on B1s, PT disabled in hardware */
@@ -13,12 +16,20 @@ enum hgpk_model_t {
 	HGPK_MODEL_D = 0x50,	/* C1, mass production */
 };
 
+enum hgpk_mode {
+	HGPK_MODE_MOUSE,
+	HGPK_MODE_GLIDESENSOR,
+	HGPK_MODE_PENTABLET,
+};
+
 struct hgpk_data {
 	struct psmouse *psmouse;
+	int mode;
 	bool powered;
 	int count, x_tally, y_tally;	/* hardware workaround stuff */
 	unsigned long recalib_window;
 	struct delayed_work recalib_wq;
+	int abs_x, abs_y;
 };
 
 #define hgpk_dbg(psmouse, format, arg...)		\
-- 
1.7.2.3

Re: [PATCH] Input: hgpk - support GlideSensor and PenTablet modes

[Dmitry Torokhov]

Hi Daniel,

On Wed, Oct 06, 2010 at 04:50:26PM +0100, Daniel Drake wrote:
> Add a "hgpk_mode" sysfs attribute that allows selection between 3 options:
> Mouse (the existing option), GlideSensor and PenTablet.
> 
> GlideSensor is an enhanced protocol for the regular touchpad mode that
> additionally reports pressure and uses absolute coordinates. We suspect
> that it may be more reliable than mouse mode in some environments.
> 
> PenTablet mode puts the touchpad into resistive mode, you must then use
> a stylus as an input. We suspect this is the most reliable way to drive
> the touchpad.
> 
> After changing the mode you must request a rescan for the change to take
> effect:
> 	echo -n rescan > /sys/bus/serio/devices/serio1/drvctl
> 

I think we can do better than that and re-create the device for users
ourselves. See drivers/input/mouse/psmouse_base.c::psmouse_attr_set_proto()

I would also look into adding a module parameter so that users could
specify desired mode on bootup.

> The GlideSensor and PenTablet devices expose themselves with the
> intention of being combined with the synaptics X11 input driver.
> 
> Based on earlier work by Paul Fox.
> 
> Signed-off-by: Daniel Drake <dsd@laptop.org>
> ---
>  drivers/input/mouse/hgpk.c |  315 +++++++++++++++++++++++++++++++++++++++++---
>  drivers/input/mouse/hgpk.h |   11 ++
>  2 files changed, 307 insertions(+), 19 deletions(-)
> 
> Dmitry: thanks for pointing out that the synaptics X11 driver is no longer
> only for synaptics hardware, that was the solution I was missing.
> 
> I'll rebase and resubmit the other patches in the original series once
> this one has passed review and merge.
> 
> diff --git a/drivers/input/mouse/hgpk.c b/drivers/input/mouse/hgpk.c
> index 1d2205b..6115e0d 100644
> --- a/drivers/input/mouse/hgpk.c
> +++ b/drivers/input/mouse/hgpk.c
> @@ -69,6 +69,13 @@ module_param(post_interrupt_delay, int, 0644);
>  MODULE_PARM_DESC(post_interrupt_delay,
>  	"delay (ms) before recal after recal interrupt detected");
>  
> +int hgpk_mode = HGPK_MODE_MOUSE;

static?

> +static const char * const mode_names[] = {
> +	[HGPK_MODE_MOUSE] = "Mouse",
> +	[HGPK_MODE_GLIDESENSOR] = "GlideSensor",
> +	[HGPK_MODE_PENTABLET] = "PenTablet",
> +};
> +
>  /*
>   * When the touchpad gets ultra-sensitive, one can keep their finger 1/2"
>   * above the pad and still have it send packets.  This causes a jump cursor
> @@ -143,23 +150,137 @@ static void hgpk_spewing_hack(struct psmouse *psmouse,
>   * swr/swl are the left/right buttons.
>   * x-neg/y-neg are the x and y delta negative bits
>   * x-over/y-over are the x and y overflow bits
> + *
> + * ---
> + *
> + * HGPK Advanced Mode - single-mode format
> + *
> + * byte 0(PT):  1    1    0    0    1    1     1     1
> + * byte 0(GS):  1    1    1    1    1    1     1     1
> + * byte 1:      0   x6   x5   x4   x3   x2    x1    x0
> + * byte 2(PT):  0    0   x9   x8   x7    ? pt-dsw    0
> + * byte 2(GS):  0  x10   x9   x8   x7    ? gs-dsw pt-dsw
> + * byte 3:      0   y9   y8   y7    1    0   swr   swl
> + * byte 4:      0   y6   y5   y4   y3   y2    y1    y0
> + * byte 5:      0   z6   z5   z4   z3   z2    z1    z0
> + *
> + * ?'s are not defined in the protocol spec, may vary between models.
> + *
> + * swr/swl are the left/right buttons.
> + *
> + * pt-dsw/gs-dsw indicate that the pt/gs sensor is detecting a
> + * pen/finger
>   */
> -static int hgpk_validate_byte(unsigned char *packet)
> +static int hgpk_validate_byte(struct psmouse *psmouse, unsigned char *packet)

Why don't you make it a 'bool' and call something hgpk_is_byte_valid?
'int' is better suited when you have multiple potential errors to
signal.

>  {
> -	return (packet[0] & 0x0C) != 0x08;
> +	struct hgpk_data *priv = psmouse->private;
> +	int pktcnt = psmouse->pktcnt;
> +	int r = 0;
> +
> +	switch (priv->mode) {
> +	case HGPK_MODE_MOUSE:
> +		r = (packet[0] & 0x0C) != 0x08;
> +		if (r)
> +			hgpk_dbg(psmouse, "bad data (%d) %02x %02x %02x\n",
> +				 psmouse->pktcnt, psmouse->packet[0],
> +				 psmouse->packet[1], psmouse->packet[2]);
> +		break;
> +
> +	case HGPK_MODE_GLIDESENSOR:
> +	case HGPK_MODE_PENTABLET:
> +		/* bytes 2 - 6 should have 0 in the highest bit */
> +		if (pktcnt >= 2 && pktcnt <= 6 && (packet[pktcnt - 1] & 0x80))
> +			r = -1;
> +		if (priv->mode == HGPK_MODE_GLIDESENSOR && packet[0] != HGPK_GS)
> +			r = -1;
> +		if (priv->mode == HGPK_MODE_PENTABLET && packet[0] != HGPK_PT)
> +			r = -1;
> +		if (r)
> +			hgpk_dbg(psmouse, "bad data, mode %d (%d) "
> +				 "%02x %02x %02x %02x %02x %02x\n",
> +				 priv->mode, psmouse->pktcnt,
> +				 psmouse->packet[0], psmouse->packet[1],
> +				 psmouse->packet[2], psmouse->packet[3],
> +				 psmouse->packet[4], psmouse->packet[5]);
> +		break;
> +	}
> +	return r;
>  }
>  
> -static void hgpk_process_packet(struct psmouse *psmouse)
> +static void hgpk_process_advanced_packet(struct psmouse *psmouse)
>  {
> -	struct input_dev *dev = psmouse->dev;
> +	struct hgpk_data *priv = psmouse->private;
> +	struct input_dev *idev = psmouse->dev;
>  	unsigned char *packet = psmouse->packet;
> -	int x, y, left, right;
> +	int left = !!(packet[3] & 1);
> +	int right = !!(packet[3] & 2);
> +	int x = packet[1] | ((packet[2] & 0x78) << 4);
> +	int y = packet[4] | ((packet[3] & 0x70) << 3);
> +	int z = packet[5];
> +	int down;
> +
> +	if (priv->mode == HGPK_MODE_GLIDESENSOR) {
> +		int pt_down = !!(packet[2] & 1);
> +		int finger_down = !!(packet[2] & 2);
> +
> +		BUG_ON(packet[0] == HGPK_PT);
i
That is heavy-handed. I'd not BUG() on data coming from the hardware.

> +		input_report_abs(idev, ABS_PRESSURE, z);
> +		down = finger_down;
> +		if (tpdebug)
> +			hgpk_dbg(psmouse, "pd=%d fd=%d ",
> +				 pt_down, finger_down);
> +	} else {
> +		BUG_ON(packet[0] == HGPK_GS);

Same here, warn but not BUG().

BTW, why don't we report pressure in this case?

> +		down = !!(packet[2] & 2);
> +		if (tpdebug)
> +			hgpk_dbg(psmouse, "pd=%d ", down);
> +	}
>  
> -	left = packet[0] & 1;
> -	right = (packet[0] >> 1) & 1;
> +	if (tpdebug)
> +		hgpk_dbg(psmouse, "l=%d r=%d x=%d y=%d z=%d\n",
> +			 left, right, x, y, z);
>  
> -	x = packet[1] - ((packet[0] << 4) & 0x100);
> -	y = ((packet[0] << 3) & 0x100) - packet[2];
> +	input_report_key(idev, BTN_TOUCH, down);
> +	input_report_key(idev, BTN_LEFT, left);
> +	input_report_key(idev, BTN_RIGHT, right);
> +
> +	/*
> +	 * if this packet says that the finger was removed, reset our position
> +	 * tracking so that we don't erroneously detect a jump on next press.
> +	 */
> +	if (!down)
> +		priv->abs_x = priv->abs_y = -1;
> +
> +	/* Report position if finger/pen is down, but weed out duplicate
> +	 * packets (we get quite a few in this mode, and they mess up our
> +	 * jump detection */
> +	if (down && (x != priv->abs_x || y != priv->abs_y)) {
> +
> +		/* Don't apply hacks in PT mode, it seems reliable */
> +		if (priv->mode != HGPK_MODE_PENTABLET && priv->abs_x != -1) {
> +			hgpk_jumpy_hack(psmouse,
> +					priv->abs_x - x, priv->abs_y - y);
> +			hgpk_spewing_hack(psmouse, left, right,
> +					  priv->abs_x - x, priv->abs_y - y);

I wonder if instead of deltas we could detect "super-sensitivity" based
on Z?

> +		}
> +
> +		input_report_abs(idev, ABS_X, x);
> +		input_report_abs(idev, ABS_Y, y);
> +		priv->abs_x = x;
> +		priv->abs_y = y;
> +	}
> +
> +	input_sync(idev);
> +}
> +
> +static void hgpk_process_simple_packet(struct psmouse *psmouse)
> +{
> +	struct input_dev *dev = psmouse->dev;
> +	unsigned char *packet = psmouse->packet;
> +	int left = packet[0] & 1;
> +	int right = (packet[0] >> 1) & 1;
> +	int x = packet[1] - ((packet[0] << 4) & 0x100);
> +	int y = ((packet[0] << 3) & 0x100) - packet[2];
>  
>  	hgpk_jumpy_hack(psmouse, x, y);
>  	hgpk_spewing_hack(psmouse, left, right, x, y);
> @@ -180,15 +301,14 @@ static psmouse_ret_t hgpk_process_byte(struct psmouse *psmouse)
>  {
>  	struct hgpk_data *priv = psmouse->private;
>  
> -	if (hgpk_validate_byte(psmouse->packet)) {
> -		hgpk_dbg(psmouse, "%s: (%d) %02x %02x %02x\n",
> -				__func__, psmouse->pktcnt, psmouse->packet[0],
> -				psmouse->packet[1], psmouse->packet[2]);
> +	if (hgpk_validate_byte(psmouse, psmouse->packet))
>  		return PSMOUSE_BAD_DATA;
> -	}
>  
>  	if (psmouse->pktcnt >= psmouse->pktsize) {
> -		hgpk_process_packet(psmouse);
> +		if (priv->mode == HGPK_MODE_MOUSE)
> +			hgpk_process_simple_packet(psmouse);
> +		else
> +			hgpk_process_advanced_packet(psmouse);
>  		return PSMOUSE_FULL_PACKET;
>  	}
>  
> @@ -210,6 +330,59 @@ static psmouse_ret_t hgpk_process_byte(struct psmouse *psmouse)
>  	return PSMOUSE_GOOD_DATA;
>  }
>  
> +static int hgpk_select_mode(struct psmouse *psmouse)
> +{
> +	struct ps2dev *ps2dev = &psmouse->ps2dev;
> +	struct hgpk_data *priv = psmouse->private;
> +	int i;
> +	int cmd;
> +
> +	/*
> +	 * 4 disables to enable advanced mode
> +	 * then 3 0xf2 bytes as the preamble for GS/PT selection
> +	 */
> +	const int advanced_init[] = {
> +		PSMOUSE_CMD_DISABLE, PSMOUSE_CMD_DISABLE,
> +		PSMOUSE_CMD_DISABLE, PSMOUSE_CMD_DISABLE,
> +		0xf2, 0xf2, 0xf2,
> +	};
> +
> +	switch (priv->mode) {
> +	case HGPK_MODE_MOUSE:
> +		psmouse->pktsize = 3;
> +		break;
> +
> +	case HGPK_MODE_GLIDESENSOR:
> +	case HGPK_MODE_PENTABLET:
> +		psmouse->pktsize = 6;
> +
> +		/* Switch to 'Advanced mode.', four disables in a row. */
> +		for (i = 0; i < ARRAY_SIZE(advanced_init); i++)
> +			if (ps2_command(ps2dev, NULL, advanced_init[i]))
> +				return -EIO;
> +
> +		/* select between GlideSensor (mouse) or PenTablet */
> +		if (priv->mode == HGPK_MODE_GLIDESENSOR)
> +			cmd = PSMOUSE_CMD_SETSCALE11;
> +		else
> +			cmd = PSMOUSE_CMD_SETSCALE21;
> +

		cmd = priv->mode == HGPK_MODE_GLIDESENSOR ?
			PSMOUSE_CMD_SETSCALE11 : PSMOUSE_CMD_SETSCALE21;
?

> +		if (ps2_command(ps2dev, NULL, cmd))
> +			return -EIO;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static void reset_hack_state(struct psmouse *psmouse)

hgpk_reset_hack_state()

> +{
> +	struct hgpk_data *priv = psmouse->private;

Empty space?

> +	priv->abs_x = priv->abs_y = -1;
> +}
> +
>  static int hgpk_force_recalibrate(struct psmouse *psmouse)
>  {
>  	struct ps2dev *ps2dev = &psmouse->ps2dev;
> @@ -236,6 +409,13 @@ static int hgpk_force_recalibrate(struct psmouse *psmouse)
>  	/* according to ALPS, 150mS is required for recalibration */
>  	msleep(150);
>  
> +	if (hgpk_select_mode(psmouse)) {
> +		hgpk_err(psmouse, "failed to select mode\n");
> +		return -1;
> +	}
> +
> +	reset_hack_state(psmouse);
> +
>  	/* XXX: If a finger is down during this delay, recalibration will
>  	 * detect capacitance incorrectly.  This is a hardware bug, and
>  	 * we don't have a good way to deal with it.  The 2s window stuff
> @@ -290,6 +470,13 @@ static int hgpk_toggle_power(struct psmouse *psmouse, int enable)
>  
>  		psmouse_reset(psmouse);
>  
> +		if (hgpk_select_mode(psmouse)) {
> +			hgpk_err(psmouse, "Failed to select mode!\n");
> +			return -1;

Maybe we should slowly start using proper error codes...

> +		}
> +
> +		reset_hack_state(psmouse);
> +
>  		/* should be all set, enable the touchpad */
>  		ps2_command(&psmouse->ps2dev, NULL, PSMOUSE_CMD_ENABLE);
>  		psmouse_set_state(psmouse, PSMOUSE_ACTIVATED);
> @@ -328,7 +515,12 @@ static int hgpk_reconnect(struct psmouse *psmouse)
>  			return 0;
>  
>  	psmouse_reset(psmouse);
> +	if (hgpk_select_mode(psmouse)) {
> +		hgpk_err(psmouse, "Failed to select mode!\n");
> +		return -1;
> +	}
>  
> +	reset_hack_state(psmouse);
>  	return 0;
>  }
>  
> @@ -366,6 +558,35 @@ static ssize_t hgpk_set_powered(struct psmouse *psmouse, void *data,
>  __PSMOUSE_DEFINE_ATTR(powered, S_IWUSR | S_IRUGO, NULL,
>  		      hgpk_show_powered, hgpk_set_powered, false);
>  
> +static ssize_t attr_show_mode(struct psmouse *psmouse, void *data, char *buf)
> +{
> +	return sprintf(buf, "%s\n", mode_names[hgpk_mode]);
> +}
> +
> +static ssize_t attr_set_mode(struct psmouse *psmouse, void *data,
> +			     const char *buf, size_t len)
> +{
> +	int i;
> +	int new_mode = -1;
> +
> +	for (i = 0; i < ARRAY_SIZE(mode_names); i++) {
> +		const char *name = mode_names[i];
> +		if (strlen(name) == len && !strncasecmp(name, buf, len)) {
> +			new_mode = i;
> +			break;
> +		}
> +	}
> +
> +	if (new_mode == -1)
> +		return -EINVAL;
> +
> +	hgpk_mode = new_mode;
> +	return len;
> +}
> +
> +__PSMOUSE_DEFINE_ATTR(hgpk_mode, S_IWUSR | S_IRUGO, NULL,
> +		      attr_show_mode, attr_set_mode, 0);
> +
>  static ssize_t hgpk_trigger_recal_show(struct psmouse *psmouse,
>  		void *data, char *buf)
>  {
> @@ -401,6 +622,8 @@ static void hgpk_disconnect(struct psmouse *psmouse)
>  
>  	device_remove_file(&psmouse->ps2dev.serio->dev,
>  			   &psmouse_attr_powered.dattr);
> +	device_remove_file(&psmouse->ps2dev.serio->dev,
> +			   &psmouse_attr_hgpk_mode.dattr);
>  
>  	if (psmouse->model >= HGPK_MODEL_C)
>  		device_remove_file(&psmouse->ps2dev.serio->dev,
> @@ -424,6 +647,8 @@ static void hgpk_recalib_work(struct work_struct *work)
>  
>  static int hgpk_register(struct psmouse *psmouse)
>  {
> +	struct hgpk_data *priv = psmouse->private;
> +	struct input_dev *idev = psmouse->dev;
>  	int err;
>  
>  	/* register handlers */
> @@ -431,13 +656,45 @@ static int hgpk_register(struct psmouse *psmouse)
>  	psmouse->poll = hgpk_poll;
>  	psmouse->disconnect = hgpk_disconnect;
>  	psmouse->reconnect = hgpk_reconnect;
> -	psmouse->pktsize = 3;
>  
>  	/* Disable the idle resync. */
>  	psmouse->resync_time = 0;
>  	/* Reset after a lot of bad bytes. */
>  	psmouse->resetafter = 1024;
>  
> +	if (priv->mode != HGPK_MODE_MOUSE) {
> +		__set_bit(EV_ABS, idev->evbit);
> +		__set_bit(EV_KEY, idev->evbit);
> +		__set_bit(BTN_TOUCH, idev->keybit);
> +		__set_bit(BTN_TOOL_FINGER, idev->keybit);
> +		__set_bit(BTN_LEFT, idev->keybit);
> +		__set_bit(BTN_RIGHT, idev->keybit);
> +		__clear_bit(EV_REL, idev->evbit);
> +		__clear_bit(REL_X, idev->relbit);
> +		__clear_bit(REL_Y, idev->relbit);
> +	}
> +
> +	if (priv->mode == HGPK_MODE_GLIDESENSOR) {
> +		/* GlideSensor has pressure sensor, PenTablet does not */
> +		input_set_abs_params(idev, ABS_PRESSURE, 0, 15, 0, 0);
> +
> +		/* From device specs */
> +		input_set_abs_params(idev, ABS_X, 0, 399, 0, 0);
> +		input_set_abs_params(idev, ABS_Y, 0, 290, 0, 0);
> +
> +		/* Calculated by hand based on usable size (52mm x 38mm) */
> +		input_abs_set_res(idev, ABS_X, 8);
> +		input_abs_set_res(idev, ABS_Y, 8);
> +	} else if (priv->mode == HGPK_MODE_PENTABLET) {
> +		/* From device specs */
> +		input_set_abs_params(idev, ABS_X, 0, 999, 0, 0);
> +		input_set_abs_params(idev, ABS_Y, 5, 239, 0, 0);
> +
> +		/* Calculated by hand based on usable size (156mm x 38mm) */
> +		input_abs_set_res(idev, ABS_X, 6);
> +		input_abs_set_res(idev, ABS_Y, 8);
> +	}
> +
>  	err = device_create_file(&psmouse->ps2dev.serio->dev,
>  				 &psmouse_attr_powered.dattr);
>  	if (err) {
> @@ -445,6 +702,13 @@ static int hgpk_register(struct psmouse *psmouse)
>  		return err;
>  	}
>  
> +	err = device_create_file(&psmouse->ps2dev.serio->dev,
> +				 &psmouse_attr_hgpk_mode.dattr);
> +	if (err) {
> +		hgpk_err(psmouse, "Failed creating 'hgpk_mode' sysfs node\n");
> +		goto err_remove_powered;
> +	}
> +
>  	/* C-series touchpads added the recalibrate command */
>  	if (psmouse->model >= HGPK_MODEL_C) {
>  		err = device_create_file(&psmouse->ps2dev.serio->dev,
> @@ -452,13 +716,19 @@ static int hgpk_register(struct psmouse *psmouse)
>  		if (err) {
>  			hgpk_err(psmouse,
>  				"Failed creating 'recalibrate' sysfs node\n");
> -			device_remove_file(&psmouse->ps2dev.serio->dev,
> -					&psmouse_attr_powered.dattr);
> -			return err;
> +			goto err_remove_mode;
>  		}
>  	}
>  
>  	return 0;
> +
> +err_remove_mode:
> +	device_remove_file(&psmouse->ps2dev.serio->dev,
> +			   &psmouse_attr_hgpk_mode.dattr);
> +err_remove_powered:
> +	device_remove_file(&psmouse->ps2dev.serio->dev,
> +			   &psmouse_attr_powered.dattr);
> +	return err;
>  }
>  
>  int hgpk_init(struct psmouse *psmouse)
> @@ -473,12 +743,19 @@ int hgpk_init(struct psmouse *psmouse)
>  	psmouse->private = priv;
>  	priv->psmouse = psmouse;
>  	priv->powered = true;
> +	priv->mode = hgpk_mode;
>  	INIT_DELAYED_WORK(&priv->recalib_wq, hgpk_recalib_work);
>  
>  	err = psmouse_reset(psmouse);
>  	if (err)
>  		goto init_fail;
>  
> +	err = hgpk_select_mode(psmouse);
> +	if (err)
> +		goto init_fail;
> +
> +	reset_hack_state(psmouse);
> +
>  	err = hgpk_register(psmouse);
>  	if (err)
>  		goto init_fail;
> diff --git a/drivers/input/mouse/hgpk.h b/drivers/input/mouse/hgpk.h
> index d61cfd3..430f29f 100644
> --- a/drivers/input/mouse/hgpk.h
> +++ b/drivers/input/mouse/hgpk.h
> @@ -5,6 +5,9 @@
>  #ifndef _HGPK_H
>  #define _HGPK_H
>  
> +#define HGPK_GS		0xff       /* The GlideSensor */
> +#define HGPK_PT		0xcf       /* The PenTablet */
> +
>  enum hgpk_model_t {
>  	HGPK_MODEL_PREA = 0x0a,	/* pre-B1s */
>  	HGPK_MODEL_A = 0x14,	/* found on B1s, PT disabled in hardware */
> @@ -13,12 +16,20 @@ enum hgpk_model_t {
>  	HGPK_MODEL_D = 0x50,	/* C1, mass production */
>  };
>  
> +enum hgpk_mode {
> +	HGPK_MODE_MOUSE,
> +	HGPK_MODE_GLIDESENSOR,
> +	HGPK_MODE_PENTABLET,
> +};
> +
>  struct hgpk_data {
>  	struct psmouse *psmouse;
> +	int mode;
>  	bool powered;
>  	int count, x_tally, y_tally;	/* hardware workaround stuff */
>  	unsigned long recalib_window;
>  	struct delayed_work recalib_wq;
> +	int abs_x, abs_y;
>  };
>  
>  #define hgpk_dbg(psmouse, format, arg...)		\
> -- 
> 1.7.2.3
> 

Thanks.

-- 
Dmitry

Re: [PATCH] Input: hgpk - support GlideSensor and PenTablet modes

[Daniel Drake]

Hi Dmitry,

On 14 October 2010 16:52, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> I think we can do better than that and re-create the device for users
> ourselves. See drivers/input/mouse/psmouse_base.c::psmouse_attr_set_proto()
>
> I would also look into adding a module parameter so that users could
> specify desired mode on bootup.

Thanks for your feedback -- all good points and ideas.

I've addressed it all in the latest patch
https://patchwork.kernel.org/patch/254381/

Hoping you can take a look at it soon :)

Thanks,
Daniel

Re: [PATCH] Input: hgpk - support GlideSensor and PenTablet modes

[Dmitry Torokhov]

Hi Daniel,

On Wed, Oct 20, 2010 at 04:53:32PM +0100, Daniel Drake wrote:
> Hi Dmitry,
> 
> On 14 October 2010 16:52, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> > I think we can do better than that and re-create the device for users
> > ourselves. See drivers/input/mouse/psmouse_base.c::psmouse_attr_set_proto()
> >
> > I would also look into adding a module parameter so that users could
> > specify desired mode on bootup.
> 
> Thanks for your feedback -- all good points and ideas.
> 
> I've addressed it all in the latest patch
> https://patchwork.kernel.org/patch/254381/
> 
> Hoping you can take a look at it soon :)
> 

Sorry for the delay, I did not quite like that we had to adjust the
global mode option to switch the device to the new mode; we should be
able to do it ourselves within the driver instead of punting the task
off to serio_rescan(). Could you please tell me if the patch below (on
top of yours) still works?

Thanks!

-- 
Dmitry


Input: hgpk - apply mode change immediately

From: Dmitry Torokhov <dmitry.torokhov@gmail.com>

When changing current device mode, instead of adjusting the global default
mode option and rely on serio_rescan to reinitialize the device do it
ourselves.

Also rename hgpk_initial_mode parameter to simply hgpk_mode.

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

 drivers/input/mouse/hgpk.c |  362 ++++++++++++++++++++++++++------------------
 drivers/input/mouse/hgpk.h |    3 
 2 files changed, 220 insertions(+), 145 deletions(-)


diff --git a/drivers/input/mouse/hgpk.c b/drivers/input/mouse/hgpk.c
index 054b28f..5abb915 100644
--- a/drivers/input/mouse/hgpk.c
+++ b/drivers/input/mouse/hgpk.c
@@ -69,13 +69,14 @@ module_param(post_interrupt_delay, int, 0644);
 MODULE_PARM_DESC(post_interrupt_delay,
 	"delay (ms) before recal after recal interrupt detected");
 
-static char *hgpk_initial_mode;
-module_param(hgpk_initial_mode, charp, 0600);
-MODULE_PARM_DESC(hgpk_initial_mode,
-	"initial mode: mouse, glidesensor or pentablet");
+static char hgpk_mode_name[16];
+module_param_string(hgpk_mode, hgpk_mode_name, sizeof(hgpk_mode_name), 0644);
+MODULE_PARM_DESC(hgpk_mode,
+	"default hgpk mode: mouse, glidesensor or pentablet");
 
-static int hgpk_mode = HGPK_MODE_MOUSE;
-static const char * const mode_names[] = {
+static int hgpk_default_mode = HGPK_MODE_MOUSE;
+
+static const char * const hgpk_mode_names[] = {
 	[HGPK_MODE_MOUSE] = "Mouse",
 	[HGPK_MODE_GLIDESENSOR] = "GlideSensor",
 	[HGPK_MODE_PENTABLET] = "PenTablet",
@@ -85,13 +86,13 @@ static int hgpk_mode_from_name(const char *buf, int len)
 {
 	int i;
 
-	for (i = 0; i < ARRAY_SIZE(mode_names); i++) {
-		const char *name = mode_names[i];
+	for (i = 0; i < ARRAY_SIZE(hgpk_mode_names); i++) {
+		const char *name = hgpk_mode_names[i];
 		if (strlen(name) == len && !strncasecmp(name, buf, len))
 			return i;
 	}
 
-	return -1;
+	return HGPK_MODE_INVALID;
 }
 
 /*
@@ -193,36 +194,37 @@ static bool hgpk_is_byte_valid(struct psmouse *psmouse, unsigned char *packet)
 {
 	struct hgpk_data *priv = psmouse->private;
 	int pktcnt = psmouse->pktcnt;
-	bool r = true;
+	bool valid;
 
 	switch (priv->mode) {
 	case HGPK_MODE_MOUSE:
-		r = (packet[0] & 0x0C) == 0x08;
-		if (!r)
-			hgpk_dbg(psmouse, "bad data (%d) %02x %02x %02x\n",
-				 psmouse->pktcnt, psmouse->packet[0],
-				 psmouse->packet[1], psmouse->packet[2]);
+		valid = (packet[0] & 0x0C) == 0x08;
 		break;
 
 	case HGPK_MODE_GLIDESENSOR:
+		valid = pktcnt == 1 ?
+			packet[0] == HGPK_GS : packet[pktcnt - 1] & 0x80;
+		break;
+
 	case HGPK_MODE_PENTABLET:
-		/* bytes 2 - 6 should have 0 in the highest bit */
-		if (pktcnt >= 2 && pktcnt <= 6 && (packet[pktcnt - 1] & 0x80))
-			r = false;
-		if (priv->mode == HGPK_MODE_GLIDESENSOR && packet[0] != HGPK_GS)
-			r = false;
-		if (priv->mode == HGPK_MODE_PENTABLET && packet[0] != HGPK_PT)
-			r = false;
-		if (!r)
-			hgpk_dbg(psmouse, "bad data, mode %d (%d) "
-				 "%02x %02x %02x %02x %02x %02x\n",
-				 priv->mode, psmouse->pktcnt,
-				 psmouse->packet[0], psmouse->packet[1],
-				 psmouse->packet[2], psmouse->packet[3],
-				 psmouse->packet[4], psmouse->packet[5]);
+		valid = pktcnt == 1 ?
+			packet[0] == HGPK_PT : packet[pktcnt - 1] & 0x80;
+		break;
+
+	default:
+		valid = false;
 		break;
 	}
-	return r;
+
+	if (!valid)
+		hgpk_dbg(psmouse,
+			 "bad data, mode %d (%d) %02x %02x %02x %02x %02x %02x\n",
+			 priv->mode, pktcnt,
+			 psmouse->packet[0], psmouse->packet[1],
+			 psmouse->packet[2], psmouse->packet[3],
+			 psmouse->packet[4], psmouse->packet[5]);
+
+	return valid;
 }
 
 static void hgpk_process_advanced_packet(struct psmouse *psmouse)
@@ -230,48 +232,49 @@ static void hgpk_process_advanced_packet(struct psmouse *psmouse)
 	struct hgpk_data *priv = psmouse->private;
 	struct input_dev *idev = psmouse->dev;
 	unsigned char *packet = psmouse->packet;
+	int down = !!(packet[2] & 2);
 	int left = !!(packet[3] & 1);
 	int right = !!(packet[3] & 2);
 	int x = packet[1] | ((packet[2] & 0x78) << 4);
 	int y = packet[4] | ((packet[3] & 0x70) << 3);
-	int z = packet[5];
-	int down;
 
 	if (priv->mode == HGPK_MODE_GLIDESENSOR) {
 		int pt_down = !!(packet[2] & 1);
 		int finger_down = !!(packet[2] & 2);
+		int z = packet[5];
 
 		input_report_abs(idev, ABS_PRESSURE, z);
-		down = finger_down;
 		if (tpdebug)
-			hgpk_dbg(psmouse, "pd=%d fd=%d ",
-				 pt_down, finger_down);
+			hgpk_dbg(psmouse, "pd=%d fd=%d z=%d",
+				 pt_down, finger_down, z);
 	} else {
-		/* PenTablet mode does not report pressure, so we don't
-		 * report it here */
-		down = !!(packet[2] & 2);
+		/*
+		 * PenTablet mode does not report pressure, so we don't
+		 * report it here
+		 */
 		if (tpdebug)
 			hgpk_dbg(psmouse, "pd=%d ", down);
 	}
 
 	if (tpdebug)
-		hgpk_dbg(psmouse, "l=%d r=%d x=%d y=%d z=%d\n",
-			 left, right, x, y, z);
+		hgpk_dbg(psmouse, "l=%d r=%d x=%d y=%d\n", left, right, x, y);
 
 	input_report_key(idev, BTN_TOUCH, down);
 	input_report_key(idev, BTN_LEFT, left);
 	input_report_key(idev, BTN_RIGHT, right);
 
 	/*
-	 * if this packet says that the finger was removed, reset our position
+	 * If this packet says that the finger was removed, reset our position
 	 * tracking so that we don't erroneously detect a jump on next press.
 	 */
 	if (!down)
 		priv->abs_x = priv->abs_y = -1;
 
-	/* Report position if finger/pen is down, but weed out duplicate
+	/*
+	 * Report position if finger/pen is down, but weed out duplicate
 	 * packets (we get quite a few in this mode, and they mess up our
-	 * jump detection */
+	 * jump detection.
+	 */
 	if (down && (x != priv->abs_x || y != priv->abs_y)) {
 
 		/* Don't apply hacks in PT mode, it seems reliable */
@@ -386,6 +389,7 @@ static int hgpk_select_mode(struct psmouse *psmouse)
 		if (ps2_command(ps2dev, NULL, cmd))
 			return -EIO;
 		break;
+
 	default:
 		return -EINVAL;
 	}
@@ -393,6 +397,71 @@ static int hgpk_select_mode(struct psmouse *psmouse)
 	return 0;
 }
 
+static void hgpk_setup_input_device(struct input_dev *input,
+				    struct input_dev *old_input,
+				    enum hgpk_mode mode)
+{
+	if (old_input) {
+		input->name = old_input->name;
+		input->phys = old_input->phys;
+		input->id = old_input->id;
+		input->dev.parent = old_input->dev.parent;
+	}
+
+	memset(input->evbit, 0, sizeof(input->evbit));
+	memset(input->relbit, 0, sizeof(input->relbit));
+	memset(input->keybit, 0, sizeof(input->keybit));
+
+	/* All modes report left and right buttons */
+	__set_bit(EV_KEY, input->evbit);
+	__set_bit(BTN_LEFT, input->keybit);
+	__set_bit(BTN_RIGHT, input->keybit);
+
+	switch (mode) {
+	case HGPK_MODE_MOUSE:
+		__set_bit(EV_REL, input->evbit);
+		__set_bit(REL_X, input->relbit);
+		__set_bit(REL_Y, input->relbit);
+		break;
+
+	case HGPK_MODE_GLIDESENSOR:
+		__set_bit(BTN_TOUCH, input->keybit);
+		__set_bit(BTN_TOOL_FINGER, input->keybit);
+
+		__set_bit(EV_ABS, input->evbit);
+
+		/* GlideSensor has pressure sensor, PenTablet does not */
+		input_set_abs_params(input, ABS_PRESSURE, 0, 15, 0, 0);
+
+		/* From device specs */
+		input_set_abs_params(input, ABS_X, 0, 399, 0, 0);
+		input_set_abs_params(input, ABS_Y, 0, 290, 0, 0);
+
+		/* Calculated by hand based on usable size (52mm x 38mm) */
+		input_abs_set_res(input, ABS_X, 8);
+		input_abs_set_res(input, ABS_Y, 8);
+		break;
+
+	case HGPK_MODE_PENTABLET:
+		__set_bit(BTN_TOUCH, input->keybit);
+		__set_bit(BTN_TOOL_FINGER, input->keybit);
+
+		__set_bit(EV_ABS, input->evbit);
+
+		/* From device specs */
+		input_set_abs_params(input, ABS_X, 0, 999, 0, 0);
+		input_set_abs_params(input, ABS_Y, 5, 239, 0, 0);
+
+		/* Calculated by hand based on usable size (156mm x 38mm) */
+		input_abs_set_res(input, ABS_X, 6);
+		input_abs_set_res(input, ABS_Y, 8);
+		break;
+
+	default:
+		BUG();
+	}
+}
+
 static void hgpk_reset_hack_state(struct psmouse *psmouse)
 {
 	struct hgpk_data *priv = psmouse->private;
@@ -400,11 +469,43 @@ static void hgpk_reset_hack_state(struct psmouse *psmouse)
 	priv->abs_x = priv->abs_y = -1;
 }
 
+static int hgpk_reset_device(struct psmouse *psmouse, bool recalibrate)
+{
+	int err;
+
+	psmouse_reset(psmouse);
+
+	if (recalibrate) {
+		struct ps2dev *ps2dev = &psmouse->ps2dev;
+
+		/* send the recalibrate request */
+		if (ps2_command(ps2dev, NULL, 0xf5) ||
+		    ps2_command(ps2dev, NULL, 0xf5) ||
+		    ps2_command(ps2dev, NULL, 0xe6) ||
+		    ps2_command(ps2dev, NULL, 0xf5)) {
+			return -1;
+		}
+
+		/* according to ALPS, 150mS is required for recalibration */
+		msleep(150);
+	}
+
+	err = hgpk_select_mode(psmouse);
+	if (err) {
+		hgpk_err(psmouse, "failed to select mode\n");
+		return err;
+	}
+
+	hgpk_reset_hack_state(psmouse);
+
+	return 0;
+}
+
 static int hgpk_force_recalibrate(struct psmouse *psmouse)
 {
 	struct ps2dev *ps2dev = &psmouse->ps2dev;
 	struct hgpk_data *priv = psmouse->private;
-	int r;
+	int err;
 
 	/* C-series touchpads added the recalibrate command */
 	if (psmouse->model < HGPK_MODEL_C)
@@ -414,28 +515,12 @@ static int hgpk_force_recalibrate(struct psmouse *psmouse)
 	psmouse_set_state(psmouse, PSMOUSE_INITIALIZING);
 
 	/* start by resetting the device */
-	psmouse_reset(psmouse);
-
-	/* send the recalibrate request */
-	if (ps2_command(ps2dev, NULL, 0xf5) ||
-	    ps2_command(ps2dev, NULL, 0xf5) ||
-	    ps2_command(ps2dev, NULL, 0xe6) ||
-	    ps2_command(ps2dev, NULL, 0xf5)) {
-		return -1;
-	}
-
-	/* according to ALPS, 150mS is required for recalibration */
-	msleep(150);
-
-	r = hgpk_select_mode(psmouse);
-	if (r) {
-		hgpk_err(psmouse, "failed to select mode\n");
-		return r;
-	}
-
-	hgpk_reset_hack_state(psmouse);
+	err = hgpk_reset_device(psmouse, true);
+	if (err)
+		return err;
 
-	/* XXX: If a finger is down during this delay, recalibration will
+	/*
+	 * XXX: If a finger is down during this delay, recalibration will
 	 * detect capacitance incorrectly.  This is a hardware bug, and
 	 * we don't have a good way to deal with it.  The 2s window stuff
 	 * (below) is our best option for now.
@@ -446,12 +531,13 @@ static int hgpk_force_recalibrate(struct psmouse *psmouse)
 
 	psmouse_set_state(psmouse, PSMOUSE_ACTIVATED);
 
-	/* After we recalibrate, we shouldn't get any packets for 2s.  If
+	/*
+	 * After we recalibrate, we shouldn't get any packets for 2s.  If
 	 * we do, it's likely that someone's finger was on the touchpad.
 	 * If someone's finger *was* on the touchpad, it's probably
 	 * miscalibrated.  So, we should schedule another recalibration
 	 */
-	priv->recalib_window = jiffies +  msecs_to_jiffies(recal_guard_time);
+	priv->recalib_window = jiffies + msecs_to_jiffies(recal_guard_time);
 
 	return 0;
 }
@@ -465,7 +551,7 @@ static int hgpk_toggle_power(struct psmouse *psmouse, int enable)
 {
 	struct ps2dev *ps2dev = &psmouse->ps2dev;
 	int timeo;
-	int r;
+	int err;
 
 	/* Added on D-series touchpads */
 	if (psmouse->model < HGPK_MODEL_D)
@@ -488,16 +574,12 @@ static int hgpk_toggle_power(struct psmouse *psmouse, int enable)
 			msleep(50);
 		}
 
-		psmouse_reset(psmouse);
-
-		r = hgpk_select_mode(psmouse);
-		if (r) {
-			hgpk_err(psmouse, "Failed to select mode!\n");
-			return r;
+		err = hgpk_reset_device(psmouse, false);
+		if (err) {
+			hgpk_err(psmouse, "Failed to reset device!\n");
+			return err;
 		}
 
-		hgpk_reset_hack_state(psmouse);
-
 		/* should be all set, enable the touchpad */
 		ps2_command(&psmouse->ps2dev, NULL, PSMOUSE_CMD_ENABLE);
 		psmouse_set_state(psmouse, PSMOUSE_ACTIVATED);
@@ -527,25 +609,17 @@ static int hgpk_poll(struct psmouse *psmouse)
 
 static int hgpk_reconnect(struct psmouse *psmouse)
 {
-	int r;
-
-	/* During suspend/resume the ps2 rails remain powered.  We don't want
+	/*
+	 * During suspend/resume the ps2 rails remain powered.  We don't want
 	 * to do a reset because it's flush data out of buffers; however,
-	 * earlier prototypes (B1) had some brokenness that required a reset. */
+	 * earlier prototypes (B1) had some brokenness that required a reset.
+	 */
 	if (olpc_board_at_least(olpc_board(0xb2)))
 		if (psmouse->ps2dev.serio->dev.power.power_state.event !=
 				PM_EVENT_ON)
 			return 0;
 
-	psmouse_reset(psmouse);
-	r = hgpk_select_mode(psmouse);
-	if (r) {
-		hgpk_err(psmouse, "Failed to select mode!\n");
-		return -1;
-	}
-
-	hgpk_reset_hack_state(psmouse);
-	return 0;
+	return hgpk_reset_device(psmouse, false);
 }
 
 static ssize_t hgpk_show_powered(struct psmouse *psmouse, void *data, char *buf)
@@ -584,24 +658,58 @@ __PSMOUSE_DEFINE_ATTR(powered, S_IWUSR | S_IRUGO, NULL,
 
 static ssize_t attr_show_mode(struct psmouse *psmouse, void *data, char *buf)
 {
-	return sprintf(buf, "%s\n", mode_names[hgpk_mode]);
+	struct hgpk_data *priv = psmouse->private;
+
+	return sprintf(buf, "%s\n", hgpk_mode_names[priv->mode]);
 }
 
 static ssize_t attr_set_mode(struct psmouse *psmouse, void *data,
 			     const char *buf, size_t len)
 {
-	int new_mode = hgpk_mode_from_name(buf, len);
+	struct hgpk_data *priv = psmouse->private;
+	enum hgpk_mode old_mode = priv->mode;
+	enum hgpk_mode new_mode = hgpk_mode_from_name(buf, len);
+	struct input_dev *old_dev = psmouse->dev;
+	struct input_dev *new_dev;
+	int err;
 
-	if (new_mode == -1)
+	if (new_mode == HGPK_MODE_INVALID)
 		return -EINVAL;
 
-	hgpk_mode = new_mode;
-	serio_rescan(psmouse->ps2dev.serio);
+	if (old_mode == new_mode)
+		return len;
+
+	new_dev = input_allocate_device();
+	if (!new_dev)
+		return -ENOMEM;
+
+	/* Switch device into the new mode */
+	priv->mode = new_mode;
+	err = hgpk_reset_device(psmouse, false);
+	if (err)
+		goto err_try_restore;
+
+	hgpk_setup_input_device(new_dev, old_dev, new_mode);
+
+	err = input_register_device(new_dev);
+	if (err)
+		goto err_try_restore;
+
+	psmouse->dev = new_dev;
+	input_unregister_device(old_dev);
+
 	return len;
+
+err_try_restore:
+	input_free_device(new_dev);
+	priv->mode = old_mode;
+	hgpk_reset_device(psmouse, false);
+
+	return err;
 }
 
-__PSMOUSE_DEFINE_ATTR(hgpk_mode, S_IWUSR | S_IRUGO, NULL,
-		      attr_show_mode, attr_set_mode, 0);
+PSMOUSE_DEFINE_ATTR(hgpk_mode, S_IWUSR | S_IRUGO, NULL,
+		    attr_show_mode, attr_set_mode);
 
 static ssize_t hgpk_trigger_recal_show(struct psmouse *psmouse,
 		void *data, char *buf)
@@ -664,7 +772,6 @@ static void hgpk_recalib_work(struct work_struct *work)
 static int hgpk_register(struct psmouse *psmouse)
 {
 	struct hgpk_data *priv = psmouse->private;
-	struct input_dev *idev = psmouse->dev;
 	int err;
 
 	/* register handlers */
@@ -678,38 +785,7 @@ static int hgpk_register(struct psmouse *psmouse)
 	/* Reset after a lot of bad bytes. */
 	psmouse->resetafter = 1024;
 
-	if (priv->mode != HGPK_MODE_MOUSE) {
-		__set_bit(EV_ABS, idev->evbit);
-		__set_bit(EV_KEY, idev->evbit);
-		__set_bit(BTN_TOUCH, idev->keybit);
-		__set_bit(BTN_TOOL_FINGER, idev->keybit);
-		__set_bit(BTN_LEFT, idev->keybit);
-		__set_bit(BTN_RIGHT, idev->keybit);
-		__clear_bit(EV_REL, idev->evbit);
-		__clear_bit(REL_X, idev->relbit);
-		__clear_bit(REL_Y, idev->relbit);
-	}
-
-	if (priv->mode == HGPK_MODE_GLIDESENSOR) {
-		/* GlideSensor has pressure sensor, PenTablet does not */
-		input_set_abs_params(idev, ABS_PRESSURE, 0, 15, 0, 0);
-
-		/* From device specs */
-		input_set_abs_params(idev, ABS_X, 0, 399, 0, 0);
-		input_set_abs_params(idev, ABS_Y, 0, 290, 0, 0);
-
-		/* Calculated by hand based on usable size (52mm x 38mm) */
-		input_abs_set_res(idev, ABS_X, 8);
-		input_abs_set_res(idev, ABS_Y, 8);
-	} else if (priv->mode == HGPK_MODE_PENTABLET) {
-		/* From device specs */
-		input_set_abs_params(idev, ABS_X, 0, 999, 0, 0);
-		input_set_abs_params(idev, ABS_Y, 5, 239, 0, 0);
-
-		/* Calculated by hand based on usable size (156mm x 38mm) */
-		input_abs_set_res(idev, ABS_X, 6);
-		input_abs_set_res(idev, ABS_Y, 8);
-	}
+	hgpk_setup_input_device(psmouse->dev, NULL, priv->mode);
 
 	err = device_create_file(&psmouse->ps2dev.serio->dev,
 				 &psmouse_attr_powered.dattr);
@@ -750,28 +826,25 @@ err_remove_powered:
 int hgpk_init(struct psmouse *psmouse)
 {
 	struct hgpk_data *priv;
-	int err = -ENOMEM;
+	int err;
 
 	priv = kzalloc(sizeof(struct hgpk_data), GFP_KERNEL);
-	if (!priv)
+	if (!priv) {
+		err = -ENOMEM;
 		goto alloc_fail;
+	}
 
 	psmouse->private = priv;
+
 	priv->psmouse = psmouse;
 	priv->powered = true;
-	priv->mode = hgpk_mode;
+	priv->mode = hgpk_default_mode;
 	INIT_DELAYED_WORK(&priv->recalib_wq, hgpk_recalib_work);
 
-	err = psmouse_reset(psmouse);
-	if (err)
-		goto init_fail;
-
-	err = hgpk_select_mode(psmouse);
+	err = hgpk_reset_device(psmouse, false);
 	if (err)
 		goto init_fail;
 
-	hgpk_reset_hack_state(psmouse);
-
 	err = hgpk_register(psmouse);
 	if (err)
 		goto init_fail;
@@ -827,10 +900,11 @@ int hgpk_detect(struct psmouse *psmouse, bool set_properties)
 
 void hgpk_module_init(void)
 {
-	if (hgpk_initial_mode) {
-		int mode = hgpk_mode_from_name(hgpk_initial_mode,
-					       strlen(hgpk_initial_mode));
-		if (mode != -1)
-			hgpk_mode = mode;
+	hgpk_default_mode = hgpk_mode_from_name(hgpk_mode_name,
+						strlen(hgpk_mode_name));
+	if (hgpk_default_mode == HGPK_MODE_INVALID) {
+		hgpk_default_mode = HGPK_MODE_MOUSE;
+		strlcpy(hgpk_mode_name, hgpk_mode_names[HGPK_MODE_MOUSE],
+			sizeof(hgpk_mode_name));
 	}
 }
diff --git a/drivers/input/mouse/hgpk.h b/drivers/input/mouse/hgpk.h
index 46aaeee..01c983b 100644
--- a/drivers/input/mouse/hgpk.h
+++ b/drivers/input/mouse/hgpk.h
@@ -20,11 +20,12 @@ enum hgpk_mode {
 	HGPK_MODE_MOUSE,
 	HGPK_MODE_GLIDESENSOR,
 	HGPK_MODE_PENTABLET,
+	HGPK_MODE_INVALID
 };
 
 struct hgpk_data {
 	struct psmouse *psmouse;
-	int mode;
+	enum hgpk_mode mode;
 	bool powered;
 	int count, x_tally, y_tally;	/* hardware workaround stuff */
 	unsigned long recalib_window;

Re: [PATCH] Input: hgpk - support GlideSensor and PenTablet modes

[Daniel Drake]

On 29 October 2010 17:59, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> Sorry for the delay, I did not quite like that we had to adjust the
> global mode option to switch the device to the new mode; we should be
> able to do it ourselves within the driver instead of punting the task
> off to serio_rescan(). Could you please tell me if the patch below (on
> top of yours) still works?

Thanks for battling through this.
It introduces some minor breakage.
Fixed by: http://dev.laptop.org/~dsd/20101029/hgpk-fixups.txt

Let me know what the next steps are

Daniel

Re: [PATCH] Input: hgpk - support GlideSensor and PenTablet modes

[Dmitry Torokhov]

On Fri, Oct 29, 2010 at 09:11:43PM +0100, Daniel Drake wrote:
> On 29 October 2010 17:59, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> > Sorry for the delay, I did not quite like that we had to adjust the
> > global mode option to switch the device to the new mode; we should be
> > able to do it ourselves within the driver instead of punting the task
> > off to serio_rescan(). Could you please tell me if the patch below (on
> > top of yours) still works?
> 
> Thanks for battling through this.
> It introduces some minor breakage.
> Fixed by: http://dev.laptop.org/~dsd/20101029/hgpk-fixups.txt
> 
> Let me know what the next steps are
> 

Thanks Daniel, I'll fold it all together and try to get it into -rc1.

-- 
Dmitry

Re: [PATCH] Input: hgpk - support GlideSensor and PenTablet modes

[Daniel Drake]

On 29 October 2010 23:40, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> Thanks Daniel, I'll fold it all together and try to get it into -rc1.

We've missed that but 2.6.38 will be fine.
Any chance you could put this in your linux-next branch?
Then I'll get on with the other pending patches.

Thanks,
Daniel

Re: [PATCH] Input: hgpk - support GlideSensor and PenTablet modes

[Dmitry Torokhov]

On Thu, Nov 04, 2010 at 09:14:51PM +0000, Daniel Drake wrote:
> On 29 October 2010 23:40, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> > Thanks Daniel, I'll fold it all together and try to get it into -rc1.
> 
> We've missed that but 2.6.38 will be fine.
> Any chance you could put this in your linux-next branch?
> Then I'll get on with the other pending patches.
> 

Ahem, sorry about this, bad scheduling on my part...

I tried merging the snippet that you mention in your last e-mail but I
think that we should not manually set state to PSMOUSE_ACTIVATED, but
rather mark the attribute as "protected" and rely on psmouse core to
disable and re-enable the device.

Does the patch below still work for you?

Thanks.

-- 
Dmitry


Input: hgpk - apply mode change immediately

When changing current device mode, instead of adjusting the global default
mode option and rely on serio_rescan to reinitialize the device do it
ourselves.

Also rename hgpk_initial_mode parameter to simply hgpk_mode.

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

 drivers/input/mouse/hgpk.c |  366 +++++++++++++++++++++++++++-----------------
 drivers/input/mouse/hgpk.h |    3 
 2 files changed, 224 insertions(+), 145 deletions(-)


diff --git a/drivers/input/mouse/hgpk.c b/drivers/input/mouse/hgpk.c
index 054b28f..3d33d95 100644
--- a/drivers/input/mouse/hgpk.c
+++ b/drivers/input/mouse/hgpk.c
@@ -69,13 +69,14 @@ module_param(post_interrupt_delay, int, 0644);
 MODULE_PARM_DESC(post_interrupt_delay,
 	"delay (ms) before recal after recal interrupt detected");
 
-static char *hgpk_initial_mode;
-module_param(hgpk_initial_mode, charp, 0600);
-MODULE_PARM_DESC(hgpk_initial_mode,
-	"initial mode: mouse, glidesensor or pentablet");
+static char hgpk_mode_name[16];
+module_param_string(hgpk_mode, hgpk_mode_name, sizeof(hgpk_mode_name), 0644);
+MODULE_PARM_DESC(hgpk_mode,
+	"default hgpk mode: mouse, glidesensor or pentablet");
 
-static int hgpk_mode = HGPK_MODE_MOUSE;
-static const char * const mode_names[] = {
+static int hgpk_default_mode = HGPK_MODE_MOUSE;
+
+static const char * const hgpk_mode_names[] = {
 	[HGPK_MODE_MOUSE] = "Mouse",
 	[HGPK_MODE_GLIDESENSOR] = "GlideSensor",
 	[HGPK_MODE_PENTABLET] = "PenTablet",
@@ -85,13 +86,13 @@ static int hgpk_mode_from_name(const char *buf, int len)
 {
 	int i;
 
-	for (i = 0; i < ARRAY_SIZE(mode_names); i++) {
-		const char *name = mode_names[i];
+	for (i = 0; i < ARRAY_SIZE(hgpk_mode_names); i++) {
+		const char *name = hgpk_mode_names[i];
 		if (strlen(name) == len && !strncasecmp(name, buf, len))
 			return i;
 	}
 
-	return -1;
+	return HGPK_MODE_INVALID;
 }
 
 /*
@@ -193,36 +194,37 @@ static bool hgpk_is_byte_valid(struct psmouse *psmouse, unsigned char *packet)
 {
 	struct hgpk_data *priv = psmouse->private;
 	int pktcnt = psmouse->pktcnt;
-	bool r = true;
+	bool valid;
 
 	switch (priv->mode) {
 	case HGPK_MODE_MOUSE:
-		r = (packet[0] & 0x0C) == 0x08;
-		if (!r)
-			hgpk_dbg(psmouse, "bad data (%d) %02x %02x %02x\n",
-				 psmouse->pktcnt, psmouse->packet[0],
-				 psmouse->packet[1], psmouse->packet[2]);
+		valid = (packet[0] & 0x0C) == 0x08;
 		break;
 
 	case HGPK_MODE_GLIDESENSOR:
+		valid = pktcnt == 1 ?
+			packet[0] == HGPK_GS : !(packet[pktcnt - 1] & 0x80);
+		break;
+
 	case HGPK_MODE_PENTABLET:
-		/* bytes 2 - 6 should have 0 in the highest bit */
-		if (pktcnt >= 2 && pktcnt <= 6 && (packet[pktcnt - 1] & 0x80))
-			r = false;
-		if (priv->mode == HGPK_MODE_GLIDESENSOR && packet[0] != HGPK_GS)
-			r = false;
-		if (priv->mode == HGPK_MODE_PENTABLET && packet[0] != HGPK_PT)
-			r = false;
-		if (!r)
-			hgpk_dbg(psmouse, "bad data, mode %d (%d) "
-				 "%02x %02x %02x %02x %02x %02x\n",
-				 priv->mode, psmouse->pktcnt,
-				 psmouse->packet[0], psmouse->packet[1],
-				 psmouse->packet[2], psmouse->packet[3],
-				 psmouse->packet[4], psmouse->packet[5]);
+		valid = pktcnt == 1 ?
+			packet[0] == HGPK_PT : !(packet[pktcnt - 1] & 0x80);
+		break;
+
+	default:
+		valid = false;
 		break;
 	}
-	return r;
+
+	if (!valid)
+		hgpk_dbg(psmouse,
+			 "bad data, mode %d (%d) %02x %02x %02x %02x %02x %02x\n",
+			 priv->mode, pktcnt,
+			 psmouse->packet[0], psmouse->packet[1],
+			 psmouse->packet[2], psmouse->packet[3],
+			 psmouse->packet[4], psmouse->packet[5]);
+
+	return valid;
 }
 
 static void hgpk_process_advanced_packet(struct psmouse *psmouse)
@@ -230,48 +232,49 @@ static void hgpk_process_advanced_packet(struct psmouse *psmouse)
 	struct hgpk_data *priv = psmouse->private;
 	struct input_dev *idev = psmouse->dev;
 	unsigned char *packet = psmouse->packet;
+	int down = !!(packet[2] & 2);
 	int left = !!(packet[3] & 1);
 	int right = !!(packet[3] & 2);
 	int x = packet[1] | ((packet[2] & 0x78) << 4);
 	int y = packet[4] | ((packet[3] & 0x70) << 3);
-	int z = packet[5];
-	int down;
 
 	if (priv->mode == HGPK_MODE_GLIDESENSOR) {
 		int pt_down = !!(packet[2] & 1);
 		int finger_down = !!(packet[2] & 2);
+		int z = packet[5];
 
 		input_report_abs(idev, ABS_PRESSURE, z);
-		down = finger_down;
 		if (tpdebug)
-			hgpk_dbg(psmouse, "pd=%d fd=%d ",
-				 pt_down, finger_down);
+			hgpk_dbg(psmouse, "pd=%d fd=%d z=%d",
+				 pt_down, finger_down, z);
 	} else {
-		/* PenTablet mode does not report pressure, so we don't
-		 * report it here */
-		down = !!(packet[2] & 2);
+		/*
+		 * PenTablet mode does not report pressure, so we don't
+		 * report it here
+		 */
 		if (tpdebug)
 			hgpk_dbg(psmouse, "pd=%d ", down);
 	}
 
 	if (tpdebug)
-		hgpk_dbg(psmouse, "l=%d r=%d x=%d y=%d z=%d\n",
-			 left, right, x, y, z);
+		hgpk_dbg(psmouse, "l=%d r=%d x=%d y=%d\n", left, right, x, y);
 
 	input_report_key(idev, BTN_TOUCH, down);
 	input_report_key(idev, BTN_LEFT, left);
 	input_report_key(idev, BTN_RIGHT, right);
 
 	/*
-	 * if this packet says that the finger was removed, reset our position
+	 * If this packet says that the finger was removed, reset our position
 	 * tracking so that we don't erroneously detect a jump on next press.
 	 */
 	if (!down)
 		priv->abs_x = priv->abs_y = -1;
 
-	/* Report position if finger/pen is down, but weed out duplicate
+	/*
+	 * Report position if finger/pen is down, but weed out duplicate
 	 * packets (we get quite a few in this mode, and they mess up our
-	 * jump detection */
+	 * jump detection.
+	 */
 	if (down && (x != priv->abs_x || y != priv->abs_y)) {
 
 		/* Don't apply hacks in PT mode, it seems reliable */
@@ -386,6 +389,7 @@ static int hgpk_select_mode(struct psmouse *psmouse)
 		if (ps2_command(ps2dev, NULL, cmd))
 			return -EIO;
 		break;
+
 	default:
 		return -EINVAL;
 	}
@@ -393,6 +397,71 @@ static int hgpk_select_mode(struct psmouse *psmouse)
 	return 0;
 }
 
+static void hgpk_setup_input_device(struct input_dev *input,
+				    struct input_dev *old_input,
+				    enum hgpk_mode mode)
+{
+	if (old_input) {
+		input->name = old_input->name;
+		input->phys = old_input->phys;
+		input->id = old_input->id;
+		input->dev.parent = old_input->dev.parent;
+	}
+
+	memset(input->evbit, 0, sizeof(input->evbit));
+	memset(input->relbit, 0, sizeof(input->relbit));
+	memset(input->keybit, 0, sizeof(input->keybit));
+
+	/* All modes report left and right buttons */
+	__set_bit(EV_KEY, input->evbit);
+	__set_bit(BTN_LEFT, input->keybit);
+	__set_bit(BTN_RIGHT, input->keybit);
+
+	switch (mode) {
+	case HGPK_MODE_MOUSE:
+		__set_bit(EV_REL, input->evbit);
+		__set_bit(REL_X, input->relbit);
+		__set_bit(REL_Y, input->relbit);
+		break;
+
+	case HGPK_MODE_GLIDESENSOR:
+		__set_bit(BTN_TOUCH, input->keybit);
+		__set_bit(BTN_TOOL_FINGER, input->keybit);
+
+		__set_bit(EV_ABS, input->evbit);
+
+		/* GlideSensor has pressure sensor, PenTablet does not */
+		input_set_abs_params(input, ABS_PRESSURE, 0, 15, 0, 0);
+
+		/* From device specs */
+		input_set_abs_params(input, ABS_X, 0, 399, 0, 0);
+		input_set_abs_params(input, ABS_Y, 0, 290, 0, 0);
+
+		/* Calculated by hand based on usable size (52mm x 38mm) */
+		input_abs_set_res(input, ABS_X, 8);
+		input_abs_set_res(input, ABS_Y, 8);
+		break;
+
+	case HGPK_MODE_PENTABLET:
+		__set_bit(BTN_TOUCH, input->keybit);
+		__set_bit(BTN_TOOL_FINGER, input->keybit);
+
+		__set_bit(EV_ABS, input->evbit);
+
+		/* From device specs */
+		input_set_abs_params(input, ABS_X, 0, 999, 0, 0);
+		input_set_abs_params(input, ABS_Y, 5, 239, 0, 0);
+
+		/* Calculated by hand based on usable size (156mm x 38mm) */
+		input_abs_set_res(input, ABS_X, 6);
+		input_abs_set_res(input, ABS_Y, 8);
+		break;
+
+	default:
+		BUG();
+	}
+}
+
 static void hgpk_reset_hack_state(struct psmouse *psmouse)
 {
 	struct hgpk_data *priv = psmouse->private;
@@ -400,11 +469,43 @@ static void hgpk_reset_hack_state(struct psmouse *psmouse)
 	priv->abs_x = priv->abs_y = -1;
 }
 
+static int hgpk_reset_device(struct psmouse *psmouse, bool recalibrate)
+{
+	int err;
+
+	psmouse_reset(psmouse);
+
+	if (recalibrate) {
+		struct ps2dev *ps2dev = &psmouse->ps2dev;
+
+		/* send the recalibrate request */
+		if (ps2_command(ps2dev, NULL, 0xf5) ||
+		    ps2_command(ps2dev, NULL, 0xf5) ||
+		    ps2_command(ps2dev, NULL, 0xe6) ||
+		    ps2_command(ps2dev, NULL, 0xf5)) {
+			return -1;
+		}
+
+		/* according to ALPS, 150mS is required for recalibration */
+		msleep(150);
+	}
+
+	err = hgpk_select_mode(psmouse);
+	if (err) {
+		hgpk_err(psmouse, "failed to select mode\n");
+		return err;
+	}
+
+	hgpk_reset_hack_state(psmouse);
+
+	return 0;
+}
+
 static int hgpk_force_recalibrate(struct psmouse *psmouse)
 {
 	struct ps2dev *ps2dev = &psmouse->ps2dev;
 	struct hgpk_data *priv = psmouse->private;
-	int r;
+	int err;
 
 	/* C-series touchpads added the recalibrate command */
 	if (psmouse->model < HGPK_MODEL_C)
@@ -414,28 +515,12 @@ static int hgpk_force_recalibrate(struct psmouse *psmouse)
 	psmouse_set_state(psmouse, PSMOUSE_INITIALIZING);
 
 	/* start by resetting the device */
-	psmouse_reset(psmouse);
-
-	/* send the recalibrate request */
-	if (ps2_command(ps2dev, NULL, 0xf5) ||
-	    ps2_command(ps2dev, NULL, 0xf5) ||
-	    ps2_command(ps2dev, NULL, 0xe6) ||
-	    ps2_command(ps2dev, NULL, 0xf5)) {
-		return -1;
-	}
-
-	/* according to ALPS, 150mS is required for recalibration */
-	msleep(150);
-
-	r = hgpk_select_mode(psmouse);
-	if (r) {
-		hgpk_err(psmouse, "failed to select mode\n");
-		return r;
-	}
-
-	hgpk_reset_hack_state(psmouse);
+	err = hgpk_reset_device(psmouse, true);
+	if (err)
+		return err;
 
-	/* XXX: If a finger is down during this delay, recalibration will
+	/*
+	 * XXX: If a finger is down during this delay, recalibration will
 	 * detect capacitance incorrectly.  This is a hardware bug, and
 	 * we don't have a good way to deal with it.  The 2s window stuff
 	 * (below) is our best option for now.
@@ -446,12 +531,13 @@ static int hgpk_force_recalibrate(struct psmouse *psmouse)
 
 	psmouse_set_state(psmouse, PSMOUSE_ACTIVATED);
 
-	/* After we recalibrate, we shouldn't get any packets for 2s.  If
+	/*
+	 * After we recalibrate, we shouldn't get any packets for 2s.  If
 	 * we do, it's likely that someone's finger was on the touchpad.
 	 * If someone's finger *was* on the touchpad, it's probably
 	 * miscalibrated.  So, we should schedule another recalibration
 	 */
-	priv->recalib_window = jiffies +  msecs_to_jiffies(recal_guard_time);
+	priv->recalib_window = jiffies + msecs_to_jiffies(recal_guard_time);
 
 	return 0;
 }
@@ -465,7 +551,7 @@ static int hgpk_toggle_power(struct psmouse *psmouse, int enable)
 {
 	struct ps2dev *ps2dev = &psmouse->ps2dev;
 	int timeo;
-	int r;
+	int err;
 
 	/* Added on D-series touchpads */
 	if (psmouse->model < HGPK_MODEL_D)
@@ -488,16 +574,12 @@ static int hgpk_toggle_power(struct psmouse *psmouse, int enable)
 			msleep(50);
 		}
 
-		psmouse_reset(psmouse);
-
-		r = hgpk_select_mode(psmouse);
-		if (r) {
-			hgpk_err(psmouse, "Failed to select mode!\n");
-			return r;
+		err = hgpk_reset_device(psmouse, false);
+		if (err) {
+			hgpk_err(psmouse, "Failed to reset device!\n");
+			return err;
 		}
 
-		hgpk_reset_hack_state(psmouse);
-
 		/* should be all set, enable the touchpad */
 		ps2_command(&psmouse->ps2dev, NULL, PSMOUSE_CMD_ENABLE);
 		psmouse_set_state(psmouse, PSMOUSE_ACTIVATED);
@@ -527,25 +609,17 @@ static int hgpk_poll(struct psmouse *psmouse)
 
 static int hgpk_reconnect(struct psmouse *psmouse)
 {
-	int r;
-
-	/* During suspend/resume the ps2 rails remain powered.  We don't want
+	/*
+	 * During suspend/resume the ps2 rails remain powered.  We don't want
 	 * to do a reset because it's flush data out of buffers; however,
-	 * earlier prototypes (B1) had some brokenness that required a reset. */
+	 * earlier prototypes (B1) had some brokenness that required a reset.
+	 */
 	if (olpc_board_at_least(olpc_board(0xb2)))
 		if (psmouse->ps2dev.serio->dev.power.power_state.event !=
 				PM_EVENT_ON)
 			return 0;
 
-	psmouse_reset(psmouse);
-	r = hgpk_select_mode(psmouse);
-	if (r) {
-		hgpk_err(psmouse, "Failed to select mode!\n");
-		return -1;
-	}
-
-	hgpk_reset_hack_state(psmouse);
-	return 0;
+	return hgpk_reset_device(psmouse, false);
 }
 
 static ssize_t hgpk_show_powered(struct psmouse *psmouse, void *data, char *buf)
@@ -584,24 +658,62 @@ __PSMOUSE_DEFINE_ATTR(powered, S_IWUSR | S_IRUGO, NULL,
 
 static ssize_t attr_show_mode(struct psmouse *psmouse, void *data, char *buf)
 {
-	return sprintf(buf, "%s\n", mode_names[hgpk_mode]);
+	struct hgpk_data *priv = psmouse->private;
+
+	return sprintf(buf, "%s\n", hgpk_mode_names[priv->mode]);
 }
 
 static ssize_t attr_set_mode(struct psmouse *psmouse, void *data,
 			     const char *buf, size_t len)
 {
-	int new_mode = hgpk_mode_from_name(buf, len);
+	struct hgpk_data *priv = psmouse->private;
+	enum hgpk_mode old_mode = priv->mode;
+	enum hgpk_mode new_mode = hgpk_mode_from_name(buf, len);
+	struct input_dev *old_dev = psmouse->dev;
+	struct input_dev *new_dev;
+	int err;
 
-	if (new_mode == -1)
+	if (new_mode == HGPK_MODE_INVALID)
 		return -EINVAL;
 
-	hgpk_mode = new_mode;
-	serio_rescan(psmouse->ps2dev.serio);
+	if (old_mode == new_mode)
+		return len;
+
+	new_dev = input_allocate_device();
+	if (!new_dev)
+		return -ENOMEM;
+
+	psmouse_set_state(psmouse, PSMOUSE_INITIALIZING);
+
+	/* Switch device into the new mode */
+	priv->mode = new_mode;
+	err = hgpk_reset_device(psmouse, false);
+	if (err)
+		goto err_try_restore;
+
+	hgpk_setup_input_device(new_dev, old_dev, new_mode);
+
+	psmouse_set_state(psmouse, PSMOUSE_CMD_MODE);
+
+	err = input_register_device(new_dev);
+	if (err)
+		goto err_try_restore;
+
+	psmouse->dev = new_dev;
+	input_unregister_device(old_dev);
+
 	return len;
+
+err_try_restore:
+	input_free_device(new_dev);
+	priv->mode = old_mode;
+	hgpk_reset_device(psmouse, false);
+
+	return err;
 }
 
-__PSMOUSE_DEFINE_ATTR(hgpk_mode, S_IWUSR | S_IRUGO, NULL,
-		      attr_show_mode, attr_set_mode, 0);
+PSMOUSE_DEFINE_ATTR(hgpk_mode, S_IWUSR | S_IRUGO, NULL,
+		    attr_show_mode, attr_set_mode);
 
 static ssize_t hgpk_trigger_recal_show(struct psmouse *psmouse,
 		void *data, char *buf)
@@ -664,7 +776,6 @@ static void hgpk_recalib_work(struct work_struct *work)
 static int hgpk_register(struct psmouse *psmouse)
 {
 	struct hgpk_data *priv = psmouse->private;
-	struct input_dev *idev = psmouse->dev;
 	int err;
 
 	/* register handlers */
@@ -678,38 +789,7 @@ static int hgpk_register(struct psmouse *psmouse)
 	/* Reset after a lot of bad bytes. */
 	psmouse->resetafter = 1024;
 
-	if (priv->mode != HGPK_MODE_MOUSE) {
-		__set_bit(EV_ABS, idev->evbit);
-		__set_bit(EV_KEY, idev->evbit);
-		__set_bit(BTN_TOUCH, idev->keybit);
-		__set_bit(BTN_TOOL_FINGER, idev->keybit);
-		__set_bit(BTN_LEFT, idev->keybit);
-		__set_bit(BTN_RIGHT, idev->keybit);
-		__clear_bit(EV_REL, idev->evbit);
-		__clear_bit(REL_X, idev->relbit);
-		__clear_bit(REL_Y, idev->relbit);
-	}
-
-	if (priv->mode == HGPK_MODE_GLIDESENSOR) {
-		/* GlideSensor has pressure sensor, PenTablet does not */
-		input_set_abs_params(idev, ABS_PRESSURE, 0, 15, 0, 0);
-
-		/* From device specs */
-		input_set_abs_params(idev, ABS_X, 0, 399, 0, 0);
-		input_set_abs_params(idev, ABS_Y, 0, 290, 0, 0);
-
-		/* Calculated by hand based on usable size (52mm x 38mm) */
-		input_abs_set_res(idev, ABS_X, 8);
-		input_abs_set_res(idev, ABS_Y, 8);
-	} else if (priv->mode == HGPK_MODE_PENTABLET) {
-		/* From device specs */
-		input_set_abs_params(idev, ABS_X, 0, 999, 0, 0);
-		input_set_abs_params(idev, ABS_Y, 5, 239, 0, 0);
-
-		/* Calculated by hand based on usable size (156mm x 38mm) */
-		input_abs_set_res(idev, ABS_X, 6);
-		input_abs_set_res(idev, ABS_Y, 8);
-	}
+	hgpk_setup_input_device(psmouse->dev, NULL, priv->mode);
 
 	err = device_create_file(&psmouse->ps2dev.serio->dev,
 				 &psmouse_attr_powered.dattr);
@@ -750,28 +830,25 @@ err_remove_powered:
 int hgpk_init(struct psmouse *psmouse)
 {
 	struct hgpk_data *priv;
-	int err = -ENOMEM;
+	int err;
 
 	priv = kzalloc(sizeof(struct hgpk_data), GFP_KERNEL);
-	if (!priv)
+	if (!priv) {
+		err = -ENOMEM;
 		goto alloc_fail;
+	}
 
 	psmouse->private = priv;
+
 	priv->psmouse = psmouse;
 	priv->powered = true;
-	priv->mode = hgpk_mode;
+	priv->mode = hgpk_default_mode;
 	INIT_DELAYED_WORK(&priv->recalib_wq, hgpk_recalib_work);
 
-	err = psmouse_reset(psmouse);
-	if (err)
-		goto init_fail;
-
-	err = hgpk_select_mode(psmouse);
+	err = hgpk_reset_device(psmouse, false);
 	if (err)
 		goto init_fail;
 
-	hgpk_reset_hack_state(psmouse);
-
 	err = hgpk_register(psmouse);
 	if (err)
 		goto init_fail;
@@ -827,10 +904,11 @@ int hgpk_detect(struct psmouse *psmouse, bool set_properties)
 
 void hgpk_module_init(void)
 {
-	if (hgpk_initial_mode) {
-		int mode = hgpk_mode_from_name(hgpk_initial_mode,
-					       strlen(hgpk_initial_mode));
-		if (mode != -1)
-			hgpk_mode = mode;
+	hgpk_default_mode = hgpk_mode_from_name(hgpk_mode_name,
+						strlen(hgpk_mode_name));
+	if (hgpk_default_mode == HGPK_MODE_INVALID) {
+		hgpk_default_mode = HGPK_MODE_MOUSE;
+		strlcpy(hgpk_mode_name, hgpk_mode_names[HGPK_MODE_MOUSE],
+			sizeof(hgpk_mode_name));
 	}
 }
diff --git a/drivers/input/mouse/hgpk.h b/drivers/input/mouse/hgpk.h
index 46aaeee..01c983b 100644
--- a/drivers/input/mouse/hgpk.h
+++ b/drivers/input/mouse/hgpk.h
@@ -20,11 +20,12 @@ enum hgpk_mode {
 	HGPK_MODE_MOUSE,
 	HGPK_MODE_GLIDESENSOR,
 	HGPK_MODE_PENTABLET,
+	HGPK_MODE_INVALID
 };
 
 struct hgpk_data {
 	struct psmouse *psmouse;
-	int mode;
+	enum hgpk_mode mode;
 	bool powered;
 	int count, x_tally, y_tally;	/* hardware workaround stuff */
 	unsigned long recalib_window;

Re: [PATCH] Input: hgpk - support GlideSensor and PenTablet modes

[Daniel Drake]

On 5 November 2010 19:36, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> I tried merging the snippet that you mention in your last e-mail but I
> think that we should not manually set state to PSMOUSE_ACTIVATED, but
> rather mark the attribute as "protected" and rely on psmouse core to
> disable and re-enable the device.
>
> Does the patch below still work for you?

Yes, that works.

Thanks!
Daniel

Re: [PATCH] Input: hgpk - support GlideSensor and PenTablet modes

[Dmitry Torokhov]

On Sat, Nov 06, 2010 at 08:02:21PM +0000, Daniel Drake wrote:
> On 5 November 2010 19:36, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> > I tried merging the snippet that you mention in your last e-mail but I
> > think that we should not manually set state to PSMOUSE_ACTIVATED, but
> > rather mark the attribute as "protected" and rely on psmouse core to
> > disable and re-enable the device.
> >
> > Does the patch below still work for you?
> 
> Yes, that works.
> 

Great, thanks for testing. Please rebase the other patches that you have
against yours + this one and send them my way.

-- 
Dmitry