[PATCH v4 03/11] led: implement LED boot API
Simon Glass
sjg at chromium.org
Thu Sep 26 23:33:18 CEST 2024
Hi Christian,
On Sat, 21 Sept 2024 at 00:51, Christian Marangi <ansuelsmth at gmail.com> wrote:
>
> Implement LED boot API to signal correct boot of the system.
>
> led_boot_on/off/blink() are introduced to turn ON, OFF and BLINK the
> designated boot LED.
>
> New Kconfig is introduced, CONFIG_LED_BOOT to enable the feature.
> This makes use of the /options/u-boot property "boot-led" to the
> define the boot LED.
> It's also introduced a new /options/u-boot property "boot-led-period"
> to define the default period when the LED is set to blink mode.
>
> If "boot-led-period" is not defined, the value of 250 (ms) is
> used by default.
>
> If CONFIG_LED_BLINK or CONFIG_LED_SW_BLINK is not enabled,
> led_boot_blink call will fallback to simple LED ON.
>
> To cache the data we repurpose the now unused led_uc_priv for storage of
> global LED uclass info.
Some things to tweak below
>
> Signed-off-by: Christian Marangi <ansuelsmth at gmail.com>
> ---
> drivers/led/Kconfig | 7 +++
> drivers/led/led-uclass.c | 100 +++++++++++++++++++++++++++++++++++++++
> include/led.h | 56 +++++++++++++++++++++-
> 3 files changed, 161 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/led/Kconfig b/drivers/led/Kconfig
> index bee74b25751..6149cfa02b8 100644
> --- a/drivers/led/Kconfig
> +++ b/drivers/led/Kconfig
> @@ -9,6 +9,13 @@ config LED
> can provide access to board-specific LEDs. Use of the device tree
> for configuration is encouraged.
>
> +config LED_BOOT
> + bool "Enable LED boot support"
> + help
> + Enable LED boot support.
> +
> + LED boot is a specific LED assigned to signal boot operation status.
Here you should link to the /options binding in
doc/device-tree-bindings/options, perhaps
> +
> config LED_BCM6328
> bool "LED Support for BCM6328"
> depends on LED && ARCH_BMIPS
> diff --git a/drivers/led/led-uclass.c b/drivers/led/led-uclass.c
> index 199d68bc25a..c5b560982b0 100644
> --- a/drivers/led/led-uclass.c
> +++ b/drivers/led/led-uclass.c
> @@ -94,17 +94,101 @@ int led_set_period(struct udevice *dev, int period_ms)
> return -ENOSYS;
> }
>
> +#ifdef CONFIG_LED_BOOT
> +int led_boot_on(void)
> +{
> + struct uclass *uc = uclass_find(UCLASS_LED);
> + struct led_uc_priv *priv;
> + struct udevice *dev;
> +
> + if (!uc)
> + return -ENOENT;
> +
The normal way is:
ret = uclass_first_device_ret(UCLASS_LED, &dev);
if (ret)
return ret;
> + priv = uclass_get_priv(uc);
> + if (!priv->boot_led_dev ||
> + uclass_get_device_tail(priv->boot_led_dev, 0, &dev)) {
That is an internal function...I suppose it should really have an
underscore and be in uclass-internal.h
But if you are looking for boot_led_label, don't you need to search
through the LEDs to find it? Or use led_get_by_label() ?
> + printf("Failed to get boot LED %s\n",
> + priv->boot_led_label);
> + return -EINVAL;
> + }
> +
> + return led_set_state(dev, LEDST_ON);
> +}
> +
> +int led_boot_off(void)
> +{
> + struct uclass *uc = uclass_find(UCLASS_LED);
> + struct led_uc_priv *priv;
> + struct udevice *dev;
> +
> + if (!uc)
> + return -ENOENT;
> +
Same here.
> + priv = uclass_get_priv(uc);
> + if (!priv->boot_led_dev ||
> + uclass_get_device_tail(priv->boot_led_dev, 0, &dev)) {
> + printf("Failed to get boot LED %s\n",
> + priv->boot_led_label);
> + return -EINVAL;
> + }
> +
> + return led_set_state(dev, LEDST_OFF);
> +}
> +
> +#if defined(CONFIG_LED_BLINK) || defined(CONFIG_LED_SW_BLINK)
> +int led_boot_blink(void)
> +{
> + struct uclass *uc = uclass_find(UCLASS_LED);
> + struct led_uc_priv *priv;
> + struct udevice *dev;
> + int ret;
> +
> + if (!uc)
> + return -ENOENT;
> +
> + priv = uclass_get_priv(uc);
> + if (!priv->boot_led_dev ||
> + uclass_get_device_tail(priv->boot_led_dev, 0, &dev)) {
> + printf("Failed to get boot LED %s\n",
> + priv->boot_led_label);
> + return -EINVAL;
> + }
This looks like the same code again. I think it should be in a
function so the code is not repeated.
> +
> + ret = led_set_period(dev, priv->boot_led_period);
> + if (ret) {
> + if (ret != -ENOSYS)
> + return ret;
> +
> + /* fallback to ON with no set_period and no SW_BLINK */
> + return led_set_state(dev, LEDST_ON);
> + }
> +
> + return led_set_state(dev, LEDST_BLINK);
> +}
> +#endif
> +#endif
> +
> 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_BOOT
> + struct led_uc_priv *priv = uclass_get_priv(dev->uclass);
> +#endif
> +
> if (!uc_plat->label)
> uc_plat->label = dev_read_string(dev, "label");
>
> if (!uc_plat->label && !dev_read_string(dev, "compatible"))
> uc_plat->label = ofnode_get_name(dev_ofnode(dev));
>
> +#ifdef CONFIG_LED_BOOT
> + /* check if we are binding boot LED and assign it */
> + if (!strcmp(priv->boot_led_label, uc_plat->label))
> + priv->boot_led_dev = dev;
> +#endif
> +
> uc_plat->default_state = LEDST_COUNT;
>
> default_state = dev_read_string(dev, "default-state");
> @@ -158,10 +242,26 @@ static int led_post_probe(struct udevice *dev)
> return ret;
> }
>
> +#ifdef CONFIG_LED_BOOT
> +static int led_init(struct uclass *uc)
> +{
> + struct led_uc_priv *priv = uclass_get_priv(uc);
> +
> + priv->boot_led_label = ofnode_options_read_str("boot-led");
> + priv->boot_led_period = ofnode_options_read_int("boot-led-period", 250);
> +
> + return 0;
> +}
> +#endif
> +
> UCLASS_DRIVER(led) = {
> .id = UCLASS_LED,
> .name = "led",
> .per_device_plat_auto = sizeof(struct led_uc_plat),
> .post_bind = led_post_bind,
> .post_probe = led_post_probe,
> +#ifdef CONFIG_LED_BOOT
> + .init = led_init,
> + .priv_auto = sizeof(struct led_uc_priv),
> +#endif
I don't love all these #ifdefs but I suppose it is OK here. Ideally we
would have static inline accessors for the fields, in the header file,
e.g. like asm-generic/global_data.h
> };
> diff --git a/include/led.h b/include/led.h
> index 99f93c5ef86..ca495217777 100644
> --- a/include/led.h
> +++ b/include/led.h
> @@ -9,6 +9,7 @@
>
> #include <stdbool.h>
> #include <cyclic.h>
> +#include <dm/ofnode.h>
>
> struct udevice;
>
> @@ -52,10 +53,16 @@ struct led_uc_plat {
> /**
> * struct led_uc_priv - Private data the uclass stores about each device
> *
> - * @period_ms: Flash period in milliseconds
> + * @boot_led_label: Boot LED label
> + * @boot_led_dev: Boot LED dev
> + * @boot_led_period: Boot LED blink period
> */
> struct led_uc_priv {
> - int period_ms;
> +#ifdef CONFIG_LED_BOOT
> + const char *boot_led_label;
> + struct udevice *boot_led_dev;
> + int boot_led_period;
> +#endif
> };
>
> struct led_ops {
> @@ -141,4 +148,49 @@ 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);
>
> +#ifdef CONFIG_LED_BOOT
> +
> +/**
> + * led_boot_on() - turn ON the designated LED for booting
> + *
> + * Return: 0 if OK, -ve on error
> + */
> +int led_boot_on(void);
> +
> +/**
> + * led_boot_off() - turn OFF the designated LED for booting
> + *
> + * Return: 0 if OK, -ve on error
> + */
> +int led_boot_off(void);
> +
> +#if defined(CONFIG_LED_BLINK) || defined(CONFIG_LED_SW_BLINK)
> +/**
> + * led_boot_blink() - turn ON the designated LED for booting
> + *
> + * Return: 0 if OK, -ve on error
> + */
> +int led_boot_blink(void);
> +
> +#else
> +/* If LED BLINK is not supported/enabled, fallback to LED ON */
> +#define led_boot_blink led_boot_on
> +#endif
> +#else
> +static inline int led_boot_on(void)
> +{
> + return -ENOSYS;
> +}
> +
> +static inline int led_boot_off(void)
> +{
> + return -ENOSYS;
> +}
> +
> +static inline int led_boot_blink(void)
> +{
> + return -ENOSYS;
> +}
> +#endif
> +
> #endif
> --
> 2.45.2
>
Regards,
Simon
More information about the U-Boot
mailing list