On 11/01/2023 19:53, Chuanhong Guo wrote: > Hi! > > On Tue, Jan 10, 2023 at 6:21 PM Lee Jones <lee@xxxxxxxxxx> wrote: >> >> On Tue, 10 Jan 2023, Krzysztof Kozlowski wrote: >> >>> On 09/01/2023 17:52, Lee Jones wrote: >>>> On Sat, 24 Dec 2022, Krzysztof Kozlowski wrote: >>>> >>>>> On 23/12/2022 18:19, Pavel Machek wrote: >>>>>> Hi! >>>>>> >>>>>>>> Add dt binding schema for WorldSemi WS2812B driven using SPI >>>>>>>> bus. >>>>>>>> >>>>>>>> Signed-off-by: Chuanhong Guo <gch981213@xxxxxxxxx> >>>>>>>> --- >>>>>>>> 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 >>>>>>>> >>>>>>>> Change since v2: >>>>>>>> drop "this patch" from commit message >>>>>>>> rename leds to led-controller >>>>>>>> drop default-brightness and default-intensity >>>>>>>> >>>>>>>> Change since v3: >>>>>>>> reword commit title >>>>>>>> >>>>>>>> .../bindings/leds/worldsemi,ws2812b.yaml | 116 ++++++++++++++++++ >>>>>>>> 1 file changed, 116 insertions(+) >>>>>>>> create mode 100644 Documentation/devicetree/bindings/leds/worldsemi,ws2812b.yaml >>>>>>>> >>>>>>>> diff --git a/Documentation/devicetree/bindings/leds/worldsemi,ws2812b.yaml b/Documentation/devicetree/bindings/leds/worldsemi,ws2812b.yaml >>>>>>>> new file mode 100644 >>>>>>>> index 000000000000..548c05ac3d31 >>>>>>>> --- /dev/null >>>>>>>> +++ b/Documentation/devicetree/bindings/leds/worldsemi,ws2812b.yaml >>>>>>>> @@ -0,0 +1,116 @@ >>>>>>>> +# 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@xxxxxxxxx> >>>>>>>> + >>>>>>>> +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: >>>> >>>> Why "index"? >>>> >>>> Isn't it just an array of colours rather than an index into something? >>> >>> Yeah. > > The corresponding sysfs interface is called 'multi_index' so I called > it this way. > >>> >>>> >>>>>>>> + description: | >>>>>>>> + A 3-item array specifying color of each components in this LED. It >>>> >>>> Why are you forcing this to 3? >>>> >>>> Surely there are multi-colour LEDs containing more or less colours? >>> >>> For this device, because it has only tuples of three. > > WS2812B has 3 colors per chip. There are chips using a similar protocol > with 4 colors but my current driver is hard-coded to support exactly 3 colors. > >> This doesn't looks like a device specific property to me. >> >> If this is not going to be used by any other device, shouldn't it >> contain a prefix? >> >>>>>>>> + should be one of the LED_COLOR_ID_* prefixed definitions from the >>>>>>>> + header include/dt-bindings/leds/common.h. Defaults to >>>> >>>> Isn't "include" a Linuxisum? >>> >>> No, better to have full paths, so automated tools can validate them. If >>> we ever decide to drop it, we can also make a easier search&replace for >>> the pattern starting with include/. >> >> Very well. It's your train set. :) >> >>>>>>>> + <LED_COLOR_ID_GREEN LED_COLOR_ID_RED LED_COLOR_ID_BLUE> >>>>>>>> + if unspecified. >>>>>>>> + $ref: /schemas/types.yaml#/definitions/uint32-array >>>>>>>> + maxItems: 3 >>>>>>> >>>>>>> In general I am fine with it, although there is still question for >>>>>>> adding more multi-color defines in binding headers to replace this >>>>>>> property - GRB/RBG/GBR and even more for RGBW. >>>>>>> >>>>>>> Pavel, Lee, any thoughts from your side? >>>>>> >>>>>> This really needs to mention the name this hardware is known as -- I >>>>>> believe it is NeoPixel. >>>>> >>>>> We wait here for feedback on colors... The binding is re-implementing >>>>> color, just because of combinations GRB/RBG/GBR, which could be achieved >>>>> with new color defines. >>>> >>>> Sure, but where does that end? >>>> >>>> How many permutations are there likely to be? >>> >>> For light emitting devices, RGB seems to be used for so long, that I >>> don't expect more permutations (e.g. CMY). On the other hand, someone >>> might create LED strip with whatever colors, so maybe indeed better to >>> allow any variations as in array. >> >> Even you suggested variation: "even more for RGBW". >> >> Caveat: as you are well aware, "I'm new here", so my input is no more >> informed or valuable as yours at this point. I'm just calling it as I >> see it. If you have strong opinions and they differ wildly from mine, >> we may have to take intervention from Pavel. As it stands, the >> property, although slightly restricted IMHO, appears fine. >> >>>> An unlimited array has more of a chance of standing the test of time. > > I have another idea that avoids this whole conversation: Abandon > color-index completely and determine colors with compatible string > and platform data. > My original idea of this property is to support WS2812B and its clones > with different colors under the same compatible string. Technically > genuine WS2812Bs only come with GRB colors and the clones > should have their own chip names (e.g. xiaomi,hm0807a for an RGB > clone and <unknown vendor>,sk6812 for an RGBW one.). It's > reasonable to have one compatible string per chip for colors, a > chip-specific property. Also, adding more compatible devices to a > driver is less invasive than adding more definitions to leds/common.h > What do you think? This sounds good. However it should not lead to compatibles like worldsemi,ws2812b-rgb, worldsemi,ws2812b-bgr etc. Best regards, Krzysztof
