[PATCH v5 1/4] leds: core: add generic support for RGB Color LED's

[PATCH v5 1/4] leds: core: add generic support for RGB Color LED's

[Heiner Kallweit]

Add generic support for RGB Color LED's.

Basic idea is to use enum led_brightness also for the hue and saturation
color components.This allows to implement the color extension w/o
changes to struct led_classdev.

Select LEDS_RGB to enable building drivers using the RGB extension.

Flag LED_SET_HUE_SAT allows to specify that hue / saturation
should be overridden even if the provided values are zero.

Some examples for writing values to /sys/class/leds/<xx>/brightness:
(now also hex notation can be used)

255 -> set full brightness and keep existing color if set
0 -> switch LED off but keep existing color so that it can be restored
     if the LED is switched on again later
0x1000000 -> switch LED off and set also hue and saturation to 0
0x00ffff -> set full brightness, full saturation and set hue to 0 (red)

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
v2:
- move extension-specific code into a separate source file and
  introduce config symbol LEDS_HSV for it
- create separate patch for the extension to sysfs
- use variable name led_cdev as in the rest if the core
- rename to_hsv to led_validate_brightness
- rename LED_BRIGHTNESS_SET_COLOR to LED_SET_HSV
- introduce helper is_off for checking whether V part
  of a HSV value is zero
v3:
- change Kconfig to use depend instead of select, add help message,
  and change config symbol to LEDS_COLOR
- change LED core object file name in Makefile
- rename flag LED_SET_HSV to LED_SET_COLOR
- rename is_off to led_is_off
- rename led_validate-brightness to led_confine_brightness
- rename variable in led_confine_brightness
- add dummy enum led_brightness value to enforce 32bit enum
- rename led-hsv-core.c to led-color-core.c
- move check of provided brightness value to led_confine_brightness
v4:
- change config symbol name to LEDS_RGB
- change name of new file to led-rgb-core.c
- factor out part of led_confine_brightness
- change led_is_off to __is_set_brightness
- in led_set_software_blink pass led_cdev->max_brightness instead of LED_FULL
- rename LED_SET_COLOR to LED_SET_HUE_SAT
v5:
- change "RGB Color LED" to "RGB LED" in Kconfig
- move definition of LED_HUE_SAT_MASK to drivers/leds/leds.h
- rename LED_DEV_CAP_HSV to LED_DEV_CAP_RGB
---
 drivers/leds/Kconfig        | 11 +++++++++++
 drivers/leds/Makefile       |  4 +++-
 drivers/leds/led-class.c    |  2 +-
 drivers/leds/led-core.c     | 16 +++++++++-------
 drivers/leds/led-rgb-core.c | 40 ++++++++++++++++++++++++++++++++++++++++
 drivers/leds/leds.h         | 18 ++++++++++++++++++
 include/linux/leds.h        |  9 +++++++++
 7 files changed, 91 insertions(+), 9 deletions(-)
 create mode 100644 drivers/leds/led-rgb-core.c

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 7f940c2..5b1c852 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -13,6 +13,13 @@ menuconfig NEW_LEDS
 
 if NEW_LEDS
 
+config LEDS_RGB
+	bool "RGB LED Support"
+	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
 	tristate "LED Class Support"
 	help
@@ -29,6 +36,10 @@ config LEDS_CLASS_FLASH
 	  for the flash related features of a LED device. It can be built
 	  as a module.
 
+if LEDS_RGB
+comment "RGB LED drivers"
+endif # LEDS_RGB
+
 comment "LED drivers"
 
 config LEDS_88PM860X
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index e9d5309..cc3676f 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_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 aa84e5b..007a5b3 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -53,7 +53,7 @@ static ssize_t brightness_store(struct device *dev,
 	if (ret)
 		goto unlock;
 
-	if (state == LED_OFF)
+	if (!__is_brightness_set(state))
 		led_trigger_remove(led_cdev);
 	led_set_brightness(led_cdev, state);
 
diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
index 3495d5d..e75b0c8 100644
--- a/drivers/leds/led-core.c
+++ b/drivers/leds/led-core.c
@@ -62,7 +62,7 @@ static void led_timer_function(unsigned long data)
 	}
 
 	brightness = led_get_brightness(led_cdev);
