[PATCH] Input: Support in the elantech driver of the trackpoint present on for instance Lenovo L530

[PATCH] Input: Support in the elantech driver of the trackpoint present on for instance Lenovo L530

[Ulrik De Bie]

Sorry it took me some time to have an email addres that would allow clean
mails. Below you can find the updated patch previously sent on linux-input
mailinglist. David Herrman asked to resent this patch as a proper git patch so
here it is now. 

It also available on https://github.com/ulrikdb/linux.git elantech_trackpoint
from commit 38dbfb59d1175ef458d006556061adeaa8751b72 (Linus 3.14-rc1) up to 
d69e103a35c944721966105790d14adf79098a4c 

Please when responding please send either to linux-input or put Ulrik De Bie <ulrik.debie-os@e2big.org> in CC:, Thanks.


The Lenovo L530 trackpoint does not work out of the box. It gives sync errors
as shown below when the trackpoint or trackpoint mouse buttons are pressed and
no input is received by userspace:
[   29.010641] psmouse serio1: Touchpad at isa0060/serio1/input0 lost sync at byte 6
The touchpad does work.

The alternative is to do a downgrade to generic ps/2 mouse (modprobe psmouse proto=bare)
but this has the disadvantage that touchpad can't be disabled (I want trackpoint, not touchpad).

With this patch, the trackpoint is provided as another input device; currently called 'TPPS/2 IBM TrackPoint'
The trackpoint now succesfully works and I can disable the touchpad with synclient TouchPadOff=1
The patch will also output messages that do not follow the expected pattern.
In the mean time I've seen 2 unknown packets occasionally:
0x04 , 0x00 , 0x00 , 0x00 , 0x00 , 0x00
0x00 , 0x00 , 0x00 , 0x02 , 0x00 , 0x00
I don't know what those are for, but they can be safely ignored.

Currently all packets that are not known to v3 touchpad and where packet[3] (the fourth byte) lowest
nibble is 6 are now recognized as PACKET_TRACKPOINT and processed by the new elantech_report_trackpoint.

This has been verified to work on a laptop where the touchpad/trackpoint combined identify themselves as:
psmouse serio1: elantech: assuming hardware version 3 (with firmware version 0x350f02)
psmouse serio1: elantech: Synaptics capabilities query result 0xb9, 0x15, 0x0c.

Since I can't send clean email from yahoo, I've switched to a different email address: ulrik.debie-os@e2big.org
Signed-off-by: Ulrik De Bie <ulrik_opensource-kernel@yahoo.com>
Signed-off-by: Ulrik De Bie <ulrik.debie-os@e2big.org>

Signed-off-by: Ulrik De Bie <ulrik.debie-os@e2big.org>
---
 drivers/input/mouse/elantech.c | 78 +++++++++++++++++++++++++++++++++++++++++-
 drivers/input/mouse/elantech.h |  3 ++
 2 files changed, 80 insertions(+), 1 deletion(-)

diff --git a/drivers/input/mouse/elantech.c b/drivers/input/mouse/elantech.c
index ef1cf52..21d693b 100644
--- a/drivers/input/mouse/elantech.c
+++ b/drivers/input/mouse/elantech.c
@@ -402,6 +402,54 @@ static void elantech_report_absolute_v2(struct psmouse *psmouse)
 	input_sync(dev);
 }
 
+static void elantech_report_trackpoint(struct psmouse *psmouse,
+				       int packet_type)
+{
+	/* byte 0:  0   0 ~sx ~sy   0   M   R   L */
+	/* byte 1: sx   0   0   0   0   0   0   0 */
+	/* byte 2: sy   0   0   0   0   0   0   0 */
+	/* byte 3:  0   0  sy  sx   0   1   1   0 */
+	/* byte 4: x7  x6  x5  x4  x3  x2  x1  x0 */
+	/* byte 5: y7  y6  y5  y4  y3  y2  y1  y0 */
+
+	/*
+	 * x and y are written in two's complement spread
+	 * over 9 bits with sx/sy the relative top bit and
+	 * x7..x0 and y7..y0 the lower bits.
+	 * The sign of y is opposite to what the input driver
+	 * expects for a relative movement
+	 */
+
+	struct elantech_data *etd = psmouse->private;
+	struct input_dev *dev2 = etd->dev2;
+	unsigned char *packet = psmouse->packet;
+	int x, y;
+	input_report_key(dev2, BTN_LEFT, packet[0] & 0x01);
+	input_report_key(dev2, BTN_RIGHT, packet[0] & 0x02);
+	input_report_key(dev2, BTN_MIDDLE, packet[0] & 0x04);
+	x = (s32) ((u32) ((packet[1] & 0x80) ? 0UL : 0xFFFFFF00UL) | (u32)
+		   packet[4]);
+	y = -(s32) ((u32) ((packet[2] & 0x80) ? 0UL : 0xFFFFFF00UL) | (u32)
+		    packet[5]);
+	input_report_rel(dev2, REL_X, x);
+	input_report_rel(dev2, REL_Y, y);
+	switch ((((u32) packet[0] & 0xF8) << 24) | ((u32) packet[1] << 16)
+		| (u32) packet[2] << 8 | (u32) packet[3]) {
+	case 0x00808036UL:
+	case 0x10008026UL:
+	case 0x20800016UL:
+	case 0x30000006UL:
+		break;
+	default:
+		/* Dump unexpected packet sequences if debug=1 (default) */
+		if (etd->debug == 1)
+			elantech_packet_dump(psmouse);
+		break;
+	}
+
+	input_sync(dev2);
+}
+
 /*
  * Interpret complete data packets and report absolute mode input events for
  * hardware version 3. (12 byte packets for two fingers)
@@ -700,8 +748,11 @@ static int elantech_packet_check_v3(struct psmouse *psmouse)
 
 		if ((packet[0] & 0x0c) == 0x0c && (packet[3] & 0xce) == 0x0c)
 			return PACKET_V3_TAIL;
+		if ((packet[3]&0x0f) == 0x06)
+			return PACKET_TRACKPOINT;
 	}
 
+
 	return PACKET_UNKNOWN;
 }
 
@@ -783,7 +834,10 @@ static psmouse_ret_t elantech_process_byte(struct psmouse *psmouse)
 		if (packet_type == PACKET_UNKNOWN)
 			return PSMOUSE_BAD_DATA;
 
-		elantech_report_absolute_v3(psmouse, packet_type);
+		if (packet_type == PACKET_TRACKPOINT)
+			elantech_report_trackpoint(psmouse, packet_type);
+		else
+			elantech_report_absolute_v3(psmouse, packet_type);
 		break;
 
 	case 4:
@@ -1400,10 +1454,15 @@ int elantech_init(struct psmouse *psmouse)
 	struct elantech_data *etd;
 	int i, error;
 	unsigned char param[3];
+	struct input_dev *dev2;
 
 	psmouse->private = etd = kzalloc(sizeof(struct elantech_data), GFP_KERNEL);
 	if (!etd)
 		return -ENOMEM;
+	dev2 = input_allocate_device();
+	if (!dev2)
+		goto init_fail;
+	etd->dev2 = dev2;
 
 	psmouse_reset(psmouse);
 
@@ -1463,9 +1522,26 @@ int elantech_init(struct psmouse *psmouse)
 	psmouse->reconnect = elantech_reconnect;
 	psmouse->pktsize = etd->hw_version > 1 ? 6 : 4;
 
+	snprintf(etd->phys, sizeof(etd->phys), "%s/input1",
+		psmouse->ps2dev.serio->phys);
+	dev2->phys = etd->phys;
+	dev2->name = "TPPS/2 IBM TrackPoint";
+	dev2->id.bustype = BUS_I8042;
+	dev2->id.vendor  = 0x0002;
+	dev2->id.product = PSMOUSE_ELANTECH;
+	dev2->id.version = 0x0000;
+	dev2->dev.parent = &psmouse->ps2dev.serio->dev;
+	dev2->evbit[0] = BIT_MASK(EV_KEY) | BIT_MASK(EV_REL);
+	dev2->relbit[BIT_WORD(REL_X)] = BIT_MASK(REL_X) | BIT_MASK(REL_Y);
+	dev2->keybit[BIT_WORD(BTN_LEFT)] =
+		BIT_MASK(BTN_LEFT) | BIT_MASK(BTN_MIDDLE) | BIT_MASK(BTN_RIGHT);
+
+	if (input_register_device(etd->dev2))
+		goto init_fail;
 	return 0;
 
  init_fail:
+	input_free_device(dev2);
 	kfree(etd);
 	return -1;
 }
diff --git a/drivers/input/mouse/elantech.h b/drivers/input/mouse/elantech.h
index 036a04a..27cf191 100644
--- a/drivers/input/mouse/elantech.h
+++ b/drivers/input/mouse/elantech.h
@@ -94,6 +94,7 @@
 #define PACKET_V4_HEAD			0x05
 #define PACKET_V4_MOTION		0x06
 #define PACKET_V4_STATUS		0x07
+#define PACKET_TRACKPOINT		0x08
 
 /*
  * track up to 5 fingers for v4 hardware
@@ -114,6 +115,8 @@ struct finger_pos {
 };
 
 struct elantech_data {
+	struct input_dev *dev2;		/* Relative device */
+	char	phys[32];
 	unsigned char reg_07;
 	unsigned char reg_10;
 	unsigned char reg_11;
