[PATCH] hid-multitouch: fix broken eGalax

Since the inclusion of eGalax devices in 2.6.39, I've got some
bug reports for 480d and other devices.
The problem lies in the reports descriptors: eGalax supports both
pen and fingers, and so the reports descriptors contained both.
But hid-multitouch relies on them to detect the last item in each
field to send the multitouch events. In 480d, the last item is not
Y as it should but Pressure. That means that the fields are not
aligned and X,Y are at 0,0 (the other touch coordinates of the report).

With this patch, the detection is made only when the field ContactID
has been detected inside the collection.

There is still a problem with the detections of the range as stylus
and fingers may not have the same min/max, but it's a start.

Signed-off-by: Benjamin Tissoires <redacted>
---

Hi guys,

I'd love to see this patch into stable too. The point is that it does
not applies out of the box. Should I send it again to stable@kernel.org
once the reviews has been made?

Cheers,
Benjamin

 drivers/hid/hid-multitouch.c |   57 ++++++++++++++++++++++++++++-------------
 1 files changed, 39 insertions(+), 18 deletions(-)

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index ecd4d2d..b2767ea 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c

@@ -64,6 +64,7 @@ struct mt_device {
 	struct mt_class *mtclass;	/* our mt device class */
 	unsigned last_field_index;	/* last field index of the report */
 	unsigned last_slot_field;	/* the last field of a slot */
+	int last_mt_collection;	/* last known mt-related collection */
 	__s8 inputmode;		/* InputMode HID feature, -1 if non-existent */
 	__u8 num_received;	/* how many contacts we received */
 	__u8 num_expected;	/* expected last contact index */

@@ -225,8 +226,10 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 				cls->sn_move);
 			/* touchscreen emulation */
 			set_abs(hi->input, ABS_X, field, cls->sn_move);
-			td->last_slot_field = usage->hid;
-			td->last_field_index = field->index;
+			if (td->last_mt_collection == usage->collection_index) {
+				td->last_slot_field = usage->hid;
+				td->last_field_index = field->index;
+			}
 			return 1;
 		case HID_GD_Y:
 			if (quirks & MT_QUIRK_EGALAX_XYZ_FIXUP)

@@ -237,8 +240,10 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 				cls->sn_move);
 			/* touchscreen emulation */
 			set_abs(hi->input, ABS_Y, field, cls->sn_move);
-			td->last_slot_field = usage->hid;
-			td->last_field_index = field->index;
+			if (td->last_mt_collection == usage->collection_index) {
+				td->last_slot_field = usage->hid;
+				td->last_field_index = field->index;
+			}
 			return 1;
 		}
 		return 0;

