[PATCH v2 0/9] Support of Nexio 42" and new default class for hid-multitouch

[PATCH v2 0/9] Support of Nexio 42" and new default class for hid-multitouch

[Benjamin Tissoires]

Hi guys,

so, this is the v2 of the support of win7/8 devices.

changes since v1:
- removed the "optimization" patches, as the benefit was minimum
- introduce a new callback "report" in hid-core that drivers can use to treat the
  report by having it entirely parsed
- rely on this new hook to support Nexio 42"

side notes:
- I've tested removing the heavy call to kzalloc in hid_input_field. The results
  are disapointing -> the processing time remains the same.
- I've also tested not to rely on .event hook in hid-multitouch but only on .report.
  Idem, I thought it would reduce the code of hid-multitouch and will enhance its
  processing time, but the results are a roughly same number of lines for hid-multitouch
  and the same processing time... :(
- these 2 tests helped in cleaning the patch set from the last time.

And again, finally, I've pass all the 40 regression tests of my db. \o/

Cheers,
Benjamin

Benjamin Tissoires (9):
  HID: core: add "report" hook, called once the report has been parsed
  HID: multitouch: use the callback "report" instead of sequential
    events
  HID: multitouch: add support for Nexio 42" panel
  HID: multitouch: fix Win8 protocol for Sharp like devices
  HID: multitouch: ensure that serial devices make no use of contact
    count
  HID: multitouch: fix protocol for Sitronix 1403:5001
  HID: multitouch: fix protocol for Cando 2087:0a02
  HID: multitouch: fix protocol for Elo panels
  HID: multitouch: make MT_CLS_ALWAYS_TRUE the new default class

 drivers/hid/hid-core.c       |   4 ++
 drivers/hid/hid-ids.h        |   3 +
 drivers/hid/hid-multitouch.c | 142 ++++++++++++++++++++++++++++++++-----------
 include/linux/hid.h          |   2 +
 4 files changed, 115 insertions(+), 36 deletions(-)

-- 
1.8.1

[PATCH v2 1/9] HID: core: add "report" hook, called once the report has been parsed

[Benjamin Tissoires]

This callback is called when the parsing of the report has been done
by hid-core (so after the calls to .event). The hid drivers can now
have access to the whole report by relying on the values stored in
the different fields.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
---
 drivers/hid/hid-core.c | 4 ++++
 include/linux/hid.h    | 2 ++
 2 files changed, 6 insertions(+)

diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 5ae2cb1..b671e4e 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1195,6 +1195,7 @@ int hid_report_raw_event(struct hid_device *hid, int type, u8 *data, int size,
 {
 	struct hid_report_enum *report_enum = hid->report_enum + type;
 	struct hid_report *report;
+	struct hid_driver *hdrv;
 	unsigned int a;
 	int rsize, csize = size;
 	u8 *cdata = data;
@@ -1231,6 +1232,9 @@ int hid_report_raw_event(struct hid_device *hid, int type, u8 *data, int size,
 	if (hid->claimed != HID_CLAIMED_HIDRAW) {
 		for (a = 0; a < report->maxfield; a++)
 			hid_input_field(hid, report->field[a], cdata, interrupt);
+		hdrv = hid->driver;
+		if (hdrv && hdrv->report)
+			hdrv->report(hid, report);
 	}
 
 	if (hid->claimed & HID_CLAIMED_INPUT)
diff --git a/include/linux/hid.h b/include/linux/hid.h
index 828726c..e14b465 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -589,6 +589,7 @@ struct hid_usage_id {
  * @raw_event: if report in report_table, this hook is called (NULL means nop)
  * @usage_table: on which events to call event (NULL means all)
  * @event: if usage in usage_table, this hook is called (NULL means nop)
+ * @report: this hook is called after parsing a report (NULL means nop)
  * @report_fixup: called before report descriptor parsing (NULL means nop)
  * @input_mapping: invoked on input registering before mapping an usage
  * @input_mapped: invoked on input registering after mapping an usage
@@ -627,6 +628,7 @@ struct hid_driver {
 	const struct hid_usage_id *usage_table;
 	int (*event)(struct hid_device *hdev, struct hid_field *field,
 			struct hid_usage *usage, __s32 value);
+	void (*report)(struct hid_device *hdev, struct hid_report *report);
 
 	__u8 *(*report_fixup)(struct hid_device *hdev, __u8 *buf,
 			unsigned int *size);
-- 
1.8.1

[PATCH v2 2/9] HID: multitouch: use the callback "report" instead of sequential events

[Benjamin Tissoires]

Nexio 42" devices requires to rely on the HID field Contact
Count to compute the valid values. However, this field is
most of the time at the end of the report, meaning that we
need to get the all report parsed before processing it.

This patch does not introduce functional changes.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
---
 drivers/hid/hid-multitouch.c | 44 ++++++++++++++++++++++++++++++++++++++------
 1 file changed, 38 insertions(+), 6 deletions(-)

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 46d8136..092c09b 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -85,6 +85,7 @@ struct mt_device {
 					   multitouch fields */
 	unsigned last_field_index;	/* last field index of the report */
 	unsigned last_slot_field;	/* the last field of a slot */
+	unsigned mt_report_id;	/* the report ID of the multitouch device */
 	__s8 inputmode;		/* InputMode HID feature, -1 if non-existent */
 	__s8 inputmode_index;	/* InputMode HID feature index in the report */
 	__s8 maxcontact_report_id;	/* Maximum Contact Number HID feature,
@@ -428,6 +429,7 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 			mt_store_field(usage, td, hi);
 			td->last_field_index = field->index;
 			td->touches_by_report++;
+			td->mt_report_id = field->report->id;
 			return 1;
 		case HID_DG_WIDTH:
 			hid_map_usage(hi, usage, bit, max,
@@ -578,6 +580,16 @@ static void mt_sync_frame(struct mt_device *td, struct input_dev *input)
 static int mt_event(struct hid_device *hid, struct hid_field *field,
 				struct hid_usage *usage, __s32 value)
 {
+	/* we will handle the hidinput part later, now remains hiddev */
+	if (hid->claimed & HID_CLAIMED_HIDDEV && hid->hiddev_hid_event)
+		hid->hiddev_hid_event(hid, field, usage, value);
+
+	return 1;
+}
+
+static void mt_process_mt_event(struct hid_device *hid, struct hid_field *field,
+				struct hid_usage *usage, __s32 value)
+{
 	struct mt_device *td = hid_get_drvdata(hid);
 	__s32 quirks = td->mtclass.quirks;
 
@@ -635,8 +647,7 @@ static int mt_event(struct hid_device *hid, struct hid_field *field,
 			break;
 
 		default:
-			/* fallback to the generic hidinput handling */
-			return 0;
+			return;
 		}
 
 		if (usage->usage_index + 1 == field->report_count) {
@@ -650,12 +661,32 @@ static int mt_event(struct hid_device *hid, struct hid_field *field,
 		}
 
 	}
+}
 
-	/* we have handled the hidinput part, now remains hiddev */
-	if (hid->claimed & HID_CLAIMED_HIDDEV && hid->hiddev_hid_event)
-		hid->hiddev_hid_event(hid, field, usage, value);
+static void mt_report(struct hid_device *hid, struct hid_report *report)
+{
+	struct mt_device *td = hid_get_drvdata(hid);
+	struct hid_field *field;
+	unsigned count;
+	int r, n;
 
-	return 1;
+	if (report->id != td->mt_report_id)
+		return;
+
+	if (!(hid->claimed & HID_CLAIMED_INPUT))
+		return;
+
+	for (r = 0; r < report->maxfield; r++) {
+		field = report->field[r];
+		count = field->report_count;
+
+		if (!(HID_MAIN_ITEM_VARIABLE & field->flags))
+			continue;
+
+		for (n = 0; n < count; n++)
+			mt_process_mt_event(hid, field, &field->usage[n],
+					field->value[n]);
+	}
 }
 
 static void mt_set_input_mode(struct hid_device *hdev)
@@ -1193,6 +1224,7 @@ static struct hid_driver mt_driver = {
 	.feature_mapping = mt_feature_mapping,
 	.usage_table = mt_grabbed_usages,
 	.event = mt_event,
+	.report = mt_report,
 #ifdef CONFIG_PM
 	.reset_resume = mt_reset_resume,
 	.resume = mt_resume,
-- 
1.8.1

[PATCH v2 3/9] HID: multitouch: add support for Nexio 42" panel

[Benjamin Tissoires]

This device is the worst device I saw. It keeps TipSwitch and InRange
at 1 for fingers that are not touching the panel.
The solution is to rely on the field ContactCount, which is accurate
as the correct information are packed at the begining of the frame.

Unfortunately, CountactCount is most of the time at the end of the report.
The solution is to pick it when we have the whole report in raw_event.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
---
 drivers/hid/hid-ids.h        |  3 +++
 drivers/hid/hid-multitouch.c | 36 ++++++++++++++++++++++++++++++------
 2 files changed, 33 insertions(+), 6 deletions(-)

diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index dad56aa..0935012 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -597,6 +597,9 @@
 #define USB_VENDOR_ID_NEC		0x073e
 #define USB_DEVICE_ID_NEC_USB_GAME_PAD	0x0301
 
+#define USB_VENDOR_ID_NEXIO		0x1870
+#define USB_DEVICE_ID_NEXIO_MULTITOUCH_420	0x010d
+
 #define USB_VENDOR_ID_NEXTWINDOW	0x1926
 #define USB_DEVICE_ID_NEXTWINDOW_TOUCHSCREEN	0x0003
 
diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 092c09b..c9b8fe5 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -54,6 +54,7 @@ MODULE_LICENSE("GPL");
 #define MT_QUIRK_NO_AREA		(1 << 9)
 #define MT_QUIRK_IGNORE_DUPLICATES	(1 << 10)
 #define MT_QUIRK_HOVERING		(1 << 11)
+#define MT_QUIRK_CONTACT_CNT_ACCURATE	(1 << 12)
 
 struct mt_slot {
 	__s32 x, y, cx, cy, p, w, h;
@@ -83,6 +84,7 @@ struct mt_device {
 	struct mt_class mtclass;	/* our mt device class */
 	struct mt_fields *fields;	/* temporary placeholder for storing the
 					   multitouch fields */
+	__s32 *contactcount;		/* contact count value in the report */
 	unsigned last_field_index;	/* last field index of the report */
 	unsigned last_slot_field;	/* the last field of a slot */
 	unsigned mt_report_id;	/* the report ID of the multitouch device */
@@ -112,6 +114,7 @@ struct mt_device {
 #define MT_CLS_DUAL_INRANGE_CONTACTNUMBER	0x0007
 #define MT_CLS_DUAL_NSMU_CONTACTID		0x0008
 #define MT_CLS_INRANGE_CONTACTNUMBER		0x0009
+#define MT_CLS_ALWAYS_TRUE			0x000a
 
 /* vendor specific classes */
 #define MT_CLS_3M				0x0101
@@ -171,6 +174,9 @@ static struct mt_class mt_classes[] = {
 	{ .name = MT_CLS_INRANGE_CONTACTNUMBER,
 		.quirks = MT_QUIRK_VALID_IS_INRANGE |
 			MT_QUIRK_SLOT_IS_CONTACTNUMBER },
+	{ .name = MT_CLS_ALWAYS_TRUE,
+		.quirks = MT_QUIRK_ALWAYS_VALID |
+			MT_QUIRK_CONTACT_CNT_ACCURATE },
 
 	/*
 	 * vendor specific classes
@@ -251,6 +257,9 @@ static ssize_t mt_set_quirks(struct device *dev,
 
 	td->mtclass.quirks = val;
 
+	if (!td->contactcount)
+		td->mtclass.quirks &= ~MT_QUIRK_CONTACT_CNT_ACCURATE;
+
 	return count;
 }
 
@@ -461,6 +470,7 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 			td->last_field_index = field->index;
 			return 1;
 		case HID_DG_CONTACTCOUNT:
+			td->contactcount = field->value + usage->usage_index;
 			td->last_field_index = field->index;
 			return 1;
 		case HID_DG_CONTACTMAX:
@@ -525,6 +535,10 @@ static int mt_compute_slot(struct mt_device *td, struct input_dev *input)
  */
 static void mt_complete_slot(struct mt_device *td, struct input_dev *input)
 {
+	if ((td->mtclass.quirks & MT_QUIRK_CONTACT_CNT_ACCURATE) &&
+	    td->num_received >= td->num_expected)
+		return;
+
 	if (td->curvalid || (td->mtclass.quirks & MT_QUIRK_ALWAYS_VALID)) {
 		int slotnum = mt_compute_slot(td, input);
 		struct mt_slot *s = &td->curdata;
@@ -635,12 +649,6 @@ static void mt_process_mt_event(struct hid_device *hid, struct hid_field *field,
 			td->curdata.h = value;
 			break;
 		case HID_DG_CONTACTCOUNT:
-			/*
-			 * Includes multi-packet support where subsequent
-			 * packets are sent with zero contactcount.
-			 */
-			if (value)
-				td->num_expected = value;
 			break;
 		case HID_DG_TOUCH:
 			/* do nothing */
@@ -676,6 +684,13 @@ static void mt_report(struct hid_device *hid, struct hid_report *report)
 	if (!(hid->claimed & HID_CLAIMED_INPUT))
 		return;
 
+	/*
+	 * Includes multi-packet support where subsequent
+	 * packets are sent with zero contactcount.
+	 */
+	if (td->contactcount && *td->contactcount)
+		td->num_expected = *td->contactcount;
+
 	for (r = 0; r < report->maxfield; r++) {
 		field = report->field[r];
 		count = field->report_count;
@@ -750,11 +765,15 @@ static void mt_post_parse_default_settings(struct mt_device *td)
 static void mt_post_parse(struct mt_device *td)
 {
 	struct mt_fields *f = td->fields;
+	struct mt_class *cls = &td->mtclass;
 
 	if (td->touches_by_report > 0) {
 		int field_count_per_touch = f->length / td->touches_by_report;
 		td->last_slot_field = f->usages[field_count_per_touch - 1];
 	}
+
+	if (!td->contactcount)
+		cls->quirks &= ~MT_QUIRK_CONTACT_CNT_ACCURATE;
 }
 
 static void mt_input_configured(struct hid_device *hdev, struct hid_input *hi)
@@ -1087,6 +1106,11 @@ static const struct hid_device_id mt_devices[] = {
 		MT_USB_DEVICE(USB_VENDOR_ID_TURBOX,
 			USB_DEVICE_ID_TURBOX_TOUCHSCREEN_MOSART) },
 
+	/* Nexio panels */
+	{ .driver_data = MT_CLS_ALWAYS_TRUE,
+		MT_USB_DEVICE(USB_VENDOR_ID_NEXIO,
+			USB_DEVICE_ID_NEXIO_MULTITOUCH_420)},
+
 	/* Panasonic panels */
 	{ .driver_data = MT_CLS_PANASONIC,
 		MT_USB_DEVICE(USB_VENDOR_ID_PANASONIC,
-- 
1.8.1

[PATCH v2 4/9] HID: multitouch: fix Win8 protocol for Sharp like devices

[Benjamin Tissoires]

The Sharp LC-20FE1-W screen (04dd:9681) behaves like the Nexio 42".
It may report out of ranges values that are filtered out by relying
on the Contact Count HID field.
Adding the quirk MT_QUIRK_CONTACT_CNT_ACCURATE makes hid-multitouch
strongest against this kind of device, without breaking the current
devices.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
---
 drivers/hid/hid-multitouch.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index c9b8fe5..585a813 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -311,6 +311,7 @@ static void mt_feature_mapping(struct hid_device *hdev,
 			*quirks |= MT_QUIRK_ALWAYS_VALID;
 			*quirks |= MT_QUIRK_IGNORE_DUPLICATES;
 			*quirks |= MT_QUIRK_HOVERING;
+			*quirks |= MT_QUIRK_CONTACT_CNT_ACCURATE;
 			*quirks &= ~MT_QUIRK_NOT_SEEN_MEANS_UP;
 			*quirks &= ~MT_QUIRK_VALID_IS_INRANGE;
 			*quirks &= ~MT_QUIRK_VALID_IS_CONFIDENCE;
-- 
1.8.1

[PATCH v2 5/9] HID: multitouch: ensure that serial devices make no use of contact count

[Benjamin Tissoires]

The serial protocol makes contact count a redondant information, and
sometimes it is not reliable (TRS-Star are in this case).
Disabling the use of contact count for these devices is thus safer.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
---
 drivers/hid/hid-multitouch.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 585a813..86531b1 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -758,6 +758,7 @@ static void mt_post_parse_default_settings(struct mt_device *td)
 		quirks &= ~MT_QUIRK_NOT_SEEN_MEANS_UP;
 		quirks &= ~MT_QUIRK_VALID_IS_INRANGE;
 		quirks &= ~MT_QUIRK_VALID_IS_CONFIDENCE;
+		quirks &= ~MT_QUIRK_CONTACT_CNT_ACCURATE;
 	}
 
 	td->mtclass.quirks = quirks;
-- 
1.8.1

[PATCH v2 6/9] HID: multitouch: fix protocol for Sitronix 1403:5001

[Benjamin Tissoires]

Since the inclusion of this device in hid-multitouch, the device
did not forward any events. Using the serial class makes it working
again.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
---
 drivers/hid/hid-multitouch.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 86531b1..71daf57 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -1168,7 +1168,7 @@ static const struct hid_device_id mt_devices[] = {
 	{ .driver_data = MT_CLS_CONFIDENCE,
 		MT_USB_DEVICE(USB_VENDOR_ID_STANTUM_STM,
 			USB_DEVICE_ID_MTP_STM)},
-	{ .driver_data = MT_CLS_CONFIDENCE,
+	{ .driver_data = MT_CLS_ALWAYS_TRUE,
 		MT_USB_DEVICE(USB_VENDOR_ID_STANTUM_SITRONIX,
 			USB_DEVICE_ID_MTP_SITRONIX)},
 
-- 
1.8.1

[PATCH v2 7/9] HID: multitouch: fix protocol for Cando 2087:0a02

[Benjamin Tissoires]

Cando 2087:0a02 was broken, this fixes it.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
---
 drivers/hid/hid-multitouch.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 71daf57..f106e90 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -115,6 +115,7 @@ struct mt_device {
 #define MT_CLS_DUAL_NSMU_CONTACTID		0x0008
 #define MT_CLS_INRANGE_CONTACTNUMBER		0x0009
 #define MT_CLS_ALWAYS_TRUE			0x000a
+#define MT_CLS_DUAL_CONTACT_NUMBER		0x0010
 
 /* vendor specific classes */
 #define MT_CLS_3M				0x0101
@@ -177,6 +178,11 @@ static struct mt_class mt_classes[] = {
 	{ .name = MT_CLS_ALWAYS_TRUE,
 		.quirks = MT_QUIRK_ALWAYS_VALID |
 			MT_QUIRK_CONTACT_CNT_ACCURATE },
+	{ .name = MT_CLS_DUAL_CONTACT_NUMBER,
+		.quirks = MT_QUIRK_ALWAYS_VALID |
+			MT_QUIRK_CONTACT_CNT_ACCURATE |
+			MT_QUIRK_SLOT_IS_CONTACTNUMBER,
+		.maxcontacts = 2 },
 
 	/*
 	 * vendor specific classes
@@ -947,7 +953,7 @@ static const struct hid_device_id mt_devices[] = {
 	{ .driver_data = MT_CLS_DUAL_INRANGE_CONTACTNUMBER,
 		MT_USB_DEVICE(USB_VENDOR_ID_CANDO,
 			USB_DEVICE_ID_CANDO_MULTI_TOUCH) },
-	{ .driver_data = MT_CLS_DUAL_INRANGE_CONTACTNUMBER,
+	{ .driver_data = MT_CLS_DUAL_CONTACT_NUMBER,
 		MT_USB_DEVICE(USB_VENDOR_ID_CANDO,
 			USB_DEVICE_ID_CANDO_MULTI_TOUCH_10_1) },
 	{ .driver_data = MT_CLS_DUAL_INRANGE_CONTACTNUMBER,
-- 
1.8.1

[PATCH v2 8/9] HID: multitouch: fix protocol for Elo panels

[Benjamin Tissoires]

The previous protocol was nearly working, but when several fingers were
present on the sensor, those that were not moving were not updated
in the next report, introducing a lot of releases.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>

Conflicts:
	drivers/hid/hid-multitouch.c
---
 drivers/hid/hid-multitouch.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index f106e90..68d511c 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -116,6 +116,7 @@ struct mt_device {
 #define MT_CLS_INRANGE_CONTACTNUMBER		0x0009
 #define MT_CLS_ALWAYS_TRUE			0x000a
 #define MT_CLS_DUAL_CONTACT_NUMBER		0x0010
+#define MT_CLS_DUAL_CONTACT_ID			0x0011
 
 /* vendor specific classes */
 #define MT_CLS_3M				0x0101
@@ -183,6 +184,11 @@ static struct mt_class mt_classes[] = {
 			MT_QUIRK_CONTACT_CNT_ACCURATE |
 			MT_QUIRK_SLOT_IS_CONTACTNUMBER,
 		.maxcontacts = 2 },
+	{ .name = MT_CLS_DUAL_CONTACT_ID,
+		.quirks = MT_QUIRK_ALWAYS_VALID |
+			MT_QUIRK_CONTACT_CNT_ACCURATE |
+			MT_QUIRK_SLOT_IS_CONTACTID,
+		.maxcontacts = 2 },
 
 	/*
 	 * vendor specific classes
@@ -1040,7 +1046,7 @@ static const struct hid_device_id mt_devices[] = {
 			USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_72C4) },
 
 	/* Elo TouchSystems IntelliTouch Plus panel */
-	{ .driver_data = MT_CLS_DUAL_NSMU_CONTACTID,
+	{ .driver_data = MT_CLS_DUAL_CONTACT_ID,
 		MT_USB_DEVICE(USB_VENDOR_ID_ELO,
 			USB_DEVICE_ID_ELO_TS2515) },
 
-- 
1.8.1

[PATCH v2 9/9] HID: multitouch: make MT_CLS_ALWAYS_TRUE the new default class

[Benjamin Tissoires]

By running a test on all the traces of the devices I have,
I noticed that the class MT_CLS_ALWAYS_TRUE could handle all
the devices I've seen so far without any other quirks.
I guess this is the behavior Win 7 requires in its driver.

We can change the default class then and keep the existing classes
for backward compatibility and performances for some of them.

Two operations have been done:
 * replaced MT_CLS_DEFAULT by MT_CLS_NSMU
 * then replaced MT_CLS_ALWAYS_TRUE by MT_CLS_DEFAULT

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
---
 drivers/hid/hid-multitouch.c | 54 ++++++++++++++++++++++----------------------
 1 file changed, 27 insertions(+), 27 deletions(-)

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 68d511c..5490e70 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -114,7 +114,7 @@ struct mt_device {
 #define MT_CLS_DUAL_INRANGE_CONTACTNUMBER	0x0007
 #define MT_CLS_DUAL_NSMU_CONTACTID		0x0008
 #define MT_CLS_INRANGE_CONTACTNUMBER		0x0009
-#define MT_CLS_ALWAYS_TRUE			0x000a
+#define MT_CLS_NSMU				0x000a
 #define MT_CLS_DUAL_CONTACT_NUMBER		0x0010
 #define MT_CLS_DUAL_CONTACT_ID			0x0011
 
@@ -150,6 +150,9 @@ static int cypress_compute_slot(struct mt_device *td)
 
 static struct mt_class mt_classes[] = {
 	{ .name = MT_CLS_DEFAULT,
+		.quirks = MT_QUIRK_ALWAYS_VALID |
+			MT_QUIRK_CONTACT_CNT_ACCURATE },
+	{ .name = MT_CLS_NSMU,
 		.quirks = MT_QUIRK_NOT_SEEN_MEANS_UP },
 	{ .name = MT_CLS_SERIAL,
 		.quirks = MT_QUIRK_ALWAYS_VALID},
@@ -176,9 +179,6 @@ static struct mt_class mt_classes[] = {
 	{ .name = MT_CLS_INRANGE_CONTACTNUMBER,
 		.quirks = MT_QUIRK_VALID_IS_INRANGE |
 			MT_QUIRK_SLOT_IS_CONTACTNUMBER },
-	{ .name = MT_CLS_ALWAYS_TRUE,
-		.quirks = MT_QUIRK_ALWAYS_VALID |
-			MT_QUIRK_CONTACT_CNT_ACCURATE },
 	{ .name = MT_CLS_DUAL_CONTACT_NUMBER,
 		.quirks = MT_QUIRK_ALWAYS_VALID |
 			MT_QUIRK_CONTACT_CNT_ACCURATE |
@@ -939,7 +939,7 @@ static const struct hid_device_id mt_devices[] = {
 			USB_DEVICE_ID_3M3266) },
 
 	/* ActionStar panels */
-	{ .driver_data = MT_CLS_DEFAULT,
+	{ .driver_data = MT_CLS_NSMU,
 		MT_USB_DEVICE(USB_VENDOR_ID_ACTIONSTAR,
 			USB_DEVICE_ID_ACTIONSTAR_1011) },
 
@@ -952,7 +952,7 @@ static const struct hid_device_id mt_devices[] = {
 			USB_DEVICE_ID_ATMEL_MXT_DIGITIZER) },
 
 	/* Baanto multitouch devices */
-	{ .driver_data = MT_CLS_DEFAULT,
+	{ .driver_data = MT_CLS_NSMU,
 		MT_USB_DEVICE(USB_VENDOR_ID_BAANTO,
 			USB_DEVICE_ID_BAANTO_MT_190W2) },
 	/* Cando panels */
@@ -970,12 +970,12 @@ static const struct hid_device_id mt_devices[] = {
 			USB_DEVICE_ID_CANDO_MULTI_TOUCH_15_6) },
 
 	/* Chunghwa Telecom touch panels */
-	{  .driver_data = MT_CLS_DEFAULT,
+	{  .driver_data = MT_CLS_NSMU,
 		MT_USB_DEVICE(USB_VENDOR_ID_CHUNGHWAT,
 			USB_DEVICE_ID_CHUNGHWAT_MULTITOUCH) },
 
 	/* CVTouch panels */
-	{ .driver_data = MT_CLS_DEFAULT,
+	{ .driver_data = MT_CLS_NSMU,
 		MT_USB_DEVICE(USB_VENDOR_ID_CVTOUCH,
 			USB_DEVICE_ID_CVTOUCH_SCREEN) },
 
@@ -1064,12 +1064,12 @@ static const struct hid_device_id mt_devices[] = {
 			USB_DEVICE_ID_GENERAL_TOUCH_WIN8_PWT_TENFINGERS) },
 
 	/* Gametel game controller */
-	{ .driver_data = MT_CLS_DEFAULT,
+	{ .driver_data = MT_CLS_NSMU,
 		MT_BT_DEVICE(USB_VENDOR_ID_FRUCTEL,
 			USB_DEVICE_ID_GAMETEL_MT_MODE) },
 
 	/* GoodTouch panels */
-	{ .driver_data = MT_CLS_DEFAULT,
+	{ .driver_data = MT_CLS_NSMU,
 		MT_USB_DEVICE(USB_VENDOR_ID_GOODTOUCH,
 			USB_DEVICE_ID_GOODTOUCH_000f) },
 
@@ -1087,7 +1087,7 @@ static const struct hid_device_id mt_devices[] = {
 			USB_DEVICE_ID_IDEACOM_IDC6651) },
 
 	/* Ilitek dual touch panel */
-	{  .driver_data = MT_CLS_DEFAULT,
+	{  .driver_data = MT_CLS_NSMU,
 		MT_USB_DEVICE(USB_VENDOR_ID_ILITEK,
 			USB_DEVICE_ID_ILITEK_MULTITOUCH) },
 
@@ -1121,7 +1121,7 @@ static const struct hid_device_id mt_devices[] = {
 			USB_DEVICE_ID_TURBOX_TOUCHSCREEN_MOSART) },
 
 	/* Nexio panels */
-	{ .driver_data = MT_CLS_ALWAYS_TRUE,
+	{ .driver_data = MT_CLS_DEFAULT,
 		MT_USB_DEVICE(USB_VENDOR_ID_NEXIO,
 			USB_DEVICE_ID_NEXIO_MULTITOUCH_420)},
 
@@ -1134,7 +1134,7 @@ static const struct hid_device_id mt_devices[] = {
 			USB_DEVICE_ID_PANABOARD_UBT880) },
 
 	/* Novatek Panel */
-	{ .driver_data = MT_CLS_DEFAULT,
+	{ .driver_data = MT_CLS_NSMU,
 		MT_USB_DEVICE(USB_VENDOR_ID_NOVATEK,
 			USB_DEVICE_ID_NOVATEK_PCT) },
 
@@ -1180,7 +1180,7 @@ static const struct hid_device_id mt_devices[] = {
 	{ .driver_data = MT_CLS_CONFIDENCE,
 		MT_USB_DEVICE(USB_VENDOR_ID_STANTUM_STM,
 			USB_DEVICE_ID_MTP_STM)},
-	{ .driver_data = MT_CLS_ALWAYS_TRUE,
+	{ .driver_data = MT_CLS_DEFAULT,
 		MT_USB_DEVICE(USB_VENDOR_ID_STANTUM_SITRONIX,
 			USB_DEVICE_ID_MTP_SITRONIX)},
 
@@ -1190,48 +1190,48 @@ static const struct hid_device_id mt_devices[] = {
 			USB_DEVICE_ID_TOPSEED2_PERIPAD_701) },
 
 	/* Touch International panels */
-	{ .driver_data = MT_CLS_DEFAULT,
+	{ .driver_data = MT_CLS_NSMU,
 		MT_USB_DEVICE(USB_VENDOR_ID_TOUCH_INTL,
 			USB_DEVICE_ID_TOUCH_INTL_MULTI_TOUCH) },
 
 	/* Unitec panels */
-	{ .driver_data = MT_CLS_DEFAULT,
+	{ .driver_data = MT_CLS_NSMU,
 		MT_USB_DEVICE(USB_VENDOR_ID_UNITEC,
 			USB_DEVICE_ID_UNITEC_USB_TOUCH_0709) },
-	{ .driver_data = MT_CLS_DEFAULT,
+	{ .driver_data = MT_CLS_NSMU,
 		MT_USB_DEVICE(USB_VENDOR_ID_UNITEC,
 			USB_DEVICE_ID_UNITEC_USB_TOUCH_0A19) },
 	/* XAT */
-	{ .driver_data = MT_CLS_DEFAULT,
+	{ .driver_data = MT_CLS_NSMU,
 		MT_USB_DEVICE(USB_VENDOR_ID_XAT,
 			USB_DEVICE_ID_XAT_CSR) },
 
 	/* Xiroku */
-	{ .driver_data = MT_CLS_DEFAULT,
+	{ .driver_data = MT_CLS_NSMU,
 		MT_USB_DEVICE(USB_VENDOR_ID_XIROKU,
 			USB_DEVICE_ID_XIROKU_SPX) },
-	{ .driver_data = MT_CLS_DEFAULT,
+	{ .driver_data = MT_CLS_NSMU,
 		MT_USB_DEVICE(USB_VENDOR_ID_XIROKU,
 			USB_DEVICE_ID_XIROKU_MPX) },
-	{ .driver_data = MT_CLS_DEFAULT,
+	{ .driver_data = MT_CLS_NSMU,
 		MT_USB_DEVICE(USB_VENDOR_ID_XIROKU,
 			USB_DEVICE_ID_XIROKU_CSR) },
-	{ .driver_data = MT_CLS_DEFAULT,
+	{ .driver_data = MT_CLS_NSMU,
 		MT_USB_DEVICE(USB_VENDOR_ID_XIROKU,
 			USB_DEVICE_ID_XIROKU_SPX1) },
-	{ .driver_data = MT_CLS_DEFAULT,
+	{ .driver_data = MT_CLS_NSMU,
 		MT_USB_DEVICE(USB_VENDOR_ID_XIROKU,
 			USB_DEVICE_ID_XIROKU_MPX1) },
-	{ .driver_data = MT_CLS_DEFAULT,
+	{ .driver_data = MT_CLS_NSMU,
 		MT_USB_DEVICE(USB_VENDOR_ID_XIROKU,
 			USB_DEVICE_ID_XIROKU_CSR1) },
-	{ .driver_data = MT_CLS_DEFAULT,
+	{ .driver_data = MT_CLS_NSMU,
 		MT_USB_DEVICE(USB_VENDOR_ID_XIROKU,
 			USB_DEVICE_ID_XIROKU_SPX2) },
-	{ .driver_data = MT_CLS_DEFAULT,
+	{ .driver_data = MT_CLS_NSMU,
 		MT_USB_DEVICE(USB_VENDOR_ID_XIROKU,
 			USB_DEVICE_ID_XIROKU_MPX2) },
-	{ .driver_data = MT_CLS_DEFAULT,
+	{ .driver_data = MT_CLS_NSMU,
 		MT_USB_DEVICE(USB_VENDOR_ID_XIROKU,
 			USB_DEVICE_ID_XIROKU_CSR2) },
 
-- 
1.8.1

Re: [PATCH v2 1/9] HID: core: add "report" hook, called once the report has been parsed

[Henrik Rydberg]

Hi Benjamin,

> This callback is called when the parsing of the report has been done
> by hid-core (so after the calls to .event). The hid drivers can now
> have access to the whole report by relying on the values stored in
> the different fields.
> 
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
> ---
>  drivers/hid/hid-core.c | 4 ++++
>  include/linux/hid.h    | 2 ++
>  2 files changed, 6 insertions(+)
> 
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index 5ae2cb1..b671e4e 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -1195,6 +1195,7 @@ int hid_report_raw_event(struct hid_device *hid, int type, u8 *data, int size,
>  {
>  	struct hid_report_enum *report_enum = hid->report_enum + type;
>  	struct hid_report *report;
> +	struct hid_driver *hdrv;
>  	unsigned int a;
>  	int rsize, csize = size;
>  	u8 *cdata = data;
> @@ -1231,6 +1232,9 @@ int hid_report_raw_event(struct hid_device *hid, int type, u8 *data, int size,
>  	if (hid->claimed != HID_CLAIMED_HIDRAW) {
>  		for (a = 0; a < report->maxfield; a++)
>  			hid_input_field(hid, report->field[a], cdata, interrupt);
> +		hdrv = hid->driver;
> +		if (hdrv && hdrv->report)
> +			hdrv->report(hid, report);

I think this is more useful if called before the individual fields. In
fact, it seems raw_event() is already doing exactly that. No need for
a new callback, in other words.

>  	}
>  
>  	if (hid->claimed & HID_CLAIMED_INPUT)
> diff --git a/include/linux/hid.h b/include/linux/hid.h
> index 828726c..e14b465 100644
> --- a/include/linux/hid.h
> +++ b/include/linux/hid.h
> @@ -589,6 +589,7 @@ struct hid_usage_id {
>   * @raw_event: if report in report_table, this hook is called (NULL means nop)
>   * @usage_table: on which events to call event (NULL means all)
>   * @event: if usage in usage_table, this hook is called (NULL means nop)
> + * @report: this hook is called after parsing a report (NULL means nop)
>   * @report_fixup: called before report descriptor parsing (NULL means nop)
>   * @input_mapping: invoked on input registering before mapping an usage
>   * @input_mapped: invoked on input registering after mapping an usage
> @@ -627,6 +628,7 @@ struct hid_driver {
>  	const struct hid_usage_id *usage_table;
>  	int (*event)(struct hid_device *hdev, struct hid_field *field,
>  			struct hid_usage *usage, __s32 value);
> +	void (*report)(struct hid_device *hdev, struct hid_report *report);
>  
>  	__u8 *(*report_fixup)(struct hid_device *hdev, __u8 *buf,
>  			unsigned int *size);
> -- 
> 1.8.1
> 

Thanks,
Henrik

Re: [PATCH v2 3/9] HID: multitouch: add support for Nexio 42" panel

[Henrik Rydberg]

Hi Benjamin,

> This device is the worst device I saw. It keeps TipSwitch and InRange
> at 1 for fingers that are not touching the panel.
> The solution is to rely on the field ContactCount, which is accurate
> as the correct information are packed at the begining of the frame.
> 
> Unfortunately, CountactCount is most of the time at the end of the report.
> The solution is to pick it when we have the whole report in raw_event.
> 
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
> ---
>  drivers/hid/hid-ids.h        |  3 +++
>  drivers/hid/hid-multitouch.c | 36 ++++++++++++++++++++++++++++++------
>  2 files changed, 33 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
> index dad56aa..0935012 100644
> --- a/drivers/hid/hid-ids.h
> +++ b/drivers/hid/hid-ids.h
> @@ -597,6 +597,9 @@
>  #define USB_VENDOR_ID_NEC		0x073e
>  #define USB_DEVICE_ID_NEC_USB_GAME_PAD	0x0301
>  
> +#define USB_VENDOR_ID_NEXIO		0x1870
> +#define USB_DEVICE_ID_NEXIO_MULTITOUCH_420	0x010d
> +
>  #define USB_VENDOR_ID_NEXTWINDOW	0x1926
>  #define USB_DEVICE_ID_NEXTWINDOW_TOUCHSCREEN	0x0003
>  
> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> index 092c09b..c9b8fe5 100644
> --- a/drivers/hid/hid-multitouch.c
> +++ b/drivers/hid/hid-multitouch.c
> @@ -54,6 +54,7 @@ MODULE_LICENSE("GPL");
>  #define MT_QUIRK_NO_AREA		(1 << 9)
>  #define MT_QUIRK_IGNORE_DUPLICATES	(1 << 10)
>  #define MT_QUIRK_HOVERING		(1 << 11)
> +#define MT_QUIRK_CONTACT_CNT_ACCURATE	(1 << 12)
>  
>  struct mt_slot {
>  	__s32 x, y, cx, cy, p, w, h;
> @@ -83,6 +84,7 @@ struct mt_device {
>  	struct mt_class mtclass;	/* our mt device class */
>  	struct mt_fields *fields;	/* temporary placeholder for storing the
>  					   multitouch fields */
> +	__s32 *contactcount;		/* contact count value in the report */

Why not an index here?

>  	unsigned last_field_index;	/* last field index of the report */
>  	unsigned last_slot_field;	/* the last field of a slot */
>  	unsigned mt_report_id;	/* the report ID of the multitouch device */
> @@ -112,6 +114,7 @@ struct mt_device {
>  #define MT_CLS_DUAL_INRANGE_CONTACTNUMBER	0x0007
>  #define MT_CLS_DUAL_NSMU_CONTACTID		0x0008
>  #define MT_CLS_INRANGE_CONTACTNUMBER		0x0009
> +#define MT_CLS_ALWAYS_TRUE			0x000a
>  
>  /* vendor specific classes */
>  #define MT_CLS_3M				0x0101
> @@ -171,6 +174,9 @@ static struct mt_class mt_classes[] = {
>  	{ .name = MT_CLS_INRANGE_CONTACTNUMBER,
>  		.quirks = MT_QUIRK_VALID_IS_INRANGE |
>  			MT_QUIRK_SLOT_IS_CONTACTNUMBER },
> +	{ .name = MT_CLS_ALWAYS_TRUE,
> +		.quirks = MT_QUIRK_ALWAYS_VALID |
> +			MT_QUIRK_CONTACT_CNT_ACCURATE },
>  
>  	/*
>  	 * vendor specific classes
> @@ -251,6 +257,9 @@ static ssize_t mt_set_quirks(struct device *dev,
>  
>  	td->mtclass.quirks = val;
>  
> +	if (!td->contactcount)
> +		td->mtclass.quirks &= ~MT_QUIRK_CONTACT_CNT_ACCURATE;
> +

Why override the overrider here?

>  	return count;
>  }
>  
> @@ -461,6 +470,7 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
>  			td->last_field_index = field->index;
>  			return 1;
>  		case HID_DG_CONTACTCOUNT:
> +			td->contactcount = field->value + usage->usage_index;

An index into the the struct actually passed in mt_report() feels safer.

>  			td->last_field_index = field->index;
>  			return 1;
>  		case HID_DG_CONTACTMAX:
> @@ -525,6 +535,10 @@ static int mt_compute_slot(struct mt_device *td, struct input_dev *input)
>   */
>  static void mt_complete_slot(struct mt_device *td, struct input_dev *input)
>  {
> +	if ((td->mtclass.quirks & MT_QUIRK_CONTACT_CNT_ACCURATE) &&
> +	    td->num_received >= td->num_expected)
> +		return;
> +
>  	if (td->curvalid || (td->mtclass.quirks & MT_QUIRK_ALWAYS_VALID)) {
>  		int slotnum = mt_compute_slot(td, input);
>  		struct mt_slot *s = &td->curdata;
> @@ -635,12 +649,6 @@ static void mt_process_mt_event(struct hid_device *hid, struct hid_field *field,
>  			td->curdata.h = value;
>  			break;
>  		case HID_DG_CONTACTCOUNT:
> -			/*
> -			 * Includes multi-packet support where subsequent
> -			 * packets are sent with zero contactcount.
> -			 */
> -			if (value)
> -				td->num_expected = value;
>  			break;
>  		case HID_DG_TOUCH:
>  			/* do nothing */
> @@ -676,6 +684,13 @@ static void mt_report(struct hid_device *hid, struct hid_report *report)
>  	if (!(hid->claimed & HID_CLAIMED_INPUT))
>  		return;
>  
> +	/*
> +	 * Includes multi-packet support where subsequent
> +	 * packets are sent with zero contactcount.
> +	 */
> +	if (td->contactcount && *td->contactcount)
> +		td->num_expected = *td->contactcount;
> +

Here, that is.

>  	for (r = 0; r < report->maxfield; r++) {
>  		field = report->field[r];
>  		count = field->report_count;
> @@ -750,11 +765,15 @@ static void mt_post_parse_default_settings(struct mt_device *td)
>  static void mt_post_parse(struct mt_device *td)
>  {
>  	struct mt_fields *f = td->fields;
> +	struct mt_class *cls = &td->mtclass;
>  
>  	if (td->touches_by_report > 0) {
>  		int field_count_per_touch = f->length / td->touches_by_report;
>  		td->last_slot_field = f->usages[field_count_per_touch - 1];
>  	}
> +
> +	if (!td->contactcount)
> +		cls->quirks &= ~MT_QUIRK_CONTACT_CNT_ACCURATE;

Since MT_QUIRK_CONTACT_CNT_ACCURATE is a quirk, modifiable by the
user, it should probably not validate num_expected in the code. Better
use the contact count index or something equivalent for that.

>  }
>  
>  static void mt_input_configured(struct hid_device *hdev, struct hid_input *hi)
> @@ -1087,6 +1106,11 @@ static const struct hid_device_id mt_devices[] = {
>  		MT_USB_DEVICE(USB_VENDOR_ID_TURBOX,
>  			USB_DEVICE_ID_TURBOX_TOUCHSCREEN_MOSART) },
>  
> +	/* Nexio panels */
> +	{ .driver_data = MT_CLS_ALWAYS_TRUE,
> +		MT_USB_DEVICE(USB_VENDOR_ID_NEXIO,
> +			USB_DEVICE_ID_NEXIO_MULTITOUCH_420)},
> +
>  	/* Panasonic panels */
>  	{ .driver_data = MT_CLS_PANASONIC,
>  		MT_USB_DEVICE(USB_VENDOR_ID_PANASONIC,
> -- 
> 1.8.1
> 

Thanks,
Henrik

Re: [PATCH v2 0/9] Support of Nexio 42" and new default class for hid-multitouch

[Henrik Rydberg]

Hi Benjamin,

> so, this is the v2 of the support of win7/8 devices.

Looks like it is getting there, thanks.

> changes since v1:
> - removed the "optimization" patches, as the benefit was minimum
> - introduce a new callback "report" in hid-core that drivers can use to treat the
>   report by having it entirely parsed
> - rely on this new hook to support Nexio 42"

As noted in the patches comments, using raw_event() seems sufficient.

> side notes:
> - I've tested removing the heavy call to kzalloc in hid_input_field. The results
>   are disapointing -> the processing time remains the same.
> - I've also tested not to rely on .event hook in hid-multitouch but only on .report.
>   Idem, I thought it would reduce the code of hid-multitouch and will enhance its
>   processing time, but the results are a roughly same number of lines for hid-multitouch
>   and the same processing time... :(
> - these 2 tests helped in cleaning the patch set from the last time.
> 
> And again, finally, I've pass all the 40 regression tests of my db. \o/

Nice. :-)

Thanks,
Henrik

Re: [PATCH v2 1/9] HID: core: add "report" hook, called once the report has been parsed

[Benjamin Tissoires]

Hi Henrik,

On Sun, Feb 3, 2013 at 1:27 PM, Henrik Rydberg <rydberg@euromail.se> wrote:
> Hi Benjamin,
>
>> This callback is called when the parsing of the report has been done
>> by hid-core (so after the calls to .event). The hid drivers can now
>> have access to the whole report by relying on the values stored in
>> the different fields.
>>
>> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
>> ---
>>  drivers/hid/hid-core.c | 4 ++++
>>  include/linux/hid.h    | 2 ++
>>  2 files changed, 6 insertions(+)
>>
>> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
>> index 5ae2cb1..b671e4e 100644
>> --- a/drivers/hid/hid-core.c
>> +++ b/drivers/hid/hid-core.c
>> @@ -1195,6 +1195,7 @@ int hid_report_raw_event(struct hid_device *hid, int type, u8 *data, int size,
>>  {
>>       struct hid_report_enum *report_enum = hid->report_enum + type;
>>       struct hid_report *report;
>> +     struct hid_driver *hdrv;
>>       unsigned int a;
>>       int rsize, csize = size;
>>       u8 *cdata = data;
>> @@ -1231,6 +1232,9 @@ int hid_report_raw_event(struct hid_device *hid, int type, u8 *data, int size,
>>       if (hid->claimed != HID_CLAIMED_HIDRAW) {
>>               for (a = 0; a < report->maxfield; a++)
>>                       hid_input_field(hid, report->field[a], cdata, interrupt);
>> +             hdrv = hid->driver;
>> +             if (hdrv && hdrv->report)
>> +                     hdrv->report(hid, report);
>
> I think this is more useful if called before the individual fields. In
> fact, it seems raw_event() is already doing exactly that. No need for
> a new callback, in other words.

I'm afraid this does not work:
- calling it before the individual fields is not possible unless more
intrusive changes to hid-core:
  * "hid_input_field" does the actual parsing of the input report, so
without the call the .value field is still set to the previous one
(not very useful then)
  * the "hid_input_field" requires to know the previous report, due to
Array type reports which send the current keys down, and not the
release.
- the raw_event() callback contains the report _unparsed_, which means
that it's up to the hid specific driver to split the fields depending
on the report descriptor (for instance, get the right bit for
tipswitch in a byte containing tipswitch, inrange, confidence,
contactId).
- the raw_event() mechanism was the one in place in the v1 of the
patch series, in which the new callback was asked.

Cheers,
Benjamin

>
>>       }
>>
>>       if (hid->claimed & HID_CLAIMED_INPUT)
>> diff --git a/include/linux/hid.h b/include/linux/hid.h
>> index 828726c..e14b465 100644
>> --- a/include/linux/hid.h
>> +++ b/include/linux/hid.h
>> @@ -589,6 +589,7 @@ struct hid_usage_id {
>>   * @raw_event: if report in report_table, this hook is called (NULL means nop)
>>   * @usage_table: on which events to call event (NULL means all)
>>   * @event: if usage in usage_table, this hook is called (NULL means nop)
>> + * @report: this hook is called after parsing a report (NULL means nop)
>>   * @report_fixup: called before report descriptor parsing (NULL means nop)
>>   * @input_mapping: invoked on input registering before mapping an usage
>>   * @input_mapped: invoked on input registering after mapping an usage
>> @@ -627,6 +628,7 @@ struct hid_driver {
>>       const struct hid_usage_id *usage_table;
>>       int (*event)(struct hid_device *hdev, struct hid_field *field,
>>                       struct hid_usage *usage, __s32 value);
>> +     void (*report)(struct hid_device *hdev, struct hid_report *report);
>>
>>       __u8 *(*report_fixup)(struct hid_device *hdev, __u8 *buf,
>>                       unsigned int *size);
>> --
>> 1.8.1
>>
>
> Thanks,
> Henrik

Re: [PATCH v2 1/9] HID: core: add "report" hook, called once the report has been parsed

[Jiri Kosina]

On Sun, 3 Feb 2013, Henrik Rydberg wrote:

> > This callback is called when the parsing of the report has been done
> > by hid-core (so after the calls to .event). The hid drivers can now
> > have access to the whole report by relying on the values stored in
> > the different fields.
> > 
> > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
> > ---
> >  drivers/hid/hid-core.c | 4 ++++
> >  include/linux/hid.h    | 2 ++
> >  2 files changed, 6 insertions(+)
> > 
> > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> > index 5ae2cb1..b671e4e 100644
> > --- a/drivers/hid/hid-core.c
> > +++ b/drivers/hid/hid-core.c
> > @@ -1195,6 +1195,7 @@ int hid_report_raw_event(struct hid_device *hid, int type, u8 *data, int size,
> >  {
> >  	struct hid_report_enum *report_enum = hid->report_enum + type;
> >  	struct hid_report *report;
> > +	struct hid_driver *hdrv;
> >  	unsigned int a;
> >  	int rsize, csize = size;
> >  	u8 *cdata = data;
> > @@ -1231,6 +1232,9 @@ int hid_report_raw_event(struct hid_device *hid, int type, u8 *data, int size,
> >  	if (hid->claimed != HID_CLAIMED_HIDRAW) {
> >  		for (a = 0; a < report->maxfield; a++)
> >  			hid_input_field(hid, report->field[a], cdata, interrupt);
> > +		hdrv = hid->driver;
> > +		if (hdrv && hdrv->report)
> > +			hdrv->report(hid, report);
> 
> I think this is more useful if called before the individual fields. In
> fact, it seems raw_event() is already doing exactly that. No need for
> a new callback, in other words.

raw_event() doesn't provide equivalent functionality though; namely, the 
report is not parsed.

Or have I missed your point?

Thanks for the extensive review, Henrik, it's really helpful.

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH v2 3/9] HID: multitouch: add support for Nexio 42" panel

[Benjamin Tissoires]

Hi Henrik,

On Sun, Feb 3, 2013 at 2:00 PM, Henrik Rydberg <rydberg@euromail.se> wrote:
> Hi Benjamin,
>
>> This device is the worst device I saw. It keeps TipSwitch and InRange
>> at 1 for fingers that are not touching the panel.
>> The solution is to rely on the field ContactCount, which is accurate
>> as the correct information are packed at the begining of the frame.
>>
>> Unfortunately, CountactCount is most of the time at the end of the report.
>> The solution is to pick it when we have the whole report in raw_event.
>>
>> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
>> ---
>>  drivers/hid/hid-ids.h        |  3 +++
>>  drivers/hid/hid-multitouch.c | 36 ++++++++++++++++++++++++++++++------
>>  2 files changed, 33 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
>> index dad56aa..0935012 100644
>> --- a/drivers/hid/hid-ids.h
>> +++ b/drivers/hid/hid-ids.h
>> @@ -597,6 +597,9 @@
>>  #define USB_VENDOR_ID_NEC            0x073e
>>  #define USB_DEVICE_ID_NEC_USB_GAME_PAD       0x0301
>>
>> +#define USB_VENDOR_ID_NEXIO          0x1870
>> +#define USB_DEVICE_ID_NEXIO_MULTITOUCH_420   0x010d
>> +
>>  #define USB_VENDOR_ID_NEXTWINDOW     0x1926
>>  #define USB_DEVICE_ID_NEXTWINDOW_TOUCHSCREEN 0x0003
>>
>> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
>> index 092c09b..c9b8fe5 100644
>> --- a/drivers/hid/hid-multitouch.c
>> +++ b/drivers/hid/hid-multitouch.c
>> @@ -54,6 +54,7 @@ MODULE_LICENSE("GPL");
>>  #define MT_QUIRK_NO_AREA             (1 << 9)
>>  #define MT_QUIRK_IGNORE_DUPLICATES   (1 << 10)
>>  #define MT_QUIRK_HOVERING            (1 << 11)
>> +#define MT_QUIRK_CONTACT_CNT_ACCURATE        (1 << 12)
>>
>>  struct mt_slot {
>>       __s32 x, y, cx, cy, p, w, h;
>> @@ -83,6 +84,7 @@ struct mt_device {
>>       struct mt_class mtclass;        /* our mt device class */
>>       struct mt_fields *fields;       /* temporary placeholder for storing the
>>                                          multitouch fields */
>> +     __s32 *contactcount;            /* contact count value in the report */
>
> Why not an index here?

Just because an index is not sufficient. You need two things: an index
within the field, and the actual field (a pointer to a struct
hid_field). Each .value is local to a field, and even if in most of
the case, the contact count is alone in its field, it would mean to
take the risk that a new device does not follow this logic.

So the actual pointer to the contact count value seemed to be the
shortest way to do it. But it can be easily changed.

>
>>       unsigned last_field_index;      /* last field index of the report */
>>       unsigned last_slot_field;       /* the last field of a slot */
>>       unsigned mt_report_id;  /* the report ID of the multitouch device */
>> @@ -112,6 +114,7 @@ struct mt_device {
>>  #define MT_CLS_DUAL_INRANGE_CONTACTNUMBER    0x0007
>>  #define MT_CLS_DUAL_NSMU_CONTACTID           0x0008
>>  #define MT_CLS_INRANGE_CONTACTNUMBER         0x0009
>> +#define MT_CLS_ALWAYS_TRUE                   0x000a
>>
>>  /* vendor specific classes */
>>  #define MT_CLS_3M                            0x0101
>> @@ -171,6 +174,9 @@ static struct mt_class mt_classes[] = {
>>       { .name = MT_CLS_INRANGE_CONTACTNUMBER,
>>               .quirks = MT_QUIRK_VALID_IS_INRANGE |
>>                       MT_QUIRK_SLOT_IS_CONTACTNUMBER },
>> +     { .name = MT_CLS_ALWAYS_TRUE,
>> +             .quirks = MT_QUIRK_ALWAYS_VALID |
>> +                     MT_QUIRK_CONTACT_CNT_ACCURATE },
>>
>>       /*
>>        * vendor specific classes
>> @@ -251,6 +257,9 @@ static ssize_t mt_set_quirks(struct device *dev,
>>
>>       td->mtclass.quirks = val;
>>
>> +     if (!td->contactcount)
>> +             td->mtclass.quirks &= ~MT_QUIRK_CONTACT_CNT_ACCURATE;
>> +
>
> Why override the overrider here?

This callback is called from the user-space through the sysfs
attribute. So, it is not called in the same time that the
mt_post_parse function. This is just to avoid a user setting this
quirk once the device is up and running leading to a potential oops.

>
>>       return count;
>>  }
>>
>> @@ -461,6 +470,7 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
>>                       td->last_field_index = field->index;
>>                       return 1;
>>               case HID_DG_CONTACTCOUNT:
>> +                     td->contactcount = field->value + usage->usage_index;
>
> An index into the the struct actually passed in mt_report() feels safer.

again, you need to store "field" and "usage->usage_index". Agree, it
would be safer but it will take more space... :)

>
>>                       td->last_field_index = field->index;
>>                       return 1;
>>               case HID_DG_CONTACTMAX:
>> @@ -525,6 +535,10 @@ static int mt_compute_slot(struct mt_device *td, struct input_dev *input)
>>   */
>>  static void mt_complete_slot(struct mt_device *td, struct input_dev *input)
>>  {
>> +     if ((td->mtclass.quirks & MT_QUIRK_CONTACT_CNT_ACCURATE) &&
>> +         td->num_received >= td->num_expected)
>> +             return;
>> +
>>       if (td->curvalid || (td->mtclass.quirks & MT_QUIRK_ALWAYS_VALID)) {
>>               int slotnum = mt_compute_slot(td, input);
>>               struct mt_slot *s = &td->curdata;
>> @@ -635,12 +649,6 @@ static void mt_process_mt_event(struct hid_device *hid, struct hid_field *field,
>>                       td->curdata.h = value;
>>                       break;
>>               case HID_DG_CONTACTCOUNT:
>> -                     /*
>> -                      * Includes multi-packet support where subsequent
>> -                      * packets are sent with zero contactcount.
>> -                      */
>> -                     if (value)
>> -                             td->num_expected = value;
>>                       break;
>>               case HID_DG_TOUCH:
>>                       /* do nothing */
>> @@ -676,6 +684,13 @@ static void mt_report(struct hid_device *hid, struct hid_report *report)
>>       if (!(hid->claimed & HID_CLAIMED_INPUT))
>>               return;
>>
>> +     /*
>> +      * Includes multi-packet support where subsequent
>> +      * packets are sent with zero contactcount.
>> +      */
>> +     if (td->contactcount && *td->contactcount)
>> +             td->num_expected = *td->contactcount;
>> +
>
> Here, that is.
>
>>       for (r = 0; r < report->maxfield; r++) {
>>               field = report->field[r];
>>               count = field->report_count;
>> @@ -750,11 +765,15 @@ static void mt_post_parse_default_settings(struct mt_device *td)
>>  static void mt_post_parse(struct mt_device *td)
>>  {
>>       struct mt_fields *f = td->fields;
>> +     struct mt_class *cls = &td->mtclass;
>>
>>       if (td->touches_by_report > 0) {
>>               int field_count_per_touch = f->length / td->touches_by_report;
>>               td->last_slot_field = f->usages[field_count_per_touch - 1];
>>       }
>> +
>> +     if (!td->contactcount)
>> +             cls->quirks &= ~MT_QUIRK_CONTACT_CNT_ACCURATE;
>
> Since MT_QUIRK_CONTACT_CNT_ACCURATE is a quirk, modifiable by the
> user, it should probably not validate num_expected in the code. Better
> use the contact count index or something equivalent for that.

As when the user changes the quirk, we validate it, this is not required.

Cheers,
Benjamin

>
>>  }
>>
>>  static void mt_input_configured(struct hid_device *hdev, struct hid_input *hi)
>> @@ -1087,6 +1106,11 @@ static const struct hid_device_id mt_devices[] = {
>>               MT_USB_DEVICE(USB_VENDOR_ID_TURBOX,
>>                       USB_DEVICE_ID_TURBOX_TOUCHSCREEN_MOSART) },
>>
>> +     /* Nexio panels */
>> +     { .driver_data = MT_CLS_ALWAYS_TRUE,
>> +             MT_USB_DEVICE(USB_VENDOR_ID_NEXIO,
>> +                     USB_DEVICE_ID_NEXIO_MULTITOUCH_420)},
>> +
>>       /* Panasonic panels */
>>       { .driver_data = MT_CLS_PANASONIC,
>>               MT_USB_DEVICE(USB_VENDOR_ID_PANASONIC,
>> --
>> 1.8.1
>>
>
> Thanks,
> Henrik

Re: [PATCH v2 0/9] Support of Nexio 42" and new default class for hid-multitouch

[Benjamin Tissoires]

Hi Henrik,

On Sun, Feb 3, 2013 at 2:07 PM, Henrik Rydberg <rydberg@euromail.se> wrote:
> Hi Benjamin,
>
>> so, this is the v2 of the support of win7/8 devices.
>
> Looks like it is getting there, thanks.

Thanks for the review. However, before sending a new patch series, I'd
like to have your answers to my comments as I mostly disagree on
everything :)

Thanks again,
Benjamin

>
>> changes since v1:
>> - removed the "optimization" patches, as the benefit was minimum
>> - introduce a new callback "report" in hid-core that drivers can use to treat the
>>   report by having it entirely parsed
>> - rely on this new hook to support Nexio 42"
>
> As noted in the patches comments, using raw_event() seems sufficient.
>
>> side notes:
>> - I've tested removing the heavy call to kzalloc in hid_input_field. The results
>>   are disapointing -> the processing time remains the same.
>> - I've also tested not to rely on .event hook in hid-multitouch but only on .report.
>>   Idem, I thought it would reduce the code of hid-multitouch and will enhance its
>>   processing time, but the results are a roughly same number of lines for hid-multitouch
>>   and the same processing time... :(
>> - these 2 tests helped in cleaning the patch set from the last time.
>>
>> And again, finally, I've pass all the 40 regression tests of my db. \o/
>
> Nice. :-)
>
> Thanks,
> Henrik

Re: [PATCH v2 1/9] HID: core: add "report" hook, called once the report has been parsed

[Benjamin Tissoires]

On Mon, Feb 4, 2013 at 10:34 AM, Jiri Kosina <jkosina@suse.cz> wrote:
> On Sun, 3 Feb 2013, Henrik Rydberg wrote:
>
>> > This callback is called when the parsing of the report has been done
>> > by hid-core (so after the calls to .event). The hid drivers can now
>> > have access to the whole report by relying on the values stored in
>> > the different fields.
>> >
>> > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
>> > ---
>> >  drivers/hid/hid-core.c | 4 ++++
>> >  include/linux/hid.h    | 2 ++
>> >  2 files changed, 6 insertions(+)
>> >
>> > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
>> > index 5ae2cb1..b671e4e 100644
>> > --- a/drivers/hid/hid-core.c
>> > +++ b/drivers/hid/hid-core.c
>> > @@ -1195,6 +1195,7 @@ int hid_report_raw_event(struct hid_device *hid, int type, u8 *data, int size,
>> >  {
>> >     struct hid_report_enum *report_enum = hid->report_enum + type;
>> >     struct hid_report *report;
>> > +   struct hid_driver *hdrv;
>> >     unsigned int a;
>> >     int rsize, csize = size;
>> >     u8 *cdata = data;
>> > @@ -1231,6 +1232,9 @@ int hid_report_raw_event(struct hid_device *hid, int type, u8 *data, int size,
>> >     if (hid->claimed != HID_CLAIMED_HIDRAW) {
>> >             for (a = 0; a < report->maxfield; a++)
>> >                     hid_input_field(hid, report->field[a], cdata, interrupt);
>> > +           hdrv = hid->driver;
>> > +           if (hdrv && hdrv->report)
>> > +                   hdrv->report(hid, report);
>>
>> I think this is more useful if called before the individual fields. In
>> fact, it seems raw_event() is already doing exactly that. No need for
>> a new callback, in other words.
>
> raw_event() doesn't provide equivalent functionality though; namely, the
> report is not parsed.
>
> Or have I missed your point?

No, you perfectly understood the purpose of the patch. raw_event() and
report() are not the same kind of callbacks at all.

>
> Thanks for the extensive review, Henrik, it's really helpful.

Yep, thanks Henrik, and thanks Jiri for having a look at it.

Cheers,
Benjamin

Re: [PATCH v2 1/9] HID: core: add "report" hook, called once the report has been parsed

[Henrik Rydberg]

Hi Benjamin,

> > I think this is more useful if called before the individual fields. In
> > fact, it seems raw_event() is already doing exactly that. No need for
> > a new callback, in other words.
> 
> I'm afraid this does not work:
> - calling it before the individual fields is not possible unless more
> intrusive changes to hid-core:
>   * "hid_input_field" does the actual parsing of the input report, so
> without the call the .value field is still set to the previous one
> (not very useful then)
>   * the "hid_input_field" requires to know the previous report, due to
> Array type reports which send the current keys down, and not the
> release.
> - the raw_event() callback contains the report _unparsed_, which means
> that it's up to the hid specific driver to split the fields depending
> on the report descriptor (for instance, get the right bit for
> tipswitch in a byte containing tipswitch, inrange, confidence,
> contactId).
> - the raw_event() mechanism was the one in place in the v1 of the
> patch series, in which the new callback was asked.

Thanks for the compelling argument list, v1 had obviously been thrown
out of the cache already. ;-) I agree with your approach.

Thanks,
Henrik

Re: [PATCH v2 3/9] HID: multitouch: add support for Nexio 42" panel

[Henrik Rydberg]

Hi Benjamin,

> > Why not an index here?
> 
> Just because an index is not sufficient. You need two things: an index
> within the field, and the actual field (a pointer to a struct
> hid_field). Each .value is local to a field, and even if in most of
> the case, the contact count is alone in its field, it would mean to
> take the risk that a new device does not follow this logic.

The field value is passed to process_mt_event() in a fairly
straight-forward fashion, I was imagining that behavior could be
copied somehow.

> So the actual pointer to the contact count value seemed to be the
> shortest way to do it. But it can be easily changed.

Keeping a pointer into the core structure creates unwanted
dependencies to the scope of that value, making an eventual core
refactoring even harder, not to mention trickier to debug. So even
though it looks neat in the code, it pushes the problem forward.

> >> @@ -251,6 +257,9 @@ static ssize_t mt_set_quirks(struct device *dev,
> >>
> >>       td->mtclass.quirks = val;
> >>
> >> +     if (!td->contactcount)
> >> +             td->mtclass.quirks &= ~MT_QUIRK_CONTACT_CNT_ACCURATE;
> >> +
> >
> > Why override the overrider here?
> 
> This callback is called from the user-space through the sysfs
> attribute. So, it is not called in the same time that the
> mt_post_parse function. This is just to avoid a user setting this
> quirk once the device is up and running leading to a potential oops.

Yes, but the quirk _is_ user modifiable. Hence, the problem lies in
equating the user-modifiable quirk with the branch control of the
program.

> > An index into the the struct actually passed in mt_report() feels safer.
> 
> again, you need to store "field" and "usage->usage_index". Agree, it
> would be safer but it will take more space... :)

If you think the code change is not only correct, but also moves the
whole code base in a good direction, by all means.

> >> @@ -750,11 +765,15 @@ static void mt_post_parse_default_settings(struct mt_device *td)
> >>  static void mt_post_parse(struct mt_device *td)
> >>  {
> >>       struct mt_fields *f = td->fields;
> >> +     struct mt_class *cls = &td->mtclass;
> >>
> >>       if (td->touches_by_report > 0) {
> >>               int field_count_per_touch = f->length / td->touches_by_report;
> >>               td->last_slot_field = f->usages[field_count_per_touch - 1];
> >>       }
> >> +
> >> +     if (!td->contactcount)
> >> +             cls->quirks &= ~MT_QUIRK_CONTACT_CNT_ACCURATE;
> >
> > Since MT_QUIRK_CONTACT_CNT_ACCURATE is a quirk, modifiable by the
> > user, it should probably not validate num_expected in the code. Better
> > use the contact count index or something equivalent for that.
> 
> As when the user changes the quirk, we validate it, this is not required.

True, barring the comments above.

Thanks,
Henrik

Re: [PATCH v2 0/9] Support of Nexio 42" and new default class for hid-multitouch

[Henrik Rydberg]

Hi Benjamin,

> Thanks for the review. However, before sending a new patch series, I'd
> like to have your answers to my comments as I mostly disagree on
> everything :)

With good reason, apparently. :-) I see no major problem with your
patches, although the discussed details show that there is room for
some refactoring.

Thanks,
Henrik

Re: [PATCH v2 3/9] HID: multitouch: add support for Nexio 42" panel

[Benjamin Tissoires]

On Mon, Feb 4, 2013 at 12:42 PM, Henrik Rydberg <rydberg@euromail.se> wrote:
> Hi Benjamin,
>
>> > Why not an index here?
>>
>> Just because an index is not sufficient. You need two things: an index
>> within the field, and the actual field (a pointer to a struct
>> hid_field). Each .value is local to a field, and even if in most of
>> the case, the contact count is alone in its field, it would mean to
>> take the risk that a new device does not follow this logic.
>
> The field value is passed to process_mt_event() in a fairly
> straight-forward fashion, I was imagining that behavior could be
> copied somehow.
>
>> So the actual pointer to the contact count value seemed to be the
>> shortest way to do it. But it can be easily changed.
>
> Keeping a pointer into the core structure creates unwanted
> dependencies to the scope of that value, making an eventual core
> refactoring even harder, not to mention trickier to debug. So even
> though it looks neat in the code, it pushes the problem forward.
>
>> >> @@ -251,6 +257,9 @@ static ssize_t mt_set_quirks(struct device *dev,
>> >>
>> >>       td->mtclass.quirks = val;
>> >>
>> >> +     if (!td->contactcount)
>> >> +             td->mtclass.quirks &= ~MT_QUIRK_CONTACT_CNT_ACCURATE;
>> >> +
>> >
>> > Why override the overrider here?
>>
>> This callback is called from the user-space through the sysfs
>> attribute. So, it is not called in the same time that the
>> mt_post_parse function. This is just to avoid a user setting this
>> quirk once the device is up and running leading to a potential oops.
>
> Yes, but the quirk _is_ user modifiable. Hence, the problem lies in
> equating the user-modifiable quirk with the branch control of the
> program.

I'm not sure I understood what you meant there.
The quirk is indeed user modifiable, but through the callback
mt_set_quirks() only. If the HID field Contact Count is not present,
this quirk should not be allowed to be present, thus the two removals
of the quirk in met_set_quirks() and mt_post_parse(). As there are no
other entry points, I'm quite confuse to understand where the pitfall
is.

>
>> > An index into the the struct actually passed in mt_report() feels safer.
>>
>> again, you need to store "field" and "usage->usage_index". Agree, it
>> would be safer but it will take more space... :)
>
> If you think the code change is not only correct, but also moves the
> whole code base in a good direction, by all means.

Ok, will do. After a deeper look at it, I can even have two int
indexes (and no pointers): one for the field and one other for the
value within the field.

Cheers,
Benjamin

>
>> >> @@ -750,11 +765,15 @@ static void mt_post_parse_default_settings(struct mt_device *td)
>> >>  static void mt_post_parse(struct mt_device *td)
>> >>  {
>> >>       struct mt_fields *f = td->fields;
>> >> +     struct mt_class *cls = &td->mtclass;
>> >>
>> >>       if (td->touches_by_report > 0) {
>> >>               int field_count_per_touch = f->length / td->touches_by_report;
>> >>               td->last_slot_field = f->usages[field_count_per_touch - 1];
>> >>       }
>> >> +
>> >> +     if (!td->contactcount)
>> >> +             cls->quirks &= ~MT_QUIRK_CONTACT_CNT_ACCURATE;
>> >
>> > Since MT_QUIRK_CONTACT_CNT_ACCURATE is a quirk, modifiable by the
>> > user, it should probably not validate num_expected in the code. Better
>> > use the contact count index or something equivalent for that.
>>
>> As when the user changes the quirk, we validate it, this is not required.
>
> True, barring the comments above.
>
> Thanks,
> Henrik

Re: [PATCH v2 0/9] Support of Nexio 42" and new default class for hid-multitouch

[Jiri Kosina]

On Mon, 4 Feb 2013, Henrik Rydberg wrote:

> > Thanks for the review. However, before sending a new patch series, I'd
> > like to have your answers to my comments as I mostly disagree on
> > everything :)
> 
> With good reason, apparently. :-) I see no major problem with your
> patches, although the discussed details show that there is room for
> some refactoring.

Thanks to both of you. As I don't object to the HID core change, I have 
now applied the patchset, so please send any further additions on top of 
for-3.9/multitouch branch of my tree.

Also, Benjamin, perhaps it'd make sense to put a link somewhere into 
in-tree documentation, with pointer to your testing suite?

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH v2 0/9] Support of Nexio 42" and new default class for hid-multitouch

[Benjamin Tissoires]

On Tue, Feb 5, 2013 at 12:13 PM, Jiri Kosina <jkosina@suse.cz> wrote:
> On Mon, 4 Feb 2013, Henrik Rydberg wrote:
>
>> > Thanks for the review. However, before sending a new patch series, I'd
>> > like to have your answers to my comments as I mostly disagree on
>> > everything :)
>>
>> With good reason, apparently. :-) I see no major problem with your
>> patches, although the discussed details show that there is room for
>> some refactoring.
>
> Thanks to both of you. As I don't object to the HID core change, I have
> now applied the patchset, so please send any further additions on top of
> for-3.9/multitouch branch of my tree.

Ok. Thanks Jiri. I will send the patch in a minute.

>
> Also, Benjamin, perhaps it'd make sense to put a link somewhere into
> in-tree documentation, with pointer to your testing suite?

Good idea. However, I prefer removing the dependencies between hid
drivers and usbhid before including such reference to avoid testers
getting kernel oops while trying to access the usb layer from the uhid
device... So, yesterday and this morning, I rebased/updated Henrik's
patches on this topic. They should be ready soon.

Do you mind if I send the usbhid dependency and the pen+multitouch
series this week, or we are too close to the 3.9 opening window?

Cheers,
Benjamin

Re: [PATCH v2 0/9] Support of Nexio 42" and new default class for hid-multitouch

[Jiri Kosina]

On Wed, 6 Feb 2013, Benjamin Tissoires wrote:

> > Also, Benjamin, perhaps it'd make sense to put a link somewhere into
> > in-tree documentation, with pointer to your testing suite?
> 
> Good idea. However, I prefer removing the dependencies between hid
> drivers and usbhid before including such reference to avoid testers
> getting kernel oops while trying to access the usb layer from the uhid
> device... So, yesterday and this morning, I rebased/updated Henrik's
> patches on this topic. They should be ready soon.

Makes sense, thanks.

> Do you mind if I send the usbhid dependency and the pen+multitouch 
> series this week, or we are too close to the 3.9 opening window?

Please just send the patches, and let's see whether they will make it for 
3.8 or I'll queue (some of) them for 3.9.

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH v2 0/9] Support of Nexio 42" and new default class for hid-multitouch

[Benjamin Tissoires]

>> Do you mind if I send the usbhid dependency and the pen+multitouch
>> series this week, or we are too close to the 3.9 opening window?
>
> Please just send the patches, and let's see whether they will make it for
> 3.8 or I'll queue (some of) them for 3.9.
>

Well, I didn't wanted to push them into 3.8 (the rc6 is already there,
and I doubt Linus would be happy to see a lot of patches now). I was
asking if it was not too late to schedule them for 3.9, or if you'd
rather wait for 3.10 :)

Anyway, I'm doing my best to push them soon.

Cheers,
Benjamin

Re: [PATCH v2 0/9] Support of Nexio 42" and new default class for hid-multitouch

[Jiri Kosina]

On Wed, 6 Feb 2013, Benjamin Tissoires wrote:

> >> Do you mind if I send the usbhid dependency and the pen+multitouch
> >> series this week, or we are too close to the 3.9 opening window?
> >
> > Please just send the patches, and let's see whether they will make it for
> > 3.8 or I'll queue (some of) them for 3.9.
> >
> 
> Well, I didn't wanted to push them into 3.8 (the rc6 is already there,
> and I doubt Linus would be happy to see a lot of patches now). I was
> asking if it was not too late to schedule them for 3.9, or if you'd
> rather wait for 3.10 :)

Yeah, sorry, my sentence was off-by-one, but I guess you got me :)

> Anyway, I'm doing my best to push them soon.

Thanks.

-- 
Jiri Kosina
SUSE Labs