-- 
1.8.5.3

Re: [PATCH] Input: Support in the elantech driver of the trackpoint present on for instance Lenovo L530

[ulrik.debie-os]

Hi Dmitry,

Could you please have a look at this patch. It solves the bugzilla bug
https://bugzilla.kernel.org/show_bug.cgi?id=48161

Thanks,
Best regards,
Ulrik De Bie

On Fri, Feb 14, 2014 at 09:51:26PM +0100, Ulrik De Bie wrote:
> Date:	Fri, 14 Feb 2014 21:51:26 +0100
> From: Ulrik De Bie <ulrik.debie-os@e2big.org>
> To: linux-input@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org, Dmitry Torokhov
>  <dmitry.torokhov@gmail.com>, David Herrmann <dh.herrmann@gmail.com>,
>  Jonathan Aquilina <eagles051387@gmail.com>, Ulrik De Bie
>  <ulrik_opensource-kernel@yahoo.com>
> Subject: [PATCH] Input: Support in the elantech driver of the trackpoint
>  present on for instance Lenovo L530
> X-Mailing-List:	linux-input@vger.kernel.org
> 
> Sorry it took me some time to have an email addres that would allow clean
> mails. Below you can find the updated patch previously sent on linux-input
> mailinglist. David Herrman asked to resent this patch as a proper git patch so
> here it is now. 
> 
> It also available on https://github.com/ulrikdb/linux.git elantech_trackpoint
> from commit 38dbfb59d1175ef458d006556061adeaa8751b72 (Linus 3.14-rc1) up to 
> d69e103a35c944721966105790d14adf79098a4c 
> 
> Please when responding please send either to linux-input or put Ulrik De Bie <ulrik.debie-os@e2big.org> in CC:, Thanks.
> 
> 
> The Lenovo L530 trackpoint does not work out of the box. It gives sync errors
> as shown below when the trackpoint or trackpoint mouse buttons are pressed and
> no input is received by userspace:
> [   29.010641] psmouse serio1: Touchpad at isa0060/serio1/input0 lost sync at byte 6
> The touchpad does work.
> 
> The alternative is to do a downgrade to generic ps/2 mouse (modprobe psmouse proto=bare)
> but this has the disadvantage that touchpad can't be disabled (I want trackpoint, not touchpad).
> 
> With this patch, the trackpoint is provided as another input device; currently called 'TPPS/2 IBM TrackPoint'
> The trackpoint now succesfully works and I can disable the touchpad with synclient TouchPadOff=1
> The patch will also output messages that do not follow the expected pattern.
> In the mean time I've seen 2 unknown packets occasionally:
> 0x04 , 0x00 , 0x00 , 0x00 , 0x00 , 0x00
> 0x00 , 0x00 , 0x00 , 0x02 , 0x00 , 0x00
> I don't know what those are for, but they can be safely ignored.
> 
> Currently all packets that are not known to v3 touchpad and where packet[3] (the fourth byte) lowest
> nibble is 6 are now recognized as PACKET_TRACKPOINT and processed by the new elantech_report_trackpoint.
> 
> This has been verified to work on a laptop where the touchpad/trackpoint combined identify themselves as:
> psmouse serio1: elantech: assuming hardware version 3 (with firmware version 0x350f02)
> psmouse serio1: elantech: Synaptics capabilities query result 0xb9, 0x15, 0x0c.
> 
> Since I can't send clean email from yahoo, I've switched to a different email address: ulrik.debie-os@e2big.org
> Signed-off-by: Ulrik De Bie <ulrik_opensource-kernel@yahoo.com>
> Signed-off-by: Ulrik De Bie <ulrik.debie-os@e2big.org>
> 
> Signed-off-by: Ulrik De Bie <ulrik.debie-os@e2big.org>
> ---
>  drivers/input/mouse/elantech.c | 78 +++++++++++++++++++++++++++++++++++++++++-
>  drivers/input/mouse/elantech.h |  3 ++
>  2 files changed, 80 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/input/mouse/elantech.c b/drivers/input/mouse/elantech.c
> index ef1cf52..21d693b 100644
> --- a/drivers/input/mouse/elantech.c
> +++ b/drivers/input/mouse/elantech.c
> @@ -402,6 +402,54 @@ static void elantech_report_absolute_v2(struct psmouse *psmouse)
>  	input_sync(dev);
>  }
>  
> +static void elantech_report_trackpoint(struct psmouse *psmouse,
> +				       int packet_type)
> +{
> +	/* byte 0:  0   0 ~sx ~sy   0   M   R   L */
> +	/* byte 1: sx   0   0   0   0   0   0   0 */
> +	/* byte 2: sy   0   0   0   0   0   0   0 */
> +	/* byte 3:  0   0  sy  sx   0   1   1   0 */
> +	/* byte 4: x7  x6  x5  x4  x3  x2  x1  x0 */
> +	/* byte 5: y7  y6  y5  y4  y3  y2  y1  y0 */
> +
> +	/*
> +	 * x and y are written in two's complement spread
> +	 * over 9 bits with sx/sy the relative top bit and
> +	 * x7..x0 and y7..y0 the lower bits.
> +	 * The sign of y is opposite to what the input driver
> +	 * expects for a relative movement
> +	 */
> +
> +	struct elantech_data *etd = psmouse->private;
> +	struct input_dev *dev2 = etd->dev2;
> +	unsigned char *packet = psmouse->packet;
> +	int x, y;
> +	input_report_key(dev2, BTN_LEFT, packet[0] & 0x01);
> +	input_report_key(dev2, BTN_RIGHT, packet[0] & 0x02);
> +	input_report_key(dev2, BTN_MIDDLE, packet[0] & 0x04);
> +	x = (s32) ((u32) ((packet[1] & 0x80) ? 0UL : 0xFFFFFF00UL) | (u32)
> +		   packet[4]);
> +	y = -(s32) ((u32) ((packet[2] & 0x80) ? 0UL : 0xFFFFFF00UL) | (u32)
> +		    packet[5]);
> +	input_report_rel(dev2, REL_X, x);
> +	input_report_rel(dev2, REL_Y, y);
> +	switch ((((u32) packet[0] & 0xF8) << 24) | ((u32) packet[1] << 16)
> +		| (u32) packet[2] << 8 | (u32) packet[3]) {
> +	case 0x00808036UL:
> +	case 0x10008026UL:
> +	case 0x20800016UL:
> +	case 0x30000006UL:
> +		break;
> +	default:
> +		/* Dump unexpected packet sequences if debug=1 (default) */
> +		if (etd->debug == 1)
> +			elantech_packet_dump(psmouse);
> +		break;
> +	}
> +
> +	input_sync(dev2);
> +}
> +
>  /*
>   * Interpret complete data packets and report absolute mode input events for
>   * hardware version 3. (12 byte packets for two fingers)
> @@ -700,8 +748,11 @@ static int elantech_packet_check_v3(struct psmouse *psmouse)
>  
>  		if ((packet[0] & 0x0c) == 0x0c && (packet[3] & 0xce) == 0x0c)
>  			return PACKET_V3_TAIL;
> +		if ((packet[3]&0x0f) == 0x06)
> +			return PACKET_TRACKPOINT;
>  	}
>  
> +
>  	return PACKET_UNKNOWN;
>  }
>  
> @@ -783,7 +834,10 @@ static psmouse_ret_t elantech_process_byte(struct psmouse *psmouse)
>  		if (packet_type == PACKET_UNKNOWN)
>  			return PSMOUSE_BAD_DATA;
>  
> -		elantech_report_absolute_v3(psmouse, packet_type);
> +		if (packet_type == PACKET_TRACKPOINT)
> +			elantech_report_trackpoint(psmouse, packet_type);
> +		else
> +			elantech_report_absolute_v3(psmouse, packet_type);
>  		break;
>  
>  	case 4:
> @@ -1400,10 +1454,15 @@ int elantech_init(struct psmouse *psmouse)
>  	struct elantech_data *etd;
>  	int i, error;
>  	unsigned char param[3];
> +	struct input_dev *dev2;
>  
>  	psmouse->private = etd = kzalloc(sizeof(struct elantech_data), GFP_KERNEL);
>  	if (!etd)
>  		return -ENOMEM;
> +	dev2 = input_allocate_device();
> +	if (!dev2)
> +		goto init_fail;
> +	etd->dev2 = dev2;
>  
>  	psmouse_reset(psmouse);
>  
> @@ -1463,9 +1522,26 @@ int elantech_init(struct psmouse *psmouse)
>  	psmouse->reconnect = elantech_reconnect;
>  	psmouse->pktsize = etd->hw_version > 1 ? 6 : 4;
>  
> +	snprintf(etd->phys, sizeof(etd->phys), "%s/input1",
> +		psmouse->ps2dev.serio->phys);
> +	dev2->phys = etd->phys;
> +	dev2->name = "TPPS/2 IBM TrackPoint";
> +	dev2->id.bustype = BUS_I8042;
> +	dev2->id.vendor  = 0x0002;
> +	dev2->id.product = PSMOUSE_ELANTECH;
> +	dev2->id.version = 0x0000;
> +	dev2->dev.parent = &psmouse->ps2dev.serio->dev;
> +	dev2->evbit[0] = BIT_MASK(EV_KEY) | BIT_MASK(EV_REL);
> +	dev2->relbit[BIT_WORD(REL_X)] = BIT_MASK(REL_X) | BIT_MASK(REL_Y);
> +	dev2->keybit[BIT_WORD(BTN_LEFT)] =
> +		BIT_MASK(BTN_LEFT) | BIT_MASK(BTN_MIDDLE) | BIT_MASK(BTN_RIGHT);
> +
> +	if (input_register_device(etd->dev2))
> +		goto init_fail;
>  	return 0;
>  
>   init_fail:
> +	input_free_device(dev2);
>  	kfree(etd);
>  	return -1;
>  }
> diff --git a/drivers/input/mouse/elantech.h b/drivers/input/mouse/elantech.h
> index 036a04a..27cf191 100644
> --- a/drivers/input/mouse/elantech.h
> +++ b/drivers/input/mouse/elantech.h
> @@ -94,6 +94,7 @@
>  #define PACKET_V4_HEAD			0x05
>  #define PACKET_V4_MOTION		0x06
>  #define PACKET_V4_STATUS		0x07
> +#define PACKET_TRACKPOINT		0x08
>  
>  /*
>   * track up to 5 fingers for v4 hardware
> @@ -114,6 +115,8 @@ struct finger_pos {
>  };
>  
>  struct elantech_data {
> +	struct input_dev *dev2;		/* Relative device */
> +	char	phys[32];
>  	unsigned char reg_07;
>  	unsigned char reg_10;
>  	unsigned char reg_11;
> -- 
> 1.8.5.3
> 
> --
> 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

