diff mbox series

[v3,1/2] dt-bindings: leds: Add multicolor PWM LED bindings

Message ID 20220126104844.246068-2-sven@svenschwermer.de
State Superseded
Headers show
Series [v3,1/2] dt-bindings: leds: Add multicolor PWM LED bindings | expand

Commit Message

Sven Schwermer Jan. 26, 2022, 10:48 a.m. UTC 
From: Sven Schwermer <sven.schwermer@disruptive-technologies.com>

This allows to group multiple PWM-connected monochrome LEDs into
multicolor LEDs, e.g. RGB LEDs.

Signed-off-by: Sven Schwermer <sven.schwermer@disruptive-technologies.com>
---

Notes:
    Changes in v3:
    * Remove multi-led unit name

 .../bindings/leds/leds-pwm-multicolor.yaml    | 75 +++++++++++++++++++
 1 file changed, 75 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/leds-pwm-multicolor.yaml

Comments

Jacek Anaszewski Jan. 27, 2022, 9:24 p.m. UTC | #1 
Hi Sven,

On 1/26/22 11:48 AM, sven@svenschwermer.de wrote:
> From: Sven Schwermer <sven.schwermer@disruptive-technologies.com>
> 
> This allows to group multiple PWM-connected monochrome LEDs into
> multicolor LEDs, e.g. RGB LEDs.
> 
> Signed-off-by: Sven Schwermer <sven.schwermer@disruptive-technologies.com>
> ---
[...]
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/leds/common.h>
> +
> +    rgb-led {
> +        compatible = "pwm-leds-multicolor";
> +
> +        multi-led {
> +          color = <LED_COLOR_ID_RGB>;
> +          function = LED_FUNCTION_INDICATOR;
> +          max-brightness = <65535>;

It doesn't make much sense to have such a big resolution of global
multi color brightness. 255 will be sufficient.

> +          led-red {
> +              pwms = <&pwm1 0 1000000>;
> +              color = <LED_COLOR_ID_RED>;
> +          };
> +
> +          led-green {
> +              pwms = <&pwm2 0 1000000>;
> +              color = <LED_COLOR_ID_GREEN>;
> +          };
> +
> +          led-blue {
> +              pwms = <&pwm3 0 1000000>;
> +              color = <LED_COLOR_ID_BLUE>;
> +          };
> +        };
> +    };
> +
> +...
>
Marek Behún Jan. 28, 2022, 8:36 p.m. UTC | #2 
On Thu, 27 Jan 2022 22:24:21 +0100
Jacek Anaszewski <jacek.anaszewski@gmail.com> wrote:

