[PATCH 00/11] Support of dual pen/multitouch and new default for win 7 certified devices

[PATCH 00/11] Support of dual pen/multitouch and new default for win 7 certified devices

[Benjamin Tissoires]

Hi Guys,

Last week, I received two new interesting devices report:
- N-trig win 8 certified pen/touch panel
- Samsung Nexio 42"


N-trig device
-------------
The first one is the origin of patches 1 to 6.
The multiouch part worked flawlessly with the win 8 patches I sent before,
but the pen part was completely ignored.
I could have used the quirk MULTI_INPUT, but by testing this quirk against
several devices report I have (https://github.com/bentiss/hid-devices), it
was a pain because some of them create 4 or 5 useless inputs.

I choose to allow the hid driver to control the creation of input devices, thus
patches 1 to 3.


Nexio device
------------
The second one was more problematic. Indeed, it was not working at all with the
current release of hid-multitouch. I had several ghost points, and any of the
available quirks worked.
I finaly found the trick, and this trick applies to all the win7 and win8
devices I saw so far (same url as before).

So I think I finally understood why the windows driver was better than us: it
first looks at the announced contact count, and treat only the right number. It
was so simple... and it works so well...

However, for us, I need to get this information from the raw_event because most
of the devices put the contact count field at the end of the report.

I also decided to change the default class as it is much more tolerant than the
previous one. I could have changed all the devices, but in the end, I changed
only those that get a benefit and that I could test.


Debug tool
----------
I was able to discover this trick only recently because I made a small C program
that allows me to replay the hid events through hid-multitouch. The code is
here: https://github.com/bentiss/hid-replay and you will need a kernel 3.6
to make it work (it requires uhid).

However, be careful, this program can be the root of many kernel oopses if the
targeted hid module tries to directly handle the usb or with any of the usbhid
function.
So, Henrik, I really need you to push your abstraction of usbhid in all hid
modules :)

Anyway, this tool can be very helpful to debug hid devices, that's why I share
it there... and also because I work for an open-source company :)

Happy reviewing.

Cheers,
Benjamin

Benjamin Tissoires (11):
  HID: hid-input factorize hid_input allocation
  HID: hid-input: simplify hid_input allocation and registration
  HID: hid-input: export hidinput_allocation function
  HID: hid-multitouch: creates and handle stylus report with dual-sensors
  HID: hid-multitouch: manually send sync event for pen input report
  HID: hid-multitouch: append " Pen" to the name of the stylus input
  HID: hid-multitouch: rename MT_CLS_DEFAULT into MT_CLS_NSMU
  HID: hid-multitouch: add support for Nexio 42" panel
  HID: hid-multitouch: check if ContactCount is given for default quirk
  HID: hid-multitouch: fix protocol for 3 devices
  HID: hid-multitouch: use MT_QUIRK_CONTACT_COUNT_ACCURATE for win 8 devices

 drivers/hid/hid-ids.h        |   3 +
 drivers/hid/hid-input.c      | 100 +++++++++++++---------
 drivers/hid/hid-multitouch.c | 198 +++++++++++++++++++++++++++++++++++--------
 include/linux/hid.h          |   1 +
 4 files changed, 229 insertions(+), 73 deletions(-)

-- 
1.8.0

[PATCH 01/11] HID: hid-input factorize hid_input allocation

[Benjamin Tissoires]

This just refactors the allocation of hid_input.
No semantic changes.

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

diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index 7015080..47f98a3 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -1163,6 +1163,38 @@ static void report_features(struct hid_device *hid)
 			}
 }
 
+static struct hid_input *hidinput_allocate(struct hid_device *hid)
+{
+	struct hid_input *hidinput = kzalloc(sizeof(*hidinput), GFP_KERNEL);
+	struct input_dev *input_dev = input_allocate_device();
+	if (!hidinput || !input_dev) {
+		kfree(hidinput);
+		input_free_device(input_dev);
+		hid_err(hid, "Out of memory during hid input probe\n");
+		return NULL;
+	}
+
+	input_set_drvdata(input_dev, hid);
+	input_dev->event = hid->ll_driver->hidinput_input_event;
+	input_dev->open = hidinput_open;
+	input_dev->close = hidinput_close;
+	input_dev->setkeycode = hidinput_setkeycode;
+	input_dev->getkeycode = hidinput_getkeycode;
+
+	input_dev->name = hid->name;
+	input_dev->phys = hid->phys;
+	input_dev->uniq = hid->uniq;
+	input_dev->id.bustype = hid->bus;
+	input_dev->id.vendor  = hid->vendor;
+	input_dev->id.product = hid->product;
+	input_dev->id.version = hid->version;
+	input_dev->dev.parent = hid->dev.parent;
+	hidinput->input = input_dev;
+	list_add_tail(&hidinput->list, &hid->inputs);
+
+	return hidinput;
+}
+
 /*
  * Register the input device; print a message.
  * Configure the input layer interface
@@ -1174,7 +1206,6 @@ int hidinput_connect(struct hid_device *hid, unsigned int force)
 	struct hid_driver *drv = hid->driver;
 	struct hid_report *report;
 	struct hid_input *hidinput = NULL;
-	struct input_dev *input_dev;
 	int i, j, k;
 
 	INIT_LIST_HEAD(&hid->inputs);
@@ -1205,33 +1236,9 @@ int hidinput_connect(struct hid_device *hid, unsigned int force)
 				continue;
 
 			if (!hidinput) {
-				hidinput = kzalloc(sizeof(*hidinput), GFP_KERNEL);
-				input_dev = input_allocate_device();
-				if (!hidinput || !input_dev) {
-					kfree(hidinput);
-					input_free_device(input_dev);
-					hid_err(hid, "Out of memory during hid input probe\n");
+				hidinput = hidinput_allocate(hid);
+				if (!hidinput)
 					goto out_unwind;
-				}
-
-				input_set_drvdata(input_dev, hid);
-				input_dev->event =
-					hid->ll_driver->hidinput_input_event;
-				input_dev->open = hidinput_open;
-				input_dev->close = hidinput_close;
-				input_dev->setkeycode = hidinput_setkeycode;
-				input_dev->getkeycode = hidinput_getkeycode;
-
-				input_dev->name = hid->name;
-				input_dev->phys = hid->phys;
-				input_dev->uniq = hid->uniq;
-				input_dev->id.bustype = hid->bus;
-				input_dev->id.vendor  = hid->vendor;
-				input_dev->id.product = hid->product;
-				input_dev->id.version = hid->version;
-				input_dev->dev.parent = hid->dev.parent;
-				hidinput->input = input_dev;
-				list_add_tail(&hidinput->list, &hid->inputs);
 			}
 
 			for (i = 0; i < report->maxfield; i++)
-- 
1.8.0

[PATCH 02/11] HID: hid-input: simplify hid_input allocation and registration

[Benjamin Tissoires]

In order to provide fine control for the creation of different
input devices in probe function of third party drivers, this patch
split the allocations, the registrations and the free of input
devices.

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

diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index 47f98a3..eea02b0 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -1206,6 +1206,7 @@ int hidinput_connect(struct hid_device *hid, unsigned int force)
 	struct hid_driver *drv = hid->driver;
 	struct hid_report *report;
 	struct hid_input *hidinput = NULL;
+	struct hid_input *next;
 	int i, j, k;
 
 	INIT_LIST_HEAD(&hid->inputs);
@@ -1238,7 +1239,7 @@ int hidinput_connect(struct hid_device *hid, unsigned int force)
 			if (!hidinput) {
 				hidinput = hidinput_allocate(hid);
 				if (!hidinput)
-					goto out_unwind;
+					goto out_cleanup;
 			}
 
 			for (i = 0; i < report->maxfield; i++)
@@ -1253,29 +1254,36 @@ int hidinput_connect(struct hid_device *hid, unsigned int force)
 				 * UGCI) cram a lot of unrelated inputs into the
 				 * same interface. */
 				hidinput->report = report;
-				if (drv->input_configured)
-					drv->input_configured(hid, hidinput);
-				if (input_register_device(hidinput->input))
-					goto out_cleanup;
 				hidinput = NULL;
 			}
 		}
 	}
 
