diff mbox series

Proposal: Add color_temperature support

Message ID CABfF9mNvf93FAxX7MWVe5KxhrBTV4_ZBzhJPs-JT+tXdyaja1g@mail.gmail.com
State New
Headers show
Series Proposal: Add color_temperature support | expand

Commit Message

Andreas Bergmeier Oct. 29, 2022, 9:54 a.m. UTC 
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
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

Comments

Andreas Bergmeier Oct. 29, 2022, 5:11 p.m. UTC | #1 
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.
Andreas Bergmeier Oct. 29, 2022, 7:18 p.m. UTC | #2 
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
====================
Andreas Bergmeier Nov. 3, 2022, 8:37 p.m. UTC | #3 
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
==================================
diff mbox series

Patch

--- 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 {