[PATCH v4 00/11] led: introduce LED boot and activity function

[PATCH v4 00/11] led: introduce LED boot and activity function

[Christian Marangi]

This series is a reworked version of the previous seried: 
misc: introduce STATUS LED activity function

This series port and expand the legacy concept of LED boot from
the legacy Status LED API to new LED API.

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.

Additional Kconfg are also introduced to set the LED boot and
activity. Those are referenced by label.

A documentation for old and these new LED API is created.

(world tested with the azure pipeline)

Changes v4:
- Drop led_set_state/period_by_label
- Switch to /options/u-boot
- Rework to cache label and dev in led uclass
- Add multiple patch for additional helper
- Rework patches to limit ifdef in some place
Changes v3:
- Switch to /config property
Changes v2:
- Drop GPIO SW implementation
- Add fix for new LED SW BLINK

Christian Marangi (11):
  led: toggle LED on initial SW blink
  dm: core: implement ofnode_options helpers
  led: implement LED boot API
  common: board_r: rework BOOT LED handling
  led: implement LED activity API
  tftp: implement support for LED activity
  mtd: implement support for LED activity
  ubi: implement support for LED activity
  doc: introduce led.rst documentation
  test: dm: Add tests for LED boot and activity
  test: dm: Expand ofnode options test with new helper

 arch/sandbox/dts/test.dts  |   5 +
 cmd/mtd.c                  |  11 +++
 cmd/ubi.c                  |  13 ++-
 common/board_r.c           |  28 ++++--
 doc/api/index.rst          |   1 +
 doc/api/led.rst            |  10 ++
 drivers/core/ofnode.c      |  33 +++++++
 drivers/led/Kconfig        |  15 +++
 drivers/led/led-uclass.c   | 188 +++++++++++++++++++++++++++++++++++++
 drivers/led/led_sw_blink.c |  10 +-
 include/dm/ofnode.h        |  41 ++++++++
 include/led.h              | 149 ++++++++++++++++++++++++++++-
 include/status_led.h       |  13 +++
 net/net.c                  |   4 +
 net/tftp.c                 |   5 +
 test/dm/led.c              |  72 ++++++++++++++
 test/dm/ofnode.c           |   9 ++
 17 files changed, 594 insertions(+), 13 deletions(-)
 create mode 100644 doc/api/led.rst

-- 
2.45.2

[PATCH v4 01/11] led: toggle LED on initial SW blink

[Christian Marangi]

We currently init the LED OFF when SW blink is triggered when
on_state_change() is called. This can be problematic for very short
period as the ON/OFF blink might never trigger.

Toggle the LED (ON if OFF, OFF if ON) on initial SW blink to handle this
corner case and better display a LED blink from the user.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
 drivers/led/led_sw_blink.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/led/led_sw_blink.c b/drivers/led/led_sw_blink.c
index 9e36edbee47..9d9820720c6 100644
--- a/drivers/led/led_sw_blink.c
+++ b/drivers/led/led_sw_blink.c
@@ -103,8 +103,16 @@ bool led_sw_on_state_change(struct udevice *dev, enum led_state_t state)
 		return false;
 
 	if (state == LEDST_BLINK) {
+		struct led_ops *ops = led_get_ops(dev);
+		enum led_state_t curr_state = led_get_state(dev);
+
+		curr_state = ops->get_state(dev);
+		/* toggle led initially */
+		ops->set_state(dev, curr_state == LEDST_ON ? LEDST_OFF :
+			       LEDST_ON);
 		/* start blinking on next led_sw_blink() call */
-		sw_blink->state = LED_SW_BLINK_ST_OFF;
+		sw_blink->state = curr_state == LEDST_ON ? LED_SW_BLINK_ST_OFF :
+				  LED_SW_BLINK_ST_ON;
 		return true;
 	}
 
-- 
2.45.2

Re: [PATCH v4 01/11] led: toggle LED on initial SW blink

[Michael Nazzareno Trimarchi]

Hi

On Sat, Sep 21, 2024 at 12:51 AM Christian Marangi <ansuelsmth@gmail.com> wrote:
>
> We currently init the LED OFF when SW blink is triggered when
> on_state_change() is called. This can be problematic for very short
> period as the ON/OFF blink might never trigger.
>
> Toggle the LED (ON if OFF, OFF if ON) on initial SW blink to handle this
> corner case and better display a LED blink from the user.
>
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> Reviewed-by: Simon Glass <sjg@chromium.org>
> ---
>  drivers/led/led_sw_blink.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/led/led_sw_blink.c b/drivers/led/led_sw_blink.c
> index 9e36edbee47..9d9820720c6 100644
> --- a/drivers/led/led_sw_blink.c
> +++ b/drivers/led/led_sw_blink.c
> @@ -103,8 +103,16 @@ bool led_sw_on_state_change(struct udevice *dev, enum led_state_t state)
>                 return false;
>
>         if (state == LEDST_BLINK) {
> +               struct led_ops *ops = led_get_ops(dev);
> +               enum led_state_t curr_state = led_get_state(dev);
> +
> +               curr_state = ops->get_state(dev);


The led_get_state return  curr_state. You need to use a) led_set/get
state or them from ops

if (state == LEDST_BLINK) {
        const struct led_ops *ops = led_get_ops(dev);
        enum led_state_t curr_state = ops->get_state(dev);
        enun led_state_t next_state;

        switch (curr_state) {
        case LEDST_ON:
                sw_blink->state = LED_SW_BLINK_ST_OFF;
                next_state = LEDST_OFF;
               break;
        case LEDST_OFF:
                sw_blink->state = LED_SW_BLINK_ST_ON;
                next_state = LEDST_ON;
                break;
        }

        ops->set_state(dev, next_state);
        return true;
}

Michael

> +               /* toggle led initially */
> +               ops->set_state(dev, curr_state == LEDST_ON ? LEDST_OFF :
> +                              LEDST_ON);
>                 /* start blinking on next led_sw_blink() call */
> -               sw_blink->state = LED_SW_BLINK_ST_OFF;
> +               sw_blink->state = curr_state == LEDST_ON ? LED_SW_BLINK_ST_OFF :
> +                                 LED_SW_BLINK_ST_ON;


>                 return true;
>         }
>
> --
> 2.45.2
>


-- 
Michael Nazzareno Trimarchi
Co-Founder & Chief Executive Officer
M. +39 347 913 2170
michael@amarulasolutions.com
__________________________________

Amarula Solutions BV
Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
T. +31 (0)85 111 9172
info@amarulasolutions.com
www.amarulasolutions.com

Re: [PATCH v4 01/11] led: toggle LED on initial SW blink

[Christian Marangi]

On Sat, Sep 21, 2024 at 01:14:36PM +0200, Michael Nazzareno Trimarchi wrote:
> Hi
> 
> On Sat, Sep 21, 2024 at 12:51 AM Christian Marangi <ansuelsmth@gmail.com> wrote:
> >
> > We currently init the LED OFF when SW blink is triggered when
> > on_state_change() is called. This can be problematic for very short
> > period as the ON/OFF blink might never trigger.
> >
> > Toggle the LED (ON if OFF, OFF if ON) on initial SW blink to handle this
> > corner case and better display a LED blink from the user.
> >
> > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > Reviewed-by: Simon Glass <sjg@chromium.org>
> > ---
> >  drivers/led/led_sw_blink.c | 10 +++++++++-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/led/led_sw_blink.c b/drivers/led/led_sw_blink.c
> > index 9e36edbee47..9d9820720c6 100644
> > --- a/drivers/led/led_sw_blink.c
> > +++ b/drivers/led/led_sw_blink.c
> > @@ -103,8 +103,16 @@ bool led_sw_on_state_change(struct udevice *dev, enum led_state_t state)
> >                 return false;
> >
> >         if (state == LEDST_BLINK) {
> > +               struct led_ops *ops = led_get_ops(dev);
> > +               enum led_state_t curr_state = led_get_state(dev);
> > +
> > +               curr_state = ops->get_state(dev);
> 
> 
> The led_get_state return  curr_state. You need to use a) led_set/get
> state or them from ops
> 
> if (state == LEDST_BLINK) {
>         const struct led_ops *ops = led_get_ops(dev);
>         enum led_state_t curr_state = ops->get_state(dev);
>         enun led_state_t next_state;
> 
>         switch (curr_state) {
>         case LEDST_ON:
>                 sw_blink->state = LED_SW_BLINK_ST_OFF;
>                 next_state = LEDST_OFF;
>                break;
>         case LEDST_OFF:
>                 sw_blink->state = LED_SW_BLINK_ST_ON;
>                 next_state = LEDST_ON;
>                 break;
>         }
> 
>         ops->set_state(dev, next_state);
>         return true;
> }
>

Hi,

I'm not following how this is different than the proposed code...
Is this a suggestion to make it more readable, I'm a bit confused.

> 
> > +               /* toggle led initially */
> > +               ops->set_state(dev, curr_state == LEDST_ON ? LEDST_OFF :
> > +                              LEDST_ON);
> >                 /* start blinking on next led_sw_blink() call */
> > -               sw_blink->state = LED_SW_BLINK_ST_OFF;
> > +               sw_blink->state = curr_state == LEDST_ON ? LED_SW_BLINK_ST_OFF :
> > +                                 LED_SW_BLINK_ST_ON;
> 
> 
> >                 return true;
> >         }
> >
> > --
> > 2.45.2
> >
> 
> 
> -- 
> Michael Nazzareno Trimarchi
> Co-Founder & Chief Executive Officer
> M. +39 347 913 2170
> michael@amarulasolutions.com
> __________________________________
> 
> Amarula Solutions BV
> Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
> T. +31 (0)85 111 9172
> info@amarulasolutions.com
> www.amarulasolutions.com