[PATCH v2 0/1] Input: Support in the elantech driver of the trackpoint present on for instance Lenovo L530

[Ulrik De Bie]

This patch adds support for trackpoint on elantech driver for v3 models

Changes since v1:
* New patch now with reference to 3.14rc1
* Added etd->trackpoint_present to indicate presence of trackpoint (based
  on MSB of etd->capabilities[0])
* trackpoint will only be registered now when MSB of etd->capabilities[0] is
  set; got confirmation that this is the indicator of trackpoint
* Added input_unregister_device/input_free_device in elantech_disconnect()
* Fixed a bug in cleaning up when elantech_init fails
* Rename commit to be more specific (now also applicable to future elantech
  v3 models with trackpoint)
* input device name 'TPPS/2 IBM TrackPoint' changed to
  'Elantech PS/2 TrackPoint', this patch is not ibm/lenovo specific!
* dev2 renamed to tp_dev to indicate that this is the trackpoint device
* etd->phys renamed to etd->tp_phys
* Added Lenovo 530 and Fujitsu H730 to the laptop list because those are now
  also known.
* Added psmouse_reset at the end of elantech_init when it fails
* Added warning when trackpoint packets are received with no trackpoint detected

The patch is also available from:
https://github.com/ulrikdb/linux/commit/74f8d3a9307c109ae40c02072dc9c16d3557c3d4

Ulrik De Bie (1):
  elantech: Add support for trackpoint found on some v3 models

 drivers/input/mouse/elantech.c | 104 +++++++++++++++++++++++++++++++++++++++--
 drivers/input/mouse/elantech.h |   4 ++
 2 files changed, 105 insertions(+), 3 deletions(-)

