[PATCH RESEND] Add multicolor support to BlinkM LED driver

[PATCH RESEND] Add multicolor support to BlinkM LED driver

[Joseph Strauss]

Added multicolor support to the BlinkM driver, making it easier to
control from userspace. The BlinkM LED is a programmable RGB LED. The
driver currently supports only the regular LED sysfs class, resulting in
the creation of three distinct classes, one for red, green, and blue.
The user then has to input three values into the three seperate
brightness files within those classes. The multicolor LED framework
makes the device easier to control with the multi_intensity file: the
user can input three values at once to form a color, while still
controlling the lightness with the brightness file.

The main struct blinkm_led has changed slightly. A struct
led_classdev_mc has been added to represent the multicolor sysfs class,
and an additional struct led_classdev pointer has been added for
convenience, which points to the struct led_classdev within struct
led_classdev_mc. The struct led_classdev for the regular sysfs classes
remain. Additionally, a field representing the multicolor LED has been
added to the struct blinkm_data, seperate from the blinkm_leds[3] array.

In the blinkm_probe function, the multicolor LED class is registered
after the regular LED classes. The blinkm_set_brightness_mc() function
had to be added to calculate the three color components and then set the
fields of the blinkm_data structure accordingly.

Signed-off-by: Joseph Strauss <jstrauss16@proton.me>

---
 Documentation/leds/leds-blinkm.rst |  24 ++++-
 drivers/leds/Kconfig               |   1 +
 drivers/leds/leds-blinkm.c         | 154 ++++++++++++++++++++++++-----
 3 files changed, 151 insertions(+), 28 deletions(-)

diff --git a/Documentation/leds/leds-blinkm.rst b/Documentation/leds/leds-blinkm.rst
index c74b5bc877b1..3f5dbd5e97b0 100644
--- a/Documentation/leds/leds-blinkm.rst
+++ b/Documentation/leds/leds-blinkm.rst
@@ -13,9 +13,27 @@ The device accepts RGB and HSB color values through separate commands.
 Also you can store blinking sequences as "scripts" in
 the controller and run them. Also fading is an option.

-The interface this driver provides is 2-fold:
+The interface this driver provides is 3-fold:

-a) LED class interface for use with triggers
+a) LED multicolor class interface for use with triggers
+#######################################################
+
+The registration follows the scheme::
+
+  blinkm-<i2c-bus-nr>-<i2c-device-nr>-multi
+
+  $ ls -h /sys/class/leds/blinkm-1-9-multi
+  brightness  device  max_brightness  multi_index  multi_intensity  power  subsystem  trigger  uevent
+
+The order in which to write the intensity values can be found in multi_index.
+Exactly three values between 0 and 255 must be written to multi_intensity to change the color::
+
+  $ echo 255 100 50 > multi_intensity
+
+The overall brightness of the color that you choose can also be changed by
+writing a value between 0 and 255 to the brightness file.
+
+b) LED class interface for use with triggers
 ############################################

 The registration follows the scheme::
@@ -50,7 +68,7 @@ E.g.::
   $


-b) Sysfs group to control rgb, fade, hsb, scripts ...
+c) Sysfs group to control rgb, fade, hsb, scripts ...
 #####################################################

 This extended interface is available as folder blinkm
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index b6742b4231bf..6f73deb7d95c 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -787,6 +787,7 @@ comment "LED driver for blink(1) USB RGB LED is under Special HID drivers (HID_T
 config LEDS_BLINKM
 	tristate "LED support for the BlinkM I2C RGB LED"
 	depends on LEDS_CLASS
+	depends on LEDS_CLASS_MULTICOLOR
 	depends on I2C
 	help
 	  This option enables support for the BlinkM RGB LED connected
diff --git a/drivers/leds/leds-blinkm.c b/drivers/leds/leds-blinkm.c
index b4e1fdff4186..a78bcc2eaff3 100644
--- a/drivers/leds/leds-blinkm.c
+++ b/drivers/leds/leds-blinkm.c
@@ -15,6 +15,9 @@
 #include <linux/pm_runtime.h>
 #include <linux/leds.h>
 #include <linux/delay.h>
+#include <linux/led-class-multicolor.h>
+
+#define NUM_LEDS 3

 /* Addresses to scan - BlinkM is on 0x09 by default*/
 static const unsigned short normal_i2c[] = { 0x09, I2C_CLIENT_END };
@@ -22,19 +25,26 @@ static const unsigned short normal_i2c[] = { 0x09, I2C_CLIENT_END };
 static int blinkm_transfer_hw(struct i2c_client *client, int cmd);
 static int blinkm_test_run(struct i2c_client *client);

+/* Contains data structures for both the color-seperated sysfs classes, and the new multicolor class */
 struct blinkm_led {
 	struct i2c_client *i2c_client;
-	struct led_classdev led_cdev;
+	struct led_classdev monochrome_led_cdev;
+	/* points to struct led_classdev inside of struct led_classdev_mc */
+	struct led_classdev *led_cdev;
+	struct led_classdev_mc mcled_cdev;
 	int id;
 };

-#define cdev_to_blmled(c)          container_of(c, struct blinkm_led, led_cdev)
+#define monochrome_led_cdev_to_blmled(c)	container_of(c, struct blinkm_led, monochrome_led_cdev)
+#define mcled_cdev_to_led(c)				container_of(c, struct blinkm_led, mcled_cdev)

 struct blinkm_data {
 	struct i2c_client *i2c_client;
 	struct mutex update_lock;
 	/* used for led class interface */
+	struct blinkm_led mc_blinkm_led;
 	struct blinkm_led blinkm_leds[3];
+
 	/* used for "blinkm" sysfs interface */
 	u8 red;			/* color red */
 	u8 green;		/* color green */
@@ -260,9 +270,9 @@ static ssize_t test_show(struct device *dev, struct device_attribute *attr,
 static ssize_t test_store(struct device *dev, struct device_attribute *attr,
 			  const char *buf, size_t count)
 {
-
 	struct i2c_client *client;
 	int ret;
+
 	client = to_i2c_client(dev);

 	/*test */
@@ -317,6 +327,7 @@ static int blinkm_read(struct i2c_client *client, int cmd, u8 *arg)
 	int result;
 	int i;
 	int retlen = blinkm_cmds[cmd].nr_ret;
+
 	for (i = 0; i < retlen; i++) {
 		/* repeat for retlen */
 		result = i2c_smbus_read_byte(client);
@@ -419,11 +430,53 @@ static int blinkm_transfer_hw(struct i2c_client *client, int cmd)
 	return 0;
 }

+static int blinkm_set_mc_brightness(struct led_classdev *led_cdev,
+				 enum led_brightness value)
+{
+	struct led_classdev_mc *mcled_cdev;
+	struct blinkm_led *led;
+	struct blinkm_data *data;
+	int i;
+
+	mcled_cdev = lcdev_to_mccdev(led_cdev);
+	led = mcled_cdev_to_led(mcled_cdev);
+	data = i2c_get_clientdata(led->i2c_client);
+
+	led_mc_calc_color_components(mcled_cdev, value);
+
+	for (i = 0; i < NUM_LEDS; i++) {
+		switch (i) {
+		case RED:
+			if (data->next_red == (u8) mcled_cdev->subled_info[i].brightness)
+				break;
+			data->next_red = (u8) mcled_cdev->subled_info[i].brightness;
+			break;
+		case GREEN:
+			if (data->next_green == (u8) mcled_cdev->subled_info[i].brightness)
+				break;
+			data->next_green = (u8) mcled_cdev->subled_info[i].brightness;
+			break;
+		case BLUE:
+			if (data->next_blue == (u8) mcled_cdev->subled_info[i].brightness)
+				break;
+			data->next_blue = (u8) mcled_cdev->subled_info[i].brightness;
+			break;
+		}
+	}
+	blinkm_transfer_hw(led->i2c_client, BLM_GO_RGB);
+	dev_dbg(&led->i2c_client->dev,
+			"# DONE # next_red = %d, next_green = %d,"
+			" next_blue = %d\n",
+			data->next_red, data->next_green,
+			data->next_blue);
+	return 0;
+}
+
 static int blinkm_led_common_set(struct led_classdev *led_cdev,
 				 enum led_brightness value, int color)
 {
 	/* led_brightness is 0, 127 or 255 - we just use it here as-is */
-	struct blinkm_led *led = cdev_to_blmled(led_cdev);
+	struct blinkm_led *led = monochrome_led_cdev_to_blmled(led_cdev);
 	struct blinkm_data *data = i2c_get_clientdata(led->i2c_client);

 	switch (color) {
@@ -570,7 +623,11 @@ static int blinkm_probe(struct i2c_client *client,
 			const struct i2c_device_id *id)
 {
 	struct blinkm_data *data;
-	struct blinkm_led *led[3];
+	/* For multicolor support */
+	struct blinkm_led *mc_led;
+	struct mc_subled *mc_led_info;
+	/* 3 seperate classes for red, green, and blue respectively */
+	struct blinkm_led *leds[3];
 	int err, i;
 	char blinkm_led_name[28];

@@ -581,6 +638,12 @@ static int blinkm_probe(struct i2c_client *client,
 		goto exit;
 	}

+	mc_led_info = devm_kmalloc_array(&client->dev, 3, sizeof(*mc_led_info),
+					GFP_KERNEL | __GFP_ZERO);
+	if (!mc_led_info) {
+		err = -ENOMEM;
+		goto exit;
+	}
 	data->i2c_addr = 0x08;
 	/* i2c addr  - use fake addr of 0x08 initially (real is 0x09) */
 	data->fw_ver = 0xfe;
@@ -599,28 +662,30 @@ static int blinkm_probe(struct i2c_client *client,
 		goto exit;
 	}

+
+	/* Register red, green, and blue sysfs classes */
 	for (i = 0; i < 3; i++) {
 		/* RED = 0, GREEN = 1, BLUE = 2 */
-		led[i] = &data->blinkm_leds[i];
-		led[i]->i2c_client = client;
-		led[i]->id = i;
-		led[i]->led_cdev.max_brightness = 255;
-		led[i]->led_cdev.flags = LED_CORE_SUSPENDRESUME;
+		leds[i] = &data->blinkm_leds[i];
+		leds[i]->i2c_client = client;
+		leds[i]->id = i;
+		leds[i]->monochrome_led_cdev.max_brightness = 255;
+		leds[i]->monochrome_led_cdev.flags = LED_CORE_SUSPENDRESUME;
 		switch (i) {
 		case RED:
 			snprintf(blinkm_led_name, sizeof(blinkm_led_name),
 					 "blinkm-%d-%d-red",
 					 client->adapter->nr,
 					 client->addr);
-			led[i]->led_cdev.name = blinkm_led_name;
-			led[i]->led_cdev.brightness_set_blocking =
+			leds[i]->monochrome_led_cdev.name = blinkm_led_name;
+			leds[i]->monochrome_led_cdev.brightness_set_blocking =
 							blinkm_led_red_set;
 			err = led_classdev_register(&client->dev,
-						    &led[i]->led_cdev);
+							&leds[i]->monochrome_led_cdev);
 			if (err < 0) {
 				dev_err(&client->dev,
 					"couldn't register LED %s\n",
-					led[i]->led_cdev.name);
+					leds[i]->monochrome_led_cdev.name);
 				goto failred;
 			}
 			break;
@@ -629,15 +694,15 @@ static int blinkm_probe(struct i2c_client *client,
 					 "blinkm-%d-%d-green",
 					 client->adapter->nr,
 					 client->addr);
-			led[i]->led_cdev.name = blinkm_led_name;
-			led[i]->led_cdev.brightness_set_blocking =
+			leds[i]->monochrome_led_cdev.name = blinkm_led_name;
+			leds[i]->monochrome_led_cdev.brightness_set_blocking =
 							blinkm_led_green_set;
 			err = led_classdev_register(&client->dev,
-						    &led[i]->led_cdev);
+							&leds[i]->monochrome_led_cdev);
 			if (err < 0) {
 				dev_err(&client->dev,
 					"couldn't register LED %s\n",
-					led[i]->led_cdev.name);
+					leds[i]->monochrome_led_cdev.name);
 				goto failgreen;
 			}
 			break;
@@ -646,34 +711,72 @@ static int blinkm_probe(struct i2c_client *client,
 					 "blinkm-%d-%d-blue",
 					 client->adapter->nr,
 					 client->addr);
-			led[i]->led_cdev.name = blinkm_led_name;
-			led[i]->led_cdev.brightness_set_blocking =
+			leds[i]->monochrome_led_cdev.name = blinkm_led_name;
+			leds[i]->monochrome_led_cdev.brightness_set_blocking =
 							blinkm_led_blue_set;
 			err = led_classdev_register(&client->dev,
-						    &led[i]->led_cdev);
+							&leds[i]->monochrome_led_cdev);
 			if (err < 0) {
 				dev_err(&client->dev,
 					"couldn't register LED %s\n",
-					led[i]->led_cdev.name);
+					leds[i]->monochrome_led_cdev.name);
 				goto failblue;
 			}
 			break;
 		}		/* end switch */
 	}			/* end for */

