* HID: Add driver for synaptics keybard with broken rdesc
@ 2015-01-23 19:03 Simon Wörner
2015-01-23 19:03 ` [PATCH] " Simon Wörner
0 siblings, 1 reply; 14+ messages in thread
From: Simon Wörner @ 2015-01-23 19:03 UTC (permalink / raw)
To: Jiri Kosina; +Cc: linux-input
Synaptics keyboards (USB ID 06cb:2968) e.g. in Acer SW5-012
have the following issue:
- The report descriptor specifies an excessively large number of consumer
usages (2^15), which is more than HID_MAX_USAGES. This prevents proper
parsing of the report descriptor.
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH] HID: Add driver for synaptics keybard with broken rdesc
2015-01-23 19:03 HID: Add driver for synaptics keybard with broken rdesc Simon Wörner
@ 2015-01-23 19:03 ` Simon Wörner
2015-01-27 10:17 ` Jiri Kosina
2015-01-27 14:16 ` Benjamin Tissoires
0 siblings, 2 replies; 14+ messages in thread
From: Simon Wörner @ 2015-01-23 19:03 UTC (permalink / raw)
To: Jiri Kosina; +Cc: linux-input, Simon Wörner
From: Simon Wörner <mail@simon-woerner.de>
Signed-off-by: Simon Wörner <mail@simon-woerner.de>
---
drivers/hid/Kconfig | 6 ++
drivers/hid/Makefile | 1 +
drivers/hid/hid-ids.h | 1 +
drivers/hid/hid-synaptics.c | 190 ++++++++++++++++++++++++++++++++++++++++++++
4 files changed, 198 insertions(+)
create mode 100644 drivers/hid/hid-synaptics.c
diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index dfdc269..9f4124e 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -708,6 +708,12 @@ config HID_SUNPLUS
---help---
Support for Sunplus wireless desktop.
+config HID_SYNAPTICS
+ tristate "Synaptics keyboard support"
+ depends on HID
+ ---help---
+ Support for Synaptics keyboard with invalid rdesc
+
config HID_RMI
tristate "Synaptics RMI4 device support"
depends on HID
diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
index debd15b..01bda39 100644
--- a/drivers/hid/Makefile
+++ b/drivers/hid/Makefile
@@ -109,6 +109,7 @@ obj-$(CONFIG_HID_SONY) += hid-sony.o
obj-$(CONFIG_HID_SPEEDLINK) += hid-speedlink.o
obj-$(CONFIG_HID_STEELSERIES) += hid-steelseries.o
obj-$(CONFIG_HID_SUNPLUS) += hid-sunplus.o
+obj-$(CONFIG_HID_SYNAPTICS) += hid-synaptics.o
obj-$(CONFIG_HID_GREENASIA) += hid-gaff.o
obj-$(CONFIG_HID_THINGM) += hid-thingm.o
obj-$(CONFIG_HID_THRUSTMASTER) += hid-tmff.o
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index 9243359..976ab39 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -873,6 +873,7 @@
#define USB_DEVICE_ID_SYNAPTICS_LTS2 0x1d10
#define USB_DEVICE_ID_SYNAPTICS_HD 0x0ac3
#define USB_DEVICE_ID_SYNAPTICS_QUAD_HD 0x1ac3
+#define USB_VENDOR_ID_SYNAPTICS_KBD 0x2968
#define USB_DEVICE_ID_SYNAPTICS_TP_V103 0x5710
#define USB_VENDOR_ID_TEXAS_INSTRUMENTS 0x2047
diff --git a/drivers/hid/hid-synaptics.c b/drivers/hid/hid-synaptics.c
new file mode 100644
index 0000000..3dab405
--- /dev/null
+++ b/drivers/hid/hid-synaptics.c
@@ -0,0 +1,190 @@
+/*
+ * HID driver for synaptics devices
+ *
+ * Copyright (c) 2015 Simon Wörner
+ */
+
+/*
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the Free
+ * Software Foundation; either version 2 of the License, or (at your option)
+ * any later version.
+ */
+
+#include <linux/device.h>
+#include <linux/hid.h>
+#include <linux/module.h>
+#include <linux/usb.h>
+
+#include "hid-ids.h"
+
+/* Synaptics keyboards (USB ID 06cb:2968) e.g. in Acer SW5-012
+ * have the following issue:
+ * - The report descriptor specifies an excessively large number of consumer
+ * usages (2^15), which is more than HID_MAX_USAGES. This prevents proper
+ * parsing of the report descriptor.
+ *
+ * The replacement descriptor below fixes the number of consumer usages.
+ */
+
+static __u8 synaptics_kbd_rdesc_fixed[] = {
+ /* Application Collection */
+ 0x06, 0x85, 0xFF, /* (GLOBAL) USAGE_PAGE (Vendor-defined) */
+ 0x09, 0x95, /* (LOCAL) USAGE (0xFF850095) */
+ 0xA1, 0x01, /* (MAIN) COLLECTION (Application) */
+ 0x85, 0x5A, /* (GLOBAL) REPORT_ID (0x5A (90) 'Z') */
+ 0x09, 0x01, /* (LOCAL) USAGE (0xFF850001) */
+ 0x26, 0xFF, 0x00, /* (GLOBAL) LOGICAL_MAXIMUM 0x00FF (255) */
+ 0x75, 0x08, /* (GLOBAL) REPORT_SIZE 0x08 (8) */
+ 0x95, 0x10, /* (GLOBAL) REPORT_COUNT 0x10 (16) */
+ 0xB1, 0x00, /* (MAIN) FEATURE 0x00 */
+ 0xC0, /* (MAIN) END_COLLECTION (Application) */
+
+ /* Application Collection */
+ 0x05, 0x01, /* (GLOBAL) USAGE_PAGE (Desktop) */
+ 0x09, 0x06, /* (LOCAL) USAGE 0x06 (Keyboard) */
+ 0xA1, 0x01, /* (MAIN) COLLECTION (Application) */
+ 0x85, 0x01, /* (GLOBAL) REPORT_ID 0x01 (1) */
+ 0x75, 0x01, /* (GLOBAL) REPORT_SIZE 0x01 (1) */
+ 0x95, 0x08, /* (GLOBAL) REPORT_COUNT 0x08 (8) */
+ 0x05, 0x07, /* (GLOBAL) USAGE_PAGE 0x07 (Keyboard) */
+ 0x19, 0xE0, /* (LOCAL) USAGE_MINIMUM 0xE0 */
+ 0x29, 0xE7, /* (LOCAL) USAGE_MAXIMUM 0xE7 */
+ 0x25, 0x01, /* (GLOBAL) LOGICAL_MAXIMUM 0x01 (1) */
+ 0x81, 0x02, /* (MAIN) INPUT 0x02 */
+ 0x95, 0x01, /* (GLOBAL) REPORT_COUNT 0x01 (1) */
+ 0x75, 0x08, /* (GLOBAL) REPORT_SIZE 0x08 (8) */
+ 0x81, 0x03, /* (MAIN) INPUT 0x03 */
+ 0x95, 0x05, /* (GLOBAL) REPORT_COUNT 0x05 (5) */
+ 0x75, 0x01, /* (GLOBAL) REPORT_SIZE 0x01 (1) */
+ 0x05, 0x08, /* (GLOBAL) USAGE_PAGE 0x08 (LED Indicator) */
+ 0x19, 0x01, /* (LOCAL) USAGE_MINIMUM 0x01 */
+ 0x29, 0x05, /* (LOCAL) USAGE_MAXIMUM 0x05 */
+ 0x91, 0x02, /* (MAIN) OUTPUT 0x02 */
+ 0x95, 0x01, /* (GLOBAL) REPORT_COUNT 0x01 (1) */
+ 0x75, 0x03, /* (GLOBAL) REPORT_SIZE 0x03 (3) */
+ 0x91, 0x03, /* (MAIN) OUTPUT 0x03 */
+ 0x95, 0x06, /* (GLOBAL) REPORT_COUNT 0x06 (6) */
+ 0x75, 0x08, /* (GLOBAL) REPORT_SIZE 0x08 (8) */
+ 0x26, 0xFF, 0x00, /* (GLOBAL) LOGICAL_MAXIMUM 0x00FF (255) */
+ 0x05, 0x07, /* (GLOBAL) USAGE_PAGE 0x0007 (Keyboard) */
+ 0x19, 0x00, /* (LOCAL) USAGE_MINIMUM 0x00 */
+ 0x2A, 0xFF, 0x00, /* (LOCAL) USAGE_MAXIMUM 0xFF */
+ 0x81, 0x00, /* (MAIN) INPUT 0x00 */
+ 0xC0, /* (MAIN) END_COLLECTION (Application) */
+
+ /* Application Collection */
+ 0x05, 0x0C, /* (GLOBAL) USAGE_PAGE (Consumer) */
+ 0x09, 0x01, /* (LOCAL) USAGE 0x01 (Consumer Control) */
+ 0xA1, 0x01, /* (MAIN) COLLECTION (Application) */
+ 0x85, 0x02, /* (GLOBAL) REPORT_ID 0x02 (2) */
+ 0x19, 0x00, /* (LOCAL) USAGE_MINIMUM 0x00 */
+ 0x2A, 0x3C, 0x02, /* (LOCAL) USAGE_MAXIMUM 0x023C */
+ 0x26, 0x3C, 0x02, /* (GLOBAL) LOGICAL_MAXIMUM 0x023C (572) */
+ 0x75, 0x10, /* (GLOBAL) REPORT_SIZE 0x10 (16) */
+ 0x95, 0x01, /* (GLOBAL) REPORT_COUNT 0x01 (1) */
+ 0x81, 0x00, /* (MAIN) INPUT 0x00 */
+ 0xC0, /* (MAIN) END_COLLECTION (Application) */
+
+ /* Application Collection */
+ 0x05, 0x01, /* (GLOBAL) USAGE_PAGE (Desktop) */
+ 0x09, 0x0C, /* (LOCAL) USAGE (Wireless Radio Controls) */
+ 0xA1, 0x01, /* (MAIN) COLLECTION (Application) */
+ 0x85, 0x03, /* (GLOBAL) REPORT_ID 0x03 (3) */
+ 0x25, 0x01, /* (GLOBAL) LOGICAL_MAXIMUM 0x01 (1) */
+ 0x09, 0xC6, /* (LOCAL) USAGE 0xC6 */
+ 0x75, 0x01, /* (GLOBAL) REPORT_SIZE 0x01 (1) */
+ 0x81, 0x06, /* (MAIN) INPUT 0x06 */
+ 0x75, 0x07, /* (GLOBAL) REPORT_SIZE 0x07 (7) */
+ 0x81, 0x03, /* (MAIN) INPUT 0x03 */
+ 0xC0, /* (MAIN) END_COLLECTION (Application) */
+
+ /* Application Collection */
+ 0x05, 0x88, /* (GLOBAL) USAGE_PAGE (0x88) */
+ 0x09, 0x01, /* (LOCAL) USAGE (0x01) */
+ 0xA1, 0x01, /* (MAIN) COLLECTION (Application) */
+ 0x85, 0x04, /* (GLOBAL) REPORT_ID 0x04 (4) */
+ 0x19, 0x00, /* (LOCAL) USAGE_MINIMUM 0x00 */
+ 0x2A, 0xFF, 0x00, /* (LOCAL) USAGE_MAXIMUM 0x00FF */
+ 0x26, 0xFF, 0x00, /* (GLOBAL) LOGICAL_MAXIMUM 0x00FF */
+ 0x75, 0x08, /* (GLOBAL) REPORT_SIZE 0x08 (8) */
+ 0x95, 0x02, /* (GLOBAL) REPORT_COUNT 0x02 (2) */
+ 0x81, 0x02, /* (MAIN) INPUT 0x02 */
+ 0xC0, /* (MAIN) END_COLLECTION (Application) */
+
+ /* Application Collection */
+ 0x05, 0x01, /* (GLOBAL) USAGE_PAGE (Desktop) */
+ 0x09, 0x80, /* (LOCAL) USAGE (System Control) */
+ 0xA1, 0x01, /* (MAIN) COLLECTION (Application) */
+ 0x85, 0x05, /* (GLOBAL) REPORT_ID 0x05 (5) */
+ 0x19, 0x81, /* (LOCAL) USAGE_MINIMUM 0x81 */
+ 0x29, 0x83, /* (LOCAL) USAGE_MAXIMUM 0x83 */
+ 0x25, 0x01, /* (GLOBAL) LOGICAL_MAXIMUM 0x01 (1) */
+ 0x95, 0x08, /* (GLOBAL) REPORT_COUNT 0x08 (8) */
+ 0x75, 0x01, /* (GLOBAL) REPORT_SIZE 0x01 (1) */
+ 0x81, 0x02, /* (MAIN) INPUT 0x02 */
+ 0xC0, /* (MAIN) END_COLLECTION (Application) */
+};
+
+static __u8 *synaptics_kbd_report_fixup(struct hid_device *hdev, __u8 *rdesc,
+ unsigned int *rsize)
+{
+ struct usb_interface *intf = to_usb_interface(hdev->dev.parent);
+
+ if (intf->cur_altsetting->desc.bInterfaceNumber == 0) {
+ rdesc = synaptics_kbd_rdesc_fixed;
+ *rsize = sizeof(synaptics_kbd_rdesc_fixed);
+ }
+ return rdesc;
+}
+
+static int synaptics_probe(struct hid_device *hdev,
+ const struct hid_device_id *id)
+{
+ int ret;
+
+ ret = hid_parse(hdev);
+ if (ret) {
+ hid_err(hdev, "parse failed\n");
+ goto err_free;
+ }
+
+ ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
+ if (ret) {
+ hid_err(hdev, "hw start failed\n");
+ goto err_free;
+ }
+
+ if (ret < 0)
+ goto err_stop;
+
+ return 0;
+err_stop:
+ hid_hw_stop(hdev);
+err_free:
+ return ret;
+}
+
+static void synaptics_remove(struct hid_device *hdev)
+{
+ hid_hw_stop(hdev);
+ kfree(hid_get_drvdata(hdev));
+}
+
+static const struct hid_device_id synaptics_devices[] = {
+ { HID_USB_DEVICE(USB_VENDOR_ID_SYNAPTICS,
+ USB_VENDOR_ID_SYNAPTICS_KBD) },
+ { }
+};
+MODULE_DEVICE_TABLE(hid, synaptics_devices);
+
+static struct hid_driver synaptics_driver = {
+ .name = "synaptics",
+ .id_table = synaptics_devices,
+ .report_fixup = synaptics_kbd_report_fixup,
+ .probe = synaptics_probe,
+ .remove = synaptics_remove,
+};
+module_hid_driver(synaptics_driver);
+
+MODULE_LICENSE("GPL");
--
2.2.2
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] HID: Add driver for synaptics keybard with broken rdesc
2015-01-23 19:03 ` [PATCH] " Simon Wörner
@ 2015-01-27 10:17 ` Jiri Kosina
2015-01-27 14:16 ` Benjamin Tissoires
1 sibling, 0 replies; 14+ messages in thread
From: Jiri Kosina @ 2015-01-27 10:17 UTC (permalink / raw)
To: Simon Wörner; +Cc: linux-input, Simon Wörner
On Fri, 23 Jan 2015, Simon Wörner wrote:
> From: Simon Wörner <mail@simon-woerner.de>
>
Changelog is missing here.
> Signed-off-by: Simon Wörner <mail@simon-woerner.de>
> ---
> drivers/hid/Kconfig | 6 ++
> drivers/hid/Makefile | 1 +
> drivers/hid/hid-ids.h | 1 +
> drivers/hid/hid-synaptics.c | 190 ++++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 198 insertions(+)
> create mode 100644 drivers/hid/hid-synaptics.c
[ ... snip ... ]
> +static int synaptics_probe(struct hid_device *hdev,
> + const struct hid_device_id *id)
> +{
> + int ret;
> +
> + ret = hid_parse(hdev);
> + if (ret) {
> + hid_err(hdev, "parse failed\n");
> + goto err_free;
> + }
> +
> + ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
> + if (ret) {
> + hid_err(hdev, "hw start failed\n");
> + goto err_free;
> + }
> +
> + if (ret < 0)
> + goto err_stop;
> +
> + return 0;
> +err_stop:
> + hid_hw_stop(hdev);
> +err_free:
Any particular reason you call this label err_free?
Otherwise it loosk good. Please resend with these two minor things
clarified/fixed, and I'll apply it.
Thanks,
--
Jiri Kosina
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] HID: Add driver for synaptics keybard with broken rdesc
2015-01-23 19:03 ` [PATCH] " Simon Wörner
2015-01-27 10:17 ` Jiri Kosina
@ 2015-01-27 14:16 ` Benjamin Tissoires
2015-01-27 15:10 ` Simon Wörner
1 sibling, 1 reply; 14+ messages in thread
From: Benjamin Tissoires @ 2015-01-27 14:16 UTC (permalink / raw)
To: Simon Wörner; +Cc: Jiri Kosina, linux-input, Simon Wörner
On Fri, Jan 23, 2015 at 2:03 PM, Simon Wörner <linux@simon-woerner.de> wrote:
> From: Simon Wörner <mail@simon-woerner.de>
>
> Signed-off-by: Simon Wörner <mail@simon-woerner.de>
> ---
> drivers/hid/Kconfig | 6 ++
> drivers/hid/Makefile | 1 +
> drivers/hid/hid-ids.h | 1 +
> drivers/hid/hid-synaptics.c | 190 ++++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 198 insertions(+)
> create mode 100644 drivers/hid/hid-synaptics.c
I am pretty sure you are missing a change in hid-core.c to add your
device to hid_have_special_driver.
Other than that (and the 2comments Jiri made), I am not very happy
with adding a hid-synaptics while we already have a hid-rmi for
synaptics devices. However, the keyboard must not follow the rmi spec,
so this might be just fine.
Few more comments:
>
> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> index dfdc269..9f4124e 100644
> --- a/drivers/hid/Kconfig
> +++ b/drivers/hid/Kconfig
> @@ -708,6 +708,12 @@ config HID_SUNPLUS
> ---help---
> Support for Sunplus wireless desktop.
>
> +config HID_SYNAPTICS
> + tristate "Synaptics keyboard support"
> + depends on HID
> + ---help---
> + Support for Synaptics keyboard with invalid rdesc
> +
> config HID_RMI
> tristate "Synaptics RMI4 device support"
> depends on HID
> diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
> index debd15b..01bda39 100644
> --- a/drivers/hid/Makefile
> +++ b/drivers/hid/Makefile
> @@ -109,6 +109,7 @@ obj-$(CONFIG_HID_SONY) += hid-sony.o
> obj-$(CONFIG_HID_SPEEDLINK) += hid-speedlink.o
> obj-$(CONFIG_HID_STEELSERIES) += hid-steelseries.o
> obj-$(CONFIG_HID_SUNPLUS) += hid-sunplus.o
> +obj-$(CONFIG_HID_SYNAPTICS) += hid-synaptics.o
> obj-$(CONFIG_HID_GREENASIA) += hid-gaff.o
> obj-$(CONFIG_HID_THINGM) += hid-thingm.o
> obj-$(CONFIG_HID_THRUSTMASTER) += hid-tmff.o
> diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
> index 9243359..976ab39 100644
> --- a/drivers/hid/hid-ids.h
> +++ b/drivers/hid/hid-ids.h
> @@ -873,6 +873,7 @@
> #define USB_DEVICE_ID_SYNAPTICS_LTS2 0x1d10
> #define USB_DEVICE_ID_SYNAPTICS_HD 0x0ac3
> #define USB_DEVICE_ID_SYNAPTICS_QUAD_HD 0x1ac3
> +#define USB_VENDOR_ID_SYNAPTICS_KBD 0x2968
> #define USB_DEVICE_ID_SYNAPTICS_TP_V103 0x5710
>
> #define USB_VENDOR_ID_TEXAS_INSTRUMENTS 0x2047
> diff --git a/drivers/hid/hid-synaptics.c b/drivers/hid/hid-synaptics.c
> new file mode 100644
> index 0000000..3dab405
> --- /dev/null
> +++ b/drivers/hid/hid-synaptics.c
> @@ -0,0 +1,190 @@
> +/*
> + * HID driver for synaptics devices
> + *
> + * Copyright (c) 2015 Simon Wörner
> + */
> +
> +/*
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the Free
> + * Software Foundation; either version 2 of the License, or (at your option)
> + * any later version.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/hid.h>
> +#include <linux/module.h>
> +#include <linux/usb.h>
Please avoid using usb.h in HID drivers. HID should be transport agnostic.
> +
> +#include "hid-ids.h"
> +
> +/* Synaptics keyboards (USB ID 06cb:2968) e.g. in Acer SW5-012
> + * have the following issue:
> + * - The report descriptor specifies an excessively large number of consumer
> + * usages (2^15), which is more than HID_MAX_USAGES. This prevents proper
> + * parsing of the report descriptor.
> + *
> + * The replacement descriptor below fixes the number of consumer usages.
> + */
> +
> +static __u8 synaptics_kbd_rdesc_fixed[] = {
> + /* Application Collection */
> + 0x06, 0x85, 0xFF, /* (GLOBAL) USAGE_PAGE (Vendor-defined) */
> + 0x09, 0x95, /* (LOCAL) USAGE (0xFF850095) */
> + 0xA1, 0x01, /* (MAIN) COLLECTION (Application) */
> + 0x85, 0x5A, /* (GLOBAL) REPORT_ID (0x5A (90) 'Z') */
> + 0x09, 0x01, /* (LOCAL) USAGE (0xFF850001) */
> + 0x26, 0xFF, 0x00, /* (GLOBAL) LOGICAL_MAXIMUM 0x00FF (255) */
> + 0x75, 0x08, /* (GLOBAL) REPORT_SIZE 0x08 (8) */
> + 0x95, 0x10, /* (GLOBAL) REPORT_COUNT 0x10 (16) */
> + 0xB1, 0x00, /* (MAIN) FEATURE 0x00 */
> + 0xC0, /* (MAIN) END_COLLECTION (Application) */
> +
> + /* Application Collection */
> + 0x05, 0x01, /* (GLOBAL) USAGE_PAGE (Desktop) */
> + 0x09, 0x06, /* (LOCAL) USAGE 0x06 (Keyboard) */
> + 0xA1, 0x01, /* (MAIN) COLLECTION (Application) */
> + 0x85, 0x01, /* (GLOBAL) REPORT_ID 0x01 (1) */
> + 0x75, 0x01, /* (GLOBAL) REPORT_SIZE 0x01 (1) */
> + 0x95, 0x08, /* (GLOBAL) REPORT_COUNT 0x08 (8) */
> + 0x05, 0x07, /* (GLOBAL) USAGE_PAGE 0x07 (Keyboard) */
> + 0x19, 0xE0, /* (LOCAL) USAGE_MINIMUM 0xE0 */
> + 0x29, 0xE7, /* (LOCAL) USAGE_MAXIMUM 0xE7 */
> + 0x25, 0x01, /* (GLOBAL) LOGICAL_MAXIMUM 0x01 (1) */
> + 0x81, 0x02, /* (MAIN) INPUT 0x02 */
> + 0x95, 0x01, /* (GLOBAL) REPORT_COUNT 0x01 (1) */
> + 0x75, 0x08, /* (GLOBAL) REPORT_SIZE 0x08 (8) */
> + 0x81, 0x03, /* (MAIN) INPUT 0x03 */
> + 0x95, 0x05, /* (GLOBAL) REPORT_COUNT 0x05 (5) */
> + 0x75, 0x01, /* (GLOBAL) REPORT_SIZE 0x01 (1) */
> + 0x05, 0x08, /* (GLOBAL) USAGE_PAGE 0x08 (LED Indicator) */
> + 0x19, 0x01, /* (LOCAL) USAGE_MINIMUM 0x01 */
> + 0x29, 0x05, /* (LOCAL) USAGE_MAXIMUM 0x05 */
> + 0x91, 0x02, /* (MAIN) OUTPUT 0x02 */
> + 0x95, 0x01, /* (GLOBAL) REPORT_COUNT 0x01 (1) */
> + 0x75, 0x03, /* (GLOBAL) REPORT_SIZE 0x03 (3) */
> + 0x91, 0x03, /* (MAIN) OUTPUT 0x03 */
> + 0x95, 0x06, /* (GLOBAL) REPORT_COUNT 0x06 (6) */
> + 0x75, 0x08, /* (GLOBAL) REPORT_SIZE 0x08 (8) */
> + 0x26, 0xFF, 0x00, /* (GLOBAL) LOGICAL_MAXIMUM 0x00FF (255) */
> + 0x05, 0x07, /* (GLOBAL) USAGE_PAGE 0x0007 (Keyboard) */
> + 0x19, 0x00, /* (LOCAL) USAGE_MINIMUM 0x00 */
> + 0x2A, 0xFF, 0x00, /* (LOCAL) USAGE_MAXIMUM 0xFF */
> + 0x81, 0x00, /* (MAIN) INPUT 0x00 */
> + 0xC0, /* (MAIN) END_COLLECTION (Application) */
> +
> + /* Application Collection */
> + 0x05, 0x0C, /* (GLOBAL) USAGE_PAGE (Consumer) */
> + 0x09, 0x01, /* (LOCAL) USAGE 0x01 (Consumer Control) */
> + 0xA1, 0x01, /* (MAIN) COLLECTION (Application) */
> + 0x85, 0x02, /* (GLOBAL) REPORT_ID 0x02 (2) */
> + 0x19, 0x00, /* (LOCAL) USAGE_MINIMUM 0x00 */
> + 0x2A, 0x3C, 0x02, /* (LOCAL) USAGE_MAXIMUM 0x023C */
> + 0x26, 0x3C, 0x02, /* (GLOBAL) LOGICAL_MAXIMUM 0x023C (572) */
> + 0x75, 0x10, /* (GLOBAL) REPORT_SIZE 0x10 (16) */
> + 0x95, 0x01, /* (GLOBAL) REPORT_COUNT 0x01 (1) */
> + 0x81, 0x00, /* (MAIN) INPUT 0x00 */
> + 0xC0, /* (MAIN) END_COLLECTION (Application) */
> +
> + /* Application Collection */
> + 0x05, 0x01, /* (GLOBAL) USAGE_PAGE (Desktop) */
> + 0x09, 0x0C, /* (LOCAL) USAGE (Wireless Radio Controls) */
> + 0xA1, 0x01, /* (MAIN) COLLECTION (Application) */
> + 0x85, 0x03, /* (GLOBAL) REPORT_ID 0x03 (3) */
> + 0x25, 0x01, /* (GLOBAL) LOGICAL_MAXIMUM 0x01 (1) */
> + 0x09, 0xC6, /* (LOCAL) USAGE 0xC6 */
> + 0x75, 0x01, /* (GLOBAL) REPORT_SIZE 0x01 (1) */
> + 0x81, 0x06, /* (MAIN) INPUT 0x06 */
> + 0x75, 0x07, /* (GLOBAL) REPORT_SIZE 0x07 (7) */
> + 0x81, 0x03, /* (MAIN) INPUT 0x03 */
> + 0xC0, /* (MAIN) END_COLLECTION (Application) */
> +
> + /* Application Collection */
> + 0x05, 0x88, /* (GLOBAL) USAGE_PAGE (0x88) */
> + 0x09, 0x01, /* (LOCAL) USAGE (0x01) */
> + 0xA1, 0x01, /* (MAIN) COLLECTION (Application) */
> + 0x85, 0x04, /* (GLOBAL) REPORT_ID 0x04 (4) */
> + 0x19, 0x00, /* (LOCAL) USAGE_MINIMUM 0x00 */
> + 0x2A, 0xFF, 0x00, /* (LOCAL) USAGE_MAXIMUM 0x00FF */
> + 0x26, 0xFF, 0x00, /* (GLOBAL) LOGICAL_MAXIMUM 0x00FF */
> + 0x75, 0x08, /* (GLOBAL) REPORT_SIZE 0x08 (8) */
> + 0x95, 0x02, /* (GLOBAL) REPORT_COUNT 0x02 (2) */
> + 0x81, 0x02, /* (MAIN) INPUT 0x02 */
> + 0xC0, /* (MAIN) END_COLLECTION (Application) */
> +
> + /* Application Collection */
> + 0x05, 0x01, /* (GLOBAL) USAGE_PAGE (Desktop) */
> + 0x09, 0x80, /* (LOCAL) USAGE (System Control) */
> + 0xA1, 0x01, /* (MAIN) COLLECTION (Application) */
> + 0x85, 0x05, /* (GLOBAL) REPORT_ID 0x05 (5) */
> + 0x19, 0x81, /* (LOCAL) USAGE_MINIMUM 0x81 */
> + 0x29, 0x83, /* (LOCAL) USAGE_MAXIMUM 0x83 */
> + 0x25, 0x01, /* (GLOBAL) LOGICAL_MAXIMUM 0x01 (1) */
> + 0x95, 0x08, /* (GLOBAL) REPORT_COUNT 0x08 (8) */
> + 0x75, 0x01, /* (GLOBAL) REPORT_SIZE 0x01 (1) */
> + 0x81, 0x02, /* (MAIN) INPUT 0x02 */
> + 0xC0, /* (MAIN) END_COLLECTION (Application) */
> +};
> +
> +static __u8 *synaptics_kbd_report_fixup(struct hid_device *hdev, __u8 *rdesc,
> + unsigned int *rsize)
> +{
> + struct usb_interface *intf = to_usb_interface(hdev->dev.parent);
> +
> + if (intf->cur_altsetting->desc.bInterfaceNumber == 0) {
Please don't. This is too fragile and we can not use uhid to replay
and test such devices.
Make a check on the size of the original descriptor, or check which
bits you are replacing to know which interface you want to change the
report descriptor.
You can also check on the HID device .type which may be different
among the various interfaces.
> + rdesc = synaptics_kbd_rdesc_fixed;
> + *rsize = sizeof(synaptics_kbd_rdesc_fixed);
> + }
> + return rdesc;
> +}
> +
> +static int synaptics_probe(struct hid_device *hdev,
> + const struct hid_device_id *id)
> +{
> + int ret;
> +
> + ret = hid_parse(hdev);
> + if (ret) {
> + hid_err(hdev, "parse failed\n");
> + goto err_free;
> + }
> +
> + ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
> + if (ret) {
> + hid_err(hdev, "hw start failed\n");
> + goto err_free;
> + }
> +
> + if (ret < 0)
> + goto err_stop;
> +
> + return 0;
> +err_stop:
> + hid_hw_stop(hdev);
> +err_free:
> + return ret;
> +}
I am pretty sure there is no need for a probe here. Hid-core will use
the generic one and you should be fine.
> +
> +static void synaptics_remove(struct hid_device *hdev)
> +{
> + hid_hw_stop(hdev);
> + kfree(hid_get_drvdata(hdev));
> +}
Same here, not mandatory.
Cheers,
Benjamin
> +
> +static const struct hid_device_id synaptics_devices[] = {
> + { HID_USB_DEVICE(USB_VENDOR_ID_SYNAPTICS,
> + USB_VENDOR_ID_SYNAPTICS_KBD) },
> + { }
> +};
> +MODULE_DEVICE_TABLE(hid, synaptics_devices);
> +
> +static struct hid_driver synaptics_driver = {
> + .name = "synaptics",
> + .id_table = synaptics_devices,
> + .report_fixup = synaptics_kbd_report_fixup,
> + .probe = synaptics_probe,
> + .remove = synaptics_remove,
> +};
> +module_hid_driver(synaptics_driver);
> +
> +MODULE_LICENSE("GPL");
> --
> 2.2.2
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] HID: Add driver for synaptics keybard with broken rdesc
2015-01-27 14:16 ` Benjamin Tissoires
@ 2015-01-27 15:10 ` Simon Wörner
2015-01-27 16:13 ` Benjamin Tissoires
0 siblings, 1 reply; 14+ messages in thread
From: Simon Wörner @ 2015-01-27 15:10 UTC (permalink / raw)
To: Benjamin Tissoires, Jiri Kosina; +Cc: linux-input, Simon Wörner
Thanks for the feedback, this is my first commit to the linux kernel so
I don't know much about how the input / hid is working.
I will add the changelog to the patch email, just need to find out how
this works.
Am 27.01.15 um 15:16 schrieb Benjamin Tissoires:
> On Fri, Jan 23, 2015 at 2:03 PM, Simon Wörner <linux@simon-woerner.de> wrote:
>> From: Simon Wörner <mail@simon-woerner.de>
>>
>> Signed-off-by: Simon Wörner <mail@simon-woerner.de>
>> ---
>> drivers/hid/Kconfig | 6 ++
>> drivers/hid/Makefile | 1 +
>> drivers/hid/hid-ids.h | 1 +
>> drivers/hid/hid-synaptics.c | 190 ++++++++++++++++++++++++++++++++++++++++++++
>> 4 files changed, 198 insertions(+)
>> create mode 100644 drivers/hid/hid-synaptics.c
>
> I am pretty sure you are missing a change in hid-core.c to add your
> device to hid_have_special_driver.
>
So I need to add this?
> static const struct hid_device_id hid_have_special_driver[] = {
> ...
> { HID_USB_DEVICE(USB_VENDOR_ID_SYNAPTICS, USB_VENDOR_ID_SYNAPTICS_KBD) },
> ...
do I also need to add
> #if IS_ENABLED(HID_SYNAPTICS)
> ...
> #endif
or will hid-core handle that for me?
> Other than that (and the 2comments Jiri made), I am not very happy
> with adding a hid-synaptics while we already have a hid-rmi for
> synaptics devices. However, the keyboard must not follow the rmi spec,
> so this might be just fine.
>
I have seen the rmi module and haven't found anything they have in
common. The USB device of the keyboard has also attached a mouse /
trackpad, but it also doens't make use of the rmi module.
I don't see any reason for putting both together, e.g. hid-holtek-* has
also two modules for keyboard rdesc fix and mouse rdesc fix.
Maybe hid-synaptics-kbd would better fit?
> Few more comments:
>
>>
>> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
>> index dfdc269..9f4124e 100644
>> --- a/drivers/hid/Kconfig
>> +++ b/drivers/hid/Kconfig
>> @@ -708,6 +708,12 @@ config HID_SUNPLUS
>> ---help---
>> Support for Sunplus wireless desktop.
>>
>> +config HID_SYNAPTICS
>> + tristate "Synaptics keyboard support"
>> + depends on HID
>> + ---help---
>> + Support for Synaptics keyboard with invalid rdesc
>> +
>> config HID_RMI
>> tristate "Synaptics RMI4 device support"
>> depends on HID
>> diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
>> index debd15b..01bda39 100644
>> --- a/drivers/hid/Makefile
>> +++ b/drivers/hid/Makefile
>> @@ -109,6 +109,7 @@ obj-$(CONFIG_HID_SONY) += hid-sony.o
>> obj-$(CONFIG_HID_SPEEDLINK) += hid-speedlink.o
>> obj-$(CONFIG_HID_STEELSERIES) += hid-steelseries.o
>> obj-$(CONFIG_HID_SUNPLUS) += hid-sunplus.o
>> +obj-$(CONFIG_HID_SYNAPTICS) += hid-synaptics.o
>> obj-$(CONFIG_HID_GREENASIA) += hid-gaff.o
>> obj-$(CONFIG_HID_THINGM) += hid-thingm.o
>> obj-$(CONFIG_HID_THRUSTMASTER) += hid-tmff.o
>> diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
>> index 9243359..976ab39 100644
>> --- a/drivers/hid/hid-ids.h
>> +++ b/drivers/hid/hid-ids.h
>> @@ -873,6 +873,7 @@
>> #define USB_DEVICE_ID_SYNAPTICS_LTS2 0x1d10
>> #define USB_DEVICE_ID_SYNAPTICS_HD 0x0ac3
>> #define USB_DEVICE_ID_SYNAPTICS_QUAD_HD 0x1ac3
>> +#define USB_VENDOR_ID_SYNAPTICS_KBD 0x2968
>> #define USB_DEVICE_ID_SYNAPTICS_TP_V103 0x5710
>>
>> #define USB_VENDOR_ID_TEXAS_INSTRUMENTS 0x2047
>> diff --git a/drivers/hid/hid-synaptics.c b/drivers/hid/hid-synaptics.c
>> new file mode 100644
>> index 0000000..3dab405
>> --- /dev/null
>> +++ b/drivers/hid/hid-synaptics.c
>> @@ -0,0 +1,190 @@
>> +/*
>> + * HID driver for synaptics devices
>> + *
>> + * Copyright (c) 2015 Simon Wörner
>> + */
>> +
>> +/*
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms of the GNU General Public License as published by the Free
>> + * Software Foundation; either version 2 of the License, or (at your option)
>> + * any later version.
>> + */
>> +
>> +#include <linux/device.h>
>> +#include <linux/hid.h>
>> +#include <linux/module.h>
>> +#include <linux/usb.h>
>
> Please avoid using usb.h in HID drivers. HID should be transport agnostic.
>
Okay.
>> +
>> +#include "hid-ids.h"
>> +
>> +/* Synaptics keyboards (USB ID 06cb:2968) e.g. in Acer SW5-012
>> + * have the following issue:
>> + * - The report descriptor specifies an excessively large number of consumer
>> + * usages (2^15), which is more than HID_MAX_USAGES. This prevents proper
>> + * parsing of the report descriptor.
>> + *
>> + * The replacement descriptor below fixes the number of consumer usages.
>> + */
>> +
>> +static __u8 synaptics_kbd_rdesc_fixed[] = {
>> + /* Application Collection */
>> + 0x06, 0x85, 0xFF, /* (GLOBAL) USAGE_PAGE (Vendor-defined) */
>> + 0x09, 0x95, /* (LOCAL) USAGE (0xFF850095) */
>> + 0xA1, 0x01, /* (MAIN) COLLECTION (Application) */
>> + 0x85, 0x5A, /* (GLOBAL) REPORT_ID (0x5A (90) 'Z') */
>> + 0x09, 0x01, /* (LOCAL) USAGE (0xFF850001) */
>> + 0x26, 0xFF, 0x00, /* (GLOBAL) LOGICAL_MAXIMUM 0x00FF (255) */
>> + 0x75, 0x08, /* (GLOBAL) REPORT_SIZE 0x08 (8) */
>> + 0x95, 0x10, /* (GLOBAL) REPORT_COUNT 0x10 (16) */
>> + 0xB1, 0x00, /* (MAIN) FEATURE 0x00 */
>> + 0xC0, /* (MAIN) END_COLLECTION (Application) */
>> +
>> + /* Application Collection */
>> + 0x05, 0x01, /* (GLOBAL) USAGE_PAGE (Desktop) */
>> + 0x09, 0x06, /* (LOCAL) USAGE 0x06 (Keyboard) */
>> + 0xA1, 0x01, /* (MAIN) COLLECTION (Application) */
>> + 0x85, 0x01, /* (GLOBAL) REPORT_ID 0x01 (1) */
>> + 0x75, 0x01, /* (GLOBAL) REPORT_SIZE 0x01 (1) */
>> + 0x95, 0x08, /* (GLOBAL) REPORT_COUNT 0x08 (8) */
>> + 0x05, 0x07, /* (GLOBAL) USAGE_PAGE 0x07 (Keyboard) */
>> + 0x19, 0xE0, /* (LOCAL) USAGE_MINIMUM 0xE0 */
>> + 0x29, 0xE7, /* (LOCAL) USAGE_MAXIMUM 0xE7 */
>> + 0x25, 0x01, /* (GLOBAL) LOGICAL_MAXIMUM 0x01 (1) */
>> + 0x81, 0x02, /* (MAIN) INPUT 0x02 */
>> + 0x95, 0x01, /* (GLOBAL) REPORT_COUNT 0x01 (1) */
>> + 0x75, 0x08, /* (GLOBAL) REPORT_SIZE 0x08 (8) */
>> + 0x81, 0x03, /* (MAIN) INPUT 0x03 */
>> + 0x95, 0x05, /* (GLOBAL) REPORT_COUNT 0x05 (5) */
>> + 0x75, 0x01, /* (GLOBAL) REPORT_SIZE 0x01 (1) */
>> + 0x05, 0x08, /* (GLOBAL) USAGE_PAGE 0x08 (LED Indicator) */
>> + 0x19, 0x01, /* (LOCAL) USAGE_MINIMUM 0x01 */
>> + 0x29, 0x05, /* (LOCAL) USAGE_MAXIMUM 0x05 */
>> + 0x91, 0x02, /* (MAIN) OUTPUT 0x02 */
>> + 0x95, 0x01, /* (GLOBAL) REPORT_COUNT 0x01 (1) */
>> + 0x75, 0x03, /* (GLOBAL) REPORT_SIZE 0x03 (3) */
>> + 0x91, 0x03, /* (MAIN) OUTPUT 0x03 */
>> + 0x95, 0x06, /* (GLOBAL) REPORT_COUNT 0x06 (6) */
>> + 0x75, 0x08, /* (GLOBAL) REPORT_SIZE 0x08 (8) */
>> + 0x26, 0xFF, 0x00, /* (GLOBAL) LOGICAL_MAXIMUM 0x00FF (255) */
>> + 0x05, 0x07, /* (GLOBAL) USAGE_PAGE 0x0007 (Keyboard) */
>> + 0x19, 0x00, /* (LOCAL) USAGE_MINIMUM 0x00 */
>> + 0x2A, 0xFF, 0x00, /* (LOCAL) USAGE_MAXIMUM 0xFF */
>> + 0x81, 0x00, /* (MAIN) INPUT 0x00 */
>> + 0xC0, /* (MAIN) END_COLLECTION (Application) */
>> +
>> + /* Application Collection */
>> + 0x05, 0x0C, /* (GLOBAL) USAGE_PAGE (Consumer) */
>> + 0x09, 0x01, /* (LOCAL) USAGE 0x01 (Consumer Control) */
>> + 0xA1, 0x01, /* (MAIN) COLLECTION (Application) */
>> + 0x85, 0x02, /* (GLOBAL) REPORT_ID 0x02 (2) */
>> + 0x19, 0x00, /* (LOCAL) USAGE_MINIMUM 0x00 */
>> + 0x2A, 0x3C, 0x02, /* (LOCAL) USAGE_MAXIMUM 0x023C */
>> + 0x26, 0x3C, 0x02, /* (GLOBAL) LOGICAL_MAXIMUM 0x023C (572) */
>> + 0x75, 0x10, /* (GLOBAL) REPORT_SIZE 0x10 (16) */
>> + 0x95, 0x01, /* (GLOBAL) REPORT_COUNT 0x01 (1) */
>> + 0x81, 0x00, /* (MAIN) INPUT 0x00 */
>> + 0xC0, /* (MAIN) END_COLLECTION (Application) */
>> +
>> + /* Application Collection */
>> + 0x05, 0x01, /* (GLOBAL) USAGE_PAGE (Desktop) */
>> + 0x09, 0x0C, /* (LOCAL) USAGE (Wireless Radio Controls) */
>> + 0xA1, 0x01, /* (MAIN) COLLECTION (Application) */
>> + 0x85, 0x03, /* (GLOBAL) REPORT_ID 0x03 (3) */
>> + 0x25, 0x01, /* (GLOBAL) LOGICAL_MAXIMUM 0x01 (1) */
>> + 0x09, 0xC6, /* (LOCAL) USAGE 0xC6 */
>> + 0x75, 0x01, /* (GLOBAL) REPORT_SIZE 0x01 (1) */
>> + 0x81, 0x06, /* (MAIN) INPUT 0x06 */
>> + 0x75, 0x07, /* (GLOBAL) REPORT_SIZE 0x07 (7) */
>> + 0x81, 0x03, /* (MAIN) INPUT 0x03 */
>> + 0xC0, /* (MAIN) END_COLLECTION (Application) */
>> +
>> + /* Application Collection */
>> + 0x05, 0x88, /* (GLOBAL) USAGE_PAGE (0x88) */
>> + 0x09, 0x01, /* (LOCAL) USAGE (0x01) */
>> + 0xA1, 0x01, /* (MAIN) COLLECTION (Application) */
>> + 0x85, 0x04, /* (GLOBAL) REPORT_ID 0x04 (4) */
>> + 0x19, 0x00, /* (LOCAL) USAGE_MINIMUM 0x00 */
>> + 0x2A, 0xFF, 0x00, /* (LOCAL) USAGE_MAXIMUM 0x00FF */
>> + 0x26, 0xFF, 0x00, /* (GLOBAL) LOGICAL_MAXIMUM 0x00FF */
>> + 0x75, 0x08, /* (GLOBAL) REPORT_SIZE 0x08 (8) */
>> + 0x95, 0x02, /* (GLOBAL) REPORT_COUNT 0x02 (2) */
>> + 0x81, 0x02, /* (MAIN) INPUT 0x02 */
>> + 0xC0, /* (MAIN) END_COLLECTION (Application) */
>> +
>> + /* Application Collection */
>> + 0x05, 0x01, /* (GLOBAL) USAGE_PAGE (Desktop) */
>> + 0x09, 0x80, /* (LOCAL) USAGE (System Control) */
>> + 0xA1, 0x01, /* (MAIN) COLLECTION (Application) */
>> + 0x85, 0x05, /* (GLOBAL) REPORT_ID 0x05 (5) */
>> + 0x19, 0x81, /* (LOCAL) USAGE_MINIMUM 0x81 */
>> + 0x29, 0x83, /* (LOCAL) USAGE_MAXIMUM 0x83 */
>> + 0x25, 0x01, /* (GLOBAL) LOGICAL_MAXIMUM 0x01 (1) */
>> + 0x95, 0x08, /* (GLOBAL) REPORT_COUNT 0x08 (8) */
>> + 0x75, 0x01, /* (GLOBAL) REPORT_SIZE 0x01 (1) */
>> + 0x81, 0x02, /* (MAIN) INPUT 0x02 */
>> + 0xC0, /* (MAIN) END_COLLECTION (Application) */
>> +};
>> +
>> +static __u8 *synaptics_kbd_report_fixup(struct hid_device *hdev, __u8 *rdesc,
>> + unsigned int *rsize)
>> +{
>> + struct usb_interface *intf = to_usb_interface(hdev->dev.parent);
>> +
>> + if (intf->cur_altsetting->desc.bInterfaceNumber == 0) {
>
> Please don't. This is too fragile and we can not use uhid to replay
> and test such devices.
> Make a check on the size of the original descriptor, or check which
> bits you are replacing to know which interface you want to change the
> report descriptor.
> You can also check on the HID device .type which may be different
> among the various interfaces.
>
Is a size check safe enough? Maybe synaptics starts to change the rdesc.
I'm just replacing two bytes:
> 0x2A, 0xFF, 0x00, /* (LOCAL) USAGE_MAXIMUM 0x00FF
> 0x26, 0xFF, 0x00, /* (GLOBAL) LOGICAL_MAXIMUM 0x00FF
used to be
> 0x2A, 0xFF, 0xFF, /* (LOCAL) USAGE_MAXIMUM 0xFFFF
> 0x26, 0xFF, 0xFF, /* (GLOBAL) LOGICAL_MAXIMUM 0xFFFF
and I removed some unneeded USAGE_MINIMUM which are already set to 0
globally (which doesn't make any difference).
The smallest solution would be a byte copy over it, but I think it
wouldn't be readable later if there occur other issue or if synaptics
starts selling devices with changed rdesc and same device id.
I have seen rdesc fixes with full copy and byte copy in other modules,
are there any suggestions when to use what?
>> + rdesc = synaptics_kbd_rdesc_fixed;
>> + *rsize = sizeof(synaptics_kbd_rdesc_fixed);
>> + }
>> + return rdesc;
>> +}
>> +
>> +static int synaptics_probe(struct hid_device *hdev,
>> + const struct hid_device_id *id)
>> +{
>> + int ret;
>> +
>> + ret = hid_parse(hdev);
>> + if (ret) {
>> + hid_err(hdev, "parse failed\n");
>> + goto err_free;
>> + }
>> +
>> + ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
>> + if (ret) {
>> + hid_err(hdev, "hw start failed\n");
>> + goto err_free;
>> + }
>> +
>> + if (ret < 0)
>> + goto err_stop;
>> +
>> + return 0;
>> +err_stop:
>> + hid_hw_stop(hdev);
>> +err_free:
>> + return ret;
>> +}
I looked around how other modules handle this and didn't came up with a
better name for the label, but this obsolete if I remove the
probe/remove (see below).
>
> I am pretty sure there is no need for a probe here. Hid-core will use
> the generic one and you should be fine.
>
So when I remove the function(s) and reference(s) in hid_driver for the
probe and the remove hid_core will handle that for me?
>> +
>> +static void synaptics_remove(struct hid_device *hdev)
>> +{
>> + hid_hw_stop(hdev);
>> + kfree(hid_get_drvdata(hdev));
>> +}
>
> Same here, not mandatory.
>
> Cheers,
> Benjamin
>
>> +
>> +static const struct hid_device_id synaptics_devices[] = {
>> + { HID_USB_DEVICE(USB_VENDOR_ID_SYNAPTICS,
>> + USB_VENDOR_ID_SYNAPTICS_KBD) },
>> + { }
>> +};
>> +MODULE_DEVICE_TABLE(hid, synaptics_devices);
>> +
>> +static struct hid_driver synaptics_driver = {
>> + .name = "synaptics",
>> + .id_table = synaptics_devices,
>> + .report_fixup = synaptics_kbd_report_fixup,
>> + .probe = synaptics_probe,
>> + .remove = synaptics_remove,
>> +};
>> +module_hid_driver(synaptics_driver);
>> +
>> +MODULE_LICENSE("GPL");
>> --
>> 2.2.2
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-input" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] HID: Add driver for synaptics keybard with broken rdesc
2015-01-27 15:10 ` Simon Wörner
@ 2015-01-27 16:13 ` Benjamin Tissoires
2015-01-27 23:46 ` Andrew Duggan
0 siblings, 1 reply; 14+ messages in thread
From: Benjamin Tissoires @ 2015-01-27 16:13 UTC (permalink / raw)
To: Simon Wörner; +Cc: Jiri Kosina, linux-input, Simon Wörner
On Tue, Jan 27, 2015 at 10:10 AM, Simon Wörner <linux@simon-woerner.de> wrote:
> Thanks for the feedback, this is my first commit to the linux kernel so
> I don't know much about how the input / hid is working.
>
> I will add the changelog to the patch email, just need to find out how
> this works.
>
> Am 27.01.15 um 15:16 schrieb Benjamin Tissoires:
>> On Fri, Jan 23, 2015 at 2:03 PM, Simon Wörner <linux@simon-woerner.de> wrote:
>>> From: Simon Wörner <mail@simon-woerner.de>
>>>
>>> Signed-off-by: Simon Wörner <mail@simon-woerner.de>
>>> ---
>>> drivers/hid/Kconfig | 6 ++
>>> drivers/hid/Makefile | 1 +
>>> drivers/hid/hid-ids.h | 1 +
>>> drivers/hid/hid-synaptics.c | 190 ++++++++++++++++++++++++++++++++++++++++++++
>>> 4 files changed, 198 insertions(+)
>>> create mode 100644 drivers/hid/hid-synaptics.c
>>
>> I am pretty sure you are missing a change in hid-core.c to add your
>> device to hid_have_special_driver.
>>
>
> So I need to add this?
>
>> static const struct hid_device_id hid_have_special_driver[] = {
>> ...
>> { HID_USB_DEVICE(USB_VENDOR_ID_SYNAPTICS, USB_VENDOR_ID_SYNAPTICS_KBD) },
>> ...
yep, seems fine.
>
> do I also need to add
>
>> #if IS_ENABLED(HID_SYNAPTICS)
>> ...
>> #endif
>
> or will hid-core handle that for me?
no, the rule here is that we just blacklist the device, and if the
module is not compiled, then... too bad :)
>
>> Other than that (and the 2comments Jiri made), I am not very happy
>> with adding a hid-synaptics while we already have a hid-rmi for
>> synaptics devices. However, the keyboard must not follow the rmi spec,
>> so this might be just fine.
>>
>
> I have seen the rmi module and haven't found anything they have in
> common. The USB device of the keyboard has also attached a mouse /
> trackpad, but it also doens't make use of the rmi module.
>
> I don't see any reason for putting both together, e.g. hid-holtek-* has
> also two modules for keyboard rdesc fix and mouse rdesc fix.
>
> Maybe hid-synaptics-kbd would better fit?
no hid-synaptics will be fine.
>
>> Few more comments:
>>
>>>
>>> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
>>> index dfdc269..9f4124e 100644
>>> --- a/drivers/hid/Kconfig
>>> +++ b/drivers/hid/Kconfig
>>> @@ -708,6 +708,12 @@ config HID_SUNPLUS
>>> ---help---
>>> Support for Sunplus wireless desktop.
>>>
>>> +config HID_SYNAPTICS
>>> + tristate "Synaptics keyboard support"
>>> + depends on HID
>>> + ---help---
>>> + Support for Synaptics keyboard with invalid rdesc
>>> +
>>> config HID_RMI
>>> tristate "Synaptics RMI4 device support"
>>> depends on HID
>>> diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
>>> index debd15b..01bda39 100644
>>> --- a/drivers/hid/Makefile
>>> +++ b/drivers/hid/Makefile
>>> @@ -109,6 +109,7 @@ obj-$(CONFIG_HID_SONY) += hid-sony.o
>>> obj-$(CONFIG_HID_SPEEDLINK) += hid-speedlink.o
>>> obj-$(CONFIG_HID_STEELSERIES) += hid-steelseries.o
>>> obj-$(CONFIG_HID_SUNPLUS) += hid-sunplus.o
>>> +obj-$(CONFIG_HID_SYNAPTICS) += hid-synaptics.o
>>> obj-$(CONFIG_HID_GREENASIA) += hid-gaff.o
>>> obj-$(CONFIG_HID_THINGM) += hid-thingm.o
>>> obj-$(CONFIG_HID_THRUSTMASTER) += hid-tmff.o
>>> diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
>>> index 9243359..976ab39 100644
>>> --- a/drivers/hid/hid-ids.h
>>> +++ b/drivers/hid/hid-ids.h
>>> @@ -873,6 +873,7 @@
>>> #define USB_DEVICE_ID_SYNAPTICS_LTS2 0x1d10
>>> #define USB_DEVICE_ID_SYNAPTICS_HD 0x0ac3
>>> #define USB_DEVICE_ID_SYNAPTICS_QUAD_HD 0x1ac3
>>> +#define USB_VENDOR_ID_SYNAPTICS_KBD 0x2968
>>> #define USB_DEVICE_ID_SYNAPTICS_TP_V103 0x5710
>>>
>>> #define USB_VENDOR_ID_TEXAS_INSTRUMENTS 0x2047
>>> diff --git a/drivers/hid/hid-synaptics.c b/drivers/hid/hid-synaptics.c
>>> new file mode 100644
>>> index 0000000..3dab405
>>> --- /dev/null
>>> +++ b/drivers/hid/hid-synaptics.c
>>> @@ -0,0 +1,190 @@
>>> +/*
>>> + * HID driver for synaptics devices
>>> + *
>>> + * Copyright (c) 2015 Simon Wörner
>>> + */
>>> +
>>> +/*
>>> + * This program is free software; you can redistribute it and/or modify it
>>> + * under the terms of the GNU General Public License as published by the Free
>>> + * Software Foundation; either version 2 of the License, or (at your option)
>>> + * any later version.
>>> + */
>>> +
>>> +#include <linux/device.h>
>>> +#include <linux/hid.h>
>>> +#include <linux/module.h>
>>> +#include <linux/usb.h>
>>
>> Please avoid using usb.h in HID drivers. HID should be transport agnostic.
>>
>
> Okay.
>
>>> +
>>> +#include "hid-ids.h"
>>> +
>>> +/* Synaptics keyboards (USB ID 06cb:2968) e.g. in Acer SW5-012
>>> + * have the following issue:
>>> + * - The report descriptor specifies an excessively large number of consumer
>>> + * usages (2^15), which is more than HID_MAX_USAGES. This prevents proper
>>> + * parsing of the report descriptor.
>>> + *
>>> + * The replacement descriptor below fixes the number of consumer usages.
>>> + */
>>> +
>>> +static __u8 synaptics_kbd_rdesc_fixed[] = {
>>> + /* Application Collection */
>>> + 0x06, 0x85, 0xFF, /* (GLOBAL) USAGE_PAGE (Vendor-defined) */
>>> + 0x09, 0x95, /* (LOCAL) USAGE (0xFF850095) */
>>> + 0xA1, 0x01, /* (MAIN) COLLECTION (Application) */
>>> + 0x85, 0x5A, /* (GLOBAL) REPORT_ID (0x5A (90) 'Z') */
>>> + 0x09, 0x01, /* (LOCAL) USAGE (0xFF850001) */
>>> + 0x26, 0xFF, 0x00, /* (GLOBAL) LOGICAL_MAXIMUM 0x00FF (255) */
>>> + 0x75, 0x08, /* (GLOBAL) REPORT_SIZE 0x08 (8) */
>>> + 0x95, 0x10, /* (GLOBAL) REPORT_COUNT 0x10 (16) */
>>> + 0xB1, 0x00, /* (MAIN) FEATURE 0x00 */
>>> + 0xC0, /* (MAIN) END_COLLECTION (Application) */
>>> +
>>> + /* Application Collection */
>>> + 0x05, 0x01, /* (GLOBAL) USAGE_PAGE (Desktop) */
>>> + 0x09, 0x06, /* (LOCAL) USAGE 0x06 (Keyboard) */
>>> + 0xA1, 0x01, /* (MAIN) COLLECTION (Application) */
>>> + 0x85, 0x01, /* (GLOBAL) REPORT_ID 0x01 (1) */
>>> + 0x75, 0x01, /* (GLOBAL) REPORT_SIZE 0x01 (1) */
>>> + 0x95, 0x08, /* (GLOBAL) REPORT_COUNT 0x08 (8) */
>>> + 0x05, 0x07, /* (GLOBAL) USAGE_PAGE 0x07 (Keyboard) */
>>> + 0x19, 0xE0, /* (LOCAL) USAGE_MINIMUM 0xE0 */
>>> + 0x29, 0xE7, /* (LOCAL) USAGE_MAXIMUM 0xE7 */
>>> + 0x25, 0x01, /* (GLOBAL) LOGICAL_MAXIMUM 0x01 (1) */
>>> + 0x81, 0x02, /* (MAIN) INPUT 0x02 */
>>> + 0x95, 0x01, /* (GLOBAL) REPORT_COUNT 0x01 (1) */
>>> + 0x75, 0x08, /* (GLOBAL) REPORT_SIZE 0x08 (8) */
>>> + 0x81, 0x03, /* (MAIN) INPUT 0x03 */
>>> + 0x95, 0x05, /* (GLOBAL) REPORT_COUNT 0x05 (5) */
>>> + 0x75, 0x01, /* (GLOBAL) REPORT_SIZE 0x01 (1) */
>>> + 0x05, 0x08, /* (GLOBAL) USAGE_PAGE 0x08 (LED Indicator) */
>>> + 0x19, 0x01, /* (LOCAL) USAGE_MINIMUM 0x01 */
>>> + 0x29, 0x05, /* (LOCAL) USAGE_MAXIMUM 0x05 */
>>> + 0x91, 0x02, /* (MAIN) OUTPUT 0x02 */
>>> + 0x95, 0x01, /* (GLOBAL) REPORT_COUNT 0x01 (1) */
>>> + 0x75, 0x03, /* (GLOBAL) REPORT_SIZE 0x03 (3) */
>>> + 0x91, 0x03, /* (MAIN) OUTPUT 0x03 */
>>> + 0x95, 0x06, /* (GLOBAL) REPORT_COUNT 0x06 (6) */
>>> + 0x75, 0x08, /* (GLOBAL) REPORT_SIZE 0x08 (8) */
>>> + 0x26, 0xFF, 0x00, /* (GLOBAL) LOGICAL_MAXIMUM 0x00FF (255) */
>>> + 0x05, 0x07, /* (GLOBAL) USAGE_PAGE 0x0007 (Keyboard) */
>>> + 0x19, 0x00, /* (LOCAL) USAGE_MINIMUM 0x00 */
>>> + 0x2A, 0xFF, 0x00, /* (LOCAL) USAGE_MAXIMUM 0xFF */
>>> + 0x81, 0x00, /* (MAIN) INPUT 0x00 */
>>> + 0xC0, /* (MAIN) END_COLLECTION (Application) */
>>> +
>>> + /* Application Collection */
>>> + 0x05, 0x0C, /* (GLOBAL) USAGE_PAGE (Consumer) */
>>> + 0x09, 0x01, /* (LOCAL) USAGE 0x01 (Consumer Control) */
>>> + 0xA1, 0x01, /* (MAIN) COLLECTION (Application) */
>>> + 0x85, 0x02, /* (GLOBAL) REPORT_ID 0x02 (2) */
>>> + 0x19, 0x00, /* (LOCAL) USAGE_MINIMUM 0x00 */
>>> + 0x2A, 0x3C, 0x02, /* (LOCAL) USAGE_MAXIMUM 0x023C */
>>> + 0x26, 0x3C, 0x02, /* (GLOBAL) LOGICAL_MAXIMUM 0x023C (572) */
>>> + 0x75, 0x10, /* (GLOBAL) REPORT_SIZE 0x10 (16) */
>>> + 0x95, 0x01, /* (GLOBAL) REPORT_COUNT 0x01 (1) */
>>> + 0x81, 0x00, /* (MAIN) INPUT 0x00 */
>>> + 0xC0, /* (MAIN) END_COLLECTION (Application) */
>>> +
>>> + /* Application Collection */
>>> + 0x05, 0x01, /* (GLOBAL) USAGE_PAGE (Desktop) */
>>> + 0x09, 0x0C, /* (LOCAL) USAGE (Wireless Radio Controls) */
>>> + 0xA1, 0x01, /* (MAIN) COLLECTION (Application) */
>>> + 0x85, 0x03, /* (GLOBAL) REPORT_ID 0x03 (3) */
>>> + 0x25, 0x01, /* (GLOBAL) LOGICAL_MAXIMUM 0x01 (1) */
>>> + 0x09, 0xC6, /* (LOCAL) USAGE 0xC6 */
>>> + 0x75, 0x01, /* (GLOBAL) REPORT_SIZE 0x01 (1) */
>>> + 0x81, 0x06, /* (MAIN) INPUT 0x06 */
>>> + 0x75, 0x07, /* (GLOBAL) REPORT_SIZE 0x07 (7) */
>>> + 0x81, 0x03, /* (MAIN) INPUT 0x03 */
>>> + 0xC0, /* (MAIN) END_COLLECTION (Application) */
>>> +
>>> + /* Application Collection */
>>> + 0x05, 0x88, /* (GLOBAL) USAGE_PAGE (0x88) */
>>> + 0x09, 0x01, /* (LOCAL) USAGE (0x01) */
>>> + 0xA1, 0x01, /* (MAIN) COLLECTION (Application) */
>>> + 0x85, 0x04, /* (GLOBAL) REPORT_ID 0x04 (4) */
>>> + 0x19, 0x00, /* (LOCAL) USAGE_MINIMUM 0x00 */
>>> + 0x2A, 0xFF, 0x00, /* (LOCAL) USAGE_MAXIMUM 0x00FF */
>>> + 0x26, 0xFF, 0x00, /* (GLOBAL) LOGICAL_MAXIMUM 0x00FF */
>>> + 0x75, 0x08, /* (GLOBAL) REPORT_SIZE 0x08 (8) */
>>> + 0x95, 0x02, /* (GLOBAL) REPORT_COUNT 0x02 (2) */
>>> + 0x81, 0x02, /* (MAIN) INPUT 0x02 */
>>> + 0xC0, /* (MAIN) END_COLLECTION (Application) */
>>> +
>>> + /* Application Collection */
>>> + 0x05, 0x01, /* (GLOBAL) USAGE_PAGE (Desktop) */
>>> + 0x09, 0x80, /* (LOCAL) USAGE (System Control) */
>>> + 0xA1, 0x01, /* (MAIN) COLLECTION (Application) */
>>> + 0x85, 0x05, /* (GLOBAL) REPORT_ID 0x05 (5) */
>>> + 0x19, 0x81, /* (LOCAL) USAGE_MINIMUM 0x81 */
>>> + 0x29, 0x83, /* (LOCAL) USAGE_MAXIMUM 0x83 */
>>> + 0x25, 0x01, /* (GLOBAL) LOGICAL_MAXIMUM 0x01 (1) */
>>> + 0x95, 0x08, /* (GLOBAL) REPORT_COUNT 0x08 (8) */
>>> + 0x75, 0x01, /* (GLOBAL) REPORT_SIZE 0x01 (1) */
>>> + 0x81, 0x02, /* (MAIN) INPUT 0x02 */
>>> + 0xC0, /* (MAIN) END_COLLECTION (Application) */
>>> +};
>>> +
>>> +static __u8 *synaptics_kbd_report_fixup(struct hid_device *hdev, __u8 *rdesc,
>>> + unsigned int *rsize)
>>> +{
>>> + struct usb_interface *intf = to_usb_interface(hdev->dev.parent);
>>> +
>>> + if (intf->cur_altsetting->desc.bInterfaceNumber == 0) {
>>
>> Please don't. This is too fragile and we can not use uhid to replay
>> and test such devices.
>> Make a check on the size of the original descriptor, or check which
>> bits you are replacing to know which interface you want to change the
>> report descriptor.
>> You can also check on the HID device .type which may be different
>> among the various interfaces.
>>
>
> Is a size check safe enough? Maybe synaptics starts to change the rdesc.
>
> I'm just replacing two bytes:
>
>> 0x2A, 0xFF, 0x00, /* (LOCAL) USAGE_MAXIMUM 0x00FF
>> 0x26, 0xFF, 0x00, /* (GLOBAL) LOGICAL_MAXIMUM 0x00FF
>
> used to be
>
>> 0x2A, 0xFF, 0xFF, /* (LOCAL) USAGE_MAXIMUM 0xFFFF
>> 0x26, 0xFF, 0xFF, /* (GLOBAL) LOGICAL_MAXIMUM 0xFFFF
>
> and I removed some unneeded USAGE_MINIMUM which are already set to 0
> globally (which doesn't make any difference).
>
> The smallest solution would be a byte copy over it, but I think it
> wouldn't be readable later if there occur other issue or if synaptics
> starts selling devices with changed rdesc and same device id.
>
> I have seen rdesc fixes with full copy and byte copy in other modules,
> are there any suggestions when to use what?
there is no rule. In your case, I would prefer a byte copy because the
change is small enough and this would prevent any copyright issues.
If possible, reuse the same way of fixing that
kye_consumer_control_fixup() in hid-kye.c
If it is clearly documented, there will not be further mistakes.
>
>
>>> + rdesc = synaptics_kbd_rdesc_fixed;
>>> + *rsize = sizeof(synaptics_kbd_rdesc_fixed);
>>> + }
>>> + return rdesc;
>>> +}
>>> +
>>> +static int synaptics_probe(struct hid_device *hdev,
>>> + const struct hid_device_id *id)
>>> +{
>>> + int ret;
>>> +
>>> + ret = hid_parse(hdev);
>>> + if (ret) {
>>> + hid_err(hdev, "parse failed\n");
>>> + goto err_free;
>>> + }
>>> +
>>> + ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
>>> + if (ret) {
>>> + hid_err(hdev, "hw start failed\n");
>>> + goto err_free;
>>> + }
>>> +
>>> + if (ret < 0)
>>> + goto err_stop;
>>> +
>>> + return 0;
>>> +err_stop:
>>> + hid_hw_stop(hdev);
>>> +err_free:
>>> + return ret;
>>> +}
>
> I looked around how other modules handle this and didn't came up with a
> better name for the label, but this obsolete if I remove the
> probe/remove (see below).
>
>>
>> I am pretty sure there is no need for a probe here. Hid-core will use
>> the generic one and you should be fine.
>>
>
> So when I remove the function(s) and reference(s) in hid_driver for the
> probe and the remove hid_core will handle that for me?
Yeah, your driver does not allocated anything, so probe and remove
should not be used. Hid-core will take care of everything.
Cheers,
Benjamin
>
>>> +
>>> +static void synaptics_remove(struct hid_device *hdev)
>>> +{
>>> + hid_hw_stop(hdev);
>>> + kfree(hid_get_drvdata(hdev));
>>> +}
>>
>> Same here, not mandatory.
>>
>> Cheers,
>> Benjamin
>>
>>> +
>>> +static const struct hid_device_id synaptics_devices[] = {
>>> + { HID_USB_DEVICE(USB_VENDOR_ID_SYNAPTICS,
>>> + USB_VENDOR_ID_SYNAPTICS_KBD) },
>>> + { }
>>> +};
>>> +MODULE_DEVICE_TABLE(hid, synaptics_devices);
>>> +
>>> +static struct hid_driver synaptics_driver = {
>>> + .name = "synaptics",
>>> + .id_table = synaptics_devices,
>>> + .report_fixup = synaptics_kbd_report_fixup,
>>> + .probe = synaptics_probe,
>>> + .remove = synaptics_remove,
>>> +};
>>> +module_hid_driver(synaptics_driver);
>>> +
>>> +MODULE_LICENSE("GPL");
>>> --
>>> 2.2.2
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-input" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] HID: Add driver for synaptics keybard with broken rdesc
2015-01-27 16:13 ` Benjamin Tissoires
@ 2015-01-27 23:46 ` Andrew Duggan
2015-01-28 4:04 ` Simon Wörner
0 siblings, 1 reply; 14+ messages in thread
From: Andrew Duggan @ 2015-01-27 23:46 UTC (permalink / raw)
To: Benjamin Tissoires, Andrew Duggan
Cc: Simon Wörner, Jiri Kosina, linux-input, Simon Wörner
On Tue, Jan 27, 2015 at 8:13 AM, Benjamin Tissoires
<benjamin.tissoires@gmail.com> wrote:
> On Tue, Jan 27, 2015 at 10:10 AM, Simon Wörner <linux@simon-woerner.de> wrote:
>> Thanks for the feedback, this is my first commit to the linux kernel so
>> I don't know much about how the input / hid is working.
>>
>> I will add the changelog to the patch email, just need to find out how
>> this works.
>>
>> Am 27.01.15 um 15:16 schrieb Benjamin Tissoires:
>>> On Fri, Jan 23, 2015 at 2:03 PM, Simon Wörner <linux@simon-woerner.de> wrote:
>>>> From: Simon Wörner <mail@simon-woerner.de>
>>>>
>>>> Signed-off-by: Simon Wörner <mail@simon-woerner.de>
>>>> ---
>>>> drivers/hid/Kconfig | 6 ++
>>>> drivers/hid/Makefile | 1 +
>>>> drivers/hid/hid-ids.h | 1 +
>>>> drivers/hid/hid-synaptics.c | 190 ++++++++++++++++++++++++++++++++++++++++++++
>>>> 4 files changed, 198 insertions(+)
>>>> create mode 100644 drivers/hid/hid-synaptics.c
This isn't actually a Synaptics keyboard. The product id matches a
touchpad so it appears that this is another situation where a vendor
is using our touchpad's VID and PID to describe a composite USB device
in a dock, even though our touchpad is only one of the devices in the
dock. This is similar to what happened on the Dell Venue Pro 11. I
think Synaptics should be removed from the name since it will be
confusing should Synaptics release a keyboard.
>>>
>>> I am pretty sure you are missing a change in hid-core.c to add your
>>> device to hid_have_special_driver.
>>>
>>
>> So I need to add this?
>>
>>> static const struct hid_device_id hid_have_special_driver[] = {
>>> ...
>>> { HID_USB_DEVICE(USB_VENDOR_ID_SYNAPTICS, USB_VENDOR_ID_SYNAPTICS_KBD) },
>>> ...
>
> yep, seems fine.
Adding the VID and PID to the hid_have_special_driver will cause the
touchpad to stop working since it will prevent hid-multitouch from
binding to it. Maybe in hid-core.c add a new hid group and a new scan
flag for HID_GD_KEYBOARD and look for it in hid_scan_report()? But,
this would also match the keyboard in the Dell Venue Pro 11 and
potentially others, so the driver would need to check to see if the
report descriptor needs this fix or not.
>>
>> do I also need to add
>>
>>> #if IS_ENABLED(HID_SYNAPTICS)
>>> ...
>>> #endif
>>
>> or will hid-core handle that for me?
>
> no, the rule here is that we just blacklist the device, and if the
> module is not compiled, then... too bad :)
>
>>
>>> Other than that (and the 2comments Jiri made), I am not very happy
>>> with adding a hid-synaptics while we already have a hid-rmi for
>>> synaptics devices. However, the keyboard must not follow the rmi spec,
>>> so this might be just fine.
>>>
>>
>> I have seen the rmi module and haven't found anything they have in
>> common. The USB device of the keyboard has also attached a mouse /
>> trackpad, but it also doens't make use of the rmi module.
>>
>> I don't see any reason for putting both together, e.g. hid-holtek-* has
>> also two modules for keyboard rdesc fix and mouse rdesc fix.
>>
>> Maybe hid-synaptics-kbd would better fit?
>
> no hid-synaptics will be fine.
>
>>
>>> Few more comments:
>>>
>>>>
>>>> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
>>>> index dfdc269..9f4124e 100644
>>>> --- a/drivers/hid/Kconfig
>>>> +++ b/drivers/hid/Kconfig
>>>> @@ -708,6 +708,12 @@ config HID_SUNPLUS
>>>> ---help---
>>>> Support for Sunplus wireless desktop.
>>>>
>>>> +config HID_SYNAPTICS
>>>> + tristate "Synaptics keyboard support"
>>>> + depends on HID
>>>> + ---help---
>>>> + Support for Synaptics keyboard with invalid rdesc
>>>> +
>>>> config HID_RMI
>>>> tristate "Synaptics RMI4 device support"
>>>> depends on HID
>>>> diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
>>>> index debd15b..01bda39 100644
>>>> --- a/drivers/hid/Makefile
>>>> +++ b/drivers/hid/Makefile
>>>> @@ -109,6 +109,7 @@ obj-$(CONFIG_HID_SONY) += hid-sony.o
>>>> obj-$(CONFIG_HID_SPEEDLINK) += hid-speedlink.o
>>>> obj-$(CONFIG_HID_STEELSERIES) += hid-steelseries.o
>>>> obj-$(CONFIG_HID_SUNPLUS) += hid-sunplus.o
>>>> +obj-$(CONFIG_HID_SYNAPTICS) += hid-synaptics.o
>>>> obj-$(CONFIG_HID_GREENASIA) += hid-gaff.o
>>>> obj-$(CONFIG_HID_THINGM) += hid-thingm.o
>>>> obj-$(CONFIG_HID_THRUSTMASTER) += hid-tmff.o
>>>> diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
>>>> index 9243359..976ab39 100644
>>>> --- a/drivers/hid/hid-ids.h
>>>> +++ b/drivers/hid/hid-ids.h
>>>> @@ -873,6 +873,7 @@
>>>> #define USB_DEVICE_ID_SYNAPTICS_LTS2 0x1d10
>>>> #define USB_DEVICE_ID_SYNAPTICS_HD 0x0ac3
>>>> #define USB_DEVICE_ID_SYNAPTICS_QUAD_HD 0x1ac3
>>>> +#define USB_VENDOR_ID_SYNAPTICS_KBD 0x2968
>>>> #define USB_DEVICE_ID_SYNAPTICS_TP_V103 0x5710
>>>>
>>>> #define USB_VENDOR_ID_TEXAS_INSTRUMENTS 0x2047
>>>> diff --git a/drivers/hid/hid-synaptics.c b/drivers/hid/hid-synaptics.c
>>>> new file mode 100644
>>>> index 0000000..3dab405
>>>> --- /dev/null
>>>> +++ b/drivers/hid/hid-synaptics.c
>>>> @@ -0,0 +1,190 @@
>>>> +/*
>>>> + * HID driver for synaptics devices
>>>> + *
>>>> + * Copyright (c) 2015 Simon Wörner
>>>> + */
>>>> +
>>>> +/*
>>>> + * This program is free software; you can redistribute it and/or modify it
>>>> + * under the terms of the GNU General Public License as published by the Free
>>>> + * Software Foundation; either version 2 of the License, or (at your option)
>>>> + * any later version.
>>>> + */
>>>> +
>>>> +#include <linux/device.h>
>>>> +#include <linux/hid.h>
>>>> +#include <linux/module.h>
>>>> +#include <linux/usb.h>
>>>
>>> Please avoid using usb.h in HID drivers. HID should be transport agnostic.
>>>
>>
>> Okay.
>>
>>>> +
>>>> +#include "hid-ids.h"
>>>> +
>>>> +/* Synaptics keyboards (USB ID 06cb:2968) e.g. in Acer SW5-012
>>>> + * have the following issue:
>>>> + * - The report descriptor specifies an excessively large number of consumer
>>>> + * usages (2^15), which is more than HID_MAX_USAGES. This prevents proper
>>>> + * parsing of the report descriptor.
>>>> + *
>>>> + * The replacement descriptor below fixes the number of consumer usages.
>>>> + */
>>>> +
>>>> +static __u8 synaptics_kbd_rdesc_fixed[] = {
>>>> + /* Application Collection */
>>>> + 0x06, 0x85, 0xFF, /* (GLOBAL) USAGE_PAGE (Vendor-defined) */
>>>> + 0x09, 0x95, /* (LOCAL) USAGE (0xFF850095) */
>>>> + 0xA1, 0x01, /* (MAIN) COLLECTION (Application) */
>>>> + 0x85, 0x5A, /* (GLOBAL) REPORT_ID (0x5A (90) 'Z') */
>>>> + 0x09, 0x01, /* (LOCAL) USAGE (0xFF850001) */
>>>> + 0x26, 0xFF, 0x00, /* (GLOBAL) LOGICAL_MAXIMUM 0x00FF (255) */
>>>> + 0x75, 0x08, /* (GLOBAL) REPORT_SIZE 0x08 (8) */
>>>> + 0x95, 0x10, /* (GLOBAL) REPORT_COUNT 0x10 (16) */
>>>> + 0xB1, 0x00, /* (MAIN) FEATURE 0x00 */
>>>> + 0xC0, /* (MAIN) END_COLLECTION (Application) */
>>>> +
>>>> + /* Application Collection */
>>>> + 0x05, 0x01, /* (GLOBAL) USAGE_PAGE (Desktop) */
>>>> + 0x09, 0x06, /* (LOCAL) USAGE 0x06 (Keyboard) */
>>>> + 0xA1, 0x01, /* (MAIN) COLLECTION (Application) */
>>>> + 0x85, 0x01, /* (GLOBAL) REPORT_ID 0x01 (1) */
>>>> + 0x75, 0x01, /* (GLOBAL) REPORT_SIZE 0x01 (1) */
>>>> + 0x95, 0x08, /* (GLOBAL) REPORT_COUNT 0x08 (8) */
>>>> + 0x05, 0x07, /* (GLOBAL) USAGE_PAGE 0x07 (Keyboard) */
>>>> + 0x19, 0xE0, /* (LOCAL) USAGE_MINIMUM 0xE0 */
>>>> + 0x29, 0xE7, /* (LOCAL) USAGE_MAXIMUM 0xE7 */
>>>> + 0x25, 0x01, /* (GLOBAL) LOGICAL_MAXIMUM 0x01 (1) */
>>>> + 0x81, 0x02, /* (MAIN) INPUT 0x02 */
>>>> + 0x95, 0x01, /* (GLOBAL) REPORT_COUNT 0x01 (1) */
>>>> + 0x75, 0x08, /* (GLOBAL) REPORT_SIZE 0x08 (8) */
>>>> + 0x81, 0x03, /* (MAIN) INPUT 0x03 */
>>>> + 0x95, 0x05, /* (GLOBAL) REPORT_COUNT 0x05 (5) */
>>>> + 0x75, 0x01, /* (GLOBAL) REPORT_SIZE 0x01 (1) */
>>>> + 0x05, 0x08, /* (GLOBAL) USAGE_PAGE 0x08 (LED Indicator) */
>>>> + 0x19, 0x01, /* (LOCAL) USAGE_MINIMUM 0x01 */
>>>> + 0x29, 0x05, /* (LOCAL) USAGE_MAXIMUM 0x05 */
>>>> + 0x91, 0x02, /* (MAIN) OUTPUT 0x02 */
>>>> + 0x95, 0x01, /* (GLOBAL) REPORT_COUNT 0x01 (1) */
>>>> + 0x75, 0x03, /* (GLOBAL) REPORT_SIZE 0x03 (3) */
>>>> + 0x91, 0x03, /* (MAIN) OUTPUT 0x03 */
>>>> + 0x95, 0x06, /* (GLOBAL) REPORT_COUNT 0x06 (6) */
>>>> + 0x75, 0x08, /* (GLOBAL) REPORT_SIZE 0x08 (8) */
>>>> + 0x26, 0xFF, 0x00, /* (GLOBAL) LOGICAL_MAXIMUM 0x00FF (255) */
>>>> + 0x05, 0x07, /* (GLOBAL) USAGE_PAGE 0x0007 (Keyboard) */
>>>> + 0x19, 0x00, /* (LOCAL) USAGE_MINIMUM 0x00 */
>>>> + 0x2A, 0xFF, 0x00, /* (LOCAL) USAGE_MAXIMUM 0xFF */
>>>> + 0x81, 0x00, /* (MAIN) INPUT 0x00 */
>>>> + 0xC0, /* (MAIN) END_COLLECTION (Application) */
>>>> +
>>>> + /* Application Collection */
>>>> + 0x05, 0x0C, /* (GLOBAL) USAGE_PAGE (Consumer) */
>>>> + 0x09, 0x01, /* (LOCAL) USAGE 0x01 (Consumer Control) */
>>>> + 0xA1, 0x01, /* (MAIN) COLLECTION (Application) */
>>>> + 0x85, 0x02, /* (GLOBAL) REPORT_ID 0x02 (2) */
>>>> + 0x19, 0x00, /* (LOCAL) USAGE_MINIMUM 0x00 */
>>>> + 0x2A, 0x3C, 0x02, /* (LOCAL) USAGE_MAXIMUM 0x023C */
>>>> + 0x26, 0x3C, 0x02, /* (GLOBAL) LOGICAL_MAXIMUM 0x023C (572) */
>>>> + 0x75, 0x10, /* (GLOBAL) REPORT_SIZE 0x10 (16) */
>>>> + 0x95, 0x01, /* (GLOBAL) REPORT_COUNT 0x01 (1) */
>>>> + 0x81, 0x00, /* (MAIN) INPUT 0x00 */
>>>> + 0xC0, /* (MAIN) END_COLLECTION (Application) */
>>>> +
>>>> + /* Application Collection */
>>>> + 0x05, 0x01, /* (GLOBAL) USAGE_PAGE (Desktop) */
>>>> + 0x09, 0x0C, /* (LOCAL) USAGE (Wireless Radio Controls) */
>>>> + 0xA1, 0x01, /* (MAIN) COLLECTION (Application) */
>>>> + 0x85, 0x03, /* (GLOBAL) REPORT_ID 0x03 (3) */
>>>> + 0x25, 0x01, /* (GLOBAL) LOGICAL_MAXIMUM 0x01 (1) */
>>>> + 0x09, 0xC6, /* (LOCAL) USAGE 0xC6 */
>>>> + 0x75, 0x01, /* (GLOBAL) REPORT_SIZE 0x01 (1) */
>>>> + 0x81, 0x06, /* (MAIN) INPUT 0x06 */
>>>> + 0x75, 0x07, /* (GLOBAL) REPORT_SIZE 0x07 (7) */
>>>> + 0x81, 0x03, /* (MAIN) INPUT 0x03 */
>>>> + 0xC0, /* (MAIN) END_COLLECTION (Application) */
>>>> +
>>>> + /* Application Collection */
>>>> + 0x05, 0x88, /* (GLOBAL) USAGE_PAGE (0x88) */
>>>> + 0x09, 0x01, /* (LOCAL) USAGE (0x01) */
>>>> + 0xA1, 0x01, /* (MAIN) COLLECTION (Application) */
>>>> + 0x85, 0x04, /* (GLOBAL) REPORT_ID 0x04 (4) */
>>>> + 0x19, 0x00, /* (LOCAL) USAGE_MINIMUM 0x00 */
>>>> + 0x2A, 0xFF, 0x00, /* (LOCAL) USAGE_MAXIMUM 0x00FF */
>>>> + 0x26, 0xFF, 0x00, /* (GLOBAL) LOGICAL_MAXIMUM 0x00FF */
>>>> + 0x75, 0x08, /* (GLOBAL) REPORT_SIZE 0x08 (8) */
>>>> + 0x95, 0x02, /* (GLOBAL) REPORT_COUNT 0x02 (2) */
>>>> + 0x81, 0x02, /* (MAIN) INPUT 0x02 */
>>>> + 0xC0, /* (MAIN) END_COLLECTION (Application) */
>>>> +
>>>> + /* Application Collection */
>>>> + 0x05, 0x01, /* (GLOBAL) USAGE_PAGE (Desktop) */
>>>> + 0x09, 0x80, /* (LOCAL) USAGE (System Control) */
>>>> + 0xA1, 0x01, /* (MAIN) COLLECTION (Application) */
>>>> + 0x85, 0x05, /* (GLOBAL) REPORT_ID 0x05 (5) */
>>>> + 0x19, 0x81, /* (LOCAL) USAGE_MINIMUM 0x81 */
>>>> + 0x29, 0x83, /* (LOCAL) USAGE_MAXIMUM 0x83 */
>>>> + 0x25, 0x01, /* (GLOBAL) LOGICAL_MAXIMUM 0x01 (1) */
>>>> + 0x95, 0x08, /* (GLOBAL) REPORT_COUNT 0x08 (8) */
>>>> + 0x75, 0x01, /* (GLOBAL) REPORT_SIZE 0x01 (1) */
>>>> + 0x81, 0x02, /* (MAIN) INPUT 0x02 */
>>>> + 0xC0, /* (MAIN) END_COLLECTION (Application) */
>>>> +};
>>>> +
>>>> +static __u8 *synaptics_kbd_report_fixup(struct hid_device *hdev, __u8 *rdesc,
>>>> + unsigned int *rsize)
>>>> +{
>>>> + struct usb_interface *intf = to_usb_interface(hdev->dev.parent);
>>>> +
>>>> + if (intf->cur_altsetting->desc.bInterfaceNumber == 0) {
>>>
>>> Please don't. This is too fragile and we can not use uhid to replay
>>> and test such devices.
>>> Make a check on the size of the original descriptor, or check which
>>> bits you are replacing to know which interface you want to change the
>>> report descriptor.
>>> You can also check on the HID device .type which may be different
>>> among the various interfaces.
>>>
>>
>> Is a size check safe enough? Maybe synaptics starts to change the rdesc.
>>
>> I'm just replacing two bytes:
>>
>>> 0x2A, 0xFF, 0x00, /* (LOCAL) USAGE_MAXIMUM 0x00FF
>>> 0x26, 0xFF, 0x00, /* (GLOBAL) LOGICAL_MAXIMUM 0x00FF
>>
>> used to be
>>
>>> 0x2A, 0xFF, 0xFF, /* (LOCAL) USAGE_MAXIMUM 0xFFFF
>>> 0x26, 0xFF, 0xFF, /* (GLOBAL) LOGICAL_MAXIMUM 0xFFFF
>>
>> and I removed some unneeded USAGE_MINIMUM which are already set to 0
>> globally (which doesn't make any difference).
>>
>> The smallest solution would be a byte copy over it, but I think it
>> wouldn't be readable later if there occur other issue or if synaptics
>> starts selling devices with changed rdesc and same device id.
>>
>> I have seen rdesc fixes with full copy and byte copy in other modules,
>> are there any suggestions when to use what?
>
> there is no rule. In your case, I would prefer a byte copy because the
> change is small enough and this would prevent any copyright issues.
> If possible, reuse the same way of fixing that
> kye_consumer_control_fixup() in hid-kye.c
>
> If it is clearly documented, there will not be further mistakes.
>
>>
>>
>>>> + rdesc = synaptics_kbd_rdesc_fixed;
>>>> + *rsize = sizeof(synaptics_kbd_rdesc_fixed);
>>>> + }
>>>> + return rdesc;
>>>> +}
>>>> +
>>>> +static int synaptics_probe(struct hid_device *hdev,
>>>> + const struct hid_device_id *id)
>>>> +{
>>>> + int ret;
>>>> +
>>>> + ret = hid_parse(hdev);
>>>> + if (ret) {
>>>> + hid_err(hdev, "parse failed\n");
>>>> + goto err_free;
>>>> + }
>>>> +
>>>> + ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
>>>> + if (ret) {
>>>> + hid_err(hdev, "hw start failed\n");
>>>> + goto err_free;
>>>> + }
>>>> +
>>>> + if (ret < 0)
>>>> + goto err_stop;
>>>> +
>>>> + return 0;
>>>> +err_stop:
>>>> + hid_hw_stop(hdev);
>>>> +err_free:
>>>> + return ret;
>>>> +}
>>
>> I looked around how other modules handle this and didn't came up with a
>> better name for the label, but this obsolete if I remove the
>> probe/remove (see below).
>>
>>>
>>> I am pretty sure there is no need for a probe here. Hid-core will use
>>> the generic one and you should be fine.
>>>
>>
>> So when I remove the function(s) and reference(s) in hid_driver for the
>> probe and the remove hid_core will handle that for me?
>
> Yeah, your driver does not allocated anything, so probe and remove
> should not be used. Hid-core will take care of everything.
>
> Cheers,
> Benjamin
>
>>
>>>> +
>>>> +static void synaptics_remove(struct hid_device *hdev)
>>>> +{
>>>> + hid_hw_stop(hdev);
>>>> + kfree(hid_get_drvdata(hdev));
>>>> +}
>>>
>>> Same here, not mandatory.
>>>
>>> Cheers,
>>> Benjamin
>>>
>>>> +
>>>> +static const struct hid_device_id synaptics_devices[] = {
>>>> + { HID_USB_DEVICE(USB_VENDOR_ID_SYNAPTICS,
>>>> + USB_VENDOR_ID_SYNAPTICS_KBD) },
>>>> + { }
>>>> +};
>>>> +MODULE_DEVICE_TABLE(hid, synaptics_devices);
>>>> +
>>>> +static struct hid_driver synaptics_driver = {
>>>> + .name = "synaptics",
>>>> + .id_table = synaptics_devices,
>>>> + .report_fixup = synaptics_kbd_report_fixup,
>>>> + .probe = synaptics_probe,
>>>> + .remove = synaptics_remove,
>>>> +};
>>>> +module_hid_driver(synaptics_driver);
>>>> +
>>>> +MODULE_LICENSE("GPL");
>>>> --
>>>> 2.2.2
>>>>
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe linux-input" in
>>>> the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] HID: Add driver for synaptics keybard with broken rdesc
2015-01-27 23:46 ` Andrew Duggan
@ 2015-01-28 4:04 ` Simon Wörner
2015-01-29 0:43 ` [PATCH] HID: Add driver for acer " linux
0 siblings, 1 reply; 14+ messages in thread
From: Simon Wörner @ 2015-01-28 4:04 UTC (permalink / raw)
To: Andrew Duggan, Benjamin Tissoires, Andrew Duggan
Cc: Jiri Kosina, linux-input, Simon Wörner
The new version is nearly finished, I will send it later today.
See below for additional changes not in the other mails.
Am 28.01.15 um 00:46 schrieb Andrew Duggan:
> On Tue, Jan 27, 2015 at 8:13 AM, Benjamin Tissoires
> <benjamin.tissoires@gmail.com> wrote:
>> On Tue, Jan 27, 2015 at 10:10 AM, Simon Wörner <linux@simon-woerner.de> wrote:
>>> Thanks for the feedback, this is my first commit to the linux kernel so
>>> I don't know much about how the input / hid is working.
>>>
>>> I will add the changelog to the patch email, just need to find out how
>>> this works.
>>>
>>> Am 27.01.15 um 15:16 schrieb Benjamin Tissoires:
>>>> On Fri, Jan 23, 2015 at 2:03 PM, Simon Wörner <linux@simon-woerner.de> wrote:
>>>>> From: Simon Wörner <mail@simon-woerner.de>
>>>>>
>>>>> Signed-off-by: Simon Wörner <mail@simon-woerner.de>
>>>>> ---
>>>>> drivers/hid/Kconfig | 6 ++
>>>>> drivers/hid/Makefile | 1 +
>>>>> drivers/hid/hid-ids.h | 1 +
>>>>> drivers/hid/hid-synaptics.c | 190 ++++++++++++++++++++++++++++++++++++++++++++
>>>>> 4 files changed, 198 insertions(+)
>>>>> create mode 100644 drivers/hid/hid-synaptics.c
>
> This isn't actually a Synaptics keyboard. The product id matches a
> touchpad so it appears that this is another situation where a vendor
> is using our touchpad's VID and PID to describe a composite USB device
> in a dock, even though our touchpad is only one of the devices in the
> dock. This is similar to what happened on the Dell Venue Pro 11. I
> think Synaptics should be removed from the name since it will be
> confusing should Synaptics release a keyboard.
>
Thanks for pointing this out. I disassembled it and it seems to be a
acer keyboard, at least there is a acer part number on it and acer
assembled it with the stolen USB ID.
So I renamed the module to 'hid_acer'.
>>>>
>>>> I am pretty sure you are missing a change in hid-core.c to add your
>>>> device to hid_have_special_driver.
>>>>
>>>
>>> So I need to add this?
>>>
>>>> static const struct hid_device_id hid_have_special_driver[] = {
>>>> ...
>>>> { HID_USB_DEVICE(USB_VENDOR_ID_SYNAPTICS, USB_VENDOR_ID_SYNAPTICS_KBD) },
>>>> ...
>>
>> yep, seems fine.
>
> Adding the VID and PID to the hid_have_special_driver will cause the
> touchpad to stop working since it will prevent hid-multitouch from
> binding to it. Maybe in hid-core.c add a new hid group and a new scan
> flag for HID_GD_KEYBOARD and look for it in hid_scan_report()? But,
> this would also match the keyboard in the Dell Venue Pro 11 and
> potentially others, so the driver would need to check to see if the
> report descriptor needs this fix or not.
>
I removed it from 'hid_have_special_driver' as in the first patch mail.
If the USB ID isn't reliable and used by multiple devices then letting
hid-core fail (on the acer keyboards) and claim the device afterwards
with a check for the defect rdesc would have minimum impact on other
devices and seems the be an acceptable solution to me.
>>>
>>> do I also need to add
>>>
>>>> #if IS_ENABLED(HID_SYNAPTICS)
>>>> ...
>>>> #endif
>>>
>>> or will hid-core handle that for me?
>>
>> no, the rule here is that we just blacklist the device, and if the
>> module is not compiled, then... too bad :)
>>
>>>
>>>> Other than that (and the 2comments Jiri made), I am not very happy
>>>> with adding a hid-synaptics while we already have a hid-rmi for
>>>> synaptics devices. However, the keyboard must not follow the rmi spec,
>>>> so this might be just fine.
>>>>
>>>
>>> I have seen the rmi module and haven't found anything they have in
>>> common. The USB device of the keyboard has also attached a mouse /
>>> trackpad, but it also doens't make use of the rmi module.
>>>
>>> I don't see any reason for putting both together, e.g. hid-holtek-* has
>>> also two modules for keyboard rdesc fix and mouse rdesc fix.
>>>
>>> Maybe hid-synaptics-kbd would better fit?
>>
>> no hid-synaptics will be fine.
>>
>>>
>>>> Few more comments:
>>>>
>>>>>
>>>>> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
>>>>> index dfdc269..9f4124e 100644
>>>>> --- a/drivers/hid/Kconfig
>>>>> +++ b/drivers/hid/Kconfig
>>>>> @@ -708,6 +708,12 @@ config HID_SUNPLUS
>>>>> ---help---
>>>>> Support for Sunplus wireless desktop.
>>>>>
>>>>> +config HID_SYNAPTICS
>>>>> + tristate "Synaptics keyboard support"
>>>>> + depends on HID
>>>>> + ---help---
>>>>> + Support for Synaptics keyboard with invalid rdesc
>>>>> +
>>>>> config HID_RMI
>>>>> tristate "Synaptics RMI4 device support"
>>>>> depends on HID
>>>>> diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
>>>>> index debd15b..01bda39 100644
>>>>> --- a/drivers/hid/Makefile
>>>>> +++ b/drivers/hid/Makefile
>>>>> @@ -109,6 +109,7 @@ obj-$(CONFIG_HID_SONY) += hid-sony.o
>>>>> obj-$(CONFIG_HID_SPEEDLINK) += hid-speedlink.o
>>>>> obj-$(CONFIG_HID_STEELSERIES) += hid-steelseries.o
>>>>> obj-$(CONFIG_HID_SUNPLUS) += hid-sunplus.o
>>>>> +obj-$(CONFIG_HID_SYNAPTICS) += hid-synaptics.o
>>>>> obj-$(CONFIG_HID_GREENASIA) += hid-gaff.o
>>>>> obj-$(CONFIG_HID_THINGM) += hid-thingm.o
>>>>> obj-$(CONFIG_HID_THRUSTMASTER) += hid-tmff.o
>>>>> diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
>>>>> index 9243359..976ab39 100644
>>>>> --- a/drivers/hid/hid-ids.h
>>>>> +++ b/drivers/hid/hid-ids.h
>>>>> @@ -873,6 +873,7 @@
>>>>> #define USB_DEVICE_ID_SYNAPTICS_LTS2 0x1d10
>>>>> #define USB_DEVICE_ID_SYNAPTICS_HD 0x0ac3
>>>>> #define USB_DEVICE_ID_SYNAPTICS_QUAD_HD 0x1ac3
>>>>> +#define USB_VENDOR_ID_SYNAPTICS_KBD 0x2968
>>>>> #define USB_DEVICE_ID_SYNAPTICS_TP_V103 0x5710
>>>>>
>>>>> #define USB_VENDOR_ID_TEXAS_INSTRUMENTS 0x2047
>>>>> diff --git a/drivers/hid/hid-synaptics.c b/drivers/hid/hid-synaptics.c
>>>>> new file mode 100644
>>>>> index 0000000..3dab405
>>>>> --- /dev/null
>>>>> +++ b/drivers/hid/hid-synaptics.c
>>>>> @@ -0,0 +1,190 @@
>>>>> +/*
>>>>> + * HID driver for synaptics devices
>>>>> + *
>>>>> + * Copyright (c) 2015 Simon Wörner
>>>>> + */
>>>>> +
>>>>> +/*
>>>>> + * This program is free software; you can redistribute it and/or modify it
>>>>> + * under the terms of the GNU General Public License as published by the Free
>>>>> + * Software Foundation; either version 2 of the License, or (at your option)
>>>>> + * any later version.
>>>>> + */
>>>>> +
>>>>> +#include <linux/device.h>
>>>>> +#include <linux/hid.h>
>>>>> +#include <linux/module.h>
>>>>> +#include <linux/usb.h>
>>>>
>>>> Please avoid using usb.h in HID drivers. HID should be transport agnostic.
>>>>
>>>
>>> Okay.
>>>
>>>>> +
>>>>> +#include "hid-ids.h"
>>>>> +
>>>>> +/* Synaptics keyboards (USB ID 06cb:2968) e.g. in Acer SW5-012
>>>>> + * have the following issue:
>>>>> + * - The report descriptor specifies an excessively large number of consumer
>>>>> + * usages (2^15), which is more than HID_MAX_USAGES. This prevents proper
>>>>> + * parsing of the report descriptor.
>>>>> + *
>>>>> + * The replacement descriptor below fixes the number of consumer usages.
>>>>> + */
>>>>> +
>>>>> +static __u8 synaptics_kbd_rdesc_fixed[] = {
>>>>> + /* Application Collection */
>>>>> + 0x06, 0x85, 0xFF, /* (GLOBAL) USAGE_PAGE (Vendor-defined) */
>>>>> + 0x09, 0x95, /* (LOCAL) USAGE (0xFF850095) */
>>>>> + 0xA1, 0x01, /* (MAIN) COLLECTION (Application) */
>>>>> + 0x85, 0x5A, /* (GLOBAL) REPORT_ID (0x5A (90) 'Z') */
>>>>> + 0x09, 0x01, /* (LOCAL) USAGE (0xFF850001) */
>>>>> + 0x26, 0xFF, 0x00, /* (GLOBAL) LOGICAL_MAXIMUM 0x00FF (255) */
>>>>> + 0x75, 0x08, /* (GLOBAL) REPORT_SIZE 0x08 (8) */
>>>>> + 0x95, 0x10, /* (GLOBAL) REPORT_COUNT 0x10 (16) */
>>>>> + 0xB1, 0x00, /* (MAIN) FEATURE 0x00 */
>>>>> + 0xC0, /* (MAIN) END_COLLECTION (Application) */
>>>>> +
>>>>> + /* Application Collection */
>>>>> + 0x05, 0x01, /* (GLOBAL) USAGE_PAGE (Desktop) */
>>>>> + 0x09, 0x06, /* (LOCAL) USAGE 0x06 (Keyboard) */
>>>>> + 0xA1, 0x01, /* (MAIN) COLLECTION (Application) */
>>>>> + 0x85, 0x01, /* (GLOBAL) REPORT_ID 0x01 (1) */
>>>>> + 0x75, 0x01, /* (GLOBAL) REPORT_SIZE 0x01 (1) */
>>>>> + 0x95, 0x08, /* (GLOBAL) REPORT_COUNT 0x08 (8) */
>>>>> + 0x05, 0x07, /* (GLOBAL) USAGE_PAGE 0x07 (Keyboard) */
>>>>> + 0x19, 0xE0, /* (LOCAL) USAGE_MINIMUM 0xE0 */
>>>>> + 0x29, 0xE7, /* (LOCAL) USAGE_MAXIMUM 0xE7 */
>>>>> + 0x25, 0x01, /* (GLOBAL) LOGICAL_MAXIMUM 0x01 (1) */
>>>>> + 0x81, 0x02, /* (MAIN) INPUT 0x02 */
>>>>> + 0x95, 0x01, /* (GLOBAL) REPORT_COUNT 0x01 (1) */
>>>>> + 0x75, 0x08, /* (GLOBAL) REPORT_SIZE 0x08 (8) */
>>>>> + 0x81, 0x03, /* (MAIN) INPUT 0x03 */
>>>>> + 0x95, 0x05, /* (GLOBAL) REPORT_COUNT 0x05 (5) */
>>>>> + 0x75, 0x01, /* (GLOBAL) REPORT_SIZE 0x01 (1) */
>>>>> + 0x05, 0x08, /* (GLOBAL) USAGE_PAGE 0x08 (LED Indicator) */
>>>>> + 0x19, 0x01, /* (LOCAL) USAGE_MINIMUM 0x01 */
>>>>> + 0x29, 0x05, /* (LOCAL) USAGE_MAXIMUM 0x05 */
>>>>> + 0x91, 0x02, /* (MAIN) OUTPUT 0x02 */
>>>>> + 0x95, 0x01, /* (GLOBAL) REPORT_COUNT 0x01 (1) */
>>>>> + 0x75, 0x03, /* (GLOBAL) REPORT_SIZE 0x03 (3) */
>>>>> + 0x91, 0x03, /* (MAIN) OUTPUT 0x03 */
>>>>> + 0x95, 0x06, /* (GLOBAL) REPORT_COUNT 0x06 (6) */
>>>>> + 0x75, 0x08, /* (GLOBAL) REPORT_SIZE 0x08 (8) */
>>>>> + 0x26, 0xFF, 0x00, /* (GLOBAL) LOGICAL_MAXIMUM 0x00FF (255) */
>>>>> + 0x05, 0x07, /* (GLOBAL) USAGE_PAGE 0x0007 (Keyboard) */
>>>>> + 0x19, 0x00, /* (LOCAL) USAGE_MINIMUM 0x00 */
>>>>> + 0x2A, 0xFF, 0x00, /* (LOCAL) USAGE_MAXIMUM 0xFF */
>>>>> + 0x81, 0x00, /* (MAIN) INPUT 0x00 */
>>>>> + 0xC0, /* (MAIN) END_COLLECTION (Application) */
>>>>> +
>>>>> + /* Application Collection */
>>>>> + 0x05, 0x0C, /* (GLOBAL) USAGE_PAGE (Consumer) */
>>>>> + 0x09, 0x01, /* (LOCAL) USAGE 0x01 (Consumer Control) */
>>>>> + 0xA1, 0x01, /* (MAIN) COLLECTION (Application) */
>>>>> + 0x85, 0x02, /* (GLOBAL) REPORT_ID 0x02 (2) */
>>>>> + 0x19, 0x00, /* (LOCAL) USAGE_MINIMUM 0x00 */
>>>>> + 0x2A, 0x3C, 0x02, /* (LOCAL) USAGE_MAXIMUM 0x023C */
>>>>> + 0x26, 0x3C, 0x02, /* (GLOBAL) LOGICAL_MAXIMUM 0x023C (572) */
>>>>> + 0x75, 0x10, /* (GLOBAL) REPORT_SIZE 0x10 (16) */
>>>>> + 0x95, 0x01, /* (GLOBAL) REPORT_COUNT 0x01 (1) */
>>>>> + 0x81, 0x00, /* (MAIN) INPUT 0x00 */
>>>>> + 0xC0, /* (MAIN) END_COLLECTION (Application) */
>>>>> +
>>>>> + /* Application Collection */
>>>>> + 0x05, 0x01, /* (GLOBAL) USAGE_PAGE (Desktop) */
>>>>> + 0x09, 0x0C, /* (LOCAL) USAGE (Wireless Radio Controls) */
>>>>> + 0xA1, 0x01, /* (MAIN) COLLECTION (Application) */
>>>>> + 0x85, 0x03, /* (GLOBAL) REPORT_ID 0x03 (3) */
>>>>> + 0x25, 0x01, /* (GLOBAL) LOGICAL_MAXIMUM 0x01 (1) */
>>>>> + 0x09, 0xC6, /* (LOCAL) USAGE 0xC6 */
>>>>> + 0x75, 0x01, /* (GLOBAL) REPORT_SIZE 0x01 (1) */
>>>>> + 0x81, 0x06, /* (MAIN) INPUT 0x06 */
>>>>> + 0x75, 0x07, /* (GLOBAL) REPORT_SIZE 0x07 (7) */
>>>>> + 0x81, 0x03, /* (MAIN) INPUT 0x03 */
>>>>> + 0xC0, /* (MAIN) END_COLLECTION (Application) */
>>>>> +
>>>>> + /* Application Collection */
>>>>> + 0x05, 0x88, /* (GLOBAL) USAGE_PAGE (0x88) */
>>>>> + 0x09, 0x01, /* (LOCAL) USAGE (0x01) */
>>>>> + 0xA1, 0x01, /* (MAIN) COLLECTION (Application) */
>>>>> + 0x85, 0x04, /* (GLOBAL) REPORT_ID 0x04 (4) */
>>>>> + 0x19, 0x00, /* (LOCAL) USAGE_MINIMUM 0x00 */
>>>>> + 0x2A, 0xFF, 0x00, /* (LOCAL) USAGE_MAXIMUM 0x00FF */
>>>>> + 0x26, 0xFF, 0x00, /* (GLOBAL) LOGICAL_MAXIMUM 0x00FF */
>>>>> + 0x75, 0x08, /* (GLOBAL) REPORT_SIZE 0x08 (8) */
>>>>> + 0x95, 0x02, /* (GLOBAL) REPORT_COUNT 0x02 (2) */
>>>>> + 0x81, 0x02, /* (MAIN) INPUT 0x02 */
>>>>> + 0xC0, /* (MAIN) END_COLLECTION (Application) */
>>>>> +
>>>>> + /* Application Collection */
>>>>> + 0x05, 0x01, /* (GLOBAL) USAGE_PAGE (Desktop) */
>>>>> + 0x09, 0x80, /* (LOCAL) USAGE (System Control) */
>>>>> + 0xA1, 0x01, /* (MAIN) COLLECTION (Application) */
>>>>> + 0x85, 0x05, /* (GLOBAL) REPORT_ID 0x05 (5) */
>>>>> + 0x19, 0x81, /* (LOCAL) USAGE_MINIMUM 0x81 */
>>>>> + 0x29, 0x83, /* (LOCAL) USAGE_MAXIMUM 0x83 */
>>>>> + 0x25, 0x01, /* (GLOBAL) LOGICAL_MAXIMUM 0x01 (1) */
>>>>> + 0x95, 0x08, /* (GLOBAL) REPORT_COUNT 0x08 (8) */
>>>>> + 0x75, 0x01, /* (GLOBAL) REPORT_SIZE 0x01 (1) */
>>>>> + 0x81, 0x02, /* (MAIN) INPUT 0x02 */
>>>>> + 0xC0, /* (MAIN) END_COLLECTION (Application) */
>>>>> +};
>>>>> +
>>>>> +static __u8 *synaptics_kbd_report_fixup(struct hid_device *hdev, __u8 *rdesc,
>>>>> + unsigned int *rsize)
>>>>> +{
>>>>> + struct usb_interface *intf = to_usb_interface(hdev->dev.parent);
>>>>> +
>>>>> + if (intf->cur_altsetting->desc.bInterfaceNumber == 0) {
>>>>
>>>> Please don't. This is too fragile and we can not use uhid to replay
>>>> and test such devices.
>>>> Make a check on the size of the original descriptor, or check which
>>>> bits you are replacing to know which interface you want to change the
>>>> report descriptor.
>>>> You can also check on the HID device .type which may be different
>>>> among the various interfaces.
>>>>
>>>
>>> Is a size check safe enough? Maybe synaptics starts to change the rdesc.
>>>
>>> I'm just replacing two bytes:
>>>
>>>> 0x2A, 0xFF, 0x00, /* (LOCAL) USAGE_MAXIMUM 0x00FF
>>>> 0x26, 0xFF, 0x00, /* (GLOBAL) LOGICAL_MAXIMUM 0x00FF
>>>
>>> used to be
>>>
>>>> 0x2A, 0xFF, 0xFF, /* (LOCAL) USAGE_MAXIMUM 0xFFFF
>>>> 0x26, 0xFF, 0xFF, /* (GLOBAL) LOGICAL_MAXIMUM 0xFFFF
>>>
>>> and I removed some unneeded USAGE_MINIMUM which are already set to 0
>>> globally (which doesn't make any difference).
>>>
>>> The smallest solution would be a byte copy over it, but I think it
>>> wouldn't be readable later if there occur other issue or if synaptics
>>> starts selling devices with changed rdesc and same device id.
>>>
>>> I have seen rdesc fixes with full copy and byte copy in other modules,
>>> are there any suggestions when to use what?
>>
>> there is no rule. In your case, I would prefer a byte copy because the
>> change is small enough and this would prevent any copyright issues.
>> If possible, reuse the same way of fixing that
>> kye_consumer_control_fixup() in hid-kye.c
>>
>> If it is clearly documented, there will not be further mistakes.
>>
>>>
>>>
>>>>> + rdesc = synaptics_kbd_rdesc_fixed;
>>>>> + *rsize = sizeof(synaptics_kbd_rdesc_fixed);
>>>>> + }
>>>>> + return rdesc;
>>>>> +}
>>>>> +
>>>>> +static int synaptics_probe(struct hid_device *hdev,
>>>>> + const struct hid_device_id *id)
>>>>> +{
>>>>> + int ret;
>>>>> +
>>>>> + ret = hid_parse(hdev);
>>>>> + if (ret) {
>>>>> + hid_err(hdev, "parse failed\n");
>>>>> + goto err_free;
>>>>> + }
>>>>> +
>>>>> + ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
>>>>> + if (ret) {
>>>>> + hid_err(hdev, "hw start failed\n");
>>>>> + goto err_free;
>>>>> + }
>>>>> +
>>>>> + if (ret < 0)
>>>>> + goto err_stop;
>>>>> +
>>>>> + return 0;
>>>>> +err_stop:
>>>>> + hid_hw_stop(hdev);
>>>>> +err_free:
>>>>> + return ret;
>>>>> +}
>>>
>>> I looked around how other modules handle this and didn't came up with a
>>> better name for the label, but this obsolete if I remove the
>>> probe/remove (see below).
>>>
>>>>
>>>> I am pretty sure there is no need for a probe here. Hid-core will use
>>>> the generic one and you should be fine.
>>>>
>>>
>>> So when I remove the function(s) and reference(s) in hid_driver for the
>>> probe and the remove hid_core will handle that for me?
>>
>> Yeah, your driver does not allocated anything, so probe and remove
>> should not be used. Hid-core will take care of everything.
>>
>> Cheers,
>> Benjamin
>>
>>>
>>>>> +
>>>>> +static void synaptics_remove(struct hid_device *hdev)
>>>>> +{
>>>>> + hid_hw_stop(hdev);
>>>>> + kfree(hid_get_drvdata(hdev));
>>>>> +}
>>>>
>>>> Same here, not mandatory.
>>>>
>>>> Cheers,
>>>> Benjamin
>>>>
>>>>> +
>>>>> +static const struct hid_device_id synaptics_devices[] = {
>>>>> + { HID_USB_DEVICE(USB_VENDOR_ID_SYNAPTICS,
>>>>> + USB_VENDOR_ID_SYNAPTICS_KBD) },
>>>>> + { }
>>>>> +};
>>>>> +MODULE_DEVICE_TABLE(hid, synaptics_devices);
>>>>> +
>>>>> +static struct hid_driver synaptics_driver = {
>>>>> + .name = "synaptics",
>>>>> + .id_table = synaptics_devices,
>>>>> + .report_fixup = synaptics_kbd_report_fixup,
>>>>> + .probe = synaptics_probe,
>>>>> + .remove = synaptics_remove,
>>>>> +};
>>>>> +module_hid_driver(synaptics_driver);
>>>>> +
>>>>> +MODULE_LICENSE("GPL");
>>>>> --
>>>>> 2.2.2
>>>>>
>>>>> --
>>>>> To unsubscribe from this list: send the line "unsubscribe linux-input" in
>>>>> the body of a message to majordomo@vger.kernel.org
>>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-input" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH] HID: Add driver for acer keybard with broken rdesc
2015-01-28 4:04 ` Simon Wörner
@ 2015-01-29 0:43 ` linux
2015-02-17 12:30 ` Jiri Kosina
0 siblings, 1 reply; 14+ messages in thread
From: linux @ 2015-01-29 0:43 UTC (permalink / raw)
To: Jiri Kosina
Cc: linux-input, Andrew Duggan, Benjamin Tissoires, Andrew Duggan,
Simon Wörner
From: Simon Wörner <mail@simon-woerner.de>
HID: Add driver for acer keybard with broken rdesc
Signed-off-by: Simon Wörner <mail@simon-woerner.de>
---
drivers/hid/Kconfig | 6 +++++
drivers/hid/Makefile | 1 +
drivers/hid/hid-acer.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++++++
drivers/hid/hid-ids.h | 3 +++
4 files changed, 80 insertions(+)
create mode 100644 drivers/hid/hid-acer.c
diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index dfdc269..f3ae543 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -98,6 +98,12 @@ config HID_A4TECH
---help---
Support for A4 tech X5 and WOP-35 / Trust 450L mice.
+config HID_ACER
+ tristate "Acer keyboard support"
+ depends on HID
+ ---help---
+ Support for acer keyboard with invalid rdesc.
+
config HID_ACRUX
tristate "ACRUX game controller support"
depends on HID
diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
index debd15b..143602b 100644
--- a/drivers/hid/Makefile
+++ b/drivers/hid/Makefile
@@ -34,6 +34,7 @@ ifdef CONFIG_DEBUG_FS
endif
obj-$(CONFIG_HID_A4TECH) += hid-a4tech.o
+obj-$(CONFIG_HID_ACER) += hid-acer.o
obj-$(CONFIG_HID_ACRUX) += hid-axff.o
obj-$(CONFIG_HID_APPLE) += hid-apple.o
obj-$(CONFIG_HID_APPLEIR) += hid-appleir.o
diff --git a/drivers/hid/hid-acer.c b/drivers/hid/hid-acer.c
new file mode 100644
index 0000000..b3ccd8b
--- /dev/null
+++ b/drivers/hid/hid-acer.c
@@ -0,0 +1,70 @@
+/*
+ * HID driver for acer devices
+ *
+ * Copyright (c) 2015 Simon Wörner
+ */
+
+/*
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the Free
+ * Software Foundation; either version 2 of the License, or (at your option)
+ * any later version.
+ */
+
+#include <linux/device.h>
+#include <linux/hid.h>
+#include <linux/module.h>
+
+#include "hid-ids.h"
+
+/* Acer keyboards e.g. in Acer SW5-012 use synaptics touchpad USB ID (06cb:2968)
+ * and have the following issue:
+ * - The report descriptor specifies an excessively large number of usages
+ * and logical max (2^16), which is more than HID_MAX_USAGES. This prevents
+ * proper parsing of the report descriptor.
+ *
+ * The byte replace in the descriptor below fixes the size.
+ */
+
+#define ACER_KBD_RDESC_ORIG_SIZE 188
+#define ACER_KBD_RDESC_CHECK_POS (150 * sizeof(__u8))
+#define ACER_KBD_RDESC_CHECK_DATA 0x2AFFFF150026FFFF
+#define ACER_KBD_RDESC_FIX_POS1 152
+#define ACER_KBD_RDESC_FIX_POS2 157
+
+static __u8 *acer_kbd_report_fixup(struct hid_device *hdev, __u8 *rdesc,
+ unsigned int *rsize)
+{
+ /* check for invalid descriptor */
+ if (*rsize == ACER_KBD_RDESC_ORIG_SIZE) {
+ __u64 check = be64_to_cpu(*(__be64 *)(rdesc + ACER_KBD_RDESC_CHECK_POS));
+
+ /* check for invalid max usages and logical 0xFFFF (2^16) */
+ if (check == ACER_KBD_RDESC_CHECK_DATA) {
+ hid_info(hdev, "fixing up acer keybaord report descriptor\n");
+
+ /* fix max values with 0xFF00 (2^8) */
+ rdesc[ACER_KBD_RDESC_FIX_POS1] = 0x00;
+ rdesc[ACER_KBD_RDESC_FIX_POS2] = 0x00;
+ }
+ }
+
+ return rdesc;
+}
+
+static const struct hid_device_id acer_devices[] = {
+ { HID_USB_DEVICE(USB_VENDOR_ID_ACER_SYNAPTICS,
+ USB_VENDOR_ID_ACER_SYNAPTICS_TP) },
+ { }
+};
+MODULE_DEVICE_TABLE(hid, acer_devices);
+
+static struct hid_driver acer_driver = {
+ .name = "acer",
+ .id_table = acer_devices,
+ .report_fixup = acer_kbd_report_fixup,
+};
+module_hid_driver(acer_driver);
+
+MODULE_AUTHOR("Simon Wörner");
+MODULE_LICENSE("GPL");
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index 9243359..5d9482b 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -35,6 +35,9 @@
#define USB_DEVICE_ID_ACECAD_FLAIR 0x0004
#define USB_DEVICE_ID_ACECAD_302 0x0008
+#define USB_VENDOR_ID_ACER_SYNAPTICS 0x06cb
+#define USB_VENDOR_ID_ACER_SYNAPTICS_TP 0x2968
+
#define USB_VENDOR_ID_ACRUX 0x1a34
#define USB_VENDOR_ID_ACTIONSTAR 0x2101
--
2.2.2
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] HID: Add driver for acer keybard with broken rdesc
2015-01-29 0:43 ` [PATCH] HID: Add driver for acer " linux
@ 2015-02-17 12:30 ` Jiri Kosina
2015-02-18 2:01 ` Andrew Duggan
0 siblings, 1 reply; 14+ messages in thread
From: Jiri Kosina @ 2015-02-17 12:30 UTC (permalink / raw)
To: linux
Cc: linux-input, Andrew Duggan, Benjamin Tissoires, Andrew Duggan,
Simon Wörner
On Thu, 29 Jan 2015, linux@simon-woerner.de wrote:
> From: Simon Wörner <mail@simon-woerner.de>
>
> HID: Add driver for acer keybard with broken rdesc
Hi Simon,
to make sure proper device <-> driver binding is performed, you also have
to add device ID to the hid_have_special_driver[] array.
--
Jiri Kosina
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] HID: Add driver for acer keybard with broken rdesc
2015-02-17 12:30 ` Jiri Kosina
@ 2015-02-18 2:01 ` Andrew Duggan
2015-02-27 14:16 ` Benjamin Tissoires
0 siblings, 1 reply; 14+ messages in thread
From: Andrew Duggan @ 2015-02-18 2:01 UTC (permalink / raw)
To: Jiri Kosina, linux
Cc: linux-input, Andrew Duggan, Benjamin Tissoires, Simon Wörner
On 02/17/2015 04:30 AM, Jiri Kosina wrote:
> On Thu, 29 Jan 2015, linux@simon-woerner.de wrote:
>
>> From: Simon Wörner <mail@simon-woerner.de>
>>
>> HID: Add driver for acer keybard with broken rdesc
> Hi Simon,
>
> to make sure proper device <-> driver binding is performed, you also have
> to add device ID to the hid_have_special_driver[] array.
>
I have been meaning to respond to this patch. Unfortunately, simply
adding this driver to the hid_have_special_driver[] list will prevent
hid-multitouch from binding to the touchpad which shares the same vid
and pid. My suggestion would be to create a new scan_flag for
GD_KEYBOARD and to add to the code in hid_scan_collection() which scans
the GENDESK usages. Similar to the code which is searching for
HID_GD_POINTER. Then in hid_scan_report() we could assign a group for
this driver based on the pid and the GD_KEYBOARD scan flag in the vendor
handling code.
I can help out with implementing this part of the patch if there aren't
any other suggestions. Adding a new group and more code to the
hid_scan_report() vendor handling code is definately not ideal. But, I
am not sure of a better way of binding different sub drivers to devices
with the same vid and pid.
An alternative would be to have hid-rmi handle all Synaptics touchpads,
even the ones which currently use hid-multitouch. Then the keyboard
report descriptor fixup could just be handled in hid-rmi. Accessing the
finger data via rmi mode would also have the benefit of being able to
report events which are currently not supported by the PTP collection
(ie ABS_MT_PRESSURE, ABS_MT_TOUCH_MAJOR/MINOR). The touchpads which
currently use hid-multitouch report finger data a little differently
then the ones currently using hid-rmi so there is some work which would
need to be done to add support for them in hid-rmi.
Andrew
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] HID: Add driver for acer keybard with broken rdesc
2015-02-18 2:01 ` Andrew Duggan
@ 2015-02-27 14:16 ` Benjamin Tissoires
2015-02-27 14:28 ` Florian Echtler
0 siblings, 1 reply; 14+ messages in thread
From: Benjamin Tissoires @ 2015-02-27 14:16 UTC (permalink / raw)
To: Andrew Duggan, Florian Echtler
Cc: Jiri Kosina, Simon Wörner, linux-input, Andrew Duggan,
Simon Wörner
[adding Florian to the thread, he is also affected by this]
On Tue, Feb 17, 2015 at 9:01 PM, Andrew Duggan <aduggan@synaptics.com> wrote:
> On 02/17/2015 04:30 AM, Jiri Kosina wrote:
>>
>> On Thu, 29 Jan 2015, linux@simon-woerner.de wrote:
>>
>>> From: Simon Wörner <mail@simon-woerner.de>
>>>
>>> HID: Add driver for acer keybard with broken rdesc
>>
>> Hi Simon,
>>
>> to make sure proper device <-> driver binding is performed, you also have
>> to add device ID to the hid_have_special_driver[] array.
>>
To the cost of an error in the kernel log, the proper binding will be
done even if if there is no entry in hid_have_special_driver :)
See Florian's log:
[ 3.153892] hid-generic 0003:06CB:2991.0003: usage index exceeded
[ 3.153896] hid-generic 0003:06CB:2991.0003: item 0 2 2 2 parsing failed
[ 3.153921] hid-generic: probe of 0003:06CB:2991.0003 failed with error -22
So hid-generic will fail to bind the device, so normally hid-acer
should bind it, fix the report descriptor and the keyboard should be
working.
Not very clean, but it should work :)
> I have been meaning to respond to this patch. Unfortunately, simply adding
> this driver to the hid_have_special_driver[] list will prevent
> hid-multitouch from binding to the touchpad which shares the same vid and
> pid. My suggestion would be to create a new scan_flag for GD_KEYBOARD and to
> add to the code in hid_scan_collection() which scans the GENDESK usages.
> Similar to the code which is searching for HID_GD_POINTER. Then in
> hid_scan_report() we could assign a group for this driver based on the pid
> and the GD_KEYBOARD scan flag in the vendor handling code.
This could work, but that means that the list of special quirks after
the parsing is going to explode :(
Plus we have to be sure that the scan_report doesn't fail or we will
not be able to tag the device correctly.
>
> I can help out with implementing this part of the patch if there aren't any
> other suggestions. Adding a new group and more code to the hid_scan_report()
> vendor handling code is definately not ideal. But, I am not sure of a better
> way of binding different sub drivers to devices with the same vid and pid.
>
> An alternative would be to have hid-rmi handle all Synaptics touchpads, even
> the ones which currently use hid-multitouch. Then the keyboard report
> descriptor fixup could just be handled in hid-rmi. Accessing the finger data
> via rmi mode would also have the benefit of being able to report events
> which are currently not supported by the PTP collection (ie ABS_MT_PRESSURE,
> ABS_MT_TOUCH_MAJOR/MINOR). The touchpads which currently use hid-multitouch
> report finger data a little differently then the ones currently using
> hid-rmi so there is some work which would need to be done to add support for
> them in hid-rmi.
>
I am not sure we want to do that. Because we don't want hid-rmi to fix
all crappy keyboards around when the OEM reuses the synaptics PID.
Maybe a better way of handling such situation is to provide a generic
mechanism to overwrite/patch the report descriptor so that we could
get rid of the drivers which just fix the report descriptor.
I have on a branch a version where I look into /lib/firmware/hid if
there is a matching device and a matching report descriptor. Then, I
read this firmware dynamically and patch the report descriptor.
This however requires that the hid modules are not included directly
in the kernel, or the /lib/firmware dir is not available and the
device doesn't bind.
Cheers,
Benjamin
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] HID: Add driver for acer keybard with broken rdesc
2015-02-27 14:16 ` Benjamin Tissoires
@ 2015-02-27 14:28 ` Florian Echtler
2015-03-24 16:03 ` Simon Wörner
0 siblings, 1 reply; 14+ messages in thread
From: Florian Echtler @ 2015-02-27 14:28 UTC (permalink / raw)
To: Benjamin Tissoires, Andrew Duggan
Cc: Jiri Kosina, Simon Wörner, linux-input, Andrew Duggan,
Simon Wörner
[-- Attachment #1: Type: text/plain, Size: 4357 bytes --]
Just as a quick side note, Simon's "hack" compiled as a standalone
module fixes the issue for me on stock kernel 3.16.0 - keyboard works
perfectly now. So device 06CB:2991 has exactly the same broken
descriptor and should probably included in any future solution.
Many thanks to everyone involved!
Best, Florian
On 27.02.2015 15:16, Benjamin Tissoires wrote:
> [adding Florian to the thread, he is also affected by this]
>
> On Tue, Feb 17, 2015 at 9:01 PM, Andrew Duggan <aduggan@synaptics.com> wrote:
>> On 02/17/2015 04:30 AM, Jiri Kosina wrote:
>>>
>>> On Thu, 29 Jan 2015, linux@simon-woerner.de wrote:
>>>
>>>> From: Simon Wörner <mail@simon-woerner.de>
>>>>
>>>> HID: Add driver for acer keybard with broken rdesc
>>>
>>> Hi Simon,
>>>
>>> to make sure proper device <-> driver binding is performed, you also have
>>> to add device ID to the hid_have_special_driver[] array.
>>>
>
> To the cost of an error in the kernel log, the proper binding will be
> done even if if there is no entry in hid_have_special_driver :)
>
> See Florian's log:
> [ 3.153892] hid-generic 0003:06CB:2991.0003: usage index exceeded
> [ 3.153896] hid-generic 0003:06CB:2991.0003: item 0 2 2 2 parsing failed
> [ 3.153921] hid-generic: probe of 0003:06CB:2991.0003 failed with error -22
>
> So hid-generic will fail to bind the device, so normally hid-acer
> should bind it, fix the report descriptor and the keyboard should be
> working.
>
> Not very clean, but it should work :)
>
>> I have been meaning to respond to this patch. Unfortunately, simply adding
>> this driver to the hid_have_special_driver[] list will prevent
>> hid-multitouch from binding to the touchpad which shares the same vid and
>> pid. My suggestion would be to create a new scan_flag for GD_KEYBOARD and to
>> add to the code in hid_scan_collection() which scans the GENDESK usages.
>> Similar to the code which is searching for HID_GD_POINTER. Then in
>> hid_scan_report() we could assign a group for this driver based on the pid
>> and the GD_KEYBOARD scan flag in the vendor handling code.
>
> This could work, but that means that the list of special quirks after
> the parsing is going to explode :(
>
> Plus we have to be sure that the scan_report doesn't fail or we will
> not be able to tag the device correctly.
>
>>
>> I can help out with implementing this part of the patch if there aren't any
>> other suggestions. Adding a new group and more code to the hid_scan_report()
>> vendor handling code is definately not ideal. But, I am not sure of a better
>> way of binding different sub drivers to devices with the same vid and pid.
>>
>> An alternative would be to have hid-rmi handle all Synaptics touchpads, even
>> the ones which currently use hid-multitouch. Then the keyboard report
>> descriptor fixup could just be handled in hid-rmi. Accessing the finger data
>> via rmi mode would also have the benefit of being able to report events
>> which are currently not supported by the PTP collection (ie ABS_MT_PRESSURE,
>> ABS_MT_TOUCH_MAJOR/MINOR). The touchpads which currently use hid-multitouch
>> report finger data a little differently then the ones currently using
>> hid-rmi so there is some work which would need to be done to add support for
>> them in hid-rmi.
>>
>
> I am not sure we want to do that. Because we don't want hid-rmi to fix
> all crappy keyboards around when the OEM reuses the synaptics PID.
>
> Maybe a better way of handling such situation is to provide a generic
> mechanism to overwrite/patch the report descriptor so that we could
> get rid of the drivers which just fix the report descriptor.
> I have on a branch a version where I look into /lib/firmware/hid if
> there is a matching device and a matching report descriptor. Then, I
> read this firmware dynamically and patch the report descriptor.
> This however requires that the hid modules are not included directly
> in the kernel, or the /lib/firmware dir is not available and the
> device doesn't bind.
>
> Cheers,
> Benjamin
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
SENT FROM MY DEC VT50 TERMINAL
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] HID: Add driver for acer keybard with broken rdesc
2015-02-27 14:28 ` Florian Echtler
@ 2015-03-24 16:03 ` Simon Wörner
0 siblings, 0 replies; 14+ messages in thread
From: Simon Wörner @ 2015-03-24 16:03 UTC (permalink / raw)
To: Florian Echtler, Benjamin Tissoires, Andrew Duggan
Cc: Jiri Kosina, Simon Wörner, linux-input, Andrew Duggan
On 27.02.2015 15:28, Florian Echtler wrote:
> Just as a quick side note, Simon's "hack" compiled as a standalone
> module fixes the issue for me on stock kernel 3.16.0 - keyboard works
> perfectly now. So device 06CB:2991 has exactly the same broken
> descriptor and should probably included in any future solution.
Okay, i will include it.
> Many thanks to everyone involved!
>
> Best, Florian
>
> On 27.02.2015 15:16, Benjamin Tissoires wrote:
>> [adding Florian to the thread, he is also affected by this]
>>
>> On Tue, Feb 17, 2015 at 9:01 PM, Andrew Duggan<aduggan@synaptics.com> wrote:
>>> On 02/17/2015 04:30 AM, Jiri Kosina wrote:
>>>> On Thu, 29 Jan 2015,linux@simon-woerner.de wrote:
>>>>
>>>>> From: Simon Wörner<mail@simon-woerner.de>
>>>>>
>>>>> HID: Add driver for acer keybard with broken rdesc
>>>> Hi Simon,
>>>>
>>>> to make sure proper device <-> driver binding is performed, you also have
>>>> to add device ID to the hid_have_special_driver[] array.
>>>>
>> To the cost of an error in the kernel log, the proper binding will be
>> done even if if there is no entry in hid_have_special_driver :)
>>
>> See Florian's log:
>> [ 3.153892] hid-generic 0003:06CB:2991.0003: usage index exceeded
>> [ 3.153896] hid-generic 0003:06CB:2991.0003: item 0 2 2 2 parsing failed
>> [ 3.153921] hid-generic: probe of 0003:06CB:2991.0003 failed with error -22
>>
>> So hid-generic will fail to bind the device, so normally hid-acer
>> should bind it, fix the report descriptor and the keyboard should be
>> working.
>>
>> Not very clean, but it should work :)
The other solutions looks like more work and at least some other problems.
So this "best" and preferred solution, or did i got any thing wrong?
>>> I have been meaning to respond to this patch. Unfortunately, simply adding
>>> this driver to the hid_have_special_driver[] list will prevent
>>> hid-multitouch from binding to the touchpad which shares the same vid and
>>> pid. My suggestion would be to create a new scan_flag for GD_KEYBOARD and to
>>> add to the code in hid_scan_collection() which scans the GENDESK usages.
>>> Similar to the code which is searching for HID_GD_POINTER. Then in
>>> hid_scan_report() we could assign a group for this driver based on the pid
>>> and the GD_KEYBOARD scan flag in the vendor handling code.
>> This could work, but that means that the list of special quirks after
>> the parsing is going to explode :(
>>
>> Plus we have to be sure that the scan_report doesn't fail or we will
>> not be able to tag the device correctly.
>>
>>> I can help out with implementing this part of the patch if there aren't any
>>> other suggestions. Adding a new group and more code to the hid_scan_report()
>>> vendor handling code is definately not ideal. But, I am not sure of a better
>>> way of binding different sub drivers to devices with the same vid and pid.
>>>
>>> An alternative would be to have hid-rmi handle all Synaptics touchpads, even
>>> the ones which currently use hid-multitouch. Then the keyboard report
>>> descriptor fixup could just be handled in hid-rmi. Accessing the finger data
>>> via rmi mode would also have the benefit of being able to report events
>>> which are currently not supported by the PTP collection (ie ABS_MT_PRESSURE,
>>> ABS_MT_TOUCH_MAJOR/MINOR). The touchpads which currently use hid-multitouch
>>> report finger data a little differently then the ones currently using
>>> hid-rmi so there is some work which would need to be done to add support for
>>> them in hid-rmi.
>>>
>> I am not sure we want to do that. Because we don't want hid-rmi to fix
>> all crappy keyboards around when the OEM reuses the synaptics PID.
>>
>> Maybe a better way of handling such situation is to provide a generic
>> mechanism to overwrite/patch the report descriptor so that we could
>> get rid of the drivers which just fix the report descriptor.
>> I have on a branch a version where I look into /lib/firmware/hid if
>> there is a matching device and a matching report descriptor. Then, I
>> read this firmware dynamically and patch the report descriptor.
>> This however requires that the hid modules are not included directly
>> in the kernel, or the /lib/firmware dir is not available and the
>> device doesn't bind.
>>
>> Cheers,
>> Benjamin
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-input" in
>> the body of a message tomajordomo@vger.kernel.org
>> More majordomo info athttp://vger.kernel.org/majordomo-info.html
>>
Is there any point i missed or we're good to go now? (expect the new
device id)
Greets,
Simon
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2015-03-24 16:10 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-01-23 19:03 HID: Add driver for synaptics keybard with broken rdesc Simon Wörner
2015-01-23 19:03 ` [PATCH] " Simon Wörner
2015-01-27 10:17 ` Jiri Kosina
2015-01-27 14:16 ` Benjamin Tissoires
2015-01-27 15:10 ` Simon Wörner
2015-01-27 16:13 ` Benjamin Tissoires
2015-01-27 23:46 ` Andrew Duggan
2015-01-28 4:04 ` Simon Wörner
2015-01-29 0:43 ` [PATCH] HID: Add driver for acer " linux
2015-02-17 12:30 ` Jiri Kosina
2015-02-18 2:01 ` Andrew Duggan
2015-02-27 14:16 ` Benjamin Tissoires
2015-02-27 14:28 ` Florian Echtler
2015-03-24 16:03 ` Simon Wörner
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.