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

[Christian Marangi <ansuelsmth@gmail.com>]

On Thu, Sep 26, 2024 at 11:33:18PM +0200, Simon Glass wrote:
> 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
> 

Hi, thanks for the review. I asked some clarification, thanks for any
comments.

> >
> > 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
>

Ok.

> > +
> >  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() ?
> 

Idea here and up is to cache the dev on bind to prevent
useless/additional loop on search in each UCLASS LED the boot LED.

uclass_get_device_tail is needed to actually trigger probe of the LED if
it hasn't been done prev.

The uclass_get_priv is followed by the rkmtd_get_cur_dev()
implementation, for the sake of getting the UCLASS alone it seems too
much to have all the additional operation to get the first device.

Also considering uclass_get_device_tail is also used by get_by_label I
assume it's ok to also use it here.

Should I ignore caching the dev and just search for the boot LED from
label everytime out of simplicity?

> > +               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
> 

Didn't know of the global_data but then I read the comments and it seems
we should not really pollute it with too much stuff. Also considering
most of the LED are driven with gpio the possibility of having this that
early on boot might be closer to 0.

> >  };
> > 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

-- 
	Ansuel