[PATCH 2/2 v2] elantech: Report multitouch with proper ABS_MT messages

[PATCH 2/2 v2] elantech: Report multitouch with proper ABS_MT messages

[Éric Piel]

Multitouch info was reported only via a old protocol used by the
proprietary X driver from elantech. Let's report the multitouch info
also following the official MT protocol.

This was done following the multi-touch-protocol.txt documentation, and
inspired by the bcm5974 implementation. Testing was light as there is
not many applications using this protocol yet, but the X synaptics
driver didn't complain and the X multitouch driver behaved correctly.

Signed-off-by: Éric Piel <eric.piel@tremplin-utc.net>
---
 drivers/input/mouse/elantech.c |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/drivers/input/mouse/elantech.c b/drivers/input/mouse/elantech.c
index 2708b22..fc3c98a 100644
--- a/drivers/input/mouse/elantech.c
+++ b/drivers/input/mouse/elantech.c
@@ -278,6 +278,10 @@ static void elantech_report_absolute_v2(struct psmouse *psmouse)
 		 * byte 5: y7  y6  y5  y4  y3  y2  y1  y0
 		 */
 		y1 = ETP_YMAX_V2 - (((packet[4] & 0x03) << 8) | packet[5]);
+		input_report_abs(dev, ABS_MT_POSITION_X, x1);
+		input_report_abs(dev, ABS_MT_POSITION_Y, y1);
+		input_mt_sync(dev);
+
 		input_report_abs(dev, ABS_TOOL_WIDTH, width);
 		input_report_abs(dev, ABS_X, x1);
 		input_report_abs(dev, ABS_Y, y1);
@@ -300,6 +304,13 @@ static void elantech_report_absolute_v2(struct psmouse *psmouse)
 		x2 = ((packet[3] & 0x10) << 4) | packet[4];
 		/* byte 5: by7 by6 by5 by4 by3 by2 by1 by0 */
 		y2 = ETP_2FT_YMAX - (((packet[3] & 0x20) << 3) | packet[5]);
+		/* Multitouch */
+		input_report_abs(dev, ABS_MT_POSITION_X, x1 << 2);
+		input_report_abs(dev, ABS_MT_POSITION_Y, y1 << 2);
+		input_mt_sync(dev);
+		input_report_abs(dev, ABS_MT_POSITION_X, x2 << 2);
+		input_report_abs(dev, ABS_MT_POSITION_Y, y2 << 2);
+		input_mt_sync(dev);
 		/*
 		 * For compatibility with the X Synaptics driver scale up
 		 * one coordinate and report as ordinary mouse movent
@@ -484,6 +495,8 @@ static void elantech_set_input_params(struct psmouse *psmouse)
 		input_set_abs_params(dev, ABS_TOOL_WIDTH, ETP_WMIN_V2, ETP_WMAX_V2, 0, 0);
 		input_set_abs_params(dev, ABS_X, ETP_XMIN_V2, ETP_XMAX_V2, 0, 0);
 		input_set_abs_params(dev, ABS_Y, ETP_YMIN_V2, ETP_YMAX_V2, 0, 0);
+		input_set_abs_params(dev, ABS_MT_POSITION_X, ETP_XMIN_V2, ETP_XMAX_V2, 0, 0);
+		input_set_abs_params(dev, ABS_MT_POSITION_Y, ETP_YMIN_V2, ETP_YMAX_V2, 0, 0);
 		input_set_abs_params(dev, ABS_HAT0X, ETP_2FT_XMIN, ETP_2FT_XMAX, 0, 0);
 		input_set_abs_params(dev, ABS_HAT0Y, ETP_2FT_YMIN, ETP_2FT_YMAX, 0, 0);
 		input_set_abs_params(dev, ABS_HAT1X, ETP_2FT_XMIN, ETP_2FT_XMAX, 0, 0);
-- 
1.7.1


--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2 v3] elantech: Report multitouch with proper ABS_MT messages

[Éric Piel]

Here is an updated version of the patch. It is identical to the previous
version but rediff'd against the lastest version of patch 1.

Eric

8<-------------------------------------------------------

Multitouch info was reported only via a old protocol used by the
proprietary X driver from elantech. Let's report the multitouch info
also following the official MT protocol.

This was done following the multi-touch-protocol.txt documentation, and
inspired by the bcm5974 implementation. Testing was light as there is
not many applications using this protocol yet, but the X synaptics
driver didn't complain and the X multitouch driver behaved correctly.

Signed-off-by: Éric Piel <eric.piel@tremplin-utc.net>
---
 drivers/input/mouse/elantech.c |   24 +++++++++++++++++++-----
 1 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/drivers/input/mouse/elantech.c b/drivers/input/mouse/elantech.c
index 3dceb0f..a226f21 100644
--- a/drivers/input/mouse/elantech.c
+++ b/drivers/input/mouse/elantech.c
@@ -271,15 +271,20 @@ static void elantech_report_absolute_v2(struct psmouse *psmouse)
 		 * byte 3: .   .   w1  w0   .   .   .   .
 		 */
 		width = ((packet[1] & 0xf0) >> 2) | ((packet[3] & 0x30) >> 4);
