[OpenWrt-Devel] base-files: diag: restore default trigger for 'boot' LED
diff mbox series

Message ID 20200131142254.24953-1-pepe2k@gmail.com
State Accepted
Delegated to: Piotr Dymacz
Headers show
Series
  • [OpenWrt-Devel] base-files: diag: restore default trigger for 'boot' LED
Related show

Commit Message

Piotr Dymacz Jan. 31, 2020, 2:22 p.m. UTC
For devices without a dedicated 'diag' LED, we use sometimes one of
other LEDs for indicating at least 'boot', 'failsafe' and 'upgrade'
stages. In some cases, at the same time these LEDs have defined default
triggers in DTS using 'linux,default-trigger' property. Current 'diag'
setup removes the trigger and turns off 'boot' LED after bootup.

One of the examples of such device is TP-Link TL-WR841N v14 (ramips)
which uses 'wlan' LED with defined 'linux,default-trigger' for 'diag':

aliases {
	led-boot = &led_wlan;
	led-failsafe = &led_wlan;
	led-upgrade = &led_wlan;
};

[...]

led_wlan: wlan {
	label = "tl-wr841n-v14:green:wlan";
	gpios = <&gpio1 9 GPIO_ACTIVE_LOW>;
	linux,default-trigger = "phy0tpt";
};

This patch extends 'diag.sh' and 'leds.sh' scripts to make sure default
trigger defined in DTS is restored for 'diag' LED which isn't used for
indicating 'running' stage.

Signed-off-by: Piotr Dymacz <pepe2k@gmail.com>
---
 package/base-files/Makefile                   |  2 +-
 package/base-files/files/etc/diag.sh          |  2 ++
 .../base-files/files/lib/functions/leds.sh    | 27 ++++++++++++++++---
 3 files changed, 26 insertions(+), 5 deletions(-)

Comments

Adrian Schmutzler Feb. 4, 2020, 12:32 p.m. UTC | #1
Hi,

> -----Original Message-----
> From: openwrt-devel [mailto:openwrt-devel-bounces@lists.openwrt.org] On
> Behalf Of Piotr Dymacz
> Sent: Freitag, 31. Januar 2020 15:23
> To: openwrt-devel@lists.openwrt.org
> Subject: [OpenWrt-Devel] [PATCH] base-files: diag: restore default trigger for
> 'boot' LED
> 
> For devices without a dedicated 'diag' LED, we use sometimes one of
> other LEDs for indicating at least 'boot', 'failsafe' and 'upgrade'
> stages. In some cases, at the same time these LEDs have defined default
> triggers in DTS using 'linux,default-trigger' property. Current 'diag'
> setup removes the trigger and turns off 'boot' LED after bootup.
> 
> One of the examples of such device is TP-Link TL-WR841N v14 (ramips)
> which uses 'wlan' LED with defined 'linux,default-trigger' for 'diag':
> 
> aliases {
> 	led-boot = &led_wlan;
> 	led-failsafe = &led_wlan;
> 	led-upgrade = &led_wlan;
> };
> 
> [...]
> 
> led_wlan: wlan {
> 	label = "tl-wr841n-v14:green:wlan";
> 	gpios = <&gpio1 9 GPIO_ACTIVE_LOW>;
> 	linux,default-trigger = "phy0tpt";
> };
> 
> This patch extends 'diag.sh' and 'leds.sh' scripts to make sure default
> trigger defined in DTS is restored for 'diag' LED which isn't used for
> indicating 'running' stage.

I'm not really a fan of using LEDs for diag in that case at all, and I'd
actually prefer to have the aliases removed there (unless vendor also used
multiple purpose LEDs the same way).

But since we have these case in our code right now, I think this workaround is
necessary ATM.

Acked-by: Adrian Schmutzler <freifunk@adrianschmutzler.de>

Best

Adrian

