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