-- 
2.0.0.rc2

[PATCH v2 1/1] elantech: Add support for trackpoint found on some v3 models

[Ulrik De Bie]

Some elantech v3 touchpad equipped laptops also have a trackpoint, before
this commit, these give sync errors. With this patch, the trackpoint is
provided as another input device: 'Elantech PS/2 TrackPoint'

The patch will also output messages that do not follow the expected pattern.
In the mean time I've seen 2 unknown packets occasionally:
0x04 , 0x00 , 0x00 , 0x00 , 0x00 , 0x00
0x00 , 0x00 , 0x00 , 0x02 , 0x00 , 0x00
I don't know what those are for, but they can be safely ignored.

Currently all packets that are not known to v3 touchpad and where
packet[3] (the fourth byte) lowest nibble is 6 are now recognized as
PACKET_TRACKPOINT and processed by the new elantech_report_trackpoint.

This has been verified to work on a laptop Lenovo L530 where the
touchpad/trackpoint combined identify themselves as:
psmouse serio1: elantech: assuming hardware version 3 (with firmware version 0x350f02)
psmouse serio1: elantech: Synaptics capabilities query result 0xb9, 0x15, 0x0c.

Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Cc: stable@vger.kernel.org
Signed-off-by: Ulrik De Bie <ulrik.debie-os@e2big.org>
---
 drivers/input/mouse/elantech.c | 104 +++++++++++++++++++++++++++++++++++++++--
 drivers/input/mouse/elantech.h |   4 ++
 2 files changed, 105 insertions(+), 3 deletions(-)

diff --git a/drivers/input/mouse/elantech.c b/drivers/input/mouse/elantech.c
index ee2a04d..7faec12 100644
--- a/drivers/input/mouse/elantech.c
+++ b/drivers/input/mouse/elantech.c
@@ -403,6 +403,63 @@ static void elantech_report_absolute_v2(struct psmouse *psmouse)
 	input_sync(dev);
 }
 
+static void elantech_report_trackpoint(struct psmouse *psmouse,
+				       int packet_type)
+{
+	/* byte 0:  0   0 ~sx ~sy   0   M   R   L */
+	/* byte 1: sx   0   0   0   0   0   0   0 */
+	/* byte 2: sy   0   0   0   0   0   0   0 */
+	/* byte 3:  0   0  sy  sx   0   1   1   0 */
+	/* byte 4: x7  x6  x5  x4  x3  x2  x1  x0 */
+	/* byte 5: y7  y6  y5  y4  y3  y2  y1  y0 */
+
+	/*
+	 * x and y are written in two's complement spread
+	 * over 9 bits with sx/sy the relative top bit and
+	 * x7..x0 and y7..y0 the lower bits.
+	 * The sign of y is opposite to what the input driver
+	 * expects for a relative movement
+	 */
+
+	struct elantech_data *etd = psmouse->private;
+	struct input_dev *tp_dev = etd->tp_dev;
+	unsigned char *packet = psmouse->packet;
+	int x, y;
+
+	if (!etd->trackpoint_present) {
+		psmouse_err(psmouse, "Unexpected trackpoint message\n");
+		if (etd->debug == 1)
+			elantech_packet_dump(psmouse);
+		return;
+	}
+
+	input_report_key(tp_dev, BTN_LEFT, packet[0] & 0x01);
+	input_report_key(tp_dev, BTN_RIGHT, packet[0] & 0x02);
+	input_report_key(tp_dev, BTN_MIDDLE, packet[0] & 0x04);
+	x = (s32) ((u32) ((packet[1] & 0x80) ? 0UL : 0xFFFFFF00UL) | (u32)
+		   packet[4]);
+	y = -(s32) ((u32) ((packet[2] & 0x80) ? 0UL : 0xFFFFFF00UL) | (u32)
+		    packet[5]);
+	input_report_rel(tp_dev, REL_X, x);
+	input_report_rel(tp_dev, REL_Y, y);
+
+	switch ((((u32) packet[0] & 0xF8) << 24) | ((u32) packet[1] << 16)
+		| (u32) packet[2] << 8 | (u32) packet[3]) {
+	case 0x00808036UL:
+	case 0x10008026UL:
+	case 0x20800016UL:
+	case 0x30000006UL:
+		break;
+	default:
+		/* Dump unexpected packet sequences if debug=1 (default) */
+		if (etd->debug == 1)
+			elantech_packet_dump(psmouse);
+		break;
+	}
+
+	input_sync(tp_dev);
+}
+
 /*
  * Interpret complete data packets and report absolute mode input events for
  * hardware version 3. (12 byte packets for two fingers)
@@ -715,6 +772,8 @@ static int elantech_packet_check_v3(struct psmouse *psmouse)
 
 		if ((packet[0] & 0x0c) == 0x0c && (packet[3] & 0xce) == 0x0c)
 			return PACKET_V3_TAIL;
+		if ((packet[3]&0x0f) == 0x06)
+			return PACKET_TRACKPOINT;
 	}
 
 	return PACKET_UNKNOWN;
@@ -798,7 +857,10 @@ static psmouse_ret_t elantech_process_byte(struct psmouse *psmouse)
 		if (packet_type == PACKET_UNKNOWN)
 			return PSMOUSE_BAD_DATA;
 
-		elantech_report_absolute_v3(psmouse, packet_type);
+		if (packet_type == PACKET_TRACKPOINT)
+			elantech_report_trackpoint(psmouse, packet_type);
+		else
+			elantech_report_absolute_v3(psmouse, packet_type);
 		break;
 
 	case 4:
@@ -1018,8 +1080,10 @@ static int elantech_get_resolution_v4(struct psmouse *psmouse,
  * Asus UX31               0x361f00        20, 15, 0e      clickpad
  * Asus UX32VD             0x361f02        00, 15, 0e      clickpad
  * Avatar AVIU-145A2       0x361f00        ?               clickpad
+ * Fujitsu H730            0x570f00        c0, 14, 0c      3 hw buttons (**)
  * Gigabyte U2442          0x450f01        58, 17, 0c      2 hw buttons
  * Lenovo L430             0x350f02        b9, 15, 0c      2 hw buttons (*)
+ * Lenovo L530             0x350f02        b9, 15, 0c      2 hw buttons (*) 
  * Samsung NF210           0x150b00        78, 14, 0a      2 hw buttons
  * Samsung NP770Z5E        0x575f01        10, 15, 0f      clickpad
  * Samsung NP700Z5B        0x361f06        21, 15, 0f      clickpad
@@ -1029,6 +1093,8 @@ static int elantech_get_resolution_v4(struct psmouse *psmouse,
  * Samsung RF710           0x450f00        ?               2 hw buttons
  * System76 Pangolin       0x250f01        ?               2 hw buttons
  * (*) + 3 trackpoint buttons
+ * (**) + 0 trackpoint buttons
+ * Note: Lenovo L430 and Lenovo L430 have the same fw_version/caps
  */
 static void elantech_set_buttonpad_prop(struct psmouse *psmouse)
 {
@@ -1324,6 +1390,11 @@ int elantech_detect(struct psmouse *psmouse, bool set_properties)
  */
 static void elantech_disconnect(struct psmouse *psmouse)
 {
+	struct elantech_data *etd = psmouse->private;
+	struct input_dev *tp_dev = etd->tp_dev;
+
+	if (etd->trackpoint_present && tp_dev) 
+		input_unregister_device(tp_dev);
 	sysfs_remove_group(&psmouse->ps2dev.serio->dev.kobj,
 			   &elantech_attr_group);
 	kfree(psmouse->private);
@@ -1440,11 +1511,11 @@ int elantech_init(struct psmouse *psmouse)
 	struct elantech_data *etd;
 	int i, error;
 	unsigned char param[3];
+	struct input_dev *tp_dev;
 
 	psmouse->private = etd = kzalloc(sizeof(struct elantech_data), GFP_KERNEL);
 	if (!etd)
 		return -ENOMEM;
-
 	psmouse_reset(psmouse);
 
 	etd->parity[0] = 1;
@@ -1497,6 +1568,28 @@ int elantech_init(struct psmouse *psmouse)
 			    error);
 		goto init_fail;
 	}
