[PATCH v4 0/9] misc: introduce STATUS LED activity function

[PATCH v4 0/9] misc: introduce STATUS LED activity function

[Christian Marangi]

This series expand the STATUS LED framework with a new color
and a big new feature. One thing that many device need is a way
to communicate to the user that the device is actually doing
something.

This is especially useful for recovery steps where an
user (for example) insert an USB drive, keep a button pressed
and the device autorecover.

There is currently no way to signal the user externally that
the bootloader is processing/recoverying aside from setting
a LED on.

A solid LED on is not enough and won't actually signal any
kind of progress.
Solution is the good old blinking LED but uboot doesn't
suggest (and support) interrupts and almost all the LED
are usually GPIO LED that doesn't support HW blink.

To fix this and handle the problem of device not supporting
HW blink, expand the STATUS LED framework with new API.

We introduce a new config LED_STATUS_ACTIVITY, that similar
to the RED, GREEN and others, takes the LED ID set in
the LED_STATUS config and is used as the global LED for activity
operations.

We add status_led_activity_start/stop() used to turn the
activity LED on and register a cyclic to make it blink.
LED will continue to blink until _stop() is called.

Function that will start some kind of action will first call
_start() and then at the end will call stop().
If (on the broken implementation) a _start() is called before
calling a _stop(), the Cyclic is first unregister and is
registered again.

Support for this is initially added to the tftp, mtd and ubi
command.

This also contains a big fixup for the gpio_led driver that
currently deviates from the Documentation and make the
coloured status led feature unusable.

(world tested with the azure pipeline)

Changes v4:
- Rebase on top of next
- Rework for cyclic changes on next
- Fix compilation error
- Fix compilation warning due to missing header 
Changes v3:
- Add log on Status LED error conditions
Changes v2:
- Add Documentation conversion
- Rework to Cyclic and simplify implementation

Christian Marangi (9):
  misc: gpio_led: fix broken coloured LED status functions
  led: status_led: add support for white LED colour
  led: status_led: add function to toggle a status LED
  led: status_led: add warning when an invalid Status LED ID is used
  led: status_led: add new activity LED config and functions
  tftp: implement support for LED status activity
  mtd: implement support for LED status activity
  ubi: implement support for LED status activity
  doc: convert README.LED to .rst documentation

 cmd/legacy_led.c          |   6 +
 cmd/mtd.c                 |  19 ++++
 cmd/ubi.c                 |  15 ++-
 common/board_f.c          |   2 +
 doc/README.LED            |  77 -------------
 doc/api/index.rst         |   1 +
 doc/api/status_led.rst    |  35 ++++++
 drivers/led/Kconfig       |  30 +++++
 drivers/misc/gpio_led.c   |  41 +++++--
 drivers/misc/status_led.c |  75 ++++++++++++-
 include/status_led.h      | 231 +++++++++++++++++++++++++++++++++++++-
 net/tftp.c                |   7 ++
 12 files changed, 440 insertions(+), 99 deletions(-)
 delete mode 100644 doc/README.LED
 create mode 100644 doc/api/status_led.rst

-- 
2.43.0

[PATCH v4 1/9] misc: gpio_led: fix broken coloured LED status functions

[Christian Marangi]

The GPIO LED driver is a backend to provide LED status functions via the
GPIO common functions.

The coloured LED functions are currently broken and deviates from what
is written in README.LED

Quoting the README.LED:

CONFIG_STATUS_LED_RED is the red LED. It is used to signal errors.
This must be a valid LED number (0-5). Other similar color LED's macros
are CONFIG_STATUS_LED_GREEN, CONFIG_STATUS_LED_YELLOW and
CONFIG_STATUS_LED_BLUE.

Hence the value of the config must refer to the ID.

Currently this is not the case and the driver expect the GPIO ID to be
put in these config. On top of this to actually have these functions, a
never define in Kconfig config must be declared to actually compile
them. (CONFIG_GPIO_LED_STUBS)

To fix this and the GPIO problem, introduce some define that reference
the LED_STATUS_BIT<n> config to have the ID->BIT connection (as
described in Docs) and drop the never defined config.

The gpio_led already provide a wrapper to the functions and should not
be enabled if the board provide their custom function hence it's ok to
also provide the wrapper for the colour functions.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 drivers/misc/gpio_led.c | 31 +++++++++++++++++++------------
 1 file changed, 19 insertions(+), 12 deletions(-)

diff --git a/drivers/misc/gpio_led.c b/drivers/misc/gpio_led.c
index e63689967a7..6626b137087 100644
--- a/drivers/misc/gpio_led.c
+++ b/drivers/misc/gpio_led.c
@@ -51,56 +51,63 @@ void __led_toggle(led_id_t mask)
 	gpio_set_value(mask, !gpio_get_value(mask));
 }
 
-#ifdef CONFIG_GPIO_LED_STUBS
-
 /* 'generic' override of colored LED stubs, to use GPIO functions instead */
 
+/* We support up to 6 LEDs, LED 0 STATUS BIT doesn't have the number suffix */
+#define GPIO_BIT0	CONFIG_LED_STATUS_BIT
+#define GPIO_BIT1	CONFIG_LED_STATUS_BIT1
+#define GPIO_BIT2	CONFIG_LED_STATUS_BIT2
+#define GPIO_BIT3	CONFIG_LED_STATUS_BIT3
+#define GPIO_BIT4	CONFIG_LED_STATUS_BIT4
+#define GPIO_BIT5	CONFIG_LED_STATUS_BIT5
+/* C preprocessor magic way to generate a GPIO_LED<id> reference */
+#define GPIO_BIT(id)	___PASTE(GPIO_BIT, id)
+
 #ifdef CONFIG_LED_STATUS_RED
+
 void red_led_on(void)
 {
-	__led_set(CONFIG_LED_STATUS_RED, CONFIG_LED_STATUS_ON);
+	__led_set(GPIO_BIT(CONFIG_LED_STATUS_RED), CONFIG_LED_STATUS_ON);
 }
 
 void red_led_off(void)
 {
-	__led_set(CONFIG_LED_STATUS_RED, CONFIG_LED_STATUS_OFF);
+	__led_set(GPIO_BIT(CONFIG_LED_STATUS_RED), CONFIG_LED_STATUS_OFF);
 }
 #endif
 
 #ifdef CONFIG_LED_STATUS_GREEN
 void green_led_on(void)
 {
-	__led_set(CONFIG_LED_STATUS_GREEN, CONFIG_LED_STATUS_ON);
+	__led_set(GPIO_BIT(CONFIG_LED_STATUS_GREEN), CONFIG_LED_STATUS_ON);
 }
 
 void green_led_off(void)
 {
-	__led_set(CONFIG_LED_STATUS_GREEN, CONFIG_LED_STATUS_OFF);
+	__led_set(GPIO_BIT(CONFIG_LED_STATUS_GREEN), CONFIG_LED_STATUS_OFF);
 }
 #endif
 
 #ifdef CONFIG_LED_STATUS_YELLOW
 void yellow_led_on(void)
 {
-	__led_set(CONFIG_LED_STATUS_YELLOW, CONFIG_LED_STATUS_ON);
+	__led_set(GPIO_BIT(CONFIG_LED_STATUS_YELLOW), CONFIG_LED_STATUS_ON);
 }
 
 void yellow_led_off(void)
 {
-	__led_set(CONFIG_LED_STATUS_YELLOW, CONFIG_LED_STATUS_OFF);
+	__led_set(GPIO_BIT(CONFIG_LED_STATUS_YELLOW), CONFIG_LED_STATUS_OFF);
 }
 #endif
 
 #ifdef CONFIG_LED_STATUS_BLUE
 void blue_led_on(void)
 {
-	__led_set(CONFIG_LED_STATUS_BLUE, CONFIG_LED_STATUS_ON);
+	__led_set(GPIO_BIT(CONFIG_LED_STATUS_BLUE), CONFIG_LED_STATUS_ON);
 }
 
 void blue_led_off(void)
 {
-	__led_set(CONFIG_LED_STATUS_BLUE, CONFIG_LED_STATUS_OFF);
+	__led_set(GPIO_BIT(CONFIG_LED_STATUS_BLUE), CONFIG_LED_STATUS_OFF);
 }
 #endif
-
-#endif /* CONFIG_GPIO_LED_STUBS */
-- 
2.43.0

[PATCH v4 2/9] led: status_led: add support for white LED colour

[Christian Marangi]

Add support for white LED colour present on many devices.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 cmd/legacy_led.c        |  6 ++++++
 common/board_f.c        |  2 ++
 drivers/led/Kconfig     | 14 ++++++++++++++
 drivers/misc/gpio_led.c | 12 ++++++++++++
 include/status_led.h    |  4 ++++
 5 files changed, 38 insertions(+)

diff --git a/cmd/legacy_led.c b/cmd/legacy_led.c
index 50de7e89d8f..18f0c3472c4 100644
--- a/cmd/legacy_led.c
+++ b/cmd/legacy_led.c
@@ -56,6 +56,9 @@ static const led_tbl_t led_commands[] = {
 #endif
 #ifdef CONFIG_LED_STATUS_BLUE
 	{ "blue", CONFIG_LED_STATUS_BLUE, blue_led_off, blue_led_on, NULL },
+#endif
+#ifdef CONFIG_LED_STATUS_WHITE
+	{ "white", CONFIG_LED_STATUS_WHITE, white_led_off, white_led_on, NULL },
 #endif
 	{ NULL, 0, NULL, NULL, NULL }
 };
@@ -179,6 +182,9 @@ U_BOOT_CMD(
 #endif
 #ifdef CONFIG_LED_STATUS_BLUE
 	"blue|"
+#endif
+#ifdef CONFIG_LED_STATUS_WHITE
+	"white|"
 #endif
 	"all] [on|off|toggle|blink] [blink-freq in ms]",
 	"[led_name] [on|off|toggle|blink] sets or clears led(s)"
diff --git a/common/board_f.c b/common/board_f.c
index 212ffb3090b..af79ed9c776 100644
--- a/common/board_f.c
+++ b/common/board_f.c
@@ -72,6 +72,8 @@ __weak void yellow_led_on(void) {}
 __weak void yellow_led_off(void) {}
 __weak void blue_led_on(void) {}
 __weak void blue_led_off(void) {}
+__weak void white_led_on(void) {}
+__weak void white_led_off(void) {}
 
 /*
  * Why is gd allocated a register? Prior to reloc it might be better to
diff --git a/drivers/led/Kconfig b/drivers/led/Kconfig
index 9837960198d..6c4f02d71f2 100644
--- a/drivers/led/Kconfig
+++ b/drivers/led/Kconfig
@@ -415,6 +415,20 @@ config LED_STATUS_GREEN
 
 endif # LED_STATUS_GREEN_ENABLE
 
+config LED_STATUS_WHITE_ENABLE
+	bool "Enable white LED"
+	help
+	  Enable white status LED.
+
+if LED_STATUS_WHITE_ENABLE
+
+config LED_STATUS_WHITE
+	int "White LED identification"
+	help
+	  Valid enabled LED device number (0-5).
+
+endif # LED_STATUS_WHITE_ENABLE
+
 config LED_STATUS_CMD
 	bool "Enable status LED commands"
 
diff --git a/drivers/misc/gpio_led.c b/drivers/misc/gpio_led.c
index 6626b137087..dfc9b009570 100644
--- a/drivers/misc/gpio_led.c
+++ b/drivers/misc/gpio_led.c
@@ -111,3 +111,15 @@ void blue_led_off(void)
 	__led_set(GPIO_BIT(CONFIG_LED_STATUS_BLUE), CONFIG_LED_STATUS_OFF);
 }
 #endif
+
+#ifdef CONFIG_LED_STATUS_WHITE
+void white_led_on(void)
+{
+	__led_set(GPIO_BIT(CONFIG_LED_STATUS_WHITE), CONFIG_LED_STATUS_ON);
+}
+
+void white_led_off(void)
+{
+	__led_set(GPIO_BIT(CONFIG_LED_STATUS_WHITE), CONFIG_LED_STATUS_OFF);
+}
+#endif
diff --git a/include/status_led.h b/include/status_led.h
index 6707ab1d29d..5ce4522b029 100644
--- a/include/status_led.h
+++ b/include/status_led.h
@@ -87,6 +87,8 @@ void yellow_led_on(void);
 void yellow_led_off(void);
 void blue_led_on(void);
 void blue_led_off(void);
+void white_led_on(void);
+void white_led_off(void);
 #else
 	.extern LED_init
 	.extern red_led_on
@@ -97,6 +99,8 @@ void blue_led_off(void);
 	.extern green_led_off
 	.extern blue_led_on
 	.extern blue_led_off
+	.extern white_led_on
+	.extern white_led_off
 #endif
 
 #endif	/* _STATUS_LED_H_	*/
-- 
2.43.0

[PATCH v4 3/9] led: status_led: add function to toggle a status LED

[Christian Marangi]

Add function to toggle a status LED by using the status LED ID reference
configs.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 drivers/misc/status_led.c | 28 +++++++++++++++++++++++-----
 include/status_led.h      |  1 +
 2 files changed, 24 insertions(+), 5 deletions(-)

diff --git a/drivers/misc/status_led.c b/drivers/misc/status_led.c
index 3b1baa4f840..2b904bfa9c2 100644
--- a/drivers/misc/status_led.c
+++ b/drivers/misc/status_led.c
@@ -103,17 +103,24 @@ void status_led_tick(ulong timestamp)
 	}
 }
 
-void status_led_set(int led, int state)
+static led_dev_t *status_get_led_dev(int led)
 {
-	led_dev_t *ld;
-
 	if (led < 0 || led >= MAX_LED_DEV)
-		return;
+		return NULL;
 
 	if (!status_led_init_done)
 		status_led_init();
 
-	ld = &led_dev[led];
+	return &led_dev[led];
+}
+
+void status_led_set(int led, int state)
+{
+	led_dev_t *ld;
+
+	ld = status_get_led_dev(led);
+	if (!ld)
+		return;
 
 	ld->state = state;
 	if (state == CONFIG_LED_STATUS_BLINKING) {
@@ -122,3 +129,14 @@ void status_led_set(int led, int state)
 	}
 	__led_set (ld->mask, state);
 }
