Re: [PATCH v5 1/2] led: Implement software led blinking

On 13.07.2024 18:13, Simon Glass wrote:

Hi Mikhail,

On Fri, 12 Jul 2024 at 06:25, 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.

Signed-off-by: Michael Polyntsov michael.polyntsov@iopsys.eu Signed-off-by: Mikhail Kshevetskiy mikhail.kshevetskiy@iopsys.eu

---

drivers/led/Kconfig | 14 +++++ drivers/led/Makefile | 1 + drivers/led/led-uclass.c | 19 +++++-- drivers/led/led_sw_blink.c | 106 +++++++++++++++++++++++++++++++++++++ include/led.h | 17 ++++++ 5 files changed, 154 insertions(+), 3 deletions(-) create mode 100644 drivers/led/led_sw_blink.c

diff --git a/drivers/led/Kconfig b/drivers/led/Kconfig index 9837960198d..dc9d4c8a757 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 completely
  

•     stops during OS booting.
  

config SPL_LED bool "Enable LED support in SPL" depends on SPL_DM diff --git a/drivers/led/Makefile b/drivers/led/Makefile index 2bcb8589087..e27aa488482 100644 --- a/drivers/led/Makefile +++ b/drivers/led/Makefile @@ -4,6 +4,7 @@ # Written by Simon Glass sjg@chromium.org

obj-y += led-uclass.o +obj-$(CONFIG_LED_SW_BLINK) += led_sw_blink.o obj-$(CONFIG_LED_BCM6328) += led_bcm6328.o obj-$(CONFIG_LED_BCM6358) += led_bcm6358.o obj-$(CONFIG_LED_BCM6753) += led_bcm6753.o diff --git a/drivers/led/led-uclass.c b/drivers/led/led-uclass.c index f37bf6a1550..4f5dd66765e 100644 --- a/drivers/led/led-uclass.c +++ b/drivers/led/led-uclass.c @@ -58,6 +58,10 @@ int led_set_state(struct udevice *dev, enum led_state_t state) if (!ops->set_state) return -ENOSYS;

•   if (IS_ENABLED(CONFIG_LED_SW_BLINK) &&
  

•       led_sw_on_state_change(dev, state))
  

•           return 0;
  

•   return ops->set_state(dev, state);
  

}

@@ -68,6 +72,12 @@ enum led_state_t led_get_state(struct udevice *dev) if (!ops->get_state) return -ENOSYS;

+#ifdef CONFIG_LED_BLINK

It looks like you can use if (IS_ENABLED...) for that ^

No, LEDST_BLINK depends on CONFIG_LED_BLINK. I can define LEDST_BLINK unconditionally, but this will produce an issue in the other place

...

•   if (IS_ENABLED(CONFIG_LED_SW_BLINK) &&
  

•       led_sw_is_blinking(dev))
  

•           return LEDST_BLINK;
  

+#endif

•   return ops->get_state(dev);
  

}

@@ -76,10 +86,13 @@ int led_set_period(struct udevice *dev, int period_ms) { struct led_ops *ops = led_get_ops(dev);

•   if (!ops->set_period)
  

•           return -ENOSYS;
  

•   if (ops->set_period)
  

•           return ops->set_period(dev, period_ms);
  

•   if (IS_ENABLED(CONFIG_LED_SW_BLINK))
  

•           return led_sw_set_period(dev, period_ms);
  
  

•   return ops->set_period(dev, period_ms);
  

•   return -ENOSYS;
  

} #endif

diff --git a/drivers/led/led_sw_blink.c b/drivers/led/led_sw_blink.c new file mode 100644 index 00000000000..ab56111a60b --- /dev/null +++ b/drivers/led/led_sw_blink.c @@ -0,0 +1,106 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*

• • Software blinking helpers
• • Copyright (C) 2024 IOPSYS Software Solutions AB
• • Author: Mikhail Kshevetskiy mikhail.kshevetskiy@iopsys.eu
• */

+#include <dm.h> +#include <led.h> +#include <time.h> +#include <stdlib.h>

+static void led_sw_blink(struct cyclic_info *c) +{

•   struct led_uc_plat *uc_plat;
  

•   struct udevice *dev;
  

•   struct led_ops *ops;
  

•   uc_plat = container_of(c, struct led_uc_plat, cyclic);
  

•   dev = uc_plat->dev;
  

•   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_NOT_READY:
  

•           /*
  

•            * led_set_period has been called, but
  

•            * led_set_state(LDST_BLINK) has not yet,
  

•            * so doing nothing
  

•            */
  

•           break;
  

•   default:
  

•           break;
  

•   }
  

+}

+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);
  

•   int half_period_us;
  

•   char *name;
  

•   int len;
  

•   half_period_us = period_ms * 1000 / 2;
  

•   name = (char *)cyclic->name;
  

•   if (name == NULL) {
  

if (!name)

...

•           len = snprintf(NULL, 0, "led_sw_blink_%s", uc_plat->label);
  

•           if (len <= 0)
  

•                   return -ENOMEM;
  

•           name = malloc(len + 1);
  

•           if (!name)
  

•                   return -ENOMEM;
  

•           snprintf(name, len + 1, "led_sw_blink_%s", uc_plat->label);
  

I think it would be OK to have a fixed-length name (say 20-30 chars) and use snprintf()...would reduce the code size.

ok, will fix

...

•   }
  

•   if (uc_plat->sw_blink_state == LED_SW_BLINK_ST_DISABLED) {
  

•           uc_plat->dev = dev;
  

•           cyclic_register(cyclic, led_sw_blink, half_period_us, name);
  

•   } else {
  

•           cyclic->delay_us = half_period_us;
  

•           cyclic->start_time_us = timer_get_us();
  

•   }
  

•   uc_plat->sw_blink_state = LED_SW_BLINK_ST_NOT_READY;
  

•   ops->set_state(dev, LEDST_OFF);
  

•   return 0;
  

+}

+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_NOT_READY);
  

drop ()

will fix

...

+}

+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->sw_blink_state != LED_SW_BLINK_ST_DISABLED) {
  

•           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->sw_blink_state = LED_SW_BLINK_ST_DISABLED;
  

•   }
  

•   return false;
  

+} diff --git a/include/led.h b/include/led.h index a6353166289..26955269d3e 100644 --- a/include/led.h +++ b/include/led.h @@ -20,6 +20,13 @@ enum led_state_t { LEDST_COUNT, };

+enum led_sw_blink_state_t {

•   LED_SW_BLINK_ST_DISABLED,
  

•   LED_SW_BLINK_ST_NOT_READY,
  

•   LED_SW_BLINK_ST_OFF,
  

•   LED_SW_BLINK_ST_ON,
  

+};

/**

• struct led_uc_plat - Platform data the uclass stores about each device

@@ -29,6 +36,11 @@ enum led_state_t { struct led_uc_plat { const char *label; enum led_state_t default_state; +#ifdef CONFIG_LED_SW_BLINK

•   struct udevice *dev;
  

•   struct cyclic_info cyclic;
  

•   enum led_sw_blink_state_t sw_blink_state;
  

+#endif };

/** @@ -118,4 +130,9 @@ int led_set_period(struct udevice *dev, int period_ms); */ int led_bind_generic(struct udevice *parent, const char *driver_name);

+/* Internal functions for software blinking. Do not use them in your code */ +int led_sw_set_period(struct udevice *dev, int period_ms); +bool led_sw_is_blinking(struct udevice *dev); +bool led_sw_on_state_change(struct udevice *dev, enum led_state_t state);

#endif

2.39.2

Regards, Simon