2022-05-23 08:55:06

by Jean-Jacques Hiblot

[permalink] [raw]
Subject: [PATCH 1/3] dt-bindings: leds: Add bindings for the TLC5925 controller

Add bindings documentation for the TLC5925 LED controller.

Signed-off-by: Jean-Jacques Hiblot <[email protected]>
---
[email protected]
.../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
@@ -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 <[email protected]>
+
+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
+ 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.
+
+patternProperties:
+ "@[a-f0-9]+$":
+ type: object
+
+ $ref: common.yaml#
+
+ properties:
+ reg:
+ items:
+ description: |
+ LED pin number (must be lower than shift_register_length).
+ The furthest LED down the chain has the pin number 0.
+
+ required:
+ - reg
+
+required:
+ - reg
+ - "#address-cells"
+ - "#size-cells"
+ - shift_register_length
+
+examples:
+ - |
+ &spi0 {
+ leds@2 {
+ compatible = "ti,tlc5925";
+ 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"
+ };
+ };
+ };
--
2.25.1



2022-05-23 10:15:03

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH 1/3] dt-bindings: leds: Add bindings for the TLC5925 controller

On 23/05/2022 10:49, Jean-Jacques Hiblot wrote:
> Add bindings documentation for the TLC5925 LED controller.
>
> Signed-off-by: Jean-Jacques Hiblot <[email protected]>

Thank you for your patch. There is something to discuss/improve.

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


> +
> +patternProperties:
> + "@[a-f0-9]+$":

How many LEDs you can have here? Usually it is limited, so the pattern
should be narrowed.

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

2022-05-23 12:33:54

by Rob Herring

[permalink] [raw]
Subject: Re: [PATCH 1/3] dt-bindings: leds: Add bindings for the TLC5925 controller

On Mon, 23 May 2022 10:49:55 +0200, Jean-Jacques Hiblot wrote:
> Add bindings documentation for the TLC5925 LED controller.
>
> Signed-off-by: Jean-Jacques Hiblot <[email protected]>
> ---
> [email protected]
> .../bindings/leds/leds-tlc5925.yaml | 100 ++++++++++++++++++
> 1 file changed, 100 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/leds/leds-tlc5925.yaml
>

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:
./Documentation/devicetree/bindings/leds/leds-tlc5925.yaml:52:15: [error] empty value in block mapping (empty-values)

dtschema/dtc warnings/errors:
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/leds/leds-tlc5925.yaml: patternProperties:@[a-f0-9]+$:properties:reg:items: 'anyOf' conditional failed, one must be fixed:
None is not of type 'object', 'boolean'
None is not of type 'array'
from schema $id: http://json-schema.org/draft-07/schema#
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/leds/leds-tlc5925.yaml: patternProperties:@[a-f0-9]+$:properties:reg:items: 'oneOf' conditional failed, one must be fixed:
None is not of type 'object'
None is not of type 'array'
from schema $id: http://devicetree.org/meta-schemas/keywords.yaml#
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/leds/leds-tlc5925.yaml: 'oneOf' conditional failed, one must be fixed:
'unevaluatedProperties' is a required property
'additionalProperties' is a required property
hint: Either unevaluatedProperties or additionalProperties must be present
from schema $id: http://devicetree.org/meta-schemas/core.yaml#
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/leds/leds-tlc5925.yaml: patternProperties:@[a-f0-9]+$:properties:reg: 'anyOf' conditional failed, one must be fixed:
'maxItems' is a required property
hint: Only "maxItems" is required for a single entry if there are no constraints defined for the values.
'items' is not one of ['maxItems', 'description', 'deprecated']
hint: Only "maxItems" is required for a single entry if there are no constraints defined for the values.
'items' is not one of ['description', 'deprecated', 'const', 'enum', 'minimum', 'maximum', 'multipleOf', 'default', '$ref', 'oneOf']
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/leds/leds-tlc5925.yaml: patternProperties:@[a-f0-9]+$:properties:reg:items: 'oneOf' conditional failed, one must be fixed:
None is not of type 'object'
None is not of type 'array'
hint: "items" can be a list defining each entry or a schema applying to all items. A list has an implicit size. A schema requires minItems/maxItems to define the size.
from schema $id: http://devicetree.org/meta-schemas/core.yaml#
hint: cell array properties must define how many entries and what the entries are when there is more than one entry.
from schema $id: http://devicetree.org/meta-schemas/core.yaml#
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/leds/leds-tlc5925.yaml: ignoring, error in schema: patternProperties: @[a-f0-9]+$: properties: reg: items
Error: Documentation/devicetree/bindings/leds/leds-tlc5925.example.dts:18.9-14 syntax error
FATAL ERROR: Unable to parse input tree
make[1]: *** [scripts/Makefile.lib:364: Documentation/devicetree/bindings/leds/leds-tlc5925.example.dtb] Error 1
make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:1401: dt_binding_check] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.


