fixes the pin settings of two LEDs on board nanopi neo plus2

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

Posted by longqi 2 years, 3 months ago

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";
+			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";
+			gpios = <&pio 0 10 GPIO_ACTIVE_HIGH>; /* PA10 */
+			linux,default-trigger = "heartbeat";
 		};
 	};
 
-- 
2.34.1

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

Posted by Krzysztof Kozlowski 2 years, 3 months ago

On 11/09/2023 16:09, longqi wrote:
> Signed-off-by: longqi <longqi90@gmail.com>

Thank you for your patch. There is something to discuss/improve.

Please use subject prefixes matching the subsystem. You can get them for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching.

Your commit msg is missing. Please describe the bug or the problem thus
it will be obvious why this change is needed.


Best regards,
Krzysztof

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

Posted by Zhang LongQi 2 years, 3 months ago

Thanks Krzysztof, I will modify and submit.

sorry for the previous email which is the html format.


On Mon, Sep 11, 2023 at 10:42 PM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 11/09/2023 16:09, longqi wrote:
> > Signed-off-by: longqi <longqi90@gmail.com>
>
> Thank you for your patch. There is something to discuss/improve.
>
> Please use subject prefixes matching the subsystem. You can get them for
> example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
> your patch is touching.
>
> Your commit msg is missing. Please describe the bug or the problem thus
> it will be obvious why this change is needed.
>
>
> Best regards,
> Krzysztof
>

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

Posted by Andre Przywara 2 years, 3 months ago

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.

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

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

Posted by Jernej Škrabec 2 years, 3 months ago

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