Hi Mikhail,
On Wed, 3 Jul 2024 at 02:02, 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.
>
> 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.
>
> Signed-off-by: Michael Polyntsov <michael.polynt...@iopsys.eu>
> Signed-off-by: Mikhail Kshevetskiy <mikhail.kshevets...@iopsys.eu>
LGTM except for the #ifdefs...I've put an idea below.
> ---
> drivers/led/Kconfig | 14 ++++++
> drivers/led/led-uclass.c | 102 +++++++++++++++++++++++++++++++++++++++
> include/led.h | 12 +++++
> 3 files changed, 128 insertions(+)
>
> diff --git a/drivers/led/Kconfig b/drivers/led/Kconfig
> index 9837960198d..1afb081df11 100644
> --- a/drivers/led/Kconfig
> +++ b/drivers/led/Kconfig
> @@ -73,6 +73,20 @@ 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. Half of the period
> + led will be ON and the rest time it will be OFF. Standard
> + led commands can be used to configure blinking. Does nothing
> + if driver supports blinking.
> + WARNING: Blinking may be inaccurate during execution of time
> + consuming commands (ex. flash reading). Also it will completely
> + stops during OS booting.
Drop the word 'will'
> +
> 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..d021c3bbf20 100644
> --- a/drivers/led/led-uclass.c
> +++ b/drivers/led/led-uclass.c
> @@ -52,6 +52,94 @@ int led_get_by_label(const char *label, struct udevice
> **devp)
> return -ENODEV;
> }
>
> +#ifdef CONFIG_LED_SW_BLINK
You can put this block into a separate file, like led_blink.c and
export the functions, since it accesses members which are behind an
#ifdef
obj-$(CONFIG_LED_SW_BLINK) += led_blink.o
> +static void led_sw_blink(void *data)
> +{
> + struct udevice *dev = data;
> + struct led_uc_plat *uc_plat = dev_get_uclass_plat(dev);
> + struct led_ops *ops = led_get_ops(dev);
> +
> + switch (uc_plat->sw_blink_state) {
> + case LED_SW_BLINK_ST_OFF:
> + uc_plat->sw_blink_state = LED_SW_BLINK_ST_ON;
> + ops->set_state(dev, LEDST_ON);
> + break;
> + case LED_SW_BLINK_ST_ON:
> + uc_plat->sw_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 int led_sw_set_period(struct udevice *dev, int period_ms)
> +{
> + struct led_uc_plat *uc_plat = dev_get_uclass_plat(dev);
> + struct cyclic_info *cyclic = uc_plat->cyclic;
> + struct led_ops *ops = led_get_ops(dev);
> + char cyclic_name[64];
> + int half_period_us;
> +
> + uc_plat->sw_blink_state = LED_SW_BLINK_ST_NONE;
> + ops->set_state(dev, LEDST_OFF);
> +
> + half_period_us = period_ms * 1000 / 2;
> +
> + if (cyclic) {
> + cyclic->delay_us = half_period_us;
> + cyclic->start_time_us = timer_get_us();
> + } else {
> + snprintf(cyclic_name, sizeof(cyclic_name),
> + "led_sw_blink_%s", uc_plat->label);
> +
> + cyclic = cyclic_register(led_sw_blink, half_period_us,
> + cyclic_name, dev);
> + if (!cyclic) {
> + log_err("Registering of blinking function for %s
> failed\n",
> + uc_plat->label);
> + return -ENOMEM;
> + }
> +
> + uc_plat->cyclic = cyclic;
> + }
> +
> + return 0;
> +}
> +
> +static bool led_sw_is_blinking(struct udevice *dev)
> +{
> + struct led_uc_plat *uc_plat = dev_get_uclass_plat(dev);
> +
> + return (uc_plat->sw_blink_state != LED_SW_BLINK_ST_NONE);
> +}
> +
> +static bool led_sw_on_state_change(struct udevice *dev, enum led_state_t
> state)
> +{
> + struct led_uc_plat *uc_plat = dev_get_uclass_plat(dev);
> +
> + if (uc_plat->cyclic) {
> + if (state == LEDST_BLINK) {
> + /* start blinking on next led_sw_blink() call */
> + uc_plat->sw_blink_state = LED_SW_BLINK_ST_OFF;
> + return true;
> + }
> +
> + /* stop blinking */
> + cyclic_unregister(uc_plat->cyclic);
> + uc_plat->cyclic = NULL;
> + uc_plat->sw_blink_state = LED_SW_BLINK_ST_NONE;
> + }
> +
> + return false;
> +}
> +#endif /* CONFIG_LED_SW_BLINK */
> +
> int led_set_state(struct udevice *dev, enum led_state_t state)
> {
> struct led_ops *ops = led_get_ops(dev);
> @@ -59,6 +147,11 @@ int led_set_state(struct udevice *dev, enum led_state_t
> state)
> if (!ops->set_state)
> return -ENOSYS;
>
> +#ifdef CONFIG_LED_SW_BLINK
Then these #ifdefs should be able to change to
if (IS_ENABLED(CONFIG_LED_SW_BLINK) &&
led_sw_on_state_change(dev, state))
...
I suppose static inlines are another way if you prefer, but as there
is only one caller it probably isn't worth it.
> + if (led_sw_on_state_change(dev, state))
> + return 0;
> +#endif
> +
> return ops->set_state(dev, state);
> }
>
> @@ -69,6 +162,11 @@ enum led_state_t led_get_state(struct udevice *dev)
> if (!ops->get_state)
> return -ENOSYS;
>
> +#ifdef CONFIG_LED_SW_BLINK
> + if (led_sw_is_blinking(dev))
> + return LEDST_BLINK;
> +#endif
> +
> return ops->get_state(dev);
> }
>
> @@ -78,7 +176,11 @@ int led_set_period(struct udevice *dev, int period_ms)
> struct led_ops *ops = led_get_ops(dev);
>
> if (!ops->set_period)
> +#ifdef CONFIG_LED_SW_BLINK
> + return led_sw_set_period(dev, period_ms);
> +#else
> return -ENOSYS;
> +#endif
>
> return ops->set_period(dev, period_ms);
> }
> diff --git a/include/led.h b/include/led.h
> index a6353166289..bd50fdf33ef 100644
> --- a/include/led.h
> +++ b/include/led.h
> @@ -20,6 +20,14 @@ enum led_state_t {
> LEDST_COUNT,
> };
>
> +#ifdef CONFIG_LED_SW_BLINK
can drop this #ifdef
> +enum led_sw_blink_state_t {
> + LED_SW_BLINK_ST_NONE = 0,
> + LED_SW_BLINK_ST_OFF = 1,
> + LED_SW_BLINK_ST_ON = 2,
> +};
> +#endif
> +
> /**
> * struct led_uc_plat - Platform data the uclass stores about each device
> *
> @@ -29,6 +37,10 @@ enum led_state_t {
> struct led_uc_plat {
> const char *label;
> enum led_state_t default_state;
> +#ifdef CONFIG_LED_SW_BLINK
> + enum led_sw_blink_state_t sw_blink_state;
> + struct cyclic_info *cyclic;
> +#endif
> };
>
Need to add function prototypes here for flashing API.
> /**
> --
> 2.39.2
>
Regards,
Simon
