Hi Benjamin,
thank you for answering my questions.
I have a few comments inlined. There are just 2 additional questions.
Best regards,
Dennis
On 15.09.2016 10:11, Benjamin Tissoires wrote:
> 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.wassenberg@xxxxxxxxxxx>
>>>> ---
>>>> 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.
>
Understood.
>>>> 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 <jm@xxxxxxxxxxxx>
>>>> + * Copyright (c) 2016 Dennis Wassenberg <dennis.wassenberg@xxxxxxxxxxx>
>>>> */
>>>>
>>>> /*
>>>> @@ -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.
I implemented the 2nd alternative you explained. But additionally I need an array/list for every lenovo_led_type to save the current state of a led type. If the devices are always connected I don't need it, I would save the state in led_table_entry or in the drvdata. But in case of that MUTE or MICMUTE toggles via thinkpad_helper (e.g. via amixer) I want the hid_lenovo driver keep updated but I can not store it at the drvdata or led_table_entry because it is not available. Thats why I would like to introduce a static array which stores the led state of each led type. If a new device is attached I can query this array and set the led of the new device appropriately. Do you agree?
>>>> +
>>>> 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).
>
I can do so, yes. I have to reordner the enum. Because I introduce the enum, it is possible. But this should be an enum representing keys for every device which is handled by the hid-lenovo driver. Is there a guarantee that the order/values of the enum will not change in the future? If not the driver will not work after that. I tried to avoid making a hardware specific value dependent from a common enumeration. If it is guaranteed I will change it.
>>> 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.
>>>
>> 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.
>
Yes, its working. Thanks for this hint!
>>> 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.
>
Can not happen there.
>>>> +
>>>> + 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
>
ok.
>>>> +
>>>> +
>>>> + 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-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
