[PATCH v5 0/2] led: implement software blinking
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.
v3 changes: * Adapt code to recent cyclic function changes * Move software blinking functions to separate file * Other small changes
v4 changes: * refactoring of led_set_period() function
v5 changes * fix compilation if CONFIG_LED_BLINK is not defined
Michael Polyntsov (2): led: Implement software led blinking led: Add dts property to specify blinking of the led
drivers/led/Kconfig | 14 +++++ drivers/led/Makefile | 1 + drivers/led/led-uclass.c | 52 +++++++++++++++--- drivers/led/led_sw_blink.c | 106 +++++++++++++++++++++++++++++++++++++ include/led.h | 17 ++++++ 5 files changed, 183 insertions(+), 7 deletions(-) create mode 100644 drivers/led/led_sw_blink.c
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 + 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) { + 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); + } + + 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); +} + +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
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_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 completelystops 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 ^
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.
}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 ()
+}
+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
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_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 completelystops 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
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 | 33 +++++++++++++++++++++++++++++---- 1 file changed, 29 insertions(+), 4 deletions(-)
diff --git a/drivers/led/led-uclass.c b/drivers/led/led-uclass.c index 4f5dd66765e..90047ad35ba 100644 --- a/drivers/led/led-uclass.c +++ b/drivers/led/led-uclass.c @@ -100,6 +100,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"); @@ -120,6 +123,12 @@ 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. @@ -132,12 +141,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 Fri, 12 Jul 2024 at 06:25, 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 | 33 +++++++++++++++++++++++++++++---- 1 file changed, 29 insertions(+), 4 deletions(-)
diff --git a/drivers/led/led-uclass.c b/drivers/led/led-uclass.c index 4f5dd66765e..90047ad35ba 100644 --- a/drivers/led/led-uclass.c +++ b/drivers/led/led-uclass.c @@ -100,6 +100,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");@@ -120,6 +123,12 @@ 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
I still wonder whether you can use IS_ENABLED() instead of adding #ifdefs?
/* * In case the LED has default-state DT property, trigger * probe() to configure its default state during startup.@@ -132,12 +141,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) = {
2.39.2
Regards, Simon
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
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 | 33 +++++++++++++++++++++++++++++---- 1 file changed, 29 insertions(+), 4 deletions(-)
diff --git a/drivers/led/led-uclass.c b/drivers/led/led-uclass.c index 4f5dd66765e..90047ad35ba 100644 --- a/drivers/led/led-uclass.c +++ b/drivers/led/led-uclass.c @@ -100,6 +100,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");@@ -120,6 +123,12 @@ 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
I still wonder whether you can use IS_ENABLED() instead of adding #ifdefs?
LEDST_BLINK depends on CONFIG_LED_BLINK, so the code will not compile if CONFIG_LED_BLINK is not defined. Defining a LEDST_BLINK unconditionally will produce an issue with cmd/led.c
/* * In case the LED has default-state DT property, trigger * probe() to configure its default state during startup.@@ -132,12 +141,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) = {
2.39.2
Regards, Simon
Hi Mikhail,
On Sat, 13 Jul 2024 at 17:32, Mikhail Kshevetskiy mikhail.kshevetskiy@genexis.eu wrote:
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
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 | 33 +++++++++++++++++++++++++++++---- 1 file changed, 29 insertions(+), 4 deletions(-)
diff --git a/drivers/led/led-uclass.c b/drivers/led/led-uclass.c index 4f5dd66765e..90047ad35ba 100644 --- a/drivers/led/led-uclass.c +++ b/drivers/led/led-uclass.c @@ -100,6 +100,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");@@ -120,6 +123,12 @@ 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
I still wonder whether you can use IS_ENABLED() instead of adding #ifdefs?
LEDST_BLINK depends on CONFIG_LED_BLINK, so the code will not compile if CONFIG_LED_BLINK is not defined. Defining a LEDST_BLINK unconditionally will produce an issue with cmd/led.c
We should not use #ifdefs around such declarations. What issue is caused by defining it always?
Regards, Simon
On 7/15/24 11:11, Simon Glass wrote:
Hi Mikhail,
On Sat, 13 Jul 2024 at 17:32, Mikhail Kshevetskiy mikhail.kshevetskiy@genexis.eu wrote:
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
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 | 33 +++++++++++++++++++++++++++++---- 1 file changed, 29 insertions(+), 4 deletions(-)
diff --git a/drivers/led/led-uclass.c b/drivers/led/led-uclass.c index 4f5dd66765e..90047ad35ba 100644 --- a/drivers/led/led-uclass.c +++ b/drivers/led/led-uclass.c @@ -100,6 +100,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");@@ -120,6 +123,12 @@ 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
I still wonder whether you can use IS_ENABLED() instead of adding #ifdefs?
LEDST_BLINK depends on CONFIG_LED_BLINK, so the code will not compile if CONFIG_LED_BLINK is not defined. Defining a LEDST_BLINK unconditionally will produce an issue with cmd/led.c
We should not use #ifdefs around such declarations. What issue is caused by defining it always?
We need to patch cmd/led.c. Without it we will get: * something like out of boundary access * enumeration value ‘LEDST_BLINK’ not handled in switch
can be fixed, but probably it should be a separate patch
Regards, Simon
On Mon, 15 Jul 2024 at 14:23, Mikhail Kshevetskiy mikhail.kshevetskiy@genexis.eu wrote:
On 7/15/24 11:11, Simon Glass wrote:
Hi Mikhail,
On Sat, 13 Jul 2024 at 17:32, Mikhail Kshevetskiy mikhail.kshevetskiy@genexis.eu wrote:
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
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 | 33 +++++++++++++++++++++++++++++---- 1 file changed, 29 insertions(+), 4 deletions(-)
diff --git a/drivers/led/led-uclass.c b/drivers/led/led-uclass.c index 4f5dd66765e..90047ad35ba 100644 --- a/drivers/led/led-uclass.c +++ b/drivers/led/led-uclass.c @@ -100,6 +100,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");@@ -120,6 +123,12 @@ 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
I still wonder whether you can use IS_ENABLED() instead of adding #ifdefs?
LEDST_BLINK depends on CONFIG_LED_BLINK, so the code will not compile if CONFIG_LED_BLINK is not defined. Defining a LEDST_BLINK unconditionally will produce an issue with cmd/led.c
We should not use #ifdefs around such declarations. What issue is caused by defining it always?
We need to patch cmd/led.c. Without it we will get:
- something like out of boundary access
- enumeration value ‘LEDST_BLINK’ not handled in switch
can be fixed, but probably it should be a separate patch
Sounds good
Reviewed-by: Simon Glass sjg@chromium.org
participants (3)
-
Mikhail Kshevetskiy -
Mikhail Kshevetskiy -
Simon Glass