[PATCH v4 0/4] Add support for Lenovo Compact Keyboard

[PATCH v4 0/4] Add support for Lenovo Compact Keyboard

[Jamie Lentin]

This patchset follows on from my previous attempts to add support for
these keyboards from Lenovo.

Changes since v3: https://lkml.org/lkml/2014/6/15/159
* Break up first patch [Antonio Ospite]
* Rename ABI docs too [Jiri Kosina]
* Spelling and Grammar, style fixes [Antonio Ospite]
* Driver should just be called 'lenovo' [Antonio Ospite]
* Remove useless id from probe functions [Antonio Ospite]
* Use KEY_SCALE to match Thinkpad ??40 models [Hans de Goede]
* Add ABI documentation for fn_lock setting [Jiri Kosina]
* Use a bool for fn_lock instead of uint [Antonio Ospite]
* hid_output_raw_report has been replaced with hid_hw_raw_request [Antonio Ospite]

Changes since v2: https://lkml.org/lkml/2014/6/10/730
* Rename hid-lenovo-tpkbd to hid-lenovo, to make it obvious this is
  for any Lenovo-manufactured device [Antonio Ospite, Jiri Kosina]
* Style fixes: function calls in conditions, combine checks for 
  both USB & BT keyboards [Antonio Ospite]

Changes since v1: https://lkml.org/lkml/2014/3/25/535
* 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

Applies against v3.16-rc4, tested with Bluetooth and USB variants of the
Compact Keyboard with Trackpoint.

Cheers,

Jamie Lentin (4):
  Rename hid-lenovo-tpkbd to hid-lenovo
  Make all base functions switch depending on product ID
  Style fixes
  Add support for Compact (Bluetooth|USB) keyboard with Trackpoint

 ...er-hid-lenovo-tpkbd => sysfs-driver-hid-lenovo} |  12 +
 drivers/hid/Kconfig                                |  16 +-
 drivers/hid/Makefile                               |   2 +-
 drivers/hid/hid-core.c                             |   4 +-
 drivers/hid/hid-ids.h                              |   2 +
 drivers/hid/hid-lenovo-tpkbd.c                     | 462 --------------
 drivers/hid/hid-lenovo.c                           | 696 +++++++++++++++++++++
 include/linux/hid.h                                |   1 +
 8 files changed, 724 insertions(+), 471 deletions(-)
 rename Documentation/ABI/testing/{sysfs-driver-hid-lenovo-tpkbd => sysfs-driver-hid-lenovo} (75%)
 delete mode 100644 drivers/hid/hid-lenovo-tpkbd.c
 create mode 100644 drivers/hid/hid-lenovo.c

-- 
2.0.0

[PATCH v4 1/4] Rename hid-lenovo-tpkbd to hid-lenovo

[Jamie Lentin]

Rename module and all functions within so we can add support for other
keyboards in the same file. Rename the _tp postfix to _tpkbd, to
signify functions relevant to the TP USB keyboard.

Signed-off-by: Jamie Lentin <jm@lentin.co.uk>
---
 ...er-hid-lenovo-tpkbd => sysfs-driver-hid-lenovo} |   0
 drivers/hid/Kconfig                                |  14 +-
 drivers/hid/Makefile                               |   2 +-
 drivers/hid/hid-core.c                             |   2 +-
 drivers/hid/{hid-lenovo-tpkbd.c => hid-lenovo.c}   | 185 +++++++++++----------
 5 files changed, 102 insertions(+), 101 deletions(-)
 rename Documentation/ABI/testing/{sysfs-driver-hid-lenovo-tpkbd => sysfs-driver-hid-lenovo} (100%)
 rename drivers/hid/{hid-lenovo-tpkbd.c => hid-lenovo.c} (64%)

diff --git a/Documentation/ABI/testing/sysfs-driver-hid-lenovo-tpkbd b/Documentation/ABI/testing/sysfs-driver-hid-lenovo
similarity index 100%
rename from Documentation/ABI/testing/sysfs-driver-hid-lenovo-tpkbd
rename to Documentation/ABI/testing/sysfs-driver-hid-lenovo
diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 800c8b6..9b7acfc 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -331,18 +331,18 @@ config HID_LCPOWER
 	---help---
 	Support for LC-Power RC1000MCE RF remote control.
 
-config HID_LENOVO_TPKBD
-	tristate "Lenovo ThinkPad USB Keyboard with TrackPoint"
+config HID_LENOVO
+	tristate "Lenovo / Thinkpad devices"
 	depends on HID
 	select NEW_LEDS
 	select LEDS_CLASS
 	---help---
-	Support for the Lenovo ThinkPad USB Keyboard with TrackPoint.
+	Support for Lenovo devices that are not fully compliant with HID standard.
 
-	Say Y here if you have a Lenovo ThinkPad USB Keyboard with TrackPoint
-	and would like to use device-specific features like changing the
-	sensitivity of the trackpoint, using the microphone mute button or
-	controlling the mute and microphone mute LEDs.
+	Say Y if you want support for the non-compliant features of the Lenovo
+	Thinkpad standalone keyboards, e.g:
+	- ThinkPad USB Keyboard with TrackPoint (supports extra LEDs and trackpoint
+	  configuration)
 
 config HID_LOGITECH
 	tristate "Logitech devices" if EXPERT
diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
index a6fa6ba..5e96be3 100644
--- a/drivers/hid/Makefile
+++ b/drivers/hid/Makefile
@@ -59,7 +59,7 @@ obj-$(CONFIG_HID_KENSINGTON)	+= hid-kensington.o
 obj-$(CONFIG_HID_KEYTOUCH)	+= hid-keytouch.o
 obj-$(CONFIG_HID_KYE)		+= hid-kye.o
 obj-$(CONFIG_HID_LCPOWER)       += hid-lcpower.o
-obj-$(CONFIG_HID_LENOVO_TPKBD)	+= hid-lenovo-tpkbd.o
+obj-$(CONFIG_HID_LENOVO)	+= hid-lenovo.o
 obj-$(CONFIG_HID_LOGITECH)	+= hid-logitech.o
 obj-$(CONFIG_HID_LOGITECH_DJ)	+= hid-logitech-dj.o
 obj-$(CONFIG_HID_MAGICMOUSE)    += hid-magicmouse.o
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 8ed66fd..55841bd 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1796,7 +1796,7 @@ static const struct hid_device_id hid_have_special_driver[] = {
 	{ HID_USB_DEVICE(USB_VENDOR_ID_KYE, USB_DEVICE_ID_KYE_EASYPEN_M610X) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_LABTEC, USB_DEVICE_ID_LABTEC_WIRELESS_KEYBOARD) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_LCPOWER, USB_DEVICE_ID_LCPOWER_LC1000 ) },
-#if IS_ENABLED(CONFIG_HID_LENOVO_TPKBD)
+#if IS_ENABLED(CONFIG_HID_LENOVO)
 	{ HID_USB_DEVICE(USB_VENDOR_ID_LENOVO, USB_DEVICE_ID_LENOVO_TPKBD) },
 #endif
 	{ HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_MX3000_RECEIVER) },
diff --git a/drivers/hid/hid-lenovo-tpkbd.c b/drivers/hid/hid-lenovo.c
similarity index 64%
rename from drivers/hid/hid-lenovo-tpkbd.c
rename to drivers/hid/hid-lenovo.c
index 2d25b6c..0320b96 100644
--- a/drivers/hid/hid-lenovo-tpkbd.c
+++ b/drivers/hid/hid-lenovo.c
@@ -20,8 +20,7 @@
 
 #include "hid-ids.h"
 
