[linux-boundary][PATCHv2] egalax_ts: Always report all touch points before input_sync()

[linux-boundary][PATCHv2] egalax_ts: Always report all touch points before input_sync()

[Erik Botö]

The previous behavior of the driver did not work properly with Qt5
QtQuick multi touch-point gestures, due to how touch-points are
reported when removing a touch-point. My interpretation of the
available documentation [1] was that the driver should report all
touch-points between SYN_REPORTs, but it is not explicitly stated so.
I've found another mail-thread [2] where the creator of the protocol
states:

"The protocol defines a generic way of sending a variable amount of
contacts. The contact count is obtained by counting the number of
non-empty finger packets between SYN_REPORT events."-Henrik Rydberg

I think this verifies my assumption that all touch-points should be
reported between SYN_REPORTs, otherwise it can not be used to obtain
the count.

[1] https://www.kernel.org/doc/Documentation/input/multi-touch-protocol.txt
[2] http://lists.x.org/archives/xorg-devel/2010-March/006466.html
---
 drivers/input/touchscreen/egalax_ts.c | 36 +++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/drivers/input/touchscreen/egalax_ts.c b/drivers/input/touchscreen/egalax_ts.c
index 0b6cde7..271f820 100644
--- a/drivers/input/touchscreen/egalax_ts.c
+++ b/drivers/input/touchscreen/egalax_ts.c
@@ -133,7 +133,6 @@ retry:
 	}
 
 	if (down) {
-		/* should also report old pointers */
 		events[id].valid = valid;
 		events[id].status = down;
 		events[id].x = x;
@@ -144,23 +143,6 @@ retry:
 		input_report_abs(input_dev, ABS_Y, y);
 		input_event(data->input_dev, EV_KEY, BTN_TOUCH, 1);
 		input_report_abs(input_dev, ABS_PRESSURE, 1);
-#else
-		for (i = 0; i < MAX_SUPPORT_POINTS; i++) {
-			if (!events[i].valid)
-				continue;
-			dev_dbg(&client->dev, "report id:%d valid:%d x:%d y:%d",
-				i, valid, x, y);
-
-			input_report_abs(input_dev,
-					 ABS_MT_TRACKING_ID, i);
-			input_report_abs(input_dev,
-					 ABS_MT_TOUCH_MAJOR, 1);
-			input_report_abs(input_dev,
-					 ABS_MT_POSITION_X, events[i].x);
-			input_report_abs(input_dev,
-					 ABS_MT_POSITION_Y, events[i].y);
-			input_mt_sync(input_dev);
-		}
 #endif
 	} else {
 		dev_dbg(&client->dev, "release id:%d\n", id);
@@ -176,6 +158,24 @@ retry:
 #endif
 	}
 
+#ifndef CONFIG_TOUCHSCREEN_EGALAX_SINGLE_TOUCH
+	/* report all pointers */
+	for (i = 0; i < MAX_SUPPORT_POINTS; i++) {
+		if (!events[i].valid)
+			continue;
+		dev_dbg(&client->dev, "report id:%d valid:%d x:%d y:%d",
+			i, valid, x, y);
+			input_report_abs(input_dev,
+				 ABS_MT_TRACKING_ID, i);
+		input_report_abs(input_dev,
+				 ABS_MT_TOUCH_MAJOR, 1);
+		input_report_abs(input_dev,
+				 ABS_MT_POSITION_X, events[i].x);
+		input_report_abs(input_dev,
+				 ABS_MT_POSITION_Y, events[i].y);
+		input_mt_sync(input_dev);
+	}
+#endif
 	input_sync(input_dev);
 	return IRQ_HANDLED;
 }
-- 
1.8.1.2

Re: [linux-boundary][PATCHv2] egalax_ts: Always report all touch points before input_sync()

[Otavio Salvador]

Hello Erik,

On Wed, Jul 3, 2013 at 11:11 AM, Erik Botö <erik.boto@pelagicore.com> wrote:
> The previous behavior of the driver did not work properly with Qt5
> QtQuick multi touch-point gestures, due to how touch-points are
> reported when removing a touch-point. My interpretation of the
> available documentation [1] was that the driver should report all
> touch-points between SYN_REPORTs, but it is not explicitly stated so.
> I've found another mail-thread [2] where the creator of the protocol
> states:
>
> "The protocol defines a generic way of sending a variable amount of
> contacts. The contact count is obtained by counting the number of
> non-empty finger packets between SYN_REPORT events."-Henrik Rydberg
>
> I think this verifies my assumption that all touch-points should be
> reported between SYN_REPORTs, otherwise it can not be used to obtain
> the count.
>
> [1] https://www.kernel.org/doc/Documentation/input/multi-touch-protocol.txt
> [2] http://lists.x.org/archives/xorg-devel/2010-March/006466.html