-- 
	Ansuel

[PATCH v4 02/11] dm: core: implement ofnode_options helpers

[Christian Marangi]

Implement ofnode_options helpers to read options in /options/u-boot to
adapt to the new way to declare options as described in [0].

[0] dtschema/schemas/options/u-boot.yaml

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 drivers/core/ofnode.c | 33 +++++++++++++++++++++++++++++++++
 include/dm/ofnode.h   | 41 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 74 insertions(+)

diff --git a/drivers/core/ofnode.c b/drivers/core/ofnode.c
index 4d563b47a5a..4404c4f4bab 100644
--- a/drivers/core/ofnode.c
+++ b/drivers/core/ofnode.c
@@ -1734,6 +1734,39 @@ const char *ofnode_conf_read_str(const char *prop_name)
 	return ofnode_read_string(node, prop_name);
 }
 
+bool ofnode_options_read_bool(const char *prop_name)
+{
+	ofnode uboot;
+
+	uboot = ofnode_path("/options/u-boot");
+	if (!ofnode_valid(uboot))
+		return false;
+
+	return ofnode_read_bool(uboot, prop_name);
+}
+
+int ofnode_options_read_int(const char *prop_name, int default_val)
+{
+	ofnode uboot;
+
+	uboot = ofnode_path("/options/u-boot");
+	if (!ofnode_valid(uboot))
+		return default_val;
+
+	return ofnode_read_u32_default(uboot, prop_name, default_val);
+}
+
+const char *ofnode_options_read_str(const char *prop_name)
+{
+	ofnode uboot;
+
+	uboot = ofnode_path("/options/u-boot");
+	if (!ofnode_valid(uboot))
+		return NULL;
+
+	return ofnode_read_string(uboot, prop_name);
+}
+
 int ofnode_read_bootscript_address(u64 *bootscr_address, u64 *bootscr_offset)
 {
 	int ret;
diff --git a/include/dm/ofnode.h b/include/dm/ofnode.h
index 5795115c490..0787758926f 100644
--- a/include/dm/ofnode.h
+++ b/include/dm/ofnode.h
@@ -1587,6 +1587,47 @@ int ofnode_conf_read_int(const char *prop_name, int default_val);
  */
 const char *ofnode_conf_read_str(const char *prop_name);
 
+/**
+ * ofnode_options_read_bool() - Read a boolean value from the U-Boot options
+ *
+ * This reads a property from the /options/u-boot/ node of the devicetree.
+ *
+ * This only works with the control FDT.
+ *
+ * See dtschema/schemas/options/u-boot.yaml in dt-schema project for bindings
+ *
+ * @prop_name:	property name to look up
+ * Return: true, if it exists, false if not
+ */
+bool ofnode_options_read_bool(const char *prop_name);
+
+/**
+ * ofnode_options_read_int() - Read an integer value from the U-Boot options
+ *
+ * This reads a property from the /options/u-boot/ node of the devicetree.
+ *
+ * See dtschema/schemas/options/u-boot.yaml in dt-schema project for bindings
+ *
+ * @prop_name: property name to look up
+ * @default_val: default value to return if the property is not found
+ * Return: integer value, if found, or @default_val if not
+ */
+int ofnode_options_read_int(const char *prop_name, int default_val);
+
+/**
+ * ofnode_options_read_str() - Read a string value from the U-Boot options
+ *
+ * This reads a property from the /options/u-boot/ node of the devicetree.
+ *
+ * This only works with the control FDT.
+ *
+ * See dtschema/schemas/options/u-boot.yaml in dt-schema project for bindings
+ *
+ * @prop_name: property name to look up
+ * Return: string value, if found, or NULL if not
+ */
+const char *ofnode_options_read_str(const char *prop_name);
+
 /**
  * ofnode_read_bootscript_address() - Read bootscr-address or bootscr-ram-offset
  *
-- 
2.45.2

Re: [PATCH v4 02/11] dm: core: implement ofnode_options helpers

[Simon Glass]

Hi Christian,

On Sat, 21 Sept 2024 at 00:51, Christian Marangi <ansuelsmth@gmail.com> wrote:
>
> Implement ofnode_options helpers to read options in /options/u-boot to
> adapt to the new way to declare options as described in [0].
>
> [0] dtschema/schemas/options/u-boot.yaml
>
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> ---
>  drivers/core/ofnode.c | 33 +++++++++++++++++++++++++++++++++
>  include/dm/ofnode.h   | 41 +++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 74 insertions(+)
>

Reviewed-by: Simon Glass <sjg@chromium.org>

It's fine to add the tests in the same patch, if you like

[PATCH v4 03/11] led: implement LED boot API

[Christian Marangi]

Implement LED boot API to signal correct boot of the system.

led_boot_on/off/blink() are introduced to turn ON, OFF and BLINK the
designated boot LED.

New Kconfig is introduced, CONFIG_LED_BOOT to enable the feature.
This makes use of the /options/u-boot property "boot-led" to the
define the boot LED.
It's also introduced a new /options/u-boot property "boot-led-period"
to define the default period when the LED is set to blink mode.

If "boot-led-period" is not defined, the value of 250 (ms) is
used by default.

If CONFIG_LED_BLINK or CONFIG_LED_SW_BLINK is not enabled,
led_boot_blink call will fallback to simple LED ON.

To cache the data we repurpose the now unused led_uc_priv for storage of
global LED uclass info.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 drivers/led/Kconfig      |   7 +++
 drivers/led/led-uclass.c | 100 +++++++++++++++++++++++++++++++++++++++
 include/led.h            |  56 +++++++++++++++++++++-
 3 files changed, 161 insertions(+), 2 deletions(-)

diff --git a/drivers/led/Kconfig b/drivers/led/Kconfig
index bee74b25751..6149cfa02b8 100644
--- a/drivers/led/Kconfig
+++ b/drivers/led/Kconfig
@@ -9,6 +9,13 @@ config LED
 	  can provide access to board-specific LEDs. Use of the device tree
 	  for configuration is encouraged.
 
+config LED_BOOT
+	bool "Enable LED boot support"
+	help
+	  Enable LED boot support.
+
+	  LED boot is a specific LED assigned to signal boot operation status.
+
 config LED_BCM6328
 	bool "LED Support for BCM6328"
 	depends on LED && ARCH_BMIPS
diff --git a/drivers/led/led-uclass.c b/drivers/led/led-uclass.c
index 199d68bc25a..c5b560982b0 100644
--- a/drivers/led/led-uclass.c
+++ b/drivers/led/led-uclass.c
@@ -94,17 +94,101 @@ int led_set_period(struct udevice *dev, int period_ms)
 	return -ENOSYS;
 }
 
+#ifdef CONFIG_LED_BOOT
+int led_boot_on(void)
+{
+	struct uclass *uc = uclass_find(UCLASS_LED);
+	struct led_uc_priv *priv;
+	struct udevice *dev;
+
+	if (!uc)
+		return -ENOENT;
+
+	priv = uclass_get_priv(uc);
+	if (!priv->boot_led_dev ||
+	    uclass_get_device_tail(priv->boot_led_dev, 0, &dev)) {
+		printf("Failed to get boot LED %s\n",
+		       priv->boot_led_label);
+		return -EINVAL;
+	}
+
+	return led_set_state(dev, LEDST_ON);
+}
+
+int led_boot_off(void)
+{
+	struct uclass *uc = uclass_find(UCLASS_LED);
+	struct led_uc_priv *priv;
+	struct udevice *dev;
+
+	if (!uc)
+		return -ENOENT;
+
+	priv = uclass_get_priv(uc);
+	if (!priv->boot_led_dev ||
+	    uclass_get_device_tail(priv->boot_led_dev, 0, &dev)) {
+		printf("Failed to get boot LED %s\n",
+		       priv->boot_led_label);
+		return -EINVAL;
+	}
+
+	return led_set_state(dev, LEDST_OFF);
+}
+
+#if defined(CONFIG_LED_BLINK) || defined(CONFIG_LED_SW_BLINK)
+int led_boot_blink(void)
+{
+	struct uclass *uc = uclass_find(UCLASS_LED);
+	struct led_uc_priv *priv;
+	struct udevice *dev;
+	int ret;
+
+	if (!uc)
+		return -ENOENT;
+
+	priv = uclass_get_priv(uc);
+	if (!priv->boot_led_dev ||
+	    uclass_get_device_tail(priv->boot_led_dev, 0, &dev)) {
+		printf("Failed to get boot LED %s\n",
+		       priv->boot_led_label);
+		return -EINVAL;
+	}
+
+	ret = led_set_period(dev, priv->boot_led_period);
+	if (ret) {
+		if (ret != -ENOSYS)
+			return ret;
+
+		/* fallback to ON with no set_period and no SW_BLINK */
+		return led_set_state(dev, LEDST_ON);
+	}
+
+	return led_set_state(dev, LEDST_BLINK);
+}
+#endif
+#endif
+
 static int led_post_bind(struct udevice *dev)
 {
 	struct led_uc_plat *uc_plat = dev_get_uclass_plat(dev);
 	const char *default_state;
 
+#ifdef CONFIG_LED_BOOT
+	struct led_uc_priv *priv = uclass_get_priv(dev->uclass);
+#endif
+
 	if (!uc_plat->label)
 		uc_plat->label = dev_read_string(dev, "label");
 
 	if (!uc_plat->label && !dev_read_string(dev, "compatible"))
 		uc_plat->label = ofnode_get_name(dev_ofnode(dev));
 
+#ifdef CONFIG_LED_BOOT
+	/* check if we are binding boot LED and assign it */
+	if (!strcmp(priv->boot_led_label, uc_plat->label))
+		priv->boot_led_dev = dev;
+#endif
+
 	uc_plat->default_state = LEDST_COUNT;
 
 	default_state = dev_read_string(dev, "default-state");
@@ -158,10 +242,26 @@ static int led_post_probe(struct udevice *dev)
 	return ret;
 }
 