+	etd->trackpoint_present = ((etd->capabilities[0] & 0x80) == 0x80);
+	if (etd->trackpoint_present) {
+		tp_dev = input_allocate_device();
+		if (!tp_dev)
+			goto init_fail_tp_alloc;
+		etd->tp_dev = tp_dev;
+		snprintf(etd->tp_phys, sizeof(etd->tp_phys), "%s/input1",
+			psmouse->ps2dev.serio->phys);
+		tp_dev->phys = etd->tp_phys;
+		tp_dev->name = "Elantech PS/2 TrackPoint";
+		tp_dev->id.bustype = BUS_I8042;
+		tp_dev->id.vendor  = 0x0002;
+		tp_dev->id.product = PSMOUSE_ELANTECH;
+		tp_dev->id.version = 0x0000;
+		tp_dev->dev.parent = &psmouse->ps2dev.serio->dev;
+		tp_dev->evbit[0] = BIT_MASK(EV_KEY) | BIT_MASK(EV_REL);
+		tp_dev->relbit[BIT_WORD(REL_X)] = BIT_MASK(REL_X) | BIT_MASK(REL_Y);
+		tp_dev->keybit[BIT_WORD(BTN_LEFT)] =
+			BIT_MASK(BTN_LEFT) | BIT_MASK(BTN_MIDDLE) | BIT_MASK(BTN_RIGHT);
+		if (input_register_device(etd->tp_dev))
+			goto init_fail_tp_reg;
+	}
 
 	psmouse->protocol_handler = elantech_process_byte;
 	psmouse->disconnect = elantech_disconnect;
@@ -1504,8 +1597,13 @@ int elantech_init(struct psmouse *psmouse)
 	psmouse->pktsize = etd->hw_version > 1 ? 6 : 4;
 
 	return 0;
-
+ init_fail_tp_reg:
+	input_free_device(tp_dev);
+ init_fail_tp_alloc:
+	sysfs_remove_group(&psmouse->ps2dev.serio->dev.kobj,
+			   &elantech_attr_group);
  init_fail:
+        psmouse_reset(psmouse);
 	kfree(etd);
 	return -1;
 }
diff --git a/drivers/input/mouse/elantech.h b/drivers/input/mouse/elantech.h
index 9e0e2a1..25cbc8c 100644
--- a/drivers/input/mouse/elantech.h
+++ b/drivers/input/mouse/elantech.h
@@ -94,6 +94,7 @@
 #define PACKET_V4_HEAD			0x05
 #define PACKET_V4_MOTION		0x06
 #define PACKET_V4_STATUS		0x07
+#define PACKET_TRACKPOINT		0x08
 
 /*
  * track up to 5 fingers for v4 hardware
@@ -114,6 +115,8 @@ struct finger_pos {
 };
 
 struct elantech_data {
+	struct input_dev *tp_dev;		/* Relative device */
+	char	tp_phys[32];
 	unsigned char reg_07;
 	unsigned char reg_10;
 	unsigned char reg_11;
