From 190d7f02ce8ef6774a69d3ec18c288c8a9601a4e Mon Sep 17 00:00:00 2001 From: Benjamin Tissoires Date: Fri, 8 Dec 2017 15:28:18 +0100 Subject: HID: input: do not increment usages when a duplicate is found This is something that bothered us from a long time. When hid-input doesn't know how to map a usage, it uses *_MISC. But there is something else which increments the usage if the evdev code is already used. This leads to few issues: - some devices may have their ABS_X mapped to ABS_Y if they export a bad set of usages (see the DragonRise joysticks IIRC -> fixed in a specific HID driver) - *_MISC + N might (will) conflict with other defined axes (my Logitech H800 exports some multitouch axes because of that) - this prevents to freely add some new evdev usages, because "hey, my headset will now report ABS_COFFEE, and it's not coffee capable". So let's try to kill this nonsense, and hope we won't break too many devices. I my headset case, the ABS_MISC axes are created because of some proprietary usages, so we might not break that many devices. For backward compatibility, a quirk HID_QUIRK_INCREMENT_USAGE_ON_DUPLICATE is created and can be applied to any device that needs this behavior. Signed-off-by: Benjamin Tissoires Acked-by: Peter Hutterer Acked-by: Dmitry Torokhov Signed-off-by: Jiri Kosina --- drivers/hid/hid-input.c | 33 +++++++++++++++++++++++++++++++-- 1 file changed, 31 insertions(+), 2 deletions(-) (limited to 'drivers/hid') diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c index 6836a856c243..04056773102e 100644 --- a/drivers/hid/hid-input.c +++ b/drivers/hid/hid-input.c @@ -1100,8 +1100,31 @@ mapped: set_bit(usage->type, input->evbit); - while (usage->code <= max && test_and_set_bit(usage->code, bit)) - usage->code = find_next_zero_bit(bit, max + 1, usage->code); + /* + * This part is *really* controversial: + * - HID aims at being generic so we should do our best to export + * all incoming events + * - HID describes what events are, so there is no reason for ABS_X + * to be mapped to ABS_Y + * - HID is using *_MISC+N as a default value, but nothing prevents + * *_MISC+N to overwrite a legitimate even, which confuses userspace + * (for instance ABS_MISC + 7 is ABS_MT_SLOT, which has a different + * processing) + * + * If devices still want to use this (at their own risk), they will + * have to use the quirk HID_QUIRK_INCREMENT_USAGE_ON_DUPLICATE, but + * the default should be a reliable mapping. + */ + while (usage->code <= max && test_and_set_bit(usage->code, bit)) { + if (device->quirks & HID_QUIRK_INCREMENT_USAGE_ON_DUPLICATE) { + usage->code = find_next_zero_bit(bit, + max + 1, + usage->code); + } else { + device->status |= HID_STAT_DUP_DETECTED; + goto ignore; + } + } if (usage->code > max) goto ignore; @@ -1611,6 +1634,8 @@ int hidinput_connect(struct hid_device *hid, unsigned int force) INIT_LIST_HEAD(&hid->inputs); INIT_WORK(&hid->led_work, hidinput_led_worker); + hid->status &= ~HID_STAT_DUP_DETECTED; + if (!force) { for (i = 0; i < hid->maxcollection; i++) { struct hid_collection *col = &hid->collection[i]; @@ -1677,6 +1702,10 @@ int hidinput_connect(struct hid_device *hid, unsigned int force) goto out_unwind; } + if (hid->status & HID_STAT_DUP_DETECTED) + hid_dbg(hid, + "Some usages could not be mapped, please use HID_QUIRK_INCREMENT_USAGE_ON_DUPLICATE if this is legitimate.\n"); + return 0; out_unwind: -- cgit v1.2.3 From e1b63c0148a7f8edf1691770ec0527fe86fb6ab8 Mon Sep 17 00:00:00 2001 From: Benjamin Tissoires Date: Tue, 24 Apr 2018 10:04:32 +0200 Subject: HID: store the full list of reports in the hidinput 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 Signed-off-by: Jiri Kosina --- drivers/hid/hid-input.c | 6 ++++++ include/linux/hid.h | 2 ++ 2 files changed, 8 insertions(+) (limited to 'drivers/hid') diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c index 04056773102e..fd1c4fe70327 100644 --- a/drivers/hid/hid-input.c +++ b/drivers/hid/hid-input.c @@ -1526,9 +1526,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; } @@ -1678,6 +1681,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 0267aa5c1ea3..396068ccc197 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 { -- cgit v1.2.3 From f07b3c1da92db108662f99417a212fc1eddc44d1 Mon Sep 17 00:00:00 2001 From: Benjamin Tissoires Date: Tue, 24 Apr 2018 10:04:33 +0200 Subject: HID: generic: create one input report per application type 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 Signed-off-by: Jiri Kosina --- 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(-) (limited to 'drivers/hid') 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 fd1c4fe70327..7463ee2a1df2 100644 --- a/drivers/hid/hid-input.c +++ b/drivers/hid/hid-input.c @@ -1610,6 +1610,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) { @@ -1670,6 +1684,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 396068ccc197..bcc91bfdd2cb 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) */ @@ -861,7 +863,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, -- cgit v1.2.3 From c554bb045511bd6b498b6a61cffa48e473853703 Mon Sep 17 00:00:00 2001 From: Benjamin Tissoires Date: Tue, 24 Apr 2018 10:04:34 +0200 Subject: HID: input: append a suffix matching the application 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 Signed-off-by: Jiri Kosina --- drivers/hid/hid-input.c | 67 +++++++++++++++++++++++++++++++++++++++++++------ include/linux/hid.h | 1 + 2 files changed, 60 insertions(+), 8 deletions(-) (limited to 'drivers/hid') diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c index 7463ee2a1df2..fea6d4898f15 100644 --- a/drivers/hid/hid-input.c +++ b/drivers/hid/hid-input.c @@ -1500,15 +1500,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); @@ -1518,7 +1559,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; @@ -1533,6 +1574,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) @@ -1578,6 +1625,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 && @@ -1646,6 +1694,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); @@ -1678,6 +1727,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. @@ -1689,7 +1740,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 bcc91bfdd2cb..f03d7a410c5d 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 */ }; -- cgit v1.2.3 From 40ec260363b2d3ca5b2e8d1a68c22bd609243411 Mon Sep 17 00:00:00 2001 From: Benjamin Tissoires Date: Tue, 24 Apr 2018 10:04:35 +0200 Subject: HID: multitouch: make use of HID_QUIRK_INPUT_PER_APP 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 Signed-off-by: Jiri Kosina --- drivers/hid/hid-multitouch.c | 72 ++++++++++++++++++++------------------------ 1 file changed, 33 insertions(+), 39 deletions(-) (limited to 'drivers/hid') 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); -- cgit v1.2.3 From 7f81c8db54898a793cc2916a936f6bf3fca41434 Mon Sep 17 00:00:00 2001 From: Benjamin Tissoires Date: Tue, 24 Apr 2018 10:04:36 +0200 Subject: HID: multitouch: simplify the settings of the various features 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 Signed-off-by: Jiri Kosina --- drivers/hid/hid-multitouch.c | 131 ++++++++++++++++++++----------------------- 1 file changed, 60 insertions(+), 71 deletions(-) (limited to 'drivers/hid') 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; } -- cgit v1.2.3 From 02946f4b43b11026b1a76857a33b09078b900939 Mon Sep 17 00:00:00 2001 From: Benjamin Tissoires Date: Tue, 24 Apr 2018 10:04:37 +0200 Subject: HID: multitouch: implement precision touchpad latency and switches 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 Signed-off-by: Jiri Kosina --- drivers/hid/hid-multitouch.c | 34 +++++++++++++++++++++++++++++----- include/linux/hid.h | 3 +++ 2 files changed, 32 insertions(+), 5 deletions(-) (limited to 'drivers/hid') 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 f03d7a410c5d..a1be991e1eae 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 /* -- cgit v1.2.3 From 99c703acade3e38c07b179d684045e9099f935e9 Mon Sep 17 00:00:00 2001 From: Jiri Kosina Date: Wed, 16 May 2018 11:02:07 +0200 Subject: HID: multitouch: fix types returned from mt_need_to_apply_feature() Some exit paths from mt_need_to_apply_feature() returned int instead of bool; fix that up. Reported-by: kbuild test robot Signed-off-by: Jiri Kosina --- drivers/hid/hid-multitouch.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'drivers/hid') diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c index 82c98bf14d60..38c4ca24343e 100644 --- a/drivers/hid/hid-multitouch.c +++ b/drivers/hid/hid-multitouch.c @@ -1206,15 +1206,15 @@ static bool mt_need_to_apply_feature(struct hid_device *hdev, case HID_DG_LATENCYMODE: field->value[index] = latency; - return 1; + return true; case HID_DG_SURFACESWITCH: field->value[index] = surface_switch; - return 1; + return true; case HID_DG_BUTTONSWITCH: field->value[index] = button_switch; - return 1; + return true; } return false; /* no need to update the report */ -- cgit v1.2.3 From abb36fe691b28f2a64926b61448d6b9610ed879a Mon Sep 17 00:00:00 2001 From: Ben Chan Date: Tue, 29 May 2018 15:56:55 -0700 Subject: HID: multitouch: fix calculation of last slot field in multi-touch reports According to [1] and also seemingly agreed by [2], the Scan Time usage (0x0D 0x56) is a report level usage, not a contact level usage. However, the hid-multitouch driver currently includes HID_DG_SCANTIME when calculating `td->last_slot_field', which may lead to mt_complete_slot() being prematurely called in certain cases (e.g. when each touch input report includes more than one contact and the Scan Time usage appears before any contact logical collection). This patch fixes the issue by skipping mt_store_field() on HID_DG_SCANTIME, similar to how HID_DG_CONTACTCOUNT and HID_DG_CONTACTMAX are handled. [1] https://docs.microsoft.com/en-us/windows-hardware/design/component-guidelines/windows-precision-touchpad-required-hid-top-level-collections#windows-precision-touchpad-input-reports [2] https://patchwork.kernel.org/patch/1742181/ Fixes: 29cc309d8bf19 ("HID: hid-multitouch: forward MSC_TIMESTAMP") Signed-off-by: Ben Chan Reviewed-by: Dmitry Torokhov Signed-off-by: Jiri Kosina --- drivers/hid/hid-multitouch.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) (limited to 'drivers/hid') diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c index 38c4ca24343e..45968f7970f8 100644 --- a/drivers/hid/hid-multitouch.c +++ b/drivers/hid/hid-multitouch.c @@ -598,13 +598,16 @@ static int mt_touch_input_mapping(struct hid_device *hdev, struct hid_input *hi, hid_map_usage(hi, usage, bit, max, EV_MSC, MSC_TIMESTAMP); input_set_capability(hi->input, EV_MSC, MSC_TIMESTAMP); - mt_store_field(usage, td, hi); /* Ignore if indexes are out of bounds. */ if (field->index >= field->report->maxfield || usage->usage_index >= field->report_count) return 1; td->scantime_index = field->index; td->scantime_val_index = usage->usage_index; + /* + * We don't set td->last_slot_field as scan time is + * global to the report. + */ return 1; case HID_DG_CONTACTCOUNT: /* Ignore if indexes are out of bounds. */ -- cgit v1.2.3