Re: [PATCH v2] hid: migrate Riso Kagaku LED driver from USB misc to HID

[Benjamin Tissoires <benjamin.tissoires@redhat.com>]

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

> ---
>  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.c
> 
> diff --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 HID
> diff --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.o
> diff --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
>