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

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

[Heiner Kallweit]

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

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

[Benjamin Tissoires]

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
> 

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

[Heiner Kallweit]

Am 14.06.2016 um 23:49 schrieb Benjamin Tissoires:
> 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>> 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).
> 
When looking at other hid device drivers hid_hw_open seems to be needed
only if the device needs special treatment.
My Riso Kagaku and Dream Cheeky devices work fine with the new drivers
w/o calling hid_hw_open.

> 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
> 
Yes, I think you're right.
With regard to the Delcom Visual Signal Indicator:
The old USB LED driver supports only generation 1 of these devices which
have a not fully HID compliant interface.
In 2008 they were replace with generation 2 which has fully a HID
compliant interface. Generation 2 is not yet supported by the kernel.
(Currently I'm trying to get hold of such a device.)

Therefore it might be better to leave support for the Gen 1 Delcom device
at USB misc. However this would mean that we have to change the config
symbol for this driver so that it doesn't conflict with the other part
we're moving to hid.
Would changing the config symbol be an issue? For now we could select
the new config symbol automatically if CONFIG_USB_LED is set.

> 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

I guess you don't mean symlink literally but adding a comment in Kconfig
that the driver was moved?

> - 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
> 
Regards, Heiner

>> ---
>>  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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> + * 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>");
>> +MODULE_DESCRIPTION("Riso Kagaku Webmail Notifier USB RGB LED driver");
>> -- 
>> 2.8.3
>>
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Benjamin Tissoires]

On Jun 15 2016 or thereabouts, Heiner Kallweit wrote:
> Am 14.06.2016 um 23:49 schrieb Benjamin Tissoires:
> > 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).
> > 
> When looking at other hid device drivers hid_hw_open seems to be needed
> only if the device needs special treatment.

Not exactly. hid_hw_open is called whenever someone opens the hidraw or
input node (or any other resource attached to the HID device). This
starts the input reading, but also calls usb_autopm_get_interface()
which will prevent the device to be autosuspended. The drivers that call
hid_hw_open() in their probe are the ones where we know no one will open
the hidraw or input node but we need to poll for events.

> My Riso Kagaku and Dream Cheeky devices work fine with the new drivers
> w/o calling hid_hw_open.

Let's keep it that way then.

