[PATCH 0/7] HID: multitouch: support of hybrid finger/pen devices

Hi guys,

Here is the support of hybrid devices presenting pen and touch at the same time.
I don't think it's possible to split the handling of the different reports by
several hid drivers unless deepest changes in the HID core subsystem.
The main problem is the control of the underlaying transport layer of the hid
device. For instance, if two drivers (let's say hid-multitouch and hid-generic)
handle the same device, and one of them is removed, then we should not call
hid_hw_stop at this point, but only when the second is also removed.

The other big problem lies with hid drivers that fix the hid report descriptors.
We can not split the hid device before we get the fix, so those drivers should
have a special behavior.

To sum up, I think does not make sense to do such deep changes, and to take such
a risk of breaking existing devices.

So the solution consists in relying inconditionaly to the quirk MULTI_INPUT for
hid-multitouch. Before registering the input device in hid-input, we can test if
it has been populated, and if not, then we simply don't register it. In order to
prevent a regression for drivers that do not fill the input device, we need to
add an other quirk.

Cheers,
Benjamin

Benjamin Tissoires (7):
  HID: input: don't register unmapped input devices
  HID: multitouch: breaks out touch handling in specific functions
  HID: multitouch: do not map usage from non used reports
  HID: multitouch: add handling for pen in dual-sensors device
  HID: multitouch: manually send sync event for pen input report
  HID: multitouch: append " Pen" to the name of the stylus input
  HID: multitouch: force BTN_STYLUS for pen devices

 drivers/hid/hid-input.c      |  77 ++++++++++++++++++++++
 drivers/hid/hid-multitouch.c | 151 ++++++++++++++++++++++++++++++++++++-------
 include/linux/hid.h          |   1 +
 3 files changed, 207 insertions(+), 22 deletions(-)

-- 
1.8.1.2

There is no need to register an input device containing no events.
This allows drivers using the quirk MULTI_INPUT to register one input
per report effectively used.

For backward compatibility, we need to add a quirk to request
this behavior.

Signed-off-by: Benjamin Tissoires <redacted>
---
 drivers/hid/hid-input.c | 77 +++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/hid.h     |  1 +
 2 files changed, 78 insertions(+)

diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index 21b196c..7aaf7d3 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c

@@ -1198,6 +1198,67 @@ static struct hid_input *hidinput_allocate(struct hid_device *hid)
 	return hidinput;
 }
 