-/* This is only used for the trackpoint part of the driver, hence _tp */
-struct tpkbd_data_pointer {
+struct lenovo_drvdata_tpkbd {
 	int led_state;
 	struct led_classdev led_mute;
 	struct led_classdev led_micmute;
@@ -35,7 +34,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 lenovo_input_mapping_tpkbd(struct hid_device *hdev,
 		struct hid_input *hi, struct hid_field *field,
 		struct hid_usage *usage, unsigned long **bit, int *max)
 {
@@ -50,10 +49,10 @@ static int tpkbd_input_mapping(struct hid_device *hdev,
 
 #undef map_key_clear
 
-static int tpkbd_features_set(struct hid_device *hdev)
+static int lenovo_features_set_tpkbd(struct hid_device *hdev)
 {
 	struct hid_report *report;
-	struct tpkbd_data_pointer *data_pointer = hid_get_drvdata(hdev);
+	struct lenovo_drvdata_tpkbd *data_pointer = hid_get_drvdata(hdev);
 
 	report = hdev->report_enum[HID_FEATURE_REPORT].report_id_hash[4];
 
@@ -69,23 +68,23 @@ static int tpkbd_features_set(struct hid_device *hdev)
 	return 0;
 }
 
-static ssize_t pointer_press_to_select_show(struct device *dev,
+static ssize_t attr_press_to_select_show_tpkbd(struct device *dev,
 		struct device_attribute *attr,
 		char *buf)
 {
 	struct hid_device *hdev = container_of(dev, struct hid_device, dev);
-	struct tpkbd_data_pointer *data_pointer = hid_get_drvdata(hdev);
+	struct lenovo_drvdata_tpkbd *data_pointer = hid_get_drvdata(hdev);
 
 	return snprintf(buf, PAGE_SIZE, "%u\n", data_pointer->press_to_select);
 }
 
-static ssize_t pointer_press_to_select_store(struct device *dev,
+static ssize_t attr_press_to_select_store_tpkbd(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 tpkbd_data_pointer *data_pointer = hid_get_drvdata(hdev);
+	struct lenovo_drvdata_tpkbd *data_pointer = hid_get_drvdata(hdev);
 	int value;
 
 	if (kstrtoint(buf, 10, &value))
@@ -94,28 +93,28 @@ static ssize_t pointer_press_to_select_store(struct device *dev,
 		return -EINVAL;
 
 	data_pointer->press_to_select = value;
-	tpkbd_features_set(hdev);
+	lenovo_features_set_tpkbd(hdev);
 
 	return count;
 }
 
-static ssize_t pointer_dragging_show(struct device *dev,
+static ssize_t attr_dragging_show_tpkbd(struct device *dev,
 		struct device_attribute *attr,
 		char *buf)
 {
 	struct hid_device *hdev = container_of(dev, struct hid_device, dev);
-	struct tpkbd_data_pointer *data_pointer = hid_get_drvdata(hdev);
+	struct lenovo_drvdata_tpkbd *data_pointer = hid_get_drvdata(hdev);
 
 	return snprintf(buf, PAGE_SIZE, "%u\n", data_pointer->dragging);
 }
 
-static ssize_t pointer_dragging_store(struct device *dev,
+static ssize_t attr_dragging_store_tpkbd(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 tpkbd_data_pointer *data_pointer = hid_get_drvdata(hdev);
+	struct lenovo_drvdata_tpkbd *data_pointer = hid_get_drvdata(hdev);
 	int value;
 
 	if (kstrtoint(buf, 10, &value))
@@ -124,28 +123,28 @@ static ssize_t pointer_dragging_store(struct device *dev,
 		return -EINVAL;
 
 	data_pointer->dragging = value;
-	tpkbd_features_set(hdev);
+	lenovo_features_set_tpkbd(hdev);
 
 	return count;
 }
 
-static ssize_t pointer_release_to_select_show(struct device *dev,
+static ssize_t attr_release_to_select_show_tpkbd(struct device *dev,
 		struct device_attribute *attr,
 		char *buf)
 {
 	struct hid_device *hdev = container_of(dev, struct hid_device, dev);
-	struct tpkbd_data_pointer *data_pointer = hid_get_drvdata(hdev);
+	struct lenovo_drvdata_tpkbd *data_pointer = hid_get_drvdata(hdev);
 
 	return snprintf(buf, PAGE_SIZE, "%u\n", data_pointer->release_to_select);
 }
 
-static ssize_t pointer_release_to_select_store(struct device *dev,
+static ssize_t attr_release_to_select_store_tpkbd(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 tpkbd_data_pointer *data_pointer = hid_get_drvdata(hdev);
+	struct lenovo_drvdata_tpkbd *data_pointer = hid_get_drvdata(hdev);
 	int value;
 
 	if (kstrtoint(buf, 10, &value))
@@ -154,28 +153,28 @@ static ssize_t pointer_release_to_select_store(struct device *dev,
 		return -EINVAL;
 
 	data_pointer->release_to_select = value;
-	tpkbd_features_set(hdev);
+	lenovo_features_set_tpkbd(hdev);
 
 	return count;
 }
 
-static ssize_t pointer_select_right_show(struct device *dev,
+static ssize_t attr_select_right_show_tpkbd(struct device *dev,
 		struct device_attribute *attr,
 		char *buf)
 {
 	struct hid_device *hdev = container_of(dev, struct hid_device, dev);
-	struct tpkbd_data_pointer *data_pointer = hid_get_drvdata(hdev);
+	struct lenovo_drvdata_tpkbd *data_pointer = hid_get_drvdata(hdev);
 
 	return snprintf(buf, PAGE_SIZE, "%u\n", data_pointer->select_right);
 }
 
-static ssize_t pointer_select_right_store(struct device *dev,
+static ssize_t attr_select_right_store_tpkbd(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 tpkbd_data_pointer *data_pointer = hid_get_drvdata(hdev);
+	struct lenovo_drvdata_tpkbd *data_pointer = hid_get_drvdata(hdev);
 	int value;
 
 	if (kstrtoint(buf, 10, &value))
@@ -184,119 +183,119 @@ static ssize_t pointer_select_right_store(struct device *dev,
 		return -EINVAL;
 
 	data_pointer->select_right = value;
-	tpkbd_features_set(hdev);
+	lenovo_features_set_tpkbd(hdev);
 
 	return count;
 }
 
-static ssize_t pointer_sensitivity_show(struct device *dev,
+static ssize_t attr_sensitivity_show_tpkbd(struct device *dev,
 		struct device_attribute *attr,
 		char *buf)
 {
 	struct hid_device *hdev = container_of(dev, struct hid_device, dev);
-	struct tpkbd_data_pointer *data_pointer = hid_get_drvdata(hdev);
+	struct lenovo_drvdata_tpkbd *data_pointer = hid_get_drvdata(hdev);
 
 	return snprintf(buf, PAGE_SIZE, "%u\n",
 		data_pointer->sensitivity);
 }
 
-static ssize_t pointer_sensitivity_store(struct device *dev,
+static ssize_t attr_sensitivity_store_tpkbd(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 tpkbd_data_pointer *data_pointer = hid_get_drvdata(hdev);
+	struct lenovo_drvdata_tpkbd *data_pointer = hid_get_drvdata(hdev);
 	int value;
 
 	if (kstrtoint(buf, 10, &value) || value < 1 || value > 255)
 		return -EINVAL;
 
 	data_pointer->sensitivity = value;
-	tpkbd_features_set(hdev);
+	lenovo_features_set_tpkbd(hdev);
 
 	return count;
 }
 
-static ssize_t pointer_press_speed_show(struct device *dev,
+static ssize_t attr_press_speed_show_tpkbd(struct device *dev,
 		struct device_attribute *attr,
 		char *buf)
 {
 	struct hid_device *hdev = container_of(dev, struct hid_device, dev);
-	struct tpkbd_data_pointer *data_pointer = hid_get_drvdata(hdev);
+	struct lenovo_drvdata_tpkbd *data_pointer = hid_get_drvdata(hdev);
 
 	return snprintf(buf, PAGE_SIZE, "%u\n",
 		data_pointer->press_speed);
 }
 
-static ssize_t pointer_press_speed_store(struct device *dev,
+static ssize_t attr_press_speed_store_tpkbd(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 tpkbd_data_pointer *data_pointer = hid_get_drvdata(hdev);
+	struct lenovo_drvdata_tpkbd *data_pointer = hid_get_drvdata(hdev);
 	int value;
 
 	if (kstrtoint(buf, 10, &value) || value < 1 || value > 255)
 		return -EINVAL;
 
 	data_pointer->press_speed = value;
-	tpkbd_features_set(hdev);
+	lenovo_features_set_tpkbd(hdev);
 
 	return count;
 }
 
-static struct device_attribute dev_attr_pointer_press_to_select =
+static struct device_attribute dev_attr_press_to_select_tpkbd =
 	__ATTR(press_to_select, S_IWUSR | S_IRUGO,
-			pointer_press_to_select_show,
-			pointer_press_to_select_store);
+			attr_press_to_select_show_tpkbd,
+			attr_press_to_select_store_tpkbd);
 
-static struct device_attribute dev_attr_pointer_dragging =
+static struct device_attribute dev_attr_dragging_tpkbd =
 	__ATTR(dragging, S_IWUSR | S_IRUGO,
-			pointer_dragging_show,
-			pointer_dragging_store);
+			attr_dragging_show_tpkbd,
+			attr_dragging_store_tpkbd);
 
-static struct device_attribute dev_attr_pointer_release_to_select =
+static struct device_attribute dev_attr_release_to_select_tpkbd =
 	__ATTR(release_to_select, S_IWUSR | S_IRUGO,
-			pointer_release_to_select_show,
-			pointer_release_to_select_store);
+			attr_release_to_select_show_tpkbd,
+			attr_release_to_select_store_tpkbd);
 
-static struct device_attribute dev_attr_pointer_select_right =
+static struct device_attribute dev_attr_select_right_tpkbd =
 	__ATTR(select_right, S_IWUSR | S_IRUGO,
-			pointer_select_right_show,
-			pointer_select_right_store);
+			attr_select_right_show_tpkbd,
+			attr_select_right_store_tpkbd);
 
-static struct device_attribute dev_attr_pointer_sensitivity =
+static struct device_attribute dev_attr_sensitivity_tpkbd =
 	__ATTR(sensitivity, S_IWUSR | S_IRUGO,
-			pointer_sensitivity_show,
-			pointer_sensitivity_store);
+			attr_sensitivity_show_tpkbd,
+			attr_sensitivity_store_tpkbd);
 
-static struct device_attribute dev_attr_pointer_press_speed =
+static struct device_attribute dev_attr_press_speed_tpkbd =
 	__ATTR(press_speed, S_IWUSR | S_IRUGO,
-			pointer_press_speed_show,
-			pointer_press_speed_store);
-
-static struct attribute *tpkbd_attributes_pointer[] = {
-	&dev_attr_pointer_press_to_select.attr,
-	&dev_attr_pointer_dragging.attr,
-	&dev_attr_pointer_release_to_select.attr,
-	&dev_attr_pointer_select_right.attr,
-	&dev_attr_pointer_sensitivity.attr,
-	&dev_attr_pointer_press_speed.attr,
+			attr_press_speed_show_tpkbd,
+			attr_press_speed_store_tpkbd);
+
+static struct attribute *lenovo_attributes_tpkbd[] = {
+	&dev_attr_press_to_select_tpkbd.attr,
+	&dev_attr_dragging_tpkbd.attr,
+	&dev_attr_release_to_select_tpkbd.attr,
+	&dev_attr_select_right_tpkbd.attr,
+	&dev_attr_sensitivity_tpkbd.attr,
+	&dev_attr_press_speed_tpkbd.attr,
 	NULL
 };
 
-static const struct attribute_group tpkbd_attr_group_pointer = {
-	.attrs = tpkbd_attributes_pointer,
+static const struct attribute_group lenovo_attr_group_tpkbd = {
+	.attrs = lenovo_attributes_tpkbd,
 };
 
-static enum led_brightness tpkbd_led_brightness_get(
+static enum led_brightness lenovo_led_brightness_get_tpkbd(
 			struct led_classdev *led_cdev)
 {
 	struct device *dev = led_cdev->dev->parent;
 	struct hid_device *hdev = container_of(dev, struct hid_device, dev);
-	struct tpkbd_data_pointer *data_pointer = hid_get_drvdata(hdev);
+	struct lenovo_drvdata_tpkbd *data_pointer = hid_get_drvdata(hdev);
 	int led_nr = 0;
 
 	if (led_cdev == &data_pointer->led_micmute)
@@ -307,12 +306,12 @@ static enum led_brightness tpkbd_led_brightness_get(
 				: LED_OFF;
 }
 
-static void tpkbd_led_brightness_set(struct led_classdev *led_cdev,
+static void lenovo_led_brightness_set_tpkbd(struct led_classdev *led_cdev,
 			enum led_brightness value)
 {
 	struct device *dev = led_cdev->dev->parent;
 	struct hid_device *hdev = container_of(dev, struct hid_device, dev);
-	struct tpkbd_data_pointer *data_pointer = hid_get_drvdata(hdev);
+	struct lenovo_drvdata_tpkbd *data_pointer = hid_get_drvdata(hdev);
 	struct hid_report *report;
 	int led_nr = 0;
 
@@ -330,10 +329,10 @@ static void tpkbd_led_brightness_set(struct led_classdev *led_cdev,
 	hid_hw_request(hdev, report, HID_REQ_SET_REPORT);
 }
 
-static int tpkbd_probe_tp(struct hid_device *hdev)
+static int lenovo_probe_tpkbd(struct hid_device *hdev)
 {
 	struct device *dev = &hdev->dev;
-	struct tpkbd_data_pointer *data_pointer;
+	struct lenovo_drvdata_tpkbd *data_pointer;
 	size_t name_sz = strlen(dev_name(dev)) + 16;
 	char *name_mute, *name_micmute;
 	int i;
@@ -347,12 +346,12 @@ static int tpkbd_probe_tp(struct hid_device *hdev)
 		return -ENODEV;
 
 	if (sysfs_create_group(&hdev->dev.kobj,
-				&tpkbd_attr_group_pointer)) {
+				&lenovo_attr_group_tpkbd)) {
 		hid_warn(hdev, "Could not create sysfs group\n");
 	}
 
 	data_pointer = devm_kzalloc(&hdev->dev,
-				    sizeof(struct tpkbd_data_pointer),
+				    sizeof(struct lenovo_drvdata_tpkbd),
 				    GFP_KERNEL);
 	if (data_pointer == NULL) {
 		hid_err(hdev, "Could not allocate memory for driver data\n");
@@ -375,23 +374,25 @@ static int tpkbd_probe_tp(struct hid_device *hdev)
 	hid_set_drvdata(hdev, data_pointer);
 
 	data_pointer->led_mute.name = name_mute;
-	data_pointer->led_mute.brightness_get = tpkbd_led_brightness_get;
-	data_pointer->led_mute.brightness_set = tpkbd_led_brightness_set;
+	data_pointer->led_mute.brightness_get = lenovo_led_brightness_get_tpkbd;
+	data_pointer->led_mute.brightness_set = lenovo_led_brightness_set_tpkbd;
 	data_pointer->led_mute.dev = dev;
 	led_classdev_register(dev, &data_pointer->led_mute);
 
 	data_pointer->led_micmute.name = name_micmute;
-	data_pointer->led_micmute.brightness_get = tpkbd_led_brightness_get;
-	data_pointer->led_micmute.brightness_set = tpkbd_led_brightness_set;
+	data_pointer->led_micmute.brightness_get =
+		lenovo_led_brightness_get_tpkbd;
+	data_pointer->led_micmute.brightness_set =
+		lenovo_led_brightness_set_tpkbd;
 	data_pointer->led_micmute.dev = dev;
 	led_classdev_register(dev, &data_pointer->led_micmute);
 
-	tpkbd_features_set(hdev);
+	lenovo_features_set_tpkbd(hdev);
 
 	return 0;
 }
 
-static int tpkbd_probe(struct hid_device *hdev,
+static int lenovo_probe(struct hid_device *hdev,
 		const struct hid_device_id *id)
 {
 	int ret;
@@ -410,7 +411,7 @@ static int tpkbd_probe(struct hid_device *hdev,
 
 	if (hid_get_drvdata(hdev)) {
 		hid_set_drvdata(hdev, NULL);
-		ret = tpkbd_probe_tp(hdev);
+		ret = lenovo_probe_tpkbd(hdev);
 		if (ret)
 			goto err_hid;
 	}
@@ -422,12 +423,12 @@ err:
 	return ret;
 }
 
-static void tpkbd_remove_tp(struct hid_device *hdev)
+static void lenovo_remove_tpkbd(struct hid_device *hdev)
 {
-	struct tpkbd_data_pointer *data_pointer = hid_get_drvdata(hdev);
+	struct lenovo_drvdata_tpkbd *data_pointer = hid_get_drvdata(hdev);
 
 	sysfs_remove_group(&hdev->dev.kobj,
-			&tpkbd_attr_group_pointer);
+			&lenovo_attr_group_tpkbd);
 
 	led_classdev_unregister(&data_pointer->led_micmute);
 	led_classdev_unregister(&data_pointer->led_mute);
@@ -435,28 +436,28 @@ static void tpkbd_remove_tp(struct hid_device *hdev)
 	hid_set_drvdata(hdev, NULL);
 }
 
-static void tpkbd_remove(struct hid_device *hdev)
+static void lenovo_remove(struct hid_device *hdev)
 {
 	if (hid_get_drvdata(hdev))
-		tpkbd_remove_tp(hdev);
+		lenovo_remove_tpkbd(hdev);
 
 	hid_hw_stop(hdev);
 }
 
-static const struct hid_device_id tpkbd_devices[] = {
+static const struct hid_device_id lenovo_devices[] = {
 	{ HID_USB_DEVICE(USB_VENDOR_ID_LENOVO, USB_DEVICE_ID_LENOVO_TPKBD) },
 	{ }
 };
 
-MODULE_DEVICE_TABLE(hid, tpkbd_devices);
+MODULE_DEVICE_TABLE(hid, lenovo_devices);
 
-static struct hid_driver tpkbd_driver = {
-	.name = "lenovo_tpkbd",
-	.id_table = tpkbd_devices,
-	.input_mapping = tpkbd_input_mapping,
-	.probe = tpkbd_probe,
-	.remove = tpkbd_remove,
+static struct hid_driver lenovo_driver = {
+	.name = "lenovo",
+	.id_table = lenovo_devices,
+	.input_mapping = lenovo_input_mapping_tpkbd,
+	.probe = lenovo_probe,
+	.remove = lenovo_remove,
 };
-module_hid_driver(tpkbd_driver);
+module_hid_driver(lenovo_driver);
 
 MODULE_LICENSE("GPL");
-- 
2.0.0

Re: [PATCH v4 1/4] Rename hid-lenovo-tpkbd to hid-lenovo

[Antonio Ospite]

On Sun, 13 Jul 2014 08:24:19 +0100
Jamie Lentin <jm@lentin.co.uk> wrote:

> Rename module and all functions within so we can add support for other
> keyboards in the same file. Rename the _tp postfix to _tpkbd, to
> signify functions relevant to the TP USB keyboard.
> 
> Signed-off-by: Jamie Lentin <jm@lentin.co.uk>

Reviewed-by: Antonio Ospite <ao2@ao2.it>

Patch looks good to me, easier to validate now :)

A few ideas below for some possible _future_ cleanups, but no reason to
hold this one any longer IMHO.

Ciao,
   Antonio

> ---
>  ...er-hid-lenovo-tpkbd => sysfs-driver-hid-lenovo} |   0
>  drivers/hid/Kconfig                                |  14 +-
>  drivers/hid/Makefile                               |   2 +-
>  drivers/hid/hid-core.c                             |   2 +-
>  drivers/hid/{hid-lenovo-tpkbd.c => hid-lenovo.c}   | 185 +++++++++++----------
>  5 files changed, 102 insertions(+), 101 deletions(-)
>  rename Documentation/ABI/testing/{sysfs-driver-hid-lenovo-tpkbd => sysfs-driver-hid-lenovo} (100%)
>  rename drivers/hid/{hid-lenovo-tpkbd.c => hid-lenovo.c} (64%)
> 
> diff --git a/Documentation/ABI/testing/sysfs-driver-hid-lenovo-tpkbd b/Documentation/ABI/testing/sysfs-driver-hid-lenovo
> similarity index 100%
> rename from Documentation/ABI/testing/sysfs-driver-hid-lenovo-tpkbd
> rename to Documentation/ABI/testing/sysfs-driver-hid-lenovo
> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> index 800c8b6..9b7acfc 100644
> --- a/drivers/hid/Kconfig
> +++ b/drivers/hid/Kconfig
> @@ -331,18 +331,18 @@ config HID_LCPOWER
>  	---help---
>  	Support for LC-Power RC1000MCE RF remote control.
>  
> -config HID_LENOVO_TPKBD
> -	tristate "Lenovo ThinkPad USB Keyboard with TrackPoint"
> +config HID_LENOVO
> +	tristate "Lenovo / Thinkpad devices"
>  	depends on HID
>  	select NEW_LEDS
>  	select LEDS_CLASS
>  	---help---
> -	Support for the Lenovo ThinkPad USB Keyboard with TrackPoint.
> +	Support for Lenovo devices that are not fully compliant with HID standard.
>  
> -	Say Y here if you have a Lenovo ThinkPad USB Keyboard with TrackPoint
> -	and would like to use device-specific features like changing the
> -	sensitivity of the trackpoint, using the microphone mute button or
> -	controlling the mute and microphone mute LEDs.
> +	Say Y if you want support for the non-compliant features of the Lenovo
> +	Thinkpad standalone keyboards, e.g:

This could become:

	Say Y if you want support for the non-HID-compliant features of Lenovo
	devices, e.g:
	...

because, in principle, the driver is not only about keyboards anymore.

>
> +	- ThinkPad USB Keyboard with TrackPoint (supports extra LEDs and trackpoint
> +	  configuration)
>  
>  config HID_LOGITECH
>  	tristate "Logitech devices" if EXPERT
> diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
> index a6fa6ba..5e96be3 100644
> --- a/drivers/hid/Makefile
> +++ b/drivers/hid/Makefile
> @@ -59,7 +59,7 @@ obj-$(CONFIG_HID_KENSINGTON)	+= hid-kensington.o
>  obj-$(CONFIG_HID_KEYTOUCH)	+= hid-keytouch.o
>  obj-$(CONFIG_HID_KYE)		+= hid-kye.o
>  obj-$(CONFIG_HID_LCPOWER)       += hid-lcpower.o
> -obj-$(CONFIG_HID_LENOVO_TPKBD)	+= hid-lenovo-tpkbd.o
> +obj-$(CONFIG_HID_LENOVO)	+= hid-lenovo.o
>  obj-$(CONFIG_HID_LOGITECH)	+= hid-logitech.o
>  obj-$(CONFIG_HID_LOGITECH_DJ)	+= hid-logitech-dj.o
>  obj-$(CONFIG_HID_MAGICMOUSE)    += hid-magicmouse.o
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index 8ed66fd..55841bd 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -1796,7 +1796,7 @@ static const struct hid_device_id hid_have_special_driver[] = {
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_KYE, USB_DEVICE_ID_KYE_EASYPEN_M610X) },
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_LABTEC, USB_DEVICE_ID_LABTEC_WIRELESS_KEYBOARD) },
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_LCPOWER, USB_DEVICE_ID_LCPOWER_LC1000 ) },
> -#if IS_ENABLED(CONFIG_HID_LENOVO_TPKBD)
> +#if IS_ENABLED(CONFIG_HID_LENOVO)
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_LENOVO, USB_DEVICE_ID_LENOVO_TPKBD) },
>  #endif
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_MX3000_RECEIVER) },
> diff --git a/drivers/hid/hid-lenovo-tpkbd.c b/drivers/hid/hid-lenovo.c
> similarity index 64%
> rename from drivers/hid/hid-lenovo-tpkbd.c
> rename to drivers/hid/hid-lenovo.c
> index 2d25b6c..0320b96 100644
> --- a/drivers/hid/hid-lenovo-tpkbd.c
> +++ b/drivers/hid/hid-lenovo.c
> @@ -20,8 +20,7 @@
>  
>  #include "hid-ids.h"
>  
> -/* This is only used for the trackpoint part of the driver, hence _tp */
> -struct tpkbd_data_pointer {
> +struct lenovo_drvdata_tpkbd {
>  	int led_state;
>  	struct led_classdev led_mute;
>  	struct led_classdev led_micmute;
> @@ -35,7 +34,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 lenovo_input_mapping_tpkbd(struct hid_device *hdev,
>  		struct hid_input *hi, struct hid_field *field,
>  		struct hid_usage *usage, unsigned long **bit, int *max)
>  {
> @@ -50,10 +49,10 @@ static int tpkbd_input_mapping(struct hid_device *hdev,
>  
>  #undef map_key_clear
>  
> -static int tpkbd_features_set(struct hid_device *hdev)
> +static int lenovo_features_set_tpkbd(struct hid_device *hdev)
>  {
>  	struct hid_report *report;
> -	struct tpkbd_data_pointer *data_pointer = hid_get_drvdata(hdev);
> +	struct lenovo_drvdata_tpkbd *data_pointer = hid_get_drvdata(hdev);


I think the word "pointer" here is misleading, sometimes in other parts
of the driver it is used as a synonym of trackpoint, but
lenovo_drvdata_tpkbd also contains data about leds, so here it looks
like it refers to C pointers. I'd call the variable tpkbd_data or
tpkbd_drvdata, throughout the driver. What do you think?

And the driver could standardize on the word "trackpoint" where
appropriate in order to eliminate any ambiguity.

I know, these are not your faults :)

>  
>  	report = hdev->report_enum[HID_FEATURE_REPORT].report_id_hash[4];
>  
> @@ -69,23 +68,23 @@ static int tpkbd_features_set(struct hid_device *hdev)
>  	return 0;
>  }
>  
> -static ssize_t pointer_press_to_select_show(struct device *dev,
> +static ssize_t attr_press_to_select_show_tpkbd(struct device *dev,
>  		struct device_attribute *attr,
>  		char *buf)
>  {
>  	struct hid_device *hdev = container_of(dev, struct hid_device, dev);
> -	struct tpkbd_data_pointer *data_pointer = hid_get_drvdata(hdev);
> +	struct lenovo_drvdata_tpkbd *data_pointer = hid_get_drvdata(hdev);
>  
>  	return snprintf(buf, PAGE_SIZE, "%u\n", data_pointer->press_to_select);
>  }
>  
> -static ssize_t pointer_press_to_select_store(struct device *dev,
> +static ssize_t attr_press_to_select_store_tpkbd(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 tpkbd_data_pointer *data_pointer = hid_get_drvdata(hdev);
> +	struct lenovo_drvdata_tpkbd *data_pointer = hid_get_drvdata(hdev);
>  	int value;
>  
>  	if (kstrtoint(buf, 10, &value))
> @@ -94,28 +93,28 @@ static ssize_t pointer_press_to_select_store(struct device *dev,
>  		return -EINVAL;
>  
>  	data_pointer->press_to_select = value;
> -	tpkbd_features_set(hdev);
> +	lenovo_features_set_tpkbd(hdev);
>  
>  	return count;
>  }
>  
> -static ssize_t pointer_dragging_show(struct device *dev,
> +static ssize_t attr_dragging_show_tpkbd(struct device *dev,
>  		struct device_attribute *attr,
>  		char *buf)
>  {
>  	struct hid_device *hdev = container_of(dev, struct hid_device, dev);
> -	struct tpkbd_data_pointer *data_pointer = hid_get_drvdata(hdev);
> +	struct lenovo_drvdata_tpkbd *data_pointer = hid_get_drvdata(hdev);
>  
>  	return snprintf(buf, PAGE_SIZE, "%u\n", data_pointer->dragging);
>  }
>  
> -static ssize_t pointer_dragging_store(struct device *dev,
> +static ssize_t attr_dragging_store_tpkbd(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 tpkbd_data_pointer *data_pointer = hid_get_drvdata(hdev);
> +	struct lenovo_drvdata_tpkbd *data_pointer = hid_get_drvdata(hdev);
>  	int value;
>  
>  	if (kstrtoint(buf, 10, &value))
> @@ -124,28 +123,28 @@ static ssize_t pointer_dragging_store(struct device *dev,
>  		return -EINVAL;
>  
>  	data_pointer->dragging = value;
> -	tpkbd_features_set(hdev);
> +	lenovo_features_set_tpkbd(hdev);
>  
>  	return count;
>  }
>  
> -static ssize_t pointer_release_to_select_show(struct device *dev,
> +static ssize_t attr_release_to_select_show_tpkbd(struct device *dev,
>  		struct device_attribute *attr,
>  		char *buf)
>  {
>  	struct hid_device *hdev = container_of(dev, struct hid_device, dev);
> -	struct tpkbd_data_pointer *data_pointer = hid_get_drvdata(hdev);
> +	struct lenovo_drvdata_tpkbd *data_pointer = hid_get_drvdata(hdev);
>  
>  	return snprintf(buf, PAGE_SIZE, "%u\n", data_pointer->release_to_select);
>  }
>  
> -static ssize_t pointer_release_to_select_store(struct device *dev,
> +static ssize_t attr_release_to_select_store_tpkbd(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 tpkbd_data_pointer *data_pointer = hid_get_drvdata(hdev);
> +	struct lenovo_drvdata_tpkbd *data_pointer = hid_get_drvdata(hdev);
>  	int value;
>  
>  	if (kstrtoint(buf, 10, &value))
> @@ -154,28 +153,28 @@ static ssize_t pointer_release_to_select_store(struct device *dev,
>  		return -EINVAL;
>  
>  	data_pointer->release_to_select = value;
> -	tpkbd_features_set(hdev);
> +	lenovo_features_set_tpkbd(hdev);
>  
>  	return count;
>  }
>  
> -static ssize_t pointer_select_right_show(struct device *dev,
> +static ssize_t attr_select_right_show_tpkbd(struct device *dev,
>  		struct device_attribute *attr,
>  		char *buf)
>  {
>  	struct hid_device *hdev = container_of(dev, struct hid_device, dev);
> -	struct tpkbd_data_pointer *data_pointer = hid_get_drvdata(hdev);
> +	struct lenovo_drvdata_tpkbd *data_pointer = hid_get_drvdata(hdev);
>  
>  	return snprintf(buf, PAGE_SIZE, "%u\n", data_pointer->select_right);
>  }
>  
> -static ssize_t pointer_select_right_store(struct device *dev,
> +static ssize_t attr_select_right_store_tpkbd(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 tpkbd_data_pointer *data_pointer = hid_get_drvdata(hdev);
> +	struct lenovo_drvdata_tpkbd *data_pointer = hid_get_drvdata(hdev);
>  	int value;
>  
>  	if (kstrtoint(buf, 10, &value))
> @@ -184,119 +183,119 @@ static ssize_t pointer_select_right_store(struct device *dev,
>  		return -EINVAL;
>  
>  	data_pointer->select_right = value;
> -	tpkbd_features_set(hdev);
> +	lenovo_features_set_tpkbd(hdev);
>  
>  	return count;
>  }
>  
> -static ssize_t pointer_sensitivity_show(struct device *dev,
> +static ssize_t attr_sensitivity_show_tpkbd(struct device *dev,
>  		struct device_attribute *attr,
>  		char *buf)
>  {
>  	struct hid_device *hdev = container_of(dev, struct hid_device, dev);
> -	struct tpkbd_data_pointer *data_pointer = hid_get_drvdata(hdev);
> +	struct lenovo_drvdata_tpkbd *data_pointer = hid_get_drvdata(hdev);
>  
>  	return snprintf(buf, PAGE_SIZE, "%u\n",
>  		data_pointer->sensitivity);
>  }
>  
> -static ssize_t pointer_sensitivity_store(struct device *dev,
> +static ssize_t attr_sensitivity_store_tpkbd(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 tpkbd_data_pointer *data_pointer = hid_get_drvdata(hdev);
> +	struct lenovo_drvdata_tpkbd *data_pointer = hid_get_drvdata(hdev);
>  	int value;
>  
>  	if (kstrtoint(buf, 10, &value) || value < 1 || value > 255)
>  		return -EINVAL;
>  
>  	data_pointer->sensitivity = value;
> -	tpkbd_features_set(hdev);
> +	lenovo_features_set_tpkbd(hdev);
>  
>  	return count;
>  }
>  
> -static ssize_t pointer_press_speed_show(struct device *dev,
> +static ssize_t attr_press_speed_show_tpkbd(struct device *dev,
>  		struct device_attribute *attr,
>  		char *buf)
>  {
>  	struct hid_device *hdev = container_of(dev, struct hid_device, dev);
> -	struct tpkbd_data_pointer *data_pointer = hid_get_drvdata(hdev);
> +	struct lenovo_drvdata_tpkbd *data_pointer = hid_get_drvdata(hdev);
>  
>  	return snprintf(buf, PAGE_SIZE, "%u\n",
>  		data_pointer->press_speed);
>  }
>  
> -static ssize_t pointer_press_speed_store(struct device *dev,
> +static ssize_t attr_press_speed_store_tpkbd(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 tpkbd_data_pointer *data_pointer = hid_get_drvdata(hdev);
> +	struct lenovo_drvdata_tpkbd *data_pointer = hid_get_drvdata(hdev);
>  	int value;
>  
>  	if (kstrtoint(buf, 10, &value) || value < 1 || value > 255)
>  		return -EINVAL;
>  
>  	data_pointer->press_speed = value;
> -	tpkbd_features_set(hdev);
> +	lenovo_features_set_tpkbd(hdev);
>  
>  	return count;
>  }
>  
> -static struct device_attribute dev_attr_pointer_press_to_select =
> +static struct device_attribute dev_attr_press_to_select_tpkbd =
>  	__ATTR(press_to_select, S_IWUSR | S_IRUGO,
> -			pointer_press_to_select_show,
> -			pointer_press_to_select_store);
> +			attr_press_to_select_show_tpkbd,
> +			attr_press_to_select_store_tpkbd);
>  
> -static struct device_attribute dev_attr_pointer_dragging =
> +static struct device_attribute dev_attr_dragging_tpkbd =
>  	__ATTR(dragging, S_IWUSR | S_IRUGO,
> -			pointer_dragging_show,
> -			pointer_dragging_store);
> +			attr_dragging_show_tpkbd,
> +			attr_dragging_store_tpkbd);
>  
> -static struct device_attribute dev_attr_pointer_release_to_select =
> +static struct device_attribute dev_attr_release_to_select_tpkbd =
>  	__ATTR(release_to_select, S_IWUSR | S_IRUGO,
> -			pointer_release_to_select_show,
> -			pointer_release_to_select_store);
> +			attr_release_to_select_show_tpkbd,
> +			attr_release_to_select_store_tpkbd);
>  
> -static struct device_attribute dev_attr_pointer_select_right =
> +static struct device_attribute dev_attr_select_right_tpkbd =
>  	__ATTR(select_right, S_IWUSR | S_IRUGO,
> -			pointer_select_right_show,
> -			pointer_select_right_store);
> +			attr_select_right_show_tpkbd,
> +			attr_select_right_store_tpkbd);
>  
> -static struct device_attribute dev_attr_pointer_sensitivity =
> +static struct device_attribute dev_attr_sensitivity_tpkbd =
>  	__ATTR(sensitivity, S_IWUSR | S_IRUGO,
> -			pointer_sensitivity_show,
> -			pointer_sensitivity_store);
> +			attr_sensitivity_show_tpkbd,
> +			attr_sensitivity_store_tpkbd);
>  
> -static struct device_attribute dev_attr_pointer_press_speed =
> +static struct device_attribute dev_attr_press_speed_tpkbd =
>  	__ATTR(press_speed, S_IWUSR | S_IRUGO,
> -			pointer_press_speed_show,
> -			pointer_press_speed_store);
> -
> -static struct attribute *tpkbd_attributes_pointer[] = {
> -	&dev_attr_pointer_press_to_select.attr,
> -	&dev_attr_pointer_dragging.attr,
> -	&dev_attr_pointer_release_to_select.attr,
> -	&dev_attr_pointer_select_right.attr,
> -	&dev_attr_pointer_sensitivity.attr,
> -	&dev_attr_pointer_press_speed.attr,
> +			attr_press_speed_show_tpkbd,
> +			attr_press_speed_store_tpkbd);
> +
> +static struct attribute *lenovo_attributes_tpkbd[] = {
> +	&dev_attr_press_to_select_tpkbd.attr,
> +	&dev_attr_dragging_tpkbd.attr,
> +	&dev_attr_release_to_select_tpkbd.attr,
> +	&dev_attr_select_right_tpkbd.attr,
> +	&dev_attr_sensitivity_tpkbd.attr,
> +	&dev_attr_press_speed_tpkbd.attr,
>  	NULL
>  };
>  
> -static const struct attribute_group tpkbd_attr_group_pointer = {
> -	.attrs = tpkbd_attributes_pointer,
> +static const struct attribute_group lenovo_attr_group_tpkbd = {
> +	.attrs = lenovo_attributes_tpkbd,
>  };
>  
> -static enum led_brightness tpkbd_led_brightness_get(
> +static enum led_brightness lenovo_led_brightness_get_tpkbd(
>  			struct led_classdev *led_cdev)
>  {
>  	struct device *dev = led_cdev->dev->parent;
>  	struct hid_device *hdev = container_of(dev, struct hid_device, dev);
> -	struct tpkbd_data_pointer *data_pointer = hid_get_drvdata(hdev);
> +	struct lenovo_drvdata_tpkbd *data_pointer = hid_get_drvdata(hdev);
>  	int led_nr = 0;
>  
>  	if (led_cdev == &data_pointer->led_micmute)
> @@ -307,12 +306,12 @@ static enum led_brightness tpkbd_led_brightness_get(
>  				: LED_OFF;
>  }
>  
> -static void tpkbd_led_brightness_set(struct led_classdev *led_cdev,
> +static void lenovo_led_brightness_set_tpkbd(struct led_classdev *led_cdev,
>  			enum led_brightness value)
>  {
>  	struct device *dev = led_cdev->dev->parent;
>  	struct hid_device *hdev = container_of(dev, struct hid_device, dev);
> -	struct tpkbd_data_pointer *data_pointer = hid_get_drvdata(hdev);
> +	struct lenovo_drvdata_tpkbd *data_pointer = hid_get_drvdata(hdev);
>  	struct hid_report *report;
>  	int led_nr = 0;
>  
> @@ -330,10 +329,10 @@ static void tpkbd_led_brightness_set(struct led_classdev *led_cdev,
>  	hid_hw_request(hdev, report, HID_REQ_SET_REPORT);
>  }
>  
> -static int tpkbd_probe_tp(struct hid_device *hdev)
> +static int lenovo_probe_tpkbd(struct hid_device *hdev)
>  {
>  	struct device *dev = &hdev->dev;
> -	struct tpkbd_data_pointer *data_pointer;
> +	struct lenovo_drvdata_tpkbd *data_pointer;
>  	size_t name_sz = strlen(dev_name(dev)) + 16;
>  	char *name_mute, *name_micmute;
>  	int i;
> @@ -347,12 +346,12 @@ static int tpkbd_probe_tp(struct hid_device *hdev)
>  		return -ENODEV;
>  
>  	if (sysfs_create_group(&hdev->dev.kobj,
> -				&tpkbd_attr_group_pointer)) {
> +				&lenovo_attr_group_tpkbd)) {
>  		hid_warn(hdev, "Could not create sysfs group\n");
>  	}
>  
>  	data_pointer = devm_kzalloc(&hdev->dev,
> -				    sizeof(struct tpkbd_data_pointer),
> +				    sizeof(struct lenovo_drvdata_tpkbd),
>  				    GFP_KERNEL);
>  	if (data_pointer == NULL) {
>  		hid_err(hdev, "Could not allocate memory for driver data\n");
> @@ -375,23 +374,25 @@ static int tpkbd_probe_tp(struct hid_device *hdev)
>  	hid_set_drvdata(hdev, data_pointer);
>  
>  	data_pointer->led_mute.name = name_mute;
> -	data_pointer->led_mute.brightness_get = tpkbd_led_brightness_get;
> -	data_pointer->led_mute.brightness_set = tpkbd_led_brightness_set;
> +	data_pointer->led_mute.brightness_get = lenovo_led_brightness_get_tpkbd;
> +	data_pointer->led_mute.brightness_set = lenovo_led_brightness_set_tpkbd;
>  	data_pointer->led_mute.dev = dev;
>  	led_classdev_register(dev, &data_pointer->led_mute);
>  
>  	data_pointer->led_micmute.name = name_micmute;
> -	data_pointer->led_micmute.brightness_get = tpkbd_led_brightness_get;
> -	data_pointer->led_micmute.brightness_set = tpkbd_led_brightness_set;
> +	data_pointer->led_micmute.brightness_get =
> +		lenovo_led_brightness_get_tpkbd;
> +	data_pointer->led_micmute.brightness_set =
> +		lenovo_led_brightness_set_tpkbd;
>  	data_pointer->led_micmute.dev = dev;
>  	led_classdev_register(dev, &data_pointer->led_micmute);
>  
> -	tpkbd_features_set(hdev);
> +	lenovo_features_set_tpkbd(hdev);
>  
>  	return 0;
>  }
>  
> -static int tpkbd_probe(struct hid_device *hdev,
> +static int lenovo_probe(struct hid_device *hdev,
>  		const struct hid_device_id *id)
>  {
>  	int ret;
> @@ -410,7 +411,7 @@ static int tpkbd_probe(struct hid_device *hdev,
>  
>  	if (hid_get_drvdata(hdev)) {
>  		hid_set_drvdata(hdev, NULL);
> -		ret = tpkbd_probe_tp(hdev);
> +		ret = lenovo_probe_tpkbd(hdev);
>  		if (ret)
>  			goto err_hid;
>  	}
> @@ -422,12 +423,12 @@ err:
>  	return ret;
>  }
>  
> -static void tpkbd_remove_tp(struct hid_device *hdev)
> +static void lenovo_remove_tpkbd(struct hid_device *hdev)
>  {
> -	struct tpkbd_data_pointer *data_pointer = hid_get_drvdata(hdev);
> +	struct lenovo_drvdata_tpkbd *data_pointer = hid_get_drvdata(hdev);
>  
>  	sysfs_remove_group(&hdev->dev.kobj,
> -			&tpkbd_attr_group_pointer);
> +			&lenovo_attr_group_tpkbd);
>  
>  	led_classdev_unregister(&data_pointer->led_micmute);
>  	led_classdev_unregister(&data_pointer->led_mute);
> @@ -435,28 +436,28 @@ static void tpkbd_remove_tp(struct hid_device *hdev)
>  	hid_set_drvdata(hdev, NULL);
>  }
>  
> -static void tpkbd_remove(struct hid_device *hdev)
> +static void lenovo_remove(struct hid_device *hdev)
>  {
>  	if (hid_get_drvdata(hdev))
> -		tpkbd_remove_tp(hdev);
> +		lenovo_remove_tpkbd(hdev);
>  
>  	hid_hw_stop(hdev);
>  }
>  
> -static const struct hid_device_id tpkbd_devices[] = {
> +static const struct hid_device_id lenovo_devices[] = {
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_LENOVO, USB_DEVICE_ID_LENOVO_TPKBD) },
>  	{ }
>  };
>  
> -MODULE_DEVICE_TABLE(hid, tpkbd_devices);
> +MODULE_DEVICE_TABLE(hid, lenovo_devices);
>  
> -static struct hid_driver tpkbd_driver = {
> -	.name = "lenovo_tpkbd",
> -	.id_table = tpkbd_devices,
> -	.input_mapping = tpkbd_input_mapping,
> -	.probe = tpkbd_probe,
> -	.remove = tpkbd_remove,
> +static struct hid_driver lenovo_driver = {
> +	.name = "lenovo",
> +	.id_table = lenovo_devices,
> +	.input_mapping = lenovo_input_mapping_tpkbd,
> +	.probe = lenovo_probe,
> +	.remove = lenovo_remove,
>  };
> -module_hid_driver(tpkbd_driver);
> +module_hid_driver(lenovo_driver);
>  
>  MODULE_LICENSE("GPL");
> -- 
> 2.0.0
> 


-- 
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 v4 1/4] Rename hid-lenovo-tpkbd to hid-lenovo

[Jamie Lentin]

On Mon, 14 Jul 2014, Antonio Ospite wrote:

> On Sun, 13 Jul 2014 08:24:19 +0100
> Jamie Lentin <jm@lentin.co.uk> wrote:
>
>> Rename module and all functions within so we can add support for other
>> keyboards in the same file. Rename the _tp postfix to _tpkbd, to
>> signify functions relevant to the TP USB keyboard.
>>
>> Signed-off-by: Jamie Lentin <jm@lentin.co.uk>
>
> Reviewed-by: Antonio Ospite <ao2@ao2.it>
>
> Patch looks good to me, easier to validate now :)
>
> A few ideas below for some possible _future_ cleanups, but no reason to
> hold this one any longer IMHO.
>
> Ciao,
>   Antonio
>
>> ---
>>  ...er-hid-lenovo-tpkbd => sysfs-driver-hid-lenovo} |   0
>>  drivers/hid/Kconfig                                |  14 +-
>>  drivers/hid/Makefile                               |   2 +-
>>  drivers/hid/hid-core.c                             |   2 +-
>>  drivers/hid/{hid-lenovo-tpkbd.c => hid-lenovo.c}   | 185 +++++++++++----------
>>  5 files changed, 102 insertions(+), 101 deletions(-)
>>  rename Documentation/ABI/testing/{sysfs-driver-hid-lenovo-tpkbd => sysfs-driver-hid-lenovo} (100%)
>>  rename drivers/hid/{hid-lenovo-tpkbd.c => hid-lenovo.c} (64%)
>>
>> diff --git a/Documentation/ABI/testing/sysfs-driver-hid-lenovo-tpkbd b/Documentation/ABI/testing/sysfs-driver-hid-lenovo
>> similarity index 100%
>> rename from Documentation/ABI/testing/sysfs-driver-hid-lenovo-tpkbd
>> rename to Documentation/ABI/testing/sysfs-driver-hid-lenovo
>> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
>> index 800c8b6..9b7acfc 100644
>> --- a/drivers/hid/Kconfig
>> +++ b/drivers/hid/Kconfig
>> @@ -331,18 +331,18 @@ config HID_LCPOWER
>>  	---help---
>>  	Support for LC-Power RC1000MCE RF remote control.
>>
>> -config HID_LENOVO_TPKBD
>> -	tristate "Lenovo ThinkPad USB Keyboard with TrackPoint"
>> +config HID_LENOVO
>> +	tristate "Lenovo / Thinkpad devices"
>>  	depends on HID
>>  	select NEW_LEDS
>>  	select LEDS_CLASS
>>  	---help---
>> -	Support for the Lenovo ThinkPad USB Keyboard with TrackPoint.
>> +	Support for Lenovo devices that are not fully compliant with HID standard.
>>
>> -	Say Y here if you have a Lenovo ThinkPad USB Keyboard with TrackPoint
>> -	and would like to use device-specific features like changing the
>> -	sensitivity of the trackpoint, using the microphone mute button or
>> -	controlling the mute and microphone mute LEDs.
>> +	Say Y if you want support for the non-compliant features of the Lenovo
>> +	Thinkpad standalone keyboards, e.g:
>
> This could become:
>
> 	Say Y if you want support for the non-HID-compliant features of Lenovo
> 	devices, e.g:
> 	...
>
> because, in principle, the driver is not only about keyboards anymore.

True, although I'd be very surprised if anything else came up :)

>>
>> +	- ThinkPad USB Keyboard with TrackPoint (supports extra LEDs and trackpoint
>> +	  configuration)
>>
>>  config HID_LOGITECH
>>  	tristate "Logitech devices" if EXPERT
>> diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
>> index a6fa6ba..5e96be3 100644
>> --- a/drivers/hid/Makefile
>> +++ b/drivers/hid/Makefile
>> @@ -59,7 +59,7 @@ obj-$(CONFIG_HID_KENSINGTON)	+= hid-kensington.o
>>  obj-$(CONFIG_HID_KEYTOUCH)	+= hid-keytouch.o
>>  obj-$(CONFIG_HID_KYE)		+= hid-kye.o
>>  obj-$(CONFIG_HID_LCPOWER)       += hid-lcpower.o
>> -obj-$(CONFIG_HID_LENOVO_TPKBD)	+= hid-lenovo-tpkbd.o
>> +obj-$(CONFIG_HID_LENOVO)	+= hid-lenovo.o
>>  obj-$(CONFIG_HID_LOGITECH)	+= hid-logitech.o
>>  obj-$(CONFIG_HID_LOGITECH_DJ)	+= hid-logitech-dj.o
>>  obj-$(CONFIG_HID_MAGICMOUSE)    += hid-magicmouse.o
>> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
>> index 8ed66fd..55841bd 100644
>> --- a/drivers/hid/hid-core.c
>> +++ b/drivers/hid/hid-core.c
>> @@ -1796,7 +1796,7 @@ static const struct hid_device_id hid_have_special_driver[] = {
>>  	{ HID_USB_DEVICE(USB_VENDOR_ID_KYE, USB_DEVICE_ID_KYE_EASYPEN_M610X) },
>>  	{ HID_USB_DEVICE(USB_VENDOR_ID_LABTEC, USB_DEVICE_ID_LABTEC_WIRELESS_KEYBOARD) },
>>  	{ HID_USB_DEVICE(USB_VENDOR_ID_LCPOWER, USB_DEVICE_ID_LCPOWER_LC1000 ) },
>> -#if IS_ENABLED(CONFIG_HID_LENOVO_TPKBD)
>> +#if IS_ENABLED(CONFIG_HID_LENOVO)
>>  	{ HID_USB_DEVICE(USB_VENDOR_ID_LENOVO, USB_DEVICE_ID_LENOVO_TPKBD) },
>>  #endif
>>  	{ HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_MX3000_RECEIVER) },
>> diff --git a/drivers/hid/hid-lenovo-tpkbd.c b/drivers/hid/hid-lenovo.c
>> similarity index 64%
>> rename from drivers/hid/hid-lenovo-tpkbd.c
>> rename to drivers/hid/hid-lenovo.c
>> index 2d25b6c..0320b96 100644
>> --- a/drivers/hid/hid-lenovo-tpkbd.c
>> +++ b/drivers/hid/hid-lenovo.c
>> @@ -20,8 +20,7 @@
>>
>>  #include "hid-ids.h"
>>
>> -/* This is only used for the trackpoint part of the driver, hence _tp */
>> -struct tpkbd_data_pointer {
>> +struct lenovo_drvdata_tpkbd {
>>  	int led_state;
>>  	struct led_classdev led_mute;
>>  	struct led_classdev led_micmute;
>> @@ -35,7 +34,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 lenovo_input_mapping_tpkbd(struct hid_device *hdev,
>>  		struct hid_input *hi, struct hid_field *field,
>>  		struct hid_usage *usage, unsigned long **bit, int *max)
>>  {
>> @@ -50,10 +49,10 @@ static int tpkbd_input_mapping(struct hid_device *hdev,
>>
>>  #undef map_key_clear
>>
>> -static int tpkbd_features_set(struct hid_device *hdev)
>> +static int lenovo_features_set_tpkbd(struct hid_device *hdev)
>>  {
>>  	struct hid_report *report;
>> -	struct tpkbd_data_pointer *data_pointer = hid_get_drvdata(hdev);
>> +	struct lenovo_drvdata_tpkbd *data_pointer = hid_get_drvdata(hdev);
>
>
> I think the word "pointer" here is misleading, sometimes in other parts
> of the driver it is used as a synonym of trackpoint, but
> lenovo_drvdata_tpkbd also contains data about leds, so here it looks
> like it refers to C pointers. I'd call the variable tpkbd_data or
> tpkbd_drvdata, throughout the driver. What do you think?

Yes, the rationale behind the name really isn't obvious but all meanings 
are sorta correct, so it's not that bad :P

I like tpkbd_drvdata better though, and can correct it in the compact 
keyboard code too. I wanted to make the variable names match, but didn't 
get around to it, so will do it next time around.

> And the driver could standardize on the word "trackpoint" where
> appropriate in order to eliminate any ambiguity.

The key other place I can think of that this happens is for the USB 
compact keyboard, for it's HID_TYPE_USBMOUSE half (versus the keyboard 
half). Saying trackpoint there is a mis-truth, since it's also where all 
the extra keys hide. But I'll have a look through anyway.

> I know, these are not your faults :)
>
>>
>>  	report = hdev->report_enum[HID_FEATURE_REPORT].report_id_hash[4];
>>
>> @@ -69,23 +68,23 @@ static int tpkbd_features_set(struct hid_device *hdev)
>>  	return 0;
>>  }
>>
>> -static ssize_t pointer_press_to_select_show(struct device *dev,
>> +static ssize_t attr_press_to_select_show_tpkbd(struct device *dev,
>>  		struct device_attribute *attr,
>>  		char *buf)
>>  {
>>  	struct hid_device *hdev = container_of(dev, struct hid_device, dev);
>> -	struct tpkbd_data_pointer *data_pointer = hid_get_drvdata(hdev);
>> +	struct lenovo_drvdata_tpkbd *data_pointer = hid_get_drvdata(hdev);
>>
>>  	return snprintf(buf, PAGE_SIZE, "%u\n", data_pointer->press_to_select);
>>  }
>>
>> -static ssize_t pointer_press_to_select_store(struct device *dev,
>> +static ssize_t attr_press_to_select_store_tpkbd(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 tpkbd_data_pointer *data_pointer = hid_get_drvdata(hdev);
>> +	struct lenovo_drvdata_tpkbd *data_pointer = hid_get_drvdata(hdev);
>>  	int value;
>>
>>  	if (kstrtoint(buf, 10, &value))
>> @@ -94,28 +93,28 @@ static ssize_t pointer_press_to_select_store(struct device *dev,
>>  		return -EINVAL;
>>
>>  	data_pointer->press_to_select = value;
>> -	tpkbd_features_set(hdev);
>> +	lenovo_features_set_tpkbd(hdev);
>>
>>  	return count;
>>  }
>>
>> -static ssize_t pointer_dragging_show(struct device *dev,
>> +static ssize_t attr_dragging_show_tpkbd(struct device *dev,
>>  		struct device_attribute *attr,
>>  		char *buf)
>>  {
>>  	struct hid_device *hdev = container_of(dev, struct hid_device, dev);
>> -	struct tpkbd_data_pointer *data_pointer = hid_get_drvdata(hdev);
>> +	struct lenovo_drvdata_tpkbd *data_pointer = hid_get_drvdata(hdev);
>>
>>  	return snprintf(buf, PAGE_SIZE, "%u\n", data_pointer->dragging);
>>  }
>>
>> -static ssize_t pointer_dragging_store(struct device *dev,
>> +static ssize_t attr_dragging_store_tpkbd(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 tpkbd_data_pointer *data_pointer = hid_get_drvdata(hdev);
>> +	struct lenovo_drvdata_tpkbd *data_pointer = hid_get_drvdata(hdev);
>>  	int value;
>>
>>  	if (kstrtoint(buf, 10, &value))
>> @@ -124,28 +123,28 @@ static ssize_t pointer_dragging_store(struct device *dev,
>>  		return -EINVAL;
>>
>>  	data_pointer->dragging = value;
>> -	tpkbd_features_set(hdev);
>> +	lenovo_features_set_tpkbd(hdev);
>>
>>  	return count;
>>  }
>>
>> -static ssize_t pointer_release_to_select_show(struct device *dev,
>> +static ssize_t attr_release_to_select_show_tpkbd(struct device *dev,
>>  		struct device_attribute *attr,
>>  		char *buf)
>>  {
>>  	struct hid_device *hdev = container_of(dev, struct hid_device, dev);
>> -	struct tpkbd_data_pointer *data_pointer = hid_get_drvdata(hdev);
>> +	struct lenovo_drvdata_tpkbd *data_pointer = hid_get_drvdata(hdev);
>>
>>  	return snprintf(buf, PAGE_SIZE, "%u\n", data_pointer->release_to_select);
>>  }
>>
>> -static ssize_t pointer_release_to_select_store(struct device *dev,
>> +static ssize_t attr_release_to_select_store_tpkbd(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 tpkbd_data_pointer *data_pointer = hid_get_drvdata(hdev);
>> +	struct lenovo_drvdata_tpkbd *data_pointer = hid_get_drvdata(hdev);
>>  	int value;
>>
>>  	if (kstrtoint(buf, 10, &value))
>> @@ -154,28 +153,28 @@ static ssize_t pointer_release_to_select_store(struct device *dev,
>>  		return -EINVAL;
>>
>>  	data_pointer->release_to_select = value;
>> -	tpkbd_features_set(hdev);
>> +	lenovo_features_set_tpkbd(hdev);
>>
>>  	return count;
>>  }
>>
>> -static ssize_t pointer_select_right_show(struct device *dev,
>> +static ssize_t attr_select_right_show_tpkbd(struct device *dev,
>>  		struct device_attribute *attr,
>>  		char *buf)
>>  {
>>  	struct hid_device *hdev = container_of(dev, struct hid_device, dev);
>> -	struct tpkbd_data_pointer *data_pointer = hid_get_drvdata(hdev);
>> +	struct lenovo_drvdata_tpkbd *data_pointer = hid_get_drvdata(hdev);
>>
>>  	return snprintf(buf, PAGE_SIZE, "%u\n", data_pointer->select_right);
>>  }
>>
>> -static ssize_t pointer_select_right_store(struct device *dev,
>> +static ssize_t attr_select_right_store_tpkbd(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 tpkbd_data_pointer *data_pointer = hid_get_drvdata(hdev);
>> +	struct lenovo_drvdata_tpkbd *data_pointer = hid_get_drvdata(hdev);
>>  	int value;
>>
>>  	if (kstrtoint(buf, 10, &value))
>> @@ -184,119 +183,119 @@ static ssize_t pointer_select_right_store(struct device *dev,
>>  		return -EINVAL;
>>
>>  	data_pointer->select_right = value;
>> -	tpkbd_features_set(hdev);
>> +	lenovo_features_set_tpkbd(hdev);
>>
>>  	return count;
>>  }
>>
>> -static ssize_t pointer_sensitivity_show(struct device *dev,
>> +static ssize_t attr_sensitivity_show_tpkbd(struct device *dev,
>>  		struct device_attribute *attr,
>>  		char *buf)
>>  {
>>  	struct hid_device *hdev = container_of(dev, struct hid_device, dev);
>> -	struct tpkbd_data_pointer *data_pointer = hid_get_drvdata(hdev);
>> +	struct lenovo_drvdata_tpkbd *data_pointer = hid_get_drvdata(hdev);
>>
>>  	return snprintf(buf, PAGE_SIZE, "%u\n",
>>  		data_pointer->sensitivity);
>>  }
>>
>> -static ssize_t pointer_sensitivity_store(struct device *dev,
>> +static ssize_t attr_sensitivity_store_tpkbd(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 tpkbd_data_pointer *data_pointer = hid_get_drvdata(hdev);
>> +	struct lenovo_drvdata_tpkbd *data_pointer = hid_get_drvdata(hdev);
>>  	int value;
>>
>>  	if (kstrtoint(buf, 10, &value) || value < 1 || value > 255)
>>  		return -EINVAL;
>>
>>  	data_pointer->sensitivity = value;
>> -	tpkbd_features_set(hdev);
>> +	lenovo_features_set_tpkbd(hdev);
>>
>>  	return count;
>>  }
>>
>> -static ssize_t pointer_press_speed_show(struct device *dev,
>> +static ssize_t attr_press_speed_show_tpkbd(struct device *dev,
>>  		struct device_attribute *attr,
>>  		char *buf)
>>  {
>>  	struct hid_device *hdev = container_of(dev, struct hid_device, dev);
>> -	struct tpkbd_data_pointer *data_pointer = hid_get_drvdata(hdev);
>> +	struct lenovo_drvdata_tpkbd *data_pointer = hid_get_drvdata(hdev);
>>
>>  	return snprintf(buf, PAGE_SIZE, "%u\n",
>>  		data_pointer->press_speed);
>>  }
>>
>> -static ssize_t pointer_press_speed_store(struct device *dev,
>> +static ssize_t attr_press_speed_store_tpkbd(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 tpkbd_data_pointer *data_pointer = hid_get_drvdata(hdev);
>> +	struct lenovo_drvdata_tpkbd *data_pointer = hid_get_drvdata(hdev);
>>  	int value;
>>
>>  	if (kstrtoint(buf, 10, &value) || value < 1 || value > 255)
>>  		return -EINVAL;
>>
>>  	data_pointer->press_speed = value;
>> -	tpkbd_features_set(hdev);
>> +	lenovo_features_set_tpkbd(hdev);
>>
>>  	return count;
>>  }
>>
>> -static struct device_attribute dev_attr_pointer_press_to_select =
>> +static struct device_attribute dev_attr_press_to_select_tpkbd =
>>  	__ATTR(press_to_select, S_IWUSR | S_IRUGO,
>> -			pointer_press_to_select_show,
>> -			pointer_press_to_select_store);
>> +			attr_press_to_select_show_tpkbd,
>> +			attr_press_to_select_store_tpkbd);
>>
>> -static struct device_attribute dev_attr_pointer_dragging =
>> +static struct device_attribute dev_attr_dragging_tpkbd =
>>  	__ATTR(dragging, S_IWUSR | S_IRUGO,
>> -			pointer_dragging_show,
>> -			pointer_dragging_store);
>> +			attr_dragging_show_tpkbd,
>> +			attr_dragging_store_tpkbd);
>>
>> -static struct device_attribute dev_attr_pointer_release_to_select =
>> +static struct device_attribute dev_attr_release_to_select_tpkbd =
>>  	__ATTR(release_to_select, S_IWUSR | S_IRUGO,
>> -			pointer_release_to_select_show,
>> -			pointer_release_to_select_store);
>> +			attr_release_to_select_show_tpkbd,
>> +			attr_release_to_select_store_tpkbd);
>>
>> -static struct device_attribute dev_attr_pointer_select_right =
>> +static struct device_attribute dev_attr_select_right_tpkbd =
>>  	__ATTR(select_right, S_IWUSR | S_IRUGO,
>> -			pointer_select_right_show,
>> -			pointer_select_right_store);
>> +			attr_select_right_show_tpkbd,
>> +			attr_select_right_store_tpkbd);
>>
>> -static struct device_attribute dev_attr_pointer_sensitivity =
>> +static struct device_attribute dev_attr_sensitivity_tpkbd =
>>  	__ATTR(sensitivity, S_IWUSR | S_IRUGO,
>> -			pointer_sensitivity_show,
>> -			pointer_sensitivity_store);
>> +			attr_sensitivity_show_tpkbd,
>> +			attr_sensitivity_store_tpkbd);
>>
>> -static struct device_attribute dev_attr_pointer_press_speed =
>> +static struct device_attribute dev_attr_press_speed_tpkbd =
>>  	__ATTR(press_speed, S_IWUSR | S_IRUGO,
>> -			pointer_press_speed_show,
>> -			pointer_press_speed_store);
>> -
>> -static struct attribute *tpkbd_attributes_pointer[] = {
>> -	&dev_attr_pointer_press_to_select.attr,
>> -	&dev_attr_pointer_dragging.attr,
>> -	&dev_attr_pointer_release_to_select.attr,
>> -	&dev_attr_pointer_select_right.attr,
>> -	&dev_attr_pointer_sensitivity.attr,
>> -	&dev_attr_pointer_press_speed.attr,
>> +			attr_press_speed_show_tpkbd,
>> +			attr_press_speed_store_tpkbd);
>> +
>> +static struct attribute *lenovo_attributes_tpkbd[] = {
>> +	&dev_attr_press_to_select_tpkbd.attr,
>> +	&dev_attr_dragging_tpkbd.attr,
>> +	&dev_attr_release_to_select_tpkbd.attr,
>> +	&dev_attr_select_right_tpkbd.attr,
>> +	&dev_attr_sensitivity_tpkbd.attr,
>> +	&dev_attr_press_speed_tpkbd.attr,
>>  	NULL
>>  };
>>
>> -static const struct attribute_group tpkbd_attr_group_pointer = {
>> -	.attrs = tpkbd_attributes_pointer,
>> +static const struct attribute_group lenovo_attr_group_tpkbd = {
>> +	.attrs = lenovo_attributes_tpkbd,
>>  };
>>
>> -static enum led_brightness tpkbd_led_brightness_get(
>> +static enum led_brightness lenovo_led_brightness_get_tpkbd(
>>  			struct led_classdev *led_cdev)
>>  {
>>  	struct device *dev = led_cdev->dev->parent;
>>  	struct hid_device *hdev = container_of(dev, struct hid_device, dev);
>> -	struct tpkbd_data_pointer *data_pointer = hid_get_drvdata(hdev);
>> +	struct lenovo_drvdata_tpkbd *data_pointer = hid_get_drvdata(hdev);
>>  	int led_nr = 0;
>>
>>  	if (led_cdev == &data_pointer->led_micmute)
>> @@ -307,12 +306,12 @@ static enum led_brightness tpkbd_led_brightness_get(
>>  				: LED_OFF;
>>  }
>>
>> -static void tpkbd_led_brightness_set(struct led_classdev *led_cdev,
>> +static void lenovo_led_brightness_set_tpkbd(struct led_classdev *led_cdev,
>>  			enum led_brightness value)
>>  {
>>  	struct device *dev = led_cdev->dev->parent;
>>  	struct hid_device *hdev = container_of(dev, struct hid_device, dev);
>> -	struct tpkbd_data_pointer *data_pointer = hid_get_drvdata(hdev);
>> +	struct lenovo_drvdata_tpkbd *data_pointer = hid_get_drvdata(hdev);
>>  	struct hid_report *report;
>>  	int led_nr = 0;
>>
>> @@ -330,10 +329,10 @@ static void tpkbd_led_brightness_set(struct led_classdev *led_cdev,
>>  	hid_hw_request(hdev, report, HID_REQ_SET_REPORT);
>>  }
>>
>> -static int tpkbd_probe_tp(struct hid_device *hdev)
>> +static int lenovo_probe_tpkbd(struct hid_device *hdev)
>>  {
>>  	struct device *dev = &hdev->dev;
>> -	struct tpkbd_data_pointer *data_pointer;
>> +	struct lenovo_drvdata_tpkbd *data_pointer;
>>  	size_t name_sz = strlen(dev_name(dev)) + 16;
>>  	char *name_mute, *name_micmute;
>>  	int i;
>> @@ -347,12 +346,12 @@ static int tpkbd_probe_tp(struct hid_device *hdev)
>>  		return -ENODEV;
>>
>>  	if (sysfs_create_group(&hdev->dev.kobj,
>> -				&tpkbd_attr_group_pointer)) {
>> +				&lenovo_attr_group_tpkbd)) {
>>  		hid_warn(hdev, "Could not create sysfs group\n");
>>  	}
>>
>>  	data_pointer = devm_kzalloc(&hdev->dev,
>> -				    sizeof(struct tpkbd_data_pointer),
>> +				    sizeof(struct lenovo_drvdata_tpkbd),
>>  				    GFP_KERNEL);
>>  	if (data_pointer == NULL) {
>>  		hid_err(hdev, "Could not allocate memory for driver data\n");
>> @@ -375,23 +374,25 @@ static int tpkbd_probe_tp(struct hid_device *hdev)
>>  	hid_set_drvdata(hdev, data_pointer);
>>
>>  	data_pointer->led_mute.name = name_mute;
>> -	data_pointer->led_mute.brightness_get = tpkbd_led_brightness_get;
>> -	data_pointer->led_mute.brightness_set = tpkbd_led_brightness_set;
>> +	data_pointer->led_mute.brightness_get = lenovo_led_brightness_get_tpkbd;
>> +	data_pointer->led_mute.brightness_set = lenovo_led_brightness_set_tpkbd;
>>  	data_pointer->led_mute.dev = dev;
>>  	led_classdev_register(dev, &data_pointer->led_mute);
>>
>>  	data_pointer->led_micmute.name = name_micmute;
>> -	data_pointer->led_micmute.brightness_get = tpkbd_led_brightness_get;
>> -	data_pointer->led_micmute.brightness_set = tpkbd_led_brightness_set;
>> +	data_pointer->led_micmute.brightness_get =
>> +		lenovo_led_brightness_get_tpkbd;
>> +	data_pointer->led_micmute.brightness_set =
>> +		lenovo_led_brightness_set_tpkbd;
>>  	data_pointer->led_micmute.dev = dev;
>>  	led_classdev_register(dev, &data_pointer->led_micmute);
>>
>> -	tpkbd_features_set(hdev);
>> +	lenovo_features_set_tpkbd(hdev);
>>
>>  	return 0;
>>  }
>>
>> -static int tpkbd_probe(struct hid_device *hdev,
>> +static int lenovo_probe(struct hid_device *hdev,
>>  		const struct hid_device_id *id)
>>  {
>>  	int ret;
>> @@ -410,7 +411,7 @@ static int tpkbd_probe(struct hid_device *hdev,
>>
>>  	if (hid_get_drvdata(hdev)) {
>>  		hid_set_drvdata(hdev, NULL);
>> -		ret = tpkbd_probe_tp(hdev);
>> +		ret = lenovo_probe_tpkbd(hdev);
>>  		if (ret)
>>  			goto err_hid;
>>  	}
>> @@ -422,12 +423,12 @@ err:
>>  	return ret;
>>  }
>>
>> -static void tpkbd_remove_tp(struct hid_device *hdev)
>> +static void lenovo_remove_tpkbd(struct hid_device *hdev)
>>  {
>> -	struct tpkbd_data_pointer *data_pointer = hid_get_drvdata(hdev);
>> +	struct lenovo_drvdata_tpkbd *data_pointer = hid_get_drvdata(hdev);
>>
>>  	sysfs_remove_group(&hdev->dev.kobj,
>> -			&tpkbd_attr_group_pointer);
>> +			&lenovo_attr_group_tpkbd);
>>
>>  	led_classdev_unregister(&data_pointer->led_micmute);
>>  	led_classdev_unregister(&data_pointer->led_mute);
>> @@ -435,28 +436,28 @@ static void tpkbd_remove_tp(struct hid_device *hdev)
>>  	hid_set_drvdata(hdev, NULL);
>>  }
>>
>> -static void tpkbd_remove(struct hid_device *hdev)
>> +static void lenovo_remove(struct hid_device *hdev)
>>  {
>>  	if (hid_get_drvdata(hdev))
>> -		tpkbd_remove_tp(hdev);
>> +		lenovo_remove_tpkbd(hdev);
>>
>>  	hid_hw_stop(hdev);
>>  }
>>
>> -static const struct hid_device_id tpkbd_devices[] = {
>> +static const struct hid_device_id lenovo_devices[] = {
>>  	{ HID_USB_DEVICE(USB_VENDOR_ID_LENOVO, USB_DEVICE_ID_LENOVO_TPKBD) },
>>  	{ }
>>  };
>>
>> -MODULE_DEVICE_TABLE(hid, tpkbd_devices);
>> +MODULE_DEVICE_TABLE(hid, lenovo_devices);
>>
>> -static struct hid_driver tpkbd_driver = {
>> -	.name = "lenovo_tpkbd",
>> -	.id_table = tpkbd_devices,
>> -	.input_mapping = tpkbd_input_mapping,
>> -	.probe = tpkbd_probe,
>> -	.remove = tpkbd_remove,
>> +static struct hid_driver lenovo_driver = {
>> +	.name = "lenovo",
>> +	.id_table = lenovo_devices,
>> +	.input_mapping = lenovo_input_mapping_tpkbd,
>> +	.probe = lenovo_probe,
>> +	.remove = lenovo_remove,
>>  };
>> -module_hid_driver(tpkbd_driver);
>> +module_hid_driver(lenovo_driver);
>>
>>  MODULE_LICENSE("GPL");
>> --
>> 2.0.0
>>
>
>
>

-- 
Jamie Lentin

Re: [PATCH v4 1/4] Rename hid-lenovo-tpkbd to hid-lenovo

[Antonio Ospite]

On Fri, 18 Jul 2014 10:35:35 +0100 (BST)
Jamie Lentin <jm@lentin.co.uk> wrote:

> On Mon, 14 Jul 2014, Antonio Ospite wrote:
> 
> > On Sun, 13 Jul 2014 08:24:19 +0100
> > Jamie Lentin <jm@lentin.co.uk> wrote:
> >
> >> Rename module and all functions within so we can add support for other
> >> keyboards in the same file. Rename the _tp postfix to _tpkbd, to
> >> signify functions relevant to the TP USB keyboard.
> >>
> >> Signed-off-by: Jamie Lentin <jm@lentin.co.uk>
> >
> > Reviewed-by: Antonio Ospite <ao2@ao2.it>
> >
> > Patch looks good to me, easier to validate now :)
> >
> > A few ideas below for some possible _future_ cleanups, but no reason to
> > hold this one any longer IMHO.
> >
> > Ciao,
> >   Antonio
> >
> >> ---
> >>  ...er-hid-lenovo-tpkbd => sysfs-driver-hid-lenovo} |   0
> >>  drivers/hid/Kconfig                                |  14 +-
> >>  drivers/hid/Makefile                               |   2 +-
> >>  drivers/hid/hid-core.c                             |   2 +-
> >>  drivers/hid/{hid-lenovo-tpkbd.c => hid-lenovo.c}   | 185 +++++++++++----------
> >>  5 files changed, 102 insertions(+), 101 deletions(-)
> >>  rename Documentation/ABI/testing/{sysfs-driver-hid-lenovo-tpkbd => sysfs-driver-hid-lenovo} (100%)
> >>  rename drivers/hid/{hid-lenovo-tpkbd.c => hid-lenovo.c} (64%)
> >>

[...]
> >> -static int tpkbd_features_set(struct hid_device *hdev)
> >> +static int lenovo_features_set_tpkbd(struct hid_device *hdev)
> >>  {
> >>  	struct hid_report *report;
> >> -	struct tpkbd_data_pointer *data_pointer = hid_get_drvdata(hdev);
> >> +	struct lenovo_drvdata_tpkbd *data_pointer = hid_get_drvdata(hdev);
> >
> >
> > I think the word "pointer" here is misleading, sometimes in other parts
> > of the driver it is used as a synonym of trackpoint, but
> > lenovo_drvdata_tpkbd also contains data about leds, so here it looks
> > like it refers to C pointers. I'd call the variable tpkbd_data or
> > tpkbd_drvdata, throughout the driver. What do you think?
> 
> Yes, the rationale behind the name really isn't obvious but all meanings 
> are sorta correct, so it's not that bad :P
> 
> I like tpkbd_drvdata better though, and can correct it in the compact 
> keyboard code too. I wanted to make the variable names match, but didn't 
> get around to it, so will do it next time around.
>

IMHO you can very well use data_pointer for now so the names are
consistent with the current ones; a cleanup can be done later to make
the naming more straightforward too.

> > And the driver could standardize on the word "trackpoint" where
> > appropriate in order to eliminate any ambiguity.
> 
> The key other place I can think of that this happens is for the USB 
> compact keyboard, for it's HID_TYPE_USBMOUSE half (versus the keyboard 
> half). Saying trackpoint there is a mis-truth, since it's also where all 
> the extra keys hide. But I'll have a look through anyway.
> 

I meant: where "pointer" means trackpoint let's just use "trackpoint",
where pointer means "not keyboard" (like in data_pointer apparently)
something else can be used if "trackpoint" is too specific.

But again, this is cleanup: it'd be nice to have it eventually but not
worth blocking this patchset.

Thanks,
   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 v4 2/4] Make all base functions switch depending on product ID

[Jamie Lentin]

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

diff --git a/drivers/hid/hid-lenovo.c b/drivers/hid/hid-lenovo.c
index 0320b96..d11e337 100644
--- a/drivers/hid/hid-lenovo.c
+++ b/drivers/hid/hid-lenovo.c
@@ -1,5 +1,6 @@
 /*
- *  HID driver for Lenovo ThinkPad USB Keyboard with TrackPoint
+ *  HID driver for Lenovo:
+ *  - ThinkPad USB Keyboard with TrackPoint (tpkbd)
  *
  *  Copyright (c) 2012 Bernhard Seibold
  */
@@ -47,6 +48,19 @@ static int lenovo_input_mapping_tpkbd(struct hid_device *hdev,
 	return 0;
 }
 
+static int lenovo_input_mapping(struct hid_device *hdev,
+		struct hid_input *hi, struct hid_field *field,
+		struct hid_usage *usage, unsigned long **bit, int *max)
+{
+	switch (hdev->product) {
+	case USB_DEVICE_ID_LENOVO_TPKBD:
+		return lenovo_input_mapping_tpkbd(hdev, hi, field,
+							usage, bit, max);
+	}
+
+	return 0;
+}
+
 #undef map_key_clear
 
 static int lenovo_features_set_tpkbd(struct hid_device *hdev)
@@ -337,6 +351,16 @@ static int lenovo_probe_tpkbd(struct hid_device *hdev)
 	char *name_mute, *name_micmute;
 	int i;
 
+	/*
+	 * If this is the pointer half of the keyboard, input_mapping should
+	 * have set drvdata to 1. Otherwise, it's the keyboard which needs
+	 * nothing special doing to it.
+	 */
+	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))
@@ -409,12 +433,13 @@ static int lenovo_probe(struct hid_device *hdev,
 		goto err;
 	}
 
-	if (hid_get_drvdata(hdev)) {
-		hid_set_drvdata(hdev, NULL);
+	switch (hdev->product) {
+	case USB_DEVICE_ID_LENOVO_TPKBD:
 		ret = lenovo_probe_tpkbd(hdev);
-		if (ret)
-			goto err_hid;
+		break;
 	}
+	if (ret)
+		goto err_hid;
 
 	return 0;
 err_hid:
@@ -430,6 +455,9 @@ static void lenovo_remove_tpkbd(struct hid_device *hdev)
 	sysfs_remove_group(&hdev->dev.kobj,
 			&lenovo_attr_group_tpkbd);
 
+	if (data_pointer == NULL)
+		return;
+
 	led_classdev_unregister(&data_pointer->led_micmute);
 	led_classdev_unregister(&data_pointer->led_mute);
 
@@ -438,8 +466,11 @@ static void lenovo_remove_tpkbd(struct hid_device *hdev)
 
 static void lenovo_remove(struct hid_device *hdev)
 {
-	if (hid_get_drvdata(hdev))
+	switch (hdev->product) {
+	case USB_DEVICE_ID_LENOVO_TPKBD:
 		lenovo_remove_tpkbd(hdev);
+		break;
+	}
 
 	hid_hw_stop(hdev);
 }
@@ -454,7 +485,7 @@ MODULE_DEVICE_TABLE(hid, lenovo_devices);
 static struct hid_driver lenovo_driver = {
 	.name = "lenovo",
 	.id_table = lenovo_devices,
-	.input_mapping = lenovo_input_mapping_tpkbd,
+	.input_mapping = lenovo_input_mapping,
 	.probe = lenovo_probe,
 	.remove = lenovo_remove,
 };
-- 
2.0.0

Re: [PATCH v4 2/4] Make all base functions switch depending on product ID

[Antonio Ospite]

On Sun, 13 Jul 2014 08:24:20 +0100
Jamie Lentin <jm@lentin.co.uk> wrote:

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

The commit message could be improved, it's not obvious to me what "base
functions" means here, I am thinking to something like the following:

---------------------------------------------------------------------
HID: lenovo: prepare support for adding other devices

Add a common lenovo_input_mapping(), and call device specific functions
conditionally in order to ease the task of adding support for other
devices.
---------------------------------------------------------------------

This way you explain the WHAT and WHY and point that this is a
_preparatory_ patch, the HOW is explained by the code anyway.

> ---
>  drivers/hid/hid-lenovo.c | 45 ++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 38 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/hid/hid-lenovo.c b/drivers/hid/hid-lenovo.c
> index 0320b96..d11e337 100644
> --- a/drivers/hid/hid-lenovo.c
> +++ b/drivers/hid/hid-lenovo.c
> @@ -1,5 +1,6 @@
>  /*
> - *  HID driver for Lenovo ThinkPad USB Keyboard with TrackPoint
> + *  HID driver for Lenovo:
> + *  - ThinkPad USB Keyboard with TrackPoint (tpkbd)
>   *
>   *  Copyright (c) 2012 Bernhard Seibold
>   */
> @@ -47,6 +48,19 @@ static int lenovo_input_mapping_tpkbd(struct hid_device *hdev,
>  	return 0;
>  }
>  
> +static int lenovo_input_mapping(struct hid_device *hdev,
> +		struct hid_input *hi, struct hid_field *field,
> +		struct hid_usage *usage, unsigned long **bit, int *max)
> +{
> +	switch (hdev->product) {
> +	case USB_DEVICE_ID_LENOVO_TPKBD:
> +		return lenovo_input_mapping_tpkbd(hdev, hi, field,
> +							usage, bit, max);
> +	}
> +
> +	return 0;
> +}
> +
>  #undef map_key_clear
>  
>  static int lenovo_features_set_tpkbd(struct hid_device *hdev)
> @@ -337,6 +351,16 @@ static int lenovo_probe_tpkbd(struct hid_device *hdev)
>  	char *name_mute, *name_micmute;
>  	int i;
>  
> +	/*
> +	 * If this is the pointer half of the keyboard, input_mapping should

s/pointer/trackpoint/ ? ;)

> +	 * have set drvdata to 1. Otherwise, it's the keyboard which needs
> +	 * nothing special doing to it.
> +	 */
> +	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))
> @@ -409,12 +433,13 @@ static int lenovo_probe(struct hid_device *hdev,
>  		goto err;
>  	}
>  
> -	if (hid_get_drvdata(hdev)) {
> -		hid_set_drvdata(hdev, NULL);
> +	switch (hdev->product) {
> +	case USB_DEVICE_ID_LENOVO_TPKBD:
>  		ret = lenovo_probe_tpkbd(hdev);
> -		if (ret)
> -			goto err_hid;
> +		break;
>  	}
> +	if (ret)
> +		goto err_hid;
>  

It is not immediately clear what this "ret" contains; the code seems to
be correct because if "ret" contains the previous return value of
hid_hw_start() it's from a successful invocation, but it took me some
reasoning to figure it out.

I'd leave the check right after the call to lenovo_probe_tpkbd(), and
repeat it when you add the other case it's slightly more code but it's
way more readable, it does not force you to remember the past or reason
on the switch logic when you read the code.

Anyhow, if you really want to have only one check after the switch, what
about adding a default case to the former switch:

	default:
		ret = 0;
		break;

Having always the default case is also a good practice IMVHO, consider
adding it to the other switch statements too.

>  	return 0;
>  err_hid:
> @@ -430,6 +455,9 @@ static void lenovo_remove_tpkbd(struct hid_device *hdev)
>  	sysfs_remove_group(&hdev->dev.kobj,
>  			&lenovo_attr_group_tpkbd);
>  
> +	if (data_pointer == NULL)
> +		return;
> +

This is OK here, as sysfs_remove_group() is safe to call
unconditionally (it will give warnings where there are no groups
AFAICS), but it could also go at the very start of lenovo_remove_tpkbd
(). It would be more symmetrical because you create the sysfs entries
only for the trackpoint half in the probe function. Maybe add a comment
like the useful one in the probe function.

>  	led_classdev_unregister(&data_pointer->led_micmute);
>  	led_classdev_unregister(&data_pointer->led_mute);
>  
> @@ -438,8 +466,11 @@ static void lenovo_remove_tpkbd(struct hid_device *hdev)
>  
>  static void lenovo_remove(struct hid_device *hdev)
>  {
> -	if (hid_get_drvdata(hdev))
> +	switch (hdev->product) {
> +	case USB_DEVICE_ID_LENOVO_TPKBD:
>  		lenovo_remove_tpkbd(hdev);
> +		break;
> +	}
>  
>  	hid_hw_stop(hdev);
>  }
> @@ -454,7 +485,7 @@ MODULE_DEVICE_TABLE(hid, lenovo_devices);
>  static struct hid_driver lenovo_driver = {
>  	.name = "lenovo",
>  	.id_table = lenovo_devices,
> -	.input_mapping = lenovo_input_mapping_tpkbd,
> +	.input_mapping = lenovo_input_mapping,
>  	.probe = lenovo_probe,
>  	.remove = lenovo_remove,
>  };
> -- 
> 2.0.0
> 
> 


-- 
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 v4 3/4] Style fixes

[Jamie Lentin]

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

diff --git a/drivers/hid/hid-lenovo.c b/drivers/hid/hid-lenovo.c
index d11e337..a1a693c 100644
--- a/drivers/hid/hid-lenovo.c
+++ b/drivers/hid/hid-lenovo.c
@@ -349,7 +349,7 @@ static int lenovo_probe_tpkbd(struct hid_device *hdev)
 	struct lenovo_drvdata_tpkbd *data_pointer;
 	size_t name_sz = strlen(dev_name(dev)) + 16;
 	char *name_mute, *name_micmute;
-	int i;
+	int i, ret;
 
 	/*
 	 * If this is the pointer half of the keyboard, input_mapping should
@@ -369,10 +369,9 @@ static int lenovo_probe_tpkbd(struct hid_device *hdev)
 	if (!hid_validate_values(hdev, HID_OUTPUT_REPORT, 3, 0, 2))
 		return -ENODEV;
 
-	if (sysfs_create_group(&hdev->dev.kobj,
-				&lenovo_attr_group_tpkbd)) {
+	ret = sysfs_create_group(&hdev->dev.kobj, &lenovo_attr_group_tpkbd);
+	if (ret)
 		hid_warn(hdev, "Could not create sysfs group\n");
-	}
 
 	data_pointer = devm_kzalloc(&hdev->dev,
 				    sizeof(struct lenovo_drvdata_tpkbd),
-- 
2.0.0

Re: [PATCH v4 3/4] Style fixes

[Antonio Ospite]

On Sun, 13 Jul 2014 08:24:21 +0100
Jamie Lentin <jm@lentin.co.uk> wrote:

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

Reviewed-by: Antonio Ospite <ao2@ao2.it>

> ---
>  drivers/hid/hid-lenovo.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/hid/hid-lenovo.c b/drivers/hid/hid-lenovo.c
> index d11e337..a1a693c 100644
> --- a/drivers/hid/hid-lenovo.c
> +++ b/drivers/hid/hid-lenovo.c
> @@ -349,7 +349,7 @@ static int lenovo_probe_tpkbd(struct hid_device *hdev)
>  	struct lenovo_drvdata_tpkbd *data_pointer;
>  	size_t name_sz = strlen(dev_name(dev)) + 16;
>  	char *name_mute, *name_micmute;
> -	int i;
> +	int i, ret;

Preferred style is one declaration per line, but this style is also
quite common in kernel, so it's not a big deal.

>
>  	/*
>  	 * If this is the pointer half of the keyboard, input_mapping should
> @@ -369,10 +369,9 @@ static int lenovo_probe_tpkbd(struct hid_device *hdev)
>  	if (!hid_validate_values(hdev, HID_OUTPUT_REPORT, 3, 0, 2))
>  		return -ENODEV;
>  
> -	if (sysfs_create_group(&hdev->dev.kobj,
> -				&lenovo_attr_group_tpkbd)) {
> +	ret = sysfs_create_group(&hdev->dev.kobj, &lenovo_attr_group_tpkbd);
> +	if (ret)
>  		hid_warn(hdev, "Could not create sysfs group\n");
> -	}
>  
>  	data_pointer = devm_kzalloc(&hdev->dev,
>  				    sizeof(struct lenovo_drvdata_tpkbd),
> -- 
> 2.0.0
> 


-- 
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 v4 4/4] Add support for Compact (Bluetooth|USB) keyboard with Trackpoint

[Jamie Lentin]

Signed-off-by: Jamie Lentin <jm@lentin.co.uk>
---
 Documentation/ABI/testing/sysfs-driver-hid-lenovo |  12 ++
 drivers/hid/Kconfig                               |   2 +
 drivers/hid/hid-core.c                            |   2 +
 drivers/hid/hid-ids.h                             |   2 +
 drivers/hid/hid-lenovo.c                          | 203 ++++++++++++++++++++++
 include/linux/hid.h                               |   1 +
 6 files changed, 222 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-driver-hid-lenovo b/Documentation/ABI/testing/sysfs-driver-hid-lenovo
index 57b92cb..53a0725 100644
--- a/Documentation/ABI/testing/sysfs-driver-hid-lenovo
+++ b/Documentation/ABI/testing/sysfs-driver-hid-lenovo
@@ -4,18 +4,21 @@ Contact:	linux-input@vger.kernel.org
 Description:	This controls if mouse clicks should be generated if the trackpoint is quickly pressed. How fast this press has to be
 		is being controlled by press_speed.
 		Values are 0 or 1.
+		Applies to Thinkpad USB Keyboard with TrackPoint.
 
 What:		/sys/bus/usb/devices/<busnum>-<devnum>:<config num>.<interface num>/<hid-bus>:<vendor-id>:<product-id>.<num>/dragging
 Date:		July 2011
 Contact:	linux-input@vger.kernel.org
 Description:	If this setting is enabled, it is possible to do dragging by pressing the trackpoint. This requires press_to_select to be enabled.
 		Values are 0 or 1.
+		Applies to Thinkpad USB Keyboard with TrackPoint.
 
 What:		/sys/bus/usb/devices/<busnum>-<devnum>:<config num>.<interface num>/<hid-bus>:<vendor-id>:<product-id>.<num>/release_to_select
 Date:		July 2011
 Contact:	linux-input@vger.kernel.org
 Description:	For details regarding this setting please refer to http://www.pc.ibm.com/ww/healthycomputing/trkpntb.html
 		Values are 0 or 1.
+		Applies to Thinkpad USB Keyboard with TrackPoint.
 
 What:		/sys/bus/usb/devices/<busnum>-<devnum>:<config num>.<interface num>/<hid-bus>:<vendor-id>:<product-id>.<num>/select_right
 Date:		July 2011
@@ -23,16 +26,25 @@ Contact:	linux-input@vger.kernel.org
 Description:	This setting controls if the mouse click events generated by pressing the trackpoint (if press_to_select is enabled) generate
 		a left or right mouse button click.
 		Values are 0 or 1.
+		Applies to Thinkpad USB Keyboard with TrackPoint.
 
 What:		/sys/bus/usb/devices/<busnum>-<devnum>:<config num>.<interface num>/<hid-bus>:<vendor-id>:<product-id>.<num>/sensitivity
 Date:		July 2011
 Contact:	linux-input@vger.kernel.org
 Description:	This file contains the trackpoint sensitivity.
 		Values are decimal integers from 1 (lowest sensitivity) to 255 (highest sensitivity).
+		Applies to Thinkpad USB Keyboard with TrackPoint.
 
 What:		/sys/bus/usb/devices/<busnum>-<devnum>:<config num>.<interface num>/<hid-bus>:<vendor-id>:<product-id>.<num>/press_speed
 Date:		July 2011
 Contact:	linux-input@vger.kernel.org
 Description:	This setting controls how fast the trackpoint needs to be pressed to generate a mouse click if press_to_select is enabled.
 		Values are decimal integers from 1 (slowest) to 255 (fastest).
+		Applies to Thinkpad USB Keyboard with TrackPoint.
 
+What:		/sys/bus/usb/devices/<busnum>-<devnum>:<config num>.<interface num>/<hid-bus>:<vendor-id>:<product-id>.<num>/fn_lock
+Date:		July 2014
+Contact:	linux-input@vger.kernel.org
+Description:	This setting controls whether Fn Lock is enabled on the keyboard (i.e. if F1 is Mute or F1)
+		Values are 0 or 1
+		Applies to ThinkPad Compact (USB|Bluetooth) Keyboard with TrackPoint.
diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 9b7acfc..1e19292 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -343,6 +343,8 @@ config HID_LENOVO
 	Thinkpad standalone keyboards, e.g:
 	- ThinkPad USB Keyboard with TrackPoint (supports extra LEDs and trackpoint
 	  configuration)
+	- ThinkPad Compact Bluetooth Keyboard with TrackPoint (supports Fn keys)
+	- ThinkPad Compact USB Keyboard with TrackPoint (supports Fn keys)
 
 config HID_LOGITECH
 	tristate "Logitech devices" if EXPERT
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 55841bd..81b3bb6 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1798,6 +1798,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)
 	{ 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 6d00bb9..d2e2a96 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -560,6 +560,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.c b/drivers/hid/hid-lenovo.c
index a1a693c..46c5db0 100644
--- a/drivers/hid/hid-lenovo.c
+++ b/drivers/hid/hid-lenovo.c
@@ -1,8 +1,11 @@
 /*
  *  HID driver for Lenovo:
  *  - ThinkPad USB Keyboard with TrackPoint (tpkbd)
+ *  - ThinkPad Compact Bluetooth Keyboard with TrackPoint (cptkbd)
+ *  - ThinkPad Compact USB Keyboard with TrackPoint (cptkbd)
  *
  *  Copyright (c) 2012 Bernhard Seibold
+ *  Copyright (c) 2014 Jamie Lentin <jm@lentin.co.uk>
  */
 
 /*
@@ -33,6 +36,10 @@ struct lenovo_drvdata_tpkbd {
 	int press_speed;
 };
 
+struct lenovo_drvdata_cptkbd {
+	bool fn_lock;
+};
+
 #define map_key_clear(c) hid_map_usage_clear(hi, usage, bit, max, EV_KEY, (c))
 
 static int lenovo_input_mapping_tpkbd(struct hid_device *hdev,
@@ -48,6 +55,49 @@ static int lenovo_input_mapping_tpkbd(struct hid_device *hdev,
 	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)
+{
+	/* 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_SCALE);
+			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 lenovo_input_mapping(struct hid_device *hdev,
 		struct hid_input *hi, struct hid_field *field,
 		struct hid_usage *usage, unsigned long **bit, int *max)
@@ -56,6 +106,10 @@ static int lenovo_input_mapping(struct hid_device *hdev,
 	case USB_DEVICE_ID_LENOVO_TPKBD:
 		return lenovo_input_mapping_tpkbd(hdev, hi, field,
 							usage, bit, max);
+	case USB_DEVICE_ID_LENOVO_CUSBKBD:
+	case USB_DEVICE_ID_LENOVO_CBTKBD:
+		return lenovo_input_mapping_cptkbd(hdev, hi, field,
+							usage, bit, max);
 	}
 
 	return 0;
@@ -63,6 +117,100 @@ static int lenovo_input_mapping(struct hid_device *hdev,
 
 #undef map_key_clear
 
+/* Send a config command to the keyboard */
+static int lenovo_send_cmd_cptkbd(struct hid_device *hdev,
+			unsigned char byte2, unsigned char byte3)
+{
+	int ret;
+	unsigned char buf[] = {0x18, byte2, byte3};
+
+	switch (hdev->product) {
+	case USB_DEVICE_ID_LENOVO_CUSBKBD:
+		ret = hid_hw_raw_request(hdev, 0x13, buf, sizeof(buf),
+					HID_FEATURE_REPORT, HID_REQ_SET_REPORT);
+		break;
+	case USB_DEVICE_ID_LENOVO_CBTKBD:
+		ret = hid_hw_output_report(hdev, buf, sizeof(buf));
+		break;
+	default:
+		ret = -EINVAL;
+	}
+
+	return ret < 0 ? ret : 0; /* BT returns 0, USB returns sizeof(buf) */
+}
+
+static void lenovo_features_set_cptkbd(struct hid_device *hdev)
+{
+	struct lenovo_drvdata_cptkbd *tpcsc = hid_get_drvdata(hdev);
+
+	if (lenovo_send_cmd_cptkbd(hdev, 0x05, tpcsc->fn_lock ? 0x01 : 0x00))
+		hid_err(hdev, "Fn-lock setting failed\n");
+}
+
+static ssize_t attr_fn_lock_show_cptkbd(struct device *dev,
+		struct device_attribute *attr,
+		char *buf)
+{
+	struct hid_device *hdev = container_of(dev, struct hid_device, dev);
+	struct lenovo_drvdata_cptkbd *tpcsc = hid_get_drvdata(hdev);
+
+	return snprintf(buf, PAGE_SIZE, "%u\n", tpcsc->fn_lock);
+}
+
+static ssize_t attr_fn_lock_store_cptkbd(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 lenovo_drvdata_cptkbd *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;
+	lenovo_features_set_cptkbd(hdev);
+
+	return count;
+}
+
+static struct device_attribute dev_attr_fn_lock_cptkbd =
+	__ATTR(fn_lock, S_IWUSR | S_IRUGO,
+			attr_fn_lock_show_cptkbd,
+			attr_fn_lock_store_cptkbd);
+
+static struct attribute *lenovo_attributes_cptkbd[] = {
+	&dev_attr_fn_lock_cptkbd.attr,
+	NULL
+};
+
+static const struct attribute_group lenovo_attr_group_cptkbd = {
+	.attrs = lenovo_attributes_cptkbd,
+};
+
+static int lenovo_raw_event(struct hid_device *hdev,
+			struct hid_report *report, u8 *data, int size)
+{
+	/*
+	 * Compact USB keyboard's Fn-F12 report holds down many other keys, and
+	 * its 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 lenovo_features_set_tpkbd(struct hid_device *hdev)
 {
 	struct hid_report *report;
@@ -415,6 +563,44 @@ static int lenovo_probe_tpkbd(struct hid_device *hdev)
 	return 0;
 }
 
+static int lenovo_probe_cptkbd(struct hid_device *hdev)
+{
+	int ret;
+	struct lenovo_drvdata_cptkbd *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) {
+		hid_dbg(hdev, "Ignoring keyboard half of device\n");
+		return 0;
+	}
+
+	/*
+	 * Tell the keyboard a driver understands it, and turn F7, F9, F11 into
+	 * regular keys
+	 */
+	ret = lenovo_send_cmd_cptkbd(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;
+	lenovo_features_set_cptkbd(hdev);
+
+	if (sysfs_create_group(&hdev->dev.kobj,
+				&lenovo_attr_group_cptkbd))
+		hid_warn(hdev, "Could not create sysfs group\n");
+
+	return 0;
+}
+
 static int lenovo_probe(struct hid_device *hdev,
 		const struct hid_device_id *id)
 {
@@ -436,6 +622,10 @@ static int lenovo_probe(struct hid_device *hdev,
 	case USB_DEVICE_ID_LENOVO_TPKBD:
 		ret = lenovo_probe_tpkbd(hdev);
 		break;
+	case USB_DEVICE_ID_LENOVO_CUSBKBD:
+	case USB_DEVICE_ID_LENOVO_CBTKBD:
+		ret = lenovo_probe_cptkbd(hdev);
+		break;
 	}
 	if (ret)
 		goto err_hid;
@@ -463,12 +653,22 @@ static void lenovo_remove_tpkbd(struct hid_device *hdev)
 	hid_set_drvdata(hdev, NULL);
 }
 
+static void lenovo_remove_cptkbd(struct hid_device *hdev)
+{
+	sysfs_remove_group(&hdev->dev.kobj,
+			&lenovo_attr_group_cptkbd);
+}
+
 static void lenovo_remove(struct hid_device *hdev)
 {
 	switch (hdev->product) {
 	case USB_DEVICE_ID_LENOVO_TPKBD:
 		lenovo_remove_tpkbd(hdev);
 		break;
+	case USB_DEVICE_ID_LENOVO_CUSBKBD:
+	case USB_DEVICE_ID_LENOVO_CBTKBD:
+		lenovo_remove_cptkbd(hdev);
+		break;
 	}
 
 	hid_hw_stop(hdev);
@@ -476,6 +676,8 @@ static void lenovo_remove(struct hid_device *hdev)
 
 static const struct hid_device_id lenovo_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) },
 	{ }
 };
 
@@ -487,6 +689,7 @@ static struct hid_driver lenovo_driver = {
 	.input_mapping = lenovo_input_mapping,
 	.probe = lenovo_probe,
 	.remove = lenovo_remove,
+	.raw_event = lenovo_raw_event,
 };
 module_hid_driver(lenovo_driver);
 
diff --git a/include/linux/hid.h b/include/linux/hid.h
index 77632cf..fca74f1 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

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

[Antonio Ospite]

On Sun, 13 Jul 2014 08:24:22 +0100
Jamie Lentin <jm@lentin.co.uk> wrote:

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

Almost there, I swear :)

> ---
>  Documentation/ABI/testing/sysfs-driver-hid-lenovo |  12 ++
>  drivers/hid/Kconfig                               |   2 +
>  drivers/hid/hid-core.c                            |   2 +
>  drivers/hid/hid-ids.h                             |   2 +
>  drivers/hid/hid-lenovo.c                          | 203 ++++++++++++++++++++++
>  include/linux/hid.h                               |   1 +
>  6 files changed, 222 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-driver-hid-lenovo b/Documentation/ABI/testing/sysfs-driver-hid-lenovo
> index 57b92cb..53a0725 100644
> --- a/Documentation/ABI/testing/sysfs-driver-hid-lenovo
> +++ b/Documentation/ABI/testing/sysfs-driver-hid-lenovo
> @@ -4,18 +4,21 @@ Contact:	linux-input@vger.kernel.org
>  Description:	This controls if mouse clicks should be generated if the trackpoint is quickly pressed. How fast this press has to be
>  		is being controlled by press_speed.
>  		Values are 0 or 1.
> +		Applies to Thinkpad USB Keyboard with TrackPoint.
>  
>  What:		/sys/bus/usb/devices/<busnum>-<devnum>:<config num>.<interface num>/<hid-bus>:<vendor-id>:<product-id>.<num>/dragging
>  Date:		July 2011
>  Contact:	linux-input@vger.kernel.org
>  Description:	If this setting is enabled, it is possible to do dragging by pressing the trackpoint. This requires press_to_select to be enabled.
>  		Values are 0 or 1.
> +		Applies to Thinkpad USB Keyboard with TrackPoint.
>  
>  What:		/sys/bus/usb/devices/<busnum>-<devnum>:<config num>.<interface num>/<hid-bus>:<vendor-id>:<product-id>.<num>/release_to_select
>  Date:		July 2011
>  Contact:	linux-input@vger.kernel.org
>  Description:	For details regarding this setting please refer to http://www.pc.ibm.com/ww/healthycomputing/trkpntb.html
>  		Values are 0 or 1.
> +		Applies to Thinkpad USB Keyboard with TrackPoint.
>  
>  What:		/sys/bus/usb/devices/<busnum>-<devnum>:<config num>.<interface num>/<hid-bus>:<vendor-id>:<product-id>.<num>/select_right
>  Date:		July 2011
> @@ -23,16 +26,25 @@ Contact:	linux-input@vger.kernel.org
>  Description:	This setting controls if the mouse click events generated by pressing the trackpoint (if press_to_select is enabled) generate
>  		a left or right mouse button click.
>  		Values are 0 or 1.
> +		Applies to Thinkpad USB Keyboard with TrackPoint.
>  
>  What:		/sys/bus/usb/devices/<busnum>-<devnum>:<config num>.<interface num>/<hid-bus>:<vendor-id>:<product-id>.<num>/sensitivity
>  Date:		July 2011
>  Contact:	linux-input@vger.kernel.org
>  Description:	This file contains the trackpoint sensitivity.
>  		Values are decimal integers from 1 (lowest sensitivity) to 255 (highest sensitivity).
> +		Applies to Thinkpad USB Keyboard with TrackPoint.
>  
>  What:		/sys/bus/usb/devices/<busnum>-<devnum>:<config num>.<interface num>/<hid-bus>:<vendor-id>:<product-id>.<num>/press_speed
>  Date:		July 2011
>  Contact:	linux-input@vger.kernel.org
>  Description:	This setting controls how fast the trackpoint needs to be pressed to generate a mouse click if press_to_select is enabled.
>  		Values are decimal integers from 1 (slowest) to 255 (fastest).
> +		Applies to Thinkpad USB Keyboard with TrackPoint.
>  
> +What:		/sys/bus/usb/devices/<busnum>-<devnum>:<config num>.<interface num>/<hid-bus>:<vendor-id>:<product-id>.<num>/fn_lock
> +Date:		July 2014
> +Contact:	linux-input@vger.kernel.org
> +Description:	This setting controls whether Fn Lock is enabled on the keyboard (i.e. if F1 is Mute or F1)
> +		Values are 0 or 1
> +		Applies to ThinkPad Compact (USB|Bluetooth) Keyboard with TrackPoint.
> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> index 9b7acfc..1e19292 100644
> --- a/drivers/hid/Kconfig
> +++ b/drivers/hid/Kconfig
> @@ -343,6 +343,8 @@ config HID_LENOVO
>  	Thinkpad standalone keyboards, e.g:
>  	- ThinkPad USB Keyboard with TrackPoint (supports extra LEDs and trackpoint
>  	  configuration)
> +	- ThinkPad Compact Bluetooth Keyboard with TrackPoint (supports Fn keys)
> +	- ThinkPad Compact USB Keyboard with TrackPoint (supports Fn keys)
>  
>  config HID_LOGITECH
>  	tristate "Logitech devices" if EXPERT
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index 55841bd..81b3bb6 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -1798,6 +1798,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)
>  	{ 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 6d00bb9..d2e2a96 100644
> --- a/drivers/hid/hid-ids.h
> +++ b/drivers/hid/hid-ids.h
> @@ -560,6 +560,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.c b/drivers/hid/hid-lenovo.c
> index a1a693c..46c5db0 100644
> --- a/drivers/hid/hid-lenovo.c
> +++ b/drivers/hid/hid-lenovo.c
> @@ -1,8 +1,11 @@
>  /*
>   *  HID driver for Lenovo:
>   *  - ThinkPad USB Keyboard with TrackPoint (tpkbd)
> + *  - ThinkPad Compact Bluetooth Keyboard with TrackPoint (cptkbd)
> + *  - ThinkPad Compact USB Keyboard with TrackPoint (cptkbd)
>   *
>   *  Copyright (c) 2012 Bernhard Seibold
> + *  Copyright (c) 2014 Jamie Lentin <jm@lentin.co.uk>
>   */
>  
>  /*
> @@ -33,6 +36,10 @@ struct lenovo_drvdata_tpkbd {
>  	int press_speed;
>  };
>  
> +struct lenovo_drvdata_cptkbd {
> +	bool fn_lock;
> +};
> +
>  #define map_key_clear(c) hid_map_usage_clear(hi, usage, bit, max, EV_KEY, (c))
>  
>  static int lenovo_input_mapping_tpkbd(struct hid_device *hdev,
> @@ -48,6 +55,49 @@ static int lenovo_input_mapping_tpkbd(struct hid_device *hdev,
>  	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)
> +{
> +	/* 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_SCALE);
> +			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 lenovo_input_mapping(struct hid_device *hdev,
>  		struct hid_input *hi, struct hid_field *field,
>  		struct hid_usage *usage, unsigned long **bit, int *max)
> @@ -56,6 +106,10 @@ static int lenovo_input_mapping(struct hid_device *hdev,
>  	case USB_DEVICE_ID_LENOVO_TPKBD:
>  		return lenovo_input_mapping_tpkbd(hdev, hi, field,
>  							usage, bit, max);
> +	case USB_DEVICE_ID_LENOVO_CUSBKBD:
> +	case USB_DEVICE_ID_LENOVO_CBTKBD:
> +		return lenovo_input_mapping_cptkbd(hdev, hi, field,
> +							usage, bit, max);
>  	}
>  
>  	return 0;
> @@ -63,6 +117,100 @@ static int lenovo_input_mapping(struct hid_device *hdev,
>  
>  #undef map_key_clear
>  
> +/* Send a config command to the keyboard */
> +static int lenovo_send_cmd_cptkbd(struct hid_device *hdev,
> +			unsigned char byte2, unsigned char byte3)
> +{
> +	int ret;
> +	unsigned char buf[] = {0x18, byte2, byte3};
> +
> +	switch (hdev->product) {
> +	case USB_DEVICE_ID_LENOVO_CUSBKBD:
> +		ret = hid_hw_raw_request(hdev, 0x13, buf, sizeof(buf),
> +					HID_FEATURE_REPORT, HID_REQ_SET_REPORT);
> +		break;
> +	case USB_DEVICE_ID_LENOVO_CBTKBD:
> +		ret = hid_hw_output_report(hdev, buf, sizeof(buf));
> +		break;
> +	default:
> +		ret = -EINVAL;
>

Add an explicit break? For regularity.

> +	}
> +
> +	return ret < 0 ? ret : 0; /* BT returns 0, USB returns sizeof(buf) */
> +}

Alternatively you could just return 'ret' and check for < 0 in the
callers, leaving the comment and adding "On Success ...".

BTW, you never actually propagate the negative error codes.

I you don't want this function to just return bool (e.g. return (ret >=
0)), at least print the returned error code in the messages after the
callers.

> +
> +static void lenovo_features_set_cptkbd(struct hid_device *hdev)
> +{
> +	struct lenovo_drvdata_cptkbd *tpcsc = hid_get_drvdata(hdev);
> +
> +	if (lenovo_send_cmd_cptkbd(hdev, 0x05, tpcsc->fn_lock ? 0x01 : 0x00))

Try to avoid function calls in conditions.

> +		hid_err(hdev, "Fn-lock setting failed\n");

Print 'ret' if lenovo_send_cmd_cptkbd() returns the negative error
codes.

> +}
> +
> +static ssize_t attr_fn_lock_show_cptkbd(struct device *dev,
> +		struct device_attribute *attr,
> +		char *buf)
> +{
> +	struct hid_device *hdev = container_of(dev, struct hid_device, dev);
> +	struct lenovo_drvdata_cptkbd *tpcsc = hid_get_drvdata(hdev);
> +
> +	return snprintf(buf, PAGE_SIZE, "%u\n", tpcsc->fn_lock);
> +}
> +
> +static ssize_t attr_fn_lock_store_cptkbd(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 lenovo_drvdata_cptkbd *tpcsc = hid_get_drvdata(hdev);
> +	int value;
> +
> +	if (kstrtoint(buf, 10, &value))

Function called in condition; however this is consistent with the other
invocation of kstrtoint() in the file, so you can leave it as it is as
an exception to the general rule.

> +		return -EINVAL;
> +	if (value < 0 || value > 1)
> +		return -EINVAL;
> +
> +	tpcsc->fn_lock = !!value;
> +	lenovo_features_set_cptkbd(hdev);
> +
> +	return count;
> +}
> +
> +static struct device_attribute dev_attr_fn_lock_cptkbd =
> +	__ATTR(fn_lock, S_IWUSR | S_IRUGO,
> +			attr_fn_lock_show_cptkbd,
> +			attr_fn_lock_store_cptkbd);
> +
> +static struct attribute *lenovo_attributes_cptkbd[] = {
> +	&dev_attr_fn_lock_cptkbd.attr,
> +	NULL
> +};
> +
> +static const struct attribute_group lenovo_attr_group_cptkbd = {
> +	.attrs = lenovo_attributes_cptkbd,
> +};
> +
> +static int lenovo_raw_event(struct hid_device *hdev,
> +			struct hid_report *report, u8 *data, int size)
> +{
> +	/*
> +	 * Compact USB keyboard's Fn-F12 report holds down many other keys, and
> +	 * its 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 lenovo_features_set_tpkbd(struct hid_device *hdev)
>  {
>  	struct hid_report *report;
> @@ -415,6 +563,44 @@ static int lenovo_probe_tpkbd(struct hid_device *hdev)
>  	return 0;
>  }
>  
> +static int lenovo_probe_cptkbd(struct hid_device *hdev)
> +{
> +	int ret;
> +	struct lenovo_drvdata_cptkbd *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) {
> +		hid_dbg(hdev, "Ignoring keyboard half of device\n");
> +		return 0;
> +	}

Maybe you can anticipate this check and avoid allocation in this case?
Not a big deal anyway.

> +
> +	/*
> +	 * Tell the keyboard a driver understands it, and turn F7, F9, F11 into
> +	 * regular keys
> +	 */
> +	ret = lenovo_send_cmd_cptkbd(hdev, 0x01, 0x03);
> +	if (ret)
> +		hid_warn(hdev, "Failed to switch F7/9/11 into regular keys\n");

Print 'ret' here.

> +
> +	/* Turn Fn-Lock on by default */
> +	tpcsc->fn_lock = 1;

tpcsc->fn_lock = true;

> +	lenovo_features_set_cptkbd(hdev);
> +
> +	if (sysfs_create_group(&hdev->dev.kobj,
> +				&lenovo_attr_group_cptkbd))

Function called in condition.

> +		hid_warn(hdev, "Could not create sysfs group\n");
> +
> +	return 0;
> +}
> +
>  static int lenovo_probe(struct hid_device *hdev,
>  		const struct hid_device_id *id)
>  {
> @@ -436,6 +622,10 @@ static int lenovo_probe(struct hid_device *hdev,
>  	case USB_DEVICE_ID_LENOVO_TPKBD:
>  		ret = lenovo_probe_tpkbd(hdev);
>  		break;
> +	case USB_DEVICE_ID_LENOVO_CUSBKBD:
> +	case USB_DEVICE_ID_LENOVO_CBTKBD:
> +		ret = lenovo_probe_cptkbd(hdev);
> +		break;
>  	}
>  	if (ret)
>  		goto err_hid;
> @@ -463,12 +653,22 @@ static void lenovo_remove_tpkbd(struct hid_device *hdev)
>  	hid_set_drvdata(hdev, NULL);
>  }
>  
> +static void lenovo_remove_cptkbd(struct hid_device *hdev)
> +{
> +	sysfs_remove_group(&hdev->dev.kobj,
> +			&lenovo_attr_group_cptkbd);
> +}
> +
>  static void lenovo_remove(struct hid_device *hdev)
>  {
>  	switch (hdev->product) {
>  	case USB_DEVICE_ID_LENOVO_TPKBD:
>  		lenovo_remove_tpkbd(hdev);
>  		break;
> +	case USB_DEVICE_ID_LENOVO_CUSBKBD:
> +	case USB_DEVICE_ID_LENOVO_CBTKBD:
> +		lenovo_remove_cptkbd(hdev);
> +		break;
>  	}
>  
>  	hid_hw_stop(hdev);
> @@ -476,6 +676,8 @@ static void lenovo_remove(struct hid_device *hdev)
>  
>  static const struct hid_device_id lenovo_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) },
>  	{ }
>  };
>  
> @@ -487,6 +689,7 @@ static struct hid_driver lenovo_driver = {
>  	.input_mapping = lenovo_input_mapping,
>  	.probe = lenovo_probe,
>  	.remove = lenovo_remove,
> +	.raw_event = lenovo_raw_event,
>  };
>  module_hid_driver(lenovo_driver);
>  
> diff --git a/include/linux/hid.h b/include/linux/hid.h
> index 77632cf..fca74f1 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
> 


-- 
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 v4 0/4] Add support for Lenovo Compact Keyboard

[Antonio Ospite]

On Sun, 13 Jul 2014 08:24:18 +0100
Jamie Lentin <jm@lentin.co.uk> wrote:

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

Hi Jamie,

in the next (and last) iteration add some context to the short commit
message, e.g. by adding a prefix:

HID: lenovo: ...

following the scheme of previous commits in the history.

> Cheers,
> 
> Jamie Lentin (4):
>   Rename hid-lenovo-tpkbd to hid-lenovo
>   Make all base functions switch depending on product ID
>   Style fixes
>   Add support for Compact (Bluetooth|USB) keyboard with Trackpoint

Maybe rename Bluetooth to BT if this becomes too long with the prefix.

Thanks,
   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?