Proposal: Add color_temperature support

Proposal: Add color_temperature support

[Andreas Bergmeier]

On the device Logitech Litra Glow it is possible to set not only the
brightness but also the color temperature (both via usb as well as via
hardware).
I am currently trying to add support for the device - and expose it
via LED - into the kernel.
So to support all device capabilities I am proposing to extend LEDs by
color temperature:
And initial patch for the headers:

 diff --git a/include/linux/leds.h b/include/linux/leds.h
index ba4861ec7..494eab49b 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -72,6 +72,9 @@ struct led_classdev {
unsigned int brightness;
unsigned int max_brightness;
int flags;
+ unsigned int color_temperature;
+ unsigned int min_color_temperature;
+ unsigned int max_color_temperature;
/* Lower 16 bits reflect status */
#define LED_SUSPENDED BIT(0)
@@ -123,6 +126,21 @@ struct led_classdev {
unsigned long *delay_on,
unsigned long *delay_off);
+ /* Set LED color temperature
+ * Must not sleep. Use color_temperature_set_blocking for drivers
+ * that can sleep while setting color temperature.
+ */
+ void (*color_temperature_set)(struct led_classdev *led_cdev,
+ unsigned int color_temperature);
+ /*
+ * Set LED color temperature immediately - it can block the caller for
+ * the time required for accessing a LED device register.
+ */
+ int (*color_temperature_set_blocking)(struct led_classdev *led_cdev,
+ unsigned int color_temperature);
+ /* Get LED color temperature */
+ unsigned int (*color_temperature_get)(struct led_classdev *led_cdev);
+
int (*pattern_set)(struct led_classdev *led_cdev,
struct led_pattern *pattern, u32 len, int repeat);
int (*pattern_clear)(struct led_classdev *led_cdev);
@@ -140,6 +158,7 @@ struct led_classdev {
void (*flash_resume)(struct led_classdev *led_cdev);
struct work_struct set_brightness_work;
+ struct work_struct set_color_temperature_work;
int delayed_set_value;
#ifdef CONFIG_LEDS_TRIGGERS
@@ -160,6 +179,10 @@ struct led_classdev {
int brightness_hw_changed;
struct kernfs_node *brightness_hw_changed_kn;
#endif
+#ifdef CONFIG_LEDS_COLOR_TEMPERATURE_HW_CHANGED
+ int color_temperature_hw_changed;
+ struct kernfs_node *color_temperature_hw_changed_kn;
+#endif
/* Ensures consistent access to the LED Flash Class device */
struct mutex led_access;
@@ -574,6 +597,14 @@ void led_classdev_notify_brightness_hw_changed(
static inline void led_classdev_notify_brightness_hw_changed(
struct led_classdev *led_cdev, enum led_brightness brightness) { }
#endif
+#ifdef CONFIG_LEDS_COLOR_TEMPERATURE_HW_CHANGED
+void led_classdev_notify_color_temperature_hw_changed(
+ struct led_classdev *led_cdev, unsigned int color_temperature);
+#else
+static inline void led_classdev_notify_color_temperature_hw_changed(
+ struct led_classdev *led_cdev, unsigned int color_temperature) { }
+#endif
+
/**
* struct led_pattern - pattern interval settings

What do you think?
Cheers

Re: Proposal: Add color_temperature support

[Pavel Machek]

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

Hi!

> On the device Logitech Litra Glow it is possible to set not only the
> brightness but also the color temperature (both via usb as well as via
> hardware).
> I am currently trying to add support for the device - and expose it
> via LED - into the kernel.
> So to support all device capabilities I am proposing to extend LEDs by
> color temperature:
> And initial patch for the headers:

Internally, the litra glow is something like RGB LED, right? Can we
support it as a multicolor LED?

Best regards,
								Pavel

>  diff --git a/include/linux/leds.h b/include/linux/leds.h
> index ba4861ec7..494eab49b 100644
> --- a/include/linux/leds.h
> +++ b/include/linux/leds.h
> @@ -72,6 +72,9 @@ struct led_classdev {
> unsigned int brightness;
> unsigned int max_brightness;
> int flags;
> + unsigned int color_temperature;
> + unsigned int min_color_temperature;
> + unsigned int max_color_temperature;
> /* Lower 16 bits reflect status */
> #define LED_SUSPENDED BIT(0)
> @@ -123,6 +126,21 @@ struct led_classdev {
> unsigned long *delay_on,
> unsigned long *delay_off);
> + /* Set LED color temperature
> + * Must not sleep. Use color_temperature_set_blocking for drivers
> + * that can sleep while setting color temperature.
> + */
> + void (*color_temperature_set)(struct led_classdev *led_cdev,
> + unsigned int color_temperature);
> + /*
> + * Set LED color temperature immediately - it can block the caller for
> + * the time required for accessing a LED device register.
> + */
> + int (*color_temperature_set_blocking)(struct led_classdev *led_cdev,
> + unsigned int color_temperature);
> + /* Get LED color temperature */
> + unsigned int (*color_temperature_get)(struct led_classdev *led_cdev);
> +
> int (*pattern_set)(struct led_classdev *led_cdev,
> struct led_pattern *pattern, u32 len, int repeat);
> int (*pattern_clear)(struct led_classdev *led_cdev);
> @@ -140,6 +158,7 @@ struct led_classdev {
> void (*flash_resume)(struct led_classdev *led_cdev);
> struct work_struct set_brightness_work;
> + struct work_struct set_color_temperature_work;
> int delayed_set_value;
> #ifdef CONFIG_LEDS_TRIGGERS
> @@ -160,6 +179,10 @@ struct led_classdev {
> int brightness_hw_changed;
> struct kernfs_node *brightness_hw_changed_kn;
> #endif
> +#ifdef CONFIG_LEDS_COLOR_TEMPERATURE_HW_CHANGED
> + int color_temperature_hw_changed;
> + struct kernfs_node *color_temperature_hw_changed_kn;
> +#endif
> /* Ensures consistent access to the LED Flash Class device */
> struct mutex led_access;
> @@ -574,6 +597,14 @@ void led_classdev_notify_brightness_hw_changed(
> static inline void led_classdev_notify_brightness_hw_changed(
> struct led_classdev *led_cdev, enum led_brightness brightness) { }
> #endif
> +#ifdef CONFIG_LEDS_COLOR_TEMPERATURE_HW_CHANGED
> +void led_classdev_notify_color_temperature_hw_changed(
> + struct led_classdev *led_cdev, unsigned int color_temperature);
> +#else
> +static inline void led_classdev_notify_color_temperature_hw_changed(
> + struct led_classdev *led_cdev, unsigned int color_temperature) { }
> +#endif
> +
> /**
> * struct led_pattern - pattern interval settings
> 
> What do you think?
> Cheers

-- 
People of Russia, stop Putin before his war on Ukraine escalates.

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

Re: Proposal: Add color_temperature support

[Andreas Bergmeier]

On Sat, 29 Oct 2022 at 18:52, Pavel Machek <pavel@ucw.cz> wrote:
> > On the device Logitech Litra Glow it is possible to set not only the
> > brightness but also the color temperature (both via usb as well as via
> > hardware).
> > I am currently trying to add support for the device - and expose it
> > via LED - into the kernel.
> > So to support all device capabilities I am proposing to extend LEDs by
> > color temperature:
> > And initial patch for the headers:
>
> Internally, the litra glow is something like RGB LED, right? Can we
> support it as a multicolor LED?
The litra glow has 2 control dimensions - brightness and color temperature.
Both dimensions can be controlled in something of a continuous range
(not sure whether these are strictly linear or not).
Thus mapping to RGB would look like more of a very sparse array.
Also, not sure which exact RGB value you would map a color temperature
to anyway.
So at first glance supporting it as a multicolor LED does not seem a
good match to me.

Re: Proposal: Add color_temperature support

[Pavel Machek]

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

Hi!

> > > On the device Logitech Litra Glow it is possible to set not only the
> > > brightness but also the color temperature (both via usb as well as via
> > > hardware).
> > > I am currently trying to add support for the device - and expose it
> > > via LED - into the kernel.
> > > So to support all device capabilities I am proposing to extend LEDs by
> > > color temperature:
> > > And initial patch for the headers:
> >
> > Internally, the litra glow is something like RGB LED, right? Can we
> > support it as a multicolor LED?
> The litra glow has 2 control dimensions - brightness and color temperature.
> Both dimensions can be controlled in something of a continuous range
> (not sure whether these are strictly linear or not).
> Thus mapping to RGB would look like more of a very sparse array.
> Also, not sure which exact RGB value you would map a color temperature
> to anyway.
> So at first glance supporting it as a multicolor LED does not seem a
> good match to me.

Hmm, and there are likely to be more lights like this, right? OTOH it
is not really an indicator LED -- using capslock trigger makes little
sense with this one.

I guess it makes sense to support this in LED subsystem. I believe it
should be only supported for "white" LEDs. I believe first step is
defining an userspace API in Documentation.

Best regards,
								Pavel
-- 
People of Russia, stop Putin before his war on Ukraine escalates.

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

Re: Proposal: Add color_temperature support

[Andreas Bergmeier]

On Sat, 29 Oct 2022 at 20:32, Pavel Machek <pavel@ucw.cz> wrote:
> Hmm, and there are likely to be more lights like this, right?
Indeed

> I guess it makes sense to support this in LED subsystem. I believe it
> should be only supported for "white" LEDs. I believe first step is
> defining an userspace API in Documentation.

RFC:
diff --git a/Documentation/leds/leds-class.rst
b/Documentation/leds/leds-class.rst
index cd155ead8..92127336e 100644
--- a/Documentation/leds/leds-class.rst
+++ b/Documentation/leds/leds-class.rst
@@ -3,10 +3,13 @@ LED handling under Linux
========================
In its simplest form, the LED class just allows control of LEDs from
-userspace. LEDs appear in /sys/class/leds/. The maximum brightness of the
-LED is defined in max_brightness file. The brightness file will set
the brightness
+userspace. LEDs appear in ``/sys/class/leds/``. The maximum brightness of the
+LED is defined in ``max_brightness`` file. The ``brightness`` file
will set the brightness
of the LED (taking a value 0-max_brightness). Most LEDs don't have hardware
brightness support so will just be turned on for non-zero brightness settings.
+If setting Color Temperature is supported there will be three files:
``color_temp``,
+``lower_color_temp`` and ``upper_color_temp``. The acceptable values
for ``color_temp`` are in the
+range of (lower_color_temp-upper_color_temp).
The class also introduces the optional concept of an LED trigger. A trigger
is a kernel based source of led events. Triggers can either be simple or
@@ -121,16 +124,29 @@ Brightness setting API
LED subsystem core exposes following API for setting brightness:
- - led_set_brightness:
+ - ``led_set_brightness``:
it is guaranteed not to sleep, passing LED_OFF stops
blinking,
- - led_set_brightness_sync:
+ - ``led_set_brightness_sync``:
for use cases when immediate effect is desired -
it can block the caller for the time required for accessing
device registers and can sleep, passing LED_OFF stops hardware
blinking, returns -EBUSY if software blink fallback is enabled.
+Color Temperature setting API
+=============================
+
+LED subsystem core exposes following API for setting Color Temperature:
+
+ - ``led_set_color_temp``:
+ it is guaranteed not to sleep
+
+ - ``led_set_color_temp_sync``:
+ for use cases when immediate effect is desired -
+ it can block the caller for the time required for accessing
+ device registers and can sleep. It returns -EBUSY if
+ software blink fallback is enabled.
LED registration API
====================

Re: Proposal: Add color_temperature support

[Andreas Bergmeier]

CC Benjamin

On Sat, 29 Oct 2022 at 21:18, Andreas Bergmeier <abergmeier@gmx.net> wrote:
>
> On Sat, 29 Oct 2022 at 20:32, Pavel Machek <pavel@ucw.cz> wrote:
> > Hmm, and there are likely to be more lights like this, right?
> Indeed
>
> > I guess it makes sense to support this in LED subsystem. I believe it
> > should be only supported for "white" LEDs. I believe first step is
> > defining an userspace API in Documentation.
>
> RFC:
I refined the RFC a bit more (was missing hw changes before):

diff --git a/Documentation/leds/leds-class.rst
b/Documentation/leds/leds-class.rst
index cd155ead8703..c3645adfe12d 100644
--- a/Documentation/leds/leds-class.rst
+++ b/Documentation/leds/leds-class.rst
@@ -3,10 +3,13 @@ LED handling under Linux
========================
In its simplest form, the LED class just allows control of LEDs from
-userspace. LEDs appear in /sys/class/leds/. The maximum brightness of the
-LED is defined in max_brightness file. The brightness file will set
the brightness
+userspace. LEDs appear in ``/sys/class/leds/``. The maximum brightness of the
+LED is defined in ``max_brightness`` file. The ``brightness`` file
will set the brightness
of the LED (taking a value 0-max_brightness). Most LEDs don't have hardware
brightness support so will just be turned on for non-zero brightness settings.
+If setting Color Temperature is supported there will be three files:
``color_temp``,
+``lower_color_temp`` and ``upper_color_temp``. The acceptable values
for ``color_temp`` are in the
+range of (lower_color_temp-upper_color_temp).
The class also introduces the optional concept of an LED trigger. A trigger
is a kernel based source of led events. Triggers can either be simple or
@@ -121,16 +124,29 @@ Brightness setting API
LED subsystem core exposes following API for setting brightness:
- - led_set_brightness:
+ - ``led_set_brightness``:
it is guaranteed not to sleep, passing LED_OFF stops
blinking,
- - led_set_brightness_sync:
+ - ``led_set_brightness_sync``:
for use cases when immediate effect is desired -
it can block the caller for the time required for accessing
device registers and can sleep, passing LED_OFF stops hardware
blinking, returns -EBUSY if software blink fallback is enabled.
+Color Temperature setting API
+=============================
+
+LED subsystem core exposes following API for setting Color Temperature:
+
+ - ``led_set_color_temp``:
+ it is guaranteed not to sleep
+
+ - ``led_set_color_temp_sync``:
+ for use cases when immediate effect is desired -
+ it can block the caller for the time required for accessing
+ device registers and can sleep. It returns -EBUSY if
+ software blink fallback is enabled.
LED registration API
====================
@@ -138,14 +154,20 @@ LED registration API
A driver wanting to register a LED classdev for use by other drivers /
userspace needs to allocate and fill a led_classdev struct and then call
`[devm_]led_classdev_register`. If the non devm version is used the driver
-must call led_classdev_unregister from its remove function before
-free-ing the led_classdev struct.
+must call ``led_classdev_unregister`` from its remove function before
+free-ing the ``led_classdev`` struct.
If the driver can detect hardware initiated brightness changes and thus
-wants to have a brightness_hw_changed attribute then the LED_BRIGHT_HW_CHANGED
+wants to have a ``brightness_hw_changed`` attribute then the
``LED_BRIGHT_HW_CHANGED``
+flag must be set in flags before registering. Calling
+``led_classdev_notify_brightness_hw_changed`` on a classdev not registered with
+the ``LED_BRIGHT_HW_CHANGED`` flag is a bug and will trigger a WARN_ON.
+
+If the driver can detect hardware initiated color temperature changes and thus
+wants to have a ``color_temp_hw_changed`` attribute then the
``LED_COLOR_TEMP_HW_CHANGED``
flag must be set in flags before registering. Calling
-led_classdev_notify_brightness_hw_changed on a classdev not registered with
-the LED_BRIGHT_HW_CHANGED flag is a bug and will trigger a WARN_ON.
+``led_classdev_notify_color_temp_hw_changed`` on a classdev not registered with
+the ``LED_COLOR_TEMP_HW_CHANGED`` flag is a bug and will trigger a WARN_ON.
Hardware accelerated blink of LEDs
==================================