RE: [PATCH v2 3/3] dt-bindings: leds: Document LTC3208 Multidisplay LED Driver

[Roleda, Jan carlo]

Hello Krzysztof,

Thank you for the review.
                    
> -----Original Message-----
> From: Krzysztof Kozlowski <krzk@xxxxxxxxxx>
> Sent: Thursday, March 26, 2026 5:13 PM
> To: Roleda, Jan carlo <Jancarlo.Roleda@xxxxxxxxxx>
> Cc: Lee Jones <lee@xxxxxxxxxx>; Pavel Machek <pavel@xxxxxxxxxx>; Rob
> Herring <robh@xxxxxxxxxx>; Krzysztof Kozlowski <krzk+dt@xxxxxxxxxx>; Conor
> Dooley <conor+dt@xxxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx; linux-
> leds@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v2 3/3] dt-bindings: leds: Document LTC3208
> Multidisplay LED Driver
> 
> [External]
> 
> On Thu, Mar 26, 2026 at 06:30:12AM +0800, Jan Carlo Roleda wrote:
> > Add Documentation for LTC3208 Multidisplay LED Driver.
> 
> Please organize the patch documenting the compatible (DT bindings) before
> the patch using that compatible.
> See also: https://urldefense.com/v3/__https://elixir.bootlin.com/linux/v6.14-
> rc6/source/Documentation/devicetree/bindings/submitting-
> patches.rst*L46__;Iw!!A3Ni8CS0y2Y!89AXjtFhkqneWfAb9nXM8SHuCn0WqS
> pM9sWtt8bobLpZEy9L5KSXPS69lkM99faQ_-SeUAFS2ZnuL4E8$
> 
> >
> > Signed-off-by: Jan Carlo Roleda <jancarlo.roleda@xxxxxxxxxx>
> > ---
> >  .../devicetree/bindings/leds/adi,ltc3208.yaml      | 158
> +++++++++++++++++++++
> >  MAINTAINERS                                        |   1 +
> >  2 files changed, 159 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/leds/adi,ltc3208.yaml
> > b/Documentation/devicetree/bindings/leds/adi,ltc3208.yaml
> > new file mode 100644
> > index 000000000000..c139937936bc
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/leds/adi,ltc3208.yaml
> > @@ -0,0 +1,158 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) # Copyright
> > +(c) 2026 Analog Devices, Inc.
> > +%YAML 1.2
> > +---
> > +$id:
> > +https://urldefense.com/v3/__http://devicetree.org/schemas/leds/adi,lt
> >
> +c3208.yaml*__;Iw!!A3Ni8CS0y2Y!89AXjtFhkqneWfAb9nXM8SHuCn0WqSp
> M9sWtt8b
> > +obLpZEy9L5KSXPS69lkM99faQ_-SeUAFS2ZsFLfHP$
> > +$schema:
> > +https://urldefense.com/v3/__http://devicetree.org/meta-schemas/core.y
> >
> +aml*__;Iw!!A3Ni8CS0y2Y!89AXjtFhkqneWfAb9nXM8SHuCn0WqSpM9sWtt
> 8bobLpZEy
> > +9L5KSXPS69lkM99faQ_-SeUAFS2SZpZqA4$
> > +
> > +title: LTC3208 Multidisplay LED Controller from Linear Technologies (Now
> Analog Devices).
> 
> Drop full stop
> 

Will remove the period (full stop) on the title.

> > +
> > +maintainers:
> > +  - Jan Carlo Roleda <jancarlo.roleda@xxxxxxxxxx>
> > +
> > +description:
> > +  The LTC3208 is a multidisplay LED controller that can support up to
> > +1A to all
> > +  connected LEDs.
> > +
> > +  The datasheet for this device can be found in
> > + https://www.analog.com/en/products/ltc3208.html
> > +
> > +properties:
> > +  compatible:
> > +    const: adi,ltc3208
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  '#address-cells':
> 
> Use consistent quotes, either ' or "
> 

Noted. I will update the use of quotes for consistency.

