On Sep 15 2016 or thereabouts, Dennis Wassenberg wrote:
> Hi Benjamin,
>
> Thank you for your comments. I will prepare new patches to replace this
> one. It will take some weeks to do this (vacation, other work). Would
> you please answer my inquiries inlined before I do this.
>
> Best regards,
>
> Dennis
>
> On 14.09.2016 17:32, Benjamin Tissoires wrote:
> > Hi Dennis,
> >
> > On Sep 14 2016 or thereabouts, Dennis Wassenberg wrote:
> >> The Lenovo X1 Tablet Cover is connected via USB. It constists of
> >> 1 device with 3 usb interfaces. Interface 0 represents keyboard,
> >> interface 1 the function / special keys and LED control, interface 2
> >> is the Synaptics touchpad and pointing stick.
> >>
> >> This driver will bind to interfaces 0 and 1 and handle function / special
> >> keys
> >> including LED control.
> >> HID: add device id for Lenovo X1 Tablet Cover
> >>
> >> Signed-off-by: Dennis Wassenberg <dennis.wassenb...@secunet.com>
> >> ---
> >> Changes v1 -> v2 (suggested by Benjamin Tissoires):
> >> * Squashed add of device IDs for Lenovo Thinkpad X1 Tablet Cover together
> >> with add of support for Lenovo Thinkpad X1 Tablet Cover into one patch
> >>
> >
> > I wanted to review the first version, but got sidetracked.
> >
> > So here it comes :)
> >
> >>
> >> drivers/hid/hid-core.c | 1 +
> >> drivers/hid/hid-ids.h | 1 +
> >> drivers/hid/hid-lenovo.c | 549
> >> +++++++++++++++++++++++++++++++++++++++++++++
> >
> > This seems to be too big for a single patch. Especially when you have
> > actually several changes that could be split for easier reviewing (LED,
> > special keys and keys stuck at least).
> >
> Whats the difference between special keys and keys stuck? This code will
> mainly handle special keys and LED control. I can split these 2 thinks.
> But I currently don't know where to do a third split.
IMO, in the first case, you are mapping unmapped keys to their proper
KEY_* event. In the other case, you are forcing the release of the keys,
which is not something we generally do given that you should be notified
of the release. Splitting this is 2 would allow to better understand the
changes and revert one if needed.
> >> include/linux/hid-lenovo.h | 15 ++
> >> 4 files changed, 566 insertions(+)
> >> create mode 100644 include/linux/hid-lenovo.h
> >>
> >> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> >> index 6add0b6..ba6a200 100644
> >> --- a/drivers/hid/hid-core.c
> >> +++ b/drivers/hid/hid-core.c
> >> @@ -2111,6 +2111,7 @@ void hid_disconnect(struct hid_device *hdev)
> >> { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_NINTENDO,
> >> USB_DEVICE_ID_NINTENDO_WIIMOTE2) },
> >> { HID_USB_DEVICE(USB_VENDOR_ID_RAZER, USB_DEVICE_ID_RAZER_BLADE_14) },
> >> { HID_USB_DEVICE(USB_VENDOR_ID_CMEDIA, USB_DEVICE_ID_CM6533) },
> >> + { HID_USB_DEVICE(USB_VENDOR_ID_LENOVO, USB_DEVICE_ID_LENOVO_X1_COVER) },
> >
> > I know it's hard given the existing code, but please try to keep the
> > list sorted and insert your device in the appropriate place.
> >
> oh sorry, yes of course.
> >> { }
> >> };
> >>
> >> diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
> >> index 3466f0d..1f08fb5 100644
> >> --- a/drivers/hid/hid-ids.h
> >> +++ b/drivers/hid/hid-ids.h
> >> @@ -615,6 +615,7 @@
> >> #define USB_DEVICE_ID_LENOVO_CUSBKBD 0x6047
> >> #define USB_DEVICE_ID_LENOVO_CBTKBD 0x6048
> >> #define USB_DEVICE_ID_LENOVO_TPPRODOCK 0x6067
> >> +#define USB_DEVICE_ID_LENOVO_X1_COVER 0x6085
> >>
> >> #define USB_VENDOR_ID_LG 0x1fd2
> >> #define USB_DEVICE_ID_LG_MULTITOUCH 0x0064
> >> diff --git a/drivers/hid/hid-lenovo.c b/drivers/hid/hid-lenovo.c
> >> index 1ac4ff4..4251aac 100644
> >> --- a/drivers/hid/hid-lenovo.c
> >> +++ b/drivers/hid/hid-lenovo.c
> >> @@ -3,9 +3,11 @@
> >> * - ThinkPad USB Keyboard with TrackPoint (tpkbd)
> >> * - ThinkPad Compact Bluetooth Keyboard with TrackPoint (cptkbd)
> >> * - ThinkPad Compact USB Keyboard with TrackPoint (cptkbd)
> >> + * - ThinkPad X1 Cover USB Keyboard with TrackPoint and Touchpad
> >> (tpx1cover)
> >> *
> >> * Copyright (c) 2012 Bernhard Seibold
> >> * Copyright (c) 2014 Jamie Lentin <j...@lentin.co.uk>
> >> + * Copyright (c) 2016 Dennis Wassenberg <dennis.wassenb...@secunet.com>
> >> */
> >>
> >> /*
> >> @@ -19,11 +21,19 @@
> >> #include <linux/sysfs.h>
> >> #include <linux/device.h>
> >> #include <linux/hid.h>
> >> +#include <linux/hid-lenovo.h>
> >> #include <linux/input.h>
> >> #include <linux/leds.h>
> >>
> >> #include "hid-ids.h"
> >>
> >> +struct led_table_entry {
> >> + struct led_classdev *dev;
> >> + uint8_t state;
> >> +};
> >> +
> >> +static struct led_table_entry hid_lenovo_led_table[HID_LENOVO_LED_MAX];
> >
> > Ouch. Please no static arrays containing random devices. The device is
> > USB, so you could have several of its kind plugged at once.
> >
> > So, please include this array in the driver data in the hid device, and
> > if you need a list of hid device connected, use an actual list of struct
> > hid_device.
> > Well, you can also use a list of struct led_table_entry if you add a
> > field with the type, and iterate over the list for each call on the API
> > in case there are 2 or more LEDs of the same type.
> >
> Yes, you are right. For my use case (X1 Tablet only) this was sufficient
> to do it in this way:) X1 Tablet is only able to connect one
> ThinKeyboard Cover.
> >> +
> >> struct lenovo_drvdata_tpkbd {
> >> int led_state;
> >> struct led_classdev led_mute;
> >> @@ -42,6 +52,37 @@ struct lenovo_drvdata_cptkbd {
> >> int sensitivity;
> >> };
> >>
> >> +struct lenovo_drvdata_tpx1cover {
> >> + uint16_t led_state;
> >> + uint8_t fnlock_state;
> >> + uint8_t led_present;
> >> + struct led_classdev led_mute;
> >> + struct led_classdev led_micmute;
> >> + struct led_classdev led_fnlock;
> >> +};
> >> +
> >> +int hid_lenovo_led_set(int led_num, bool on)
> >
> > You are declaring an enum for LEDs, I'd prefer see it used here (so you
> > have to give it a name first).
> >
> ok.
> >> +{
> >> + struct led_classdev *dev;
> >> +
> >> + if (led_num >= HID_LENOVO_LED_MAX)
> >> + return -EINVAL;
> >> +
> >> + dev = hid_lenovo_led_table[led_num].dev;
> >> + hid_lenovo_led_table[led_num].state = on;
> >> +
> >> + if (!dev)
> >> + return -ENODEV;
> >> +
> >> + if (!dev->brightness_set)
> >> + return -ENODEV;
> >> +
> >> + dev->brightness_set(dev, on ? LED_FULL : LED_OFF);
> >> +
> >> + return 0;
> >> +}
> >> +EXPORT_SYMBOL_GPL(hid_lenovo_led_set);
> >> +
> >> #define map_key_clear(c) hid_map_usage_clear(hi, usage, bit, max, EV_KEY,
> >> (c))
> >>
> >> static const __u8 lenovo_pro_dock_need_fixup_collection[] = {
> >> @@ -86,6 +127,84 @@ static int lenovo_input_mapping_tpkbd(struct
> >> hid_device *hdev,
> >> return 0;
> >> }
> >>
> >> +static int lenovo_input_mapping_tpx1cover(struct hid_device *hdev,
> >> + struct hid_input *hi, struct hid_field *field,
> >> + struct hid_usage *usage, unsigned long **bit, int *max)
> >> +{
> >> + if ((usage->hid & HID_USAGE_PAGE) == HID_UP_CONSUMER) {
> >> + switch (usage->hid & HID_USAGE) {
> >> + case 0x0001: // Unknown keys -> Idenditied by usage index!
> >
> > Why don't use directly usage->hid and check for HID_CP_CONSUMERCONTROL
> > (0x000c0001)?
> >
> ok, I will use this.
>
> > Note that here you are working with keys while previously it was LED.
> > Split in 2 patches please.
> >
> ok.
> >
> >> + map_key_clear(KEY_UNKNOWN);
> >
> > why?
> > If the key doesn't seem to be used, please don't map it and return -1.
> >
> ok.
> >> + switch (usage->usage_index) {
> >> + case 0x8:
> >
> > It feels weird to have an hexadecimal representation when dealing with
> > indexes.
> >
> ok.
> >> + input_set_capability(hi->input, EV_KEY, KEY_FN);
> >
> > You should consider using map_key_clear(KEY_FN); instead. This way the
> > event handling code will be cheaper.
> >
> > Rince, wash reapeat in the following cases.
> >
> ok.
> >> + break;
> >> +
> >> + case 0x9:
> >> + input_set_capability(hi->input, EV_KEY,
> >> KEY_MICMUTE);
> >> + break;
> >> +
> >> + case 0xa:
> >> + input_set_capability(hi->input, EV_KEY,
> >> KEY_CONFIG);
> >> + break;
> >> +
> >> + case 0xb:
> >> + input_set_capability(hi->input, EV_KEY,
> >> KEY_SEARCH);
> >> + break;
> >> +
> >> + case 0xc:
> >> + input_set_capability(hi->input, EV_KEY,
> >> KEY_SETUP);
> >> + break;
> >> +
> >> + case 0xd:
> >> + input_set_capability(hi->input, EV_KEY,
> >> KEY_SWITCHVIDEOMODE);
> >> + break;
> >> +
> >> + case 0xe:
> >> + input_set_capability(hi->input, EV_KEY,
> >> KEY_RFKILL);
> >> + break;
> >> +
> >> + default:
> >> + return -1;
> >> + }
> >> +
> >> + return 1;
> >> +
> >> + case 0x006f: // Consumer.006f ---> Key.BrightnessUp
> >> + map_key_clear(KEY_BRIGHTNESSUP);
> >
> > Why are you overriding the existing behavior from hid-input if you are
> > using the same code?
> >
> > Just return 0 and hid-input will set the values for you.
> >
> > Rince, wash repeat for the rest of the cases.
> >
> ok.
> >> + return 1;
> >> +
> >> + case 0x0070: // Consumer.0070 ---> Key.BrightnessDown
> >> + map_key_clear(KEY_BRIGHTNESSDOWN);
> >> + return 1;
> >> +
> >> + case 0x00b7:// Consumer.00b7 ---> Key.StopCD
> >> + map_key_clear(KEY_STOPCD);
> >> + return 1;
> >> +
> >> + case 0x00cd: // Consumer.00cd ---> Key.PlayPause
> >> + map_key_clear(KEY_PLAYPAUSE);
> >> + return 1;
> >> +
> >> + case 0x00e0: // Consumer.00e0 ---> Absolute.Volume
> >> + return 0;
> >> + case 0x00e2: // Consumer.00e2 ---> Key.Mute
> >> + map_key_clear(KEY_MUTE);
> >> + return 1;
> >> +
> >> + case 0x00e9: // Consumer.00e9 ---> Key.VolumeUp
> >> + map_key_clear(KEY_VOLUMEUP);
> >> + return 1;
> >> +
> >> + case 0x00ea: // Consumer.00ea ---> Key.VolumeDown
> >> + map_key_clear(KEY_VOLUMEDOWN);
> >> + return 1;
> >> + }
> >> + }
> >> +
> >> + return 0;
> >> +}
> >> +
> >> static int lenovo_input_mapping_cptkbd(struct hid_device *hdev,
> >> struct hid_input *hi, struct hid_field *field,
> >> struct hid_usage *usage, unsigned long **bit, int *max)
> >> @@ -172,6 +291,9 @@ static int lenovo_input_mapping(struct hid_device
> >> *hdev,
> >> case USB_DEVICE_ID_LENOVO_CBTKBD:
> >> return lenovo_input_mapping_cptkbd(hdev, hi, field,
> >> usage, bit, max);
> >> + case USB_DEVICE_ID_LENOVO_X1_COVER:
> >> + return lenovo_input_mapping_tpx1cover(hdev, hi, field,
> >> + usage, bit, max);
> >> default:
> >> return 0;
> >> }
> >> @@ -362,6 +484,143 @@ static int lenovo_event_cptkbd(struct hid_device
> >> *hdev,
> >> return 0;
> >> }
> >>
> >> +static enum led_brightness lenovo_led_brightness_get_tpx1cover(
> >> + struct led_classdev *led_cdev)
> >> +{
> >> + struct device *dev = led_cdev->dev->parent;
> >> + struct hid_device *hdev = to_hid_device(dev);
> >> + struct lenovo_drvdata_tpx1cover *drv_data = hid_get_drvdata(hdev);
> >> + int led_nr = 0;
> >
> > Would be even better to use the enum.
> >
> ok.
> >> +
> >> + if (led_cdev == &drv_data->led_mute)
> >> + led_nr = 0;
> >> + else if (led_cdev == &drv_data->led_micmute)
> >> + led_nr = 1;
> >> + else if (led_cdev == &drv_data->led_fnlock)
> >> + led_nr = 2;
> >> + else
> >> + return LED_OFF;
> >> +
> >> + return drv_data->led_state & (1 << led_nr)
> >> + ? LED_FULL
> >> + : LED_OFF;
> >> +}
> >> +
> >> +static void lenovo_led_brightness_set_tpx1cover(struct led_classdev
> >> *led_cdev,
> >> + enum led_brightness value)
> >> +{
> >> + struct device *dev = led_cdev->dev->parent;
> >> + struct hid_device *hdev = to_hid_device(dev);
> >> + struct lenovo_drvdata_tpx1cover *drv_data = hid_get_drvdata(hdev);
> >> + struct hid_report *report;
> >> + int led_nr = -1;
> >
> > Likewise, the enum would be nice
> >
> ok.
> >> + int led_nr_hw = -1;
> >> +
> >> + if (led_cdev == &drv_data->led_mute) {
> >> + led_nr = 0;
> >> + led_nr_hw = 0x64;
> >
> > Are you sure you are not overriding bits in 0x44?
> >
> ???
See the comment just below.
It feels weird that you need to have 0x54, 0x64, 0x74 to set an LED. I
wouldn't be surprised if the (constant) 0x44 part is there to set a
different feature of the cover.
However, I might have a wrong view of the protocol and using those plain
values are probably enough (and maybe the firmware uses 0x44 as the flag
that you are working with LEDs).
> > If you reorder the enum, I'd say the led_nr_hw could be represented as:
> > ((led_nr + 1) << 4) | 0x44
> >
> > So I think this is too much to be just a coincidence.
> >
> ok.
> >> + } else if (led_cdev == &drv_data->led_micmute) {
> >> + led_nr = 1;
> >> + led_nr_hw = 0x74;
> >> + } else if (led_cdev == &drv_data->led_fnlock) {
> >> + led_nr = 2;
> >> + led_nr_hw = 0x54;
> >> + } else {
> >> + hid_warn(hdev, "Invalid LED to set.\n");
> >> + return;
> >> + }
> >> +
> >> + if (value == LED_OFF)
> >> + drv_data->led_state &= ~(1 << led_nr);
> >> + else
> >> + drv_data->led_state |= 1 << led_nr;
> >> +
> >> + report = hdev->report_enum[HID_OUTPUT_REPORT].report_id_hash[9];
> >> + if (report) {
> >> + report->field[0]->value[0] = led_nr_hw;
> >> + report->field[0]->value[1] = (drv_data->led_state & (1 <<
> >> led_nr))
> >> + ? 0x02 : 0x01;
> >> + hid_hw_request(hdev, report, HID_REQ_SET_REPORT);
> >> + }
> >> +}
> >> +
> >> +static int lenovo_event_tpx1cover(struct hid_device *hdev,
> >> + struct hid_field *field, struct hid_usage *usage, __s32 value)
> >> +{
> >> + int ret = 0;
> >> +
> >> + if ((usage->hid & HID_USAGE_PAGE) == HID_UP_CONSUMER
> >> + && (usage->hid & HID_USAGE) == 0x0001) {
> >> +
> >> + if (usage->usage_index == 0x8 && value == 1) {
> >> + struct lenovo_drvdata_tpx1cover *drv_data =
> >> hid_get_drvdata(hdev);
> >> +
> >> + if (drv_data && drv_data->led_present) {
> >> + drv_data->fnlock_state =
> >> lenovo_led_brightness_get_tpx1cover(
> >> + &drv_data->led_fnlock) ==
> >> LED_OFF ? 1 : 0;
> >> + lenovo_led_brightness_set_tpx1cover(
> >> + &drv_data->led_fnlock,
> >> + drv_data->fnlock_state ? LED_FULL :
> >> LED_OFF);
> >> + }
> >
> > This looks like a different semantic change where you sync the actual LED
> > with the incoming event.
> > This is not something we usually do from the kernel but rely on the
> > userspace to do it for us. Not sure about the FN lock state though.
> >
> In case of X1 Tablet the fn lock state and the fn lock led state is the
> same. You can only change the fn lock while changing the fn lock led
> state. I will check if it behaves different if I map KEY_FN appropriate.
> > Anyway, if this needs to be there, it should have its own patch
> >
> ok. I will double check if this is relly needed. If so I will put in
> into a separate patch.
Thanks.
> >> + }
> >> +
> >> + if (usage->usage_index == 0x9 && value == 1) {
> >> + input_event(field->hidinput->input, EV_KEY,
> >> KEY_MICMUTE, 1);
> >> + input_sync(field->hidinput->input);
> >> + input_event(field->hidinput->input, EV_KEY,
> >> KEY_MICMUTE, 0);
> >> + input_sync(field->hidinput->input);
> >> + ret = 1;
> >
> > Aren't you notified when the key is released?
> >
> Does map_key_clear work if a key can only identified by
> usage->usage_index and not by usage->hid? If yes, I can remove this.
Basically, when you have usage_index > 0, that means that the report
descriptor is declaring more than one field with the same usage in a
row. Given that you should use map_key_clear, you are mapping the HID
usage (the element in the array) to a KEY_* value and you should be
fine.
Consider that each field in the report gets assigned its own usage, and
usage->page, usage->usage and usage->usage_index are just a description
of the usage.
> Otherwise I have to track the previously pressed keys to not issue a key
> release of every special key each time one special key is pressed/released.
Just give a shot at it, but I am pretty confident it will work.
> > If so, you should just drop the change because you used map_key_clear
> > above and hid-input will simply do the right thing for you.
> >
> > If you are not notified, this is big enough a difference to have its own
> > patch.
> >
> > Rince wash repeat
> >
> >> + }
> >> +
> >> + if (usage->usage_index == 0xa && value == 1) {
> >> + input_event(field->hidinput->input, EV_KEY, KEY_CONFIG,
> >> 1);
> >> + input_sync(field->hidinput->input);
> >> + input_event(field->hidinput->input, EV_KEY, KEY_CONFIG,
> >> 0);
> >> + input_sync(field->hidinput->input);
> >> +
> >> + ret = 1;
> >> + }
> >> +
> >> + if (usage->usage_index == 0xb && value == 1) {
> >> + input_event(field->hidinput->input, EV_KEY, KEY_SEARCH,
> >> 1);
> >> + input_sync(field->hidinput->input);
> >> + input_event(field->hidinput->input, EV_KEY, KEY_SEARCH,
> >> 0);
> >> + input_sync(field->hidinput->input);
> >> +
> >> + ret = 1;
> >> + }
> >> +
> >> + if (usage->usage_index == 0xc && value == 1) {
> >> + input_event(field->hidinput->input, EV_KEY, KEY_SETUP,
> >> 1);
> >> + input_sync(field->hidinput->input);
> >> + input_event(field->hidinput->input, EV_KEY, KEY_SETUP,
> >> 0);
> >> + input_sync(field->hidinput->input);
> >> +
> >> + ret = 1;
> >> + }
> >> +
> >> + if (usage->usage_index == 0xd && value == 1) {
> >> + input_event(field->hidinput->input, EV_KEY,
> >> KEY_SWITCHVIDEOMODE, 1);
> >> + input_sync(field->hidinput->input);
> >> + input_event(field->hidinput->input, EV_KEY,
> >> KEY_SWITCHVIDEOMODE, 0);
> >> + input_sync(field->hidinput->input);
> >> +
> >> + ret = 1;
> >> + }
> >> +
> >> + if (usage->usage_index == 0xe && value == 1) {
> >> + input_event(field->hidinput->input, EV_KEY, KEY_RFKILL,
> >> 1);
> >> + input_sync(field->hidinput->input);
> >> + input_event(field->hidinput->input, EV_KEY, KEY_RFKILL,
> >> 0);
> >> + input_sync(field->hidinput->input);
> >> +
> >> + ret = 1;
> >> + }
> >> + }
> >> +
> >> + return ret;
> >> +}
> >> +
> >> static int lenovo_event(struct hid_device *hdev, struct hid_field *field,
> >> struct hid_usage *usage, __s32 value)
> >> {
> >> @@ -369,6 +628,8 @@ static int lenovo_event(struct hid_device *hdev,
> >> struct hid_field *field,
> >> case USB_DEVICE_ID_LENOVO_CUSBKBD:
> >> case USB_DEVICE_ID_LENOVO_CBTKBD:
> >> return lenovo_event_cptkbd(hdev, field, usage, value);
> >> + case USB_DEVICE_ID_LENOVO_X1_COVER:
> >> + return lenovo_event_tpx1cover(hdev, field, usage, value);
> >> default:
> >> return 0;
> >> }
> >> @@ -731,6 +992,251 @@ static int lenovo_probe_tpkbd(struct hid_device
> >> *hdev)
> >> return ret;
> >> }
> >>
> >> +static int lenovo_probe_tpx1cover_configure(struct hid_device *hdev)
> >> +{
> >> + struct hid_report *report =
> >> hdev->report_enum[HID_OUTPUT_REPORT].report_id_hash[9];
> >> + struct lenovo_drvdata_tpx1cover *drv_data = hid_get_drvdata(hdev);
> >> +
> >> + if (!drv_data)
> >> + return -ENODEV;
> >
> > Can this really happen?
> >
> I will remove this.
That was really a question, please make sure drv_data can't be null.
> >> +
> >> + if (!report)
> >> + return -ENOENT;
> >> +
> >> + report->field[0]->value[0] = 0x54;
> >> + report->field[0]->value[1] = 0x20;
> >> + hid_hw_request(hdev, report, HID_REQ_SET_REPORT);
> >> + hid_hw_wait(hdev);
> >> +
> >> + report->field[0]->value[0] = 0x54;
> >> + report->field[0]->value[1] = 0x08;
> >> + hid_hw_request(hdev, report, HID_REQ_SET_REPORT);
> >> + hid_hw_wait(hdev);
> >> +
> >> + report->field[0]->value[0] = 0xA0;
> >> + report->field[0]->value[1] = 0x02;
> >> + hid_hw_request(hdev, report, HID_REQ_SET_REPORT);
> >> + hid_hw_wait(hdev);
> >> +
> >> + lenovo_led_brightness_set_tpx1cover(&drv_data->led_mute,
> >> + hid_lenovo_led_table[HID_LENOVO_LED_MUTE].state ? LED_FULL :
> >> LED_OFF);
> >> + hid_hw_wait(hdev);
> >> +
> >> + lenovo_led_brightness_set_tpx1cover(&drv_data->led_micmute,
> >> + hid_lenovo_led_table[HID_LENOVO_LED_MICMUTE].state ? LED_FULL :
> >> LED_OFF);
> >> + hid_hw_wait(hdev);
> >> +
> >> + lenovo_led_brightness_set_tpx1cover(&drv_data->led_fnlock, LED_FULL);
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static int lenovo_probe_tpx1cover_special_functions(struct hid_device
> >> *hdev)
> >> +{
> >> + struct device *dev = &hdev->dev;
> >> + struct lenovo_drvdata_tpx1cover *drv_data = NULL;
> >> +
> >> + size_t name_sz = strlen(dev_name(dev)) + 16;
> >> + char *name_led = NULL;
> >> +
> >> + struct hid_report *report;
> >> + bool report_match = 1;
> >> +
> >> + int ret = 0;
> >> +
> >> + report = hid_validate_values(hdev, HID_INPUT_REPORT, 2, 0, 3);
> >> + report_match &= report ? 1 : 0;
> >> + report = hid_validate_values(hdev, HID_INPUT_REPORT, 3, 0, 16);
> >> + report_match &= report ? 1 : 0;
> >> + report = hid_validate_values(hdev, HID_OUTPUT_REPORT, 9, 0, 2);
> >> + report_match &= report ? 1 : 0;
> >> + report = hid_validate_values(hdev, HID_FEATURE_REPORT, 32, 0, 1);
> >> + report_match &= report ? 1 : 0;
> >> + report = hid_validate_values(hdev, HID_FEATURE_REPORT, 84, 0, 1);
> >> + report_match &= report ? 1 : 0;
> >> + report = hid_validate_values(hdev, HID_FEATURE_REPORT, 100, 0, 1);
> >> + report_match &= report ? 1 : 0;
> >> + report = hid_validate_values(hdev, HID_FEATURE_REPORT, 116, 0, 1);
> >> + report_match &= report ? 1 : 0;
> >> + report = hid_validate_values(hdev, HID_FEATURE_REPORT, 132, 0, 1);
> >> + report_match &= report ? 1 : 0;
> >> + report = hid_validate_values(hdev, HID_FEATURE_REPORT, 144, 0, 1);
> >> + report_match &= report ? 1 : 0;
> >> + report = hid_validate_values(hdev, HID_FEATURE_REPORT, 162, 0, 1);
> >> + report_match &= report ? 1 : 0;
> >
> > It feels weird to have you check if those reports are actually long
> > enough. I think this is related to checking which interface you have,
> > but you should be able to reduce the list to only those you are actually
> > using (report id "9" seems like a good candidate).
> >
> Yes, its related to select the USB Interface. I queried all existing
> reports of Interface 1 here. I will reduce this list but report id 9 is
> not sufficient because IF2 has report id 9 too. So I will add one other
> report id which enables a clear decision.
> > And please add a comment why you are checking some specific report IDs.
> >
> ok.
> >> +
> >> + if (!report_match) {
> >> + ret = -ENODEV;
> >> + goto err;
> >
> > just return -ENODEV here.
> >
> ok.
> >> + }
> >> +
> >> + drv_data = devm_kzalloc(&hdev->dev,
> >> + sizeof(struct lenovo_drvdata_tpx1cover),
> >> + GFP_KERNEL);
> >> +
> >> + if (!drv_data) {
> >> + hid_err(hdev,
> >> + "Could not allocate memory for tpx1cover driver
> >> data\n");
> >> + ret = -ENOMEM;
> >> + goto err;
> >
> > Just return -ENODEV too, the devres manager will kfree drv_data for you.
> >
> ok.
> >> + }
> >> +
> >> + name_led = devm_kzalloc(&hdev->dev, name_sz, GFP_KERNEL);
> >> + if (!name_led) {
> >> + hid_err(hdev, "Could not allocate memory for mute led data\n");
> >> + ret = -ENOMEM;
> >> + goto err_cleanup;
> >
> > likewise
> >
> ok.
> >> + }
> >> + snprintf(name_led, name_sz, "%s:amber:mute", dev_name(dev));
> >> +
> >> + drv_data->led_mute.name = name_led;
> >> + drv_data->led_mute.brightness_get = lenovo_led_brightness_get_tpx1cover;
> >> + drv_data->led_mute.brightness_set = lenovo_led_brightness_set_tpx1cover;
> >> + drv_data->led_mute.dev = dev;
> >> + hid_lenovo_led_table[HID_LENOVO_LED_MUTE].dev = &drv_data->led_mute;
> >> + led_classdev_register(dev, &drv_data->led_mute);
> >
> > Isn't devm_led_class_register working?
> >
> > That would be nice because you could drop all of your cleanup paths
> >
> Ah, ok, didn't know that function until yet.
I *think* it should be fine to use it with hid->dev. If you use it with
input->dev, that won't do and you'll need to do some weird things to
remove the LED device (sadly that's what I had to do in the wacom
driver).
Just make sure /sys/class/leds/ doesn't contain a dangling LED device
when you remove the cover and you should know if this worked or not.
Cheers,
Benjamin
> >> +
> >> +
> >> + name_led = devm_kzalloc(&hdev->dev, name_sz, GFP_KERNEL);
> >> + if (!name_led) {
> >> + hid_err(hdev,
> >> + "Could not allocate memory for mic mute led data\n");
> >> + ret = -ENOMEM;
> >> + goto err_cleanup;
> >
> > ditto
> >
> ok.
> >> + }
> >> + snprintf(name_led, name_sz, "%s:amber:micmute", dev_name(dev));
> >> +
> >> + drv_data->led_micmute.name = name_led;
> >> + drv_data->led_micmute.brightness_get =
> >> lenovo_led_brightness_get_tpx1cover;
> >> + drv_data->led_micmute.brightness_set =
> >> lenovo_led_brightness_set_tpx1cover;
> >> + drv_data->led_micmute.dev = dev;
> >> + hid_lenovo_led_table[HID_LENOVO_LED_MICMUTE].dev =
> >> &drv_data->led_micmute;
> >> + led_classdev_register(dev, &drv_data->led_micmute);
> >> +
> >> +
> >> + name_led = devm_kzalloc(&hdev->dev, name_sz, GFP_KERNEL);
> >> + if (!name_led) {
> >> + hid_err(hdev,
> >> + "Could not allocate memory for FN lock led data\n");
> >> + ret = -ENOMEM;
> >> + goto err_cleanup;
> >
> > ditto
> >
> ok.
> >> + }
> >> +
> >> + snprintf(name_led, name_sz, "%s:amber:fnlock", dev_name(dev));
> >> +
> >> + drv_data->led_fnlock.name = name_led;
> >> + drv_data->led_fnlock.brightness_get =
> >> lenovo_led_brightness_get_tpx1cover;
> >> + drv_data->led_fnlock.brightness_set =
> >> lenovo_led_brightness_set_tpx1cover;
> >> + drv_data->led_fnlock.dev = dev;
> >> + hid_lenovo_led_table[HID_LENOVO_LED_FNLOCK].dev = &drv_data->led_fnlock;
> >> + led_classdev_register(dev, &drv_data->led_fnlock);
> >
> > ditto
> >
> ok.
> >> +
> >> + drv_data->led_state = 0;
> >> + drv_data->fnlock_state = 1;
> >> + drv_data->led_present = 1;
> >> +
> >> + hid_set_drvdata(hdev, drv_data);
> >> +
> >> + return lenovo_probe_tpx1cover_configure(hdev);
> >> +
> >> +err_cleanup:
> >
> > Shouldn't be required if you use devm_led_classdev_register().
> >
> ok.
> >> + if (drv_data->led_fnlock.name) {
> >> + led_classdev_unregister(&drv_data->led_fnlock);
> >> + devm_kfree(&hdev->dev, (void *) drv_data->led_fnlock.name);
> >> + }
> >> +
> >> + if (drv_data->led_micmute.name) {
> >> + led_classdev_unregister(&drv_data->led_micmute);
> >> + devm_kfree(&hdev->dev, (void *) drv_data->led_micmute.name);
> >> + }
> >> +
> >> + if (drv_data->led_mute.name) {
> >> + led_classdev_unregister(&drv_data->led_mute);
> >> + devm_kfree(&hdev->dev, (void *) drv_data->led_mute.name);
> >> + }
> >> +
> >> + if (drv_data)
> >> + kfree(drv_data);
> >> +
> >> +err:
> >> + return ret;
> >> +}
> >> +
> >> +static int lenovo_probe_tpx1cover_touch(struct hid_device *hdev)
> >> +{
> >> + struct hid_report *report;
> >> + bool report_match = 1;
> >> + int ret = 0;
> >> +
> >> + report = hid_validate_values(hdev, HID_INPUT_REPORT, 2, 0, 2);
> >> + report_match &= report ? 1 : 0;
> >> + report = hid_validate_values(hdev, HID_INPUT_REPORT, 2, 1, 2);
> >> + report_match &= report ? 1 : 0;
> >> + report = hid_validate_values(hdev, HID_INPUT_REPORT, 11, 0, 61);
> >> + report_match &= report ? 1 : 0;
> >> + report = hid_validate_values(hdev, HID_INPUT_REPORT, 12, 0, 61);
> >> + report_match &= report ? 1 : 0;
> >> + report = hid_validate_values(hdev, HID_INPUT_REPORT, 16, 0, 3);
> >> + report_match &= report ? 1 : 0;
> >> + report = hid_validate_values(hdev, HID_INPUT_REPORT, 16, 1, 2);
> >> + report_match &= report ? 1 : 0;
> >> + report = hid_validate_values(hdev, HID_OUTPUT_REPORT, 9, 0, 20);
> >> + report_match &= report ? 1 : 0;
> >> + report = hid_validate_values(hdev, HID_OUTPUT_REPORT, 10, 0, 20);
> >> + report_match &= report ? 1 : 0;
> >> + report = hid_validate_values(hdev, HID_FEATURE_REPORT, 14, 0, 1);
> >> + report_match &= report ? 1 : 0;
> >> + report = hid_validate_values(hdev, HID_FEATURE_REPORT, 15, 0, 3);
> >> + report_match &= report ? 1 : 0;
> >
> > Feels weird too (especially if there is no comment explaining why you
> > are doing those checks).
> >
> ok.
> >> +
> >> + if (!report_match)
> >> + ret = -ENODEV;
> >> +
> >> + return ret;
> >> +}
> >> +
> >> +static int lenovo_probe_tpx1cover(struct hid_device *hdev)
> >> +{
> >> + int ret = 0;
> >> +
> >> + /*
> >> + * Probing for special function keys and LED control -> usb intf 1
> >> + * Probing for touch input -> usb intf 2 (handled by rmi4 driver)
> >> + * Other (keyboard) -> usb intf 0
> >> + */
> >> + if (!lenovo_probe_tpx1cover_special_functions(hdev)) {
> >> + // special function keys and LED control
> >
> > No C++ style comments please.
> >
> Oops
> >> + ret = 0;
> >> + } else if (!lenovo_probe_tpx1cover_touch(hdev)) {
> >> + // handled by rmi
> >> + ret = -ENODEV;
> >
> > I don't quite get how the touch can be handled if you just return
> > -ENODEV here. Given that the device has been added to
> > hid_have_special_driver, if you don't claim the device, no one else will
> > unless you add the ID in the other HID driver.
> >
> Yes, ok. This is because I have some additional RMI4 code which handles
> the touch. At the current upstream situation I should return 0 here such
> that this touch works basically.
> >> + } else {
> >> + // keyboard
> >
> > Why is that keyboard chunk not given it's own probe function?
> >
> Because it is just a few lines:) I will add a probe function.
> >> + struct lenovo_drvdata_tpx1cover *drv_data;
> >> +
> >> + drv_data = devm_kzalloc(&hdev->dev,
> >> + sizeof(struct lenovo_drvdata_tpx1cover),
> >> + GFP_KERNEL);
> >> +
> >> + if (!drv_data) {
> >> + hid_err(hdev,
> >> + "Could not allocate memory for tpx1cover driver
> >> data\n");
> >> + ret = -ENOMEM;
> >> + goto out;
> >
> > no need for a goto here. Just a plain return -ENOMEM should be good
> > enough.
> >
> ok
> >> + }
> >> +
> >> + drv_data->led_state = 0;
> >> + drv_data->led_present = 0;
> >> + drv_data->fnlock_state = 0;
> >> + hid_set_drvdata(hdev, drv_data);
> >> +
> >> + ret = 0;
> >> + }
> >> +
> >> +out:
> >> + return ret;
> >> +}
> >> +
> >> static int lenovo_probe_cptkbd(struct hid_device *hdev)
> >> {
> >> int ret;
> >> @@ -803,6 +1309,9 @@ static int lenovo_probe(struct hid_device *hdev,
> >> case USB_DEVICE_ID_LENOVO_CBTKBD:
> >> ret = lenovo_probe_cptkbd(hdev);
> >> break;
> >> + case USB_DEVICE_ID_LENOVO_X1_COVER:
> >> + ret = lenovo_probe_tpx1cover(hdev);
> >> + break;
> >> default:
> >> ret = 0;
> >> break;
> >> @@ -843,6 +1352,42 @@ static void lenovo_remove_cptkbd(struct hid_device
> >> *hdev)
> >> &lenovo_attr_group_cptkbd);
> >> }
> >>
> >> +static void lenovo_remove_tpx1cover(struct hid_device *hdev)
> >> +{
> >> + struct lenovo_drvdata_tpx1cover *drv_data = hid_get_drvdata(hdev);
> >> +
> >> + if (!drv_data)
> >> + return;
> >> +
> >> + if (drv_data->led_present) {
> >> + if (drv_data->led_fnlock.name) {
> >> + hid_lenovo_led_table[HID_LENOVO_LED_FNLOCK].dev = NULL;
> >> +
> >> + led_classdev_unregister(&drv_data->led_fnlock);
> >> + devm_kfree(&hdev->dev, (void *)
> >> drv_data->led_fnlock.name);
> >
> > Calling yourself devm_kfree usually means there is something wrong.
> > Here, if you used devm_led_*, you could just drop the entire remove
> > function.
> >
> ok.
> >> + }
> >> +
> >> + if (drv_data->led_micmute.name) {
> >> + hid_lenovo_led_table[HID_LENOVO_LED_MICMUTE].dev = NULL;
> >> +
> >> + led_classdev_unregister(&drv_data->led_micmute);
> >> + devm_kfree(&hdev->dev, (void *)
> >> drv_data->led_micmute.name);
> >> + }
> >> +
> >> + if (drv_data->led_mute.name) {
> >> + hid_lenovo_led_table[HID_LENOVO_LED_MUTE].dev = NULL;
> >> +
> >> + led_classdev_unregister(&drv_data->led_mute);
> >> + devm_kfree(&hdev->dev, (void *)
> >> drv_data->led_mute.name);
> >> + }
> >> + }
> >> +
> >> + if (drv_data)
> >> + devm_kfree(&hdev->dev, drv_data);
> >> +
> >> + hid_set_drvdata(hdev, NULL);
> >> +}
> >> +
> >> static void lenovo_remove(struct hid_device *hdev)
> >> {
> >> switch (hdev->product) {
> >> @@ -853,6 +1398,9 @@ static void lenovo_remove(struct hid_device *hdev)
> >> case USB_DEVICE_ID_LENOVO_CBTKBD:
> >> lenovo_remove_cptkbd(hdev);
> >> break;
> >> + case USB_DEVICE_ID_LENOVO_X1_COVER:
> >> + lenovo_remove_tpx1cover(hdev);
> >> + break;
> >> }
> >>
> >> hid_hw_stop(hdev);
> >> @@ -883,6 +1431,7 @@ static int lenovo_input_configured(struct hid_device
> >> *hdev,
> >> { HID_USB_DEVICE(USB_VENDOR_ID_LENOVO, USB_DEVICE_ID_LENOVO_CUSBKBD) },
> >> { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LENOVO,
> >> USB_DEVICE_ID_LENOVO_CBTKBD) },
> >> { HID_USB_DEVICE(USB_VENDOR_ID_LENOVO, USB_DEVICE_ID_LENOVO_TPPRODOCK)
> >> },
> >> + { HID_USB_DEVICE(USB_VENDOR_ID_LENOVO, USB_DEVICE_ID_LENOVO_X1_COVER) },
> >> { }
> >> };
> >>
> >> diff --git a/include/linux/hid-lenovo.h b/include/linux/hid-lenovo.h
> >> new file mode 100644
> >> index 0000000..d0b0145
> >> --- /dev/null
> >> +++ b/include/linux/hid-lenovo.h
> >> @@ -0,0 +1,15 @@
> >> +
> >> +#ifndef __HID_LENOVO_H__
> >> +#define __HID_LENOVO_H__
> >> +
> >> +
> >> +enum {
> >> + HID_LENOVO_LED_MUTE,
> >
> > I'd rather have a name for the enum (so you can reuse it), and also have
> > each enum given its numerical value (or at least the first and last.
> >
> ok.
> >> + HID_LENOVO_LED_MICMUTE,
> >> + HID_LENOVO_LED_FNLOCK,
> >> + HID_LENOVO_LED_MAX,
> >> +};
> >> +
> >> +int hid_lenovo_led_set(int led_num, bool on);
> >> +
> >> +#endif /* __HID_LENOVO_H_ */
> >> --
> >> 1.i9.1
> >
> >
> > Cheers,
> > Benjamin
> >
--
To unsubscribe from this list: send the line "unsubscribe linux-sound" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