+
+
+	/* Register multicolor sysfs class */
+	mc_led = &data->mc_blinkm_led;
+	mc_led->i2c_client = client;
+	mc_led->id = 4;
+
+	mc_led_info[0].color_index = LED_COLOR_ID_RED;
+	mc_led_info[0].channel = 0;
+	mc_led_info[1].color_index = LED_COLOR_ID_GREEN;
+	mc_led_info[1].channel = 1;
+	mc_led_info[2].color_index = LED_COLOR_ID_BLUE;
+	mc_led_info[2].channel = 2;
+	mc_led->mcled_cdev.subled_info = mc_led_info;
+	mc_led->mcled_cdev.num_colors = NUM_LEDS;
+
+	mc_led->led_cdev = &mc_led->mcled_cdev.led_cdev;
+	mc_led->led_cdev->brightness = 255;
+	mc_led->led_cdev->max_brightness = 255;
+	mc_led->led_cdev->flags = LED_CORE_SUSPENDRESUME;
+	snprintf(blinkm_led_name, sizeof(blinkm_led_name),
+		 "blinkm-%d-%d-multi",
+		 client->adapter->nr,
+		 client->addr);
+	mc_led->led_cdev->name = blinkm_led_name;
+	mc_led->led_cdev->brightness_set_blocking =
+					blinkm_set_mc_brightness;
+
+	err = led_classdev_multicolor_register(&client->dev, &mc_led->mcled_cdev);
+	if (err < 0) {
+		dev_err(&client->dev, "couldn't register LED %s\n",
+				mc_led->led_cdev->name);
+		goto failmulti;
+	}
 	/* Initialize the blinkm */
 	blinkm_init_hw(client);

 	return 0;

+failmulti:
+	led_classdev_unregister(&leds[BLUE]->monochrome_led_cdev);
+
 failblue:
-	led_classdev_unregister(&led[GREEN]->led_cdev);
+	led_classdev_unregister(&leds[GREEN]->monochrome_led_cdev);

 failgreen:
-	led_classdev_unregister(&led[RED]->led_cdev);
+	led_classdev_unregister(&leds[RED]->monochrome_led_cdev);

 failred:
 	sysfs_remove_group(&client->dev.kobj, &blinkm_group);
+
 exit:
 	return err;
 }
@@ -685,8 +788,9 @@ static int blinkm_remove(struct i2c_client *client)
 	int i;

 	/* make sure no workqueue entries are pending */
+	led_classdev_unregister(&data->mc_blinkm_led.mcled_cdev.led_cdev);
 	for (i = 0; i < 3; i++)
-		led_classdev_unregister(&data->blinkm_leds[i].led_cdev);
+		led_classdev_unregister(&data->blinkm_leds[i].monochrome_led_cdev);

 	/* reset rgb */
 	data->next_red = 0x00;
--
2.38.1

Re: [PATCH RESEND] Add multicolor support to BlinkM LED driver

[Christophe JAILLET]

Le 14/12/2022 à 23:25, Joseph Strauss a écrit :
> Added multicolor support to the BlinkM driver, making it easier to
> control from userspace. The BlinkM LED is a programmable RGB LED. The
> driver currently supports only the regular LED sysfs class, resulting in
> the creation of three distinct classes, one for red, green, and blue.
> The user then has to input three values into the three seperate
> brightness files within those classes. The multicolor LED framework
> makes the device easier to control with the multi_intensity file: the
> user can input three values at once to form a color, while still
> controlling the lightness with the brightness file.
> 
> The main struct blinkm_led has changed slightly. A struct
> led_classdev_mc has been added to represent the multicolor sysfs class,
> and an additional struct led_classdev pointer has been added for
> convenience, which points to the struct led_classdev within struct
> led_classdev_mc. The struct led_classdev for the regular sysfs classes
> remain. Additionally, a field representing the multicolor LED has been
> added to the struct blinkm_data, seperate from the blinkm_leds[3] array.
> 
> In the blinkm_probe function, the multicolor LED class is registered
> after the regular LED classes. The blinkm_set_brightness_mc() function
> had to be added to calculate the three color components and then set the
> fields of the blinkm_data structure accordingly.
> 
> Signed-off-by: Joseph Strauss <jstrauss16@proton.me>

Hi,

a few nits below, should it help.

CJ

> 
> ---
>   Documentation/leds/leds-blinkm.rst |  24 ++++-
>   drivers/leds/Kconfig               |   1 +
>   drivers/leds/leds-blinkm.c         | 154 ++++++++++++++++++++++++-----
>   3 files changed, 151 insertions(+), 28 deletions(-)
> 
> diff --git a/Documentation/leds/leds-blinkm.rst b/Documentation/leds/leds-blinkm.rst
> index c74b5bc877b1..3f5dbd5e97b0 100644
> --- a/Documentation/leds/leds-blinkm.rst
> +++ b/Documentation/leds/leds-blinkm.rst
> @@ -13,9 +13,27 @@ The device accepts RGB and HSB color values through separate commands.
>   Also you can store blinking sequences as "scripts" in
>   the controller and run them. Also fading is an option.
> 
> -The interface this driver provides is 2-fold:
> +The interface this driver provides is 3-fold:
> 
> -a) LED class interface for use with triggers
> +a) LED multicolor class interface for use with triggers
> +#######################################################
> +
> +The registration follows the scheme::
> +
> +  blinkm-<i2c-bus-nr>-<i2c-device-nr>-multi
> +
> +  $ ls -h /sys/class/leds/blinkm-1-9-multi
> +  brightness  device  max_brightness  multi_index  multi_intensity  power  subsystem  trigger  uevent
> +
> +The order in which to write the intensity values can be found in multi_index.
> +Exactly three values between 0 and 255 must be written to multi_intensity to change the color::
> +
> +  $ echo 255 100 50 > multi_intensity
> +
> +The overall brightness of the color that you choose can also be changed by
> +writing a value between 0 and 255 to the brightness file.
> +
> +b) LED class interface for use with triggers
>   ############################################
> 
>   The registration follows the scheme::
> @@ -50,7 +68,7 @@ E.g.::
>     $
> 
> 
> -b) Sysfs group to control rgb, fade, hsb, scripts ...
> +c) Sysfs group to control rgb, fade, hsb, scripts ...
>   #####################################################
> 
>   This extended interface is available as folder blinkm
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index b6742b4231bf..6f73deb7d95c 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -787,6 +787,7 @@ comment "LED driver for blink(1) USB RGB LED is under Special HID drivers (HID_T
>   config LEDS_BLINKM
>   	tristate "LED support for the BlinkM I2C RGB LED"
>   	depends on LEDS_CLASS
> +	depends on LEDS_CLASS_MULTICOLOR
>   	depends on I2C
>   	help
>   	  This option enables support for the BlinkM RGB LED connected
> diff --git a/drivers/leds/leds-blinkm.c b/drivers/leds/leds-blinkm.c
> index b4e1fdff4186..a78bcc2eaff3 100644
> --- a/drivers/leds/leds-blinkm.c
> +++ b/drivers/leds/leds-blinkm.c
> @@ -15,6 +15,9 @@
>   #include <linux/pm_runtime.h>
>   #include <linux/leds.h>
>   #include <linux/delay.h>
> +#include <linux/led-class-multicolor.h>
> +
> +#define NUM_LEDS 3
> 
>   /* Addresses to scan - BlinkM is on 0x09 by default*/
>   static const unsigned short normal_i2c[] = { 0x09, I2C_CLIENT_END };
> @@ -22,19 +25,26 @@ static const unsigned short normal_i2c[] = { 0x09, I2C_CLIENT_END };
>   static int blinkm_transfer_hw(struct i2c_client *client, int cmd);
>   static int blinkm_test_run(struct i2c_client *client);
> 
> +/* Contains data structures for both the color-seperated sysfs classes, and the new multicolor class */
>   struct blinkm_led {
>   	struct i2c_client *i2c_client;
> -	struct led_classdev led_cdev;
> +	struct led_classdev monochrome_led_cdev;
> +	/* points to struct led_classdev inside of struct led_classdev_mc */
> +	struct led_classdev *led_cdev;
> +	struct led_classdev_mc mcled_cdev;
>   	int id;
>   };
> 
> -#define cdev_to_blmled(c)          container_of(c, struct blinkm_led, led_cdev)
> +#define monochrome_led_cdev_to_blmled(c)	container_of(c, struct blinkm_led, monochrome_led_cdev)
> +#define mcled_cdev_to_led(c)				container_of(c, struct blinkm_led, mcled_cdev)
> 
>   struct blinkm_data {
>   	struct i2c_client *i2c_client;
>   	struct mutex update_lock;
>   	/* used for led class interface */
> +	struct blinkm_led mc_blinkm_led;
>   	struct blinkm_led blinkm_leds[3];

s/3/NUM_LEDS/ ?

> +
>   	/* used for "blinkm" sysfs interface */
>   	u8 red;			/* color red */
>   	u8 green;		/* color green */
> @@ -260,9 +270,9 @@ static ssize_t test_show(struct device *dev, struct device_attribute *attr,
>   static ssize_t test_store(struct device *dev, struct device_attribute *attr,
>   			  const char *buf, size_t count)
>   {
> -
>   	struct i2c_client *client;
>   	int ret;
> +

Such things should be part of another patch, because it is unrelated.

>   	client = to_i2c_client(dev);
> 
>   	/*test */
> @@ -317,6 +327,7 @@ static int blinkm_read(struct i2c_client *client, int cmd, u8 *arg)
>   	int result;
>   	int i;
>   	int retlen = blinkm_cmds[cmd].nr_ret;
> +

Ditto

>   	for (i = 0; i < retlen; i++) {
>   		/* repeat for retlen */
>   		result = i2c_smbus_read_byte(client);
> @@ -419,11 +430,53 @@ static int blinkm_transfer_hw(struct i2c_client *client, int cmd)
>   	return 0;
>   }
> 
> +static int blinkm_set_mc_brightness(struct led_classdev *led_cdev,
> +				 enum led_brightness value)
> +{
> +	struct led_classdev_mc *mcled_cdev;
> +	struct blinkm_led *led;
> +	struct blinkm_data *data;
> +	int i;
> +
> +	mcled_cdev = lcdev_to_mccdev(led_cdev);
> +	led = mcled_cdev_to_led(mcled_cdev);
> +	data = i2c_get_clientdata(led->i2c_client);
> +
> +	led_mc_calc_color_components(mcled_cdev, value);
> +
> +	for (i = 0; i < NUM_LEDS; i++) {
> +		switch (i) {
> +		case RED:
> +			if (data->next_red == (u8) mcled_cdev->subled_info[i].brightness)
> +				break;
> +			data->next_red = (u8) mcled_cdev->subled_info[i].brightness;
> +			break;
> +		case GREEN:
> +			if (data->next_green == (u8) mcled_cdev->subled_info[i].brightness)
> +				break;
> +			data->next_green = (u8) mcled_cdev->subled_info[i].brightness;
> +			break;
> +		case BLUE:
> +			if (data->next_blue == (u8) mcled_cdev->subled_info[i].brightness)
> +				break;
> +			data->next_blue = (u8) mcled_cdev->subled_info[i].brightness;
> +			break;
> +		}
> +	}

Does it really worth all these LoC?

Is this enough?
	data->next_red = (u8)mcled_cdev->subled_info[RED].brightness;
	data->next_green = (u8)mcled_cdev->subled_info[GREEN].brightness;
	data->next_blue = (u8)mcled_cdev->subled_info[BLUE].brightness;

> +	blinkm_transfer_hw(led->i2c_client, BLM_GO_RGB);
> +	dev_dbg(&led->i2c_client->dev,
> +			"# DONE # next_red = %d, next_green = %d,"
> +			" next_blue = %d\n",

Keep the string on the same line.

> +			data->next_red, data->next_green,
> +			data->next_blue);
> +	return 0;
> +}
> +
>   static int blinkm_led_common_set(struct led_classdev *led_cdev,
>   				 enum led_brightness value, int color)
>   {
>   	/* led_brightness is 0, 127 or 255 - we just use it here as-is */
> -	struct blinkm_led *led = cdev_to_blmled(led_cdev);
> +	struct blinkm_led *led = monochrome_led_cdev_to_blmled(led_cdev);
>   	struct blinkm_data *data = i2c_get_clientdata(led->i2c_client);
> 
>   	switch (color) {
> @@ -570,7 +623,11 @@ static int blinkm_probe(struct i2c_client *client,
>   			const struct i2c_device_id *id)
>   {
>   	struct blinkm_data *data;
> -	struct blinkm_led *led[3];
> +	/* For multicolor support */
> +	struct blinkm_led *mc_led;
> +	struct mc_subled *mc_led_info;
> +	/* 3 seperate classes for red, green, and blue respectively */
> +	struct blinkm_led *leds[3];

s/3/NUM_LEDS/ ?

>   	int err, i;
>   	char blinkm_led_name[28];
> 
> @@ -581,6 +638,12 @@ static int blinkm_probe(struct i2c_client *client,
>   		goto exit;
>   	}
> 
> +	mc_led_info = devm_kmalloc_array(&client->dev, 3, sizeof(*mc_led_info),

s/3/NUM_LEDS/ ?
s/devm_kmalloc_array(...|__GFP_ZERO)/devm_kcalloc/ ?

> +					GFP_KERNEL | __GFP_ZERO);
> +	if (!mc_led_info) {
> +		err = -ENOMEM;
> +		goto exit;
> +	}
>   	data->i2c_addr = 0x08;
>   	/* i2c addr  - use fake addr of 0x08 initially (real is 0x09) */
>   	data->fw_ver = 0xfe;
> @@ -599,28 +662,30 @@ static int blinkm_probe(struct i2c_client *client,
>   		goto exit;
>   	}
> 
> +
> +	/* Register red, green, and blue sysfs classes */
>   	for (i = 0; i < 3; i++) {

s/3/NUM_LEDS/?

>   		/* RED = 0, GREEN = 1, BLUE = 2 */
> -		led[i] = &data->blinkm_leds[i];
> -		led[i]->i2c_client = client;
> -		led[i]->id = i;
> -		led[i]->led_cdev.max_brightness = 255;
> -		led[i]->led_cdev.flags = LED_CORE_SUSPENDRESUME;
> +		leds[i] = &data->blinkm_leds[i];
> +		leds[i]->i2c_client = client;
> +		leds[i]->id = i;
> +		leds[i]->monochrome_led_cdev.max_brightness = 255;
> +		leds[i]->monochrome_led_cdev.flags = LED_CORE_SUSPENDRESUME;
>   		switch (i) {
>   		case RED:
>   			snprintf(blinkm_led_name, sizeof(blinkm_led_name),
>   					 "blinkm-%d-%d-red",
>   					 client->adapter->nr,
>   					 client->addr);
> -			led[i]->led_cdev.name = blinkm_led_name;
> -			led[i]->led_cdev.brightness_set_blocking =
> +			leds[i]->monochrome_led_cdev.name = blinkm_led_name;
> +			leds[i]->monochrome_led_cdev.brightness_set_blocking =
>   							blinkm_led_red_set;
>   			err = led_classdev_register(&client->dev,
> -						    &led[i]->led_cdev);
> +							&leds[i]->monochrome_led_cdev);
>   			if (err < 0) {
>   				dev_err(&client->dev,
>   					"couldn't register LED %s\n",
> -					led[i]->led_cdev.name);
> +					leds[i]->monochrome_led_cdev.name);
>   				goto failred;
>   			}
>   			break;
> @@ -629,15 +694,15 @@ static int blinkm_probe(struct i2c_client *client,
>   					 "blinkm-%d-%d-green",
>   					 client->adapter->nr,
>   					 client->addr);
> -			led[i]->led_cdev.name = blinkm_led_name;
> -			led[i]->led_cdev.brightness_set_blocking =
> +			leds[i]->monochrome_led_cdev.name = blinkm_led_name;
> +			leds[i]->monochrome_led_cdev.brightness_set_blocking =
>   							blinkm_led_green_set;
>   			err = led_classdev_register(&client->dev,
> -						    &led[i]->led_cdev);
> +							&leds[i]->monochrome_led_cdev);

Keep the original indentation.

>   			if (err < 0) {
>   				dev_err(&client->dev,
>   					"couldn't register LED %s\n",
> -					led[i]->led_cdev.name);
> +					leds[i]->monochrome_led_cdev.name);
>   				goto failgreen;
>   			}
>   			break;
> @@ -646,34 +711,72 @@ static int blinkm_probe(struct i2c_client *client,
>   					 "blinkm-%d-%d-blue",
>   					 client->adapter->nr,
>   					 client->addr);
> -			led[i]->led_cdev.name = blinkm_led_name;
> -			led[i]->led_cdev.brightness_set_blocking =
> +			leds[i]->monochrome_led_cdev.name = blinkm_led_name;
> +			leds[i]->monochrome_led_cdev.brightness_set_blocking =
>   							blinkm_led_blue_set;
>   			err = led_classdev_register(&client->dev,
> -						    &led[i]->led_cdev);
> +							&leds[i]->monochrome_led_cdev);

Keep the original indentation.

>   			if (err < 0) {
>   				dev_err(&client->dev,
>   					"couldn't register LED %s\n",
> -					led[i]->led_cdev.name);
> +					leds[i]->monochrome_led_cdev.name);
>   				goto failblue;
>   			}
>   			break;
>   		}		/* end switch */
>   	}			/* end for */
> 
> +
> +

