Re: [PATCH v2] hid: migrate Riso Kagaku LED driver from USB misc to HID
From: Benjamin Tissoires <hidden>
Date: 2016-06-14 21:49:17
Also in:
linux-leds
On Jun 12 2016 or thereabouts, Heiner Kallweit wrote:
The Riso Kagaku Webmail Notifier (and its clones) is supported as part of usb/misc/usbled driver currently. This patch migrates the driver for this device to the HID subsystem. Benefits: - Avoid using USB low-level calls and use the HID subsystem instead (as this device provides a USB HID interface) - Use standard LED subsystem instead of proprietary sysfs entries, this allows e.g. to use the device with features like triggers I know at least one cheap clone coming with green and blue LED switched. There's a module paramater to deal with this. There might be users of the current module, therefore so far allow compilation of the new driver only if the old one is disabled. Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- v2: - change config symbol from HID_RIKA to HID_RISO_KAGAKU - use full name Riso Kagaku instead of rika in several places - don't remove device from ignore list if CONFIG_USB_LED is defined - fix module parameter permissions
Thanks for the respin. This version (and the other for hid-rika) looks fine. Though I wonder if you should not call hid_hw_open() at the end of probe (and hid_hw_close() during remove) to keep the device opened. Otherwise I am unsure whether your usb commands will get treated (I might be wrong). There is also one high level comment I would like to do: even if your driver is better than usbled.c, people won't use it because distributions won't enable it as there is 2 issues: - one is you are breaking kernel ABI (we already discussed it) - the other one is that if people want to use your new hid drivers, they have to disable CONFIG_USB_LED, which removes support of the DELCOM_VISUAL_SIGNAL_INDICATOR I really think it would not cost too much to do the whole change in 2 passes: - move the entire usbled to hid, keeping the old sysfs API in place, and eventually add a symlink from the old place (under the usb device) to the HID device to keep path stable - then add support for the classdev for the whole 3 types of devices Plus that would means only one driver to maintain instead of 3 that are close enough. Cheers, Benjamin
quoted hunk ↗ jump to hunk
--- drivers/hid/Kconfig | 9 +++ drivers/hid/Makefile | 1 + drivers/hid/hid-core.c | 3 + drivers/hid/hid-riso-kagaku.c | 171 ++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 184 insertions(+) create mode 100644 drivers/hid/hid-riso-kagaku.cdiff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig index 5646ca4..16fe27d 100644 --- a/drivers/hid/Kconfig +++ b/drivers/hid/Kconfig@@ -784,6 +784,15 @@ config HID_HYPERV_MOUSE ---help--- Select this option to enable the Hyper-V mouse driver. +config HID_RISO_KAGAKU + tristate "Riso Kagaku Webmail Notifier USB LED" + depends on HID + depends on LEDS_CLASS + depends on USB_LED = 'n' + ---help--- + Support for the Riso Kagaku Webmail Notifier. This driver registers + a LED class instance for red, green, and blue color component. + config HID_SMARTJOYPLUS tristate "SmartJoy PLUS PS2/USB adapter support" depends on HIDdiff --git a/drivers/hid/Makefile b/drivers/hid/Makefile index a2fb562..90300f3 100644 --- a/drivers/hid/Makefile +++ b/drivers/hid/Makefile@@ -91,6 +91,7 @@ obj-$(CONFIG_HID_STEELSERIES) += hid-steelseries.o obj-$(CONFIG_HID_SUNPLUS) += hid-sunplus.o obj-$(CONFIG_HID_GREENASIA) += hid-gaff.o obj-$(CONFIG_HID_THINGM) += hid-thingm.o +obj-$(CONFIG_HID_RISO_KAGAKU) += hid-riso-kagaku.o obj-$(CONFIG_HID_THRUSTMASTER) += hid-tmff.o obj-$(CONFIG_HID_TIVO) += hid-tivo.o obj-$(CONFIG_HID_TOPSEED) += hid-topseed.odiff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c index 8ea3a26..730589a 100644 --- a/drivers/hid/hid-core.c +++ b/drivers/hid/hid-core.c@@ -2008,6 +2008,7 @@ static const struct hid_device_id hid_have_special_driver[] = { { HID_USB_DEVICE(USB_VENDOR_ID_PETALYNX, USB_DEVICE_ID_PETALYNX_MAXTER_REMOTE) }, { HID_USB_DEVICE(USB_VENDOR_ID_PLANTRONICS, HID_ANY_ID) }, { HID_USB_DEVICE(USB_VENDOR_ID_PRIMAX, USB_DEVICE_ID_PRIMAX_KEYBOARD) }, + { HID_USB_DEVICE(USB_VENDOR_ID_RISO_KAGAKU, USB_DEVICE_ID_RI_KA_WEBMAIL) }, #if IS_ENABLED(CONFIG_HID_ROCCAT) { HID_USB_DEVICE(USB_VENDOR_ID_ROCCAT, USB_DEVICE_ID_ROCCAT_ARVO) }, { HID_USB_DEVICE(USB_VENDOR_ID_ROCCAT, USB_DEVICE_ID_ROCCAT_ISKU) },@@ -2486,7 +2487,9 @@ static const struct hid_device_id hid_ignore_list[] = { { HID_USB_DEVICE(USB_VENDOR_ID_SYNAPTICS, USB_DEVICE_ID_SYNAPTICS_DPAD) }, #endif { HID_USB_DEVICE(USB_VENDOR_ID_YEALINK, USB_DEVICE_ID_YEALINK_P1K_P4K_B2K) }, +#if IS_ENABLED(CONFIG_USB_LED) { HID_USB_DEVICE(USB_VENDOR_ID_RISO_KAGAKU, USB_DEVICE_ID_RI_KA_WEBMAIL) }, +#endif { } };diff --git a/drivers/hid/hid-riso-kagaku.c b/drivers/hid/hid-riso-kagaku.c new file mode 100644 index 0000000..0a37f18 --- /dev/null +++ b/drivers/hid/hid-riso-kagaku.c@@ -0,0 +1,171 @@ +/* + * Riso Kagaku Webmail Notifier USB RGB LED driver + * + * Copyright 2016 Heiner Kallweit <hkallweit1@gmail.com> + * Based on drivers/hid/hid-thingm.c and + * drivers/usb/misc/usbled.c + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License as + * published by the Free Software Foundation, version 2. + */ + +#include <linux/hid.h> +#include <linux/hidraw.h> +#include <linux/leds.h> +#include <linux/module.h> +#include <linux/mutex.h> + +#include "hid-ids.h" + +#define REPORT_SIZE 6 + +static unsigned const char riso_kagaku_tbl[] = { +/* R+2G+4B -> riso kagaku color index */ + [0] = 0, /* black */ + [1] = 2, /* red */ + [2] = 1, /* green */ + [3] = 5, /* yellow */ + [4] = 3, /* blue */ + [5] = 6, /* magenta */ + [6] = 4, /* cyan */ + [7] = 7 /* white */ +}; + +#define RISO_KAGAKU_IX(r, g, b) riso_kagaku_tbl[((r)?1:0)+((g)?2:0)+((b)?4:0)] + +struct rika_led { + struct led_classdev cdev; + struct rika_device *rdev; + char name[32]; +}; + +struct rika_device { + struct rika_led red; + struct rika_led green; + struct rika_led blue; + struct hid_device *hdev; + struct mutex lock; +}; + +#define to_rika_led(arg) container_of(arg, struct rika_led, cdev) + +static bool switch_green_blue; +module_param(switch_green_blue, bool, S_IRUGO | S_IWUSR); +MODULE_PARM_DESC(switch_green_blue, "switch green and blue RGB component"); + +static u8 rika_index(struct rika_device *rdev) +{ + enum led_brightness r, g, b; + + r = rdev->red.cdev.brightness; + g = rdev->green.cdev.brightness; + b = rdev->blue.cdev.brightness; + + if (switch_green_blue) + return RISO_KAGAKU_IX(r, b, g); + else + return RISO_KAGAKU_IX(r, g, b); +} + +static int rika_write_color(struct led_classdev *cdev, enum led_brightness br) +{ + struct rika_led *rled = to_rika_led(cdev); + struct rika_device *rdev = rled->rdev; + __u8 buf[REPORT_SIZE] = {}; + int ret; + + buf[1] = rika_index(rdev); + + mutex_lock(&rdev->lock); + ret = hid_hw_output_report(rdev->hdev, buf, REPORT_SIZE); + mutex_unlock(&rdev->lock); + + if (ret < 0) + return ret; + + return (ret == REPORT_SIZE) ? 0 : -EMSGSIZE; +} + +static int rika_init_led(struct rika_led *led, const char *color_name, + struct rika_device *rdev, int minor) +{ + snprintf(led->name, sizeof(led->name), "rika%d:%s", minor, color_name); + led->cdev.name = led->name; + led->cdev.max_brightness = 1; + led->cdev.brightness_set_blocking = rika_write_color; + led->cdev.flags = LED_HW_PLUGGABLE; + led->rdev = rdev; + + return devm_led_classdev_register(&rdev->hdev->dev, &led->cdev); +} + +static int rika_init_rgb(struct rika_device *rdev, int minor) +{ + int ret; + + /* Register the red diode */ + ret = rika_init_led(&rdev->red, "red", rdev, minor); + if (ret) + return ret; + + /* Register the green diode */ + ret = rika_init_led(&rdev->green, "green", rdev, minor); + if (ret) + return ret; + + /* Register the blue diode */ + return rika_init_led(&rdev->blue, "blue", rdev, minor); +} + +static int rika_probe(struct hid_device *hdev, const struct hid_device_id *id) +{ + struct rika_device *rdev; + int ret, minor; + + rdev = devm_kzalloc(&hdev->dev, sizeof(*rdev), GFP_KERNEL); + if (!rdev) + return -ENOMEM; + + ret = hid_parse(hdev); + if (ret) + return ret; + + rdev->hdev = hdev; + mutex_init(&rdev->lock); + + ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW); + if (ret) + return ret; + + minor = ((struct hidraw *) hdev->hidraw)->minor; + + ret = rika_init_rgb(rdev, minor); + if (ret) { + hid_hw_stop(hdev); + return ret; + } + + dev_info(&hdev->dev, "Riso Kagaku Webmail Notifier %d initialized\n", minor); + + return 0; +} + +static const struct hid_device_id rika_table[] = { + { HID_USB_DEVICE(USB_VENDOR_ID_RISO_KAGAKU, + USB_DEVICE_ID_RI_KA_WEBMAIL) }, + { } +}; +MODULE_DEVICE_TABLE(hid, rika_table); + +static struct hid_driver rika_driver = { + .name = "riso-kagaku", + .probe = rika_probe, + .id_table = rika_table, +}; + +module_hid_driver(rika_driver); + +MODULE_LICENSE("GPL"); +MODULE_AUTHOR("Heiner Kallweit <hkallweit1@gmail.com>"); +MODULE_DESCRIPTION("Riso Kagaku Webmail Notifier USB RGB LED driver");-- 2.8.3