> 
> > 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
> > 
> Yes, I think you're right.
> With regard to the Delcom Visual Signal Indicator:
> The old USB LED driver supports only generation 1 of these devices which
> have a not fully HID compliant interface.
> In 2008 they were replace with generation 2 which has fully a HID
> compliant interface. Generation 2 is not yet supported by the kernel.
> (Currently I'm trying to get hold of such a device.)
> 
> Therefore it might be better to leave support for the Gen 1 Delcom device
> at USB misc. However this would mean that we have to change the config
> symbol for this driver so that it doesn't conflict with the other part
> we're moving to hid.
> Would changing the config symbol be an issue? For now we could select
> the new config symbol automatically if CONFIG_USB_LED is set.

Why would you change the old config symbol for the existing devices. If
(and I don't think that's the best option) you were to add a new symbol,
you could use this to select between HID and USB for the new drivers:
CONFIG_USB_LED_LEGACY_SUPPORT would depend on CONFIG_USB_LED, and you
choose to enable the HID driver based on CONFIG_USB_LED_LEGACY_SUPPORT.

> 
> > 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
> 
> I guess you don't mean symlink literally but adding a comment in Kconfig
> that the driver was moved?

No. I'd rather not having the files moved around when they are KABI. So
either you add the old sysfs files directly at the same level, or you
symlink to them. Some HID driver already take a pointer to the
underlying USB device, so you can do the same.

> 
> > - 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
> > 
> Regards, Heiner
> 

Cheers,
Benjamin

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

[Heiner Kallweit]

Am 15.06.2016 um 09:31 schrieb Benjamin Tissoires:
> On Jun 15 2016 or thereabouts, Heiner Kallweit wrote:
>> Am 14.06.2016 um 23:49 schrieb Benjamin Tissoires:
>>> 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>>> ---
>>>> 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).
>>>
>> When looking at other hid device drivers hid_hw_open seems to be needed
>> only if the device needs special treatment.
> 
> Not exactly. hid_hw_open is called whenever someone opens the hidraw or
> input node (or any other resource attached to the HID device). This
> starts the input reading, but also calls usb_autopm_get_interface()
> which will prevent the device to be autosuspended. The drivers that call
> hid_hw_open() in their probe are the ones where we know no one will open
> the hidraw or input node but we need to poll for events.
> 
>> My Riso Kagaku and Dream Cheeky devices work fine with the new drivers
>> w/o calling hid_hw_open.
> 
> Let's keep it that way then.
> 
>>
>>> 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
>>>
>> Yes, I think you're right.
>> With regard to the Delcom Visual Signal Indicator:
>> The old USB LED driver supports only generation 1 of these devices which
>> have a not fully HID compliant interface.
>> In 2008 they were replace with generation 2 which has fully a HID
>> compliant interface. Generation 2 is not yet supported by the kernel.
>> (Currently I'm trying to get hold of such a device.)
>>
>> Therefore it might be better to leave support for the Gen 1 Delcom device
>> at USB misc. However this would mean that we have to change the config
>> symbol for this driver so that it doesn't conflict with the other part
>> we're moving to hid.
>> Would changing the config symbol be an issue? For now we could select
>> the new config symbol automatically if CONFIG_USB_LED is set.
> 
> Why would you change the old config symbol for the existing devices. If
> (and I don't think that's the best option) you were to add a new symbol,
> you could use this to select between HID and USB for the new drivers:
> CONFIG_USB_LED_LEGACY_SUPPORT would depend on CONFIG_USB_LED, and you
> choose to enable the HID driver based on CONFIG_USB_LED_LEGACY_SUPPORT.
> 
>>
>>> 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
>>
>> I guess you don't mean symlink literally but adding a comment in Kconfig
>> that the driver was moved?
> 
> No. I'd rather not having the files moved around when they are KABI. So
> either you add the old sysfs files directly at the same level, or you
> symlink to them. Some HID driver already take a pointer to the
> underlying USB device, so you can do the same.
> 
OK, thanks for the further explanations. So, what I would do:
- merge new LED HID drivers to a "new USB LED" driver
- remove Riso Kagaku and Dream Cheeky support from old USB LED driver
  (the old driver then just supports the non-HID Gen 1 Delcom device)
- let new USB LED driver create sysmlinks in sysfs to not break the old ABI

The latter can be achieved easier than expected as the old sysfs
attributes share the same semantics as the brightness attribute of
the new led_classdev. Means I don't have to implement the attribute
show / store ops for the old attributes.
Therefore I think we can do the change in one pass.

For creating the symlinks I just need something like:

ret = devm_led_classdev_register(&dcdev->hdev->dev, &led->cdev);
if (ret)
	return ret;

node = sysfs_get_dirent(led->cdev.dev->kobj.sd, "brightness");
kernfs_create_link(dcdev->hdev->dev.parent->kobj.sd, color_name, node);
sysfs_put(node);

Looks a little bit scary but works perfectly fine here.
It creates the following link (same location as with old USB LED driver)
/sys/devices/pci0000:00/0000:00:14.0/usb1/1-6/1-6:1.0/green
like this:
green -> 0003:1D34:0004.0002/leds/dream_cheeky0:green/brightness

The only issue so far is that kernfs_create_link is public but misses
an EXPORT_SYMBOL_GPL declaration. Therefore I can't use it in a module.
For my tests I exported it and the module worked fine.

Seems like symlinking to kernfs_node is not supported yet for modules,
only symlinking to kobject.
Having said that I either have to convince the sysfs/kernfs maintainers
that this export is a good thing or the new USB LED driver is not available
as a module for now.

Regards, Heiner

>>
>>> - 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
>>>
>> Regards, Heiner
>>
> 
> Cheers,
> Benjamin
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Heiner Kallweit]

