Re: [PATCH v2 1/2] hid-lenovo: Add support for X1 Tablet special keys and LED control

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux Media Devel]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Linux Wireless Networking]     [Linux Omap]

  Powered by Linux