From: Benjamin Tissoires <benjamin.tissoires@gmail.com>
To: "Simon Wörner" <linux@simon-woerner.de>
Cc: "Jiri Kosina" <jkosina@suse.cz>,
linux-input <linux-input@vger.kernel.org>,
"Simon Wörner" <mail@simon-woerner.de>
Subject: Re: [PATCH] HID: Add driver for synaptics keybard with broken rdesc
Date: Tue, 27 Jan 2015 09:16:10 -0500 [thread overview]
Message-ID: <CAN+gG=FWsAqM-UZS0_1j1-5qdqTw93=ccjpe-kRf3knYwm43Kw@mail.gmail.com> (raw)
In-Reply-To: <1422039810-26327-2-git-send-email-linux@simon-woerner.de>
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
next prev parent reply other threads:[~2015-01-27 14:16 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAN+gG=FWsAqM-UZS0_1j1-5qdqTw93=ccjpe-kRf3knYwm43Kw@mail.gmail.com' \
--to=benjamin.tissoires@gmail.com \
--cc=jkosina@suse.cz \
--cc=linux-input@vger.kernel.org \
--cc=linux@simon-woerner.de \
--cc=mail@simon-woerner.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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.