-	if (hidinput) {
+	list_for_each_entry(hidinput, &hid->inputs, list) {
 		if (drv->input_configured)
 			drv->input_configured(hid, hidinput);
 		if (input_register_device(hidinput->input))
-			goto out_cleanup;
+			goto out_unwind;
 	}
 
 	return 0;
 
 out_cleanup:
-	list_del(&hidinput->list);
-	input_free_device(hidinput->input);
-	kfree(hidinput);
+	list_for_each_entry_safe(hidinput, next, &hid->inputs, list) {
+		list_del(&hidinput->list);
+		input_free_device(hidinput->input);
+		kfree(hidinput);
+	}
+	return -1;
+
 out_unwind:
+	/* free the non-registered hidinput, starting from the faulty one */
+	list_for_each_entry_safe_from(hidinput, next, &hid->inputs, list) {
+		list_del(&hidinput->list);
+		input_free_device(hidinput->input);
+		kfree(hidinput);
+	}
+
 	/* unwind the ones we already registered */
 	hidinput_disconnect(hid);
 
-- 
1.8.0

[PATCH 03/11] HID: hid-input: export hidinput_allocation function

[Benjamin Tissoires]

During the probe, third party drivers can now safely create a new
input devices depending on the parsing of the reports descriptor.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
---
 drivers/hid/hid-input.c | 14 +++++++++++---
 include/linux/hid.h     |  1 +
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index eea02b0..b0572d0 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -1163,7 +1163,7 @@ static void report_features(struct hid_device *hid)
 			}
 }
 
-static struct hid_input *hidinput_allocate(struct hid_device *hid)
+struct hid_input *hidinput_allocate(struct hid_device *hid)
 {
 	struct hid_input *hidinput = kzalloc(sizeof(*hidinput), GFP_KERNEL);
 	struct input_dev *input_dev = input_allocate_device();
@@ -1190,10 +1190,16 @@ static struct hid_input *hidinput_allocate(struct hid_device *hid)
 	input_dev->id.version = hid->version;
 	input_dev->dev.parent = hid->dev.parent;
 	hidinput->input = input_dev;
-	list_add_tail(&hidinput->list, &hid->inputs);
+	list_add(&hidinput->list, &hid->inputs);
 
 	return hidinput;
 }
+EXPORT_SYMBOL_GPL(hidinput_allocate);
+
+static struct hid_input *hid_get_latest_hidinput(struct hid_device *hid)
+{
+	return list_first_entry(&hid->inputs, struct hid_input, list);
+}
 
 /*
  * Register the input device; print a message.
@@ -1243,9 +1249,11 @@ int hidinput_connect(struct hid_device *hid, unsigned int force)
 			}
 
 			for (i = 0; i < report->maxfield; i++)
-				for (j = 0; j < report->field[i]->maxusage; j++)
+				for (j = 0; j < report->field[i]->maxusage; j++) {
+					hidinput = hid_get_latest_hidinput(hid);
 					hidinput_configure_usage(hidinput, report->field[i],
 								 report->field[i]->usage + j);
+				}
 
 			if (hid->quirks & HID_QUIRK_MULTI_INPUT) {
 				/* This will leave hidinput NULL, so that it
diff --git a/include/linux/hid.h b/include/linux/hid.h
index d2c42dd..42b02d6 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -701,6 +701,7 @@ extern void hidinput_hid_event(struct hid_device *, struct hid_field *, struct h
 extern void hidinput_report_event(struct hid_device *hid, struct hid_report *report);
 extern int hidinput_connect(struct hid_device *hid, unsigned int force);
 extern void hidinput_disconnect(struct hid_device *);
+extern struct hid_input *hidinput_allocate(struct hid_device *hid);
 
 int hid_set_field(struct hid_field *, unsigned, __s32);
 int hid_input_report(struct hid_device *, int type, u8 *, int, int);
-- 
1.8.0

[PATCH 04/11] HID: hid-multitouch: creates and handle stylus report with dual-sensors

[Benjamin Tissoires]

Now that drivers can create new inputs, let's use this to split
the reports coming from the pen and from the touch.

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

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 61543c0..c894306 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -98,6 +98,8 @@ struct mt_device {
 	bool serial_maybe;	/* need to check for serial protocol */
 	bool curvalid;		/* is the current contact valid? */
 	unsigned mt_flags;	/* flags to pass to input-mt */
+	unsigned application;	/* the current HID application (pen or touch) */
+	unsigned physical;	/* the current HID physical (pen or touch) */
 };
 
 /* classes of device behavior */
@@ -342,7 +344,8 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 	/* Only map fields from TouchScreen or TouchPad collections.
 	* We need to ignore fields that belong to other collections
 	* such as Mouse that might have the same GenericDesktop usages. */
-	if (field->application == HID_DG_TOUCHSCREEN)
+	if (field->application == HID_DG_TOUCHSCREEN ||
+	    field->application == HID_DG_PEN)
 		td->mt_flags |= INPUT_MT_DIRECT;
 	else if (field->application != HID_DG_TOUCHPAD)
 		return 0;
@@ -354,11 +357,22 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 	    (usage->hid & HID_USAGE_PAGE) == HID_UP_BUTTON)
 		td->mt_flags |= INPUT_MT_POINTER;
 
-	/* eGalax devices provide a Digitizer.Stylus input which overrides
-	 * the correct Digitizers.Finger X/Y ranges.
-	 * Let's just ignore this input. */
+	/*
+	 * eGalax and newer N-Trig devices provide a Digitizer.Stylus input.
+	 * Create a new input device for this collection and let hid-input
+	 * handling it.
+	 */
+	if (td->application != field->application ||
+	    td->physical != field->physical) {
+		if (td->application)
+			/* a hidinput is already here, allocate a new one */
+			hi = hidinput_allocate(hdev);
+		td->application = field->application;
+		td->physical = field->physical;
+	}
+
 	if (field->physical == HID_DG_STYLUS)
-		return -1;
+		return 0;
 
 	if (usage->usage_index)
 		prev_usage = &field->usage[usage->usage_index - 1];
