Re: [PATCH v2 2/3] dt-bindings: leds: add dt schema for worldsemi,ws2812b-spi

[Chuanhong Guo]

Hi!

On Fri, Dec 2, 2022 at 7:14 PM Krzysztof Kozlowski
<krzysztof.kozlowski@xxxxxxxxxx> wrote:
>
> On 02/12/2022 04:42, Chuanhong Guo wrote:
> > This patch adds dt binding schema for WorldSemi WS2812B driven using SPI
> > bus.
>
> Do not use "This commit/patch".
> https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95

OK.

> [...]
> > +
> > +      default-brightness:
> > +        description:
> > +          The default brightness that should be applied to the LED by the operating
> > +          system on start-up. The brightness should not exceed the brightness the
> > +          LED can provide.
> > +        $ref: /schemas/types.yaml#/definitions/uint32
> > +        minimum: 0
> > +        maximum: 255
> > +        default: 0
> > +
> > +      default-intensity:
> > +        description: |
> > +          An array of 3 integer specifying the default intensity of each color
> > +          components in this LED. <255 255 255> if unspecified.
> > +        $ref: /schemas/types.yaml#/definitions/uint32-array
>
> I am still not convinced these two properties are correct. Why this LED
> is special and defines default brightness and intensity and other LEDs
> do not? You explained you are doing it for user-space which is usually
> not a valid reason for changes specific to one binding. Either all
> bindings should support it or none.

There's already a default-state for simple LEDs without brightness
control so I think it makes sense to add default-brightness for LEDs
with brightness control and default-intensity for colored LEDs.
The default-state seems to be implemented in various LED drivers,
so I implemented these two properties in my LED driver.
There's nothing device-specific about these two properties.

>
> > +        maxItems: 3
> > +        items:
> > +          minimum: 0
> > +          maximum: 255
> > +
> > +      reg:
> > +        description: |
> > +          Which LED this node represents. The reg of the first LED on the chain
> > +          is 0.
> > +        maxItems: 1
> > +
> > +    required:
> > +      - reg
> > +      - color
> > +      - function
> > +
> > +required:
> > +  - compatible
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/leds/common.h>
> > +    spi {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        leds@0 {
>
> git grep leds@ -- Documentation/devicetree/ | wc -l
> 1
> git grep led@ -- Documentation/devicetree/ | wc -l
> 165
>
> so rather not the first one ("leds").

As you can see, this node describes a chain of LEDs, not
a single LED, so the plural form is more appropriate than
the singular form.

>
> There is also:
> git grep led-controller@ -- Documentation/devicetree/ | wc -l
> 30

This also isn't appropriate. WS2812B is a single LED package
of 3 diodes and a microcontroller. If we treat every package
as a LED, the SPI MOSI is connected directly to the LED
packages themselves with no controller in between.
If we treat the microcontroller as a led-controller, every
LED contains its own controller, instead of one controller
controlling all LEDs, and the parent node still shouldn't
be called a led-controller.

Here's a picture of the WS2812B LED package:
https://cdn-shop.adafruit.com/970x728/1655-00.jpg
and a chain of them:
https://cdn-shop.adafruit.com/970x728/1463-00.jpg

--
Regards,
Chuanhong Guo