linux-kernel - Re: [PATCH 1/3] dt-bindings: leds: Add bindings for the TLC5925 controller

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 for Android: free password hash cracker in your pocket

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

Message-ID: <0e1e417a-6444-ddb5-5c48-c89bd78c5fe8@traphandler.com>
Date:   Mon, 23 May 2022 17:16:09 +0200
From:   Jean-Jacques Hiblot <jjhiblot@...phandler.com>
To:     Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
        Pavel Machek <pavel@....cz>, Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzk+dt@...nel.org>
CC:     <linux-leds@...r.kernel.org>, <devicetree@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/3] dt-bindings: leds: Add bindings for the TLC5925
 controller


On 23/05/2022 12:14, Krzysztof Kozlowski wrote:
> On 23/05/2022 10:49, Jean-Jacques Hiblot wrote:
>> Add bindings documentation for the TLC5925 LED controller.
>>
>> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@...phandler.com>
> Thank you for your patch. There is something to discuss/improve.
>
>> ---
>> devicetree@...r.kernel.org
>>   .../bindings/leds/leds-tlc5925.yaml           | 100 ++++++++++++++++++
>>   1 file changed, 100 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/leds/leds-tlc5925.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/leds/leds-tlc5925.yaml b/Documentation/devicetree/bindings/leds/leds-tlc5925.yaml
>> new file mode 100644
>> index 000000000000..156db599d5a1
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/leds/leds-tlc5925.yaml
> Filename: vendor,device
> so "ti,tlc5925-leds.yaml" for example.
>
>
>
>> @@ -0,0 +1,100 @@
>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/leds/leds-tlc5925.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: LEDs connected to TI TLC5925 controller
>> +
>> +maintainers:
>> +  - Jean-Jacques Hiblot <jjhiblot@...phandler.com>
>> +
>> +description: |
>> +  The TLC5925 is a low-power 16-channel constant-current LED sink driver.
>> +  It is controlled through a SPI interface.
>> +  It is built around a shift register and latches which convert serial
>> +  input data into a parallel output. Several TLC5925 can be chained to
>> +  control more than 16 LEDs with a single chip-select.
>> +  The brightness level cannot be controlled, each LED is either on or off.
>> +
>> +  Each LED is represented as a sub-node of the ti,tlc5925 device.
>> +
>> +properties:
>> +  compatible:
>> +    const: ti,tlc5925
>> +
>> +  shift_register_length:
>> +    maxItems: 1
> No...
> 1. Did you test your bindings with dt_binding_check? This fails
> obviously... please, do not send untested bindings.
>
> 2. vendor prefix, no underscores, proper type, maxItems look wrong here
>
>> +    description: |
>> +      The length of the shift register. If several TLC5925 are chained,
>> +      shift_register_length should be set to 16 times the number of TLC5925.
>> +      The value must be a multiple of 8.
>> +
>> +  "#address-cells":
>> +    const: 1
>> +
>> +  "#size-cells":
>> +    const: 0
>> +
>> +  output-enable-b-gpios:
>> +    description: |
>> +      GPIO pins to enable/disable the parallel output. They describe the GPIOs
>> +      connected to the OE/ pin of the TLC5925s.
> maxItems

There is no limitation in the driver itself. The actual number of items 
here really depends on the number of chips and how they are wired.

>
>
>> +
>> +patternProperties:
>> +  "@[a-f0-9]+$":
> How many LEDs you can have here? Usually it is limited, so the pattern
> should be narrowed.

There is no limitation here either. The chips can be chained to augment 
the number of LEDs.

The max number of LED is equal to the length of the shift-register.


Jean-Jacques

>
>> +    type: object
>> +
>> +    $ref: common.yaml#
>> +
>> +    properties:
>> +      reg:
>> +        items:
> Not correct syntax... I will stop reviewing. There is no point to use
> reviewers time to do the job of a tool.
>
>
>> +examples:
>> +  - |
>> +    &spi0 {
>> +        leds@2 {
>> +                compatible = "ti,tlc5925";
> Messed up indentation. 4 spaces for DTS example.
>
>> +                reg = <0x02>;
>> +                spi-max-frequency = <30000000>;
>> +                shift_register_length = <32>;
>> +                output-enable-b-gpios = <&gpio0b 9 GPIO_ACTIVE_HIGH>, <&gpio0b 7 GPIO_ACTIVE_HIGH>;
>> +                #address-cells = <1>;
>> +                #size-cells = <0>;
>> +
>> +                led-satus@0 {
>> +                        reg = <0>;
>> +                        function = LED_FUNCTION_STATUS;
>> +                        color = <LED_COLOR_ID_GREEN>;
>> +                };
>> +
>> +                led-satus@4 {
>> +                        reg = <4>;
>> +                        function = LED_FUNCTION_STATUS;
>> +                        color = <LED_COLOR_ID_RED>;
>> +                };
>> +
>> +                led-alive@24 {
>> +                        reg = <24>;
>> +                        label = "green:alive"
>> +                };
>> +
>> +                led-panic@31 {
>> +                        reg = <31>;
>> +                        label = "red:panic"
>> +                };
>> +        };
>> +    };
>
> Best regards,
> Krzysztof