One NL is enough.

> +	/* Register multicolor sysfs class */
> +	mc_led = &data->mc_blinkm_led;
> +	mc_led->i2c_client = client;
> +	mc_led->id = 4;

Why 4?
NUM_LEDS+1?

> +
> +	mc_led_info[0].color_index = LED_COLOR_ID_RED;
> +	mc_led_info[0].channel = 0;
> +	mc_led_info[1].color_index = LED_COLOR_ID_GREEN;
> +	mc_led_info[1].channel = 1;
> +	mc_led_info[2].color_index = LED_COLOR_ID_BLUE;
> +	mc_led_info[2].channel = 2;
> +	mc_led->mcled_cdev.subled_info = mc_led_info;
> +	mc_led->mcled_cdev.num_colors = NUM_LEDS;
> +
> +	mc_led->led_cdev = &mc_led->mcled_cdev.led_cdev;
> +	mc_led->led_cdev->brightness = 255;
> +	mc_led->led_cdev->max_brightness = 255;
> +	mc_led->led_cdev->flags = LED_CORE_SUSPENDRESUME;
> +	snprintf(blinkm_led_name, sizeof(blinkm_led_name),
> +		 "blinkm-%d-%d-multi",
> +		 client->adapter->nr,
> +		 client->addr);
> +	mc_led->led_cdev->name = blinkm_led_name;
> +	mc_led->led_cdev->brightness_set_blocking =
> +					blinkm_set_mc_brightness;
> +
> +	err = led_classdev_multicolor_register(&client->dev, &mc_led->mcled_cdev);
> +	if (err < 0) {
> +		dev_err(&client->dev, "couldn't register LED %s\n",
> +				mc_led->led_cdev->name);
> +		goto failmulti;
> +	}
>   	/* Initialize the blinkm */
>   	blinkm_init_hw(client);
> 
>   	return 0;
> 
> +failmulti:
> +	led_classdev_unregister(&leds[BLUE]->monochrome_led_cdev);
> +
>   failblue:
> -	led_classdev_unregister(&led[GREEN]->led_cdev);
> +	led_classdev_unregister(&leds[GREEN]->monochrome_led_cdev);
> 
>   failgreen:
> -	led_classdev_unregister(&led[RED]->led_cdev);
> +	led_classdev_unregister(&leds[RED]->monochrome_led_cdev);
> 
>   failred:
>   	sysfs_remove_group(&client->dev.kobj, &blinkm_group);
> +
>   exit:
>   	return err;
>   }
> @@ -685,8 +788,9 @@ static int blinkm_remove(struct i2c_client *client)
>   	int i;
> 
>   	/* make sure no workqueue entries are pending */
> +	led_classdev_unregister(&data->mc_blinkm_led.mcled_cdev.led_cdev);
>   	for (i = 0; i < 3; i++)

s/3/NUM_LEDS/?

> -		led_classdev_unregister(&data->blinkm_leds[i].led_cdev);
> +		led_classdev_unregister(&data->blinkm_leds[i].monochrome_led_cdev);
> 
>   	/* reset rgb */
>   	data->next_red = 0x00;

Re: [PATCH RESEND] Add multicolor support to BlinkM LED driver

[Lee Jones]

On Wed, 14 Dec 2022, Joseph Strauss wrote:

Would you mind composing your mails such that my Key Manager doens't ask
me for a password in order to view them please?

-- 
Lee Jones [李琼斯]

Re: [PATCH RESEND] Add multicolor support to BlinkM LED driver

[Joseph Strauss]

