[PATCH 1/2] led: Implement software led blinking

[PATCH 1/2] led: Implement software led blinking

[Mikhail Kshevetskiy]

From: Michael Polyntsov <michael.polyntsov@iopsys.eu>

If hardware (or driver) doesn't support leds blinking, it's
now possible to use software implementation of blinking instead.
This relies on cyclic functions.

Signed-off-by: Michael Polyntsov <michael.polyntsov@iopsys.eu>
Signed-off-by: Mikhail Kshevetskiy <mikhail.kshevetskiy@iopsys.eu>
---
 drivers/led/Kconfig      |   9 ++
 drivers/led/led-uclass.c | 190 ++++++++++++++++++++++++++++++++++++++-
 2 files changed, 195 insertions(+), 4 deletions(-)

diff --git a/drivers/led/Kconfig b/drivers/led/Kconfig
index 9837960198d..4330f014239 100644
--- a/drivers/led/Kconfig
+++ b/drivers/led/Kconfig
@@ -73,6 +73,15 @@ config LED_BLINK
 	  This option enables support for this which adds slightly to the
 	  code size.
 
+config LED_SW_BLINK
+	bool "Support software LED blinking"
+	depends on LED_BLINK
+	select CYCLIC
+	help
+	  Turns on led blinking implemented in the software, useful when
+	  the hardware doesn't support led blinking. Does nothing if
+	  driver supports blinking.
+
 config SPL_LED
 	bool "Enable LED support in SPL"
 	depends on SPL_DM
diff --git a/drivers/led/led-uclass.c b/drivers/led/led-uclass.c
index a4be56fc258..b35964f2e99 100644
--- a/drivers/led/led-uclass.c
+++ b/drivers/led/led-uclass.c
@@ -15,6 +15,10 @@
 #include <dm/root.h>
 #include <dm/uclass-internal.h>
 
+#ifdef CONFIG_LED_SW_BLINK
+#include <malloc.h>
+#endif
+
 int led_bind_generic(struct udevice *parent, const char *driver_name)
 {
 	struct udevice *dev;
@@ -41,6 +45,7 @@ int led_get_by_label(const char *label, struct udevice **devp)
 	ret = uclass_get(UCLASS_LED, &uc);
 	if (ret)
 		return ret;
+
 	uclass_foreach_dev(dev, uc) {
 		struct led_uc_plat *uc_plat = dev_get_uclass_plat(dev);
 
@@ -52,14 +57,180 @@ int led_get_by_label(const char *label, struct udevice **devp)
 	return -ENODEV;
 }
 
-int led_set_state(struct udevice *dev, enum led_state_t state)
+#ifdef CONFIG_LED_SW_BLINK
+
+enum led_sw_blink_state_t {
+	LED_SW_BLINK_ST_OFF = 0,
+	LED_SW_BLINK_ST_ON = 1,
+	LED_SW_BLINK_ST_NONE = 2,
+};
+
+struct sw_blink_state {
+	struct udevice *dev;
+	enum led_sw_blink_state_t cur_blink_state;
+};
+
+static bool led_driver_supports_hw_blinking(const struct udevice *dev)
+{
+	struct led_ops *ops = led_get_ops(dev);
+
+	/*
+	 * We assume that if driver supports set_period, then it correctly
+	 * handles all other requests, for example, that
+	 * led_set_state(LEDST_BLINK) works correctly.
+	 */
+	return ops->set_period != NULL;
+}
+
+static const char *led_sw_label_to_cyclic_func_name(const char *label)
+{
+#define MAX_NAME_LEN 50
+	static char cyclic_func_name[MAX_NAME_LEN] = {0};
+
+	snprintf(cyclic_func_name, MAX_NAME_LEN, "sw_blink_%s", label);
+	return cyclic_func_name;
+#undef MAX_NAME_LEN
+}
+
+static struct cyclic_info *led_sw_find_blinking_led(const char *label)
+{
+	struct cyclic_info *cyclic;
+	const char *cyclic_name;
+
+	cyclic_name = led_sw_label_to_cyclic_func_name(label);
+
+	hlist_for_each_entry(cyclic, cyclic_get_list(), list) {
+		if (strcmp(cyclic->name, cyclic_name) == 0)
+			return cyclic;
+	}
+
+	return NULL;
+}
+
+static bool led_sw_is_blinking(struct udevice *dev)
+{
+	struct led_uc_plat *uc_plat = dev_get_uclass_plat(dev);
+	struct cyclic_info *cyclic = led_sw_find_blinking_led(uc_plat->label);
+
+	if (cyclic != NULL) {
+		struct sw_blink_state *state;
+
+		state = (struct sw_blink_state *)cyclic->ctx;
+		return state->cur_blink_state != LED_SW_BLINK_ST_NONE;
+	}
+
+	return false;
+}
+
+static void led_sw_blink(void *void_state)
+{
+	struct sw_blink_state *state = (struct sw_blink_state *)void_state;
+	struct udevice *dev = state->dev;
+	struct led_ops *ops = led_get_ops(dev);
+
+	switch (state->cur_blink_state) {
+	case LED_SW_BLINK_ST_OFF:
+		state->cur_blink_state = LED_SW_BLINK_ST_ON;
+		ops->set_state(dev, LEDST_ON);
+		break;
+	case LED_SW_BLINK_ST_ON:
+		state->cur_blink_state = LED_SW_BLINK_ST_OFF;
+		ops->set_state(dev, LEDST_OFF);
+		break;
+	case LED_SW_BLINK_ST_NONE:
+		/*
+		 * led_set_period has been called, but
+		 * led_set_state(LDST_BLINK) has not yet,
+		 * so doing nothing
+		 */
+		break;
+	}
+}
+
+static void led_sw_free_cyclic(struct cyclic_info *cyclic)
+{
+	free(cyclic->ctx);
+	cyclic_unregister(cyclic);
+}
+
+static int led_sw_set_period(struct udevice *dev, int period_ms)
+{
+	struct cyclic_info *cyclic;
+	struct sw_blink_state *state;
+	struct led_uc_plat *uc_plat = dev_get_uclass_plat(dev);
+	const char *cyclic_func_name;
+
+	state = malloc(sizeof(struct sw_blink_state));
+	if (state == NULL) {
+		printf("Allocating memory for sw_blink_state for %s failed\n",
+		       uc_plat->label);
+		return -ENOMEM;
+	}
+
+	state->cur_blink_state = LED_SW_BLINK_ST_NONE;
+	state->dev = dev;
+
+	/*
+	 * Make sure that there is no cyclic function already
+	 * registered for this label
+	 */
+	cyclic = led_sw_find_blinking_led(uc_plat->label);
+	if (cyclic != NULL)
+		led_sw_free_cyclic(cyclic);
+
+	cyclic_func_name = led_sw_label_to_cyclic_func_name(uc_plat->label);
+
+	cyclic = cyclic_register(led_sw_blink, period_ms * 1000,
+				 cyclic_func_name, (void *)state);
+	if (cyclic == NULL) {
+		printf("Registering of blinking function for %s failed\n",
+		       uc_plat->label);
+		free(state);
+		return -ENOMEM;
+	}
+
+	return 0;
+}
+
+#endif
+
+int led_set_state(struct udevice *dev, enum led_state_t new_state)
 {
 	struct led_ops *ops = led_get_ops(dev);
 
 	if (!ops->set_state)
 		return -ENOSYS;
 
-	return ops->set_state(dev, state);
+#ifdef CONFIG_LED_SW_BLINK
+	if (!led_driver_supports_hw_blinking(dev)) {
+		struct cyclic_info *cyclic;
+		struct led_uc_plat *uc_plat = dev_get_uclass_plat(dev);
+
+		cyclic = led_sw_find_blinking_led(uc_plat->label);
+
+		if (cyclic != NULL) {
+			if (new_state == LEDST_BLINK) {
+				struct sw_blink_state *cur_st;
+
+				cur_st = (struct sw_blink_state *)cyclic->ctx;
+
+				/*
+				 * Next call to led_sw_blink will start blinking
+				 */
+				cur_st->cur_blink_state = LED_SW_BLINK_ST_OFF;
+				return 0;
+			}
+
+			/*
+			 * Changing current blinking state to
+			 * something else
+			 */
+			led_sw_free_cyclic(cyclic);
+		}
+	}
+#endif
+
+	return ops->set_state(dev, new_state);
 }
 
 enum led_state_t led_get_state(struct udevice *dev)
@@ -69,19 +240,31 @@ enum led_state_t led_get_state(struct udevice *dev)
 	if (!ops->get_state)
 		return -ENOSYS;
 
+#ifdef CONFIG_LED_SW_BLINK
+	if (!led_driver_supports_hw_blinking(dev) && led_sw_is_blinking(dev))
+		return LEDST_BLINK;
+#endif
+
 	return ops->get_state(dev);
 }
 
 #ifdef CONFIG_LED_BLINK
+
 int led_set_period(struct udevice *dev, int period_ms)
 {
 	struct led_ops *ops = led_get_ops(dev);
 
-	if (!ops->set_period)
+	if (!ops->set_period) {
+#ifdef CONFIG_LED_SW_BLINK
+		return led_sw_set_period(dev, period_ms);
+#else
 		return -ENOSYS;
+#endif
+	}
 
 	return ops->set_period(dev, period_ms);
 }
+
 #endif
 
 static int led_post_bind(struct udevice *dev)
@@ -113,7 +296,6 @@ static int led_post_bind(struct udevice *dev)
 	 * probe() to configure its default state during startup.
 	 */
 	dev_or_flags(dev, DM_FLAG_PROBE_AFTER_BIND);
-
 	return 0;
 }
 
-- 
2.43.0

[PATCH 2/2] led: Add dts property to specify blinking of the led

[Mikhail Kshevetskiy]

From: Michael Polyntsov <michael.polyntsov@iopsys.eu>

The standard property

    linux,default-trigger = "pattern";

used to get an effect. No blinking parameters can be set yet.

Signed-off-by: Michael Polyntsov <michael.polyntsov@iopsys.eu>
Signed-off-by: Mikhail Kshevetskiy <mikhail.kshevetskiy@iopsys.eu>
---
 drivers/led/led-uclass.c | 34 ++++++++++++++++++++++++++++++----
 1 file changed, 30 insertions(+), 4 deletions(-)

diff --git a/drivers/led/led-uclass.c b/drivers/led/led-uclass.c
index b35964f2e99..645d0069efe 100644
--- a/drivers/led/led-uclass.c
+++ b/drivers/led/led-uclass.c
@@ -271,6 +271,9 @@ 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_BLINK
+	const char *trigger;
+#endif
 
 	if (!uc_plat->label)
 		uc_plat->label = dev_read_string(dev, "label");
@@ -291,6 +294,13 @@ static int led_post_bind(struct udevice *dev)
 	else
 		return 0;
 
