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?
> +
> 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
