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]

 



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.

> >>  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.
> >> +
> >>  struct lenovo_drvdata_tpkbd {
> >>  	int led_state;
> >>  	struct led_classdev led_mute;
> >> @@ -42,6 +52,37 @@ struct lenovo_drvdata_cptkbd {
> >>  	int sensitivity;
> >>  };
> >>  
> >> +struct lenovo_drvdata_tpx1cover {
> >> +	uint16_t led_state;
> >> +	uint8_t fnlock_state;
> >> +	uint8_t led_present;
> >> +	struct led_classdev led_mute;
> >> +	struct led_classdev led_micmute;
> >> +	struct led_classdev led_fnlock;
> >> +};
> >> +
> >> +int hid_lenovo_led_set(int led_num, bool on)
> > 
> > You are declaring an enum for LEDs, I'd prefer see it used here (so you
> > have to give it a name first).
> > 
> ok.
> >> +{
> >> +	struct led_classdev *dev;
> >> +
> >> +	if (led_num >= HID_LENOVO_LED_MAX)
> >> +		return -EINVAL;
> >> +
> >> +	dev = hid_lenovo_led_table[led_num].dev;
> >> +	hid_lenovo_led_table[led_num].state = on;
> >> +
> >> +	if (!dev)
> >> +		return -ENODEV;
> >> +
> >> +	if (!dev->brightness_set)
> >> +		return -ENODEV;
> >> +
> >> +	dev->brightness_set(dev, on ? LED_FULL : LED_OFF);
> >> +
> >> +	return 0;
> >> +}
> >> +EXPORT_SYMBOL_GPL(hid_lenovo_led_set);
> >> +
> >>  #define map_key_clear(c) hid_map_usage_clear(hi, usage, bit, max, EV_KEY, (c))
> >>  
> >>  static const __u8 lenovo_pro_dock_need_fixup_collection[] = {
> >> @@ -86,6 +127,84 @@ static int lenovo_input_mapping_tpkbd(struct hid_device *hdev,
> >>  	return 0;
> >>  }
> >>  
> >> +static int lenovo_input_mapping_tpx1cover(struct hid_device *hdev,
> >> +		struct hid_input *hi, struct hid_field *field,
> >> +		struct hid_usage *usage, unsigned long **bit, int *max)
> >> +{
> >> +	if ((usage->hid & HID_USAGE_PAGE) == HID_UP_CONSUMER) {
> >> +		switch (usage->hid & HID_USAGE) {
> >> +		case 0x0001: // Unknown keys -> Idenditied by usage index!
> > 
> > Why don't use directly usage->hid and check for HID_CP_CONSUMERCONTROL (0x000c0001)?
> > 
> ok, I will use this.
> 
> > Note that here you are working with keys while previously it was LED.
> > Split in 2 patches please.
> > 
> ok.
> > 
> >> +			map_key_clear(KEY_UNKNOWN);
> > 
> > why?
> > If the key doesn't seem to be used, please don't map it and return -1.
> > 
> ok.
> >> +			switch (usage->usage_index) {
> >> +			case 0x8:
> > 
> > It feels weird to have an hexadecimal representation when dealing with
> > indexes.
> > 
> ok.
> >> +				input_set_capability(hi->input, EV_KEY, KEY_FN);
> > 
> > You should consider using map_key_clear(KEY_FN); instead. This way the
> > event handling code will be cheaper.
> > 
> > Rince, wash reapeat in the following cases.
> > 
> ok.
> >> +				break;
> >> +
> >> +			case 0x9:
> >> +				input_set_capability(hi->input, EV_KEY, KEY_MICMUTE);
> >> +				break;
> >> +
> >> +			case 0xa:
> >> +				input_set_capability(hi->input, EV_KEY, KEY_CONFIG);
> >> +				break;
> >> +
> >> +			case 0xb:
> >> +				input_set_capability(hi->input, EV_KEY, KEY_SEARCH);
> >> +				break;
> >> +
> >> +			case 0xc:
> >> +				input_set_capability(hi->input, EV_KEY, KEY_SETUP);
> >> +				break;
> >> +
> >> +			case 0xd:
> >> +				input_set_capability(hi->input, EV_KEY, KEY_SWITCHVIDEOMODE);
> >> +				break;
> >> +
> >> +			case 0xe:
> >> +				input_set_capability(hi->input, EV_KEY, KEY_RFKILL);
> >> +				break;
> >> +
> >> +			default:
> >> +				return -1;
> >> +			}
> >> +
> >> +			return 1;
> >> +
> >> +		case 0x006f: // Consumer.006f ---> Key.BrightnessUp
> >> +			map_key_clear(KEY_BRIGHTNESSUP);
> > 
> > Why are you overriding the existing behavior from hid-input if you are
> > using the same code?
> > 
> > Just return 0 and hid-input will set the values for you.
> > 
> > Rince, wash repeat for the rest of the cases.
> > 
> ok.
> >> +			return 1;
> >> +
> >> +		case 0x0070: // Consumer.0070 ---> Key.BrightnessDown
> >> +			map_key_clear(KEY_BRIGHTNESSDOWN);
> >> +			return 1;
> >> +
> >> +		case 0x00b7:// Consumer.00b7 ---> Key.StopCD
> >> +			map_key_clear(KEY_STOPCD);
> >> +			return 1;
> >> +
> >> +		case 0x00cd: // Consumer.00cd ---> Key.PlayPause
> >> +			map_key_clear(KEY_PLAYPAUSE);
> >> +			return 1;
> >> +
> >> +		case 0x00e0: // Consumer.00e0 ---> Absolute.Volume
> >> +			return 0;
> >> +		case 0x00e2: // Consumer.00e2 ---> Key.Mute
> >> +			map_key_clear(KEY_MUTE);
> >> +			return 1;
> >> +
> >> +		case 0x00e9: // Consumer.00e9 ---> Key.VolumeUp
> >> +			map_key_clear(KEY_VOLUMEUP);
> >> +			return 1;
> >> +
> >> +		case 0x00ea: // Consumer.00ea ---> Key.VolumeDown
> >> +			map_key_clear(KEY_VOLUMEDOWN);
> >> +			return 1;
> >> +		}
> >> +	}
> >> +
> >> +	return 0;
> >> +}
> >> +
> >>  static int lenovo_input_mapping_cptkbd(struct hid_device *hdev,
> >>  		struct hid_input *hi, struct hid_field *field,
> >>  		struct hid_usage *usage, unsigned long **bit, int *max)
> >> @@ -172,6 +291,9 @@ static int lenovo_input_mapping(struct hid_device *hdev,
> >>  	case USB_DEVICE_ID_LENOVO_CBTKBD:
> >>  		return lenovo_input_mapping_cptkbd(hdev, hi, field,
> >>  							usage, bit, max);
> >> +	case USB_DEVICE_ID_LENOVO_X1_COVER:
> >> +		return lenovo_input_mapping_tpx1cover(hdev, hi, field,
> >> +							usage, bit, max);
> >>  	default:
> >>  		return 0;
> >>  	}
> >> @@ -362,6 +484,143 @@ static int lenovo_event_cptkbd(struct hid_device *hdev,
> >>  	return 0;
> >>  }
> >>  
> >> +static enum led_brightness lenovo_led_brightness_get_tpx1cover(
> >> +			struct led_classdev *led_cdev)
> >> +{
> >> +	struct device *dev = led_cdev->dev->parent;
> >> +	struct hid_device *hdev = to_hid_device(dev);
> >> +	struct lenovo_drvdata_tpx1cover *drv_data = hid_get_drvdata(hdev);
> >> +	int led_nr = 0;
> > 
> > Would be even better to use the enum.
> > 
> ok.
> >> +
> >> +	if (led_cdev == &drv_data->led_mute)
> >> +		led_nr = 0;
> >> +	else if (led_cdev == &drv_data->led_micmute)
> >> +		led_nr = 1;
> >> +	else if (led_cdev == &drv_data->led_fnlock)
> >> +		led_nr = 2;
> >> +	else
> >> +		return LED_OFF;
> >> +
> >> +	return drv_data->led_state & (1 << led_nr)
> >> +				? LED_FULL
> >> +				: LED_OFF;
> >> +}
> >> +
> >> +static void lenovo_led_brightness_set_tpx1cover(struct led_classdev *led_cdev,
> >> +			enum led_brightness value)
> >> +{
> >> +	struct device *dev = led_cdev->dev->parent;
> >> +	struct hid_device *hdev = to_hid_device(dev);
> >> +	struct lenovo_drvdata_tpx1cover *drv_data = hid_get_drvdata(hdev);
> >> +	struct hid_report *report;
> >> +	int led_nr = -1;
> > 
> > Likewise, the enum would be nice
> > 
> ok.
> >> +	int led_nr_hw = -1;
> >> +
> >> +	if (led_cdev == &drv_data->led_mute) {
> >> +		led_nr = 0;
> >> +		led_nr_hw = 0x64;
> > 
> > Are you sure you are not overriding bits in 0x44?
> > 
> ???

See the comment just below.
It feels weird that you need to have 0x54, 0x64, 0x74 to set an LED. I
wouldn't be surprised if the (constant) 0x44 part is there to set a
different feature of the cover.

However, I might have a wrong view of the protocol and using those plain
values are probably enough (and maybe the firmware uses 0x44 as the flag
that you are working with LEDs).

> > If you reorder the enum, I'd say the led_nr_hw could be represented as:
> > ((led_nr + 1) << 4) | 0x44
> > 
> > So I think this is too much to be just a coincidence.
> > 
> ok.
> >> +	} else if (led_cdev == &drv_data->led_micmute) {
> >> +		led_nr = 1;
> >> +		led_nr_hw = 0x74;
> >> +	} else if (led_cdev == &drv_data->led_fnlock) {
> >> +		led_nr = 2;
> >> +		led_nr_hw = 0x54;
> >> +	} else {
> >> +		hid_warn(hdev, "Invalid LED to set.\n");
> >> +		return;
> >> +	}
> >> +
> >> +	if (value == LED_OFF)
> >> +		drv_data->led_state &= ~(1 << led_nr);
> >> +	else
> >> +		drv_data->led_state |= 1 << led_nr;
> >> +
> >> +	report = hdev->report_enum[HID_OUTPUT_REPORT].report_id_hash[9];
> >> +	if (report) {
> >> +		report->field[0]->value[0] = led_nr_hw;
> >> +		report->field[0]->value[1] = (drv_data->led_state & (1 << led_nr))
> >> +			? 0x02 : 0x01;
> >> +		hid_hw_request(hdev, report, HID_REQ_SET_REPORT);
> >> +	}
> >> +}
> >> +
> >> +static int lenovo_event_tpx1cover(struct hid_device *hdev,
> >> +		struct hid_field *field, struct hid_usage *usage, __s32 value)
> >> +{
> >> +	int ret = 0;
> >> +
> >> +	if ((usage->hid & HID_USAGE_PAGE) == HID_UP_CONSUMER
> >> +		&& (usage->hid & HID_USAGE) == 0x0001) {
> >> +
> >> +		if (usage->usage_index == 0x8 && value == 1) {
> >> +			struct lenovo_drvdata_tpx1cover *drv_data = hid_get_drvdata(hdev);
> >> +
> >> +			if (drv_data && drv_data->led_present) {
> >> +				drv_data->fnlock_state = lenovo_led_brightness_get_tpx1cover(
> >> +						&drv_data->led_fnlock) == LED_OFF ? 1 : 0;
> >> +				lenovo_led_brightness_set_tpx1cover(
> >> +					&drv_data->led_fnlock,
> >> +					drv_data->fnlock_state ? LED_FULL : LED_OFF);
> >> +			}
> > 
> > This looks like a different semantic change where you sync the actual LED with the incoming event.
> > This is not something we usually do from the kernel but rely on the
> > userspace to do it for us. Not sure about the FN lock state though.
> > 
> In case of X1 Tablet the fn lock state and the fn lock led state is the
> same. You can only change the fn lock while changing the fn lock led
> state. I will check if it behaves different if I map KEY_FN 	appropriate.
> > Anyway, if this needs to be there, it should have its own patch
> > 
> ok. I will double check if this is relly needed. If so I will put in
> into a separate patch.

Thanks.

> >> +		}
> >> +
> >> +		if (usage->usage_index == 0x9 && value == 1) {
> >> +			input_event(field->hidinput->input, EV_KEY, KEY_MICMUTE, 1);
> >> +			input_sync(field->hidinput->input);
> >> +			input_event(field->hidinput->input, EV_KEY, KEY_MICMUTE, 0);
> >> +			input_sync(field->hidinput->input);
> >> +			ret = 1;
> > 
> > Aren't you notified when the key is released?
> > 
> Does map_key_clear work if a key can only identified by
> usage->usage_index and not by usage->hid? If yes, I can remove this.

Basically, when you have usage_index > 0, that means that the report
descriptor is declaring more than one field with the same usage in a
row. Given that you should use map_key_clear, you are mapping the HID
usage (the element in the array) to a KEY_* value and you should be
fine.

Consider that each field in the report gets assigned its own usage, and
usage->page, usage->usage and usage->usage_index are just a description
of the usage.

> Otherwise I have to track the previously pressed keys to not issue a key
> release of every special key each time one special key is pressed/released.

Just give a shot at it, but I am pretty confident it will work.

> > If so, you should just drop the change because you used map_key_clear
> > above and hid-input will simply do the right thing for you.
> > 
> > If you are not notified, this is big enough a difference to have its own
> > patch.
> > 
> > Rince wash repeat
> > 
> >> +		}
> >> +
> >> +		if (usage->usage_index == 0xa && value == 1) {
> >> +			input_event(field->hidinput->input, EV_KEY, KEY_CONFIG, 1);
> >> +			input_sync(field->hidinput->input);
> >> +			input_event(field->hidinput->input, EV_KEY, KEY_CONFIG, 0);
> >> +			input_sync(field->hidinput->input);
> >> +
> >> +			ret = 1;
> >> +		}
> >> +
> >> +		if (usage->usage_index == 0xb && value == 1) {
> >> +			input_event(field->hidinput->input, EV_KEY, KEY_SEARCH, 1);
> >> +			input_sync(field->hidinput->input);
> >> +			input_event(field->hidinput->input, EV_KEY, KEY_SEARCH, 0);
> >> +			input_sync(field->hidinput->input);
> >> +
> >> +			ret = 1;
> >> +		}
> >> +
> >> +		if (usage->usage_index == 0xc && value == 1) {
> >> +			input_event(field->hidinput->input, EV_KEY, KEY_SETUP, 1);
> >> +			input_sync(field->hidinput->input);
> >> +			input_event(field->hidinput->input, EV_KEY, KEY_SETUP, 0);
> >> +			input_sync(field->hidinput->input);
> >> +
> >> +			ret = 1;
> >> +		}
> >> +
> >> +		if (usage->usage_index == 0xd && value == 1) {
> >> +			input_event(field->hidinput->input, EV_KEY, KEY_SWITCHVIDEOMODE, 1);
> >> +			input_sync(field->hidinput->input);
> >> +			input_event(field->hidinput->input, EV_KEY, KEY_SWITCHVIDEOMODE, 0);
> >> +			input_sync(field->hidinput->input);
> >> +
> >> +			ret = 1;
> >> +		}
> >> +
> >> +		if (usage->usage_index == 0xe && value == 1) {
> >> +			input_event(field->hidinput->input, EV_KEY, KEY_RFKILL, 1);
> >> +			input_sync(field->hidinput->input);
> >> +			input_event(field->hidinput->input, EV_KEY, KEY_RFKILL, 0);
> >> +			input_sync(field->hidinput->input);
> >> +
> >> +			ret = 1;
> >> +		}
> >> +	}
> >> +
> >> +	return ret;
> >> +}
> >> +
> >>  static int lenovo_event(struct hid_device *hdev, struct hid_field *field,
> >>  		struct hid_usage *usage, __s32 value)
> >>  {
> >> @@ -369,6 +628,8 @@ static int lenovo_event(struct hid_device *hdev, struct hid_field *field,
> >>  	case USB_DEVICE_ID_LENOVO_CUSBKBD:
> >>  	case USB_DEVICE_ID_LENOVO_CBTKBD:
> >>  		return lenovo_event_cptkbd(hdev, field, usage, value);
> >> +	case USB_DEVICE_ID_LENOVO_X1_COVER:
> >> +		return lenovo_event_tpx1cover(hdev, field, usage, value);
> >>  	default:
> >>  		return 0;
> >>  	}
> >> @@ -731,6 +992,251 @@ static int lenovo_probe_tpkbd(struct hid_device *hdev)
> >>  	return ret;
> >>  }
> >>  
> >> +static int lenovo_probe_tpx1cover_configure(struct hid_device *hdev)
> >> +{
> >> +	struct hid_report *report = hdev->report_enum[HID_OUTPUT_REPORT].report_id_hash[9];
> >> +	struct lenovo_drvdata_tpx1cover *drv_data = hid_get_drvdata(hdev);
> >> +
> >> +	if (!drv_data)
> >> +		return -ENODEV;
> > 
> > Can this really happen?
> > 
> I will remove this.

That was really a question, please make sure drv_data can't be null.

> >> +
> >> +	if (!report)
> >> +		return -ENOENT;
> >> +
> >> +	report->field[0]->value[0] = 0x54;
> >> +	report->field[0]->value[1] = 0x20;
> >> +	hid_hw_request(hdev, report, HID_REQ_SET_REPORT);
> >> +	hid_hw_wait(hdev);
> >> +
> >> +	report->field[0]->value[0] = 0x54;
> >> +	report->field[0]->value[1] = 0x08;
> >> +	hid_hw_request(hdev, report, HID_REQ_SET_REPORT);
> >> +	hid_hw_wait(hdev);
> >> +
> >> +	report->field[0]->value[0] = 0xA0;
> >> +	report->field[0]->value[1] = 0x02;
> >> +	hid_hw_request(hdev, report, HID_REQ_SET_REPORT);
> >> +	hid_hw_wait(hdev);
> >> +
> >> +	lenovo_led_brightness_set_tpx1cover(&drv_data->led_mute,
> >> +		hid_lenovo_led_table[HID_LENOVO_LED_MUTE].state ? LED_FULL : LED_OFF);
> >> +	hid_hw_wait(hdev);
> >> +
> >> +	lenovo_led_brightness_set_tpx1cover(&drv_data->led_micmute,
> >> +		hid_lenovo_led_table[HID_LENOVO_LED_MICMUTE].state ? LED_FULL : LED_OFF);
> >> +	hid_hw_wait(hdev);
> >> +
> >> +	lenovo_led_brightness_set_tpx1cover(&drv_data->led_fnlock, LED_FULL);
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static int lenovo_probe_tpx1cover_special_functions(struct hid_device *hdev)
> >> +{
> >> +	struct device *dev = &hdev->dev;
> >> +	struct lenovo_drvdata_tpx1cover *drv_data = NULL;
> >> +
> >> +	size_t name_sz = strlen(dev_name(dev)) + 16;
> >> +	char *name_led = NULL;
> >> +
> >> +	struct hid_report *report;
> >> +	bool report_match = 1;
> >> +
> >> +	int ret = 0;
> >> +
> >> +	report = hid_validate_values(hdev, HID_INPUT_REPORT, 2, 0, 3);
> >> +	report_match &= report ? 1 : 0;
> >> +	report = hid_validate_values(hdev, HID_INPUT_REPORT, 3, 0, 16);
> >> +	report_match &= report ? 1 : 0;
> >> +	report = hid_validate_values(hdev, HID_OUTPUT_REPORT, 9, 0, 2);
> >> +	report_match &= report ? 1 : 0;
> >> +	report = hid_validate_values(hdev, HID_FEATURE_REPORT, 32, 0, 1);
> >> +	report_match &= report ? 1 : 0;
> >> +	report = hid_validate_values(hdev, HID_FEATURE_REPORT, 84, 0, 1);
> >> +	report_match &= report ? 1 : 0;
> >> +	report = hid_validate_values(hdev, HID_FEATURE_REPORT, 100, 0, 1);
> >> +	report_match &= report ? 1 : 0;
> >> +	report = hid_validate_values(hdev, HID_FEATURE_REPORT, 116, 0, 1);
> >> +	report_match &= report ? 1 : 0;
> >> +	report = hid_validate_values(hdev, HID_FEATURE_REPORT, 132, 0, 1);
> >> +	report_match &= report ? 1 : 0;
> >> +	report = hid_validate_values(hdev, HID_FEATURE_REPORT, 144, 0, 1);
> >> +	report_match &= report ? 1 : 0;
> >> +	report = hid_validate_values(hdev, HID_FEATURE_REPORT, 162, 0, 1);
> >> +	report_match &= report ? 1 : 0;
> > 
> > It feels weird to have you check if those reports are actually long
> > enough. I think this is related to checking which interface you have,
> > but you should be able to reduce the list to only those you are actually
> > using (report id "9" seems like a good candidate).
> > 
> Yes, its related to select the USB Interface. I queried all existing
> reports of Interface 1 here. I will reduce this list but report id 9 is
> not sufficient because IF2 has report id 9 too. So I will add one other
> report id which enables a clear decision.
> > And please add a comment why you are checking some specific report IDs.
> > 
> ok.
> >> +
> >> +	if (!report_match) {
> >> +		ret = -ENODEV;
> >> +		goto err;
> > 
> > just return -ENODEV here.
> > 
> ok.
> >> +	}
> >> +
> >> +	drv_data = devm_kzalloc(&hdev->dev,
> >> +			sizeof(struct lenovo_drvdata_tpx1cover),
> >> +			GFP_KERNEL);
> >> +
> >> +	if (!drv_data) {
> >> +		hid_err(hdev,
> >> +			"Could not allocate memory for tpx1cover driver data\n");
> >> +		ret = -ENOMEM;
> >> +		goto err;
> > 
> > Just return -ENODEV too, the devres manager will kfree drv_data for you.
> > 
> ok.
> >> +	}
> >> +
> >> +	name_led = devm_kzalloc(&hdev->dev, name_sz, GFP_KERNEL);
> >> +	if (!name_led) {
> >> +		hid_err(hdev, "Could not allocate memory for mute led data\n");
> >> +		ret = -ENOMEM;
> >> +		goto err_cleanup;
> > 
> > likewise
> > 
> ok.
> >> +	}
> >> +	snprintf(name_led, name_sz, "%s:amber:mute", dev_name(dev));
> >> +
> >> +	drv_data->led_mute.name = name_led;
> >> +	drv_data->led_mute.brightness_get = lenovo_led_brightness_get_tpx1cover;
> >> +	drv_data->led_mute.brightness_set = lenovo_led_brightness_set_tpx1cover;
> >> +	drv_data->led_mute.dev = dev;
> >> +	hid_lenovo_led_table[HID_LENOVO_LED_MUTE].dev = &drv_data->led_mute;
> >> +	led_classdev_register(dev, &drv_data->led_mute);
> > 
> > Isn't devm_led_class_register working?
> > 
> > That would be nice because you could drop all of your cleanup paths
> > 
> Ah, ok, didn't know that function until yet.

I *think* it should be fine to use it with hid->dev. If you use it with
input->dev, that won't do and you'll need to do some weird things to
remove the LED device (sadly that's what I had to do in the wacom
driver).

Just make sure /sys/class/leds/ doesn't contain a dangling LED device
when you remove the cover and you should know if this worked or not.

Cheers,
Benjamin

> >> +
> >> +
> >> +	name_led = devm_kzalloc(&hdev->dev, name_sz, GFP_KERNEL);
> >> +	if (!name_led) {
> >> +		hid_err(hdev,
> >> +			"Could not allocate memory for mic mute led data\n");
> >> +		ret = -ENOMEM;
> >> +		goto err_cleanup;
> > 
> > ditto
> > 
> ok.
> >> +	}
> >> +	snprintf(name_led, name_sz, "%s:amber:micmute", dev_name(dev));
> >> +
> >> +	drv_data->led_micmute.name = name_led;
> >> +	drv_data->led_micmute.brightness_get = lenovo_led_brightness_get_tpx1cover;
> >> +	drv_data->led_micmute.brightness_set = lenovo_led_brightness_set_tpx1cover;
> >> +	drv_data->led_micmute.dev = dev;
> >> +	hid_lenovo_led_table[HID_LENOVO_LED_MICMUTE].dev = &drv_data->led_micmute;
> >> +	led_classdev_register(dev, &drv_data->led_micmute);
> >> +
> >> +
> >> +	name_led = devm_kzalloc(&hdev->dev, name_sz, GFP_KERNEL);
> >> +	if (!name_led) {
> >> +		hid_err(hdev,
> >> +			"Could not allocate memory for FN lock led data\n");
> >> +		ret = -ENOMEM;
> >> +		goto err_cleanup;
> > 
> > ditto
> > 
> ok.
> >> +	}
> >> +
> >> +	snprintf(name_led, name_sz, "%s:amber:fnlock", dev_name(dev));
> >> +
> >> +	drv_data->led_fnlock.name = name_led;
> >> +	drv_data->led_fnlock.brightness_get = lenovo_led_brightness_get_tpx1cover;
> >> +	drv_data->led_fnlock.brightness_set = lenovo_led_brightness_set_tpx1cover;
> >> +	drv_data->led_fnlock.dev = dev;
> >> +	hid_lenovo_led_table[HID_LENOVO_LED_FNLOCK].dev = &drv_data->led_fnlock;
> >> +	led_classdev_register(dev, &drv_data->led_fnlock);
> > 
> > ditto
> > 
> ok.
> >> +
> >> +	drv_data->led_state = 0;
> >> +	drv_data->fnlock_state = 1;
> >> +	drv_data->led_present = 1;
> >> +
> >> +	hid_set_drvdata(hdev, drv_data);
> >> +
> >> +	return lenovo_probe_tpx1cover_configure(hdev);
> >> +
> >> +err_cleanup:
> > 
> > Shouldn't be required if you use devm_led_classdev_register().
> > 
> ok.
> >> +	if (drv_data->led_fnlock.name) {
> >> +		led_classdev_unregister(&drv_data->led_fnlock);
> >> +		devm_kfree(&hdev->dev, (void *) drv_data->led_fnlock.name);
> >> +	}
> >> +
> >> +	if (drv_data->led_micmute.name) {
> >> +		led_classdev_unregister(&drv_data->led_micmute);
> >> +		devm_kfree(&hdev->dev, (void *) drv_data->led_micmute.name);
> >> +	}
> >> +
> >> +	if (drv_data->led_mute.name) {
> >> +		led_classdev_unregister(&drv_data->led_mute);
> >> +		devm_kfree(&hdev->dev, (void *) drv_data->led_mute.name);
> >> +	}
> >> +
> >> +	if (drv_data)
> >> +		kfree(drv_data);
> >> +
> >> +err:
> >> +	return ret;
> >> +}
> >> +
> >> +static int lenovo_probe_tpx1cover_touch(struct hid_device *hdev)
> >> +{
> >> +	struct hid_report *report;
> >> +	bool report_match = 1;
> >> +	int ret = 0;
> >> +
> >> +	report = hid_validate_values(hdev, HID_INPUT_REPORT, 2, 0, 2);
> >> +	report_match &= report ? 1 : 0;
> >> +	report = hid_validate_values(hdev, HID_INPUT_REPORT, 2, 1, 2);
> >> +	report_match &= report ? 1 : 0;
> >> +	report = hid_validate_values(hdev, HID_INPUT_REPORT, 11, 0, 61);
> >> +	report_match &= report ? 1 : 0;
> >> +	report = hid_validate_values(hdev, HID_INPUT_REPORT, 12, 0, 61);
> >> +	report_match &= report ? 1 : 0;
> >> +	report = hid_validate_values(hdev, HID_INPUT_REPORT, 16, 0, 3);
> >> +	report_match &= report ? 1 : 0;
> >> +	report = hid_validate_values(hdev, HID_INPUT_REPORT, 16, 1, 2);
> >> +	report_match &= report ? 1 : 0;
> >> +	report = hid_validate_values(hdev, HID_OUTPUT_REPORT, 9, 0, 20);
> >> +	report_match &= report ? 1 : 0;
> >> +	report = hid_validate_values(hdev, HID_OUTPUT_REPORT, 10, 0, 20);
> >> +	report_match &= report ? 1 : 0;
> >> +	report = hid_validate_values(hdev, HID_FEATURE_REPORT, 14, 0, 1);
> >> +	report_match &= report ? 1 : 0;
> >> +	report = hid_validate_values(hdev, HID_FEATURE_REPORT, 15, 0, 3);
> >> +	report_match &= report ? 1 : 0;
> > 
> > Feels weird too (especially if there is no comment explaining why you
> > are doing those checks).
> > 
> ok.
> >> +
> >> +	if (!report_match)
> >> +		ret = -ENODEV;
> >> +
> >> +	return ret;
> >> +}
> >> +
> >> +static int lenovo_probe_tpx1cover(struct hid_device *hdev)
> >> +{
> >> +	int ret = 0;
> >> +
> >> +	/*
> >> +	 * Probing for special function keys and LED control -> usb intf 1
> >> +	 * Probing for touch input -> usb intf 2 (handled by rmi4 driver)
> >> +	 * Other (keyboard) -> usb intf 0
> >> +	 */
> >> +	if (!lenovo_probe_tpx1cover_special_functions(hdev)) {
> >> +		// special function keys and LED control
> > 
> > No C++ style comments please.
> > 
> Oops
> >> +		ret = 0;
> >> +	} else if (!lenovo_probe_tpx1cover_touch(hdev)) {
> >> +		// handled by rmi
> >> +		ret = -ENODEV;
> > 
> > I don't quite get how the touch can be handled if you just return
> > -ENODEV here. Given that the device has been added to
> > hid_have_special_driver, if you don't claim the device, no one else will
> > unless you add the ID in the other HID driver.
> > 
> Yes, ok. This is because I have some additional RMI4 code which handles
> the touch. At the current upstream situation I should return 0 here such
> that this touch works basically.
> >> +	} else {
> >> +		// keyboard
> > 
> > Why is that keyboard chunk not given it's own probe function?
> > 
> Because it is just a few lines:) I will add a probe function.
> >> +		struct lenovo_drvdata_tpx1cover *drv_data;
> >> +
> >> +		drv_data = devm_kzalloc(&hdev->dev,
> >> +					sizeof(struct lenovo_drvdata_tpx1cover),
> >> +					GFP_KERNEL);
> >> +
> >> +		if (!drv_data) {
> >> +			hid_err(hdev,
> >> +				"Could not allocate memory for tpx1cover driver data\n");
> >> +			ret = -ENOMEM;
> >> +			goto out;
> > 
> > no need for a goto here. Just a plain return -ENOMEM should be good
> > enough.
> > 
> ok
> >> +		}
> >> +
> >> +		drv_data->led_state = 0;
> >> +		drv_data->led_present = 0;
> >> +		drv_data->fnlock_state = 0;
> >> +		hid_set_drvdata(hdev, drv_data);
> >> +
> >> +		ret = 0;
> >> +	}
> >> +
> >> +out:
> >> +	return ret;
> >> +}
> >> +
> >>  static int lenovo_probe_cptkbd(struct hid_device *hdev)
> >>  {
> >>  	int ret;
> >> @@ -803,6 +1309,9 @@ static int lenovo_probe(struct hid_device *hdev,
> >>  	case USB_DEVICE_ID_LENOVO_CBTKBD:
> >>  		ret = lenovo_probe_cptkbd(hdev);
> >>  		break;
> >> +	case USB_DEVICE_ID_LENOVO_X1_COVER:
> >> +		ret = lenovo_probe_tpx1cover(hdev);
> >> +		break;
> >>  	default:
> >>  		ret = 0;
> >>  		break;
> >> @@ -843,6 +1352,42 @@ static void lenovo_remove_cptkbd(struct hid_device *hdev)
> >>  			&lenovo_attr_group_cptkbd);
> >>  }
> >>  
> >> +static void lenovo_remove_tpx1cover(struct hid_device *hdev)
> >> +{
> >> +	struct lenovo_drvdata_tpx1cover *drv_data = hid_get_drvdata(hdev);
> >> +
> >> +	if (!drv_data)
> >> +		return;
> >> +
> >> +	if (drv_data->led_present) {
> >> +		if (drv_data->led_fnlock.name) {
> >> +			hid_lenovo_led_table[HID_LENOVO_LED_FNLOCK].dev = NULL;
> >> +
> >> +			led_classdev_unregister(&drv_data->led_fnlock);
> >> +			devm_kfree(&hdev->dev, (void *) drv_data->led_fnlock.name);
> > 
> > Calling yourself devm_kfree usually means there is something wrong.
> > Here, if you used devm_led_*, you could just drop the entire remove
> > function.
> > 
> ok.
> >> +		}
> >> +
> >> +		if (drv_data->led_micmute.name) {
> >> +			hid_lenovo_led_table[HID_LENOVO_LED_MICMUTE].dev = NULL;
> >> +
> >> +			led_classdev_unregister(&drv_data->led_micmute);
> >> +			devm_kfree(&hdev->dev, (void *) drv_data->led_micmute.name);
> >> +		}
> >> +
> >> +		if (drv_data->led_mute.name) {
> >> +			hid_lenovo_led_table[HID_LENOVO_LED_MUTE].dev = NULL;
> >> +
> >> +			led_classdev_unregister(&drv_data->led_mute);
> >> +			devm_kfree(&hdev->dev, (void *) drv_data->led_mute.name);
> >> +		}
> >> +	}
> >> +
> >> +	if (drv_data)
> >> +		devm_kfree(&hdev->dev, drv_data);
> >> +
> >> +	hid_set_drvdata(hdev, NULL);
> >> +}
> >> +
> >>  static void lenovo_remove(struct hid_device *hdev)
> >>  {
> >>  	switch (hdev->product) {
> >> @@ -853,6 +1398,9 @@ static void lenovo_remove(struct hid_device *hdev)
> >>  	case USB_DEVICE_ID_LENOVO_CBTKBD:
> >>  		lenovo_remove_cptkbd(hdev);
> >>  		break;
> >> +	case USB_DEVICE_ID_LENOVO_X1_COVER:
> >> +		lenovo_remove_tpx1cover(hdev);
> >> +		break;
> >>  	}
> >>  
> >>  	hid_hw_stop(hdev);
> >> @@ -883,6 +1431,7 @@ static int lenovo_input_configured(struct hid_device *hdev,
> >>  	{ HID_USB_DEVICE(USB_VENDOR_ID_LENOVO, USB_DEVICE_ID_LENOVO_CUSBKBD) },
> >>  	{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LENOVO, USB_DEVICE_ID_LENOVO_CBTKBD) },
> >>  	{ HID_USB_DEVICE(USB_VENDOR_ID_LENOVO, USB_DEVICE_ID_LENOVO_TPPRODOCK) },
> >> +	{ HID_USB_DEVICE(USB_VENDOR_ID_LENOVO, USB_DEVICE_ID_LENOVO_X1_COVER) },
> >>  	{ }
> >>  };
> >>  
> >> diff --git a/include/linux/hid-lenovo.h b/include/linux/hid-lenovo.h
> >> new file mode 100644
> >> index 0000000..d0b0145
> >> --- /dev/null
> >> +++ b/include/linux/hid-lenovo.h
> >> @@ -0,0 +1,15 @@
> >> +
> >> +#ifndef __HID_LENOVO_H__
> >> +#define __HID_LENOVO_H__
> >> +
> >> +
> >> +enum {
> >> +	HID_LENOVO_LED_MUTE,
> > 
> > I'd rather have a name for the enum (so you can reuse it), and also have
> > each enum given its numerical value (or at least the first and last.
> > 
> ok.
> >> +	HID_LENOVO_LED_MICMUTE,
> >> +	HID_LENOVO_LED_FNLOCK,
> >> +	HID_LENOVO_LED_MAX,
> >> +};
> >> +
> >> +int hid_lenovo_led_set(int led_num, bool on);
> >> +
> >> +#endif /* __HID_LENOVO_H_ */
> >> -- 
> >> 1.i9.1
> > 
> > 
> > Cheers,
> > Benjamin
> > 
--
To unsubscribe from this list: send the line "unsubscribe linux-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