+#ifdef CONFIG_LED_BLINK
+	trigger = dev_read_string(dev, "linux,default-trigger");
+	if (trigger && !strncmp(trigger, "pattern", 7)) {
+		uc_plat->default_state = LEDST_BLINK;
+	}
+#endif
+
 	/*
 	 * In case the LED has default-state DT property, trigger
 	 * probe() to configure its default state during startup.
@@ -302,12 +312,28 @@ static int led_post_bind(struct udevice *dev)
 static int led_post_probe(struct udevice *dev)
 {
 	struct led_uc_plat *uc_plat = dev_get_uclass_plat(dev);
+	int rc = 0;
 
-	if (uc_plat->default_state == LEDST_ON ||
-	    uc_plat->default_state == LEDST_OFF)
-		led_set_state(dev, uc_plat->default_state);
+	switch (uc_plat->default_state) {
+	case LEDST_ON:
+	case LEDST_OFF:
+		rc = led_set_state(dev, uc_plat->default_state);
+		break;
+#ifdef CONFIG_LED_BLINK
+	case LEDST_BLINK: {
+		const int default_period_ms = 1000;
 
-	return 0;
+		rc = led_set_period(dev, default_period_ms);
+		if (rc == 0)
+			rc = led_set_state(dev, uc_plat->default_state);
+		break;
+	}
+#endif
+	default:
+		break;
+	}
+
+	return rc;
 }
 
 UCLASS_DRIVER(led) = {
-- 
2.43.0

Re: [PATCH 1/2] led: Implement software led blinking

[Simon Glass]

Hi Mikhail,

On Thu, 27 Jun 2024 at 12:31, Mikhail Kshevetskiy
<mikhail.kshevetskiy@iopsys.eu> wrote:
>
> From: Michael Polyntsov <michael.polyntsov@iopsys.eu>
>
> If hardware (or driver) doesn't support leds blinking, it's
> now possible to use software implementation of blinking instead.
> This relies on cyclic functions.
>
> Signed-off-by: Michael Polyntsov <michael.polyntsov@iopsys.eu>
> Signed-off-by: Mikhail Kshevetskiy <mikhail.kshevetskiy@iopsys.eu>
> ---
>  drivers/led/Kconfig      |   9 ++
>  drivers/led/led-uclass.c | 190 ++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 195 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/led/Kconfig b/drivers/led/Kconfig
> index 9837960198d..4330f014239 100644
> --- a/drivers/led/Kconfig
> +++ b/drivers/led/Kconfig
> @@ -73,6 +73,15 @@ config LED_BLINK
>           This option enables support for this which adds slightly to the
>           code size.
>
> +config LED_SW_BLINK
> +       bool "Support software LED blinking"
> +       depends on LED_BLINK
> +       select CYCLIC
> +       help
> +         Turns on led blinking implemented in the software, useful when
> +         the hardware doesn't support led blinking. Does nothing if
> +         driver supports blinking.

Can you talk about the blinking p[eriod / API?

> +
>  config SPL_LED
>         bool "Enable LED support in SPL"
>         depends on SPL_DM
> diff --git a/drivers/led/led-uclass.c b/drivers/led/led-uclass.c
> index a4be56fc258..b35964f2e99 100644
> --- a/drivers/led/led-uclass.c
> +++ b/drivers/led/led-uclass.c
> @@ -15,6 +15,10 @@
>  #include <dm/root.h>
>  #include <dm/uclass-internal.h>
>
> +#ifdef CONFIG_LED_SW_BLINK
> +#include <malloc.h>
> +#endif

You should not need to #ifdef include files

> +
>  int led_bind_generic(struct udevice *parent, const char *driver_name)
>  {
>         struct udevice *dev;
> @@ -41,6 +45,7 @@ int led_get_by_label(const char *label, struct udevice **devp)
>         ret = uclass_get(UCLASS_LED, &uc);
>         if (ret)
>                 return ret;
> +
>         uclass_foreach_dev(dev, uc) {
>                 struct led_uc_plat *uc_plat = dev_get_uclass_plat(dev);
>
> @@ -52,14 +57,180 @@ int led_get_by_label(const char *label, struct udevice **devp)
>         return -ENODEV;
>  }
>
> -int led_set_state(struct udevice *dev, enum led_state_t state)
> +#ifdef CONFIG_LED_SW_BLINK
> +
> +enum led_sw_blink_state_t {
> +       LED_SW_BLINK_ST_OFF = 0,
> +       LED_SW_BLINK_ST_ON = 1,
> +       LED_SW_BLINK_ST_NONE = 2,
> +};
> +
> +struct sw_blink_state {
> +       struct udevice *dev;
> +       enum led_sw_blink_state_t cur_blink_state;
> +};
> +
> +static bool led_driver_supports_hw_blinking(const struct udevice *dev)
> +{
> +       struct led_ops *ops = led_get_ops(dev);
> +
> +       /*
> +        * We assume that if driver supports set_period, then it correctly
> +        * handles all other requests, for example, that
> +        * led_set_state(LEDST_BLINK) works correctly.
> +        */
> +       return ops->set_period != NULL;
> +}
> +
> +static const char *led_sw_label_to_cyclic_func_name(const char *label)
> +{
> +#define MAX_NAME_LEN 50
> +       static char cyclic_func_name[MAX_NAME_LEN] = {0};
> +
> +       snprintf(cyclic_func_name, MAX_NAME_LEN, "sw_blink_%s", label);
> +       return cyclic_func_name;
> +#undef MAX_NAME_LEN
> +}
> +
> +static struct cyclic_info *led_sw_find_blinking_led(const char *label)
> +{
> +       struct cyclic_info *cyclic;
> +       const char *cyclic_name;
> +
> +       cyclic_name = led_sw_label_to_cyclic_func_name(label);
> +
> +       hlist_for_each_entry(cyclic, cyclic_get_list(), list) {
> +               if (strcmp(cyclic->name, cyclic_name) == 0)
> +                       return cyclic;
> +       }
> +
> +       return NULL;
> +}
> +
> +static bool led_sw_is_blinking(struct udevice *dev)
> +{
> +       struct led_uc_plat *uc_plat = dev_get_uclass_plat(dev);
> +       struct cyclic_info *cyclic = led_sw_find_blinking_led(uc_plat->label);
> +
> +       if (cyclic != NULL) {

if (cyclic) {

> +               struct sw_blink_state *state;
> +
> +               state = (struct sw_blink_state *)cyclic->ctx;
> +               return state->cur_blink_state != LED_SW_BLINK_ST_NONE;
> +       }
> +
> +       return false;
> +}
> +
> +static void led_sw_blink(void *void_state)
> +{
> +       struct sw_blink_state *state = (struct sw_blink_state *)void_state;

You should not need that cast

> +       struct udevice *dev = state->dev;
> +       struct led_ops *ops = led_get_ops(dev);
> +
> +       switch (state->cur_blink_state) {
> +       case LED_SW_BLINK_ST_OFF:
> +               state->cur_blink_state = LED_SW_BLINK_ST_ON;
> +               ops->set_state(dev, LEDST_ON);
> +               break;
> +       case LED_SW_BLINK_ST_ON:
> +               state->cur_blink_state = LED_SW_BLINK_ST_OFF;
> +               ops->set_state(dev, LEDST_OFF);
> +               break;
> +       case LED_SW_BLINK_ST_NONE:
> +               /*
> +                * led_set_period has been called, but
> +                * led_set_state(LDST_BLINK) has not yet,
> +                * so doing nothing
> +                */
> +               break;
> +       }
> +}
> +
> +static void led_sw_free_cyclic(struct cyclic_info *cyclic)
> +{
> +       free(cyclic->ctx);
> +       cyclic_unregister(cyclic);
> +}
> +
> +static int led_sw_set_period(struct udevice *dev, int period_ms)
> +{
> +       struct cyclic_info *cyclic;
> +       struct sw_blink_state *state;
> +       struct led_uc_plat *uc_plat = dev_get_uclass_plat(dev);
> +       const char *cyclic_func_name;
> +
> +       state = malloc(sizeof(struct sw_blink_state));
> +       if (state == NULL) {
> +               printf("Allocating memory for sw_blink_state for %s failed\n",
> +                      uc_plat->label);
> +               return -ENOMEM;
> +       }

I think it would be better to put struct sw_blink_state fields inside
uc_plat. After all, it is pretty tiny. We try not to malloc() data for
devices.

> +
> +       state->cur_blink_state = LED_SW_BLINK_ST_NONE;
> +       state->dev = dev;
> +
> +       /*
> +        * Make sure that there is no cyclic function already
> +        * registered for this label
> +        */
> +       cyclic = led_sw_find_blinking_led(uc_plat->label);
> +       if (cyclic != NULL)

if (cyclic)

please fix globally

> +               led_sw_free_cyclic(cyclic);
> +
> +       cyclic_func_name = led_sw_label_to_cyclic_func_name(uc_plat->label);
> +
> +       cyclic = cyclic_register(led_sw_blink, period_ms * 1000,
> +                                cyclic_func_name, (void *)state);
> +       if (cyclic == NULL) {
> +               printf("Registering of blinking function for %s failed\n",
> +                      uc_plat->label);

log_err()

> +               free(state);
> +               return -ENOMEM;
> +       }
> +
> +       return 0;
> +}
> +
> +#endif
> +
> +int led_set_state(struct udevice *dev, enum led_state_t new_state)
>  {
>         struct led_ops *ops = led_get_ops(dev);
>
>         if (!ops->set_state)
>                 return -ENOSYS;
>
> -       return ops->set_state(dev, state);
> +#ifdef CONFIG_LED_SW_BLINK

if (IS_ENABLED(CONFIG_LED_SW_BLINK))

> +       if (!led_driver_supports_hw_blinking(dev)) {
> +               struct cyclic_info *cyclic;
> +               struct led_uc_plat *uc_plat = dev_get_uclass_plat(dev);
> +
> +               cyclic = led_sw_find_blinking_led(uc_plat->label);
> +
> +               if (cyclic != NULL) {
> +                       if (new_state == LEDST_BLINK) {
> +                               struct sw_blink_state *cur_st;
> +
> +                               cur_st = (struct sw_blink_state *)cyclic->ctx;
> +
> +                               /*
> +                                * Next call to led_sw_blink will start blinking
> +                                */
> +                               cur_st->cur_blink_state = LED_SW_BLINK_ST_OFF;
> +                               return 0;
> +                       }
> +
> +                       /*
> +                        * Changing current blinking state to
> +                        * something else
> +                        */
> +                       led_sw_free_cyclic(cyclic);
> +               }
> +       }
> +#endif
> +
> +       return ops->set_state(dev, new_state);
>  }
>
>  enum led_state_t led_get_state(struct udevice *dev)
> @@ -69,19 +240,31 @@ enum led_state_t led_get_state(struct udevice *dev)
>         if (!ops->get_state)
>                 return -ENOSYS;
>
> +#ifdef CONFIG_LED_SW_BLINK
> +       if (!led_driver_supports_hw_blinking(dev) && led_sw_is_blinking(dev))
> +               return LEDST_BLINK;
> +#endif
> +
>         return ops->get_state(dev);
>  }
>
>  #ifdef CONFIG_LED_BLINK
> +
>  int led_set_period(struct udevice *dev, int period_ms)
>  {
>         struct led_ops *ops = led_get_ops(dev);
>
> -       if (!ops->set_period)
> +       if (!ops->set_period) {
> +#ifdef CONFIG_LED_SW_BLINK
> +               return led_sw_set_period(dev, period_ms);
> +#else
>                 return -ENOSYS;
> +#endif
> +       }

This is a bit strange...you are in effect creating a default implementation?

>
>         return ops->set_period(dev, period_ms);
>  }
> +
>  #endif
>
>  static int led_post_bind(struct udevice *dev)
> @@ -113,7 +296,6 @@ static int led_post_bind(struct udevice *dev)
>          * probe() to configure its default state during startup.
>          */
>         dev_or_flags(dev, DM_FLAG_PROBE_AFTER_BIND);
> -

Unrelated change (we use a blank line before the final return in each function)

>         return 0;
>  }
>
> --
> 2.43.0
>

Regards,
Simon

Re: [PATCH 1/2] led: Implement software led blinking

[Tom Rini]

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

On Thu, Jun 27, 2024 at 02:31:13PM +0300, Mikhail Kshevetskiy wrote:

> From: Michael Polyntsov <michael.polyntsov@iopsys.eu>
> 
> If hardware (or driver) doesn't support leds blinking, it's
> now possible to use software implementation of blinking instead.
> This relies on cyclic functions.
> 
> Signed-off-by: Michael Polyntsov <michael.polyntsov@iopsys.eu>
> Signed-off-by: Mikhail Kshevetskiy <mikhail.kshevetskiy@iopsys.eu>
> ---
>  drivers/led/Kconfig      |   9 ++
>  drivers/led/led-uclass.c | 190 ++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 195 insertions(+), 4 deletions(-)

As one of those "well, hunh" kind of moment, I'm cc'ing Christian
Marangi who also just posted patches foor this kind of functionality.

-- 
Tom

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

Re: [PATCH 1/2] led: Implement software led blinking

[Christian Marangi]

On Thu, Jun 27, 2024 at 01:36:09PM -0600, Tom Rini wrote:
> On Thu, Jun 27, 2024 at 02:31:13PM +0300, Mikhail Kshevetskiy wrote:
> 
> > From: Michael Polyntsov <michael.polyntsov@iopsys.eu>
> > 
> > If hardware (or driver) doesn't support leds blinking, it's
> > now possible to use software implementation of blinking instead.
> > This relies on cyclic functions.
> > 
> > Signed-off-by: Michael Polyntsov <michael.polyntsov@iopsys.eu>
> > Signed-off-by: Mikhail Kshevetskiy <mikhail.kshevetskiy@iopsys.eu>
> > ---
> >  drivers/led/Kconfig      |   9 ++
> >  drivers/led/led-uclass.c | 190 ++++++++++++++++++++++++++++++++++++++-
> >  2 files changed, 195 insertions(+), 4 deletions(-)
> 
> As one of those "well, hunh" kind of moment, I'm cc'ing Christian
> Marangi who also just posted patches foor this kind of functionality.
> 