+
+void status_led_toggle(int led)
+{
+	led_dev_t *ld;
+
+	ld = status_get_led_dev(led);
+	if (!ld)
+		return;
+
+	__led_toggle(ld->mask);
+}
diff --git a/include/status_led.h b/include/status_led.h
index 5ce4522b029..fe0c84fb4b4 100644
--- a/include/status_led.h
+++ b/include/status_led.h
@@ -38,6 +38,7 @@
 void status_led_init(void);
 void status_led_tick(unsigned long timestamp);
 void status_led_set(int led, int state);
+void status_led_toggle(int led);
 
 /*****  MVS v1  **********************************************************/
 #if (defined(CONFIG_MVS) && CONFIG_MVS < 2)
-- 
2.43.0

[PATCH v4 4/9] led: status_led: add warning when an invalid Status LED ID is used

[Christian Marangi]

Add a warning when an invalid Status LED ID is used to make the user
aware of bad configurations.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 drivers/misc/status_led.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/misc/status_led.c b/drivers/misc/status_led.c
index 2b904bfa9c2..7912b4a9448 100644
--- a/drivers/misc/status_led.c
+++ b/drivers/misc/status_led.c
@@ -105,8 +105,10 @@ void status_led_tick(ulong timestamp)
 
 static led_dev_t *status_get_led_dev(int led)
 {
-	if (led < 0 || led >= MAX_LED_DEV)
+	if (led < 0 || led >= MAX_LED_DEV) {
+		printf("Invalid Status LED ID %d.", led);
 		return NULL;
+	}
 
 	if (!status_led_init_done)
 		status_led_init();
-- 
2.43.0

Re: [PATCH v4 4/9] led: status_led: add warning when an invalid Status LED ID is used

[Peter Robinson]

On Thu, 20 Jun 2024 at 02:51, Christian Marangi <ansuelsmth@gmail.com> wrote:
>
> Add a warning when an invalid Status LED ID is used to make the user
> aware of bad configurations.
>
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> ---
>  drivers/misc/status_led.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/misc/status_led.c b/drivers/misc/status_led.c
> index 2b904bfa9c2..7912b4a9448 100644
> --- a/drivers/misc/status_led.c
> +++ b/drivers/misc/status_led.c
> @@ -105,8 +105,10 @@ void status_led_tick(ulong timestamp)
>
>  static led_dev_t *status_get_led_dev(int led)
>  {
> -       if (led < 0 || led >= MAX_LED_DEV)
> +       if (led < 0 || led >= MAX_LED_DEV) {
> +               printf("Invalid Status LED ID %d.", led);

I think this needs a \n to ensure what ever the next message is it
will be on a new line.

>                 return NULL;
> +       }
>
>         if (!status_led_init_done)
>                 status_led_init();
> --
> 2.43.0
>

[PATCH v4 5/9] led: status_led: add new activity LED config and functions

[Christian Marangi]

Add a new activity LED config and additional functions to implement a
simple software blink feature to signal activity of any kind.

Usual activity might be a file transfer with TFTP, a flash write...

User of this API will call status_led_activity_start/stop() on each
activity and LED will be toggled based on the defined FREQ config value.

With this enabled, cyclic API are also enabled as this makes use of
cyclic API to handle LED blink.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 drivers/led/Kconfig       | 16 +++++++++++++++
 drivers/misc/status_led.c | 43 +++++++++++++++++++++++++++++++++++++++
 include/status_led.h      |  2 ++
 3 files changed, 61 insertions(+)

diff --git a/drivers/led/Kconfig b/drivers/led/Kconfig
index 6c4f02d71f2..869ed78e87f 100644
--- a/drivers/led/Kconfig
+++ b/drivers/led/Kconfig
@@ -359,6 +359,22 @@ config LED_STATUS_BOOT
 
 endif # LED_STATUS_BOOT_ENABLE
 
+config LED_STATUS_ACTIVITY_ENABLE
+	bool "Enable BOOT LED"
+	select CYCLIC
+	help
+	  Enable to turn an LED on when the board is doing some
+	  activity (flash write, file download).
+
+if LED_STATUS_ACTIVITY_ENABLE
+
+config LED_STATUS_ACTIVITY
+	int "LED to light when the board is doing some activity"
+	help
+	  Valid enabled LED device number.
+
+endif # LED_STATUS_ACTIVITY_ENABLE
+
 config LED_STATUS_RED_ENABLE
 	bool "Enable red LED"
 	help
diff --git a/drivers/misc/status_led.c b/drivers/misc/status_led.c
index 7912b4a9448..019096562ca 100644
--- a/drivers/misc/status_led.c
+++ b/drivers/misc/status_led.c
@@ -4,7 +4,10 @@
  * Wolfgang Denk, DENX Software Engineering, wd@denx.de.
  */
 
+#include <cyclic.h>
 #include <status_led.h>
+#include <stdio.h>
+#include <linux/kernel.h>
 #include <linux/types.h>
 
 /*
@@ -23,6 +26,7 @@ typedef struct {
 	int state;
 	int period;
 	int cnt;
+	struct cyclic_info cyclic;
 } led_dev_t;
 
 led_dev_t led_dev[] = {
@@ -142,3 +146,42 @@ void status_led_toggle(int led)
 
 	__led_toggle(ld->mask);
 }
+
+static void status_led_activity_toggle(struct cyclic_info *ctx)
+{
+	led_dev_t *ld = container_of(ctx, led_dev_t, cyclic);
+	__led_toggle(ld->mask);
+}
+
+void status_led_activity_start(int led)
+{
+	led_dev_t *ld;
+
+	ld = status_get_led_dev(led);
+	if (!ld)
+		return;
+
+	/* Use status LED state to track if cyclic is already register */
+	if (ld->state == CONFIG_LED_STATUS_BLINKING) {
+		printf("Cyclic for activity status LED %d already registered. THIS IS AN ERROR.\n",
+		       led);
+		cyclic_unregister(&ld->cyclic);
+	}
+
+	status_led_set(led, CONFIG_LED_STATUS_BLINKING);
+
+	cyclic_register(&ld->cyclic, status_led_activity_toggle,
+			ld->period * 500, "activity");
+}
+
+void status_led_activity_stop(int led)
+{
+	led_dev_t *ld;
+
+	ld = status_get_led_dev(led);
+	if (!ld)
+		return;
+
+	cyclic_unregister(&ld->cyclic);
+	status_led_set(led, CONFIG_LED_STATUS_OFF);
+}
diff --git a/include/status_led.h b/include/status_led.h
index fe0c84fb4b4..7de40551621 100644
--- a/include/status_led.h
+++ b/include/status_led.h
@@ -39,6 +39,8 @@ void status_led_init(void);
 void status_led_tick(unsigned long timestamp);
 void status_led_set(int led, int state);
 void status_led_toggle(int led);
+void status_led_activity_start(int led);
+void status_led_activity_stop(int led);
 
 /*****  MVS v1  **********************************************************/
 #if (defined(CONFIG_MVS) && CONFIG_MVS < 2)
-- 
2.43.0

[PATCH v4 6/9] tftp: implement support for LED status activity

[Christian Marangi]

Implement support for LED status activity. If the feature is enabled,
make the defined ACTIVITY LED to signal traffic.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 net/tftp.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/net/tftp.c b/net/tftp.c
index 6b16bdcbe4c..bcc31face7a 100644
--- a/net/tftp.c
+++ b/net/tftp.c
@@ -18,6 +18,7 @@
 #include <asm/global_data.h>
 #include <net/tftp.h>
 #include "bootp.h"
+#include <status_led.h>
 
 DECLARE_GLOBAL_DATA_PTR;
 
@@ -192,6 +193,9 @@ static void new_transfer(void)
 #ifdef CONFIG_CMD_TFTPPUT
 	tftp_put_final_block_sent = 0;
 #endif
+#ifdef CONFIG_LED_STATUS_ACTIVITY_ENABLE
+	status_led_activity_start(CONFIG_LED_STATUS_ACTIVITY);
+#endif
 }
 
 #ifdef CONFIG_CMD_TFTPPUT
@@ -301,6 +305,9 @@ static void tftp_complete(void)
 			time_start * 1000, "/s");
 	}
 	puts("\ndone\n");