-		input_report_abs(dev, ABS_TOOL_WIDTH, width);
-		input_report_abs(dev, ABS_X,
-			((packet[1] & 0x07) << 8) | packet[2]);
+		x1 = ((packet[1] & 0x07) << 8) | packet[2];
 		/*
 		 * byte 4:  .   .   .   .   .   .  y9  y8
 		 * byte 5: y7  y6  y5  y4  y3  y2  y1  y0
 		 */
-		input_report_abs(dev, ABS_Y,
-			ETP_YMAX_V2 - (((packet[4] & 0x03) << 8) | packet[5]));
+		y1 = ETP_YMAX_V2 - (((packet[4] & 0x03) << 8) | packet[5]);
+		/* Multitouch */
+		input_report_abs(dev, ABS_MT_POSITION_X, x1);
+		input_report_abs(dev, ABS_MT_POSITION_Y, y1);
+		input_mt_sync(dev);
+
+		input_report_abs(dev, ABS_TOOL_WIDTH, width);
+		input_report_abs(dev, ABS_X, x1);
+		input_report_abs(dev, ABS_Y, y1);
 		break;
 
 	case 2:
@@ -299,6 +304,13 @@ static void elantech_report_absolute_v2(struct psmouse *psmouse)
 		x2 = ((packet[3] & 0x10) << 4) | packet[4];
 		/* byte 5: by7 by6 by5 by4 by3 by2 by1 by0 */
 		y2 = ETP_2FT_YMAX - (((packet[3] & 0x20) << 3) | packet[5]);