> 
> Signed-off-by: Piotr Dymacz <pepe2k@gmail.com>
> ---
>  package/base-files/Makefile                   |  2 +-
>  package/base-files/files/etc/diag.sh          |  2 ++
>  .../base-files/files/lib/functions/leds.sh    | 27 ++++++++++++++++---
>  3 files changed, 26 insertions(+), 5 deletions(-)
> 
> diff --git a/package/base-files/Makefile b/package/base-files/Makefile
> index e389148d47..18325564dc 100644
> --- a/package/base-files/Makefile
> +++ b/package/base-files/Makefile
> @@ -12,7 +12,7 @@ include $(INCLUDE_DIR)/version.mk
>  include $(INCLUDE_DIR)/feeds.mk
> 
>  PKG_NAME:=base-files
> -PKG_RELEASE:=213
> +PKG_RELEASE:=214
>  PKG_FLAGS:=nonshared
> 
>  PKG_FILE_DEPENDS:=$(PLATFORM_DIR)/ $(GENERIC_PLATFORM_DIR)/base-
> files/
> diff --git a/package/base-files/files/etc/diag.sh b/package/base-
> files/files/etc/diag.sh
> index 8eb36c6feb..37a8ec758e 100644
> --- a/package/base-files/files/etc/diag.sh
> +++ b/package/base-files/files/etc/diag.sh
> @@ -37,6 +37,8 @@ set_led_state() {
>  		;;
>  	done)
>  		status_led_off
> +		[ "$status_led" != "$running" ] && \
> +			status_led_restore_trigger "boot"
>  		[ -n "$running" ] && {
>  			status_led="$running"
>  			status_led_on
> diff --git a/package/base-files/files/lib/functions/leds.sh b/package/base-
> files/files/lib/functions/leds.sh
> index 8a1d21caef..43b2fe02ed 100644
> --- a/package/base-files/files/lib/functions/leds.sh
> +++ b/package/base-files/files/lib/functions/leds.sh
> @@ -1,16 +1,24 @@
>  #!/bin/sh
>  # Copyright (C) 2013 OpenWrt.org
> 
> -get_dt_led() {
> -	local label
> +get_dt_led_path() {
>  	local ledpath
>  	local basepath="/proc/device-tree"
>  	local nodepath="$basepath/aliases/led-$1"
> 
>  	[ -f "$nodepath" ] && ledpath=$(cat "$nodepath")
> +	[ -n "$ledpath" ] && ledpath="$basepath$ledpath"
> +
> +	echo "$ledpath"
> +}
> +
> +get_dt_led() {
> +	local label
> +	local ledpath=$(get_dt_led_path $1)
> +
>  	[ -n "$ledpath" ] && \
> -		label=$(cat "$basepath$ledpath/label" 2>/dev/null) || \
> -		label=$(cat "$basepath$ledpath/chan-name" 2>/dev/null)
> +		label=$(cat "$ledpath/label" 2>/dev/null) || \
> +		label=$(cat "$ledpath/chan-name" 2>/dev/null)
> 
>  	echo "$label"
>  }
> @@ -35,6 +43,17 @@ led_off() {
>  	led_set_attr $1 "brightness" 0
>  }
> 
> +status_led_restore_trigger() {
> +	local trigger
> +	local ledpath=$(get_dt_led_path $1)
> +
> +	[ -n "$ledpath" ] && \
> +		trigger=$(cat "$ledpath/linux,default-trigger" 2>/dev/null)
> +
> +	[ -n "$trigger" ] && \
> +		led_set_attr "$(get_dt_led $1)" "trigger" "$trigger"
> +}
> +
>  status_led_set_timer() {
>  	led_timer $status_led "$1" "$2"
>  	[ -n "$status_led2" ] && led_timer $status_led2 "$1" "$2"
> --
> 2.20.1
> 
> 
> _______________________________________________
> openwrt-devel mailing list
> openwrt-devel@lists.openwrt.org
> https://lists.openwrt.org/mailman/listinfo/openwrt-devel
Piotr Dymacz Feb. 4, 2020, 1:32 p.m. UTC | #2
Hi Adrian,

On 04.02.2020 13:32, Adrian Schmutzler wrote:
> Hi,
> 
>> -----Original Message-----
>> From: openwrt-devel [mailto:openwrt-devel-bounces@lists.openwrt.org] On
>> Behalf Of Piotr Dymacz
>> Sent: Freitag, 31. Januar 2020 15:23
>> To: openwrt-devel@lists.openwrt.org
>> Subject: [OpenWrt-Devel] [PATCH] base-files: diag: restore default trigger for
>> 'boot' LED
>> 
>> For devices without a dedicated 'diag' LED, we use sometimes one of
>> other LEDs for indicating at least 'boot', 'failsafe' and 'upgrade'
>> stages. In some cases, at the same time these LEDs have defined default
>> triggers in DTS using 'linux,default-trigger' property. Current 'diag'
>> setup removes the trigger and turns off 'boot' LED after bootup.
>> 
>> One of the examples of such device is TP-Link TL-WR841N v14 (ramips)
>> which uses 'wlan' LED with defined 'linux,default-trigger' for 'diag':
>> 
>> aliases {
>> 	led-boot = &led_wlan;
>> 	led-failsafe = &led_wlan;
>> 	led-upgrade = &led_wlan;
>> };
>> 
>> [...]
>> 
>> led_wlan: wlan {
>> 	label = "tl-wr841n-v14:green:wlan";
>> 	gpios = <&gpio1 9 GPIO_ACTIVE_LOW>;
>> 	linux,default-trigger = "phy0tpt";
>> };
>> 
>> This patch extends 'diag.sh' and 'leds.sh' scripts to make sure default
>> trigger defined in DTS is restored for 'diag' LED which isn't used for
>> indicating 'running' stage.
> 
> I'm not really a fan of using LEDs for diag in that case at all, and I'd
> actually prefer to have the aliases removed there (unless vendor also used
> multiple purpose LEDs the same way).

Without any LED defined at least for 'boot' and 'failsafe', users might 
have problems entering failsafe and knowing whether device boots at all.
Alberto Bursi Feb. 4, 2020, 3:23 p.m. UTC | #3
On 04/02/20 13:32, Adrian Schmutzler wrote:
> Hi,
>
>> -----Original Message-----
>> From: openwrt-devel [mailto:openwrt-devel-bounces@lists.openwrt.org] On
>> Behalf Of Piotr Dymacz
>> Sent: Freitag, 31. Januar 2020 15:23
>> To: openwrt-devel@lists.openwrt.org
>> Subject: [OpenWrt-Devel] [PATCH] base-files: diag: restore default trigger for
>> 'boot' LED
>>
>> For devices without a dedicated 'diag' LED, we use sometimes one of
>> other LEDs for indicating at least 'boot', 'failsafe' and 'upgrade'
>> stages. In some cases, at the same time these LEDs have defined default
>> triggers in DTS using 'linux,default-trigger' property. Current 'diag'
>> setup removes the trigger and turns off 'boot' LED after bootup.
>>
>> One of the examples of such device is TP-Link TL-WR841N v14 (ramips)
>> which uses 'wlan' LED with defined 'linux,default-trigger' for 'diag':
>>
>> aliases {
>> 	led-boot = &led_wlan;
>> 	led-failsafe = &led_wlan;
>> 	led-upgrade = &led_wlan;
>> };
>>
>> [...]
>>
>> led_wlan: wlan {
>> 	label = "tl-wr841n-v14:green:wlan";
>> 	gpios = <&gpio1 9 GPIO_ACTIVE_LOW>;
>> 	linux,default-trigger = "phy0tpt";
>> };
>>
>> This patch extends 'diag.sh' and 'leds.sh' scripts to make sure default
>> trigger defined in DTS is restored for 'diag' LED which isn't used for
>> indicating 'running' stage.
> I'm not really a fan of using LEDs for diag in that case at all, and I'd
> actually prefer to have the aliases removed there (unless vendor also used
> multiple purpose LEDs the same way).
>
I don't like this either, but I think functionality always wins over 
esthetics.

Showing boot status and the moment to press the reset button to enter 
failsafe is
more important than the "uglyness" of hijacking a LED.

Besides, in many cases (wifi led for example) the device won't be using 
that led during boot anyway.


-Alberto

Patch
diff mbox series

diff --git a/package/base-files/Makefile b/package/base-files/Makefile
index e389148d47..18325564dc 100644
--- a/package/base-files/Makefile
+++ b/package/base-files/Makefile
@@ -12,7 +12,7 @@  include $(INCLUDE_DIR)/version.mk
 include $(INCLUDE_DIR)/feeds.mk
 
 PKG_NAME:=base-files
-PKG_RELEASE:=213
+PKG_RELEASE:=214
 PKG_FLAGS:=nonshared
 
 PKG_FILE_DEPENDS:=$(PLATFORM_DIR)/ $(GENERIC_PLATFORM_DIR)/base-files/
diff --git a/package/base-files/files/etc/diag.sh b/package/base-files/files/etc/diag.sh
index 8eb36c6feb..37a8ec758e 100644
--- a/package/base-files/files/etc/diag.sh
+++ b/package/base-files/files/etc/diag.sh
@@ -37,6 +37,8 @@  set_led_state() {
 		;;
 	done)
 		status_led_off
+		[ "$status_led" != "$running" ] && \
+			status_led_restore_trigger "boot"
 		[ -n "$running" ] && {
 			status_led="$running"
 			status_led_on
diff --git a/package/base-files/files/lib/functions/leds.sh b/package/base-files/files/lib/functions/leds.sh
index 8a1d21caef..43b2fe02ed 100644
--- a/package/base-files/files/lib/functions/leds.sh
+++ b/package/base-files/files/lib/functions/leds.sh
@@ -1,16 +1,24 @@ 
 #!/bin/sh
 # Copyright (C) 2013 OpenWrt.org
 
-get_dt_led() {
-	local label
+get_dt_led_path() {
 	local ledpath
 	local basepath="/proc/device-tree"
 	local nodepath="$basepath/aliases/led-$1"
 
 	[ -f "$nodepath" ] && ledpath=$(cat "$nodepath")
+	[ -n "$ledpath" ] && ledpath="$basepath$ledpath"
+
+	echo "$ledpath"
+}
+
+get_dt_led() {
+	local label
+	local ledpath=$(get_dt_led_path $1)
+
 	[ -n "$ledpath" ] && \
-		label=$(cat "$basepath$ledpath/label" 2>/dev/null) || \
-		label=$(cat "$basepath$ledpath/chan-name" 2>/dev/null)
+		label=$(cat "$ledpath/label" 2>/dev/null) || \
+		label=$(cat "$ledpath/chan-name" 2>/dev/null)
 
 	echo "$label"
 }
@@ -35,6 +43,17 @@  led_off() {
 	led_set_attr $1 "brightness" 0
 }
 
+status_led_restore_trigger() {
+	local trigger
+	local ledpath=$(get_dt_led_path $1)
+
+	[ -n "$ledpath" ] && \
+		trigger=$(cat "$ledpath/linux,default-trigger" 2>/dev/null)
+
+	[ -n "$trigger" ] && \
+		led_set_attr "$(get_dt_led $1)" "trigger" "$trigger"
+}
+
 status_led_set_timer() {
 	led_timer $status_led "$1" "$2"
 	[ -n "$status_led2" ] && led_timer $status_led2 "$1" "$2"