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]

Date:   Mon, 11 Sep 2023 15:54:18 +0100
From:   Andre Przywara <andre.przywara@....com>
To:     longqi <longqi90@...il.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>,
        Jernej Skrabec <jernej.skrabec@...il.com>,
        Samuel Holland <samuel@...lland.org>,
        devicetree@...r.kernel.org (open list:OPEN FIRMWARE AND FLATTENED
        DEVICE TREE BINDINGS),
        linux-arm-kernel@...ts.infradead.org (moderated list:ARM/Allwinner
        sunXi SoC support),
        linux-sunxi@...ts.linux.dev (open list:ARM/Allwinner sunXi SoC support),
        linux-kernel@...r.kernel.org (open list)
Subject: Re: [PATCH] fixes the pin settings of two LEDs on board nanopi neo
 plus2

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.

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