+		/* Multitouch */
+		input_report_abs(dev, ABS_MT_POSITION_X, x1 << 2);
+		input_report_abs(dev, ABS_MT_POSITION_Y, y1 << 2);
+		input_mt_sync(dev);
+		input_report_abs(dev, ABS_MT_POSITION_X, x2 << 2);
+		input_report_abs(dev, ABS_MT_POSITION_Y, y2 << 2);
+		input_mt_sync(dev);
 		/*
 		 * For compatibility with the X Synaptics driver scale up
 		 * one coordinate and report as ordinary mouse movent
@@ -483,6 +495,8 @@ static void elantech_set_input_params(struct psmouse *psmouse)
 		input_set_abs_params(dev, ABS_TOOL_WIDTH, ETP_WMIN_V2, ETP_WMAX_V2, 0, 0);
 		input_set_abs_params(dev, ABS_X, ETP_XMIN_V2, ETP_XMAX_V2, 0, 0);
 		input_set_abs_params(dev, ABS_Y, ETP_YMIN_V2, ETP_YMAX_V2, 0, 0);
+		input_set_abs_params(dev, ABS_MT_POSITION_X, ETP_XMIN_V2, ETP_XMAX_V2, 0, 0);
+		input_set_abs_params(dev, ABS_MT_POSITION_Y, ETP_YMIN_V2, ETP_YMAX_V2, 0, 0);
 		input_set_abs_params(dev, ABS_HAT0X, ETP_2FT_XMIN, ETP_2FT_XMAX, 0, 0);
 		input_set_abs_params(dev, ABS_HAT0Y, ETP_2FT_YMIN, ETP_2FT_YMAX, 0, 0);
 		input_set_abs_params(dev, ABS_HAT1X, ETP_2FT_XMIN, ETP_2FT_XMAX, 0, 0);
-- 
1.7.1


--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2 v3] elantech: Report multitouch with proper ABS_MT messages

[Henrik Rydberg]

Hi again Éric,

thanks for your changes, the first patch looks better now. However, both patches
still have style problems. Also, the second patch now makes unrelated changes to
the ABS_X etc, so it is difficult to even know what was tested, and whether the
first patch really works as claimed.

Henrik

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2 v3] elantech: Report multitouch with proper ABS_MT messages

[Éric Piel]

Op 10-05-10 22:53, Henrik Rydberg schreef:
> Hi again Éric,
> 
> thanks for your changes, the first patch looks better now. However, both patches
> still have style problems. Also, the second patch now makes unrelated changes to
> the ABS_X etc, so it is difficult to even know what was tested, and whether the
> first patch really works as claimed.
What are the style problems of the first patch? I tried to fix all of them.

Concerning the second patch, it's necessary to change the lines with
ABS_X, because otherwise the computation would be duplicated for the MT
part and it would make everything look more complicated. Honestly, I
think this change is still very limited and can easily be reviewed for
not changing the semantic. Well, I think it would be a bit overkill, but
if really considered necessary, I can do a third intermediary patch...
Dmitry let me know if you can merge the patch as is.

Eric

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2 v3] elantech: Report multitouch with proper ABS_MT messages

[Henrik Rydberg]

Éric Piel wrote:
> Op 10-05-10 22:53, Henrik Rydberg schreef:
>> Hi again Éric,
>>
>> thanks for your changes, the first patch looks better now. However, both patches
>> still have style problems. Also, the second patch now makes unrelated changes to
>> the ABS_X etc, so it is difficult to even know what was tested, and whether the
>> first patch really works as claimed.
> What are the style problems of the first patch? I tried to fix all of them.

WARNING: line over 80 characters
#212: FILE: drivers/input/mouse/elantech.c:483:
+		input_set_abs_params(dev, ABS_TOOL_WIDTH, ETP_WMIN_V2, ETP_WMAX_V2, 0, 0);

WARNING: line over 80 characters
#119: FILE: drivers/input/mouse/elantech.c:498:
+		input_set_abs_params(dev, ABS_MT_POSITION_X, ETP_XMIN_V2, ETP_XMAX_V2, 0, 0);

WARNING: line over 80 characters
#120: FILE: drivers/input/mouse/elantech.c:499:
+		input_set_abs_params(dev, ABS_MT_POSITION_Y, ETP_YMIN_V2, ETP_YMAX_V2, 0, 0);

> 
> Concerning the second patch, it's necessary to change the lines with
> ABS_X, because otherwise the computation would be duplicated for the MT
> part and it would make everything look more complicated. Honestly, I
> think this change is still very limited and can easily be reviewed for
> not changing the semantic.

Yes, you are right about x1 and y1. The patch patch touches the width variable
for no apparent reason, and I stopped reading there.

If this process seems trivial and tedious to you, consider how it must appear to
those who deal with patches every day. Most people will not even look at a patch
with style problems, so the real review _starts_ when those issues are fixed.

Henrik
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2 v3] elantech: Report multitouch with proper ABS_MT messages

[Dmitry Torokhov]

On Tue, May 11, 2010 at 01:58:43AM +0200, Henrik Rydberg wrote:
> Éric Piel wrote:
> > Op 10-05-10 22:53, Henrik Rydberg schreef:
> >> Hi again Éric,
> >>
> >> thanks for your changes, the first patch looks better now. However, both patches
> >> still have style problems. Also, the second patch now makes unrelated changes to
> >> the ABS_X etc, so it is difficult to even know what was tested, and whether the
> >> first patch really works as claimed.
> > What are the style problems of the first patch? I tried to fix all of them.
> 
> WARNING: line over 80 characters
> #212: FILE: drivers/input/mouse/elantech.c:483:
> +		input_set_abs_params(dev, ABS_TOOL_WIDTH, ETP_WMIN_V2, ETP_WMAX_V2, 0, 0);
> 
> WARNING: line over 80 characters
> #119: FILE: drivers/input/mouse/elantech.c:498:
> +		input_set_abs_params(dev, ABS_MT_POSITION_X, ETP_XMIN_V2, ETP_XMAX_V2, 0, 0);
> 
> WARNING: line over 80 characters
> #120: FILE: drivers/input/mouse/elantech.c:499:
> +		input_set_abs_params(dev, ABS_MT_POSITION_Y, ETP_YMIN_V2, ETP_YMAX_V2, 0, 0);
> 

Do not worry about the 80 char limit that much, especially if it
produces less readable code due to wierd line splits. The code above
could be broken cleanly, but it's not a big deal.

The rest of the checkpatch warnirngs (if any) I prefer to have
addressed though.

> > 
> > Concerning the second patch, it's necessary to change the lines with
> > ABS_X, because otherwise the computation would be duplicated for the MT
> > part and it would make everything look more complicated. Honestly, I
> > think this change is still very limited and can easily be reviewed for
> > not changing the semantic.
> 
> Yes, you are right about x1 and y1. The patch patch touches the width variable
> for no apparent reason, and I stopped reading there.
> 
> If this process seems trivial and tedious to you, consider how it must appear to
> those who deal with patches every day. Most people will not even look at a patch
> with style problems, so the real review _starts_ when those issues are fixed.
> 
> Henrik

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2 v3] elantech: Report multitouch with proper ABS_MT messages

[Henrik Rydberg]

Dmitry Torokhov wrote:
> Do not worry about the 80 char limit that much, especially if it
> produces less readable code due to wierd line splits. The code above
> could be broken cleanly, but it's not a big deal.
> 
> The rest of the checkpatch warnirngs (if any) I prefer to have
> addressed though.

Fair enough, ACK on the content of both patches, and thanks Eric for your patience.

Henrik