[PATCH v2 0/2] Add support for Lenovo Compact Keyboard

[PATCH v2 0/2] Add support for Lenovo Compact Keyboard

[Jamie Lentin]

This patchset follows on from my previous attempt[1] to add support for
these keyboards from Lenovo.

Changes since v1:
* Merge driver into hid-lenovo-tpkbd.c instead of creating our own
  driver for the hardware [Jiri Kosina]
* Remove key mappings which are now supported by standard
* Use KEY_FILE for Fn-F12 (opens My Computer on Windows)
* Support the USB variant as well as Bluetooth
* Expose the Fn Lock setting as a sysfs attribute instead of trying to
  build a mechanism to toggle into the kernel

I got rid of the Fn-lock toggle code since it didn't play nicely with
the USB keyboard; from what I could ascertain, hid_output_raw_report
wants to sleep when sending a report to a USB keyboard, which isn't
allowed. The same was true when using an input handler.

A sysfs attribute is more flexible anyway, and one could trivially
script a user-space toggling mechanism around it. I suspect in
reality so long as Fn-Lock is left on nobody will really care what the
Fn-Lock key does :)

Applies and tested against 3.14.5.

Cheers,

[1] https://lkml.org/lkml/2014/3/25/535

Jamie Lentin (2):
  Loosen seams to allow support of other keyboards
  Add support for Compact (Bluetooth|USB) keyboard with Trackpoint

 drivers/hid/hid-core.c         |   2 +
 drivers/hid/hid-ids.h          |   2 +
 drivers/hid/hid-lenovo-tpkbd.c | 236 +++++++++++++++++++++++++++++++++++++++--
 include/linux/hid.h            |   1 +
 4 files changed, 232 insertions(+), 9 deletions(-)

-- 
2.0.0.rc2

[PATCH v2 1/2] Loosen seams to allow support of other keyboards

[Jamie Lentin]

Signed-off-by: Jamie Lentin <jm@lentin.co.uk>
---
 drivers/hid/hid-lenovo-tpkbd.c | 33 ++++++++++++++++++++++++---------
 1 file changed, 24 insertions(+), 9 deletions(-)

diff --git a/drivers/hid/hid-lenovo-tpkbd.c b/drivers/hid/hid-lenovo-tpkbd.c
index 2d25b6c..3bec9f5 100644
--- a/drivers/hid/hid-lenovo-tpkbd.c
+++ b/drivers/hid/hid-lenovo-tpkbd.c
@@ -1,5 +1,6 @@
 /*
- *  HID driver for Lenovo ThinkPad USB Keyboard with TrackPoint
+ *  HID driver for Lenovo:-
+ *  * ThinkPad USB Keyboard with TrackPoint
  *
  *  Copyright (c) 2012 Bernhard Seibold
  */