On 22/12/15 08:31PM, Christophe JAILLET wrote:
> Le 14/12/2022 à 23:25, Joseph Strauss a écrit :
> > Added multicolor support to the BlinkM driver, making it easier to
> > control from userspace. The BlinkM LED is a programmable RGB LED. The
> > driver currently supports only the regular LED sysfs class, resulting in
> > the creation of three distinct classes, one for red, green, and blue.
> > The user then has to input three values into the three seperate
> > brightness files within those classes. The multicolor LED framework
> > makes the device easier to control with the multi_intensity file: the
> > user can input three values at once to form a color, while still
> > controlling the lightness with the brightness file.
> >
> > The main struct blinkm_led has changed slightly. A struct
> > led_classdev_mc has been added to represent the multicolor sysfs class,
> > and an additional struct led_classdev pointer has been added for
> > convenience, which points to the struct led_classdev within struct
> > led_classdev_mc. The struct led_classdev for the regular sysfs classes
> > remain. Additionally, a field representing the multicolor LED has been
> > added to the struct blinkm_data, seperate from the blinkm_leds[3] array.
> >
> > In the blinkm_probe function, the multicolor LED class is registered
> > after the regular LED classes. The blinkm_set_brightness_mc() function
> > had to be added to calculate the three color components and then set the
> > fields of the blinkm_data structure accordingly.
> >
> > Signed-off-by: Joseph Strauss <jstrauss16@proton.me>
>
> Hi,
>
> a few nits below, should it help.
>
> CJ
>
> >
> > ---
> >   Documentation/leds/leds-blinkm.rst |  24 ++++-
> >   drivers/leds/Kconfig               |   1 +
> >   drivers/leds/leds-blinkm.c         | 154 ++++++++++++++++++++++++-----
> >   3 files changed, 151 insertions(+), 28 deletions(-)
> >
> > diff --git a/Documentation/leds/leds-blinkm.rst b/Documentation/leds/leds-blinkm.rst
> > index c74b5bc877b1..3f5dbd5e97b0 100644
> > --- a/Documentation/leds/leds-blinkm.rst
> > +++ b/Documentation/leds/leds-blinkm.rst
> > @@ -13,9 +13,27 @@ The device accepts RGB and HSB color values through separate commands.
> >   Also you can store blinking sequences as "scripts" in
> >   the controller and run them. Also fading is an option.
> >
> > -The interface this driver provides is 2-fold:
> > +The interface this driver provides is 3-fold:
> >
> > -a) LED class interface for use with triggers
> > +a) LED multicolor class interface for use with triggers
> > +#######################################################
> > +
> > +The registration follows the scheme::
> > +
> > +  blinkm-<i2c-bus-nr>-<i2c-device-nr>-multi
> > +
> > +  $ ls -h /sys/class/leds/blinkm-1-9-multi
> > +  brightness  device  max_brightness  multi_index  multi_intensity  power  subsystem  trigger  uevent
> > +
> > +The order in which to write the intensity values can be found in multi_index.
> > +Exactly three values between 0 and 255 must be written to multi_intensity to change the color::
> > +
> > +  $ echo 255 100 50 > multi_intensity
> > +
> > +The overall brightness of the color that you choose can also be changed by
> > +writing a value between 0 and 255 to the brightness file.
> > +
> > +b) LED class interface for use with triggers
> >   ############################################
> >
> >   The registration follows the scheme::
> > @@ -50,7 +68,7 @@ E.g.::
> >     $
> >
> >
> > -b) Sysfs group to control rgb, fade, hsb, scripts ...
> > +c) Sysfs group to control rgb, fade, hsb, scripts ...
> >   #####################################################
> >
> >   This extended interface is available as folder blinkm
> > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> > index b6742b4231bf..6f73deb7d95c 100644
> > --- a/drivers/leds/Kconfig
> > +++ b/drivers/leds/Kconfig
> > @@ -787,6 +787,7 @@ comment "LED driver for blink(1) USB RGB LED is under Special HID drivers (HID_T
> >   config LEDS_BLINKM
> >   	tristate "LED support for the BlinkM I2C RGB LED"
> >   	depends on LEDS_CLASS
> > +	depends on LEDS_CLASS_MULTICOLOR
> >   	depends on I2C
> >   	help
> >   	  This option enables support for the BlinkM RGB LED connected
> > diff --git a/drivers/leds/leds-blinkm.c b/drivers/leds/leds-blinkm.c
> > index b4e1fdff4186..a78bcc2eaff3 100644
> > --- a/drivers/leds/leds-blinkm.c
> > +++ b/drivers/leds/leds-blinkm.c
> > @@ -15,6 +15,9 @@
> >   #include <linux/pm_runtime.h>
> >   #include <linux/leds.h>
> >   #include <linux/delay.h>
> > +#include <linux/led-class-multicolor.h>
> > +
> > +#define NUM_LEDS 3
> >
> >   /* Addresses to scan - BlinkM is on 0x09 by default*/
> >   static const unsigned short normal_i2c[] = { 0x09, I2C_CLIENT_END };
> > @@ -22,19 +25,26 @@ static const unsigned short normal_i2c[] = { 0x09, I2C_CLIENT_END };
> >   static int blinkm_transfer_hw(struct i2c_client *client, int cmd);
> >   static int blinkm_test_run(struct i2c_client *client);
> >
> > +/* Contains data structures for both the color-seperated sysfs classes, and the new multicolor class */
> >   struct blinkm_led {
> >   	struct i2c_client *i2c_client;
> > -	struct led_classdev led_cdev;
> > +	struct led_classdev monochrome_led_cdev;
> > +	/* points to struct led_classdev inside of struct led_classdev_mc */
> > +	struct led_classdev *led_cdev;
> > +	struct led_classdev_mc mcled_cdev;
> >   	int id;
> >   };
> >
> > -#define cdev_to_blmled(c)          container_of(c, struct blinkm_led, led_cdev)
> > +#define monochrome_led_cdev_to_blmled(c)	container_of(c, struct blinkm_led, monochrome_led_cdev)
> > +#define mcled_cdev_to_led(c)				container_of(c, struct blinkm_led, mcled_cdev)
> >
> >   struct blinkm_data {
> >   	struct i2c_client *i2c_client;
> >   	struct mutex update_lock;
> >   	/* used for led class interface */
> > +	struct blinkm_led mc_blinkm_led;
> >   	struct blinkm_led blinkm_leds[3];
>
> s/3/NUM_LEDS/ ?
>
> > +
> >   	/* used for "blinkm" sysfs interface */
> >   	u8 red;			/* color red */
> >   	u8 green;		/* color green */
> > @@ -260,9 +270,9 @@ static ssize_t test_show(struct device *dev, struct device_attribute *attr,
> >   static ssize_t test_store(struct device *dev, struct device_attribute *attr,
> >   			  const char *buf, size_t count)
> >   {
> > -
> >   	struct i2c_client *client;
> >   	int ret;
> > +
>
> Such things should be part of another patch, because it is unrelated.
>
> >   	client = to_i2c_client(dev);
> >
> >   	/*test */
> > @@ -317,6 +327,7 @@ static int blinkm_read(struct i2c_client *client, int cmd, u8 *arg)
> >   	int result;
> >   	int i;
> >   	int retlen = blinkm_cmds[cmd].nr_ret;
> > +
>
> Ditto
>
> >   	for (i = 0; i < retlen; i++) {
> >   		/* repeat for retlen */
> >   		result = i2c_smbus_read_byte(client);
> > @@ -419,11 +430,53 @@ static int blinkm_transfer_hw(struct i2c_client *client, int cmd)
> >   	return 0;
> >   }
> >
> > +static int blinkm_set_mc_brightness(struct led_classdev *led_cdev,
> > +				 enum led_brightness value)
> > +{
> > +	struct led_classdev_mc *mcled_cdev;
> > +	struct blinkm_led *led;
> > +	struct blinkm_data *data;
> > +	int i;
> > +
> > +	mcled_cdev = lcdev_to_mccdev(led_cdev);
> > +	led = mcled_cdev_to_led(mcled_cdev);
> > +	data = i2c_get_clientdata(led->i2c_client);
> > +
> > +	led_mc_calc_color_components(mcled_cdev, value);
> > +
> > +	for (i = 0; i < NUM_LEDS; i++) {
> > +		switch (i) {
> > +		case RED:
> > +			if (data->next_red == (u8) mcled_cdev->subled_info[i].brightness)
> > +				break;
> > +			data->next_red = (u8) mcled_cdev->subled_info[i].brightness;
> > +			break;
> > +		case GREEN:
> > +			if (data->next_green == (u8) mcled_cdev->subled_info[i].brightness)
> > +				break;
> > +			data->next_green = (u8) mcled_cdev->subled_info[i].brightness;
> > +			break;
> > +		case BLUE:
> > +			if (data->next_blue == (u8) mcled_cdev->subled_info[i].brightness)
> > +				break;
> > +			data->next_blue = (u8) mcled_cdev->subled_info[i].brightness;
> > +			break;
> > +		}
> > +	}
>
> Does it really worth all these LoC?
>
> Is this enough?
> 	data->next_red = (u8)mcled_cdev->subled_info[RED].brightness;
> 	data->next_green = (u8)mcled_cdev->subled_info[GREEN].brightness;
> 	data->next_blue = (u8)mcled_cdev->subled_info[BLUE].brightness;
>
> > +	blinkm_transfer_hw(led->i2c_client, BLM_GO_RGB);
> > +	dev_dbg(&led->i2c_client->dev,
> > +			"# DONE # next_red = %d, next_green = %d,"
> > +			" next_blue = %d\n",
>
> Keep the string on the same line.
>
> > +			data->next_red, data->next_green,
> > +			data->next_blue);
> > +	return 0;
> > +}
> > +
> >   static int blinkm_led_common_set(struct led_classdev *led_cdev,
> >   				 enum led_brightness value, int color)
> >   {
> >   	/* led_brightness is 0, 127 or 255 - we just use it here as-is */
> > -	struct blinkm_led *led = cdev_to_blmled(led_cdev);
> > +	struct blinkm_led *led = monochrome_led_cdev_to_blmled(led_cdev);
> >   	struct blinkm_data *data = i2c_get_clientdata(led->i2c_client);
> >
> >   	switch (color) {
> > @@ -570,7 +623,11 @@ static int blinkm_probe(struct i2c_client *client,
> >   			const struct i2c_device_id *id)
> >   {
> >   	struct blinkm_data *data;
> > -	struct blinkm_led *led[3];
> > +	/* For multicolor support */
> > +	struct blinkm_led *mc_led;
> > +	struct mc_subled *mc_led_info;
> > +	/* 3 seperate classes for red, green, and blue respectively */
> > +	struct blinkm_led *leds[3];
>
> s/3/NUM_LEDS/ ?
>
> >   	int err, i;
> >   	char blinkm_led_name[28];
> >
> > @@ -581,6 +638,12 @@ static int blinkm_probe(struct i2c_client *client,
> >   		goto exit;
> >   	}
> >
> > +	mc_led_info = devm_kmalloc_array(&client->dev, 3, sizeof(*mc_led_info),
>
> s/3/NUM_LEDS/ ?
> s/devm_kmalloc_array(...|__GFP_ZERO)/devm_kcalloc/ ?
>
> > +					GFP_KERNEL | __GFP_ZERO);
> > +	if (!mc_led_info) {
> > +		err = -ENOMEM;
> > +		goto exit;
> > +	}
> >   	data->i2c_addr = 0x08;
> >   	/* i2c addr  - use fake addr of 0x08 initially (real is 0x09) */
> >   	data->fw_ver = 0xfe;
> > @@ -599,28 +662,30 @@ static int blinkm_probe(struct i2c_client *client,
> >   		goto exit;
> >   	}
> >
> > +
> > +	/* Register red, green, and blue sysfs classes */
> >   	for (i = 0; i < 3; i++) {
>
> s/3/NUM_LEDS/?
>
> >   		/* RED = 0, GREEN = 1, BLUE = 2 */
> > -		led[i] = &data->blinkm_leds[i];
> > -		led[i]->i2c_client = client;
> > -		led[i]->id = i;
> > -		led[i]->led_cdev.max_brightness = 255;
> > -		led[i]->led_cdev.flags = LED_CORE_SUSPENDRESUME;
> > +		leds[i] = &data->blinkm_leds[i];
> > +		leds[i]->i2c_client = client;
> > +		leds[i]->id = i;
> > +		leds[i]->monochrome_led_cdev.max_brightness = 255;
> > +		leds[i]->monochrome_led_cdev.flags = LED_CORE_SUSPENDRESUME;
> >   		switch (i) {
> >   		case RED:
> >   			snprintf(blinkm_led_name, sizeof(blinkm_led_name),
> >   					 "blinkm-%d-%d-red",
> >   					 client->adapter->nr,
> >   					 client->addr);
> > -			led[i]->led_cdev.name = blinkm_led_name;
> > -			led[i]->led_cdev.brightness_set_blocking =
> > +			leds[i]->monochrome_led_cdev.name = blinkm_led_name;
> > +			leds[i]->monochrome_led_cdev.brightness_set_blocking =
> >   							blinkm_led_red_set;
> >   			err = led_classdev_register(&client->dev,
> > -						    &led[i]->led_cdev);
> > +							&leds[i]->monochrome_led_cdev);
> >   			if (err < 0) {
> >   				dev_err(&client->dev,
> >   					"couldn't register LED %s\n",
> > -					led[i]->led_cdev.name);
> > +					leds[i]->monochrome_led_cdev.name);
> >   				goto failred;
> >   			}
> >   			break;
> > @@ -629,15 +694,15 @@ static int blinkm_probe(struct i2c_client *client,
> >   					 "blinkm-%d-%d-green",
> >   					 client->adapter->nr,
> >   					 client->addr);
> > -			led[i]->led_cdev.name = blinkm_led_name;
> > -			led[i]->led_cdev.brightness_set_blocking =
> > +			leds[i]->monochrome_led_cdev.name = blinkm_led_name;
> > +			leds[i]->monochrome_led_cdev.brightness_set_blocking =
> >   							blinkm_led_green_set;
> >   			err = led_classdev_register(&client->dev,
> > -						    &led[i]->led_cdev);
> > +							&leds[i]->monochrome_led_cdev);
>
> Keep the original indentation.
>
> >   			if (err < 0) {
> >   				dev_err(&client->dev,
> >   					"couldn't register LED %s\n",
> > -					led[i]->led_cdev.name);
> > +					leds[i]->monochrome_led_cdev.name);
> >   				goto failgreen;
> >   			}
> >   			break;
> > @@ -646,34 +711,72 @@ static int blinkm_probe(struct i2c_client *client,
> >   					 "blinkm-%d-%d-blue",
> >   					 client->adapter->nr,
> >   					 client->addr);
> > -			led[i]->led_cdev.name = blinkm_led_name;
> > -			led[i]->led_cdev.brightness_set_blocking =
> > +			leds[i]->monochrome_led_cdev.name = blinkm_led_name;
> > +			leds[i]->monochrome_led_cdev.brightness_set_blocking =
> >   							blinkm_led_blue_set;
> >   			err = led_classdev_register(&client->dev,
> > -						    &led[i]->led_cdev);
> > +							&leds[i]->monochrome_led_cdev);
>
> Keep the original indentation.
>
> >   			if (err < 0) {
> >   				dev_err(&client->dev,
> >   					"couldn't register LED %s\n",
> > -					led[i]->led_cdev.name);
> > +					leds[i]->monochrome_led_cdev.name);
> >   				goto failblue;
> >   			}
> >   			break;
> >   		}		/* end switch */
> >   	}			/* end for */
> >
> > +
> > +
>
> One NL is enough.
>
> > +	/* Register multicolor sysfs class */
> > +	mc_led = &data->mc_blinkm_led;
> > +	mc_led->i2c_client = client;
> > +	mc_led->id = 4;
>
> Why 4?
> NUM_LEDS+1?
>
> > +
> > +	mc_led_info[0].color_index = LED_COLOR_ID_RED;
> > +	mc_led_info[0].channel = 0;
> > +	mc_led_info[1].color_index = LED_COLOR_ID_GREEN;
> > +	mc_led_info[1].channel = 1;
> > +	mc_led_info[2].color_index = LED_COLOR_ID_BLUE;
> > +	mc_led_info[2].channel = 2;
> > +	mc_led->mcled_cdev.subled_info = mc_led_info;
> > +	mc_led->mcled_cdev.num_colors = NUM_LEDS;
> > +
> > +	mc_led->led_cdev = &mc_led->mcled_cdev.led_cdev;
> > +	mc_led->led_cdev->brightness = 255;
> > +	mc_led->led_cdev->max_brightness = 255;
> > +	mc_led->led_cdev->flags = LED_CORE_SUSPENDRESUME;
> > +	snprintf(blinkm_led_name, sizeof(blinkm_led_name),
> > +		 "blinkm-%d-%d-multi",
> > +		 client->adapter->nr,
> > +		 client->addr);
> > +	mc_led->led_cdev->name = blinkm_led_name;
> > +	mc_led->led_cdev->brightness_set_blocking =
> > +					blinkm_set_mc_brightness;
> > +
> > +	err = led_classdev_multicolor_register(&client->dev, &mc_led->mcled_cdev);
> > +	if (err < 0) {
> > +		dev_err(&client->dev, "couldn't register LED %s\n",
> > +				mc_led->led_cdev->name);
> > +		goto failmulti;
> > +	}
> >   	/* Initialize the blinkm */
> >   	blinkm_init_hw(client);
> >
> >   	return 0;
> >
> > +failmulti:
> > +	led_classdev_unregister(&leds[BLUE]->monochrome_led_cdev);
> > +
> >   failblue:
> > -	led_classdev_unregister(&led[GREEN]->led_cdev);
> > +	led_classdev_unregister(&leds[GREEN]->monochrome_led_cdev);
> >
> >   failgreen:
> > -	led_classdev_unregister(&led[RED]->led_cdev);
> > +	led_classdev_unregister(&leds[RED]->monochrome_led_cdev);
> >
> >   failred:
> >   	sysfs_remove_group(&client->dev.kobj, &blinkm_group);
> > +
> >   exit:
> >   	return err;
> >   }
> > @@ -685,8 +788,9 @@ static int blinkm_remove(struct i2c_client *client)
> >   	int i;
> >
> >   	/* make sure no workqueue entries are pending */
> > +	led_classdev_unregister(&data->mc_blinkm_led.mcled_cdev.led_cdev);
> >   	for (i = 0; i < 3; i++)
>
> s/3/NUM_LEDS/?
>
> > -		led_classdev_unregister(&data->blinkm_leds[i].led_cdev);
> > +		led_classdev_unregister(&data->blinkm_leds[i].monochrome_led_cdev);
> >
> >   	/* reset rgb */
> >   	data->next_red = 0x00;
>
Hi!

Thank you very much for reviewing my patch. I will be sure to make those
changes, as they were mostly small oversights and silly mistakes.

Best Regards,
Joe

Re: [PATCH RESEND] Add multicolor support to BlinkM LED driver

[Joseph Strauss]

On 22/12/23 12:15PM, Lee Jones wrote:
> On Wed, 14 Dec 2022, Joseph Strauss wrote:
>
> Would you mind composing your mails such that my Key Manager doens't ask
> me for a password in order to view them please?
>
> --
> Lee Jones [李琼斯]
I would be happy to, but I am not sure how to diagnose or fix the
problem. I used git send-email to send the patch. I have no such problem
when viewing the email from my own client. I looked at the email headers
and did not see anything out of the ordinary, and the thread shows up
fine on lore.kernel.org. Is there any other information you can provide
about the problem? Has it happened before?

Best Regards, Joe

Re: [PATCH RESEND] Add multicolor support to BlinkM LED driver

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1682 bytes --]

Hey Joe,

I thought I replied to Lee's message the other day but obviously forgot
to send it.

On Tue, Dec 27, 2022 at 04:47:58PM +0000, Joseph Strauss wrote:
> On 22/12/23 12:15PM, Lee Jones wrote:
> > On Wed, 14 Dec 2022, Joseph Strauss wrote:
> >
> > Would you mind composing your mails such that my Key Manager doens't ask
> > me for a password in order to view them please?

> I would be happy to, but I am not sure how to diagnose or fix the
> problem. I used git send-email to send the patch. I have no such problem
> when viewing the email from my own client. I looked at the email headers
> and did not see anything out of the ordinary, and the thread shows up
> fine on lore.kernel.org.

> Is there any other information you can provide
> about the problem? Has it happened before?

Unfortunately it has - and it is a protonmail "feature".
kernel.org publishes keys for it's users via WKD:
https://www.kernel.org/doc/html/v4.17/process/maintainer-pgp-guide.html#configure-auto-key-retrieval-using-wkd-and-dane

