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

From: Chuanhong Guo
Date: Fri Dec 02 2022 - 07:55:50 EST


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