Eh... Well this implementation is better. I had the idea of adding
support on the uclass level but I preferred to limit it only to GPIO.

Think this would match how it's done in upstream linux kernel so I think
mine should be ignored and this taken (for sw blink).

Should not change a thing for the series since all the bits were using
generic LED functions.

-- 
	Ansuel

Re: [PATCH 1/2] led: Implement software led blinking

[Mikhail Kshevetskiy]

On 27.06.2024 22:05, Simon Glass wrote:
> Hi Mikhail,
>
> On Thu, 27 Jun 2024 at 12:31, Mikhail Kshevetskiy
> <mikhail.kshevetskiy@iopsys.eu> wrote:
>> From: Michael Polyntsov <michael.polyntsov@iopsys.eu>
>>
>> If hardware (or driver) doesn't support leds blinking, it's
>> now possible to use software implementation of blinking instead.
>> This relies on cyclic functions.
>>
>> Signed-off-by: Michael Polyntsov <michael.polyntsov@iopsys.eu>
>> Signed-off-by: Mikhail Kshevetskiy <mikhail.kshevetskiy@iopsys.eu>
>> ---
>>  drivers/led/Kconfig      |   9 ++
>>  drivers/led/led-uclass.c | 190 ++++++++++++++++++++++++++++++++++++++-
>>  2 files changed, 195 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/led/Kconfig b/drivers/led/Kconfig
>> index 9837960198d..4330f014239 100644
>> --- a/drivers/led/Kconfig
>> +++ b/drivers/led/Kconfig
>> @@ -73,6 +73,15 @@ config LED_BLINK
>>           This option enables support for this which adds slightly to the
>>           code size.
>>
>> +config LED_SW_BLINK
>> +       bool "Support software LED blinking"
>> +       depends on LED_BLINK
>> +       select CYCLIC
>> +       help
>> +         Turns on led blinking implemented in the software, useful when
>> +         the hardware doesn't support led blinking. Does nothing if
>> +         driver supports blinking.
> Can you talk about the blinking p[eriod / API?

Could you clarify what do you mean?

>> +
>>  config SPL_LED
>>         bool "Enable LED support in SPL"
>>         depends on SPL_DM
>> diff --git a/drivers/led/led-uclass.c b/drivers/led/led-uclass.c
>> index a4be56fc258..b35964f2e99 100644
>> --- a/drivers/led/led-uclass.c
>> +++ b/drivers/led/led-uclass.c
>> @@ -15,6 +15,10 @@
>>  #include <dm/root.h>
>>  #include <dm/uclass-internal.h>
>>
>> +#ifdef CONFIG_LED_SW_BLINK
>> +#include <malloc.h>
>> +#endif
> You should not need to #ifdef include files
will fix
>> +
>>  int led_bind_generic(struct udevice *parent, const char *driver_name)
>>  {
>>         struct udevice *dev;
>> @@ -41,6 +45,7 @@ int led_get_by_label(const char *label, struct udevice **devp)
>>         ret = uclass_get(UCLASS_LED, &uc);
>>         if (ret)
>>                 return ret;
>> +
>>         uclass_foreach_dev(dev, uc) {
>>                 struct led_uc_plat *uc_plat = dev_get_uclass_plat(dev);
>>
>> @@ -52,14 +57,180 @@ int led_get_by_label(const char *label, struct udevice **devp)
>>         return -ENODEV;
>>  }
>>
>> -int led_set_state(struct udevice *dev, enum led_state_t state)
>> +#ifdef CONFIG_LED_SW_BLINK
>> +
>> +enum led_sw_blink_state_t {
>> +       LED_SW_BLINK_ST_OFF = 0,
>> +       LED_SW_BLINK_ST_ON = 1,
>> +       LED_SW_BLINK_ST_NONE = 2,
>> +};
>> +
>> +struct sw_blink_state {
>> +       struct udevice *dev;
>> +       enum led_sw_blink_state_t cur_blink_state;
>> +};
>> +
>> +static bool led_driver_supports_hw_blinking(const struct udevice *dev)
>> +{
>> +       struct led_ops *ops = led_get_ops(dev);
>> +
>> +       /*
>> +        * We assume that if driver supports set_period, then it correctly
>> +        * handles all other requests, for example, that
>> +        * led_set_state(LEDST_BLINK) works correctly.
>> +        */
>> +       return ops->set_period != NULL;
>> +}
>> +
>> +static const char *led_sw_label_to_cyclic_func_name(const char *label)
>> +{
>> +#define MAX_NAME_LEN 50
>> +       static char cyclic_func_name[MAX_NAME_LEN] = {0};
>> +
>> +       snprintf(cyclic_func_name, MAX_NAME_LEN, "sw_blink_%s", label);
>> +       return cyclic_func_name;
>> +#undef MAX_NAME_LEN
>> +}
>> +
>> +static struct cyclic_info *led_sw_find_blinking_led(const char *label)
>> +{
>> +       struct cyclic_info *cyclic;
>> +       const char *cyclic_name;
>> +
>> +       cyclic_name = led_sw_label_to_cyclic_func_name(label);
>> +
>> +       hlist_for_each_entry(cyclic, cyclic_get_list(), list) {
>> +               if (strcmp(cyclic->name, cyclic_name) == 0)
>> +                       return cyclic;
>> +       }
>> +
>> +       return NULL;
>> +}
>> +
>> +static bool led_sw_is_blinking(struct udevice *dev)
>> +{
>> +       struct led_uc_plat *uc_plat = dev_get_uclass_plat(dev);
>> +       struct cyclic_info *cyclic = led_sw_find_blinking_led(uc_plat->label);
>> +
>> +       if (cyclic != NULL) {
> if (cyclic) {

will fix

>
>> +               struct sw_blink_state *state;
>> +
>> +               state = (struct sw_blink_state *)cyclic->ctx;
>> +               return state->cur_blink_state != LED_SW_BLINK_ST_NONE;
>> +       }
>> +
>> +       return false;
>> +}
>> +
>> +static void led_sw_blink(void *void_state)
>> +{
>> +       struct sw_blink_state *state = (struct sw_blink_state *)void_state;
> You should not need that cast
will fix
>
>> +       struct udevice *dev = state->dev;
>> +       struct led_ops *ops = led_get_ops(dev);
>> +
>> +       switch (state->cur_blink_state) {
>> +       case LED_SW_BLINK_ST_OFF:
>> +               state->cur_blink_state = LED_SW_BLINK_ST_ON;
>> +               ops->set_state(dev, LEDST_ON);
>> +               break;
>> +       case LED_SW_BLINK_ST_ON:
>> +               state->cur_blink_state = LED_SW_BLINK_ST_OFF;
>> +               ops->set_state(dev, LEDST_OFF);
>> +               break;
>> +       case LED_SW_BLINK_ST_NONE:
>> +               /*
>> +                * led_set_period has been called, but
>> +                * led_set_state(LDST_BLINK) has not yet,
>> +                * so doing nothing
>> +                */
>> +               break;
>> +       }
>> +}
>> +
>> +static void led_sw_free_cyclic(struct cyclic_info *cyclic)
>> +{
>> +       free(cyclic->ctx);
>> +       cyclic_unregister(cyclic);
>> +}
>> +
>> +static int led_sw_set_period(struct udevice *dev, int period_ms)
>> +{
>> +       struct cyclic_info *cyclic;
>> +       struct sw_blink_state *state;
>> +       struct led_uc_plat *uc_plat = dev_get_uclass_plat(dev);
>> +       const char *cyclic_func_name;
>> +
>> +       state = malloc(sizeof(struct sw_blink_state));
>> +       if (state == NULL) {
>> +               printf("Allocating memory for sw_blink_state for %s failed\n",
>> +                      uc_plat->label);
>> +               return -ENOMEM;
>> +       }
> I think it would be better to put struct sw_blink_state fields inside
> uc_plat. After all, it is pretty tiny. We try not to malloc() data for
> devices.
>
>> +
>> +       state->cur_blink_state = LED_SW_BLINK_ST_NONE;
>> +       state->dev = dev;
>> +
>> +       /*
>> +        * Make sure that there is no cyclic function already
>> +        * registered for this label
>> +        */
>> +       cyclic = led_sw_find_blinking_led(uc_plat->label);
>> +       if (cyclic != NULL)
> if (cyclic)
>
> please fix globally
>
>> +               led_sw_free_cyclic(cyclic);
>> +
>> +       cyclic_func_name = led_sw_label_to_cyclic_func_name(uc_plat->label);
>> +
>> +       cyclic = cyclic_register(led_sw_blink, period_ms * 1000,
>> +                                cyclic_func_name, (void *)state);
>> +       if (cyclic == NULL) {
>> +               printf("Registering of blinking function for %s failed\n",
>> +                      uc_plat->label);
> log_err()
>
>> +               free(state);
>> +               return -ENOMEM;
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>> +#endif
>> +
>> +int led_set_state(struct udevice *dev, enum led_state_t new_state)
>>  {
>>         struct led_ops *ops = led_get_ops(dev);
>>
>>         if (!ops->set_state)
>>                 return -ENOSYS;
>>
>> -       return ops->set_state(dev, state);
>> +#ifdef CONFIG_LED_SW_BLINK
> if (IS_ENABLED(CONFIG_LED_SW_BLINK))
>
>> +       if (!led_driver_supports_hw_blinking(dev)) {
>> +               struct cyclic_info *cyclic;
>> +               struct led_uc_plat *uc_plat = dev_get_uclass_plat(dev);
>> +
>> +               cyclic = led_sw_find_blinking_led(uc_plat->label);
>> +
>> +               if (cyclic != NULL) {
>> +                       if (new_state == LEDST_BLINK) {
>> +                               struct sw_blink_state *cur_st;
>> +
>> +                               cur_st = (struct sw_blink_state *)cyclic->ctx;
>> +
>> +                               /*
>> +                                * Next call to led_sw_blink will start blinking
>> +                                */
>> +                               cur_st->cur_blink_state = LED_SW_BLINK_ST_OFF;
>> +                               return 0;
>> +                       }
>> +
>> +                       /*
>> +                        * Changing current blinking state to
>> +                        * something else
>> +                        */
>> +                       led_sw_free_cyclic(cyclic);
>> +               }
>> +       }
>> +#endif
>> +
>> +       return ops->set_state(dev, new_state);
>>  }
>>
>>  enum led_state_t led_get_state(struct udevice *dev)
>> @@ -69,19 +240,31 @@ enum led_state_t led_get_state(struct udevice *dev)
>>         if (!ops->get_state)
>>                 return -ENOSYS;
>>
>> +#ifdef CONFIG_LED_SW_BLINK
>> +       if (!led_driver_supports_hw_blinking(dev) && led_sw_is_blinking(dev))
>> +               return LEDST_BLINK;
>> +#endif
>> +
>>         return ops->get_state(dev);
>>  }
>>
>>  #ifdef CONFIG_LED_BLINK
>> +
>>  int led_set_period(struct udevice *dev, int period_ms)
>>  {
>>         struct led_ops *ops = led_get_ops(dev);
>>
>> -       if (!ops->set_period)
>> +       if (!ops->set_period) {
>> +#ifdef CONFIG_LED_SW_BLINK
>> +               return led_sw_set_period(dev, period_ms);
>> +#else
>>                 return -ENOSYS;
>> +#endif
>> +       }
> This is a bit strange...you are in effect creating a default implementation?
>
>>         return ops->set_period(dev, period_ms);
>>  }
>> +
>>  #endif
>>
>>  static int led_post_bind(struct udevice *dev)
>> @@ -113,7 +296,6 @@ static int led_post_bind(struct udevice *dev)
>>          * probe() to configure its default state during startup.
>>          */
>>         dev_or_flags(dev, DM_FLAG_PROBE_AFTER_BIND);
>> -
> Unrelated change (we use a blank line before the final return in each function)
>
>>         return 0;
>>  }
>>
>> --
>> 2.43.0
>>
> Regards,
> Simon

Re: [PATCH 1/2] led: Implement software led blinking

[Simon Glass]

Hi Mikhail,

On Tue, 2 Jul 2024 at 12:54, Mikhail Kshevetskiy
<mikhail.kshevetskiy@genexis.eu> wrote:
>
>
> On 27.06.2024 22:05, Simon Glass wrote:
> > Hi Mikhail,
> >
> > On Thu, 27 Jun 2024 at 12:31, Mikhail Kshevetskiy
> > <mikhail.kshevetskiy@iopsys.eu> wrote:
> >> From: Michael Polyntsov <michael.polyntsov@iopsys.eu>
> >>
> >> If hardware (or driver) doesn't support leds blinking, it's
> >> now possible to use software implementation of blinking instead.
> >> This relies on cyclic functions.
> >>
> >> Signed-off-by: Michael Polyntsov <michael.polyntsov@iopsys.eu>
> >> Signed-off-by: Mikhail Kshevetskiy <mikhail.kshevetskiy@iopsys.eu>
> >> ---
> >>  drivers/led/Kconfig      |   9 ++
> >>  drivers/led/led-uclass.c | 190 ++++++++++++++++++++++++++++++++++++++-
> >>  2 files changed, 195 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/led/Kconfig b/drivers/led/Kconfig
> >> index 9837960198d..4330f014239 100644
> >> --- a/drivers/led/Kconfig
> >> +++ b/drivers/led/Kconfig
> >> @@ -73,6 +73,15 @@ config LED_BLINK
> >>           This option enables support for this which adds slightly to the
> >>           code size.
> >>
> >> +config LED_SW_BLINK
> >> +       bool "Support software LED blinking"
> >> +       depends on LED_BLINK
> >> +       select CYCLIC
> >> +       help
> >> +         Turns on led blinking implemented in the software, useful when
> >> +         the hardware doesn't support led blinking. Does nothing if
> >> +         driver supports blinking.
> > Can you talk about the blinking p[eriod / API?
>
> Could you clarify what do you mean?

I mean can you explain in this help

[..]

Regards,
Simon

led blinking patches

[Mikhail Kshevetskiy]

Hi Simon and all,

This patch series implements:
 * software led blinking (via cyclic functions)
 * add support of dts property to specify blinking of the led

v2 changes:
 * Drop sw_blink_state structure, move its necessary fields to
   led_uc_plat structure.
 * Add cyclic_info pointer to led_uc_plat structure. This
   simplify code a lot.
 * Remove cyclic function search logic. Not needed anymore.
 * Fix blinking period. It was twice large.
 * Other cleanups.

Thanks,
Mikhail Kshevetskiy

[PATCH 1/2] led: Implement software led blinking

[Mikhail Kshevetskiy]

From: Michael Polyntsov <michael.polyntsov@iopsys.eu>

If hardware (or driver) doesn't support leds blinking, it's
now possible to use software implementation of blinking instead.
This relies on cyclic functions.

v2 changes:
 * Drop sw_blink_state structure, move its necessary fields to
   led_uc_plat structure.
 * Add cyclic_info pointer to led_uc_plat structure. This
   simplify code a lot.
 * Remove cyclic function search logic. Not needed anymore.
 * Fix blinking period. It was twice large.
 * Other cleanups.

Signed-off-by: Michael Polyntsov <michael.polyntsov@iopsys.eu>
Signed-off-by: Mikhail Kshevetskiy <mikhail.kshevetskiy@iopsys.eu>
---
 drivers/led/Kconfig      |  14 ++++++
 drivers/led/led-uclass.c | 102 +++++++++++++++++++++++++++++++++++++++
 include/led.h            |  12 +++++
 3 files changed, 128 insertions(+)

diff --git a/drivers/led/Kconfig b/drivers/led/Kconfig
index 9837960198d..1afb081df11 100644
--- a/drivers/led/Kconfig
+++ b/drivers/led/Kconfig
@@ -73,6 +73,20 @@ config LED_BLINK
 	  This option enables support for this which adds slightly to the
 	  code size.
 
+config LED_SW_BLINK
+	bool "Support software LED blinking"
+	depends on LED_BLINK
+	select CYCLIC
+	help
+	  Turns on led blinking implemented in the software, useful when
+	  the hardware doesn't support led blinking. Half of the period
+	  led will be ON and the rest time it will be OFF. Standard
+	  led commands can be used to configure blinking. Does nothing
+	  if driver supports blinking.
+	  WARNING: Blinking may be inaccurate during execution of time
+	  consuming commands (ex. flash reading). Also it will completely
+	  stops during OS booting.
+
 config SPL_LED
 	bool "Enable LED support in SPL"
 	depends on SPL_DM
diff --git a/drivers/led/led-uclass.c b/drivers/led/led-uclass.c
index a4be56fc258..d021c3bbf20 100644
--- a/drivers/led/led-uclass.c
+++ b/drivers/led/led-uclass.c
@@ -52,6 +52,94 @@ int led_get_by_label(const char *label, struct udevice **devp)
 	return -ENODEV;
 }
 
+#ifdef CONFIG_LED_SW_BLINK
+static void led_sw_blink(void *data)
+{
+	struct udevice *dev = data;
+	struct led_uc_plat *uc_plat = dev_get_uclass_plat(dev);
+	struct led_ops *ops = led_get_ops(dev);
+
+	switch (uc_plat->sw_blink_state) {
+	case LED_SW_BLINK_ST_OFF:
+		uc_plat->sw_blink_state = LED_SW_BLINK_ST_ON;
+		ops->set_state(dev, LEDST_ON);
+		break;
+	case LED_SW_BLINK_ST_ON:
+		uc_plat->sw_blink_state = LED_SW_BLINK_ST_OFF;
+		ops->set_state(dev, LEDST_OFF);
+		break;
+	case LED_SW_BLINK_ST_NONE:
+		/*
+		 * led_set_period has been called, but
+		 * led_set_state(LDST_BLINK) has not yet,
+		 * so doing nothing
+		 */
+		break;
+	}
+}
+
+static int led_sw_set_period(struct udevice *dev, int period_ms)
+{
+	struct led_uc_plat *uc_plat = dev_get_uclass_plat(dev);
+	struct cyclic_info *cyclic = uc_plat->cyclic;
+	struct led_ops *ops = led_get_ops(dev);
+	char cyclic_name[64];
+	int half_period_us;
+
+	uc_plat->sw_blink_state = LED_SW_BLINK_ST_NONE;
+	ops->set_state(dev, LEDST_OFF);
+
+	half_period_us = period_ms * 1000 / 2;
+
+	if (cyclic) {
+		cyclic->delay_us = half_period_us;
+		cyclic->start_time_us = timer_get_us();
+	} else {
+		snprintf(cyclic_name, sizeof(cyclic_name),
+			 "led_sw_blink_%s", uc_plat->label);
+
+		cyclic = cyclic_register(led_sw_blink, half_period_us,
+					 cyclic_name, dev);
+		if (!cyclic) {
+			log_err("Registering of blinking function for %s failed\n",
+				uc_plat->label);
+			return -ENOMEM;
+		}
+
+		uc_plat->cyclic = cyclic;
+	}
+
+	return 0;
+}
+
+static bool led_sw_is_blinking(struct udevice *dev)
+{
+	struct led_uc_plat *uc_plat = dev_get_uclass_plat(dev);
+
+	return (uc_plat->sw_blink_state != LED_SW_BLINK_ST_NONE);
+}
+
+static bool led_sw_on_state_change(struct udevice *dev, enum led_state_t state)
+{
+	struct led_uc_plat *uc_plat = dev_get_uclass_plat(dev);
+
+	if (uc_plat->cyclic) {
+		if (state == LEDST_BLINK) {
+			/* start blinking on next led_sw_blink() call */
+			uc_plat->sw_blink_state = LED_SW_BLINK_ST_OFF;
+			return true;
+		}
+
+		/* stop blinking */
+		cyclic_unregister(uc_plat->cyclic);
+		uc_plat->cyclic = NULL;
+		uc_plat->sw_blink_state = LED_SW_BLINK_ST_NONE;
+	}
+
+	return false;
+}
+#endif /* CONFIG_LED_SW_BLINK */
+
 int led_set_state(struct udevice *dev, enum led_state_t state)
 {
 	struct led_ops *ops = led_get_ops(dev);
@@ -59,6 +147,11 @@ int led_set_state(struct udevice *dev, enum led_state_t state)
 	if (!ops->set_state)
 		return -ENOSYS;
 
+#ifdef CONFIG_LED_SW_BLINK
+	if (led_sw_on_state_change(dev, state))
+		return 0;
+#endif
+
 	return ops->set_state(dev, state);
 }
 
@@ -69,6 +162,11 @@ enum led_state_t led_get_state(struct udevice *dev)
 	if (!ops->get_state)
 		return -ENOSYS;
 
+#ifdef CONFIG_LED_SW_BLINK
+	if (led_sw_is_blinking(dev))
+		return LEDST_BLINK;
+#endif
+
 	return ops->get_state(dev);
 }
 
@@ -78,7 +176,11 @@ int led_set_period(struct udevice *dev, int period_ms)
 	struct led_ops *ops = led_get_ops(dev);
 
 	if (!ops->set_period)
+#ifdef CONFIG_LED_SW_BLINK
+		return led_sw_set_period(dev, period_ms);
+#else
 		return -ENOSYS;
+#endif
 
 	return ops->set_period(dev, period_ms);
 }
diff --git a/include/led.h b/include/led.h
index a6353166289..bd50fdf33ef 100644
--- a/include/led.h
+++ b/include/led.h
@@ -20,6 +20,14 @@ enum led_state_t {
 	LEDST_COUNT,
 };
 
+#ifdef CONFIG_LED_SW_BLINK
+enum led_sw_blink_state_t {
+	LED_SW_BLINK_ST_NONE = 0,
+	LED_SW_BLINK_ST_OFF = 1,
+	LED_SW_BLINK_ST_ON = 2,
+};
+#endif
+
 /**
  * struct led_uc_plat - Platform data the uclass stores about each device
  *
@@ -29,6 +37,10 @@ enum led_state_t {
 struct led_uc_plat {
 	const char *label;
 	enum led_state_t default_state;
+#ifdef CONFIG_LED_SW_BLINK
+	enum led_sw_blink_state_t sw_blink_state;
+	struct cyclic_info *cyclic;
+#endif
 };
 
 /**
-- 
2.39.2

[PATCH 2/2] led: Add dts property to specify blinking of the led

[Mikhail Kshevetskiy]

From: Michael Polyntsov <michael.polyntsov@iopsys.eu>

The standard property

    linux,default-trigger = "pattern";

used to get an effect. No blinking parameters can be set yet.

Signed-off-by: Michael Polyntsov <michael.polyntsov@iopsys.eu>
Signed-off-by: Mikhail Kshevetskiy <mikhail.kshevetskiy@iopsys.eu>
---
 drivers/led/led-uclass.c | 34 ++++++++++++++++++++++++++++++----
 1 file changed, 30 insertions(+), 4 deletions(-)

diff --git a/drivers/led/led-uclass.c b/drivers/led/led-uclass.c
index d021c3bbf20..78d1a3d152b 100644
--- a/drivers/led/led-uclass.c
+++ b/drivers/led/led-uclass.c
@@ -190,6 +190,9 @@ 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_BLINK
+	const char *trigger;
+#endif
 
 	if (!uc_plat->label)
 		uc_plat->label = dev_read_string(dev, "label");
@@ -210,6 +213,13 @@ static int led_post_bind(struct udevice *dev)
 	else
 		return 0;
 
+#ifdef CONFIG_LED_BLINK
+	trigger = dev_read_string(dev, "linux,default-trigger");
+	if (trigger && !strncmp(trigger, "pattern", 7)) {
+		uc_plat->default_state = LEDST_BLINK;
+	}
+#endif
+
 	/*
 	 * In case the LED has default-state DT property, trigger
 	 * probe() to configure its default state during startup.
@@ -222,12 +232,28 @@ static int led_post_bind(struct udevice *dev)
 static int led_post_probe(struct udevice *dev)
 {
 	struct led_uc_plat *uc_plat = dev_get_uclass_plat(dev);
+	int rc = 0;
 
-	if (uc_plat->default_state == LEDST_ON ||
-	    uc_plat->default_state == LEDST_OFF)
-		led_set_state(dev, uc_plat->default_state);
+	switch (uc_plat->default_state) {
+	case LEDST_ON:
+	case LEDST_OFF:
+		rc = led_set_state(dev, uc_plat->default_state);
+		break;
+#ifdef CONFIG_LED_BLINK
+	case LEDST_BLINK: {
+		const int default_period_ms = 1000;
 
-	return 0;
+		rc = led_set_period(dev, default_period_ms);
+		if (rc == 0)
+			rc = led_set_state(dev, uc_plat->default_state);
+		break;
+	}
+#endif
+	default:
+		break;
+	}
+
+	return rc;
 }
 
 UCLASS_DRIVER(led) = {
-- 
2.39.2

Re: [PATCH 1/2] led: Implement software led blinking

[Simon Glass]

Hi Mikhail,

On Wed, 3 Jul 2024 at 02:02, Mikhail Kshevetskiy
<mikhail.kshevetskiy@iopsys.eu> wrote:
>
> From: Michael Polyntsov <michael.polyntsov@iopsys.eu>
>
> If hardware (or driver) doesn't support leds blinking, it's
> now possible to use software implementation of blinking instead.
> This relies on cyclic functions.
>
> v2 changes:
>  * Drop sw_blink_state structure, move its necessary fields to
>    led_uc_plat structure.
>  * Add cyclic_info pointer to led_uc_plat structure. This
>    simplify code a lot.
>  * Remove cyclic function search logic. Not needed anymore.
>  * Fix blinking period. It was twice large.
>  * Other cleanups.
>
> Signed-off-by: Michael Polyntsov <michael.polyntsov@iopsys.eu>
> Signed-off-by: Mikhail Kshevetskiy <mikhail.kshevetskiy@iopsys.eu>

LGTM except for the #ifdefs...I've put an idea below.

> ---
>  drivers/led/Kconfig      |  14 ++++++
>  drivers/led/led-uclass.c | 102 +++++++++++++++++++++++++++++++++++++++
>  include/led.h            |  12 +++++
>  3 files changed, 128 insertions(+)
>
> diff --git a/drivers/led/Kconfig b/drivers/led/Kconfig
> index 9837960198d..1afb081df11 100644
> --- a/drivers/led/Kconfig
> +++ b/drivers/led/Kconfig
> @@ -73,6 +73,20 @@ config LED_BLINK
>           This option enables support for this which adds slightly to the
>           code size.
>
> +config LED_SW_BLINK
> +       bool "Support software LED blinking"
> +       depends on LED_BLINK
> +       select CYCLIC
> +       help
> +         Turns on led blinking implemented in the software, useful when
> +         the hardware doesn't support led blinking. Half of the period
> +         led will be ON and the rest time it will be OFF. Standard
> +         led commands can be used to configure blinking. Does nothing
> +         if driver supports blinking.
> +         WARNING: Blinking may be inaccurate during execution of time
> +         consuming commands (ex. flash reading). Also it will completely
> +         stops during OS booting.

Drop the word 'will'

> +
>  config SPL_LED
>         bool "Enable LED support in SPL"
>         depends on SPL_DM
> diff --git a/drivers/led/led-uclass.c b/drivers/led/led-uclass.c
> index a4be56fc258..d021c3bbf20 100644
> --- a/drivers/led/led-uclass.c
> +++ b/drivers/led/led-uclass.c
> @@ -52,6 +52,94 @@ int led_get_by_label(const char *label, struct udevice **devp)
>         return -ENODEV;
>  }
>
> +#ifdef CONFIG_LED_SW_BLINK

You can put this block into a separate file, like led_blink.c and
export the functions, since it accesses members which are behind an
#ifdef

obj-$(CONFIG_LED_SW_BLINK) += led_blink.o

> +static void led_sw_blink(void *data)
> +{
> +       struct udevice *dev = data;
> +       struct led_uc_plat *uc_plat = dev_get_uclass_plat(dev);
> +       struct led_ops *ops = led_get_ops(dev);
> +
> +       switch (uc_plat->sw_blink_state) {
> +       case LED_SW_BLINK_ST_OFF:
> +               uc_plat->sw_blink_state = LED_SW_BLINK_ST_ON;
> +               ops->set_state(dev, LEDST_ON);
> +               break;
> +       case LED_SW_BLINK_ST_ON:
> +               uc_plat->sw_blink_state = LED_SW_BLINK_ST_OFF;
> +               ops->set_state(dev, LEDST_OFF);
> +               break;
> +       case LED_SW_BLINK_ST_NONE:
> +               /*
> +                * led_set_period has been called, but
> +                * led_set_state(LDST_BLINK) has not yet,
> +                * so doing nothing
> +                */
> +               break;
> +       }
> +}
> +
> +static int led_sw_set_period(struct udevice *dev, int period_ms)
> +{
> +       struct led_uc_plat *uc_plat = dev_get_uclass_plat(dev);
> +       struct cyclic_info *cyclic = uc_plat->cyclic;
> +       struct led_ops *ops = led_get_ops(dev);
> +       char cyclic_name[64];
> +       int half_period_us;
> +
> +       uc_plat->sw_blink_state = LED_SW_BLINK_ST_NONE;
> +       ops->set_state(dev, LEDST_OFF);
> +
> +       half_period_us = period_ms * 1000 / 2;
> +
> +       if (cyclic) {
> +               cyclic->delay_us = half_period_us;
> +               cyclic->start_time_us = timer_get_us();
> +       } else {
> +               snprintf(cyclic_name, sizeof(cyclic_name),
> +                        "led_sw_blink_%s", uc_plat->label);
> +
> +               cyclic = cyclic_register(led_sw_blink, half_period_us,
> +                                        cyclic_name, dev);
> +               if (!cyclic) {
> +                       log_err("Registering of blinking function for %s failed\n",
> +                               uc_plat->label);
> +                       return -ENOMEM;
> +               }
> +
> +               uc_plat->cyclic = cyclic;
> +       }
> +
> +       return 0;
> +}
> +
> +static bool led_sw_is_blinking(struct udevice *dev)
> +{
> +       struct led_uc_plat *uc_plat = dev_get_uclass_plat(dev);
> +
> +       return (uc_plat->sw_blink_state != LED_SW_BLINK_ST_NONE);
> +}
> +
> +static bool led_sw_on_state_change(struct udevice *dev, enum led_state_t state)
> +{
> +       struct led_uc_plat *uc_plat = dev_get_uclass_plat(dev);
> +
> +       if (uc_plat->cyclic) {
> +               if (state == LEDST_BLINK) {
> +                       /* start blinking on next led_sw_blink() call */
> +                       uc_plat->sw_blink_state = LED_SW_BLINK_ST_OFF;
> +                       return true;
> +               }
> +
> +               /* stop blinking */
> +               cyclic_unregister(uc_plat->cyclic);
> +               uc_plat->cyclic = NULL;
> +               uc_plat->sw_blink_state = LED_SW_BLINK_ST_NONE;
> +       }
> +
> +       return false;
> +}
> +#endif /* CONFIG_LED_SW_BLINK */
> +
>  int led_set_state(struct udevice *dev, enum led_state_t state)
>  {
>         struct led_ops *ops = led_get_ops(dev);
> @@ -59,6 +147,11 @@ int led_set_state(struct udevice *dev, enum led_state_t state)
>         if (!ops->set_state)
>                 return -ENOSYS;
>
> +#ifdef CONFIG_LED_SW_BLINK

Then these #ifdefs should be able to change to

if (IS_ENABLED(CONFIG_LED_SW_BLINK) &&
    led_sw_on_state_change(dev, state))
...

I suppose static inlines are another way if you prefer, but as there
is only one caller it probably isn't worth it.

> +       if (led_sw_on_state_change(dev, state))
> +               return 0;
> +#endif
> +
>         return ops->set_state(dev, state);
>  }
>
> @@ -69,6 +162,11 @@ enum led_state_t led_get_state(struct udevice *dev)
>         if (!ops->get_state)
>                 return -ENOSYS;
>
> +#ifdef CONFIG_LED_SW_BLINK
> +       if (led_sw_is_blinking(dev))
> +               return LEDST_BLINK;
> +#endif
> +
>         return ops->get_state(dev);
>  }
>
> @@ -78,7 +176,11 @@ int led_set_period(struct udevice *dev, int period_ms)
>         struct led_ops *ops = led_get_ops(dev);
>
>         if (!ops->set_period)
> +#ifdef CONFIG_LED_SW_BLINK
> +               return led_sw_set_period(dev, period_ms);
> +#else
>                 return -ENOSYS;
> +#endif
>
>         return ops->set_period(dev, period_ms);
>  }
> diff --git a/include/led.h b/include/led.h
> index a6353166289..bd50fdf33ef 100644
> --- a/include/led.h
> +++ b/include/led.h
> @@ -20,6 +20,14 @@ enum led_state_t {
>         LEDST_COUNT,
>  };
>
> +#ifdef CONFIG_LED_SW_BLINK