protonmail, as part of the "back end" or w/e automagically picks up the
keys and uses them to encrypt the message. The list (and people without
WKD set up for their domains) will get the un-encrypted version

When I was a proton user, I could not find a way in the UI to disable
it. I contacted their support at the time who told me it was a feature!

Recently it cropped up again:
https://lore.kernel.org/all/20221122213754.45474-1-alobakin@mailbox.org/
Unfortunately, the "workaround" was to avoid proton there too.

I was wondering, when I saw Lee's mail, if we should add some sort of
comment about this proton behaviour to process/email-clients..

Conor.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH RESEND] Add multicolor support to BlinkM LED driver

[Lee Jones]

On Thu, 13 Jul 2023, Joseph Strauss wrote:

> Add multicolor support to the BlinkM driver, making it easier to control
> from userspace. The BlinkM LED is a programmable RGB LED. The driver
> currently supports only the regular LED sysfs class, resulting in the
> creation of three distinct classes, one for red, green, and blue. The
> user then has to input three values into the three seperate brightness
> files within those classes. The multicolor LED framework makes the
> device easier to control with the multi_intensity file: the user can
> input three values at once to form a color, while still controlling the
> lightness with the brightness file.
> 
> The main struct blinkm_led has changed slightly. The struct led_classdev
> for the regular sysfs classes remain. The blinkm_probe function checks
> CONFIG_LEDS_BLINKM_MULTICOLOR to decide whether to load the seperate
> sysfs classes or the single multicolor one, but never both. The
> blinkm_set_mc_brightness() function had to be added to calculate the
> three color components and then set the fields of the blinkm_data
> structure accordingly.
> 
> Signed-off-by: Joseph Strauss <jstrauss@mailbox.org>
> ---
> Changes in v2:
> - Replaced instances of the constant 3 with NUM_LEDS, where applicable
> - Fixed formatting errors
> - Replaced loop inside of blinkm_set_mc_brightness() with equivalent
>   statements
> - Changed id of multicolor class from 4 to 3
> - Replaced call to devm_kmalloc_array() with devm_kcalloc()
> Changes in v3:
> - Add CONFIG_LEDS_BLINKM_MULTICOLOR to check whether to use multicolor
>   funcitonality
> - Extend well-known-leds.txt to include standard names for RGB and indicator
>   LEDS
> - Change name of Blinkm sysfs class according to well-known-leds.txt
> - Simplify struct blinkm_led and struct blinkm_data
> - Remove magic numbers
> - Fix formatting errors
> - Remove unrelated changes
> 
>  Documentation/leds/leds-blinkm.rst     |  27 +++-
>  Documentation/leds/well-known-leds.txt |   8 +
>  drivers/leds/Kconfig                   |   8 +
>  drivers/leds/leds-blinkm.c             | 203 +++++++++++++++++--------
>  4 files changed, 177 insertions(+), 69 deletions(-)

Getting there.  Couple of nits now.

> diff --git a/Documentation/leds/leds-blinkm.rst b/Documentation/leds/leds-blinkm.rst
> index c74b5bc877b1..a483f20aead1 100644
> --- a/Documentation/leds/leds-blinkm.rst
> +++ b/Documentation/leds/leds-blinkm.rst
> @@ -13,9 +13,27 @@ The device accepts RGB and HSB color values through separate commands.
>  Also you can store blinking sequences as "scripts" in
>  the controller and run them. Also fading is an option.
>  
> -The interface this driver provides is 2-fold:
> +The interface this driver provides is 3-fold:
>  
> -a) LED class interface for use with triggers
> +a) LED multicolor class interface for use with triggers
> +#######################################################
> +
> +The registration follows the scheme::
> +
> +  blinkm-<i2c-bus-nr>-<i2c-device-nr>:rgb:indicator
> +
> +  $ ls -h /sys/class/leds/blinkm-1-9:rgb:indicator
> +  brightness  device  max_brightness  multi_index  multi_intensity  power  subsystem  trigger  uevent
> +
> +The order in which to write the intensity values can be found in multi_index.
> +Exactly three values between 0 and 255 must be written to multi_intensity to change the color::
> +
> +  $ echo 255 100 50 > multi_intensity
> +
> +The overall brightness of the color that you choose can also be changed by
> +writing a value between 0 and 255 to the brightness file.
> +
> +b) LED class interface for use with triggers
>  ############################################
>  
>  The registration follows the scheme::
> @@ -50,7 +68,7 @@ E.g.::
>    $
>  
>  
> -b) Sysfs group to control rgb, fade, hsb, scripts ...
> +c) Sysfs group to control rgb, fade, hsb, scripts ...
>  #####################################################
>  
>  This extended interface is available as folder blinkm
> @@ -79,6 +97,7 @@ E.g.::
>  
>  
>  
> -as of 6/2012
> +as of 05/2023
>  
>  dl9pf <at> gmx <dot> de
> +jstrauss <at> mailbox <dot> org
> diff --git a/Documentation/leds/well-known-leds.txt b/Documentation/leds/well-known-leds.txt
> index 2160382c86be..2ac4eaed1454 100644
> --- a/Documentation/leds/well-known-leds.txt
> +++ b/Documentation/leds/well-known-leds.txt
> @@ -70,3 +70,11 @@ Good: "platform:*:charging" (allwinner sun50i)
>  * Screen
>  
>  Good: ":backlight" (Motorola Droid 4)
> +
> +* Indicators
> +
> +Good: ":indicator" (Blinkm)
> +
> +* RGB
> +
> +Good: ":rgb" (Blinkm)
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 499d0f215a8b..f38d786f9a89 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -736,6 +736,14 @@ config LEDS_BLINKM
>  	  This option enables support for the BlinkM RGB LED connected
>  	  through I2C. Say Y to enable support for the BlinkM LED.
>  
> +config LEDS_BLINKM_MULTICOLOR
> +	bool "Enable multicolor support for BlinkM I2C RGB LED"
> +	depends on LEDS_BLINKM
> +	depends on LEDS_CLASS_MULTICOLOR
> +	help
> +	  This option enables multicolor sysfs class support for BlinkM LED and
> +	  disables the older, separated sysfs interface
> +
>  config LEDS_POWERNV
>  	tristate "LED support for PowerNV Platform"
>  	depends on LEDS_CLASS
> diff --git a/drivers/leds/leds-blinkm.c b/drivers/leds/leds-blinkm.c
> index e19cc8a7b7ca..a2295d0d0a2b 100644
> --- a/drivers/leds/leds-blinkm.c
> +++ b/drivers/leds/leds-blinkm.c
> @@ -2,6 +2,7 @@
>  /*
>   *  leds-blinkm.c
>   *  (c) Jan-Simon Möller (dl9pf@gmx.de)
> + *  (c) Joseph Strauss (jstrauss@mailbox.org)
>   */
>  
>  #include <linux/module.h>
> @@ -15,6 +16,10 @@
>  #include <linux/pm_runtime.h>
>  #include <linux/leds.h>
>  #include <linux/delay.h>
> +#include <linux/led-class-multicolor.h>
> +#include <linux/kconfig.h>
> +
> +#define NUM_LEDS 3
>  
>  /* Addresses to scan - BlinkM is on 0x09 by default*/
>  static const unsigned short normal_i2c[] = { 0x09, I2C_CLIENT_END };
> @@ -22,19 +27,24 @@ static const unsigned short normal_i2c[] = { 0x09, I2C_CLIENT_END };
>  static int blinkm_transfer_hw(struct i2c_client *client, int cmd);
>  static int blinkm_test_run(struct i2c_client *client);
>  
> +/* Contains data structures for both the color-seperated sysfs classes, and the new multicolor class */
>  struct blinkm_led {
>  	struct i2c_client *i2c_client;
> +	/* used when multicolor support is disabled */
>  	struct led_classdev led_cdev;
> +	struct led_classdev_mc mcled_cdev;
>  	int id;
>  };
>  
> -#define cdev_to_blmled(c)          container_of(c, struct blinkm_led, led_cdev)
> +#define led_cdev_to_blmled(c)				container_of(c, struct blinkm_led, led_cdev)
> +#define mcled_cdev_to_led(c)				container_of(c, struct blinkm_led, mcled_cdev)
>  
>  struct blinkm_data {
>  	struct i2c_client *i2c_client;
>  	struct mutex update_lock;
>  	/* used for led class interface */
> -	struct blinkm_led blinkm_leds[3];
> +	struct blinkm_led blinkm_leds[NUM_LEDS];
> +
>  	/* used for "blinkm" sysfs interface */
>  	u8 red;			/* color red */
>  	u8 green;		/* color green */
> @@ -419,11 +429,29 @@ static int blinkm_transfer_hw(struct i2c_client *client, int cmd)
>  	return 0;
>  }
>  
> +static int blinkm_set_mc_brightness(struct led_classdev *led_cdev,
> +				 enum led_brightness value)
> +{
> +	struct led_classdev_mc *mcled_cdev = lcdev_to_mccdev(led_cdev);
> +	struct blinkm_led *led = mcled_cdev_to_led(mcled_cdev);
> +	struct blinkm_data *data = i2c_get_clientdata(led->i2c_client);
> +
> +	led_mc_calc_color_components(mcled_cdev, value);
> +
> +	data->next_red = (u8) mcled_cdev->subled_info[RED].brightness;
> +	data->next_green = (u8) mcled_cdev->subled_info[GREEN].brightness;
> +	data->next_blue = (u8) mcled_cdev->subled_info[BLUE].brightness;
> +
> +	blinkm_transfer_hw(led->i2c_client, BLM_GO_RGB);
> +
> +	return 0;
> +}
> +
>  static int blinkm_led_common_set(struct led_classdev *led_cdev,
>  				 enum led_brightness value, int color)
>  {
>  	/* led_brightness is 0, 127 or 255 - we just use it here as-is */
> -	struct blinkm_led *led = cdev_to_blmled(led_cdev);
> +	struct blinkm_led *led = led_cdev_to_blmled(led_cdev);
>  	struct blinkm_data *data = i2c_get_clientdata(led->i2c_client);
>  
>  	switch (color) {
> @@ -569,7 +597,11 @@ static int blinkm_probe(struct i2c_client *client,
>  			const struct i2c_device_id *id)
>  {
>  	struct blinkm_data *data;
> -	struct blinkm_led *led[3];
> +	/* For multicolor support */
> +	struct blinkm_led *mc_led;
> +	struct mc_subled *mc_led_info;
> +	/* 3 seperate classes for red, green, and blue respectively */
> +	struct blinkm_led *leds[NUM_LEDS];
>  	int err, i;
>  	char blinkm_led_name[28];
>  
> @@ -580,6 +612,12 @@ static int blinkm_probe(struct i2c_client *client,
>  		goto exit;
>  	}
>  
> +	mc_led_info = devm_kcalloc(&client->dev, NUM_LEDS, sizeof(*mc_led_info),
> +					GFP_KERNEL);
> +	if (!mc_led_info) {
> +		err = -ENOMEM;
> +		goto exit;
> +	}
>  	data->i2c_addr = 0x08;
>  	/* i2c addr  - use fake addr of 0x08 initially (real is 0x09) */
>  	data->fw_ver = 0xfe;
> @@ -598,81 +636,116 @@ static int blinkm_probe(struct i2c_client *client,
>  		goto exit;
>  	}
>  
> -	for (i = 0; i < 3; i++) {
> -		/* RED = 0, GREEN = 1, BLUE = 2 */
> -		led[i] = &data->blinkm_leds[i];
> -		led[i]->i2c_client = client;
> -		led[i]->id = i;
> -		led[i]->led_cdev.max_brightness = 255;
> -		led[i]->led_cdev.flags = LED_CORE_SUSPENDRESUME;
> -		switch (i) {
> -		case RED:
> -			snprintf(blinkm_led_name, sizeof(blinkm_led_name),
> -					 "blinkm-%d-%d-red",
> -					 client->adapter->nr,
> -					 client->addr);
> -			led[i]->led_cdev.name = blinkm_led_name;
> -			led[i]->led_cdev.brightness_set_blocking =
> -							blinkm_led_red_set;
> -			err = led_classdev_register(&client->dev,
> -						    &led[i]->led_cdev);
> +	if (!IS_ENABLED(CONFIG_LEDS_BLINKM_MULTICOLOR)) {
> +		/* Register red, green, and blue sysfs classes */
> +		for (i = 0; i < NUM_LEDS; i++) {
> +			/* RED = 0, GREEN = 1, BLUE = 2 */
> +			leds[i] = &data->blinkm_leds[i];
> +			leds[i]->i2c_client = client;
> +			leds[i]->id = i;
> +			leds[i]->led_cdev.max_brightness = 255;
> +			leds[i]->led_cdev.flags = LED_CORE_SUSPENDRESUME;
> +			switch (i) {
> +			case RED:
> +				snprintf(blinkm_led_name, sizeof(blinkm_led_name),
> +						 "blinkm-%d-%d-red",
> +						 client->adapter->nr,
> +						 client->addr);
> +				leds[i]->led_cdev.name = blinkm_led_name;
> +				leds[i]->led_cdev.brightness_set_blocking =
> +								blinkm_led_red_set;
> +				err = led_classdev_register(&client->dev,
> +								&leds[i]->led_cdev);
> +				if (err < 0) {
> +					dev_err(&client->dev,
> +						"couldn't register LED %s\n",
> +						leds[i]->led_cdev.name);
> +					goto failred;
> +				}
> +				break;
> +			case GREEN:
> +				snprintf(blinkm_led_name, sizeof(blinkm_led_name),
> +						 "blinkm-%d-%d-green",
> +						 client->adapter->nr,
> +						 client->addr);
> +				leds[i]->led_cdev.name = blinkm_led_name;
> +				leds[i]->led_cdev.brightness_set_blocking =
> +								blinkm_led_green_set;
> +				err = led_classdev_register(&client->dev,
> +							&leds[i]->led_cdev);
>  			if (err < 0) {
>  				dev_err(&client->dev,
>  					"couldn't register LED %s\n",

This tabbing needs adjusting too.

> -					led[i]->led_cdev.name);
> -				goto failred;
> -			}
> -			break;
> -		case GREEN:
> -			snprintf(blinkm_led_name, sizeof(blinkm_led_name),
> -					 "blinkm-%d-%d-green",
> -					 client->adapter->nr,
> -					 client->addr);
> -			led[i]->led_cdev.name = blinkm_led_name;
> -			led[i]->led_cdev.brightness_set_blocking =
> -							blinkm_led_green_set;
> -			err = led_classdev_register(&client->dev,
> -						    &led[i]->led_cdev);
> -			if (err < 0) {
> -				dev_err(&client->dev,
> -					"couldn't register LED %s\n",
> -					led[i]->led_cdev.name);
> +					leds[i]->led_cdev.name);
>  				goto failgreen;
>  			}
> -			break;
> -		case BLUE:
> -			snprintf(blinkm_led_name, sizeof(blinkm_led_name),
> -					 "blinkm-%d-%d-blue",
> -					 client->adapter->nr,
> -					 client->addr);
> -			led[i]->led_cdev.name = blinkm_led_name;
> -			led[i]->led_cdev.brightness_set_blocking =
> -							blinkm_led_blue_set;
> -			err = led_classdev_register(&client->dev,
> -						    &led[i]->led_cdev);
> -			if (err < 0) {
> -				dev_err(&client->dev,
> -					"couldn't register LED %s\n",
> -					led[i]->led_cdev.name);
> -				goto failblue;
> -			}
> -			break;
> -		}		/* end switch */
> -	}			/* end for */
> +				break;
> +			case BLUE:
> +				snprintf(blinkm_led_name, sizeof(blinkm_led_name),
> +						 "blinkm-%d-%d-blue",
> +						 client->adapter->nr,
> +						 client->addr);
> +				leds[i]->led_cdev.name = blinkm_led_name;
> +				leds[i]->led_cdev.brightness_set_blocking =
> +								blinkm_led_blue_set;
> +				err = led_classdev_register(&client->dev,
> +								&leds[i]->led_cdev);
> +				if (err < 0) {
> +					dev_err(&client->dev,
> +						"couldn't register LED %s\n",
> +						leds[i]->led_cdev.name);
> +					goto failblue;
> +				}
> +				break;
> +			}		/* end switch */

No default?

> +		}			/* end for */
> +	} else {
> +		/* Register multicolor sysfs class */
> +		/* The first element of leds is used for multicolor facilities */
> +		mc_led = &data->blinkm_leds[RED];
> +		mc_led->i2c_client = client;
> +
> +		mc_led_info[RED].color_index = LED_COLOR_ID_RED;
> +		mc_led_info[GREEN].color_index = LED_COLOR_ID_GREEN;
> +		mc_led_info[BLUE].color_index = LED_COLOR_ID_BLUE;
> +
> +		mc_led->mcled_cdev.subled_info = mc_led_info;
> +		mc_led->mcled_cdev.num_colors = NUM_LEDS;
> +		mc_led->mcled_cdev.led_cdev.brightness = 255;
> +		mc_led->mcled_cdev.led_cdev.max_brightness = 255;
> +		mc_led->mcled_cdev.led_cdev.flags = LED_CORE_SUSPENDRESUME;
> +
> +		snprintf(blinkm_led_name, sizeof(blinkm_led_name),
> +			 "blinkm-%d-%d:rgb:indicator",
> +			 client->adapter->nr,
> +			 client->addr);
> +		mc_led->mcled_cdev.led_cdev.name = blinkm_led_name;
> +		mc_led->mcled_cdev.led_cdev.brightness_set_blocking = blinkm_set_mc_brightness;
> +
> +		err = led_classdev_multicolor_register(&client->dev, &mc_led->mcled_cdev);
> +		if (err < 0) {
> +			dev_err(&client->dev, "couldn't register LED %s\n",
> +					mc_led->led_cdev.name);
> +			goto failmulti;
> +		}
> +	}
>  
> -	/* Initialize the blinkm */
>  	blinkm_init_hw(client);
>  
>  	return 0;
>  
> +failmulti:
> +	led_classdev_unregister(&leds[BLUE]->led_cdev);
> +
>  failblue:
> -	led_classdev_unregister(&led[GREEN]->led_cdev);
> +	led_classdev_unregister(&leds[GREEN]->led_cdev);
>  
>  failgreen:
> -	led_classdev_unregister(&led[RED]->led_cdev);
> +	led_classdev_unregister(&leds[RED]->led_cdev);
>  
>  failred:
>  	sysfs_remove_group(&client->dev.kobj, &blinkm_group);
> +
>  exit:
>  	return err;
>  }
> @@ -684,7 +757,7 @@ static void blinkm_remove(struct i2c_client *client)
>  	int i;
>  
>  	/* make sure no workqueue entries are pending */
> -	for (i = 0; i < 3; i++)
> +	for (i = 0; i < NUM_LEDS; i++)
>  		led_classdev_unregister(&data->blinkm_leds[i].led_cdev);
>  
>  	/* reset rgb */
> @@ -741,6 +814,6 @@ static struct i2c_driver blinkm_driver = {
>  module_i2c_driver(blinkm_driver);
>  
>  MODULE_AUTHOR("Jan-Simon Moeller <dl9pf@gmx.de>");
> +MODULE_AUTHOR("Joseph Strauss <jstrauss@mailbox.org>");
>  MODULE_DESCRIPTION("BlinkM RGB LED driver");
>  MODULE_LICENSE("GPL");
> -
> -- 
> 2.40.1
> 

-- 
Lee Jones [李琼斯]

[PATCH RESEND] Add multicolor support to BlinkM LED driver

[Joseph Strauss]

Add multicolor support to the BlinkM driver, making it easier to control
from userspace. The BlinkM LED is a programmable RGB LED. The driver
currently supports only the regular LED sysfs class, resulting in the
creation of three distinct classes, one for red, green, and blue. The
user then has to input three values into the three seperate brightness
files within those classes. The multicolor LED framework makes the
device easier to control with the multi_intensity file: the user can
input three values at once to form a color, while still controlling the
lightness with the brightness file.

The main struct blinkm_led has changed slightly. The struct led_classdev
for the regular sysfs classes remain. The blinkm_probe function checks
CONFIG_LEDS_BLINKM_MULTICOLOR to decide whether to load the seperate
sysfs classes or the single multicolor one, but never both. The
blinkm_set_mc_brightness() function had to be added to calculate the
three color components and then set the fields of the blinkm_data
structure accordingly.

Signed-off-by: Joseph Strauss <jstrauss@mailbox.org>
---
Changes in v2:
- Replaced instances of the constant 3 with NUM_LEDS, where applicable
- Fixed formatting errors
- Replaced loop inside of blinkm_set_mc_brightness() with equivalent
  statements
- Changed id of multicolor class from 4 to 3
- Replaced call to devm_kmalloc_array() with devm_kcalloc()
Changes in v3:
- Add CONFIG_LEDS_BLINKM_MULTICOLOR to check whether to use multicolor
  funcitonality
- Extend well-known-leds.txt to include standard names for RGB and indicator
  LEDS
- Change name of Blinkm sysfs class according to well-known-leds.txt
- Simplify struct blinkm_led and struct blinkm_data
- Remove magic numbers
- Fix formatting errors
- Remove unrelated changes

 Documentation/leds/leds-blinkm.rst     |  27 +++-
 Documentation/leds/well-known-leds.txt |   8 +
 drivers/leds/Kconfig                   |   8 +
 drivers/leds/leds-blinkm.c             | 203 +++++++++++++++++--------
 4 files changed, 177 insertions(+), 69 deletions(-)

diff --git a/Documentation/leds/leds-blinkm.rst b/Documentation/leds/leds-blinkm.rst
index c74b5bc877b1..a483f20aead1 100644
--- a/Documentation/leds/leds-blinkm.rst
+++ b/Documentation/leds/leds-blinkm.rst
@@ -13,9 +13,27 @@ The device accepts RGB and HSB color values through separate commands.
 Also you can store blinking sequences as "scripts" in
 the controller and run them. Also fading is an option.
 
-The interface this driver provides is 2-fold:
+The interface this driver provides is 3-fold:
 
-a) LED class interface for use with triggers
+a) LED multicolor class interface for use with triggers
+#######################################################
+
+The registration follows the scheme::
+
+  blinkm-<i2c-bus-nr>-<i2c-device-nr>:rgb:indicator
+
+  $ ls -h /sys/class/leds/blinkm-1-9:rgb:indicator
+  brightness  device  max_brightness  multi_index  multi_intensity  power  subsystem  trigger  uevent
+
+The order in which to write the intensity values can be found in multi_index.
+Exactly three values between 0 and 255 must be written to multi_intensity to change the color::
+
+  $ echo 255 100 50 > multi_intensity
+
+The overall brightness of the color that you choose can also be changed by
+writing a value between 0 and 255 to the brightness file.
+
+b) LED class interface for use with triggers
 ############################################
 
 The registration follows the scheme::
