by Jamie Lentin

[permalink] [raw]

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

On Mon, 14 Jul 2014, Antonio Ospite wrote:

> On Sun, 13 Jul 2014 08:24:19 +0100
> Jamie Lentin <[email protected]> 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 <[email protected]>
>
> Reviewed-by: Antonio Ospite <[email protected]>
>
> 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

2014-07-21 13:55:23

by Antonio Ospite

[permalink] [raw]

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

On Fri, 18 Jul 2014 10:35:35 +0100 (BST)
Jamie Lentin <[email protected]> wrote:

> On Mon, 14 Jul 2014, Antonio Ospite wrote:
>
> > On Sun, 13 Jul 2014 08:24:19 +0100
> > Jamie Lentin <[email protected]> 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 <[email protected]>
> >
> > Reviewed-by: Antonio Ospite <[email protected]>
> >
> > 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?