| Message ID | 20221129140955.137361-1-gch981213@gmail.com |
|---|---|
| Headers | show |
| Series | leds: add driver for SPI driven WorldSemi WS2812B RGB LEDs | expand |
Hi, On Tue, Nov 29, 2022 at 05:54:52PM +0100, Krzysztof Kozlowski wrote: > On 29/11/2022 15:09, Chuanhong Guo wrote: > > This patch adds dt binding schema for WorldSemi WS2812B driven using SPI > > bus. Nice, I have a hobby project for something similar (SK6812, which is basically WS2812 with an extra white channel). I will switch to this work and extend it once it lands :) [...] > > + 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 > > + minItems: 3 > > Drop minItems.... but: > > > + maxItems: 3 > > + items: > > + minimum: 0 > > + maximum: 255 > > default: 255 I would argue, that the default should be 0 (off) instead of 255 (full power). > What controls the intensity? Don't you have PWM there? WS2812 is a RGB led, which contains a small Microcontroller. The µC takes 24 byte intensity data from a serial input and then passes on any following bits to the next LED. SPI clk and chip-select are ignored (chip-select support can be trivially added though). You can find them everywhere nowadays, since they are quite cheap (a few cents per LED) and need only one MOSI pin to control hundreds of LEDs. [...] -- Sebastian
On 30/11/2022 01:14, Sebastian Reichel wrote: >>> + 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 >>> + minItems: 3 >> >> Drop minItems.... but: >> >>> + maxItems: 3 >>> + items: >>> + minimum: 0 >>> + maximum: 255 >> >> default: 255 > > I would argue, that the default should be 0 (off) instead of 255 > (full power). Yes. > >> What controls the intensity? Don't you have PWM there? > > WS2812 is a RGB led, which contains a small Microcontroller. The µC > takes 24 byte intensity data from a serial input and then passes on > any following bits to the next LED. SPI clk and chip-select are > ignored (chip-select support can be trivially added though). > > You can find them everywhere nowadays, since they are quite cheap > (a few cents per LED) and need only one MOSI pin to control hundreds > of LEDs. OK. This should be anyway existing property, so default-brightness. Best regards, Krzysztof
Hi! On Wed, Nov 30, 2022 at 12:54 AM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > +description: | > > + WorldSemi WS2812B is a individually addressable LED chip that can be chained > > + together and controlled individually using a single wire. > > + This driver simulates the protocol used by this LED chip with SPI bus. > > Drop references to Linux driver, unless important for the binding. I think the SPI part is important. (I'll explain it below.) What about: This binding describes a chain of WS2812B LEDs connected to the SPI MOSI pin. instead? > > + 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-spi > > Drop "-spi". Compatibles are not supposed to include bus information. > The same for file name. WS2812B isn't a SPI chip. It's controlled with only a single wire and can be driven using anything that can produce a long and a short pulse meeting its timing requirement. This driver uses a SPI bus to send the pulses, but it can also be controlled with I2S and the PIO pins on a Raspberry Pi Pico. This spi suffix is to distinguish it from other possible implementations if someone else submits a support with a different peripheral. > > > + > > + reg: > > + description: The chip-select line on the SPI bus > > Drop description, it's obvious. OK. > > > + maxItems: 1 > > + > > + spi-max-frequency: > > + $ref: /schemas/types.yaml#/definitions/uint32 > > + description: > > + Maximum SPI clocking speed of the device in Hz. > > No need for ref and description. It comes from spi-peripheral-props. OK. > > > + minimum: 2105000 > > + maximum: 2850000 > > + > > + "#address-cells": > > + const: 1 > > + > > + "#size-cells": > > + const: 0 > > + > > +patternProperties: > > + "^multi-led(@[0-9a-f])?$": > > Why unit address is optional? It isn't. I copy-pasted it from led-class-multicolor.yaml and didn't check the exact regex. I'll fix it in the next version. > > > + type: object > > + $ref: leds-class-multicolor.yaml# > > unevaluatedProperties: false OK. > > + > > + 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 > > + minItems: 3 > > Drop minItems.... but see comment below: > > > + maxItems: 3 > > Why this is different than other multi-color LEDs? I would expect here > children with common.yaml. WS2812B is a single LED package with 3 diodes and a microcontroller. Each LED package has 3 colors. The original chip comes with GRB color while there are some clones with RGB arrangement instead. The LED chain can be really long so I'd like to simplify the binding by using a single property to override the only variable, color, here. > > > + > > + 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 > > + minItems: 3 > > Drop minItems.... but: > > > + maxItems: 3 > > + items: > > + minimum: 0 > > + maximum: 255 > > default: 255 > > What controls the intensity? Don't you have PWM there? The LED takes 3-byte brightness value of each color. This property is used to specify the default multi_intensity field for the multi-color LED. The final brightness value is calculated with led_mc_calc_color_components like this: mcled_cdev->subled_info[i].brightness = brightness * mcled_cdev->subled_info[i].intensity / led_cdev->max_brightness; The LED chip takes exactly 8 bits for the brightness (max_brightness = 255 which can't be changed.), so according to the formula above the maximum intensity should be 255. > > > + > > + reg: > > + description: | > > + Which LED this node represents. The reg of the first LED on the chain > > + is 0. > > maxItems: 1 OK. > > > + > > + required: > > + - reg > > + - color > > + - function > > + > > +required: > > + - compatible > > + > > +additionalProperties: false > > + > > +examples: > > + - | > > + #include <dt-bindings/leds/common.h> > > + spi { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + ws2812b@0 { > > Node names should be generic. > https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation OK. I'll use leds instead. > > > + #address-cells = <1>; > > + #size-cells = <0>; > > + compatible = "worldsemi,ws2812b-spi"; > > + reg = <0>; > > compatible is first property, reg is second. Got it. -- Regards, Chuanhong Guo
Hi! On Wed, Nov 30, 2022 at 4:33 PM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > >> What controls the intensity? Don't you have PWM there? > > > > WS2812 is a RGB led, which contains a small Microcontroller. The µC > > takes 24 byte intensity data from a serial input and then passes on > > any following bits to the next LED. SPI clk and chip-select are > > ignored (chip-select support can be trivially added though). > > > > You can find them everywhere nowadays, since they are quite cheap > > (a few cents per LED) and need only one MOSI pin to control hundreds > > of LEDs. > > OK. This should be anyway existing property, so default-brightness. default-intensity is a different property. Intensity controls only the color for a multicolor led. The final brightness is calculated with this intensity value with the single brightness value of the current LED. (See my previous mail.)
On 30/11/2022 09:36, Chuanhong Guo wrote: > Hi! > > On Wed, Nov 30, 2022 at 12:54 AM Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> wrote: >>> +description: | >>> + WorldSemi WS2812B is a individually addressable LED chip that can be chained >>> + together and controlled individually using a single wire. >>> + This driver simulates the protocol used by this LED chip with SPI bus. >> >> Drop references to Linux driver, unless important for the binding. > > I think the SPI part is important. (I'll explain it below.) What about: > > This binding describes a chain of WS2812B LEDs connected to the SPI MOSI pin. > > instead? OK > >>> + 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-spi >> >> Drop "-spi". Compatibles are not supposed to include bus information. >> The same for file name. > > WS2812B isn't a SPI chip. It's controlled with only a single wire and > can be driven > using anything that can produce a long and a short pulse meeting its timing > requirement. > This driver uses a SPI bus to send the pulses, but it can also be > controlled with > I2S and the PIO pins on a Raspberry Pi Pico. > This spi suffix is to distinguish it from other possible > implementations if someone > else submits a support with a different peripheral. And that's exactly what I said - the compatibles should not include bus information. The bus information comes from... the bus! > >> >>> + >>> + reg: >>> + description: The chip-select line on the SPI bus >> >> Drop description, it's obvious. > > OK. > >> >>> + maxItems: 1 >>> + >>> + spi-max-frequency: >>> + $ref: /schemas/types.yaml#/definitions/uint32 >>> + description: >>> + Maximum SPI clocking speed of the device in Hz. >> >> No need for ref and description. It comes from spi-peripheral-props. > > OK. > >> >>> + minimum: 2105000 >>> + maximum: 2850000 >>> + >>> + "#address-cells": >>> + const: 1 >>> + >>> + "#size-cells": >>> + const: 0 >>> + >>> +patternProperties: >>> + "^multi-led(@[0-9a-f])?$": >> >> Why unit address is optional? > > It isn't. I copy-pasted it from led-class-multicolor.yaml and > didn't check the exact regex. > I'll fix it in the next version. Make it required and matching your case. > >> >>> + type: object >>> + $ref: leds-class-multicolor.yaml# >> >> unevaluatedProperties: false > > OK. > >>> + >>> + 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 >>> + minItems: 3 >> >> Drop minItems.... but see comment below: >> >>> + maxItems: 3 >> >> Why this is different than other multi-color LEDs? I would expect here >> children with common.yaml. > > WS2812B is a single LED package with 3 diodes and a microcontroller. > Each LED package has 3 colors. The original chip comes with GRB > color while there are some clones with RGB arrangement instead. > The LED chain can be really long so I'd like to simplify the binding > by using a single property to override the only variable, color, here. OK, that makes sense. > >> >>> + >>> + 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 >>> + minItems: 3 >> >> Drop minItems.... but: >> >>> + maxItems: 3 >>> + items: >>> + minimum: 0 >>> + maximum: 255 >> >> default: 255 >> >> What controls the intensity? Don't you have PWM there? > > The LED takes 3-byte brightness value of each color. This property is used to > specify the default multi_intensity field for the multi-color LED. The final > brightness value is calculated with led_mc_calc_color_components like this: > > mcled_cdev->subled_info[i].brightness = brightness * > mcled_cdev->subled_info[i].intensity / led_cdev->max_brightness; > > The LED chip takes exactly 8 bits for the brightness (max_brightness = 255 > which can't be changed.), so according to the formula above the maximum > intensity should be 255. So this is brightness of each color... why insisting on calling it differently? Best regards, Krzysztof
Hi! On Wed, Nov 30, 2022 at 5:08 PM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > And that's exactly what I said - the compatibles should not include bus > information. The bus information comes from... the bus! Oh. I thought there will be a conflict if there is a SPI driver and , say, an I2C driver with the same compatible string. > [...] > >> > >> Why unit address is optional? > > > > It isn't. I copy-pasted it from led-class-multicolor.yaml and > > didn't check the exact regex. > > I'll fix it in the next version. > > Make it required and matching your case. Got it. > [...] > >>> + 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 > >>> + minItems: 3 > [...] > So this is brightness of each color... I don't think so. See the kernel doc for multicolor LED: https://docs.kernel.org/leds/leds-class-multicolor.html This property sets the sysfs file multi_intensity while the actual LED brightness is controlled with another sysfs file called 'brightness'. Setting multi_intensity alone doesn't change the LED brightness at all.
On 30/11/2022 10:25, Chuanhong Guo wrote: > Hi! > > On Wed, Nov 30, 2022 at 5:08 PM Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> wrote: >> And that's exactly what I said - the compatibles should not include bus >> information. The bus information comes from... the bus! > > Oh. I thought there will be a conflict if there is a SPI driver and > , say, an I2C driver with the same compatible string. We already have such. For example: adi,adxl312 > >> [...] >>>> >>>> Why unit address is optional? >>> >>> It isn't. I copy-pasted it from led-class-multicolor.yaml and >>> didn't check the exact regex. >>> I'll fix it in the next version. >> >> Make it required and matching your case. > > Got it. > >> [...] >>>>> + 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 >>>>> + minItems: 3 >> [...] >> So this is brightness of each color... > > I don't think so. > See the kernel doc for multicolor LED: > https://docs.kernel.org/leds/leds-class-multicolor.html > This property sets the sysfs file multi_intensity while the > actual LED brightness is controlled with another sysfs > file called 'brightness'. > Setting multi_intensity alone doesn't change the LED > brightness at all. If you had brightness, that would be correct. But you do not have brightness, right? Therefore the final brightness is always: subled[i].brightness = 255 * subled[i].intensity / max_brightness (also 255); Or your bindings are incomplete... Best regards, Krzysztof