[PATCH v2 0/6] HID: input cleanups and mt additions

[PATCH v2 0/6] HID: input cleanups and mt additions

[Benjamin Tissoires]

Hi Jiri,

following the thread about the 'not incrementing ABS_MISC', here is the
actual submission of the series.

Compared to the 2 patches I sent last week, there are a few more:
- patch 1 needed to be added or some (all?) Advanced Silicon touchscreen
  would fail the tests with HID_QUIRK_INPUT_PER_APPLICATION
- patch 3 is extracted from v1-2/2, as it has grown significantly, still
  because of the same issue with Advanced Silicon panels
- patch 4 is something I had in mind while debugging some other
  not-so-talkative touchpads, because of patch 5
- patch 5 should put us at the same level than the Windows driver now.
  Note that we could unset the features (button/surface reporting, and
  latency during suspend, but not sure it helps that much. We should probably
  test the current consumption with and without)

The series has been tested against regressions thanks to my new hid test suite
https://github.com/bentiss/hid-tools (I rewrote hid-replay in python and hook
up python tests). I also used the loaner from Dell (thanks Mario!), and XPS
9360 that has both touchscreen and touchpad, and on which the touchpad has the
new features I implemented here.

Cheers,
Benjamin


Benjamin Tissoires (6):
  HID: store the full list of reports in the hidinput
  HID: generic: create one input report per application type
  HID: input: append a suffix matching the application
  HID: multitouch: make use of HID_QUIRK_INPUT_PER_APP
  HID: multitouch: simplify the settings of the various features
  HID: multitouch: implement precision touchpad latency and switches

 drivers/hid/hid-core.c       |  19 ++--
 drivers/hid/hid-generic.c    |  15 +++
 drivers/hid/hid-gfrm.c       |   2 +-
 drivers/hid/hid-input.c      |  90 +++++++++++++++--
 drivers/hid/hid-magicmouse.c |   6 +-
 drivers/hid/hid-multitouch.c | 227 ++++++++++++++++++++++---------------------
 include/linux/hid.h          |  16 ++-
 7 files changed, 244 insertions(+), 131 deletions(-)

-- 
2.14.3

[PATCH v2 1/6] HID: store the full list of reports in the hidinput

[Benjamin Tissoires]

We were only storing the report in case of QUIRK_MULTI_INPUT.
It is interesting for the upcoming  HID_QUIRK_INPUT_PER_APP to also
store the full list of reports that are attached to it.

We need the full list because a device (Advanced Silicon has some)
might want to use a different report ID for the Input reports and
the Output reports. Storing the full list allows the drivers to
have all the data.

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

diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index a8a33e56b43e..62e42664955e 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -1536,9 +1536,12 @@ static struct hid_input *hidinput_allocate(struct hid_device *hid)
 	input_dev->id.product = hid->product;
 	input_dev->id.version = hid->version;
 	input_dev->dev.parent = &hid->dev;
+
 	hidinput->input = input_dev;
 	list_add_tail(&hidinput->list, &hid->inputs);
 
+	INIT_LIST_HEAD(&hidinput->reports);
+
 	return hidinput;
 }
 
@@ -1688,6 +1691,9 @@ int hidinput_connect(struct hid_device *hid, unsigned int force)
 
 			if (hid->quirks & HID_QUIRK_MULTI_INPUT)
 				hidinput->report = report;
+
+			list_add_tail(&report->hidinput_list,
+				      &hidinput->reports);
 		}
 	}
 