@@ -35,7 +36,7 @@ struct tpkbd_data_pointer {
 
 #define map_key_clear(c) hid_map_usage_clear(hi, usage, bit, max, EV_KEY, (c))
 
-static int tpkbd_input_mapping(struct hid_device *hdev,
+static int tpkbd_input_mapping_tp(struct hid_device *hdev,
 		struct hid_input *hi, struct hid_field *field,
 		struct hid_usage *usage, unsigned long **bit, int *max)
 {
@@ -48,6 +49,15 @@ static int tpkbd_input_mapping(struct hid_device *hdev,
 	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);
+	return 0;
+}
+
 #undef map_key_clear
 
 static int tpkbd_features_set(struct hid_device *hdev)
@@ -338,6 +348,11 @@ static int tpkbd_probe_tp(struct hid_device *hdev)
 	char *name_mute, *name_micmute;
 	int i;
 
+	/* Ignore unless tpkbd_input_mapping_tp marked it as a pointer */
+	if (!hid_get_drvdata(hdev))
+		return 0;
+	hid_set_drvdata(hdev, NULL);
+
 	/* Validate required reports. */
 	for (i = 0; i < 4; i++) {
 		if (!hid_validate_values(hdev, HID_FEATURE_REPORT, 4, i, 1))
@@ -408,12 +423,9 @@ static int tpkbd_probe(struct hid_device *hdev,
 		goto err;
 	}
 
-	if (hid_get_drvdata(hdev)) {
-		hid_set_drvdata(hdev, NULL);
-		ret = tpkbd_probe_tp(hdev);
-		if (ret)
-			goto err_hid;
-	}
+	if (hdev->product == USB_DEVICE_ID_LENOVO_TPKBD
+			&& tpkbd_probe_tp(hdev))
+		goto err_hid;
 
 	return 0;
 err_hid:
@@ -437,7 +449,10 @@ static void tpkbd_remove_tp(struct hid_device *hdev)
 
 static void tpkbd_remove(struct hid_device *hdev)
 {
-	if (hid_get_drvdata(hdev))
+	if (!hid_get_drvdata(hdev))
+		return;
+
+	if (hdev->product == USB_DEVICE_ID_LENOVO_TPKBD)
 		tpkbd_remove_tp(hdev);
 
 	hid_hw_stop(hdev);
-- 
2.0.0.rc2

Re: [PATCH v2 1/2] Loosen seams to allow support of other keyboards

[Antonio Ospite]

Hi Jamie,

some few ideas in case there will be a v3, most of them are really just
nitpicks.

On Tue, 10 Jun 2014 23:24:53 +0100
Jamie Lentin <jm@lentin.co.uk> wrote:

> Signed-off-by: Jamie Lentin <jm@lentin.co.uk>
> ---
>  drivers/hid/hid-lenovo-tpkbd.c | 33 ++++++++++++++++++++++++---------
>  1 file changed, 24 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/hid/hid-lenovo-tpkbd.c b/drivers/hid/hid-lenovo-tpkbd.c
> index 2d25b6c..3bec9f5 100644
> --- a/drivers/hid/hid-lenovo-tpkbd.c
> +++ b/drivers/hid/hid-lenovo-tpkbd.c
> @@ -1,5 +1,6 @@
>  /*
> - *  HID driver for Lenovo ThinkPad USB Keyboard with TrackPoint
> + *  HID driver for Lenovo:-
> + *  * ThinkPad USB Keyboard with TrackPoint
>   *

Remove the - after the : and use the - as the bullet.
The asterisk is used for the comment already.

>   *  Copyright (c) 2012 Bernhard Seibold
>   */
> @@ -35,7 +36,7 @@ struct tpkbd_data_pointer {
>  
>  #define map_key_clear(c) hid_map_usage_clear(hi, usage, bit, max, EV_KEY, (c))
>  
> -static int tpkbd_input_mapping(struct hid_device *hdev,
> +static int tpkbd_input_mapping_tp(struct hid_device *hdev,
>  		struct hid_input *hi, struct hid_field *field,
>  		struct hid_usage *usage, unsigned long **bit, int *max)
>  {
> @@ -48,6 +49,15 @@ static int tpkbd_input_mapping(struct hid_device *hdev,
>  	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);
> +	return 0;
> +}
> +
>  #undef map_key_clear
>  
>  static int tpkbd_features_set(struct hid_device *hdev)
> @@ -338,6 +348,11 @@ static int tpkbd_probe_tp(struct hid_device *hdev)
>  	char *name_mute, *name_micmute;
>  	int i;
>  
> +	/* Ignore unless tpkbd_input_mapping_tp marked it as a pointer */
> +	if (!hid_get_drvdata(hdev))
> +		return 0;
> +	hid_set_drvdata(hdev, NULL);
> +

Maybe add a blank line before hid_set_drvdata().

JFTR this logic, already in the original code, relies on the fact that
the input_mapping callback is invoked before tpkbd_probe_tp():

	hid_hw_start()
		hid_connect()
			hidinput_connect()
				hidinput_configure_usage()
					device->driver->input_mapping()
	tpkbd_probe_tp()

which was not completely trivial to an occasional reader like me.

>  	/* Validate required reports. */
>  	for (i = 0; i < 4; i++) {
>  		if (!hid_validate_values(hdev, HID_FEATURE_REPORT, 4, i, 1))
> @@ -408,12 +423,9 @@ static int tpkbd_probe(struct hid_device *hdev,
>  		goto err;
>  	}
>  
> -	if (hid_get_drvdata(hdev)) {
> -		hid_set_drvdata(hdev, NULL);
> -		ret = tpkbd_probe_tp(hdev);
> -		if (ret)
> -			goto err_hid;
> -	}
> +	if (hdev->product == USB_DEVICE_ID_LENOVO_TPKBD
> +			&& tpkbd_probe_tp(hdev))
> +		goto err_hid;
>

I'd avoid calling functions in conditions, especially when mixed with
other checks, but that's mostly personal taste. If you decide to call
the function in the if body you also get that the hdev->product check
will be exactly the same as in the symmetric one in tpkbd_remove().

>  	return 0;
>  err_hid:
> @@ -437,7 +449,10 @@ static void tpkbd_remove_tp(struct hid_device *hdev)
>  
>  static void tpkbd_remove(struct hid_device *hdev)
>  {
> -	if (hid_get_drvdata(hdev))
> +	if (!hid_get_drvdata(hdev))
> +		return;
> +

I think this check can just become a:

	if (data_pointer == NULL)
		return;

early in tpkbd_remove_tp().

Also, I don't think you could just return in tpkbd_remove() like
you are doing as hid_hw_stop() must be always called.

> +	if (hdev->product == USB_DEVICE_ID_LENOVO_TPKBD)
>  		tpkbd_remove_tp(hdev);
>  
>  	hid_hw_stop(hdev);
> -- 
> 2.0.0.rc2
> 
> --
> 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

Ciao Ciao,
   Antonio

-- 
Antonio Ospite
http://ao2.it

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?

[PATCH v2 2/2] Add support for Compact (Bluetooth|USB) keyboard with Trackpoint

[Jamie Lentin]

Signed-off-by: Jamie Lentin <jm@lentin.co.uk>
---
 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
  *
  *  Copyright (c) 2012 Bernhard Seibold
+ *  Copyright (c) 2014 Jamie Lentin <jm@lentin.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,
+		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)
+		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)) {
+		hid_warn(hdev, "Could not create sysfs group\n");
+	}
+
+	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))
+		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)
+		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

Re: [PATCH v2 2/2] Add support for Compact (Bluetooth|USB) keyboard with Trackpoint

[Antonio Ospite]

On Tue, 10 Jun 2014 23:24:54 +0100
Jamie Lentin <jm@lentin.co.uk> wrote:

> Signed-off-by: Jamie Lentin <jm@lentin.co.uk>

Some minor comments here too.

> ---
>  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@lentin.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.

> +		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)

> +		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@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Ciao ciao,
   Antonio

-- 
Antonio Ospite
http://ao2.it

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?

Re: [PATCH v2 2/2] Add support for Compact (Bluetooth|USB) keyboard with Trackpoint

[Jamie Lentin]

On Wed, 11 Jun 2014, Antonio Ospite wrote:

> On Tue, 10 Jun 2014 23:24:54 +0100
> Jamie Lentin <jm@lentin.co.uk> wrote:
>
>> Signed-off-by: Jamie Lentin <jm@lentin.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@lentin.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@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>
> Ciao ciao,
>   Antonio
>
>

-- 
Jamie Lentin

Re: [PATCH v2 2/2] Add support for Compact (Bluetooth|USB) keyboard with Trackpoint

[Antonio Ospite]

On Thu, 12 Jun 2014 09:56:41 +0100 (BST)
Jamie Lentin <jm@lentin.co.uk> wrote:

> On Wed, 11 Jun 2014, Antonio Ospite wrote:
> 

[...]

> >> +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:-
> 

I am OK with a rename. Most files in drivers/hid are per-vendor after
all. Jiri?

> * lenovo_input_mapping_tpkbd
> * lenovo_input_mapping_compacttp

I think you meant lenovo_input_mapping_compactkbd ? :)

> ...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.
> 
[...]

Ciao,
   Antonio

-- 
Antonio Ospite
http://ao2.it

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?

Re: [PATCH v2 2/2] Add support for Compact (Bluetooth|USB) keyboard with Trackpoint

[Jiri Kosina]

On Fri, 13 Jun 2014, Antonio Ospite wrote:

> > 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:-
> > 
> 
> I am OK with a rename. Most files in drivers/hid are per-vendor after
> all. Jiri?

Fine by me; the module doesn't take any parameters, so we are not risking 
introducing regression for people who'd have put parameter settings in 
modprobe.conf or some such.

So please go ahead with the rename.

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH v2 0/2] Add support for Lenovo Compact Keyboard

[Jiri Kosina]

On Tue, 10 Jun 2014, Jamie Lentin wrote:

> This patchset follows on from my previous attempt[1] to add support for
> these keyboards from Lenovo.

Jamie,

thanks for the driver. Please re-send v3 with Antonio's comments 
addressed, and I'll be happy to apply it.

-- 
Jiri Kosina
SUSE Labs