Date:	Thu, 12 Jun 2014 09:56:41 +0100 (BST)
From:	Jamie Lentin <jm@...tin.co.uk>
To:	Antonio Ospite <ao2@....it>
cc:	Jiri Kosina <jkosina@...e.cz>, linux-input@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/2] Add support for Compact (Bluetooth|USB) keyboard
 with Trackpoint

On Wed, 11 Jun 2014, Antonio Ospite wrote:

> On Tue, 10 Jun 2014 23:24:54 +0100
> Jamie Lentin <jm@...tin.co.uk> wrote:
>
>> Signed-off-by: Jamie Lentin <jm@...tin.co.uk>
>
> Some minor comments here too.

Thankyou for taking the time over both sets! All comments make sense.

>> ---
>>  drivers/hid/hid-core.c         |   2 +
>>  drivers/hid/hid-ids.h          |   2 +
>>  drivers/hid/hid-lenovo-tpkbd.c | 203 +++++++++++++++++++++++++++++++++++++++++
>>  include/linux/hid.h            |   1 +
>>  4 files changed, 208 insertions(+)
>>
>> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
>> index 8a5384c..6591f4e 100644
>> --- a/drivers/hid/hid-core.c
>> +++ b/drivers/hid/hid-core.c
>> @@ -1738,6 +1738,8 @@ static const struct hid_device_id hid_have_special_driver[] = {
>>  	{ HID_USB_DEVICE(USB_VENDOR_ID_LCPOWER, USB_DEVICE_ID_LCPOWER_LC1000 ) },
>>  #if IS_ENABLED(CONFIG_HID_LENOVO_TPKBD)
>>  	{ HID_USB_DEVICE(USB_VENDOR_ID_LENOVO, USB_DEVICE_ID_LENOVO_TPKBD) },
>> +	{ HID_USB_DEVICE(USB_VENDOR_ID_LENOVO, USB_DEVICE_ID_LENOVO_CUSBKBD) },
>> +	{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LENOVO, USB_DEVICE_ID_LENOVO_CBTKBD) },
>>  #endif
>>  	{ HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_MX3000_RECEIVER) },
>>  	{ HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_S510_RECEIVER) },
>> diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
>> index 6e12cd0..1763a07 100644
>> --- a/drivers/hid/hid-ids.h
>> +++ b/drivers/hid/hid-ids.h
>> @@ -551,6 +551,8 @@
>>
>>  #define USB_VENDOR_ID_LENOVO		0x17ef
>>  #define USB_DEVICE_ID_LENOVO_TPKBD	0x6009
>> +#define USB_DEVICE_ID_LENOVO_CUSBKBD	0x6047
>> +#define USB_DEVICE_ID_LENOVO_CBTKBD		0x6048
>>
>>  #define USB_VENDOR_ID_LG		0x1fd2
>>  #define USB_DEVICE_ID_LG_MULTITOUCH	0x0064
>> diff --git a/drivers/hid/hid-lenovo-tpkbd.c b/drivers/hid/hid-lenovo-tpkbd.c
>> index 3bec9f5..1671e7a 100644
>> --- a/drivers/hid/hid-lenovo-tpkbd.c
>> +++ b/drivers/hid/hid-lenovo-tpkbd.c
>> @@ -1,8 +1,11 @@
>>  /*
>>   *  HID driver for Lenovo:-
>>   *  * ThinkPad USB Keyboard with TrackPoint
>> + *  * ThinkPad Compact Bluetooth Keyboard with TrackPoint
>> + *  * ThinkPad Compact USB Keyboard with TrackPoint
>
> Use - as the bullet.
>
>>   *
>>   *  Copyright (c) 2012 Bernhard Seibold
>> + *  Copyright (c) 2014 Jamie Lentin <jm@...tin.co.uk>
>>   */
>>
>>  /*
>> @@ -34,6 +37,10 @@ struct tpkbd_data_pointer {
>>  	int press_speed;
>>  };
>>
>> +struct tpcompactkbd_sc {
>> +	unsigned int fn_lock;
>> +};
>> +
>>  #define map_key_clear(c) hid_map_usage_clear(hi, usage, bit, max, EV_KEY, (c))
>>
>>  static int tpkbd_input_mapping_tp(struct hid_device *hdev,
>> @@ -49,17 +56,154 @@ static int tpkbd_input_mapping_tp(struct hid_device *hdev,
>>  	return 0;
>>  }
>>
>> +static int tpcompactkbd_input_mapping(struct hid_device *hdev,
>
> Maybe name these functions like tpkbd_input_mapping_compact()?
>
> This way the namespace is more consistent, and follows the logic of
> patch 1/2 more closely.
>
> Use this scheme at least for functions which have a _tp() counterpart.

Previously the tpkbd driver had various functions marked "_tp" to indicate 
that it's for the "mouse" half of the keyboard as the kernel sees it, 
however it does nothing special with the keyboard half. I was intending 
(somewhat sloppily) to repurpose this into having versions of each 
function for each keyboard, and a common function to switch between them. 
Should make it fairly easy to add extra keyboards in the future.

The problem, as ever, is choosing decent names for them. It should 
probably be either:-

* tpkbd_input_mapping_usbkbd
* tpkbd_input_mapping_compactkbd
...and tpkbd_input_mapping switches between them

or rename the driver to hid-lenovo and do:-

* lenovo_input_mapping_tpkbd
* lenovo_input_mapping_compacttp
...and lenovo_input_mapping switches between them

The latter seems a bit too invasive, but I'm not sure how obvious with the 
former that it'd be that the "Compact USB keyboard" is in-fact a 
compactkbd not a usbkbd. The former is probably what I'll go for unless 
you have any thoughts.

>> +		struct hid_input *hi, struct hid_field *field,
>> +		struct hid_usage *usage, unsigned long **bit, int *max)
>> +{
>> +	/* HID_UP_LNVENDOR = USB, HID_UP_MSVENDOR = BT */
>> +	if ((usage->hid & HID_USAGE_PAGE) == HID_UP_MSVENDOR ||
>> +	    (usage->hid & HID_USAGE_PAGE) == HID_UP_LNVENDOR) {
>> +		set_bit(EV_REP, hi->input->evbit);
>> +		switch (usage->hid & HID_USAGE) {
>> +		case 0x00f1: /* Fn-F4: Mic mute */
>> +			map_key_clear(KEY_MICMUTE);
>> +			return 1;
>> +		case 0x00f2: /* Fn-F5: Brightness down */
>> +			map_key_clear(KEY_BRIGHTNESSDOWN);
>> +			return 1;
>> +		case 0x00f3: /* Fn-F6: Brightness up */
>> +			map_key_clear(KEY_BRIGHTNESSUP);
>> +			return 1;
>> +		case 0x00f4: /* Fn-F7: External display (projector) */
>> +			map_key_clear(KEY_SWITCHVIDEOMODE);
>> +			return 1;
>> +		case 0x00f5: /* Fn-F8: Wireless */
>> +			map_key_clear(KEY_WLAN);
>> +			return 1;
>> +		case 0x00f6: /* Fn-F9: Control panel */
>> +			map_key_clear(KEY_CONFIG);
>> +			return 1;
>> +		case 0x00f8: /* Fn-F11: View open applications (3 boxes) */
>> +			map_key_clear(KEY_FN_F11);
>> +			return 1;
>> +		case 0x00fa: /* Fn-Esc: Fn-lock toggle */
>> +			map_key_clear(KEY_FN_ESC);
>> +			return 1;
>> +		case 0x00fb: /* Fn-F12: Open My computer (6 boxes) USB-only */
>> +			/* NB: This mapping is invented in raw_event below */
>> +			map_key_clear(KEY_FILE);
>> +			return 1;
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>  static int tpkbd_input_mapping(struct hid_device *hdev,
>>  		struct hid_input *hi, struct hid_field *field,
>>  		struct hid_usage *usage, unsigned long **bit, int *max)
>>  {
>>  	if (hdev->product == USB_DEVICE_ID_LENOVO_TPKBD)
>>  		return tpkbd_input_mapping_tp(hdev, hi, field, usage, bit, max);
>> +	if (hdev->product == USB_DEVICE_ID_LENOVO_CUSBKBD)
>> +		return tpcompactkbd_input_mapping(hdev, hi, field, usage, bit, max);
>> +	if (hdev->product == USB_DEVICE_ID_LENOVO_CBTKBD)
>
> What about combining the checks?
>
> 	if (hdev->product == USB_DEVICE_ID_LENOVO_CUSBKBD ||
> 	    hdev->product == USB_DEVICE_ID_LENOVO_CBTKBD)

A switch statement might be neater, but either way yes could combine them.

>> +		return tpcompactkbd_input_mapping(hdev, hi, field, usage, bit, max);
>>  	return 0;
>>  }
>>
>>  #undef map_key_clear
>>
>> +/* Send a config command to the keyboard */
>> +static int tpcompactkbd_send_cmd(struct hid_device *hdev,
>> +			unsigned char byte2, unsigned char byte3)
>> +{
>> +	int ret;
>> +	unsigned char buf[] = {0x18, byte2, byte3};
>> +	unsigned char report_type = HID_OUTPUT_REPORT;
>> +
>> +	/* The USB keyboard accepts commands via SET_FEATURE */
>> +	if (hdev->product == USB_DEVICE_ID_LENOVO_CUSBKBD) {
>> +		buf[0] = 0x13;
>> +		report_type = HID_FEATURE_REPORT;
>> +	}
>> +
>> +	ret = hdev->hid_output_raw_report(hdev, buf, sizeof(buf), report_type);
>> +	return ret < 0 ? ret : 0; /* BT returns 0, USB returns sizeof(buf) */
>> +}
>> +
>> +static void tpcompactkbd_features_set(struct hid_device *hdev)
>> +{
>> +	struct tpcompactkbd_sc *tpcsc = hid_get_drvdata(hdev);
>> +
>> +	if (tpcompactkbd_send_cmd(hdev, 0x05, tpcsc->fn_lock ? 0x01 : 0x00))
>> +		hid_err(hdev, "Fn-lock setting failed\n");
>> +}
>> +
>> +static ssize_t tpcompactkbd_fn_lock_show(struct device *dev,
>> +		struct device_attribute *attr,
>> +		char *buf)
>> +{
>> +	struct hid_device *hdev = container_of(dev, struct hid_device, dev);
>> +	struct tpcompactkbd_sc *tpcsc = hid_get_drvdata(hdev);
>> +
>> +	return snprintf(buf, PAGE_SIZE, "%u\n", tpcsc->fn_lock);
>> +}
>> +
>> +static ssize_t tpcompactkbd_fn_lock_store(struct device *dev,
>> +		struct device_attribute *attr,
>> +		const char *buf,
>> +		size_t count)
>> +{
>> +	struct hid_device *hdev = container_of(dev, struct hid_device, dev);
>> +	struct tpcompactkbd_sc *tpcsc = hid_get_drvdata(hdev);
>> +	int value;
>> +
>> +	if (kstrtoint(buf, 10, &value))
>> +		return -EINVAL;
>> +	if (value < 0 || value > 1)
>> +		return -EINVAL;
>> +
>> +	tpcsc->fn_lock = value;
>> +	tpcompactkbd_features_set(hdev);
>> +
>> +	return count;
>> +}
>> +
>> +static struct device_attribute dev_attr_pointer_fn_lock =
>> +	__ATTR(fn_lock, S_IWUSR | S_IRUGO,
>> +			tpcompactkbd_fn_lock_show,
>> +			tpcompactkbd_fn_lock_store);
>> +
>> +static struct attribute *tpcompactkbd_attributes[] = {
>> +	&dev_attr_pointer_fn_lock.attr,
>> +	NULL
>> +};
>> +
>> +static const struct attribute_group tpcompactkbd_attr_group = {
>> +	.attrs = tpcompactkbd_attributes,
>> +};
>> +
>> +static int tpkbd_raw_event(struct hid_device *hdev,
>> +			struct hid_report *report, u8 *data, int size)
>> +{
>> +	/*
>> +	 * USB keyboard's Fn-F12 report holds down many other keys, and it's
>> +	 * own key is outside the usage page range. Remove extra keypresses and
>> +	 * remap to inside usage page.
>> +	 */
>> +	if (unlikely(hdev->product == USB_DEVICE_ID_LENOVO_CUSBKBD
>> +			&& size == 3
>> +			&& data[0] == 0x15
>> +			&& data[1] == 0x94
>> +			&& data[2] == 0x01)) {
>> +		data[1] = 0x0;
>> +		data[2] = 0x4;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>  static int tpkbd_features_set(struct hid_device *hdev)
>>  {
>>  	struct hid_report *report;
>> @@ -406,6 +550,46 @@ static int tpkbd_probe_tp(struct hid_device *hdev)
>>  	return 0;
>>  }
>>
>> +static int tpcompactkbd_probe(struct hid_device *hdev,
>> +			const struct hid_device_id *id)
>> +{
>> +	int ret;
>> +	struct tpcompactkbd_sc *tpcsc;
>> +
>> +	tpcsc = devm_kzalloc(&hdev->dev, sizeof(*tpcsc), GFP_KERNEL);
>> +	if (tpcsc == NULL) {
>> +		hid_err(hdev, "can't alloc keyboard descriptor\n");
>> +		return -ENOMEM;
>> +	}
>> +	hid_set_drvdata(hdev, tpcsc);
>> +
>> +	/* All the custom action happens on the mouse device for USB */
>> +	if (hdev->product == USB_DEVICE_ID_LENOVO_CUSBKBD
>> +			&& hdev->type != HID_TYPE_USBMOUSE) {
>> +		pr_debug("Ignoring keyboard half of device\n");
>> +		return 0;
>> +	}
>> +
>> +	/*
>> +	 * Tell the keyboard a driver understands it, and turn F7, F9, F11 into
>> +	 * regular keys
>> +	 */
>> +	ret = tpcompactkbd_send_cmd(hdev, 0x01, 0x03);
>> +	if (ret)
>> +		hid_warn(hdev, "Failed to switch F7/9/11 into regular keys\n");
>> +
>> +	/* Turn Fn-Lock on by default */
>> +	tpcsc->fn_lock = 1;
>> +	tpcompactkbd_features_set(hdev);
>> +
>> +	if (sysfs_create_group(&hdev->dev.kobj,
>> +				&tpcompactkbd_attr_group)) {
>
> Use:
> 	ret = sysfs_create_group()
> 	if (ret) {
>
>> +		hid_warn(hdev, "Could not create sysfs group\n");
>> +	}
>
> with only one statement in the if body curly braces are not needed.
>
>> +
>> +	return 0;
>> +}
>> +
>>  static int tpkbd_probe(struct hid_device *hdev,
>>  		const struct hid_device_id *id)
>>  {
>> @@ -426,6 +610,12 @@ static int tpkbd_probe(struct hid_device *hdev,
>>  	if (hdev->product == USB_DEVICE_ID_LENOVO_TPKBD
>>  			&& tpkbd_probe_tp(hdev))
>>  		goto err_hid;
>> +	if (hdev->product == USB_DEVICE_ID_LENOVO_CUSBKBD
>> +			&& tpcompactkbd_probe(hdev, id))
>> +		goto err_hid;
>> +	if (hdev->product == USB_DEVICE_ID_LENOVO_CBTKBD
>> +			&& tpcompactkbd_probe(hdev, id))
>
> Again, what about combining the checks?
> Calling the function in the if body (and checking the return value of
> course :P).
>
>> +		goto err_hid;
>>
>>  	return 0;
>>  err_hid:
>> @@ -447,6 +637,12 @@ static void tpkbd_remove_tp(struct hid_device *hdev)
>>  	hid_set_drvdata(hdev, NULL);
>>  }
>>
>> +static void tpcompactkbd_remove(struct hid_device *hdev)
>> +{
>> +	sysfs_remove_group(&hdev->dev.kobj,
>> +			&tpcompactkbd_attr_group);
>> +}
>> +
>>  static void tpkbd_remove(struct hid_device *hdev)
>>  {
>>  	if (!hid_get_drvdata(hdev))
>> @@ -454,12 +650,18 @@ static void tpkbd_remove(struct hid_device *hdev)
>>
>>  	if (hdev->product == USB_DEVICE_ID_LENOVO_TPKBD)
>>  		tpkbd_remove_tp(hdev);
>> +	if (hdev->product == USB_DEVICE_ID_LENOVO_CUSBKBD)
>> +		tpcompactkbd_remove(hdev);
>> +	if (hdev->product == USB_DEVICE_ID_LENOVO_CBTKBD)
>
> Here too.
>
>> +		tpcompactkbd_remove(hdev);
>>
>>  	hid_hw_stop(hdev);
>>  }
>>
>>  static const struct hid_device_id tpkbd_devices[] = {
>>  	{ HID_USB_DEVICE(USB_VENDOR_ID_LENOVO, USB_DEVICE_ID_LENOVO_TPKBD) },
>> +	{ HID_USB_DEVICE(USB_VENDOR_ID_LENOVO, USB_DEVICE_ID_LENOVO_CUSBKBD) },
>> +	{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LENOVO, USB_DEVICE_ID_LENOVO_CBTKBD) },
>>  	{ }
>>  };
>>
>> @@ -471,6 +673,7 @@ static struct hid_driver tpkbd_driver = {
>>  	.input_mapping = tpkbd_input_mapping,
>>  	.probe = tpkbd_probe,
>>  	.remove = tpkbd_remove,
>> +	.raw_event = tpkbd_raw_event,
>>  };
>>  module_hid_driver(tpkbd_driver);
>>
>> diff --git a/include/linux/hid.h b/include/linux/hid.h
>> index 31b9d29..ed23d6a 100644
>> --- a/include/linux/hid.h
>> +++ b/include/linux/hid.h
>> @@ -167,6 +167,7 @@ struct hid_item {
>>  #define HID_UP_MSVENDOR		0xff000000
>>  #define HID_UP_CUSTOM		0x00ff0000
>>  #define HID_UP_LOGIVENDOR	0xffbc0000
>> +#define HID_UP_LNVENDOR		0xffa00000
>>  #define HID_UP_SENSOR		0x00200000
>>
>>  #define HID_USAGE		0x0000ffff
>> --
>> 2.0.0.rc2
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-input" in
>> the body of a message to majordomo@...r.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>
> Ciao ciao,
>   Antonio
>
>

-- 
Jamie Lentin
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists