fixes the pin settings of two LEDs on board nanopi neo plus2
Subsystems (2)
PATCH 1/1 fixes the pin settings of two LEDs on board nanopi neo plus2
--- .../boot/dts/allwinner/sun50i-h5-nanopi-neo-plus2.dts | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)
arch/arm64/boot/dts/allwinner/sun50i-h5-nanopi-neo-plus2.dts +6 -5
@@ -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"; }; };
Signature · 1 line
2.34.1
Quoted 1 line · longqi
Signed-off-by longqi <[email protected]>
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
Thanks Krzysztof, I will modify and submit.
sorry for the previous email which is the html format.
Quoted 17 lines · Krzysztof Kozlowski
Quoted 1 line · longqi
Signed-off-by longqi <[email protected]>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_FILEon 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
On Mon, 11 Sep 2023 22:09:59 +0800
longqi <[email protected]> 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:
Quoted 10 lines
Signed-off-by longqi <[email protected]>--- .../boot/dts/allwinner/sun50i-h5-nanopi-neo-plus2.dts | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)arch/arm64/boot/dts/allwinner/sun50i-h5-nanopi-neo-plus2.dts +0
-0
arch/arm64/boot/dts/allwinner/sun50i-h5-nanopi-neo-plus2.dts leds { · line 29
Quoted 7 lines
arch/arm64/boot/dts/allwinner/sun50i-h5-nanopi-neo-plus2.dts +1
-3@@ -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
arch/arm64/boot/dts/allwinner/sun50i-h5-nanopi-neo-plus2.dts leds { · line 35
Quoted 8 lines
arch/arm64/boot/dts/allwinner/sun50i-h5-nanopi-neo-plus2.dts +3
-2+ 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".
arch/arm64/boot/dts/allwinner/sun50i-h5-nanopi-neo-plus2.dts leds { · line 37
Quoted 2 lines
arch/arm64/boot/dts/allwinner/sun50i-h5-nanopi-neo-plus2.dts +2
-0+ 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
Jernej Škrabec
maintainerDne ponedeljek, 11. september 2023 ob 16:54:18 CEST je Andre Przywara napisal(a):
Quoted 9 lines
On Mon, 11 Sep 2023 22:09:59 +0800
longqi <[email protected]> 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
Quoted 31 lines
In general this is a good catch, so thanks for sending this!
More below:
Quoted 12 lines
Signed-off-by longqi <[email protected]>---
.../boot/dts/allwinner/sun50i-h5-nanopi-neo-plus2.dts | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)(quoted diff) +0
-0
b/arch/arm64/boot/dts/allwinner/sun50i-h5-nanopi-neo-plus2.dtsindex4c3921ac236c..0cb7a07ab1de100644arch/arm64/boot/dts/allwinner/sun50i-h5-nanopi-neo-plus2.dts leds { · line 31
Quoted 9 lines
arch/arm64/boot/dts/allwinner/sun50i-h5-nanopi-neo-plus2.dts +1
-3@@ -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
Quoted 1 line
arch/arm64/boot/dts/allwinner/sun50i-h5-nanopi-neo-plus2.dts leds { · line 32
Quoted 1 line
arch/arm64/boot/dts/allwinner/sun50i-h5-nanopi-neo-plus2.dts +1
-0+ gpios = <&r_pio 0 10 GPIO_ACTIVE_HIGH>; /*
PL10 */
Quoted 6 lines
Quoted 6 lines
arch/arm64/boot/dts/allwinner/sun50i-h5-nanopi-neo-plus2.dts +1
-0+ linux,default-trigger = "default-on"; }; led-1 {
Quoted 7 lines
arch/arm64/boot/dts/allwinner/sun50i-h5-nanopi-neo-plus2.dts leds { · line 35
Quoted 3 lines
arch/arm64/boot/dts/allwinner/sun50i-h5-nanopi-neo-plus2.dts +1
-2- 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".
arch/arm64/boot/dts/allwinner/sun50i-h5-nanopi-neo-plus2.dts
Quoted 1 line
arch/arm64/boot/dts/allwinner/sun50i-h5-nanopi-neo-plus2.dts +1
-0+ gpios = <&pio 0 10 GPIO_ACTIVE_HIGH>; /*
PA10 */
Quoted 8 lines
arch/arm64/boot/dts/allwinner/sun50i-h5-nanopi-neo-plus2.dts leds { · line 37
Quoted 1 line
arch/arm64/boot/dts/allwinner/sun50i-h5-nanopi-neo-plus2.dts +1
-0+ 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