lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  PHC 
Open Source and information security mailing list archives
 
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Mon, 16 Jun 2014 22:40:07 +0200
From:	Antonio Ospite <ao2@....it>
To:	Jamie Lentin <jm@...tin.co.uk>
Cc:	Jiri Kosina <jkosina@...e.cz>,
	Alexander Clouter <alex@...riz.org.uk>,
	linux-input@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 1/2] Rename hid-lenovo-tpkbd to hid-lenovo, so we can
 add other keyboards

On Sun, 15 Jun 2014 21:39:49 +0100
Jamie Lentin <jm@...tin.co.uk> wrote:

Almost there :)

> Signed-off-by: Jamie Lentin <jm@...tin.co.uk>
> ---
>  drivers/hid/Kconfig                              |  14 +-
>  drivers/hid/Makefile                             |   2 +-
>  drivers/hid/hid-core.c                           |   2 +-
>  drivers/hid/{hid-lenovo-tpkbd.c => hid-lenovo.c} | 233 +++++++++++++----------
>  4 files changed, 142 insertions(+), 109 deletions(-)
>  rename drivers/hid/{hid-lenovo-tpkbd.c => hid-lenovo.c} (59%)
> 
> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> index f722001..dd07d59 100644
> --- a/drivers/hid/Kconfig
> +++ b/drivers/hid/Kconfig
> @@ -322,18 +322,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.
>

Try to wrap text in Kconfig at 80 chars.

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

Maybe don't mention keyboards just yet on the line above since the
driver is now supposed to support other devices too.

> +	- ThinkPad USB Keyboard with TrackPoint (supports extra LEDs and trackpoint
> +	  configuration)
>

Wrap at 80 chars here too.

>  config HID_LOGITECH
>  	tristate "Logitech devices" if EXPERT
> diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
> index 30e4431..384f981 100644
> --- a/drivers/hid/Makefile
> +++ b/drivers/hid/Makefile
> @@ -58,7 +58,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 8a5384c..e8ce932 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -1736,7 +1736,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 59%
> rename from drivers/hid/hid-lenovo-tpkbd.c
> rename to drivers/hid/hid-lenovo.c
> index 2d25b6c..aabf084 100644
> --- a/drivers/hid/hid-lenovo-tpkbd.c
> +++ b/drivers/hid/hid-lenovo.c
> @@ -1,5 +1,6 @@
>  /*
> - *  HID driver for Lenovo ThinkPad USB Keyboard with TrackPoint
> + *  HID driver for Lenovo:-

The dash before the colon is not needed (really not a big deal I
mentioned that again just because there are other fixes too).

> + *  - ThinkPad USB Keyboard with TrackPoint (tpkbd)
>   *

This hunk could go into a preparatory patch separated from the rename
one, see below.

>   *  Copyright (c) 2012 Bernhard Seibold
>   */
> @@ -20,8 +21,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 +35,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,

I'd name this just lenovo_input_mapping() for now.

>  		struct hid_input *hi, struct hid_field *field,
>  		struct hid_usage *usage, unsigned long **bit, int *max)
>  {
> @@ -48,12 +48,26 @@ static int tpkbd_input_mapping(struct hid_device *hdev,
>  	return 0;
>  }
>  
> +static int lenovo_input_mapping(struct hid_device *hdev,

And add this separation in a preparatory patch, more or less equal to
your previous patch 1, which would become a following patch on top of
the rename one.

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

break after return is not needed.

I also like explicit default cases, but I guess it's just personal
preference.

> +	}
> +
> +	return 0;
> +}
> +
>  #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 +83,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,

I have no definitive opinion on these renames, I guess they are OK;
maybe having these functions end in _store and _show is slightly more
readable, but this is not a big deal.

>  		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 +108,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 +138,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 +168,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 +198,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 +321,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,13 +344,24 @@ 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,
> +			const struct hid_device_id *id)

Just curios, why did the function change the signature too here?
The "id" argument is not used in either of the "internal" probe
routines.

>  {
>  	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;
> +	int i, ret;
> +
> +	/*
> +	 * 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);
>

This last part would also go in the preparatory patch.

>  	/* Validate required reports. */
>  	for (i = 0; i < 4; i++) {
> @@ -346,13 +371,12 @@ static int tpkbd_probe_tp(struct hid_device *hdev)
>  	if (!hid_validate_values(hdev, HID_OUTPUT_REPORT, 3, 0, 2))
>  		return -ENODEV;
>  
> -	if (sysfs_create_group(&hdev->dev.kobj,
> -				&tpkbd_attr_group_pointer)) {
> +	ret = sysfs_create_group(&hdev->dev.kobj, &lenovo_attr_group_tpkbd);
> +	if (ret)
>  		hid_warn(hdev, "Could not create sysfs group\n");
> -	}

This is a cleanup of existing code, this ideally would go alone in a
dedicated "cleanup" patch.

>  
>  	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 +399,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;
> @@ -408,12 +434,13 @@ static int tpkbd_probe(struct hid_device *hdev,
>  		goto err;
>  	}
>  
> -	if (hid_get_drvdata(hdev)) {
> -		hid_set_drvdata(hdev, NULL);
> -		ret = tpkbd_probe_tp(hdev);
> -		if (ret)
> -			goto err_hid;
> +	switch (hdev->product) {
> +	case USB_DEVICE_ID_LENOVO_TPKBD:
> +		ret = lenovo_probe_tpkbd(hdev, id);
> +		break;
>  	}
> +	if (ret)
> +		goto err_hid;
>

This hunk would also go in the preparatory patch.

>  	return 0;
>  err_hid:
> @@ -422,12 +449,15 @@ 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);
> +
> +	if (data_pointer == NULL)
> +		return;
>

And this too.

>  	led_classdev_unregister(&data_pointer->led_micmute);
>  	led_classdev_unregister(&data_pointer->led_mute);
> @@ -435,28 +465,31 @@ 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);
> +	switch (hdev->product) {
> +	case USB_DEVICE_ID_LENOVO_TPKBD:
> +		lenovo_remove_tpkbd(hdev);
> +		break;
> +	}
>

And this.

>  	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 = {
> +static struct hid_driver lenovo_driver = {
>  	.name = "lenovo_tpkbd",

The name could now become just "lenovo".

> -	.id_table = tpkbd_devices,
> -	.input_mapping = tpkbd_input_mapping,
> -	.probe = tpkbd_probe,
> -	.remove = tpkbd_remove,
> +	.id_table = lenovo_devices,
> +	.input_mapping = lenovo_input_mapping,
> +	.probe = lenovo_probe,
> +	.remove = lenovo_remove,
>  };
> -module_hid_driver(tpkbd_driver);
> +module_hid_driver(lenovo_driver);
>  
>  MODULE_LICENSE("GPL");
> -- 
> 2.0.0.rc2
> 
> 


-- 
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?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists