Message-ID: <CAJsYDVJC15cePQ65BR=dxKY8ADoRepbiiFqXTNQzh_6RTAeMYA@mail.gmail.com>
Date:   Thu, 12 Jan 2023 02:53:27 +0800
From:   Chuanhong Guo <gch981213@...il.com>
To:     Lee Jones <lee@...nel.org>
Cc:     Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
        Pavel Machek <pavel@....cz>, linux-leds@...r.kernel.org,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Stanislav Jakubek <stano.jakubek@...il.com>,
        Linus Walleij <linus.walleij@...aro.org>,
        Shawn Guo <shawnguo@...nel.org>,
        Johan Hovold <johan+linaro@...nel.org>,
        Bjorn Andersson <andersson@...nel.org>,
        Marijn Suijten <marijn.suijten@...ainline.org>,
        Sven Schwermer <sven.schwermer@...ruptive-technologies.com>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v4 2/3] dt-bindings: leds: add worldsemi,ws2812b

Hi!

On Tue, Jan 10, 2023 at 6:21 PM Lee Jones <lee@...nel.org> 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@...il.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
> > >>>>>
> > >>>>> 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@...il.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:
> > >
> > > 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?

-- 
Regards,
Chuanhong Guo

Powered by blists - more mailing lists