[PATCH 0/4] HID: (thingm) introduces blink(1) mk2

[PATCH 0/4] HID: (thingm) introduces blink(1) mk2

[Vivien Didelot]

This patchset introduces the support of the blink(1) mk2 device, after a
refactoring of the thingm driver. It removes non-standard sysfs attributes and
uses work queues to fix the usage of triggers with this device.

Vivien Didelot (4):
  HID: (thingm) remove the "play" sysfs attribute
  HID: (thingm) remove the "fade" sysfs attribute
  HID: (thingm) refactor blink(1) support
  HID: (thingm) add support for blink(1) mk2

 Documentation/ABI/testing/sysfs-driver-hid-thingm |  23 --
 drivers/hid/hid-thingm.c                          | 361 ++++++++++++----------
 drivers/leds/Kconfig                              |   2 +
 3 files changed, 203 insertions(+), 183 deletions(-)
 delete mode 100644 Documentation/ABI/testing/sysfs-driver-hid-thingm

-- 
1.9.1

[PATCH 1/4] HID: (thingm) remove the "play" sysfs attribute

[Vivien Didelot]

When the thingm driver registers an instance of LED class, it creates a
"play" sysfs attribute for this blink(1) specific feature.

Since this feature is not specific to the RGB chip but to the HID device
itself, let's remove this attribute from the LED instance and only
implement what is useful to switch on and off the LED.

This feature is still easily accessible through hidraw.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 Documentation/ABI/testing/sysfs-driver-hid-thingm |  7 -----
 drivers/hid/hid-thingm.c                          | 33 -----------------------
 2 files changed, 40 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-driver-hid-thingm b/Documentation/ABI/testing/sysfs-driver-hid-thingm
index abcffee..fda6185 100644
--- a/Documentation/ABI/testing/sysfs-driver-hid-thingm
+++ b/Documentation/ABI/testing/sysfs-driver-hid-thingm
@@ -14,10 +14,3 @@ Description:	This attribute allows to set a fade time in milliseconds for
 		the next color change. Read the attribute to know the current
 		fade time. The default value is set to 0 (no fade time). For
 		instance, set a fade time of 2 seconds with: echo 2000 > fade
-
-What:		/sys/class/leds/blink1::<serial>/play
-Date:		January 2013
-Contact:	Vivien Didelot <vivien.didelot@savoirfairelinux.com>
-Description:	This attribute is used to play/pause the light patterns. Write 1
-		to start playing, 0 to stop. Reading this attribute returns the
-		current playing status.
diff --git a/drivers/hid/hid-thingm.c b/drivers/hid/hid-thingm.c
index a97c788..7e376b8 100644
--- a/drivers/hid/hid-thingm.c
+++ b/drivers/hid/hid-thingm.c
@@ -28,7 +28,6 @@
  * @rgb:		8-bit per channel RGB notation.
  * @fade:		fade time in hundredths of a second.
  * @brightness:		brightness coefficient.
