Hi Heiner,

On Jun 17 2016 or thereabouts, Heiner Kallweit wrote:

This patch migrates the USB LED driver to the HID subsystem.
Supported are Dream Cheeky Webmail Notifier / Friends Alert
and Riso Kagaku Webmail Notifier.

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

Successfully tested with a Dream Cheeky Webmail Notifier and a
Riso Kagaku Webmail Notifier compatible device.

Signed-off-by: Heiner Kallweit <redacted>

Looks good, though I have a few comments (sorry, again...)

---
 drivers/hid/Kconfig       |  12 ++
 drivers/hid/Makefile      |   1 +
 drivers/hid/hid-core.c    |   6 +-
 drivers/hid/hid-ids.h     |   2 +
 drivers/hid/hid-usb-led.c | 276 ++++++++++++++++++++++++++++++++++++++++++++++

There is nothing USB related now. How about just calling it hid-led.c?

I'd say remove all "usb" strings from the driver and use hled or hidled instead
of usbled.

+
+struct usbled_type {
+	const char		*name;
+	const char		*short_name;
+	enum led_brightness	max_brightness;
+	size_t			report_size;
+	enum led_report_type	report_type;
+	u8			report_id;
+	int (*init)(struct usbled_device *udev);
+	int (*write)(struct led_classdev *cdev, enum led_brightness br);
+};
+
+struct usbled_led {
+	struct led_classdev	cdev;
+	struct usbled_device	*udev;
+	char			name[32];
+};
+
+struct usbled_device {
+	const struct usbled_type *type;
+	struct usbled_led	red;
+	struct usbled_led	green;
+	struct usbled_led	blue;
+	struct hid_device       *hdev;
+	struct mutex		lock;
+};
+
+#define MAX_REPORT_SIZE		16
+
+#define to_usbled_led(arg) container_of(arg, struct usbled_led, cdev)
+
+static bool riso_kagaku_switch_green_blue;
+module_param(riso_kagaku_switch_green_blue, bool, S_IRUGO | S_IWUSR);
+MODULE_PARM_DESC(riso_kagaku_switch_green_blue,
+	"switch green and blue RGB component for Riso Kagaku devices");
+
+static int usbled_send(struct usbled_device *udev, __u8 *buf)
+{
+	int ret;
+
+	buf[0] = udev->type->report_id;
+
+	mutex_lock(&udev->lock);
+
+	if (udev->type->report_type == RAW_REQUEST)
+		ret = hid_hw_raw_request(udev->hdev, buf[0], buf,
+					 udev->type->report_size,
+					 HID_FEATURE_REPORT,
+					 HID_REQ_SET_REPORT);
+	else if (udev->type->report_type == OUTPUT_REPORT)
+		ret = hid_hw_output_report(udev->hdev, buf,
+					   udev->type->report_size);
+	else
+		ret = -EINVAL;
+
+	mutex_unlock(&udev->lock);
+
+	if (ret < 0)
+		return ret;
+
+	return ret == udev->type->report_size ? 0 : -EMSGSIZE;
+}
+
+static u8 riso_kagaku_index(struct usbled_device *udev)
+{
+	enum led_brightness r, g, b;
+
+	r = udev->red.cdev.brightness;
+	g = udev->green.cdev.brightness;
+	b = udev->blue.cdev.brightness;
+
+	if (riso_kagaku_switch_green_blue)
+		return RISO_KAGAKU_IX(r, b, g);
+	else
+		return RISO_KAGAKU_IX(r, g, b);
+}
+
+static int riso_kagaku_write(struct led_classdev *cdev, enum led_brightness br)
+{
+	struct usbled_led *uled = to_usbled_led(cdev);
+	struct usbled_device *udev = uled->udev;
+	__u8 buf[MAX_REPORT_SIZE] = {};
+
+	buf[1] = riso_kagaku_index(udev);
+
+	return usbled_send(udev, buf);
+}
+
+static const struct usbled_type usbled_riso_kagaku = {
+	.name = "Riso Kagaku Webmail Notifier",
+	.short_name = "riso_kagaku",
+	.max_brightness = 1,
+	.report_size = 6,
+	.report_type = OUTPUT_REPORT,
+	.report_id = 0,
+	.write = riso_kagaku_write,
+};
+
+static int dream_cheeky_write(struct led_classdev *cdev, enum led_brightness br)
+{
+	struct usbled_led *uled = to_usbled_led(cdev);
+	struct usbled_device *udev = uled->udev;
+	__u8 buf[MAX_REPORT_SIZE] = {};
+
+	buf[1] = udev->red.cdev.brightness;
+	buf[2] = udev->green.cdev.brightness;
+	buf[3] = udev->blue.cdev.brightness;
+	buf[7] = 0x1a;
+	buf[8] = 0x05;
+
+	return usbled_send(udev, buf);
+}
+
+static int dream_cheeky_init(struct usbled_device *udev)
+{
+	__u8 buf[MAX_REPORT_SIZE] = {};
+
+	/* Dream Cheeky magic */
+	buf[1] = 0x1f;
+	buf[2] = 0x02;
+	buf[4] = 0x5f;
+	buf[7] = 0x1a;
+	buf[8] = 0x03;
+
+	return usbled_send(udev, buf);
+}
+
+static const struct usbled_type usbled_dream_cheeky = {
+	.name = "Dream Cheeky Webmail Notifier",
+	.short_name = "dream_cheeky",
+	.max_brightness = 31,
+	.report_size = 9,
+	.report_type = RAW_REQUEST,
+	.report_id = 0,
+	.init = dream_cheeky_init,
+	.write = dream_cheeky_write,
+};
+
+static int usbled_init_led(struct usbled_led *led, const char *color_name,
+			   struct usbled_device *udev, unsigned int minor)
+{
+	snprintf(led->name, sizeof(led->name), "%s%u:%s",
+		 udev->type->short_name, minor, color_name);
+	led->cdev.name = led->name;
+	led->cdev.max_brightness = udev->type->max_brightness;
+	led->cdev.brightness_set_blocking = udev->type->write;
+	led->cdev.flags = LED_HW_PLUGGABLE;
+	led->udev = udev;
+
+	return devm_led_classdev_register(&udev->hdev->dev, &led->cdev);
+}
+
+static int usbled_init_rgb(struct usbled_device *udev, unsigned int minor)
+{
+	int ret;
+
+	/* Register the red diode */
+	ret = usbled_init_led(&udev->red, "red", udev, minor);
+	if (ret)
+		return ret;
+
+	/* Register the green diode */
+	ret = usbled_init_led(&udev->green, "green", udev, minor);
+	if (ret)
+		return ret;
+
+	/* Register the blue diode */
+	return usbled_init_led(&udev->blue, "blue", udev, minor);
+}
+
+static int usbled_probe(struct hid_device *hdev, const struct hid_device_id *id)
+{
+	struct usbled_device *udev;
+	unsigned int minor;
+	int ret;
+
+	udev = devm_kzalloc(&hdev->dev, sizeof(*udev), GFP_KERNEL);
+	if (!udev)
+		return -ENOMEM;
+
+	ret = hid_parse(hdev);
+	if (ret)
+		return ret;
+
+	udev->hdev = hdev;
+	udev->type = (const struct usbled_type *)id->driver_data;

I don't think it is a good idea to have an internal pointer in the
driver_data. Users can bind a new device to your driver through the
sysfs (/sys/bus/hid/drivers/hid-usb-led/bind) and having a pointer is
not easy for them.

I'd rather use a plain enum or simply decide about the type based on the
vendor id.

+	mutex_init(&udev->lock);
+
+	if (udev->type->init) {

udev->type can be null (if manually bound from userspace as mentioned
above)

+		ret = udev->type->init(udev);
+		if (ret)
+			return ret;
+	}
+
+	ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
+	if (ret)
+		return ret;
+
+	minor = ((struct hidraw *) hdev->hidraw)->minor;
+
+	ret = usbled_init_rgb(udev, minor);
+	if (ret) {
+		hid_hw_stop(hdev);
+		return ret;
+	}
+
+	dev_info(&hdev->dev, "%s initialized\n", udev->type->name);

use hid_info(hdev, "initialized"); (not sure the name is that important
in this info message, but it's up to you).

Cheers,
Benjamin

+
+	return 0;
+}
+
+static const struct hid_device_id usbled_table[] = {
+	{ HID_USB_DEVICE(USB_VENDOR_ID_RISO_KAGAKU,
+	  USB_DEVICE_ID_RI_KA_WEBMAIL),
+	  .driver_data = (kernel_ulong_t)&usbled_riso_kagaku },
+	{ HID_USB_DEVICE(USB_VENDOR_ID_DREAM_CHEEKY,
+	  USB_DEVICE_ID_DREAM_CHEEKY_WN),
+	  .driver_data = (kernel_ulong_t)&usbled_dream_cheeky },
+	{ HID_USB_DEVICE(USB_VENDOR_ID_DREAM_CHEEKY,
+	  USB_DEVICE_ID_DREAM_CHEEKY_FA),
+	  .driver_data = (kernel_ulong_t)&usbled_dream_cheeky },
+	{ }
+};
+MODULE_DEVICE_TABLE(hid, usbled_table);
+
+static struct hid_driver usbled_driver = {
+	.name = "usb-led",
+	.probe = usbled_probe,
+	.id_table = usbled_table,
+};
+
+module_hid_driver(usbled_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Heiner Kallweit [off-list ref]");
+MODULE_DESCRIPTION("Simple 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