@@ -246,31 +251,40 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 	case HID_UP_DIGITIZER:
 		switch (usage->hid) {
 		case HID_DG_INRANGE:
-			td->last_slot_field = usage->hid;
-			td->last_field_index = field->index;
+			if (td->last_mt_collection == usage->collection_index) {
+				td->last_slot_field = usage->hid;
+				td->last_field_index = field->index;
+			}
 			return 1;
 		case HID_DG_CONFIDENCE:
-			td->last_slot_field = usage->hid;
-			td->last_field_index = field->index;
+			if (td->last_mt_collection == usage->collection_index) {
+				td->last_slot_field = usage->hid;
+				td->last_field_index = field->index;
+			}
 			return 1;
 		case HID_DG_TIPSWITCH:
 			hid_map_usage(hi, usage, bit, max, EV_KEY, BTN_TOUCH);
 			input_set_capability(hi->input, EV_KEY, BTN_TOUCH);
-			td->last_slot_field = usage->hid;
-			td->last_field_index = field->index;
+			if (td->last_mt_collection == usage->collection_index) {
+				td->last_slot_field = usage->hid;
+				td->last_field_index = field->index;
+			}
 			return 1;
 		case HID_DG_CONTACTID:
 			input_mt_init_slots(hi->input, td->maxcontacts);
 			td->last_slot_field = usage->hid;
 			td->last_field_index = field->index;
+			td->last_mt_collection = usage->collection_index;
 			return 1;
 		case HID_DG_WIDTH:
 			hid_map_usage(hi, usage, bit, max,
 					EV_ABS, ABS_MT_TOUCH_MAJOR);
 			set_abs(hi->input, ABS_MT_TOUCH_MAJOR, field,
 				cls->sn_width);
-			td->last_slot_field = usage->hid;
-			td->last_field_index = field->index;
+			if (td->last_mt_collection == usage->collection_index) {
+				td->last_slot_field = usage->hid;
+				td->last_field_index = field->index;
+			}
 			return 1;
 		case HID_DG_HEIGHT:
 			hid_map_usage(hi, usage, bit, max,

@@ -279,8 +293,10 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 				cls->sn_height);
 			input_set_abs_params(hi->input,
 					ABS_MT_ORIENTATION, 0, 1, 0, 0);
-			td->last_slot_field = usage->hid;
-			td->last_field_index = field->index;
+			if (td->last_mt_collection == usage->collection_index) {
+				td->last_slot_field = usage->hid;
+				td->last_field_index = field->index;
+			}
 			return 1;
 		case HID_DG_TIPPRESSURE:
 			if (quirks & MT_QUIRK_EGALAX_XYZ_FIXUP)

@@ -292,16 +308,20 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 			/* touchscreen emulation */
 			set_abs(hi->input, ABS_PRESSURE, field,
 				cls->sn_pressure);
-			td->last_slot_field = usage->hid;
-			td->last_field_index = field->index;
+			if (td->last_mt_collection == usage->collection_index) {
+				td->last_slot_field = usage->hid;
+				td->last_field_index = field->index;
+			}
 			return 1;
 		case HID_DG_CONTACTCOUNT:
-			td->last_field_index = field->index;
+			if (td->last_mt_collection == usage->collection_index)
+				td->last_field_index = field->index;
 			return 1;
 		case HID_DG_CONTACTMAX:
 			/* we don't set td->last_slot_field as contactcount and
 			 * contact max are global to the report */
-			td->last_field_index = field->index;
+			if (td->last_mt_collection == usage->collection_index)
+				td->last_field_index = field->index;
 			return -1;
 		}
 		/* let hid-input decide for the others */

@@ -516,6 +536,7 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	}
 	td->mtclass = mtclass;
 	td->inputmode = -1;
+	td->last_mt_collection = -1;
 	hid_set_drvdata(hdev, td);
 
 	ret = hid_parse(hdev);

-- 
1.7.4.4

Hi Benjamin,

With this patch, the detection is made only when the field ContactID
has been detected inside the collection.

The patch introduces an order dependence by relying on ContactID
occuring early on, which seems unnecessary. How about checking for the
presence of ContactID instead, and simply modify the logic which uses
last_field_index and last_slot_field?

Thanks,
Henrik

Hi Henrik,

On Sun, Jun 12, 2011 at 17:29, Henrik Rydberg [off-list ref] wrote:

Hi Benjamin,

quoted

With this patch, the detection is made only when the field ContactID
has been detected inside the collection.

The patch introduces an order dependence by relying on ContactID
occuring early on, which seems unnecessary. How about checking for the
presence of ContactID instead, and simply modify the logic which uses
last_field_index and last_slot_field?

That's exactly what it does. A touch report has to contain the
contactID field. And each touch is at least in one collection (some
devices send one touch per collection, others only one collection for
the whole report).
The idea is when hardware makers introduce several input mode for
their device, they has to put them in different collections.

The logic behind the last_field_index and last_slot_field are only for
multitouch. So it does not matter if the contactID comes first or last
in the collection, it will set the right index/field.

Cheers,
Benjamin

Thanks,
Henrik

Hi Benjamin,

quoted

quoted

