led: core: RfC - add RGB LED handling to the core

When playing with a ThingM Blink(1) USB RGB LED device I found that there are few drivers for RGB LED's spread across the kernel and each one implements the RGB functionality in its own way.
Some examples:
- drivers/hid/hid-thingm.c
- drivers/usb/misc/usbled.c
- drivers/leds/leds-bd2802.c
- drivers/leds/leds-blinkm.c
...
IMHO it would make sense to add generic RGB functionality to the LED core.

Things I didn't like too much in other driver implementations:
- one led_classdev per color or at least
- one sysfs attribute per color
Colors are not really independent therefore I'd prefer one led_classdev per RGB LED. Also userspace should be able to change the color with one call -> therefore only one sysfs attribute for the RGB values.

Also the current brightness-related functionality should not be effected by the RGB extension.

This patch is intended to demonstrate the idea of the extension. Most likely it's not ready yet for submission.

Main idea is to base the effective RGB value on a combination of brightness and a scaled-to-max RGB value.
The RGB-related callbacks are basically the same as for brightness. RGB functionally can be switched on with a new flag in the led_classdev.flags bitmap.

Experimentally I changed the thingm driver to use this extension and it works quite well. As one result the driver could be very much simplified.

Any feedback is appreciated.

Signed-off-by Heiner Kallweit <[email protected]>

---
  drivers/leds/led-class.c |  62 +++++++++++++++++++++++++++
  drivers/leds/led-core.c  | 106 +++++++++++++++++++++++++++++++++++++++++++++++
  drivers/leds/leds.h      |  12 ++++++
  include/linux/leds.h     |  13 ++++++
  4 files changed, 193 insertions(+)

drivers/leds/led-class.c +62 -0

index 14139c3..5e4c2f2 100644
@@ -64,6 +64,47 @@ unlock:
  }
  static DEVICE_ATTR_RW(brightness);
 