@@ -50,7 +68,7 @@ E.g.::
   $
 
 
-b) Sysfs group to control rgb, fade, hsb, scripts ...
+c) Sysfs group to control rgb, fade, hsb, scripts ...
 #####################################################
 
 This extended interface is available as folder blinkm
@@ -79,6 +97,7 @@ E.g.::
 
 
 
-as of 6/2012
+as of 05/2023
 
 dl9pf <at> gmx <dot> de
+jstrauss <at> mailbox <dot> org
diff --git a/Documentation/leds/well-known-leds.txt b/Documentation/leds/well-known-leds.txt
index 2160382c86be..2ac4eaed1454 100644
--- a/Documentation/leds/well-known-leds.txt
+++ b/Documentation/leds/well-known-leds.txt
@@ -70,3 +70,11 @@ Good: "platform:*:charging" (allwinner sun50i)
 * Screen
 
 Good: ":backlight" (Motorola Droid 4)
+
+* Indicators
+
+Good: ":indicator" (Blinkm)
+
+* RGB
+
+Good: ":rgb" (Blinkm)
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 499d0f215a8b..f38d786f9a89 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -736,6 +736,14 @@ config LEDS_BLINKM
 	  This option enables support for the BlinkM RGB LED connected
 	  through I2C. Say Y to enable support for the BlinkM LED.
 
+config LEDS_BLINKM_MULTICOLOR
+	bool "Enable multicolor support for BlinkM I2C RGB LED"
+	depends on LEDS_BLINKM
+	depends on LEDS_CLASS_MULTICOLOR
+	help
+	  This option enables multicolor sysfs class support for BlinkM LED and
+	  disables the older, separated sysfs interface
+
 config LEDS_POWERNV
 	tristate "LED support for PowerNV Platform"
 	depends on LEDS_CLASS
diff --git a/drivers/leds/leds-blinkm.c b/drivers/leds/leds-blinkm.c
index e19cc8a7b7ca..a2295d0d0a2b 100644
--- a/drivers/leds/leds-blinkm.c
+++ b/drivers/leds/leds-blinkm.c
@@ -2,6 +2,7 @@
 /*
  *  leds-blinkm.c
  *  (c) Jan-Simon Möller (dl9pf@gmx.de)
+ *  (c) Joseph Strauss (jstrauss@mailbox.org)
  */
 
 #include <linux/module.h>
@@ -15,6 +16,10 @@
 #include <linux/pm_runtime.h>
 #include <linux/leds.h>
 #include <linux/delay.h>
+#include <linux/led-class-multicolor.h>
+#include <linux/kconfig.h>
+
+#define NUM_LEDS 3
 
 /* Addresses to scan - BlinkM is on 0x09 by default*/
 static const unsigned short normal_i2c[] = { 0x09, I2C_CLIENT_END };
@@ -22,19 +27,24 @@ static const unsigned short normal_i2c[] = { 0x09, I2C_CLIENT_END };
 static int blinkm_transfer_hw(struct i2c_client *client, int cmd);
 static int blinkm_test_run(struct i2c_client *client);
 
+/* Contains data structures for both the color-seperated sysfs classes, and the new multicolor class */
 struct blinkm_led {
 	struct i2c_client *i2c_client;
+	/* used when multicolor support is disabled */
 	struct led_classdev led_cdev;
+	struct led_classdev_mc mcled_cdev;
 	int id;
 };
 
-#define cdev_to_blmled(c)          container_of(c, struct blinkm_led, led_cdev)
+#define led_cdev_to_blmled(c)				container_of(c, struct blinkm_led, led_cdev)
+#define mcled_cdev_to_led(c)				container_of(c, struct blinkm_led, mcled_cdev)
 
 struct blinkm_data {
 	struct i2c_client *i2c_client;
 	struct mutex update_lock;
 	/* used for led class interface */
-	struct blinkm_led blinkm_leds[3];
+	struct blinkm_led blinkm_leds[NUM_LEDS];
+
 	/* used for "blinkm" sysfs interface */
 	u8 red;			/* color red */
 	u8 green;		/* color green */
@@ -419,11 +429,29 @@ static int blinkm_transfer_hw(struct i2c_client *client, int cmd)
 	return 0;
 }
 