@@ -130,6 +133,7 @@ struct elantech_data {
 	bool jumpy_cursor;
 	bool reports_pressure;
 	bool crc_enabled;
+	bool trackpoint_present;
 	bool set_hw_resolution;
 	unsigned char hw_version;
 	unsigned int fw_version;
-- 
2.0.0.rc2

Re: [PATCH v2 1/1] elantech: Add support for trackpoint found on some v3 models

[Greg KH]

On Thu, Jun 12, 2014 at 08:15:01PM +0200, Ulrik De Bie wrote:
> Some elantech v3 touchpad equipped laptops also have a trackpoint, before
> this commit, these give sync errors. With this patch, the trackpoint is
> provided as another input device: 'Elantech PS/2 TrackPoint'
> 
> The patch will also output messages that do not follow the expected pattern.
> In the mean time I've seen 2 unknown packets occasionally:
> 0x04 , 0x00 , 0x00 , 0x00 , 0x00 , 0x00
> 0x00 , 0x00 , 0x00 , 0x02 , 0x00 , 0x00
> I don't know what those are for, but they can be safely ignored.
> 
> Currently all packets that are not known to v3 touchpad and where
> packet[3] (the fourth byte) lowest nibble is 6 are now recognized as
> PACKET_TRACKPOINT and processed by the new elantech_report_trackpoint.
> 
> This has been verified to work on a laptop Lenovo L530 where the
> touchpad/trackpoint combined identify themselves as:
> psmouse serio1: elantech: assuming hardware version 3 (with firmware version 0x350f02)
> psmouse serio1: elantech: Synaptics capabilities query result 0xb9, 0x15, 0x0c.
> 
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Ulrik De Bie <ulrik.debie-os@e2big.org>
> ---
>  drivers/input/mouse/elantech.c | 104 +++++++++++++++++++++++++++++++++++++++--
>  drivers/input/mouse/elantech.h |   4 ++
>  2 files changed, 105 insertions(+), 3 deletions(-)

This seems like a new feature to me and not a stable kernel patch.  How
does this fit in with the Documentation/stable_kernel_rules.txt
requirements?

thanks,

greg k-h

Re: [PATCH v2 1/1] elantech: Add support for trackpoint found on some v3 models

[ulrik.debie-os]

Hi Greg,

Without this patch the elantech.c driver will still bind to the
touchpad, and then get out of sync each time the user touches
the trackpoint or the 3 mouse buttons associated with the trackpoint.

This out of sync will result in a few dmesg lines and a few lost
touchpad input events. The fact that the trackpoint + buttons itself
do not work at all is a minor annoyance compared to the sync loss
effect on the touchpad events.

This resulted in multiple bug reports with patches:
http://tojaj.com/fedora-20-howto-fix-elantech-trackpoint-on-lenovo-thinkpad-l430/
http://permalink.gmane.org/gmane.linux.kernel.input/27763
https://bugzilla.kernel.org/show_bug.cgi?id=48161

Hans suggested me to consider a cc:stable.

Thank you,
Greetings,
Ulrik


On Thu, Jun 12, 2014 at 11:48:44AM -0700, Greg KH wrote:
> On Thu, Jun 12, 2014 at 08:15:01PM +0200, Ulrik De Bie wrote:
> > Some elantech v3 touchpad equipped laptops also have a trackpoint, before
> > this commit, these give sync errors. With this patch, the trackpoint is
> > provided as another input device: 'Elantech PS/2 TrackPoint'
> > 
> > The patch will also output messages that do not follow the expected pattern.
> > In the mean time I've seen 2 unknown packets occasionally:
> > 0x04 , 0x00 , 0x00 , 0x00 , 0x00 , 0x00
> > 0x00 , 0x00 , 0x00 , 0x02 , 0x00 , 0x00
> > I don't know what those are for, but they can be safely ignored.
> > 
> > Currently all packets that are not known to v3 touchpad and where
> > packet[3] (the fourth byte) lowest nibble is 6 are now recognized as
> > PACKET_TRACKPOINT and processed by the new elantech_report_trackpoint.
> > 
> > This has been verified to work on a laptop Lenovo L530 where the
> > touchpad/trackpoint combined identify themselves as:
> > psmouse serio1: elantech: assuming hardware version 3 (with firmware version 0x350f02)
> > psmouse serio1: elantech: Synaptics capabilities query result 0xb9, 0x15, 0x0c.
> > 
> > Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Ulrik De Bie <ulrik.debie-os@e2big.org>
> > ---
> >  drivers/input/mouse/elantech.c | 104 +++++++++++++++++++++++++++++++++++++++--
> >  drivers/input/mouse/elantech.h |   4 ++
> >  2 files changed, 105 insertions(+), 3 deletions(-)
> 
> This seems like a new feature to me and not a stable kernel patch.  How
> does this fit in with the Documentation/stable_kernel_rules.txt
> requirements?
> 
> thanks,
> 
> greg k-h

Re: [PATCH v2 1/1] elantech: Add support for trackpoint found on some v3 models

[Greg KH]

On Thu, Jun 12, 2014 at 09:03:45PM +0200, ulrik.debie-os@e2big.org wrote:
> Hi Greg,
> 
> Without this patch the elantech.c driver will still bind to the
> touchpad, and then get out of sync each time the user touches
> the trackpoint or the 3 mouse buttons associated with the trackpoint.
> 
> This out of sync will result in a few dmesg lines and a few lost
> touchpad input events. The fact that the trackpoint + buttons itself
> do not work at all is a minor annoyance compared to the sync loss
> effect on the touchpad events.
> 
> This resulted in multiple bug reports with patches:
> http://tojaj.com/fedora-20-howto-fix-elantech-trackpoint-on-lenovo-thinkpad-l430/
> http://permalink.gmane.org/gmane.linux.kernel.input/27763
> https://bugzilla.kernel.org/show_bug.cgi?id=48161
> 
> Hans suggested me to consider a cc:stable.

Broken hardware is never nice, but as this isn't a regression, and has
never worked, asking to update to a new kernel seems reasonable.

thanks,

greg k-h

Re: [PATCH v2 1/1] elantech: Add support for trackpoint found on some v3 models

[David Herrmann]

Hi

On Thu, Jun 12, 2014 at 8:15 PM, Ulrik De Bie <ulrik.debie-os@e2big.org> wrote:
> Some elantech v3 touchpad equipped laptops also have a trackpoint, before
> this commit, these give sync errors. With this patch, the trackpoint is
> provided as another input device: 'Elantech PS/2 TrackPoint'
>
> The patch will also output messages that do not follow the expected pattern.
> In the mean time I've seen 2 unknown packets occasionally:
> 0x04 , 0x00 , 0x00 , 0x00 , 0x00 , 0x00
> 0x00 , 0x00 , 0x00 , 0x02 , 0x00 , 0x00
> I don't know what those are for, but they can be safely ignored.
>
> Currently all packets that are not known to v3 touchpad and where
> packet[3] (the fourth byte) lowest nibble is 6 are now recognized as
> PACKET_TRACKPOINT and processed by the new elantech_report_trackpoint.
>
> This has been verified to work on a laptop Lenovo L530 where the
> touchpad/trackpoint combined identify themselves as:
> psmouse serio1: elantech: assuming hardware version 3 (with firmware version 0x350f02)
> psmouse serio1: elantech: Synaptics capabilities query result 0xb9, 0x15, 0x0c.

Thanks for pushing on this. Patch looks good to me, mostly minor nitpicks below.

> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Ulrik De Bie <ulrik.debie-os@e2big.org>
> ---
>  drivers/input/mouse/elantech.c | 104 +++++++++++++++++++++++++++++++++++++++--
>  drivers/input/mouse/elantech.h |   4 ++
>  2 files changed, 105 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/input/mouse/elantech.c b/drivers/input/mouse/elantech.c
> index ee2a04d..7faec12 100644
> --- a/drivers/input/mouse/elantech.c
> +++ b/drivers/input/mouse/elantech.c
> @@ -403,6 +403,63 @@ static void elantech_report_absolute_v2(struct psmouse *psmouse)
>         input_sync(dev);
>  }
>
> +static void elantech_report_trackpoint(struct psmouse *psmouse,
> +                                      int packet_type)
> +{
> +       /* byte 0:  0   0 ~sx ~sy   0   M   R   L */
> +       /* byte 1: sx   0   0   0   0   0   0   0 */
> +       /* byte 2: sy   0   0   0   0   0   0   0 */
> +       /* byte 3:  0   0  sy  sx   0   1   1   0 */
> +       /* byte 4: x7  x6  x5  x4  x3  x2  x1  x0 */
> +       /* byte 5: y7  y6  y5  y4  y3  y2  y1  y0 */

Please make this a proper multi-byte comment instead of 'n'-comments.

> +
> +       /*
> +        * x and y are written in two's complement spread
> +        * over 9 bits with sx/sy the relative top bit and
> +        * x7..x0 and y7..y0 the lower bits.
> +        * The sign of y is opposite to what the input driver
> +        * expects for a relative movement
> +        */
> +
> +       struct elantech_data *etd = psmouse->private;
> +       struct input_dev *tp_dev = etd->tp_dev;
> +       unsigned char *packet = psmouse->packet;
> +       int x, y;
> +
> +       if (!etd->trackpoint_present) {

Why not drop "etd->trackpoint_present" entirely and rely on "etd->tp_dev"?

> +               psmouse_err(psmouse, "Unexpected trackpoint message\n");

Ugh, please use some protection here. I mean _if_ this message occurs,
it is very likely that it will occur thousands of times. So something
like:

static bool __section(.data.unlikely) __warned;

if (!etd->tp_dev) {
        if (!__warned) {
                __warned = true;
                psmouse_err(psmouse, "...");
        }

        return;
}

> +               if (etd->debug == 1)
> +                       elantech_packet_dump(psmouse);
> +               return;
> +       }
> +
> +       input_report_key(tp_dev, BTN_LEFT, packet[0] & 0x01);
> +       input_report_key(tp_dev, BTN_RIGHT, packet[0] & 0x02);
> +       input_report_key(tp_dev, BTN_MIDDLE, packet[0] & 0x04);
> +       x = (s32) ((u32) ((packet[1] & 0x80) ? 0UL : 0xFFFFFF00UL) | (u32)
> +                  packet[4]);
> +       y = -(s32) ((u32) ((packet[2] & 0x80) ? 0UL : 0xFFFFFF00UL) | (u32)
> +                   packet[5]);

nitpick: No need for "UL" here, you can use just "U". In the kernel we
rely on "unsigned int" to be 32bit, always! But the compiler should
optimize that, anyway.

Both (u32)-casts seem redundant. packet[] is "unsigned char" so it's
properly up-casted to "unsigned int". And "unsigned int" is
automatically casted to "int", so you can also drop the (s32) cast.
This should work:
    x = ((packet[1] & 0x80) ? 0U : 0xFFFFFF00U) | packet[4];
    y = -(int)(((packet[2] & 0x80) ? 0U : 0xFFFFFF00U) | packet[5]);
But please verify with tests.

> +       input_report_rel(tp_dev, REL_X, x);
> +       input_report_rel(tp_dev, REL_Y, y);

How about some new-lines between input_report_key() and the variable
assignments?

> +
> +       switch ((((u32) packet[0] & 0xF8) << 24) | ((u32) packet[1] << 16)
> +               | (u32) packet[2] << 8 | (u32) packet[3]) {

Please use a temporary variable for that. And we don't use spaces in
front of casts:

u32 t = ...;
switch(t) {
}

> +       case 0x00808036UL:
> +       case 0x10008026UL:
> +       case 0x20800016UL:
> +       case 0x30000006UL:
> +               break;
> +       default:
> +               /* Dump unexpected packet sequences if debug=1 (default) */
> +               if (etd->debug == 1)
> +                       elantech_packet_dump(psmouse);
> +               break;
> +       }
> +
> +       input_sync(tp_dev);
> +}
> +
>  /*
>   * Interpret complete data packets and report absolute mode input events for
>   * hardware version 3. (12 byte packets for two fingers)
> @@ -715,6 +772,8 @@ static int elantech_packet_check_v3(struct psmouse *psmouse)
>
>                 if ((packet[0] & 0x0c) == 0x0c && (packet[3] & 0xce) == 0x0c)
>                         return PACKET_V3_TAIL;
> +               if ((packet[3]&0x0f) == 0x06)

coding-stlye: please add spaces around binary-operators (a & b) like above.

> +                       return PACKET_TRACKPOINT;
>         }
>
>         return PACKET_UNKNOWN;
> @@ -798,7 +857,10 @@ static psmouse_ret_t elantech_process_byte(struct psmouse *psmouse)
>                 if (packet_type == PACKET_UNKNOWN)
>                         return PSMOUSE_BAD_DATA;
>
> -               elantech_report_absolute_v3(psmouse, packet_type);
> +               if (packet_type == PACKET_TRACKPOINT)
> +                       elantech_report_trackpoint(psmouse, packet_type);
> +               else
> +                       elantech_report_absolute_v3(psmouse, packet_type);
>                 break;
>
>         case 4:
> @@ -1018,8 +1080,10 @@ static int elantech_get_resolution_v4(struct psmouse *psmouse,
>   * Asus UX31               0x361f00        20, 15, 0e      clickpad
>   * Asus UX32VD             0x361f02        00, 15, 0e      clickpad
>   * Avatar AVIU-145A2       0x361f00        ?               clickpad
> + * Fujitsu H730            0x570f00        c0, 14, 0c      3 hw buttons (**)
>   * Gigabyte U2442          0x450f01        58, 17, 0c      2 hw buttons
>   * Lenovo L430             0x350f02        b9, 15, 0c      2 hw buttons (*)
> + * Lenovo L530             0x350f02        b9, 15, 0c      2 hw buttons (*)
>   * Samsung NF210           0x150b00        78, 14, 0a      2 hw buttons
>   * Samsung NP770Z5E        0x575f01        10, 15, 0f      clickpad
>   * Samsung NP700Z5B        0x361f06        21, 15, 0f      clickpad
> @@ -1029,6 +1093,8 @@ static int elantech_get_resolution_v4(struct psmouse *psmouse,
>   * Samsung RF710           0x450f00        ?               2 hw buttons
>   * System76 Pangolin       0x250f01        ?               2 hw buttons
>   * (*) + 3 trackpoint buttons
> + * (**) + 0 trackpoint buttons
> + * Note: Lenovo L430 and Lenovo L430 have the same fw_version/caps
>   */
>  static void elantech_set_buttonpad_prop(struct psmouse *psmouse)
>  {
> @@ -1324,6 +1390,11 @@ int elantech_detect(struct psmouse *psmouse, bool set_properties)
>   */
>  static void elantech_disconnect(struct psmouse *psmouse)
>  {
> +       struct elantech_data *etd = psmouse->private;
> +       struct input_dev *tp_dev = etd->tp_dev;

I think there's no need for "tp_dev" here. There's no cast involved,
so you can access it via "etd" safely.

> +
> +       if (etd->trackpoint_present && tp_dev)
> +               input_unregister_device(tp_dev);

Please change this to:

if (etd->tp_dev)
        input_unregister_device(etd->tp_dev);

There is no reason to test for trackpoint_present. tp_dev must be NULL
if not allocated.

>         sysfs_remove_group(&psmouse->ps2dev.serio->dev.kobj,
>                            &elantech_attr_group);
>         kfree(psmouse->private);
> @@ -1440,11 +1511,11 @@ int elantech_init(struct psmouse *psmouse)
>         struct elantech_data *etd;
>         int i, error;
>         unsigned char param[3];
> +       struct input_dev *tp_dev;
>
>         psmouse->private = etd = kzalloc(sizeof(struct elantech_data), GFP_KERNEL);
>         if (!etd)
>                 return -ENOMEM;
> -
>         psmouse_reset(psmouse);
>
>         etd->parity[0] = 1;
> @@ -1497,6 +1568,28 @@ int elantech_init(struct psmouse *psmouse)
>                             error);
>                 goto init_fail;
>         }