+static bool hidinput_has_been_populated(struct hid_input *hidinput)
+{
+	int i;
+	bool r = 0;
+
+	for (i = 0; i < BITS_TO_LONGS(EV_CNT); i++)
+		r = r || hidinput->input->evbit[i];
+
+	for (i = 0; i < BITS_TO_LONGS(KEY_CNT); i++)
+		r = r || hidinput->input->keybit[i];
+
+	for (i = 0; i < BITS_TO_LONGS(REL_CNT); i++)
+		r = r || hidinput->input->relbit[i];
+
+	for (i = 0; i < BITS_TO_LONGS(ABS_CNT); i++)
+		r = r || hidinput->input->absbit[i];
+
+	for (i = 0; i < BITS_TO_LONGS(MSC_CNT); i++)
+		r = r || hidinput->input->mscbit[i];
+
+	for (i = 0; i < BITS_TO_LONGS(LED_CNT); i++)
+		r = r || hidinput->input->ledbit[i];
+
+	for (i = 0; i < BITS_TO_LONGS(SND_CNT); i++)
+		r = r || hidinput->input->sndbit[i];
+
+	for (i = 0; i < BITS_TO_LONGS(FF_CNT); i++)
+		r = r || hidinput->input->ffbit[i];
+
+	for (i = 0; i < BITS_TO_LONGS(SW_CNT); i++)
+		r = r || hidinput->input->swbit[i];
+
+	return !!r;
+}
+
+static void hidinput_cleanup_hidinput(struct hid_device *hid,
+		struct hid_input *hidinput)
+{
+	struct hid_report *report;
+	int i, k;
+
+	list_del(&hidinput->list);
+	input_free_device(hidinput->input);
+
+	for (k = HID_INPUT_REPORT; k <= HID_OUTPUT_REPORT; k++) {
+		if (k == HID_OUTPUT_REPORT &&
+			hid->quirks & HID_QUIRK_SKIP_OUTPUT_REPORTS)
+			continue;
+
+		list_for_each_entry(report, &hid->report_enum[k].report_list,
+				    list) {
+
+			for (i = 0; i < report->maxfield; i++)
+				if (report->field[i]->hidinput == hidinput)
+					report->field[i]->hidinput = NULL;
+		}
+	}
+
+	kfree(hidinput);
+}
+
 /*
  * Register the input device; print a message.
  * Configure the input layer interface

@@ -1249,6 +1310,10 @@ int hidinput_connect(struct hid_device *hid, unsigned int force)
 					hidinput_configure_usage(hidinput, report->field[i],
 								 report->field[i]->usage + j);
 
+			if ((hid->quirks & HID_QUIRK_NO_EMPTY_INPUT) &&
+			    !hidinput_has_been_populated(hidinput))
+				continue;
+
 			if (hid->quirks & HID_QUIRK_MULTI_INPUT) {
 				/* This will leave hidinput NULL, so that it
 				 * allocates another one if we have more inputs on

@@ -1265,6 +1330,18 @@ int hidinput_connect(struct hid_device *hid, unsigned int force)
 		}
 	}
 
+	if (hidinput && (hid->quirks & HID_QUIRK_NO_EMPTY_INPUT) &&
+	    !hidinput_has_been_populated(hidinput)) {
+		/* no need to register an input device not populated */
+		hidinput_cleanup_hidinput(hid, hidinput);
+		hidinput = NULL;
+	}
+
+	if (list_empty(&hid->inputs)) {
+		hid_err(hid, "No inputs registered, leaving\n");
+		goto out_unwind;
+	}
+
 	if (hidinput) {
 		if (drv->input_configured)
 			drv->input_configured(hid, hidinput);

diff --git a/include/linux/hid.h b/include/linux/hid.h
index 7071eb3..15b98a6 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h

@@ -282,6 +282,7 @@ struct hid_item {
 #define HID_QUIRK_BADPAD			0x00000020
 #define HID_QUIRK_MULTI_INPUT			0x00000040
 #define HID_QUIRK_HIDINPUT_FORCE		0x00000080
+#define HID_QUIRK_NO_EMPTY_INPUT		0x00000100
 #define HID_QUIRK_SKIP_OUTPUT_REPORTS		0x00010000
 #define HID_QUIRK_FULLSPEED_INTERVAL		0x10000000
 #define HID_QUIRK_NO_INIT_REPORTS		0x20000000

-- 
1.8.1.2

hid-multitouch only handles touch events, so there is no point in
mapping other kind of events.

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 6314473..315500c 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c

@@ -726,7 +726,7 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 	* such as Mouse that might have the same GenericDesktop usages. */
 	if (field->application != HID_DG_TOUCHSCREEN &&
 	    field->application != HID_DG_TOUCHPAD)
-		return 0;
+		return -1;
 
 	/* eGalax devices provide a Digitizer.Stylus input which overrides
 	 * the correct Digitizers.Finger X/Y ranges.

-- 
1.8.1.2

Dual sensors devices reports pen and touch on two different reports.
Using the quirk HID_QUIRK_MULTI_INPUT allows us to create a new input
device to forward pen events.

The quirk HID_QUIRK_NO_EMPTY_INPUT avoids the creation of input devices
for the not used mouse emulation present on Win7 certified devices.

Signed-off-by: Benjamin Tissoires <redacted>
---
 drivers/hid/hid-multitouch.c | 55 ++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 51 insertions(+), 4 deletions(-)

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 315500c..77ba751 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c

@@ -100,6 +100,7 @@ struct mt_device {
 	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 */
+	unsigned pen_report_id;	/* the report ID of the pen 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,

@@ -365,6 +366,35 @@ static void mt_store_field(struct hid_usage *usage, struct mt_device *td,
 	f->usages[f->length++] = usage->hid;
 }
 
+static int mt_pen_input_mapping(struct hid_device *hdev, struct hid_input *hi,
+		struct hid_field *field, struct hid_usage *usage,
+		unsigned long **bit, int *max)
+{
+	struct mt_device *td = hid_get_drvdata(hdev);
+
+	td->pen_report_id = field->report->id;
+
+	return 0;
+}
+
+static int mt_pen_input_mapped(struct hid_device *hdev, struct hid_input *hi,
+		struct hid_field *field, struct hid_usage *usage,
+		unsigned long **bit, int *max)
+{
+	return 0;
+}
+
+static int mt_pen_event(struct hid_device *hid, struct hid_field *field,
+				struct hid_usage *usage, __s32 value)
+{
+	/* let hid-input handle it */
+	return 0;
+}
+
+static void mt_pen_report(struct hid_device *hid, struct hid_report *report)
+{
+}
+
 static int mt_touch_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 		struct hid_field *field, struct hid_usage *usage,
 		unsigned long **bit, int *max)

@@ -725,14 +755,12 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 	* 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 &&
+	    field->application != HID_DG_PEN &&
 	    field->application != HID_DG_TOUCHPAD)
 		return -1;
 