diff --git a/include/linux/hid.h b/include/linux/hid.h
index 0a8d5c320b6c..e1ec7f91f926 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -464,6 +464,7 @@ struct hid_field {
 
 struct hid_report {
 	struct list_head list;
+	struct list_head hidinput_list;
 	unsigned id;					/* id of this report */
 	unsigned type;					/* report type */
 	struct hid_field *field[HID_MAX_FIELDS];	/* fields of the report */
@@ -510,6 +511,7 @@ struct hid_input {
 	struct hid_report *report;
 	struct input_dev *input;
 	bool registered;
+	struct list_head reports;	/* the list of reports */
 };
 
 enum hid_type {
-- 
2.14.3

[PATCH v2 2/6] HID: generic: create one input report per application type

[Benjamin Tissoires]

It is not a good idea to try to fit all types of applications in the
same input report. There are a lot of devices that are needing
the quirk HID_MULTI_INPUT but this quirk doesn't match the actual HID
description as it is based on the report ID.

Given that most devices with MULTI_INPUT I can think of split nicely
the devices inputs into application, it is a good thing to split the
devices by default based on this assumption.

Also make hid-multitouch following this rule, to not have to deal
with too many input created.

While we are at it, fix some checkpatch complaints about converting
'unsigned' to 'unsigned int'.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/hid-core.c       | 19 +++++++++++++------
 drivers/hid/hid-generic.c    | 15 +++++++++++++++
 drivers/hid/hid-gfrm.c       |  2 +-
 drivers/hid/hid-input.c      | 17 +++++++++++++++++
 drivers/hid/hid-magicmouse.c |  6 +++---
 include/linux/hid.h          | 10 +++++++---
 6 files changed, 56 insertions(+), 13 deletions(-)

diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 5d7cc6bbbac6..68819106f4fc 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -57,7 +57,9 @@ MODULE_PARM_DESC(ignore_special_drivers, "Ignore any special drivers and handle
  * Register a new report for a device.
  */
 
-struct hid_report *hid_register_report(struct hid_device *device, unsigned type, unsigned id)
+struct hid_report *hid_register_report(struct hid_device *device,
+				       unsigned int type, unsigned int id,
+				       unsigned int application)
 {
 	struct hid_report_enum *report_enum = device->report_enum + type;
 	struct hid_report *report;
@@ -78,6 +80,7 @@ struct hid_report *hid_register_report(struct hid_device *device, unsigned type,
 	report->type = type;
 	report->size = 0;
 	report->device = device;
+	report->application = application;
 	report_enum->report_id_hash[id] = report;
 
 	list_add_tail(&report->list, &report_enum->report_list);
@@ -221,11 +224,15 @@ static int hid_add_field(struct hid_parser *parser, unsigned report_type, unsign
 {
 	struct hid_report *report;
 	struct hid_field *field;
-	unsigned usages;
-	unsigned offset;
-	unsigned i;
+	unsigned int usages;
+	unsigned int offset;
+	unsigned int i;
+	unsigned int application;
+
+	application = hid_lookup_collection(parser, HID_COLLECTION_APPLICATION);
 
-	report = hid_register_report(parser->device, report_type, parser->global.report_id);
+	report = hid_register_report(parser->device, report_type,
+				     parser->global.report_id, application);
 	if (!report) {
 		hid_err(parser->device, "hid_register_report failed\n");
 		return -1;
@@ -259,7 +266,7 @@ static int hid_add_field(struct hid_parser *parser, unsigned report_type, unsign
 
 	field->physical = hid_lookup_collection(parser, HID_COLLECTION_PHYSICAL);
 	field->logical = hid_lookup_collection(parser, HID_COLLECTION_LOGICAL);
-	field->application = hid_lookup_collection(parser, HID_COLLECTION_APPLICATION);
+	field->application = application;
 
 	for (i = 0; i < usages; i++) {
 		unsigned j = i;
diff --git a/drivers/hid/hid-generic.c b/drivers/hid/hid-generic.c
index c25b4718de44..3b6eccbc2519 100644
--- a/drivers/hid/hid-generic.c
+++ b/drivers/hid/hid-generic.c
@@ -56,6 +56,20 @@ static bool hid_generic_match(struct hid_device *hdev,
 	return true;
 }
 
+static int hid_generic_probe(struct hid_device *hdev,
+			     const struct hid_device_id *id)
+{
+	int ret;
+
+	hdev->quirks |= HID_QUIRK_INPUT_PER_APP;
+
+	ret = hid_parse(hdev);
+	if (ret)
+		return ret;
+
+	return hid_hw_start(hdev, HID_CONNECT_DEFAULT);
+}
+
 static const struct hid_device_id hid_table[] = {
 	{ HID_DEVICE(HID_BUS_ANY, HID_GROUP_ANY, HID_ANY_ID, HID_ANY_ID) },
 	{ }
@@ -66,6 +80,7 @@ static struct hid_driver hid_generic = {
 	.name = "hid-generic",
 	.id_table = hid_table,
 	.match = hid_generic_match,
+	.probe = hid_generic_probe,
 };
 module_hid_driver(hid_generic);
 
diff --git a/drivers/hid/hid-gfrm.c b/drivers/hid/hid-gfrm.c
index 075b1c020846..cf477f8c8f4c 100644
--- a/drivers/hid/hid-gfrm.c
+++ b/drivers/hid/hid-gfrm.c
@@ -116,7 +116,7 @@ static int gfrm_probe(struct hid_device *hdev, const struct hid_device_id *id)
 		 * those reports reach gfrm_raw_event() from hid_input_report().
 		 */
 		if (!hid_register_report(hdev, HID_INPUT_REPORT,
-					 GFRM100_SEARCH_KEY_REPORT_ID)) {
+					 GFRM100_SEARCH_KEY_REPORT_ID, 0)) {
 			ret = -ENOMEM;
 			goto done;
 		}
diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index 62e42664955e..361643683c08 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -1620,6 +1620,20 @@ static struct hid_input *hidinput_match(struct hid_report *report)
 	return NULL;
 }
 
+static struct hid_input *hidinput_match_application(struct hid_report *report)
+{
+	struct hid_device *hid = report->device;
+	struct hid_input *hidinput;
+
+	list_for_each_entry(hidinput, &hid->inputs, list) {
+		if (hidinput->report &&
+		    hidinput->report->application == report->application)
+			return hidinput;
+	}
+
+	return NULL;
+}
+
 static inline void hidinput_configure_usages(struct hid_input *hidinput,
 					     struct hid_report *report)
 {
@@ -1680,6 +1694,9 @@ int hidinput_connect(struct hid_device *hid, unsigned int force)
 			 */
 			if (hid->quirks & HID_QUIRK_MULTI_INPUT)
 				hidinput = hidinput_match(report);
+			else if (hid->maxapplication > 1 &&
+				 (hid->quirks & HID_QUIRK_INPUT_PER_APP))
+				hidinput = hidinput_match_application(report);
 
 			if (!hidinput) {
 				hidinput = hidinput_allocate(hid);
diff --git a/drivers/hid/hid-magicmouse.c b/drivers/hid/hid-magicmouse.c
index 42ed887ba0be..b454c4386157 100644
--- a/drivers/hid/hid-magicmouse.c
+++ b/drivers/hid/hid-magicmouse.c
@@ -531,12 +531,12 @@ static int magicmouse_probe(struct hid_device *hdev,
 
 	if (id->product == USB_DEVICE_ID_APPLE_MAGICMOUSE)
 		report = hid_register_report(hdev, HID_INPUT_REPORT,
-			MOUSE_REPORT_ID);
+			MOUSE_REPORT_ID, 0);
 	else { /* USB_DEVICE_ID_APPLE_MAGICTRACKPAD */
 		report = hid_register_report(hdev, HID_INPUT_REPORT,
-			TRACKPAD_REPORT_ID);
+			TRACKPAD_REPORT_ID, 0);
 		report = hid_register_report(hdev, HID_INPUT_REPORT,
-			DOUBLE_REPORT_ID);
+			DOUBLE_REPORT_ID, 0);
 	}
 
 	if (!report) {
diff --git a/include/linux/hid.h b/include/linux/hid.h
index e1ec7f91f926..d5ebeacc3b57 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -341,6 +341,7 @@ struct hid_item {
 /* BIT(8) reserved for backward compatibility, was HID_QUIRK_NO_EMPTY_INPUT */
 /* BIT(9) reserved for backward compatibility, was NO_INIT_INPUT_REPORTS */
 #define HID_QUIRK_ALWAYS_POLL			BIT(10)
+#define HID_QUIRK_INPUT_PER_APP			BIT(11)
 #define HID_QUIRK_SKIP_OUTPUT_REPORTS		BIT(16)
 #define HID_QUIRK_SKIP_OUTPUT_REPORT_ID		BIT(17)
 #define HID_QUIRK_NO_OUTPUT_REPORTS_ON_INTR_EP	BIT(18)
@@ -465,8 +466,9 @@ struct hid_field {
 struct hid_report {
 	struct list_head list;
 	struct list_head hidinput_list;
-	unsigned id;					/* id of this report */
-	unsigned type;					/* report type */
+	unsigned int id;				/* id of this report */
+	unsigned int type;				/* report type */
+	unsigned int application;			/* application usage for this report */
 	struct hid_field *field[HID_MAX_FIELDS];	/* fields of the report */
 	unsigned maxfield;				/* maximum valid field index */
 	unsigned size;					/* size of the report (bits) */
@@ -868,7 +870,9 @@ void hid_output_report(struct hid_report *report, __u8 *data);
 void __hid_request(struct hid_device *hid, struct hid_report *rep, int reqtype);
 u8 *hid_alloc_report_buf(struct hid_report *report, gfp_t flags);
 struct hid_device *hid_allocate_device(void);
-struct hid_report *hid_register_report(struct hid_device *device, unsigned type, unsigned id);
+struct hid_report *hid_register_report(struct hid_device *device,
+				       unsigned int type, unsigned int id,
+				       unsigned int application);
 int hid_parse_report(struct hid_device *hid, __u8 *start, unsigned size);
 struct hid_report *hid_validate_values(struct hid_device *hid,
 				       unsigned int type, unsigned int id,
-- 
2.14.3

[PATCH v2 3/6] HID: input: append a suffix matching the application

[Benjamin Tissoires]

Given that we create one input node per application, we should name
the input node accordingly to not lose userspace.

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

diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index 361643683c08..ab93dd5927c3 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -1510,15 +1510,56 @@ static void report_features(struct hid_device *hid)
 		}
 }
 
-static struct hid_input *hidinput_allocate(struct hid_device *hid)
+static struct hid_input *hidinput_allocate(struct hid_device *hid,
+					   unsigned int application)
 {
 	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;
+	const char *suffix = NULL;
+
+	if (!hidinput || !input_dev)
+		goto fail;
+
+	if ((hid->quirks & HID_QUIRK_INPUT_PER_APP) &&
+	    hid->maxapplication > 1) {
+		switch (application) {
+		case HID_GD_KEYBOARD:
+			suffix = "Keyboard";
+			break;
+		case HID_GD_KEYPAD:
+			suffix = "Keypad";
+			break;
+		case HID_GD_MOUSE:
+			suffix = "Mouse";
+			break;
+		case HID_DG_STYLUS:
+			suffix = "Pen";
+			break;
+		case HID_DG_TOUCHSCREEN:
+			suffix = "Touchscreen";
+			break;
+		case HID_DG_TOUCHPAD:
+			suffix = "Touchpad";
+			break;
+		case HID_GD_SYSTEM_CONTROL:
+			suffix = "System Control";
+			break;
+		case HID_CP_CONSUMER_CONTROL:
+			suffix = "Consumer Control";
+			break;
+		case HID_GD_WIRELESS_RADIO_CTLS:
+			suffix = "Wireless Radio Control";
+			break;
+		default:
+			break;
+		}
+	}
+
+	if (suffix) {
+		hidinput->name = kasprintf(GFP_KERNEL, "%s %s",
+					   hid->name, suffix);
+		if (!hidinput->name)
+			goto fail;
 	}
 
 	input_set_drvdata(input_dev, hid);
@@ -1528,7 +1569,7 @@ static struct hid_input *hidinput_allocate(struct hid_device *hid)
 	input_dev->setkeycode = hidinput_setkeycode;
 	input_dev->getkeycode = hidinput_getkeycode;
 
-	input_dev->name = hid->name;
+	input_dev->name = hidinput->name ? hidinput->name : hid->name;
 	input_dev->phys = hid->phys;
 	input_dev->uniq = hid->uniq;
 	input_dev->id.bustype = hid->bus;
@@ -1543,6 +1584,12 @@ static struct hid_input *hidinput_allocate(struct hid_device *hid)
 	INIT_LIST_HEAD(&hidinput->reports);
 
 	return hidinput;
+
+fail:
+	kfree(hidinput);
+	input_free_device(input_dev);
+	hid_err(hid, "Out of memory during hid input probe\n");
+	return NULL;
 }
 
 static bool hidinput_has_been_populated(struct hid_input *hidinput)
@@ -1588,6 +1635,7 @@ static void hidinput_cleanup_hidinput(struct hid_device *hid,
 
 	list_del(&hidinput->list);
 	input_free_device(hidinput->input);
+	kfree(hidinput->name);
 
 	for (k = HID_INPUT_REPORT; k <= HID_OUTPUT_REPORT; k++) {
 		if (k == HID_OUTPUT_REPORT &&
@@ -1656,6 +1704,7 @@ int hidinput_connect(struct hid_device *hid, unsigned int force)
 	struct hid_driver *drv = hid->driver;
 	struct hid_report *report;
 	struct hid_input *next, *hidinput = NULL;
+	unsigned int application;
 	int i, k;
 
 	INIT_LIST_HEAD(&hid->inputs);
@@ -1688,6 +1737,8 @@ int hidinput_connect(struct hid_device *hid, unsigned int force)
 			if (!report->maxfield)
 				continue;
 
+			application = report->application;
+
 			/*
 			 * Find the previous hidinput report attached
 			 * to this report id.
@@ -1699,7 +1750,7 @@ int hidinput_connect(struct hid_device *hid, unsigned int force)
 				hidinput = hidinput_match_application(report);
 
 			if (!hidinput) {
-				hidinput = hidinput_allocate(hid);
+				hidinput = hidinput_allocate(hid, application);
 				if (!hidinput)
 					goto out_unwind;
 			}
diff --git a/include/linux/hid.h b/include/linux/hid.h
index d5ebeacc3b57..d3bd83d9127f 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -512,6 +512,7 @@ struct hid_input {
 	struct list_head list;
 	struct hid_report *report;
 	struct input_dev *input;
+	const char *name;
 	bool registered;
 	struct list_head reports;	/* the list of reports */
 };
-- 
2.14.3

[PATCH v2 4/6] HID: multitouch: make use of HID_QUIRK_INPUT_PER_APP

[Benjamin Tissoires]

We now have HID_QUIRK_INPUT_PER_APPLICATION that splits the devices
into several devices. This helps us as we can now rely on hid-input
to set the names for us.

Also, this helps removing some magical numbers '0' when calling
.input_configured().

The only thing to take care of is that the field .report in struct
hid_input is now null. We need to iterate over the full list of
reports attached to a hid_input.

This is required for some Advanced Silicon touchscreen to correctly apply
the HID_QUIRK_INPUT_PER_APPLICATION as they have 2 reports associated
with the hidinput node. One contains the Input data, the other one
contains the Output data.

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

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index dad2fbb0e3f8..43784d31a1a3 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -1274,54 +1274,48 @@ static int mt_input_configured(struct hid_device *hdev, struct hid_input *hi)
 	struct mt_device *td = hid_get_drvdata(hdev);
 	char *name;
 	const char *suffix = NULL;
-	struct hid_field *field = hi->report->field[0];
+	unsigned int application = 0;
+	struct hid_report *report;
 	int ret;
 
-	if (hi->report->id == td->mt_report_id) {
-		ret = mt_touch_input_configured(hdev, hi);
-		if (ret)
-			return ret;
+	list_for_each_entry(report, &hi->reports, hidinput_list) {
+		application = report->application;
+		if (report->id == td->mt_report_id) {
+			ret = mt_touch_input_configured(hdev, hi);
+			if (ret)
+				return ret;
+		}
+
+		/*
+		 * some egalax touchscreens have "application == DG_TOUCHSCREEN"
+		 * for the stylus. Check this first, and then rely on
+		 * the application field.
+		 */
+		if (report->field[0]->physical == HID_DG_STYLUS) {
+			suffix = "Pen";
+			/* force BTN_STYLUS to allow tablet matching in udev */
+			__set_bit(BTN_STYLUS, hi->input->keybit);
+		}
 	}
 
-	/*
-	 * some egalax touchscreens have "application == HID_DG_TOUCHSCREEN"
-	 * for the stylus. Check this first, and then rely on the application
-	 * field.
-	 */
-	if (hi->report->field[0]->physical == HID_DG_STYLUS) {
-		suffix = "Pen";
-		/* force BTN_STYLUS to allow tablet matching in udev */
-		__set_bit(BTN_STYLUS, hi->input->keybit);
-	} else {
-		switch (field->application) {
+	if (!suffix) {
+		switch (application) {
 		case HID_GD_KEYBOARD:
-			suffix = "Keyboard";
-			break;
 		case HID_GD_KEYPAD:
-			suffix = "Keypad";
-			break;
 		case HID_GD_MOUSE:
-			suffix = "Mouse";
-			break;
-		case HID_DG_STYLUS:
-			suffix = "Pen";
-			/* force BTN_STYLUS to allow tablet matching in udev */
-			__set_bit(BTN_STYLUS, hi->input->keybit);
-			break;
-		case HID_DG_TOUCHSCREEN:
-			/* we do not set suffix = "Touchscreen" */
-			break;
 		case HID_DG_TOUCHPAD:
-			suffix = "Touchpad";
-			break;
 		case HID_GD_SYSTEM_CONTROL:
-			suffix = "System Control";
-			break;
 		case HID_CP_CONSUMER_CONTROL:
-			suffix = "Consumer Control";
-			break;
 		case HID_GD_WIRELESS_RADIO_CTLS:
-			suffix = "Wireless Radio Control";
+			/* already handled by hid core */
+			break;
+		case HID_DG_TOUCHSCREEN:
+			/* we do not set suffix = "Touchscreen" */
+			hi->input->name = hdev->name;
+			break;
+		case HID_DG_STYLUS:
+			/* force BTN_STYLUS to allow tablet matching in udev */
+			__set_bit(BTN_STYLUS, hi->input->keybit);
 			break;
 		case HID_VD_ASUS_CUSTOM_MEDIA_KEYS:
 			suffix = "Custom Media Keys";
@@ -1459,10 +1453,10 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
 
 	/*
 	 * This allows the driver to handle different input sensors
-	 * that emits events through different reports on the same HID
+	 * that emits events through different applications on the same HID
 	 * device.
 	 */
-	hdev->quirks |= HID_QUIRK_MULTI_INPUT;
+	hdev->quirks |= HID_QUIRK_INPUT_PER_APP;
 
 	timer_setup(&td->release_timer, mt_expired_timeout, 0);
 
-- 
2.14.3

[PATCH v2 5/6] HID: multitouch: simplify the settings of the various features

[Benjamin Tissoires]

The Win8 spec also declare other features we want to support:
latency and surface and button switches.

Though it doesn't seem we need to activate those by default, we have been
proved in the past that manufacturers rely on the Windows driver behavior
so we better mimic it to prevent further issues.

The current way of setting the features is cumbersome. It avoids iterating
over the list of features, but the way we store/retrieve the data just
doesn't scale with more than two values.

So iterate over the features when we decide to switch on the device and
make it simpler to extend.

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

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 43784d31a1a3..8878de9eedba 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -127,11 +127,7 @@ struct mt_device {
 	int left_button_state;	/* left button state */
 	unsigned last_slot_field;	/* the last field of a slot */
 	unsigned mt_report_id;	/* the report ID of the multitouch device */
-	__s16 inputmode;	/* InputMode HID feature, -1 if non-existent */
-	__s16 inputmode_index;	/* InputMode HID feature index in the report */
-	__s16 maxcontact_report_id;	/* Maximum Contact Number HID feature,
-				   -1 if non-existent */
-	__u8 inputmode_value;  /* InputMode HID feature value */
+	__u8 inputmode_value;	/* InputMode HID feature value */
 	__u8 num_received;	/* how many contacts we received */
 	__u8 num_expected;	/* expected last contact index */
 	__u8 maxcontacts;
@@ -415,32 +411,9 @@ static void mt_feature_mapping(struct hid_device *hdev,
 	struct mt_device *td = hid_get_drvdata(hdev);
 
 	switch (usage->hid) {
-	case HID_DG_INPUTMODE:
-		/* Ignore if value index is out of bounds. */
-		if (usage->usage_index >= field->report_count) {
-			dev_err(&hdev->dev, "HID_DG_INPUTMODE out of range\n");
-			break;
-		}
-
-		if (td->inputmode < 0) {
-			td->inputmode = field->report->id;
-			td->inputmode_index = usage->usage_index;
-		} else {
-			/*
-			 * Some elan panels wrongly declare 2 input mode
-			 * features, and silently ignore when we set the
-			 * value in the second field. Skip the second feature
-			 * and hope for the best.
-			 */
-			dev_info(&hdev->dev,
-				 "Ignoring the extra HID_DG_INPUTMODE\n");
-		}
-
-		break;
 	case HID_DG_CONTACTMAX:
 		mt_get_feature(hdev, field->report);
 
-		td->maxcontact_report_id = field->report->id;
 		td->maxcontacts = field->value[0];
 		if (!td->maxcontacts &&
 		    field->logical_maximum <= MT_MAX_MAXCONTACT)
@@ -1181,61 +1154,81 @@ static void mt_report(struct hid_device *hid, struct hid_report *report)
 		input_sync(field->hidinput->input);
 }
 
-static void mt_set_input_mode(struct hid_device *hdev)
+static bool mt_need_to_apply_feature(struct hid_device *hdev,
+				     struct hid_field *field,
+				     struct hid_usage *usage)
 {
 	struct mt_device *td = hid_get_drvdata(hdev);
-	struct hid_report *r;
-	struct hid_report_enum *re;
 	struct mt_class *cls = &td->mtclass;
+	struct hid_report *report = field->report;
+	unsigned int index = usage->usage_index;
 	char *buf;
 	u32 report_len;
+	int max;
 
-	if (td->inputmode < 0)
-		return;
-
-	re = &(hdev->report_enum[HID_FEATURE_REPORT]);
-	r = re->report_id_hash[td->inputmode];
-	if (r) {
+	switch (usage->hid) {
+	case HID_DG_INPUTMODE:
 		if (cls->quirks & MT_QUIRK_FORCE_GET_FEATURE) {
-			report_len = hid_report_len(r);
-			buf = hid_alloc_report_buf(r, GFP_KERNEL);
+			report_len = hid_report_len(report);
+			buf = hid_alloc_report_buf(report, GFP_KERNEL);
 			if (!buf) {
-				hid_err(hdev, "failed to allocate buffer for report\n");
-				return;
+				hid_err(hdev,
+					"failed to allocate buffer for report\n");
+				return false;
 			}
-			hid_hw_raw_request(hdev, r->id, buf, report_len,
+			hid_hw_raw_request(hdev, report->id, buf, report_len,
 					   HID_FEATURE_REPORT,
 					   HID_REQ_GET_REPORT);
 			kfree(buf);
 		}
-		r->field[0]->value[td->inputmode_index] = td->inputmode_value;
-		hid_hw_request(hdev, r, HID_REQ_SET_REPORT);
-	}
-}
 
-static void mt_set_maxcontacts(struct hid_device *hdev)
-{
-	struct mt_device *td = hid_get_drvdata(hdev);
-	struct hid_report *r;
-	struct hid_report_enum *re;
-	int fieldmax, max;
+		field->value[index] = td->inputmode_value;
+		return true;
 
-	if (td->maxcontact_report_id < 0)
-		return;
+	case HID_DG_CONTACTMAX:
+		if (td->mtclass.maxcontacts) {
+			max = min_t(int, field->logical_maximum,
+				    td->mtclass.maxcontacts);
+			if (field->value[index] != max) {
+				field->value[index] = max;
+				return true;
+			}
+		}
+		break;
+	}
 
-	if (!td->mtclass.maxcontacts)
-		return;
+	return false; /* no need to update the report */
+}
 
-	re = &hdev->report_enum[HID_FEATURE_REPORT];
-	r = re->report_id_hash[td->maxcontact_report_id];
-	if (r) {
-		max = td->mtclass.maxcontacts;
-		fieldmax = r->field[0]->logical_maximum;
-		max = min(fieldmax, max);
-		if (r->field[0]->value[0] != max) {
-			r->field[0]->value[0] = max;
-			hid_hw_request(hdev, r, HID_REQ_SET_REPORT);
+static void mt_set_modes(struct hid_device *hdev)
+{
+	struct hid_report_enum *rep_enum;
+	struct hid_report *rep;
+	struct hid_usage *usage;
+	int i, j;
+	bool update_report;
+
+	rep_enum = &hdev->report_enum[HID_FEATURE_REPORT];
+	list_for_each_entry(rep, &rep_enum->report_list, list) {
+		update_report = false;
+
+		for (i = 0; i < rep->maxfield; i++) {
+			/* Ignore if report count is out of bounds. */
+			if (rep->field[i]->report_count < 1)
+				continue;
+
+			for (j = 0; j < rep->field[i]->maxusage; j++) {
+				usage = &rep->field[i]->usage[j];
+
+				if (mt_need_to_apply_feature(hdev,
+							     rep->field[i],
+							     usage))
+					update_report = true;
+			}
 		}
+
+		if (update_report)
+			hid_hw_request(hdev, rep, HID_REQ_SET_REPORT);
 	}
 }
 
@@ -1428,8 +1421,6 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	}
 	td->hdev = hdev;
 	td->mtclass = *mtclass;
-	td->inputmode = -1;
-	td->maxcontact_report_id = -1;
 	td->inputmode_value = MT_INPUTMODE_TOUCHSCREEN;
 	td->cc_index = -1;
 	td->scantime_index = -1;
@@ -1476,8 +1467,7 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
 		dev_warn(&hdev->dev, "Cannot allocate sysfs group for %s\n",
 				hdev->name);
 
-	mt_set_maxcontacts(hdev);
-	mt_set_input_mode(hdev);
+	mt_set_modes(hdev);
 
 	/* release .fields memory as it is not used anymore */
 	devm_kfree(&hdev->dev, td->fields);
@@ -1490,8 +1480,7 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
 static int mt_reset_resume(struct hid_device *hdev)
 {
 	mt_release_contacts(hdev);
-	mt_set_maxcontacts(hdev);
-	mt_set_input_mode(hdev);
+	mt_set_modes(hdev);
 	return 0;
 }
 
-- 
2.14.3

[PATCH v2 6/6] HID: multitouch: implement precision touchpad latency and switches

[Benjamin Tissoires]

The Win 8.1 precision touchpad spec introduce new modes for touchpads
that can come in handy[1].

Implement the settings of these modes, so we are not taken off-guard if
a firmware decides to enforce them.

[1] https://docs.microsoft.com/en-us/windows-hardware/design/component-guidelines/windows-precision-touchpad-required-hid-top-level-collections

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/hid-multitouch.c | 34 +++++++++++++++++++++++++++++-----
 include/linux/hid.h          |  3 +++
 2 files changed, 32 insertions(+), 5 deletions(-)

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 8878de9eedba..82c98bf14d60 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -81,6 +81,11 @@ MODULE_LICENSE("GPL");
 
 #define MT_BUTTONTYPE_CLICKPAD		0
 
+enum latency_mode {
+	HID_LATENCY_NORMAL = 0,
+	HID_LATENCY_HIGH = 1,
+};
+
 #define MT_IO_FLAGS_RUNNING		0
 #define MT_IO_FLAGS_ACTIVE_SLOTS	1
 #define MT_IO_FLAGS_PENDING_SLOTS	2
@@ -1156,7 +1161,10 @@ static void mt_report(struct hid_device *hid, struct hid_report *report)
 
 static bool mt_need_to_apply_feature(struct hid_device *hdev,
 				     struct hid_field *field,
-				     struct hid_usage *usage)
+				     struct hid_usage *usage,
+				     enum latency_mode latency,
+				     bool surface_switch,
+				     bool button_switch)
 {
 	struct mt_device *td = hid_get_drvdata(hdev);
 	struct mt_class *cls = &td->mtclass;
@@ -1195,12 +1203,25 @@ static bool mt_need_to_apply_feature(struct hid_device *hdev,
 			}
 		}
 		break;
+
+	case HID_DG_LATENCYMODE:
+		field->value[index] = latency;
+		return 1;
+
+	case HID_DG_SURFACESWITCH:
+		field->value[index] = surface_switch;
+		return 1;
+
+	case HID_DG_BUTTONSWITCH:
+		field->value[index] = button_switch;
+		return 1;
 	}
 
 	return false; /* no need to update the report */
 }
 
-static void mt_set_modes(struct hid_device *hdev)
+static void mt_set_modes(struct hid_device *hdev, enum latency_mode latency,
+			 bool surface_switch, bool button_switch)
 {
 	struct hid_report_enum *rep_enum;
 	struct hid_report *rep;
@@ -1222,7 +1243,10 @@ static void mt_set_modes(struct hid_device *hdev)
 
 				if (mt_need_to_apply_feature(hdev,
 							     rep->field[i],
-							     usage))
+							     usage,
+							     latency,
+							     surface_switch,
+							     button_switch))
 					update_report = true;
 			}
 		}
@@ -1467,7 +1491,7 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
 		dev_warn(&hdev->dev, "Cannot allocate sysfs group for %s\n",
 				hdev->name);
 
-	mt_set_modes(hdev);
+	mt_set_modes(hdev, HID_LATENCY_NORMAL, true, true);
 
 	/* release .fields memory as it is not used anymore */
 	devm_kfree(&hdev->dev, td->fields);
@@ -1480,7 +1504,7 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
 static int mt_reset_resume(struct hid_device *hdev)
 {
 	mt_release_contacts(hdev);
-	mt_set_modes(hdev);
+	mt_set_modes(hdev, HID_LATENCY_NORMAL, true, true);
 	return 0;
 }
 
diff --git a/include/linux/hid.h b/include/linux/hid.h
index d3bd83d9127f..d65ce4ab82d6 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -292,9 +292,12 @@ struct hid_item {
 #define HID_DG_CONTACTCOUNT	0x000d0054
 #define HID_DG_CONTACTMAX	0x000d0055
 #define HID_DG_SCANTIME		0x000d0056
+#define HID_DG_SURFACESWITCH	0x000d0057
+#define HID_DG_BUTTONSWITCH	0x000d0058
 #define HID_DG_BUTTONTYPE	0x000d0059
 #define HID_DG_BARRELSWITCH2	0x000d005a
 #define HID_DG_TOOLSERIALNUMBER	0x000d005b
+#define HID_DG_LATENCYMODE	0x000d0060
 
 #define HID_VD_ASUS_CUSTOM_MEDIA_KEYS	0xff310076
 /*
-- 
2.14.3

Re: [PATCH v2 0/6] HID: input cleanups and mt additions

[Jiri Kosina]

On Tue, 24 Apr 2018, Benjamin Tissoires wrote:

> following the thread about the 'not incrementing ABS_MISC', here is the
> actual submission of the series.

Thanks Benjamin, this looks nice. I've queued it for 4.18.

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH v2 2/6] HID: generic: create one input report per application type

[Dmitry Torokhov]

Hi Benjamin, Jiri,

On Tue, Apr 24, 2018 at 1:04 AM Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
>
> It is not a good idea to try to fit all types of applications in the
> same input report. There are a lot of devices that are needing
> the quirk HID_MULTI_INPUT but this quirk doesn't match the actual HID
> description as it is based on the report ID.
>
> Given that most devices with MULTI_INPUT I can think of split nicely
> the devices inputs into application, it is a good thing to split the
> devices by default based on this assumption.
>
> Also make hid-multitouch following this rule, to not have to deal
> with too many input created.
>
> While we are at it, fix some checkpatch complaints about converting
> 'unsigned' to 'unsigned int'.
>
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> ---

I see a spike of user reports on Bugzilla with HID devices being
broken on 4.18 that seem to point to this commit. For example:
https://bugzilla.kernel.org/show_bug.cgi?id=200849

Thanks.

-- 
Dmitry

Re: [PATCH v2 2/6] HID: generic: create one input report per application type

[Benjamin Tissoires]

On Fri, Aug 24, 2018 at 5:45 PM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> Hi Benjamin, Jiri,
>
> On Tue, Apr 24, 2018 at 1:04 AM Benjamin Tissoires
> <benjamin.tissoires@redhat.com> wrote:
> >
> > It is not a good idea to try to fit all types of applications in the
> > same input report. There are a lot of devices that are needing
> > the quirk HID_MULTI_INPUT but this quirk doesn't match the actual HID
> > description as it is based on the report ID.
> >
> > Given that most devices with MULTI_INPUT I can think of split nicely
> > the devices inputs into application, it is a good thing to split the
> > devices by default based on this assumption.
> >
> > Also make hid-multitouch following this rule, to not have to deal
> > with too many input created.
> >
> > While we are at it, fix some checkpatch complaints about converting
> > 'unsigned' to 'unsigned int'.
> >
> > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > ---
>
> I see a spike of user reports on Bugzilla with HID devices being
> broken on 4.18 that seem to point to this commit. For example:
> https://bugzilla.kernel.org/show_bug.cgi?id=200849

Thanks Dmitry. I have requested logs from the reporters. It is
possible that the device will get split in 2: one for the pointer and
one for the buttons, and systemd would have no clue on how to handle
those cases. Depending on the logs, I'll see how we can fix that in a
clean way.

Cheers,
Benjamin