+#ifdef CONFIG_LED_STATUS_ACTIVITY_ENABLE
+	status_led_activity_stop(CONFIG_LED_STATUS_ACTIVITY);
+#endif
 	if (!tftp_put_active)
 		efi_set_bootdev("Net", "", tftp_filename,
 				map_sysmem(tftp_load_addr, 0),
-- 
2.43.0

[PATCH v4 7/9] mtd: implement support for LED status activity

[Christian Marangi]

Implement support for LED status activity. If the feature is enabled,
make the defined ACTIVITY LED to signal mtd write or erase operations.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 cmd/mtd.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/cmd/mtd.c b/cmd/mtd.c
index 795aaa2b37d..d36dff7875c 100644
--- a/cmd/mtd.c
+++ b/cmd/mtd.c
@@ -10,6 +10,7 @@
 
 #include <command.h>
 #include <console.h>
+#include <status_led.h>
 #if CONFIG_IS_ENABLED(CMD_MTD_OTP)
 #include <hexdump.h>
 #endif
@@ -558,6 +559,11 @@ static int do_mtd_io(struct cmd_tbl *cmdtp, int flag, int argc,
 	while (mtd_block_isbad(mtd, off))
 		off += mtd->erasesize;
 
+#ifdef CONFIG_LED_STATUS_ACTIVITY_ENABLE
+	if (!read)
+		status_led_activity_start(CONFIG_LED_STATUS_ACTIVITY);
+#endif
+
 	/* Loop over the pages to do the actual read/write */
 	while (remaining) {
 		/* Skip the block if it is bad */
@@ -585,6 +591,11 @@ static int do_mtd_io(struct cmd_tbl *cmdtp, int flag, int argc,
 		io_op.oobbuf += io_op.oobretlen;
 	}
 
+#ifdef CONFIG_LED_STATUS_ACTIVITY_ENABLE
+	if (!read)
+		status_led_activity_stop(CONFIG_LED_STATUS_ACTIVITY);
+#endif
+
 	if (!ret && dump)
 		mtd_dump_device_buf(mtd, start_off, buf, len, woob);
 
@@ -652,6 +663,10 @@ static int do_mtd_erase(struct cmd_tbl *cmdtp, int flag, int argc,
 	erase_op.addr = off;
 	erase_op.len = mtd->erasesize;
 
+#ifdef CONFIG_LED_STATUS_ACTIVITY_ENABLE
+	status_led_activity_start(CONFIG_LED_STATUS_ACTIVITY);
+#endif
+
 	while (len) {
 		if (!scrub) {
 			ret = mtd_block_isbad(mtd, erase_op.addr);
@@ -680,6 +695,10 @@ static int do_mtd_erase(struct cmd_tbl *cmdtp, int flag, int argc,
 		erase_op.addr += mtd->erasesize;
 	}
 
+#ifdef CONFIG_LED_STATUS_ACTIVITY_ENABLE
+	status_led_activity_stop(CONFIG_LED_STATUS_ACTIVITY);
+#endif
+
 	if (ret && ret != -EIO)
 		ret = CMD_RET_FAILURE;
 	else
-- 
2.43.0

[PATCH v4 8/9] ubi: implement support for LED status activity

[Christian Marangi]

Implement support for LED status activity. If the feature is enabled,
make the defined ACTIVITY LED to signal ubi write operation.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 cmd/ubi.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/cmd/ubi.c b/cmd/ubi.c
index 8c1b5df0572..cfc26d53d65 100644
--- a/cmd/ubi.c
+++ b/cmd/ubi.c
@@ -19,6 +19,7 @@
 #include <mtd.h>
 #include <nand.h>
 #include <onenand_uboot.h>
+#include <status_led.h>
 #include <dm/devres.h>
 #include <linux/mtd/mtd.h>
 #include <linux/mtd/partitions.h>
@@ -416,7 +417,19 @@ int ubi_volume_begin_write(char *volume, void *buf, size_t size,
 
 int ubi_volume_write(char *volume, void *buf, size_t size)
 {
-	return ubi_volume_begin_write(volume, buf, size, size);
+	int ret;
+
+#ifdef CONFIG_LED_STATUS_ACTIVITY_ENABLE
+	status_led_activity_start(CONFIG_LED_STATUS_ACTIVITY);
+#endif
+
+	ret = ubi_volume_begin_write(volume, buf, size, size);
+
+#ifdef CONFIG_LED_STATUS_ACTIVITY_ENABLE
+	status_led_activity_stop(CONFIG_LED_STATUS_ACTIVITY);
+#endif
+
+	return ret;
 }
 
 int ubi_volume_read(char *volume, char *buf, size_t size)
-- 
2.43.0

[PATCH v4 9/9] doc: convert README.LED to .rst documentation

[Christian Marangi]

Convert README.LED to .rst documentation and include all the relevant
documentation in the status_led.h.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 doc/README.LED         |  77 --------------
 doc/api/index.rst      |   1 +
 doc/api/status_led.rst |  35 +++++++
 include/status_led.h   | 224 ++++++++++++++++++++++++++++++++++++++++-
 4 files changed, 256 insertions(+), 81 deletions(-)
 delete mode 100644 doc/README.LED
 create mode 100644 doc/api/status_led.rst

diff --git a/doc/README.LED b/doc/README.LED
deleted file mode 100644
index c21c9d53ec3..00000000000
--- a/doc/README.LED
+++ /dev/null
@@ -1,77 +0,0 @@
-Status LED
-========================================
-
-This README describes the status LED API.
-
-The API is defined by the include file include/status_led.h
-
-The first step is to enable CONFIG_LED_STATUS in menuconfig:
-> Device Drivers > LED Support.
-
-If the LED support is only for specific board, enable
-CONFIG_LED_STATUS_BOARD_SPECIFIC in the menuconfig.
-
-Status LEDS 0 to 5 are enabled by the following configurations at menuconfig:
-CONFIG_STATUS_LED0, CONFIG_STATUS_LED1, ... CONFIG_STATUS_LED5
-
-The following should be configured for each of the enabled LEDs:
-CONFIG_STATUS_LED_BIT<n>
-CONFIG_STATUS_LED_STATE<n>
-CONFIG_STATUS_LED_FREQ<n>
-Where <n> is an integer 1 through 5 (empty for 0).
-
-CONFIG_STATUS_LED_BIT is passed into the __led_* functions to identify which LED
-is being acted on. As such, the value choose must be unique with with respect to
-the other CONFIG_STATUS_LED_BIT's. Mapping the value to a physical LED is the
-reponsiblity of the __led_* function.
-
-CONFIG_STATUS_LED_STATE is the initial state of the LED. It should be set to one
-of these values: CONFIG_LED_STATUS_OFF or CONFIG_LED_STATUS_ON.
-
-CONFIG_STATUS_LED_FREQ determines the LED blink frequency.
-Values range from 2 to 10.
-
-Some other LED macros
----------------------
-
-CONFIG_STATUS_LED_BOOT is the LED to light when the board is booting.
-This must be a valid LED number (0-5).
-
-CONFIG_STATUS_LED_RED is the red LED. It is used to signal errors. This must be
-a valid LED number (0-5). Other similar color LED's macros are
-CONFIG_STATUS_LED_GREEN, CONFIG_STATUS_LED_YELLOW and CONFIG_STATUS_LED_BLUE.
-
-General LED functions
----------------------
-The following functions should be defined:
-
-__led_init is called once to initialize the LED to CONFIG_STATUS_LED_STATE.
-One time start up code should be placed here.
-
-__led_set is called to change the state of the LED.
-
-__led_toggle is called to toggle the current state of the LED.
-
-Colour LED
-========================================
-
-Colour LED's are at present only used by ARM.
-
-The functions names explain their purpose.
-
-coloured_LED_init
-red_LED_on
-red_LED_off
-green_LED_on
-green_LED_off
-yellow_LED_on
-yellow_LED_off
-blue_LED_on
-blue_LED_off
-
-These are weakly defined in arch/arm/lib/board.c to noops. Where applicable, define
-these functions in the board specific source.
-
-TBD : Describe older board dependent macros similar to what is done for
-
-TBD : Describe general support via asm/status_led.h
diff --git a/doc/api/index.rst b/doc/api/index.rst
index 51b2013af36..d40e90801d1 100644
--- a/doc/api/index.rst
+++ b/doc/api/index.rst
@@ -22,6 +22,7 @@ U-Boot API documentation
    rng
    sandbox
    serial
+   status_led
    sysreset
    timer
    unicode
diff --git a/doc/api/status_led.rst b/doc/api/status_led.rst
new file mode 100644
index 00000000000..44bbea47157
--- /dev/null
+++ b/doc/api/status_led.rst
@@ -0,0 +1,35 @@
+.. SPDX-License-Identifier: GPL-2.0+
+
+Status LED
+==========
+
+.. kernel-doc:: include/status_led.h
+   :doc: Overview
+
+CONFIG_STATUS_LED Description
+-----------------------------
+
+.. kernel-doc:: include/status_led.h
+   :doc: CONFIG Description
+
+Special Status LED Configs
+--------------------------
+.. kernel-doc:: include/status_led.h
+   :doc: LED Status Config
+
+Colour Status LED Configs
+-------------------------
+.. kernel-doc:: include/status_led.h
+   :doc: LED Colour Config
+
+Required API
+------------
+
+.. kernel-doc:: include/status_led.h
+   :doc: Required API
+
+Status LED API
+--------------
+
+.. kernel-doc:: include/status_led.h
+   :internal:
diff --git a/include/status_led.h b/include/status_led.h
index 7de40551621..6893d1d68e0 100644
--- a/include/status_led.h
+++ b/include/status_led.h
@@ -4,18 +4,102 @@
  * Wolfgang Denk, DENX Software Engineering, wd@denx.de.
  */
 
-/*
- * The purpose of this code is to signal the operational status of a
+/**
+ * DOC: Overview
+ *
+ * Status LED is a Low-Level way to handle LEDs to signal state of the
+ * bootloader, for example boot progress, file transfer fail, activity
+ * of some sort like tftp transfer, mtd write/erase.
+ *
+ * The original usage these API were to signal the operational status of a
  * target which usually boots over the network; while running in
  * PCBoot, a status LED is blinking. As soon as a valid BOOTP reply
  * message has been received, the LED is turned off. The Linux
  * kernel, once it is running, will start blinking the LED again,
  * with another frequency.
+ *
+ * Status LED require support Low Level and the board to implement
+ * the specific functions to correctly works.
  */
 
 #ifndef _STATUS_LED_H_
 #define	_STATUS_LED_H_
 
+/**
+ * DOC: CONFIG Description
+ *
+ * Enable `CONFIG_LED_STATUS` to support the Status LED under
+ * > Device Drivers > LED Support.
+ *
+ * The Status LED can be defined in various ways, but most of the time,
+ * specific functions will need to be defined in the board file.
+ * If this is the case, enable `CONFIG_LED_STATUS_BOARD_SPECIFIC`.
+ *
+ * If the LEDs are GPIO-driven, you can use the GPIO wrapper driver
+ * instead of defining specific board functions.
+ * If this is the case, enable `CONFIG_LED_STATUS_GPIO`.
+ * (Note that `CONFIG_LED_STATUS_BOARD_SPECIFIC` is also required.)
+ *
+ * The Status LED allows defining up to six different LEDs, from 0 to 5,
+ * with the following configurations:
+ * `CONFIG_STATUS_LED`, `CONFIG_STATUS_LED1`, ..., `CONFIG_STATUS_LED5`.
+ *
+ * For each LED, the following options are required:
+ *  * `CONFIG_STATUS_LED_BIT<n>`
+ *  * `CONFIG_STATUS_LED_STATE<n>`
+ *  * `CONFIG_STATUS_LED_FREQ<n>`
+ *
+ * Where `<n>` is an integer from 1 through 5. (Note that LED 0 doesn't have the
+ * integer suffix.)
+ *
+ * `CONFIG_STATUS_LED_BIT` is passed into the `__led_*` functions to identify
+ * which LED is being acted on. The value is opaque, meaning it depends on how
+ * the low-level API handles this value. It can be an ID to reference the LED
+ * internally, or in the case of the GPIO wrapper, it's the GPIO number of the LED.
+ * Mapping the value to a physical LED is the responsibility of the `__led_*` function.
+ *
+ * `CONFIG_STATUS_LED_STATE` sets the initial state of the LED. It should be set
+ * to one of these values: `CONFIG_LED_STATUS_OFF` or `CONFIG_LED_STATUS_ON`.
+ *
+ * `CONFIG_STATUS_LED_FREQ` determines the LED blink frequency.
+ * Values range from 2 to 10.
+ */
+/**
+ * DOC: LED Status Config
+ *
+ * The Status LED uses two special configurations for common operations:
+ *
+ *   * CONFIG_STATUS_LED_BOOT is the LED that lights up when the board is booting.
+ *   * CONFIG_STATUS_LED_ACTIVITY is the LED that lights and blinks during activity
+ *     (e.g., file transfer, flash write).
+ *
+ * The values set in these configurations refer to the LED ID previously set.
+ *
+ * - ``CONFIG_STATUS_LED_RED=0`` will refer to the option ``CONFIG_STATUS_LED_BIT``.
+ * - ``CONFIG_STATUS_LED_RED=1`` will refer to the option ``CONFIG_STATUS_LED_BIT1``.
+ * - ``CONFIG_STATUS_LED_RED=2`` will refer to the option ``CONFIG_STATUS_LED_BIT2``.
+ * - ...
+ */
+/**
+ * DOC: LED Colour Config
+ *
+ * The Status LED exposes specific configurations for LEDs of different colors.
+ *
+ * The values set in these configurations refer to the LED ID previously set.
+ *
+ * - ``CONFIG_STATUS_LED_RED=0`` will refer to the option ``CONFIG_STATUS_LED_BIT``.
+ * - ``CONFIG_STATUS_LED_RED=1`` will refer to the option ``CONFIG_STATUS_LED_BIT1``.
+ * - ``CONFIG_STATUS_LED_RED=2`` will refer to the option ``CONFIG_STATUS_LED_BIT2``.
+ * - ...
+ *
+ * Supported colors are:
+ *   * red
+ *   * green
+ *   * yellow
+ *   * blue
+ *   * white
+ */
+
 #ifdef CONFIG_LED_STATUS
 
 #define LED_STATUS_PERIOD	(CONFIG_SYS_HZ / CONFIG_LED_STATUS_FREQ)
@@ -35,11 +119,49 @@
 #define LED_STATUS_PERIOD5	(CONFIG_SYS_HZ / CONFIG_LED_STATUS_FREQ5)
 #endif /* CONFIG_LED_STATUS5 */
 
+/**
+ * void status_led_init - Init the Status LED with all the required structs.
+ */
 void status_led_init(void);
+/**
+ * void status_led_tick - Blink each LED that is currently set in blinking
+ *   mode
+ * @timestamp: currently unused
+ */
 void status_led_tick(unsigned long timestamp);
+/**
+ * void status_led_set - Set the LED ID passed as first arg to the mode set
+ * @led: reference to the Status LED ID
+ * @state: state to set the LED to
+ *
+ * Modes for state arE:
+ *   * CONFIG_LED_STATUS_OFF (LED off)
+ *   * CONFIG_LED_STATUS_ON (LED on)
+ *   * CONFIG_LED_STATUS_BLINK (LED initially on, expected to blink)
+ */
 void status_led_set(int led, int state);
+/**
+ * void status_led_toggle - toggle the LED ID
+ * @led: reference to the Status LED ID
+ *
+ * Toggle the LED ID passed as first arg. If it's ON became OFF, if it's
+ * OFF became ON.
+ */
 void status_led_toggle(int led);
+/**
+ * void status_led_activity_start - start a LED activity
+ * @led: reference to the Status LED ID
+ *
+ * Set the Status LED ON and start the Cyclic to make the LED blink at
+ * the configured freq.
+ */
 void status_led_activity_start(int led);
+/**
+ * void status_led_activity_stop - stop a LED activity
+ * @led: reference to the Status LED ID
+ *
+ * Stop and free the Cyclic and turn the LED OFF.
+ */
 void status_led_activity_stop(int led);
 
 /*****  MVS v1  **********************************************************/
@@ -62,9 +184,61 @@ void status_led_activity_stop(int led);
 /* led_id_t is unsigned long mask */
 typedef unsigned long led_id_t;
 
+/**
+ * DOC: Required API
+ *
+ * The Status LED requires the following API functions to operate correctly
+ * and compile:
+ *
+ * - ``__led_toggle``: Low-level function to toggle the LED at the specified
+ *   mask.
+ * - ``__led_init``: Initializes the Status LED, sets up required tables, and
+ *   configures registers.
+ * - ``__led_set``: Low-level function to set the state of the LED at the
+ *   specified mask.
+ * - ``__led_blink``: Low-level function to set the LED to blink at the
+ *   specified frequency.
+ *
+ * The Status LED also provides optional functions to control colored LEDs.
+ * Each supported LED color has corresponding ``_on`` and ``_off`` functions.
+ *
+ * There is also support for ``coloured_LED_init`` for LEDs that provide
+ * multiple colors. (Currently, this is only used by ARM.)
+ *
+ * Each function is weakly defined and should be implemented in the
+ * board-specific source file. (This does not apply to the GPIO LED wrapper.)
+ */
+/**
+ * void __led_toggle - toggle LED at @mask
+ * @mask: opaque value to reference the LED
+ *
+ * Low-Level function to toggle the LED at mask.
+ */
 extern void __led_toggle (led_id_t mask);
+/**
+ * void __led_init - Init the LED at @mask
+ * @mask: opaque value to reference the LED
+ * @state: starting state of the LED
+ *
+ * Init the Status LED, init required tables, setup regs...
+ */
 extern void __led_init (led_id_t mask, int state);
+/**
+ * void __led_set - Set the LED at @mask
+ * @mask: opaque value to reference the LED
+ * @state: state to set the LED to
+ *
+ * Init the Status LED, init required tables, setup regs...
+ */
 extern void __led_set (led_id_t mask, int state);
+/**
+ * void __led_blink - Set the LED at @mask to HW blink
+ * @mask: opaque value to reference the LED
+ * @freq: freq to make the LED blink at
+ *
+ * Low-Level function to set the LED at HW blink by the
+ * passed freq.
+ */
 void __led_blink(led_id_t mask, int freq);
 #else
 # error Status LED configuration missing
@@ -77,20 +251,62 @@ void __led_blink(led_id_t mask, int freq);
 
 #endif	/* CONFIG_LED_STATUS	*/
 
-/*
- * Coloured LEDs API
+/**
+ * DOC: Coloured LED API
+ *
+ * Status LED expose optional functions to control coloured LED.
+ * Each LED color supported expose _on and _off function.
+ *
+ * There is also support for coloured_LED_init for LED that provide
+ * multiple colours. (currently only used by ARM)
+ *
+ * Each function is weakly defined and should be defined in the board
+ * specific source. (doesn't apply for GPIO LED wrapper)
  */
 #ifndef	__ASSEMBLY__
+/**
+ * void coloured_LED_init - Init multi colour LED
+ */
 void coloured_LED_init(void);
+/**
+ * void red_led_on - Turn LED Red on
+ */
 void red_led_on(void);
+/**
+ * void red_led_off - Turn LED Red off
+ */
 void red_led_off(void);
+/**
+ * void green_led_on - Turn LED Green on
+ */
 void green_led_on(void);
+/**
+ * void green_led_off - Turn LED Green off
+ */
 void green_led_off(void);
+/**
+ * void yellow_led_on - Turn LED Yellow on
+ */
 void yellow_led_on(void);
+/**
+ * void yellow_led_off - Turn LED Yellow off
+ */
 void yellow_led_off(void);
+/**
+ * void blue_led_on - Turn LED Blue on
+ */
 void blue_led_on(void);
+/**
+ * void blue_led_off - Turn LED Blue off
+ */
 void blue_led_off(void);
+/**
+ * void white_led_on - Turn LED White on
+ */
 void white_led_on(void);
+/**
+ * void white_led_off - Turn LED White off
+ */
 void white_led_off(void);
 #else
 	.extern LED_init
-- 
2.43.0

Re: [PATCH v4 9/9] doc: convert README.LED to .rst documentation

[Heinrich Schuchardt]

On 6/20/24 01:03, Christian Marangi wrote:
> Convert README.LED to .rst documentation and include all the relevant
> documentation in the status_led.h.
>
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> ---
>   doc/README.LED         |  77 --------------
>   doc/api/index.rst      |   1 +
>   doc/api/status_led.rst |  35 +++++++
>   include/status_led.h   | 224 ++++++++++++++++++++++++++++++++++++++++-
>   4 files changed, 256 insertions(+), 81 deletions(-)
>   delete mode 100644 doc/README.LED
>   create mode 100644 doc/api/status_led.rst
>
> diff --git a/doc/README.LED b/doc/README.LED
> deleted file mode 100644
> index c21c9d53ec3..00000000000
> --- a/doc/README.LED
> +++ /dev/null
> @@ -1,77 +0,0 @@
> -Status LED
> -========================================
> -
> -This README describes the status LED API.
> -
> -The API is defined by the include file include/status_led.h
> -
> -The first step is to enable CONFIG_LED_STATUS in menuconfig:
> -> Device Drivers > LED Support.
> -
> -If the LED support is only for specific board, enable
> -CONFIG_LED_STATUS_BOARD_SPECIFIC in the menuconfig.
> -
> -Status LEDS 0 to 5 are enabled by the following configurations at menuconfig:
> -CONFIG_STATUS_LED0, CONFIG_STATUS_LED1, ... CONFIG_STATUS_LED5
> -
> -The following should be configured for each of the enabled LEDs:
> -CONFIG_STATUS_LED_BIT<n>
> -CONFIG_STATUS_LED_STATE<n>
> -CONFIG_STATUS_LED_FREQ<n>
> -Where <n> is an integer 1 through 5 (empty for 0).
> -
> -CONFIG_STATUS_LED_BIT is passed into the __led_* functions to identify which LED
> -is being acted on. As such, the value choose must be unique with with respect to
> -the other CONFIG_STATUS_LED_BIT's. Mapping the value to a physical LED is the
> -reponsiblity of the __led_* function.
> -
> -CONFIG_STATUS_LED_STATE is the initial state of the LED. It should be set to one
> -of these values: CONFIG_LED_STATUS_OFF or CONFIG_LED_STATUS_ON.
> -
> -CONFIG_STATUS_LED_FREQ determines the LED blink frequency.
> -Values range from 2 to 10.
> -
> -Some other LED macros
> ----------------------
> -
> -CONFIG_STATUS_LED_BOOT is the LED to light when the board is booting.
> -This must be a valid LED number (0-5).
> -
> -CONFIG_STATUS_LED_RED is the red LED. It is used to signal errors. This must be
> -a valid LED number (0-5). Other similar color LED's macros are
> -CONFIG_STATUS_LED_GREEN, CONFIG_STATUS_LED_YELLOW and CONFIG_STATUS_LED_BLUE.
> -
> -General LED functions
> ----------------------
> -The following functions should be defined:
> -
> -__led_init is called once to initialize the LED to CONFIG_STATUS_LED_STATE.
> -One time start up code should be placed here.
> -
> -__led_set is called to change the state of the LED.
> -
> -__led_toggle is called to toggle the current state of the LED.
> -
> -Colour LED
> -========================================
> -
> -Colour LED's are at present only used by ARM.
> -
> -The functions names explain their purpose.
> -
> -coloured_LED_init
> -red_LED_on
> -red_LED_off
> -green_LED_on
> -green_LED_off
> -yellow_LED_on
> -yellow_LED_off
> -blue_LED_on
> -blue_LED_off
> -
> -These are weakly defined in arch/arm/lib/board.c to noops. Where applicable, define
> -these functions in the board specific source.
> -
> -TBD : Describe older board dependent macros similar to what is done for
> -
> -TBD : Describe general support via asm/status_led.h
> diff --git a/doc/api/index.rst b/doc/api/index.rst
> index 51b2013af36..d40e90801d1 100644
> --- a/doc/api/index.rst
> +++ b/doc/api/index.rst
> @@ -22,6 +22,7 @@ U-Boot API documentation
>      rng
>      sandbox
>      serial
> +   status_led
>      sysreset
>      timer
>      unicode
> diff --git a/doc/api/status_led.rst b/doc/api/status_led.rst
> new file mode 100644
> index 00000000000..44bbea47157
> --- /dev/null
> +++ b/doc/api/status_led.rst
> @@ -0,0 +1,35 @@
> +.. SPDX-License-Identifier: GPL-2.0+
> +
> +Status LED
> +==========
> +
> +.. kernel-doc:: include/status_led.h
> +   :doc: Overview
> +
> +CONFIG_STATUS_LED Description
> +-----------------------------
> +
> +.. kernel-doc:: include/status_led.h
> +   :doc: CONFIG Description
> +
> +Special Status LED Configs
> +--------------------------
> +.. kernel-doc:: include/status_led.h
> +   :doc: LED Status Config
> +
> +Colour Status LED Configs
> +-------------------------
> +.. kernel-doc:: include/status_led.h
> +   :doc: LED Colour Config
> +
> +Required API
> +------------
> +
> +.. kernel-doc:: include/status_led.h
> +   :doc: Required API
> +
> +Status LED API
> +--------------
> +
> +.. kernel-doc:: include/status_led.h
> +   :internal:
> diff --git a/include/status_led.h b/include/status_led.h
> index 7de40551621..6893d1d68e0 100644
> --- a/include/status_led.h
> +++ b/include/status_led.h
> @@ -4,18 +4,102 @@
>    * Wolfgang Denk, DENX Software Engineering, wd@denx.de.
>    */
>
> -/*
> - * The purpose of this code is to signal the operational status of a
> +/**
> + * DOC: Overview
> + *
> + * Status LED is a Low-Level way to handle LEDs to signal state of the

Status LEDs can be used to signal the operational status of a

> + * bootloader, for example boot progress, file transfer fail, activity
> + * of some sort like tftp transfer, mtd write/erase.

for example boot progress, file transfer failure, or ongoing activity
like tftp transfer or mtd write/erase.

> + *
> + * The original usage these API were to signal the operational status of a

One usage of this API is signalling the operational status ...

>    * target which usually boots over the network; while running in
>    * PCBoot, a status LED is blinking. As soon as a valid BOOTP reply

What does "PCBoot" refer to? Do you mean U-Boot?

>    * message has been received, the LED is turned off. The Linux
>    * kernel, once it is running, will start blinking the LED again,
>    * with another frequency.
> + *
> + * Status LED require support Low Level and the board to implement



> + * the specific functions to correctly works.
>    */
>
>   #ifndef _STATUS_LED_H_
>   #define	_STATUS_LED_H_
>
> +/**
> + * DOC: CONFIG Description

DOC: Configuration

> + *
> + * Enable `CONFIG_LED_STATUS` to support the Status LED under
> + * > Device Drivers > LED Support.
> + *
> + * The Status LED can be defined in various ways, but most of the time,
> + * specific functions will need to be defined in the board file.
> + * If this is the case, enable `CONFIG_LED_STATUS_BOARD_SPECIFIC`.

CONFIG_LED_STATUS_BOARD_SPECIFIC is not used in any defconfig.

GPIO is the typical case and board-specific the exception.

> + *
> + * If the LEDs are GPIO-driven, you can use the GPIO wrapper driver
> + * instead of defining specific board functions.
> + * If this is the case, enable `CONFIG_LED_STATUS_GPIO`.
> + * (Note that `CONFIG_LED_STATUS_BOARD_SPECIFIC` is also required.)
> + *
> + * The Status LED allows defining up to six different LEDs, from 0 to 5,
> + * with the following configurations:
> + * `CONFIG_STATUS_LED`, `CONFIG_STATUS_LED1`, ..., `CONFIG_STATUS_LED5`.
> + *
> + * For each LED, the following options are required:
> + *  * `CONFIG_STATUS_LED_BIT<n>`
> + *  * `CONFIG_STATUS_LED_STATE<n>`
> + *  * `CONFIG_STATUS_LED_FREQ<n>`
> + *
> + * Where `<n>` is an integer from 1 through 5. (Note that LED 0 doesn't have the
> + * integer suffix.)
> + *
> + * `CONFIG_STATUS_LED_BIT` is passed into the `__led_*` functions to identify
> + * which LED is being acted on. The value is opaque, meaning it depends on how
> + * the low-level API handles this value. It can be an ID to reference the LED
> + * internally, or in the case of the GPIO wrapper, it's the GPIO number of the LED.
> + * Mapping the value to a physical LED is the responsibility of the `__led_*` function.
> + *
> + * `CONFIG_STATUS_LED_STATE` sets the initial state of the LED. It should be set
> + * to one of these values: `CONFIG_LED_STATUS_OFF` or `CONFIG_LED_STATUS_ON`.
> + *
> + * `CONFIG_STATUS_LED_FREQ` determines the LED blink frequency.
> + * Values range from 2 to 10.
> + */
> +/**
> + * DOC: LED Status Config
> + *
> + * The Status LED uses two special configurations for common operations:
> + *
> + *   * CONFIG_STATUS_LED_BOOT is the LED that lights up when the board is booting.
> + *   * CONFIG_STATUS_LED_ACTIVITY is the LED that lights and blinks during activity
> + *     (e.g., file transfer, flash write).
> + *
> + * The values set in these configurations refer to the LED ID previously set.
> + *
> + * - ``CONFIG_STATUS_LED_RED=0`` will refer to the option ``CONFIG_STATUS_LED_BIT``.
> + * - ``CONFIG_STATUS_LED_RED=1`` will refer to the option ``CONFIG_STATUS_LED_BIT1``.
> + * - ``CONFIG_STATUS_LED_RED=2`` will refer to the option ``CONFIG_STATUS_LED_BIT2``.
> + * - ...
> + */
> +/**
> + * DOC: LED Colour Config
> + *
> + * The Status LED exposes specific configurations for LEDs of different colors.
> + *
> + * The values set in these configurations refer to the LED ID previously set.
> + *
> + * - ``CONFIG_STATUS_LED_RED=0`` will refer to the option ``CONFIG_STATUS_LED_BIT``.
> + * - ``CONFIG_STATUS_LED_RED=1`` will refer to the option ``CONFIG_STATUS_LED_BIT1``.
> + * - ``CONFIG_STATUS_LED_RED=2`` will refer to the option ``CONFIG_STATUS_LED_BIT2``.
> + * - ...
> + *
> + * Supported colors are:
> + *   * red
> + *   * green
> + *   * yellow
> + *   * blue
> + *   * white
> + */
> +
>   #ifdef CONFIG_LED_STATUS
>
>   #define LED_STATUS_PERIOD	(CONFIG_SYS_HZ / CONFIG_LED_STATUS_FREQ)
> @@ -35,11 +119,49 @@
>   #define LED_STATUS_PERIOD5	(CONFIG_SYS_HZ / CONFIG_LED_STATUS_FREQ5)
>   #endif /* CONFIG_LED_STATUS5 */
>
> +/**
> + * void status_led_init - Init the Status LED with all the required structs.
> + */
>   void status_led_init(void);
> +/**
> + * void status_led_tick - Blink each LED that is currently set in blinking
> + *   mode
> + * @timestamp: currently unused
> + */
>   void status_led_tick(unsigned long timestamp);
> +/**
> + * void status_led_set - Set the LED ID passed as first arg to the mode set
> + * @led: reference to the Status LED ID
> + * @state: state to set the LED to
> + *
> + * Modes for state arE:
> + *   * CONFIG_LED_STATUS_OFF (LED off)
> + *   * CONFIG_LED_STATUS_ON (LED on)
> + *   * CONFIG_LED_STATUS_BLINK (LED initially on, expected to blink)
> + */
>   void status_led_set(int led, int state);
> +/**
> + * void status_led_toggle - toggle the LED ID
> + * @led: reference to the Status LED ID
> + *
> + * Toggle the LED ID passed as first arg. If it's ON became OFF, if it's
> + * OFF became ON.
> + */
>   void status_led_toggle(int led);
> +/**
> + * void status_led_activity_start - start a LED activity
> + * @led: reference to the Status LED ID
> + *
> + * Set the Status LED ON and start the Cyclic to make the LED blink at
> + * the configured freq.
> + */
>   void status_led_activity_start(int led);
> +/**
> + * void status_led_activity_stop - stop a LED activity
> + * @led: reference to the Status LED ID
> + *
> + * Stop and free the Cyclic and turn the LED OFF.

'Cyclic' is and adjective and cannot stand alone. Do you mean:

Stop and free the cyclic function and turn off the LED.

> + */
>   void status_led_activity_stop(int led);
>
>   /*****  MVS v1  **********************************************************/

We don't have 'config MVS' in any Kconfig. Please remove the dead code.

> @@ -62,9 +184,61 @@ void status_led_activity_stop(int led);
>   /* led_id_t is unsigned long mask */
>   typedef unsigned long led_id_t;
>
> +/**
> + * DOC: Required API
> + *
> + * The Status LED requires the following API functions to operate correctly

The following functions are implemented by the GPIO LED driver. If using
CONFIG_LED_STATUS_BOARD_SPECIFIC=y, they have to be implemented by the
board code.

> + * and compile:
> + *
> + * - ``__led_toggle``: Low-level function to toggle the LED at the specified
> + *   mask.
> + * - ``__led_init``: Initializes the Status LED, sets up required tables, and
> + *   configures registers.
> + * - ``__led_set``: Low-level function to set the state of the LED at the
> + *   specified mask.
> + * - ``__led_blink``: Low-level function to set the LED to blink at the
> + *   specified frequency.
> + *
> + * The Status LED also provides optional functions to control colored LEDs.

Do you mean

Controlling colored LEDs requires the additional functions see :doc:
`Coloured LED API`.

> + * Each supported LED color has corresponding ``_on`` and ``_off`` functions.
> + *
> + * There is also support for ``coloured_LED_init`` for LEDs that provide
> + * multiple colors. (Currently, this is only used by ARM.)

We have hundreds of ARM based boards. Could you be a bit more specific,
please.

> + *
> + * Each function is weakly defined and should be implemented in the
> + * board-specific source file. (This does not apply to the GPIO LED wrapper.)
> + */
> +/**
> + * void __led_toggle - toggle LED at @mask
> + * @mask: opaque value to reference the LED
> + *
> + * Low-Level function to toggle the LED at mask.
> + */
>   extern void __led_toggle (led_id_t mask);
> +/**
> + * void __led_init - Init the LED at @mask
> + * @mask: opaque value to reference the LED
> + * @state: starting state of the LED
> + *
> + * Init the Status LED, init required tables, setup regs...
> + */
>   extern void __led_init (led_id_t mask, int state);
> +/**
> + * void __led_set - Set the LED at @mask
> + * @mask: opaque value to reference the LED
> + * @state: state to set the LED to
> + *
> + * Init the Status LED, init required tables, setup regs...
> + */
>   extern void __led_set (led_id_t mask, int state);
> +/**
> + * void __led_blink - Set the LED at @mask to HW blink
> + * @mask: opaque value to reference the LED
> + * @freq: freq to make the LED blink at
> + *
> + * Low-Level function to set the LED at HW blink by the
> + * passed freq.
> + */
>   void __led_blink(led_id_t mask, int freq);
>   #else
>   # error Status LED configuration missing
> @@ -77,20 +251,62 @@ void __led_blink(led_id_t mask, int freq);
>
>   #endif	/* CONFIG_LED_STATUS	*/
>
> -/*
> - * Coloured LEDs API
> +/**
> + * DOC: Coloured LED API
> + *
> + * Status LED expose optional functions to control coloured LED.

The status LED API uses optional functions ...

> + * Each LED color supported expose _on and _off function.
> + *
> + * There is also support for coloured_LED_init for LED that provide
> + * multiple colours. (currently only used by ARM)

No clue why you refer to ARM. I found only the following files that
providing board specific implementations:

board/beagle/beagle/led.c
board/siemens/corvus/board.c

Best regards

Heinrich

> + *
> + * Each function is weakly defined and should be defined in the board
> + * specific source. (doesn't apply for GPIO LED wrapper)
>    */
>   #ifndef	__ASSEMBLY__
> +/**
> + * void coloured_LED_init - Init multi colour LED
> + */
>   void coloured_LED_init(void);
> +/**
> + * void red_led_on - Turn LED Red on
> + */
>   void red_led_on(void);
> +/**
> + * void red_led_off - Turn LED Red off
> + */
>   void red_led_off(void);
> +/**
> + * void green_led_on - Turn LED Green on
> + */
>   void green_led_on(void);
> +/**
> + * void green_led_off - Turn LED Green off
> + */
>   void green_led_off(void);
> +/**
> + * void yellow_led_on - Turn LED Yellow on
> + */
>   void yellow_led_on(void);
> +/**
> + * void yellow_led_off - Turn LED Yellow off
> + */
>   void yellow_led_off(void);
> +/**
> + * void blue_led_on - Turn LED Blue on
> + */
>   void blue_led_on(void);
> +/**
> + * void blue_led_off - Turn LED Blue off
> + */
>   void blue_led_off(void);
> +/**
> + * void white_led_on - Turn LED White on
> + */
>   void white_led_on(void);
> +/**
> + * void white_led_off - Turn LED White off
> + */
>   void white_led_off(void);
>   #else
>   	.extern LED_init

Re: [PATCH v4 9/9] doc: convert README.LED to .rst documentation

[Christian Marangi]

On Thu, Jun 20, 2024 at 08:13:34AM +0200, Heinrich Schuchardt wrote:
> On 6/20/24 01:03, Christian Marangi wrote:
> > Convert README.LED to .rst documentation and include all the relevant
> > documentation in the status_led.h.
> > 
> > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > ---
> >   doc/README.LED         |  77 --------------
> >   doc/api/index.rst      |   1 +
> >   doc/api/status_led.rst |  35 +++++++
> >   include/status_led.h   | 224 ++++++++++++++++++++++++++++++++++++++++-
> >   4 files changed, 256 insertions(+), 81 deletions(-)
> >   delete mode 100644 doc/README.LED
> >   create mode 100644 doc/api/status_led.rst
> > 
> > diff --git a/doc/README.LED b/doc/README.LED
> > deleted file mode 100644
> > index c21c9d53ec3..00000000000
> > --- a/doc/README.LED
> > +++ /dev/null
> > @@ -1,77 +0,0 @@
> > -Status LED
> > -========================================
> > -
> > -This README describes the status LED API.
> > -
> > -The API is defined by the include file include/status_led.h
> > -
> > -The first step is to enable CONFIG_LED_STATUS in menuconfig:
> > -> Device Drivers > LED Support.
> > -
> > -If the LED support is only for specific board, enable
> > -CONFIG_LED_STATUS_BOARD_SPECIFIC in the menuconfig.
> > -
> > -Status LEDS 0 to 5 are enabled by the following configurations at menuconfig:
> > -CONFIG_STATUS_LED0, CONFIG_STATUS_LED1, ... CONFIG_STATUS_LED5
> > -
> > -The following should be configured for each of the enabled LEDs:
> > -CONFIG_STATUS_LED_BIT<n>
> > -CONFIG_STATUS_LED_STATE<n>
> > -CONFIG_STATUS_LED_FREQ<n>
> > -Where <n> is an integer 1 through 5 (empty for 0).
> > -
> > -CONFIG_STATUS_LED_BIT is passed into the __led_* functions to identify which LED
> > -is being acted on. As such, the value choose must be unique with with respect to
> > -the other CONFIG_STATUS_LED_BIT's. Mapping the value to a physical LED is the
> > -reponsiblity of the __led_* function.
> > -
> > -CONFIG_STATUS_LED_STATE is the initial state of the LED. It should be set to one
> > -of these values: CONFIG_LED_STATUS_OFF or CONFIG_LED_STATUS_ON.
> > -
> > -CONFIG_STATUS_LED_FREQ determines the LED blink frequency.
> > -Values range from 2 to 10.
> > -
> > -Some other LED macros
> > ----------------------
> > -
> > -CONFIG_STATUS_LED_BOOT is the LED to light when the board is booting.
> > -This must be a valid LED number (0-5).
> > -
> > -CONFIG_STATUS_LED_RED is the red LED. It is used to signal errors. This must be
> > -a valid LED number (0-5). Other similar color LED's macros are
> > -CONFIG_STATUS_LED_GREEN, CONFIG_STATUS_LED_YELLOW and CONFIG_STATUS_LED_BLUE.
> > -
> > -General LED functions
> > ----------------------
> > -The following functions should be defined:
> > -
> > -__led_init is called once to initialize the LED to CONFIG_STATUS_LED_STATE.
> > -One time start up code should be placed here.
> > -
> > -__led_set is called to change the state of the LED.
> > -
> > -__led_toggle is called to toggle the current state of the LED.
> > -
> > -Colour LED
> > -========================================
> > -
> > -Colour LED's are at present only used by ARM.
> > -
> > -The functions names explain their purpose.
> > -
> > -coloured_LED_init
> > -red_LED_on
> > -red_LED_off
> > -green_LED_on
> > -green_LED_off
> > -yellow_LED_on
> > -yellow_LED_off
> > -blue_LED_on
> > -blue_LED_off
> > -
> > -These are weakly defined in arch/arm/lib/board.c to noops. Where applicable, define
> > -these functions in the board specific source.
> > -
> > -TBD : Describe older board dependent macros similar to what is done for
> > -
> > -TBD : Describe general support via asm/status_led.h
> > diff --git a/doc/api/index.rst b/doc/api/index.rst
> > index 51b2013af36..d40e90801d1 100644
> > --- a/doc/api/index.rst
> > +++ b/doc/api/index.rst
> > @@ -22,6 +22,7 @@ U-Boot API documentation
> >      rng
> >      sandbox
> >      serial
> > +   status_led
> >      sysreset
> >      timer
> >      unicode
> > diff --git a/doc/api/status_led.rst b/doc/api/status_led.rst
> > new file mode 100644
> > index 00000000000..44bbea47157
> > --- /dev/null
> > +++ b/doc/api/status_led.rst
> > @@ -0,0 +1,35 @@
> > +.. SPDX-License-Identifier: GPL-2.0+
> > +
> > +Status LED
> > +==========
> > +
> > +.. kernel-doc:: include/status_led.h
> > +   :doc: Overview
> > +
> > +CONFIG_STATUS_LED Description
> > +-----------------------------
> > +
> > +.. kernel-doc:: include/status_led.h
> > +   :doc: CONFIG Description
> > +
> > +Special Status LED Configs
> > +--------------------------
> > +.. kernel-doc:: include/status_led.h
> > +   :doc: LED Status Config
> > +
> > +Colour Status LED Configs
> > +-------------------------
> > +.. kernel-doc:: include/status_led.h
> > +   :doc: LED Colour Config
> > +
> > +Required API
> > +------------
> > +
> > +.. kernel-doc:: include/status_led.h
> > +   :doc: Required API
> > +
> > +Status LED API
> > +--------------
> > +
> > +.. kernel-doc:: include/status_led.h
> > +   :internal:
> > diff --git a/include/status_led.h b/include/status_led.h
> > index 7de40551621..6893d1d68e0 100644
> > --- a/include/status_led.h
> > +++ b/include/status_led.h
> > @@ -4,18 +4,102 @@
> >    * Wolfgang Denk, DENX Software Engineering, wd@denx.de.
> >    */
> > 
> > -/*
> > - * The purpose of this code is to signal the operational status of a
> > +/**
> > + * DOC: Overview
> > + *
> > + * Status LED is a Low-Level way to handle LEDs to signal state of the
> 
> Status LEDs can be used to signal the operational status of a
> 
> > + * bootloader, for example boot progress, file transfer fail, activity
> > + * of some sort like tftp transfer, mtd write/erase.
> 
> for example boot progress, file transfer failure, or ongoing activity
> like tftp transfer or mtd write/erase.
> 
> > + *
> > + * The original usage these API were to signal the operational status of a
> 
> One usage of this API is signalling the operational status ...
> 
> >    * target which usually boots over the network; while running in
> >    * PCBoot, a status LED is blinking. As soon as a valid BOOTP reply
> 
> What does "PCBoot" refer to? Do you mean U-Boot?
> 
> >    * message has been received, the LED is turned off. The Linux
> >    * kernel, once it is running, will start blinking the LED again,
> >    * with another frequency.
> > + *
> > + * Status LED require support Low Level and the board to implement
> 
> 
> 
> > + * the specific functions to correctly works.
> >    */
> > 
> >   #ifndef _STATUS_LED_H_
> >   #define	_STATUS_LED_H_
> > 
> > +/**
> > + * DOC: CONFIG Description
> 
> DOC: Configuration
> 
> > + *
> > + * Enable `CONFIG_LED_STATUS` to support the Status LED under
> > + * > Device Drivers > LED Support.
> > + *
> > + * The Status LED can be defined in various ways, but most of the time,
> > + * specific functions will need to be defined in the board file.
> > + * If this is the case, enable `CONFIG_LED_STATUS_BOARD_SPECIFIC`.
> 
> CONFIG_LED_STATUS_BOARD_SPECIFIC is not used in any defconfig.
> 
> GPIO is the typical case and board-specific the exception.
> 
> > + *
> > + * If the LEDs are GPIO-driven, you can use the GPIO wrapper driver
> > + * instead of defining specific board functions.
> > + * If this is the case, enable `CONFIG_LED_STATUS_GPIO`.
> > + * (Note that `CONFIG_LED_STATUS_BOARD_SPECIFIC` is also required.)
> > + *
> > + * The Status LED allows defining up to six different LEDs, from 0 to 5,
> > + * with the following configurations:
> > + * `CONFIG_STATUS_LED`, `CONFIG_STATUS_LED1`, ..., `CONFIG_STATUS_LED5`.
> > + *
> > + * For each LED, the following options are required:
> > + *  * `CONFIG_STATUS_LED_BIT<n>`
> > + *  * `CONFIG_STATUS_LED_STATE<n>`
> > + *  * `CONFIG_STATUS_LED_FREQ<n>`
> > + *
> > + * Where `<n>` is an integer from 1 through 5. (Note that LED 0 doesn't have the
> > + * integer suffix.)
> > + *
> > + * `CONFIG_STATUS_LED_BIT` is passed into the `__led_*` functions to identify
> > + * which LED is being acted on. The value is opaque, meaning it depends on how
> > + * the low-level API handles this value. It can be an ID to reference the LED
> > + * internally, or in the case of the GPIO wrapper, it's the GPIO number of the LED.
> > + * Mapping the value to a physical LED is the responsibility of the `__led_*` function.
> > + *
> > + * `CONFIG_STATUS_LED_STATE` sets the initial state of the LED. It should be set
> > + * to one of these values: `CONFIG_LED_STATUS_OFF` or `CONFIG_LED_STATUS_ON`.
> > + *
> > + * `CONFIG_STATUS_LED_FREQ` determines the LED blink frequency.
> > + * Values range from 2 to 10.
> > + */
> > +/**
> > + * DOC: LED Status Config
> > + *
> > + * The Status LED uses two special configurations for common operations:
> > + *
> > + *   * CONFIG_STATUS_LED_BOOT is the LED that lights up when the board is booting.
> > + *   * CONFIG_STATUS_LED_ACTIVITY is the LED that lights and blinks during activity
> > + *     (e.g., file transfer, flash write).
> > + *
> > + * The values set in these configurations refer to the LED ID previously set.
> > + *
> > + * - ``CONFIG_STATUS_LED_RED=0`` will refer to the option ``CONFIG_STATUS_LED_BIT``.
> > + * - ``CONFIG_STATUS_LED_RED=1`` will refer to the option ``CONFIG_STATUS_LED_BIT1``.
> > + * - ``CONFIG_STATUS_LED_RED=2`` will refer to the option ``CONFIG_STATUS_LED_BIT2``.
> > + * - ...
> > + */
> > +/**
> > + * DOC: LED Colour Config
> > + *
> > + * The Status LED exposes specific configurations for LEDs of different colors.
> > + *
> > + * The values set in these configurations refer to the LED ID previously set.
> > + *
> > + * - ``CONFIG_STATUS_LED_RED=0`` will refer to the option ``CONFIG_STATUS_LED_BIT``.
> > + * - ``CONFIG_STATUS_LED_RED=1`` will refer to the option ``CONFIG_STATUS_LED_BIT1``.
> > + * - ``CONFIG_STATUS_LED_RED=2`` will refer to the option ``CONFIG_STATUS_LED_BIT2``.
> > + * - ...
> > + *
> > + * Supported colors are:
> > + *   * red
> > + *   * green
> > + *   * yellow
> > + *   * blue
> > + *   * white
> > + */
> > +
> >   #ifdef CONFIG_LED_STATUS
> > 
> >   #define LED_STATUS_PERIOD	(CONFIG_SYS_HZ / CONFIG_LED_STATUS_FREQ)
> > @@ -35,11 +119,49 @@
> >   #define LED_STATUS_PERIOD5	(CONFIG_SYS_HZ / CONFIG_LED_STATUS_FREQ5)
> >   #endif /* CONFIG_LED_STATUS5 */
> > 
> > +/**
> > + * void status_led_init - Init the Status LED with all the required structs.
> > + */
> >   void status_led_init(void);
> > +/**
> > + * void status_led_tick - Blink each LED that is currently set in blinking
> > + *   mode
> > + * @timestamp: currently unused
> > + */
> >   void status_led_tick(unsigned long timestamp);
> > +/**
> > + * void status_led_set - Set the LED ID passed as first arg to the mode set
> > + * @led: reference to the Status LED ID
> > + * @state: state to set the LED to
> > + *
> > + * Modes for state arE:
> > + *   * CONFIG_LED_STATUS_OFF (LED off)
> > + *   * CONFIG_LED_STATUS_ON (LED on)
> > + *   * CONFIG_LED_STATUS_BLINK (LED initially on, expected to blink)
> > + */
> >   void status_led_set(int led, int state);
> > +/**
> > + * void status_led_toggle - toggle the LED ID
> > + * @led: reference to the Status LED ID
> > + *
> > + * Toggle the LED ID passed as first arg. If it's ON became OFF, if it's
> > + * OFF became ON.
> > + */
> >   void status_led_toggle(int led);
> > +/**
> > + * void status_led_activity_start - start a LED activity
> > + * @led: reference to the Status LED ID
> > + *
> > + * Set the Status LED ON and start the Cyclic to make the LED blink at
> > + * the configured freq.
> > + */
> >   void status_led_activity_start(int led);
> > +/**
> > + * void status_led_activity_stop - stop a LED activity
> > + * @led: reference to the Status LED ID
> > + *
> > + * Stop and free the Cyclic and turn the LED OFF.
> 
> 'Cyclic' is and adjective and cannot stand alone. Do you mean:
> 
> Stop and free the cyclic function and turn off the LED.
> 
> > + */
> >   void status_led_activity_stop(int led);
> > 
> >   /*****  MVS v1  **********************************************************/
> 
> We don't have 'config MVS' in any Kconfig. Please remove the dead code.
> 
> > @@ -62,9 +184,61 @@ void status_led_activity_stop(int led);
> >   /* led_id_t is unsigned long mask */
> >   typedef unsigned long led_id_t;
> > 
> > +/**
> > + * DOC: Required API
> > + *
> > + * The Status LED requires the following API functions to operate correctly
> 
> The following functions are implemented by the GPIO LED driver. If using
> CONFIG_LED_STATUS_BOARD_SPECIFIC=y, they have to be implemented by the
> board code.
>

Well CONFIG_LED_STATUS_BOARD_SPECIFIC is mandatory for these function to
be exporsed. GPIO is just a way to provide these function using the GPIO
common implementation. Maybe I will make it more clear but it should be
also written at the start of the DOC.

> > + * and compile:
> > + *
> > + * - ``__led_toggle``: Low-level function to toggle the LED at the specified
> > + *   mask.
> > + * - ``__led_init``: Initializes the Status LED, sets up required tables, and
> > + *   configures registers.
> > + * - ``__led_set``: Low-level function to set the state of the LED at the
> > + *   specified mask.
> > + * - ``__led_blink``: Low-level function to set the LED to blink at the
> > + *   specified frequency.
> > + *
> > + * The Status LED also provides optional functions to control colored LEDs.
> 
> Do you mean
> 
> Controlling colored LEDs requires the additional functions see :doc:
> `Coloured LED API`.
> 
> > + * Each supported LED color has corresponding ``_on`` and ``_off`` functions.
> > + *
> > + * There is also support for ``coloured_LED_init`` for LEDs that provide
> > + * multiple colors. (Currently, this is only used by ARM.)
> 
> We have hundreds of ARM based boards. Could you be a bit more specific,
> please.
> 

Well the only one used is the RED colour and it's in arm/lib/crt0.S in
the ASM code.

> > + *
> > + * Each function is weakly defined and should be implemented in the
> > + * board-specific source file. (This does not apply to the GPIO LED wrapper.)
> > + */
> > +/**
> > + * void __led_toggle - toggle LED at @mask
> > + * @mask: opaque value to reference the LED
> > + *
> > + * Low-Level function to toggle the LED at mask.
> > + */
> >   extern void __led_toggle (led_id_t mask);
> > +/**
> > + * void __led_init - Init the LED at @mask
> > + * @mask: opaque value to reference the LED
> > + * @state: starting state of the LED
> > + *
> > + * Init the Status LED, init required tables, setup regs...
> > + */
> >   extern void __led_init (led_id_t mask, int state);
> > +/**
> > + * void __led_set - Set the LED at @mask
> > + * @mask: opaque value to reference the LED
> > + * @state: state to set the LED to
> > + *
> > + * Init the Status LED, init required tables, setup regs...
> > + */
> >   extern void __led_set (led_id_t mask, int state);
> > +/**
> > + * void __led_blink - Set the LED at @mask to HW blink
> > + * @mask: opaque value to reference the LED
> > + * @freq: freq to make the LED blink at
> > + *
> > + * Low-Level function to set the LED at HW blink by the
> > + * passed freq.
> > + */
> >   void __led_blink(led_id_t mask, int freq);
> >   #else
> >   # error Status LED configuration missing
> > @@ -77,20 +251,62 @@ void __led_blink(led_id_t mask, int freq);
> > 
> >   #endif	/* CONFIG_LED_STATUS	*/
> > 
> > -/*
> > - * Coloured LEDs API
> > +/**
> > + * DOC: Coloured LED API
> > + *
> > + * Status LED expose optional functions to control coloured LED.
> 
> The status LED API uses optional functions ...
> 
> > + * Each LED color supported expose _on and _off function.
> > + *
> > + * There is also support for coloured_LED_init for LED that provide
> > + * multiple colours. (currently only used by ARM)
> 
> No clue why you refer to ARM. I found only the following files that
> providing board specific implementations:
> 

Another case of arm/lib/crt0.S

These confusing thing are picked from the old txt just to not drop info
and try to do a 1:1 conversion.

> board/beagle/beagle/led.c
> board/siemens/corvus/board.c
> 
> Best regards
> 
> Heinrich
> 
> > + *
> > + * Each function is weakly defined and should be defined in the board
> > + * specific source. (doesn't apply for GPIO LED wrapper)
> >    */
> >   #ifndef	__ASSEMBLY__
> > +/**
> > + * void coloured_LED_init - Init multi colour LED
> > + */
> >   void coloured_LED_init(void);
> > +/**
> > + * void red_led_on - Turn LED Red on
> > + */
> >   void red_led_on(void);
> > +/**
> > + * void red_led_off - Turn LED Red off
> > + */
> >   void red_led_off(void);
> > +/**
> > + * void green_led_on - Turn LED Green on
> > + */
> >   void green_led_on(void);
> > +/**
> > + * void green_led_off - Turn LED Green off
> > + */
> >   void green_led_off(void);
> > +/**
> > + * void yellow_led_on - Turn LED Yellow on
> > + */
> >   void yellow_led_on(void);
> > +/**
> > + * void yellow_led_off - Turn LED Yellow off
> > + */
> >   void yellow_led_off(void);
> > +/**
> > + * void blue_led_on - Turn LED Blue on
> > + */
> >   void blue_led_on(void);
> > +/**
> > + * void blue_led_off - Turn LED Blue off
> > + */
> >   void blue_led_off(void);
> > +/**
> > + * void white_led_on - Turn LED White on
> > + */
> >   void white_led_on(void);
> > +/**
> > + * void white_led_off - Turn LED White off
> > + */
> >   void white_led_off(void);
> >   #else
> >   	.extern LED_init
> 

-- 
	Ansuel

Re: [PATCH v4 9/9] doc: convert README.LED to .rst documentation

[Heinrich Schuchardt]

On 6/20/24 06:37, Christian Marangi wrote:
> On Thu, Jun 20, 2024 at 08:13:34AM +0200, Heinrich Schuchardt wrote:
>> On 6/20/24 01:03, Christian Marangi wrote:
>>> Convert README.LED to .rst documentation and include all the relevant
>>> documentation in the status_led.h.
>>>
>>> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
>>> ---
>>>    doc/README.LED         |  77 --------------
>>>    doc/api/index.rst      |   1 +
>>>    doc/api/status_led.rst |  35 +++++++
>>>    include/status_led.h   | 224 ++++++++++++++++++++++++++++++++++++++++-
>>>    4 files changed, 256 insertions(+), 81 deletions(-)
>>>    delete mode 100644 doc/README.LED
>>>    create mode 100644 doc/api/status_led.rst
>>>
>>> diff --git a/doc/README.LED b/doc/README.LED
>>> deleted file mode 100644
>>> index c21c9d53ec3..00000000000
>>> --- a/doc/README.LED
>>> +++ /dev/null
>>> @@ -1,77 +0,0 @@
>>> -Status LED
>>> -========================================
>>> -
>>> -This README describes the status LED API.
>>> -
>>> -The API is defined by the include file include/status_led.h
>>> -
>>> -The first step is to enable CONFIG_LED_STATUS in menuconfig:
>>> -> Device Drivers > LED Support.
>>> -
>>> -If the LED support is only for specific board, enable
>>> -CONFIG_LED_STATUS_BOARD_SPECIFIC in the menuconfig.
>>> -
>>> -Status LEDS 0 to 5 are enabled by the following configurations at menuconfig:
>>> -CONFIG_STATUS_LED0, CONFIG_STATUS_LED1, ... CONFIG_STATUS_LED5
>>> -
>>> -The following should be configured for each of the enabled LEDs:
>>> -CONFIG_STATUS_LED_BIT<n>
>>> -CONFIG_STATUS_LED_STATE<n>
>>> -CONFIG_STATUS_LED_FREQ<n>
>>> -Where <n> is an integer 1 through 5 (empty for 0).
>>> -
>>> -CONFIG_STATUS_LED_BIT is passed into the __led_* functions to identify which LED
>>> -is being acted on. As such, the value choose must be unique with with respect to
>>> -the other CONFIG_STATUS_LED_BIT's. Mapping the value to a physical LED is the
>>> -reponsiblity of the __led_* function.
>>> -
>>> -CONFIG_STATUS_LED_STATE is the initial state of the LED. It should be set to one
>>> -of these values: CONFIG_LED_STATUS_OFF or CONFIG_LED_STATUS_ON.
>>> -
>>> -CONFIG_STATUS_LED_FREQ determines the LED blink frequency.
>>> -Values range from 2 to 10.
>>> -
>>> -Some other LED macros
>>> ----------------------
>>> -
>>> -CONFIG_STATUS_LED_BOOT is the LED to light when the board is booting.
>>> -This must be a valid LED number (0-5).
>>> -
>>> -CONFIG_STATUS_LED_RED is the red LED. It is used to signal errors. This must be
>>> -a valid LED number (0-5). Other similar color LED's macros are
>>> -CONFIG_STATUS_LED_GREEN, CONFIG_STATUS_LED_YELLOW and CONFIG_STATUS_LED_BLUE.
>>> -
>>> -General LED functions
>>> ----------------------
>>> -The following functions should be defined:
>>> -
>>> -__led_init is called once to initialize the LED to CONFIG_STATUS_LED_STATE.
>>> -One time start up code should be placed here.
>>> -
>>> -__led_set is called to change the state of the LED.
>>> -
>>> -__led_toggle is called to toggle the current state of the LED.
>>> -
>>> -Colour LED
>>> -========================================
>>> -
>>> -Colour LED's are at present only used by ARM.
>>> -
>>> -The functions names explain their purpose.
>>> -
>>> -coloured_LED_init
>>> -red_LED_on
>>> -red_LED_off
>>> -green_LED_on
>>> -green_LED_off
>>> -yellow_LED_on
>>> -yellow_LED_off
>>> -blue_LED_on
>>> -blue_LED_off
>>> -
>>> -These are weakly defined in arch/arm/lib/board.c to noops. Where applicable, define
>>> -these functions in the board specific source.
>>> -
>>> -TBD : Describe older board dependent macros similar to what is done for
>>> -
>>> -TBD : Describe general support via asm/status_led.h
>>> diff --git a/doc/api/index.rst b/doc/api/index.rst
>>> index 51b2013af36..d40e90801d1 100644
>>> --- a/doc/api/index.rst
>>> +++ b/doc/api/index.rst
>>> @@ -22,6 +22,7 @@ U-Boot API documentation
>>>       rng
>>>       sandbox
>>>       serial
>>> +   status_led
>>>       sysreset
>>>       timer
>>>       unicode
>>> diff --git a/doc/api/status_led.rst b/doc/api/status_led.rst
>>> new file mode 100644
>>> index 00000000000..44bbea47157
>>> --- /dev/null
>>> +++ b/doc/api/status_led.rst
>>> @@ -0,0 +1,35 @@
>>> +.. SPDX-License-Identifier: GPL-2.0+
>>> +
>>> +Status LED
>>> +==========
>>> +
>>> +.. kernel-doc:: include/status_led.h
>>> +   :doc: Overview
>>> +
>>> +CONFIG_STATUS_LED Description
>>> +-----------------------------
>>> +
>>> +.. kernel-doc:: include/status_led.h
>>> +   :doc: CONFIG Description
>>> +
>>> +Special Status LED Configs
>>> +--------------------------
>>> +.. kernel-doc:: include/status_led.h
>>> +   :doc: LED Status Config
>>> +
>>> +Colour Status LED Configs
>>> +-------------------------
>>> +.. kernel-doc:: include/status_led.h
>>> +   :doc: LED Colour Config
>>> +
>>> +Required API
>>> +------------
>>> +
>>> +.. kernel-doc:: include/status_led.h
>>> +   :doc: Required API
>>> +
>>> +Status LED API
>>> +--------------
>>> +
>>> +.. kernel-doc:: include/status_led.h
>>> +   :internal:
>>> diff --git a/include/status_led.h b/include/status_led.h
>>> index 7de40551621..6893d1d68e0 100644
>>> --- a/include/status_led.h
>>> +++ b/include/status_led.h
>>> @@ -4,18 +4,102 @@
>>>     * Wolfgang Denk, DENX Software Engineering, wd@denx.de.
>>>     */
>>>
>>> -/*
>>> - * The purpose of this code is to signal the operational status of a
>>> +/**
>>> + * DOC: Overview
>>> + *
>>> + * Status LED is a Low-Level way to handle LEDs to signal state of the
>>
>> Status LEDs can be used to signal the operational status of a
>>
>>> + * bootloader, for example boot progress, file transfer fail, activity
>>> + * of some sort like tftp transfer, mtd write/erase.
>>
>> for example boot progress, file transfer failure, or ongoing activity
>> like tftp transfer or mtd write/erase.
>>
>>> + *
>>> + * The original usage these API were to signal the operational status of a
>>
>> One usage of this API is signalling the operational status ...
>>
>>>     * target which usually boots over the network; while running in
>>>     * PCBoot, a status LED is blinking. As soon as a valid BOOTP reply
>>
>> What does "PCBoot" refer to? Do you mean U-Boot?
>>
>>>     * message has been received, the LED is turned off. The Linux
>>>     * kernel, once it is running, will start blinking the LED again,
>>>     * with another frequency.
>>> + *
>>> + * Status LED require support Low Level and the board to implement
>>
>>
>>
>>> + * the specific functions to correctly works.
>>>     */
>>>
>>>    #ifndef _STATUS_LED_H_
>>>    #define	_STATUS_LED_H_
>>>
>>> +/**
>>> + * DOC: CONFIG Description
>>
>> DOC: Configuration
>>
>>> + *
>>> + * Enable `CONFIG_LED_STATUS` to support the Status LED under
>>> + * > Device Drivers > LED Support.
>>> + *
>>> + * The Status LED can be defined in various ways, but most of the time,
>>> + * specific functions will need to be defined in the board file.
>>> + * If this is the case, enable `CONFIG_LED_STATUS_BOARD_SPECIFIC`.
>>
>> CONFIG_LED_STATUS_BOARD_SPECIFIC is not used in any defconfig.
>>
>> GPIO is the typical case and board-specific the exception.
>>
>>> + *
>>> + * If the LEDs are GPIO-driven, you can use the GPIO wrapper driver
>>> + * instead of defining specific board functions.
>>> + * If this is the case, enable `CONFIG_LED_STATUS_GPIO`.
>>> + * (Note that `CONFIG_LED_STATUS_BOARD_SPECIFIC` is also required.)
>>> + *
>>> + * The Status LED allows defining up to six different LEDs, from 0 to 5,
>>> + * with the following configurations:
>>> + * `CONFIG_STATUS_LED`, `CONFIG_STATUS_LED1`, ..., `CONFIG_STATUS_LED5`.
>>> + *
>>> + * For each LED, the following options are required:
>>> + *  * `CONFIG_STATUS_LED_BIT<n>`
>>> + *  * `CONFIG_STATUS_LED_STATE<n>`
>>> + *  * `CONFIG_STATUS_LED_FREQ<n>`
>>> + *
>>> + * Where `<n>` is an integer from 1 through 5. (Note that LED 0 doesn't have the
>>> + * integer suffix.)
>>> + *
>>> + * `CONFIG_STATUS_LED_BIT` is passed into the `__led_*` functions to identify
>>> + * which LED is being acted on. The value is opaque, meaning it depends on how
>>> + * the low-level API handles this value. It can be an ID to reference the LED
>>> + * internally, or in the case of the GPIO wrapper, it's the GPIO number of the LED.
>>> + * Mapping the value to a physical LED is the responsibility of the `__led_*` function.
>>> + *
>>> + * `CONFIG_STATUS_LED_STATE` sets the initial state of the LED. It should be set
>>> + * to one of these values: `CONFIG_LED_STATUS_OFF` or `CONFIG_LED_STATUS_ON`.
>>> + *
>>> + * `CONFIG_STATUS_LED_FREQ` determines the LED blink frequency.
>>> + * Values range from 2 to 10.
>>> + */
>>> +/**
>>> + * DOC: LED Status Config
>>> + *
>>> + * The Status LED uses two special configurations for common operations:
>>> + *
>>> + *   * CONFIG_STATUS_LED_BOOT is the LED that lights up when the board is booting.
>>> + *   * CONFIG_STATUS_LED_ACTIVITY is the LED that lights and blinks during activity
>>> + *     (e.g., file transfer, flash write).
>>> + *
>>> + * The values set in these configurations refer to the LED ID previously set.
>>> + *
>>> + * - ``CONFIG_STATUS_LED_RED=0`` will refer to the option ``CONFIG_STATUS_LED_BIT``.
>>> + * - ``CONFIG_STATUS_LED_RED=1`` will refer to the option ``CONFIG_STATUS_LED_BIT1``.
>>> + * - ``CONFIG_STATUS_LED_RED=2`` will refer to the option ``CONFIG_STATUS_LED_BIT2``.
>>> + * - ...
>>> + */
>>> +/**
>>> + * DOC: LED Colour Config
>>> + *
>>> + * The Status LED exposes specific configurations for LEDs of different colors.
>>> + *
>>> + * The values set in these configurations refer to the LED ID previously set.
>>> + *
>>> + * - ``CONFIG_STATUS_LED_RED=0`` will refer to the option ``CONFIG_STATUS_LED_BIT``.
>>> + * - ``CONFIG_STATUS_LED_RED=1`` will refer to the option ``CONFIG_STATUS_LED_BIT1``.
>>> + * - ``CONFIG_STATUS_LED_RED=2`` will refer to the option ``CONFIG_STATUS_LED_BIT2``.
>>> + * - ...
>>> + *
>>> + * Supported colors are:
>>> + *   * red
>>> + *   * green
>>> + *   * yellow
>>> + *   * blue
>>> + *   * white
>>> + */
>>> +
>>>    #ifdef CONFIG_LED_STATUS
>>>
>>>    #define LED_STATUS_PERIOD	(CONFIG_SYS_HZ / CONFIG_LED_STATUS_FREQ)
>>> @@ -35,11 +119,49 @@
>>>    #define LED_STATUS_PERIOD5	(CONFIG_SYS_HZ / CONFIG_LED_STATUS_FREQ5)
>>>    #endif /* CONFIG_LED_STATUS5 */
>>>
>>> +/**
>>> + * void status_led_init - Init the Status LED with all the required structs.
>>> + */
>>>    void status_led_init(void);
>>> +/**
>>> + * void status_led_tick - Blink each LED that is currently set in blinking
>>> + *   mode
>>> + * @timestamp: currently unused
>>> + */
>>>    void status_led_tick(unsigned long timestamp);
>>> +/**
>>> + * void status_led_set - Set the LED ID passed as first arg to the mode set
>>> + * @led: reference to the Status LED ID
>>> + * @state: state to set the LED to
>>> + *
>>> + * Modes for state arE:
>>> + *   * CONFIG_LED_STATUS_OFF (LED off)
>>> + *   * CONFIG_LED_STATUS_ON (LED on)
>>> + *   * CONFIG_LED_STATUS_BLINK (LED initially on, expected to blink)
>>> + */
>>>    void status_led_set(int led, int state);
>>> +/**
>>> + * void status_led_toggle - toggle the LED ID
>>> + * @led: reference to the Status LED ID
>>> + *
>>> + * Toggle the LED ID passed as first arg. If it's ON became OFF, if it's
>>> + * OFF became ON.
>>> + */
>>>    void status_led_toggle(int led);
>>> +/**
>>> + * void status_led_activity_start - start a LED activity
>>> + * @led: reference to the Status LED ID
>>> + *
>>> + * Set the Status LED ON and start the Cyclic to make the LED blink at
>>> + * the configured freq.
>>> + */
>>>    void status_led_activity_start(int led);
>>> +/**
>>> + * void status_led_activity_stop - stop a LED activity
>>> + * @led: reference to the Status LED ID
>>> + *
>>> + * Stop and free the Cyclic and turn the LED OFF.
>>
>> 'Cyclic' is and adjective and cannot stand alone. Do you mean:
>>
>> Stop and free the cyclic function and turn off the LED.
>>
>>> + */
>>>    void status_led_activity_stop(int led);
>>>
>>>    /*****  MVS v1  **********************************************************/
>>
>> We don't have 'config MVS' in any Kconfig. Please remove the dead code.
>>
>>> @@ -62,9 +184,61 @@ void status_led_activity_stop(int led);
>>>    /* led_id_t is unsigned long mask */
>>>    typedef unsigned long led_id_t;
>>>
>>> +/**
>>> + * DOC: Required API
>>> + *
>>> + * The Status LED requires the following API functions to operate correctly
>>
>> The following functions are implemented by the GPIO LED driver. If using
>> CONFIG_LED_STATUS_BOARD_SPECIFIC=y, they have to be implemented by the
>> board code.
>>
>
> Well CONFIG_LED_STATUS_BOARD_SPECIFIC is mandatory for these function to
> be exporsed. GPIO is just a way to provide these function using the GPIO
> common implementation. Maybe I will make it more clear but it should be
> also written at the start of the DOC.

The Linux kernel defines LEDs in the device-tree including labels. Why
do we need CONFIG_LED_STATUS_BOARD_SPECIFIC at all?

Shouldn't we eliminate all the legacy code like cmd/legacy_led.c instead
of enhancing it?

Best regards

Heinrich

>
>>> + * and compile:
>>> + *
>>> + * - ``__led_toggle``: Low-level function to toggle the LED at the specified
>>> + *   mask.
>>> + * - ``__led_init``: Initializes the Status LED, sets up required tables, and
>>> + *   configures registers.
>>> + * - ``__led_set``: Low-level function to set the state of the LED at the
>>> + *   specified mask.
>>> + * - ``__led_blink``: Low-level function to set the LED to blink at the
>>> + *   specified frequency.
>>> + *
>>> + * The Status LED also provides optional functions to control colored LEDs.
>>
>> Do you mean
>>
>> Controlling colored LEDs requires the additional functions see :doc:
>> `Coloured LED API`.
>>
>>> + * Each supported LED color has corresponding ``_on`` and ``_off`` functions.
>>> + *
>>> + * There is also support for ``coloured_LED_init`` for LEDs that provide
>>> + * multiple colors. (Currently, this is only used by ARM.)
>>
>> We have hundreds of ARM based boards. Could you be a bit more specific,
>> please.
>>
>
> Well the only one used is the RED colour and it's in arm/lib/crt0.S in
> the ASM code.
>
>>> + *
>>> + * Each function is weakly defined and should be implemented in the
>>> + * board-specific source file. (This does not apply to the GPIO LED wrapper.)
>>> + */
>>> +/**
>>> + * void __led_toggle - toggle LED at @mask
>>> + * @mask: opaque value to reference the LED
>>> + *
>>> + * Low-Level function to toggle the LED at mask.
>>> + */
>>>    extern void __led_toggle (led_id_t mask);
>>> +/**
>>> + * void __led_init - Init the LED at @mask
>>> + * @mask: opaque value to reference the LED
>>> + * @state: starting state of the LED
>>> + *
>>> + * Init the Status LED, init required tables, setup regs...
>>> + */
>>>    extern void __led_init (led_id_t mask, int state);
>>> +/**
>>> + * void __led_set - Set the LED at @mask
>>> + * @mask: opaque value to reference the LED
>>> + * @state: state to set the LED to
>>> + *
>>> + * Init the Status LED, init required tables, setup regs...
>>> + */
>>>    extern void __led_set (led_id_t mask, int state);
>>> +/**
>>> + * void __led_blink - Set the LED at @mask to HW blink
>>> + * @mask: opaque value to reference the LED
>>> + * @freq: freq to make the LED blink at
>>> + *
>>> + * Low-Level function to set the LED at HW blink by the
>>> + * passed freq.
>>> + */
>>>    void __led_blink(led_id_t mask, int freq);
>>>    #else
>>>    # error Status LED configuration missing
>>> @@ -77,20 +251,62 @@ void __led_blink(led_id_t mask, int freq);
>>>
>>>    #endif	/* CONFIG_LED_STATUS	*/
>>>
>>> -/*
>>> - * Coloured LEDs API
>>> +/**
>>> + * DOC: Coloured LED API
>>> + *
>>> + * Status LED expose optional functions to control coloured LED.
>>
>> The status LED API uses optional functions ...
>>
>>> + * Each LED color supported expose _on and _off function.
>>> + *
>>> + * There is also support for coloured_LED_init for LED that provide
>>> + * multiple colours. (currently only used by ARM)
>>
>> No clue why you refer to ARM. I found only the following files that
>> providing board specific implementations:
>>
>
> Another case of arm/lib/crt0.S
>
> These confusing thing are picked from the old txt just to not drop info
> and try to do a 1:1 conversion.
>
>> board/beagle/beagle/led.c
>> board/siemens/corvus/board.c
>>
>> Best regards
>>
>> Heinrich
>>
>>> + *
>>> + * Each function is weakly defined and should be defined in the board
>>> + * specific source. (doesn't apply for GPIO LED wrapper)
>>>     */
>>>    #ifndef	__ASSEMBLY__
>>> +/**
>>> + * void coloured_LED_init - Init multi colour LED
>>> + */
>>>    void coloured_LED_init(void);
>>> +/**
>>> + * void red_led_on - Turn LED Red on
>>> + */
>>>    void red_led_on(void);
>>> +/**
>>> + * void red_led_off - Turn LED Red off
>>> + */
>>>    void red_led_off(void);
>>> +/**
>>> + * void green_led_on - Turn LED Green on
>>> + */
>>>    void green_led_on(void);
>>> +/**
>>> + * void green_led_off - Turn LED Green off
>>> + */
>>>    void green_led_off(void);
>>> +/**
>>> + * void yellow_led_on - Turn LED Yellow on
>>> + */
>>>    void yellow_led_on(void);
>>> +/**
>>> + * void yellow_led_off - Turn LED Yellow off
>>> + */
>>>    void yellow_led_off(void);
>>> +/**
>>> + * void blue_led_on - Turn LED Blue on
>>> + */
>>>    void blue_led_on(void);
>>> +/**
>>> + * void blue_led_off - Turn LED Blue off
>>> + */
>>>    void blue_led_off(void);
>>> +/**
>>> + * void white_led_on - Turn LED White on
>>> + */
>>>    void white_led_on(void);
>>> +/**
>>> + * void white_led_off - Turn LED White off
>>> + */
>>>    void white_led_off(void);
>>>    #else
>>>    	.extern LED_init
>>
>

Re: [PATCH v4 0/9] misc: introduce STATUS LED activity function

[Heinrich Schuchardt]

On 6/20/24 01:03, Christian Marangi wrote:
> This series expand the STATUS LED framework with a new color
> and a big new feature. One thing that many device need is a way
> to communicate to the user that the device is actually doing
> something.
>
> This is especially useful for recovery steps where an
> user (for example) insert an USB drive, keep a button pressed
> and the device autorecover.
>
> There is currently no way to signal the user externally that
> the bootloader is processing/recoverying aside from setting
> a LED on.
>
> A solid LED on is not enough and won't actually signal any
> kind of progress.
> Solution is the good old blinking LED but uboot doesn't
> suggest (and support) interrupts and almost all the LED
> are usually GPIO LED that doesn't support HW blink.
>
> To fix this and handle the problem of device not supporting
> HW blink, expand the STATUS LED framework with new API.
>
> We introduce a new config LED_STATUS_ACTIVITY, that similar
> to the RED, GREEN and others, takes the LED ID set in
> the LED_STATUS config and is used as the global LED for activity
> operations.
>
> We add status_led_activity_start/stop() used to turn the
> activity LED on and register a cyclic to make it blink.
> LED will continue to blink until _stop() is called.
>
> Function that will start some kind of action will first call
> _start() and then at the end will call stop().
> If (on the broken implementation) a _start() is called before
> calling a _stop(), the Cyclic is first unregister and is
> registered again.
>
> Support for this is initially added to the tftp, mtd and ubi
> command.
>
> This also contains a big fixup for the gpio_led driver that
> currently deviates from the Documentation and make the
> coloured status led feature unusable.
>
> (world tested with the azure pipeline)

Hello Christian,

What is the relationship between CONFIG_LED and CONFIG_LED_STATUS?
Can they be used at the same time or should they be exclusive?
Should CONFIG_LED_STATUS depend on CONFIG_LED?

This should be clarified both in Kconfig and in the documentation.

At least cmd/led.c and cmd/legacy_led.c are conflicting as both provide
a command 'led'.

Can we unify the too?

Best regards

Heinrich

>
> Changes v4:
> - Rebase on top of next
> - Rework for cyclic changes on next
> - Fix compilation error
> - Fix compilation warning due to missing header
> Changes v3:
> - Add log on Status LED error conditions
> Changes v2:
> - Add Documentation conversion
> - Rework to Cyclic and simplify implementation
>
> Christian Marangi (9):
>    misc: gpio_led: fix broken coloured LED status functions
>    led: status_led: add support for white LED colour
>    led: status_led: add function to toggle a status LED
>    led: status_led: add warning when an invalid Status LED ID is used
>    led: status_led: add new activity LED config and functions
>    tftp: implement support for LED status activity
>    mtd: implement support for LED status activity
>    ubi: implement support for LED status activity
>    doc: convert README.LED to .rst documentation
>
>   cmd/legacy_led.c          |   6 +
>   cmd/mtd.c                 |  19 ++++
>   cmd/ubi.c                 |  15 ++-
>   common/board_f.c          |   2 +
>   doc/README.LED            |  77 -------------
>   doc/api/index.rst         |   1 +
>   doc/api/status_led.rst    |  35 ++++++
>   drivers/led/Kconfig       |  30 +++++
>   drivers/misc/gpio_led.c   |  41 +++++--
>   drivers/misc/status_led.c |  75 ++++++++++++-
>   include/status_led.h      | 231 +++++++++++++++++++++++++++++++++++++-
>   net/tftp.c                |   7 ++
>   12 files changed, 440 insertions(+), 99 deletions(-)
>   delete mode 100644 doc/README.LED
>   create mode 100644 doc/api/status_led.rst
>

Re: [PATCH v4 0/9] misc: introduce STATUS LED activity function

[Christian Marangi]

On Thu, Jun 20, 2024 at 08:34:03AM +0200, Heinrich Schuchardt wrote:
> On 6/20/24 01:03, Christian Marangi wrote:
> > This series expand the STATUS LED framework with a new color
> > and a big new feature. One thing that many device need is a way
> > to communicate to the user that the device is actually doing
> > something.
> > 
> > This is especially useful for recovery steps where an
> > user (for example) insert an USB drive, keep a button pressed
> > and the device autorecover.
> > 
> > There is currently no way to signal the user externally that
> > the bootloader is processing/recoverying aside from setting
> > a LED on.
> > 
> > A solid LED on is not enough and won't actually signal any
> > kind of progress.
> > Solution is the good old blinking LED but uboot doesn't
> > suggest (and support) interrupts and almost all the LED
> > are usually GPIO LED that doesn't support HW blink.
> > 
> > To fix this and handle the problem of device not supporting
> > HW blink, expand the STATUS LED framework with new API.
> > 
> > We introduce a new config LED_STATUS_ACTIVITY, that similar
> > to the RED, GREEN and others, takes the LED ID set in
> > the LED_STATUS config and is used as the global LED for activity
> > operations.
> > 
> > We add status_led_activity_start/stop() used to turn the
> > activity LED on and register a cyclic to make it blink.
> > LED will continue to blink until _stop() is called.
> > 
> > Function that will start some kind of action will first call
> > _start() and then at the end will call stop().
> > If (on the broken implementation) a _start() is called before
> > calling a _stop(), the Cyclic is first unregister and is
> > registered again.
> > 
> > Support for this is initially added to the tftp, mtd and ubi
> > command.
> > 
> > This also contains a big fixup for the gpio_led driver that
> > currently deviates from the Documentation and make the
> > coloured status led feature unusable.
> > 
> > (world tested with the azure pipeline)
> 
> Hello Christian,
> 
> What is the relationship between CONFIG_LED and CONFIG_LED_STATUS?
> Can they be used at the same time or should they be exclusive?
> Should CONFIG_LED_STATUS depend on CONFIG_LED?

Well this is something that should have been fixed long time ago.
Given the 2 cmd and some function clash with each other they are totally
incompatible with each other and one should exclude the other.

> 
> This should be clarified both in Kconfig and in the documentation.
> 
> At least cmd/led.c and cmd/legacy_led.c are conflicting as both provide
> a command 'led'.
> 
> Can we unify the too?

Unify them is complicated (I tried) as the Status LED goes very deep and
can be supported by the board with specific implementation. LED driver
is all based on DT and most of the time it's just GPIO. (aka require OF
support)

> 
> Best regards
> 
> Heinrich
> 
> > 
> > Changes v4:
> > - Rebase on top of next
> > - Rework for cyclic changes on next
> > - Fix compilation error
> > - Fix compilation warning due to missing header
> > Changes v3:
> > - Add log on Status LED error conditions
> > Changes v2:
> > - Add Documentation conversion
> > - Rework to Cyclic and simplify implementation
> > 
> > Christian Marangi (9):
> >    misc: gpio_led: fix broken coloured LED status functions
> >    led: status_led: add support for white LED colour
> >    led: status_led: add function to toggle a status LED
> >    led: status_led: add warning when an invalid Status LED ID is used
> >    led: status_led: add new activity LED config and functions
> >    tftp: implement support for LED status activity
> >    mtd: implement support for LED status activity
> >    ubi: implement support for LED status activity
> >    doc: convert README.LED to .rst documentation
> > 
> >   cmd/legacy_led.c          |   6 +
> >   cmd/mtd.c                 |  19 ++++
> >   cmd/ubi.c                 |  15 ++-
> >   common/board_f.c          |   2 +
> >   doc/README.LED            |  77 -------------
> >   doc/api/index.rst         |   1 +
> >   doc/api/status_led.rst    |  35 ++++++
> >   drivers/led/Kconfig       |  30 +++++
> >   drivers/misc/gpio_led.c   |  41 +++++--
> >   drivers/misc/status_led.c |  75 ++++++++++++-
> >   include/status_led.h      | 231 +++++++++++++++++++++++++++++++++++++-
> >   net/tftp.c                |   7 ++
> >   12 files changed, 440 insertions(+), 99 deletions(-)
> >   delete mode 100644 doc/README.LED
> >   create mode 100644 doc/api/status_led.rst
> > 
> 

-- 
	Ansuel

Re: [PATCH v4 0/9] misc: introduce STATUS LED activity function

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 3113 bytes --]

On Thu, Jun 20, 2024 at 06:29:23AM +0200, Christian Marangi wrote:
> On Thu, Jun 20, 2024 at 08:34:03AM +0200, Heinrich Schuchardt wrote:
> > On 6/20/24 01:03, Christian Marangi wrote:
> > > This series expand the STATUS LED framework with a new color
> > > and a big new feature. One thing that many device need is a way
> > > to communicate to the user that the device is actually doing
> > > something.
> > > 
> > > This is especially useful for recovery steps where an
> > > user (for example) insert an USB drive, keep a button pressed
> > > and the device autorecover.
> > > 
> > > There is currently no way to signal the user externally that
> > > the bootloader is processing/recoverying aside from setting
> > > a LED on.
> > > 
> > > A solid LED on is not enough and won't actually signal any
> > > kind of progress.
> > > Solution is the good old blinking LED but uboot doesn't
> > > suggest (and support) interrupts and almost all the LED
> > > are usually GPIO LED that doesn't support HW blink.
> > > 
> > > To fix this and handle the problem of device not supporting
> > > HW blink, expand the STATUS LED framework with new API.
> > > 
> > > We introduce a new config LED_STATUS_ACTIVITY, that similar
> > > to the RED, GREEN and others, takes the LED ID set in
> > > the LED_STATUS config and is used as the global LED for activity
> > > operations.
> > > 
> > > We add status_led_activity_start/stop() used to turn the
> > > activity LED on and register a cyclic to make it blink.
> > > LED will continue to blink until _stop() is called.
> > > 
> > > Function that will start some kind of action will first call
> > > _start() and then at the end will call stop().
> > > If (on the broken implementation) a _start() is called before
> > > calling a _stop(), the Cyclic is first unregister and is
> > > registered again.
> > > 
> > > Support for this is initially added to the tftp, mtd and ubi
> > > command.
> > > 
> > > This also contains a big fixup for the gpio_led driver that
> > > currently deviates from the Documentation and make the
> > > coloured status led feature unusable.
> > > 
> > > (world tested with the azure pipeline)
> > 
> > Hello Christian,
> > 
> > What is the relationship between CONFIG_LED and CONFIG_LED_STATUS?
> > Can they be used at the same time or should they be exclusive?
> > Should CONFIG_LED_STATUS depend on CONFIG_LED?
> 
> Well this is something that should have been fixed long time ago.
> Given the 2 cmd and some function clash with each other they are totally
> incompatible with each other and one should exclude the other.

Yes, sorry, I missed this earlier on, my fault. The CONFIG_LED_STATUS
code is legacy and indeed shouldn't be enabled on anything new (and
ideally, someone would care enough to transition the few platforms using
the old framework over). So this functionality should be added to the
drivers/led/ framework and then yes, your platforms defining the LED via
device tree, with the same compatibles the Linux needs anyhow to be able
to control it too.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]