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

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

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 <redacted>
---
 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

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 <redacted>
---
 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

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 <redacted>
---
 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

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 <redacted>

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

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

Signed-off-by: Benjamin Tissoires <redacted>
---
 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

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 <redacted>
---
 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

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 <redacted>
---
 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

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 <redacted>
---
 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

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 <redacted>
---
 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

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.

Thanks,
Henrik

Hi Benjamin,

Why not an index here?

Why override the overrider here?

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

Here, that is.

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.

Thanks,
Henrik

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

Hi Henrik,

On Sun, Feb 3, 2013 at 1:27 PM, Henrik Rydberg [off-list ref] wrote:

Hi Benjamin,

quoted

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 <redacted>
---
 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

quoted

      }

      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

On Sun, 3 Feb 2013, Henrik Rydberg wrote:

quoted

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 <redacted>
---
 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

Hi Henrik,

On Sun, Feb 3, 2013 at 2:00 PM, Henrik Rydberg [off-list ref] wrote:

Hi Benjamin,

quoted

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 <redacted>
---
 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.

quoted

      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.

quoted

      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... :)

quoted

                      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.

quoted

      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

quoted

 }

 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

Hi Henrik,

On Sun, Feb 3, 2013 at 2:07 PM, Henrik Rydberg [off-list ref] wrote:

Hi Benjamin,

quoted

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

quoted

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.

quoted

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

On Mon, Feb 4, 2013 at 10:34 AM, Jiri Kosina [off-list ref] wrote:

On Sun, 3 Feb 2013, Henrik Rydberg wrote:

quoted

quoted

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 <redacted>
---
 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

Hi Benjamin,

quoted

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

Hi Benjamin,

quoted

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.

quoted

quoted

@@ -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.

quoted

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.

quoted

quoted

@@ -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

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

On Mon, Feb 4, 2013 at 12:42 PM, Henrik Rydberg [off-list ref] wrote:

Hi Benjamin,

quoted

quoted

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.

quoted

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.

quoted

quoted

quoted

@@ -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.

quoted

quoted

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

quoted

quoted

quoted

@@ -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

On Mon, 4 Feb 2013, Henrik Rydberg wrote:

quoted

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

On Tue, Feb 5, 2013 at 12:13 PM, Jiri Kosina [off-list ref] wrote:

On Mon, 4 Feb 2013, Henrik Rydberg wrote:

quoted

quoted

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

On Wed, 6 Feb 2013, Benjamin Tissoires wrote:

quoted

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

quoted

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

On Wed, 6 Feb 2013, Benjamin Tissoires wrote:

quoted

quoted

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