Re: [PATCH v6 2/2] dt-bindings: leds: lp50xx: Document child reg, fix example

[Conor Dooley <conor@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 4225 bytes --]

On Mon, Jun 16, 2025 at 03:45:29PM +0000, Johan Adolfsson wrote:
> 
> ________________________________
> From: Conor Dooley
> Sent: Monday, June 16, 2025 17:03
> To: Johan Adolfsson
> Cc: Lee Jones; Pavel Machek; Rob Herring; Krzysztof Kozlowski; Conor Dooley; Andrew Davis; Jacek Anaszewski; linux-leds@vger.kernel.org; linux-kernel@vger.kernel.org; devicetree@vger.kernel.org; Kernel
> Subject: Re: [PATCH v6 2/2] dt-bindings: leds: lp50xx: Document child reg, fix example
> 
> On Mon, Jun 16, 2025 at 01:25:35PM +0200, Johan Adolfsson wrote:
> > The led child reg node is the index within the bank, document that
> > and update the example accordingly.
> >
> > Signed-off-by: Johan Adolfsson <johan.adolfsson@axis.com>
> > ---
> >  .../devicetree/bindings/leds/leds-lp50xx.yaml       | 21 ++++++++++++++-------
> >  1 file changed, 14 insertions(+), 7 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/leds/leds-lp50xx.yaml b/Documentation/devicetree/bindings/leds/leds-lp50xx.yaml
> > index 402c25424525..cb450aed718c 100644
> > --- a/Documentation/devicetree/bindings/leds/leds-lp50xx.yaml
> > +++ b/Documentation/devicetree/bindings/leds/leds-lp50xx.yaml
> > @@ -81,7 +81,14 @@ patternProperties:
> >
> >>          properties:
> >>            reg:
> >> -            maxItems: 1
> >> +            items:
> >> +              - minimum: 0
> >> +                maximum: 2
> >> +
> >> +            description:
> >> +              This property denotes the index within the LED bank.
> 
> >> +              The value will act as the index in the multi_index file to give
> >> +              consistent result independent of devicetree processing order.
> >
> >This looks like commentary on the particulars of the driver
> >implementation in linux, which shouldn't be in a binding.
> 
> Just trying to explain what the reg value actually does (and why).
> Before my patch the bindings were there but no code that handled it.
> If the weird reverse processing order wasn't a thing there would not have been a problem.

That's all driver implementation detail that another OS might not care
about if implemented differently, and is therefore not permitted in the
binding. The text about "index within the LED bank" should be sufficient
to describe how the hardware is configured, no?

> >>          required:
> >>            - reg
> >> @@ -138,18 +145,18 @@ examples:
> >>                  color = <LED_COLOR_ID_RGB>;
> >>                  function = LED_FUNCTION_STANDBY;
> >>
> >> -                led@3 {
> >> -                    reg = <0x3>;
> >> +                led@0 {
> >> +                    reg = <0x0>;
> >
> >Do you have any explanation for why these numbers, outside the range you
> >said is valid, were in the binding's example?
> 
> No idea, the driver hasn't handled the reg property on the led child until my patch, but
> the property was introduced by this commit:
> commit 3eb229f203c2bc42efbfbafba7f83c8deeca80c9
> Author: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Date:   Tue Jun 7 09:52:46 2022 +0200
> 
>     dt-bindings: leds: lp50xx: correct reg/unit addresses in example
> 
>     The multi-led node defined address/size cells, so it is intended to have
>     children with unit addresses.
> 
>     The second multi-led's reg property defined three LED indexes within one
>     reg item, which is not correct - these are three separate items.
> 
>     Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>     Reviewed-by: Rob Herring <robh@kernel.org>
>     Signed-off-by: Rob Herring <robh@kernel.org>
>     Link: https://lore.kernel.org/r/20220607075247.58048-1-krzysztof.kozlowski@linaro.org

Right, so random shite that Krzysztof put in, based on the reg property
in the multi-led parent, to satisfy the tooling. It's worth mentioning
in your commit message that these values you're replacing were
speculative.

> >Additionally, can you mention in the commit message what the source was
> >for the 0-2 range?
> The LED driver chip has banks with 3 outputs each, this is how I have interpreted what the code does.

Please put that in the commit message.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]