[1/2] led: Implement software led blinking

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(-)

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

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.

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.

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

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

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