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

[Andre Przywara <andre.przywara@arm.com>]

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