On 27.06.2024 22:05, Simon Glass wrote:
> Hi Mikhail,
>
> On Thu, 27 Jun 2024 at 12:31, Mikhail Kshevetskiy
> <mikhail.kshevets...@iopsys.eu> wrote:
>> From: Michael Polyntsov <michael.polynt...@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.polynt...@iopsys.eu>
>> Signed-off-by: Mikhail Kshevetskiy <mikhail.kshevets...@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