+static int blinkm_set_mc_brightness(struct led_classdev *led_cdev,
+				 enum led_brightness value)
+{
+	struct led_classdev_mc *mcled_cdev = lcdev_to_mccdev(led_cdev);
+	struct blinkm_led *led = mcled_cdev_to_led(mcled_cdev);
+	struct blinkm_data *data = i2c_get_clientdata(led->i2c_client);
+
+	led_mc_calc_color_components(mcled_cdev, value);
+
+	data->next_red = (u8) mcled_cdev->subled_info[RED].brightness;
+	data->next_green = (u8) mcled_cdev->subled_info[GREEN].brightness;
+	data->next_blue = (u8) mcled_cdev->subled_info[BLUE].brightness;
+
+	blinkm_transfer_hw(led->i2c_client, BLM_GO_RGB);
+
+	return 0;
+}
+
 static int blinkm_led_common_set(struct led_classdev *led_cdev,
 				 enum led_brightness value, int color)
 {
 	/* led_brightness is 0, 127 or 255 - we just use it here as-is */
-	struct blinkm_led *led = cdev_to_blmled(led_cdev);
+	struct blinkm_led *led = led_cdev_to_blmled(led_cdev);
 	struct blinkm_data *data = i2c_get_clientdata(led->i2c_client);
 
 	switch (color) {
@@ -569,7 +597,11 @@ static int blinkm_probe(struct i2c_client *client,
 			const struct i2c_device_id *id)
 {
 	struct blinkm_data *data;
-	struct blinkm_led *led[3];
+	/* For multicolor support */
+	struct blinkm_led *mc_led;
+	struct mc_subled *mc_led_info;
+	/* 3 seperate classes for red, green, and blue respectively */
+	struct blinkm_led *leds[NUM_LEDS];
 	int err, i;
 	char blinkm_led_name[28];
 
@@ -580,6 +612,12 @@ static int blinkm_probe(struct i2c_client *client,
 		goto exit;
 	}
 
+	mc_led_info = devm_kcalloc(&client->dev, NUM_LEDS, sizeof(*mc_led_info),
+					GFP_KERNEL);
+	if (!mc_led_info) {
+		err = -ENOMEM;
+		goto exit;
+	}
 	data->i2c_addr = 0x08;
 	/* i2c addr  - use fake addr of 0x08 initially (real is 0x09) */
 	data->fw_ver = 0xfe;
@@ -598,81 +636,116 @@ static int blinkm_probe(struct i2c_client *client,
 		goto exit;
 	}
 
-	for (i = 0; i < 3; i++) {
-		/* RED = 0, GREEN = 1, BLUE = 2 */
-		led[i] = &data->blinkm_leds[i];
-		led[i]->i2c_client = client;
-		led[i]->id = i;
-		led[i]->led_cdev.max_brightness = 255;
-		led[i]->led_cdev.flags = LED_CORE_SUSPENDRESUME;
-		switch (i) {
-		case RED:
-			snprintf(blinkm_led_name, sizeof(blinkm_led_name),
-					 "blinkm-%d-%d-red",
-					 client->adapter->nr,
-					 client->addr);
-			led[i]->led_cdev.name = blinkm_led_name;
-			led[i]->led_cdev.brightness_set_blocking =
-							blinkm_led_red_set;
-			err = led_classdev_register(&client->dev,
-						    &led[i]->led_cdev);
+	if (!IS_ENABLED(CONFIG_LEDS_BLINKM_MULTICOLOR)) {
+		/* Register red, green, and blue sysfs classes */
+		for (i = 0; i < NUM_LEDS; i++) {
+			/* RED = 0, GREEN = 1, BLUE = 2 */
+			leds[i] = &data->blinkm_leds[i];
+			leds[i]->i2c_client = client;
+			leds[i]->id = i;
+			leds[i]->led_cdev.max_brightness = 255;
+			leds[i]->led_cdev.flags = LED_CORE_SUSPENDRESUME;
+			switch (i) {
+			case RED:
+				snprintf(blinkm_led_name, sizeof(blinkm_led_name),
+						 "blinkm-%d-%d-red",
+						 client->adapter->nr,
+						 client->addr);
+				leds[i]->led_cdev.name = blinkm_led_name;
+				leds[i]->led_cdev.brightness_set_blocking =
+								blinkm_led_red_set;
+				err = led_classdev_register(&client->dev,
+								&leds[i]->led_cdev);
+				if (err < 0) {
+					dev_err(&client->dev,
+						"couldn't register LED %s\n",
+						leds[i]->led_cdev.name);
+					goto failred;
+				}
+				break;
+			case GREEN:
+				snprintf(blinkm_led_name, sizeof(blinkm_led_name),
+						 "blinkm-%d-%d-green",
+						 client->adapter->nr,
+						 client->addr);
+				leds[i]->led_cdev.name = blinkm_led_name;
+				leds[i]->led_cdev.brightness_set_blocking =
+								blinkm_led_green_set;
+				err = led_classdev_register(&client->dev,
+							&leds[i]->led_cdev);
 			if (err < 0) {
 				dev_err(&client->dev,
 					"couldn't register LED %s\n",
-					led[i]->led_cdev.name);
-				goto failred;
-			}
-			break;
-		case GREEN:
-			snprintf(blinkm_led_name, sizeof(blinkm_led_name),
-					 "blinkm-%d-%d-green",
-					 client->adapter->nr,
-					 client->addr);
-			led[i]->led_cdev.name = blinkm_led_name;
-			led[i]->led_cdev.brightness_set_blocking =
-							blinkm_led_green_set;
-			err = led_classdev_register(&client->dev,
-						    &led[i]->led_cdev);
-			if (err < 0) {
-				dev_err(&client->dev,
-					"couldn't register LED %s\n",
-					led[i]->led_cdev.name);
+					leds[i]->led_cdev.name);
 				goto failgreen;
 			}
-			break;
-		case BLUE:
-			snprintf(blinkm_led_name, sizeof(blinkm_led_name),
-					 "blinkm-%d-%d-blue",
-					 client->adapter->nr,
-					 client->addr);
-			led[i]->led_cdev.name = blinkm_led_name;
-			led[i]->led_cdev.brightness_set_blocking =
-							blinkm_led_blue_set;
-			err = led_classdev_register(&client->dev,
-						    &led[i]->led_cdev);
-			if (err < 0) {
-				dev_err(&client->dev,
-					"couldn't register LED %s\n",
-					led[i]->led_cdev.name);
-				goto failblue;
-			}
-			break;
-		}		/* end switch */
-	}			/* end for */
+				break;
+			case BLUE:
+				snprintf(blinkm_led_name, sizeof(blinkm_led_name),
+						 "blinkm-%d-%d-blue",
+						 client->adapter->nr,
+						 client->addr);
+				leds[i]->led_cdev.name = blinkm_led_name;
+				leds[i]->led_cdev.brightness_set_blocking =
+								blinkm_led_blue_set;
+				err = led_classdev_register(&client->dev,
+								&leds[i]->led_cdev);
+				if (err < 0) {
+					dev_err(&client->dev,
+						"couldn't register LED %s\n",
+						leds[i]->led_cdev.name);
+					goto failblue;
+				}
+				break;
+			}		/* end switch */
+		}			/* end for */
+	} else {
+		/* Register multicolor sysfs class */
+		/* The first element of leds is used for multicolor facilities */
+		mc_led = &data->blinkm_leds[RED];
+		mc_led->i2c_client = client;
+
+		mc_led_info[RED].color_index = LED_COLOR_ID_RED;
+		mc_led_info[GREEN].color_index = LED_COLOR_ID_GREEN;
+		mc_led_info[BLUE].color_index = LED_COLOR_ID_BLUE;
+
+		mc_led->mcled_cdev.subled_info = mc_led_info;
+		mc_led->mcled_cdev.num_colors = NUM_LEDS;
+		mc_led->mcled_cdev.led_cdev.brightness = 255;
+		mc_led->mcled_cdev.led_cdev.max_brightness = 255;
+		mc_led->mcled_cdev.led_cdev.flags = LED_CORE_SUSPENDRESUME;
+
+		snprintf(blinkm_led_name, sizeof(blinkm_led_name),
+			 "blinkm-%d-%d:rgb:indicator",
+			 client->adapter->nr,
+			 client->addr);
+		mc_led->mcled_cdev.led_cdev.name = blinkm_led_name;
+		mc_led->mcled_cdev.led_cdev.brightness_set_blocking = blinkm_set_mc_brightness;
+
+		err = led_classdev_multicolor_register(&client->dev, &mc_led->mcled_cdev);
+		if (err < 0) {
+			dev_err(&client->dev, "couldn't register LED %s\n",
+					mc_led->led_cdev.name);
+			goto failmulti;
+		}
+	}
 
-	/* Initialize the blinkm */
 	blinkm_init_hw(client);
 
 	return 0;
 
+failmulti:
+	led_classdev_unregister(&leds[BLUE]->led_cdev);
+
 failblue:
-	led_classdev_unregister(&led[GREEN]->led_cdev);
+	led_classdev_unregister(&leds[GREEN]->led_cdev);
 
 failgreen:
-	led_classdev_unregister(&led[RED]->led_cdev);
+	led_classdev_unregister(&leds[RED]->led_cdev);
 
 failred:
 	sysfs_remove_group(&client->dev.kobj, &blinkm_group);
+
 exit:
 	return err;
 }
@@ -684,7 +757,7 @@ static void blinkm_remove(struct i2c_client *client)
 	int i;
 
 	/* make sure no workqueue entries are pending */
-	for (i = 0; i < 3; i++)
+	for (i = 0; i < NUM_LEDS; i++)
 		led_classdev_unregister(&data->blinkm_leds[i].led_cdev);
 
 	/* reset rgb */
@@ -741,6 +814,6 @@ static struct i2c_driver blinkm_driver = {
 module_i2c_driver(blinkm_driver);
 
 MODULE_AUTHOR("Jan-Simon Moeller <dl9pf@gmx.de>");
+MODULE_AUTHOR("Joseph Strauss <jstrauss@mailbox.org>");
 MODULE_DESCRIPTION("BlinkM RGB LED driver");
 MODULE_LICENSE("GPL");
-
-- 
2.40.1

Re: [PATCH RESEND] Add multicolor support to BlinkM LED driver

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 602 bytes --]

On Tue, Jan 03, 2023 at 12:26:05PM +0000, Lee Jones wrote:
> On Tue, 27 Dec 2022, jstrauss wrote:
> 
> > Hi Conor,
> > 
> > Thanks for the insight, I suspected it was some silent Proton issue. I
> > have switched to Mailbox.org, so all of my future emails and patches
> > will originate from this address.
> 
> Thanks.
> 
> Although when replying to this message, I get:
> 
>   Reply to Y6slQLto568WfmfZ@spud? ([yes]/no/?):

Judging by the "spud", that's my message-id from the previous mail.
Joe, might wanna figure out why your mailer is adding a reply-to header
with that in it!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH RESEND] Add multicolor support to BlinkM LED driver

[jstrauss]

On 23/01/03 12:26PM, Lee Jones wrote:
> Also, you will have to resubmit your patch please.

I have just recently submitted a newer version of this patch here:
https://lore.kernel.org/all/20221228010958.9670-1-jstrauss@mailbox.org/

Joe

Re: [PATCH RESEND] Add multicolor support to BlinkM LED driver

[Lee Jones]

On Tue, 27 Dec 2022, jstrauss wrote:

> Hi Conor,
> 
> Thanks for the insight, I suspected it was some silent Proton issue. I
> have switched to Mailbox.org, so all of my future emails and patches
> will originate from this address.

Thanks.

Although when replying to this message, I get:

  Reply to Y6slQLto568WfmfZ@spud? ([yes]/no/?):

No idea what that is either.

Also, you will have to resubmit your patch please.

-- 
Lee Jones [李琼斯]

Re: [PATCH RESEND] Add multicolor support to BlinkM LED driver

[jstrauss]

Hi Conor,

Thanks for the insight, I suspected it was some silent Proton issue. I
have switched to Mailbox.org, so all of my future emails and patches
will originate from this address.

Joe

[PATCH RESEND] Add multicolor support to BlinkM LED driver

[Joseph Strauss]

Added multicolor support to the BlinkM driver, making it easier to
control from userspace. The BlinkM LED is a programmable RGB LED. The
driver currently supports only the regular LED sysfs class, resulting in
the creation of three distinct classes, one for red, green, and blue.
The user then has to input three values into the three seperate
brightness files within those classes. The multicolor LED framework
makes the device easier to control with the multi_intensity file: the
user can input three values at once to form a color, while still
controlling the lightness with the brightness file.

The main struct blinkm_led has changed slightly. A struct
led_classdev_mc has been added to represent the multicolor sysfs class,
and an additional struct led_classdev pointer has been added for
convenience, which points to the struct led_classdev within struct
led_classdev_mc. The struct led_classdev for the regular sysfs classes
remain. Additionally, a field representing the multicolor LED has been
added to the struct blinkm_data, seperate from the blinkm_leds[3] array.

In the blinkm_probe function, the multicolor LED class is registered
after the regular LED classes. The blinkm_set_brightness_mc() function
had to be added to calculate the three color components and then set the
fields of the blinkm_data structure accordingly.

Signed-off-by: Joseph Strauss <jstrauss16@proton.me>

---
 Documentation/leds/leds-blinkm.rst |  24 ++++-
 drivers/leds/Kconfig               |   1 +
 drivers/leds/leds-blinkm.c         | 154 ++++++++++++++++++++++++-----
 3 files changed, 151 insertions(+), 28 deletions(-)

diff --git a/Documentation/leds/leds-blinkm.rst b/Documentation/leds/leds-blinkm.rst
index c74b5bc877b1..3f5dbd5e97b0 100644
--- a/Documentation/leds/leds-blinkm.rst
+++ b/Documentation/leds/leds-blinkm.rst
@@ -13,9 +13,27 @@ The device accepts RGB and HSB color values through separate commands.
 Also you can store blinking sequences as "scripts" in
 the controller and run them. Also fading is an option.

-The interface this driver provides is 2-fold:
+The interface this driver provides is 3-fold:

