On 27.06.2024 22:05, Simon Glass wrote:
Hi Mikhail,
On Thu, 27 Jun 2024 at 12:31, 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 | 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_BLINKselect CYCLIChelpTurns on led blinking implemented in the software, useful whenthe hardware doesn't support led blinking. Does nothing ifdriver supports blinking.Can you talk about the blinking p[eriod / API?
Could you clarify what do you mean?
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
will fix
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) {
will fix
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
will fix
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