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

[Jernej Škrabec]

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@xxxxxxxxx> 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@xxxxxxxxx>
> > ---
> > 
> >  .../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