> Hi Sven,
> 
> On 1/26/22 11:48 AM, sven@svenschwermer.de wrote:
> > From: Sven Schwermer <sven.schwermer@disruptive-technologies.com>
> > 
> > This allows to group multiple PWM-connected monochrome LEDs into
> > multicolor LEDs, e.g. RGB LEDs.
> > 
> > Signed-off-by: Sven Schwermer <sven.schwermer@disruptive-technologies.com>
> > ---  
> [...]
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/leds/common.h>
> > +
> > +    rgb-led {
> > +        compatible = "pwm-leds-multicolor";
> > +
> > +        multi-led {
> > +          color = <LED_COLOR_ID_RGB>;
> > +          function = LED_FUNCTION_INDICATOR;
> > +          max-brightness = <65535>;  
> 
> It doesn't make much sense to have such a big resolution of global
> multi color brightness. 255 will be sufficient.

If the PWM supports it, why not?
On Omnia the default is 255, and since it is PWM, the change from 0/255
to 1/255 is much bigger then from, say, 15/255 to 16/255. So if 1/255
is too bright, you are then unable to set it less bright. I think 1024
or ever 65535 makes sense with PWMs.

Marek
Marek Behún Jan. 28, 2022, 11:26 p.m. UTC | #3 
On Sat, 29 Jan 2022 00:04:01 +0100
Jacek Anaszewski <jacek.anaszewski@gmail.com> wrote:

> On 1/28/22 9:36 PM, Marek Behún wrote:
> > On Thu, 27 Jan 2022 22:24:21 +0100
> > Jacek Anaszewski <jacek.anaszewski@gmail.com> wrote:
> >   
> >> Hi Sven,
> >>
> >> On 1/26/22 11:48 AM, sven@svenschwermer.de wrote:  
> >>> From: Sven Schwermer <sven.schwermer@disruptive-technologies.com>
> >>>
> >>> This allows to group multiple PWM-connected monochrome LEDs into
> >>> multicolor LEDs, e.g. RGB LEDs.
> >>>
> >>> Signed-off-by: Sven Schwermer <sven.schwermer@disruptive-technologies.com>
> >>> ---  
> >> [...]  
> >>> +
> >>> +additionalProperties: false
> >>> +
> >>> +examples:
> >>> +  - |
> >>> +    #include <dt-bindings/leds/common.h>
> >>> +
> >>> +    rgb-led {
> >>> +        compatible = "pwm-leds-multicolor";
> >>> +
> >>> +        multi-led {
> >>> +          color = <LED_COLOR_ID_RGB>;
> >>> +          function = LED_FUNCTION_INDICATOR;
> >>> +          max-brightness = <65535>;  
> >>
> >> It doesn't make much sense to have such a big resolution of global
> >> multi color brightness. 255 will be sufficient.  
> > 
> > If the PWM supports it, why not?
> > On Omnia the default is 255, and since it is PWM, the change from 0/255
> > to 1/255 is much bigger then from, say, 15/255 to 16/255. So if 1/255
> > is too bright, you are then unable to set it less bright. I think 1024
> > or ever 65535 makes sense with PWMs.  
> 
> With values other than 255 we will not achieve 24-bit RGB, which is one
> problem, and the other one is non-linear brightness that can be achieved
> with PWM. So probably we would need to add an additional note in the
> documentation [0], saying that changing global brightness allows to
> preserve combined LED hue only when all sub-leds are linear, and that it
> will not be the case for PWM LEDs.
> 
> And I propose to change multi-led 'color' DT property value from
> LED_COLOR_ID_RGB to LED_COLOR_ID_MULTI to avoid the impression that it
> will work as traditional 24-bit RGB.
> 
> [0] Documentation/leds/leds-class-multicolor.rst

I know that color curves were being discussed at the time multicolor
was being introduced, and AFAIK Pavel didn't like it, but I don't
remember the reasons anymore.

As far as I understand it though, for PWM LEDs there is an equation for
gamma correction. So either we need to rename this LED to MULTI, or the
driver needs to do gamma correction so that the LED behaves RGB.

Pavel, what is your opinion on this?

Marek
Sven Schwermer Jan. 31, 2022, 8:55 a.m. UTC | #4 
Hi,

On 1/31/22 08:10, Alexander Dahl wrote:
>> I know that color curves were being discussed at the time multicolor
>> was being introduced, and AFAIK Pavel didn't like it, but I don't
>> remember the reasons anymore.
>>
>> As far as I understand it though, for PWM LEDs there is an equation for
>> gamma correction.
> 
> That's right, and it gets a little more complicated if you have RGB
> instead of a single LED.  A start for reading might be this:
> 
> https://hackaday.com/2016/08/23/rgb-leds-how-to-master-gamma-and-hue-for-perfect-brightness/
> 
> (I had bookmarked that back when I was hacking on firmware for an 8bit
> microcontroller controlling an RGB LED through soft PWM few years
> ago.  A very simple solution is a precalculated static lookup table.)

This is a very interesting topic which I was gonna ask about on this 
list anyways. IMHO, the Linux leds subsystem needs a uniform way to deal 
with lightness/gamma correction. There is some previous work in pwm_bl 
and led_bl with their `brightness-levels` device tree property and 
cie1931 correction function (drivers/video/backlight/pwm_bl.c). It would 
be very nice to have this functionality in the led class. Having 
something similar for multi-color as well would be fantastic.

Sven
Pavel Machek Feb. 12, 2022, 11:54 a.m. UTC | #5 
Hi!

> > > > Signed-off-by: Sven Schwermer <sven.schwermer@disruptive-technologies.com>
> > > > ---
> > > [...]
> > > > +
> > > > +additionalProperties: false
> > > > +
> > > > +examples:
> > > > +  - |
> > > > +    #include <dt-bindings/leds/common.h>
> > > > +
> > > > +    rgb-led {
> > > > +        compatible = "pwm-leds-multicolor";
> > > > +
> > > > +        multi-led {
> > > > +          color = <LED_COLOR_ID_RGB>;
> > > > +          function = LED_FUNCTION_INDICATOR;
> > > > +          max-brightness = <65535>;
> > > 
> > > It doesn't make much sense to have such a big resolution of global
> > > multi color brightness. 255 will be sufficient.
> > 
> > If the PWM supports it, why not?
> > On Omnia the default is 255, and since it is PWM, the change from 0/255
> > to 1/255 is much bigger then from, say, 15/255 to 16/255. So if 1/255
> > is too bright, you are then unable to set it less bright. I think 1024
> > or ever 65535 makes sense with PWMs.
> 
> With values other than 255 we will not achieve 24-bit RGB, which is one
> problem, and the other one is non-linear brightness that can be achieved
> with PWM. So probably we would need to add an additional note in the
> documentation [0], saying that changing global brightness allows to
> preserve combined LED hue only when all sub-leds are linear, and that it
> will not be the case for PWM LEDs.

I have yet to see a LED that was anything but linear. If we'll want to
match colorspace of RGB displays, we'll need to do more work. (Often,
255/255/255 is not white with RGB LEDs, for example).

So 16-bit pwm makes sense to me.

Best regards,
								Pavel
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/leds/leds-pwm-multicolor.yaml b/Documentation/devicetree/bindings/leds/leds-pwm-multicolor.yaml
new file mode 100644
index 000000000000..5a7ed5e1bb9f
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-pwm-multicolor.yaml
@@ -0,0 +1,75 @@ 
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/leds/leds-pwm-multicolor.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Multi-color LEDs connected to PWM
+
+maintainers:
+  - Sven Schwermer <sven.schwermer@disruptive-technologies.com>
+
+description: |
+  This driver combines several monochrome PWM LEDs into one multi-color
+  LED using the multicolor LED class.
+
+properties:
+  compatible:
+    const: pwm-leds-multicolor
+
+  multi-led:
+    type: object
+    allOf:
+      - $ref: leds-class-multicolor.yaml#
+
+    patternProperties:
+      "^led-[0-9a-z]+$":
+        type: object
+        properties:
+          pwms:
+            maxItems: 1
+
+          pwm-names: true
+
+          color:
+            $ref: common.yaml#/properties/color
+
+        required:
+          - pwms
+          - color
+
+required:
+  - compatible
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/leds/common.h>
+
+    rgb-led {
+        compatible = "pwm-leds-multicolor";
+
+        multi-led {
+          color = <LED_COLOR_ID_RGB>;
+          function = LED_FUNCTION_INDICATOR;
+          max-brightness = <65535>;
+
+          led-red {
+              pwms = <&pwm1 0 1000000>;
+              color = <LED_COLOR_ID_RED>;
+          };
+
+          led-green {
+              pwms = <&pwm2 0 1000000>;
+              color = <LED_COLOR_ID_GREEN>;
+          };
+
+          led-blue {
+              pwms = <&pwm3 0 1000000>;
+              color = <LED_COLOR_ID_BLUE>;
+          };
+        };
+    };
+
+...