Re: [PATCH v4 2/3] dt-bindings: leds: add worldsemi,ws2812b

[Lee Jones]

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.
> 
> > 
> >>>>> +        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.

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.

-- 
Lee Jones [李琼斯]