-a) LED class interface for use with triggers
+a) LED multicolor class interface for use with triggers
+#######################################################
+
+The registration follows the scheme::
+
+  blinkm-<i2c-bus-nr>-<i2c-device-nr>-multi
+
+  $ ls -h /sys/class/leds/blinkm-1-9-multi
+  brightness  device  max_brightness  multi_index  multi_intensity  power  subsystem  trigger  uevent
+
+The order in which to write the intensity values can be found in multi_index.
+Exactly three values between 0 and 255 must be written to multi_intensity to change the color::
+
+  $ echo 255 100 50 > multi_intensity
+
+The overall brightness of the color that you choose can also be changed by
+writing a value between 0 and 255 to the brightness file.
+
+b) LED class interface for use with triggers
 ############################################

 The registration follows the scheme::
@@ -50,7 +68,7 @@ E.g.::
   $


-b) Sysfs group to control rgb, fade, hsb, scripts ...
+c) Sysfs group to control rgb, fade, hsb, scripts ...
 #####################################################

 This extended interface is available as folder blinkm
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index b6742b4231bf..6f73deb7d95c 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -787,6 +787,7 @@ comment "LED driver for blink(1) USB RGB LED is under Special HID drivers (HID_T
 config LEDS_BLINKM
 	tristate "LED support for the BlinkM I2C RGB LED"
 	depends on LEDS_CLASS
+	depends on LEDS_CLASS_MULTICOLOR
 	depends on I2C
 	help
 	  This option enables support for the BlinkM RGB LED connected
diff --git a/drivers/leds/leds-blinkm.c b/drivers/leds/leds-blinkm.c
index b4e1fdff4186..a78bcc2eaff3 100644
--- a/drivers/leds/leds-blinkm.c
+++ b/drivers/leds/leds-blinkm.c
@@ -15,6 +15,9 @@
 #include <linux/pm_runtime.h>
 #include <linux/leds.h>
 #include <linux/delay.h>
+#include <linux/led-class-multicolor.h>
+
+#define NUM_LEDS 3

 /* Addresses to scan - BlinkM is on 0x09 by default*/
 static const unsigned short normal_i2c[] = { 0x09, I2C_CLIENT_END };
@@ -22,19 +25,26 @@ static const unsigned short normal_i2c[] = { 0x09, I2C_CLIENT_END };
 static int blinkm_transfer_hw(struct i2c_client *client, int cmd);
 static int blinkm_test_run(struct i2c_client *client);

+/* Contains data structures for both the color-seperated sysfs classes, and the new multicolor class */
 struct blinkm_led {
 	struct i2c_client *i2c_client;
-	struct led_classdev led_cdev;
+	struct led_classdev monochrome_led_cdev;
+	/* points to struct led_classdev inside of struct led_classdev_mc */
+	struct led_classdev *led_cdev;
+	struct led_classdev_mc mcled_cdev;
 	int id;
 };

-#define cdev_to_blmled(c)          container_of(c, struct blinkm_led, led_cdev)
+#define monochrome_led_cdev_to_blmled(c)	container_of(c, struct blinkm_led, monochrome_led_cdev)
+#define mcled_cdev_to_led(c)				container_of(c, struct blinkm_led, mcled_cdev)

 struct blinkm_data {
 	struct i2c_client *i2c_client;
 	struct mutex update_lock;
 	/* used for led class interface */
+	struct blinkm_led mc_blinkm_led;
 	struct blinkm_led blinkm_leds[3];
+
 	/* used for "blinkm" sysfs interface */
 	u8 red;			/* color red */
 	u8 green;		/* color green */
@@ -260,9 +270,9 @@ static ssize_t test_show(struct device *dev, struct device_attribute *attr,
 static ssize_t test_store(struct device *dev, struct device_attribute *attr,
 			  const char *buf, size_t count)
 {
-
 	struct i2c_client *client;
 	int ret;
+
 	client = to_i2c_client(dev);

 	/*test */
@@ -317,6 +327,7 @@ static int blinkm_read(struct i2c_client *client, int cmd, u8 *arg)
 	int result;
 	int i;
 	int retlen = blinkm_cmds[cmd].nr_ret;
+
 	for (i = 0; i < retlen; i++) {
 		/* repeat for retlen */
 		result = i2c_smbus_read_byte(client);
@@ -419,11 +430,53 @@ static int blinkm_transfer_hw(struct i2c_client *client, int cmd)
 	return 0;
 }

+static int blinkm_set_mc_brightness(struct led_classdev *led_cdev,
+				 enum led_brightness value)
+{
+	struct led_classdev_mc *mcled_cdev;
+	struct blinkm_led *led;
+	struct blinkm_data *data;
+	int i;
+
+	mcled_cdev = lcdev_to_mccdev(led_cdev);
+	led = mcled_cdev_to_led(mcled_cdev);
+	data = i2c_get_clientdata(led->i2c_client);
+
+	led_mc_calc_color_components(mcled_cdev, value);
+
+	for (i = 0; i < NUM_LEDS; i++) {
+		switch (i) {
+		case RED:
+			if (data->next_red == (u8) mcled_cdev->subled_info[i].brightness)
+				break;
+			data->next_red = (u8) mcled_cdev->subled_info[i].brightness;
+			break;
+		case GREEN:
+			if (data->next_green == (u8) mcled_cdev->subled_info[i].brightness)
+				break;
+			data->next_green = (u8) mcled_cdev->subled_info[i].brightness;
+			break;
+		case BLUE:
+			if (data->next_blue == (u8) mcled_cdev->subled_info[i].brightness)
+				break;
+			data->next_blue = (u8) mcled_cdev->subled_info[i].brightness;
+			break;
+		}
+	}
+	blinkm_transfer_hw(led->i2c_client, BLM_GO_RGB);
+	dev_dbg(&led->i2c_client->dev,
+			"# DONE # next_red = %d, next_green = %d,"
+			" next_blue = %d\n",
+			data->next_red, data->next_green,
+			data->next_blue);
+	return 0;
+}
+
 static int blinkm_led_common_set(struct led_classdev *led_cdev,
 				 enum led_brightness value, int color)
 {
 	/* led_brightness is 0, 127 or 255 - we just use it here as-is */
-	struct blinkm_led *led = cdev_to_blmled(led_cdev);
+	struct blinkm_led *led = monochrome_led_cdev_to_blmled(led_cdev);
 	struct blinkm_data *data = i2c_get_clientdata(led->i2c_client);

 	switch (color) {
@@ -570,7 +623,11 @@ static int blinkm_probe(struct i2c_client *client,
 			const struct i2c_device_id *id)
 {
 	struct blinkm_data *data;
-	struct blinkm_led *led[3];
+	/* For multicolor support */
+	struct blinkm_led *mc_led;
+	struct mc_subled *mc_led_info;
+	/* 3 seperate classes for red, green, and blue respectively */
+	struct blinkm_led *leds[3];
 	int err, i;
 	char blinkm_led_name[28];

@@ -581,6 +638,12 @@ static int blinkm_probe(struct i2c_client *client,
 		goto exit;
 	}

+	mc_led_info = devm_kmalloc_array(&client->dev, 3, sizeof(*mc_led_info),
+					GFP_KERNEL | __GFP_ZERO);
+	if (!mc_led_info) {
+		err = -ENOMEM;
+		goto exit;
+	}
 	data->i2c_addr = 0x08;
 	/* i2c addr  - use fake addr of 0x08 initially (real is 0x09) */
 	data->fw_ver = 0xfe;
@@ -599,28 +662,30 @@ static int blinkm_probe(struct i2c_client *client,
 		goto exit;
 	}

+
+	/* Register red, green, and blue sysfs classes */
 	for (i = 0; i < 3; i++) {
 		/* RED = 0, GREEN = 1, BLUE = 2 */
-		led[i] = &data->blinkm_leds[i];
-		led[i]->i2c_client = client;
-		led[i]->id = i;
-		led[i]->led_cdev.max_brightness = 255;
-		led[i]->led_cdev.flags = LED_CORE_SUSPENDRESUME;
+		leds[i] = &data->blinkm_leds[i];
+		leds[i]->i2c_client = client;
+		leds[i]->id = i;
+		leds[i]->monochrome_led_cdev.max_brightness = 255;
+		leds[i]->monochrome_led_cdev.flags = LED_CORE_SUSPENDRESUME;
 		switch (i) {
 		case RED:
 			snprintf(blinkm_led_name, sizeof(blinkm_led_name),
 					 "blinkm-%d-%d-red",
 					 client->adapter->nr,
 					 client->addr);
-			led[i]->led_cdev.name = blinkm_led_name;
-			led[i]->led_cdev.brightness_set_blocking =
+			leds[i]->monochrome_led_cdev.name = blinkm_led_name;
+			leds[i]->monochrome_led_cdev.brightness_set_blocking =
 							blinkm_led_red_set;
 			err = led_classdev_register(&client->dev,
-						    &led[i]->led_cdev);
+							&leds[i]->monochrome_led_cdev);
 			if (err < 0) {
 				dev_err(&client->dev,
 					"couldn't register LED %s\n",
-					led[i]->led_cdev.name);
+					leds[i]->monochrome_led_cdev.name);
 				goto failred;
 			}
 			break;
@@ -629,15 +694,15 @@ static int blinkm_probe(struct i2c_client *client,
 					 "blinkm-%d-%d-green",
 					 client->adapter->nr,
 					 client->addr);
-			led[i]->led_cdev.name = blinkm_led_name;
-			led[i]->led_cdev.brightness_set_blocking =
+			leds[i]->monochrome_led_cdev.name = blinkm_led_name;
+			leds[i]->monochrome_led_cdev.brightness_set_blocking =
 							blinkm_led_green_set;
 			err = led_classdev_register(&client->dev,
-						    &led[i]->led_cdev);
+							&leds[i]->monochrome_led_cdev);
 			if (err < 0) {
 				dev_err(&client->dev,
 					"couldn't register LED %s\n",
-					led[i]->led_cdev.name);
+					leds[i]->monochrome_led_cdev.name);
 				goto failgreen;
 			}
 			break;
@@ -646,34 +711,72 @@ static int blinkm_probe(struct i2c_client *client,
 					 "blinkm-%d-%d-blue",
 					 client->adapter->nr,
 					 client->addr);
-			led[i]->led_cdev.name = blinkm_led_name;
-			led[i]->led_cdev.brightness_set_blocking =
+			leds[i]->monochrome_led_cdev.name = blinkm_led_name;
+			leds[i]->monochrome_led_cdev.brightness_set_blocking =
 							blinkm_led_blue_set;
 			err = led_classdev_register(&client->dev,
-						    &led[i]->led_cdev);
+							&leds[i]->monochrome_led_cdev);
 			if (err < 0) {
 				dev_err(&client->dev,
 					"couldn't register LED %s\n",
-					led[i]->led_cdev.name);
+					leds[i]->monochrome_led_cdev.name);
 				goto failblue;
 			}
 			break;
 		}		/* end switch */
 	}			/* end for */

+
+
+	/* Register multicolor sysfs class */
+	mc_led = &data->mc_blinkm_led;
+	mc_led->i2c_client = client;
+	mc_led->id = 4;
+
+	mc_led_info[0].color_index = LED_COLOR_ID_RED;
+	mc_led_info[0].channel = 0;
+	mc_led_info[1].color_index = LED_COLOR_ID_GREEN;
+	mc_led_info[1].channel = 1;
+	mc_led_info[2].color_index = LED_COLOR_ID_BLUE;
+	mc_led_info[2].channel = 2;
+	mc_led->mcled_cdev.subled_info = mc_led_info;
+	mc_led->mcled_cdev.num_colors = NUM_LEDS;
+
+	mc_led->led_cdev = &mc_led->mcled_cdev.led_cdev;
+	mc_led->led_cdev->brightness = 255;
+	mc_led->led_cdev->max_brightness = 255;
+	mc_led->led_cdev->flags = LED_CORE_SUSPENDRESUME;
+	snprintf(blinkm_led_name, sizeof(blinkm_led_name),
+		 "blinkm-%d-%d-multi",
+		 client->adapter->nr,
+		 client->addr);
+	mc_led->led_cdev->name = blinkm_led_name;
+	mc_led->led_cdev->brightness_set_blocking =
+					blinkm_set_mc_brightness;
+
+	err = led_classdev_multicolor_register(&client->dev, &mc_led->mcled_cdev);
+	if (err < 0) {
+		dev_err(&client->dev, "couldn't register LED %s\n",
+				mc_led->led_cdev->name);
+		goto failmulti;
+	}
 	/* Initialize the blinkm */
 	blinkm_init_hw(client);

 	return 0;

+failmulti:
+	led_classdev_unregister(&leds[BLUE]->monochrome_led_cdev);
+
 failblue:
-	led_classdev_unregister(&led[GREEN]->led_cdev);
+	led_classdev_unregister(&leds[GREEN]->monochrome_led_cdev);

 failgreen:
-	led_classdev_unregister(&led[RED]->led_cdev);
+	led_classdev_unregister(&leds[RED]->monochrome_led_cdev);

 failred:
 	sysfs_remove_group(&client->dev.kobj, &blinkm_group);
+
 exit:
 	return err;
 }
@@ -685,8 +788,9 @@ static int blinkm_remove(struct i2c_client *client)
 	int i;

 	/* make sure no workqueue entries are pending */
+	led_classdev_unregister(&data->mc_blinkm_led.mcled_cdev.led_cdev);
 	for (i = 0; i < 3; i++)
-		led_classdev_unregister(&data->blinkm_leds[i].led_cdev);
+		led_classdev_unregister(&data->blinkm_leds[i].monochrome_led_cdev);

 	/* reset rgb */
 	data->next_red = 0x00;
--
2.37.2