| Message ID | 20240703010131.1735100-2-mikhail.kshevetskiy@iopsys.eu |
|---|---|
| State | Changes Requested |
| Delegated to: | Simon Glass |
| Headers | show |
| Series | [1/2] led: Implement software led blinking | expand |
Hi Mikhail, On Wed, 3 Jul 2024 at 02:02, Mikhail Kshevetskiy <mikhail.kshevetskiy@iopsys.eu> wrote: > > From: Michael Polyntsov <michael.polyntsov@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.polyntsov@iopsys.eu> > Signed-off-by: Mikhail Kshevetskiy <mikhail.kshevetskiy@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
Mikhail Kshevetskiy <mikhail.kshevetskiy@iopsys.eu> writes: > + > +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; > + } You need to be aware of the API change that is by now in master, see https://lore.kernel.org/u-boot/20240521084652.1726460-1-rasmus.villemoes@prevas.dk/ and in particular commits 3a11eada38e and 008c4b3c3115. The latter you'll find soon enough because this won't build. The former is a bit more subtle and would silently break here (as passing an auto array is no longer allowed) - consider whether you really need the led_sw_blink_ to be part of the name, or if uc_plat->label itself isn't descriptive enough. Rasmus
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. + 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 +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 + 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 +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 }; /**