can drop this #ifdef

> +enum led_sw_blink_state_t {
> +       LED_SW_BLINK_ST_NONE = 0,
> +       LED_SW_BLINK_ST_OFF = 1,
> +       LED_SW_BLINK_ST_ON = 2,
> +};
> +#endif
> +
>  /**
>   * struct led_uc_plat - Platform data the uclass stores about each device
>   *
> @@ -29,6 +37,10 @@ enum led_state_t {
>  struct led_uc_plat {
>         const char *label;
>         enum led_state_t default_state;
> +#ifdef CONFIG_LED_SW_BLINK
> +       enum led_sw_blink_state_t sw_blink_state;
> +       struct cyclic_info *cyclic;
> +#endif
>  };
>

Need to add function prototypes here for flashing API.

>  /**
> --
> 2.39.2
>

Regards,
Simon

Re: [PATCH 2/2] led: Add dts property to specify blinking of the led

[Simon Glass]

Hi Mikhail,

On Wed, 3 Jul 2024 at 02:02, Mikhail Kshevetskiy
<mikhail.kshevetskiy@iopsys.eu> wrote:
>
> From: Michael Polyntsov <michael.polyntsov@iopsys.eu>
>
> The standard property
>
>     linux,default-trigger = "pattern";
>
> used to get an effect. No blinking parameters can be set yet.
>
> Signed-off-by: Michael Polyntsov <michael.polyntsov@iopsys.eu>
> Signed-off-by: Mikhail Kshevetskiy <mikhail.kshevetskiy@iopsys.eu>
> ---
>  drivers/led/led-uclass.c | 34 ++++++++++++++++++++++++++++++----
>  1 file changed, 30 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/led/led-uclass.c b/drivers/led/led-uclass.c
> index d021c3bbf20..78d1a3d152b 100644
> --- a/drivers/led/led-uclass.c
> +++ b/drivers/led/led-uclass.c
> @@ -190,6 +190,9 @@ 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_BLINK
> +       const char *trigger;

It looks like this is not used, so you can drop it and use a local
variable here?

> +#endif
>
>         if (!uc_plat->label)
>                 uc_plat->label = dev_read_string(dev, "label");
> @@ -210,6 +213,13 @@ static int led_post_bind(struct udevice *dev)
>         else
>                 return 0;
>
> +#ifdef CONFIG_LED_BLINK

if (IS_ENABLED(CONFIG_LED_BLINK)

> +       trigger = dev_read_string(dev, "linux,default-trigger");
> +       if (trigger && !strncmp(trigger, "pattern", 7)) {
> +               uc_plat->default_state = LEDST_BLINK;
> +       }

No {} around single lines. You can use patman to send patches and it
will check this for you (I hope!).

> +#endif
> +
>         /*
>          * In case the LED has default-state DT property, trigger
>          * probe() to configure its default state during startup.
> @@ -222,12 +232,28 @@ static int led_post_bind(struct udevice *dev)
>  static int led_post_probe(struct udevice *dev)
>  {
>         struct led_uc_plat *uc_plat = dev_get_uclass_plat(dev);
> +       int rc = 0;
>
> -       if (uc_plat->default_state == LEDST_ON ||
> -           uc_plat->default_state == LEDST_OFF)
> -               led_set_state(dev, uc_plat->default_state);
> +       switch (uc_plat->default_state) {
> +       case LEDST_ON:
> +       case LEDST_OFF:
> +               rc = led_set_state(dev, uc_plat->default_state);
> +               break;
> +#ifdef CONFIG_LED_BLINK
> +       case LEDST_BLINK: {

Here again you can use if IS_ENABLED(CONFIG_LED_BLINK) inside the case
which should produce very similar code size, hopefully the same.

> +               const int default_period_ms = 1000;
>
> -       return 0;
> +               rc = led_set_period(dev, default_period_ms);
> +               if (rc == 0)
> +                       rc = led_set_state(dev, uc_plat->default_state);
> +               break;
> +       }
> +#endif
> +       default:
> +               break;
> +       }
> +
> +       return rc;
>  }
>
>  UCLASS_DRIVER(led) = {
> --
> 2.39.2
>

Regards,
Simon

Re: [PATCH 1/2] led: Implement software led blinking

[Rasmus Villemoes]

Mikhail Kshevetskiy <mikhail.kshevetskiy@iopsys.eu> writes:

> +
> +static int led_sw_set_period(struct udevice *dev, int period_ms)
> +{
> +	struct led_uc_plat *uc_plat = dev_get_uclass_plat(dev);
> +	struct cyclic_info *cyclic = uc_plat->cyclic;
> +	struct led_ops *ops = led_get_ops(dev);
> +	char cyclic_name[64];
> +	int half_period_us;
> +
> +	uc_plat->sw_blink_state = LED_SW_BLINK_ST_NONE;
> +	ops->set_state(dev, LEDST_OFF);
> +
> +	half_period_us = period_ms * 1000 / 2;
> +
> +	if (cyclic) {
> +		cyclic->delay_us = half_period_us;
> +		cyclic->start_time_us = timer_get_us();
> +	} else {
> +		snprintf(cyclic_name, sizeof(cyclic_name),
> +			 "led_sw_blink_%s", uc_plat->label);
> +
> +		cyclic = cyclic_register(led_sw_blink, half_period_us,
> +					 cyclic_name, dev);
> +		if (!cyclic) {
> +			log_err("Registering of blinking function for %s failed\n",
> +				uc_plat->label);
> +			return -ENOMEM;
> +		}
> +
> +		uc_plat->cyclic = cyclic;
> +	}

You need to be aware of the API change that is by now in master, see
https://lore.kernel.org/u-boot/20240521084652.1726460-1-rasmus.villemoes@prevas.dk/
and in particular commits 3a11eada38e and 008c4b3c3115. The latter
you'll find soon enough because this won't build.

The former is a bit more subtle and would silently break here (as
passing an auto array is no longer allowed) - consider whether you
really need the led_sw_blink_ to be part of the name, or if
uc_plat->label itself isn't descriptive enough.

Rasmus

Re: [PATCH 2/2] led: Add dts property to specify blinking of the led

[Mikhail Kshevetskiy]

On 7/3/24 12:08, Simon Glass wrote:
> Hi Mikhail,
>
> On Wed, 3 Jul 2024 at 02:02, Mikhail Kshevetskiy
> <mikhail.kshevetskiy@iopsys.eu> wrote:
>> From: Michael Polyntsov <michael.polyntsov@iopsys.eu>
>>
>> The standard property
>>
>>     linux,default-trigger = "pattern";
>>
>> used to get an effect. No blinking parameters can be set yet.
>>
>> Signed-off-by: Michael Polyntsov <michael.polyntsov@iopsys.eu>
>> Signed-off-by: Mikhail Kshevetskiy <mikhail.kshevetskiy@iopsys.eu>
>> ---
>>  drivers/led/led-uclass.c | 34 ++++++++++++++++++++++++++++++----
>>  1 file changed, 30 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/led/led-uclass.c b/drivers/led/led-uclass.c
>> index d021c3bbf20..78d1a3d152b 100644
>> --- a/drivers/led/led-uclass.c
>> +++ b/drivers/led/led-uclass.c
>> @@ -190,6 +190,9 @@ 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_BLINK
>> +       const char *trigger;
> It looks like this is not used, so you can drop it and use a local
> variable here?

Unfortunately no, see below.

>
>> +#endif
>>
>>         if (!uc_plat->label)
>>                 uc_plat->label = dev_read_string(dev, "label");
>> @@ -210,6 +213,13 @@ static int led_post_bind(struct udevice *dev)
>>         else
>>                 return 0;
>>
>> +#ifdef CONFIG_LED_BLINK
> if (IS_ENABLED(CONFIG_LED_BLINK)

This is not so easy. The definition of LEDST_BLINK depends on
CONFIG_LED_BLINK, thus this code will not compile if  CONFIG_LED_BLINK
is not defined. We can define LEDST_BLINK unconditionally, but it will
cause an issue in the cmd/led.c.

>> +       trigger = dev_read_string(dev, "linux,default-trigger");
>> +       if (trigger && !strncmp(trigger, "pattern", 7)) {
>> +               uc_plat->default_state = LEDST_BLINK;
>> +       }
> No {} around single lines. You can use patman to send patches and it
> will check this for you (I hope!).
>
>> +#endif
>> +
>>         /*
>>          * In case the LED has default-state DT property, trigger
>>          * probe() to configure its default state during startup.
>> @@ -222,12 +232,28 @@ static int led_post_bind(struct udevice *dev)
>>  static int led_post_probe(struct udevice *dev)
>>  {
>>         struct led_uc_plat *uc_plat = dev_get_uclass_plat(dev);
>> +       int rc = 0;
>>
>> -       if (uc_plat->default_state == LEDST_ON ||
>> -           uc_plat->default_state == LEDST_OFF)
>> -               led_set_state(dev, uc_plat->default_state);
>> +       switch (uc_plat->default_state) {
>> +       case LEDST_ON:
>> +       case LEDST_OFF:
>> +               rc = led_set_state(dev, uc_plat->default_state);
>> +               break;
>> +#ifdef CONFIG_LED_BLINK
>> +       case LEDST_BLINK: {
> Here again you can use if IS_ENABLED(CONFIG_LED_BLINK) inside the case
> which should produce very similar code size, hopefully the same.
Do you suggest an if inside the handling of "default:" case?
Also see above notes about dependency of LEDST_BLINK on CONFIG_LED_BLINK.
>> +               const int default_period_ms = 1000;
>>
>> -       return 0;
>> +               rc = led_set_period(dev, default_period_ms);
>> +               if (rc == 0)
>> +                       rc = led_set_state(dev, uc_plat->default_state);
>> +               break;
>> +       }
>> +#endif
>> +       default:
>> +               break;
>> +       }
>> +
>> +       return rc;
>>  }
>>
>>  UCLASS_DRIVER(led) = {
>> --
>> 2.39.2
>>
> Regards,
> Simon

[PATCH 1/2] led: Implement software led blinking

[Mikhail Kshevetskiy]

From: Michael Polyntsov <michael.polyntsov@iopsys.eu>

If hardware (or driver) doesn't support leds blinking, it's
now possible to use software implementation of blinking instead.
This relies on cyclic functions.

v2 changes:
 * Drop sw_blink_state structure, move its necessary fields to
   led_uc_plat structure.
 * Add cyclic_info pointer to led_uc_plat structure. This
   simplify code a lot.
 * Remove cyclic function search logic. Not needed anymore.
 * Fix blinking period. It was twice large.
 * Other cleanups.

v3 changes:
 * Adapt code to recent cyclic function changes
 * Move software blinking functions to separate file
 * Other small changes

Signed-off-by: Michael Polyntsov <michael.polyntsov@iopsys.eu>
Signed-off-by: Mikhail Kshevetskiy <mikhail.kshevetskiy@iopsys.eu>
---
 drivers/led/Kconfig        |  14 +++++
 drivers/led/Makefile       |   1 +
 drivers/led/led-uclass.c   |  16 +++++-
 drivers/led/led_sw_blink.c | 106 +++++++++++++++++++++++++++++++++++++
 include/led.h              |  17 ++++++
 5 files changed, 152 insertions(+), 2 deletions(-)
 create mode 100644 drivers/led/led_sw_blink.c

diff --git a/drivers/led/Kconfig b/drivers/led/Kconfig
index 9837960198d..dc9d4c8a757 100644
--- a/drivers/led/Kconfig
+++ b/drivers/led/Kconfig
@@ -73,6 +73,20 @@ config LED_BLINK
 	  This option enables support for this which adds slightly to the
 	  code size.
 
+config LED_SW_BLINK
+	bool "Support software LED blinking"
+	depends on LED_BLINK
+	select CYCLIC
+	help
+	  Turns on led blinking implemented in the software, useful when
+	  the hardware doesn't support led blinking. Half of the period
+	  led will be ON and the rest time it will be OFF. Standard
+	  led commands can be used to configure blinking. Does nothing
+	  if driver supports blinking.
+	  WARNING: Blinking may be inaccurate during execution of time
+	  consuming commands (ex. flash reading). Also it completely
+	  stops during OS booting.
+
 config SPL_LED
 	bool "Enable LED support in SPL"
 	depends on SPL_DM
diff --git a/drivers/led/Makefile b/drivers/led/Makefile
index 2bcb8589087..e27aa488482 100644
--- a/drivers/led/Makefile
+++ b/drivers/led/Makefile
@@ -4,6 +4,7 @@
 # Written by Simon Glass <sjg@chromium.org>
 
 obj-y += led-uclass.o
+obj-$(CONFIG_LED_SW_BLINK) += led_sw_blink.o
 obj-$(CONFIG_LED_BCM6328) += led_bcm6328.o
 obj-$(CONFIG_LED_BCM6358) += led_bcm6358.o
 obj-$(CONFIG_LED_BCM6753) += led_bcm6753.o
diff --git a/drivers/led/led-uclass.c b/drivers/led/led-uclass.c
index f37bf6a1550..37dc99cecdc 100644
--- a/drivers/led/led-uclass.c
+++ b/drivers/led/led-uclass.c
@@ -58,6 +58,10 @@ int led_set_state(struct udevice *dev, enum led_state_t state)
 	if (!ops->set_state)
 		return -ENOSYS;
 
+	if (IS_ENABLED(CONFIG_LED_SW_BLINK) &&
+	    led_sw_on_state_change(dev, state))
+		return 0;
+
 	return ops->set_state(dev, state);
 }
 
@@ -68,6 +72,10 @@ enum led_state_t led_get_state(struct udevice *dev)
 	if (!ops->get_state)
 		return -ENOSYS;
 
+	if (IS_ENABLED(CONFIG_LED_SW_BLINK) &&
+	    led_sw_is_blinking(dev))
+		return LEDST_BLINK;
+
 	return ops->get_state(dev);
 }
 
@@ -76,8 +84,12 @@ int led_set_period(struct udevice *dev, int period_ms)
 {
 	struct led_ops *ops = led_get_ops(dev);
 
-	if (!ops->set_period)
-		return -ENOSYS;
+	if (!ops->set_period) {
+		if (IS_ENABLED(CONFIG_LED_SW_BLINK))
+			return led_sw_set_period(dev, period_ms);
+		else
+			return -ENOSYS;
+	}
 
 	return ops->set_period(dev, period_ms);
 }
diff --git a/drivers/led/led_sw_blink.c b/drivers/led/led_sw_blink.c
new file mode 100644
index 00000000000..ab56111a60b
--- /dev/null
+++ b/drivers/led/led_sw_blink.c
@@ -0,0 +1,106 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Software blinking helpers
+ * Copyright (C) 2024 IOPSYS Software Solutions AB
+ * Author: Mikhail Kshevetskiy <mikhail.kshevetskiy@iopsys.eu>
+ */
+
+#include <dm.h>
+#include <led.h>
+#include <time.h>
+#include <stdlib.h>
+
+static void led_sw_blink(struct cyclic_info *c)
+{
+	struct led_uc_plat *uc_plat;
+	struct udevice *dev;
+	struct led_ops *ops;
+
+	uc_plat = container_of(c, struct led_uc_plat, cyclic);
+	dev = uc_plat->dev;
+	ops = led_get_ops(dev);
+
+	switch (uc_plat->sw_blink_state) {
+	case LED_SW_BLINK_ST_OFF:
+		uc_plat->sw_blink_state = LED_SW_BLINK_ST_ON;
+		ops->set_state(dev, LEDST_ON);
+		break;
+	case LED_SW_BLINK_ST_ON:
+		uc_plat->sw_blink_state = LED_SW_BLINK_ST_OFF;
+		ops->set_state(dev, LEDST_OFF);
+		break;
+	case LED_SW_BLINK_ST_NOT_READY:
+		/*
+		 * led_set_period has been called, but
+		 * led_set_state(LDST_BLINK) has not yet,
+		 * so doing nothing
+		 */
+		break;
+	default:
+		break;
+	}
+}
+
+int led_sw_set_period(struct udevice *dev, int period_ms)
+{
+	struct led_uc_plat *uc_plat = dev_get_uclass_plat(dev);
+	struct cyclic_info *cyclic = &uc_plat->cyclic;
+	struct led_ops *ops = led_get_ops(dev);
+	int half_period_us;
+	char *name;
+	int len;
+
+	half_period_us = period_ms * 1000 / 2;
+
+	name = (char *)cyclic->name;
+	if (name == NULL) {
+		len = snprintf(NULL, 0, "led_sw_blink_%s", uc_plat->label);
+		if (len <= 0)
+			return -ENOMEM;
+
+		name = malloc(len + 1);
+		if (!name)
+			return -ENOMEM;
+
+		snprintf(name, len + 1, "led_sw_blink_%s", uc_plat->label);
+	}
+
+	if (uc_plat->sw_blink_state == LED_SW_BLINK_ST_DISABLED) {
+		uc_plat->dev = dev;
+		cyclic_register(cyclic, led_sw_blink, half_period_us, name);
+	} else {
+		cyclic->delay_us = half_period_us;
+		cyclic->start_time_us = timer_get_us();
+	}
+
+	uc_plat->sw_blink_state = LED_SW_BLINK_ST_NOT_READY;
+	ops->set_state(dev, LEDST_OFF);
+
+	return 0;
+}
+
+bool led_sw_is_blinking(struct udevice *dev)
+{
+	struct led_uc_plat *uc_plat = dev_get_uclass_plat(dev);
+
+	return (uc_plat->sw_blink_state > LED_SW_BLINK_ST_NOT_READY);
+}
+
+bool led_sw_on_state_change(struct udevice *dev, enum led_state_t state)
+{
+	struct led_uc_plat *uc_plat = dev_get_uclass_plat(dev);
+
+	if (uc_plat->sw_blink_state != LED_SW_BLINK_ST_DISABLED) {
+		if (state == LEDST_BLINK) {
+			/* start blinking on next led_sw_blink() call */
+			uc_plat->sw_blink_state = LED_SW_BLINK_ST_OFF;
+			return true;
+		}
+
+		/* stop blinking */
+		cyclic_unregister(&uc_plat->cyclic);
+		uc_plat->sw_blink_state = LED_SW_BLINK_ST_DISABLED;
+	}
+
+	return false;
+}
diff --git a/include/led.h b/include/led.h
index a6353166289..26955269d3e 100644
--- a/include/led.h
+++ b/include/led.h
@@ -20,6 +20,13 @@ enum led_state_t {
 	LEDST_COUNT,
 };
 
