Re: [PATCH v6 2/3] dt-bindings: leds: Add multicolor PWM LED bindings

[Alexander Dahl <ada@xxxxxxxxxxx>]

Hei hei,

Am Thu, Feb 10, 2022 at 08:55:07AM +0100 schrieb Sven Schwermer:
> Hi Alex,
> 
> On 2/9/22 10:17, Alexander Dahl wrote:
> > > +    rgb-led {
> > 
> > I think this should be 'led-controller'. See
> > Documentation/devicetree/bindings/leds/common.yaml for reference.
> 
> Sure, I don't have a preference.
> 
> > > +        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>;
> > > +          };
> > 
> > Not sure if those node names should be more generic like led-0, led-1
> > etc.?  At least the color information is redundant here.  This would
> > make it more similar to bindings of other LED drivers.
> 
> I don't see how naming them led-{0,1,2} would be better in any way, please
> elaborate.

- consistency with other LED driver bindings
- spot the number of "sub"-LEDs more easily
- prevent all kinds of different names people will come up with, if
  all is allowed instead of a clear scheme

The color is in the color property anyways.

> > And how is it supposed to be named if you have multiple
> > "multi-led"s, e.g. one on three PWM channels, and another one on three
> > different PWM channels?
> 
> I'm not 100% sure what you mean. If you want multiple instances of these
> multi-color PWM LEDs, you'd do something like this:
> 
> indicator-led-controller {
>     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>;
>       };
>     };
> };
> status-led-controller {
>     compatible = "pwm-leds-multicolor";
>     multi-led {
>       color = <LED_COLOR_ID_MULTI>;
>       function = LED_FUNCTION_STATUS;
>       max-brightness = <255>;
>       led-red {
>           pwms = <&pwm4 0 1000000>;
>           color = <LED_COLOR_ID_RED>;
>       };
>       led-amber {
>           pwms = <&pwm5 0 1000000>;
>           color = <LED_COLOR_ID_AMBER>;
>       };
>     };
> };

I would have expected something like this:

  led-controller-0 {
      compatible = "pwm-leds-multicolor";

      multi-led-0 {
        color = <LED_COLOR_ID_RGB>;
        function = LED_FUNCTION_INDICATOR;
        max-brightness = <65535>;

        led-0 {
            pwms = <&pwm1 0 1000000>;
            color = <LED_COLOR_ID_RED>;
        };

        led-1 {
            pwms = <&pwm2 0 1000000>;
            color = <LED_COLOR_ID_GREEN>;
        };

        led-2 {
            pwms = <&pwm3 0 1000000>;
            color = <LED_COLOR_ID_BLUE>;
        };
      };

      multi-led-1 {
        color = <LED_COLOR_ID_RGB>;
        function = LED_FUNCTION_INDICATOR;
        max-brightness = <65535>;

        led-0 {
            pwms = <&pwm1 0 1000000>;
            color = <LED_COLOR_ID_RED>;
        };

        led-1 {
            pwms = <&pwm2 0 1000000>;
            color = <LED_COLOR_ID_GREEN>;
        };

        led-2 {
            pwms = <&pwm3 0 1000000>;
            color = <LED_COLOR_ID_BLUE>;
        };
      };
  };

Greets
Alex