On Thu, Sep 19, 2024 at 04:13:44PM +0200, Simon Glass wrote:
> Hi Christian,
>
> On Mon, 12 Aug 2024 at 12:33, Christian Marangi <ansuels...@gmail.com> wrote:
> >
> > We currently init the LED OFF when SW blink is triggered when
> > on_state_change() is called. This can be problematic for very short
> > period as the ON/OFF blink might never trigger.
> >
> > Turn LED ON on initial SW blink to handle this corner case and better
> > display a LED blink from the user.
> >
> > Signed-off-by: Christian Marangi <ansuels...@gmail.com>
> > ---
> > drivers/led/led_sw_blink.c | 5 ++++-
> > 1 file changed, 4 insertions(+), 1 deletion(-)
>
> Reviewed-by: Simon Glass <s...@chromium.org>
>
> nit below
>
> >
> > diff --git a/drivers/led/led_sw_blink.c b/drivers/led/led_sw_blink.c
> > index 9e36edbee47..853278670b9 100644
> > --- a/drivers/led/led_sw_blink.c
> > +++ b/drivers/led/led_sw_blink.c
> > @@ -103,8 +103,11 @@ bool led_sw_on_state_change(struct udevice *dev, enum
> > led_state_t state)
> > return false;
> >
> > if (state == LEDST_BLINK) {
> > + struct led_ops *ops = led_get_ops(dev);
> > +
> > + ops->set_state(dev, LEDST_ON);
>
> Normally in drivers we define functions like led_set_state() in the
> uclass, rather than calling things directly like this.
>
I used ops directly as I'm following how it's done in led_sw_blink and
because the support for these ops is already validated and we don't need
to check for the -ENOSYS condition.
Hope it's ok. Also as suggested I changed the function to toggle the LED
as suggested. I added the review tag. Tell me if I have to drop it in
the next revision.
> > /* start blinking on next led_sw_blink() call */
> > - sw_blink->state = LED_SW_BLINK_ST_OFF;
> > + sw_blink->state = LED_SW_BLINK_ST_ON;
> > return true;
> > }
> >
> > --
> > 2.45.2
> >
>
> Regards,
> Simon
--
Ansuel