- * @play:		play/pause in-memory patterns.
  */
 struct blink1_data {
 	struct hid_device *hdev;
@@ -36,7 +35,6 @@ struct blink1_data {
 	u32 rgb;
 	u16 fade;
 	u8 brightness;
-	bool play;
 };
 
 static int blink1_send_command(struct blink1_data *data,
@@ -155,41 +153,10 @@ static ssize_t blink1_store_fade(struct device *dev,
 static DEVICE_ATTR(fade, S_IRUGO | S_IWUSR,
 		blink1_show_fade, blink1_store_fade);
 
-static ssize_t blink1_show_play(struct device *dev,
-		struct device_attribute *attr, char *buf)
-{
-	struct blink1_data *data = dev_get_drvdata(dev->parent);
-
-	return sprintf(buf, "%d\n", data->play);
-}
-
-static ssize_t blink1_store_play(struct device *dev,
-		struct device_attribute *attr, const char *buf, size_t count)
-{
-	struct blink1_data *data = dev_get_drvdata(dev->parent);
-	u8 cmd[BLINK1_CMD_SIZE] = { 1, 'p', 0, 0, 0, 0, 0, 0, 0 };
-	long unsigned int play;
-	int ret;
-
-	ret = kstrtoul(buf, 10, &play);
-	if (ret)
-		return ret;
-
-	data->play = !!play;
-	cmd[2] = data->play;
-	ret = blink1_send_command(data, cmd);
-
-	return ret ? ret : count;
-}
-
-static DEVICE_ATTR(play, S_IRUGO | S_IWUSR,
-		blink1_show_play, blink1_store_play);
-
 static const struct attribute_group blink1_sysfs_group = {
 	.attrs = (struct attribute *[]) {
 		&dev_attr_rgb.attr,
 		&dev_attr_fade.attr,
-		&dev_attr_play.attr,
 		NULL
 	},
 };
-- 
1.9.1

[PATCH 2/4] HID: (thingm) remove the "fade" sysfs attribute

[Vivien Didelot]

As for the "play" sysfs attribute, remove this other non-standard
attribute, so the driver only implements what is required to switch the
LED on and off. Thus, a fade time won't be ideal for some fast-changing
triggers.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 Documentation/ABI/testing/sysfs-driver-hid-thingm |  8 -----
 drivers/hid/hid-thingm.c                          | 41 -----------------------
 2 files changed, 49 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-driver-hid-thingm b/Documentation/ABI/testing/sysfs-driver-hid-thingm
index fda6185..735c5cb 100644
--- a/Documentation/ABI/testing/sysfs-driver-hid-thingm
+++ b/Documentation/ABI/testing/sysfs-driver-hid-thingm
@@ -6,11 +6,3 @@ Description:	The ThingM blink1 is an USB RGB LED. The color notation is
 		color. Write the 24-bit hexadecimal color to change the current
 		LED color. The default color is full white (0xFFFFFF).
 		For instance, set the color to green with: echo 00FF00 > rgb
-
-What:		/sys/class/leds/blink1::<serial>/fade
-Date:		January 2013
-Contact:	Vivien Didelot <vivien.didelot@savoirfairelinux.com>
-Description:	This attribute allows to set a fade time in milliseconds for
-		the next color change. Read the attribute to know the current
-		fade time. The default value is set to 0 (no fade time). For
-		instance, set a fade time of 2 seconds with: echo 2000 > fade
diff --git a/drivers/hid/hid-thingm.c b/drivers/hid/hid-thingm.c
index 7e376b8..e3b6647 100644
--- a/drivers/hid/hid-thingm.c
+++ b/drivers/hid/hid-thingm.c
@@ -26,14 +26,12 @@
  * @hdev:		HID device.
  * @led_cdev:		LED class instance.
  * @rgb:		8-bit per channel RGB notation.
- * @fade:		fade time in hundredths of a second.
  * @brightness:		brightness coefficient.
  */
 struct blink1_data {
 	struct hid_device *hdev;
 	struct led_classdev led_cdev;
 	u32 rgb;
-	u16 fade;
 	u8 brightness;
 };
 
@@ -64,12 +62,6 @@ static int blink1_update_color(struct blink1_data *data)
 		buf[4] = DIV_ROUND_CLOSEST(blink1_rgb_to_b(data->rgb), coef);
 	}
 
-	if (data->fade) {
-		buf[1] = 'c';
-		buf[5] = (data->fade & 0xFF00) >> 8;
-		buf[6] = (data->fade & 0x00FF);
-	}
-
 	return blink1_send_command(data, buf);
 }
 
@@ -121,42 +113,9 @@ static ssize_t blink1_store_rgb(struct device *dev,
 
 static DEVICE_ATTR(rgb, S_IRUGO | S_IWUSR, blink1_show_rgb, blink1_store_rgb);
 
-static ssize_t blink1_show_fade(struct device *dev,
-		struct device_attribute *attr, char *buf)
-{
-	struct blink1_data *data = dev_get_drvdata(dev->parent);
-
-	return sprintf(buf, "%d\n", data->fade * 10);
-}
-
-static ssize_t blink1_store_fade(struct device *dev,
-		struct device_attribute *attr, const char *buf, size_t count)
-{
-	struct blink1_data *data = dev_get_drvdata(dev->parent);
-	long unsigned int fade;
-	int ret;
-
-	ret = kstrtoul(buf, 10, &fade);
-	if (ret)
-		return ret;
-
-	/* blink(1) accepts 16-bit fade time, number of 10ms ticks */
-	fade = DIV_ROUND_CLOSEST(fade, 10);
-	if (fade > 65535)
-		return -EINVAL;
-
-	data->fade = fade;
-
-	return count;
-}
-
-static DEVICE_ATTR(fade, S_IRUGO | S_IWUSR,
-		blink1_show_fade, blink1_store_fade);
-
 static const struct attribute_group blink1_sysfs_group = {
 	.attrs = (struct attribute *[]) {
 		&dev_attr_rgb.attr,
-		&dev_attr_fade.attr,
 		NULL
 	},
 };
-- 
1.9.1

[PATCH 3/4] HID: (thingm) refactor blink(1) support

[Vivien Didelot]

This patch refactors the way the thingm driver registers a blink(1) LED.
In order to make the driver simpler and more standard, drop the "rgb"
sysfs attribute and create one instance of LED class per RGB channel.

Actually, the name of the LED class instance registered for a blink(1)
device is "blink1::ABCD", where ABCD is the last 4 chars of the serial
number. The driver now registers 3 instances per RGB chip, named
"thingmX:{red,green,blue}:ledY" where X is the hidraw minor number and Y
is the RGB chip number (as seen by the firmware).

This patch also uses work queues to defer calls with the device, which
now allows triggers to work as expected with this LED device.

Also remove the brightness structure field and the brightness_get
backend, as it is already handled by the LED class, and changes the
prefix of functions and structures to thingm_ to match the driver name.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 Documentation/ABI/testing/sysfs-driver-hid-thingm |   8 -
 drivers/hid/hid-thingm.c                          | 304 +++++++++++++++-------
 drivers/leds/Kconfig                              |   2 +
 3 files changed, 209 insertions(+), 105 deletions(-)
 delete mode 100644 Documentation/ABI/testing/sysfs-driver-hid-thingm

diff --git a/Documentation/ABI/testing/sysfs-driver-hid-thingm b/Documentation/ABI/testing/sysfs-driver-hid-thingm
deleted file mode 100644
index 735c5cb..0000000
--- a/Documentation/ABI/testing/sysfs-driver-hid-thingm
+++ /dev/null
@@ -1,8 +0,0 @@
-What:		/sys/class/leds/blink1::<serial>/rgb
-Date:		January 2013
-Contact:	Vivien Didelot <vivien.didelot@savoirfairelinux.com>
-Description:	The ThingM blink1 is an USB RGB LED. The color notation is
-		3-byte hexadecimal. Read this attribute to get the last set
-		color. Write the 24-bit hexadecimal color to change the current
-		LED color. The default color is full white (0xFFFFFF).
-		For instance, set the color to green with: echo 00FF00 > rgb
diff --git a/drivers/hid/hid-thingm.c b/drivers/hid/hid-thingm.c
index e3b6647..0af0eb4 100644
--- a/drivers/hid/hid-thingm.c
+++ b/drivers/hid/hid-thingm.c
@@ -1,7 +1,7 @@
 /*
  * ThingM blink(1) USB RGB LED driver
  *
- * Copyright 2013 Savoir-faire Linux Inc.
+ * Copyright 2013-2014 Savoir-faire Linux Inc.
  *	Vivien Didelot <vivien.didelot@savoirfairelinux.com>
  *
  * This program is free software; you can redistribute it and/or
@@ -10,170 +10,280 @@
  */
 
 #include <linux/hid.h>
+#include <linux/hidraw.h>
 #include <linux/leds.h>
 #include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/workqueue.h>
 
 #include "hid-ids.h"
 
-#define BLINK1_CMD_SIZE		9
+#define REPORT_ID	1
+#define REPORT_SIZE	9
 
-#define blink1_rgb_to_r(rgb)	((rgb & 0xFF0000) >> 16)
-#define blink1_rgb_to_g(rgb)	((rgb & 0x00FF00) >> 8)
-#define blink1_rgb_to_b(rgb)	((rgb & 0x0000FF) >> 0)
+/* Firmware major number of supported devices */
+#define THINGM_MAJOR_MK1	'1'
 
-/**
- * struct blink1_data - blink(1) device specific data
- * @hdev:		HID device.
- * @led_cdev:		LED class instance.
- * @rgb:		8-bit per channel RGB notation.
- * @brightness:		brightness coefficient.
- */
-struct blink1_data {
+struct thingm_fwinfo {
+	char major;
+	unsigned numrgb;
+	unsigned first;
+};
+
+const struct thingm_fwinfo thingm_fwinfo[] = {
+	{
+		.major = THINGM_MAJOR_MK1,
+		.numrgb = 1,
+		.first = 0,
+	}
+};
+
+/* A red, green or blue channel, part of an RGB chip */
+struct thingm_led {
+	struct thingm_rgb *rgb;
+	struct led_classdev ldev;
+	char name[32];
+};
+
+/* Basically a WS2812 5050 RGB LED chip */
+struct thingm_rgb {
+	struct thingm_device *tdev;
+	struct thingm_led red;
+	struct thingm_led green;
+	struct thingm_led blue;
+	struct work_struct work;
+	u8 num;
+};
+
+struct thingm_device {
 	struct hid_device *hdev;
-	struct led_classdev led_cdev;
-	u32 rgb;
-	u8 brightness;
+	struct {
+		char major;
+		char minor;
+	} version;
+	const struct thingm_fwinfo *fwinfo;
+	struct mutex lock;
+	struct thingm_rgb *rgb;
 };
 
-static int blink1_send_command(struct blink1_data *data,
-		u8 buf[BLINK1_CMD_SIZE])
+static int thingm_send(struct thingm_device *tdev, u8 buf[REPORT_SIZE])
 {
 	int ret;
 
-	hid_dbg(data->hdev, "command: %d%c%.2x%.2x%.2x%.2x%.2x%.2x%.2x\n",
+	hid_dbg(tdev->hdev, "-> %d %c %02hhx %02hhx %02hhx %02hhx %02hhx %02hhx %02hhx\n",
 			buf[0], buf[1], buf[2], buf[3], buf[4],
 			buf[5], buf[6], buf[7], buf[8]);
 
-	ret = hid_hw_raw_request(data->hdev, buf[0], buf, BLINK1_CMD_SIZE,
-				 HID_FEATURE_REPORT, HID_REQ_SET_REPORT);
+	ret = hid_hw_raw_request(tdev->hdev, buf[0], buf, REPORT_SIZE,
+			HID_FEATURE_REPORT, HID_REQ_SET_REPORT);
 
 	return ret < 0 ? ret : 0;
 }
 
-static int blink1_update_color(struct blink1_data *data)
+static int thingm_recv(struct thingm_device *tdev, u8 buf[REPORT_SIZE])
 {
-	u8 buf[BLINK1_CMD_SIZE] = { 1, 'n', 0, 0, 0, 0, 0, 0, 0 };
+	int ret;
 
-	if (data->brightness) {
-		unsigned int coef = DIV_ROUND_CLOSEST(255, data->brightness);
+	ret = hid_hw_raw_request(tdev->hdev, buf[0], buf, REPORT_SIZE,
+			HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
+	if (ret < 0)
+		return ret;
 
-		buf[2] = DIV_ROUND_CLOSEST(blink1_rgb_to_r(data->rgb), coef);
-		buf[3] = DIV_ROUND_CLOSEST(blink1_rgb_to_g(data->rgb), coef);
-		buf[4] = DIV_ROUND_CLOSEST(blink1_rgb_to_b(data->rgb), coef);
-	}
+	hid_dbg(tdev->hdev, "<- %d %c %02hhx %02hhx %02hhx %02hhx %02hhx %02hhx %02hhx\n",
+			buf[0], buf[1], buf[2], buf[3], buf[4],
+			buf[5], buf[6], buf[7], buf[8]);
 
-	return blink1_send_command(data, buf);
+	return 0;
 }
 
-static void blink1_led_set(struct led_classdev *led_cdev,
-		enum led_brightness brightness)
+static int thingm_version(struct thingm_device *tdev)
 {
-	struct blink1_data *data = dev_get_drvdata(led_cdev->dev->parent);
+	u8 buf[REPORT_SIZE] = { REPORT_ID, 'v', 0, 0, 0, 0, 0, 0, 0 };
+	int err;
+
+	err = thingm_send(tdev, buf);
+	if (err)
+		return err;
+
+	err = thingm_recv(tdev, buf);
+	if (err)
+		return err;
 
-	data->brightness = brightness;
-	if (blink1_update_color(data))
-		hid_err(data->hdev, "failed to update color\n");
+	tdev->version.major = buf[3];
+	tdev->version.minor = buf[4];
+
+	return 0;
 }
 
-static enum led_brightness blink1_led_get(struct led_classdev *led_cdev)
+static int thingm_write_color(struct thingm_rgb *rgb)
 {
-	struct blink1_data *data = dev_get_drvdata(led_cdev->dev->parent);
+	u8 buf[REPORT_SIZE] = { REPORT_ID, 'n', 0, 0, 0, 0, 0, 0, 0 };
+
+	buf[2] = rgb->red.ldev.brightness;
+	buf[3] = rgb->green.ldev.brightness;
+	buf[4] = rgb->blue.ldev.brightness;
 
-	return data->brightness;
+	return thingm_send(rgb->tdev, buf);
 }
 
-static ssize_t blink1_show_rgb(struct device *dev,
-		struct device_attribute *attr, char *buf)
+static void thingm_work(struct work_struct *work)
 {
-	struct blink1_data *data = dev_get_drvdata(dev->parent);
+	struct thingm_rgb *rgb = container_of(work, struct thingm_rgb, work);
 
-	return sprintf(buf, "%.6X\n", data->rgb);
+	mutex_lock(&rgb->tdev->lock);
+
+	if (thingm_write_color(rgb))
+		hid_err(rgb->tdev->hdev, "failed to write color\n");
+
+	mutex_unlock(&rgb->tdev->lock);
 }
 
-static ssize_t blink1_store_rgb(struct device *dev,
-		struct device_attribute *attr, const char *buf, size_t count)
+static void thingm_led_set(struct led_classdev *ldev,
+		enum led_brightness brightness)
 {
-	struct blink1_data *data = dev_get_drvdata(dev->parent);
-	long unsigned int rgb;
-	int ret;
+	struct thingm_led *led = container_of(ldev, struct thingm_led, ldev);
 
-	ret = kstrtoul(buf, 16, &rgb);
-	if (ret)
-		return ret;
+	/* the ledclass has already stored the brightness value */
+	schedule_work(&led->rgb->work);
+}
 
-	/* RGB triplet notation is 24-bit hexadecimal */
-	if (rgb > 0xFFFFFF)
-		return -EINVAL;
+static int thingm_init_rgb(struct thingm_rgb *rgb)
+{
+	const int minor = ((struct hidraw *) rgb->tdev->hdev->hidraw)->minor;
+	int err;
+
+	/* Register the red diode */
+	snprintf(rgb->red.name, sizeof(rgb->red.name),
+			"thingm%d:red:led%d", minor, rgb->num);
+	rgb->red.ldev.name = rgb->red.name;
+	rgb->red.ldev.max_brightness = 255;
+	rgb->red.ldev.brightness_set = thingm_led_set;
+	rgb->red.rgb = rgb;
+
+	err = led_classdev_register(&rgb->tdev->hdev->dev, &rgb->red.ldev);
+	if (err)
+		return err;
+
+	/* Register the green diode */
+	snprintf(rgb->green.name, sizeof(rgb->green.name),
+			"thingm%d:green:led%d", minor, rgb->num);
+	rgb->green.ldev.name = rgb->green.name;
+	rgb->green.ldev.max_brightness = 255;
+	rgb->green.ldev.brightness_set = thingm_led_set;
+	rgb->green.rgb = rgb;
+
+	err = led_classdev_register(&rgb->tdev->hdev->dev, &rgb->green.ldev);
+	if (err)
+		goto unregister_red;
+
+	/* Register the blue diode */
+	snprintf(rgb->blue.name, sizeof(rgb->blue.name),
+			"thingm%d:blue:led%d", minor, rgb->num);
+	rgb->blue.ldev.name = rgb->blue.name;
+	rgb->blue.ldev.max_brightness = 255;
+	rgb->blue.ldev.brightness_set = thingm_led_set;
+	rgb->blue.rgb = rgb;
+
+	err = led_classdev_register(&rgb->tdev->hdev->dev, &rgb->blue.ldev);
+	if (err)
+		goto unregister_green;
+
+	INIT_WORK(&rgb->work, thingm_work);
 
-	data->rgb = rgb;
-	ret = blink1_update_color(data);
+	return 0;
 
-	return ret ? ret : count;
-}
+unregister_green:
+	led_classdev_unregister(&rgb->green.ldev);
 
-static DEVICE_ATTR(rgb, S_IRUGO | S_IWUSR, blink1_show_rgb, blink1_store_rgb);
+unregister_red:
+	led_classdev_unregister(&rgb->red.ldev);
 
-static const struct attribute_group blink1_sysfs_group = {
-	.attrs = (struct attribute *[]) {
-		&dev_attr_rgb.attr,
-		NULL
-	},
-};
+	return err;
+}
+
+static void thingm_remove_rgb(struct thingm_rgb *rgb)
+{
+	flush_work(&rgb->work);
+	led_classdev_unregister(&rgb->red.ldev);
+	led_classdev_unregister(&rgb->green.ldev);
+	led_classdev_unregister(&rgb->blue.ldev);
+}
 
 static int thingm_probe(struct hid_device *hdev, const struct hid_device_id *id)
 {
-	struct blink1_data *data;
-	struct led_classdev *led;
-	char led_name[13];
-	int ret;
+	struct thingm_device *tdev;
+	int i, err;
 
-	data = devm_kzalloc(&hdev->dev, sizeof(struct blink1_data), GFP_KERNEL);
-	if (!data)
+	tdev = devm_kzalloc(&hdev->dev, sizeof(struct thingm_device),
+			GFP_KERNEL);
+	if (!tdev)
 		return -ENOMEM;
 
-	hid_set_drvdata(hdev, data);
-	data->hdev = hdev;
-	data->rgb = 0xFFFFFF; /* set a default white color */
+	tdev->hdev = hdev;
+	hid_set_drvdata(hdev, tdev);
 
-	ret = hid_parse(hdev);
-	if (ret)
+	err = hid_parse(hdev);
+	if (err)
 		goto error;
 
-	ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
-	if (ret)
+	err = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
+	if (err)
 		goto error;
 
-	/* blink(1) serial numbers range is 0x1A001000 to 0x1A002FFF */
-	led = &data->led_cdev;
-	snprintf(led_name, sizeof(led_name), "blink1::%s", hdev->uniq + 4);
-	led->name = led_name;
-	led->brightness_set = blink1_led_set;
-	led->brightness_get = blink1_led_get;
-	ret = led_classdev_register(&hdev->dev, led);
-	if (ret)
+	mutex_init(&tdev->lock);
+
+	err = thingm_version(tdev);
+	if (err)
 		goto stop;
 
-	ret = sysfs_create_group(&led->dev->kobj, &blink1_sysfs_group);
-	if (ret)
-		goto remove_led;
+	hid_dbg(hdev, "firmware version: %c.%c\n",
+			tdev->version.major, tdev->version.minor);
 
-	return 0;
+	for (i = 0; i < ARRAY_SIZE(thingm_fwinfo) && !tdev->fwinfo; ++i)
+		if (thingm_fwinfo[i].major == tdev->version.major)
+			tdev->fwinfo = &thingm_fwinfo[i];
+
+	if (!tdev->fwinfo) {
+		hid_err(hdev, "unsupported firmware %c\n", tdev->version.major);
+		goto stop;
+	}
+
+	tdev->rgb = devm_kzalloc(&hdev->dev,
+			sizeof(struct thingm_rgb) * tdev->fwinfo->numrgb,
+			GFP_KERNEL);
+	if (!tdev->rgb) {
+		err = -ENOMEM;
+		goto stop;
+	}
+
+	for (i = 0; i < tdev->fwinfo->numrgb; ++i) {
+		struct thingm_rgb *rgb = tdev->rgb + i;
+
+		rgb->tdev = tdev;
+		rgb->num = tdev->fwinfo->first + i;
+		err = thingm_init_rgb(rgb);
+		if (err) {
+			while (--i >= 0)
+				thingm_remove_rgb(tdev->rgb + i);
+			goto stop;
+		}
+	}
 
-remove_led:
-	led_classdev_unregister(led);
+	return 0;
 stop:
 	hid_hw_stop(hdev);
 error:
-	return ret;
+	return err;
 }
 
 static void thingm_remove(struct hid_device *hdev)
 {
-	struct blink1_data *data = hid_get_drvdata(hdev);
-	struct led_classdev *led = &data->led_cdev;
+	struct thingm_device *tdev = hid_get_drvdata(hdev);
+	int i;
+
+	for (i = 0; i < tdev->fwinfo->numrgb; ++i)
+		thingm_remove_rgb(tdev->rgb + i);
 
-	sysfs_remove_group(&led->dev->kobj, &blink1_sysfs_group);
-	led_classdev_unregister(led);
 	hid_hw_stop(hdev);
 }
 
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 72156c1..46c7d79 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -479,6 +479,8 @@ config LEDS_OT200
 	  This option enables support for the LEDs on the Bachmann OT200.
 	  Say Y to enable LEDs on the Bachmann OT200.
 
+comment "LED driver for blink(1) USB RGB LED is under Special HID drivers (HID_THINGM)"
+
 config LEDS_BLINKM
 	tristate "LED support for the BlinkM I2C RGB LED"
 	depends on LEDS_CLASS
-- 
1.9.1

[PATCH 4/4] HID: (thingm) add support for blink(1) mk2

[Vivien Didelot]

The blink(1) mk2 is a new version of the blink(1) USB RGB LED. The new
generation has 2 individually-controllable RGB chips.

This patch adds support for this device to the thingm driver, which
registers 3 new standard LED class instances for the second RGB chip.

Note that the 'n' (set) command does not support setting a color for a
single RGB chip, so it was changed to 'c' (fade) with a timeout of 0.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 drivers/hid/hid-thingm.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/hid-thingm.c b/drivers/hid/hid-thingm.c
index 0af0eb4..31de890 100644
--- a/drivers/hid/hid-thingm.c
+++ b/drivers/hid/hid-thingm.c
@@ -23,6 +23,7 @@
 
 /* Firmware major number of supported devices */
 #define THINGM_MAJOR_MK1	'1'
+#define THINGM_MAJOR_MK2	'2'
 
 struct thingm_fwinfo {
 	char major;
@@ -35,6 +36,10 @@ const struct thingm_fwinfo thingm_fwinfo[] = {
 		.major = THINGM_MAJOR_MK1,
 		.numrgb = 1,
 		.first = 0,
+	}, {
+		.major = THINGM_MAJOR_MK2,
+		.numrgb = 2,
+		.first = 1,
 	}
 };
 
@@ -117,7 +122,7 @@ static int thingm_version(struct thingm_device *tdev)
 
 static int thingm_write_color(struct thingm_rgb *rgb)
 {
-	u8 buf[REPORT_SIZE] = { REPORT_ID, 'n', 0, 0, 0, 0, 0, 0, 0 };
+	u8 buf[REPORT_SIZE] = { REPORT_ID, 'c', 0, 0, 0, 0, 0, rgb->num, 0 };
 
 	buf[2] = rgb->red.ldev.brightness;
 	buf[3] = rgb->green.ldev.brightness;
-- 
1.9.1

Re: [PATCH 0/4] HID: (thingm) introduces blink(1) mk2

[Jiri Kosina]

On Mon, 14 Apr 2014, Vivien Didelot wrote:

> This patchset introduces the support of the blink(1) mk2 device, after a
> refactoring of the thingm driver. It removes non-standard sysfs attributes and
> uses work queues to fix the usage of triggers with this device.

Queued for 3.16. Thanks,

-- 
Jiri Kosina
SUSE Labs