-	/* eGalax devices provide a Digitizer.Stylus input which overrides
-	 * the correct Digitizers.Finger X/Y ranges.
-	 * Let's just ignore this input. */
 	if (field->physical == HID_DG_STYLUS)
-		return -1;
+		return mt_pen_input_mapping(hdev, hi, field, usage, bit, max);
 
 	return mt_touch_input_mapping(hdev, hi, field, usage, bit, max);
 }

@@ -741,6 +769,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 mt_pen_input_mapped(hdev, hi, field, usage, bit, max);
+
 	return mt_touch_input_mapped(hdev, hi, field, usage, bit, max);
 }
 

@@ -752,6 +783,9 @@ static int mt_event(struct hid_device *hid, struct hid_field *field,
 	if (field->report->id == td->mt_report_id)
 		return mt_touch_event(hid, field, usage, value);
 
+	if (field->report->id == td->pen_report_id)
+		return mt_pen_event(hid, field, usage, value);
+
 	/* ignore other reports */
 	return 1;
 }

@@ -765,6 +799,9 @@ static void mt_report(struct hid_device *hid, struct hid_report *report)
 
 	if (report->id == td->mt_report_id)
 		mt_touch_report(hid, report);
+
+	if (report->id == td->pen_report_id)
+		mt_pen_report(hid, report);
 }
 
 static void mt_set_input_mode(struct hid_device *hdev)

@@ -887,6 +924,14 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	 */
 	hdev->quirks |= HID_QUIRK_NO_INPUT_SYNC;
 
+	/*
+	 * This allows the driver to handle different input sensors
+	 * that emits events through different reports on the same HID
+	 * device.
+	 */
+	hdev->quirks |= HID_QUIRK_MULTI_INPUT;
+	hdev->quirks |= HID_QUIRK_NO_EMPTY_INPUT;
+
 	td = kzalloc(sizeof(struct mt_device), GFP_KERNEL);
 	if (!td) {
 		dev_err(&hdev->dev, "cannot allocate multitouch data\n");

@@ -896,6 +941,8 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	td->inputmode = -1;
 	td->maxcontact_report_id = -1;
 	td->cc_index = -1;
+	td->mt_report_id = -1;
+	td->pen_report_id = -1;
 	hid_set_drvdata(hdev, td);
 
 	td->fields = kzalloc(sizeof(struct mt_fields), GFP_KERNEL);

-- 
1.8.1.2

The "tablet" udev rule relies on BTN_STYLUS to be triggered.

Signed-off-by: Benjamin Tissoires <redacted>
---
 drivers/hid/hid-multitouch.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index faeec95..499954c1 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c

@@ -382,6 +382,8 @@ static int mt_pen_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 		}
 	}
 
+	/* force BTN_STYLUS to allow tablet matching in udev */
+	__set_bit(BTN_STYLUS, hi->input->keybit);
 	td->pen_report_id = field->report->id;
 
 	return 0;

-- 
1.8.1.2

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

Signed-off-by: Benjamin Tissoires <redacted>
---
 drivers/hid/hid-multitouch.c | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index d89f0eb..faeec95 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c

@@ -44,6 +44,7 @@
 #include <linux/slab.h>
 #include <linux/usb.h>
 #include <linux/input/mt.h>
+#include <linux/string.h>
 
 
 MODULE_AUTHOR("Stephane Chatty <chatty@enac.fr>");

@@ -371,6 +372,15 @@ static int mt_pen_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 		unsigned long **bit, int *max)
 {
 	struct mt_device *td = hid_get_drvdata(hdev);
+	char *name;
+
+	if (hi->input->name == hdev->name) {
+		name = kzalloc(sizeof(hdev->name) + 5, GFP_KERNEL);
+		if (name) {
+			sprintf(name, "%s Pen", hdev->name);
+			hi->input->name = name;
+		}
+	}
 
 	td->pen_report_id = field->report->id;
 

@@ -914,6 +924,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) {

@@ -964,7 +975,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);
 

@@ -976,6 +987,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);

@@ -1020,8 +1035,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.1.2

Since hid-multitouch sets the quirk HID_QUIRK_NO_INPUT_SYNC, we need
to manually send SYN events for pen report too.

Signed-off-by: Benjamin Tissoires <redacted>
---
 drivers/hid/hid-multitouch.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 77ba751..d89f0eb 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c

