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

Mikhail Kshevetskiy mikhail.kshevetskiy at genexis.eu
Tue Jul 2 13:54:27 CEST 2024


On 27.06.2024 22:05, Simon Glass wrote:
> 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?

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


More information about the U-Boot mailing list