+enum led_sw_blink_state_t {
+	LED_SW_BLINK_ST_DISABLED,
+	LED_SW_BLINK_ST_NOT_READY,
+	LED_SW_BLINK_ST_OFF,
+	LED_SW_BLINK_ST_ON,
+};
+
 /**
  * struct led_uc_plat - Platform data the uclass stores about each device
  *
@@ -29,6 +36,11 @@ enum led_state_t {
 struct led_uc_plat {
 	const char *label;
 	enum led_state_t default_state;
+#ifdef CONFIG_LED_SW_BLINK
+	struct udevice *dev;
+	struct cyclic_info cyclic;
+	enum led_sw_blink_state_t sw_blink_state;
+#endif
 };
 
 /**
@@ -118,4 +130,9 @@ int led_set_period(struct udevice *dev, int period_ms);
  */
 int led_bind_generic(struct udevice *parent, const char *driver_name);
 
+/* Internal functions for software blinking. Do not use them in your code */
+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);
+
 #endif
-- 
2.39.2

Re: [PATCH 1/2] led: Implement software led blinking

[Mark Kettenis]

> From: Mikhail Kshevetskiy <mikhail.kshevetskiy@iopsys.eu>
> Date: Fri,  5 Jul 2024 06:26:24 +0400
> 
> From: Michael Polyntsov <michael.polyntsov@iopsys.eu>
> 
> If hardware (or driver) doesn't support leds blinking, it's
> now possible to use software implementation of blinking instead.
> This relies on cyclic functions.
> 
> v2 changes:
>  * Drop sw_blink_state structure, move its necessary fields to
>    led_uc_plat structure.
>  * Add cyclic_info pointer to led_uc_plat structure. This
>    simplify code a lot.
>  * Remove cyclic function search logic. Not needed anymore.
>  * Fix blinking period. It was twice large.
>  * Other cleanups.
> 
> v3 changes:
>  * Adapt code to recent cyclic function changes
>  * Move software blinking functions to separate file
>  * Other small changes
> 
> Signed-off-by: Michael Polyntsov <michael.polyntsov@iopsys.eu>
> Signed-off-by: Mikhail Kshevetskiy <mikhail.kshevetskiy@iopsys.eu>
> ---
>  drivers/led/Kconfig        |  14 +++++
>  drivers/led/Makefile       |   1 +
>  drivers/led/led-uclass.c   |  16 +++++-
>  drivers/led/led_sw_blink.c | 106 +++++++++++++++++++++++++++++++++++++
>  include/led.h              |  17 ++++++
>  5 files changed, 152 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/led/led_sw_blink.c
> 
> diff --git a/drivers/led/Kconfig b/drivers/led/Kconfig
> index 9837960198d..dc9d4c8a757 100644
> --- a/drivers/led/Kconfig
> +++ b/drivers/led/Kconfig
> @@ -73,6 +73,20 @@ config LED_BLINK
>  	  This option enables support for this which adds slightly to the
>  	  code size.
>  
> +config LED_SW_BLINK
> +	bool "Support software LED blinking"
> +	depends on LED_BLINK
> +	select CYCLIC
> +	help
> +	  Turns on led blinking implemented in the software, useful when
> +	  the hardware doesn't support led blinking. Half of the period
> +	  led will be ON and the rest time it will be OFF. Standard
> +	  led commands can be used to configure blinking. Does nothing
> +	  if driver supports blinking.
> +	  WARNING: Blinking may be inaccurate during execution of time
> +	  consuming commands (ex. flash reading). Also it completely
> +	  stops during OS booting.

