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

[Chuanhong Guo]

Hi!

On Sat, Dec 3, 2022 at 6:52 PM Krzysztof Kozlowski
<krzysztof.kozlowski@xxxxxxxxxx> wrote:
>
> On 02/12/2022 13:55, Chuanhong Guo wrote:
> >>> +
> >>> +      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.
>
> default-state has a bit different purpose - to prevent any
> glitches/changes when probing driver.

OK. I didn't know that property is used in this way.
I can live without them. I'll drop it in the next version.

>
> >>
> >>> +        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
>
> Then your bindings and DTS do not represent the hardware.

How should this hardware be represented, then?

The connection can be:

SPI-MOSI---LED1---LED2---LED3---...---LEDN

or

SPI-MOSI---Tri-state signal gate---LED1---LED2---LED3---...---LEDN
SPI-CS-----|

-- 
Regards,
Chuanhong Guo