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_BLINKselect CYCLIChelpTurns on led blinking implemented in the software, useful whenthe hardware doesn't support led blinking. Half of the periodled will be ON and the rest time it will be OFF. Standardled commands can be used to configure blinking. Does nothingif driver supports blinking.WARNING: Blinking may be inaccurate during execution of timeconsuming commands (ex. flash reading). Also it will completelystops 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