2022-05-23 13:07:45

by Jean-Jacques Hiblot

[permalink] [raw]
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 <[email protected]>
> Thank you for your patch. There is something to discuss/improve.

Thanks for the review. I didn't know about dt_binding_check.

I'll make sure it passes next time.

JJ

>> ---
>> [email protected]
>> .../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 <[email protected]>
>> +
>> +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
>
>
>> +
>> +patternProperties:
>> + "@[a-f0-9]+$":
> How many LEDs you can have here? Usually it is limited, so the pattern
> should be narrowed.
>
>> + 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

2022-05-23 15:17:10

by Jean-Jacques Hiblot

[permalink] [raw]
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 <[email protected]>
> Thank you for your patch. There is something to discuss/improve.
>
>> ---
>> [email protected]
>> .../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 <[email protected]>
>> +
>> +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

2022-05-23 15:30:49

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH 1/3] dt-bindings: leds: Add bindings for the TLC5925 controller

On 23/05/2022 17:16, Jean-Jacques Hiblot wrote:
>
> 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 <[email protected]>
>> Thank you for your patch. There is something to discuss/improve.
>>
>>> ---
>>> [email protected]
>>> .../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 <[email protected]>
>>> +
>>> +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.

So you could daisy chain 4 billion of devices? Because by not using any
limit you claim that 4 billion is doable?

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



Best regards,
Krzysztof

2022-05-23 15:38:44

by Jean-Jacques Hiblot

[permalink] [raw]
Subject: Re: [PATCH 1/3] dt-bindings: leds: Add bindings for the TLC5925 controller


On 23/05/2022 17:30, Krzysztof Kozlowski wrote:
> On 23/05/2022 17:16, Jean-Jacques Hiblot wrote:
>> 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 <[email protected]>
>>> Thank you for your patch. There is something to discuss/improve.
>>>
>>>> ---
>>>> [email protected]
>>>> .../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 <[email protected]>
>>>> +
>>>> +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.
> So you could daisy chain 4 billion of devices? Because by not using any
> limit you claim that 4 billion is doable?

You could chain 1000 devices or more and have 16000 leds. It would be a
bit tedious to describe them all in the DTS though.

We can impose a limit but it will be arbitrary. Is this how it is
usually treated ?

>>>
>>>> +
>>>> +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.
>
>
> Best regards,
> Krzysztof

2022-05-23 15:40:03

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH 1/3] dt-bindings: leds: Add bindings for the TLC5925 controller

On 23/05/2022 17:37, Jean-Jacques Hiblot wrote:
>
> On 23/05/2022 17:30, Krzysztof Kozlowski wrote:
>> On 23/05/2022 17:16, Jean-Jacques Hiblot wrote:
>>> 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 <[email protected]>
>>>> Thank you for your patch. There is something to discuss/improve.
>>>>
>>>>> ---
>>>>> [email protected]
>>>>> .../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 <[email protected]>
>>>>> +
>>>>> +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.
>> So you could daisy chain 4 billion of devices? Because by not using any
>> limit you claim that 4 billion is doable?
>
> You could chain 1000 devices or more and have 16000 leds. It would be a
> bit tedious to describe them all in the DTS though.
>
> We can impose a limit but it will be arbitrary. Is this how it is
> usually treated ?

1000 is still less than billion, although above that number it probably
does not make sense to have any limit.

Best regards,
Krzysztof

2022-05-23 20:44:21

by Pavel Machek

[permalink] [raw]
Subject: Re: [PATCH 1/3] dt-bindings: leds: Add bindings for the TLC5925 controller

Hi!

> + led-satus@0 {

led-status.


> + led-alive@24 {
> + reg = <24>;
> + label = "green:alive"
> + };
> +
> + led-panic@31 {
> + reg = <31>;
> + label = "red:panic"
> + };

Please check documentation, if live and panic leds are common, you may
want to add it there. But better make it green:power or something.


BR,
Pavel

--
People of Russia, stop Putin before his war on Ukraine escalates.


Attachments:
(No filename) (605.00 B)
signature.asc (201.00 B)
Download all attachments