linux-kernel - Re: [PATCH] fixes the pin settings of two LEDs on board nanopi neo plus2

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>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1862064.tdWV9SEqCh@jernej-laptop>
Date:   Mon, 11 Sep 2023 17:04:34 +0200
From:   Jernej Škrabec <jernej.skrabec@...il.com>
To:     longqi <longqi90@...il.com>,
        Andre Przywara <andre.przywara@....com>
Cc:     Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Chen-Yu Tsai <wens@...e.org>,
        Samuel Holland <samuel@...lland.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>,
        "moderated list:ARM/Allwinner sunXi SoC support" 
        <linux-arm-kernel@...ts.infradead.org>,
        "open list:ARM/Allwinner sunXi SoC support" 
        <linux-sunxi@...ts.linux.dev>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] fixes the pin settings of two LEDs on board nanopi neo plus2

Dne ponedeljek, 11. september 2023 ob 16:54:18 CEST je Andre Przywara 
napisal(a):
> On Mon, 11 Sep 2023 22:09:59 +0800
> longqi <longqi90@...il.com> wrote:
> 
> Hi,
> 
> please add a few lines here saying why you fix this: it looks like the red
> LED wasn't working at all, because of the wrong GPIO, and the colours were
> actually swapped? This would be good to know for everyone reading this
> patch.

Additionally, add "Fixes" tag, since it's a bug in DT.

In any case, go through
https://www.kernel.org/doc/html/latest/process/submitting-patches.html
so your patch will be correctly formatted.

Best regards,
Jernej

> 
> In general this is a good catch, so thanks for sending this!
> 
> More below:
> > Signed-off-by: longqi <longqi90@...il.com>
> > ---
> > 
> >  .../boot/dts/allwinner/sun50i-h5-nanopi-neo-plus2.dts | 11 ++++++-----
> >  1 file changed, 6 insertions(+), 5 deletions(-)
> > 
> > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h5-nanopi-neo-plus2.dts
> > b/arch/arm64/boot/dts/allwinner/sun50i-h5-nanopi-neo-plus2.dts index
> > 4c3921ac236c..0cb7a07ab1de 100644
> > --- a/arch/arm64/boot/dts/allwinner/sun50i-h5-nanopi-neo-plus2.dts
> > +++ b/arch/arm64/boot/dts/allwinner/sun50i-h5-nanopi-neo-plus2.dts
> > @@ -26,14 +26,15 @@ leds {
> > 
> >  		compatible = "gpio-leds";
> >  		
> >  		led-0 {
> > 
> > -			label = "nanopi:green:pwr";
> > -			gpios = <&r_pio 0 10 GPIO_ACTIVE_HIGH>;
> > -			default-state = "on";
> > +			label = "nanopi:red:pwr";
> 
> label is now a deprecated property. That's fine for existing DTs, but I
> guess touching this node means you should update this, by using "function"
> and "color", as described in:
> Documentation/devicetree/bindings/leds/common.yaml
> 
> > +			gpios = <&r_pio 0 10 GPIO_ACTIVE_HIGH>; /* 
PL10 */
> > +			linux,default-trigger = "default-on";
> > 
> >  		};
> >  		
> >  		led-1 {
> > 
> > -			label = "nanopi:red:status";
> > -			gpios = <&pio 0 20 GPIO_ACTIVE_HIGH>;
> > +			label = "nanopi:green:status";
> 
> Same thing with label here, please use "function" and "color".
> 
> > +			gpios = <&pio 0 10 GPIO_ACTIVE_HIGH>; /* 
PA10 */
> > +			linux,default-trigger = "heartbeat";
> 
> I don't think we assign default triggers for LED merely labelled "status",
> as people might have different use cases for this. For instance I'd find
> a USB or eMMC trigger more useful. So you can just drop that line.
> 
> Cheers,
> Andre