RGB support prototype

RGB support prototype

[Pavel Machek]

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


Hi!

You mentioned you was working on RGB support prototype. Could you post
copy of the patches (even if unfinished)?

Thanks,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: RGB support prototype

[Jacek Anaszewski]

Hi,

On 08/13/2017 12:48 AM, Pavel Machek wrote:
> 
> Hi!
> 
> You mentioned you was working on RGB support prototype. Could you post
> copy of the patches (even if unfinished)?

Unfortunately it is at the stage of unfinished proof of concept and
I haven't managed yet to try how it fits to all API use cases we have.
Nor is it in a shape ready to post to the lists.

I think we could try to discuss the design here. I'll list all the
issues I encountered during the implementation:

Currently we set LED brightness with following API:

void led_set_brightness(struct led_classdev *led_cdev,
                        enum led_brightness brightness);

In case of RGB LED we could have something like this:

struct led_color_triplet {
        enum led_brightness red;
        enum led_brightness green;
        enum led_brightness blue;
};

void led_rgb_set_brightness(struct led_rgb_classdev *led_rgb_cdev,
                        struct led_color_triplet *color);

We've agreed that LED RGB class device color could be set by writing
space separated list of "r g b" color values to a sysfs "color" file.

While the above itself shouldn't raise too many doubts, they
arise quickly while trying to adapt it to the internal LED core
facilities:

- led_base_timer_function()
- set_brightness_delayed()
- led_blink_* API family

All these introduce problems especially with brightness/color type.
I tried to add a new abstraction layer by introducing
struct led_base_cdev with a set of generic ops that could be initialized
by particular type of LED but still the problem with brightness type
generalization remains. One solution could be an union with fields
mapping to either single or three brightness components.

Other option could be void *brightness type which could be then
cast to the required brightness type basing on the LED flag.

Generally it seems that all existing LED class API should get its
generic counterparts.

Other ideas are welcome.

-- 
Best regards,
Jacek Anaszewski

Re: RGB support prototype

[Pavel Machek]

Hi!

> > You mentioned you was working on RGB support prototype. Could you post
> > copy of the patches (even if unfinished)?
> 
> Unfortunately it is at the stage of unfinished proof of concept and
> I haven't managed yet to try how it fits to all API use cases we have.
> Nor is it in a shape ready to post to the lists.
> 
> I think we could try to discuss the design here. I'll list all the
> issues I encountered during the implementation:
> 
> Currently we set LED brightness with following API:
> 
> void led_set_brightness(struct led_classdev *led_cdev,
>                         enum led_brightness brightness);
> 
> In case of RGB LED we could have something like this:
> 
> struct led_color_triplet {
>         enum led_brightness red;
>         enum led_brightness green;
>         enum led_brightness blue;
> };
> 
> void led_rgb_set_brightness(struct led_rgb_classdev *led_rgb_cdev,
>                         struct led_color_triplet *color);
> 
> We've agreed that LED RGB class device color could be set by writing
> space separated list of "r g b" color values to a sysfs "color" file.
> 
> While the above itself shouldn't raise too many doubts, they
> arise quickly while trying to adapt it to the internal LED core
> facilities:
> 
> - led_base_timer_function()
> - set_brightness_delayed()
> - led_blink_* API family
> 
> All these introduce problems especially with brightness/color type.
> I tried to add a new abstraction layer by introducing
> struct led_base_cdev with a set of generic ops that could be initialized
> by particular type of LED but still the problem with brightness type
> generalization remains. One solution could be an union with fields
> mapping to either single or three brightness components.
> 
> Other option could be void *brightness type which could be then
> cast to the required brightness type basing on the LED flag.

Void *brightness is not really a good option.

We could pass triplet even in case of single-color LEDs, and then use
just one component.

Another option would be to store color in HSV colorspace (not RGB). Then existing
functions would get brightness (== value in HSV), and RGB-aware functions would
operate on all 3 components. Triggers/blinking/etc. would then continue operating,
without modifications.

We could even export (read-only) hue/saturation for single-color LEDs...

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: RGB support prototype

[Pavel Machek]

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

Hi!

> > You mentioned you was working on RGB support prototype. Could you post
> > copy of the patches (even if unfinished)?
> 
> Unfortunately it is at the stage of unfinished proof of concept and
> I haven't managed yet to try how it fits to all API use cases we have.
> Nor is it in a shape ready to post to the lists.

Well, if it compiles, it is in shape. If it does not compile, it is in
shape, too :-).
									Pavel
									
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: RGB support prototype

[Jacek Anaszewski]

Hi,

