[prev in list] [next in list] [prev in thread] [next in thread]
List: linux-sound
Subject: Re: [PATCH v2 1/2] hid-lenovo: Add support for X1 Tablet special keys and LED control
From: Dennis Wassenberg <dennis.wassenberg () secunet ! com>
Date: 2016-12-09 12:01:44
Message-ID: f8c4a4b7-c524-2742-2903-25bb6b4d565a () secunet ! com
[Download RAW message or body]
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@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.
>
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@lentin.co.uk>
> > > > + * Copyright (c) 2016 Dennis Wassenberg <dennis.wassenberg@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.
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@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-sound" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic