Hi Christian, On Sat, 21 Sept 2024 at 00:51, Christian Marangi <[email protected]> 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 <[email protected]> > --- > 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