@@ -393,6 +393,9 @@ static int mt_pen_event(struct hid_device *hid, struct hid_field *field,
 
 static void mt_pen_report(struct hid_device *hid, struct hid_report *report)
 {
+	struct hid_field *field = report->field[0];
+
+	input_sync(field->hidinput->input);
 }
 
 static int mt_touch_input_mapping(struct hid_device *hdev, struct hid_input *hi,

-- 
1.8.1.2

This will allow easier integration of hybrid pen and touch devices.

Signed-off-by: Benjamin Tissoires <redacted>
---
 drivers/hid/hid-multitouch.c | 75 +++++++++++++++++++++++++++++++-------------
 1 file changed, 54 insertions(+), 21 deletions(-)

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 3af9efdd..6314473 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c

@@ -365,7 +365,7 @@ static void mt_store_field(struct hid_usage *usage, struct mt_device *td,
 	f->usages[f->length++] = usage->hid;
 }
 
-static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
+static int mt_touch_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 		struct hid_field *field, struct hid_usage *usage,
 		unsigned long **bit, int *max)
 {

@@ -374,13 +374,8 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 	int code;
 	struct hid_usage *prev_usage = NULL;
 
-	/* 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)
 		td->mt_flags |= INPUT_MT_DIRECT;
-	else if (field->application != HID_DG_TOUCHPAD)
-		return 0;
 
 	/*
 	 * Model touchscreens providing buttons as touchpads.

@@ -389,12 +384,6 @@ 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. */
-	if (field->physical == HID_DG_STYLUS)
-		return -1;
-
 	if (usage->usage_index)
 		prev_usage = &field->usage[usage->usage_index - 1];
 

@@ -526,7 +515,7 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 	return 0;
 }
 
-static int mt_input_mapped(struct hid_device *hdev, struct hid_input *hi,
+static int mt_touch_input_mapped(struct hid_device *hdev, struct hid_input *hi,
 		struct hid_field *field, struct hid_usage *usage,
 		unsigned long **bit, int *max)
 {

@@ -617,7 +606,7 @@ static void mt_sync_frame(struct mt_device *td, struct input_dev *input)
 	td->num_received = 0;
 }
 
-static int mt_event(struct hid_device *hid, struct hid_field *field,
+static int mt_touch_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 */

@@ -697,19 +686,13 @@ static void mt_process_mt_event(struct hid_device *hid, struct hid_field *field,
 	}
 }
 
-static void mt_report(struct hid_device *hid, struct hid_report *report)
+static void mt_touch_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;
 
-	if (report->id != td->mt_report_id)
-		return;
-
-	if (!(hid->claimed & HID_CLAIMED_INPUT))
-		return;
-
 	/*
 	 * Includes multi-packet support where subsequent
 	 * packets are sent with zero contactcount.

@@ -734,6 +717,56 @@ static void mt_report(struct hid_device *hid, struct hid_report *report)
 	}
 }
 
+static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
+		struct hid_field *field, struct hid_usage *usage,
+		unsigned long **bit, int *max)
+{
+	/* 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 &&
+	    field->application != HID_DG_TOUCHPAD)
+		return 0;
+
+	/* eGalax devices provide a Digitizer.Stylus input which overrides
+	 * the correct Digitizers.Finger X/Y ranges.
+	 * Let's just ignore this input. */
+	if (field->physical == HID_DG_STYLUS)
+		return -1;
+
+	return mt_touch_input_mapping(hdev, hi, field, usage, bit, max);
+}
+
+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)
+{
+	return mt_touch_input_mapped(hdev, hi, field, usage, bit, max);
+}
+
+static int mt_event(struct hid_device *hid, struct hid_field *field,
+				struct hid_usage *usage, __s32 value)
+{
+	struct mt_device *td = hid_get_drvdata(hid);
+
+	if (field->report->id == td->mt_report_id)
+		return mt_touch_event(hid, field, usage, value);
+
+	/* ignore other reports */
+	return 1;
+}
+
+static void mt_report(struct hid_device *hid, struct hid_report *report)
+{
+	struct mt_device *td = hid_get_drvdata(hid);
+
+	if (!(hid->claimed & HID_CLAIMED_INPUT))
+		return;
+
+	if (report->id == td->mt_report_id)
+		mt_touch_report(hid, report);
+}
+
 static void mt_set_input_mode(struct hid_device *hdev)
 {
 	struct mt_device *td = hid_get_drvdata(hdev);

-- 
1.8.1.2

On Wed, 27 Feb 2013, Benjamin Tissoires wrote:

So the solution consists in relying inconditionaly to the quirk MULTI_INPUT for
hid-multitouch. Before registering the input device in hid-input, we can test if
it has been populated, and if not, then we simply don't register it. In order to
prevent a regression for drivers that do not fill the input device, we need to
add an other quirk.

Hi everybody,

I am in progress of reviewing this patchset, and so far don't have any 
major obejctions; I expect to have the review finished this week, so 3.10 
is definitely a realistic target.

Just to set my own expectations correctly -- Henrik, are you going to 
review this patchset as well, or there is no need for me to wait for your 
feedback?

Thanks,

-- 
Jiri Kosina
SUSE Labs

Hi Benjamin,

I believe there is a bit count method that will do this for you (weight).

+
+	for (i = 0; i < BITS_TO_LONGS(KEY_CNT); i++)
+		r = r || hidinput->input->keybit[i];
+
+	for (i = 0; i < BITS_TO_LONGS(REL_CNT); i++)
+		r = r || hidinput->input->relbit[i];
+
+	for (i = 0; i < BITS_TO_LONGS(ABS_CNT); i++)
+		r = r || hidinput->input->absbit[i];
+
+	for (i = 0; i < BITS_TO_LONGS(MSC_CNT); i++)
+		r = r || hidinput->input->mscbit[i];
+
+	for (i = 0; i < BITS_TO_LONGS(LED_CNT); i++)
+		r = r || hidinput->input->ledbit[i];
+
+	for (i = 0; i < BITS_TO_LONGS(SND_CNT); i++)
+		r = r || hidinput->input->sndbit[i];
+
+	for (i = 0; i < BITS_TO_LONGS(FF_CNT); i++)
+		r = r || hidinput->input->ffbit[i];
+
+	for (i = 0; i < BITS_TO_LONGS(SW_CNT); i++)
+		r = r || hidinput->input->swbit[i];
+
+	return !!r;
+}
+
+static void hidinput_cleanup_hidinput(struct hid_device *hid,
+		struct hid_input *hidinput)
+{
+	struct hid_report *report;
+	int i, k;
+
+	list_del(&hidinput->list);
+	input_free_device(hidinput->input);
+
+	for (k = HID_INPUT_REPORT; k <= HID_OUTPUT_REPORT; k++) {
+		if (k == HID_OUTPUT_REPORT &&
+			hid->quirks & HID_QUIRK_SKIP_OUTPUT_REPORTS)
+			continue;
+
+		list_for_each_entry(report, &hid->report_enum[k].report_list,
+				    list) {
+
+			for (i = 0; i < report->maxfield; i++)
+				if (report->field[i]->hidinput == hidinput)
+					report->field[i]->hidinput = NULL;

Why test before clearing?

Is there possibly a subset of input properties that may be populated
but still not duplicated? Or the other way around?

Hi Benjamin,

Dual sensors devices reports pen and touch on two different reports.
Using the quirk HID_QUIRK_MULTI_INPUT allows us to create a new input
device to forward pen events.

The quirk HID_QUIRK_NO_EMPTY_INPUT avoids the creation of input devices
for the not used mouse emulation present on Win7 certified devices.

Signed-off-by: Benjamin Tissoires <redacted>
---
 drivers/hid/hid-multitouch.c | 55 ++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 51 insertions(+), 4 deletions(-)

I like the approach, it is indeed quite simple. Some comment below, though.

Should the next patch not go in here directly?

I seem to recall problem with MULTI_INPUT on some devices. Should
these not be turned on based on the device type?

Thanks,
Henrik

Hi Benjamin,

Why not simply duplicate the the string? This kind of magic sharing is
not really helping.  Also, for multi input devices, the assumptions in
hidinput_allocate() seem to be the culprit. Perhaps the fix should be
there instead.

Thanks,
Henrik

Hi Jiri, Benjamin,

quoted

So the solution consists in relying inconditionaly to the quirk MULTI_INPUT for
hid-multitouch. Before registering the input device in hid-input, we can test if
it has been populated, and if not, then we simply don't register it. In order to
prevent a regression for drivers that do not fill the input device, we need to
add an other quirk.

Hi everybody,

I am in progress of reviewing this patchset, and so far don't have any 
major obejctions; I expect to have the review finished this week, so 3.10 
is definitely a realistic target.

I agree, I think it looks sound in general.

Just to set my own expectations correctly -- Henrik, are you going to 
review this patchset as well, or there is no need for me to wait for your 
feedback?

No need to wait any longer ;-)

Cheers,
Henrik

Hi Henrik,

first, thanks for the review of the series.

On 03/19/2013 10:25 PM, Henrik Rydberg wrote:

Hi Benjamin,

quoted

There is no need to register an input device containing no events.
This allows drivers using the quirk MULTI_INPUT to register one input
per report effectively used.

For backward compatibility, we need to add a quirk to request
this behavior.

Signed-off-by: Benjamin Tissoires <redacted>
---
 drivers/hid/hid-input.c | 77 +++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/hid.h     |  1 +
 2 files changed, 78 insertions(+)

diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index 21b196c..7aaf7d3 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c

@@ -1198,6 +1198,67 @@ static struct hid_input *hidinput_allocate(struct hid_device *hid)
 	return hidinput;
 }
 
+static bool hidinput_has_been_populated(struct hid_input *hidinput)
+{
+	int i;
+	bool r = 0;
+
+	for (i = 0; i < BITS_TO_LONGS(EV_CNT); i++)
+		r = r || hidinput->input->evbit[i];

I believe there is a bit count method that will do this for you (weight).

thanks for that. Will change it in v2.

quoted

+
+	for (i = 0; i < BITS_TO_LONGS(KEY_CNT); i++)
+		r = r || hidinput->input->keybit[i];
+
+	for (i = 0; i < BITS_TO_LONGS(REL_CNT); i++)
+		r = r || hidinput->input->relbit[i];
+
+	for (i = 0; i < BITS_TO_LONGS(ABS_CNT); i++)
+		r = r || hidinput->input->absbit[i];
+
+	for (i = 0; i < BITS_TO_LONGS(MSC_CNT); i++)
+		r = r || hidinput->input->mscbit[i];
+
+	for (i = 0; i < BITS_TO_LONGS(LED_CNT); i++)
+		r = r || hidinput->input->ledbit[i];
+
+	for (i = 0; i < BITS_TO_LONGS(SND_CNT); i++)
+		r = r || hidinput->input->sndbit[i];
+
+	for (i = 0; i < BITS_TO_LONGS(FF_CNT); i++)
+		r = r || hidinput->input->ffbit[i];
+
+	for (i = 0; i < BITS_TO_LONGS(SW_CNT); i++)
+		r = r || hidinput->input->swbit[i];
+
+	return !!r;
+}
+
+static void hidinput_cleanup_hidinput(struct hid_device *hid,
+		struct hid_input *hidinput)
+{
+	struct hid_report *report;
+	int i, k;
+
+	list_del(&hidinput->list);
+	input_free_device(hidinput->input);
+
+	for (k = HID_INPUT_REPORT; k <= HID_OUTPUT_REPORT; k++) {
+		if (k == HID_OUTPUT_REPORT &&
+			hid->quirks & HID_QUIRK_SKIP_OUTPUT_REPORTS)
+			continue;
+
+		list_for_each_entry(report, &hid->report_enum[k].report_list,
+				    list) {
+
+			for (i = 0; i < report->maxfield; i++)
+				if (report->field[i]->hidinput == hidinput)
+					report->field[i]->hidinput = NULL;

Why test before clearing?

Well, the idea is to remove blank hidinput from the hid device, keeping
the populated ones. Thus, the argument "struct hid_input *".
In many cases, blanks hidinput and populated ones are set on the same
hid device:
Let's take a win7 device. hid-mt will populate the multitouch report,
discarding the mouse emulation report. Thus, we need to only remove the
hidinput created from the mouse emulation report, keeping the multitouch
one.

Does that makes sense?

quoted

+		}
+	}
+
+	kfree(hidinput);
+}
+
 /*
  * Register the input device; print a message.
  * Configure the input layer interface

@@ -1249,6 +1310,10 @@ int hidinput_connect(struct hid_device *hid, unsigned int force)
 					hidinput_configure_usage(hidinput, report->field[i],
 								 report->field[i]->usage + j);
 
+			if ((hid->quirks & HID_QUIRK_NO_EMPTY_INPUT) &&
+			    !hidinput_has_been_populated(hidinput))
+				continue;
+

Is there possibly a subset of input properties that may be populated
but still not duplicated? Or the other way around?

Not sure I understand your point here.
The hid spec says that reports are independent. Thus, you can have
several devices presenting different semantic (absolute, relative,
touch, pen, 3d, gyros, etc...) within the same hid interface. If you
activate both the quirks NO_EMPTY_INPUT and MULTI_INPUT, you have this
behavior correctly handled as per the spec IMO.

The thing is that it was not the default before. For instance,
hid-magicmouse for magic trackpads use the hid-input core functionality
to create its input device, but it does not populate it: input_mapping()
returns -1. The declaration of the axes is done later, once the
hid_hw_start() has returned.
Besides the fact that there may be a race with udev checking for the
device and assigning it to the touchpad class, the input is not
populated here (no matter the MULTI_INPUT quirk in place or not).
Thus, the need to specifically introduce this quirk.

So I would say that if the driver is using NO_EMPTY_INPUT, then it's its
responsability to check that its inputs are correctly configured (which
I do in hid-multitouch).

Cheers,
Benjamin

quoted

 			if (hid->quirks & HID_QUIRK_MULTI_INPUT) {
 				/* This will leave hidinput NULL, so that it
 				 * allocates another one if we have more inputs on

@@ -1265,6 +1330,18 @@ int hidinput_connect(struct hid_device *hid, unsigned int force)
 		}
 	}
 
+	if (hidinput && (hid->quirks & HID_QUIRK_NO_EMPTY_INPUT) &&
+	    !hidinput_has_been_populated(hidinput)) {
+		/* no need to register an input device not populated */
+		hidinput_cleanup_hidinput(hid, hidinput);
+		hidinput = NULL;
+	}
+
+	if (list_empty(&hid->inputs)) {
+		hid_err(hid, "No inputs registered, leaving\n");
+		goto out_unwind;
+	}
+
 	if (hidinput) {
 		if (drv->input_configured)
 			drv->input_configured(hid, hidinput);

diff --git a/include/linux/hid.h b/include/linux/hid.h
index 7071eb3..15b98a6 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h

@@ -282,6 +282,7 @@ struct hid_item {
 #define HID_QUIRK_BADPAD			0x00000020
 #define HID_QUIRK_MULTI_INPUT			0x00000040
 #define HID_QUIRK_HIDINPUT_FORCE		0x00000080
+#define HID_QUIRK_NO_EMPTY_INPUT		0x00000100
 #define HID_QUIRK_SKIP_OUTPUT_REPORTS		0x00010000
 #define HID_QUIRK_FULLSPEED_INTERVAL		0x10000000
 #define HID_QUIRK_NO_INIT_REPORTS		0x20000000

-- 
1.8.1.2

Hi Henrik,

On 03/19/2013 10:32 PM, Henrik Rydberg wrote:

Hi Benjamin,

quoted

Dual sensors devices reports pen and touch on two different reports.
Using the quirk HID_QUIRK_MULTI_INPUT allows us to create a new input
device to forward pen events.

The quirk HID_QUIRK_NO_EMPTY_INPUT avoids the creation of input devices
for the not used mouse emulation present on Win7 certified devices.

Signed-off-by: Benjamin Tissoires <redacted>
---
 drivers/hid/hid-multitouch.c | 55 ++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 51 insertions(+), 4 deletions(-)

I like the approach, it is indeed quite simple. Some comment below, though.

quoted

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 315500c..77ba751 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c

@@ -100,6 +100,7 @@ struct mt_device {
 	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 */
+	unsigned pen_report_id;	/* the report ID of the pen 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,

@@ -365,6 +366,35 @@ static void mt_store_field(struct hid_usage *usage, struct mt_device *td,
 	f->usages[f->length++] = usage->hid;
 }
 
+static int mt_pen_input_mapping(struct hid_device *hdev, struct hid_input *hi,
+		struct hid_field *field, struct hid_usage *usage,
+		unsigned long **bit, int *max)
+{
+	struct mt_device *td = hid_get_drvdata(hdev);
+
+	td->pen_report_id = field->report->id;
+
+	return 0;
+}
+
+static int mt_pen_input_mapped(struct hid_device *hdev, struct hid_input *hi,
+		struct hid_field *field, struct hid_usage *usage,
+		unsigned long **bit, int *max)
+{
+	return 0;
+}
+
+static int mt_pen_event(struct hid_device *hid, struct hid_field *field,
+				struct hid_usage *usage, __s32 value)
+{
+	/* let hid-input handle it */
+	return 0;
+}
+
+static void mt_pen_report(struct hid_device *hid, struct hid_report *report)
+{
+}

Should the next patch not go in here directly?

yes, possibly. Will squash them in v2.

quoted

+
 static int mt_touch_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 		struct hid_field *field, struct hid_usage *usage,
 		unsigned long **bit, int *max)

@@ -725,14 +755,12 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 	* 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 &&
+	    field->application != HID_DG_PEN &&
 	    field->application != HID_DG_TOUCHPAD)
 		return -1;
 
