On 08/03/2024 22:50, Patrick Gansterer wrote:
> Add Device Tree bindings for Texas Instruments LM3509 - a
> High Efficiency Boost for White LED's and/or OLED Displays
>
> Signed-off-by: Patrick Gansterer <paroga@xxxxxxxxxx>
Thank you for your patch. There is something to discuss/improve.
> + compatible:
> + const: ti,lm3509
> +
> + reg:
> + maxItems: 1
> +
> + "#address-cells":
> + const: 1
> +
> + "#size-cells":
> + const: 0
> +
> + reset-gpios:
> + maxItems: 1
> +
> + ti,brightness-rate-of-change-us:
> + description: Brightness Rate of Change in microseconds.
> + enum: [51, 13000, 26000, 52000]
> +
> + ti,oled-mode:
> + description: Enable OLED mode.
> + type: boolean
> +
> +required:
required: goes after all properties.
> + - compatible
> + - reg
> +
> +patternProperties:
> + "^led@[01]$":
> + type: object
> + description: Properties for a string of connected LEDs.
Are you sure this is a string of LEDs? How does a string/tape work with
a backlight, I mean physically? How could it look like?
> +
> + allOf:
> + - $ref: common.yaml#
> +
> + properties:
> + reg:
> + description:
> + The control register that is used to program the two current sinks.
> + The LM3509 has two registers (BMAIN and BSUB) and are represented
> + as 0 or 1 in this property. The two current sinks can be controlled
> + independently with both registers, or register BMAIN can be
> + configured to control both sinks with the led-sources property.
> + minimum: 0
> + maximum: 1
> +
> + label:
> + maxItems: 1
It's a string, not string-array, so maxItems are not needed, just ":true".
> +
> + led-sources:
> + allOf:
> + - minItems: 1
> + maxItems: 2
> + items:
> + minimum: 0
> + maximum: 1
> +
> + default-brightness:
> + minimum: 0
> + maximum: 31
Not a required property? Then "default:".
> +
> + max-brightness:
> + minimum: 0
> + maximum: 31
Some of your examples miss it, so there is some kind of default. Add it.
> +
> + required:
> + - reg
> +
> + unevaluatedProperties: false
> +
> +unevaluatedProperties: false
You rewrote everything, so my previous comment obviously does not make
sense in such case. This must be additionalProperties: false. Look at
other bindings or example-schema how it is done (to repeat myself).
> +
> +examples:
> + - |
> + #include <dt-bindings/gpio/gpio.h>
> + i2c {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + backlight@36 {
> + compatible = "ti,lm3509";
> + reg = <0x36>;
> + reset-gpios = <&gpio2 5 GPIO_ACTIVE_LOW>;
> +
> + ti,oled-mode;
> + ti,brightness-rate-of-change-us = <52000>;
> +
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + led@0 {
Best regards,
Krzysztof
