On Tue, Jan 27, 2015 at 10:10 AM, Simon Wörner <linux@xxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxx> wrote:
>>> From: Simon Wörner <mail@xxxxxxxxxxxxxxxx>
>>>
>>> Signed-off-by: Simon Wörner <mail@xxxxxxxxxxxxxxxx>
>>> ---
>>> 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@xxxxxxxxxxxxxxx
>>> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
