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
