| Message ID | 20221202034240.455831-3-gch981213@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | leds: add driver for SPI driven WorldSemi WS2812B RGB LEDs | expand |
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. Other values should not be controller by default, at least not for this specific LED. If you want to change common LED bindings and LED maintainers agree, sure, that's different case. Your device does not require these default brightness/intensity properties... or at least you did not provide rationale why your device (not driver... you keep referring to driver for some reason) needs them. >> >>> + 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. Best regards, Krzysztof
Hi! On Sat, Dec 3, 2022 at 6:52 PM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> 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-----|
On 03/12/2022 13:11, Chuanhong Guo wrote: > Hi! > > On Sat, Dec 3, 2022 at 6:52 PM Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> 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-----| I would look at it this as either of: 1. serially linked separate LED controllers (so multiple device nodes) 2. one LED controller handling the entire LED system of many sub-controllers. For the (2) I proposed the name led-controller. If you think this is not (2), then looks like (1) and you should have many devices... which probably is not doable on itself. And how to call this entire set of separate LED controllers? Heh, can be also some controller, like led-controller. Best regards, Krzysztof
On Sat, Dec 3, 2022 at 8:25 PM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > >>>>> + 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-----| > > I would look at it this as either of: > 1. serially linked separate LED controllers (so multiple device nodes) > > 2. one LED controller handling the entire LED system of many > sub-controllers. > > For the (2) I proposed the name led-controller. If you think this is not > (2), then looks like (1) and you should have many devices... which > probably is not doable on itself. And how to call this entire set of > separate LED controllers? Heh, can be also some controller, like > led-controller. OK. I'll name it led-controller in the next version.
diff --git a/Documentation/devicetree/bindings/leds/worldsemi,ws2812b.yaml b/Documentation/devicetree/bindings/leds/worldsemi,ws2812b.yaml new file mode 100644 index 000000000000..f91908d0acef --- /dev/null +++ b/Documentation/devicetree/bindings/leds/worldsemi,ws2812b.yaml @@ -0,0 +1,138 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/leds/worldsemi,ws2812b.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: WS2812B LEDs driven using SPI + +maintainers: + - Chuanhong Guo <gch981213@gmail.com> + +description: | + WorldSemi WS2812B is a individually addressable LED chip that can be chained + together and controlled individually using a single wire. + This binding describes a chain of WS2812B LEDs connected to the SPI MOSI pin. + Typical setups includes connecting the data pin of the LED chain to MOSI as + the only device or using CS and MOSI with a tri-state voltage-level shifter + for the data pin. + The SPI frequency needs to be 2.105MHz~2.85MHz for the timing to be correct + and the controller needs to send all the bytes continuously. + +allOf: + - $ref: /schemas/spi/spi-peripheral-props.yaml# + +properties: + compatible: + const: worldsemi,ws2812b + + reg: + maxItems: 1 + + spi-max-frequency: + minimum: 2105000 + maximum: 2850000 + + "#address-cells": + const: 1 + + "#size-cells": + const: 0 + +patternProperties: + "^multi-led@[0-9a-f]+$": + type: object + $ref: leds-class-multicolor.yaml# + unevaluatedProperties: false + + properties: + color-index: + description: | + A 3-item array specifying color of each components in this LED. It + should be one of the LED_COLOR_ID_* prefixed definitions from the + header include/dt-bindings/leds/common.h. Defaults to + <LED_COLOR_ID_GREEN LED_COLOR_ID_RED LED_COLOR_ID_BLUE> + if unspecified. + $ref: /schemas/types.yaml#/definitions/uint32-array + maxItems: 3 + + 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 + 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 { + compatible = "worldsemi,ws2812b"; + reg = <0>; + #address-cells = <1>; + #size-cells = <0>; + spi-max-frequency = <2850000>; + multi-led@0 { + reg = <0>; + color-index = <LED_COLOR_ID_RED LED_COLOR_ID_GREEN LED_COLOR_ID_BLUE>; + default-brightness = <30>; + default-intensity = <10 50 90>; + color = <LED_COLOR_ID_RGB>; + function = LED_FUNCTION_STATUS; + function-enumerator = <0>; + }; + multi-led@1 { + reg = <1>; + color = <LED_COLOR_ID_RGB>; + function = LED_FUNCTION_STATUS; + function-enumerator = <1>; + }; + multi-led@2 { + reg = <2>; + color = <LED_COLOR_ID_RGB>; + function = LED_FUNCTION_STATUS; + function-enumerator = <2>; + }; + multi-led@3 { + reg = <3>; + color = <LED_COLOR_ID_RGB>; + function = LED_FUNCTION_STATUS; + function-enumerator = <3>; + }; + }; + }; + +...
This patch adds dt binding schema for WorldSemi WS2812B driven using SPI bus. Signed-off-by: Chuanhong Guo <gch981213@gmail.com> --- Changes since v1: remove linux driver reference from description remove some obvious descriptions fix unit address regex in multi-led property drop various minItems add maxItems = 1 to reg fix node names and property orders in binding example drop -spi from compatible string add default-brightness .../bindings/leds/worldsemi,ws2812b.yaml | 138 ++++++++++++++++++ 1 file changed, 138 insertions(+) create mode 100644 Documentation/devicetree/bindings/leds/worldsemi,ws2812b.yaml