-	if (!brightness) {
+	if (!__is_brightness_set(brightness)) {
 		/* Time to switch the LED on. */
 		brightness = led_cdev->blink_brightness;
 		delay = led_cdev->blink_delay_on;
@@ -133,8 +133,9 @@ static void led_set_software_blink(struct led_classdev *led_cdev,
 	if (current_brightness)
 		led_cdev->blink_brightness = current_brightness;
 	if (!led_cdev->blink_brightness)
-		led_cdev->blink_brightness = led_cdev->max_brightness;
-
+		led_cdev->blink_brightness =
+				led_confine_brightness(led_cdev,
+						led_cdev->max_brightness);
 	led_cdev->blink_delay_on = delay_on;
 	led_cdev->blink_delay_off = delay_off;
 
@@ -235,12 +236,13 @@ void led_set_brightness(struct led_classdev *led_cdev,
 		 * work queue task to avoid problems in case we are called
 		 * from hard irq context.
 		 */
-		if (brightness == LED_OFF) {
+		if (!__is_brightness_set(brightness)) {
 			led_cdev->flags |= LED_BLINK_DISABLE;
 			schedule_work(&led_cdev->set_brightness_work);
 		} else {
 			led_cdev->flags |= LED_BLINK_BRIGHTNESS_CHANGE;
-			led_cdev->blink_brightness = brightness;
+			led_cdev->blink_brightness =
+				led_confine_brightness(led_cdev, brightness);
 		}
 		return;
 	}
@@ -265,7 +267,7 @@ EXPORT_SYMBOL_GPL(led_set_brightness_nopm);
 void led_set_brightness_nosleep(struct led_classdev *led_cdev,
 				enum led_brightness value)
 {
-	led_cdev->brightness = min(value, led_cdev->max_brightness);
+	led_cdev->brightness = led_confine_brightness(led_cdev, value);
 
 	if (led_cdev->flags & LED_SUSPENDED)
 		return;
@@ -280,7 +282,7 @@ int led_set_brightness_sync(struct led_classdev *led_cdev,
 	if (led_cdev->blink_delay_on || led_cdev->blink_delay_off)
 		return -EBUSY;
 
-	led_cdev->brightness = min(value, led_cdev->max_brightness);
+	led_cdev->brightness = led_confine_brightness(led_cdev, value);
 
 	if (led_cdev->flags & LED_SUSPENDED)
 		return 0;
diff --git a/drivers/leds/led-rgb-core.c b/drivers/leds/led-rgb-core.c
new file mode 100644
index 0000000..f6591f1
--- /dev/null
+++ b/drivers/leds/led-rgb-core.c
@@ -0,0 +1,40 @@
+/*
+ * LED Class Color 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/kernel.h>
+#include <linux/leds.h>
+#include "leds.h"
+
+/*
+ * The color extension handles RGB LEDs but uses a HSV color model internally.
+ * led_rgb_adjust_hue_sat sets hue and saturation part of the HSV color value.
+ */
+static enum led_brightness led_rgb_adjust_hue_sat(struct led_classdev *led_cdev,
+						  enum led_brightness value)
+{
+	/* LED_SET_HUE_SAT sets hue and saturation even if both are zero */
+	if (value & LED_SET_HUE_SAT || value > LED_FULL)
+		return value & LED_HUE_SAT_MASK;
+	else
+		return led_cdev->brightness & ~LED_BRIGHTNESS_MASK;
+}
+
+enum led_brightness led_confine_brightness(struct led_classdev *led_cdev,
+					   enum led_brightness value)
+{
+	enum led_brightness brightness = 0;
+
+	if (led_cdev->flags & LED_DEV_CAP_RGB)
+		brightness = led_rgb_adjust_hue_sat(led_cdev, value);
+
+	return brightness |
+	       min(value & LED_BRIGHTNESS_MASK, led_cdev->max_brightness);
+}
diff --git a/drivers/leds/leds.h b/drivers/leds/leds.h
index db3f20d..b853f54 100644
--- a/drivers/leds/leds.h
+++ b/drivers/leds/leds.h
@@ -16,17 +16,35 @@
 #include <linux/rwsem.h>
 #include <linux/leds.h>
 
+#define LED_BRIGHTNESS_MASK	0x000000ff
+#define LED_HUE_SAT_MASK	0x00ffff00
+
 static inline int led_get_brightness(struct led_classdev *led_cdev)
 {
 	return led_cdev->brightness;
 }
 
+static inline bool __is_brightness_set(enum led_brightness brightness)
+{
+	return (brightness & LED_BRIGHTNESS_MASK) != LED_OFF;
+}
+
 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);
+#if IS_ENABLED(CONFIG_LEDS_RGB)
+enum led_brightness led_confine_brightness(struct led_classdev *led_cdev,
+					   enum led_brightness value);
+#else
+static inline enum led_brightness led_confine_brightness(
+		struct led_classdev *led_cdev, enum led_brightness value)
+{
+	return min(value, led_cdev->max_brightness);
+}
+#endif
 
 extern struct rw_semaphore leds_list_lock;
 extern struct list_head leds_list;
diff --git a/include/linux/leds.h b/include/linux/leds.h
index f203a8f..bbf24bb 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -29,8 +29,16 @@ enum led_brightness {
 	LED_OFF		= 0,
 	LED_HALF	= 127,
 	LED_FULL	= 255,
+	/*
+	 * dummy enum value to make gcc use a 32 bit type for the enum
+	 * even if compiled with -fshort-enums. This is needed for
+	 * the enum to store hsv values.
+	 */
+	LED_LEVEL_DUMMY	= 0xffffffff,
 };
 
+#define LED_SET_HUE_SAT		BIT(24)
+
 struct led_classdev {
 	const char		*name;
 	enum led_brightness	 brightness;
@@ -50,6 +58,7 @@ struct led_classdev {
 #define LED_SYSFS_DISABLE	(1 << 22)
 #define LED_DEV_CAP_FLASH	(1 << 23)
 #define LED_HW_PLUGGABLE	(1 << 24)
+#define LED_DEV_CAP_RGB		(1 << 25)
 
 	/* Set LED brightness level
 	 * Must not sleep. Use brightness_set_blocking for drivers
-- 
2.7.1

Re: [PATCH v5 1/4] leds: core: add generic support for RGB Color LED's

[Jacek Anaszewski]

Hi Heiner,

Thanks for the updated version. Some nitpicking below.

Please remove "Color" from the patch title.

On 03/01/2016 10:26 PM, Heiner Kallweit wrote:
> Add generic support for RGB Color LED's.
                                ^^^
Ditto.

>
> Basic idea is to use enum led_brightness also for the hue and saturation
> color components.This allows to implement the color extension w/o
> changes to struct led_classdev.
>
> Select LEDS_RGB to enable building drivers using the RGB extension.
>
> Flag LED_SET_HUE_SAT allows to specify that hue / saturation
> should be overridden even if the provided values are zero.
>
> Some examples for writing values to /sys/class/leds/<xx>/brightness:
> (now also hex notation can be used)
>
> 255 -> set full brightness and keep existing color if set
> 0 -> switch LED off but keep existing color so that it can be restored
>       if the LED is switched on again later
> 0x1000000 -> switch LED off and set also hue and saturation to 0
> 0x00ffff -> set full brightness, full saturation and set hue to 0 (red)
>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
> v2:
> - move extension-specific code into a separate source file and
>    introduce config symbol LEDS_HSV for it
> - create separate patch for the extension to sysfs
> - use variable name led_cdev as in the rest if the core
> - rename to_hsv to led_validate_brightness
> - rename LED_BRIGHTNESS_SET_COLOR to LED_SET_HSV
> - introduce helper is_off for checking whether V part
>    of a HSV value is zero
> v3:
> - change Kconfig to use depend instead of select, add help message,
>    and change config symbol to LEDS_COLOR
> - change LED core object file name in Makefile
> - rename flag LED_SET_HSV to LED_SET_COLOR
> - rename is_off to led_is_off
> - rename led_validate-brightness to led_confine_brightness
> - rename variable in led_confine_brightness
> - add dummy enum led_brightness value to enforce 32bit enum
> - rename led-hsv-core.c to led-color-core.c
> - move check of provided brightness value to led_confine_brightness
> v4:
> - change config symbol name to LEDS_RGB
> - change name of new file to led-rgb-core.c
> - factor out part of led_confine_brightness
> - change led_is_off to __is_set_brightness
> - in led_set_software_blink pass led_cdev->max_brightness instead of LED_FULL
> - rename LED_SET_COLOR to LED_SET_HUE_SAT
> v5:
> - change "RGB Color LED" to "RGB LED" in Kconfig
> - move definition of LED_HUE_SAT_MASK to drivers/leds/leds.h
> - rename LED_DEV_CAP_HSV to LED_DEV_CAP_RGB
> ---
>   drivers/leds/Kconfig        | 11 +++++++++++
>   drivers/leds/Makefile       |  4 +++-
>   drivers/leds/led-class.c    |  2 +-
>   drivers/leds/led-core.c     | 16 +++++++++-------
>   drivers/leds/led-rgb-core.c | 40 ++++++++++++++++++++++++++++++++++++++++
>   drivers/leds/leds.h         | 18 ++++++++++++++++++
>   include/linux/leds.h        |  9 +++++++++
>   7 files changed, 91 insertions(+), 9 deletions(-)
>   create mode 100644 drivers/leds/led-rgb-core.c
>
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 7f940c2..5b1c852 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -13,6 +13,13 @@ menuconfig NEW_LEDS
>
>   if NEW_LEDS
>
> +config LEDS_RGB
> +	bool "RGB LED Support"
> +	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
>   	tristate "LED Class Support"
>   	help
> @@ -29,6 +36,10 @@ config LEDS_CLASS_FLASH
>   	  for the flash related features of a LED device. It can be built
>   	  as a module.
>
> +if LEDS_RGB
> +comment "RGB LED drivers"
> +endif # LEDS_RGB
> +
>   comment "LED drivers"
>
>   config LEDS_88PM860X
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index e9d5309..cc3676f 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_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 aa84e5b..007a5b3 100644
> --- a/drivers/leds/led-class.c
> +++ b/drivers/leds/led-class.c
> @@ -53,7 +53,7 @@ static ssize_t brightness_store(struct device *dev,
>   	if (ret)
>   		goto unlock;
>
> -	if (state == LED_OFF)
> +	if (!__is_brightness_set(state))
>   		led_trigger_remove(led_cdev);
>   	led_set_brightness(led_cdev, state);
>
> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
> index 3495d5d..e75b0c8 100644
> --- a/drivers/leds/led-core.c
> +++ b/drivers/leds/led-core.c
> @@ -62,7 +62,7 @@ static void led_timer_function(unsigned long data)
>   	}
>
>   	brightness = led_get_brightness(led_cdev);
> -	if (!brightness) {
> +	if (!__is_brightness_set(brightness)) {
>   		/* Time to switch the LED on. */
>   		brightness = led_cdev->blink_brightness;
>   		delay = led_cdev->blink_delay_on;
> @@ -133,8 +133,9 @@ static void led_set_software_blink(struct led_classdev *led_cdev,
>   	if (current_brightness)
>   		led_cdev->blink_brightness = current_brightness;
>   	if (!led_cdev->blink_brightness)
> -		led_cdev->blink_brightness = led_cdev->max_brightness;
> -
> +		led_cdev->blink_brightness =
> +				led_confine_brightness(led_cdev,
> +						led_cdev->max_brightness);
>   	led_cdev->blink_delay_on = delay_on;
>   	led_cdev->blink_delay_off = delay_off;
>
> @@ -235,12 +236,13 @@ void led_set_brightness(struct led_classdev *led_cdev,
>   		 * work queue task to avoid problems in case we are called
>   		 * from hard irq context.
>   		 */
> -		if (brightness == LED_OFF) {
> +		if (!__is_brightness_set(brightness)) {
>   			led_cdev->flags |= LED_BLINK_DISABLE;
>   			schedule_work(&led_cdev->set_brightness_work);
>   		} else {
>   			led_cdev->flags |= LED_BLINK_BRIGHTNESS_CHANGE;
> -			led_cdev->blink_brightness = brightness;
> +			led_cdev->blink_brightness =
> +				led_confine_brightness(led_cdev, brightness);
>   		}
>   		return;
>   	}
> @@ -265,7 +267,7 @@ EXPORT_SYMBOL_GPL(led_set_brightness_nopm);
>   void led_set_brightness_nosleep(struct led_classdev *led_cdev,
>   				enum led_brightness value)
>   {
> -	led_cdev->brightness = min(value, led_cdev->max_brightness);
> +	led_cdev->brightness = led_confine_brightness(led_cdev, value);
>
>   	if (led_cdev->flags & LED_SUSPENDED)
>   		return;
> @@ -280,7 +282,7 @@ int led_set_brightness_sync(struct led_classdev *led_cdev,
>   	if (led_cdev->blink_delay_on || led_cdev->blink_delay_off)
>   		return -EBUSY;
>
> -	led_cdev->brightness = min(value, led_cdev->max_brightness);
> +	led_cdev->brightness = led_confine_brightness(led_cdev, value);
>
>   	if (led_cdev->flags & LED_SUSPENDED)
>   		return 0;
> diff --git a/drivers/leds/led-rgb-core.c b/drivers/leds/led-rgb-core.c
> new file mode 100644
> index 0000000..f6591f1
> --- /dev/null
> +++ b/drivers/leds/led-rgb-core.c
> @@ -0,0 +1,40 @@
> +/*
> + * LED Class Color 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/kernel.h>

The seems to be redundant here.

> +#include <linux/leds.h>
> +#include "leds.h"
> +
> +/*
> + * The color extension handles RGB LEDs but uses a HSV color model internally.
> + * led_rgb_adjust_hue_sat sets hue and saturation part of the HSV color value.
> + */
> +static enum led_brightness led_rgb_adjust_hue_sat(struct led_classdev *led_cdev,
> +						  enum led_brightness value)
> +{
> +	/* LED_SET_HUE_SAT sets hue and saturation even if both are zero */
> +	if (value & LED_SET_HUE_SAT || value > LED_FULL)
> +		return value & LED_HUE_SAT_MASK;
> +	else
> +		return led_cdev->brightness & ~LED_BRIGHTNESS_MASK;
> +}
> +
> +enum led_brightness led_confine_brightness(struct led_classdev *led_cdev,
> +					   enum led_brightness value)
> +{
> +	enum led_brightness brightness = 0;
> +
> +	if (led_cdev->flags & LED_DEV_CAP_RGB)
> +		brightness = led_rgb_adjust_hue_sat(led_cdev, value);
> +
> +	return brightness |
> +	       min(value & LED_BRIGHTNESS_MASK, led_cdev->max_brightness);
> +}
> diff --git a/drivers/leds/leds.h b/drivers/leds/leds.h
> index db3f20d..b853f54 100644
> --- a/drivers/leds/leds.h
> +++ b/drivers/leds/leds.h
> @@ -16,17 +16,35 @@
>   #include <linux/rwsem.h>
>   #include <linux/leds.h>
>
> +#define LED_BRIGHTNESS_MASK	0x000000ff
> +#define LED_HUE_SAT_MASK	0x00ffff00
> +
>   static inline int led_get_brightness(struct led_classdev *led_cdev)
>   {
>   	return led_cdev->brightness;
>   }
>
> +static inline bool __is_brightness_set(enum led_brightness brightness)
> +{
> +	return (brightness & LED_BRIGHTNESS_MASK) != LED_OFF;
> +}
> +
>   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);
> +#if IS_ENABLED(CONFIG_LEDS_RGB)
> +enum led_brightness led_confine_brightness(struct led_classdev *led_cdev,
> +					   enum led_brightness value);
> +#else
> +static inline enum led_brightness led_confine_brightness(
> +		struct led_classdev *led_cdev, enum led_brightness value)
> +{
> +	return min(value, led_cdev->max_brightness);
> +}
> +#endif
>
>   extern struct rw_semaphore leds_list_lock;
>   extern struct list_head leds_list;
> diff --git a/include/linux/leds.h b/include/linux/leds.h
> index f203a8f..bbf24bb 100644
> --- a/include/linux/leds.h
> +++ b/include/linux/leds.h
> @@ -29,8 +29,16 @@ enum led_brightness {
>   	LED_OFF		= 0,
>   	LED_HALF	= 127,
>   	LED_FULL	= 255,
> +	/*
> +	 * dummy enum value to make gcc use a 32 bit type for the enum
> +	 * even if compiled with -fshort-enums. This is needed for
> +	 * the enum to store hsv values.
> +	 */
> +	LED_LEVEL_DUMMY	= 0xffffffff,
>   };
>
> +#define LED_SET_HUE_SAT		BIT(24)
> +
>   struct led_classdev {
>   	const char		*name;
>   	enum led_brightness	 brightness;
> @@ -50,6 +58,7 @@ struct led_classdev {
>   #define LED_SYSFS_DISABLE	(1 << 22)
>   #define LED_DEV_CAP_FLASH	(1 << 23)
>   #define LED_HW_PLUGGABLE	(1 << 24)
> +#define LED_DEV_CAP_RGB		(1 << 25)

Please keep CAP flags next to each other. You can change values
of the existing flags, they're internal to kernel.

>
>   	/* Set LED brightness level
>   	 * Must not sleep. Use brightness_set_blocking for drivers
>


-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v5 1/4] leds: core: add generic support for RGB Color LED's

[Pavel Machek]

Hi!

First, please Cc me on RGB color support.

> Add generic support for RGB Color LED's.
> 
> Basic idea is to use enum led_brightness also for the hue and saturation
> color components.This allows to implement the color extension w/o
> changes to struct led_classdev.
> 
> Select LEDS_RGB to enable building drivers using the RGB extension.
> 
> Flag LED_SET_HUE_SAT allows to specify that hue / saturation
> should be overridden even if the provided values are zero.
> 
> Some examples for writing values to /sys/class/leds/<xx>/brightness:
> (now also hex notation can be used)
> 
> 255 -> set full brightness and keep existing color if set
> 0 -> switch LED off but keep existing color so that it can be restored
>      if the LED is switched on again later
> 0x1000000 -> switch LED off and set also hue and saturation to 0
> 0x00ffff -> set full brightness, full saturation and set hue to 0
> (red)

Umm, that's rather strange interface -- and three values in single sysfs
file is actually forbidden.

Plus, it is very much unlike existing interfaces for RGB LEDs, which
we already have supported in the tree. (At least nokia N900 and Sony
motion controller already contain supported three-color LEDs).

Now... yes, there's work to be done for the 3-color LEDs. Currently,
they are treated as three different LEDs. (Which makes some sense, you
can use "battery charging" trigger for LED, and CPU activity trigger
for green, for example). It would be good to have some kind of
grouping, so that userspace can tell "these 3 leds are actually
combined into one light".

Second, we should define that LED brightness has similar gamma to the
monitor, so that expected colors are displayed when user requests
them.

(And then.. I guess we should talk about more advanced stuff, like
hardware that can drive the LED changes independently of the main
CPU.)

Best regards,
								Pavel

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

Re: [PATCH v5 1/4] leds: core: add generic support for RGB Color LED's

[Heiner Kallweit]

Am 29.03.2016 um 12:02 schrieb Pavel Machek:
> Hi!
> 
> First, please Cc me on RGB color support.
> 
>> Add generic support for RGB Color LED's.
>>
>> Basic idea is to use enum led_brightness also for the hue and saturation
>> color components.This allows to implement the color extension w/o
>> changes to struct led_classdev.
>>
>> Select LEDS_RGB to enable building drivers using the RGB extension.
>>
>> Flag LED_SET_HUE_SAT allows to specify that hue / saturation
>> should be overridden even if the provided values are zero.
>>
>> Some examples for writing values to /sys/class/leds/<xx>/brightness:
>> (now also hex notation can be used)
>>
>> 255 -> set full brightness and keep existing color if set
>> 0 -> switch LED off but keep existing color so that it can be restored
>>      if the LED is switched on again later
>> 0x1000000 -> switch LED off and set also hue and saturation to 0
>> 0x00ffff -> set full brightness, full saturation and set hue to 0
>> (red)
> 
> Umm, that's rather strange interface -- and three values in single sysfs
> file is actually forbidden.
> 
> Plus, it is very much unlike existing interfaces for RGB LEDs, which
> we already have supported in the tree. (At least nokia N900 and Sony
> motion controller already contain supported three-color LEDs).
> 
> Now... yes, there's work to be done for the 3-color LEDs. Currently,
> they are treated as three different LEDs. (Which makes some sense, you
> can use "battery charging" trigger for LED, and CPU activity trigger
> for green, for example). It would be good to have some kind of
> grouping, so that userspace can tell "these 3 leds are actually
> combined into one light".
> 
At first thanks for the review comments.
Treating the three physical LEDs of a RGB LED as separate LED devices
might have been implemented due to the lack of alternatives.
With one trigger controlling the red LED and another controlling the green
LED we may end up with a yellow light. Not sure whether this is what we want.

One driver for this extension was the idea of triggers using color
to visualize states etc.
Therefore it's not only about userspace controlling the color.
As a trigger is bound to a led_classdev we need a led_classdev
representing a RGB LED device.

And ok: If required the sysfs interface can be splitted into separate
attributes for hue, saturation, and (existing) brightness.

Rgds, Heiner

> 
> Second, we should define that LED brightness has similar gamma to the
> monitor, so that expected colors are displayed when user requests
> them.
> 
> (And then.. I guess we should talk about more advanced stuff, like
> hardware that can drive the LED changes independently of the main
> CPU.)
> 
> Best regards,
> 								Pavel
> 

Re: [PATCH v5 1/4] leds: core: add generic support for RGB Color LED's

[Pavel Machek]

Hi!

> > First, please Cc me on RGB color support.
> > 
> >> Add generic support for RGB Color LED's.
> >>
> >> Basic idea is to use enum led_brightness also for the hue and saturation
> >> color components.This allows to implement the color extension w/o
> >> changes to struct led_classdev.
> >>
> >> Select LEDS_RGB to enable building drivers using the RGB extension.
> >>
> >> Flag LED_SET_HUE_SAT allows to specify that hue / saturation
> >> should be overridden even if the provided values are zero.
> >>
> >> Some examples for writing values to /sys/class/leds/<xx>/brightness:
> >> (now also hex notation can be used)
> >>
> >> 255 -> set full brightness and keep existing color if set
> >> 0 -> switch LED off but keep existing color so that it can be restored
> >>      if the LED is switched on again later
> >> 0x1000000 -> switch LED off and set also hue and saturation to 0
> >> 0x00ffff -> set full brightness, full saturation and set hue to 0
> >> (red)
> > 
> > Umm, that's rather strange interface -- and three values in single sysfs
> > file is actually forbidden.
> > 
> > Plus, it is very much unlike existing interfaces for RGB LEDs, which
> > we already have supported in the tree. (At least nokia N900 and Sony
> > motion controller already contain supported three-color LEDs).
> > 
> > Now... yes, there's work to be done for the 3-color LEDs. Currently,
> > they are treated as three different LEDs. (Which makes some sense, you
> > can use "battery charging" trigger for LED, and CPU activity trigger
> > for green, for example). It would be good to have some kind of
> > grouping, so that userspace can tell "these 3 leds are actually
> > combined into one light".
> > 
> At first thanks for the review comments.
> Treating the three physical LEDs of a RGB LED as separate LED devices
> might have been implemented due to the lack of alternatives.

To be fair... they _are_ separate LED devices. In N900 case, you can
even see light comming from slightly different places if you look closely.

> With one trigger controlling the red LED and another controlling the green
> LED we may end up with a yellow light. Not sure whether this is what
> we want.

Well, it should be understandable for most people.

> One driver for this extension was the idea of triggers using color
> to visualize states etc.
> Therefore it's not only about userspace controlling the color.
> As a trigger is bound to a led_classdev we need a led_classdev
> representing a RGB LED device.
> 
> And ok: If required the sysfs interface can be splitted into separate
> attributes for hue, saturation, and (existing) brightness.

Required.

Ok, so:

a) Do we want RGB leds to be handled by existing subsystem, or do we
need separate layer on top of that?

b) Does RGB make sense, or HSV? RGB is quite widely used in graphics,
and it is what hardware implements. (But we'd need to do gamma
correction).

c) My hardware has "acceleration engine", LED is independend from
CPU. That's rather big deal. Does yours? It seems to be quite common,
at least in cellphones.

Ideally, I'd like to have "triggers", but different ones. As in: if
charging, do yellow " .xX" pattern. If fully charged, do green steady
light. If message is waiting, do blue " x x" pattern. If none of
above, do slow white blinking. (Plus priorities of events). But that's
quite different from existing support...)

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

Re: [PATCH v5 1/4] leds: core: add generic support for RGB Color LED's

[Pavel Machek]

Hi!

> > One driver for this extension was the idea of triggers using color
> > to visualize states etc.
> > Therefore it's not only about userspace controlling the color.
> > As a trigger is bound to a led_classdev we need a led_classdev
> > representing a RGB LED device.
> > 
> > And ok: If required the sysfs interface can be splitted into separate
> > attributes for hue, saturation, and (existing) brightness.
> 
> Required.
> 
> Ok, so:
> 
> a) Do we want RGB leds to be handled by existing subsystem, or do we
> need separate layer on top of that?

And subquestion: if using existing subsystem, should the RGB led be
one led, or three?

Kernel currently uses three leds for one RGB led, and even before
that, there were leds such as "charging::yellow", "charging::green"
that were as close as leds in RGB module are.

Best regards,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 1/4] leds: core: add generic support for RGB Color LED's

[Pavel Machek]

Hi!

> > One driver for this extension was the idea of triggers using color
> > to visualize states etc.
> > Therefore it's not only about userspace controlling the color.
> > As a trigger is bound to a led_classdev we need a led_classdev
> > representing a RGB LED device.
> > 
> > And ok: If required the sysfs interface can be splitted into separate
> > attributes for hue, saturation, and (existing) brightness.
> 
> Required.
> 
> Ok, so:
> 
> a) Do we want RGB leds to be handled by existing subsystem, or do we
> need separate layer on top of that?

And subquestion: if using existing subsystem, should the RGB led be
one led, or three?

Kernel currently uses three leds for one RGB led, and even before
that, there were leds such as "charging::yellow", "charging::green"
that were as close as leds in RGB module are.

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

Re: [PATCH v5 1/4] leds: core: add generic support for RGB Color LED's

[Heiner Kallweit]

Am 29.03.2016 um 23:43 schrieb Pavel Machek:
> Hi!
> 
>>> First, please Cc me on RGB color support.
>>>
>>>> Add generic support for RGB Color LED's.
>>>>
>>>> Basic idea is to use enum led_brightness also for the hue and saturation
>>>> color components.This allows to implement the color extension w/o
>>>> changes to struct led_classdev.
>>>>
>>>> Select LEDS_RGB to enable building drivers using the RGB extension.
>>>>
>>>> Flag LED_SET_HUE_SAT allows to specify that hue / saturation
>>>> should be overridden even if the provided values are zero.
>>>>
>>>> Some examples for writing values to /sys/class/leds/<xx>/brightness:
>>>> (now also hex notation can be used)
>>>>
>>>> 255 -> set full brightness and keep existing color if set
>>>> 0 -> switch LED off but keep existing color so that it can be restored
>>>>      if the LED is switched on again later
>>>> 0x1000000 -> switch LED off and set also hue and saturation to 0
>>>> 0x00ffff -> set full brightness, full saturation and set hue to 0
>>>> (red)
>>>
>>> Umm, that's rather strange interface -- and three values in single sysfs
>>> file is actually forbidden.
>>>
>>> Plus, it is very much unlike existing interfaces for RGB LEDs, which
>>> we already have supported in the tree. (At least nokia N900 and Sony
>>> motion controller already contain supported three-color LEDs).
>>>
>>> Now... yes, there's work to be done for the 3-color LEDs. Currently,
>>> they are treated as three different LEDs. (Which makes some sense, you
>>> can use "battery charging" trigger for LED, and CPU activity trigger
>>> for green, for example). It would be good to have some kind of
>>> grouping, so that userspace can tell "these 3 leds are actually
>>> combined into one light".
>>>
>> At first thanks for the review comments.
>> Treating the three physical LEDs of a RGB LED as separate LED devices
>> might have been implemented due to the lack of alternatives.
> 
> To be fair... they _are_ separate LED devices. In N900 case, you can
> even see light comming from slightly different places if you look closely.
> 
I mainly work with encapsulated USB HID LED devices like Thingm blink(1).
Due to the diffuse plastic cover you don't see the individual LEDs on the chip.

>> With one trigger controlling the red LED and another controlling the green
>> LED we may end up with a yellow light. Not sure whether this is what
>> we want.
> 
> Well, it should be understandable for most people.
> 
>> One driver for this extension was the idea of triggers using color
>> to visualize states etc.
>> Therefore it's not only about userspace controlling the color.
>> As a trigger is bound to a led_classdev we need a led_classdev
>> representing a RGB LED device.
>>
>> And ok: If required the sysfs interface can be splitted into separate
>> attributes for hue, saturation, and (existing) brightness.
> 
> Required.
> 
> Ok, so:
> 
> a) Do we want RGB leds to be handled by existing subsystem, or do we
> need separate layer on top of that?
> 
> b) Does RGB make sense, or HSV? RGB is quite widely used in graphics,
> and it is what hardware implements. (But we'd need to do gamma
> correction).
> 
HSV has the charme that the current monochrome V-only is a subset.
Therefore the current API can be used also with color LEDs.
However there might be good reasons for using RGB too.

> c) My hardware has "acceleration engine", LED is independend from
> CPU. That's rather big deal. Does yours? It seems to be quite common,
> at least in cellphones.
> 
Devices like blink(1) support storing and re-playing patterns, fading etc.

> Ideally, I'd like to have "triggers", but different ones. As in: if
> charging, do yellow " .xX" pattern. If fully charged, do green steady
> light. If message is waiting, do blue " x x" pattern. If none of
> above, do slow white blinking. (Plus priorities of events). But that's
> quite different from existing support...)
> 
I think for this a separate layer would be helpful.
Your primary intention is to e.g. display "charging" on the RGB LED
device. Most likely you don't want to split yellow into its red + green
component and then write to the respective (sub-)LEDs.
Also just think of the case that later you might decide that orange
is nicer than yellow.

> 								Pavel
> 

Re: [PATCH v5 1/4] leds: core: add generic support for RGB Color LED's

[Jacek Anaszewski]

Hi Heiner and Pavel,

On 03/29/2016 10:38 PM, Heiner Kallweit wrote:
> Am 29.03.2016 um 12:02 schrieb Pavel Machek:
>> Hi!
>>
>> First, please Cc me on RGB color support.
>>
>>> Add generic support for RGB Color LED's.
>>>
>>> Basic idea is to use enum led_brightness also for the hue and saturation
>>> color components.This allows to implement the color extension w/o
>>> changes to struct led_classdev.
>>>
>>> Select LEDS_RGB to enable building drivers using the RGB extension.
>>>
>>> Flag LED_SET_HUE_SAT allows to specify that hue / saturation
>>> should be overridden even if the provided values are zero.
>>>
>>> Some examples for writing values to /sys/class/leds/<xx>/brightness:
>>> (now also hex notation can be used)
>>>
>>> 255 -> set full brightness and keep existing color if set
>>> 0 -> switch LED off but keep existing color so that it can be restored
>>>       if the LED is switched on again later
>>> 0x1000000 -> switch LED off and set also hue and saturation to 0
>>> 0x00ffff -> set full brightness, full saturation and set hue to 0
>>> (red)
>>
>> Umm, that's rather strange interface -- and three values in single sysfs
>> file is actually forbidden.
>>
>> Plus, it is very much unlike existing interfaces for RGB LEDs, which
>> we already have supported in the tree. (At least nokia N900 and Sony
>> motion controller already contain supported three-color LEDs).
>>
>> Now... yes, there's work to be done for the 3-color LEDs. Currently,
>> they are treated as three different LEDs. (Which makes some sense, you
>> can use "battery charging" trigger for LED, and CPU activity trigger
>> for green, for example). It would be good to have some kind of
>> grouping, so that userspace can tell "these 3 leds are actually
>> combined into one light".
>>
> At first thanks for the review comments.
> Treating the three physical LEDs of a RGB LED as separate LED devices
> might have been implemented due to the lack of alternatives.
> With one trigger controlling the red LED and another controlling the green
> LED we may end up with a yellow light. Not sure whether this is what we want.
>
> One driver for this extension was the idea of triggers using color
> to visualize states etc.
> Therefore it's not only about userspace controlling the color.
> As a trigger is bound to a led_classdev we need a led_classdev
> representing a RGB LED device.
>
> And ok: If required the sysfs interface can be splitted into separate
> attributes for hue, saturation, and (existing) brightness.

It would have the same downsides as in case of having r, g and b in
separate attributes, i.e. - problems with setting LED colour in
a consistent way. This way LED blinking in whatever colour couldn't
be supported reliably. It was one of your primary rationale standing
behind this design, if I remember correctly. Second - what about
triggers? We've had a long discussion about it and this design turned
out to be most fitting.

It's hard to address these requirements by having the settings in
separate attributes, due to synchronization issues, and LED trigger
mechanism specificity.

There is a question whether we can bend the sysfs "one value per sysfs
file" rule down to RGB LEDs needs.

Of course other brilliant ideas on how to approach the problem are
more than expected.

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v5 1/4] leds: core: add generic support for RGB Color LED's

[Jacek Anaszewski]

On 03/29/2016 11:43 PM, Pavel Machek wrote:
> Hi!
>
>>> First, please Cc me on RGB color support.
>>>
>>>> Add generic support for RGB Color LED's.
>>>>
>>>> Basic idea is to use enum led_brightness also for the hue and saturation
>>>> color components.This allows to implement the color extension w/o
>>>> changes to struct led_classdev.
>>>>
>>>> Select LEDS_RGB to enable building drivers using the RGB extension.
>>>>
>>>> Flag LED_SET_HUE_SAT allows to specify that hue / saturation
>>>> should be overridden even if the provided values are zero.
>>>>
>>>> Some examples for writing values to /sys/class/leds/<xx>/brightness:
>>>> (now also hex notation can be used)
>>>>
>>>> 255 -> set full brightness and keep existing color if set
>>>> 0 -> switch LED off but keep existing color so that it can be restored
>>>>       if the LED is switched on again later
>>>> 0x1000000 -> switch LED off and set also hue and saturation to 0
>>>> 0x00ffff -> set full brightness, full saturation and set hue to 0
>>>> (red)
>>>
>>> Umm, that's rather strange interface -- and three values in single sysfs
>>> file is actually forbidden.
>>>
>>> Plus, it is very much unlike existing interfaces for RGB LEDs, which
>>> we already have supported in the tree. (At least nokia N900 and Sony
>>> motion controller already contain supported three-color LEDs).
>>>
>>> Now... yes, there's work to be done for the 3-color LEDs. Currently,
>>> they are treated as three different LEDs. (Which makes some sense, you
>>> can use "battery charging" trigger for LED, and CPU activity trigger
>>> for green, for example). It would be good to have some kind of
>>> grouping, so that userspace can tell "these 3 leds are actually
>>> combined into one light".
>>>
>> At first thanks for the review comments.
>> Treating the three physical LEDs of a RGB LED as separate LED devices
>> might have been implemented due to the lack of alternatives.
>
> To be fair... they _are_ separate LED devices. In N900 case, you can
> even see light comming from slightly different places if you look closely.
>
>> With one trigger controlling the red LED and another controlling the green
>> LED we may end up with a yellow light. Not sure whether this is what
>> we want.
>
> Well, it should be understandable for most people.
>
>> One driver for this extension was the idea of triggers using color
>> to visualize states etc.
>> Therefore it's not only about userspace controlling the color.
>> As a trigger is bound to a led_classdev we need a led_classdev
>> representing a RGB LED device.
>>
>> And ok: If required the sysfs interface can be splitted into separate
>> attributes for hue, saturation, and (existing) brightness.
>
> Required.
>
> Ok, so:
>
> a) Do we want RGB leds to be handled by existing subsystem, or do we
> need separate layer on top of that?
>
> b) Does RGB make sense, or HSV? RGB is quite widely used in graphics,
> and it is what hardware implements. (But we'd need to do gamma
> correction).
>
> c) My hardware has "acceleration engine", LED is independend from
> CPU. That's rather big deal. Does yours? It seems to be quite common,
> at least in cellphones.
>
> Ideally, I'd like to have "triggers", but different ones. As in: if
> charging, do yellow " .xX" pattern. If fully charged, do green steady
> light. If message is waiting, do blue " x x" pattern. If none of
> above, do slow white blinking. (Plus priorities of events). But that's
> quite different from existing support...)

Please note that HSV colour scheme allows to neatly project monochrome
brightness semantics on the RGB realm. I.e. you can have fixed
hue and saturation, and by changing the brightness component a perceived
colour intensity can be altered.

-- 
Best regards,
Jacek Anaszewski
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 1/4] leds: core: add generic support for RGB Color LED's

[Jacek Anaszewski]

On 03/29/2016 11:43 PM, Pavel Machek wrote:
> Hi!
>
>>> First, please Cc me on RGB color support.
>>>
>>>> Add generic support for RGB Color LED's.
>>>>
>>>> Basic idea is to use enum led_brightness also for the hue and saturation
>>>> color components.This allows to implement the color extension w/o
>>>> changes to struct led_classdev.
>>>>
>>>> Select LEDS_RGB to enable building drivers using the RGB extension.
>>>>
>>>> Flag LED_SET_HUE_SAT allows to specify that hue / saturation
>>>> should be overridden even if the provided values are zero.
>>>>
>>>> Some examples for writing values to /sys/class/leds/<xx>/brightness:
>>>> (now also hex notation can be used)
>>>>
>>>> 255 -> set full brightness and keep existing color if set
>>>> 0 -> switch LED off but keep existing color so that it can be restored
>>>>       if the LED is switched on again later
>>>> 0x1000000 -> switch LED off and set also hue and saturation to 0
>>>> 0x00ffff -> set full brightness, full saturation and set hue to 0
>>>> (red)
>>>
>>> Umm, that's rather strange interface -- and three values in single sysfs
>>> file is actually forbidden.
>>>
>>> Plus, it is very much unlike existing interfaces for RGB LEDs, which
>>> we already have supported in the tree. (At least nokia N900 and Sony
>>> motion controller already contain supported three-color LEDs).
>>>
>>> Now... yes, there's work to be done for the 3-color LEDs. Currently,
>>> they are treated as three different LEDs. (Which makes some sense, you
>>> can use "battery charging" trigger for LED, and CPU activity trigger
>>> for green, for example). It would be good to have some kind of
>>> grouping, so that userspace can tell "these 3 leds are actually
>>> combined into one light".
>>>
>> At first thanks for the review comments.
>> Treating the three physical LEDs of a RGB LED as separate LED devices
>> might have been implemented due to the lack of alternatives.
>
> To be fair... they _are_ separate LED devices. In N900 case, you can
> even see light comming from slightly different places if you look closely.
>
>> With one trigger controlling the red LED and another controlling the green
>> LED we may end up with a yellow light. Not sure whether this is what
>> we want.
>
> Well, it should be understandable for most people.
>
>> One driver for this extension was the idea of triggers using color
>> to visualize states etc.
>> Therefore it's not only about userspace controlling the color.
>> As a trigger is bound to a led_classdev we need a led_classdev
>> representing a RGB LED device.
>>
>> And ok: If required the sysfs interface can be splitted into separate
>> attributes for hue, saturation, and (existing) brightness.
>
> Required.
>
> Ok, so:
>
> a) Do we want RGB leds to be handled by existing subsystem, or do we
> need separate layer on top of that?
>
> b) Does RGB make sense, or HSV? RGB is quite widely used in graphics,
> and it is what hardware implements. (But we'd need to do gamma
> correction).
>
> c) My hardware has "acceleration engine", LED is independend from
> CPU. That's rather big deal. Does yours? It seems to be quite common,
> at least in cellphones.
>
> Ideally, I'd like to have "triggers", but different ones. As in: if
> charging, do yellow " .xX" pattern. If fully charged, do green steady
> light. If message is waiting, do blue " x x" pattern. If none of
> above, do slow white blinking. (Plus priorities of events). But that's
> quite different from existing support...)

Please note that HSV colour scheme allows to neatly project monochrome
brightness semantics on the RGB realm. I.e. you can have fixed
hue and saturation, and by changing the brightness component a perceived
colour intensity can be altered.

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v5 1/4] leds: core: add generic support for RGB Color LED's

[Pavel Machek]

Hi!

> >Ok, so:
> >
> >a) Do we want RGB leds to be handled by existing subsystem, or do we
> >need separate layer on top of that?
> >
> >b) Does RGB make sense, or HSV? RGB is quite widely used in graphics,
> >and it is what hardware implements. (But we'd need to do gamma
> >correction).
> >
> >c) My hardware has "acceleration engine", LED is independend from
> >CPU. That's rather big deal. Does yours? It seems to be quite common,
> >at least in cellphones.
> >
> >Ideally, I'd like to have "triggers", but different ones. As in: if
> >charging, do yellow " .xX" pattern. If fully charged, do green steady
> >light. If message is waiting, do blue " x x" pattern. If none of
> >above, do slow white blinking. (Plus priorities of events). But that's
> >quite different from existing support...)
> 
> Please note that HSV colour scheme allows to neatly project monochrome
> brightness semantics on the RGB realm. I.e. you can have fixed
> hue and saturation, and by changing the brightness component a perceived
> colour intensity can be altered.

I see HSV has some advantages. But we already have LEDs with multiple
colors, and kernel already handles them:

pavel@duo:~$ ls -1 /sys/class/leds/
tpacpi:green:batt
tpacpi:orange:batt

This is physically 2 leds but hidden under one indicator, so you got
"off", "green", "orange" and "green+orange".
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH v5 1/4] leds: core: add generic support for RGB Color LED's

[Heiner Kallweit]

On Wed, Mar 30, 2016 at 3:03 PM, Pavel Machek <pavel@ucw.cz> wrote:
> Hi!
>
>> >Ok, so:
>> >
>> >a) Do we want RGB leds to be handled by existing subsystem, or do we
>> >need separate layer on top of that?
>> >
>> >b) Does RGB make sense, or HSV? RGB is quite widely used in graphics,
>> >and it is what hardware implements. (But we'd need to do gamma
>> >correction).
>> >
>> >c) My hardware has "acceleration engine", LED is independend from
>> >CPU. That's rather big deal. Does yours? It seems to be quite common,
>> >at least in cellphones.
>> >
>> >Ideally, I'd like to have "triggers", but different ones. As in: if
>> >charging, do yellow " .xX" pattern. If fully charged, do green steady
>> >light. If message is waiting, do blue " x x" pattern. If none of
>> >above, do slow white blinking. (Plus priorities of events). But that's
>> >quite different from existing support...)
>>
>> Please note that HSV colour scheme allows to neatly project monochrome
>> brightness semantics on the RGB realm. I.e. you can have fixed
>> hue and saturation, and by changing the brightness component a perceived
>> colour intensity can be altered.
>
> I see HSV has some advantages. But we already have LEDs with multiple
> colors, and kernel already handles them:
>
> pavel@duo:~$ ls -1 /sys/class/leds/
> tpacpi:green:batt
> tpacpi:orange:batt
>
> This is physically 2 leds but hidden under one indicator, so you got
> "off", "green", "orange" and "green+orange".

That's a good example. As long as you can recognize green+orange as
separate lights/colors
(w/o magnifying glass) I wouldn't call it "a LED with multiple colors"
but "multiple
LED devices".

In my use case we talk about RGB LEDs like the commonly used 5050 SMD RGB LEDs.
And it's not only about using a handful of discrete colors but about
displaying any arbitrary
color.
So far the kernel exposes the physical RGB LEDs as separate LEDs only
and I can't use
a trigger to e.g. set "magenta with 50% brightness".

Heiner

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

Re: [PATCH v5 1/4] leds: core: add generic support for RGB Color LED's

[Jacek Anaszewski]

Hi Heiner,

On 03/30/2016 03:59 PM, Heiner Kallweit wrote:
> On Wed, Mar 30, 2016 at 3:03 PM, Pavel Machek <pavel@ucw.cz> wrote:
>> Hi!
>>
>>>> Ok, so:
>>>>
>>>> a) Do we want RGB leds to be handled by existing subsystem, or do we
>>>> need separate layer on top of that?
>>>>
>>>> b) Does RGB make sense, or HSV? RGB is quite widely used in graphics,
>>>> and it is what hardware implements. (But we'd need to do gamma
>>>> correction).
>>>>
>>>> c) My hardware has "acceleration engine", LED is independend from
>>>> CPU. That's rather big deal. Does yours? It seems to be quite common,
>>>> at least in cellphones.
>>>>
>>>> Ideally, I'd like to have "triggers", but different ones. As in: if
>>>> charging, do yellow " .xX" pattern. If fully charged, do green steady
>>>> light. If message is waiting, do blue " x x" pattern. If none of
>>>> above, do slow white blinking. (Plus priorities of events). But that's
>>>> quite different from existing support...)
>>>
>>> Please note that HSV colour scheme allows to neatly project monochrome
>>> brightness semantics on the RGB realm. I.e. you can have fixed
>>> hue and saturation, and by changing the brightness component a perceived
>>> colour intensity can be altered.
>>
>> I see HSV has some advantages. But we already have LEDs with multiple
>> colors, and kernel already handles them:
>>
>> pavel@duo:~$ ls -1 /sys/class/leds/
>> tpacpi:green:batt
>> tpacpi:orange:batt
>>
>> This is physically 2 leds but hidden under one indicator, so you got
>> "off", "green", "orange" and "green+orange".
>
> That's a good example. As long as you can recognize green+orange as
> separate lights/colors
> (w/o magnifying glass) I wouldn't call it "a LED with multiple colors"
> but "multiple
> LED devices".
>
> In my use case we talk about RGB LEDs like the commonly used 5050 SMD RGB LEDs.
> And it's not only about using a handful of discrete colors but about
> displaying any arbitrary
> color.
> So far the kernel exposes the physical RGB LEDs as separate LEDs only
> and I can't use
> a trigger to e.g. set "magenta with 50% brightness".

I think that we should consult more people before pushing the solution
upstream. Would you mind writing a message with an explanation of the
issue to linux-api list?

Please keep in mind also the information from the "Attributes" section
of Documentation/filesystems/sysfs.txt.

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v5 1/4] leds: core: add generic support for RGB Color LED's

[Pavel Machek]

Hi!

> > To be fair... they _are_ separate LED devices. In N900 case, you can
> > even see light comming from slightly different places if you look closely.
> > 
> I mainly work with encapsulated USB HID LED devices like Thingm blink(1).
> Due to the diffuse plastic cover you don't see the individual LEDs on the chip.

Yeah, so on N900, you can't really see the individual LEDs, either.
But white is not uniform white.

On PS/3 motion controller (another device that is already supported),
diffusing works a bit better. 

> > Required.
> > 
> > Ok, so:
> > 
> > a) Do we want RGB leds to be handled by existing subsystem, or do we
> > need separate layer on top of that?
> > 
> > b) Does RGB make sense, or HSV? RGB is quite widely used in graphics,
> > and it is what hardware implements. (But we'd need to do gamma
> > correction).
> > 
> HSV has the charme that the current monochrome V-only is a subset.
> Therefore the current API can be used also with color LEDs.
> However there might be good reasons for using RGB too.

Yes, nice, but we already have RGB LED support in kernel, and it looks
different from what is proposed here. And quite incompatible. 

> > c) My hardware has "acceleration engine", LED is independend from
> > CPU. That's rather big deal. Does yours? It seems to be quite common,
> > at least in cellphones.
> > 
> Devices like blink(1) support storing and re-playing patterns, fading etc.
> 
> > Ideally, I'd like to have "triggers", but different ones. As in: if
> > charging, do yellow " .xX" pattern. If fully charged, do green steady
> > light. If message is waiting, do blue " x x" pattern. If none of
> > above, do slow white blinking. (Plus priorities of events). But that's
> > quite different from existing support...)
> > 
> I think for this a separate layer would be helpful.
> Your primary intention is to e.g. display "charging" on the RGB LED
> device. Most likely you don't want to split yellow into its red + green
> component and then write to the respective (sub-)LEDs.
> Also just think of the case that later you might decide that orange
> is nicer than yellow.

Well, so what about keeping existing red/green/blue LED devices (to
stay backward compatible) and then add separate device that links to
these, and controls patterns and colors?

Small complication is that (at least on N900) the pattern capability
can control keyboard backlight LEDs, too. It has nine channels, and
you select 3 channels that are connected to pattern generator.

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

Re: [PATCH v5 1/4] leds: core: add generic support for RGB Color LED's

[Pavel Machek]

Hi!

> >Ideally, I'd like to have "triggers", but different ones. As in: if
> >charging, do yellow " .xX" pattern. If fully charged, do green steady
> >light. If message is waiting, do blue " x x" pattern. If none of
> >above, do slow white blinking. (Plus priorities of events). But that's
> >quite different from existing support...)
> 
> Please note that HSV colour scheme allows to neatly project monochrome
> brightness semantics on the RGB realm. I.e. you can have fixed
> hue and saturation, and by changing the brightness component a perceived
> colour intensity can be altered.

Yes, that's nice, but it is incompatible with already existing RGB
support in kernel. Plus, echoing hue into file called brightness is
extremely ugly, and it violates sysfs rules.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 1/4] leds: core: add generic support for RGB Color LED's

[Pavel Machek]

Hi!

> >Ideally, I'd like to have "triggers", but different ones. As in: if
> >charging, do yellow " .xX" pattern. If fully charged, do green steady
> >light. If message is waiting, do blue " x x" pattern. If none of
> >above, do slow white blinking. (Plus priorities of events). But that's
> >quite different from existing support...)
> 
> Please note that HSV colour scheme allows to neatly project monochrome
> brightness semantics on the RGB realm. I.e. you can have fixed
> hue and saturation, and by changing the brightness component a perceived
> colour intensity can be altered.

Yes, that's nice, but it is incompatible with already existing RGB
support in kernel. Plus, echoing hue into file called brightness is
extremely ugly, and it violates sysfs rules.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH v5 1/4] leds: core: add generic support for RGB Color LED's

[Pavel Machek]

Hi!

> > pavel@duo:~$ ls -1 /sys/class/leds/
> > tpacpi:green:batt
> > tpacpi:orange:batt
> >
> > This is physically 2 leds but hidden under one indicator, so you got
> > "off", "green", "orange" and "green+orange".
> 
> That's a good example. As long as you can recognize green+orange as
> separate lights/colors
> (w/o magnifying glass) I wouldn't call it "a LED with multiple colors"
> but "multiple
> LED devices".

Well, that's how it is currently handled. But for the user, it looks
as a LED with multiple colors.

> In my use case we talk about RGB LEDs like the commonly used 5050 SMD RGB LEDs.
> And it's not only about using a handful of discrete colors but about
> displaying any arbitrary
> color.
> So far the kernel exposes the physical RGB LEDs as separate LEDs only
> and I can't use
> a trigger to e.g. set "magenta with 50% brightness".

Why not?

What do you do if you want to display magenta on your LCD?

You compute RGB values, then display them.

What would you do for the LEDs? Same thing.

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

Re: [PATCH v5 1/4] leds: core: add generic support for RGB Color LED's

[Jacek Anaszewski]

On 04/01/2016 02:55 PM, Pavel Machek wrote:
> Hi!
>
>>> pavel@duo:~$ ls -1 /sys/class/leds/
>>> tpacpi:green:batt
>>> tpacpi:orange:batt
>>>
>>> This is physically 2 leds but hidden under one indicator, so you got
>>> "off", "green", "orange" and "green+orange".
>>
>> That's a good example. As long as you can recognize green+orange as
>> separate lights/colors
>> (w/o magnifying glass) I wouldn't call it "a LED with multiple colors"
>> but "multiple
>> LED devices".
>
> Well, that's how it is currently handled. But for the user, it looks
> as a LED with multiple colors.
>
>> In my use case we talk about RGB LEDs like the commonly used 5050 SMD RGB LEDs.
>> And it's not only about using a handful of discrete colors but about
>> displaying any arbitrary
>> color.
>> So far the kernel exposes the physical RGB LEDs as separate LEDs only
>> and I can't use
>> a trigger to e.g. set "magenta with 50% brightness".
>
> Why not?
>
> What do you do if you want to display magenta on your LCD?
>
> You compute RGB values, then display them.

The main drawback is that you can't set the colour at one go,
but have to set brightness of each LED class device (R,G,B)
separately. It incurs delays between setting each colour component.

It is also impossible to set arbitrary colour from a trigger.
Similarly blinking with arbitrarily chosen colour from RGB palette
is impossible if separate colour components are treated as
separate LEDs.

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v5 1/4] leds: core: add generic support for RGB Color LED's

[Pavel Machek]

Hi!
On Wed 2016-03-30 09:57:38, Jacek Anaszewski wrote:
> Hi Heiner and Pavel,
> 
> On 03/29/2016 10:38 PM, Heiner Kallweit wrote:
> >Am 29.03.2016 um 12:02 schrieb Pavel Machek:
> >>Hi!
> >>
> >>First, please Cc me on RGB color support.
> >>
> >>>Add generic support for RGB Color LED's.
> >>>
> >>>Basic idea is to use enum led_brightness also for the hue and saturation
> >>>color components.This allows to implement the color extension w/o
> >>>changes to struct led_classdev.
> >>>
> >>>Select LEDS_RGB to enable building drivers using the RGB extension.
> >>>
> >>>Flag LED_SET_HUE_SAT allows to specify that hue / saturation
> >>>should be overridden even if the provided values are zero.
> >>>
> >>>Some examples for writing values to /sys/class/leds/<xx>/brightness:
> >>>(now also hex notation can be used)
> >>>
> >>>255 -> set full brightness and keep existing color if set
> >>>0 -> switch LED off but keep existing color so that it can be restored
> >>>      if the LED is switched on again later
> >>>0x1000000 -> switch LED off and set also hue and saturation to 0
> >>>0x00ffff -> set full brightness, full saturation and set hue to 0
> >>>(red)
> >>
> >>Umm, that's rather strange interface -- and three values in single sysfs
> >>file is actually forbidden.
> >>
> >>Plus, it is very much unlike existing interfaces for RGB LEDs, which
> >>we already have supported in the tree. (At least nokia N900 and Sony
> >>motion controller already contain supported three-color LEDs).
> >>
> >>Now... yes, there's work to be done for the 3-color LEDs. Currently,
> >>they are treated as three different LEDs. (Which makes some sense, you
> >>can use "battery charging" trigger for LED, and CPU activity trigger
> >>for green, for example). It would be good to have some kind of
> >>grouping, so that userspace can tell "these 3 leds are actually
> >>combined into one light".
Hi!

> >At first thanks for the review comments.
> >Treating the three physical LEDs of a RGB LED as separate LED devices
> >might have been implemented due to the lack of alternatives.
> >With one trigger controlling the red LED and another controlling the green
> >LED we may end up with a yellow light. Not sure whether this is what we want.
> >
> >One driver for this extension was the idea of triggers using color
> >to visualize states etc.
> >Therefore it's not only about userspace controlling the color.
> >As a trigger is bound to a led_classdev we need a led_classdev
> >representing a RGB LED device.
> >
> >And ok: If required the sysfs interface can be splitted into separate
> >attributes for hue, saturation, and (existing) brightness.
> 
> It would have the same downsides as in case of having r, g and b in
> separate attributes, i.e. - problems with setting LED colour in
> a consistent way. This way LED blinking in whatever colour couldn't
> be supported reliably. It was one of your primary rationale standing
> behind this design, if I remember correctly. Second - what about
> triggers? We've had a long discussion about it and this design turned
> out to be most fitting.

Are on/off triggers really that useful for a LED that can produce 16
million colors?

I believe we should support patterns for RGB LEDs. Something like
[ (time, r, g, b), ... ] . Ok, what about this one?

Lets say we have

/sys/class/pattern/lp5533::0
/sys/class/pattern/software::0

/sys/class/led/n900::red ; default trigger "lp5533::0:0"
/sys/class/led/n900::green ; default trigger "lp5533::0:1"
/sys/class/led/n900::blue ; default trigger "lp5533::0:2"

Normally, pattern would correspond to one RGB LED. We could have
attribute "/sys/class/pattern/lp5533::0/color" containing R,G,B for
this pattern. Then we could have normal "trigger" mechanism, working
with the color used. Probably recognizing "none" for manual control,
and "pattern" for pattern control. (Pattern would be controlled as
described above).

> It's hard to address these requirements by having the settings in
> separate attributes, due to synchronization issues, and LED trigger
> mechanism specificity.
> 
> There is a question whether we can bend the sysfs "one value per sysfs
> file" rule down to RGB LEDs needs.
> 
> Of course other brilliant ideas on how to approach the problem are
> more than expected.

linux-api sounds like interesting idea, please cc me if you do that.

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

Re: [PATCH v5 1/4] leds: core: add generic support for RGB Color LED's

[Pavel Machek]

Hi!

> >>>pavel@duo:~$ ls -1 /sys/class/leds/
> >>>tpacpi:green:batt
> >>>tpacpi:orange:batt
> >>>
> >>>This is physically 2 leds but hidden under one indicator, so you got
> >>>"off", "green", "orange" and "green+orange".
> >>
> >>That's a good example. As long as you can recognize green+orange as
> >>separate lights/colors
> >>(w/o magnifying glass) I wouldn't call it "a LED with multiple colors"
> >>but "multiple
> >>LED devices".
> >
> >Well, that's how it is currently handled. But for the user, it looks
> >as a LED with multiple colors.
> >
> >>In my use case we talk about RGB LEDs like the commonly used 5050 SMD RGB LEDs.
> >>And it's not only about using a handful of discrete colors but about
> >>displaying any arbitrary
> >>color.
> >>So far the kernel exposes the physical RGB LEDs as separate LEDs only
> >>and I can't use
> >>a trigger to e.g. set "magenta with 50% brightness".
> >
> >Why not?
> >
> >What do you do if you want to display magenta on your LCD?
> >
> >You compute RGB values, then display them.
> 
> The main drawback is that you can't set the colour at one go,
> but have to set brightness of each LED class device (R,G,B)
> separately. It incurs delays between setting each colour component.

Yeah. Well, on some hardware, that's just the way it is. If the leds
are separate devices on i2c, you can't really set them in one go.

But some hardware has hardware pattern controls, and it can set them
atomically. 

> It is also impossible to set arbitrary colour from a trigger.
> Similarly blinking with arbitrarily chosen colour from RGB palette
> is impossible if separate colour components are treated as
> separate LEDs.

Yes, see the proposal in the other mail. I believe we should have
separate R, G, B LED devices, and separate pattern controller.

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

Re: [PATCH v5 1/4] leds: core: add generic support for RGB Color LED's

[Jacek Anaszewski]

On 04/01/2016 04:07 PM, Pavel Machek wrote:
> Hi!
>
>>>>> pavel@duo:~$ ls -1 /sys/class/leds/
>>>>> tpacpi:green:batt
>>>>> tpacpi:orange:batt
>>>>>
>>>>> This is physically 2 leds but hidden under one indicator, so you got
>>>>> "off", "green", "orange" and "green+orange".
>>>>
>>>> That's a good example. As long as you can recognize green+orange as
>>>> separate lights/colors
>>>> (w/o magnifying glass) I wouldn't call it "a LED with multiple colors"
>>>> but "multiple
>>>> LED devices".
>>>
>>> Well, that's how it is currently handled. But for the user, it looks
>>> as a LED with multiple colors.
>>>
>>>> In my use case we talk about RGB LEDs like the commonly used 5050 SMD RGB LEDs.
>>>> And it's not only about using a handful of discrete colors but about
>>>> displaying any arbitrary
>>>> color.
>>>> So far the kernel exposes the physical RGB LEDs as separate LEDs only
>>>> and I can't use
>>>> a trigger to e.g. set "magenta with 50% brightness".
>>>
>>> Why not?
>>>
>>> What do you do if you want to display magenta on your LCD?
>>>
>>> You compute RGB values, then display them.
>>
>> The main drawback is that you can't set the colour at one go,
>> but have to set brightness of each LED class device (R,G,B)
>> separately. It incurs delays between setting each colour component.
>
> Yeah. Well, on some hardware, that's just the way it is. If the leds
> are separate devices on i2c, you can't really set them in one go.

Delays can occur even if the LEDs are controlled by the same device.
Brightness of each LED class device is set with separate system
call and there will be always some time shift between particular I2C
transmissions that set the brightness for each sub-LED.

If the three sub-LEDs were controlled by a single LED class device
then we could setup the brightness of each sub-LED with single I2C
transmission.

> But some hardware has hardware pattern controls, and it can set them
> atomically.
>
>> It is also impossible to set arbitrary colour from a trigger.
>> Similarly blinking with arbitrarily chosen colour from RGB palette
>> is impossible if separate colour components are treated as
>> separate LEDs.
>
> Yes, see the proposal in the other mail. I believe we should have
> separate R, G, B LED devices, and separate pattern controller.
>
> Best regards,
> 									Pavel
>


-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v5 1/4] leds: core: add generic support for RGB Color LED's

[Pavel Machek]

Hi!

> >>The main drawback is that you can't set the colour at one go,
> >>but have to set brightness of each LED class device (R,G,B)
> >>separately. It incurs delays between setting each colour component.
> >
> >Yeah. Well, on some hardware, that's just the way it is. If the leds
> >are separate devices on i2c, you can't really set them in one go.
> 
> Delays can occur even if the LEDs are controlled by the same device.
> Brightness of each LED class device is set with separate system
> call and there will be always some time shift between particular I2C
> transmissions that set the brightness for each sub-LED.
> 
> If the three sub-LEDs were controlled by a single LED class device
> then we could setup the brightness of each sub-LED with single I2C
> transmission.

Ok, well, yes, maybe you could.

You can still do that with the proposed interface, but yes, it will be
trickier.

OTOH proposed interface will also help with the hardware pattern
support, will work with existing leds, and matches the way hardware
works. So I believe it is worth it.

Best regards,

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

Re: [PATCH v5 1/4] leds: core: add generic support for RGB Color LED's

[Jacek Anaszewski]

On 04/01/2016 03:57 PM, Pavel Machek wrote:
> Hi!
> On Wed 2016-03-30 09:57:38, Jacek Anaszewski wrote:
>> Hi Heiner and Pavel,
>>
>> On 03/29/2016 10:38 PM, Heiner Kallweit wrote:
>>> Am 29.03.2016 um 12:02 schrieb Pavel Machek:
>>>> Hi!
>>>>
>>>> First, please Cc me on RGB color support.
>>>>
>>>>> Add generic support for RGB Color LED's.
>>>>>
>>>>> Basic idea is to use enum led_brightness also for the hue and saturation
>>>>> color components.This allows to implement the color extension w/o
>>>>> changes to struct led_classdev.
>>>>>
>>>>> Select LEDS_RGB to enable building drivers using the RGB extension.
>>>>>
>>>>> Flag LED_SET_HUE_SAT allows to specify that hue / saturation
>>>>> should be overridden even if the provided values are zero.
>>>>>
>>>>> Some examples for writing values to /sys/class/leds/<xx>/brightness:
>>>>> (now also hex notation can be used)
>>>>>
>>>>> 255 -> set full brightness and keep existing color if set
>>>>> 0 -> switch LED off but keep existing color so that it can be restored
>>>>>       if the LED is switched on again later
>>>>> 0x1000000 -> switch LED off and set also hue and saturation to 0
>>>>> 0x00ffff -> set full brightness, full saturation and set hue to 0
>>>>> (red)
>>>>
>>>> Umm, that's rather strange interface -- and three values in single sysfs
>>>> file is actually forbidden.
>>>>
>>>> Plus, it is very much unlike existing interfaces for RGB LEDs, which
>>>> we already have supported in the tree. (At least nokia N900 and Sony
>>>> motion controller already contain supported three-color LEDs).
>>>>
>>>> Now... yes, there's work to be done for the 3-color LEDs. Currently,
>>>> they are treated as three different LEDs. (Which makes some sense, you
>>>> can use "battery charging" trigger for LED, and CPU activity trigger
>>>> for green, for example). It would be good to have some kind of
>>>> grouping, so that userspace can tell "these 3 leds are actually
>>>> combined into one light".
> Hi!
>
>>> At first thanks for the review comments.
>>> Treating the three physical LEDs of a RGB LED as separate LED devices
>>> might have been implemented due to the lack of alternatives.
>>> With one trigger controlling the red LED and another controlling the green
>>> LED we may end up with a yellow light. Not sure whether this is what we want.
>>>
>>> One driver for this extension was the idea of triggers using color
>>> to visualize states etc.
>>> Therefore it's not only about userspace controlling the color.
>>> As a trigger is bound to a led_classdev we need a led_classdev
>>> representing a RGB LED device.
>>>
>>> And ok: If required the sysfs interface can be splitted into separate
>>> attributes for hue, saturation, and (existing) brightness.
>>
>> It would have the same downsides as in case of having r, g and b in
>> separate attributes, i.e. - problems with setting LED colour in
>> a consistent way. This way LED blinking in whatever colour couldn't
>> be supported reliably. It was one of your primary rationale standing
>> behind this design, if I remember correctly. Second - what about
>> triggers? We've had a long discussion about it and this design turned
>> out to be most fitting.
>
> Are on/off triggers really that useful for a LED that can produce 16
> million colors?
>
> I believe we should support patterns for RGB LEDs. Something like
> [ (time, r, g, b), ... ] . Ok, what about this one?
>
> Lets say we have
>
> /sys/class/pattern/lp5533::0
> /sys/class/pattern/software::0
>
> /sys/class/led/n900::red ; default trigger "lp5533::0:0"
> /sys/class/led/n900::green ; default trigger "lp5533::0:1"
> /sys/class/led/n900::blue ; default trigger "lp5533::0:2"
>
> Normally, pattern would correspond to one RGB LED. We could have
> attribute "/sys/class/pattern/lp5533::0/color" containing R,G,B for
> this pattern.

This involves the same issue you were opposed to: three values per
sysfs attribute.

> Then we could have normal "trigger" mechanism, working
> with the color used. Probably recognizing "none" for manual control,
> and "pattern" for pattern control. (Pattern would be controlled as
> described above).
>
>> It's hard to address these requirements by having the settings in
>> separate attributes, due to synchronization issues, and LED trigger
>> mechanism specificity.
>>
>> There is a question whether we can bend the sysfs "one value per sysfs
>> file" rule down to RGB LEDs needs.
>>
>> Of course other brilliant ideas on how to approach the problem are
>> more than expected.
>
> linux-api sounds like interesting idea, please cc me if you do that.
>
> Best regards,
> 									Pavel
>

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v5 1/4] leds: core: add generic support for RGB Color LED's

[Pavel Machek]

Hi!

> >>It would have the same downsides as in case of having r, g and b in
> >>separate attributes, i.e. - problems with setting LED colour in
> >>a consistent way. This way LED blinking in whatever colour couldn't
> >>be supported reliably. It was one of your primary rationale standing
> >>behind this design, if I remember correctly. Second - what about
> >>triggers? We've had a long discussion about it and this design turned
> >>out to be most fitting.
> >
> >Are on/off triggers really that useful for a LED that can produce 16
> >million colors?
> >
> >I believe we should support patterns for RGB LEDs. Something like
> >[ (time, r, g, b), ... ] . Ok, what about this one?
> >
> >Lets say we have
> >
> >/sys/class/pattern/lp5533::0
> >/sys/class/pattern/software::0
> >
> >/sys/class/led/n900::red ; default trigger "lp5533::0:0"
> >/sys/class/led/n900::green ; default trigger "lp5533::0:1"
> >/sys/class/led/n900::blue ; default trigger "lp5533::0:2"
> >
> >Normally, pattern would correspond to one RGB LED. We could have
> >attribute "/sys/class/pattern/lp5533::0/color" containing R,G,B for
> >this pattern.
> 
> This involves the same issue you were opposed to: three values per
> sysfs attribute.

And solves a lot of other things. Like actually being backwards
compatible.

And yes, it involves three values in a file, but now it is array of
led brightnesses, and that might actually be acceptable. (At least the
values have uniform meaning).

Plus, it is not "issue you were opposed to" it is "something that is
not permitted by sysfs maintainers".

Thanks,
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 1/4] leds: core: add generic support for RGB Color LED's

[Pavel Machek]

Hi!

> >>It would have the same downsides as in case of having r, g and b in
> >>separate attributes, i.e. - problems with setting LED colour in
> >>a consistent way. This way LED blinking in whatever colour couldn't
> >>be supported reliably. It was one of your primary rationale standing
> >>behind this design, if I remember correctly. Second - what about
> >>triggers? We've had a long discussion about it and this design turned
> >>out to be most fitting.
> >
> >Are on/off triggers really that useful for a LED that can produce 16
> >million colors?
> >
> >I believe we should support patterns for RGB LEDs. Something like
> >[ (time, r, g, b), ... ] . Ok, what about this one?
> >
> >Lets say we have
> >
> >/sys/class/pattern/lp5533::0
> >/sys/class/pattern/software::0
> >
> >/sys/class/led/n900::red ; default trigger "lp5533::0:0"
> >/sys/class/led/n900::green ; default trigger "lp5533::0:1"
> >/sys/class/led/n900::blue ; default trigger "lp5533::0:2"
> >
> >Normally, pattern would correspond to one RGB LED. We could have
> >attribute "/sys/class/pattern/lp5533::0/color" containing R,G,B for
> >this pattern.
> 
> This involves the same issue you were opposed to: three values per
> sysfs attribute.

And solves a lot of other things. Like actually being backwards
compatible.

And yes, it involves three values in a file, but now it is array of
led brightnesses, and that might actually be acceptable. (At least the
values have uniform meaning).

Plus, it is not "issue you were opposed to" it is "something that is
not permitted by sysfs maintainers".

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

Re: [PATCH v5 1/4] leds: core: add generic support for RGB Color LED's

[Jacek Anaszewski]

Hi Pavel,

On 04/01/2016 11:18 PM, Pavel Machek wrote:
> Hi!
>
>>>> It would have the same downsides as in case of having r, g and b in
>>>> separate attributes, i.e. - problems with setting LED colour in
>>>> a consistent way. This way LED blinking in whatever colour couldn't
>>>> be supported reliably. It was one of your primary rationale standing
>>>> behind this design, if I remember correctly. Second - what about
>>>> triggers? We've had a long discussion about it and this design turned
>>>> out to be most fitting.
>>>
>>> Are on/off triggers really that useful for a LED that can produce 16
>>> million colors?
>>>
>>> I believe we should support patterns for RGB LEDs. Something like
>>> [ (time, r, g, b), ... ] . Ok, what about this one?
>>>
>>> Lets say we have
>>>
>>> /sys/class/pattern/lp5533::0
>>> /sys/class/pattern/software::0
>>>
>>> /sys/class/led/n900::red ; default trigger "lp5533::0:0"
>>> /sys/class/led/n900::green ; default trigger "lp5533::0:1"
>>> /sys/class/led/n900::blue ; default trigger "lp5533::0:2"
>>>
>>> Normally, pattern would correspond to one RGB LED. We could have
>>> attribute "/sys/class/pattern/lp5533::0/color" containing R,G,B for
>>> this pattern.

Could you give an example on how to set a color for RGB LED using
this interface? Would it be compatible with LED triggers?
Where the "pattern" class would be implemented?

>> This involves the same issue you were opposed to: three values per
>> sysfs attribute.
>
> And solves a lot of other things. Like actually being backwards
> compatible.
>
> And yes, it involves three values in a file, but now it is array of
> led brightnesses, and that might actually be acceptable. (At least the
> values have uniform meaning).
 >
> Plus, it is not "issue you were opposed to" it is "something that is
> not permitted by sysfs maintainers".

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v5 1/4] leds: core: add generic support for RGB Color LED's

[Pavel Machek]

Hi!

> >>>>It would have the same downsides as in case of having r, g and b in
> >>>>separate attributes, i.e. - problems with setting LED colour in
> >>>>a consistent way. This way LED blinking in whatever colour couldn't
> >>>>be supported reliably. It was one of your primary rationale standing
> >>>>behind this design, if I remember correctly. Second - what about
> >>>>triggers? We've had a long discussion about it and this design turned
> >>>>out to be most fitting.
> >>>
> >>>Are on/off triggers really that useful for a LED that can produce 16
> >>>million colors?
> >>>
> >>>I believe we should support patterns for RGB LEDs. Something like
> >>>[ (time, r, g, b), ... ] . Ok, what about this one?
> >>>
> >>>Lets say we have
> >>>
> >>>/sys/class/pattern/lp5533::0
> >>>/sys/class/pattern/software::0
> >>>
> >>>/sys/class/led/n900::red ; default trigger "lp5533::0:0"
> >>>/sys/class/led/n900::green ; default trigger "lp5533::0:1"
> >>>/sys/class/led/n900::blue ; default trigger "lp5533::0:2"
> >>>
> >>>Normally, pattern would correspond to one RGB LED. We could have
> >>>attribute "/sys/class/pattern/lp5533::0/color" containing R,G,B for
> >>>this pattern.
> 
> Could you give an example on how to set a color for RGB LED using
> this interface? Would it be compatible with LED triggers?
> Where the "pattern" class would be implemented?

Well, 'echo "50 60 70" > /sys/class/pattern/lp5533::0/color' should
set the color for the led. 'echo "trigger-name" > trigger' would set
the trigger, probably just toggling between LED off and set color for
the old triggers.

Where to implement the patterns is different question, but for example
drivers/leds/pattern?

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

Re: [PATCH v5 1/4] leds: core: add generic support for RGB Color LED's

[Jacek Anaszewski]

On 04/05/2016 11:01 AM, Pavel Machek wrote:
> Hi!
>
>>>>>> It would have the same downsides as in case of having r, g and b in
>>>>>> separate attributes, i.e. - problems with setting LED colour in
>>>>>> a consistent way. This way LED blinking in whatever colour couldn't
>>>>>> be supported reliably. It was one of your primary rationale standing
>>>>>> behind this design, if I remember correctly. Second - what about
>>>>>> triggers? We've had a long discussion about it and this design turned
>>>>>> out to be most fitting.
>>>>>
>>>>> Are on/off triggers really that useful for a LED that can produce 16
>>>>> million colors?
>>>>>
>>>>> I believe we should support patterns for RGB LEDs. Something like
>>>>> [ (time, r, g, b), ... ] . Ok, what about this one?
>>>>>
>>>>> Lets say we have
>>>>>
>>>>> /sys/class/pattern/lp5533::0
>>>>> /sys/class/pattern/software::0
>>>>>
>>>>> /sys/class/led/n900::red ; default trigger "lp5533::0:0"
>>>>> /sys/class/led/n900::green ; default trigger "lp5533::0:1"
>>>>> /sys/class/led/n900::blue ; default trigger "lp5533::0:2"
>>>>>
>>>>> Normally, pattern would correspond to one RGB LED. We could have
>>>>> attribute "/sys/class/pattern/lp5533::0/color" containing R,G,B for
>>>>> this pattern.
>>
>> Could you give an example on how to set a color for RGB LED using
>> this interface? Would it be compatible with LED triggers?
>> Where the "pattern" class would be implemented?
>
> Well, 'echo "50 60 70" > /sys/class/pattern/lp5533::0/color' should
> set the color for the led. 'echo "trigger-name" > trigger' would set
> the trigger, probably just toggling between LED off and set color for
> the old triggers.
>
> Where to implement the patterns is different question, but for example
> drivers/leds/pattern?

I'd rather leave the pattern issue for now, since it seems to be
different from the problem Heiner was trying to solve with his LED RGB
extension. Moreover, hardware patterns are device specific and it could
be hard to propose a generic interface.
Drivers can always expose their custom sysfs attributes for configuring
the patterns.

Regardless of the above, some of your considerations brought me an idea
on how to add generic and backwards compatible support for setting RGB
color at one go.

Currently LED class drivers of RGB LED controllers expose three LED
class devices - one per R, G and B color component. I propose that
such drivers set LED_DEV_CAP_RGB flag for each LED class device they
register. LED core, seeing the flag, would create a generic "color"
sysfs attribute for each of the three LED class devices.

The "color" attribute would contain "R G B" values. Setting the "color"
attribute of any of the three LED class devices would affect brightness
properties (i.e. constituent colors) of the remaining two ones.
It would result in disabling any active triggers and writing all the
three color settings to the RGB LED controller at one go.

We would probably need additional op in the LED core : color_set.

Having the color set to nonzero value would signify the the three LED
class devices are in sync and that setting a trigger on any of them
applies to the remaining two ones. It would have to be considered
whether existing triggers could be made compatible with synchronized
RGB LED class devices.

I'm curious what do you think about the idea.

Pavel, Heiner, others?

-- 
Best regards,
Jacek Anaszewski
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 1/4] leds: core: add generic support for RGB Color LED's

[Jacek Anaszewski]

On 04/05/2016 11:01 AM, Pavel Machek wrote:
> Hi!
>
>>>>>> It would have the same downsides as in case of having r, g and b in
>>>>>> separate attributes, i.e. - problems with setting LED colour in
>>>>>> a consistent way. This way LED blinking in whatever colour couldn't
>>>>>> be supported reliably. It was one of your primary rationale standing
>>>>>> behind this design, if I remember correctly. Second - what about
>>>>>> triggers? We've had a long discussion about it and this design turned
>>>>>> out to be most fitting.
>>>>>
>>>>> Are on/off triggers really that useful for a LED that can produce 16
>>>>> million colors?
>>>>>
>>>>> I believe we should support patterns for RGB LEDs. Something like
>>>>> [ (time, r, g, b), ... ] . Ok, what about this one?
>>>>>
>>>>> Lets say we have
>>>>>
>>>>> /sys/class/pattern/lp5533::0
>>>>> /sys/class/pattern/software::0
>>>>>
>>>>> /sys/class/led/n900::red ; default trigger "lp5533::0:0"
>>>>> /sys/class/led/n900::green ; default trigger "lp5533::0:1"
>>>>> /sys/class/led/n900::blue ; default trigger "lp5533::0:2"
>>>>>
>>>>> Normally, pattern would correspond to one RGB LED. We could have
>>>>> attribute "/sys/class/pattern/lp5533::0/color" containing R,G,B for
>>>>> this pattern.
>>
>> Could you give an example on how to set a color for RGB LED using
>> this interface? Would it be compatible with LED triggers?
>> Where the "pattern" class would be implemented?
>
> Well, 'echo "50 60 70" > /sys/class/pattern/lp5533::0/color' should
> set the color for the led. 'echo "trigger-name" > trigger' would set
> the trigger, probably just toggling between LED off and set color for
> the old triggers.
>
> Where to implement the patterns is different question, but for example
> drivers/leds/pattern?

I'd rather leave the pattern issue for now, since it seems to be
different from the problem Heiner was trying to solve with his LED RGB
extension. Moreover, hardware patterns are device specific and it could
be hard to propose a generic interface.
Drivers can always expose their custom sysfs attributes for configuring
the patterns.

Regardless of the above, some of your considerations brought me an idea
on how to add generic and backwards compatible support for setting RGB
color at one go.

Currently LED class drivers of RGB LED controllers expose three LED
class devices - one per R, G and B color component. I propose that
such drivers set LED_DEV_CAP_RGB flag for each LED class device they
register. LED core, seeing the flag, would create a generic "color"
sysfs attribute for each of the three LED class devices.

The "color" attribute would contain "R G B" values. Setting the "color"
attribute of any of the three LED class devices would affect brightness
properties (i.e. constituent colors) of the remaining two ones.
It would result in disabling any active triggers and writing all the
three color settings to the RGB LED controller at one go.

We would probably need additional op in the LED core : color_set.

Having the color set to nonzero value would signify the the three LED
class devices are in sync and that setting a trigger on any of them
applies to the remaining two ones. It would have to be considered
whether existing triggers could be made compatible with synchronized
RGB LED class devices.

I'm curious what do you think about the idea.

Pavel, Heiner, others?

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v5 1/4] leds: core: add generic support for RGB Color LED's

[Heiner Kallweit]

Am 05.04.2016 um 21:45 schrieb Jacek Anaszewski:
> On 04/05/2016 11:01 AM, Pavel Machek wrote:
>> Hi!
>>
>>>>>>> It would have the same downsides as in case of having r, g and b in
>>>>>>> separate attributes, i.e. - problems with setting LED colour in
>>>>>>> a consistent way. This way LED blinking in whatever colour couldn't
>>>>>>> be supported reliably. It was one of your primary rationale standing
>>>>>>> behind this design, if I remember correctly. Second - what about
>>>>>>> triggers? We've had a long discussion about it and this design turned
>>>>>>> out to be most fitting.
>>>>>>
>>>>>> Are on/off triggers really that useful for a LED that can produce 16
>>>>>> million colors?
>>>>>>
>>>>>> I believe we should support patterns for RGB LEDs. Something like
>>>>>> [ (time, r, g, b), ... ] . Ok, what about this one?
>>>>>>
>>>>>> Lets say we have
>>>>>>
>>>>>> /sys/class/pattern/lp5533::0
>>>>>> /sys/class/pattern/software::0
>>>>>>
>>>>>> /sys/class/led/n900::red ; default trigger "lp5533::0:0"
>>>>>> /sys/class/led/n900::green ; default trigger "lp5533::0:1"
>>>>>> /sys/class/led/n900::blue ; default trigger "lp5533::0:2"
>>>>>>
>>>>>> Normally, pattern would correspond to one RGB LED. We could have
>>>>>> attribute "/sys/class/pattern/lp5533::0/color" containing R,G,B for
>>>>>> this pattern.
>>>
>>> Could you give an example on how to set a color for RGB LED using
>>> this interface? Would it be compatible with LED triggers?
>>> Where the "pattern" class would be implemented?
>>
>> Well, 'echo "50 60 70" > /sys/class/pattern/lp5533::0/color' should
>> set the color for the led. 'echo "trigger-name" > trigger' would set
>> the trigger, probably just toggling between LED off and set color for
>> the old triggers.
>>
>> Where to implement the patterns is different question, but for example
>> drivers/leds/pattern?
> 
> I'd rather leave the pattern issue for now, since it seems to be
> different from the problem Heiner was trying to solve with his LED RGB
> extension. Moreover, hardware patterns are device specific and it could
> be hard to propose a generic interface.
> Drivers can always expose their custom sysfs attributes for configuring
> the patterns.
> 
> Regardless of the above, some of your considerations brought me an idea
> on how to add generic and backwards compatible support for setting RGB
> color at one go.
> 
> Currently LED class drivers of RGB LED controllers expose three LED
> class devices - one per R, G and B color component. I propose that
> such drivers set LED_DEV_CAP_RGB flag for each LED class device they
> register. LED core, seeing the flag, would create a generic "color"
> sysfs attribute for each of the three LED class devices.
> 
> The "color" attribute would contain "R G B" values. Setting the "color"
> attribute of any of the three LED class devices would affect brightness
> properties (i.e. constituent colors) of the remaining two ones.
> It would result in disabling any active triggers and writing all the
> three color settings to the RGB LED controller at one go.
> 
> We would probably need additional op in the LED core : color_set.
> 
> Having the color set to nonzero value would signify the the three LED
> class devices are in sync and that setting a trigger on any of them
> applies to the remaining two ones. It would have to be considered
> whether existing triggers could be made compatible with synchronized
> RGB LED class devices.
> 
> I'm curious what do you think about the idea.
> 
> Pavel, Heiner, others?
> 
Exposing "coupled LED devices" as separate LED devices most likely is ok
when accessed from user space as the name of the led_classdev's indicates
that they belong together.
But how about a trigger wanting to set a RGB LED to a specific color?
(That's not available yet but one possible use case for RGB LED's)
A trigger is bound to a led_classdev currently. In addition we'd need
to introduce some kind of super_led_classdev having links to the respective
R/G/B led_classdev's (+ trigger functions dealing with this super_led_classdev).

These changes / extensions are not needed if a RGB LED is exposed as one
led_classdev, just with flag LED_DEV_CAP_RGB set.
OK, we'd still have to change the sysfs interface as obviously setting
hue/sat/brightness via one "brightness" attribute is not acceptable.
However this constraint might not affect the kernel-internal trigger API
(usage of parameter brightness in led_trigger_event).

I see Pavel's point that there might be different types of multi-color LED's.
At least we have:
- multi-color LED's where each single LED is visible even if all are switched on
- multi-color LED's like RGB LED's where you usually just see a uniform color

Last but not least regarding the patterns:
Something like proposed by Pavel is e.g. (partially) supported by the blink(1)
firmware. That would be an example of such a "hardware-accelerated" pattern.

As I see it the current blinking support then would be one special case of a pattern.
As a consequence once having pattern support we might be able to switch users of blinking
to pattern and remove the blinking support.

Regards, Heiner

Re: [PATCH v5 1/4] leds: core: add generic support for RGB Color LED's

[Jacek Anaszewski]

Hi Heiner,

Thanks for the feedback.

On 04/05/2016 10:43 PM, Heiner Kallweit wrote:
> Am 05.04.2016 um 21:45 schrieb Jacek Anaszewski:
>> On 04/05/2016 11:01 AM, Pavel Machek wrote:
>>> Hi!
>>>
>>>>>>>> It would have the same downsides as in case of having r, g and b in
>>>>>>>> separate attributes, i.e. - problems with setting LED colour in
>>>>>>>> a consistent way. This way LED blinking in whatever colour couldn't
>>>>>>>> be supported reliably. It was one of your primary rationale standing
>>>>>>>> behind this design, if I remember correctly. Second - what about
>>>>>>>> triggers? We've had a long discussion about it and this design turned
>>>>>>>> out to be most fitting.
>>>>>>>
>>>>>>> Are on/off triggers really that useful for a LED that can produce 16
>>>>>>> million colors?
>>>>>>>
>>>>>>> I believe we should support patterns for RGB LEDs. Something like
>>>>>>> [ (time, r, g, b), ... ] . Ok, what about this one?
>>>>>>>
>>>>>>> Lets say we have
>>>>>>>
>>>>>>> /sys/class/pattern/lp5533::0
>>>>>>> /sys/class/pattern/software::0
>>>>>>>
>>>>>>> /sys/class/led/n900::red ; default trigger "lp5533::0:0"
>>>>>>> /sys/class/led/n900::green ; default trigger "lp5533::0:1"
>>>>>>> /sys/class/led/n900::blue ; default trigger "lp5533::0:2"
>>>>>>>
>>>>>>> Normally, pattern would correspond to one RGB LED. We could have
>>>>>>> attribute "/sys/class/pattern/lp5533::0/color" containing R,G,B for
>>>>>>> this pattern.
>>>>
>>>> Could you give an example on how to set a color for RGB LED using
>>>> this interface? Would it be compatible with LED triggers?
>>>> Where the "pattern" class would be implemented?
>>>
>>> Well, 'echo "50 60 70" > /sys/class/pattern/lp5533::0/color' should
>>> set the color for the led. 'echo "trigger-name" > trigger' would set
>>> the trigger, probably just toggling between LED off and set color for
>>> the old triggers.
>>>
>>> Where to implement the patterns is different question, but for example
>>> drivers/leds/pattern?
>>
>> I'd rather leave the pattern issue for now, since it seems to be
>> different from the problem Heiner was trying to solve with his LED RGB
>> extension. Moreover, hardware patterns are device specific and it could
>> be hard to propose a generic interface.
>> Drivers can always expose their custom sysfs attributes for configuring
>> the patterns.
>>
>> Regardless of the above, some of your considerations brought me an idea
>> on how to add generic and backwards compatible support for setting RGB
>> color at one go.
>>
>> Currently LED class drivers of RGB LED controllers expose three LED
>> class devices - one per R, G and B color component. I propose that
>> such drivers set LED_DEV_CAP_RGB flag for each LED class device they
>> register. LED core, seeing the flag, would create a generic "color"
>> sysfs attribute for each of the three LED class devices.
>>
>> The "color" attribute would contain "R G B" values. Setting the "color"
>> attribute of any of the three LED class devices would affect brightness
>> properties (i.e. constituent colors) of the remaining two ones.
>> It would result in disabling any active triggers and writing all the
>> three color settings to the RGB LED controller at one go.
>>
>> We would probably need additional op in the LED core : color_set.
>>
>> Having the color set to nonzero value would signify the the three LED
>> class devices are in sync and that setting a trigger on any of them
>> applies to the remaining two ones. It would have to be considered
>> whether existing triggers could be made compatible with synchronized
>> RGB LED class devices.
>>
>> I'm curious what do you think about the idea.
>>
>> Pavel, Heiner, others?
>>
 >
> Exposing "coupled LED devices" as separate LED devices most likely is ok
> when accessed from user space as the name of the led_classdev's indicates
> that they belong together.
> But how about a trigger wanting to set a RGB LED to a specific color?

RGB triggers would use a new color_set op. It means that currently
implemented triggers would be unable to set arbitrary color, but
they could be used only in a monochrome context.

> (That's not available yet but one possible use case for RGB LED's)
> A trigger is bound to a led_classdev currently. In addition we'd need
> to introduce some kind of super_led_classdev having links to the respective
> R/G/B led_classdev's (+ trigger functions dealing with this super_led_classdev).
>
> These changes / extensions are not needed if a RGB LED is exposed as one
> led_classdev, just with flag LED_DEV_CAP_RGB set.
> OK, we'd still have to change the sysfs interface as obviously setting
> hue/sat/brightness via one "brightness" attribute is not acceptable.
> However this constraint might not affect the kernel-internal trigger API
> (usage of parameter brightness in led_trigger_event).

We would still have to abuse brightness parameter semantics.

> I see Pavel's point that there might be different types of multi-color LED's.
> At least we have:
> - multi-color LED's where each single LED is visible even if all are switched on
> - multi-color LED's like RGB LED's where you usually just see a uniform color

I think that if we are talking about RGB LEDs it is always the second
case.

> Last but not least regarding the patterns:
> Something like proposed by Pavel is e.g. (partially) supported by the blink(1)
> firmware. That would be an example of such a "hardware-accelerated" pattern.
>
> As I see it the current blinking support then would be one special case of a pattern.
> As a consequence once having pattern support we might be able to switch users of blinking
> to pattern and remove the blinking support.

Let's split patterns related discussion into a separate thread.
It would be best if it began with a patch.

-- 
Best regards,
Jacek Anaszewski
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 1/4] leds: core: add generic support for RGB Color LED's

[Jacek Anaszewski]

Hi Heiner,

Thanks for the feedback.

On 04/05/2016 10:43 PM, Heiner Kallweit wrote:
> Am 05.04.2016 um 21:45 schrieb Jacek Anaszewski:
>> On 04/05/2016 11:01 AM, Pavel Machek wrote:
>>> Hi!
>>>
>>>>>>>> It would have the same downsides as in case of having r, g and b in
>>>>>>>> separate attributes, i.e. - problems with setting LED colour in
>>>>>>>> a consistent way. This way LED blinking in whatever colour couldn't
>>>>>>>> be supported reliably. It was one of your primary rationale standing
>>>>>>>> behind this design, if I remember correctly. Second - what about
>>>>>>>> triggers? We've had a long discussion about it and this design turned
>>>>>>>> out to be most fitting.
>>>>>>>
>>>>>>> Are on/off triggers really that useful for a LED that can produce 16
>>>>>>> million colors?
>>>>>>>
>>>>>>> I believe we should support patterns for RGB LEDs. Something like
>>>>>>> [ (time, r, g, b), ... ] . Ok, what about this one?
>>>>>>>
>>>>>>> Lets say we have
>>>>>>>
>>>>>>> /sys/class/pattern/lp5533::0
>>>>>>> /sys/class/pattern/software::0
>>>>>>>
>>>>>>> /sys/class/led/n900::red ; default trigger "lp5533::0:0"
>>>>>>> /sys/class/led/n900::green ; default trigger "lp5533::0:1"
>>>>>>> /sys/class/led/n900::blue ; default trigger "lp5533::0:2"
>>>>>>>
>>>>>>> Normally, pattern would correspond to one RGB LED. We could have
>>>>>>> attribute "/sys/class/pattern/lp5533::0/color" containing R,G,B for
>>>>>>> this pattern.
>>>>
>>>> Could you give an example on how to set a color for RGB LED using
>>>> this interface? Would it be compatible with LED triggers?
>>>> Where the "pattern" class would be implemented?
>>>
>>> Well, 'echo "50 60 70" > /sys/class/pattern/lp5533::0/color' should
>>> set the color for the led. 'echo "trigger-name" > trigger' would set
>>> the trigger, probably just toggling between LED off and set color for
>>> the old triggers.
>>>
>>> Where to implement the patterns is different question, but for example
>>> drivers/leds/pattern?
>>
>> I'd rather leave the pattern issue for now, since it seems to be
>> different from the problem Heiner was trying to solve with his LED RGB
>> extension. Moreover, hardware patterns are device specific and it could
>> be hard to propose a generic interface.
>> Drivers can always expose their custom sysfs attributes for configuring
>> the patterns.
>>
>> Regardless of the above, some of your considerations brought me an idea
>> on how to add generic and backwards compatible support for setting RGB
>> color at one go.
>>
>> Currently LED class drivers of RGB LED controllers expose three LED
>> class devices - one per R, G and B color component. I propose that
>> such drivers set LED_DEV_CAP_RGB flag for each LED class device they
>> register. LED core, seeing the flag, would create a generic "color"
>> sysfs attribute for each of the three LED class devices.
>>
>> The "color" attribute would contain "R G B" values. Setting the "color"
>> attribute of any of the three LED class devices would affect brightness
>> properties (i.e. constituent colors) of the remaining two ones.
>> It would result in disabling any active triggers and writing all the
>> three color settings to the RGB LED controller at one go.
>>
>> We would probably need additional op in the LED core : color_set.
>>
>> Having the color set to nonzero value would signify the the three LED
>> class devices are in sync and that setting a trigger on any of them
>> applies to the remaining two ones. It would have to be considered
>> whether existing triggers could be made compatible with synchronized
>> RGB LED class devices.
>>
>> I'm curious what do you think about the idea.
>>
>> Pavel, Heiner, others?
>>
 >
> Exposing "coupled LED devices" as separate LED devices most likely is ok
> when accessed from user space as the name of the led_classdev's indicates
> that they belong together.
> But how about a trigger wanting to set a RGB LED to a specific color?

RGB triggers would use a new color_set op. It means that currently
implemented triggers would be unable to set arbitrary color, but
they could be used only in a monochrome context.

> (That's not available yet but one possible use case for RGB LED's)
> A trigger is bound to a led_classdev currently. In addition we'd need
> to introduce some kind of super_led_classdev having links to the respective
> R/G/B led_classdev's (+ trigger functions dealing with this super_led_classdev).
>
> These changes / extensions are not needed if a RGB LED is exposed as one
> led_classdev, just with flag LED_DEV_CAP_RGB set.
> OK, we'd still have to change the sysfs interface as obviously setting
> hue/sat/brightness via one "brightness" attribute is not acceptable.
> However this constraint might not affect the kernel-internal trigger API
> (usage of parameter brightness in led_trigger_event).

We would still have to abuse brightness parameter semantics.

> I see Pavel's point that there might be different types of multi-color LED's.
> At least we have:
> - multi-color LED's where each single LED is visible even if all are switched on
> - multi-color LED's like RGB LED's where you usually just see a uniform color

I think that if we are talking about RGB LEDs it is always the second
case.

> Last but not least regarding the patterns:
> Something like proposed by Pavel is e.g. (partially) supported by the blink(1)
> firmware. That would be an example of such a "hardware-accelerated" pattern.
>
> As I see it the current blinking support then would be one special case of a pattern.
> As a consequence once having pattern support we might be able to switch users of blinking
> to pattern and remove the blinking support.

Let's split patterns related discussion into a separate thread.
It would be best if it began with a patch.

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v5 1/4] leds: core: add generic support for RGB Color LED's

[Pavel Machek]

Hi!

> >>>>>Lets say we have
> >>>>>
> >>>>>/sys/class/pattern/lp5533::0
> >>>>>/sys/class/pattern/software::0
> >>>>>
> >>>>>/sys/class/led/n900::red ; default trigger "lp5533::0:0"
> >>>>>/sys/class/led/n900::green ; default trigger "lp5533::0:1"
> >>>>>/sys/class/led/n900::blue ; default trigger "lp5533::0:2"
> >>>>>
> >>>>>Normally, pattern would correspond to one RGB LED. We could have
> >>>>>attribute "/sys/class/pattern/lp5533::0/color" containing R,G,B for
> >>>>>this pattern.
> >>
> >>Could you give an example on how to set a color for RGB LED using
> >>this interface? Would it be compatible with LED triggers?
> >>Where the "pattern" class would be implemented?
> >
> >Well, 'echo "50 60 70" > /sys/class/pattern/lp5533::0/color' should
> >set the color for the led. 'echo "trigger-name" > trigger' would set
> >the trigger, probably just toggling between LED off and set color for
> >the old triggers.
> >
> >Where to implement the patterns is different question, but for example
> >drivers/leds/pattern?
> 
> I'd rather leave the pattern issue for now, since it seems to be
> different from the problem Heiner was trying to solve with his LED RGB
> extension. Moreover, hardware patterns are device specific and it could
> be hard to propose a generic interface.

Well, RGB leds are basically useless without pattern support. And I
believe we can do generic interface.

> Drivers can always expose their custom sysfs attributes for configuring
> the patterns.
> 
> Regardless of the above, some of your considerations brought me an idea
> on how to add generic and backwards compatible support for setting RGB
> color at one go.
> 
> Currently LED class drivers of RGB LED controllers expose three LED
> class devices - one per R, G and B color component. I propose that
> such drivers set LED_DEV_CAP_RGB flag for each LED class device they
> register. LED core, seeing the flag, would create a generic "color"
> sysfs attribute for each of the three LED class devices.
> 
> The "color" attribute would contain "R G B" values. Setting the "color"
> attribute of any of the three LED class devices would affect brightness
> properties (i.e. constituent colors) of the remaining two ones.
> It would result in disabling any active triggers and writing all the
> three color settings to the RGB LED controller at one go.

Having one attribute across three devices is rather ugly. And we'll
need to solve the pattern issue one day.

What's tricky about patterns is that you need to control 3 (or more)
leds at a time. Problem you are trying to solve here is ... control of
3 leds, at the same time.

So let's solve them together.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH v5 1/4] leds: core: add generic support for RGB Color LED's

[Pavel Machek]

Hi!

> > We would probably need additional op in the LED core : color_set.
> > 
> > Having the color set to nonzero value would signify the the three LED
> > class devices are in sync and that setting a trigger on any of them
> > applies to the remaining two ones. It would have to be considered
> > whether existing triggers could be made compatible with synchronized
> > RGB LED class devices.
> > 
> > I'm curious what do you think about the idea.
> > 
> > Pavel, Heiner, others?
> > 
> Exposing "coupled LED devices" as separate LED devices most likely is ok
> when accessed from user space as the name of the led_classdev's indicates
> that they belong together.
> But how about a trigger wanting to set a RGB LED to a specific color?
> (That's not available yet but one possible use case for RGB LED's)
> A trigger is bound to a led_classdev currently. In addition we'd need
> to introduce some kind of super_led_classdev having links to the respective
> R/G/B led_classdev's (+ trigger functions dealing with this super_led_classdev).
> 
> These changes / extensions are not needed if a RGB LED is exposed as one
> led_classdev, just with flag LED_DEV_CAP_RGB set.
> OK, we'd still have to change the sysfs interface as obviously setting
> hue/sat/brightness via one "brightness" attribute is not acceptable.
> However this constraint might not affect the kernel-internal trigger API
> (usage of parameter brightness in led_trigger_event).

Your proposal would break existing hardware. We already have RGB LEDs
exposed as three LEDs. It is too late to change interface there.

> I see Pavel's point that there might be different types of multi-color LED's.
> At least we have:
> - multi-color LED's where each single LED is visible even if all are switched on
> - multi-color LED's like RGB LED's where you usually just see a
> uniform color

Well, I suggest we ignore that distinction. Yes, I can see different
colors coming from different directions, but the LED was clearly
designed to look like single light. 

> Last but not least regarding the patterns:
> Something like proposed by Pavel is e.g. (partially) supported by the blink(1)
> firmware. That would be an example of such a "hardware-accelerated" pattern.
> 
> As I see it the current blinking support then would be one special case of a pattern.
> As a consequence once having pattern support we might be able to switch users of blinking
> to pattern and remove the blinking support.

No, you can't remove existing blinking support, due to backwards
compatibility reasons.

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

Re: [PATCH v5 1/4] leds: core: add generic support for RGB Color LED's

[Pavel Machek]

Hi!

> >As I see it the current blinking support then would be one special case of a pattern.
> >As a consequence once having pattern support we might be able to switch users of blinking
> >to pattern and remove the blinking support.
> 
> Let's split patterns related discussion into a separate thread.
> It would be best if it began with a patch.

Lets design userland interface first, then decide how to implement it
in the kernel. Patches are useless at this point.

And actually... without patterns, existing interface works just
fine. Even if you can't "atomically" write values to three different
files, operation is so fast that user will not see the intermediate
state, anyway. So solving the "atomic" issue without solving the rest
is pretty much useless.

Best regards,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 1/4] leds: core: add generic support for RGB Color LED's

[Pavel Machek]

Hi!

> >As I see it the current blinking support then would be one special case of a pattern.
> >As a consequence once having pattern support we might be able to switch users of blinking
> >to pattern and remove the blinking support.
> 
> Let's split patterns related discussion into a separate thread.
> It would be best if it began with a patch.

Lets design userland interface first, then decide how to implement it
in the kernel. Patches are useless at this point.

And actually... without patterns, existing interface works just
fine. Even if you can't "atomically" write values to three different
files, operation is so fast that user will not see the intermediate
state, anyway. So solving the "atomic" issue without solving the rest
is pretty much useless.

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

Re: [PATCH v5 1/4] leds: core: add generic support for RGB Color LED's

[Jacek Anaszewski]

On 04/06/2016 10:52 AM, Pavel Machek wrote:
> Hi!
>
>>>>>>> Lets say we have
>>>>>>>
>>>>>>> /sys/class/pattern/lp5533::0
>>>>>>> /sys/class/pattern/software::0
>>>>>>>
>>>>>>> /sys/class/led/n900::red ; default trigger "lp5533::0:0"
>>>>>>> /sys/class/led/n900::green ; default trigger "lp5533::0:1"
>>>>>>> /sys/class/led/n900::blue ; default trigger "lp5533::0:2"
>>>>>>>
>>>>>>> Normally, pattern would correspond to one RGB LED. We could have
>>>>>>> attribute "/sys/class/pattern/lp5533::0/color" containing R,G,B for
>>>>>>> this pattern.
>>>>
>>>> Could you give an example on how to set a color for RGB LED using
>>>> this interface? Would it be compatible with LED triggers?
>>>> Where the "pattern" class would be implemented?
>>>
>>> Well, 'echo "50 60 70" > /sys/class/pattern/lp5533::0/color' should
>>> set the color for the led. 'echo "trigger-name" > trigger' would set
>>> the trigger, probably just toggling between LED off and set color for
>>> the old triggers.
>>>
>>> Where to implement the patterns is different question, but for example
>>> drivers/leds/pattern?
>>
>> I'd rather leave the pattern issue for now, since it seems to be
>> different from the problem Heiner was trying to solve with his LED RGB
>> extension. Moreover, hardware patterns are device specific and it could
>> be hard to propose a generic interface.
>
> Well, RGB leds are basically useless without pattern support. And I
> believe we can do generic interface.
>
>> Drivers can always expose their custom sysfs attributes for configuring
>> the patterns.
>>
>> Regardless of the above, some of your considerations brought me an idea
>> on how to add generic and backwards compatible support for setting RGB
>> color at one go.
>>
>> Currently LED class drivers of RGB LED controllers expose three LED
>> class devices - one per R, G and B color component. I propose that
>> such drivers set LED_DEV_CAP_RGB flag for each LED class device they
>> register. LED core, seeing the flag, would create a generic "color"
>> sysfs attribute for each of the three LED class devices.
>>
>> The "color" attribute would contain "R G B" values. Setting the "color"
>> attribute of any of the three LED class devices would affect brightness
>> properties (i.e. constituent colors) of the remaining two ones.
>> It would result in disabling any active triggers and writing all the
>> three color settings to the RGB LED controller at one go.
>
> Having one attribute across three devices is rather ugly. And we'll
> need to solve the pattern issue one day.
>
> What's tricky about patterns is that you need to control 3 (or more)
> leds at a time. Problem you are trying to solve here is ... control of
> 3 leds, at the same time.
>
> So let's solve them together.

OK, now I've got your point. So we'd need to have a means for defining
patterns. The interface could be located at /sys/class/leds/patterns.

We'd need to have a flexible way for defining LED class devices involved
in a pattern. Since we cannot guarantee no space in a LED class device
name, then a single attribute containing space separated list is not an
option. We'd have to create a predefined set of attributes that would
contain LED class device name. Predefined implies that it would be
a fixed number, i.e. either some attributes would always remain unused
or, which is even worse, we could run out of free attributes for some
use cases.

The same constraints would appear if we wanted to be able to define
more than one pattern.

It would be best to work out more flexible solution. I wonder if
ioctl interface isn't the only option.

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v5 1/4] leds: core: add generic support for RGB Color LED's

[Pavel Machek]

Hi!

> >>The "color" attribute would contain "R G B" values. Setting the "color"
> >>attribute of any of the three LED class devices would affect brightness
> >>properties (i.e. constituent colors) of the remaining two ones.
> >>It would result in disabling any active triggers and writing all the
> >>three color settings to the RGB LED controller at one go.
> >
> >Having one attribute across three devices is rather ugly. And we'll
> >need to solve the pattern issue one day.
> >
> >What's tricky about patterns is that you need to control 3 (or more)
> >leds at a time. Problem you are trying to solve here is ... control of
> >3 leds, at the same time.
> >
> >So let's solve them together.
> 
> OK, now I've got your point. So we'd need to have a means for defining
> patterns. The interface could be located at /sys/class/leds/patterns.
> 
> We'd need to have a flexible way for defining LED class devices involved
> in a pattern. Since we cannot guarantee no space in a LED class device
> name, then a single attribute containing space separated list is not an
> option. We'd have to create a predefined set of attributes that would
> contain LED class device name. Predefined implies that it would be
> a fixed number, i.e. either some attributes would always remain unused
> or, which is even worse, we could run out of free attributes for some
> use cases.

There's a better solution: make pattern behave as a trigger for leds
it controls.

So we'd have

/sys/class/leds/patterns/lp5523

then we'd have

/sys/class/leds/lp5523::red/trigger = "lp5523:1"
/sys/class/leds/lp5523::green/trigger = "lp5523:2"
/sys/class/leds/lp5523::blue/trigger = "lp5523:3"

(or something similar, I'd have to boot the n900 to see the exact
names).

That means that we don't need space-separated lists. (And actually
gives us more flexibility; Maemo for example used the pattern engine
not for RGB led, but for 6 keyboard backlight leds.)

> The same constraints would appear if we wanted to be able to define
> more than one pattern.

We'd like to have more than one pattern _engine_, but it should be
enough to have one pattern per pattern engine at a time. 

> It would be best to work out more flexible solution. I wonder if
> ioctl interface isn't the only option.

Well, there's configs, which is more flexible, but...

Best regards,

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

Re: [PATCH v5 1/4] leds: core: add generic support for RGB Color LED's

[Jacek Anaszewski]

On 04/07/2016 10:45 PM, Pavel Machek wrote:
> Hi!
>
>>>> The "color" attribute would contain "R G B" values. Setting the "color"
>>>> attribute of any of the three LED class devices would affect brightness
>>>> properties (i.e. constituent colors) of the remaining two ones.
>>>> It would result in disabling any active triggers and writing all the
>>>> three color settings to the RGB LED controller at one go.
>>>
>>> Having one attribute across three devices is rather ugly. And we'll
>>> need to solve the pattern issue one day.
>>>
>>> What's tricky about patterns is that you need to control 3 (or more)
>>> leds at a time. Problem you are trying to solve here is ... control of
>>> 3 leds, at the same time.
>>>
>>> So let's solve them together.
>>
>> OK, now I've got your point. So we'd need to have a means for defining
>> patterns. The interface could be located at /sys/class/leds/patterns.
>>
>> We'd need to have a flexible way for defining LED class devices involved
>> in a pattern. Since we cannot guarantee no space in a LED class device
>> name, then a single attribute containing space separated list is not an
>> option. We'd have to create a predefined set of attributes that would
>> contain LED class device name. Predefined implies that it would be
>> a fixed number, i.e. either some attributes would always remain unused
>> or, which is even worse, we could run out of free attributes for some
>> use cases.
>
> There's a better solution: make pattern behave as a trigger for leds
> it controls.
>
> So we'd have
>
> /sys/class/leds/patterns/lp5523
>
> then we'd have
>
> /sys/class/leds/lp5523::red/trigger = "lp5523:1"
> /sys/class/leds/lp5523::green/trigger = "lp5523:2"
> /sys/class/leds/lp5523::blue/trigger = "lp5523:3"
>
> (or something similar, I'd have to boot the n900 to see the exact
> names).

How about implementing patterns as a specific typer of triggers?
Let's say we have ledtrig-rgb-pattern:

After setting a trigger following sysfs attribute would appear
in a LED class device sysfs interface:

$cat /sys/class/lp5523::red/rgb_color
red green blue [none]

$echo "red" > /sys/class/leds/lp5523::red/rgb_color

and similarly

$echo "green" > /sys/class/leds/lp5523::green/rgb_color
$echo "blue" > /sys/class/leds/lp5523::blue/rgb_color

Similar approach could be applied for blink patterns:
There could be additional attributes provided for defining
the position in a blink sequence, or/and blink period.

Now it has become apparent to me that triggers in fact assure
LED class device synchronization.

> That means that we don't need space-separated lists. (And actually
> gives us more flexibility; Maemo for example used the pattern engine
> not for RGB led, but for 6 keyboard backlight leds.)
>
>> The same constraints would appear if we wanted to be able to define
>> more than one pattern.
>
> We'd like to have more than one pattern _engine_, but it should be
> enough to have one pattern per pattern engine at a time.
>
>> It would be best to work out more flexible solution. I wonder if
>> ioctl interface isn't the only option.
>
> Well, there's configs, which is more flexible, but...
>
> Best regards,
>
> 									Pavel
>

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v5 1/4] leds: core: add generic support for RGB Color LED's

[Pavel Machek]

Hi!

> >>>What's tricky about patterns is that you need to control 3 (or more)
> >>>leds at a time. Problem you are trying to solve here is ... control of
> >>>3 leds, at the same time.
> >>>
> >>>So let's solve them together.
> >>
> >>OK, now I've got your point. So we'd need to have a means for defining
> >>patterns. The interface could be located at /sys/class/leds/patterns.
> >>
> >>We'd need to have a flexible way for defining LED class devices involved
> >>in a pattern. Since we cannot guarantee no space in a LED class device
> >>name, then a single attribute containing space separated list is not an
> >>option. We'd have to create a predefined set of attributes that would
> >>contain LED class device name. Predefined implies that it would be
> >>a fixed number, i.e. either some attributes would always remain unused
> >>or, which is even worse, we could run out of free attributes for some
> >>use cases.
> >
> >There's a better solution: make pattern behave as a trigger for leds
> >it controls.
> >
> >So we'd have
> >
> >/sys/class/leds/patterns/lp5523
> >
> >then we'd have
> >
> >/sys/class/leds/lp5523::red/trigger = "lp5523:1"
> >/sys/class/leds/lp5523::green/trigger = "lp5523:2"
> >/sys/class/leds/lp5523::blue/trigger = "lp5523:3"
> >
> >(or something similar, I'd have to boot the n900 to see the exact
> >names).
> 
> How about implementing patterns as a specific typer of triggers?
> Let's say we have ledtrig-rgb-pattern:

Well, we'd need ledtrig-rgb-pattern-1, ledtrig-rgb-pattern-2, ... , as we
can have more than one rgb led. But yes.

> After setting a trigger following sysfs attribute would appear
> in a LED class device sysfs interface:
> 
> $cat /sys/class/lp5523::red/rgb_color
> red green blue [none]
> 
> $echo "red" > /sys/class/leds/lp5523::red/rgb_color
> 
> and similarly
> 
> $echo "green" > /sys/class/leds/lp5523::green/rgb_color
> $echo "blue" > /sys/class/leds/lp5523::blue/rgb_color

Yes, that would work -- selecting channels from the pattern.

> Similar approach could be applied for blink patterns:
> There could be additional attributes provided for defining
> the position in a blink sequence, or/and blink period.

For patterns, I'd suggest array of (r g b time) values.

Pattern engines can do stuff like "slowly turn LED from off to red, then switch color to
white, then slowly turn it to yellow, then turn it off at once" with defined speeds
for "slowly" and option of either linear on non-linear brightness ramping.

The last option might be a bit too much, but I believe we should support the rest.

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

Re: [PATCH v5 1/4] leds: core: add generic support for RGB Color LED's

[Jacek Anaszewski]

On 04/09/2016 06:01 PM, Pavel Machek wrote:
> Hi!
>
>>>>> What's tricky about patterns is that you need to control 3 (or more)
>>>>> leds at a time. Problem you are trying to solve here is ... control of
>>>>> 3 leds, at the same time.
>>>>>
>>>>> So let's solve them together.
>>>>
>>>> OK, now I've got your point. So we'd need to have a means for defining
>>>> patterns. The interface could be located at /sys/class/leds/patterns.
>>>>
>>>> We'd need to have a flexible way for defining LED class devices involved
>>>> in a pattern. Since we cannot guarantee no space in a LED class device
>>>> name, then a single attribute containing space separated list is not an
>>>> option. We'd have to create a predefined set of attributes that would
>>>> contain LED class device name. Predefined implies that it would be
>>>> a fixed number, i.e. either some attributes would always remain unused
>>>> or, which is even worse, we could run out of free attributes for some
>>>> use cases.
>>>
>>> There's a better solution: make pattern behave as a trigger for leds
>>> it controls.
>>>
>>> So we'd have
>>>
>>> /sys/class/leds/patterns/lp5523
>>>
>>> then we'd have
>>>
>>> /sys/class/leds/lp5523::red/trigger = "lp5523:1"
>>> /sys/class/leds/lp5523::green/trigger = "lp5523:2"
>>> /sys/class/leds/lp5523::blue/trigger = "lp5523:3"
>>>
>>> (or something similar, I'd have to boot the n900 to see the exact
>>> names).
>>
>> How about implementing patterns as a specific typer of triggers?
>> Let's say we have ledtrig-rgb-pattern:
>
> Well, we'd need ledtrig-rgb-pattern-1, ledtrig-rgb-pattern-2, ... , as we
> can have more than one rgb led. But yes.

Triggers can have many listeners, i.e. led_trigger_event() sets
brightness on all LED class devices registered on given trigger.
We could have led_trigger_rgb_event() that would set brightness
on all groups-of-three LEDs registered on given rgb-trigger.

I agree that ledtrig-rgb-pattern-1, ledtrig-rgb-pattern-2, etc. would
be also needed to add a capability of setting different colors on
different LED devices.

>> After setting a trigger following sysfs attribute would appear
>> in a LED class device sysfs interface:
>>
>> $cat /sys/class/lp5523::red/rgb_color
>> red green blue [none]
>>
>> $echo "red" > /sys/class/leds/lp5523::red/rgb_color
>>
>> and similarly
>>
>> $echo "green" > /sys/class/leds/lp5523::green/rgb_color
>> $echo "blue" > /sys/class/leds/lp5523::blue/rgb_color
>
> Yes, that would work -- selecting channels from the pattern.
>
>> Similar approach could be applied for blink patterns:
>> There could be additional attributes provided for defining
>> the position in a blink sequence, or/and blink period.
>
> For patterns, I'd suggest array of (r g b time) values.
>
> Pattern engines can do stuff like "slowly turn LED from off to red, then switch color to
> white, then slowly turn it to yellow, then turn it off at once" with defined speeds
> for "slowly" and option of either linear on non-linear brightness ramping.
>
> The last option might be a bit too much, but I believe we should support the rest.

Yes, that's an interesting idea. It also turns out that trigger based
patterns could be also used for defining generic patterns for a group
of monochrome LEDs.

-- 
Best regards,
Jacek Anaszewski
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 1/4] leds: core: add generic support for RGB Color LED's

[Jacek Anaszewski]

On 04/09/2016 06:01 PM, Pavel Machek wrote:
> Hi!
>
>>>>> What's tricky about patterns is that you need to control 3 (or more)
>>>>> leds at a time. Problem you are trying to solve here is ... control of
>>>>> 3 leds, at the same time.
>>>>>
>>>>> So let's solve them together.
>>>>
>>>> OK, now I've got your point. So we'd need to have a means for defining
>>>> patterns. The interface could be located at /sys/class/leds/patterns.
>>>>
>>>> We'd need to have a flexible way for defining LED class devices involved
>>>> in a pattern. Since we cannot guarantee no space in a LED class device
>>>> name, then a single attribute containing space separated list is not an
>>>> option. We'd have to create a predefined set of attributes that would
>>>> contain LED class device name. Predefined implies that it would be
>>>> a fixed number, i.e. either some attributes would always remain unused
>>>> or, which is even worse, we could run out of free attributes for some
>>>> use cases.
>>>
>>> There's a better solution: make pattern behave as a trigger for leds
>>> it controls.
>>>
>>> So we'd have
>>>
>>> /sys/class/leds/patterns/lp5523
>>>
>>> then we'd have
>>>
>>> /sys/class/leds/lp5523::red/trigger = "lp5523:1"
>>> /sys/class/leds/lp5523::green/trigger = "lp5523:2"
>>> /sys/class/leds/lp5523::blue/trigger = "lp5523:3"
>>>
>>> (or something similar, I'd have to boot the n900 to see the exact
>>> names).
>>
>> How about implementing patterns as a specific typer of triggers?
>> Let's say we have ledtrig-rgb-pattern:
>
> Well, we'd need ledtrig-rgb-pattern-1, ledtrig-rgb-pattern-2, ... , as we
> can have more than one rgb led. But yes.

Triggers can have many listeners, i.e. led_trigger_event() sets
brightness on all LED class devices registered on given trigger.
We could have led_trigger_rgb_event() that would set brightness
on all groups-of-three LEDs registered on given rgb-trigger.

I agree that ledtrig-rgb-pattern-1, ledtrig-rgb-pattern-2, etc. would
be also needed to add a capability of setting different colors on
different LED devices.

>> After setting a trigger following sysfs attribute would appear
>> in a LED class device sysfs interface:
>>
>> $cat /sys/class/lp5523::red/rgb_color
>> red green blue [none]
>>
>> $echo "red" > /sys/class/leds/lp5523::red/rgb_color
>>
>> and similarly
>>
>> $echo "green" > /sys/class/leds/lp5523::green/rgb_color
>> $echo "blue" > /sys/class/leds/lp5523::blue/rgb_color
>
> Yes, that would work -- selecting channels from the pattern.
>
>> Similar approach could be applied for blink patterns:
>> There could be additional attributes provided for defining
>> the position in a blink sequence, or/and blink period.
>
> For patterns, I'd suggest array of (r g b time) values.
>
> Pattern engines can do stuff like "slowly turn LED from off to red, then switch color to
> white, then slowly turn it to yellow, then turn it off at once" with defined speeds
> for "slowly" and option of either linear on non-linear brightness ramping.
>
> The last option might be a bit too much, but I believe we should support the rest.

Yes, that's an interesting idea. It also turns out that trigger based
patterns could be also used for defining generic patterns for a group
of monochrome LEDs.

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v5 1/4] leds: core: add generic support for RGB Color LED's

[Pavel Machek]

Hi!

> >>How about implementing patterns as a specific typer of triggers?
> >>Let's say we have ledtrig-rgb-pattern:
> >
> >Well, we'd need ledtrig-rgb-pattern-1, ledtrig-rgb-pattern-2, ... , as we
> >can have more than one rgb led. But yes.
> 
> Triggers can have many listeners, i.e. led_trigger_event() sets
> brightness on all LED class devices registered on given trigger.
> We could have led_trigger_rgb_event() that would set brightness
> on all groups-of-three LEDs registered on given rgb-trigger.

I do not understand that.

> I agree that ledtrig-rgb-pattern-1, ledtrig-rgb-pattern-2, etc. would
> be also needed to add a capability of setting different colors on
> different LED devices.

Ok.

> >For patterns, I'd suggest array of (r g b time) values.
> >
> >Pattern engines can do stuff like "slowly turn LED from off to red, then switch color to
> >white, then slowly turn it to yellow, then turn it off at once" with defined speeds
> >for "slowly" and option of either linear on non-linear brightness ramping.
> >
> >The last option might be a bit too much, but I believe we should support the rest.
> 
> Yes, that's an interesting idea. It also turns out that trigger based
> patterns could be also used for defining generic patterns for a group
> of monochrome LEDs.

Yes, controlling monochrome LEDs synchronously is another task for
patterns. Actually, N900 uses that to control 6 keyboard backlight
LEDs synchronously... and yes, it would be somehow nice to preserve
this functionality.

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

Re: [PATCH v5 1/4] leds: core: add generic support for RGB Color LED's

[Jacek Anaszewski]

Hi Pavel,

On 04/15/2016 01:53 PM, Pavel Machek wrote:
> Hi!
>
>>>> How about implementing patterns as a specific typer of triggers?
>>>> Let's say we have ledtrig-rgb-pattern:
>>>
>>> Well, we'd need ledtrig-rgb-pattern-1, ledtrig-rgb-pattern-2, ... , as we
>>> can have more than one rgb led. But yes.
>>
>> Triggers can have many listeners, i.e. led_trigger_event() sets
>> brightness on all LED class devices registered on given trigger.
>> We could have led_trigger_rgb_event() that would set brightness
>> on all groups-of-three LEDs registered on given rgb-trigger.
>
> I do not understand that.

Triggers are defined as kernel source of led events.

Currently we have two types of triggers as far as the source of
led event is concerned:
- triggers that are created per LED class device and therefore each
   LED class device has their own source of kernel event,
   initialized on trigger activation (e.g. ledtrig-timer,
   ledtrig-heartbeat, ledtrig-oneshot),
- triggers that propagate kernel events from one source to many
   listeners (e.g.ledtrig-ide-disk, ledtrig-cpu) - they internally
   use led_trigger_event(), which iterates through all LED class devices
   registered on a trigger and applies the same brightness.

In case of RGB trigger we'd like to have a common source of kernel
events for three LED class devices. After deeper analysis I'd modify
the approach a bit, in order to add a capability of generating kernel
led events from user space.

Let's say that we have LED RGB driver that exposes three LED class
devices: lp5523:red, lp5523:green, lp5523 blue.

$echo "rgb-pattern-lp5523" > /sys/class/leds/lp5523::red/trigger
$echo "red" > /sys/class/leds/lp5523::red/rgb_color
$echo "rgb-pattern-lp5523" > /sys/class/leds/lp5523::green/trigger
$echo "green" > /sys/class/leds/lp5523::green/rgb_color
$echo "rgb-pattern-lp5523" > /sys/class/leds/lp5523::blue/trigger
$echo "blue" > /sys/class/leds/lp5523::blue/rgb_color

led-rgb-pattern trigger would create a new trigger each time a non
existing rgp-pattern-* suffix is passed. In order to make it possible
for the user space to generate trigger events, a dedicated sysfs
interface would have to be exposed. How about creating a new LED RGB
class device that would expose "color" sysfs attribute with three space
separated R G B values? The device would appear in the sysfs after
rgb-pattern trigger is created.

Internally the LED RGB class device would use a new
led_trigger_rgb_event() to set the color:


void led_trigger_rgb_event(struct led_trigger *trig,
             enum led_brightness red,
             enum led_brightness green,
             enum led_brightness blue,
{
         struct led_classdev *led_cdev;

         if (!trig)
             return;

         read_lock(&trig->leddev_list_lock);
         list_for_each_entry(led_cdev, &trig->led_cdevs, trig_list)
                if (led_cdev>flags & LED_RGB_COLOR_R)
                    led_set_brightness(led_cdev, red);
                else if (led_cdev>flags & LED_RGB_COLOR_G)
                        led_set_brightness(led_cdev, green);
                else if (led_cdev>flags & LED_RGB_COLOR_B)
                        led_set_brightness(led_cdev, blue);
         read_unlock(&trig->leddev_list_lock);
}



>
>> I agree that ledtrig-rgb-pattern-1, ledtrig-rgb-pattern-2, etc. would
>> be also needed to add a capability of setting different colors on
>> different LED devices.
>
> Ok.
>
>>> For patterns, I'd suggest array of (r g b time) values.
>>>
>>> Pattern engines can do stuff like "slowly turn LED from off to red, then switch color to
>>> white, then slowly turn it to yellow, then turn it off at once" with defined speeds
>>> for "slowly" and option of either linear on non-linear brightness ramping.
>>>
>>> The last option might be a bit too much, but I believe we should support the rest.
>>
>> Yes, that's an interesting idea. It also turns out that trigger based
>> patterns could be also used for defining generic patterns for a group
>> of monochrome LEDs.
>
> Yes, controlling monochrome LEDs synchronously is another task for
> patterns. Actually, N900 uses that to control 6 keyboard backlight
> LEDs synchronously... and yes, it would be somehow nice to preserve
> this functionality.


-- 
Best regards,
Jacek Anaszewski
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 1/4] leds: core: add generic support for RGB Color LED's

[Jacek Anaszewski]

Hi Pavel,

On 04/15/2016 01:53 PM, Pavel Machek wrote:
> Hi!
>
>>>> How about implementing patterns as a specific typer of triggers?
>>>> Let's say we have ledtrig-rgb-pattern:
>>>
>>> Well, we'd need ledtrig-rgb-pattern-1, ledtrig-rgb-pattern-2, ... , as we
>>> can have more than one rgb led. But yes.
>>
>> Triggers can have many listeners, i.e. led_trigger_event() sets
>> brightness on all LED class devices registered on given trigger.
>> We could have led_trigger_rgb_event() that would set brightness
>> on all groups-of-three LEDs registered on given rgb-trigger.
>
> I do not understand that.

Triggers are defined as kernel source of led events.

Currently we have two types of triggers as far as the source of
led event is concerned:
- triggers that are created per LED class device and therefore each
   LED class device has their own source of kernel event,
   initialized on trigger activation (e.g. ledtrig-timer,
   ledtrig-heartbeat, ledtrig-oneshot),
- triggers that propagate kernel events from one source to many
   listeners (e.g.ledtrig-ide-disk, ledtrig-cpu) - they internally
   use led_trigger_event(), which iterates through all LED class devices
   registered on a trigger and applies the same brightness.

In case of RGB trigger we'd like to have a common source of kernel
events for three LED class devices. After deeper analysis I'd modify
the approach a bit, in order to add a capability of generating kernel
led events from user space.

Let's say that we have LED RGB driver that exposes three LED class
devices: lp5523:red, lp5523:green, lp5523 blue.

$echo "rgb-pattern-lp5523" > /sys/class/leds/lp5523::red/trigger
$echo "red" > /sys/class/leds/lp5523::red/rgb_color
$echo "rgb-pattern-lp5523" > /sys/class/leds/lp5523::green/trigger
$echo "green" > /sys/class/leds/lp5523::green/rgb_color
$echo "rgb-pattern-lp5523" > /sys/class/leds/lp5523::blue/trigger
$echo "blue" > /sys/class/leds/lp5523::blue/rgb_color

led-rgb-pattern trigger would create a new trigger each time a non
existing rgp-pattern-* suffix is passed. In order to make it possible
for the user space to generate trigger events, a dedicated sysfs
interface would have to be exposed. How about creating a new LED RGB
class device that would expose "color" sysfs attribute with three space
separated R G B values? The device would appear in the sysfs after
rgb-pattern trigger is created.

Internally the LED RGB class device would use a new
led_trigger_rgb_event() to set the color:


void led_trigger_rgb_event(struct led_trigger *trig,
             enum led_brightness red,
             enum led_brightness green,
             enum led_brightness blue,
{
         struct led_classdev *led_cdev;

         if (!trig)
             return;

         read_lock(&trig->leddev_list_lock);
         list_for_each_entry(led_cdev, &trig->led_cdevs, trig_list)
                if (led_cdev>flags & LED_RGB_COLOR_R)
                    led_set_brightness(led_cdev, red);
                else if (led_cdev>flags & LED_RGB_COLOR_G)
                        led_set_brightness(led_cdev, green);
                else if (led_cdev>flags & LED_RGB_COLOR_B)
                        led_set_brightness(led_cdev, blue);
         read_unlock(&trig->leddev_list_lock);
}



>
>> I agree that ledtrig-rgb-pattern-1, ledtrig-rgb-pattern-2, etc. would
>> be also needed to add a capability of setting different colors on
>> different LED devices.
>
> Ok.
>
>>> For patterns, I'd suggest array of (r g b time) values.
>>>
>>> Pattern engines can do stuff like "slowly turn LED from off to red, then switch color to
>>> white, then slowly turn it to yellow, then turn it off at once" with defined speeds
>>> for "slowly" and option of either linear on non-linear brightness ramping.
>>>
>>> The last option might be a bit too much, but I believe we should support the rest.
>>
>> Yes, that's an interesting idea. It also turns out that trigger based
>> patterns could be also used for defining generic patterns for a group
>> of monochrome LEDs.
>
> Yes, controlling monochrome LEDs synchronously is another task for
> patterns. Actually, N900 uses that to control 6 keyboard backlight
> LEDs synchronously... and yes, it would be somehow nice to preserve
> this functionality.


-- 
Best regards,
Jacek Anaszewski