Am 15.06.2016 um 22:59 schrieb Heiner Kallweit:
> Am 15.06.2016 um 09:31 schrieb Benjamin Tissoires:
>> On Jun 15 2016 or thereabouts, Heiner Kallweit wrote:
>>> Am 14.06.2016 um 23:49 schrieb Benjamin Tissoires:
>>>> 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).
>>>>
>>> When looking at other hid device drivers hid_hw_open seems to be needed
>>> only if the device needs special treatment.
>>
>> Not exactly. hid_hw_open is called whenever someone opens the hidraw or
>> input node (or any other resource attached to the HID device). This
>> starts the input reading, but also calls usb_autopm_get_interface()
>> which will prevent the device to be autosuspended. The drivers that call
>> hid_hw_open() in their probe are the ones where we know no one will open
>> the hidraw or input node but we need to poll for events.
>>
>>> My Riso Kagaku and Dream Cheeky devices work fine with the new drivers
>>> w/o calling hid_hw_open.
>>
>> Let's keep it that way then.
>>
>>>
>>>> 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
>>>>
>>> Yes, I think you're right.
>>> With regard to the Delcom Visual Signal Indicator:
>>> The old USB LED driver supports only generation 1 of these devices which
>>> have a not fully HID compliant interface.
>>> In 2008 they were replace with generation 2 which has fully a HID
>>> compliant interface. Generation 2 is not yet supported by the kernel.
>>> (Currently I'm trying to get hold of such a device.)
>>>
>>> Therefore it might be better to leave support for the Gen 1 Delcom device
>>> at USB misc. However this would mean that we have to change the config
>>> symbol for this driver so that it doesn't conflict with the other part
>>> we're moving to hid.
>>> Would changing the config symbol be an issue? For now we could select
>>> the new config symbol automatically if CONFIG_USB_LED is set.
>>
>> Why would you change the old config symbol for the existing devices. If
>> (and I don't think that's the best option) you were to add a new symbol,
>> you could use this to select between HID and USB for the new drivers:
>> CONFIG_USB_LED_LEGACY_SUPPORT would depend on CONFIG_USB_LED, and you
>> choose to enable the HID driver based on CONFIG_USB_LED_LEGACY_SUPPORT.
>>
>>>
>>>> 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
>>>
>>> I guess you don't mean symlink literally but adding a comment in Kconfig
>>> that the driver was moved?
>>
>> No. I'd rather not having the files moved around when they are KABI. So
>> either you add the old sysfs files directly at the same level, or you
>> symlink to them. Some HID driver already take a pointer to the
>> underlying USB device, so you can do the same.
>>
> OK, thanks for the further explanations. So, what I would do:
> - merge new LED HID drivers to a "new USB LED" driver
> - remove Riso Kagaku and Dream Cheeky support from old USB LED driver
>   (the old driver then just supports the non-HID Gen 1 Delcom device)
> - let new USB LED driver create sysmlinks in sysfs to not break the old ABI
> 
> The latter can be achieved easier than expected as the old sysfs
> attributes share the same semantics as the brightness attribute of
> the new led_classdev. Means I don't have to implement the attribute
> show / store ops for the old attributes.
> Therefore I think we can do the change in one pass.
> 
> For creating the symlinks I just need something like:
> 
> ret = devm_led_classdev_register(&dcdev->hdev->dev, &led->cdev);
> if (ret)
> 	return ret;
> 
> node = sysfs_get_dirent(led->cdev.dev->kobj.sd, "brightness");
> kernfs_create_link(dcdev->hdev->dev.parent->kobj.sd, color_name, node);
> sysfs_put(node);
> 
> Looks a little bit scary but works perfectly fine here.
> It creates the following link (same location as with old USB LED driver)
> /sys/devices/pci0000:00/0000:00:14.0/usb1/1-6/1-6:1.0/green
> like this:
> green -> 0003:1D34:0004.0002/leds/dream_cheeky0:green/brightness
> 
> The only issue so far is that kernfs_create_link is public but misses
> an EXPORT_SYMBOL_GPL declaration. Therefore I can't use it in a module.
> For my tests I exported it and the module worked fine.
> 
> Seems like symlinking to kernfs_node is not supported yet for modules,
> only symlinking to kobject.
> Having said that I either have to convince the sysfs/kernfs maintainers
> that this export is a good thing or the new USB LED driver is not available
> as a module for now.
> 
Greg, as original author of the USB LED driver you've been on cc anyway and
just by chance you're also maintainer of sysfs / kernfs.

Would exporting kernfs_create_link be fine with you and if yes: Should we
add a sysfs_.. wrapper (e.g. sysfs_create_node_link) in linux/sysfs.h
like for other kernfs_.. calls ?

Regards, Heiner

> Regards, Heiner
> 
>>>
>>>> - 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
>>>>
>>> Regards, Heiner
>>>
>>
>> Cheers,
>> Benjamin
>>
> 

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

[Greg Kroah-Hartman]

On Thu, Jun 16, 2016 at 07:45:35AM +0200, Heiner Kallweit wrote:
> Am 15.06.2016 um 22:59 schrieb Heiner Kallweit:
> > Am 15.06.2016 um 09:31 schrieb Benjamin Tissoires:
> >> On Jun 15 2016 or thereabouts, Heiner Kallweit wrote:
> >>> Am 14.06.2016 um 23:49 schrieb Benjamin Tissoires:
> >>>> 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).
> >>>>
> >>> When looking at other hid device drivers hid_hw_open seems to be needed
> >>> only if the device needs special treatment.
> >>
> >> Not exactly. hid_hw_open is called whenever someone opens the hidraw or
> >> input node (or any other resource attached to the HID device). This
> >> starts the input reading, but also calls usb_autopm_get_interface()
> >> which will prevent the device to be autosuspended. The drivers that call
> >> hid_hw_open() in their probe are the ones where we know no one will open
> >> the hidraw or input node but we need to poll for events.
> >>
> >>> My Riso Kagaku and Dream Cheeky devices work fine with the new drivers
> >>> w/o calling hid_hw_open.
> >>
> >> Let's keep it that way then.
> >>
> >>>
> >>>> 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
> >>>>
> >>> Yes, I think you're right.
> >>> With regard to the Delcom Visual Signal Indicator:
> >>> The old USB LED driver supports only generation 1 of these devices which
> >>> have a not fully HID compliant interface.
> >>> In 2008 they were replace with generation 2 which has fully a HID
> >>> compliant interface. Generation 2 is not yet supported by the kernel.
> >>> (Currently I'm trying to get hold of such a device.)
> >>>
> >>> Therefore it might be better to leave support for the Gen 1 Delcom device
> >>> at USB misc. However this would mean that we have to change the config
> >>> symbol for this driver so that it doesn't conflict with the other part
> >>> we're moving to hid.
> >>> Would changing the config symbol be an issue? For now we could select
> >>> the new config symbol automatically if CONFIG_USB_LED is set.
> >>
> >> Why would you change the old config symbol for the existing devices. If
> >> (and I don't think that's the best option) you were to add a new symbol,
> >> you could use this to select between HID and USB for the new drivers:
> >> CONFIG_USB_LED_LEGACY_SUPPORT would depend on CONFIG_USB_LED, and you
> >> choose to enable the HID driver based on CONFIG_USB_LED_LEGACY_SUPPORT.
> >>
> >>>
> >>>> 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
> >>>
> >>> I guess you don't mean symlink literally but adding a comment in Kconfig
> >>> that the driver was moved?
> >>
> >> No. I'd rather not having the files moved around when they are KABI. So
> >> either you add the old sysfs files directly at the same level, or you
> >> symlink to them. Some HID driver already take a pointer to the
> >> underlying USB device, so you can do the same.
> >>
> > OK, thanks for the further explanations. So, what I would do:
> > - merge new LED HID drivers to a "new USB LED" driver
> > - remove Riso Kagaku and Dream Cheeky support from old USB LED driver
> >   (the old driver then just supports the non-HID Gen 1 Delcom device)
> > - let new USB LED driver create sysmlinks in sysfs to not break the old ABI
> > 
> > The latter can be achieved easier than expected as the old sysfs
> > attributes share the same semantics as the brightness attribute of
> > the new led_classdev. Means I don't have to implement the attribute
> > show / store ops for the old attributes.
> > Therefore I think we can do the change in one pass.
> > 
> > For creating the symlinks I just need something like:
> > 
> > ret = devm_led_classdev_register(&dcdev->hdev->dev, &led->cdev);
> > if (ret)
> > 	return ret;
> > 
> > node = sysfs_get_dirent(led->cdev.dev->kobj.sd, "brightness");
> > kernfs_create_link(dcdev->hdev->dev.parent->kobj.sd, color_name, node);
> > sysfs_put(node);
> > 
> > Looks a little bit scary but works perfectly fine here.
> > It creates the following link (same location as with old USB LED driver)
> > /sys/devices/pci0000:00/0000:00:14.0/usb1/1-6/1-6:1.0/green
> > like this:
> > green -> 0003:1D34:0004.0002/leds/dream_cheeky0:green/brightness
> > 
> > The only issue so far is that kernfs_create_link is public but misses
> > an EXPORT_SYMBOL_GPL declaration. Therefore I can't use it in a module.
> > For my tests I exported it and the module worked fine.
> > 
> > Seems like symlinking to kernfs_node is not supported yet for modules,
> > only symlinking to kobject.
> > Having said that I either have to convince the sysfs/kernfs maintainers
> > that this export is a good thing or the new USB LED driver is not available
> > as a module for now.
> > 
> Greg, as original author of the USB LED driver you've been on cc anyway and
> just by chance you're also maintainer of sysfs / kernfs.
> 
> Would exporting kernfs_create_link be fine with you and if yes: Should we
> add a sysfs_.. wrapper (e.g. sysfs_create_node_link) in linux/sysfs.h
> like for other kernfs_.. calls ?

Ick, no.  Wait, I wrote this?  Oh yeah, no, just delete it, and use your
new version instead.  No need to preserve this bizarre one-off api for
any reason, I don't think anyone uses it.  The device I had is
long-gone.  If anyone objects after we remove it, we can think about
adding it back, but for now, let's just delete it.

thanks,

greg k-h

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

[Heiner Kallweit]

Am 16.06.2016 um 09:08 schrieb Greg Kroah-Hartman:
> On Thu, Jun 16, 2016 at 07:45:35AM +0200, Heiner Kallweit wrote:
>> Am 15.06.2016 um 22:59 schrieb Heiner Kallweit:
>>> Am 15.06.2016 um 09:31 schrieb Benjamin Tissoires:
>>>> On Jun 15 2016 or thereabouts, Heiner Kallweit wrote:
>>>>> Am 14.06.2016 um 23:49 schrieb Benjamin Tissoires:
>>>>>> 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).
>>>>>>
>>>>> When looking at other hid device drivers hid_hw_open seems to be needed
>>>>> only if the device needs special treatment.
>>>>
>>>> Not exactly. hid_hw_open is called whenever someone opens the hidraw or
>>>> input node (or any other resource attached to the HID device). This
>>>> starts the input reading, but also calls usb_autopm_get_interface()
>>>> which will prevent the device to be autosuspended. The drivers that call
>>>> hid_hw_open() in their probe are the ones where we know no one will open
>>>> the hidraw or input node but we need to poll for events.
>>>>
>>>>> My Riso Kagaku and Dream Cheeky devices work fine with the new drivers
>>>>> w/o calling hid_hw_open.
>>>>
>>>> Let's keep it that way then.
>>>>
>>>>>
>>>>>> 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
>>>>>>
>>>>> Yes, I think you're right.
>>>>> With regard to the Delcom Visual Signal Indicator:
>>>>> The old USB LED driver supports only generation 1 of these devices which
>>>>> have a not fully HID compliant interface.
>>>>> In 2008 they were replace with generation 2 which has fully a HID
>>>>> compliant interface. Generation 2 is not yet supported by the kernel.
>>>>> (Currently I'm trying to get hold of such a device.)
>>>>>
>>>>> Therefore it might be better to leave support for the Gen 1 Delcom device
>>>>> at USB misc. However this would mean that we have to change the config
>>>>> symbol for this driver so that it doesn't conflict with the other part
>>>>> we're moving to hid.
>>>>> Would changing the config symbol be an issue? For now we could select
>>>>> the new config symbol automatically if CONFIG_USB_LED is set.
>>>>
>>>> Why would you change the old config symbol for the existing devices. If
>>>> (and I don't think that's the best option) you were to add a new symbol,
>>>> you could use this to select between HID and USB for the new drivers:
>>>> CONFIG_USB_LED_LEGACY_SUPPORT would depend on CONFIG_USB_LED, and you
>>>> choose to enable the HID driver based on CONFIG_USB_LED_LEGACY_SUPPORT.
>>>>
>>>>>
>>>>>> 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
>>>>>
>>>>> I guess you don't mean symlink literally but adding a comment in Kconfig
>>>>> that the driver was moved?
>>>>
>>>> No. I'd rather not having the files moved around when they are KABI. So
>>>> either you add the old sysfs files directly at the same level, or you
>>>> symlink to them. Some HID driver already take a pointer to the
>>>> underlying USB device, so you can do the same.
>>>>
>>> OK, thanks for the further explanations. So, what I would do:
>>> - merge new LED HID drivers to a "new USB LED" driver
>>> - remove Riso Kagaku and Dream Cheeky support from old USB LED driver
>>>   (the old driver then just supports the non-HID Gen 1 Delcom device)
>>> - let new USB LED driver create sysmlinks in sysfs to not break the old ABI
>>>
>>> The latter can be achieved easier than expected as the old sysfs
>>> attributes share the same semantics as the brightness attribute of
>>> the new led_classdev. Means I don't have to implement the attribute
>>> show / store ops for the old attributes.
>>> Therefore I think we can do the change in one pass.
>>>
>>> For creating the symlinks I just need something like:
>>>
>>> ret = devm_led_classdev_register(&dcdev->hdev->dev, &led->cdev);
>>> if (ret)
>>> 	return ret;
>>>
>>> node = sysfs_get_dirent(led->cdev.dev->kobj.sd, "brightness");
>>> kernfs_create_link(dcdev->hdev->dev.parent->kobj.sd, color_name, node);
>>> sysfs_put(node);
>>>
>>> Looks a little bit scary but works perfectly fine here.
>>> It creates the following link (same location as with old USB LED driver)
>>> /sys/devices/pci0000:00/0000:00:14.0/usb1/1-6/1-6:1.0/green
>>> like this:
>>> green -> 0003:1D34:0004.0002/leds/dream_cheeky0:green/brightness
>>>
>>> The only issue so far is that kernfs_create_link is public but misses
>>> an EXPORT_SYMBOL_GPL declaration. Therefore I can't use it in a module.
>>> For my tests I exported it and the module worked fine.
>>>
>>> Seems like symlinking to kernfs_node is not supported yet for modules,
>>> only symlinking to kobject.
>>> Having said that I either have to convince the sysfs/kernfs maintainers
>>> that this export is a good thing or the new USB LED driver is not available
>>> as a module for now.
>>>
>> Greg, as original author of the USB LED driver you've been on cc anyway and
>> just by chance you're also maintainer of sysfs / kernfs.
>>
>> Would exporting kernfs_create_link be fine with you and if yes: Should we
>> add a sysfs_.. wrapper (e.g. sysfs_create_node_link) in linux/sysfs.h
>> like for other kernfs_.. calls ?
> 
> Ick, no.  Wait, I wrote this?  Oh yeah, no, just delete it, and use your
> new version instead.  No need to preserve this bizarre one-off api for
> any reason, I don't think anyone uses it.  The device I had is
> long-gone.  If anyone objects after we remove it, we can think about
> adding it back, but for now, let's just delete it.
> 
OK, then I'll submit a patch for removing the old USB LED driver.

Regards, Heiner

> thanks,
> 
> greg k-h
>