I am adding Eric Nelson, to review it and possibly apply it in their
Boundary's tree, and Mahesh to ask for internal review in Freescale.

--
Otavio Salvador                             O.S. Systems
http://www.ossystems.com.br        http://projetos.ossystems.com.br
Mobile: +55 (53) 9981-7854            Mobile: +1 (347) 903-9750

Re: [linux-boundary][PATCHv2] egalax_ts: Always report all touch points before input_sync()

[Eric Nelson]

Hi Erik,

Sorry for the delay, but I finally got around to checking this patch.

On 07/03/2013 07:11 AM, Erik Botö wrote:
> The previous behavior of the driver did not work properly with Qt5
> QtQuick multi touch-point gestures, due to how touch-points are
> reported when removing a touch-point. My interpretation of the
> available documentation [1] was that the driver should report all
> touch-points between SYN_REPORTs, but it is not explicitly stated so.
> I've found another mail-thread [2] where the creator of the protocol
> states:
>
> "The protocol defines a generic way of sending a variable amount of
> contacts. The contact count is obtained by counting the number of
> non-empty finger packets between SYN_REPORT events."-Henrik Rydberg
>
> I think this verifies my assumption that all touch-points should be
> reported between SYN_REPORTs, otherwise it can not be used to obtain
> the count.
>

Your assumptions are correct, and if I read things correctly, they're
already fixed, but in a different source tree...

I'm guessing that someone forgot to mention that there are **two**
primary kernel trees for i.MX. Android and not-Android.

The Android kernel seems to have this fixed in a different way,
by doing some additional book-keeping of current touches:
	http://git.freescale.com/git/cgit.cgi/imx/linux-2.6-imx.git/tree/drivers/input/touchscreen/egalax_ts.c?id=3b9608406bb699de5ff02760c745e62212b4c280

In Freescale's repository, the current Android stuff is in a tag
(jb4.2.2_1.0.0-ga) instead of a branch, so it's a bit difficult
to navigate through the web interface:

http://git.freescale.com/git/cgit.cgi/imx/linux-2.6-imx.git/tag/?id=jb4.2.2_1.0.0-ga

Hey Freescalers, can we get a head nod that we can consolidate
these? I think all that's needed is to add the SINGLE_TOUCH stuff
into the Android branch.

Erik, can you try out the Android version and see if it works for
you?

Please advise,


Eric

Re: [linux-boundary][PATCHv2] egalax_ts: Always report all touch points before input_sync()

[Erik Botö]