> > +    const: 1
> > +
> > +  '#size-cells':
> > +    const: 0
> > +
> > +  adi,disable-camhl-pin:
> > +    type: boolean
> > +    description:
> > +      Configures whether the external CAMHL pin is disabled.
> > +      if disabled then the output pins associated with CAM will
> > + always select
> 
> s/if/If/.
> 

Sorry, I'm not familiar with this acronym.
Is this referring to capitalization of "if" to "If"?
If so, I will update this in the next patch.

> > +      the CAM register's high half-byte brightness.
> > +
> > +  adi,cfg-enrgbs-pin:
> > +    type: boolean
> > +    description:
> > +      Configures which channel the ENRGBS pin toggles when it receives a
> signal.
> > +      ENRGBS pin controls the SUB channel's output pins if this is set,
> > +      or RGB channel's output pins if this is unset.
> > +
> > +  adi,disable-rgb-aux4-dropout:
> > +    type: boolean
> > +    description:
> > +      Configures the RGB and AUX4 dropout signals to be disabled.
> > +
> > +  adi,aux1-channel:
> > +    $ref: /schemas/types.yaml#/definitions/string
> > +    description:
> > +      LED Channel that the AUX1 output pin mirrors its brightness level from.
> > +    enum: [aux, main, sub, cam]
> > +    default: aux
> > +
> > +  adi,aux2-channel:
> > +    $ref: /schemas/types.yaml#/definitions/string
> > +    description:
> > +      LED Channel that the AUX2 output pin mirrors its brightness level from.
> > +    enum: [aux, main, sub, cam]
> > +    default: aux
> > +
> > +  adi,aux3-channel:
> > +    $ref: /schemas/types.yaml#/definitions/string
> > +    description:
> > +      LED Channel that the AUX3 output pin mirrors its brightness level from.
> > +    enum: [aux, main, sub, cam]
> > +    default: aux
> > +
> > +  adi,aux4-channel:
> > +    $ref: /schemas/types.yaml#/definitions/string
> > +    description:
> > +      LED Channel that the AUX4 output pin mirrors its brightness level from.
> > +    enum: [aux, main, sub, cam]
> > +    default: aux
> > +
> > +patternProperties:
> > +  "^led@[0-7]$":
> > +    type: object
> > +    $ref: /schemas/leds/common.yaml#
> > +    unevaluatedProperties: false
> > +    properties:
> > +      reg:
> > +        description:
> > +          LED Channel Number. each channel maps to a specific channel group
> used
> > +          to configure the brightness level of the output pins corresponding to
> > +          the channel.
> > +        enum:
> > +          - 0 # Main Channel (8-bit brightness)
> > +          - 1 # Sub Channel (8-bit brightness)
> > +          - 2 # AUX Channel (4-bit brightness)
> > +          - 3 # Camera Channel, Low-side byte (4-bit brightness)
> > +          - 4 # Camera Channel, High-side byte (4-bit brightness)
> > +          - 5 # Red Channel (4-bit brightness)
> > +          - 6 # Blue Channel (4-bit brightness)
> > +          - 7 # Green Channel (4-bit brightness)
> > +    required:
> > +      - reg
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/gpio/gpio.h>
> 
> Where do you use it?
> 

You're right. This is not relevant for this example. I'll remove it in the next patch.

> > +    #include <dt-bindings/leds/common.h>
> > +    i2c {
> > +      #address-cells = <1>;
> > +      #size-cells = <0>;
> > +
> > +      led-controller@1b {
> > +        compatible = "adi,ltc3208";
> > +        reg = <0x1b>;
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +        adi,disable-camhl-pin;
> > +        adi,cfg-enrgbs-pin;
> > +        adi,disable-rgb-aux4-dropout;
> > +
> > +        led@0 {
> > +          reg = <0>;
> 
> Please list other applicable properties. Otherwise listing all these children is
> pointless - feels deducible from the compatible.
> 

Noted. Only the reg of the led is relevant for the compatible,
So I'll reduce it to a single led child in the example for brevity.

> > +        };
> > +
> > +        led@1 {
> > +          reg = <1>;
> > +        };
> 
> Best regards,
> Krzysztof

I will update this binding accordingly in the next patch.
Thank you.

Regards,
Carlo