Please add a new-line here.

> +       etd->trackpoint_present = ((etd->capabilities[0] & 0x80) == 0x80);
> +       if (etd->trackpoint_present) {
> +               tp_dev = input_allocate_device();
> +               if (!tp_dev)
> +                       goto init_fail_tp_alloc;

Again, a new-line here please.

> +               etd->tp_dev = tp_dev;
> +               snprintf(etd->tp_phys, sizeof(etd->tp_phys), "%s/input1",
> +                       psmouse->ps2dev.serio->phys);

You rely on external data here, so please check for truncation. If
anyone changes psmouse->ps2dev.serio->phys, they would not notice that
you rely on it here. So I'd do sth like:

size_t n = snprintf(etd->tp_phys, sizeof(etd->tp_phys), "%s/input1",
psmouse->ps2dev.serio->phys);
if (n >= sizeof(etd->tp_phys))
        goto init_fail_tp_reg;

Or at least force a terminating-zero on phys by decreasing the length
by 1 (phys is initialized to zero):

snprintf(etd->tp_phys, sizeof(etd->tp_phys) - 1, "%s/input1",
psmouse->ps2dev.serio->phys);

> +               tp_dev->phys = etd->tp_phys;
> +               tp_dev->name = "Elantech PS/2 TrackPoint";
> +               tp_dev->id.bustype = BUS_I8042;
> +               tp_dev->id.vendor  = 0x0002;
> +               tp_dev->id.product = PSMOUSE_ELANTECH;
> +               tp_dev->id.version = 0x0000;
> +               tp_dev->dev.parent = &psmouse->ps2dev.serio->dev;
> +               tp_dev->evbit[0] = BIT_MASK(EV_KEY) | BIT_MASK(EV_REL);
> +               tp_dev->relbit[BIT_WORD(REL_X)] = BIT_MASK(REL_X) | BIT_MASK(REL_Y);
> +               tp_dev->keybit[BIT_WORD(BTN_LEFT)] =
> +                       BIT_MASK(BTN_LEFT) | BIT_MASK(BTN_MIDDLE) | BIT_MASK(BTN_RIGHT);

Ugh, why not use __set_bit()? But ok, seems to be normal to use these
shortcuts for mice.

> +               if (input_register_device(etd->tp_dev))
> +                       goto init_fail_tp_reg;

Please forward the error code:

int error = -EINVAL;

...

error = input_register_device(tp_dev);
if (error < 0)
        goto init_fail_tp_reg;

...

init_fail:
kfree(etd);
return error;

> +       }
>
>         psmouse->protocol_handler = elantech_process_byte;
>         psmouse->disconnect = elantech_disconnect;
> @@ -1504,8 +1597,13 @@ int elantech_init(struct psmouse *psmouse)
>         psmouse->pktsize = etd->hw_version > 1 ? 6 : 4;
>
>         return 0;
> -
> + init_fail_tp_reg:
> +       input_free_device(tp_dev);
> + init_fail_tp_alloc:
> +       sysfs_remove_group(&psmouse->ps2dev.serio->dev.kobj,
> +                          &elantech_attr_group);
>   init_fail:
> +        psmouse_reset(psmouse);

