linux-kernel - Re: [PATCH v2 2/3] dt-bindings: leds: add dt schema for worldsemi,ws2812b-spi

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAJsYDVLwyCG2xnWXDo72H-T4Tk7Edxmv_GSfJFvvWKrXZgBtCA@mail.gmail.com>
Date:   Fri, 2 Dec 2022 20:55:32 +0800
From:   Chuanhong Guo <gch981213@...il.com>
To:     Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
Cc:     linux-leds@...r.kernel.org, Pavel Machek <pavel@....cz>,
        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 v2 2/3] dt-bindings: leds: add dt schema for worldsemi,ws2812b-spi

Hi!

On Fri, Dec 2, 2022 at 7:14 PM Krzysztof Kozlowski
<krzysztof.kozlowski@...aro.org> wrote:
>
> On 02/12/2022 04:42, Chuanhong Guo wrote:
> > This patch adds dt binding schema for WorldSemi WS2812B driven using SPI
> > bus.
>
> Do not use "This commit/patch".
> https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95

OK.

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

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

--
Regards,
Chuanhong Guo