On 08/13/2017 01:41 PM, Pavel Machek wrote:
> Hi!
> 
>>> You mentioned you was working on RGB support prototype. Could you post
>>> copy of the patches (even if unfinished)?
>>
>> Unfortunately it is at the stage of unfinished proof of concept and
>> I haven't managed yet to try how it fits to all API use cases we have.
>> Nor is it in a shape ready to post to the lists.
>>
>> I think we could try to discuss the design here. I'll list all the
>> issues I encountered during the implementation:
>>
>> Currently we set LED brightness with following API:
>>
>> void led_set_brightness(struct led_classdev *led_cdev,
>>                         enum led_brightness brightness);
>>
>> In case of RGB LED we could have something like this:
>>
>> struct led_color_triplet {
>>         enum led_brightness red;
>>         enum led_brightness green;
>>         enum led_brightness blue;
>> };
>>
>> void led_rgb_set_brightness(struct led_rgb_classdev *led_rgb_cdev,
>>                         struct led_color_triplet *color);
>>
>> We've agreed that LED RGB class device color could be set by writing
>> space separated list of "r g b" color values to a sysfs "color" file.
>>
>> While the above itself shouldn't raise too many doubts, they
>> arise quickly while trying to adapt it to the internal LED core
>> facilities:
>>
>> - led_base_timer_function()
>> - set_brightness_delayed()
>> - led_blink_* API family
>>
>> All these introduce problems especially with brightness/color type.
>> I tried to add a new abstraction layer by introducing
>> struct led_base_cdev with a set of generic ops that could be initialized
>> by particular type of LED but still the problem with brightness type
>> generalization remains. One solution could be an union with fields
>> mapping to either single or three brightness components.
>>
>> Other option could be void *brightness type which could be then
>> cast to the required brightness type basing on the LED flag.
> 
> Void *brightness is not really a good option.
> 
> We could pass triplet even in case of single-color LEDs, and then use
> just one component.
> 
> Another option would be to store color in HSV colorspace (not RGB). Then existing
> functions would get brightness (== value in HSV), and RGB-aware functions would
> operate on all 3 components. Triggers/blinking/etc. would then continue operating,
> without modifications.
> 
> We could even export (read-only) hue/saturation for single-color LEDs...

Related patches from Heiner Kallweit are still sitting on devel branch
of linux-leds.git:

https://git.kernel.org/pub/scm/linux/kernel/git/j.anaszewski/linux-leds.git/log/?h=devel

Possibly it can serve as a basis for further development.

I liked that approach because it was compatible with monochrome
LEDs and triggers.

-- 
Best regards,
Jacek Anaszewski

Re: RGB support prototype

[Pavel Machek]

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

Hi!

> > We could even export (read-only) hue/saturation for single-color LEDs...
> 
> Related patches from Heiner Kallweit are still sitting on devel branch
> of linux-leds.git:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/j.anaszewski/linux-leds.git/log/?h=devel
> 
> Possibly it can serve as a basis for further development.

> I liked that approach because it was compatible with monochrome
> LEDs and triggers.

Yeah, I like that. I don't like the implementation and the sysfs
interface.

Let me try and see if I can turn them into something better...

I'm using full 32-bits for parameters; that should give us enough
precision. In particular, 32-bits is important for "brightness"; I
already have light that goes from .1lm to 1000lm -- which is more than
8-bits can handle.

lp5523 conversion... is a very quick hack.

Signed-off-by: Pavel Machek <pavel@ucw.cz>


									Pavel

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 594b24d..bad8a6e 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -19,6 +19,14 @@ config LEDS_CLASS
 	  This option enables the led sysfs class in /sys/class/leds.  You'll
 	  need this to do anything useful with LEDs.  If unsure, say N.
 
+config LEDS_CLASS_RGB
+	bool "LED RGB Class Support"
+	depends on LEDS_CLASS
+	help
+	  This option enables support for RGB LED devices.
+	  Sysfs attribute brightness is interpreted as a HSV color value.
+	  For details see Documentation/leds/leds-class.txt.
+
 config LEDS_CLASS_FLASH
 	tristate "LED Flash Class Support"
 	depends on LEDS_CLASS
@@ -38,6 +46,10 @@ config LEDS_BRIGHTNESS_HW_CHANGED
 
 	  See Documentation/ABI/testing/sysfs-class-led for details.
 
+if LEDS_CLASS_RGB
+comment "RGB LED drivers"
+endif # LEDS_CLASS_RGB
+
 comment "LED drivers"
 
 config LEDS_88PM860X
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index 909dae6..d5b5e76 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -1,6 +1,8 @@
 
 # LED Core
-obj-$(CONFIG_NEW_LEDS)			+= led-core.o
+obj-$(CONFIG_NEW_LEDS)			+= led-core-objs.o
+led-core-objs-y				:= led-core.o
+led-core-objs-$(CONFIG_LEDS_CLASS_RGB)	+= led-rgb-core.o
 obj-$(CONFIG_LEDS_CLASS)		+= led-class.o
 obj-$(CONFIG_LEDS_CLASS_FLASH)		+= led-class-flash.o
 obj-$(CONFIG_LEDS_TRIGGERS)		+= led-triggers.o
diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index b0e2d55..0bd68a4 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -65,6 +65,83 @@ static ssize_t brightness_store(struct device *dev,
 }
 static DEVICE_ATTR_RW(brightness);
 