+static ssize_t rgb_show(struct device *dev, struct device_attribute *attr,
+			char *buf)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+	u32 rgb_val;
+
+	mutex_lock(&led_cdev->led_access);
+	led_update_rgb_val(led_cdev);
+	rgb_val = led_get_rgb_val(led_cdev);
+	mutex_unlock(&led_cdev->led_access);
+
+	return sprintf(buf, "0x%06x\n", rgb_val);
+}
+
+static ssize_t rgb_store(struct device *dev, struct device_attribute *attr,
+			 const char *buf, size_t size)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+	unsigned long state;
+	ssize_t ret;
+
+	mutex_lock(&led_cdev->led_access);
+
+	if (led_sysfs_is_disabled(led_cdev)) {
+		ret = -EBUSY;
+		goto unlock;
+	}
+
+	ret = kstrtoul(buf, 0, &state);
+	if (ret)
+		goto unlock;
+
+	led_set_rgb_val(led_cdev, state);
+
+	ret = size;
+unlock:
+	mutex_unlock(&led_cdev->led_access);
+	return ret;
+}
+static DEVICE_ATTR_RW(rgb);
+
  static ssize_t max_brightness_show(struct device *dev,
  		struct device_attribute *attr, char *buf)
  {

@@ -190,6 +231,16 @@ int led_classdev_register(struct device *parent, struct led_classdev *led_cdev)
  	char name[64];
  	int ret;
 
+	/* FLASH is not supported for RGB LEDs so far
+	 * and RGB enforces max_brightness = LED_FULL.
+	 * Initialize the color as white.
+	 */
+	if (led_isRGB(led_cdev)) {
+		led_cdev->flags &= ~LED_DEV_CAP_FLASH;
+		led_cdev->max_brightness = LED_FULL;
+		led_cdev->rgb_val = 0xffffff;
+	}
+
  	ret = led_classdev_next_name(led_cdev->name, name, sizeof(name));
  	if (ret < 0)
  		return ret;

@@ -203,6 +254,14 @@ int led_classdev_register(struct device *parent, struct led_classdev *led_cdev)
  		dev_warn(parent, "Led %s renamed to %s due to name collision",
  				led_cdev->name, dev_name(led_cdev->dev));
 
+	if (led_isRGB(led_cdev)) {
+		ret = device_create_file(led_cdev->dev, &dev_attr_rgb);
+		if (ret) {
+			device_unregister(led_cdev->dev);
+			return ret;
+		}
+	}
+
  #ifdef CONFIG_LEDS_TRIGGERS
  	init_rwsem(&led_cdev->trigger_lock);
  #endif

@@ -252,6 +311,9 @@ void led_classdev_unregister(struct led_classdev *led_cdev)
 
  	flush_work(&led_cdev->set_brightness_work);
 
+	if (led_isRGB(led_cdev))
+		device_remove_file(led_cdev->dev, &dev_attr_rgb);
+
  	device_unregister(led_cdev->dev);
 
  	down_write(&leds_list_lock);

drivers/leds/led-core.c +106 -0

index 19e1e60..6563bd3 100644
@@ -25,6 +25,48 @@ EXPORT_SYMBOL_GPL(leds_list_lock);
  LIST_HEAD(leds_list);
  EXPORT_SYMBOL_GPL(leds_list);
 
+static void led_set_rgb_raw(struct led_classdev *cdev, u32 rgb)
+{
+	u32 red_raw = (rgb >> 16) & 0xff;
+	u32 green_raw = (rgb >> 8) & 0xff;
+	u32 blue_raw = rgb & 0xff;
+	u32 max_raw, red, green, blue;
+
+	max_raw = max(red_raw, green_raw);
+	if (blue_raw > max_raw)
+		max_raw = blue_raw;
+
+	if (!max_raw) {
+		cdev->brightness = 0;
+		cdev->rgb_val = 0;
+		return;
+	}
+
+	red = DIV_ROUND_CLOSEST(red_raw * LED_FULL, max_raw);
+	green = DIV_ROUND_CLOSEST(green_raw * LED_FULL, max_raw);
+	blue = DIV_ROUND_CLOSEST(blue_raw * LED_FULL, max_raw);
+
+	cdev->brightness = max_raw;
+	cdev->rgb_val = (red << 16) + (green << 8) + blue;
+}
+
+static u32 led_get_rgb_raw(struct led_classdev *cdev, bool delayed)
+{
+	u32 rgb = delayed ? cdev->delayed_rgb_value : cdev->rgb_val;
+	u32 brightness = delayed ? cdev->delayed_set_value :
+			 cdev->brightness;
+	u32 red = (rgb >> 16) & 0xff;
+	u32 green = (rgb >> 8) & 0xff;
+	u32 blue = rgb & 0xff;
+	u32 red_raw, green_raw, blue_raw;
+
+	red_raw = DIV_ROUND_CLOSEST(red * brightness, LED_FULL);
+	green_raw = DIV_ROUND_CLOSEST(green * brightness, LED_FULL);
+	blue_raw = DIV_ROUND_CLOSEST(blue * brightness, LED_FULL);
+
+	return (red_raw << 16) + (green_raw << 8) + blue_raw;
+}
+
  static void led_timer_function(unsigned long data)
  {
  	struct led_classdev *led_cdev = (void *)data;

@@ -91,6 +133,21 @@ static void set_brightness_delayed(struct work_struct *ws)
  		led_cdev->flags &= ~LED_BLINK_DISABLE;
  	}
 
+	if (led_isRGB(led_cdev)) {
+		u32 rgb = led_get_rgb_raw(led_cdev, true);
+
+		if (led_cdev->rgb_set)
+			led_cdev->rgb_set(led_cdev, rgb);
+		else if (led_cdev->rgb_set_blocking)
+			ret = led_cdev->rgb_set_blocking(led_cdev, rgb);
+		else
+			ret = -EOPNOTSUPP;
+		if (ret < 0)
+			dev_err(led_cdev->dev,
+				"Setting LED RGB value failed (%d)\n", ret);
+		return;
+	}
+
  	if (led_cdev->brightness_set)
  		led_cdev->brightness_set(led_cdev, led_cdev->delayed_set_value);
  	else if (led_cdev->brightness_set_blocking)

@@ -229,9 +286,35 @@ void led_set_brightness(struct led_classdev *led_cdev,
  }
  EXPORT_SYMBOL_GPL(led_set_brightness);
 
+void led_set_rgb_val(struct led_classdev *led_cdev, u32 rgb_val)
+{
+	if (!led_isRGB(led_cdev) || (led_cdev->flags & LED_SUSPENDED))
+		return;
+
+	led_set_rgb_raw(led_cdev, rgb_val);
+
+	if (led_cdev->rgb_set) {
+		led_cdev->rgb_set(led_cdev, rgb_val);
+		return;
+	}
+
+	/* If RGB setting can sleep, delegate it to a work queue task */
+	led_cdev->delayed_set_value = led_cdev->brightness;
+	led_cdev->delayed_rgb_value = led_cdev->rgb_val;
+	schedule_work(&led_cdev->set_brightness_work);
+}
+EXPORT_SYMBOL_GPL(led_set_rgb_val);
+
  void led_set_brightness_nopm(struct led_classdev *led_cdev,
  			      enum led_brightness value)
  {
+	if (led_isRGB(led_cdev) && led_cdev->rgb_set) {
+		u32 rgb = led_get_rgb_raw(led_cdev, false);
+
+		led_cdev->rgb_set(led_cdev, rgb);
+		return;
+	}
+
  	/* Use brightness_set op if available, it is guaranteed not to sleep */
  	if (led_cdev->brightness_set) {
  		led_cdev->brightness_set(led_cdev, value);

@@ -240,6 +323,7 @@ void led_set_brightness_nopm(struct led_classdev *led_cdev,
 
  	/* If brightness setting can sleep, delegate it to a work queue task */
  	led_cdev->delayed_set_value = value;
+	led_cdev->delayed_rgb_value = led_cdev->rgb_val;
  	schedule_work(&led_cdev->set_brightness_work);
  }
  EXPORT_SYMBOL_GPL(led_set_brightness_nopm);

@@ -267,6 +351,12 @@ int led_set_brightness_sync(struct led_classdev *led_cdev,
  	if (led_cdev->flags & LED_SUSPENDED)
  		return 0;
 
+	if (led_isRGB(led_cdev) && led_cdev->rgb_set_blocking) {
+		u32 rgb = led_get_rgb_raw(led_cdev, false);
+
+		return led_cdev->rgb_set_blocking(led_cdev, rgb);
+	}
+
  	if (led_cdev->brightness_set_blocking)
  		return led_cdev->brightness_set_blocking(led_cdev,
  							 led_cdev->brightness);

@@ -290,6 +380,22 @@ int led_update_brightness(struct led_classdev *led_cdev)
  }
  EXPORT_SYMBOL_GPL(led_update_brightness);
 
+int led_update_rgb_val(struct led_classdev *led_cdev)
+{
+	s32 ret = 0;
+
+	if (led_isRGB(led_cdev) && led_cdev->rgb_get) {
+		ret = led_cdev->rgb_get(led_cdev);
+		if (ret >= 0) {
+			led_set_rgb_raw(led_cdev, ret);
+			return 0;
+		}
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(led_update_rgb_val);
+
  /* Caller must ensure led_cdev->led_access held */
  void led_sysfs_disable(struct led_classdev *led_cdev)
  {

drivers/leds/leds.h +12 -0

index db3f20d..c733777 100644
@@ -16,17 +16,29 @@
  #include <linux/rwsem.h>
  #include <linux/leds.h>
 
+static inline bool led_isRGB(struct led_classdev *led_cdev)
+{
+	return (led_cdev->flags & LED_RGB) != 0;
+}
+
  static inline int led_get_brightness(struct led_classdev *led_cdev)
  {
  	return led_cdev->brightness;
  }
 
+static inline u32 led_get_rgb_val(struct led_classdev *led_cdev)
+{
+	return led_cdev->rgb_val;
+}
+
  void led_init_core(struct led_classdev *led_cdev);
  void led_stop_software_blink(struct led_classdev *led_cdev);
  void led_set_brightness_nopm(struct led_classdev *led_cdev,
  				enum led_brightness value);
  void led_set_brightness_nosleep(struct led_classdev *led_cdev,
  				enum led_brightness value);
+void led_set_rgb_val(struct led_classdev *led_cdev, u32 rgb_val);
+int led_update_rgb_val(struct led_classdev *led_cdev);
 
  extern struct rw_semaphore leds_list_lock;
  extern struct list_head leds_list;

include/linux/leds.h +13 -0

index 4429887..83d2912 100644
@@ -35,6 +35,7 @@ struct led_classdev {
  	const char		*name;
  	enum led_brightness	 brightness;
  	enum led_brightness	 max_brightness;
+	u32			 rgb_val;
  	int			 flags;
 
  	/* Lower 16 bits reflect status */

@@ -48,6 +49,7 @@ struct led_classdev {
  #define LED_BLINK_DISABLE	(1 << 21)
  #define LED_SYSFS_DISABLE	(1 << 22)
  #define LED_DEV_CAP_FLASH	(1 << 23)
+#define LED_RGB			(1 << 24)
 
  	/* Set LED brightness level */
  	/* Must not sleep. If no non-blocking version can be provided

@@ -56,15 +58,25 @@ struct led_classdev {
  	 */
  	void		(*brightness_set)(struct led_classdev *led_cdev,
  					  enum led_brightness brightness);
+
+	void		(*rgb_set)(struct led_classdev *led_cdev,
+				   u32 rgb_val);
  	/*
  	 * Set LED brightness level immediately - it can block the caller for
  	 * the time required for accessing a LED device register.
  	 */
  	int (*brightness_set_blocking)(struct led_classdev *led_cdev,
  				       enum led_brightness brightness);
+
+	int (*rgb_set_blocking)(struct led_classdev *led_cdev,
+				u32 rgb_val);
+
  	/* Get LED brightness level */
  	enum led_brightness (*brightness_get)(struct led_classdev *led_cdev);
 
+	/* Get LED RGB val */
+	s32 (*rgb_get)(struct led_classdev *led_cdev);
+
  	/*
  	 * Activate hardware accelerated blink, delays are in milliseconds
  	 * and if both are zero then a sensible default should be chosen.

@@ -90,6 +102,7 @@ struct led_classdev {
 
  	struct work_struct	set_brightness_work;
  	int			delayed_set_value;
+	u32			delayed_rgb_value;
 
  #ifdef CONFIG_LEDS_TRIGGERS
  	/* Protects the trigger data below */

Hi Heiner,

Thank you for the patch. We have to ask a question here - what benefits these modifications bring about. Isn't all of this achievable in the user space? And I think that RGB LED problem can be boiled down to even more generic one, namely: how to make all the sub-LEDs controllable with a single LED class device. This subject pops out from time to time and it'd be good to have it tackled finally. Please refer to [1] and follow the links.

[1] http://www.spinics.net/lists/linux-leds/msg04508.html

Best Regards,
Jacek Anaszewski

Quoted 35 lines

When playing with a ThingM Blink(1) USB RGB LED device I found that there are few drivers for RGB LED's spread across the kernel and each one implements the RGB functionality in its own way.
Some examples:
- drivers/hid/hid-thingm.c
- drivers/usb/misc/usbled.c
- drivers/leds/leds-bd2802.c
- drivers/leds/leds-blinkm.c
...
IMHO it would make sense to add generic RGB functionality to the LED core.

Things I didn't like too much in other driver implementations:
- one led_classdev per color or at least
- one sysfs attribute per color
Colors are not really independent therefore I'd prefer one led_classdev per RGB LED. Also userspace should be able to change the color with one call -> therefore only one sysfs attribute for the RGB values.

Also the current brightness-related functionality should not be effected by the RGB extension.

This patch is intended to demonstrate the idea of the extension. Most likely it's not ready yet for submission.

Main idea is to base the effective RGB value on a combination of brightness and a scaled-to-max RGB value.
The RGB-related callbacks are basically the same as for brightness. RGB functionally can be switched on with a new flag in the led_classdev.flags bitmap.

Experimentally I changed the thingm driver to use this extension and it works quite well. As one result the driver could be very much simplified.

Any feedback is appreciated.
[...]

Am 13.01.2016 um 10:42 schrieb Jacek Anaszewski:

Quoted 10 lines

Hi Heiner,

Thank you for the patch. We have to ask a question here - what benefits these modifications bring about. Isn't all of this achievable in the user space? And I think that RGB LED problem can be boiled down to even more generic one, namely: how to make all the sub-LEDs controllable with a single LED class device. This subject pops out from time to time and it'd be good to have it tackled finally. Please refer to [1] and follow the links.

Quoted 5 lines

[1] http://www.spinics.net/lists/linux-leds/msg04508.html

Best Regards,
Jacek Anaszewski

Thanks for the prompt feedback, Jacek.
Regarding RGB support I also think of eventually making setting the color available via triggers. This would allow kernel events to use colors. I think of use cases like:
- add RGB support to heartbeat trigger: Then, with increasing load, the color can change from green to red.

- combine temperature monitoring with a RGB LED trigger.
- in general visualizing parameters in the kernel with >2 states or a range of possible values

Therefore I think having RGB LED support in the kernel makes sense. And we have it anyway also as of today, just in different forms in different corners of the kernel code.
Prerequiste for RGB-based triggers is one central API for controlling RGB LEDs.

When we come to sub-LEDs:
I'd define sub-LEDs as LEDs which are somehow grouped but basically independent. I don't think this definition applies to a RGB LED as the three LEDs are not independent.
I had a look at the referenced mail thread, however the usecase wasn't clear to me. I understand that it's about making LEDs blink synchronously but there was no example what it could be good for. Can you list some usecases? Maybe once I see some usecases for sub-LEDs it becomes clearer what they have in common with RGB LEDs.

The primary question with sub-LEDs most likely is:
Which properties can be controlled on a per-LED basis and which ones only on a LED group basis?

(Or should it be supported that I can fiddle with properties on a per-LED and on a group basis in parallel?)
I'd assume that the following idea is not brand-new: I could imagine to have sysfs nodes for LED groups under /sys/class/leds including:
- attributes for the LED properties delegated to the group
- links to the led_classdev nodes of the respective LEDs in the group

Kind Regards, Heiner

Quoted 39 lines · Heiner Kallweit

Quoted 35 lines

When playing with a ThingM Blink(1) USB RGB LED device I found that there are few drivers for RGB LED's spread across the kernel and each one implements the RGB functionality in its own way.
Some examples:
- drivers/hid/hid-thingm.c
- drivers/usb/misc/usbled.c
- drivers/leds/leds-bd2802.c
- drivers/leds/leds-blinkm.c
...
IMHO it would make sense to add generic RGB functionality to the LED core.

Things I didn't like too much in other driver implementations:
- one led_classdev per color or at least
- one sysfs attribute per color
Colors are not really independent therefore I'd prefer one led_classdev per RGB LED. Also userspace should be able to change the color with one call -> therefore only one sysfs attribute for the RGB values.

Also the current brightness-related functionality should not be effected by the RGB extension.

This patch is intended to demonstrate the idea of the extension. Most likely it's not ready yet for submission.

Main idea is to base the effective RGB value on a combination of brightness and a scaled-to-max RGB value.
The RGB-related callbacks are basically the same as for brightness. RGB functionally can be switched on with a new flag in the led_classdev.flags bitmap.

Experimentally I changed the thingm driver to use this extension and it works quite well. As one result the driver could be very much simplified.

Any feedback is appreciated.
[...]

Quoted 17 lines · Heiner Kallweit

Am 13.01.2016 um 10:42 schrieb Jacek Anaszewski:

Quoted 10 lines

Hi Heiner,

Thank you for the patch. We have to ask a question here - what benefits these modifications bring about. Isn't all of this achievable in the user space? And I think that RGB LED problem can be boiled down to even more generic one, namely: how to make all the sub-LEDs controllable with a single LED class device. This subject pops out from time to time and it'd be good to have it tackled finally. Please refer to [1] and follow the links.

Quoted 5 lines

[1] http://www.spinics.net/lists/linux-leds/msg04508.html

Best Regards,
Jacek Anaszewski

Quoted 6 lines

Thanks for the prompt feedback, Jacek.
Regarding RGB support I also think of eventually making setting the color available via triggers. This would allow kernel events to use colors. I think of use cases like:
- add RGB support to heartbeat trigger: Then, with increasing load, the color can change from green to red.

What do you mean by increasing load? Heartbeat is just blinking with specific time intervals and fixed brightness.

Quoted 12 lines

- combine temperature monitoring with a RGB LED trigger.
- in general visualizing parameters in the kernel with >2 states or a range
of possible values

Therefore I think having RGB LED support in the kernel makes sense. And we have it anyway also as of today, just in different forms in different
corners of the kernel code.
Prerequiste for RGB-based triggers is one central API for controlling RGB LEDs.

Quoted 14 lines

When we come to sub-LEDs:
I'd define sub-LEDs as LEDs which are somehow grouped but basically independent.
I don't think this definition applies to a RGB LED as the three LEDs are not
independent.
I had a look at the referenced mail thread, however the usecase wasn't clear
to me. I understand that it's about making LEDs blink synchronously but there was no example what it could be good for. Can you list some usecases?
Maybe once I see some usecases for sub-LEDs it becomes clearer what they have
in common with RGB LEDs.

I got influenced by the fact that led-blinkm driver exposes separate LED class devices per each color component. It automatically brought sync subLEDs problem to my mind. The main benefit of synchronizing subLEDs would be possibility of setting a trigger per group of subLEDs, but current API doesn't allow for setting different brightness per triggered subLED, so it wouldn't work properly for RGB LEDs.

It was not an issue in case of flash LEDs, from which the subject arose originally. Only strobe operation was to be synchronized then and brightness could had been set in one of the previous steps.

max77693-flash device driver was being developed then (drivers/leds/leds-max77693.c), which exposes two current outputs, that are controlled separately, but can be joint to double the maximum current for the flash strobe of a single LED. Synchronization would be beneficial for the case when two separate LEDs are connected to both outputs and we'd like to synchronize the flash strobe. Strobing both LEDs required accessing a single register.

All the above seem to be irrelevant for RGB LEDs case.

Nonetheless, one straightforward idea came to my mind - we could expose RGB LED as a single LED class device and control the intensity of each color as usual with brightness attribute.

It is of enum type (i.e. at least 4 bytes), so each color component could be stored in one byte. With this approach no changes would be needed to the LED core to meet requirements from your RFC. Please let me know if I missed something?

When playing with a ThingM Blink(1) USB RGB LED device I found that there are few drivers for RGB LED's spread across the kernel and each one implements the RGB functionality in its own way.
Some examples:
- drivers/hid/hid-thingm.c
- drivers/usb/misc/usbled.c
- drivers/leds/leds-bd2802.c
- drivers/leds/leds-blinkm.c
...
IMHO it would make sense to add generic RGB functionality to the LED core.

Things I didn't like too much in other driver implementations:
- one led_classdev per color or at least
- one sysfs attribute per color
Colors are not really independent therefore I'd prefer one led_classdev per RGB LED. Also userspace should be able to change the color with one call -> therefore only one sysfs attribute for the RGB values.

Also the current brightness-related functionality should not be effected by the RGB extension.

This patch is intended to demonstrate the idea of the extension. Most likely it's not ready yet for submission.

Main idea is to base the effective RGB value on a combination of brightness and a scaled-to-max RGB value.
The RGB-related callbacks are basically the same as for brightness. RGB functionally can be switched on with a new flag in the led_classdev.flags bitmap.

Experimentally I changed the thingm driver to use this extension and it works quite well. As one result the driver could be very much simplified.

Any feedback is appreciated.

Signed-off-by Heiner Kallweit <[email protected]>

---
  drivers/leds/led-class.c |  62 +++++++++++++++++++++++++++
  drivers/leds/led-core.c  | 106 +++++++++++++++++++++++++++++++++++++++++++++++
  drivers/leds/leds.h      |  12 ++++++
  include/linux/leds.h     |  13 ++++++
  4 files changed, 193 insertions(+)

drivers/leds/led-class.c +23 -0

index 14139c3..5e4c2f2 100644
@@ -64,6 +64,47 @@ unlock:
  }
  static DEVICE_ATTR_RW(brightness);
 
+static ssize_t rgb_show(struct device *dev, struct device_attribute *attr,
+			char *buf)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+	u32 rgb_val;
+
+	mutex_lock(&led_cdev->led_access);
+	led_update_rgb_val(led_cdev);
+	rgb_val = led_get_rgb_val(led_cdev);
+	mutex_unlock(&led_cdev->led_access);
+
+	return sprintf(buf, "0x%06x\n", rgb_val);
+}
+
+static ssize_t rgb_store(struct device *dev, struct device_attribute *attr,
+			 const char *buf, size_t size)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+	unsigned long state;
+	ssize_t ret;
+
+	mutex_lock(&led_cdev->led_access);
+

drivers/leds/led-class.c +8 -0

+
+	ret = size;
+unlock:
+	mutex_unlock(&led_cdev->led_access);
+	return ret;
+}
+static DEVICE_ATTR_RW(rgb);
+
  static ssize_t max_brightness_show(struct device *dev,
  		struct device_attribute *attr, char *buf)
  {

drivers/leds/led-class.c +9 -0

+		if (ret) {
+			device_unregister(led_cdev->dev);
+			return ret;
+		}
+	}
+
  #ifdef CONFIG_LEDS_TRIGGERS
  	init_rwsem(&led_cdev->trigger_lock);
  #endif

@@ -252,6 +311,9 @@ void led_classdev_unregister(struct led_classdev *led_cdev)
 
  	flush_work(&led_cdev->set_brightness_work);
 
+	if (led_isRGB(led_cdev))
+		device_remove_file(led_cdev->dev, &dev_attr_rgb);
+
  	device_unregister(led_cdev->dev);
 
  	down_write(&leds_list_lock);

drivers/leds/led-core.c +14 -0

index 19e1e60..6563bd3 100644
@@ -25,6 +25,48 @@ EXPORT_SYMBOL_GPL(leds_list_lock);
  LIST_HEAD(leds_list);
  EXPORT_SYMBOL_GPL(leds_list);
 
+static void led_set_rgb_raw(struct led_classdev *cdev, u32 rgb)
+{
+	u32 red_raw = (rgb >> 16) & 0xff;
+	u32 green_raw = (rgb >> 8) & 0xff;
+	u32 blue_raw = rgb & 0xff;
+	u32 max_raw, red, green, blue;
+
+	max_raw = max(red_raw, green_raw);
+	if (blue_raw > max_raw)
+		max_raw = blue_raw;
+
+	if (!max_raw) {
+		cdev->brightness = 0;
+		cdev->rgb_val = 0;

drivers/leds/led-core.c +22 -0

+static u32 led_get_rgb_raw(struct led_classdev *cdev, bool delayed)
+{
+	u32 rgb = delayed ? cdev->delayed_rgb_value : cdev->rgb_val;
+	u32 brightness = delayed ? cdev->delayed_set_value :
+			 cdev->brightness;
+	u32 red = (rgb >> 16) & 0xff;
+	u32 green = (rgb >> 8) & 0xff;
+	u32 blue = rgb & 0xff;
+	u32 red_raw, green_raw, blue_raw;
+
+	red_raw = DIV_ROUND_CLOSEST(red * brightness, LED_FULL);
+	green_raw = DIV_ROUND_CLOSEST(green * brightness, LED_FULL);
+	blue_raw = DIV_ROUND_CLOSEST(blue * brightness, LED_FULL);
+
+	return (red_raw << 16) + (green_raw << 8) + blue_raw;
+}
+
  static void led_timer_function(unsigned long data)
  {
  	struct led_classdev *led_cdev = (void *)data;

@@ -91,6 +133,21 @@ static void set_brightness_delayed(struct work_struct *ws)
  		led_cdev->flags &= ~LED_BLINK_DISABLE;
  	}
 
+	if (led_isRGB(led_cdev)) {
+		u32 rgb = led_get_rgb_raw(led_cdev, true);
+
+		if (led_cdev->rgb_set)
+			led_cdev->rgb_set(led_cdev, rgb);

drivers/leds/led-core.c +6 -0

  	if (led_cdev->brightness_set)
  		led_cdev->brightness_set(led_cdev, led_cdev->delayed_set_value);
  	else if (led_cdev->brightness_set_blocking)

@@ -229,9 +286,35 @@ void led_set_brightness(struct led_classdev *led_cdev,
  }
  EXPORT_SYMBOL_GPL(led_set_brightness);
 
+void led_set_rgb_val(struct led_classdev *led_cdev, u32 rgb_val)
+{
+	if (!led_isRGB(led_cdev) || (led_cdev->flags & LED_SUSPENDED))
+		return;
+
+	led_set_rgb_raw(led_cdev, rgb_val);

drivers/leds/led-core.c +33 -0

+}
+EXPORT_SYMBOL_GPL(led_set_rgb_val);
+
  void led_set_brightness_nopm(struct led_classdev *led_cdev,
  			      enum led_brightness value)
  {
+	if (led_isRGB(led_cdev) && led_cdev->rgb_set) {
+		u32 rgb = led_get_rgb_raw(led_cdev, false);
+
+		led_cdev->rgb_set(led_cdev, rgb);
+		return;
+	}
+
  	/* Use brightness_set op if available, it is guaranteed not to sleep */
  	if (led_cdev->brightness_set) {
  		led_cdev->brightness_set(led_cdev, value);

@@ -240,6 +323,7 @@ void led_set_brightness_nopm(struct led_classdev *led_cdev,
 
  	/* If brightness setting can sleep, delegate it to a work queue task */
  	led_cdev->delayed_set_value = value;
+	led_cdev->delayed_rgb_value = led_cdev->rgb_val;
  	schedule_work(&led_cdev->set_brightness_work);
  }
  EXPORT_SYMBOL_GPL(led_set_brightness_nopm);

@@ -267,6 +351,12 @@ int led_set_brightness_sync(struct led_classdev *led_cdev,
  	if (led_cdev->flags & LED_SUSPENDED)
  		return 0;
 
+	if (led_isRGB(led_cdev) && led_cdev->rgb_set_blocking) {
+		u32 rgb = led_get_rgb_raw(led_cdev, false);
+
+		return led_cdev->rgb_set_blocking(led_cdev, rgb);
+	}
+
  	if (led_cdev->brightness_set_blocking)
  		return led_cdev->brightness_set_blocking(led_cdev,
  							 led_cdev->brightness);

@@ -290,6 +380,22 @@ int led_update_brightness(struct led_classdev *led_cdev)
  }
  EXPORT_SYMBOL_GPL(led_update_brightness);
 
+int led_update_rgb_val(struct led_classdev *led_cdev)
+{
+	s32 ret = 0;
+
+	if (led_isRGB(led_cdev) && led_cdev->rgb_get) {
+		ret = led_cdev->rgb_get(led_cdev);
+		if (ret >= 0) {
+			led_set_rgb_raw(led_cdev, ret);
+			return 0;
+		}
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(led_update_rgb_val);
+
  /* Caller must ensure led_cdev->led_access held */
  void led_sysfs_disable(struct led_classdev *led_cdev)
  {

drivers/leds/leds.h +12 -0

index db3f20d..c733777 100644
@@ -16,17 +16,29 @@
  #include <linux/rwsem.h>
  #include <linux/leds.h>
 
+static inline bool led_isRGB(struct led_classdev *led_cdev)
+{
+	return (led_cdev->flags & LED_RGB) != 0;
+}
+
  static inline int led_get_brightness(struct led_classdev *led_cdev)
  {
  	return led_cdev->brightness;
  }
 
+static inline u32 led_get_rgb_val(struct led_classdev *led_cdev)
+{
+	return led_cdev->rgb_val;
+}
+
  void led_init_core(struct led_classdev *led_cdev);
  void led_stop_software_blink(struct led_classdev *led_cdev);
  void led_set_brightness_nopm(struct led_classdev *led_cdev,
  				enum led_brightness value);
  void led_set_brightness_nosleep(struct led_classdev *led_cdev,
  				enum led_brightness value);
+void led_set_rgb_val(struct led_classdev *led_cdev, u32 rgb_val);
+int led_update_rgb_val(struct led_classdev *led_cdev);
 
  extern struct rw_semaphore leds_list_lock;
  extern struct list_head leds_list;

include/linux/leds.h +1 -0

index 4429887..83d2912 100644
@@ -35,6 +35,7 @@ struct led_classdev {
  	const char		*name;
  	enum led_brightness	 brightness;
  	enum led_brightness	 max_brightness;
+	u32			 rgb_val;
  	int			 flags;
 
  	/* Lower 16 bits reflect status */

include/linux/leds.h +0 -0

  #ifdef CONFIG_LEDS_TRIGGERS
  	/* Protects the trigger data below */
 

Quoted 78 lines

When playing with a ThingM Blink(1) USB RGB LED device I found that there are few drivers for RGB LED's spread across the kernel and each one implements the RGB functionality in its own way.
Some examples:
- drivers/hid/hid-thingm.c
- drivers/usb/misc/usbled.c
- drivers/leds/leds-bd2802.c
- drivers/leds/leds-blinkm.c
...
IMHO it would make sense to add generic RGB functionality to the LED core.

Things I didn't like too much in other driver implementations:
- one led_classdev per color or at least
- one sysfs attribute per color
Colors are not really independent therefore I'd prefer one led_classdev per RGB LED. Also userspace should be able to change the color with one call -> therefore only one sysfs attribute for the RGB values.

Also the current brightness-related functionality should not be effected by the RGB extension.

This patch is intended to demonstrate the idea of the extension. Most likely it's not ready yet for submission.

Main idea is to base the effective RGB value on a combination of brightness and a scaled-to-max RGB value.
The RGB-related callbacks are basically the same as for brightness. RGB functionally can be switched on with a new flag in the led_classdev.flags bitmap.

Experimentally I changed the thingm driver to use this extension and it works quite well. As one result the driver could be very much simplified.

Any feedback is appreciated.

Signed-off-by Heiner Kallweit <[email protected]>

---
  drivers/leds/led-class.c |  62 +++++++++++++++++++++++++++
  drivers/leds/led-core.c  | 106 +++++++++++++++++++++++++++++++++++++++++++++++
  drivers/leds/leds.h      |  12 ++++++
  include/linux/leds.h     |  13 ++++++
  4 files changed, 193 insertions(+)

drivers/leds/led-class.c +27 -0

index 14139c3..5e4c2f2 100644
@@ -64,6 +64,47 @@ unlock:
  }
  static DEVICE_ATTR_RW(brightness);
 
+static ssize_t rgb_show(struct device *dev, struct device_attribute *attr,
+            char *buf)
+{
+    struct led_classdev *led_cdev = dev_get_drvdata(dev);
+    u32 rgb_val;
+
+    mutex_lock(&led_cdev->led_access);
+    led_update_rgb_val(led_cdev);
+    rgb_val = led_get_rgb_val(led_cdev);
+    mutex_unlock(&led_cdev->led_access);
+
+    return sprintf(buf, "0x%06x\n", rgb_val);
+}
+
+static ssize_t rgb_store(struct device *dev, struct device_attribute *attr,
+             const char *buf, size_t size)
+{
+    struct led_classdev *led_cdev = dev_get_drvdata(dev);
+    unsigned long state;
+    ssize_t ret;
+
+    mutex_lock(&led_cdev->led_access);
+
+    if (led_sysfs_is_disabled(led_cdev)) {
+        ret = -EBUSY;
+        goto unlock;
+    }

drivers/leds/led-class.c unlock: · line 99

Quoted 6 lines

drivers/leds/led-class.c +6 -0

+
+    ret = kstrtoul(buf, 0, &state);
+    if (ret)
+        goto unlock;
+
+    led_set_rgb_val(led_cdev, state);

Please adhere to the current LED API naming style: led_rgb_set.

Quoted 15 lines

drivers/leds/led-class.c +8 -0

+
+    ret = size;
+unlock:
+    mutex_unlock(&led_cdev->led_access);
+    return ret;
+}
+static DEVICE_ATTR_RW(rgb);
+
  static ssize_t max_brightness_show(struct device *dev,
          struct device_attribute *attr, char *buf)
  {

@@ -190,6 +231,16 @@ int led_classdev_register(struct device *parent, struct led_classdev *led_cdev)
      char name[64];
      int ret;
 

drivers/leds/led-class.c int led_classdev_register(struct device *parent, struct led_classdev *led_cdev) · line 238

Quoted 5 lines

drivers/leds/led-class.c +5 -0

+    /* FLASH is not supported for RGB LEDs so far
+     * and RGB enforces max_brightness = LED_FULL.
+     * Initialize the color as white.
+     */
+    if (led_isRGB(led_cdev)) {

Don't use camel case. Change it to led_is_rgb or check the flag directly.

drivers/leds/led-class.c int led_classdev_register(struct device *parent, struct led_classdev *led_cdev) · line 239

Quoted 1 line

drivers/leds/led-class.c +1 -0

+        led_cdev->flags &= ~LED_DEV_CAP_FLASH;

I'd rather check whether both FLASH and RGB flag are set and return error in this case.

Quoted 11 lines

drivers/leds/led-class.c +4 -0

+        led_cdev->max_brightness = LED_FULL;
+        led_cdev->rgb_val = 0xffffff;
+    }
+
      ret = led_classdev_next_name(led_cdev->name, name, sizeof(name));
      if (ret < 0)
          return ret;

@@ -203,6 +254,14 @@ int led_classdev_register(struct device *parent, struct led_classdev *led_cdev)
          dev_warn(parent, "Led %s renamed to %s due to name collision",
                  led_cdev->name, dev_name(led_cdev->dev));
 

drivers/leds/led-class.c int led_classdev_register(struct device *parent, struct led_classdev *led_cdev) · line 258

Quoted 2 lines

drivers/leds/led-class.c +2 -0

+    if (led_isRGB(led_cdev)) {
+        ret = device_create_file(led_cdev->dev, &dev_attr_rgb);

Please use led_cdev->groups for this. You can refer to drivers/leds/led-class-flash.c.

Quoted 47 lines

drivers/leds/led-class.c +9 -0

+        if (ret) {
+            device_unregister(led_cdev->dev);
+            return ret;
+        }
+    }
+
  #ifdef CONFIG_LEDS_TRIGGERS
      init_rwsem(&led_cdev->trigger_lock);
  #endif

@@ -252,6 +311,9 @@ void led_classdev_unregister(struct led_classdev *led_cdev)
 
      flush_work(&led_cdev->set_brightness_work);
 
+    if (led_isRGB(led_cdev))
+        device_remove_file(led_cdev->dev, &dev_attr_rgb);
+
      device_unregister(led_cdev->dev);
 
      down_write(&leds_list_lock);

drivers/leds/led-core.c +20 -0

index 19e1e60..6563bd3 100644
@@ -25,6 +25,48 @@ EXPORT_SYMBOL_GPL(leds_list_lock);
  LIST_HEAD(leds_list);
  EXPORT_SYMBOL_GPL(leds_list);
 
+static void led_set_rgb_raw(struct led_classdev *cdev, u32 rgb)
+{
+    u32 red_raw = (rgb >> 16) & 0xff;
+    u32 green_raw = (rgb >> 8) & 0xff;
+    u32 blue_raw = rgb & 0xff;
+    u32 max_raw, red, green, blue;
+
+    max_raw = max(red_raw, green_raw);
+    if (blue_raw > max_raw)
+        max_raw = blue_raw;
+
+    if (!max_raw) {
+        cdev->brightness = 0;
+        cdev->rgb_val = 0;
+        return;
+    }
+
+    red = DIV_ROUND_CLOSEST(red_raw * LED_FULL, max_raw);
+    green = DIV_ROUND_CLOSEST(green_raw * LED_FULL, max_raw);
+    blue = DIV_ROUND_CLOSEST(blue_raw * LED_FULL, max_raw);

drivers/leds/led-core.c EXPORT_SYMBOL_GPL(leds_list_lock); · line 51

Quoted 4 lines

drivers/leds/led-core.c +4 -0

+
+    cdev->brightness = max_raw;
+    cdev->rgb_val = (red << 16) + (green << 8) + blue;
+}

I think that we shouldn't impose specific way of calculating brightness depending on the rgb value set. We should just pass value from userspace to the driver.

Quoted 33 lines

drivers/leds/led-core.c +26 -0

+static u32 led_get_rgb_raw(struct led_classdev *cdev, bool delayed)
+{
+    u32 rgb = delayed ? cdev->delayed_rgb_value : cdev->rgb_val;
+    u32 brightness = delayed ? cdev->delayed_set_value :
+             cdev->brightness;
+    u32 red = (rgb >> 16) & 0xff;
+    u32 green = (rgb >> 8) & 0xff;
+    u32 blue = rgb & 0xff;
+    u32 red_raw, green_raw, blue_raw;
+
+    red_raw = DIV_ROUND_CLOSEST(red * brightness, LED_FULL);
+    green_raw = DIV_ROUND_CLOSEST(green * brightness, LED_FULL);
+    blue_raw = DIV_ROUND_CLOSEST(blue * brightness, LED_FULL);
+
+    return (red_raw << 16) + (green_raw << 8) + blue_raw;
+}
+
  static void led_timer_function(unsigned long data)
  {
      struct led_classdev *led_cdev = (void *)data;

@@ -91,6 +133,21 @@ static void set_brightness_delayed(struct work_struct *ws)
          led_cdev->flags &= ~LED_BLINK_DISABLE;
      }
 
+    if (led_isRGB(led_cdev)) {
+        u32 rgb = led_get_rgb_raw(led_cdev, true);
+
+        if (led_cdev->rgb_set)
+            led_cdev->rgb_set(led_cdev, rgb);
+        else if (led_cdev->rgb_set_blocking)
+            ret = led_cdev->rgb_set_blocking(led_cdev, rgb);
+        else
+            ret = -EOPNOTSUPP;

drivers/leds/led-core.c static void set_brightness_delayed(struct work_struct *ws) · line 150

Quoted 6 lines

drivers/leds/led-core.c +6 -0

+        if (ret < 0)
+            dev_err(led_cdev->dev,
+                "Setting LED RGB value failed (%d)\n", ret);
+        return;
+    }
+

Why this is needed? Could you share a use case?

Quoted 13 lines

drivers/leds/led-core.c +6 -0

      if (led_cdev->brightness_set)
          led_cdev->brightness_set(led_cdev, led_cdev->delayed_set_value);
      else if (led_cdev->brightness_set_blocking)

@@ -229,9 +286,35 @@ void led_set_brightness(struct led_classdev *led_cdev,
  }
  EXPORT_SYMBOL_GPL(led_set_brightness);
 
+void led_set_rgb_val(struct led_classdev *led_cdev, u32 rgb_val)
+{
+    if (!led_isRGB(led_cdev) || (led_cdev->flags & LED_SUSPENDED))
+        return;
+
+    led_set_rgb_raw(led_cdev, rgb_val);

drivers/leds/led-core.c void led_set_brightness(struct led_classdev *led_cdev, · line 304

Quoted 10 lines

drivers/leds/led-core.c +10 -0

+
+    if (led_cdev->rgb_set) {
+        led_cdev->rgb_set(led_cdev, rgb_val);
+        return;
+    }
+
+    /* If RGB setting can sleep, delegate it to a work queue task */
+    led_cdev->delayed_set_value = led_cdev->brightness;
+    led_cdev->delayed_rgb_value = led_cdev->rgb_val;
+    schedule_work(&led_cdev->set_brightness_work);

I think that settings should be written to the device only in the brightness_set op. We wouldn't need additional rgb op then.

Quoted 105 lines

drivers/leds/led-core.c +33 -0

+}
+EXPORT_SYMBOL_GPL(led_set_rgb_val);
+
  void led_set_brightness_nopm(struct led_classdev *led_cdev,
                    enum led_brightness value)
  {
+    if (led_isRGB(led_cdev) && led_cdev->rgb_set) {
+        u32 rgb = led_get_rgb_raw(led_cdev, false);
+
+        led_cdev->rgb_set(led_cdev, rgb);
+        return;
+    }
+
      /* Use brightness_set op if available, it is guaranteed not to sleep */
      if (led_cdev->brightness_set) {
          led_cdev->brightness_set(led_cdev, value);

@@ -240,6 +323,7 @@ void led_set_brightness_nopm(struct led_classdev *led_cdev,
 
      /* If brightness setting can sleep, delegate it to a work queue task */
      led_cdev->delayed_set_value = value;
+    led_cdev->delayed_rgb_value = led_cdev->rgb_val;
      schedule_work(&led_cdev->set_brightness_work);
  }
  EXPORT_SYMBOL_GPL(led_set_brightness_nopm);

@@ -267,6 +351,12 @@ int led_set_brightness_sync(struct led_classdev *led_cdev,
      if (led_cdev->flags & LED_SUSPENDED)
          return 0;
 
+    if (led_isRGB(led_cdev) && led_cdev->rgb_set_blocking) {
+        u32 rgb = led_get_rgb_raw(led_cdev, false);
+
+        return led_cdev->rgb_set_blocking(led_cdev, rgb);
+    }
+
      if (led_cdev->brightness_set_blocking)
          return led_cdev->brightness_set_blocking(led_cdev,
                               led_cdev->brightness);

@@ -290,6 +380,22 @@ int led_update_brightness(struct led_classdev *led_cdev)
  }
  EXPORT_SYMBOL_GPL(led_update_brightness);
 
+int led_update_rgb_val(struct led_classdev *led_cdev)
+{
+    s32 ret = 0;
+
+    if (led_isRGB(led_cdev) && led_cdev->rgb_get) {
+        ret = led_cdev->rgb_get(led_cdev);
+        if (ret >= 0) {
+            led_set_rgb_raw(led_cdev, ret);
+            return 0;
+        }
+    }
+
+    return ret;
+}
+EXPORT_SYMBOL_GPL(led_update_rgb_val);
+
  /* Caller must ensure led_cdev->led_access held */
  void led_sysfs_disable(struct led_classdev *led_cdev)
  {

drivers/leds/leds.h +12 -0

index db3f20d..c733777 100644
@@ -16,17 +16,29 @@
  #include <linux/rwsem.h>
  #include <linux/leds.h>
 
+static inline bool led_isRGB(struct led_classdev *led_cdev)
+{
+    return (led_cdev->flags & LED_RGB) != 0;
+}
+
  static inline int led_get_brightness(struct led_classdev *led_cdev)
  {
      return led_cdev->brightness;
  }
 
+static inline u32 led_get_rgb_val(struct led_classdev *led_cdev)
+{
+    return led_cdev->rgb_val;
+}
+
  void led_init_core(struct led_classdev *led_cdev);
  void led_stop_software_blink(struct led_classdev *led_cdev);
  void led_set_brightness_nopm(struct led_classdev *led_cdev,
                  enum led_brightness value);
  void led_set_brightness_nosleep(struct led_classdev *led_cdev,
                  enum led_brightness value);
+void led_set_rgb_val(struct led_classdev *led_cdev, u32 rgb_val);
+int led_update_rgb_val(struct led_classdev *led_cdev);
 
  extern struct rw_semaphore leds_list_lock;
  extern struct list_head leds_list;

include/linux/leds.h +1 -0

index 4429887..83d2912 100644
@@ -35,6 +35,7 @@ struct led_classdev {
      const char        *name;
      enum led_brightness     brightness;
      enum led_brightness     max_brightness;
+    u32             rgb_val;
      int             flags;
 

include/linux/leds.h struct led_classdev { · line 52

Quoted 6 lines

include/linux/leds.h +1 -0

      /* Lower 16 bits reflect status */

@@ -48,6 +49,7 @@ struct led_classdev {
  #define LED_BLINK_DISABLE    (1 << 21)
  #define LED_SYSFS_DISABLE    (1 << 22)
  #define LED_DEV_CAP_FLASH    (1 << 23)
+#define LED_RGB            (1 << 24)

Please rename it to LED_DEV_CAP_RGB.

include/linux/leds.h struct led_classdev { · line 63

Quoted 10 lines

include/linux/leds.h +3 -0

      /* Set LED brightness level */
      /* Must not sleep. If no non-blocking version can be provided

@@ -56,15 +58,25 @@ struct led_classdev {
       */
      void        (*brightness_set)(struct led_classdev *led_cdev,
                        enum led_brightness brightness);
+
+    void        (*rgb_set)(struct led_classdev *led_cdev,
+                   u32 rgb_val);

Without this everything would be much simpler.

If we wanted to change the color and intensity we would need to:
1. Set rgb (value only cached in the LED core)
2. Set brightness (write both brightness and rgb settings to the hw)

Current brightness can always be obtained with led_get_brightness in case only rgb is to be set.

include/linux/leds.h struct led_classdev { · line 73

Quoted 10 lines

include/linux/leds.h +4 -0

      /*
       * Set LED brightness level immediately - it can block the caller for
       * the time required for accessing a LED device register.
       */
      int (*brightness_set_blocking)(struct led_classdev *led_cdev,
                         enum led_brightness brightness);
+
+    int (*rgb_set_blocking)(struct led_classdev *led_cdev,
+                u32 rgb_val);
+

This is also not needed.

include/linux/leds.h struct led_classdev { · line 79

Quoted 6 lines

include/linux/leds.h +3 -0

      /* Get LED brightness level */
      enum led_brightness (*brightness_get)(struct led_classdev *led_cdev);
 
+    /* Get LED RGB val */
+    s32 (*rgb_get)(struct led_classdev *led_cdev);
+

Ditto.

Quoted 5 lines

include/linux/leds.h +0 -0

      /*
       * Activate hardware accelerated blink, delays are in milliseconds
       * and if both are zero then a sensible default should be chosen.

@@ -90,6 +102,7 @@ struct led_classdev {
 

include/linux/leds.h struct led_classdev { · line 105

Quoted 3 lines

include/linux/leds.h +1 -0

      struct work_struct    set_brightness_work;
      int            delayed_set_value;
+    u32            delayed_rgb_value;

Ditto.

include/linux/leds.h struct led_classdev {

Quoted 4 lines

include/linux/leds.h +0 -0

  #ifdef CONFIG_LEDS_TRIGGERS
      /* Protects the trigger data below */
 

Am 14.01.2016 um 13:08 schrieb Jacek Anaszewski:

Quoted 79 lines · Heiner Kallweit

Quoted 78 lines

When playing with a ThingM Blink(1) USB RGB LED device I found that there are few drivers for RGB LED's spread across the kernel and each one implements the RGB functionality in its own way.
Some examples:
- drivers/hid/hid-thingm.c
- drivers/usb/misc/usbled.c
- drivers/leds/leds-bd2802.c
- drivers/leds/leds-blinkm.c
...
IMHO it would make sense to add generic RGB functionality to the LED core.

Things I didn't like too much in other driver implementations:
- one led_classdev per color or at least
- one sysfs attribute per color
Colors are not really independent therefore I'd prefer one led_classdev per RGB LED. Also userspace should be able to change the color with one call -> therefore only one sysfs attribute for the RGB values.

Also the current brightness-related functionality should not be effected by the RGB extension.

This patch is intended to demonstrate the idea of the extension. Most likely it's not ready yet for submission.

Main idea is to base the effective RGB value on a combination of brightness and a scaled-to-max RGB value.
The RGB-related callbacks are basically the same as for brightness. RGB functionally can be switched on with a new flag in the led_classdev.flags bitmap.

Experimentally I changed the thingm driver to use this extension and it works quite well. As one result the driver could be very much simplified.

Any feedback is appreciated.

Signed-off-by Heiner Kallweit <[email protected]>

---
   drivers/leds/led-class.c |  62 +++++++++++++++++++++++++++
   drivers/leds/led-core.c  | 106 +++++++++++++++++++++++++++++++++++++++++++++++
   drivers/leds/leds.h      |  12 ++++++
   include/linux/leds.h     |  13 ++++++
   4 files changed, 193 insertions(+)

drivers/leds/led-class.c +27 -0

index 14139c3..5e4c2f2 100644
@@ -64,6 +64,47 @@ unlock:
   }
   static DEVICE_ATTR_RW(brightness);
 
+static ssize_t rgb_show(struct device *dev, struct device_attribute *attr,
+            char *buf)
+{
+    struct led_classdev *led_cdev = dev_get_drvdata(dev);
+    u32 rgb_val;
+
+    mutex_lock(&led_cdev->led_access);
+    led_update_rgb_val(led_cdev);
+    rgb_val = led_get_rgb_val(led_cdev);
+    mutex_unlock(&led_cdev->led_access);
+
+    return sprintf(buf, "0x%06x\n", rgb_val);
+}
+
+static ssize_t rgb_store(struct device *dev, struct device_attribute *attr,
+             const char *buf, size_t size)
+{
+    struct led_classdev *led_cdev = dev_get_drvdata(dev);
+    unsigned long state;
+    ssize_t ret;
+
+    mutex_lock(&led_cdev->led_access);
+
+    if (led_sysfs_is_disabled(led_cdev)) {
+        ret = -EBUSY;
+        goto unlock;
+    }

Quoted 9 lines

drivers/leds/led-class.c unlock: · line 99

Quoted 6 lines

drivers/leds/led-class.c +6 -0

+
+    ret = kstrtoul(buf, 0, &state);
+    if (ret)
+        goto unlock;
+
+    led_set_rgb_val(led_cdev, state);

Please adhere to the current LED API naming style: led_rgb_set.

Seems like currently two naming styles are use in the LED core. We have e.g. led_blink_set and led_set_brightness. However I'm fine with the suggested led_rgb_set.

Quoted 15 lines

Quoted 15 lines

drivers/leds/led-class.c +8 -0

+
+    ret = size;
+unlock:
+    mutex_unlock(&led_cdev->led_access);
+    return ret;
+}
+static DEVICE_ATTR_RW(rgb);
+
   static ssize_t max_brightness_show(struct device *dev,
           struct device_attribute *attr, char *buf)
   {

@@ -190,6 +231,16 @@ int led_classdev_register(struct device *parent, struct led_classdev *led_cdev)
       char name[64];
       int ret;
 

Quoted 9 lines

drivers/leds/led-class.c int led_classdev_register(struct device *parent, struct led_classdev *led_cdev) · line 238

Quoted 5 lines

drivers/leds/led-class.c +5 -0

+    /* FLASH is not supported for RGB LEDs so far
+     * and RGB enforces max_brightness = LED_FULL.
+     * Initialize the color as white.
+     */
+    if (led_isRGB(led_cdev)) {

Don't use camel case. Change it to led_is_rgb or check the flag directly.

OK, will change it to led_is_rgb.

Quoted 5 lines

drivers/leds/led-class.c int led_classdev_register(struct device *parent, struct led_classdev *led_cdev) · line 239

Quoted 1 line

drivers/leds/led-class.c +1 -0

+        led_cdev->flags &= ~LED_DEV_CAP_FLASH;

I'd rather check whether both FLASH and RGB flag are set and return error in this case.

OK

Quoted 11 lines

Quoted 11 lines

drivers/leds/led-class.c +4 -0

+        led_cdev->max_brightness = LED_FULL;
+        led_cdev->rgb_val = 0xffffff;
+    }
+
       ret = led_classdev_next_name(led_cdev->name, name, sizeof(name));
       if (ret < 0)
           return ret;

@@ -203,6 +254,14 @@ int led_classdev_register(struct device *parent, struct led_classdev *led_cdev)
           dev_warn(parent, "Led %s renamed to %s due to name collision",
                   led_cdev->name, dev_name(led_cdev->dev));
 

Quoted 5 lines

drivers/leds/led-class.c int led_classdev_register(struct device *parent, struct led_classdev *led_cdev) · line 258

Quoted 2 lines

drivers/leds/led-class.c +2 -0

+    if (led_isRGB(led_cdev)) {
+        ret = device_create_file(led_cdev->dev, &dev_attr_rgb);

Please use led_cdev->groups for this. You can refer to drivers/leds/led-class-flash.c.

OK

Quoted 47 lines

Quoted 47 lines

drivers/leds/led-class.c +9 -0

+        if (ret) {
+            device_unregister(led_cdev->dev);
+            return ret;
+        }
+    }
+
   #ifdef CONFIG_LEDS_TRIGGERS
       init_rwsem(&led_cdev->trigger_lock);
   #endif

@@ -252,6 +311,9 @@ void led_classdev_unregister(struct led_classdev *led_cdev)
 
       flush_work(&led_cdev->set_brightness_work);
 
+    if (led_isRGB(led_cdev))
+        device_remove_file(led_cdev->dev, &dev_attr_rgb);
+
       device_unregister(led_cdev->dev);
 
       down_write(&leds_list_lock);

drivers/leds/led-core.c +20 -0

index 19e1e60..6563bd3 100644
@@ -25,6 +25,48 @@ EXPORT_SYMBOL_GPL(leds_list_lock);
   LIST_HEAD(leds_list);
   EXPORT_SYMBOL_GPL(leds_list);
 
+static void led_set_rgb_raw(struct led_classdev *cdev, u32 rgb)
+{
+    u32 red_raw = (rgb >> 16) & 0xff;
+    u32 green_raw = (rgb >> 8) & 0xff;
+    u32 blue_raw = rgb & 0xff;
+    u32 max_raw, red, green, blue;
+
+    max_raw = max(red_raw, green_raw);
+    if (blue_raw > max_raw)
+        max_raw = blue_raw;
+
+    if (!max_raw) {
+        cdev->brightness = 0;
+        cdev->rgb_val = 0;
+        return;
+    }
+
+    red = DIV_ROUND_CLOSEST(red_raw * LED_FULL, max_raw);
+    green = DIV_ROUND_CLOSEST(green_raw * LED_FULL, max_raw);
+    blue = DIV_ROUND_CLOSEST(blue_raw * LED_FULL, max_raw);

Quoted 9 lines

drivers/leds/led-core.c EXPORT_SYMBOL_GPL(leds_list_lock); · line 51

Quoted 4 lines

drivers/leds/led-core.c +4 -0

+
+    cdev->brightness = max_raw;
+    cdev->rgb_val = (red << 16) + (green << 8) + blue;
+}

I think that we shouldn't impose specific way of calculating brightness depending on the rgb value set. We should just pass value from userspace to the driver.

Right, setting brightness from userspace is no problem.
But how about led_update_brightness? It's supposed to update the brightness value in led_cdev with the actual value. And for a RGB LED we have only the RGB values, so we need to calculate the brightness based on RGB values. Or do you see a better way?

This leads me to another question: What do we need led_update_brightness for in general? The cached and the actual value should always be in sync, except the actual brightness can be changed from outside the driver.
Do we have such cases?

Last but not least I saw that led_update_brightness is called in led_classdev_register.
Instead of updating our cached value with the actual value shouldn't we initialize cached and actual value (most likely to 0)?
The actual value theoretically could be uninitialized.
Seems like we rely on BIOS / boot loader to initialize LEDs.

Quoted 33 lines

Quoted 33 lines

drivers/leds/led-core.c +26 -0

+static u32 led_get_rgb_raw(struct led_classdev *cdev, bool delayed)
+{
+    u32 rgb = delayed ? cdev->delayed_rgb_value : cdev->rgb_val;
+    u32 brightness = delayed ? cdev->delayed_set_value :
+             cdev->brightness;
+    u32 red = (rgb >> 16) & 0xff;
+    u32 green = (rgb >> 8) & 0xff;
+    u32 blue = rgb & 0xff;
+    u32 red_raw, green_raw, blue_raw;
+
+    red_raw = DIV_ROUND_CLOSEST(red * brightness, LED_FULL);
+    green_raw = DIV_ROUND_CLOSEST(green * brightness, LED_FULL);
+    blue_raw = DIV_ROUND_CLOSEST(blue * brightness, LED_FULL);
+
+    return (red_raw << 16) + (green_raw << 8) + blue_raw;
+}
+
   static void led_timer_function(unsigned long data)
   {
       struct led_classdev *led_cdev = (void *)data;

@@ -91,6 +133,21 @@ static void set_brightness_delayed(struct work_struct *ws)
           led_cdev->flags &= ~LED_BLINK_DISABLE;
       }
 
+    if (led_isRGB(led_cdev)) {
+        u32 rgb = led_get_rgb_raw(led_cdev, true);
+
+        if (led_cdev->rgb_set)
+            led_cdev->rgb_set(led_cdev, rgb);
+        else if (led_cdev->rgb_set_blocking)
+            ret = led_cdev->rgb_set_blocking(led_cdev, rgb);
+        else
+            ret = -EOPNOTSUPP;

Quoted 9 lines

drivers/leds/led-core.c static void set_brightness_delayed(struct work_struct *ws) · line 150

Quoted 6 lines

drivers/leds/led-core.c +6 -0

+        if (ret < 0)
+            dev_err(led_cdev->dev,
+                "Setting LED RGB value failed (%d)\n", ret);
+        return;
+    }
+

Why this is needed? Could you share a use case?

If we re-use the existing brightness ops as suggested by you then most of this code isn't needed.

Quoted 132 lines

Quoted 13 lines

drivers/leds/led-core.c +6 -0

       if (led_cdev->brightness_set)
           led_cdev->brightness_set(led_cdev, led_cdev->delayed_set_value);
       else if (led_cdev->brightness_set_blocking)

@@ -229,9 +286,35 @@ void led_set_brightness(struct led_classdev *led_cdev,
   }
   EXPORT_SYMBOL_GPL(led_set_brightness);
 
+void led_set_rgb_val(struct led_classdev *led_cdev, u32 rgb_val)
+{
+    if (!led_isRGB(led_cdev) || (led_cdev->flags & LED_SUSPENDED))
+        return;
+
+    led_set_rgb_raw(led_cdev, rgb_val);

drivers/leds/led-core.c void led_set_brightness(struct led_classdev *led_cdev, · line 304

Quoted 10 lines

drivers/leds/led-core.c +10 -0

+
+    if (led_cdev->rgb_set) {
+        led_cdev->rgb_set(led_cdev, rgb_val);
+        return;
+    }
+
+    /* If RGB setting can sleep, delegate it to a work queue task */
+    led_cdev->delayed_set_value = led_cdev->brightness;
+    led_cdev->delayed_rgb_value = led_cdev->rgb_val;
+    schedule_work(&led_cdev->set_brightness_work);

I think that settings should be written to the device only in the brightness_set op. We wouldn't need additional rgb op then.

Quoted 105 lines

drivers/leds/led-core.c +33 -0

+}
+EXPORT_SYMBOL_GPL(led_set_rgb_val);
+
   void led_set_brightness_nopm(struct led_classdev *led_cdev,
                     enum led_brightness value)
   {
+    if (led_isRGB(led_cdev) && led_cdev->rgb_set) {
+        u32 rgb = led_get_rgb_raw(led_cdev, false);
+
+        led_cdev->rgb_set(led_cdev, rgb);
+        return;
+    }
+
       /* Use brightness_set op if available, it is guaranteed not to sleep */
       if (led_cdev->brightness_set) {
           led_cdev->brightness_set(led_cdev, value);

@@ -240,6 +323,7 @@ void led_set_brightness_nopm(struct led_classdev *led_cdev,
 
       /* If brightness setting can sleep, delegate it to a work queue task */
       led_cdev->delayed_set_value = value;
+    led_cdev->delayed_rgb_value = led_cdev->rgb_val;
       schedule_work(&led_cdev->set_brightness_work);
   }
   EXPORT_SYMBOL_GPL(led_set_brightness_nopm);

@@ -267,6 +351,12 @@ int led_set_brightness_sync(struct led_classdev *led_cdev,
       if (led_cdev->flags & LED_SUSPENDED)
           return 0;
 
+    if (led_isRGB(led_cdev) && led_cdev->rgb_set_blocking) {
+        u32 rgb = led_get_rgb_raw(led_cdev, false);
+
+        return led_cdev->rgb_set_blocking(led_cdev, rgb);
+    }
+
       if (led_cdev->brightness_set_blocking)
           return led_cdev->brightness_set_blocking(led_cdev,
                                led_cdev->brightness);

@@ -290,6 +380,22 @@ int led_update_brightness(struct led_classdev *led_cdev)
   }
   EXPORT_SYMBOL_GPL(led_update_brightness);
 
+int led_update_rgb_val(struct led_classdev *led_cdev)
+{
+    s32 ret = 0;
+
+    if (led_isRGB(led_cdev) && led_cdev->rgb_get) {
+        ret = led_cdev->rgb_get(led_cdev);
+        if (ret >= 0) {
+            led_set_rgb_raw(led_cdev, ret);
+            return 0;
+        }
+    }
+
+    return ret;
+}
+EXPORT_SYMBOL_GPL(led_update_rgb_val);
+
   /* Caller must ensure led_cdev->led_access held */
   void led_sysfs_disable(struct led_classdev *led_cdev)
   {

drivers/leds/leds.h +12 -0

index db3f20d..c733777 100644
@@ -16,17 +16,29 @@
   #include <linux/rwsem.h>
   #include <linux/leds.h>
 
+static inline bool led_isRGB(struct led_classdev *led_cdev)
+{
+    return (led_cdev->flags & LED_RGB) != 0;
+}
+
   static inline int led_get_brightness(struct led_classdev *led_cdev)
   {
       return led_cdev->brightness;
   }
 
+static inline u32 led_get_rgb_val(struct led_classdev *led_cdev)
+{
+    return led_cdev->rgb_val;
+}
+
   void led_init_core(struct led_classdev *led_cdev);
   void led_stop_software_blink(struct led_classdev *led_cdev);
   void led_set_brightness_nopm(struct led_classdev *led_cdev,
                   enum led_brightness value);
   void led_set_brightness_nosleep(struct led_classdev *led_cdev,
                   enum led_brightness value);
+void led_set_rgb_val(struct led_classdev *led_cdev, u32 rgb_val);
+int led_update_rgb_val(struct led_classdev *led_cdev);
 
   extern struct rw_semaphore leds_list_lock;
   extern struct list_head leds_list;

include/linux/leds.h +1 -0

index 4429887..83d2912 100644
@@ -35,6 +35,7 @@ struct led_classdev {
       const char        *name;
       enum led_brightness     brightness;
       enum led_brightness     max_brightness;
+    u32             rgb_val;
       int             flags;
 

Quoted 9 lines

include/linux/leds.h struct led_classdev { · line 52

Quoted 6 lines

include/linux/leds.h +1 -0

       /* Lower 16 bits reflect status */

@@ -48,6 +49,7 @@ struct led_classdev {
   #define LED_BLINK_DISABLE    (1 << 21)
   #define LED_SYSFS_DISABLE    (1 << 22)
   #define LED_DEV_CAP_FLASH    (1 << 23)
+#define LED_RGB            (1 << 24)

Please rename it to LED_DEV_CAP_RGB.

OK

Quoted 13 lines

include/linux/leds.h struct led_classdev { · line 63

Quoted 10 lines

include/linux/leds.h +3 -0

       /* Set LED brightness level */
       /* Must not sleep. If no non-blocking version can be provided

@@ -56,15 +58,25 @@ struct led_classdev {
        */
       void        (*brightness_set)(struct led_classdev *led_cdev,
                         enum led_brightness brightness);
+
+    void        (*rgb_set)(struct led_classdev *led_cdev,
+                   u32 rgb_val);

Without this everything would be much simpler.

Sure, we can re-use the existing ops. I just thought it would be a little ugly to misuse enum led_brightness for RGB values. But as it internally is an int we can also do it this way.

Quoted 35 lines

If we wanted to change the color and intensity we would need to:
1. Set rgb (value only cached in the LED core)
2. Set brightness (write both brightness and rgb settings to the hw)

Current brightness can always be obtained with led_get_brightness in case only rgb is to be set.

include/linux/leds.h struct led_classdev { · line 73

Quoted 10 lines

include/linux/leds.h +4 -0

       /*
        * Set LED brightness level immediately - it can block the caller for
        * the time required for accessing a LED device register.
        */
       int (*brightness_set_blocking)(struct led_classdev *led_cdev,
                          enum led_brightness brightness);
+
+    int (*rgb_set_blocking)(struct led_classdev *led_cdev,
+                u32 rgb_val);
+

This is also not needed.

include/linux/leds.h struct led_classdev { · line 79

Quoted 6 lines

include/linux/leds.h +3 -0

       /* Get LED brightness level */
       enum led_brightness (*brightness_get)(struct led_classdev *led_cdev);
 
+    /* Get LED RGB val */
+    s32 (*rgb_get)(struct led_classdev *led_cdev);
+

Ditto.

Quoted 5 lines

include/linux/leds.h +0 -0

       /*
        * Activate hardware accelerated blink, delays are in milliseconds
        * and if both are zero then a sensible default should be chosen.

@@ -90,6 +102,7 @@ struct led_classdev {
 

Quoted 6 lines

include/linux/leds.h struct led_classdev { · line 105

Quoted 3 lines

include/linux/leds.h +1 -0

       struct work_struct    set_brightness_work;
       int            delayed_set_value;
+    u32            delayed_rgb_value;

Ditto.

If we have a blocking set callback then we have to set a RGB value from the workqueue. And we need brightness + RGB to calculate the effective RGB values in the worker func. Therefore I think delayed_rgb_val is needed.

Quoted 6 lines

include/linux/leds.h struct led_classdev {

Quoted 4 lines

include/linux/leds.h +0 -0

   #ifdef CONFIG_LEDS_TRIGGERS
       /* Protects the trigger data below */
 

Thanks for the review, Heiner

--
To unsubscribe from this list: send the line "unsubscribe linux-leds" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 89 lines

Am 14.01.2016 um 13:08 schrieb Jacek Anaszewski:

Quoted 79 lines · Heiner Kallweit

Quoted 78 lines

When playing with a ThingM Blink(1) USB RGB LED device I found that there are few drivers for RGB LED's spread across the kernel and each one implements the RGB functionality in its own way.
Some examples:
- drivers/hid/hid-thingm.c
- drivers/usb/misc/usbled.c
- drivers/leds/leds-bd2802.c
- drivers/leds/leds-blinkm.c
...
IMHO it would make sense to add generic RGB functionality to the LED core.

Things I didn't like too much in other driver implementations:
- one led_classdev per color or at least
- one sysfs attribute per color
Colors are not really independent therefore I'd prefer one led_classdev per RGB LED. Also userspace should be able to change the color with one call -> therefore only one sysfs attribute for the RGB values.

Also the current brightness-related functionality should not be effected by the RGB extension.

This patch is intended to demonstrate the idea of the extension. Most likely it's not ready yet for submission.

Main idea is to base the effective RGB value on a combination of brightness and a scaled-to-max RGB value.
The RGB-related callbacks are basically the same as for brightness. RGB functionally can be switched on with a new flag in the led_classdev.flags bitmap.

Experimentally I changed the thingm driver to use this extension and it works quite well. As one result the driver could be very much simplified.

Any feedback is appreciated.

Signed-off-by Heiner Kallweit <[email protected]>

---
   drivers/leds/led-class.c |  62 +++++++++++++++++++++++++++
   drivers/leds/led-core.c  | 106 +++++++++++++++++++++++++++++++++++++++++++++++
   drivers/leds/leds.h      |  12 ++++++
   include/linux/leds.h     |  13 ++++++
   4 files changed, 193 insertions(+)

drivers/leds/led-class.c +27 -0

index 14139c3..5e4c2f2 100644
@@ -64,6 +64,47 @@ unlock:
   }
   static DEVICE_ATTR_RW(brightness);
 
+static ssize_t rgb_show(struct device *dev, struct device_attribute *attr,
+            char *buf)
+{
+    struct led_classdev *led_cdev = dev_get_drvdata(dev);
+    u32 rgb_val;
+
+    mutex_lock(&led_cdev->led_access);
+    led_update_rgb_val(led_cdev);
+    rgb_val = led_get_rgb_val(led_cdev);
+    mutex_unlock(&led_cdev->led_access);
+
+    return sprintf(buf, "0x%06x\n", rgb_val);
+}
+
+static ssize_t rgb_store(struct device *dev, struct device_attribute *attr,
+             const char *buf, size_t size)
+{
+    struct led_classdev *led_cdev = dev_get_drvdata(dev);
+    unsigned long state;
+    ssize_t ret;
+
+    mutex_lock(&led_cdev->led_access);
+
+    if (led_sysfs_is_disabled(led_cdev)) {
+        ret = -EBUSY;
+        goto unlock;
+    }

Quoted 9 lines

drivers/leds/led-class.c unlock: · line 99

Quoted 6 lines

drivers/leds/led-class.c +6 -0

+
+    ret = kstrtoul(buf, 0, &state);
+    if (ret)
+        goto unlock;
+
+    led_set_rgb_val(led_cdev, state);

Please adhere to the current LED API naming style: led_rgb_set.

Quoted 3 lines

Seems like currently two naming styles are use in the LED core. We have e.g. led_blink_set and led_set_brightness. However I'm fine with the suggested led_rgb_set.

Indeed. Your version would be preferable in this case. I wanted this to match led_set_brightness style, but had been too lazy to double check that :-/

Quoted 107 lines

Quoted 15 lines

Quoted 15 lines

drivers/leds/led-class.c +8 -0

+
+    ret = size;
+unlock:
+    mutex_unlock(&led_cdev->led_access);
+    return ret;
+}
+static DEVICE_ATTR_RW(rgb);
+
   static ssize_t max_brightness_show(struct device *dev,
           struct device_attribute *attr, char *buf)
   {

@@ -190,6 +231,16 @@ int led_classdev_register(struct device *parent, struct led_classdev *led_cdev)
       char name[64];
       int ret;
 

Quoted 9 lines

drivers/leds/led-class.c int led_classdev_register(struct device *parent, struct led_classdev *led_cdev) · line 238

Quoted 5 lines

drivers/leds/led-class.c +5 -0

+    /* FLASH is not supported for RGB LEDs so far
+     * and RGB enforces max_brightness = LED_FULL.
+     * Initialize the color as white.
+     */
+    if (led_isRGB(led_cdev)) {

Don't use camel case. Change it to led_is_rgb or check the flag directly.

OK, will change it to led_is_rgb.

Quoted 5 lines

drivers/leds/led-class.c int led_classdev_register(struct device *parent, struct led_classdev *led_cdev) · line 239

Quoted 1 line

drivers/leds/led-class.c +1 -0

+        led_cdev->flags &= ~LED_DEV_CAP_FLASH;

I'd rather check whether both FLASH and RGB flag are set and return error in this case.

OK

Quoted 11 lines

Quoted 11 lines

drivers/leds/led-class.c +4 -0

+        led_cdev->max_brightness = LED_FULL;
+        led_cdev->rgb_val = 0xffffff;
+    }
+
       ret = led_classdev_next_name(led_cdev->name, name, sizeof(name));
       if (ret < 0)
           return ret;

@@ -203,6 +254,14 @@ int led_classdev_register(struct device *parent, struct led_classdev *led_cdev)
           dev_warn(parent, "Led %s renamed to %s due to name collision",
                   led_cdev->name, dev_name(led_cdev->dev));
 

Quoted 5 lines

drivers/leds/led-class.c int led_classdev_register(struct device *parent, struct led_classdev *led_cdev) · line 258

Quoted 2 lines

drivers/leds/led-class.c +2 -0

+    if (led_isRGB(led_cdev)) {
+        ret = device_create_file(led_cdev->dev, &dev_attr_rgb);

Please use led_cdev->groups for this. You can refer to drivers/leds/led-class-flash.c.

OK

Quoted 47 lines

Quoted 47 lines

drivers/leds/led-class.c +9 -0

+        if (ret) {
+            device_unregister(led_cdev->dev);
+            return ret;
+        }
+    }
+
   #ifdef CONFIG_LEDS_TRIGGERS
       init_rwsem(&led_cdev->trigger_lock);
   #endif

@@ -252,6 +311,9 @@ void led_classdev_unregister(struct led_classdev *led_cdev)
 
       flush_work(&led_cdev->set_brightness_work);
 
+    if (led_isRGB(led_cdev))
+        device_remove_file(led_cdev->dev, &dev_attr_rgb);
+
       device_unregister(led_cdev->dev);
 
       down_write(&leds_list_lock);

drivers/leds/led-core.c +20 -0

index 19e1e60..6563bd3 100644
@@ -25,6 +25,48 @@ EXPORT_SYMBOL_GPL(leds_list_lock);
   LIST_HEAD(leds_list);
   EXPORT_SYMBOL_GPL(leds_list);
 
+static void led_set_rgb_raw(struct led_classdev *cdev, u32 rgb)
+{
+    u32 red_raw = (rgb >> 16) & 0xff;
+    u32 green_raw = (rgb >> 8) & 0xff;
+    u32 blue_raw = rgb & 0xff;
+    u32 max_raw, red, green, blue;
+
+    max_raw = max(red_raw, green_raw);
+    if (blue_raw > max_raw)
+        max_raw = blue_raw;
+
+    if (!max_raw) {
+        cdev->brightness = 0;
+        cdev->rgb_val = 0;
+        return;
+    }
+
+    red = DIV_ROUND_CLOSEST(red_raw * LED_FULL, max_raw);
+    green = DIV_ROUND_CLOSEST(green_raw * LED_FULL, max_raw);
+    blue = DIV_ROUND_CLOSEST(blue_raw * LED_FULL, max_raw);

Quoted 9 lines

drivers/leds/led-core.c EXPORT_SYMBOL_GPL(leds_list_lock); · line 51

Quoted 4 lines

drivers/leds/led-core.c +4 -0

+
+    cdev->brightness = max_raw;
+    cdev->rgb_val = (red << 16) + (green << 8) + blue;
+}

I think that we shouldn't impose specific way of calculating brightness depending on the rgb value set. We should just pass value from userspace to the driver.

Quoted 5 lines

Right, setting brightness from userspace is no problem.
But how about led_update_brightness? It's supposed to update the brightness value in led_cdev with the actual value. And for a RGB LED we have only the RGB values, so we need to calculate the brightness based on RGB values. Or do you see a better way?

I thought that you had some device using both brightness and rgb components settings (no idea if this could be sane in any way). If there are only three color components, then why not just redefine brightness to store three components in case of the LEDs with LED_DEV_CAP_RGB flags set?

Quoted 5 lines

This leads me to another question: What do we need led_update_brightness for in general? The cached and the actual value should always be in sync, except the actual brightness can be changed from outside the driver.
Do we have such cases?

It is possible that hardware will limit the brightness, e.g. due to the low battery voltage level.

Quoted 7 lines

Last but not least I saw that led_update_brightness is called in led_classdev_register.
Instead of updating our cached value with the actual value shouldn't we initialize cached and actual value (most likely to 0)?
The actual value theoretically could be uninitialized.
Seems like we rely on BIOS / boot loader to initialize LEDs.

Drivers are generally expected to initialize brightness to what they deem valid initial value. If a driver implements brightness_get op then brightness will be synchronized with device settings upon registering automatically thanks to this call.

Quoted 201 lines

Quoted 33 lines

Quoted 33 lines

drivers/leds/led-core.c +26 -0

+static u32 led_get_rgb_raw(struct led_classdev *cdev, bool delayed)
+{
+    u32 rgb = delayed ? cdev->delayed_rgb_value : cdev->rgb_val;
+    u32 brightness = delayed ? cdev->delayed_set_value :
+             cdev->brightness;
+    u32 red = (rgb >> 16) & 0xff;
+    u32 green = (rgb >> 8) & 0xff;
+    u32 blue = rgb & 0xff;
+    u32 red_raw, green_raw, blue_raw;
+
+    red_raw = DIV_ROUND_CLOSEST(red * brightness, LED_FULL);
+    green_raw = DIV_ROUND_CLOSEST(green * brightness, LED_FULL);
+    blue_raw = DIV_ROUND_CLOSEST(blue * brightness, LED_FULL);
+
+    return (red_raw << 16) + (green_raw << 8) + blue_raw;
+}
+
   static void led_timer_function(unsigned long data)
   {
       struct led_classdev *led_cdev = (void *)data;

@@ -91,6 +133,21 @@ static void set_brightness_delayed(struct work_struct *ws)
           led_cdev->flags &= ~LED_BLINK_DISABLE;
       }
 
+    if (led_isRGB(led_cdev)) {
+        u32 rgb = led_get_rgb_raw(led_cdev, true);
+
+        if (led_cdev->rgb_set)
+            led_cdev->rgb_set(led_cdev, rgb);
+        else if (led_cdev->rgb_set_blocking)
+            ret = led_cdev->rgb_set_blocking(led_cdev, rgb);
+        else
+            ret = -EOPNOTSUPP;

Quoted 9 lines

drivers/leds/led-core.c static void set_brightness_delayed(struct work_struct *ws) · line 150

Quoted 6 lines

drivers/leds/led-core.c +6 -0

+        if (ret < 0)
+            dev_err(led_cdev->dev,
+                "Setting LED RGB value failed (%d)\n", ret);
+        return;
+    }
+

Why this is needed? Could you share a use case?

If we re-use the existing brightness ops as suggested by you then most of this code isn't needed.

Quoted 132 lines

Quoted 13 lines

drivers/leds/led-core.c +6 -0

       if (led_cdev->brightness_set)
           led_cdev->brightness_set(led_cdev, led_cdev->delayed_set_value);
       else if (led_cdev->brightness_set_blocking)

@@ -229,9 +286,35 @@ void led_set_brightness(struct led_classdev *led_cdev,
   }
   EXPORT_SYMBOL_GPL(led_set_brightness);
 
+void led_set_rgb_val(struct led_classdev *led_cdev, u32 rgb_val)
+{
+    if (!led_isRGB(led_cdev) || (led_cdev->flags & LED_SUSPENDED))
+        return;
+
+    led_set_rgb_raw(led_cdev, rgb_val);

drivers/leds/led-core.c void led_set_brightness(struct led_classdev *led_cdev, · line 304

Quoted 10 lines

drivers/leds/led-core.c +10 -0

+
+    if (led_cdev->rgb_set) {
+        led_cdev->rgb_set(led_cdev, rgb_val);
+        return;
+    }
+
+    /* If RGB setting can sleep, delegate it to a work queue task */
+    led_cdev->delayed_set_value = led_cdev->brightness;
+    led_cdev->delayed_rgb_value = led_cdev->rgb_val;
+    schedule_work(&led_cdev->set_brightness_work);

I think that settings should be written to the device only in the brightness_set op. We wouldn't need additional rgb op then.

Quoted 105 lines

drivers/leds/led-core.c +33 -0

+}
+EXPORT_SYMBOL_GPL(led_set_rgb_val);
+
   void led_set_brightness_nopm(struct led_classdev *led_cdev,
                     enum led_brightness value)
   {
+    if (led_isRGB(led_cdev) && led_cdev->rgb_set) {
+        u32 rgb = led_get_rgb_raw(led_cdev, false);
+
+        led_cdev->rgb_set(led_cdev, rgb);
+        return;
+    }
+
       /* Use brightness_set op if available, it is guaranteed not to sleep */
       if (led_cdev->brightness_set) {
           led_cdev->brightness_set(led_cdev, value);

@@ -240,6 +323,7 @@ void led_set_brightness_nopm(struct led_classdev *led_cdev,
 
       /* If brightness setting can sleep, delegate it to a work queue task */
       led_cdev->delayed_set_value = value;
+    led_cdev->delayed_rgb_value = led_cdev->rgb_val;
       schedule_work(&led_cdev->set_brightness_work);
   }
   EXPORT_SYMBOL_GPL(led_set_brightness_nopm);

@@ -267,6 +351,12 @@ int led_set_brightness_sync(struct led_classdev *led_cdev,
       if (led_cdev->flags & LED_SUSPENDED)
           return 0;
 
+    if (led_isRGB(led_cdev) && led_cdev->rgb_set_blocking) {
+        u32 rgb = led_get_rgb_raw(led_cdev, false);
+
+        return led_cdev->rgb_set_blocking(led_cdev, rgb);
+    }
+
       if (led_cdev->brightness_set_blocking)
           return led_cdev->brightness_set_blocking(led_cdev,
                                led_cdev->brightness);

@@ -290,6 +380,22 @@ int led_update_brightness(struct led_classdev *led_cdev)
   }
   EXPORT_SYMBOL_GPL(led_update_brightness);
 
+int led_update_rgb_val(struct led_classdev *led_cdev)
+{
+    s32 ret = 0;
+
+    if (led_isRGB(led_cdev) && led_cdev->rgb_get) {
+        ret = led_cdev->rgb_get(led_cdev);
+        if (ret >= 0) {
+            led_set_rgb_raw(led_cdev, ret);
+            return 0;
+        }
+    }
+
+    return ret;
+}
+EXPORT_SYMBOL_GPL(led_update_rgb_val);
+
   /* Caller must ensure led_cdev->led_access held */
   void led_sysfs_disable(struct led_classdev *led_cdev)
   {

drivers/leds/leds.h +12 -0

index db3f20d..c733777 100644
@@ -16,17 +16,29 @@
   #include <linux/rwsem.h>
   #include <linux/leds.h>
 
+static inline bool led_isRGB(struct led_classdev *led_cdev)
+{
+    return (led_cdev->flags & LED_RGB) != 0;
+}
+
   static inline int led_get_brightness(struct led_classdev *led_cdev)
   {
       return led_cdev->brightness;
   }
 
+static inline u32 led_get_rgb_val(struct led_classdev *led_cdev)
+{
+    return led_cdev->rgb_val;
+}
+
   void led_init_core(struct led_classdev *led_cdev);
   void led_stop_software_blink(struct led_classdev *led_cdev);
   void led_set_brightness_nopm(struct led_classdev *led_cdev,
                   enum led_brightness value);
   void led_set_brightness_nosleep(struct led_classdev *led_cdev,
                   enum led_brightness value);
+void led_set_rgb_val(struct led_classdev *led_cdev, u32 rgb_val);
+int led_update_rgb_val(struct led_classdev *led_cdev);
 
   extern struct rw_semaphore leds_list_lock;
   extern struct list_head leds_list;

include/linux/leds.h +1 -0

index 4429887..83d2912 100644
@@ -35,6 +35,7 @@ struct led_classdev {
       const char        *name;
       enum led_brightness     brightness;
       enum led_brightness     max_brightness;
+    u32             rgb_val;
       int             flags;
 

Quoted 9 lines

include/linux/leds.h struct led_classdev { · line 52

Quoted 6 lines

include/linux/leds.h +1 -0

       /* Lower 16 bits reflect status */

@@ -48,6 +49,7 @@ struct led_classdev {
   #define LED_BLINK_DISABLE    (1 << 21)
   #define LED_SYSFS_DISABLE    (1 << 22)
   #define LED_DEV_CAP_FLASH    (1 << 23)
+#define LED_RGB            (1 << 24)

Please rename it to LED_DEV_CAP_RGB.

OK

Quoted 13 lines

include/linux/leds.h struct led_classdev { · line 63

Quoted 10 lines

include/linux/leds.h +3 -0

       /* Set LED brightness level */
       /* Must not sleep. If no non-blocking version can be provided

@@ -56,15 +58,25 @@ struct led_classdev {
        */
       void        (*brightness_set)(struct led_classdev *led_cdev,
                         enum led_brightness brightness);
+
+    void        (*rgb_set)(struct led_classdev *led_cdev,
+                   u32 rgb_val);

Without this everything would be much simpler.

Quoted 3 lines

Sure, we can re-use the existing ops. I just thought it would be a little ugly to misuse enum led_brightness for RGB values. But as it internally is an int we can also do it this way.

I think that this is a good idea. It would require only addition of one flag and maybe we could think of some new sysfs attribute which would allow to detect if a LED is of RGB type.

Quoted 58 lines

Quoted 35 lines

If we wanted to change the color and intensity we would need to:
1. Set rgb (value only cached in the LED core)
2. Set brightness (write both brightness and rgb settings to the hw)

Current brightness can always be obtained with led_get_brightness in case only rgb is to be set.

include/linux/leds.h struct led_classdev { · line 73

Quoted 10 lines

include/linux/leds.h +4 -0

       /*
        * Set LED brightness level immediately - it can block the caller for
        * the time required for accessing a LED device register.
        */
       int (*brightness_set_blocking)(struct led_classdev *led_cdev,
                          enum led_brightness brightness);
+
+    int (*rgb_set_blocking)(struct led_classdev *led_cdev,
+                u32 rgb_val);
+

This is also not needed.

include/linux/leds.h struct led_classdev { · line 79

Quoted 6 lines

include/linux/leds.h +3 -0

       /* Get LED brightness level */
       enum led_brightness (*brightness_get)(struct led_classdev *led_cdev);
 
+    /* Get LED RGB val */
+    s32 (*rgb_get)(struct led_classdev *led_cdev);
+

Ditto.

Quoted 5 lines

include/linux/leds.h +0 -0

       /*
        * Activate hardware accelerated blink, delays are in milliseconds
        * and if both are zero then a sensible default should be chosen.

@@ -90,6 +102,7 @@ struct led_classdev {
 

Quoted 6 lines

include/linux/leds.h struct led_classdev { · line 105

Quoted 3 lines

include/linux/leds.h +1 -0

       struct work_struct    set_brightness_work;
       int            delayed_set_value;
+    u32            delayed_rgb_value;

Ditto.

If we have a blocking set callback then we have to set a RGB value from the workqueue. And we need brightness + RGB to calculate the effective RGB values in the worker func. Therefore I think delayed_rgb_val is needed.

Quoted 6 lines

include/linux/leds.h struct led_classdev {

Quoted 4 lines

include/linux/leds.h +0 -0

   #ifdef CONFIG_LEDS_TRIGGERS
       /* Protects the trigger data below */
 

Thanks for the review, Heiner

Signature · 3 lines

To unsubscribe from this list: send the line "unsubscribe linux-leds" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Am 17.01.2016 um 23:31 schrieb Jacek Anaszewski:

Quoted 46 lines · Heiner Kallweit

Quoted 45 lines

Am 14.01.2016 um 13:08 schrieb Jacek Anaszewski:

Quoted 44 lines · Heiner Kallweit

Quoted 43 lines

When playing with a ThingM Blink(1) USB RGB LED device I found that there are few drivers for RGB LED's spread across the kernel and each one implements the RGB functionality in its own way.
Some examples:
- drivers/hid/hid-thingm.c
- drivers/usb/misc/usbled.c
- drivers/leds/leds-bd2802.c
- drivers/leds/leds-blinkm.c
...
IMHO it would make sense to add generic RGB functionality to the LED core.

Things I didn't like too much in other driver implementations:
- one led_classdev per color or at least
- one sysfs attribute per color
Colors are not really independent therefore I'd prefer one led_classdev per RGB LED. Also userspace should be able to change the color with one call -> therefore only one sysfs attribute for the RGB values.

Also the current brightness-related functionality should not be effected by the RGB extension.

This patch is intended to demonstrate the idea of the extension. Most likely it's not ready yet for submission.

Main idea is to base the effective RGB value on a combination of brightness and a scaled-to-max RGB value.
The RGB-related callbacks are basically the same as for brightness. RGB functionally can be switched on with a new flag in the led_classdev.flags bitmap.

Experimentally I changed the thingm driver to use this extension and it works quite well. As one result the driver could be very much simplified.

Any feedback is appreciated.

Signed-off-by Heiner Kallweit <[email protected]>

---
    drivers/leds/led-class.c |  62 +++++++++++++++++++++++++++
    drivers/leds/led-core.c  | 106 +++++++++++++++++++++++++++++++++++++++++++++++
    drivers/leds/leds.h      |  12 ++++++
    include/linux/leds.h     |  13 ++++++
    4 files changed, 193 insertions(+)

Quoted 35 lines

Quoted 29 lines

Quoted 20 lines

Quoted 20 lines

drivers/leds/led-core.c +20 -0

+static void led_set_rgb_raw(struct led_classdev *cdev, u32 rgb)
+{
+    u32 red_raw = (rgb >> 16) & 0xff;
+    u32 green_raw = (rgb >> 8) & 0xff;
+    u32 blue_raw = rgb & 0xff;
+    u32 max_raw, red, green, blue;
+
+    max_raw = max(red_raw, green_raw);
+    if (blue_raw > max_raw)
+        max_raw = blue_raw;
+
+    if (!max_raw) {
+        cdev->brightness = 0;
+        cdev->rgb_val = 0;
+        return;
+    }
+
+    red = DIV_ROUND_CLOSEST(red_raw * LED_FULL, max_raw);
+    green = DIV_ROUND_CLOSEST(green_raw * LED_FULL, max_raw);
+    blue = DIV_ROUND_CLOSEST(blue_raw * LED_FULL, max_raw);

Quoted 9 lines

drivers/leds/led-core.c EXPORT_SYMBOL_GPL(leds_list_lock); · line 51

Quoted 4 lines

drivers/leds/led-core.c +4 -0

+
+    cdev->brightness = max_raw;
+    cdev->rgb_val = (red << 16) + (green << 8) + blue;
+}

I think that we shouldn't impose specific way of calculating brightness depending on the rgb value set. We should just pass value from userspace to the driver.

Quoted 5 lines

Right, setting brightness from userspace is no problem.
But how about led_update_brightness? It's supposed to update the brightness value in led_cdev with the actual value. And for a RGB LED we have only the RGB values, so we need to calculate the brightness based on RGB values. Or do you see a better way?

Quoted 6 lines

I thought that you had some device using both brightness and rgb components settings (no idea if this could be sane in any way). If there are only three color components, then why not just redefine brightness to store three components in case of the LEDs with LED_DEV_CAP_RGB flags set?

My devices just have the three color components (normal 8 bits per color). Even in the new RGB mode I don't want to break the current brightness-based API but extend it with RGB functionality. Therefore I think it's best to store brightness and color separately.
Just one argument: If we store the color components only then we'll lose the color information if the brightness is set to 0.
And I would like to support e.g. software blink in whatever color.

Hi Heiner,

Quoted 62 lines · Heiner Kallweit

Am 17.01.2016 um 23:31 schrieb Jacek Anaszewski:

Quoted 60 lines · Heiner Kallweit

Quoted 58 lines

Am 14.01.2016 um 13:08 schrieb Jacek Anaszewski:

Quoted 56 lines · Heiner Kallweit

Quoted 54 lines

When playing with a ThingM Blink(1) USB RGB LED device I found that there
are few drivers for RGB LED's spread across the kernel and each one implements the RGB functionality in its own way.
Some examples:
- drivers/hid/hid-thingm.c
- drivers/usb/misc/usbled.c
- drivers/leds/leds-bd2802.c
- drivers/leds/leds-blinkm.c
...
IMHO it would make sense to add generic RGB functionality to the LED core.

Things I didn't like too much in other driver implementations:
- one led_classdev per color or at least
- one sysfs attribute per color
Colors are not really independent therefore I'd prefer one led_classdev
per RGB LED. Also userspace should be able to change the color with one
call -> therefore only one sysfs attribute for the RGB values.

Also the current brightness-related functionality should not be effected
by the RGB extension.

This patch is intended to demonstrate the idea of the extension. Most likely
it's not ready yet for submission.

Main idea is to base the effective RGB value on a combination of brightness
and a scaled-to-max RGB value.
The RGB-related callbacks are basically the same as for brightness. RGB functionally can be switched on with a new flag in the led_classdev.flags
bitmap.

Experimentally I changed the thingm driver to use this extension and it works
quite well. As one result the driver could be very much simplified.

Any feedback is appreciated.

Signed-off-by Heiner Kallweit <[email protected]>

---
    drivers/leds/led-class.c |  62 +++++++++++++++++++++++++++
    drivers/leds/led-core.c  | 106
+++++++++++++++++++++++++++++++++++++++++++++++
    drivers/leds/leds.h      |  12 ++++++
    include/linux/leds.h     |  13 ++++++
    4 files changed, 193 insertions(+)

[...]

Quoted 46 lines

Quoted 40 lines

Quoted 31 lines

Quoted 22 lines

Quoted 22 lines

drivers/leds/led-core.c +22 -0

+static void led_set_rgb_raw(struct led_classdev *cdev, u32 rgb)
+{
+    u32 red_raw = (rgb >> 16) & 0xff;
+    u32 green_raw = (rgb >> 8) & 0xff;
+    u32 blue_raw = rgb & 0xff;
+    u32 max_raw, red, green, blue;
+
+    max_raw = max(red_raw, green_raw);
+    if (blue_raw > max_raw)
+        max_raw = blue_raw;
+
+    if (!max_raw) {
+        cdev->brightness = 0;
+        cdev->rgb_val = 0;
+        return;
+    }
+
+    red = DIV_ROUND_CLOSEST(red_raw * LED_FULL, max_raw);
+    green = DIV_ROUND_CLOSEST(green_raw * LED_FULL, max_raw);
+    blue = DIV_ROUND_CLOSEST(blue_raw * LED_FULL, max_raw);
+
+    cdev->brightness = max_raw;

Quoted 9 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+    cdev->rgb_val = (red << 16) + (green << 8) + blue;
+}

I think that we shouldn't impose specific way of calculating brightness depending on the rgb value set. We should just pass value from userspace
to the driver.

Quoted 7 lines

Right, setting brightness from userspace is no problem.
But how about led_update_brightness? It's supposed to update the brightness
value in led_cdev with the actual value. And for a RGB LED we have only the RGB values, so we need to calculate the brightness based on RGB values.
Or do you see a better way?

Quoted 6 lines

I thought that you had some device using both brightness and rgb components settings (no idea if this could be sane in any way). If there are only three color components, then why not just redefine brightness to store three components in case of the LEDs with LED_DEV_CAP_RGB flags set?

Quoted 8 lines

My devices just have the three color components (normal 8 bits per color). Even in the new RGB mode I don't want to break the current brightness-based API
but extend it with RGB functionality. Therefore I think it's best to store brightness and color separately.
Just one argument: If we store the color components only then we'll lose the color information if the brightness is set to 0.
And I would like to support e.g. software blink in whatever color.

Currently blinking works properly and brightness value isn't being lost. We store it in the led_cdev->blink_brightness property. Could you present exact use case you'd like to support and which is not feasible with current LED core design?

--
Best Regards,
Jacek Anaszewski

Quoted 124 lines · Jacek Anaszewski

Hi Heiner,

Quoted 62 lines · Heiner Kallweit

Am 17.01.2016 um 23:31 schrieb Jacek Anaszewski:

Quoted 60 lines · Heiner Kallweit

Quoted 58 lines

Am 14.01.2016 um 13:08 schrieb Jacek Anaszewski:

Quoted 56 lines · Heiner Kallweit

Quoted 54 lines

When playing with a ThingM Blink(1) USB RGB LED device I found that there
are few drivers for RGB LED's spread across the kernel and each one implements the RGB functionality in its own way.
Some examples:
- drivers/hid/hid-thingm.c
- drivers/usb/misc/usbled.c
- drivers/leds/leds-bd2802.c
- drivers/leds/leds-blinkm.c
...
IMHO it would make sense to add generic RGB functionality to the LED core.

Things I didn't like too much in other driver implementations:
- one led_classdev per color or at least
- one sysfs attribute per color
Colors are not really independent therefore I'd prefer one led_classdev
per RGB LED. Also userspace should be able to change the color with one
call -> therefore only one sysfs attribute for the RGB values.

Also the current brightness-related functionality should not be effected
by the RGB extension.

This patch is intended to demonstrate the idea of the extension. Most likely
it's not ready yet for submission.

Main idea is to base the effective RGB value on a combination of brightness
and a scaled-to-max RGB value.
The RGB-related callbacks are basically the same as for brightness. RGB functionally can be switched on with a new flag in the led_classdev.flags
bitmap.

Experimentally I changed the thingm driver to use this extension and it works
quite well. As one result the driver could be very much simplified.

Any feedback is appreciated.

Signed-off-by Heiner Kallweit <[email protected]>

---
     drivers/leds/led-class.c |  62 +++++++++++++++++++++++++++
     drivers/leds/led-core.c  | 106
+++++++++++++++++++++++++++++++++++++++++++++++
     drivers/leds/leds.h      |  12 ++++++
     include/linux/leds.h     |  13 ++++++
     4 files changed, 193 insertions(+)

[...]

Quoted 46 lines

Quoted 40 lines

Quoted 31 lines

Quoted 22 lines

Quoted 22 lines

(quoted diff) +22 -0

+static void led_set_rgb_raw(struct led_classdev *cdev, u32 rgb)
+{
+    u32 red_raw = (rgb >> 16) & 0xff;
+    u32 green_raw = (rgb >> 8) & 0xff;
+    u32 blue_raw = rgb & 0xff;
+    u32 max_raw, red, green, blue;
+
+    max_raw = max(red_raw, green_raw);
+    if (blue_raw > max_raw)
+        max_raw = blue_raw;
+
+    if (!max_raw) {
+        cdev->brightness = 0;
+        cdev->rgb_val = 0;
+        return;
+    }
+
+    red = DIV_ROUND_CLOSEST(red_raw * LED_FULL, max_raw);
+    green = DIV_ROUND_CLOSEST(green_raw * LED_FULL, max_raw);
+    blue = DIV_ROUND_CLOSEST(blue_raw * LED_FULL, max_raw);
+
+    cdev->brightness = max_raw;

Quoted 9 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+    cdev->rgb_val = (red << 16) + (green << 8) + blue;
+}

I think that we shouldn't impose specific way of calculating brightness depending on the rgb value set. We should just pass value from userspace
to the driver.

Quoted 7 lines

Right, setting brightness from userspace is no problem.
But how about led_update_brightness? It's supposed to update the brightness
value in led_cdev with the actual value. And for a RGB LED we have only the RGB values, so we need to calculate the brightness based on RGB values.
Or do you see a better way?

Quoted 6 lines

I thought that you had some device using both brightness and rgb components settings (no idea if this could be sane in any way). If there are only three color components, then why not just redefine brightness to store three components in case of the LEDs with LED_DEV_CAP_RGB flags set?

Quoted 8 lines

My devices just have the three color components (normal 8 bits per color). Even in the new RGB mode I don't want to break the current brightness-based API
but extend it with RGB functionality. Therefore I think it's best to store brightness and color separately.
Just one argument: If we store the color components only then we'll lose the color information if the brightness is set to 0.
And I would like to support e.g. software blink in whatever color.

Quoted 5 lines

Currently blinking works properly and brightness value isn't being lost. We store it in the led_cdev->blink_brightness property. Could you present exact use case you'd like to support and which is not feasible with current LED core design?

I didn't have a closer look on how soft blinking works and you're right, this was a bad example.
It's not about that something is wrong with the current LED core design but that IMHO storing just the raw RGB values wouldn't be a good idea and we should store brightness and color separately.

So let's take another example:
I set a color via sysfs and a trigger is controling the LED. Once the LED brightness is set to LED_OFF by the trigger and e.g. a sysfs read triggers an update the stored RGB value is 0,0,0.
If the LED is switched on again, then which color to choose?

That's why I'm saying we should store color and brightness separately to avoid losing the color information information even if the brightness is set to zero.

Regards, Heiner

Quoted 137 lines

Quoted 124 lines · Jacek Anaszewski

Hi Heiner,

Quoted 62 lines · Heiner Kallweit

Am 17.01.2016 um 23:31 schrieb Jacek Anaszewski:

Quoted 60 lines · Heiner Kallweit

Quoted 58 lines

Am 14.01.2016 um 13:08 schrieb Jacek Anaszewski:

Quoted 56 lines · Heiner Kallweit

Quoted 54 lines

When playing with a ThingM Blink(1) USB RGB LED device I found that there
are few drivers for RGB LED's spread across the kernel and each one implements the RGB functionality in its own way.
Some examples:
- drivers/hid/hid-thingm.c
- drivers/usb/misc/usbled.c
- drivers/leds/leds-bd2802.c
- drivers/leds/leds-blinkm.c
...
IMHO it would make sense to add generic RGB functionality to the LED core.

Things I didn't like too much in other driver implementations:
- one led_classdev per color or at least
- one sysfs attribute per color
Colors are not really independent therefore I'd prefer one led_classdev
per RGB LED. Also userspace should be able to change the color with one
call -> therefore only one sysfs attribute for the RGB values.

Also the current brightness-related functionality should not be effected
by the RGB extension.

This patch is intended to demonstrate the idea of the extension. Most likely
it's not ready yet for submission.

Main idea is to base the effective RGB value on a combination of brightness
and a scaled-to-max RGB value.
The RGB-related callbacks are basically the same as for brightness. RGB functionally can be switched on with a new flag in the led_classdev.flags
bitmap.

Experimentally I changed the thingm driver to use this extension and it works
quite well. As one result the driver could be very much simplified.

Any feedback is appreciated.

Signed-off-by Heiner Kallweit <[email protected]>

---
     drivers/leds/led-class.c |  62 +++++++++++++++++++++++++++
     drivers/leds/led-core.c  | 106
+++++++++++++++++++++++++++++++++++++++++++++++
     drivers/leds/leds.h      |  12 ++++++
     include/linux/leds.h     |  13 ++++++
     4 files changed, 193 insertions(+)

[...]

Quoted 46 lines

Quoted 40 lines

Quoted 31 lines

Quoted 22 lines

Quoted 22 lines

(quoted diff) +22 -0

+static void led_set_rgb_raw(struct led_classdev *cdev, u32 rgb)
+{
+    u32 red_raw = (rgb >> 16) & 0xff;
+    u32 green_raw = (rgb >> 8) & 0xff;
+    u32 blue_raw = rgb & 0xff;
+    u32 max_raw, red, green, blue;
+
+    max_raw = max(red_raw, green_raw);
+    if (blue_raw > max_raw)
+        max_raw = blue_raw;
+
+    if (!max_raw) {
+        cdev->brightness = 0;
+        cdev->rgb_val = 0;
+        return;
+    }
+
+    red = DIV_ROUND_CLOSEST(red_raw * LED_FULL, max_raw);
+    green = DIV_ROUND_CLOSEST(green_raw * LED_FULL, max_raw);
+    blue = DIV_ROUND_CLOSEST(blue_raw * LED_FULL, max_raw);
+
+    cdev->brightness = max_raw;

Quoted 9 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+    cdev->rgb_val = (red << 16) + (green << 8) + blue;
+}

I think that we shouldn't impose specific way of calculating brightness depending on the rgb value set. We should just pass value from userspace
to the driver.

Quoted 7 lines

Right, setting brightness from userspace is no problem.
But how about led_update_brightness? It's supposed to update the brightness
value in led_cdev with the actual value. And for a RGB LED we have only the RGB values, so we need to calculate the brightness based on RGB values.
Or do you see a better way?

Quoted 6 lines

I thought that you had some device using both brightness and rgb components settings (no idea if this could be sane in any way). If there are only three color components, then why not just redefine brightness to store three components in case of the LEDs with LED_DEV_CAP_RGB flags set?

Quoted 8 lines

My devices just have the three color components (normal 8 bits per color). Even in the new RGB mode I don't want to break the current brightness-based API
but extend it with RGB functionality. Therefore I think it's best to store brightness and color separately.
Just one argument: If we store the color components only then we'll lose the color information if the brightness is set to 0.
And I would like to support e.g. software blink in whatever color.

Quoted 5 lines

Currently blinking works properly and brightness value isn't being lost. We store it in the led_cdev->blink_brightness property. Could you present exact use case you'd like to support and which is not feasible with current LED core design?

I didn't have a closer look on how soft blinking works and you're right, this was a bad example.
It's not about that something is wrong with the current LED core design but that IMHO storing just the raw RGB values wouldn't be a good idea and we should store brightness and color separately.

Quoted 5 lines

So let's take another example:
I set a color via sysfs and a trigger is controling the LED. Once the LED brightness is set to LED_OFF by the trigger and e.g. a sysfs read triggers an update the stored RGB value is 0,0,0.
If the LED is switched on again, then which color to choose?

The one stored in blink_brightness. I don't get how changing the interpretation of brightness value can affect anything here.

Quoted 5 lines

That's why I'm saying we should store color and brightness separately to avoid losing the color information information even if the brightness is set to zero.

Regards, Heiner

Am 25.01.2016 um 11:52 schrieb Jacek Anaszewski:

Quoted 144 lines · Heiner Kallweit

Quoted 142 lines

Quoted 124 lines · Jacek Anaszewski

Hi Heiner,

Quoted 62 lines · Heiner Kallweit

Am 17.01.2016 um 23:31 schrieb Jacek Anaszewski:

Quoted 60 lines · Heiner Kallweit

Quoted 58 lines

Am 14.01.2016 um 13:08 schrieb Jacek Anaszewski:

Quoted 56 lines · Heiner Kallweit

Quoted 54 lines

When playing with a ThingM Blink(1) USB RGB LED device I found that there
are few drivers for RGB LED's spread across the kernel and each one implements the RGB functionality in its own way.
Some examples:
- drivers/hid/hid-thingm.c
- drivers/usb/misc/usbled.c
- drivers/leds/leds-bd2802.c
- drivers/leds/leds-blinkm.c
...
IMHO it would make sense to add generic RGB functionality to the LED core.

Things I didn't like too much in other driver implementations:
- one led_classdev per color or at least
- one sysfs attribute per color
Colors are not really independent therefore I'd prefer one led_classdev
per RGB LED. Also userspace should be able to change the color with one
call -> therefore only one sysfs attribute for the RGB values.

Also the current brightness-related functionality should not be effected
by the RGB extension.

This patch is intended to demonstrate the idea of the extension. Most likely
it's not ready yet for submission.

Main idea is to base the effective RGB value on a combination of brightness
and a scaled-to-max RGB value.
The RGB-related callbacks are basically the same as for brightness. RGB functionally can be switched on with a new flag in the led_classdev.flags
bitmap.

Experimentally I changed the thingm driver to use this extension and it works
quite well. As one result the driver could be very much simplified.

Any feedback is appreciated.

Signed-off-by Heiner Kallweit <[email protected]>

---
      drivers/leds/led-class.c |  62 +++++++++++++++++++++++++++
      drivers/leds/led-core.c  | 106
+++++++++++++++++++++++++++++++++++++++++++++++
      drivers/leds/leds.h      |  12 ++++++
      include/linux/leds.h     |  13 ++++++
      4 files changed, 193 insertions(+)

[...]

Quoted 46 lines

Quoted 40 lines

Quoted 31 lines

Quoted 22 lines

Quoted 22 lines

(quoted diff) +22 -0

+static void led_set_rgb_raw(struct led_classdev *cdev, u32 rgb)
+{
+    u32 red_raw = (rgb >> 16) & 0xff;
+    u32 green_raw = (rgb >> 8) & 0xff;
+    u32 blue_raw = rgb & 0xff;
+    u32 max_raw, red, green, blue;
+
+    max_raw = max(red_raw, green_raw);
+    if (blue_raw > max_raw)
+        max_raw = blue_raw;
+
+    if (!max_raw) {
+        cdev->brightness = 0;
+        cdev->rgb_val = 0;
+        return;
+    }
+
+    red = DIV_ROUND_CLOSEST(red_raw * LED_FULL, max_raw);
+    green = DIV_ROUND_CLOSEST(green_raw * LED_FULL, max_raw);
+    blue = DIV_ROUND_CLOSEST(blue_raw * LED_FULL, max_raw);
+
+    cdev->brightness = max_raw;

Quoted 9 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+    cdev->rgb_val = (red << 16) + (green << 8) + blue;
+}

I think that we shouldn't impose specific way of calculating brightness depending on the rgb value set. We should just pass value from userspace
to the driver.

Quoted 7 lines

Right, setting brightness from userspace is no problem.
But how about led_update_brightness? It's supposed to update the brightness
value in led_cdev with the actual value. And for a RGB LED we have only the RGB values, so we need to calculate the brightness based on RGB values.
Or do you see a better way?

Quoted 6 lines

I thought that you had some device using both brightness and rgb components settings (no idea if this could be sane in any way). If there are only three color components, then why not just redefine brightness to store three components in case of the LEDs with LED_DEV_CAP_RGB flags set?

Quoted 8 lines

My devices just have the three color components (normal 8 bits per color). Even in the new RGB mode I don't want to break the current brightness-based API
but extend it with RGB functionality. Therefore I think it's best to store brightness and color separately.
Just one argument: If we store the color components only then we'll lose the color information if the brightness is set to 0.
And I would like to support e.g. software blink in whatever color.

Quoted 5 lines

Currently blinking works properly and brightness value isn't being lost. We store it in the led_cdev->blink_brightness property. Could you present exact use case you'd like to support and which is not feasible with current LED core design?

I didn't have a closer look on how soft blinking works and you're right, this was a bad example.
It's not about that something is wrong with the current LED core design but that IMHO storing just the raw RGB values wouldn't be a good idea and we should store brightness and color separately.

So let's take another example:
I set a color via sysfs and a trigger is controling the LED. Once the LED brightness is set to LED_OFF by the trigger and e.g. a sysfs read triggers an update the stored RGB value is 0,0,0.
If the LED is switched on again, then which color to choose?

Quoted 3 lines

The one stored in blink_brightness. I don't get how changing the interpretation of brightness value can affect anything here.

But blink_brightness is used in case of soft blink only.

If a trigger or userspace sets the brightness to zero then blink_brightness isn't used.

I agreed that soft blink was a bad example.

Quoted 8 lines

Quoted 5 lines

That's why I'm saying we should store color and brightness separately to avoid losing the color information information even if the brightness is set to zero.

Regards, Heiner

Quoted 145 lines

Am 25.01.2016 um 11:52 schrieb Jacek Anaszewski:

Quoted 144 lines · Heiner Kallweit

Quoted 142 lines

Quoted 124 lines · Jacek Anaszewski

Hi Heiner,

Quoted 62 lines · Heiner Kallweit

Am 17.01.2016 um 23:31 schrieb Jacek Anaszewski:

Quoted 60 lines · Heiner Kallweit

Quoted 58 lines

Am 14.01.2016 um 13:08 schrieb Jacek Anaszewski:

Quoted 56 lines · Heiner Kallweit

Quoted 54 lines

When playing with a ThingM Blink(1) USB RGB LED device I found that there
are few drivers for RGB LED's spread across the kernel and each one implements the RGB functionality in its own way.
Some examples:
- drivers/hid/hid-thingm.c
- drivers/usb/misc/usbled.c
- drivers/leds/leds-bd2802.c
- drivers/leds/leds-blinkm.c
...
IMHO it would make sense to add generic RGB functionality to the LED core.

Things I didn't like too much in other driver implementations:
- one led_classdev per color or at least
- one sysfs attribute per color
Colors are not really independent therefore I'd prefer one led_classdev
per RGB LED. Also userspace should be able to change the color with one
call -> therefore only one sysfs attribute for the RGB values.

Also the current brightness-related functionality should not be effected
by the RGB extension.

This patch is intended to demonstrate the idea of the extension. Most likely
it's not ready yet for submission.

Main idea is to base the effective RGB value on a combination of brightness
and a scaled-to-max RGB value.
The RGB-related callbacks are basically the same as for brightness. RGB functionally can be switched on with a new flag in the led_classdev.flags
bitmap.

Experimentally I changed the thingm driver to use this extension and it works
quite well. As one result the driver could be very much simplified.

Any feedback is appreciated.

Signed-off-by Heiner Kallweit <[email protected]>

---
      drivers/leds/led-class.c |  62 +++++++++++++++++++++++++++
      drivers/leds/led-core.c  | 106
+++++++++++++++++++++++++++++++++++++++++++++++
      drivers/leds/leds.h      |  12 ++++++
      include/linux/leds.h     |  13 ++++++
      4 files changed, 193 insertions(+)

[...]

Quoted 46 lines

Quoted 40 lines

Quoted 31 lines

Quoted 22 lines

Quoted 22 lines

(quoted diff) +22 -0

+static void led_set_rgb_raw(struct led_classdev *cdev, u32 rgb)
+{
+    u32 red_raw = (rgb >> 16) & 0xff;
+    u32 green_raw = (rgb >> 8) & 0xff;
+    u32 blue_raw = rgb & 0xff;
+    u32 max_raw, red, green, blue;
+
+    max_raw = max(red_raw, green_raw);
+    if (blue_raw > max_raw)
+        max_raw = blue_raw;
+
+    if (!max_raw) {
+        cdev->brightness = 0;
+        cdev->rgb_val = 0;
+        return;
+    }
+
+    red = DIV_ROUND_CLOSEST(red_raw * LED_FULL, max_raw);
+    green = DIV_ROUND_CLOSEST(green_raw * LED_FULL, max_raw);
+    blue = DIV_ROUND_CLOSEST(blue_raw * LED_FULL, max_raw);
+
+    cdev->brightness = max_raw;

Quoted 9 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+    cdev->rgb_val = (red << 16) + (green << 8) + blue;
+}

I think that we shouldn't impose specific way of calculating brightness depending on the rgb value set. We should just pass value from userspace
to the driver.

Quoted 7 lines

Right, setting brightness from userspace is no problem.
But how about led_update_brightness? It's supposed to update the brightness
value in led_cdev with the actual value. And for a RGB LED we have only the RGB values, so we need to calculate the brightness based on RGB values.
Or do you see a better way?

Quoted 6 lines

I thought that you had some device using both brightness and rgb components settings (no idea if this could be sane in any way). If there are only three color components, then why not just redefine brightness to store three components in case of the LEDs with LED_DEV_CAP_RGB flags set?

Quoted 8 lines

My devices just have the three color components (normal 8 bits per color). Even in the new RGB mode I don't want to break the current brightness-based API
but extend it with RGB functionality. Therefore I think it's best to store brightness and color separately.
Just one argument: If we store the color components only then we'll lose the color information if the brightness is set to 0.
And I would like to support e.g. software blink in whatever color.

Quoted 5 lines

Currently blinking works properly and brightness value isn't being lost. We store it in the led_cdev->blink_brightness property. Could you present exact use case you'd like to support and which is not feasible with current LED core design?

I didn't have a closer look on how soft blinking works and you're right, this was a bad example.
It's not about that something is wrong with the current LED core design but that IMHO storing just the raw RGB values wouldn't be a good idea and we should store brightness and color separately.

So let's take another example:
I set a color via sysfs and a trigger is controling the LED. Once the LED brightness is set to LED_OFF by the trigger and e.g. a sysfs read triggers an update the stored RGB value is 0,0,0.
If the LED is switched on again, then which color to choose?

Quoted 4 lines

Quoted 3 lines

The one stored in blink_brightness. I don't get how changing the interpretation of brightness value can affect anything here.

But blink_brightness is used in case of soft blink only.

In case of hardware blinking it is driver's responsibility to report current brightness while blinking is on. It is hardware specific whether it allows to read current LED state during blinking. Nevertheless I don't see where is the problem here either.

Quoted 2 lines

If a trigger or userspace sets the brightness to zero then blink_brightness isn't used.

Setting brightness to 0 disables trigger. Excerpt from leds-class.txt:

"However, if you set the brightness value to LED_OFF it will also disable the timer trigger."

Now I see that documentation is inexact here. The paragraph says about triggers, using timer trigger as an example. This trigger is specific because of the software fallback implemented in the core.

Setting brightness to LED_OFF disables any trigger only when set from sysfs (in drivers/leds/led-class.c brightness_store calls led_trigger_remove if passed brightness is LED_OFF).

However, when setting brightness to LED_OFF directly through LED API, and not through sysfs, only blink_timer is being disabled, but trigger isn't being removed. Probably we'd have to remove trigger from set_brightness_delayed(), in case LED_BLINK_DISABLE is set, to make implementation in line with the documentation.

Quoted 12 lines

I agreed that soft blink was a bad example.

Quoted 8 lines

Quoted 5 lines

That's why I'm saying we should store color and brightness separately to avoid losing the color information information even if the brightness is set to zero.

Regards, Heiner

Am 26.01.2016 um 10:37 schrieb Jacek Anaszewski:

Quoted 172 lines · Heiner Kallweit

Quoted 145 lines

Am 25.01.2016 um 11:52 schrieb Jacek Anaszewski:

Quoted 144 lines · Heiner Kallweit

Quoted 142 lines

Quoted 124 lines · Jacek Anaszewski

Hi Heiner,

Quoted 62 lines · Heiner Kallweit

Am 17.01.2016 um 23:31 schrieb Jacek Anaszewski:

Quoted 60 lines · Heiner Kallweit

Quoted 58 lines

Am 14.01.2016 um 13:08 schrieb Jacek Anaszewski:

Quoted 56 lines · Heiner Kallweit

Quoted 54 lines

When playing with a ThingM Blink(1) USB RGB LED device I found that there
are few drivers for RGB LED's spread across the kernel and each one implements the RGB functionality in its own way.
Some examples:
- drivers/hid/hid-thingm.c
- drivers/usb/misc/usbled.c
- drivers/leds/leds-bd2802.c
- drivers/leds/leds-blinkm.c
...
IMHO it would make sense to add generic RGB functionality to the LED core.

Things I didn't like too much in other driver implementations:
- one led_classdev per color or at least
- one sysfs attribute per color
Colors are not really independent therefore I'd prefer one led_classdev
per RGB LED. Also userspace should be able to change the color with one
call -> therefore only one sysfs attribute for the RGB values.

Also the current brightness-related functionality should not be effected
by the RGB extension.

This patch is intended to demonstrate the idea of the extension. Most likely
it's not ready yet for submission.

Main idea is to base the effective RGB value on a combination of brightness
and a scaled-to-max RGB value.
The RGB-related callbacks are basically the same as for brightness. RGB functionally can be switched on with a new flag in the led_classdev.flags
bitmap.

Experimentally I changed the thingm driver to use this extension and it works
quite well. As one result the driver could be very much simplified.

Any feedback is appreciated.

Signed-off-by Heiner Kallweit <[email protected]>

---
       drivers/leds/led-class.c |  62 +++++++++++++++++++++++++++
       drivers/leds/led-core.c  | 106
+++++++++++++++++++++++++++++++++++++++++++++++
       drivers/leds/leds.h      |  12 ++++++
       include/linux/leds.h     |  13 ++++++
       4 files changed, 193 insertions(+)

[...]

Quoted 46 lines

Quoted 40 lines

Quoted 31 lines

Quoted 22 lines

Quoted 22 lines

(quoted diff) +22 -0

+static void led_set_rgb_raw(struct led_classdev *cdev, u32 rgb)
+{
+    u32 red_raw = (rgb >> 16) & 0xff;
+    u32 green_raw = (rgb >> 8) & 0xff;
+    u32 blue_raw = rgb & 0xff;
+    u32 max_raw, red, green, blue;
+
+    max_raw = max(red_raw, green_raw);
+    if (blue_raw > max_raw)
+        max_raw = blue_raw;
+
+    if (!max_raw) {
+        cdev->brightness = 0;
+        cdev->rgb_val = 0;
+        return;
+    }
+
+    red = DIV_ROUND_CLOSEST(red_raw * LED_FULL, max_raw);
+    green = DIV_ROUND_CLOSEST(green_raw * LED_FULL, max_raw);
+    blue = DIV_ROUND_CLOSEST(blue_raw * LED_FULL, max_raw);
+
+    cdev->brightness = max_raw;

Quoted 9 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+    cdev->rgb_val = (red << 16) + (green << 8) + blue;
+}

I think that we shouldn't impose specific way of calculating brightness depending on the rgb value set. We should just pass value from userspace
to the driver.

Quoted 7 lines

Right, setting brightness from userspace is no problem.
But how about led_update_brightness? It's supposed to update the brightness
value in led_cdev with the actual value. And for a RGB LED we have only the RGB values, so we need to calculate the brightness based on RGB values.
Or do you see a better way?

Quoted 6 lines

I thought that you had some device using both brightness and rgb components settings (no idea if this could be sane in any way). If there are only three color components, then why not just redefine brightness to store three components in case of the LEDs with LED_DEV_CAP_RGB flags set?

Quoted 8 lines

My devices just have the three color components (normal 8 bits per color). Even in the new RGB mode I don't want to break the current brightness-based API
but extend it with RGB functionality. Therefore I think it's best to store brightness and color separately.
Just one argument: If we store the color components only then we'll lose the color information if the brightness is set to 0.
And I would like to support e.g. software blink in whatever color.

Quoted 5 lines

Currently blinking works properly and brightness value isn't being lost. We store it in the led_cdev->blink_brightness property. Could you present exact use case you'd like to support and which is not feasible with current LED core design?

I didn't have a closer look on how soft blinking works and you're right, this was a bad example.
It's not about that something is wrong with the current LED core design but that IMHO storing just the raw RGB values wouldn't be a good idea and we should store brightness and color separately.

So let's take another example:
I set a color via sysfs and a trigger is controling the LED. Once the LED brightness is set to LED_OFF by the trigger and e.g. a sysfs read triggers an update the stored RGB value is 0,0,0.
If the LED is switched on again, then which color to choose?

Quoted 4 lines

Quoted 3 lines

The one stored in blink_brightness. I don't get how changing the interpretation of brightness value can affect anything here.

But blink_brightness is used in case of soft blink only.

In case of hardware blinking it is driver's responsibility to report current brightness while blinking is on. It is hardware specific whether it allows to read current LED state during blinking. Nevertheless I don't see where is the problem here either.

Quoted 2 lines

If a trigger or userspace sets the brightness to zero then blink_brightness isn't used.

Setting brightness to 0 disables trigger. Excerpt from leds-class.txt:

"However, if you set the brightness value to LED_OFF it will also disable the timer trigger."

Now I see that documentation is inexact here. The paragraph says about triggers, using timer trigger as an example. This trigger is specific because of the software fallback implemented in the core.

Setting brightness to LED_OFF disables any trigger only when set from sysfs (in drivers/leds/led-class.c brightness_store calls led_trigger_remove if passed brightness is LED_OFF).

Quoted 6 lines

However, when setting brightness to LED_OFF directly through LED API, and not through sysfs, only blink_timer is being disabled, but trigger isn't being removed. Probably we'd have to remove trigger from set_brightness_delayed(), in case LED_BLINK_DISABLE is set, to make implementation in line with the documentation.

Sorry, I'm afraid we lost track of the original subject.
At least I'm a little lost ..
My understanding is that this discussion is about whether:
- use current brightness member of led_classdev to store brightness and color (change semantics of brightness to hold a <00000000><RRRRRRRR><GGGGGGGG><BBBBBBBB> value) or
- store brightness and color separately (introduce a new member for the color)

Is this also your understanding? Or are you focussing on a different aspect and I missed something?

Quoted 186 lines

Am 26.01.2016 um 10:37 schrieb Jacek Anaszewski:

Quoted 172 lines · Heiner Kallweit

Quoted 145 lines

Am 25.01.2016 um 11:52 schrieb Jacek Anaszewski:

Quoted 144 lines · Heiner Kallweit

Quoted 142 lines

Quoted 124 lines · Jacek Anaszewski

Hi Heiner,

Quoted 62 lines · Heiner Kallweit

Am 17.01.2016 um 23:31 schrieb Jacek Anaszewski:

Quoted 60 lines · Heiner Kallweit

Quoted 58 lines

Am 14.01.2016 um 13:08 schrieb Jacek Anaszewski:

Quoted 56 lines · Heiner Kallweit

Quoted 54 lines

When playing with a ThingM Blink(1) USB RGB LED device I found that there
are few drivers for RGB LED's spread across the kernel and each one implements the RGB functionality in its own way.
Some examples:
- drivers/hid/hid-thingm.c
- drivers/usb/misc/usbled.c
- drivers/leds/leds-bd2802.c
- drivers/leds/leds-blinkm.c
...
IMHO it would make sense to add generic RGB functionality to the LED core.

Things I didn't like too much in other driver implementations:
- one led_classdev per color or at least
- one sysfs attribute per color
Colors are not really independent therefore I'd prefer one led_classdev
per RGB LED. Also userspace should be able to change the color with one
call -> therefore only one sysfs attribute for the RGB values.

Also the current brightness-related functionality should not be effected
by the RGB extension.

This patch is intended to demonstrate the idea of the extension. Most likely
it's not ready yet for submission.

Main idea is to base the effective RGB value on a combination of brightness
and a scaled-to-max RGB value.
The RGB-related callbacks are basically the same as for brightness. RGB functionally can be switched on with a new flag in the led_classdev.flags
bitmap.

Experimentally I changed the thingm driver to use this extension and it works
quite well. As one result the driver could be very much simplified.

Any feedback is appreciated.

Signed-off-by Heiner Kallweit <[email protected]>

---
       drivers/leds/led-class.c |  62 +++++++++++++++++++++++++++
       drivers/leds/led-core.c  | 106
+++++++++++++++++++++++++++++++++++++++++++++++
       drivers/leds/leds.h      |  12 ++++++
       include/linux/leds.h     |  13 ++++++
       4 files changed, 193 insertions(+)

[...]

Quoted 46 lines

Quoted 40 lines

Quoted 31 lines

Quoted 22 lines

Quoted 22 lines

(quoted diff) +22 -0

+static void led_set_rgb_raw(struct led_classdev *cdev, u32 rgb)
+{
+    u32 red_raw = (rgb >> 16) & 0xff;
+    u32 green_raw = (rgb >> 8) & 0xff;
+    u32 blue_raw = rgb & 0xff;
+    u32 max_raw, red, green, blue;
+
+    max_raw = max(red_raw, green_raw);
+    if (blue_raw > max_raw)
+        max_raw = blue_raw;
+
+    if (!max_raw) {
+        cdev->brightness = 0;
+        cdev->rgb_val = 0;
+        return;
+    }
+
+    red = DIV_ROUND_CLOSEST(red_raw * LED_FULL, max_raw);
+    green = DIV_ROUND_CLOSEST(green_raw * LED_FULL, max_raw);
+    blue = DIV_ROUND_CLOSEST(blue_raw * LED_FULL, max_raw);
+
+    cdev->brightness = max_raw;

Quoted 9 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+    cdev->rgb_val = (red << 16) + (green << 8) + blue;
+}

I think that we shouldn't impose specific way of calculating brightness depending on the rgb value set. We should just pass value from userspace
to the driver.

Quoted 7 lines

Right, setting brightness from userspace is no problem.
But how about led_update_brightness? It's supposed to update the brightness
value in led_cdev with the actual value. And for a RGB LED we have only the RGB values, so we need to calculate the brightness based on RGB values.
Or do you see a better way?

Quoted 6 lines

I thought that you had some device using both brightness and rgb components settings (no idea if this could be sane in any way). If there are only three color components, then why not just redefine brightness to store three components in case of the LEDs with LED_DEV_CAP_RGB flags set?

Quoted 8 lines

My devices just have the three color components (normal 8 bits per color). Even in the new RGB mode I don't want to break the current brightness-based API
but extend it with RGB functionality. Therefore I think it's best to store brightness and color separately.
Just one argument: If we store the color components only then we'll lose the color information if the brightness is set to 0.
And I would like to support e.g. software blink in whatever color.

Quoted 5 lines

Currently blinking works properly and brightness value isn't being lost. We store it in the led_cdev->blink_brightness property. Could you present exact use case you'd like to support and which is not feasible with current LED core design?

I didn't have a closer look on how soft blinking works and you're right, this was a bad example.
It's not about that something is wrong with the current LED core design but that IMHO storing just the raw RGB values wouldn't be a good idea and we should store brightness and color separately.

So let's take another example:
I set a color via sysfs and a trigger is controling the LED. Once the LED brightness is set to LED_OFF by the trigger and e.g. a sysfs read triggers an update the stored RGB value is 0,0,0.
If the LED is switched on again, then which color to choose?

Quoted 4 lines

Quoted 3 lines

The one stored in blink_brightness. I don't get how changing the interpretation of brightness value can affect anything here.

But blink_brightness is used in case of soft blink only.

In case of hardware blinking it is driver's responsibility to report current brightness while blinking is on. It is hardware specific whether it allows to read current LED state during blinking. Nevertheless I don't see where is the problem here either.

Quoted 2 lines

If a trigger or userspace sets the brightness to zero then blink_brightness isn't used.

Setting brightness to 0 disables trigger. Excerpt from leds-class.txt:

"However, if you set the brightness value to LED_OFF it will also disable the timer trigger."

Now I see that documentation is inexact here. The paragraph says about triggers, using timer trigger as an example. This trigger is specific because of the software fallback implemented in the core.

Setting brightness to LED_OFF disables any trigger only when set from sysfs (in drivers/leds/led-class.c brightness_store calls led_trigger_remove if passed brightness is LED_OFF).

Quoted 6 lines

However, when setting brightness to LED_OFF directly through LED API, and not through sysfs, only blink_timer is being disabled, but trigger isn't being removed. Probably we'd have to remove trigger from set_brightness_delayed(), in case LED_BLINK_DISABLE is set, to make implementation in line with the documentation.

Sorry, I'm afraid we lost track of the original subject.
At least I'm a little lost ..
My understanding is that this discussion is about whether:
- use current brightness member of led_classdev to store brightness and color (change semantics of brightness to hold a <00000000><RRRRRRRR><GGGGGGGG><BBBBBBBB> value) or
- store brightness and color separately (introduce a new member for the color)

Quoted 2 lines

Is this also your understanding? Or are you focussing on a different aspect and I missed something?

Yes it is. I intended to explain that setting brightness to 0 is supposed to turn the trigger off. In effect the brightness information is not required afterwards because trigger is disabled.

During the analysis I realized about little discrepancy between the documentation and the implementation and proposed potential fix.

I analyzed this basing on the current interpretation of brightness. I think we need to focus on your following requirement:

"And I would like to support e.g. software blink in whatever color"

Could you provide more details?

Am 27.01.2016 um 09:27 schrieb Jacek Anaszewski:

Quoted 199 lines · Heiner Kallweit

Quoted 186 lines

Am 26.01.2016 um 10:37 schrieb Jacek Anaszewski:

Quoted 172 lines · Heiner Kallweit

Quoted 145 lines

Am 25.01.2016 um 11:52 schrieb Jacek Anaszewski:

Quoted 144 lines · Heiner Kallweit

Quoted 142 lines

Quoted 124 lines · Jacek Anaszewski

Hi Heiner,

Quoted 62 lines · Heiner Kallweit

Am 17.01.2016 um 23:31 schrieb Jacek Anaszewski:

Quoted 60 lines · Heiner Kallweit

Quoted 58 lines

Am 14.01.2016 um 13:08 schrieb Jacek Anaszewski:

Quoted 56 lines · Heiner Kallweit

Quoted 54 lines

When playing with a ThingM Blink(1) USB RGB LED device I found that
there
are few drivers for RGB LED's spread across the kernel and each one
implements the RGB functionality in its own way.
Some examples:
- drivers/hid/hid-thingm.c
- drivers/usb/misc/usbled.c
- drivers/leds/leds-bd2802.c
- drivers/leds/leds-blinkm.c
...
IMHO it would make sense to add generic RGB functionality to the LED
core.

Things I didn't like too much in other driver implementations:
- one led_classdev per color or at least
- one sysfs attribute per color
Colors are not really independent therefore I'd prefer one
led_classdev
per RGB LED. Also userspace should be able to change the color with
one
call -> therefore only one sysfs attribute for the RGB values.

Also the current brightness-related functionality should not be
effected
by the RGB extension.

This patch is intended to demonstrate the idea of the extension. Most
likely
it's not ready yet for submission.

Main idea is to base the effective RGB value on a combination of
brightness
and a scaled-to-max RGB value.
The RGB-related callbacks are basically the same as for brightness.
RGB functionally can be switched on with a new flag in the
led_classdev.flags
bitmap.

Experimentally I changed the thingm driver to use this extension and
it works
quite well. As one result the driver could be very much simplified.

Any feedback is appreciated.

Signed-off-by: Heiner Kallweit <[email protected]>
---
drivers/leds/led-class.c | 62 +++++++++++++++++++++++++++
drivers/leds/led-core.c | 106
+++++++++++++++++++++++++++++++++++++++++++++++
drivers/leds/leds.h | 12 ++++++
include/linux/leds.h | 13 ++++++
4 files changed, 193 insertions(+)

[...]

Quoted 46 lines

Quoted 40 lines

Quoted 31 lines

Quoted 22 lines

Quoted 22 lines

+static void led_set_rgb_raw(struct led_classdev *cdev, u32 rgb)
+{
+ u32 red_raw = (rgb >> 16) & 0xff;
+ u32 green_raw = (rgb >> 8) & 0xff;
+ u32 blue_raw = rgb & 0xff;
+ u32 max_raw, red, green, blue;
+
+ max_raw = max(red_raw, green_raw);
+ if (blue_raw > max_raw)
+ max_raw = blue_raw;
+
+ if (!max_raw) {
+ cdev->brightness = 0;
+ cdev->rgb_val = 0;
+ return;
+ }
+
+ red = DIV_ROUND_CLOSEST(red_raw * LED_FULL, max_raw);
+ green = DIV_ROUND_CLOSEST(green_raw * LED_FULL, max_raw);
+ blue = DIV_ROUND_CLOSEST(blue_raw * LED_FULL, max_raw);
+
+ cdev->brightness = max_raw;

Quoted 9 lines

Quoted 2 lines

+ cdev->rgb_val = (red << 16) + (green << 8) + blue;
+}

I think that we shouldn't impose specific way of calculating brightness depending on the rgb value set. We should just pass value from userspace
to the driver.

Quoted 7 lines

Right, setting brightness from userspace is no problem.
But how about led_update_brightness? It's supposed to update the brightness
value in led_cdev with the actual value. And for a RGB LED we have only the RGB values, so we need to calculate the brightness based on RGB values.
Or do you see a better way?

Quoted 6 lines

I thought that you had some device using both brightness and rgb components settings (no idea if this could be sane in any way). If there are only three color components, then why not just redefine brightness to store three components in case of the LEDs with LED_DEV_CAP_RGB flags set?

Quoted 8 lines

My devices just have the three color components (normal 8 bits per color). Even in the new RGB mode I don't want to break the current brightness-based API
but extend it with RGB functionality. Therefore I think it's best to store brightness and color separately.
Just one argument: If we store the color components only then we'll lose the color information if the brightness is set to 0.
And I would like to support e.g. software blink in whatever color.

Quoted 5 lines

Currently blinking works properly and brightness value isn't being lost. We store it in the led_cdev->blink_brightness property. Could you present exact use case you'd like to support and which is not feasible with current LED core design?

I didn't have a closer look on how soft blinking works and you're right, this was a bad example.
It's not about that something is wrong with the current LED core design but that IMHO storing just the raw RGB values wouldn't be a good idea and we should store brightness and color separately.

So let's take another example:
I set a color via sysfs and a trigger is controling the LED. Once the LED brightness is set to LED_OFF by the trigger and e.g. a sysfs read triggers an update the stored RGB value is 0,0,0.
If the LED is switched on again, then which color to choose?

Quoted 4 lines

Quoted 3 lines

The one stored in blink_brightness. I don't get how changing the interpretation of brightness value can affect anything here.

But blink_brightness is used in case of soft blink only.

In case of hardware blinking it is driver's responsibility to report current brightness while blinking is on. It is hardware specific whether it allows to read current LED state during blinking. Nevertheless I don't see where is the problem here either.

Quoted 2 lines

If a trigger or userspace sets the brightness to zero then blink_brightness isn't used.

Setting brightness to 0 disables trigger. Excerpt from leds-class.txt:

"However, if you set the brightness value to LED_OFF it will also disable the timer trigger."

Now I see that documentation is inexact here. The paragraph says about triggers, using timer trigger as an example. This trigger is specific because of the software fallback implemented in the core.

Setting brightness to LED_OFF disables any trigger only when set from sysfs (in drivers/leds/led-class.c brightness_store calls led_trigger_remove if passed brightness is LED_OFF).

Quoted 6 lines

However, when setting brightness to LED_OFF directly through LED API, and not through sysfs, only blink_timer is being disabled, but trigger isn't being removed. Probably we'd have to remove trigger from set_brightness_delayed(), in case LED_BLINK_DISABLE is set, to make implementation in line with the documentation.

Sorry, I'm afraid we lost track of the original subject.
At least I'm a little lost ..
My understanding is that this discussion is about whether:
- use current brightness member of led_classdev to store brightness and color (change semantics of brightness to hold a <00000000><RRRRRRRR><GGGGGGGG><BBBBBBBB> value) or
- store brightness and color separately (introduce a new member for the color)

Quoted 2 lines

Is this also your understanding? Or are you focussing on a different aspect and I missed something?

Yes it is. I intended to explain that setting brightness to 0 is supposed to turn the trigger off. In effect the brightness information is not required afterwards because trigger is disabled.

During the analysis I realized about little discrepancy between the documentation and the implementation and proposed potential fix.

I analyzed this basing on the current interpretation of brightness. I think we need to focus on your following requirement: