Re: [PATCH v4 03/11] led: implement LED boot API

Hi Christian,

On Sat, 21 Sept 2024 at 00:51, Christian Marangi ansuelsmth@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@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