Doesn't that make this feature pretty much pointless?

>  config SPL_LED
>  	bool "Enable LED support in SPL"
>  	depends on SPL_DM
> diff --git a/drivers/led/Makefile b/drivers/led/Makefile
> index 2bcb8589087..e27aa488482 100644
> --- a/drivers/led/Makefile
> +++ b/drivers/led/Makefile
> @@ -4,6 +4,7 @@
>  # Written by Simon Glass <sjg@chromium.org>
>  
>  obj-y += led-uclass.o
> +obj-$(CONFIG_LED_SW_BLINK) += led_sw_blink.o
>  obj-$(CONFIG_LED_BCM6328) += led_bcm6328.o
>  obj-$(CONFIG_LED_BCM6358) += led_bcm6358.o
>  obj-$(CONFIG_LED_BCM6753) += led_bcm6753.o
> diff --git a/drivers/led/led-uclass.c b/drivers/led/led-uclass.c
> index f37bf6a1550..37dc99cecdc 100644
> --- a/drivers/led/led-uclass.c
> +++ b/drivers/led/led-uclass.c
> @@ -58,6 +58,10 @@ int led_set_state(struct udevice *dev, enum led_state_t state)
>  	if (!ops->set_state)
>  		return -ENOSYS;
>  
> +	if (IS_ENABLED(CONFIG_LED_SW_BLINK) &&
> +	    led_sw_on_state_change(dev, state))
> +		return 0;
> +
>  	return ops->set_state(dev, state);
>  }
>  
> @@ -68,6 +72,10 @@ enum led_state_t led_get_state(struct udevice *dev)
>  	if (!ops->get_state)
>  		return -ENOSYS;
>  
> +	if (IS_ENABLED(CONFIG_LED_SW_BLINK) &&
> +	    led_sw_is_blinking(dev))
> +		return LEDST_BLINK;
> +
>  	return ops->get_state(dev);
>  }
>  
> @@ -76,8 +84,12 @@ int led_set_period(struct udevice *dev, int period_ms)
>  {
>  	struct led_ops *ops = led_get_ops(dev);
>  
> -	if (!ops->set_period)
> -		return -ENOSYS;
> +	if (!ops->set_period) {
> +		if (IS_ENABLED(CONFIG_LED_SW_BLINK))
> +			return led_sw_set_period(dev, period_ms);
> +		else
> +			return -ENOSYS;
> +	}
>  
>  	return ops->set_period(dev, period_ms);
>  }
> diff --git a/drivers/led/led_sw_blink.c b/drivers/led/led_sw_blink.c
> new file mode 100644
> index 00000000000..ab56111a60b
> --- /dev/null
> +++ b/drivers/led/led_sw_blink.c
> @@ -0,0 +1,106 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Software blinking helpers
> + * Copyright (C) 2024 IOPSYS Software Solutions AB
> + * Author: Mikhail Kshevetskiy <mikhail.kshevetskiy@iopsys.eu>
> + */
> +
> +#include <dm.h>
> +#include <led.h>
> +#include <time.h>
> +#include <stdlib.h>
> +
> +static void led_sw_blink(struct cyclic_info *c)
> +{
> +	struct led_uc_plat *uc_plat;
> +	struct udevice *dev;
> +	struct led_ops *ops;
> +
> +	uc_plat = container_of(c, struct led_uc_plat, cyclic);
> +	dev = uc_plat->dev;
> +	ops = led_get_ops(dev);
> +
> +	switch (uc_plat->sw_blink_state) {
> +	case LED_SW_BLINK_ST_OFF:
> +		uc_plat->sw_blink_state = LED_SW_BLINK_ST_ON;
> +		ops->set_state(dev, LEDST_ON);
> +		break;
> +	case LED_SW_BLINK_ST_ON:
> +		uc_plat->sw_blink_state = LED_SW_BLINK_ST_OFF;
> +		ops->set_state(dev, LEDST_OFF);
> +		break;
> +	case LED_SW_BLINK_ST_NOT_READY:
> +		/*
> +		 * led_set_period has been called, but
> +		 * led_set_state(LDST_BLINK) has not yet,
> +		 * so doing nothing
> +		 */
> +		break;
> +	default:
> +		break;
> +	}
> +}
> +
> +int led_sw_set_period(struct udevice *dev, int period_ms)
> +{
> +	struct led_uc_plat *uc_plat = dev_get_uclass_plat(dev);
> +	struct cyclic_info *cyclic = &uc_plat->cyclic;
> +	struct led_ops *ops = led_get_ops(dev);
> +	int half_period_us;
> +	char *name;
> +	int len;
> +
> +	half_period_us = period_ms * 1000 / 2;
> +
> +	name = (char *)cyclic->name;
> +	if (name == NULL) {
> +		len = snprintf(NULL, 0, "led_sw_blink_%s", uc_plat->label);
> +		if (len <= 0)
> +			return -ENOMEM;
> +
> +		name = malloc(len + 1);
> +		if (!name)
> +			return -ENOMEM;
> +
> +		snprintf(name, len + 1, "led_sw_blink_%s", uc_plat->label);
> +	}
> +
> +	if (uc_plat->sw_blink_state == LED_SW_BLINK_ST_DISABLED) {
> +		uc_plat->dev = dev;
> +		cyclic_register(cyclic, led_sw_blink, half_period_us, name);
> +	} else {
> +		cyclic->delay_us = half_period_us;
> +		cyclic->start_time_us = timer_get_us();
> +	}
> +
> +	uc_plat->sw_blink_state = LED_SW_BLINK_ST_NOT_READY;
> +	ops->set_state(dev, LEDST_OFF);
> +
> +	return 0;
> +}
> +
> +bool led_sw_is_blinking(struct udevice *dev)
> +{
> +	struct led_uc_plat *uc_plat = dev_get_uclass_plat(dev);
> +
> +	return (uc_plat->sw_blink_state > LED_SW_BLINK_ST_NOT_READY);
> +}
> +
> +bool led_sw_on_state_change(struct udevice *dev, enum led_state_t state)
> +{
> +	struct led_uc_plat *uc_plat = dev_get_uclass_plat(dev);
> +
> +	if (uc_plat->sw_blink_state != LED_SW_BLINK_ST_DISABLED) {
> +		if (state == LEDST_BLINK) {
> +			/* start blinking on next led_sw_blink() call */
> +			uc_plat->sw_blink_state = LED_SW_BLINK_ST_OFF;
> +			return true;
> +		}
> +
> +		/* stop blinking */
> +		cyclic_unregister(&uc_plat->cyclic);
> +		uc_plat->sw_blink_state = LED_SW_BLINK_ST_DISABLED;
> +	}
> +
> +	return false;
> +}
> diff --git a/include/led.h b/include/led.h
> index a6353166289..26955269d3e 100644
> --- a/include/led.h
> +++ b/include/led.h
> @@ -20,6 +20,13 @@ enum led_state_t {
>  	LEDST_COUNT,
>  };
>  
> +enum led_sw_blink_state_t {
> +	LED_SW_BLINK_ST_DISABLED,
> +	LED_SW_BLINK_ST_NOT_READY,
> +	LED_SW_BLINK_ST_OFF,
> +	LED_SW_BLINK_ST_ON,
> +};
> +
>  /**
>   * struct led_uc_plat - Platform data the uclass stores about each device
>   *
> @@ -29,6 +36,11 @@ enum led_state_t {
>  struct led_uc_plat {
>  	const char *label;
>  	enum led_state_t default_state;
> +#ifdef CONFIG_LED_SW_BLINK
> +	struct udevice *dev;
> +	struct cyclic_info cyclic;
> +	enum led_sw_blink_state_t sw_blink_state;
> +#endif
>  };
>  
>  /**
> @@ -118,4 +130,9 @@ int led_set_period(struct udevice *dev, int period_ms);
>   */
>  int led_bind_generic(struct udevice *parent, const char *driver_name);
>  
> +/* Internal functions for software blinking. Do not use them in your code */
> +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);
> +
>  #endif
> -- 
> 2.39.2
> 
> 