-	/* eGalax devices provide a Digitizer.Stylus input which overrides
-	 * the correct Digitizers.Finger X/Y ranges.
-	 * Let's just ignore this input. */
 	if (field->physical == HID_DG_STYLUS)
-		return -1;
+		return mt_pen_input_mapping(hdev, hi, field, usage, bit, max);
 
 	return mt_touch_input_mapping(hdev, hi, field, usage, bit, max);
 }

@@ -741,6 +769,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 mt_pen_input_mapped(hdev, hi, field, usage, bit, max);
+
 	return mt_touch_input_mapped(hdev, hi, field, usage, bit, max);
 }
 

@@ -752,6 +783,9 @@ static int mt_event(struct hid_device *hid, struct hid_field *field,
 	if (field->report->id == td->mt_report_id)
 		return mt_touch_event(hid, field, usage, value);
 
+	if (field->report->id == td->pen_report_id)
+		return mt_pen_event(hid, field, usage, value);
+
 	/* ignore other reports */
 	return 1;
 }

@@ -765,6 +799,9 @@ static void mt_report(struct hid_device *hid, struct hid_report *report)
 
 	if (report->id == td->mt_report_id)
 		mt_touch_report(hid, report);
+
+	if (report->id == td->pen_report_id)
+		mt_pen_report(hid, report);
 }
 
 static void mt_set_input_mode(struct hid_device *hdev)

