diff mbox series

[1/3] dt-bindings: leds: Add bindings for the TLC5925 controller

Message ID 20220523084958.2723943-2-jjhiblot@traphandler.com
State New
Headers show
Series [1/3] dt-bindings: leds: Add bindings for the TLC5925 controller | expand

Commit Message

Jean-Jacques Hiblot May 23, 2022, 8:49 a.m. UTC
Add bindings documentation for the TLC5925 LED controller.

Signed-off-by: Jean-Jacques Hiblot <jjhiblot@traphandler.com>
---
devicetree@vger.kernel.org
 .../bindings/leds/leds-tlc5925.yaml           | 100 ++++++++++++++++++
 1 file changed, 100 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/leds-tlc5925.yaml

Comments

Rob Herring (Arm) May 23, 2022, 12:33 p.m. UTC | #1
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 <jjhiblot@traphandler.com>
> ---
> devicetree@vger.kernel.org
>  .../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.
Krzysztof Kozlowski May 23, 2022, 3:30 p.m. UTC | #2
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 <jjhiblot@traphandler.com>
>> Thank you for your patch. There is something to discuss/improve.
>>
>>> ---
>>> devicetree@vger.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@traphandler.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.

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
Krzysztof Kozlowski May 23, 2022, 3:39 p.m. UTC | #3
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 <jjhiblot@traphandler.com>
>>>> Thank you for your patch. There is something to discuss/improve.
>>>>
>>>>> ---
>>>>> devicetree@vger.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@traphandler.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.
>> 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
Pavel Machek May 23, 2022, 8:05 p.m. UTC | #4
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
diff mbox series

Patch

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 <jjhiblot@traphandler.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
+    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"
+                };
+        };
+    };