Re: [PATCH 1/2] led: Implement software led blinking

[Mikhail Kshevetskiy]

On 05.07.2024 11:29, Mark Kettenis wrote:
> [You don't often get email from mark.kettenis@xs4all.nl. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
>> From: Mikhail Kshevetskiy <mikhail.kshevetskiy@iopsys.eu>
>> Date: Fri,  5 Jul 2024 06:26:24 +0400
>>
>> From: Michael Polyntsov <michael.polyntsov@iopsys.eu>
>>
>> If hardware (or driver) doesn't support leds blinking, it's
>> now possible to use software implementation of blinking instead.
>> This relies on cyclic functions.
>>
>> v2 changes:
>>  * Drop sw_blink_state structure, move its necessary fields to
>>    led_uc_plat structure.
>>  * Add cyclic_info pointer to led_uc_plat structure. This
>>    simplify code a lot.
>>  * Remove cyclic function search logic. Not needed anymore.
>>  * Fix blinking period. It was twice large.
>>  * Other cleanups.
>>
>> v3 changes:
>>  * Adapt code to recent cyclic function changes
>>  * Move software blinking functions to separate file
>>  * Other small changes
>>
>> Signed-off-by: Michael Polyntsov <michael.polyntsov@iopsys.eu>
>> Signed-off-by: Mikhail Kshevetskiy <mikhail.kshevetskiy@iopsys.eu>
>> ---
>>  drivers/led/Kconfig        |  14 +++++
>>  drivers/led/Makefile       |   1 +
>>  drivers/led/led-uclass.c   |  16 +++++-
>>  drivers/led/led_sw_blink.c | 106 +++++++++++++++++++++++++++++++++++++
>>  include/led.h              |  17 ++++++
>>  5 files changed, 152 insertions(+), 2 deletions(-)
>>  create mode 100644 drivers/led/led_sw_blink.c
>>
>> diff --git a/drivers/led/Kconfig b/drivers/led/Kconfig
>> index 9837960198d..dc9d4c8a757 100644
>> --- a/drivers/led/Kconfig
>> +++ b/drivers/led/Kconfig
>> @@ -73,6 +73,20 @@ config LED_BLINK
>>         This option enables support for this which adds slightly to the
>>         code size.
>>
>> +config LED_SW_BLINK
>> +     bool "Support software LED blinking"
>> +     depends on LED_BLINK
>> +     select CYCLIC
>> +     help
>> +       Turns on led blinking implemented in the software, useful when
>> +       the hardware doesn't support led blinking. Half of the period
>> +       led will be ON and the rest time it will be OFF. Standard
>> +       led commands can be used to configure blinking. Does nothing
>> +       if driver supports blinking.
>> +       WARNING: Blinking may be inaccurate during execution of time
>> +       consuming commands (ex. flash reading). Also it completely
>> +       stops during OS booting.
> Doesn't that make this feature pretty much pointless?

It steel make sense if total time of blinking is much large than
blinking period. For example this is a case of router rescue process.
One should upload and flash the firmware. Usually it takes a lot of
time. Vendors often wants led blinking to differ router rescue mode from
normal boot.

>>  config SPL_LED
>>       bool "Enable LED support in SPL"
>>       depends on SPL_DM
>> diff --git a/drivers/led/Makefile b/drivers/led/Makefile
>> index 2bcb8589087..e27aa488482 100644
>> --- a/drivers/led/Makefile
>> +++ b/drivers/led/Makefile
>> @@ -4,6 +4,7 @@
>>  # Written by Simon Glass <sjg@chromium.org>
>>
>>  obj-y += led-uclass.o
>> +obj-$(CONFIG_LED_SW_BLINK) += led_sw_blink.o
>>  obj-$(CONFIG_LED_BCM6328) += led_bcm6328.o
>>  obj-$(CONFIG_LED_BCM6358) += led_bcm6358.o
>>  obj-$(CONFIG_LED_BCM6753) += led_bcm6753.o
>> diff --git a/drivers/led/led-uclass.c b/drivers/led/led-uclass.c
>> index f37bf6a1550..37dc99cecdc 100644
>> --- a/drivers/led/led-uclass.c
>> +++ b/drivers/led/led-uclass.c
>> @@ -58,6 +58,10 @@ int led_set_state(struct udevice *dev, enum led_state_t state)
>>       if (!ops->set_state)
>>               return -ENOSYS;
>>
>> +     if (IS_ENABLED(CONFIG_LED_SW_BLINK) &&
>> +         led_sw_on_state_change(dev, state))
>> +             return 0;
>> +
>>       return ops->set_state(dev, state);
>>  }
>>
>> @@ -68,6 +72,10 @@ enum led_state_t led_get_state(struct udevice *dev)
>>       if (!ops->get_state)
>>               return -ENOSYS;
>>
>> +     if (IS_ENABLED(CONFIG_LED_SW_BLINK) &&
>> +         led_sw_is_blinking(dev))
>> +             return LEDST_BLINK;
>> +
>>       return ops->get_state(dev);
>>  }
>>
>> @@ -76,8 +84,12 @@ int led_set_period(struct udevice *dev, int period_ms)
>>  {
>>       struct led_ops *ops = led_get_ops(dev);
>>
>> -     if (!ops->set_period)
>> -             return -ENOSYS;
>> +     if (!ops->set_period) {
>> +             if (IS_ENABLED(CONFIG_LED_SW_BLINK))
>> +                     return led_sw_set_period(dev, period_ms);
>> +             else
>> +                     return -ENOSYS;
>> +     }
>>
>>       return ops->set_period(dev, period_ms);
>>  }
>> diff --git a/drivers/led/led_sw_blink.c b/drivers/led/led_sw_blink.c
>> new file mode 100644
>> index 00000000000..ab56111a60b
>> --- /dev/null
>> +++ b/drivers/led/led_sw_blink.c
>> @@ -0,0 +1,106 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + * Software blinking helpers
>> + * Copyright (C) 2024 IOPSYS Software Solutions AB
>> + * Author: Mikhail Kshevetskiy <mikhail.kshevetskiy@iopsys.eu>
>> + */
>> +
>> +#include <dm.h>
>> +#include <led.h>
>> +#include <time.h>
>> +#include <stdlib.h>
>> +
>> +static void led_sw_blink(struct cyclic_info *c)
>> +{
>> +     struct led_uc_plat *uc_plat;
>> +     struct udevice *dev;
>> +     struct led_ops *ops;
>> +
>> +     uc_plat = container_of(c, struct led_uc_plat, cyclic);
>> +     dev = uc_plat->dev;
>> +     ops = led_get_ops(dev);
>> +
>> +     switch (uc_plat->sw_blink_state) {
>> +     case LED_SW_BLINK_ST_OFF:
>> +             uc_plat->sw_blink_state = LED_SW_BLINK_ST_ON;
>> +             ops->set_state(dev, LEDST_ON);
>> +             break;
>> +     case LED_SW_BLINK_ST_ON:
>> +             uc_plat->sw_blink_state = LED_SW_BLINK_ST_OFF;
>> +             ops->set_state(dev, LEDST_OFF);
>> +             break;
>> +     case LED_SW_BLINK_ST_NOT_READY:
>> +             /*
>> +              * led_set_period has been called, but
>> +              * led_set_state(LDST_BLINK) has not yet,
>> +              * so doing nothing
>> +              */
>> +             break;
>> +     default:
>> +             break;
>> +     }
>> +}
>> +
>> +int led_sw_set_period(struct udevice *dev, int period_ms)
>> +{
>> +     struct led_uc_plat *uc_plat = dev_get_uclass_plat(dev);
>> +     struct cyclic_info *cyclic = &uc_plat->cyclic;
>> +     struct led_ops *ops = led_get_ops(dev);
>> +     int half_period_us;
>> +     char *name;
>> +     int len;
>> +
>> +     half_period_us = period_ms * 1000 / 2;
>> +
>> +     name = (char *)cyclic->name;
>> +     if (name == NULL) {
>> +             len = snprintf(NULL, 0, "led_sw_blink_%s", uc_plat->label);
>> +             if (len <= 0)
>> +                     return -ENOMEM;
>> +
>> +             name = malloc(len + 1);
>> +             if (!name)
>> +                     return -ENOMEM;
>> +
>> +             snprintf(name, len + 1, "led_sw_blink_%s", uc_plat->label);
>> +     }
>> +
>> +     if (uc_plat->sw_blink_state == LED_SW_BLINK_ST_DISABLED) {
>> +             uc_plat->dev = dev;
>> +             cyclic_register(cyclic, led_sw_blink, half_period_us, name);
>> +     } else {
>> +             cyclic->delay_us = half_period_us;
>> +             cyclic->start_time_us = timer_get_us();
>> +     }
>> +
>> +     uc_plat->sw_blink_state = LED_SW_BLINK_ST_NOT_READY;
>> +     ops->set_state(dev, LEDST_OFF);
>> +
>> +     return 0;
>> +}
>> +
>> +bool led_sw_is_blinking(struct udevice *dev)
>> +{
>> +     struct led_uc_plat *uc_plat = dev_get_uclass_plat(dev);
>> +
>> +     return (uc_plat->sw_blink_state > LED_SW_BLINK_ST_NOT_READY);
>> +}
>> +
>> +bool led_sw_on_state_change(struct udevice *dev, enum led_state_t state)
>> +{
>> +     struct led_uc_plat *uc_plat = dev_get_uclass_plat(dev);
>> +
>> +     if (uc_plat->sw_blink_state != LED_SW_BLINK_ST_DISABLED) {
>> +             if (state == LEDST_BLINK) {
>> +                     /* start blinking on next led_sw_blink() call */
>> +                     uc_plat->sw_blink_state = LED_SW_BLINK_ST_OFF;
>> +                     return true;
>> +             }
>> +
>> +             /* stop blinking */
>> +             cyclic_unregister(&uc_plat->cyclic);
>> +             uc_plat->sw_blink_state = LED_SW_BLINK_ST_DISABLED;
>> +     }
>> +
>> +     return false;
>> +}
>> diff --git a/include/led.h b/include/led.h
>> index a6353166289..26955269d3e 100644
>> --- a/include/led.h
>> +++ b/include/led.h
>> @@ -20,6 +20,13 @@ enum led_state_t {
>>       LEDST_COUNT,
>>  };
>>
>> +enum led_sw_blink_state_t {
>> +     LED_SW_BLINK_ST_DISABLED,
>> +     LED_SW_BLINK_ST_NOT_READY,
>> +     LED_SW_BLINK_ST_OFF,
>> +     LED_SW_BLINK_ST_ON,
>> +};
>> +
>>  /**
>>   * struct led_uc_plat - Platform data the uclass stores about each device
>>   *
>> @@ -29,6 +36,11 @@ enum led_state_t {
>>  struct led_uc_plat {
>>       const char *label;
>>       enum led_state_t default_state;
>> +#ifdef CONFIG_LED_SW_BLINK
>> +     struct udevice *dev;
>> +     struct cyclic_info cyclic;
>> +     enum led_sw_blink_state_t sw_blink_state;
>> +#endif
>>  };
>>
>>  /**
>> @@ -118,4 +130,9 @@ int led_set_period(struct udevice *dev, int period_ms);
>>   */
>>  int led_bind_generic(struct udevice *parent, const char *driver_name);
>>
>> +/* Internal functions for software blinking. Do not use them in your code */
>> +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);
>> +
>>  #endif
>> --
>> 2.39.2
>>
>>