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

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

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@gmail.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@gmail.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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel