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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Message-ID: <20250616-thigh-ferocity-c8e1d7ee18bb@spud>
Date: Mon, 16 Jun 2025 16:56:06 +0100
From: Conor Dooley <conor@...nel.org>
To: Johan Adolfsson <Johan.Adolfsson@...s.com>
Cc: Lee Jones <lee@...nel.org>, Pavel Machek <pavel@...nel.org>,
	Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>, Andrew Davis <afd@...com>,
	Jacek Anaszewski <jacek.anaszewski@...il.com>,
	"linux-leds@...r.kernel.org" <linux-leds@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	Kernel <Kernel@...s.com>
Subject: Re: [PATCH v6 2/2] dt-bindings: leds: lp50xx: Document child reg,
 fix example

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@...r.kernel.org; linux-kernel@...r.kernel.org; devicetree@...r.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@...s.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@...aro.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@...aro.org>
>     Reviewed-by: Rob Herring <robh@...nel.org>
>     Signed-off-by: Rob Herring <robh@...nel.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.


Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)