elantech_disconnect() does not call psmouse_reset, so why add it here?
What am I missing?

>         kfree(etd);
>         return -1;
>  }
> diff --git a/drivers/input/mouse/elantech.h b/drivers/input/mouse/elantech.h
> index 9e0e2a1..25cbc8c 100644
> --- a/drivers/input/mouse/elantech.h
> +++ b/drivers/input/mouse/elantech.h
> @@ -94,6 +94,7 @@
>  #define PACKET_V4_HEAD                 0x05
>  #define PACKET_V4_MOTION               0x06
>  #define PACKET_V4_STATUS               0x07
> +#define PACKET_TRACKPOINT              0x08
>
>  /*
>   * track up to 5 fingers for v4 hardware
> @@ -114,6 +115,8 @@ struct finger_pos {
>  };
>
>  struct elantech_data {
> +       struct input_dev *tp_dev;               /* Relative device */

This comment is really misleading, I'd just drop it or change it to
"trackpoint device".

> +       char    tp_phys[32];
>         unsigned char reg_07;
>         unsigned char reg_10;
>         unsigned char reg_11;
> @@ -130,6 +133,7 @@ struct elantech_data {
>         bool jumpy_cursor;
>         bool reports_pressure;
>         bool crc_enabled;
> +       bool trackpoint_present;

Why do you use "tp_*" prefix above, but change it to "trackpoint_*"
here? "tp_present" seems more consistent to me.

Thanks
David

>         bool set_hw_resolution;
>         unsigned char hw_version;
>         unsigned int fw_version;
> --
> 2.0.0.rc2
>

Re: [PATCH v2 1/1] elantech: Add support for trackpoint found on some v3 models

[David Herrmann]

Hi

On Fri, Jun 13, 2014 at 9:24 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>> You rely on external data here, so please check for truncation. If
>> anyone changes psmouse->ps2dev.serio->phys, they would not notice that
>> you rely on it here. So I'd do sth like:
>>
>> size_t n = snprintf(etd->tp_phys, sizeof(etd->tp_phys), "%s/input1",
>> psmouse->ps2dev.serio->phys);
>> if (n >= sizeof(etd->tp_phys))
>>         goto init_fail_tp_reg;
>
> Ugh no, we've this snprintf pattern for foo->phys everywhere in the kernel
> and we don't do a truncation check anywhere. No-one in its right mind will
> make phys smaller, and even if phys gets truncated things will likely
> still work fine.
>
>> Or at least force a terminating-zero on phys by decreasing the length
>> by 1 (phys is initialized to zero):
>
> snprintf already forces a terminating zero, no need for weird tricks.
>
>>
>> snprintf(etd->tp_phys, sizeof(etd->tp_phys) - 1, "%s/input1",
>> psmouse->ps2dev.serio->phys);
>
> So this is wrong, no need for the - 1.

Oh, right, snprintf() always writes null. Fair enough.

>>
>>> +               tp_dev->phys = etd->tp_phys;
>>> +               tp_dev->name = "Elantech PS/2 TrackPoint";
>>> +               tp_dev->id.bustype = BUS_I8042;
>>> +               tp_dev->id.vendor  = 0x0002;
>>> +               tp_dev->id.product = PSMOUSE_ELANTECH;
>>> +               tp_dev->id.version = 0x0000;
>>> +               tp_dev->dev.parent = &psmouse->ps2dev.serio->dev;
>>> +               tp_dev->evbit[0] = BIT_MASK(EV_KEY) | BIT_MASK(EV_REL);
>>> +               tp_dev->relbit[BIT_WORD(REL_X)] = BIT_MASK(REL_X) | BIT_MASK(REL_Y);
>>> +               tp_dev->keybit[BIT_WORD(BTN_LEFT)] =
>>> +                       BIT_MASK(BTN_LEFT) | BIT_MASK(BTN_MIDDLE) | BIT_MASK(BTN_RIGHT);
>>
>> Ugh, why not use __set_bit()? But ok, seems to be normal to use these
>> shortcuts for mice.
>>
>>> +               if (input_register_device(etd->tp_dev))
>>> +                       goto init_fail_tp_reg;
>>
>> Please forward the error code:
>>
>> int error = -EINVAL;
>>
>> ...
>>
>> error = input_register_device(tp_dev);
>> if (error < 0)
>>         goto init_fail_tp_reg;
>>
>> ...
>>
>> init_fail:
>> kfree(etd);
>> return error;
>>
>>> +       }
>>>
>>>         psmouse->protocol_handler = elantech_process_byte;
>>>         psmouse->disconnect = elantech_disconnect;
>>> @@ -1504,8 +1597,13 @@ int elantech_init(struct psmouse *psmouse)
>>>         psmouse->pktsize = etd->hw_version > 1 ? 6 : 4;
>>>
>>>         return 0;
>>> -
>>> + init_fail_tp_reg:
>>> +       input_free_device(tp_dev);
>>> + init_fail_tp_alloc:
>>> +       sysfs_remove_group(&psmouse->ps2dev.serio->dev.kobj,
>>> +                          &elantech_attr_group);
>>>   init_fail:
>>> +        psmouse_reset(psmouse);
>>
>> elantech_disconnect() does not call psmouse_reset, so why add it here?
>> What am I missing?
>
> AFAIK elantech disconnect will only get called if the ps2 mouse driver gets
> unloaded / the mouse gets hot-unplugged. Where as this gets called on
> probe failure, after which we want to turn the ps/2 mouse emulation
> back on so that it will work as a regular mouse.

But this sounds like a bugfix to me, which is unrelated to this
comment? If it is, please consider sending it separately.

Thanks
David