@@ -887,6 +924,14 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	 */
 	hdev->quirks |= HID_QUIRK_NO_INPUT_SYNC;
 
+	/*
+	 * This allows the driver to handle different input sensors
+	 * that emits events through different reports on the same HID
+	 * device.
+	 */
+	hdev->quirks |= HID_QUIRK_MULTI_INPUT;
+	hdev->quirks |= HID_QUIRK_NO_EMPTY_INPUT;

I seem to recall problem with MULTI_INPUT on some devices. Should
these not be turned on based on the device type?

IIRC, those problems appeared a long time ago, when hid-mt was not
handling properly the different collections. Now, the tests I conducted
show that all the device I know are properly handled this way.
Anyway, if you think it would be safer, we can add a quirk in the
mtclass and set the MULTI_INPUT quirk if requested.

Cheers,
Benjamin

quoted

+
 	td = kzalloc(sizeof(struct mt_device), GFP_KERNEL);
 	if (!td) {
 		dev_err(&hdev->dev, "cannot allocate multitouch data\n");

@@ -896,6 +941,8 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	td->inputmode = -1;
 	td->maxcontact_report_id = -1;
 	td->cc_index = -1;
+	td->mt_report_id = -1;
+	td->pen_report_id = -1;
 	hid_set_drvdata(hdev, td);
 
 	td->fields = kzalloc(sizeof(struct mt_fields), GFP_KERNEL);

-- 
1.8.1.2

Thanks,
Henrik

On 03/19/2013 10:38 PM, Henrik Rydberg wrote:

Hi Benjamin,

quoted

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

Signed-off-by: Benjamin Tissoires <redacted>
---
 drivers/hid/hid-multitouch.c | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index d89f0eb..faeec95 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c

@@ -44,6 +44,7 @@
 #include <linux/slab.h>
 #include <linux/usb.h>
 #include <linux/input/mt.h>
+#include <linux/string.h>
 
 
 MODULE_AUTHOR("Stephane Chatty <chatty@enac.fr>");

@@ -371,6 +372,15 @@ static int mt_pen_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 		unsigned long **bit, int *max)
 {
 	struct mt_device *td = hid_get_drvdata(hdev);
+	char *name;
+
+	if (hi->input->name == hdev->name) {
+		name = kzalloc(sizeof(hdev->name) + 5, GFP_KERNEL);
+		if (name) {
+			sprintf(name, "%s Pen", hdev->name);
+			hi->input->name = name;
+		}
+	}

Why not simply duplicate the the string? This kind of magic sharing is
not really helping.  Also, for multi input devices, the assumptions in
hidinput_allocate() seem to be the culprit. Perhaps the fix should be
there instead.

Yes, hidinput_allocate() clearly steals the reference here. However,
fixing this in hid-core would not be simpler: some drivers (ntrig does
at least) assume that there is no need to free input->name, which means
that:
- either we need to fix each individual driver
- either we introduce some kind of mechanism in order not to free "const
char *" declarations like it happens in ntrig.

In v2, I'll just realloc for each input the input->name, and then if the
pen is here, then it will realloc the new name.

Cheers,
Benjamin

quoted

 
 	td->pen_report_id = field->report->id;
 

@@ -914,6 +924,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) {

@@ -964,7 +975,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);
 

