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

Simon Glass sjg at chromium.org
Thu Jun 27 21:05:43 CEST 2024


Hi Mikhail,

On Thu, 27 Jun 2024 at 12:31, Mikhail Kshevetskiy
<mikhail.kshevetskiy at iopsys.eu> wrote:
>
> From: Michael Polyntsov <michael.polyntsov at 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 at iopsys.eu>
> Signed-off-by: Mikhail Kshevetskiy <mikhail.kshevetskiy at 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


More information about the U-Boot mailing list