+static ssize_t saturation_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+
+	/* no lock needed for this */
+	led_update_brightness(led_cdev);
+
+	return sprintf(buf, "%u\n", led_cdev->saturation);
+}
+
+static ssize_t saturation_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, 10, &state);
+	if (ret)
+		goto unlock;
+
+	led_cdev->saturation = state;
+
+	ret = size;
+unlock:
+	mutex_unlock(&led_cdev->led_access);
+	return ret;
+}
+static DEVICE_ATTR_RW(saturation);
+
+static ssize_t hue_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+
+	/* no lock needed for this */
+	led_update_brightness(led_cdev);
+
+	return sprintf(buf, "%u\n", led_cdev->hue);
+}
+
+static ssize_t hue_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, 10, &state);
+	if (ret)
+		goto unlock;
+
+	led_cdev->hue = state;
+
+	ret = size;
+unlock:
+	mutex_unlock(&led_cdev->led_access);
+	return ret;
+}
+static DEVICE_ATTR_RW(hue);
+
+
 static ssize_t max_brightness_show(struct device *dev,
 		struct device_attribute *attr, char *buf)
 {
@@ -88,6 +165,8 @@ static const struct attribute_group led_trigger_group = {
 static struct attribute *led_class_attrs[] = {
 	&dev_attr_brightness.attr,
 	&dev_attr_max_brightness.attr,
+	&dev_attr_hue.attr,
+	&dev_attr_saturation.attr,
 	NULL,
 };
 
diff --git a/drivers/leds/led-rgb-core.c b/drivers/leds/led-rgb-core.c
new file mode 100644
index 0000000..3d79ba1
--- /dev/null
+++ b/drivers/leds/led-rgb-core.c
@@ -0,0 +1,60 @@
+/*
+ * LED RGB Class Support
+ *
+ * Author: Heiner Kallweit <hkallweit1@gmail.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ */
+
+#include <linux/leds.h>
+#include "leds.h"
+
+struct led_rgb led_hsv_to_rgb(struct led_hsv hsv)
+{
+	int h = hsv.hue >> 24;
+	int s = hsv.saturation >> 24;
+	int v = hsv.value >> 24;
+	int f, p, q, t, r, g, b;
+	struct led_rgb res;
+
+	if (!v) {
+		res.red = 0;
+		res.green = 0;
+		res.blue = 0;
+		return res;
+	}
+	if (!s) {
+		res.red = v << 24;
+		res.green = v << 24;
+		res.blue = v << 24;
+		return res;
+	}
+
+	f = DIV_ROUND_CLOSEST((h % 42) * 255, 42);
+	p = v - DIV_ROUND_CLOSEST(s * v, 255);
+	q = v - DIV_ROUND_CLOSEST(f * s * v, 255 * 255);
+	t = v - DIV_ROUND_CLOSEST((255 - f) * s * v, 255 * 255);
+
+	switch (h / 42) {
+	case 0:
+		r = v; g = t; b = p; break;
+	case 1:
+		r = q; g = v; b = p; break;
+	case 2:
+		r = p; g = v; b = t; break;
+	case 3:
+		r = p; g = q; b = v; break;
+	case 4:
+		r = t; g = p; b = v; break;
+	case 5:
+		r = v; g = p; b = q; break;
+	}
+
+	res.red = r << 24;
+	res.green = g << 24;
+	res.blue = b << 24;
+}
+EXPORT_SYMBOL_GPL(led_hsv_to_rgb);
diff --git a/drivers/leds/leds-lp5523.c b/drivers/leds/leds-lp5523.c
index 924e50a..861f4df2 100644
--- a/drivers/leds/leds-lp5523.c
+++ b/drivers/leds/leds-lp5523.c
@@ -802,6 +802,26 @@ static ssize_t store_master_fader_leds(struct device *dev,
 	return ret;
 }
 
+int lp5523_rgb_brightness(struct lp55xx_led *led, struct led_rgb r)
+{
+	struct lp55xx_chip *chip = led->chip;
+	int ret;
+
+	mutex_lock(&chip->lock);
+	r.red >>= 24;
+	r.green >>= 24;
+	r.blue >>= 24;	
+	printk("RGB brightness %d %d %d\n", r.red, r.green, r.blue);
+	ret = lp55xx_write(chip, LP5523_REG_LED_PWM_BASE + led->chan_nr, r.red);
+	if (!ret)
+		lp55xx_write(chip, LP5523_REG_LED_PWM_BASE + led->chan_nr - 1, r.green);
+	if (!ret)
+		lp55xx_write(chip, LP5523_REG_LED_PWM_BASE + led->chan_nr - 1, r.blue);
+	mutex_unlock(&chip->lock);
+	return ret;
+
+}
+
 static int lp5523_led_brightness(struct lp55xx_led *led)
 {
 	struct lp55xx_chip *chip = led->chip;
diff --git a/drivers/leds/leds-lp55xx-common.c b/drivers/leds/leds-lp55xx-common.c
index 5377f22..16e65dc 100644
--- a/drivers/leds/leds-lp55xx-common.c
+++ b/drivers/leds/leds-lp55xx-common.c
@@ -134,6 +134,8 @@ static struct attribute *lp55xx_led_attrs[] = {
 };
 ATTRIBUTE_GROUPS(lp55xx_led);
 
+extern int lp5523_rgb_brightness(struct lp55xx_led *led, struct led_rgb r);
+
 static int lp55xx_set_brightness(struct led_classdev *cdev,
 			     enum led_brightness brightness)
 {
@@ -141,6 +143,18 @@ static int lp55xx_set_brightness(struct led_classdev *cdev,
 	struct lp55xx_device_config *cfg = led->chip->cfg;
 
 	led->brightness = (u8)brightness;
+
+	if (led->chan_nr == 6) {
+		struct led_rgb r;
+		struct led_hsv hsv;
+
+		printk("RGB set request: %d %d %d\n", cdev->brightness, cdev->hue >> 24, cdev->saturation >> 24);
+		hsv.value = brightness << 24;
+		hsv.hue = cdev->hue;
+		hsv.saturation = cdev->saturation;
+		r = led_hsv_to_rgb(hsv);
+		return lp5523_rgb_brightness(led, r);
+	}
 	return cfg->brightness_fn(led);
 }
 
@@ -176,6 +190,8 @@ static int lp55xx_init_led(struct lp55xx_led *led,
 	led->cdev.brightness_set_blocking = lp55xx_set_brightness;
 	led->cdev.groups = lp55xx_led_groups;
 
+	printk("Led %d name %s\n", chan, pdata->led_config[chan].name);
+
 	if (pdata->led_config[chan].name) {
 		led->cdev.name = pdata->led_config[chan].name;
 	} else {
diff --git a/include/linux/leds.h b/include/linux/leds.h
index 64c56d4..bd0ae46 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -37,6 +37,8 @@ struct led_classdev {
 	const char		*name;
 	enum led_brightness	 brightness;
 	enum led_brightness	 max_brightness;
+	int			 hue;
+	int			 saturation;
 	int			 flags;
 
 	/* Lower 16 bits reflect status */
@@ -49,6 +51,7 @@ struct led_classdev {
 #define LED_HW_PLUGGABLE	(1 << 19)
 #define LED_PANIC_INDICATOR	(1 << 20)
 #define LED_BRIGHT_HW_CHANGED	(1 << 21)
+#define LED_DEV_CAP_RGB		(1 << 24)
 
 	/* set_brightness_work / blink_timer flags, atomic, private. */
 	unsigned long		work_flags;
@@ -238,6 +241,26 @@ static inline bool led_sysfs_is_disabled(struct led_classdev *led_cdev)
 	return led_cdev->flags & LED_SYSFS_DISABLE;
 }
 
+struct led_hsv {
+	u32 hue;
+	u32 saturation;
+	u32 value;
+};
+
+struct led_rgb {
+	u32 red;
+	u32 green;
+	u32 blue;
+};
+
+/**
+ * led_hsv_to_rgb - convert a hsv color value to rgb color model
+ * @hsv: the hsv value to convert
+ *
+ * Returns: the resulting rgb value
+ */
+struct led_rgb led_hsv_to_rgb(struct led_hsv hsv);
+
 /*
  * LED Triggers
  */

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: RGB support prototype

[Pavel Machek]

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

Hi!

> > > We could even export (read-only) hue/saturation for single-color LEDs...
> > 
> > Related patches from Heiner Kallweit are still sitting on devel branch
> > of linux-leds.git:
> > 
> > https://git.kernel.org/pub/scm/linux/kernel/git/j.anaszewski/linux-leds.git/log/?h=devel
> > 
> > Possibly it can serve as a basis for further development.
> 
> > I liked that approach because it was compatible with monochrome
> > LEDs and triggers.
> 
> Yeah, I like that. I don't like the implementation and the sysfs
> interface.
> 
> Let me try and see if I can turn them into something better...
> 
> I'm using full 32-bits for parameters; that should give us enough
> precision. In particular, 32-bits is important for "brightness"; I
> already have light that goes from .1lm to 1000lm -- which is more than
> 8-bits can handle.
> 
> lp5523 conversion... is a very quick hack.

And here's a better version. I'll still need to adjust white to be
white... but I guess that would need to be done with any solution.

I've checked, and triggers seem to work as expected (and I can select
a color for them).

Signed-off-by: Pavel Machek <pavel@ucw.cz>

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 594b24d..bad8a6e 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -19,6 +19,14 @@ config LEDS_CLASS
 	  This option enables the led sysfs class in /sys/class/leds.  You'll
 	  need this to do anything useful with LEDs.  If unsure, say N.
 
+config LEDS_CLASS_RGB
+	bool "LED RGB Class Support"
+	depends on LEDS_CLASS
+	help
+	  This option enables support for RGB LED devices.
+	  Sysfs attribute brightness is interpreted as a HSV color value.
+	  For details see Documentation/leds/leds-class.txt.
+
 config LEDS_CLASS_FLASH
 	tristate "LED Flash Class Support"
 	depends on LEDS_CLASS
@@ -38,6 +46,10 @@ config LEDS_BRIGHTNESS_HW_CHANGED
 
 	  See Documentation/ABI/testing/sysfs-class-led for details.
 
+if LEDS_CLASS_RGB
+comment "RGB LED drivers"
+endif # LEDS_CLASS_RGB
+
 comment "LED drivers"
 
 config LEDS_88PM860X
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index 909dae6..d5b5e76 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -1,6 +1,8 @@
 
 # LED Core
-obj-$(CONFIG_NEW_LEDS)			+= led-core.o
+obj-$(CONFIG_NEW_LEDS)			+= led-core-objs.o
+led-core-objs-y				:= led-core.o
+led-core-objs-$(CONFIG_LEDS_CLASS_RGB)	+= led-rgb-core.o
 obj-$(CONFIG_LEDS_CLASS)		+= led-class.o
 obj-$(CONFIG_LEDS_CLASS_FLASH)		+= led-class-flash.o
 obj-$(CONFIG_LEDS_TRIGGERS)		+= led-triggers.o
diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index b0e2d55..0bd68a4 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -65,6 +65,83 @@ static ssize_t brightness_store(struct device *dev,
 }
 static DEVICE_ATTR_RW(brightness);
 
+static ssize_t saturation_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+
+	/* no lock needed for this */
+	led_update_brightness(led_cdev);
+
+	return sprintf(buf, "%u\n", led_cdev->saturation);
+}
+
+static ssize_t saturation_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, 10, &state);
+	if (ret)
+		goto unlock;
+
+	led_cdev->saturation = state;
+
+	ret = size;
+unlock:
+	mutex_unlock(&led_cdev->led_access);
+	return ret;
+}
+static DEVICE_ATTR_RW(saturation);
+
+static ssize_t hue_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+
+	/* no lock needed for this */
+	led_update_brightness(led_cdev);
+
+	return sprintf(buf, "%u\n", led_cdev->hue);
+}
+
+static ssize_t hue_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, 10, &state);
+	if (ret)
+		goto unlock;
+
+	led_cdev->hue = state;
+
+	ret = size;
+unlock:
+	mutex_unlock(&led_cdev->led_access);
+	return ret;
+}
+static DEVICE_ATTR_RW(hue);
+
+
 static ssize_t max_brightness_show(struct device *dev,
 		struct device_attribute *attr, char *buf)
 {
@@ -88,6 +165,8 @@ static const struct attribute_group led_trigger_group = {
 static struct attribute *led_class_attrs[] = {
 	&dev_attr_brightness.attr,
 	&dev_attr_max_brightness.attr,
+	&dev_attr_hue.attr,
+	&dev_attr_saturation.attr,
 	NULL,
 };
 
diff --git a/drivers/leds/led-rgb-core.c b/drivers/leds/led-rgb-core.c
new file mode 100644
index 0000000..3733032
--- /dev/null
+++ b/drivers/leds/led-rgb-core.c
@@ -0,0 +1,64 @@
+/*
+ * LED RGB Class Support
+ *
+ * Author: Heiner Kallweit <hkallweit1@gmail.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ */
+
+#include <linux/leds.h>
+#include "leds.h"
+
+struct led_rgb led_hsv_to_rgb(struct led_hsv hsv)
+{
+	unsigned int h = hsv.hue >> 24;
+	unsigned int s = hsv.saturation >> 24;
+	unsigned int v = hsv.value >> 24;
+	unsigned int f, p, q, t, r, g, b;
+	struct led_rgb res;
+
+	if (!v) {
+		res.red = 0;
+		res.green = 0;
+		res.blue = 0;
+		return res;
+	}
+	if (!s) {
+		res.red = v << 24;
+		res.green = v << 24;
+		res.blue = v << 24;
+		return res;
+	}
+
+	f = DIV_ROUND_CLOSEST((h % 42) * 255, 42);
+	p = v - DIV_ROUND_CLOSEST(s * v, 255);
+	q = v - DIV_ROUND_CLOSEST(f * s * v, 255 * 255);
+	t = v - DIV_ROUND_CLOSEST((255 - f) * s * v, 255 * 255);
+
+	switch (h / 42) {
+	case 0:
+		r = v; g = t; b = p; break;
+	case 1:
+		r = q; g = v; b = p; break;
+	case 2:
+		r = p; g = v; b = t; break;
+	case 3:
+		r = p; g = q; b = v; break;
+	case 4:
+		r = t; g = p; b = v; break;
+	case 5:
+		r = v; g = p; b = q; break;
+	}
+
+	printk("hsv_to: h %5d, s %5d, v %5d -> %5d, %5d, %5d\n",
+	       h*390, s*390, v*390, r*390, g*390, b*390);
+	
+	res.red = r << 24;
+	res.green = g << 24;
+	res.blue = b << 24;
+	return res;
+}
+EXPORT_SYMBOL_GPL(led_hsv_to_rgb);
diff --git a/drivers/leds/leds-lp5523.c b/drivers/leds/leds-lp5523.c
index 924e50a..3244c24 100644
--- a/drivers/leds/leds-lp5523.c
+++ b/drivers/leds/leds-lp5523.c
@@ -802,6 +802,26 @@ static ssize_t store_master_fader_leds(struct device *dev,
 	return ret;
 }
 
+int lp5523_rgb_brightness(struct lp55xx_led *led, struct led_rgb r)
+{
+	struct lp55xx_chip *chip = led->chip;
+	int ret;
+
+	mutex_lock(&chip->lock);
+	r.red >>= 24;
+	r.green >>= 24;
+	r.blue >>= 24;	
+	printk("RGB brightness %d %d %d\n", r.red, r.green, r.blue);
+	ret = lp55xx_write(chip, LP5523_REG_LED_PWM_BASE + led->chan_nr, r.red);
+	if (!ret)
+		lp55xx_write(chip, LP5523_REG_LED_PWM_BASE + led->chan_nr - 1, r.green);
+	if (!ret)
+		lp55xx_write(chip, LP5523_REG_LED_PWM_BASE + led->chan_nr - 2, r.blue);
+	mutex_unlock(&chip->lock);
+	return ret;
+
+}
+
 static int lp5523_led_brightness(struct lp55xx_led *led)
 {
 	struct lp55xx_chip *chip = led->chip;
diff --git a/drivers/leds/leds-lp55xx-common.c b/drivers/leds/leds-lp55xx-common.c
index 5377f22..16e65dc 100644
--- a/drivers/leds/leds-lp55xx-common.c
+++ b/drivers/leds/leds-lp55xx-common.c
@@ -134,6 +134,8 @@ static struct attribute *lp55xx_led_attrs[] = {
 };
 ATTRIBUTE_GROUPS(lp55xx_led);
 
+extern int lp5523_rgb_brightness(struct lp55xx_led *led, struct led_rgb r);
+
 static int lp55xx_set_brightness(struct led_classdev *cdev,
 			     enum led_brightness brightness)
 {
@@ -141,6 +143,18 @@ static int lp55xx_set_brightness(struct led_classdev *cdev,
 	struct lp55xx_device_config *cfg = led->chip->cfg;
 
 	led->brightness = (u8)brightness;
+
+	if (led->chan_nr == 6) {
+		struct led_rgb r;
+		struct led_hsv hsv;
+
+		printk("RGB set request: %d %d %d\n", cdev->brightness, cdev->hue >> 24, cdev->saturation >> 24);
+		hsv.value = brightness << 24;
+		hsv.hue = cdev->hue;
+		hsv.saturation = cdev->saturation;
+		r = led_hsv_to_rgb(hsv);
+		return lp5523_rgb_brightness(led, r);
+	}
 	return cfg->brightness_fn(led);
 }
 
@@ -176,6 +190,8 @@ static int lp55xx_init_led(struct lp55xx_led *led,
 	led->cdev.brightness_set_blocking = lp55xx_set_brightness;
 	led->cdev.groups = lp55xx_led_groups;
 
+	printk("Led %d name %s\n", chan, pdata->led_config[chan].name);
+
 	if (pdata->led_config[chan].name) {
 		led->cdev.name = pdata->led_config[chan].name;
 	} else {
diff --git a/include/linux/leds.h b/include/linux/leds.h
index 64c56d4..bd0ae46 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -37,6 +37,8 @@ struct led_classdev {
 	const char		*name;
 	enum led_brightness	 brightness;
 	enum led_brightness	 max_brightness;
+	int			 hue;
+	int			 saturation;
 	int			 flags;
 
 	/* Lower 16 bits reflect status */
@@ -49,6 +51,7 @@ struct led_classdev {
 #define LED_HW_PLUGGABLE	(1 << 19)
 #define LED_PANIC_INDICATOR	(1 << 20)
 #define LED_BRIGHT_HW_CHANGED	(1 << 21)
+#define LED_DEV_CAP_RGB		(1 << 24)
 
 	/* set_brightness_work / blink_timer flags, atomic, private. */
 	unsigned long		work_flags;
@@ -238,6 +241,26 @@ static inline bool led_sysfs_is_disabled(struct led_classdev *led_cdev)
 	return led_cdev->flags & LED_SYSFS_DISABLE;
 }
 
+struct led_hsv {
+	u32 hue;
+	u32 saturation;
+	u32 value;
+};
+
+struct led_rgb {
+	u32 red;
+	u32 green;
+	u32 blue;
+};
+
+/**
+ * led_hsv_to_rgb - convert a hsv color value to rgb color model
+ * @hsv: the hsv value to convert
+ *
+ * Returns: the resulting rgb value
+ */
+struct led_rgb led_hsv_to_rgb(struct led_hsv hsv);
+
 /*
  * LED Triggers
  */


-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: RGB support prototype

[Jacek Anaszewski]

Hi Pavel,

Thanks for your work. Few comments below.

On 08/31/2017 11:33 AM, Pavel Machek wrote:
> Hi!
> 
>>>> We could even export (read-only) hue/saturation for single-color LEDs...
>>>
>>> Related patches from Heiner Kallweit are still sitting on devel branch
>>> of linux-leds.git:
>>>
>>> https://git.kernel.org/pub/scm/linux/kernel/git/j.anaszewski/linux-leds.git/log/?h=devel
>>>
>>> Possibly it can serve as a basis for further development.
>>
>>> I liked that approach because it was compatible with monochrome
>>> LEDs and triggers.
>>
>> Yeah, I like that. I don't like the implementation and the sysfs
>> interface.
>>
>> Let me try and see if I can turn them into something better...
>>
>> I'm using full 32-bits for parameters; that should give us enough
>> precision. In particular, 32-bits is important for "brightness"; I
>> already have light that goes from .1lm to 1000lm -- which is more than
>> 8-bits can handle.
>>
>> lp5523 conversion... is a very quick hack.
> 
> And here's a better version. I'll still need to adjust white to be
> white... but I guess that would need to be done with any solution.
> 
> I've checked, and triggers seem to work as expected (and I can select
> a color for them).
> 
> Signed-off-by: Pavel Machek <pavel@ucw.cz>
> 
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 594b24d..bad8a6e 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -19,6 +19,14 @@ config LEDS_CLASS
>  	  This option enables the led sysfs class in /sys/class/leds.  You'll
>  	  need this to do anything useful with LEDs.  If unsure, say N.
>  
> +config LEDS_CLASS_RGB
> +	bool "LED RGB Class Support"
> +	depends on LEDS_CLASS
> +	help
> +	  This option enables support for RGB LED devices.
> +	  Sysfs attribute brightness is interpreted as a HSV color value.
> +	  For details see Documentation/leds/leds-class.txt.
> +
>  config LEDS_CLASS_FLASH
>  	tristate "LED Flash Class Support"
>  	depends on LEDS_CLASS
> @@ -38,6 +46,10 @@ config LEDS_BRIGHTNESS_HW_CHANGED
>  
>  	  See Documentation/ABI/testing/sysfs-class-led for details.
>  
> +if LEDS_CLASS_RGB
> +comment "RGB LED drivers"
> +endif # LEDS_CLASS_RGB
> +
>  comment "LED drivers"
>  
>  config LEDS_88PM860X
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index 909dae6..d5b5e76 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -1,6 +1,8 @@
>  
>  # LED Core
> -obj-$(CONFIG_NEW_LEDS)			+= led-core.o
> +obj-$(CONFIG_NEW_LEDS)			+= led-core-objs.o
> +led-core-objs-y				:= led-core.o
> +led-core-objs-$(CONFIG_LEDS_CLASS_RGB)	+= led-rgb-core.o
>  obj-$(CONFIG_LEDS_CLASS)		+= led-class.o
>  obj-$(CONFIG_LEDS_CLASS_FLASH)		+= led-class-flash.o
>  obj-$(CONFIG_LEDS_TRIGGERS)		+= led-triggers.o
> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> index b0e2d55..0bd68a4 100644
> --- a/drivers/leds/led-class.c
> +++ b/drivers/leds/led-class.c
> @@ -65,6 +65,83 @@ static ssize_t brightness_store(struct device *dev,
>  }
>  static DEVICE_ATTR_RW(brightness);
>  
> +static ssize_t saturation_show(struct device *dev,
> +		struct device_attribute *attr, char *buf)
> +{
> +	struct led_classdev *led_cdev = dev_get_drvdata(dev);
> +
> +	/* no lock needed for this */
> +	led_update_brightness(led_cdev);

We will need to update documentation of brightness_get to
require updating hue and saturation for RGB LED drivers.

Also hue and saturation attributes will need related ABI
documentation entries. From what I see you're proposing
that hue and sat would be written to the device on the
occasion of brightness setting, right? We're tossing this
responsibility to to drivers, without exposing an API for
it, so it would have to be highlighted.

> +
> +	return sprintf(buf, "%u\n", led_cdev->saturation);
> +}
> +
> +static ssize_t saturation_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, 10, &state);
> +	if (ret)
> +		goto unlock;
> +
> +	led_cdev->saturation = state;
> +
> +	ret = size;
> +unlock:
> +	mutex_unlock(&led_cdev->led_access);
> +	return ret;
> +}
> +static DEVICE_ATTR_RW(saturation);
> +
> +static ssize_t hue_show(struct device *dev,
> +		struct device_attribute *attr, char *buf)
> +{
> +	struct led_classdev *led_cdev = dev_get_drvdata(dev);
> +
> +	/* no lock needed for this */
> +	led_update_brightness(led_cdev);
> +
> +	return sprintf(buf, "%u\n", led_cdev->hue);
> +}
> +
> +static ssize_t hue_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, 10, &state);
> +	if (ret)
> +		goto unlock;
> +
> +	led_cdev->hue = state;
> +
> +	ret = size;
> +unlock:
> +	mutex_unlock(&led_cdev->led_access);
> +	return ret;
> +}
> +static DEVICE_ATTR_RW(hue);
> +
> +
>  static ssize_t max_brightness_show(struct device *dev,
>  		struct device_attribute *attr, char *buf)
>  {
> @@ -88,6 +165,8 @@ static const struct attribute_group led_trigger_group = {
>  static struct attribute *led_class_attrs[] = {
>  	&dev_attr_brightness.attr,
>  	&dev_attr_max_brightness.attr,
> +	&dev_attr_hue.attr,
> +	&dev_attr_saturation.attr,
>  	NULL,
>  };
>  
> diff --git a/drivers/leds/led-rgb-core.c b/drivers/leds/led-rgb-core.c
> new file mode 100644
> index 0000000..3733032
> --- /dev/null
> +++ b/drivers/leds/led-rgb-core.c
> @@ -0,0 +1,64 @@
> +/*
> + * LED RGB Class Support
> + *
> + * Author: Heiner Kallweit <hkallweit1@gmail.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + */
> +
> +#include <linux/leds.h>
> +#include "leds.h"
> +
> +struct led_rgb led_hsv_to_rgb(struct led_hsv hsv)
> +{
> +	unsigned int h = hsv.hue >> 24;
> +	unsigned int s = hsv.saturation >> 24;
> +	unsigned int v = hsv.value >> 24;
> +	unsigned int f, p, q, t, r, g, b;
> +	struct led_rgb res;
> +
> +	if (!v) {
> +		res.red = 0;
> +		res.green = 0;
> +		res.blue = 0;
> +		return res;
> +	}
> +	if (!s) {
> +		res.red = v << 24;
> +		res.green = v << 24;
> +		res.blue = v << 24;

Why the shifting?

> +		return res;
> +	}
> +
> +	f = DIV_ROUND_CLOSEST((h % 42) * 255, 42);
> +	p = v - DIV_ROUND_CLOSEST(s * v, 255);
> +	q = v - DIV_ROUND_CLOSEST(f * s * v, 255 * 255);
> +	t = v - DIV_ROUND_CLOSEST((255 - f) * s * v, 255 * 255);
> +
> +	switch (h / 42) {
> +	case 0:
> +		r = v; g = t; b = p; break;
> +	case 1:
> +		r = q; g = v; b = p; break;
> +	case 2:
> +		r = p; g = v; b = t; break;
> +	case 3:
> +		r = p; g = q; b = v; break;
> +	case 4:
> +		r = t; g = p; b = v; break;
> +	case 5:
> +		r = v; g = p; b = q; break;
> +	}
> +
> +	printk("hsv_to: h %5d, s %5d, v %5d -> %5d, %5d, %5d\n",
> +	       h*390, s*390, v*390, r*390, g*390, b*390);
> +	
> +	res.red = r << 24;
> +	res.green = g << 24;
> +	res.blue = b << 24;
> +	return res;
> +}
> +EXPORT_SYMBOL_GPL(led_hsv_to_rgb);
> diff --git a/drivers/leds/leds-lp5523.c b/drivers/leds/leds-lp5523.c
> index 924e50a..3244c24 100644
> --- a/drivers/leds/leds-lp5523.c
> +++ b/drivers/leds/leds-lp5523.c
> @@ -802,6 +802,26 @@ static ssize_t store_master_fader_leds(struct device *dev,
>  	return ret;
>  }
>  
> +int lp5523_rgb_brightness(struct lp55xx_led *led, struct led_rgb r)
> +{
> +	struct lp55xx_chip *chip = led->chip;
> +	int ret;
> +
> +	mutex_lock(&chip->lock);
> +	r.red >>= 24;
> +	r.green >>= 24;
> +	r.blue >>= 24;	
> +	printk("RGB brightness %d %d %d\n", r.red, r.green, r.blue);
> +	ret = lp55xx_write(chip, LP5523_REG_LED_PWM_BASE + led->chan_nr, r.red);
> +	if (!ret)
> +		lp55xx_write(chip, LP5523_REG_LED_PWM_BASE + led->chan_nr - 1, r.green);
> +	if (!ret)
> +		lp55xx_write(chip, LP5523_REG_LED_PWM_BASE + led->chan_nr - 2, r.blue);
> +	mutex_unlock(&chip->lock);
> +	return ret;
> +
> +}
> +
>  static int lp5523_led_brightness(struct lp55xx_led *led)
>  {
>  	struct lp55xx_chip *chip = led->chip;
> diff --git a/drivers/leds/leds-lp55xx-common.c b/drivers/leds/leds-lp55xx-common.c
> index 5377f22..16e65dc 100644
> --- a/drivers/leds/leds-lp55xx-common.c
> +++ b/drivers/leds/leds-lp55xx-common.c
> @@ -134,6 +134,8 @@ static struct attribute *lp55xx_led_attrs[] = {
>  };
>  ATTRIBUTE_GROUPS(lp55xx_led);
>  
> +extern int lp5523_rgb_brightness(struct lp55xx_led *led, struct led_rgb r);
> +
>  static int lp55xx_set_brightness(struct led_classdev *cdev,
>  			     enum led_brightness brightness)
>  {
> @@ -141,6 +143,18 @@ static int lp55xx_set_brightness(struct led_classdev *cdev,
>  	struct lp55xx_device_config *cfg = led->chip->cfg;
>  
>  	led->brightness = (u8)brightness;
> +
> +	if (led->chan_nr == 6) {
> +		struct led_rgb r;
> +		struct led_hsv hsv;
> +
> +		printk("RGB set request: %d %d %d\n", cdev->brightness, cdev->hue >> 24, cdev->saturation >> 24);
> +		hsv.value = brightness << 24;
> +		hsv.hue = cdev->hue;
> +		hsv.saturation = cdev->saturation;
> +		r = led_hsv_to_rgb(hsv);
> +		return lp5523_rgb_brightness(led, r);
> +	}
>  	return cfg->brightness_fn(led);
>  }
>  
> @@ -176,6 +190,8 @@ static int lp55xx_init_led(struct lp55xx_led *led,
>  	led->cdev.brightness_set_blocking = lp55xx_set_brightness;
>  	led->cdev.groups = lp55xx_led_groups;
>  
> +	printk("Led %d name %s\n", chan, pdata->led_config[chan].name);
> +
>  	if (pdata->led_config[chan].name) {
>  		led->cdev.name = pdata->led_config[chan].name;
>  	} else {
> diff --git a/include/linux/leds.h b/include/linux/leds.h
> index 64c56d4..bd0ae46 100644
> --- a/include/linux/leds.h
> +++ b/include/linux/leds.h
> @@ -37,6 +37,8 @@ struct led_classdev {
>  	const char		*name;
>  	enum led_brightness	 brightness;
>  	enum led_brightness	 max_brightness;
> +	int			 hue;
> +	int			 saturation;
>  	int			 flags;
>  
>  	/* Lower 16 bits reflect status */
> @@ -49,6 +51,7 @@ struct led_classdev {
>  #define LED_HW_PLUGGABLE	(1 << 19)
>  #define LED_PANIC_INDICATOR	(1 << 20)
>  #define LED_BRIGHT_HW_CHANGED	(1 << 21)
> +#define LED_DEV_CAP_RGB		(1 << 24)
>  
>  	/* set_brightness_work / blink_timer flags, atomic, private. */
>  	unsigned long		work_flags;
> @@ -238,6 +241,26 @@ static inline bool led_sysfs_is_disabled(struct led_classdev *led_cdev)
>  	return led_cdev->flags & LED_SYSFS_DISABLE;
>  }
>  
> +struct led_hsv {
> +	u32 hue;
> +	u32 saturation;
> +	u32 value;
> +};
> +
> +struct led_rgb {
> +	u32 red;
> +	u32 green;
> +	u32 blue;
> +};
> +
> +/**
> + * led_hsv_to_rgb - convert a hsv color value to rgb color model
> + * @hsv: the hsv value to convert
> + *
> + * Returns: the resulting rgb value
> + */
> +struct led_rgb led_hsv_to_rgb(struct led_hsv hsv);
> +
>  /*
>   * LED Triggers
>   */
> 
> 

-- 
Best regards,
Jacek Anaszewski