[PATCH 1/2] led: Implement software led blinking
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_BLINK + select CYCLIC + help + Turns on led blinking implemented in the software, useful when + the hardware doesn't support led blinking. Does nothing if + driver supports blinking. + 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 + 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) { + 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; + 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; + } + + 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) + 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); + 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 (!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 + }
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); - return 0; }
From: Michael Polyntsov michael.polyntsov@iopsys.eu
The standard property
linux,default-trigger = "pattern";
used to get an effect. No blinking parameters can be set yet.
Signed-off-by: Michael Polyntsov michael.polyntsov@iopsys.eu Signed-off-by: Mikhail Kshevetskiy mikhail.kshevetskiy@iopsys.eu --- drivers/led/led-uclass.c | 34 ++++++++++++++++++++++++++++++---- 1 file changed, 30 insertions(+), 4 deletions(-)
diff --git a/drivers/led/led-uclass.c b/drivers/led/led-uclass.c index b35964f2e99..645d0069efe 100644 --- a/drivers/led/led-uclass.c +++ b/drivers/led/led-uclass.c @@ -271,6 +271,9 @@ static int led_post_bind(struct udevice *dev) { struct led_uc_plat *uc_plat = dev_get_uclass_plat(dev); const char *default_state; +#ifdef CONFIG_LED_BLINK + const char *trigger; +#endif
if (!uc_plat->label) uc_plat->label = dev_read_string(dev, "label"); @@ -291,6 +294,13 @@ static int led_post_bind(struct udevice *dev) else return 0;
+#ifdef CONFIG_LED_BLINK + trigger = dev_read_string(dev, "linux,default-trigger"); + if (trigger && !strncmp(trigger, "pattern", 7)) { + uc_plat->default_state = LEDST_BLINK; + } +#endif + /* * In case the LED has default-state DT property, trigger * probe() to configure its default state during startup. @@ -302,12 +312,28 @@ static int led_post_bind(struct udevice *dev) static int led_post_probe(struct udevice *dev) { struct led_uc_plat *uc_plat = dev_get_uclass_plat(dev); + int rc = 0;
- if (uc_plat->default_state == LEDST_ON || - uc_plat->default_state == LEDST_OFF) - led_set_state(dev, uc_plat->default_state); + switch (uc_plat->default_state) { + case LEDST_ON: + case LEDST_OFF: + rc = led_set_state(dev, uc_plat->default_state); + break; +#ifdef CONFIG_LED_BLINK + case LEDST_BLINK: { + const int default_period_ms = 1000;
- return 0; + rc = led_set_period(dev, default_period_ms); + if (rc == 0) + rc = led_set_state(dev, uc_plat->default_state); + break; + } +#endif + default: + break; + } + + return rc; }
UCLASS_DRIVER(led) = {
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?
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
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) {
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
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
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
Hi Mikhail,
On Tue, 2 Jul 2024 at 12:54, Mikhail Kshevetskiy mikhail.kshevetskiy@genexis.eu wrote:
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?
I mean can you explain in this help
[..]
Regards, Simon
Hi Simon and all,
This patch series implements: * software led blinking (via cyclic functions) * add support of dts property to specify blinking of the led
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.
Thanks, Mikhail Kshevetskiy
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 --- 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. + 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 };
/**
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
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@pre... 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
From: Michael Polyntsov michael.polyntsov@iopsys.eu
The standard property
linux,default-trigger = "pattern";
used to get an effect. No blinking parameters can be set yet.
Signed-off-by: Michael Polyntsov michael.polyntsov@iopsys.eu Signed-off-by: Mikhail Kshevetskiy mikhail.kshevetskiy@iopsys.eu --- drivers/led/led-uclass.c | 34 ++++++++++++++++++++++++++++++---- 1 file changed, 30 insertions(+), 4 deletions(-)
diff --git a/drivers/led/led-uclass.c b/drivers/led/led-uclass.c index d021c3bbf20..78d1a3d152b 100644 --- a/drivers/led/led-uclass.c +++ b/drivers/led/led-uclass.c @@ -190,6 +190,9 @@ static int led_post_bind(struct udevice *dev) { struct led_uc_plat *uc_plat = dev_get_uclass_plat(dev); const char *default_state; +#ifdef CONFIG_LED_BLINK + const char *trigger; +#endif
if (!uc_plat->label) uc_plat->label = dev_read_string(dev, "label"); @@ -210,6 +213,13 @@ static int led_post_bind(struct udevice *dev) else return 0;
+#ifdef CONFIG_LED_BLINK + trigger = dev_read_string(dev, "linux,default-trigger"); + if (trigger && !strncmp(trigger, "pattern", 7)) { + uc_plat->default_state = LEDST_BLINK; + } +#endif + /* * In case the LED has default-state DT property, trigger * probe() to configure its default state during startup. @@ -222,12 +232,28 @@ static int led_post_bind(struct udevice *dev) static int led_post_probe(struct udevice *dev) { struct led_uc_plat *uc_plat = dev_get_uclass_plat(dev); + int rc = 0;
- if (uc_plat->default_state == LEDST_ON || - uc_plat->default_state == LEDST_OFF) - led_set_state(dev, uc_plat->default_state); + switch (uc_plat->default_state) { + case LEDST_ON: + case LEDST_OFF: + rc = led_set_state(dev, uc_plat->default_state); + break; +#ifdef CONFIG_LED_BLINK + case LEDST_BLINK: { + const int default_period_ms = 1000;
- return 0; + rc = led_set_period(dev, default_period_ms); + if (rc == 0) + rc = led_set_state(dev, uc_plat->default_state); + break; + } +#endif + default: + break; + } + + return rc; }
UCLASS_DRIVER(led) = {
Hi Mikhail,
On Wed, 3 Jul 2024 at 02:02, Mikhail Kshevetskiy mikhail.kshevetskiy@iopsys.eu wrote:
From: Michael Polyntsov michael.polyntsov@iopsys.eu
The standard property
linux,default-trigger = "pattern";used to get an effect. No blinking parameters can be set yet.
Signed-off-by: Michael Polyntsov michael.polyntsov@iopsys.eu Signed-off-by: Mikhail Kshevetskiy mikhail.kshevetskiy@iopsys.eu
drivers/led/led-uclass.c | 34 ++++++++++++++++++++++++++++++---- 1 file changed, 30 insertions(+), 4 deletions(-)
diff --git a/drivers/led/led-uclass.c b/drivers/led/led-uclass.c index d021c3bbf20..78d1a3d152b 100644 --- a/drivers/led/led-uclass.c +++ b/drivers/led/led-uclass.c @@ -190,6 +190,9 @@ static int led_post_bind(struct udevice *dev) { struct led_uc_plat *uc_plat = dev_get_uclass_plat(dev); const char *default_state; +#ifdef CONFIG_LED_BLINK
const char *trigger;
It looks like this is not used, so you can drop it and use a local variable here?
+#endif
if (!uc_plat->label) uc_plat->label = dev_read_string(dev, "label");@@ -210,6 +213,13 @@ static int led_post_bind(struct udevice *dev) else return 0;
+#ifdef CONFIG_LED_BLINK
if (IS_ENABLED(CONFIG_LED_BLINK)
trigger = dev_read_string(dev, "linux,default-trigger");if (trigger && !strncmp(trigger, "pattern", 7)) {uc_plat->default_state = LEDST_BLINK;}
No {} around single lines. You can use patman to send patches and it will check this for you (I hope!).
+#endif
/* * In case the LED has default-state DT property, trigger * probe() to configure its default state during startup.@@ -222,12 +232,28 @@ static int led_post_bind(struct udevice *dev) static int led_post_probe(struct udevice *dev) { struct led_uc_plat *uc_plat = dev_get_uclass_plat(dev);
int rc = 0;
if (uc_plat->default_state == LEDST_ON ||uc_plat->default_state == LEDST_OFF)led_set_state(dev, uc_plat->default_state);
switch (uc_plat->default_state) {case LEDST_ON:case LEDST_OFF:rc = led_set_state(dev, uc_plat->default_state);break;+#ifdef CONFIG_LED_BLINK
case LEDST_BLINK: {
Here again you can use if IS_ENABLED(CONFIG_LED_BLINK) inside the case which should produce very similar code size, hopefully the same.
const int default_period_ms = 1000;
return 0;
rc = led_set_period(dev, default_period_ms);if (rc == 0)rc = led_set_state(dev, uc_plat->default_state);break;}+#endif
default:break;}return rc;}
UCLASS_DRIVER(led) = {
2.39.2
Regards, Simon
On 7/3/24 12:08, Simon Glass wrote:
Hi Mikhail,
On Wed, 3 Jul 2024 at 02:02, Mikhail Kshevetskiy mikhail.kshevetskiy@iopsys.eu wrote:
From: Michael Polyntsov michael.polyntsov@iopsys.eu
The standard property
linux,default-trigger = "pattern";used to get an effect. No blinking parameters can be set yet.
Signed-off-by: Michael Polyntsov michael.polyntsov@iopsys.eu Signed-off-by: Mikhail Kshevetskiy mikhail.kshevetskiy@iopsys.eu
drivers/led/led-uclass.c | 34 ++++++++++++++++++++++++++++++---- 1 file changed, 30 insertions(+), 4 deletions(-)
diff --git a/drivers/led/led-uclass.c b/drivers/led/led-uclass.c index d021c3bbf20..78d1a3d152b 100644 --- a/drivers/led/led-uclass.c +++ b/drivers/led/led-uclass.c @@ -190,6 +190,9 @@ static int led_post_bind(struct udevice *dev) { struct led_uc_plat *uc_plat = dev_get_uclass_plat(dev); const char *default_state; +#ifdef CONFIG_LED_BLINK
const char *trigger;It looks like this is not used, so you can drop it and use a local variable here?
Unfortunately no, see below.
+#endif
if (!uc_plat->label) uc_plat->label = dev_read_string(dev, "label");@@ -210,6 +213,13 @@ static int led_post_bind(struct udevice *dev) else return 0;
+#ifdef CONFIG_LED_BLINK
if (IS_ENABLED(CONFIG_LED_BLINK)
This is not so easy. The definition of LEDST_BLINK depends on CONFIG_LED_BLINK, thus this code will not compile if CONFIG_LED_BLINK is not defined. We can define LEDST_BLINK unconditionally, but it will cause an issue in the cmd/led.c.
trigger = dev_read_string(dev, "linux,default-trigger");if (trigger && !strncmp(trigger, "pattern", 7)) {uc_plat->default_state = LEDST_BLINK;}No {} around single lines. You can use patman to send patches and it will check this for you (I hope!).
+#endif
/* * In case the LED has default-state DT property, trigger * probe() to configure its default state during startup.@@ -222,12 +232,28 @@ static int led_post_bind(struct udevice *dev) static int led_post_probe(struct udevice *dev) { struct led_uc_plat *uc_plat = dev_get_uclass_plat(dev);
int rc = 0;
if (uc_plat->default_state == LEDST_ON ||uc_plat->default_state == LEDST_OFF)led_set_state(dev, uc_plat->default_state);
switch (uc_plat->default_state) {case LEDST_ON:case LEDST_OFF:rc = led_set_state(dev, uc_plat->default_state);break;+#ifdef CONFIG_LED_BLINK
case LEDST_BLINK: {Here again you can use if IS_ENABLED(CONFIG_LED_BLINK) inside the case which should produce very similar code size, hopefully the same.
Do you suggest an if inside the handling of "default:" case? Also see above notes about dependency of LEDST_BLINK on CONFIG_LED_BLINK.
const int default_period_ms = 1000;
return 0;
rc = led_set_period(dev, default_period_ms);if (rc == 0)rc = led_set_state(dev, uc_plat->default_state);break;}+#endif
default:break;}return rc;}
UCLASS_DRIVER(led) = {
2.39.2
Regards, Simon
On Thu, Jun 27, 2024 at 02:31:13PM +0300, Mikhail Kshevetskiy 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(-)
As one of those "well, hunh" kind of moment, I'm cc'ing Christian Marangi who also just posted patches foor this kind of functionality.
On Thu, Jun 27, 2024 at 01:36:09PM -0600, Tom Rini wrote:
On Thu, Jun 27, 2024 at 02:31:13PM +0300, Mikhail Kshevetskiy 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(-)
As one of those "well, hunh" kind of moment, I'm cc'ing Christian Marangi who also just posted patches foor this kind of functionality.
Eh... Well this implementation is better. I had the idea of adding support on the uclass level but I preferred to limit it only to GPIO.
Think this would match how it's done in upstream linux kernel so I think mine should be ignored and this taken (for sw blink).
Should not change a thing for the series since all the bits were using generic LED functions.
participants (6)
-
Christian Marangi -
Mikhail Kshevetskiy -
Mikhail Kshevetskiy -
Rasmus Villemoes -
Simon Glass -
Tom Rini