+#ifdef CONFIG_LED_BOOT
+static int led_init(struct uclass *uc)
+{
+	struct led_uc_priv *priv = uclass_get_priv(uc);
+
+	priv->boot_led_label = ofnode_options_read_str("boot-led");
+	priv->boot_led_period = ofnode_options_read_int("boot-led-period", 250);
+
+	return 0;
+}
+#endif
+
 UCLASS_DRIVER(led) = {
 	.id		= UCLASS_LED,
 	.name		= "led",
 	.per_device_plat_auto	= sizeof(struct led_uc_plat),
 	.post_bind	= led_post_bind,
 	.post_probe	= led_post_probe,
+#ifdef CONFIG_LED_BOOT
+	.init		= led_init,
+	.priv_auto	= sizeof(struct led_uc_priv),
+#endif
 };
diff --git a/include/led.h b/include/led.h
index 99f93c5ef86..ca495217777 100644
--- a/include/led.h
+++ b/include/led.h
@@ -9,6 +9,7 @@
 
 #include <stdbool.h>
 #include <cyclic.h>
+#include <dm/ofnode.h>
 
 struct udevice;
 
@@ -52,10 +53,16 @@ struct led_uc_plat {
 /**
  * struct led_uc_priv - Private data the uclass stores about each device
  *
- * @period_ms:	Flash period in milliseconds
+ * @boot_led_label:	Boot LED label
+ * @boot_led_dev:	Boot LED dev
+ * @boot_led_period:	Boot LED blink period
  */
 struct led_uc_priv {
-	int period_ms;
+#ifdef CONFIG_LED_BOOT
+	const char *boot_led_label;
+	struct udevice *boot_led_dev;
+	int boot_led_period;
+#endif
 };
 
 struct led_ops {
@@ -141,4 +148,49 @@ int led_sw_set_period(struct udevice *dev, int period_ms);
 bool led_sw_is_blinking(struct udevice *dev);
 bool led_sw_on_state_change(struct udevice *dev, enum led_state_t state);
 
+#ifdef CONFIG_LED_BOOT
+
+/**
+ * led_boot_on() - turn ON the designated LED for booting
+ *
+ * Return: 0 if OK, -ve on error
+ */
+int led_boot_on(void);
+
+/**
+ * led_boot_off() - turn OFF the designated LED for booting
+ *
+ * Return: 0 if OK, -ve on error
+ */
+int led_boot_off(void);
+
+#if defined(CONFIG_LED_BLINK) || defined(CONFIG_LED_SW_BLINK)
+/**
+ * led_boot_blink() - turn ON the designated LED for booting
+ *
+ * Return: 0 if OK, -ve on error
+ */
+int led_boot_blink(void);
+
+#else
+/* If LED BLINK is not supported/enabled, fallback to LED ON */
+#define led_boot_blink led_boot_on
+#endif
+#else
+static inline int led_boot_on(void)
+{
+	return -ENOSYS;
+}
+
+static inline int led_boot_off(void)
+{
+	return -ENOSYS;
+}
+
+static inline int led_boot_blink(void)
+{
+	return -ENOSYS;
+}
+#endif
+
 #endif
-- 
2.45.2

Re: [PATCH v4 03/11] led: implement LED boot API

[Simon Glass]

Hi Christian,

On Sat, 21 Sept 2024 at 00:51, Christian Marangi <ansuelsmth@gmail.com> wrote:
>
> Implement LED boot API to signal correct boot of the system.
>
> led_boot_on/off/blink() are introduced to turn ON, OFF and BLINK the
> designated boot LED.
>
> New Kconfig is introduced, CONFIG_LED_BOOT to enable the feature.
> This makes use of the /options/u-boot property "boot-led" to the
> define the boot LED.
> It's also introduced a new /options/u-boot property "boot-led-period"
> to define the default period when the LED is set to blink mode.
>
> If "boot-led-period" is not defined, the value of 250 (ms) is
> used by default.
>
> If CONFIG_LED_BLINK or CONFIG_LED_SW_BLINK is not enabled,
> led_boot_blink call will fallback to simple LED ON.
>
> To cache the data we repurpose the now unused led_uc_priv for storage of
> global LED uclass info.

Some things to tweak below

>
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> ---
>  drivers/led/Kconfig      |   7 +++
>  drivers/led/led-uclass.c | 100 +++++++++++++++++++++++++++++++++++++++
>  include/led.h            |  56 +++++++++++++++++++++-
>  3 files changed, 161 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/led/Kconfig b/drivers/led/Kconfig
> index bee74b25751..6149cfa02b8 100644
> --- a/drivers/led/Kconfig
> +++ b/drivers/led/Kconfig
> @@ -9,6 +9,13 @@ config LED
>           can provide access to board-specific LEDs. Use of the device tree
>           for configuration is encouraged.
>
> +config LED_BOOT
> +       bool "Enable LED boot support"
> +       help
> +         Enable LED boot support.
> +
> +         LED boot is a specific LED assigned to signal boot operation status.

Here you should link to the /options binding in
doc/device-tree-bindings/options, perhaps

> +
>  config LED_BCM6328
>         bool "LED Support for BCM6328"
>         depends on LED && ARCH_BMIPS
> diff --git a/drivers/led/led-uclass.c b/drivers/led/led-uclass.c
> index 199d68bc25a..c5b560982b0 100644
> --- a/drivers/led/led-uclass.c
> +++ b/drivers/led/led-uclass.c
> @@ -94,17 +94,101 @@ int led_set_period(struct udevice *dev, int period_ms)
>         return -ENOSYS;
>  }
>
> +#ifdef CONFIG_LED_BOOT
> +int led_boot_on(void)
> +{
> +       struct uclass *uc = uclass_find(UCLASS_LED);
> +       struct led_uc_priv *priv;
> +       struct udevice *dev;
> +
> +       if (!uc)
> +               return -ENOENT;
> +

The normal way is:

ret = uclass_first_device_ret(UCLASS_LED, &dev);
if (ret)
   return ret;

> +       priv = uclass_get_priv(uc);
> +       if (!priv->boot_led_dev ||
> +           uclass_get_device_tail(priv->boot_led_dev, 0, &dev)) {

That is an internal function...I suppose it should really have an
underscore and be in uclass-internal.h

But if you are looking for boot_led_label, don't you need to search
through the LEDs to find it? Or use led_get_by_label() ?

> +               printf("Failed to get boot LED %s\n",
> +                      priv->boot_led_label);
> +               return -EINVAL;
> +       }
> +
> +       return led_set_state(dev, LEDST_ON);
> +}
> +
> +int led_boot_off(void)
> +{
> +       struct uclass *uc = uclass_find(UCLASS_LED);
> +       struct led_uc_priv *priv;
> +       struct udevice *dev;
> +
> +       if (!uc)
> +               return -ENOENT;
> +

Same here.

> +       priv = uclass_get_priv(uc);
> +       if (!priv->boot_led_dev ||
> +           uclass_get_device_tail(priv->boot_led_dev, 0, &dev)) {
> +               printf("Failed to get boot LED %s\n",
> +                      priv->boot_led_label);
> +               return -EINVAL;
> +       }
> +
> +       return led_set_state(dev, LEDST_OFF);
> +}
> +
> +#if defined(CONFIG_LED_BLINK) || defined(CONFIG_LED_SW_BLINK)
> +int led_boot_blink(void)
> +{
> +       struct uclass *uc = uclass_find(UCLASS_LED);
> +       struct led_uc_priv *priv;
> +       struct udevice *dev;
> +       int ret;
> +
> +       if (!uc)
> +               return -ENOENT;
> +
> +       priv = uclass_get_priv(uc);
> +       if (!priv->boot_led_dev ||
> +           uclass_get_device_tail(priv->boot_led_dev, 0, &dev)) {
> +               printf("Failed to get boot LED %s\n",
> +                      priv->boot_led_label);
> +               return -EINVAL;
> +       }

This looks like the same code again. I think it should be in a
function so the code is not repeated.

> +
> +       ret = led_set_period(dev, priv->boot_led_period);
> +       if (ret) {
> +               if (ret != -ENOSYS)
> +                       return ret;
> +
> +               /* fallback to ON with no set_period and no SW_BLINK */
> +               return led_set_state(dev, LEDST_ON);
> +       }
> +
> +       return led_set_state(dev, LEDST_BLINK);
> +}
> +#endif
> +#endif
> +
>  static int led_post_bind(struct udevice *dev)
>  {
>         struct led_uc_plat *uc_plat = dev_get_uclass_plat(dev);
>         const char *default_state;
>
> +#ifdef CONFIG_LED_BOOT
> +       struct led_uc_priv *priv = uclass_get_priv(dev->uclass);
> +#endif
> +
>         if (!uc_plat->label)
>                 uc_plat->label = dev_read_string(dev, "label");
>
>         if (!uc_plat->label && !dev_read_string(dev, "compatible"))
>                 uc_plat->label = ofnode_get_name(dev_ofnode(dev));
>
> +#ifdef CONFIG_LED_BOOT
> +       /* check if we are binding boot LED and assign it */
> +       if (!strcmp(priv->boot_led_label, uc_plat->label))
> +               priv->boot_led_dev = dev;
> +#endif
> +
>         uc_plat->default_state = LEDST_COUNT;
>
>         default_state = dev_read_string(dev, "default-state");
> @@ -158,10 +242,26 @@ static int led_post_probe(struct udevice *dev)
>         return ret;
>  }
>
> +#ifdef CONFIG_LED_BOOT
> +static int led_init(struct uclass *uc)
> +{
> +       struct led_uc_priv *priv = uclass_get_priv(uc);
> +
> +       priv->boot_led_label = ofnode_options_read_str("boot-led");
> +       priv->boot_led_period = ofnode_options_read_int("boot-led-period", 250);
> +
> +       return 0;
> +}
> +#endif
> +
>  UCLASS_DRIVER(led) = {
>         .id             = UCLASS_LED,
>         .name           = "led",
>         .per_device_plat_auto   = sizeof(struct led_uc_plat),
>         .post_bind      = led_post_bind,
>         .post_probe     = led_post_probe,
> +#ifdef CONFIG_LED_BOOT
> +       .init           = led_init,
> +       .priv_auto      = sizeof(struct led_uc_priv),
> +#endif

I don't love all these #ifdefs but I suppose it is OK here. Ideally we
would have static inline accessors for the fields, in the header file,
e.g. like asm-generic/global_data.h

>  };
> diff --git a/include/led.h b/include/led.h
> index 99f93c5ef86..ca495217777 100644
> --- a/include/led.h
> +++ b/include/led.h
> @@ -9,6 +9,7 @@
>
>  #include <stdbool.h>
>  #include <cyclic.h>
> +#include <dm/ofnode.h>
>
>  struct udevice;
>
> @@ -52,10 +53,16 @@ struct led_uc_plat {
>  /**
>   * struct led_uc_priv - Private data the uclass stores about each device
>   *
> - * @period_ms: Flash period in milliseconds
> + * @boot_led_label:    Boot LED label
> + * @boot_led_dev:      Boot LED dev
> + * @boot_led_period:   Boot LED blink period
>   */
>  struct led_uc_priv {
> -       int period_ms;
> +#ifdef CONFIG_LED_BOOT
> +       const char *boot_led_label;
> +       struct udevice *boot_led_dev;
> +       int boot_led_period;
> +#endif
>  };
>
>  struct led_ops {
> @@ -141,4 +148,49 @@ int led_sw_set_period(struct udevice *dev, int period_ms);
>  bool led_sw_is_blinking(struct udevice *dev);
>  bool led_sw_on_state_change(struct udevice *dev, enum led_state_t state);
>
> +#ifdef CONFIG_LED_BOOT
> +
> +/**
> + * led_boot_on() - turn ON the designated LED for booting
> + *
> + * Return: 0 if OK, -ve on error
> + */
> +int led_boot_on(void);
> +
> +/**
> + * led_boot_off() - turn OFF the designated LED for booting
> + *
> + * Return: 0 if OK, -ve on error
> + */
> +int led_boot_off(void);
> +
> +#if defined(CONFIG_LED_BLINK) || defined(CONFIG_LED_SW_BLINK)
> +/**
> + * led_boot_blink() - turn ON the designated LED for booting
> + *
> + * Return: 0 if OK, -ve on error
> + */
> +int led_boot_blink(void);
> +
> +#else
> +/* If LED BLINK is not supported/enabled, fallback to LED ON */
> +#define led_boot_blink led_boot_on
> +#endif
> +#else
> +static inline int led_boot_on(void)
> +{
> +       return -ENOSYS;
> +}
> +
> +static inline int led_boot_off(void)
> +{
> +       return -ENOSYS;
> +}
> +
> +static inline int led_boot_blink(void)
> +{
> +       return -ENOSYS;
> +}
> +#endif
> +
>  #endif
> --
> 2.45.2
>

Regards,
Simon

Re: [PATCH v4 03/11] led: implement LED boot API

[Christian Marangi]

On Thu, Sep 26, 2024 at 11:33:18PM +0200, Simon Glass wrote:
> Hi Christian,
> 
> On Sat, 21 Sept 2024 at 00:51, Christian Marangi <ansuelsmth@gmail.com> wrote:
> >
> > Implement LED boot API to signal correct boot of the system.
> >
> > led_boot_on/off/blink() are introduced to turn ON, OFF and BLINK the
> > designated boot LED.
> >
> > New Kconfig is introduced, CONFIG_LED_BOOT to enable the feature.
> > This makes use of the /options/u-boot property "boot-led" to the
> > define the boot LED.
> > It's also introduced a new /options/u-boot property "boot-led-period"
> > to define the default period when the LED is set to blink mode.
> >
> > If "boot-led-period" is not defined, the value of 250 (ms) is
> > used by default.
> >
> > If CONFIG_LED_BLINK or CONFIG_LED_SW_BLINK is not enabled,
> > led_boot_blink call will fallback to simple LED ON.
> >
> > To cache the data we repurpose the now unused led_uc_priv for storage of
> > global LED uclass info.
> 
> Some things to tweak below
> 

Hi, thanks for the review. I asked some clarification, thanks for any
comments.

> >
> > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > ---
> >  drivers/led/Kconfig      |   7 +++
> >  drivers/led/led-uclass.c | 100 +++++++++++++++++++++++++++++++++++++++
> >  include/led.h            |  56 +++++++++++++++++++++-
> >  3 files changed, 161 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/led/Kconfig b/drivers/led/Kconfig
> > index bee74b25751..6149cfa02b8 100644
> > --- a/drivers/led/Kconfig
> > +++ b/drivers/led/Kconfig
> > @@ -9,6 +9,13 @@ config LED
> >           can provide access to board-specific LEDs. Use of the device tree
> >           for configuration is encouraged.
> >
> > +config LED_BOOT
> > +       bool "Enable LED boot support"
> > +       help
> > +         Enable LED boot support.
> > +
> > +         LED boot is a specific LED assigned to signal boot operation status.
> 
> Here you should link to the /options binding in
> doc/device-tree-bindings/options, perhaps
>

Ok.

> > +
> >  config LED_BCM6328
> >         bool "LED Support for BCM6328"
> >         depends on LED && ARCH_BMIPS
> > diff --git a/drivers/led/led-uclass.c b/drivers/led/led-uclass.c
> > index 199d68bc25a..c5b560982b0 100644
> > --- a/drivers/led/led-uclass.c
> > +++ b/drivers/led/led-uclass.c
> > @@ -94,17 +94,101 @@ int led_set_period(struct udevice *dev, int period_ms)
> >         return -ENOSYS;
> >  }
> >
> > +#ifdef CONFIG_LED_BOOT
> > +int led_boot_on(void)
> > +{
> > +       struct uclass *uc = uclass_find(UCLASS_LED);
> > +       struct led_uc_priv *priv;
> > +       struct udevice *dev;
> > +
> > +       if (!uc)
> > +               return -ENOENT;
> > +
> 
> The normal way is:
> 
> ret = uclass_first_device_ret(UCLASS_LED, &dev);
> if (ret)
>    return ret;
> 
> > +       priv = uclass_get_priv(uc);
> > +       if (!priv->boot_led_dev ||
> > +           uclass_get_device_tail(priv->boot_led_dev, 0, &dev)) {
> 
> That is an internal function...I suppose it should really have an
> underscore and be in uclass-internal.h
> 
> But if you are looking for boot_led_label, don't you need to search
> through the LEDs to find it? Or use led_get_by_label() ?
> 

Idea here and up is to cache the dev on bind to prevent
useless/additional loop on search in each UCLASS LED the boot LED.

uclass_get_device_tail is needed to actually trigger probe of the LED if
it hasn't been done prev.

The uclass_get_priv is followed by the rkmtd_get_cur_dev()
implementation, for the sake of getting the UCLASS alone it seems too
much to have all the additional operation to get the first device.

Also considering uclass_get_device_tail is also used by get_by_label I
assume it's ok to also use it here.

Should I ignore caching the dev and just search for the boot LED from
label everytime out of simplicity?

> > +               printf("Failed to get boot LED %s\n",
> > +                      priv->boot_led_label);
> > +               return -EINVAL;
> > +       }
> > +
> > +       return led_set_state(dev, LEDST_ON);
> > +}
> > +
> > +int led_boot_off(void)
> > +{
> > +       struct uclass *uc = uclass_find(UCLASS_LED);
> > +       struct led_uc_priv *priv;
> > +       struct udevice *dev;
> > +
> > +       if (!uc)
> > +               return -ENOENT;
> > +
> 
> Same here.
> 
> > +       priv = uclass_get_priv(uc);
> > +       if (!priv->boot_led_dev ||
> > +           uclass_get_device_tail(priv->boot_led_dev, 0, &dev)) {
> > +               printf("Failed to get boot LED %s\n",
> > +                      priv->boot_led_label);
> > +               return -EINVAL;
> > +       }
> > +
> > +       return led_set_state(dev, LEDST_OFF);
> > +}
> > +
> > +#if defined(CONFIG_LED_BLINK) || defined(CONFIG_LED_SW_BLINK)
> > +int led_boot_blink(void)
> > +{
> > +       struct uclass *uc = uclass_find(UCLASS_LED);
> > +       struct led_uc_priv *priv;
> > +       struct udevice *dev;
> > +       int ret;
> > +
> > +       if (!uc)
> > +               return -ENOENT;
> > +
> > +       priv = uclass_get_priv(uc);
> > +       if (!priv->boot_led_dev ||
> > +           uclass_get_device_tail(priv->boot_led_dev, 0, &dev)) {
> > +               printf("Failed to get boot LED %s\n",
> > +                      priv->boot_led_label);
> > +               return -EINVAL;
> > +       }
> 
> This looks like the same code again. I think it should be in a
> function so the code is not repeated.
> 
> > +
> > +       ret = led_set_period(dev, priv->boot_led_period);
> > +       if (ret) {
> > +               if (ret != -ENOSYS)
> > +                       return ret;
> > +
> > +               /* fallback to ON with no set_period and no SW_BLINK */
> > +               return led_set_state(dev, LEDST_ON);
> > +       }
> > +
> > +       return led_set_state(dev, LEDST_BLINK);
> > +}
> > +#endif
> > +#endif
> > +
> >  static int led_post_bind(struct udevice *dev)
> >  {
> >         struct led_uc_plat *uc_plat = dev_get_uclass_plat(dev);
> >         const char *default_state;
> >
> > +#ifdef CONFIG_LED_BOOT
> > +       struct led_uc_priv *priv = uclass_get_priv(dev->uclass);
> > +#endif
> > +
> >         if (!uc_plat->label)
> >                 uc_plat->label = dev_read_string(dev, "label");
> >
> >         if (!uc_plat->label && !dev_read_string(dev, "compatible"))
> >                 uc_plat->label = ofnode_get_name(dev_ofnode(dev));
> >
> > +#ifdef CONFIG_LED_BOOT
> > +       /* check if we are binding boot LED and assign it */
> > +       if (!strcmp(priv->boot_led_label, uc_plat->label))
> > +               priv->boot_led_dev = dev;
> > +#endif
> > +
> >         uc_plat->default_state = LEDST_COUNT;
> >
> >         default_state = dev_read_string(dev, "default-state");
> > @@ -158,10 +242,26 @@ static int led_post_probe(struct udevice *dev)
> >         return ret;
> >  }
> >
> > +#ifdef CONFIG_LED_BOOT
> > +static int led_init(struct uclass *uc)
> > +{
> > +       struct led_uc_priv *priv = uclass_get_priv(uc);
> > +
> > +       priv->boot_led_label = ofnode_options_read_str("boot-led");
> > +       priv->boot_led_period = ofnode_options_read_int("boot-led-period", 250);
> > +
> > +       return 0;
> > +}
> > +#endif
> > +
> >  UCLASS_DRIVER(led) = {
> >         .id             = UCLASS_LED,
> >         .name           = "led",
> >         .per_device_plat_auto   = sizeof(struct led_uc_plat),
> >         .post_bind      = led_post_bind,
> >         .post_probe     = led_post_probe,
> > +#ifdef CONFIG_LED_BOOT
> > +       .init           = led_init,
> > +       .priv_auto      = sizeof(struct led_uc_priv),
> > +#endif
> 
> I don't love all these #ifdefs but I suppose it is OK here. Ideally we
> would have static inline accessors for the fields, in the header file,
> e.g. like asm-generic/global_data.h
> 

Didn't know of the global_data but then I read the comments and it seems
we should not really pollute it with too much stuff. Also considering
most of the LED are driven with gpio the possibility of having this that
early on boot might be closer to 0.

> >  };
> > diff --git a/include/led.h b/include/led.h
> > index 99f93c5ef86..ca495217777 100644
> > --- a/include/led.h
> > +++ b/include/led.h
> > @@ -9,6 +9,7 @@
> >
> >  #include <stdbool.h>
> >  #include <cyclic.h>
> > +#include <dm/ofnode.h>
> >
> >  struct udevice;
> >
> > @@ -52,10 +53,16 @@ struct led_uc_plat {
> >  /**
> >   * struct led_uc_priv - Private data the uclass stores about each device
> >   *
> > - * @period_ms: Flash period in milliseconds
> > + * @boot_led_label:    Boot LED label
> > + * @boot_led_dev:      Boot LED dev
> > + * @boot_led_period:   Boot LED blink period
> >   */
> >  struct led_uc_priv {
> > -       int period_ms;
> > +#ifdef CONFIG_LED_BOOT
> > +       const char *boot_led_label;
> > +       struct udevice *boot_led_dev;
> > +       int boot_led_period;
> > +#endif
> >  };
> >
> >  struct led_ops {
> > @@ -141,4 +148,49 @@ int led_sw_set_period(struct udevice *dev, int period_ms);
> >  bool led_sw_is_blinking(struct udevice *dev);
> >  bool led_sw_on_state_change(struct udevice *dev, enum led_state_t state);
> >
> > +#ifdef CONFIG_LED_BOOT
> > +
> > +/**
> > + * led_boot_on() - turn ON the designated LED for booting
> > + *
> > + * Return: 0 if OK, -ve on error
> > + */
> > +int led_boot_on(void);
> > +
> > +/**
> > + * led_boot_off() - turn OFF the designated LED for booting
> > + *
> > + * Return: 0 if OK, -ve on error
> > + */
> > +int led_boot_off(void);
> > +
> > +#if defined(CONFIG_LED_BLINK) || defined(CONFIG_LED_SW_BLINK)
> > +/**
> > + * led_boot_blink() - turn ON the designated LED for booting
> > + *
> > + * Return: 0 if OK, -ve on error
> > + */
> > +int led_boot_blink(void);
> > +
> > +#else
> > +/* If LED BLINK is not supported/enabled, fallback to LED ON */
> > +#define led_boot_blink led_boot_on
> > +#endif
> > +#else
> > +static inline int led_boot_on(void)
> > +{
> > +       return -ENOSYS;
> > +}
> > +
> > +static inline int led_boot_off(void)
> > +{
> > +       return -ENOSYS;
> > +}
> > +
> > +static inline int led_boot_blink(void)
> > +{
> > +       return -ENOSYS;
> > +}
> > +#endif
> > +
> >  #endif
> > --
> > 2.45.2
> >
> 
> Regards,
> Simon

-- 
	Ansuel

[PATCH v4 04/11] common: board_r: rework BOOT LED handling

[Christian Marangi]

Rework BOOT LED handling. There is currently one legacy implementation
for BOOT LED from Status Led API.

This work on ancient implementation used by BOOTP by setting the LED
to Blink on boot and to turn it OFF when the firmware was correctly
received by network.

Now that we new LED implementation have support for LED boot, rework
this by also set the new BOOT LED to blink and also set it to ON before
entering main loop to confirm successful boot.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 common/board_r.c     | 28 ++++++++++++++++++++--------
 include/status_led.h | 13 +++++++++++++
 2 files changed, 33 insertions(+), 8 deletions(-)

diff --git a/common/board_r.c b/common/board_r.c
index d4ba245ac69..c3f8dd5d4ee 100644
--- a/common/board_r.c
+++ b/common/board_r.c
@@ -39,6 +39,7 @@
 #include <initcall.h>
 #include <kgdb.h>
 #include <irq_func.h>
+#include <led.h>
 #include <malloc.h>
 #include <mapmem.h>
 #include <miiphy.h>
@@ -459,17 +460,28 @@ static int initr_malloc_bootparams(void)
 }
 #endif
 
-#if defined(CONFIG_LED_STATUS)
 static int initr_status_led(void)
 {
-#if defined(CONFIG_LED_STATUS_BOOT)
-	status_led_set(CONFIG_LED_STATUS_BOOT, CONFIG_LED_STATUS_BLINKING);
-#else
 	status_led_init();
-#endif
+
+	return 0;
+}
+
+static int initr_boot_led_blink(void)
+{
+	status_led_boot_blink();
+
+	led_boot_blink();
+
+	return 0;
+}
+
+static int initr_boot_led_on(void)
+{
+	led_boot_on();
+
 	return 0;
 }
-#endif
 
 #ifdef CONFIG_CMD_NET
 static int initr_net(void)
@@ -713,9 +725,8 @@ static init_fnc_t init_sequence_r[] = {
 #if defined(CONFIG_MICROBLAZE) || defined(CONFIG_M68K)
 	timer_init,		/* initialize timer */
 #endif
-#if defined(CONFIG_LED_STATUS)
 	initr_status_led,
-#endif
+	initr_boot_led_blink,
 	/* PPC has a udelay(20) here dating from 2002. Why? */
 #ifdef CONFIG_BOARD_LATE_INIT
 	board_late_init,
@@ -738,6 +749,7 @@ static init_fnc_t init_sequence_r[] = {
 #if defined(CFG_PRAM)
 	initr_mem,
 #endif
+	initr_boot_led_on,
 	run_main_loop,
 };
 
diff --git a/include/status_led.h b/include/status_led.h
index 6707ab1d29d..1282022253e 100644
--- a/include/status_led.h
+++ b/include/status_led.h
@@ -39,6 +39,13 @@ void status_led_init(void);
 void status_led_tick(unsigned long timestamp);
 void status_led_set(int led, int state);
 
+static inline void status_led_boot_blink(void)
+{
+#ifdef CONFIG_LED_STATUS_BOOT_ENABLE
+	status_led_set(CONFIG_LED_STATUS_BOOT, CONFIG_LED_STATUS_BLINKING);
+#endif
+}
+
 /*****  MVS v1  **********************************************************/
 #if (defined(CONFIG_MVS) && CONFIG_MVS < 2)
 # define STATUS_LED_PAR		im_ioport.iop_pdpar
@@ -72,6 +79,12 @@ void __led_blink(led_id_t mask, int freq);
 # include <asm/status_led.h>
 #endif
 
+#else
+
+static inline void status_led_init(void) { }
+static inline void status_led_set(int led, int state) { }
+static inline void status_led_boot_blink(void) { }
+
 #endif	/* CONFIG_LED_STATUS	*/
 
 /*
-- 
2.45.2

Re: [PATCH v4 04/11] common: board_r: rework BOOT LED handling

[Simon Glass]

On Sat, 21 Sept 2024 at 00:51, Christian Marangi <ansuelsmth@gmail.com> wrote:
>
> Rework BOOT LED handling. There is currently one legacy implementation
> for BOOT LED from Status Led API.
>
> This work on ancient implementation used by BOOTP by setting the LED
> to Blink on boot and to turn it OFF when the firmware was correctly
> received by network.
>
> Now that we new LED implementation have support for LED boot, rework
> this by also set the new BOOT LED to blink and also set it to ON before
> entering main loop to confirm successful boot.
>
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> ---
>  common/board_r.c     | 28 ++++++++++++++++++++--------
>  include/status_led.h | 13 +++++++++++++
>  2 files changed, 33 insertions(+), 8 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

[PATCH v4 05/11] led: implement LED activity API

[Christian Marangi]

Implement LED activity API similar to BOOT LED API.

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

User of this API will call led_activity_on/off/blink() to signal these
kind of activity.

New Kconfig is implemented similar to BOOT LED, LED_ACTIVITY to
enable support for it.

It's introduced a new /options/u-boot property "activity-led" and
"activity-led-period" to define the activity LED label and the
default period when the activity LED is set to blink mode.

If "activity-led-period" is not defined, the value of 250 (ms) is
used by default.

If CONFIG_LED_BLINK or CONFIG_LED_SW_BLINK is not enabled,
led_boot_blink call will fallback to simple LED ON.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 drivers/led/Kconfig      |  8 ++++
 drivers/led/led-uclass.c | 94 ++++++++++++++++++++++++++++++++++++++--
 include/led.h            | 52 ++++++++++++++++++++++
 3 files changed, 151 insertions(+), 3 deletions(-)

diff --git a/drivers/led/Kconfig b/drivers/led/Kconfig
index 6149cfa02b8..f0434f247a4 100644
--- a/drivers/led/Kconfig
+++ b/drivers/led/Kconfig
@@ -16,6 +16,14 @@ config LED_BOOT
 
 	  LED boot is a specific LED assigned to signal boot operation status.
 
+config LED_ACTIVITY
+	bool "Enable LED activity support"
+	help
+	  Enable LED activity support.
+
+	  LED activity is a specific LED assigned to signal activity operation
+	  like file trasnfer, flash write/erase...
+
 config LED_BCM6328
 	bool "LED Support for BCM6328"
 	depends on LED && ARCH_BMIPS
diff --git a/drivers/led/led-uclass.c b/drivers/led/led-uclass.c
index c5b560982b0..c87fc7540c7 100644
--- a/drivers/led/led-uclass.c
+++ b/drivers/led/led-uclass.c
@@ -168,12 +168,86 @@ int led_boot_blink(void)
 #endif
 #endif
 
+#ifdef CONFIG_LED_ACTIVITY
+int led_activity_on(void)
+{
+	struct uclass *uc = uclass_find(UCLASS_LED);
+	struct led_uc_priv *priv;
+	struct udevice *dev;
+
+	if (!uc)
+		return -ENOENT;
+
+	priv = uclass_get_priv(uc);
+	if (!priv->activity_led_dev ||
+	    uclass_get_device_tail(priv->activity_led_dev, 0, &dev)) {
+		printf("Failed to get activity LED %s\n",
+		       priv->activity_led_label);
+		return -EINVAL;
+	}
+
+	return led_set_state(dev, LEDST_ON);
+}
+
+int led_activity_off(void)
+{
+	struct uclass *uc = uclass_find(UCLASS_LED);
+	struct led_uc_priv *priv;
+	struct udevice *dev;
+
+	if (!uc)
+		return -ENOENT;
+
+	priv = uclass_get_priv(uc);
+	if (!priv->activity_led_dev ||
+	    uclass_get_device_tail(priv->activity_led_dev, 0, &dev)) {
+		printf("Failed to get activity LED %s\n",
+		       priv->activity_led_label);
+		return -EINVAL;
+	}
+
+	return led_set_state(dev, LEDST_OFF);
+}
+
+#if defined(CONFIG_LED_BLINK) || defined(CONFIG_LED_SW_BLINK)
+int led_activity_blink(void)
+{
+	struct uclass *uc = uclass_find(UCLASS_LED);
+	struct led_uc_priv *priv;
+	struct udevice *dev;
+	int ret;
+
+	if (!uc)
+		return -ENOENT;
+
+	priv = uclass_get_priv(uc);
+	if (!priv->activity_led_dev ||
+	    uclass_get_device_tail(priv->activity_led_dev, 0, &dev)) {
+		printf("Failed to get activity LED %s\n",
+		       priv->activity_led_label);
+		return -EINVAL;
+	}
+
+	ret = led_set_period(dev, priv->activity_led_period);
+	if (ret) {
+		if (ret != -ENOSYS)
+			return ret;
+
+		/* fallback to ON with no set_period and no SW_BLINK */
+		return led_set_state(dev, LEDST_ON);
+	}
+
+	return led_set_state(dev, LEDST_BLINK);
+}
+#endif
+#endif
+
 static int led_post_bind(struct udevice *dev)
 {
 	struct led_uc_plat *uc_plat = dev_get_uclass_plat(dev);
 	const char *default_state;
 
-#ifdef CONFIG_LED_BOOT
+#if defined(CONFIG_LED_BOOT) || defined(CONFIG_LED_ACTIVITY)
 	struct led_uc_priv *priv = uclass_get_priv(dev->uclass);
 #endif
 
@@ -189,6 +263,12 @@ static int led_post_bind(struct udevice *dev)
 		priv->boot_led_dev = dev;
 #endif
 
+#ifdef CONFIG_LED_ACTIVITY
+	/* check if we are binding activity LED and assign it */
+	if (!strcmp(priv->activity_led_label, uc_plat->label))
+		priv->activity_led_dev = dev;
+#endif
+
 	uc_plat->default_state = LEDST_COUNT;
 
 	default_state = dev_read_string(dev, "default-state");
@@ -242,13 +322,21 @@ static int led_post_probe(struct udevice *dev)
 	return ret;
 }
 
-#ifdef CONFIG_LED_BOOT
+#if defined(CONFIG_LED_BOOT) || defined(CONFIG_LED_ACTIVITY)
 static int led_init(struct uclass *uc)
 {
 	struct led_uc_priv *priv = uclass_get_priv(uc);
 
+#ifdef CONFIG_LED_BOOT
 	priv->boot_led_label = ofnode_options_read_str("boot-led");
 	priv->boot_led_period = ofnode_options_read_int("boot-led-period", 250);
+#endif
+
+#ifdef CONFIG_LED_ACTIVITY
+	priv->activity_led_label = ofnode_options_read_str("activity-led");
+	priv->activity_led_period = ofnode_options_read_int("activity-led-period",
+							    250);
+#endif
 
 	return 0;
 }
@@ -260,7 +348,7 @@ UCLASS_DRIVER(led) = {
 	.per_device_plat_auto	= sizeof(struct led_uc_plat),
 	.post_bind	= led_post_bind,
 	.post_probe	= led_post_probe,
-#ifdef CONFIG_LED_BOOT
+#if defined(CONFIG_LED_BOOT) || defined(CONFIG_LED_ACTIVITY)
 	.init		= led_init,
 	.priv_auto	= sizeof(struct led_uc_priv),
 #endif
diff --git a/include/led.h b/include/led.h
index ca495217777..bba8c0009ca 100644
--- a/include/led.h
+++ b/include/led.h
@@ -54,8 +54,11 @@ struct led_uc_plat {
  * struct led_uc_priv - Private data the uclass stores about each device
  *
  * @boot_led_label:	Boot LED label
+ * @activity_led_label:	Activity LED label
  * @boot_led_dev:	Boot LED dev
+ * @activity_led_dev:	Activity LED dev
  * @boot_led_period:	Boot LED blink period
+ * @activity_led_period: Activity LED blink period
  */
 struct led_uc_priv {
 #ifdef CONFIG_LED_BOOT
@@ -63,6 +66,11 @@ struct led_uc_priv {
 	struct udevice *boot_led_dev;
 	int boot_led_period;
 #endif
+#ifdef CONFIG_LED_ACTIVITY
+	const char *activity_led_label;
+	struct udevice *activity_led_dev;
+	int activity_led_period;
+#endif
 };
 
 struct led_ops {
@@ -193,4 +201,48 @@ static inline int led_boot_blink(void)
 }
 #endif
 
+#ifdef CONFIG_LED_ACTIVITY
+
+/**
+ * led_activity_on() - turn ON the designated LED for activity
+ *
+ * Return: 0 if OK, -ve on error
+ */
+int led_activity_on(void);
+
+/**
+ * led_activity_off() - turn OFF the designated LED for activity
+ *
+ * Return: 0 if OK, -ve on error
+ */
+int led_activity_off(void);
+
+#if defined(CONFIG_LED_BLINK) || defined(CONFIG_LED_SW_BLINK)
+/**
+ * led_activity_blink() - turn ON the designated LED for activity
+ *
+ * Return: 0 if OK, -ve on error
+ */
+int led_activity_blink(void);
+#else
+/* If LED BLINK is not supported/enabled, fallback to LED ON */
+#define led_activity_blink led_activity_on
+#endif
+#else
+static inline int led_activity_on(void)
+{
+	return -ENOSYS;
+}
+
+static inline int led_activity_off(void)
+{
+	return -ENOSYS;
+}
+
+static inline int led_activity_blink(void)
+{
+	return -ENOSYS;
+}
+#endif
+
 #endif
-- 
2.45.2

[PATCH v4 06/11] tftp: implement support for LED activity

[Christian Marangi]

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

Also turn the ACTIVITY LED OFF if a CTRL-C is detected in the main
net loop function.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 net/net.c  | 4 ++++
 net/tftp.c | 5 +++++
 2 files changed, 9 insertions(+)

diff --git a/net/net.c b/net/net.c
index d9bc9df643f..94bfde43f85 100644
--- a/net/net.c
+++ b/net/net.c
@@ -87,6 +87,7 @@
 #include <env_internal.h>
 #include <errno.h>
 #include <image.h>
+#include <led.h>
 #include <log.h>
 #include <net.h>
 #include <net6.h>
@@ -659,6 +660,9 @@ restart:
 			/* Invalidate the last protocol */
 			eth_set_last_protocol(BOOTP);
 
+			/* Turn off activity LED if triggered */
+			led_activity_off();
+
 			puts("\nAbort\n");
 			/* include a debug print as well incase the debug
 			   messages are directed to stderr */
diff --git a/net/tftp.c b/net/tftp.c
index 2e073183d5a..5cb06d2038b 100644
--- a/net/tftp.c
+++ b/net/tftp.c
@@ -10,6 +10,7 @@
 #include <efi_loader.h>
 #include <env.h>
 #include <image.h>
+#include <led.h>
 #include <lmb.h>
 #include <log.h>
 #include <mapmem.h>
@@ -192,6 +193,7 @@ static void new_transfer(void)
 #ifdef CONFIG_CMD_TFTPPUT
 	tftp_put_final_block_sent = 0;
 #endif
+	led_activity_blink();
 }
 
 #ifdef CONFIG_CMD_TFTPPUT
@@ -301,6 +303,9 @@ static void tftp_complete(void)
 			time_start * 1000, "/s");
 	}
 	puts("\ndone\n");
+
+	led_activity_off();
+
 	if (!tftp_put_active)
 		efi_set_bootdev("Net", "", tftp_filename,
 				map_sysmem(tftp_load_addr, 0),
-- 
2.45.2

Re: [PATCH v4 06/11] tftp: implement support for LED activity

[Simon Glass]

On Sat, 21 Sept 2024 at 00:51, Christian Marangi <ansuelsmth@gmail.com> wrote:
>
> Implement support for LED activity. If the feature is enabled,
> make the defined ACTIVITY LED to signal traffic.
>
> Also turn the ACTIVITY LED OFF if a CTRL-C is detected in the main
> net loop function.
>
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> ---
>  net/net.c  | 4 ++++
>  net/tftp.c | 5 +++++
>  2 files changed, 9 insertions(+)
>

Reviewed-by: Simon Glass <sjg@chromium.org>

[PATCH v4 07/11] mtd: implement support for LED activity

[Christian Marangi]

Implement support for LED 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 | 11 +++++++++++
 1 file changed, 11 insertions(+)

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

Re: [PATCH v4 07/11] mtd: implement support for LED activity

[Miquel Raynal]

Hi Christian,

ansuelsmth@gmail.com wrote on Sat, 21 Sep 2024 00:50:00 +0200:

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

I'm curious, why did you not consider reads in your proposal? I think
in general as long as you use a device the LED should blink. While
you're performing a read you cannot do anything else with the chip so I
would definitely consider the read path as well.

Also, I would expect the blinking to continue when the device is
accessed, no matter who is asking for it. So for instance when I load
my kernel into RAM, I believe it should blink. Hence, why not
considering the mtd layer rather than the command .c file?

> 
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> ---
>  cmd/mtd.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/cmd/mtd.c b/cmd/mtd.c
> index 795aaa2b37d..dae90b0e6e4 100644
> --- a/cmd/mtd.c
> +++ b/cmd/mtd.c

Thanks,
Miquèl

Re: [PATCH v4 07/11] mtd: implement support for LED activity

[Christian Marangi]

On Sat, Sep 21, 2024 at 12:13:34PM +0200, Miquel Raynal wrote:
> Hi Christian,
> 
> ansuelsmth@gmail.com wrote on Sat, 21 Sep 2024 00:50:00 +0200:
> 
> > Implement support for LED activity. If the feature is enabled,
> > make the defined ACTIVITY LED to signal mtd write or erase operations.
> 
> I'm curious, why did you not consider reads in your proposal? I think
> in general as long as you use a device the LED should blink. While
> you're performing a read you cannot do anything else with the chip so I
> would definitely consider the read path as well.
> 
> Also, I would expect the blinking to continue when the device is
> accessed, no matter who is asking for it. So for instance when I load
> my kernel into RAM, I believe it should blink. Hence, why not
> considering the mtd layer rather than the command .c file?
>

My idea for the feature was really for recovery and flashing usage, soo
everything that is initiated by the user or that does change thing.

Example a button trigger an automatic recovery procedure from an
attached USB drive. Having the LED blink signal the user the procedure
is actually working and something is happening.

But yes yours is not a bad idea, I don't think it would make a recovery
procedure confusing, lets see if others agree on that ok?

> > 
> > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > ---
> >  cmd/mtd.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> > 
> > diff --git a/cmd/mtd.c b/cmd/mtd.c
> > index 795aaa2b37d..dae90b0e6e4 100644
> > --- a/cmd/mtd.c
> > +++ b/cmd/mtd.c
> 
> Thanks,
> Miquèl

-- 
	Ansuel

[PATCH v4 08/11] ubi: implement support for LED activity

[Christian Marangi]

Implement support for LED 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 | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/cmd/ubi.c b/cmd/ubi.c
index 0e62e449327..56d7da82629 100644
--- a/cmd/ubi.c
+++ b/cmd/ubi.c
@@ -14,6 +14,7 @@
 #include <command.h>
 #include <env.h>
 #include <exports.h>
+#include <led.h>
 #include <malloc.h>
 #include <memalign.h>
 #include <mtd.h>
@@ -488,10 +489,18 @@ exit:
 
 int ubi_volume_write(char *volume, void *buf, loff_t offset, size_t size)
 {
+	int ret;
+
+	led_activity_blink();
+
 	if (!offset)
-		return ubi_volume_begin_write(volume, buf, size, size);
+		ret = ubi_volume_begin_write(volume, buf, size, size);
+	else
+		ret = ubi_volume_offset_write(volume, buf, offset, size);
 
-	return ubi_volume_offset_write(volume, buf, offset, size);
+	led_activity_off();
+
+	return ret;
 }
 
 int ubi_volume_read(char *volume, char *buf, loff_t offset, size_t size)
-- 
2.45.2

Re: [PATCH v4 08/11] ubi: implement support for LED activity

[Simon Glass]

On Sat, 21 Sept 2024 at 00:52, Christian Marangi <ansuelsmth@gmail.com> wrote:
>
> Implement support for LED 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 | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
>

Reviewed-by: Simon Glass <sjg@chromium.org>

[PATCH v4 09/11] doc: introduce led.rst documentation

[Christian Marangi]

Introduce simple led.rst documentation to document all the additional
Kconfig and the current limitation of LED_BLINK and GPIO software blink.

Also add missing definition for sw_blink in led_uc_plat struct.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 doc/api/index.rst |  1 +
 doc/api/led.rst   | 10 ++++++++++
 include/led.h     | 41 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 52 insertions(+)
 create mode 100644 doc/api/led.rst

diff --git a/doc/api/index.rst b/doc/api/index.rst
index ec0b8adb2cf..9f7f23f868f 100644
--- a/doc/api/index.rst
+++ b/doc/api/index.rst
@@ -14,6 +14,7 @@ U-Boot API documentation
    event
    getopt
    interrupt
+   led
    linker_lists
    lmb
    logging
diff --git a/doc/api/led.rst b/doc/api/led.rst
new file mode 100644
index 00000000000..e52e350d1bb
--- /dev/null
+++ b/doc/api/led.rst
@@ -0,0 +1,10 @@
+.. SPDX-License-Identifier: GPL-2.0+
+
+LED
+===
+
+.. kernel-doc:: include/led.h
+   :doc: Overview
+
+.. kernel-doc:: include/led.h
+   :internal:
\ No newline at end of file
diff --git a/include/led.h b/include/led.h
index bba8c0009ca..a9dd55efd1f 100644
--- a/include/led.h
+++ b/include/led.h
@@ -11,6 +11,46 @@
 #include <cyclic.h>
 #include <dm/ofnode.h>
 
+/**
+ * DOC: Overview
+ *
+ * Generic LED API provided when a supported compatible is defined in DeviceTree.
+ *
+ * To enable support for LEDs, enable the `CONFIG_LED` Kconfig option.
+ *
+ * The most common implementation is for GPIO-connected LEDs. If using GPIO-connected LEDs,
+ * enable the `LED_GPIO` Kconfig option.
+ *
+ * `LED_BLINK` support requires LED driver support and is therefore optional. If LED blink
+ * functionality is needed, enable the `LED_BLINK` Kconfig option. If LED driver doesn't
+ * support HW Blink, SW Blink can be used with the Cyclic framework by enabling the
+ * CONFIG_LED_SW_BLINK.
+ *
+ * Boot and Activity LEDs are also supported. These LEDs can signal various system operations
+ * during runtime, such as boot initialization, file transfers, and flash write/erase operations.
+ *
+ * To enable a Boot LED, enable `CONFIG_LED_BOOT` and define in `/options/u-boot` root node the
+ * property `boot-led`. This will enable the specified LED to blink and turn ON when
+ * the bootloader initializes correctly.
+ *
+ * To enable an Activity LED, enable `CONFIG_LED_ACTIVITY` and define in `/options/u-boot` root
+ * node the property `activity-led`.
+ * This will enable the specified LED to blink and turn ON during file transfers or flash
+ * write/erase operations.
+ *
+ * Both Boot and Activity LEDs provide a simple API to turn the LED ON or OFF:
+ * `led_boot_on()`, `led_boot_off()`, `led_activity_on()`, and `led_activity_off()`.
+ *
+ * Both configurations can optionally define a `boot/activity-led-period` property
+ * if `CONFIG_LED_BLINK` or `CONFIG_LED_SW_BLINK` is enabled for LED blink operations, which
+ * is usually used by the Activity LED. If not defined the default value of 250 (ms) is used.
+ *
+ * When `CONFIG_LED_BLINK` or `CONFIG_LED_SW_BLINK` is enabled, additional APIs are exposed:
+ * `led_boot_blink()` and `led_activity_blink()`. Note that if `CONFIG_LED_BLINK` or
+ * `CONFIG_LED_SW_BLINK` is disabled, these APIs will behave like the `led_boot_on()` and
+ * `led_activity_on()` APIs, respectively.
+ */
+
 struct udevice;
 
 enum led_state_t {
@@ -41,6 +81,7 @@ struct led_sw_blink {
  *
  * @label:	LED label
  * @default_state:	LED default state
+ * @sw_blink:	LED software blink struct
  */
 struct led_uc_plat {
 	const char *label;
-- 
2.45.2

Re: [PATCH v4 09/11] doc: introduce led.rst documentation

[Simon Glass]

On Sat, 21 Sept 2024 at 00:52, Christian Marangi <ansuelsmth@gmail.com> wrote:
>
> Introduce simple led.rst documentation to document all the additional
> Kconfig and the current limitation of LED_BLINK and GPIO software blink.
>
> Also add missing definition for sw_blink in led_uc_plat struct.
>
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> ---
>  doc/api/index.rst |  1 +
>  doc/api/led.rst   | 10 ++++++++++
>  include/led.h     | 41 +++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 52 insertions(+)
>  create mode 100644 doc/api/led.rst

Reviewed-by: Simon Glass <sjg@chromium.org>

[PATCH v4 10/11] test: dm: Add tests for LED boot and activity

[Christian Marangi]

Add tests for LED boot and activity feature and add required property in
sandbox test DTS.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 arch/sandbox/dts/test.dts |  2 ++
 test/dm/led.c             | 72 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 74 insertions(+)

diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts
index 5fb5eac862e..25859ad852d 100644
--- a/arch/sandbox/dts/test.dts
+++ b/arch/sandbox/dts/test.dts
@@ -101,6 +101,8 @@
 			bootscr-ram-offset = /bits/ 64 <0x12345678>;
 			bootscr-flash-offset = /bits/ 64 <0>;
 			bootscr-flash-size = /bits/ 64 <0x2000>;
+			boot-led = "sandbox:green";
+			activity-led = "sandbox:red";
 		};
 	};
 
diff --git a/test/dm/led.c b/test/dm/led.c
index c28fa044f45..4b019c71f3a 100644
--- a/test/dm/led.c
+++ b/test/dm/led.c
@@ -137,3 +137,75 @@ static int dm_test_led_blink(struct unit_test_state *uts)
 }
 DM_TEST(dm_test_led_blink, UT_TESTF_SCAN_PDATA | UT_TESTF_SCAN_FDT);
 #endif
+
+/* Test LED boot */
+#ifdef CONFIG_LED_BOOT
+static int dm_test_led_boot(struct unit_test_state *uts)
+{
+	struct udevice *dev
+
+	/* options/u-boot/boot-led is set to "sandbox:green" */
+	ut_assertok(led_get_by_label("sandbox:green", &dev));
+	ut_asserteq(LEDST_OFF, led_get_state(dev));
+	ut_assertok(led_boot_on());
+	ut_asserteq(LEDST_ON, led_get_state(dev));
+	ut_assertok(led_boot_off());
+	ut_asserteq(LEDST_OFF, led_get_state(dev));
+
+	return 0;
+}
+
+/* Test LED boot blink fallback */
+#ifndef CONFIG_LED_BLINK
+static int dm_test_led_boot(struct unit_test_state *uts)
+{
+	struct udevice *dev
+
+	/* options/u-boot/boot-led is set to "sandbox:green" */
+	ut_assertok(led_get_by_label("sandbox:green", &dev));
+	ut_asserteq(LEDST_OFF, led_get_state(dev));
+	ut_assertok(led_boot_blink());
+	ut_asserteq(LEDST_ON, led_get_state(dev));
+	ut_assertok(led_boot_off());
+	ut_asserteq(LEDST_OFF, led_get_state(dev));
+
+	return 0;
+}
+#endif
+#endif
+
+/* Test LED activity */
+#ifdef CONFIG_LED_ACTIVITY
+static int dm_test_led_boot(struct unit_test_state *uts)
+{
+	struct udevice *dev
+
+	/* options/u-boot/activity-led is set to "sandbox:red" */
+	ut_assertok(led_get_by_label("sandbox:red", &dev));
+	ut_asserteq(LEDST_OFF, led_get_state(dev));
+	ut_assertok(led_activity_on());
+	ut_asserteq(LEDST_ON, led_get_state(dev));
+	ut_assertok(led_activity_off());
+	ut_asserteq(LEDST_OFF, led_get_state(dev));
+
+	return 0;
+}
+
+/* Test LED activity blink fallback */
+#ifndef CONFIG_LED_BLINK
+static int dm_test_led_boot(struct unit_test_state *uts)
+{
+	struct udevice *dev
+
+	/* options/u-boot/activity-led is set to "sandbox:red" */
+	ut_assertok(led_get_by_label("sandbox:red", &dev));
+	ut_asserteq(LEDST_OFF, led_get_state(dev));
+	ut_assertok(led_activity_blink());
+	ut_asserteq(LEDST_ON, led_get_state(dev));
+	ut_assertok(led_activity_off());
+	ut_asserteq(LEDST_OFF, led_get_state(dev));
+
+	return 0;
+}
+#endif
+#endif
-- 
2.45.2

Re: [PATCH v4 10/11] test: dm: Add tests for LED boot and activity

[Simon Glass]

On Sat, 21 Sept 2024 at 00:52, Christian Marangi <ansuelsmth@gmail.com> wrote:
>
> Add tests for LED boot and activity feature and add required property in
> sandbox test DTS.
>
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> ---
>  arch/sandbox/dts/test.dts |  2 ++
>  test/dm/led.c             | 72 +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 74 insertions(+)

Reviewed-by: Simon Glass <sjg@chromium.org>

[PATCH v4 11/11] test: dm: Expand ofnode options test with new helper

[Christian Marangi]

Expand ofnode options test with new generic helper for bool, int and
string.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 arch/sandbox/dts/test.dts | 3 +++
 test/dm/ofnode.c          | 9 +++++++++
 2 files changed, 12 insertions(+)

diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts
index 25859ad852d..e5381b56da4 100644
--- a/arch/sandbox/dts/test.dts
+++ b/arch/sandbox/dts/test.dts
@@ -103,6 +103,9 @@
 			bootscr-flash-size = /bits/ 64 <0x2000>;
 			boot-led = "sandbox:green";
 			activity-led = "sandbox:red";
+			testing-bool;
+			testing-int = <123>;
+			testing-str = "testing";
 		};
 	};
 
diff --git a/test/dm/ofnode.c b/test/dm/ofnode.c
index 39191d7f52b..7c0adcd596b 100644
--- a/test/dm/ofnode.c
+++ b/test/dm/ofnode.c
@@ -614,6 +614,15 @@ static int dm_test_ofnode_options(struct unit_test_state *uts)
 	u64 bootscr_address, bootscr_offset;
 	u64 bootscr_flash_offset, bootscr_flash_size;
 
+	ut_assert(!ofnode_options_read_bool("missing"));
+	ut_assert(ofnode_options_read_bool("testing-bool"));
+
+	ut_asserteq(123, ofnode_options_read_int("testing-int", 0));
+	ut_asserteq(6, ofnode_options_read_int("missing", 6));
+
+	ut_assertnull(ofnode_options_read_str("missing"));
+	ut_asserteq_str("testing", ofnode_options_read_str("testing-str"));
+
 	ut_assertok(ofnode_read_bootscript_address(&bootscr_address,
 						   &bootscr_offset));
 	ut_asserteq_64(0, bootscr_address);
-- 
2.45.2

Re: [PATCH v4 11/11] test: dm: Expand ofnode options test with new helper

[Simon Glass]

On Sat, 21 Sept 2024 at 00:52, Christian Marangi <ansuelsmth@gmail.com> wrote:
>
> Expand ofnode options test with new generic helper for bool, int and
> string.
>
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> ---
>  arch/sandbox/dts/test.dts | 3 +++
>  test/dm/ofnode.c          | 9 +++++++++
>  2 files changed, 12 insertions(+)
>

Reviewed-by: Simon Glass <sjg@chromium.org>