@@ -492,6 +506,9 @@ static int mt_input_mapped(struct hid_device *hdev, struct hid_input *hi,
 		struct hid_field *field, struct hid_usage *usage,
 		unsigned long **bit, int *max)
 {
+	if (field->physical == HID_DG_STYLUS)
+		return 0;
+
 	if (usage->type == EV_KEY || usage->type == EV_ABS)
 		set_bit(usage->type, hi->input->evbit);
 
@@ -581,6 +598,9 @@ static int mt_event(struct hid_device *hid, struct hid_field *field,
 	struct mt_device *td = hid_get_drvdata(hid);
 	__s32 quirks = td->mtclass.quirks;
 
+	if (field->physical == HID_DG_STYLUS)
+		return 0;
+
 	if (hid->claimed & HID_CLAIMED_INPUT) {
 		switch (usage->hid) {
 		case HID_DG_INRANGE:
-- 
1.8.0

[PATCH 05/11] HID: hid-multitouch: manually send sync event for pen input report

[Benjamin Tissoires]

Since hid-multitouch sets the quirk HID_QUIRK_NO_INPUT_SYNC, we need
to manually send the events and then make the SYN_EV.

This patch needs to export hidinput_hid_event

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
---
 drivers/hid/hid-input.c      |  1 +
 drivers/hid/hid-multitouch.c | 17 ++++++++++++++---
 2 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index b0572d0..66ddbb6 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -1057,6 +1057,7 @@ void hidinput_hid_event(struct hid_device *hid, struct hid_field *field, struct
 	if ((field->flags & HID_MAIN_ITEM_RELATIVE) && (usage->type == EV_KEY))
 		input_event(input, usage->type, usage->code, 0);
 }
+EXPORT_SYMBOL_GPL(hidinput_hid_event);
 
 void hidinput_report_event(struct hid_device *hid, struct hid_report *report)
 {
diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index c894306..0a4c062 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -84,6 +84,7 @@ struct mt_device {
 	struct mt_fields *fields;	/* temporary placeholder for storing the
 					   multitouch fields */
 	unsigned last_field_index;	/* last field index of the report */
+	unsigned last_pen_field_index;	/* last field index of the pen report */
 	unsigned last_slot_field;	/* the last field of a slot */
 	__s8 inputmode;		/* InputMode HID feature, -1 if non-existent */
 	__s8 inputmode_index;	/* InputMode HID feature index in the report */
@@ -371,8 +372,10 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 		td->physical = field->physical;
 	}
 
-	if (field->physical == HID_DG_STYLUS)
+	if (field->physical == HID_DG_STYLUS) {
+		td->last_pen_field_index = field->index;
 		return 0;
+	}
 
 	if (usage->usage_index)
 		prev_usage = &field->usage[usage->usage_index - 1];
@@ -598,8 +601,16 @@ static int mt_event(struct hid_device *hid, struct hid_field *field,
 	struct mt_device *td = hid_get_drvdata(hid);
 	__s32 quirks = td->mtclass.quirks;
 
-	if (field->physical == HID_DG_STYLUS)
-		return 0;
+	if (field->physical == HID_DG_STYLUS) {
+		if (hid->claimed & HID_CLAIMED_INPUT)
+			hidinput_hid_event(hid, field, usage, value);
+		if (hid->claimed & HID_CLAIMED_HIDDEV && hid->hiddev_hid_event)
+			hid->hiddev_hid_event(hid, field, usage, value);
+		if (usage->usage_index + 1 == field->report_count &&
+		    field->index == td->last_pen_field_index)
+			input_sync(field->hidinput->input);
+		return 1;
+	}
 
 	if (hid->claimed & HID_CLAIMED_INPUT) {
 		switch (usage->hid) {
-- 
1.8.0

[PATCH 06/11] HID: hid-multitouch: append " Pen" to the name of the stylus input

[Benjamin Tissoires]

This is not just cosmetics, it can help to write udev and X.org
rules.

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

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 0a4c062..f10105f 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -32,6 +32,7 @@
 #include <linux/slab.h>
 #include <linux/usb.h>
 #include <linux/input/mt.h>
+#include <linux/string.h>
 #include "usbhid/usbhid.h"
 
 
@@ -341,6 +342,7 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 	struct mt_class *cls = &td->mtclass;
 	int code;
 	struct hid_usage *prev_usage = NULL;
+	char *name;
 
 	/* Only map fields from TouchScreen or TouchPad collections.
 	* We need to ignore fields that belong to other collections
@@ -370,6 +372,13 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 			hi = hidinput_allocate(hdev);
 		td->application = field->application;
 		td->physical = field->physical;
+		if (field->physical == HID_DG_STYLUS) {
+			name = kzalloc(sizeof(hdev->name) + 4, GFP_KERNEL);
+			if (name) {
+				sprintf(name, "%s Pen", hdev->name);
+				hi->input->name = name;
+			}
+		}
 	}
 
 	if (field->physical == HID_DG_STYLUS) {
@@ -791,6 +800,7 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	int ret, i;
 	struct mt_device *td;
 	struct mt_class *mtclass = mt_classes; /* MT_CLS_DEFAULT */
+	struct hid_input *hi;
 
 	for (i = 0; mt_classes[i].name ; i++) {
 		if (id->driver_data == mt_classes[i].name) {
@@ -830,7 +840,7 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
 
 	ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
 	if (ret)
-		goto fail;
+		goto hid_fail;
 
 	ret = sysfs_create_group(&hdev->dev.kobj, &mt_attribute_group);
 
@@ -842,6 +852,10 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
 
 	return 0;
 
+hid_fail:
+	list_for_each_entry(hi, &hdev->inputs, list)
+		if (hi->input->name != hdev->name)
+			kfree(hi->input->name);
 fail:
 	kfree(td->fields);
 	kfree(td);
@@ -886,8 +900,15 @@ static int mt_resume(struct hid_device *hdev)
 static void mt_remove(struct hid_device *hdev)
 {
 	struct mt_device *td = hid_get_drvdata(hdev);
+	struct hid_input *hi;
+
 	sysfs_remove_group(&hdev->dev.kobj, &mt_attribute_group);
 	hid_hw_stop(hdev);
+
+	list_for_each_entry(hi, &hdev->inputs, list)
+		if (hi->input->name != hdev->name)
+			kfree(hi->input->name);
+
 	kfree(td);
 	hid_set_drvdata(hdev, NULL);
 }
-- 
1.8.0

[PATCH 07/11] HID: hid-multitouch: rename MT_CLS_DEFAULT into MT_CLS_NSMU

[Benjamin Tissoires]

While trying to add support for Nexio 1870:0100, I found that the
trick required to make it work is much more reliable than the previous
default class.
Rename the current default class for backward compatibility.

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

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index f10105f..50fb79f 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -115,6 +115,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_NSMU				0x000a
 
 /* vendor specific classes */
 #define MT_CLS_3M				0x0101
@@ -147,7 +148,8 @@ static int cypress_compute_slot(struct mt_device *td)
 }
 
 static struct mt_class mt_classes[] = {
-	{ .name = MT_CLS_DEFAULT,
+	{ .name = MT_CLS_DEFAULT},
+	{ .name = MT_CLS_NSMU,
 		.quirks = MT_QUIRK_NOT_SEEN_MEANS_UP },
 	{ .name = MT_CLS_SERIAL,
 		.quirks = MT_QUIRK_ALWAYS_VALID},
@@ -927,7 +929,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) },
 
@@ -940,7 +942,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 */
@@ -958,12 +960,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) },
 
@@ -1052,12 +1054,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) },
 
@@ -1075,7 +1077,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) },
 
@@ -1085,7 +1087,7 @@ static const struct hid_device_id mt_devices[] = {
 			USB_DEVICE_ID_IRTOUCH_INFRARED_USB) },
 
 	/* LG Display panels */
-	{ .driver_data = MT_CLS_DEFAULT,
+	{ .driver_data = MT_CLS_NSMU,
 		MT_USB_DEVICE(USB_VENDOR_ID_LG,
 			USB_DEVICE_ID_LG_MULTITOUCH) },
 
@@ -1117,7 +1119,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) },
 
@@ -1173,48 +1175,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.0

[PATCH 08/11] HID: 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.

Fortunately, it occurs that this behavior is pretty well compliant
with all the devices I saw so far. We can make this class the default then.

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

diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index b84790a..9dfc465 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -599,6 +599,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	0x0100
+
 #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 50fb79f..36cf346 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -33,6 +33,8 @@
 #include <linux/usb.h>
 #include <linux/input/mt.h>
 #include <linux/string.h>
+#include <asm/unaligned.h>
+#include <asm/byteorder.h>
 #include "usbhid/usbhid.h"
 
 
@@ -55,6 +57,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_COUNT_ACCURATE	(1 << 12)
 
 struct mt_slot {
 	__s32 x, y, cx, cy, p, w, h;
@@ -84,6 +87,8 @@ struct mt_device {
 	struct mt_class mtclass;	/* our mt device class */
 	struct mt_fields *fields;	/* temporary placeholder for storing the
 					   multitouch fields */
+	struct hid_field *contactcount;	/* the hid_field contact count that
+					   will be picked in mt_raw_event */
 	unsigned last_field_index;	/* last field index of the report */
 	unsigned last_pen_field_index;	/* last field index of the pen report */
 	unsigned last_slot_field;	/* the last field of a slot */
@@ -148,7 +153,9 @@ static int cypress_compute_slot(struct mt_device *td)
 }
 
 static struct mt_class mt_classes[] = {
-	{ .name = MT_CLS_DEFAULT},
+	{ .name = MT_CLS_DEFAULT,
+		.quirks = MT_QUIRK_ALWAYS_VALID |
+			MT_QUIRK_CONTACT_COUNT_ACCURATE },
 	{ .name = MT_CLS_NSMU,
 		.quirks = MT_QUIRK_NOT_SEEN_MEANS_UP },
 	{ .name = MT_CLS_SERIAL,
@@ -487,6 +494,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;
 			td->last_field_index = field->index;
 			return 1;
 		case HID_DG_CONTACTMAX:
@@ -554,6 +562,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_COUNT_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;
@@ -665,12 +677,6 @@ static int 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 */
@@ -700,6 +706,62 @@ static int mt_event(struct hid_device *hid, struct hid_field *field,
 	return 1;
 }
 
+/*
+ * Extract/implement a data field from/to a little endian report (bit array).
+ * Copied from hid-core.c.
+ *
+ * Code sort-of follows HID spec:
+ *     http://www.usb.org/developers/devclass_docs/HID1_11.pdf
+ *
+ * While the USB HID spec allows unlimited length bit fields in "report
+ * descriptors", most devices never use more than 16 bits.
+ * One model of UPS is claimed to report "LINEV" as a 32-bit field.
+ * Search linux-kernel and linux-usb-devel archives for "hid-core extract".
+ */
+
+static __u32 extract(const struct hid_device *hid, __u8 *report,
+		     unsigned offset, unsigned n)
+{
+	u64 x;
+
+	if (n > 32)
+		hid_warn(hid, "extract() called with n (%d) > 32! (%s)\n",
+			 n, current->comm);
+
+	report += offset >> 3;  /* adjust byte index */
+	offset &= 7;            /* now only need bit offset into one byte */
+	x = get_unaligned_le64(report);
+	x = (x >> offset) & ((1ULL << n) - 1);  /* extract bit field */
+	return (u32) x;
+}
+
+
+static int mt_raw_event(struct hid_device *hid, struct hid_report *report,
+			u8 *data, int size)
+{
+	struct mt_device *td = hid_get_drvdata(hid);
+	struct hid_field *field = td->contactcount;
+	unsigned value;
+
+	if (field && report->id == field->report->id) {
+		/*
+		 * Pick in advance the field HID_DG_CONTACTCOUNT as it is
+		 * often placed at the end of the report.
+		 */
+		if (report->id)
+			data++;
+		value = extract(hid, data, field->report_offset,
+				field->report_size);
+		/*
+		 * Includes multi-packet support where subsequent
+		 * packets are sent with zero contactcount.
+		 */
+		if (value)
+			td->num_expected = value;
+	}
+	return 0;
+}
+
 static void mt_set_input_mode(struct hid_device *hdev)
 {
 	struct mt_device *td = hid_get_drvdata(hdev);
@@ -1110,6 +1172,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_DEFAULT,
+		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,
@@ -1247,6 +1314,7 @@ static struct hid_driver mt_driver = {
 	.feature_mapping = mt_feature_mapping,
 	.usage_table = mt_grabbed_usages,
 	.event = mt_event,
+	.raw_event = mt_raw_event,
 #ifdef CONFIG_PM
 	.reset_resume = mt_reset_resume,
 	.resume = mt_resume,
-- 
1.8.0

[PATCH 09/11] HID: hid-multitouch: check if ContactCount is given for default quirk

[Benjamin Tissoires]

By testing the new MT_CLS_DEFAULT against the collection of device
I have[1], I noticed that eGalax ones do not work because they don't
provide the field ContactCount in their report descriptor.

Removing this quirk for this kind of device allows them to work.

[1] https://github.com/bentiss/hid-devices -> all these devices can be
reinjected in the hid subsystem through uhid (kernel > 3.6) and
hid-replay here: https://github.com/bentiss/hid-replay

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

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 36cf346..5e0a4d7 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -263,6 +263,9 @@ static ssize_t mt_set_quirks(struct device *dev,
 
 	td->mtclass.quirks = val;
 
+	if (!td->contactcount)
+		td->mtclass.quirks &= ~MT_QUIRK_CONTACT_COUNT_ACCURATE;
+
 	return count;
 }
 
@@ -856,6 +859,9 @@ static void mt_input_configured(struct hid_device *hdev, struct hid_input *hi)
 
 	input_mt_init_slots(input, td->maxcontacts, td->mt_flags);
 
+	if (!td->contactcount)
+		cls->quirks &= ~MT_QUIRK_CONTACT_COUNT_ACCURATE;
+
 	td->mt_flags = 0;
 }
 
-- 
1.8.0

[PATCH 10/11] HID: hid-multitouch: fix protocol for 3 devices

[Benjamin Tissoires]

Cando 2087:0a02 was broken, this fixes it.
ActionStar and LG panels can be optimized by using the new default
class.

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

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 5e0a4d7..b374a5a 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -997,7 +997,7 @@ static const struct hid_device_id mt_devices[] = {
 			USB_DEVICE_ID_3M3266) },
 
 	/* ActionStar panels */
-	{ .driver_data = MT_CLS_NSMU,
+	{ .driver_data = MT_CLS_DEFAULT,
 		MT_USB_DEVICE(USB_VENDOR_ID_ACTIONSTAR,
 			USB_DEVICE_ID_ACTIONSTAR_1011) },
 
@@ -1017,7 +1017,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_DEFAULT,
 		MT_USB_DEVICE(USB_VENDOR_ID_CANDO,
 			USB_DEVICE_ID_CANDO_MULTI_TOUCH_10_1) },
 	{ .driver_data = MT_CLS_DUAL_INRANGE_CONTACTNUMBER,
@@ -1155,7 +1155,7 @@ static const struct hid_device_id mt_devices[] = {
 			USB_DEVICE_ID_IRTOUCH_INFRARED_USB) },
 
 	/* LG Display panels */
-	{ .driver_data = MT_CLS_NSMU,
+	{ .driver_data = MT_CLS_DEFAULT,
 		MT_USB_DEVICE(USB_VENDOR_ID_LG,
 			USB_DEVICE_ID_LG_MULTITOUCH) },
 
-- 
1.8.0

[PATCH 11/11] HID: hid-multitouch: use MT_QUIRK_CONTACT_COUNT_ACCURATE for win 8 devices

[Benjamin Tissoires]

Win 8 devices can use MT_QUIRK_CONTACT_COUNT_ACCURATE instead of
MT_QUIRK_IGNORE_DUPLICATES. The process is a little bit faster
for MT_QUIRK_CONTACT_COUNT_ACCURATE, so let's use it.

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 b374a5a..15d2587 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -315,7 +315,7 @@ static void mt_feature_mapping(struct hid_device *hdev,
 			/* Win 8 devices need special quirks */
 			__s32 *quirks = &td->mtclass.quirks;
 			*quirks |= MT_QUIRK_ALWAYS_VALID;
-			*quirks |= MT_QUIRK_IGNORE_DUPLICATES;
+			*quirks |= MT_QUIRK_CONTACT_COUNT_ACCURATE;
 			*quirks |= MT_QUIRK_HOVERING;
 			*quirks &= ~MT_QUIRK_NOT_SEEN_MEANS_UP;
 			*quirks &= ~MT_QUIRK_VALID_IS_INRANGE;
-- 
1.8.0

Re: [PATCH 08/11] HID: hid-multitouch: add support for Nexio 42" panel

[Benjamin Tissoires]

On Fri, Nov 23, 2012 at 4:31 PM, Benjamin Tissoires
<benjamin.tissoires@gmail.com> wrote:
> 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.
>
> Fortunately, it occurs that this behavior is pretty well compliant
> with all the devices I saw so far. We can make this class the default then.
>
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
> ---
>  drivers/hid/hid-ids.h        |  3 ++
>  drivers/hid/hid-multitouch.c | 82 ++++++++++++++++++++++++++++++++++++++++----
>  2 files changed, 78 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
> index b84790a..9dfc465 100644
> --- a/drivers/hid/hid-ids.h
> +++ b/drivers/hid/hid-ids.h
> @@ -599,6 +599,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     0x0100

Oops, I made a typo here. The PID should be 0x010d.

I can send a v2 if needed.

Cheers,
Benjamin

> +
>  #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 50fb79f..36cf346 100644
> --- a/drivers/hid/hid-multitouch.c
> +++ b/drivers/hid/hid-multitouch.c
> @@ -33,6 +33,8 @@
>  #include <linux/usb.h>
>  #include <linux/input/mt.h>
>  #include <linux/string.h>
> +#include <asm/unaligned.h>
> +#include <asm/byteorder.h>
>  #include "usbhid/usbhid.h"
>
>
> @@ -55,6 +57,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_COUNT_ACCURATE        (1 << 12)
>
>  struct mt_slot {
>         __s32 x, y, cx, cy, p, w, h;
> @@ -84,6 +87,8 @@ struct mt_device {
>         struct mt_class mtclass;        /* our mt device class */
>         struct mt_fields *fields;       /* temporary placeholder for storing the
>                                            multitouch fields */
> +       struct hid_field *contactcount; /* the hid_field contact count that
> +                                          will be picked in mt_raw_event */
>         unsigned last_field_index;      /* last field index of the report */
>         unsigned last_pen_field_index;  /* last field index of the pen report */
>         unsigned last_slot_field;       /* the last field of a slot */
> @@ -148,7 +153,9 @@ static int cypress_compute_slot(struct mt_device *td)
>  }
>
>  static struct mt_class mt_classes[] = {
> -       { .name = MT_CLS_DEFAULT},
> +       { .name = MT_CLS_DEFAULT,
> +               .quirks = MT_QUIRK_ALWAYS_VALID |
> +                       MT_QUIRK_CONTACT_COUNT_ACCURATE },
>         { .name = MT_CLS_NSMU,
>                 .quirks = MT_QUIRK_NOT_SEEN_MEANS_UP },
>         { .name = MT_CLS_SERIAL,
> @@ -487,6 +494,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;
>                         td->last_field_index = field->index;
>                         return 1;
>                 case HID_DG_CONTACTMAX:
> @@ -554,6 +562,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_COUNT_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;
> @@ -665,12 +677,6 @@ static int 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 */
> @@ -700,6 +706,62 @@ static int mt_event(struct hid_device *hid, struct hid_field *field,
>         return 1;
>  }
>
> +/*
> + * Extract/implement a data field from/to a little endian report (bit array).
> + * Copied from hid-core.c.
> + *
> + * Code sort-of follows HID spec:
> + *     http://www.usb.org/developers/devclass_docs/HID1_11.pdf
> + *
> + * While the USB HID spec allows unlimited length bit fields in "report
> + * descriptors", most devices never use more than 16 bits.
> + * One model of UPS is claimed to report "LINEV" as a 32-bit field.
> + * Search linux-kernel and linux-usb-devel archives for "hid-core extract".
> + */
> +
> +static __u32 extract(const struct hid_device *hid, __u8 *report,
> +                    unsigned offset, unsigned n)
> +{
> +       u64 x;
> +
> +       if (n > 32)
> +               hid_warn(hid, "extract() called with n (%d) > 32! (%s)\n",
> +                        n, current->comm);
> +
> +       report += offset >> 3;  /* adjust byte index */
> +       offset &= 7;            /* now only need bit offset into one byte */
> +       x = get_unaligned_le64(report);
> +       x = (x >> offset) & ((1ULL << n) - 1);  /* extract bit field */
> +       return (u32) x;
> +}
> +
> +
> +static int mt_raw_event(struct hid_device *hid, struct hid_report *report,
> +                       u8 *data, int size)
> +{
> +       struct mt_device *td = hid_get_drvdata(hid);
> +       struct hid_field *field = td->contactcount;
> +       unsigned value;
> +
> +       if (field && report->id == field->report->id) {
> +               /*
> +                * Pick in advance the field HID_DG_CONTACTCOUNT as it is
> +                * often placed at the end of the report.
> +                */
> +               if (report->id)
> +                       data++;
> +               value = extract(hid, data, field->report_offset,
> +                               field->report_size);
> +               /*
> +                * Includes multi-packet support where subsequent
> +                * packets are sent with zero contactcount.
> +                */
> +               if (value)
> +                       td->num_expected = value;
> +       }
> +       return 0;
> +}
> +
>  static void mt_set_input_mode(struct hid_device *hdev)
>  {
>         struct mt_device *td = hid_get_drvdata(hdev);
> @@ -1110,6 +1172,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_DEFAULT,
> +               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,
> @@ -1247,6 +1314,7 @@ static struct hid_driver mt_driver = {
>         .feature_mapping = mt_feature_mapping,
>         .usage_table = mt_grabbed_usages,
>         .event = mt_event,
> +       .raw_event = mt_raw_event,
>  #ifdef CONFIG_PM
>         .reset_resume = mt_reset_resume,
>         .resume = mt_resume,
> --
> 1.8.0
>

Re: [PATCH 01/11] HID: hid-input factorize hid_input allocation

[Henrik Rydberg]

Hi Benjamin,

On Fri, Nov 23, 2012 at 04:31:24PM +0100, Benjamin Tissoires wrote:
> This just refactors the allocation of hid_input.

I think "breaks out the allocation" would be a more appropriate description.

> No semantic changes.
> 
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
> ---
>  drivers/hid/hid-input.c | 61 +++++++++++++++++++++++++++----------------------
>  1 file changed, 34 insertions(+), 27 deletions(-)

    Reviewed-by: Henrik Rydberg <rydberg@euromail.se>

Thanks,
Henrik

Re: [PATCH 01/11] HID: hid-input factorize hid_input allocation

[Jiri Kosina]

On Fri, 23 Nov 2012, Benjamin Tissoires wrote:

> This just refactors the allocation of hid_input.
> No semantic changes.
> 
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>

Fine by me, thanks.

> ---
>  drivers/hid/hid-input.c | 61 +++++++++++++++++++++++++++----------------------
>  1 file changed, 34 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
> index 7015080..47f98a3 100644
> --- a/drivers/hid/hid-input.c
> +++ b/drivers/hid/hid-input.c
> @@ -1163,6 +1163,38 @@ static void report_features(struct hid_device *hid)
>  			}
>  }
>  
> +static struct hid_input *hidinput_allocate(struct hid_device *hid)
> +{
> +	struct hid_input *hidinput = kzalloc(sizeof(*hidinput), GFP_KERNEL);
> +	struct input_dev *input_dev = input_allocate_device();
> +	if (!hidinput || !input_dev) {
> +		kfree(hidinput);
> +		input_free_device(input_dev);
> +		hid_err(hid, "Out of memory during hid input probe\n");
> +		return NULL;
> +	}
> +
> +	input_set_drvdata(input_dev, hid);
> +	input_dev->event = hid->ll_driver->hidinput_input_event;
> +	input_dev->open = hidinput_open;
> +	input_dev->close = hidinput_close;
> +	input_dev->setkeycode = hidinput_setkeycode;
> +	input_dev->getkeycode = hidinput_getkeycode;
> +
> +	input_dev->name = hid->name;
> +	input_dev->phys = hid->phys;
> +	input_dev->uniq = hid->uniq;
> +	input_dev->id.bustype = hid->bus;
> +	input_dev->id.vendor  = hid->vendor;
> +	input_dev->id.product = hid->product;
> +	input_dev->id.version = hid->version;
> +	input_dev->dev.parent = hid->dev.parent;
> +	hidinput->input = input_dev;
> +	list_add_tail(&hidinput->list, &hid->inputs);
> +
> +	return hidinput;
> +}
> +
>  /*
>   * Register the input device; print a message.
>   * Configure the input layer interface
> @@ -1174,7 +1206,6 @@ int hidinput_connect(struct hid_device *hid, unsigned int force)
>  	struct hid_driver *drv = hid->driver;
>  	struct hid_report *report;
>  	struct hid_input *hidinput = NULL;
> -	struct input_dev *input_dev;
>  	int i, j, k;
>  
>  	INIT_LIST_HEAD(&hid->inputs);
> @@ -1205,33 +1236,9 @@ int hidinput_connect(struct hid_device *hid, unsigned int force)
>  				continue;
>  
>  			if (!hidinput) {
> -				hidinput = kzalloc(sizeof(*hidinput), GFP_KERNEL);
> -				input_dev = input_allocate_device();
> -				if (!hidinput || !input_dev) {
> -					kfree(hidinput);
> -					input_free_device(input_dev);
> -					hid_err(hid, "Out of memory during hid input probe\n");
> +				hidinput = hidinput_allocate(hid);
> +				if (!hidinput)
>  					goto out_unwind;
> -				}
> -
> -				input_set_drvdata(input_dev, hid);
> -				input_dev->event =
> -					hid->ll_driver->hidinput_input_event;
> -				input_dev->open = hidinput_open;
> -				input_dev->close = hidinput_close;
> -				input_dev->setkeycode = hidinput_setkeycode;
> -				input_dev->getkeycode = hidinput_getkeycode;
> -
> -				input_dev->name = hid->name;
> -				input_dev->phys = hid->phys;
> -				input_dev->uniq = hid->uniq;
> -				input_dev->id.bustype = hid->bus;
> -				input_dev->id.vendor  = hid->vendor;
> -				input_dev->id.product = hid->product;
> -				input_dev->id.version = hid->version;
> -				input_dev->dev.parent = hid->dev.parent;
> -				hidinput->input = input_dev;
> -				list_add_tail(&hidinput->list, &hid->inputs);
>  			}
>  
>  			for (i = 0; i < report->maxfield; i++)
> -- 
> 1.8.0
> 

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH 02/11] HID: hid-input: simplify hid_input allocation and registration

[Henrik Rydberg]

Hi Benjamin,

> In order to provide fine control for the creation of different
> input devices in probe function of third party drivers, this patch
> split the allocations, the registrations and the free of input
> devices.
> 
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
> ---
>  drivers/hid/hid-input.c | 28 ++++++++++++++++++----------
>  1 file changed, 18 insertions(+), 10 deletions(-)

I don't like this patch, nor its purpose. Drivers should not depend on
the hid core working in a particular way internally, that spells
disaster. There must be some other way in which the same effect can be
achieved?

> 
> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
> index 47f98a3..eea02b0 100644
> --- a/drivers/hid/hid-input.c
> +++ b/drivers/hid/hid-input.c
> @@ -1206,6 +1206,7 @@ int hidinput_connect(struct hid_device *hid, unsigned int force)
>  	struct hid_driver *drv = hid->driver;
>  	struct hid_report *report;
>  	struct hid_input *hidinput = NULL;
> +	struct hid_input *next;
>  	int i, j, k;
>  
>  	INIT_LIST_HEAD(&hid->inputs);
> @@ -1238,7 +1239,7 @@ int hidinput_connect(struct hid_device *hid, unsigned int force)
>  			if (!hidinput) {
>  				hidinput = hidinput_allocate(hid);
>  				if (!hidinput)
> -					goto out_unwind;
> +					goto out_cleanup;
>  			}
>  
>  			for (i = 0; i < report->maxfield; i++)
> @@ -1253,29 +1254,36 @@ int hidinput_connect(struct hid_device *hid, unsigned int force)
>  				 * UGCI) cram a lot of unrelated inputs into the
>  				 * same interface. */
>  				hidinput->report = report;
> -				if (drv->input_configured)
> -					drv->input_configured(hid, hidinput);
> -				if (input_register_device(hidinput->input))
> -					goto out_cleanup;
>  				hidinput = NULL;
>  			}
>  		}
>  	}
>  
> -	if (hidinput) {
> +	list_for_each_entry(hidinput, &hid->inputs, list) {
>  		if (drv->input_configured)
>  			drv->input_configured(hid, hidinput);
>  		if (input_register_device(hidinput->input))
> -			goto out_cleanup;
> +			goto out_unwind;
>  	}
>  
>  	return 0;
>  
>  out_cleanup:
> -	list_del(&hidinput->list);
> -	input_free_device(hidinput->input);
> -	kfree(hidinput);
> +	list_for_each_entry_safe(hidinput, next, &hid->inputs, list) {
> +		list_del(&hidinput->list);
> +		input_free_device(hidinput->input);
> +		kfree(hidinput);
> +	}
> +	return -1;

Irregular return in the error path?

> +
>  out_unwind:
> +	/* free the non-registered hidinput, starting from the faulty one */
> +	list_for_each_entry_safe_from(hidinput, next, &hid->inputs, list) {
> +		list_del(&hidinput->list);
> +		input_free_device(hidinput->input);
> +		kfree(hidinput);
> +	}
> +
>  	/* unwind the ones we already registered */
>  	hidinput_disconnect(hid);
>  
> -- 
> 1.8.0
> 

Thanks,
Henrik

Re: [PATCH 02/11] HID: hid-input: simplify hid_input allocation and registration

[Jiri Kosina]

On Tue, 27 Nov 2012, Henrik Rydberg wrote:

> > In order to provide fine control for the creation of different
> > input devices in probe function of third party drivers, this patch
> > split the allocations, the registrations and the free of input
> > devices.
> > 
> > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
> > ---
> >  drivers/hid/hid-input.c | 28 ++++++++++++++++++----------
> >  1 file changed, 18 insertions(+), 10 deletions(-)
> 
> I don't like this patch, nor its purpose. Drivers should not depend on
> the hid core working in a particular way internally, that spells
> disaster. There must be some other way in which the same effect can be
> achieved?

The changelog doesn't seem to be really verbose enough to me.

What exactly is the scenario you are looking at here, Benjamin, please? 

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH 03/11] HID: hid-input: export hidinput_allocation function

[Henrik Rydberg]

On Fri, Nov 23, 2012 at 04:31:26PM +0100, Benjamin Tissoires wrote:
> During the probe, third party drivers can now safely create a new
> input devices depending on the parsing of the reports descriptor.
> 
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
> ---
>  drivers/hid/hid-input.c | 14 +++++++++++---
>  include/linux/hid.h     |  1 +
>  2 files changed, 12 insertions(+), 3 deletions(-)

I can think of two mechanisms that might be useful in finding a
way to achieve this cleanly: a) Let a driver return a value telling
whether to change input device, and b) Let a second driver have a go
at the same device report. Some return value or state could determine
logic in the hid core saying "we are not done with this device, try
another driver". Or something. Just not this way, please.

> 
> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
> index eea02b0..b0572d0 100644
> --- a/drivers/hid/hid-input.c
> +++ b/drivers/hid/hid-input.c
> @@ -1163,7 +1163,7 @@ static void report_features(struct hid_device *hid)
>  			}
>  }
>  
> -static struct hid_input *hidinput_allocate(struct hid_device *hid)
> +struct hid_input *hidinput_allocate(struct hid_device *hid)
>  {
>  	struct hid_input *hidinput = kzalloc(sizeof(*hidinput), GFP_KERNEL);
>  	struct input_dev *input_dev = input_allocate_device();
> @@ -1190,10 +1190,16 @@ static struct hid_input *hidinput_allocate(struct hid_device *hid)
>  	input_dev->id.version = hid->version;
>  	input_dev->dev.parent = hid->dev.parent;
>  	hidinput->input = input_dev;
> -	list_add_tail(&hidinput->list, &hid->inputs);
> +	list_add(&hidinput->list, &hid->inputs);
>  
>  	return hidinput;
>  }
> +EXPORT_SYMBOL_GPL(hidinput_allocate);
> +
> +static struct hid_input *hid_get_latest_hidinput(struct hid_device *hid)
> +{
> +	return list_first_entry(&hid->inputs, struct hid_input, list);
> +}
>  
>  /*
>   * Register the input device; print a message.
> @@ -1243,9 +1249,11 @@ int hidinput_connect(struct hid_device *hid, unsigned int force)
>  			}
>  
>  			for (i = 0; i < report->maxfield; i++)
> -				for (j = 0; j < report->field[i]->maxusage; j++)
> +				for (j = 0; j < report->field[i]->maxusage; j++) {
> +					hidinput = hid_get_latest_hidinput(hid);
>  					hidinput_configure_usage(hidinput, report->field[i],
>  								 report->field[i]->usage + j);
> +				}
>  
>  			if (hid->quirks & HID_QUIRK_MULTI_INPUT) {
>  				/* This will leave hidinput NULL, so that it
> diff --git a/include/linux/hid.h b/include/linux/hid.h
> index d2c42dd..42b02d6 100644
> --- a/include/linux/hid.h
> +++ b/include/linux/hid.h
> @@ -701,6 +701,7 @@ extern void hidinput_hid_event(struct hid_device *, struct hid_field *, struct h
>  extern void hidinput_report_event(struct hid_device *hid, struct hid_report *report);
>  extern int hidinput_connect(struct hid_device *hid, unsigned int force);
>  extern void hidinput_disconnect(struct hid_device *);
> +extern struct hid_input *hidinput_allocate(struct hid_device *hid);
>  
>  int hid_set_field(struct hid_field *, unsigned, __s32);
>  int hid_input_report(struct hid_device *, int type, u8 *, int, int);
> -- 
> 1.8.0
> 

Thanks,
Henrik

Re: [PATCH 04/11] HID: hid-multitouch: creates and handle stylus report with dual-sensors

[Henrik Rydberg]

On Fri, Nov 23, 2012 at 04:31:27PM +0100, Benjamin Tissoires wrote:
> Now that drivers can create new inputs, let's use this to split
> the reports coming from the pen and from the touch.
> 
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
> ---
>  drivers/hid/hid-multitouch.c | 30 +++++++++++++++++++++++++-----
>  1 file changed, 25 insertions(+), 5 deletions(-)

NAK on this one in its present form. I appreciate the idea, just not
the implementation, sorry.

Thanks,
Henrik

Re: [PATCH 01/11] HID: hid-input factorize hid_input allocation

[Jiri Kosina]

On Fri, 23 Nov 2012, Benjamin Tissoires wrote:

> This just refactors the allocation of hid_input.
> No semantic changes.

As this is a generic cleanup, I am taking this one through 
for-3.8/upstream branch.

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH 01/11] HID: hid-input factorize hid_input allocation

[Benjamin Tissoires]

On Thu, Nov 29, 2012 at 3:00 PM, Jiri Kosina <jkosina@suse.cz> wrote:
> On Fri, 23 Nov 2012, Benjamin Tissoires wrote:
>
>> This just refactors the allocation of hid_input.
>> No semantic changes.
>
> As this is a generic cleanup, I am taking this one through
> for-3.8/upstream branch.

Thanks Jiri.
Sorry for not answering earlier, I was working on an other solution
for pen devices before speculating on the review of the other patches
:)

Cheers,
Benjamin

>
> Thanks,
>
> --
> Jiri Kosina
> SUSE Labs

Re: [PATCH 03/11] HID: hid-input: export hidinput_allocation function

[Benjamin Tissoires]

On Tue, Nov 27, 2012 at 9:21 PM, Henrik Rydberg <rydberg@euromail.se> wrote:
> On Fri, Nov 23, 2012 at 04:31:26PM +0100, Benjamin Tissoires wrote:
>> During the probe, third party drivers can now safely create a new
>> input devices depending on the parsing of the reports descriptor.
>>
>> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
>> ---
>>  drivers/hid/hid-input.c | 14 +++++++++++---
>>  include/linux/hid.h     |  1 +
>>  2 files changed, 12 insertions(+), 3 deletions(-)
>
> I can think of two mechanisms that might be useful in finding a
> way to achieve this cleanly: a) Let a driver return a value telling
> whether to change input device, and b) Let a second driver have a go
> at the same device report. Some return value or state could determine
> logic in the hid core saying "we are not done with this device, try
> another driver". Or something. Just not this way, please.

Hi Henrik,

ok for the implementation of this patch series, it has to be reworked.

As for your proposals:

a) We can not rely on input_mapping because there is a temporal issue:
we already want to have the new input when we are in input_mapping.
So, this implies to create a new callback.

b) This would implies just too much work in hid-core for taking into
account a special case of one type of devices.
Here, we have in the same usb interface 2 different type of reports
coming from different sensors. It's far too different from the usual
configuration we have with legacy devices: when we have several hid
drivers handling the same usb device, it was when hardware makers
split the different sensors in different interfaces. This situation is
correctly handled in the usb subsystem and the hid layer has only to
deal with one driver at a time for a specific interface.

So, in the next series, I propose a new callback ("new_report" -- the
name is awful, but I can not manage to find another one ATM) which
will be called before we call input_mapping for a whole report.
The driver will then have the possibility to:
- continue normally (default behavior)
- ask for a new input device
- skip the entire report

Anyway, Henrik , could you also have a look at patches 7 to 11, they
have nothing to do with pen support, and I'm sure that you want to say
something on them too.

Cheers,
Benjamin

>
>>
>> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
>> index eea02b0..b0572d0 100644
>> --- a/drivers/hid/hid-input.c
>> +++ b/drivers/hid/hid-input.c
>> @@ -1163,7 +1163,7 @@ static void report_features(struct hid_device *hid)
>>                       }
>>  }
>>
>> -static struct hid_input *hidinput_allocate(struct hid_device *hid)
>> +struct hid_input *hidinput_allocate(struct hid_device *hid)
>>  {
>>       struct hid_input *hidinput = kzalloc(sizeof(*hidinput), GFP_KERNEL);
>>       struct input_dev *input_dev = input_allocate_device();
>> @@ -1190,10 +1190,16 @@ static struct hid_input *hidinput_allocate(struct hid_device *hid)
>>       input_dev->id.version = hid->version;
>>       input_dev->dev.parent = hid->dev.parent;
>>       hidinput->input = input_dev;
>> -     list_add_tail(&hidinput->list, &hid->inputs);
>> +     list_add(&hidinput->list, &hid->inputs);
>>
>>       return hidinput;
>>  }
>> +EXPORT_SYMBOL_GPL(hidinput_allocate);
>> +
>> +static struct hid_input *hid_get_latest_hidinput(struct hid_device *hid)
>> +{
>> +     return list_first_entry(&hid->inputs, struct hid_input, list);
>> +}
>>
>>  /*
>>   * Register the input device; print a message.
>> @@ -1243,9 +1249,11 @@ int hidinput_connect(struct hid_device *hid, unsigned int force)
>>                       }
>>
>>                       for (i = 0; i < report->maxfield; i++)
>> -                             for (j = 0; j < report->field[i]->maxusage; j++)
>> +                             for (j = 0; j < report->field[i]->maxusage; j++) {
>> +                                     hidinput = hid_get_latest_hidinput(hid);
>>                                       hidinput_configure_usage(hidinput, report->field[i],
>>                                                                report->field[i]->usage + j);
>> +                             }
>>
>>                       if (hid->quirks & HID_QUIRK_MULTI_INPUT) {
>>                               /* This will leave hidinput NULL, so that it
>> diff --git a/include/linux/hid.h b/include/linux/hid.h
>> index d2c42dd..42b02d6 100644
>> --- a/include/linux/hid.h
>> +++ b/include/linux/hid.h
>> @@ -701,6 +701,7 @@ extern void hidinput_hid_event(struct hid_device *, struct hid_field *, struct h
>>  extern void hidinput_report_event(struct hid_device *hid, struct hid_report *report);
>>  extern int hidinput_connect(struct hid_device *hid, unsigned int force);
>>  extern void hidinput_disconnect(struct hid_device *);
>> +extern struct hid_input *hidinput_allocate(struct hid_device *hid);
>>
>>  int hid_set_field(struct hid_field *, unsigned, __s32);
>>  int hid_input_report(struct hid_device *, int type, u8 *, int, int);
>> --
>> 1.8.0
>>
>
> Thanks,
> Henrik

Re: [PATCH 03/11] HID: hid-input: export hidinput_allocation function

[Henrik Rydberg]

Hi Benjamin,

> > I can think of two mechanisms that might be useful in finding a
> > way to achieve this cleanly: a) Let a driver return a value telling
> > whether to change input device, and b) Let a second driver have a go
> > at the same device report. Some return value or state could determine
> > logic in the hid core saying "we are not done with this device, try
> > another driver". Or something. Just not this way, please.
> 
> Hi Henrik,
> 
> ok for the implementation of this patch series, it has to be reworked.
> 
> As for your proposals:
> 
> a) We can not rely on input_mapping because there is a temporal issue:
> we already want to have the new input when we are in input_mapping.
> So, this implies to create a new callback.
> 
> b) This would implies just too much work in hid-core for taking into
> account a special case of one type of devices.

I may look like a special case, but perhaps it is not. Routing
different sensors to different drivers is what we do all the time, but
we call that the device-driver bus model. To fit into that concept, we
would need to split the sensors into separate devices first, then
apply the driver logic onto those devices. Thus, the problem is not
really a driver issue at all, but a bus driver one.

Imagine a partition function that is called before device add, and
which distributes the sensors of a usb device onto a set of hid
devices. We already started small in this direction by introducing
device groups, and this particular problem seems to be one of the
issues that would be resolved by such a construct.

> Here, we have in the same usb interface 2 different type of reports
> coming from different sensors. It's far too different from the usual
> configuration we have with legacy devices: when we have several hid
> drivers handling the same usb device, it was when hardware makers
> split the different sensors in different interfaces. This situation is
> correctly handled in the usb subsystem and the hid layer has only to
> deal with one driver at a time for a specific interface.
> 
> So, in the next series, I propose a new callback ("new_report" -- the
> name is awful, but I can not manage to find another one ATM) which
> will be called before we call input_mapping for a whole report.
> The driver will then have the possibility to:
> - continue normally (default behavior)
> - ask for a new input device
> - skip the entire report

This sounds like a better solution, yes, but the root problem still
remains: do we really want to handle both pen and touch in the same
driver? And do we _have_ to?

> Anyway, Henrik , could you also have a look at patches 7 to 11, they

> have nothing to do with pen support, and I'm sure that you want to say
> something on them too.

Indeed, I will just comment here, saying I do not see why you change
the name of the default. I would prefer it if you resend that set
cleaned up, with the default name unchanged.

Thanks,
Henrik

Re: [PATCH 00/11] Support of dual pen/multitouch and new default for win 7 certified devices

[Jiri Kosina]

On Fri, 23 Nov 2012, Benjamin Tissoires wrote:

> Last week, I received two new interesting devices report:
> - N-trig win 8 certified pen/touch panel
> - Samsung Nexio 42"

Bejmanin, Henrik, what are the plans with this patchset please? Planning 
respin for 3.9?

Thanks.

> 
> 
> N-trig device
> -------------
> The first one is the origin of patches 1 to 6.
> The multiouch part worked flawlessly with the win 8 patches I sent before,
> but the pen part was completely ignored.
> I could have used the quirk MULTI_INPUT, but by testing this quirk against
> several devices report I have (https://github.com/bentiss/hid-devices), it
> was a pain because some of them create 4 or 5 useless inputs.
> 
> I choose to allow the hid driver to control the creation of input devices, thus
> patches 1 to 3.
> 
> 
> Nexio device
> ------------
> The second one was more problematic. Indeed, it was not working at all with the
> current release of hid-multitouch. I had several ghost points, and any of the
> available quirks worked.
> I finaly found the trick, and this trick applies to all the win7 and win8
> devices I saw so far (same url as before).
> 
> So I think I finally understood why the windows driver was better than us: it
> first looks at the announced contact count, and treat only the right number. It
> was so simple... and it works so well...
> 
> However, for us, I need to get this information from the raw_event because most
> of the devices put the contact count field at the end of the report.
> 
> I also decided to change the default class as it is much more tolerant than the
> previous one. I could have changed all the devices, but in the end, I changed
> only those that get a benefit and that I could test.
> 
> 
> Debug tool
> ----------
> I was able to discover this trick only recently because I made a small C program
> that allows me to replay the hid events through hid-multitouch. The code is
> here: https://github.com/bentiss/hid-replay and you will need a kernel 3.6
> to make it work (it requires uhid).
> 
> However, be careful, this program can be the root of many kernel oopses if the
> targeted hid module tries to directly handle the usb or with any of the usbhid
> function.
> So, Henrik, I really need you to push your abstraction of usbhid in all hid
> modules :)
> 
> Anyway, this tool can be very helpful to debug hid devices, that's why I share
> it there... and also because I work for an open-source company :)
> 
> Happy reviewing.
> 
> Cheers,
> Benjamin
> 
> Benjamin Tissoires (11):
>   HID: hid-input factorize hid_input allocation
>   HID: hid-input: simplify hid_input allocation and registration
>   HID: hid-input: export hidinput_allocation function
>   HID: hid-multitouch: creates and handle stylus report with dual-sensors
>   HID: hid-multitouch: manually send sync event for pen input report
>   HID: hid-multitouch: append " Pen" to the name of the stylus input
>   HID: hid-multitouch: rename MT_CLS_DEFAULT into MT_CLS_NSMU
>   HID: hid-multitouch: add support for Nexio 42" panel
>   HID: hid-multitouch: check if ContactCount is given for default quirk
>   HID: hid-multitouch: fix protocol for 3 devices
>   HID: hid-multitouch: use MT_QUIRK_CONTACT_COUNT_ACCURATE for win 8 devices
> 
>  drivers/hid/hid-ids.h        |   3 +
>  drivers/hid/hid-input.c      | 100 +++++++++++++---------
>  drivers/hid/hid-multitouch.c | 198 +++++++++++++++++++++++++++++++++++--------
>  include/linux/hid.h          |   1 +
>  4 files changed, 229 insertions(+), 73 deletions(-)
> 
> -- 
> 1.8.0
> 

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH 00/11] Support of dual pen/multitouch and new default for win 7 certified devices

[Benjamin Tissoires]

On Thu, Jan 3, 2013 at 10:50 AM, Jiri Kosina <jkosina@suse.cz> wrote:
> On Fri, 23 Nov 2012, Benjamin Tissoires wrote:
>
>> Last week, I received two new interesting devices report:
>> - N-trig win 8 certified pen/touch panel
>> - Samsung Nexio 42"
>
> Bejmanin, Henrik, what are the plans with this patchset please? Planning
> respin for 3.9?
>

Hi Jiri,

sure. I intend to make a respin of this patch series. I spent a lot of
time lately to automate regressions tests for hid-multitouch. Lucky
me, because some patches of the original series would have broken some
devices. It also helped me to find out 2 other broken devices (they
were broken since their inclusion in hid-multitouch)....

The tests automation is pretty useful, however it can not be used as
this on a vanilla kernel due to the direct dependencies against
usbhid.
Henrik, you told us that you have a patch set fixing these
dependencies on all the hid drivers, will you prepare it for 3.9? Do
you want me to continue your work?

Cheers,
Benjamin


> Thanks.
>
>>
>>
>> N-trig device
>> -------------
>> The first one is the origin of patches 1 to 6.
>> The multiouch part worked flawlessly with the win 8 patches I sent before,
>> but the pen part was completely ignored.
>> I could have used the quirk MULTI_INPUT, but by testing this quirk against
>> several devices report I have (https://github.com/bentiss/hid-devices), it
>> was a pain because some of them create 4 or 5 useless inputs.
>>
>> I choose to allow the hid driver to control the creation of input devices, thus
>> patches 1 to 3.
>>
>>
>> Nexio device
>> ------------
>> The second one was more problematic. Indeed, it was not working at all with the
>> current release of hid-multitouch. I had several ghost points, and any of the
>> available quirks worked.
>> I finaly found the trick, and this trick applies to all the win7 and win8
>> devices I saw so far (same url as before).
>>
>> So I think I finally understood why the windows driver was better than us: it
>> first looks at the announced contact count, and treat only the right number. It
>> was so simple... and it works so well...
>>
>> However, for us, I need to get this information from the raw_event because most
>> of the devices put the contact count field at the end of the report.
>>
>> I also decided to change the default class as it is much more tolerant than the
>> previous one. I could have changed all the devices, but in the end, I changed
>> only those that get a benefit and that I could test.
>>
>>
>> Debug tool
>> ----------
>> I was able to discover this trick only recently because I made a small C program
>> that allows me to replay the hid events through hid-multitouch. The code is
>> here: https://github.com/bentiss/hid-replay and you will need a kernel 3.6
>> to make it work (it requires uhid).
>>
>> However, be careful, this program can be the root of many kernel oopses if the
>> targeted hid module tries to directly handle the usb or with any of the usbhid
>> function.
>> So, Henrik, I really need you to push your abstraction of usbhid in all hid
>> modules :)
>>
>> Anyway, this tool can be very helpful to debug hid devices, that's why I share
>> it there... and also because I work for an open-source company :)
>>
>> Happy reviewing.
>>
>> Cheers,
>> Benjamin
>>
>> Benjamin Tissoires (11):
>>   HID: hid-input factorize hid_input allocation
>>   HID: hid-input: simplify hid_input allocation and registration
>>   HID: hid-input: export hidinput_allocation function
>>   HID: hid-multitouch: creates and handle stylus report with dual-sensors
>>   HID: hid-multitouch: manually send sync event for pen input report
>>   HID: hid-multitouch: append " Pen" to the name of the stylus input
>>   HID: hid-multitouch: rename MT_CLS_DEFAULT into MT_CLS_NSMU
>>   HID: hid-multitouch: add support for Nexio 42" panel
>>   HID: hid-multitouch: check if ContactCount is given for default quirk
>>   HID: hid-multitouch: fix protocol for 3 devices
>>   HID: hid-multitouch: use MT_QUIRK_CONTACT_COUNT_ACCURATE for win 8 devices
>>
>>  drivers/hid/hid-ids.h        |   3 +
>>  drivers/hid/hid-input.c      | 100 +++++++++++++---------
>>  drivers/hid/hid-multitouch.c | 198 +++++++++++++++++++++++++++++++++++--------
>>  include/linux/hid.h          |   1 +
>>  4 files changed, 229 insertions(+), 73 deletions(-)
>>
>> --
>> 1.8.0
>>
>
> --
> Jiri Kosina
> SUSE Labs

Re: [PATCH 00/11] Support of dual pen/multitouch and new default for win 7 certified devices

[Henrik Rydberg]

Hi Benjamin,

> Henrik, you told us that you have a patch set fixing these
> dependencies on all the hid drivers, will you prepare it for 3.9? Do
> you want me to continue your work?

Yes, no, yes. :-)

That is, it would be great if you want to pick up those patches. I
will send you a pm shortly.

Thanks,
Henrik

Re: [PATCH 00/11] Support of dual pen/multitouch and new default for win 7 certified devices

[Jiri Kosina]

On Sun, 6 Jan 2013, Henrik Rydberg wrote:

> > Henrik, you told us that you have a patch set fixing these
> > dependencies on all the hid drivers, will you prepare it for 3.9? Do
> > you want me to continue your work?
> 
> Yes, no, yes. :-)
> 
> That is, it would be great if you want to pick up those patches. I
> will send you a pm shortly.

So, what is the status of this code now, please?

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH 00/11] Support of dual pen/multitouch and new default for win 7 certified devices

[Benjamin Tissoires]

On Tue, Jan 15, 2013 at 5:04 PM, Jiri Kosina <jkosina@suse.cz> wrote:
> On Sun, 6 Jan 2013, Henrik Rydberg wrote:
>
>> > Henrik, you told us that you have a patch set fixing these
>> > dependencies on all the hid drivers, will you prepare it for 3.9? Do
>> > you want me to continue your work?
>>
>> Yes, no, yes. :-)
>>
>> That is, it would be great if you want to pick up those patches. I
>> will send you a pm shortly.
>
> So, what is the status of this code now, please?

Hi Jiri,

well here are my plans:
1. polishing the support of Nexio 42" + new default class (95% done)
2. add support for pen (it may requires more reviews) (60% done)
3. cleanup Henrik's patches and send them. (1% done - I just went
through them). Honestly, I don't know if I will be able to make it for
3.9 for this series. For the time being, it only bothers me in my
regressions tests, so depending on the work done, we may postpone it
for later.

Last thing, this week, I'm in Boston. The good is that I should be
able to test tomorrow Mika's patch about i2c-hid at Intel's lab
directly, but I won't have much time to work on hid-multitouch before
next Wednesday (the 23rd).

Cheers,
Benjamin

>
> Thanks,
>
> --
> Jiri Kosina
> SUSE Labs