With this patch, the detection is made only when the field ContactID
has been detected inside the collection.

The patch introduces an order dependence by relying on ContactID
occuring early on, which seems unnecessary. How about checking for the
presence of ContactID instead, and simply modify the logic which uses
last_field_index and last_slot_field?

That's exactly what it does. A touch report has to contain the
contactID field. And each touch is at least in one collection (some
devices send one touch per collection, others only one collection for
the whole report).
The idea is when hardware makers introduce several input mode for
their device, they has to put them in different collections.

Ah, right you are, the ensured presence of ContactID is enough to
prove that the patch code is equivalent.

The logic behind the last_field_index and last_slot_field are only for
multitouch. So it does not matter if the contactID comes first or last
in the collection, it will set the right index/field.

Thanks for the clarification.

    Reviewed-by: Henrik Rydberg [off-list ref]

Cheers,
Henrik

On Sun, 12 Jun 2011, Benjamin Tissoires wrote:

Since the inclusion of eGalax devices in 2.6.39, I've got some
bug reports for 480d and other devices.
The problem lies in the reports descriptors: eGalax supports both
pen and fingers, and so the reports descriptors contained both.
But hid-multitouch relies on them to detect the last item in each
field to send the multitouch events. In 480d, the last item is not
Y as it should but Pressure. That means that the fields are not
aligned and X,Y are at 0,0 (the other touch coordinates of the report).

With this patch, the detection is made only when the field ContactID
has been detected inside the collection.

There is still a problem with the detections of the range as stylus
and fingers may not have the same min/max, but it's a start.

Signed-off-by: Benjamin Tissoires <redacted>
---

Hi guys,

I'd love to see this patch into stable too. The point is that it does
not applies out of the box. Should I send it again to stable@kernel.org
once the reviews has been made?

The best thing is to wait for the patch to appear in Linus' tree (which is 
a prerequisity for -stable inclusion anyway), and then send a backported 
version, with upstream reference commit, to stable@kernel.org.

On Sun, 12 Jun 2011, Henrik Rydberg wrote:

Thanks for the clarification.

    Reviewed-by: Henrik Rydberg [off-list ref]

Thanks, I have now applied it.

I am waiting with pushing my upstream-fixes branch to Linus before the 
magicmouse issue has been sorted out (waiting for tester Ack). If this 
doesn't happen in a day or two, I will be pushing it anyway.

Thanks,

-- 
Jiri Kosina
SUSE Labs

On Sun, Jun 12, 2011 at 1:22 AM, Benjamin Tissoires
[off-list ref] wrote:

Since the inclusion of eGalax devices in 2.6.39, I've got some
bug reports for 480d and other devices.
The problem lies in the reports descriptors: eGalax supports both
pen and fingers, and so the reports descriptors contained both.
But hid-multitouch relies on them to detect the last item in each
field to send the multitouch events. In 480d, the last item is not
Y as it should but Pressure. That means that the fields are not
aligned and X,Y are at 0,0 (the other touch coordinates of the report).

With this patch, the detection is made only when the field ContactID
has been detected inside the collection.

There is still a problem with the detections of the range as stylus
and fingers may not have the same min/max, but it's a start.

Signed-off-by: Benjamin Tissoires <redacted>

This patch was good timing for me.  I was giving the latest
hid-multitouch a try on my MosArt touchscreen (0486:0186).  hid-mosart
was basically working but once merged into hid-multitouch,
hid-multitouch was not.

The symptom was once you touched screen, it acted like user never
released touch.

I tracked it down today to last_field_index being reset from 14 to 2
because of an unused extra input in descriptor that looked like a
mouse (X/Y and 2 buttons).  This to small index caused a delay of
sending valid data by 1 packet and since no packets sent after touch
release, you'd not get the release at appropriate time.

In my case, only the input with ContactID's are useful (there are 2
ContactID's spread between 2 collections) so this patch should prevent
last_field_index from being reset by the unused X/Y's.

Chris