@@ -976,6 +987,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);

@@ -1020,8 +1035,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.1.2

Thanks,
Henrik

On 03/19/2013 10:25 PM, Henrik Rydberg wrote:

Hi Benjamin,

quoted

There is no need to register an input device containing no events.
This allows drivers using the quirk MULTI_INPUT to register one input
per report effectively used.

For backward compatibility, we need to add a quirk to request
this behavior.

Signed-off-by: Benjamin Tissoires <redacted>
---
 drivers/hid/hid-input.c | 77 +++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/hid.h     |  1 +
 2 files changed, 78 insertions(+)

diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index 21b196c..7aaf7d3 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c

@@ -1198,6 +1198,67 @@ static struct hid_input *hidinput_allocate(struct hid_device *hid)
 	return hidinput;
 }
 
+static bool hidinput_has_been_populated(struct hid_input *hidinput)
+{
+	int i;
+	bool r = 0;
+
+	for (i = 0; i < BITS_TO_LONGS(EV_CNT); i++)
+		r = r || hidinput->input->evbit[i];

I believe there is a bit count method that will do this for you (weight).

Actually, weight does not work here. evbit[] is an array of long, and we
still need to check if each field of this array is not null.

Thus, the following can be used (r being a long here):
r |= hidinput->input->evbit[i];

Cheers,
Benjamin