[-- Attachment #1: Type: text/plain, Size: 3191 bytes --]

Hi Eric,


On Wed, Jul 3, 2013 at 8:53 PM, Eric Nelson <eric.nelson@boundarydevices.com
> wrote:

> Hi Erik,
>
> Sorry for the delay, but I finally got around to checking this patch.
>
>
> On 07/03/2013 07:11 AM, Erik Botö wrote:
>
>> The previous behavior of the driver did not work properly with Qt5
>> QtQuick multi touch-point gestures, due to how touch-points are
>> reported when removing a touch-point. My interpretation of the
>> available documentation [1] was that the driver should report all
>> touch-points between SYN_REPORTs, but it is not explicitly stated so.
>> I've found another mail-thread [2] where the creator of the protocol
>> states:
>>
>> "The protocol defines a generic way of sending a variable amount of
>> contacts. The contact count is obtained by counting the number of
>> non-empty finger packets between SYN_REPORT events."-Henrik Rydberg
>>
>> I think this verifies my assumption that all touch-points should be
>> reported between SYN_REPORTs, otherwise it can not be used to obtain
>> the count.
>>
>>
> Your assumptions are correct, and if I read things correctly, they're
> already fixed, but in a different source tree...
>
> I'm guessing that someone forgot to mention that there are **two**
> primary kernel trees for i.MX. Android and not-Android.
>
> The Android kernel seems to have this fixed in a different way,
> by doing some additional book-keeping of current touches:
>         http://git.freescale.com/git/**cgit.cgi/imx/linux-2.6-imx.**
> git/tree/drivers/input/**touchscreen/egalax_ts.c?id=**
> 3b9608406bb699de5ff02760c745e6**2212b4c280<http://git.freescale.com/git/cgit.cgi/imx/linux-2.6-imx.git/tree/drivers/input/touchscreen/egalax_ts.c?id=3b9608406bb699de5ff02760c745e62212b4c280>
>
> In Freescale's repository, the current Android stuff is in a tag
> (jb4.2.2_1.0.0-ga) instead of a branch, so it's a bit difficult
> to navigate through the web interface:
>
> http://git.freescale.com/git/**cgit.cgi/imx/linux-2.6-imx.**
> git/tag/?id=jb4.2.2_1.0.0-ga<http://git.freescale.com/git/cgit.cgi/imx/linux-2.6-imx.git/tag/?id=jb4.2.2_1.0.0-ga>
>
> Hey Freescalers, can we get a head nod that we can consolidate
> these? I think all that's needed is to add the SINGLE_TOUCH stuff
> into the Android branch.
>
> Erik, can you try out the Android version and see if it works for
> you?
>

I did a quick test where I just copied the Android version, removed the
EARLYSUSPEND stuff so it would build inside a regular kernel but it is not
behaving properly for me. Even one finger clicks doesn't seem to work,
looks like it never gets released.

Unfortunately I don't have much time to investigate this further. But since
the two kernels are different anyway I guess there's no danger with merging
my patch suggestion in the non-android kernel?

Cheers,
Erik


>
> Please advise,
>
>
> Eric
>
>


-- 
=============================================
Erik Botö
Senior Software Engineer
Pelagicore AB
Ekelundsgatan 4, 6tr, SE-411 18 Gothenburg, Sweden
Mobile: +46 (0)76 881 72 03
E-Mail: erik.boto@pelagicore.com
=============================================

[-- Attachment #2: Type: text/html, Size: 4162 bytes --]

Re: [linux-boundary][PATCHv2] egalax_ts: Always report all touch points before input_sync()

[Eric Nelson]

Hi Erik,

On 07/04/2013 11:18 PM, Erik Botö wrote:
> Hi Eric,
>
> On Wed, Jul 3, 2013 at 8:53 PM, Eric Nelson
> <eric.nelson@boundarydevices.com
> <mailto:eric.nelson@boundarydevices.com>> wrote:
>
>     Hi Erik,
>
>     Sorry for the delay, but I finally got around to checking this patch.
>
>
>     On 07/03/2013 07:11 AM, Erik Botö wrote:
>
>         The previous behavior of the driver did not work properly with Qt5
>         QtQuick multi touch-point gestures, due to how touch-points are
>         reported when removing a touch-point. My interpretation of the
>         available documentation [1] was that the driver should report all
>         touch-points between SYN_REPORTs, but it is not explicitly
>         stated so.
>         I've found another mail-thread [2] where the creator of the protocol
>         states:
>
>         "The protocol defines a generic way of sending a variable amount of
>         contacts. The contact count is obtained by counting the number of
>         non-empty finger packets between SYN_REPORT events."-Henrik Rydberg
>
>         I think this verifies my assumption that all touch-points should be
>         reported between SYN_REPORTs, otherwise it can not be used to obtain
>         the count.
>
>
>     Your assumptions are correct, and if I read things correctly, they're
>     already fixed, but in a different source tree...
>
>     I'm guessing that someone forgot to mention that there are **two**
>     primary kernel trees for i.MX. Android and not-Android.
>
>     The Android kernel seems to have this fixed in a different way,
>     by doing some additional book-keeping of current touches:
>     http://git.freescale.com/git/__cgit.cgi/imx/linux-2.6-imx.__git/tree/drivers/input/__touchscreen/egalax_ts.c?id=__3b9608406bb699de5ff02760c745e6__2212b4c280
>     <http://git.freescale.com/git/cgit.cgi/imx/linux-2.6-imx.git/tree/drivers/input/touchscreen/egalax_ts.c?id=3b9608406bb699de5ff02760c745e62212b4c280>
>
>     In Freescale's repository, the current Android stuff is in a tag
>     (jb4.2.2_1.0.0-ga) instead of a branch, so it's a bit difficult
>     to navigate through the web interface:
>
>     http://git.freescale.com/git/__cgit.cgi/imx/linux-2.6-imx.__git/tag/?id=jb4.2.2_1.0.0-ga
>     <http://git.freescale.com/git/cgit.cgi/imx/linux-2.6-imx.git/tag/?id=jb4.2.2_1.0.0-ga>
>
>     Hey Freescalers, can we get a head nod that we can consolidate
>     these? I think all that's needed is to add the SINGLE_TOUCH stuff
>     into the Android branch.
>
>     Erik, can you try out the Android version and see if it works for
>     you?
>
>
> I did a quick test where I just copied the Android version, removed the
> EARLYSUSPEND stuff so it would build inside a regular kernel but it is
> not behaving properly for me. Even one finger clicks doesn't seem to
> work, looks like it never gets released.
>
Thanks for testing that.

It doesn't surprise me that Android expects something off-spec.

> Unfortunately I don't have much time to investigate this further. But
> since the two kernels are different anyway I guess there's no danger
> with merging my patch suggestion in the non-android kernel?
>

No worries on my end.

I was just hoping to consolidate the two, since it makes future bug
fixes easier.

Regards,


Eric