Re: [PATCH v4 01/11] led: toggle LED on initial SW blink

[Christian Marangi <ansuelsmth@gmail.com>]

On Sat, Sep 21, 2024 at 01:14:36PM +0200, Michael Nazzareno Trimarchi wrote:
> Hi
> 
> On Sat, Sep 21, 2024 at 12:51 AM Christian Marangi <ansuelsmth@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.
> >
> > Toggle the LED (ON if OFF, OFF if ON) on initial SW blink to handle this
> > corner case and better display a LED blink from the user.
> >
> > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > Reviewed-by: Simon Glass <sjg@chromium.org>
> > ---
> >  drivers/led/led_sw_blink.c | 10 +++++++++-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/led/led_sw_blink.c b/drivers/led/led_sw_blink.c
> > index 9e36edbee47..9d9820720c6 100644
> > --- a/drivers/led/led_sw_blink.c
> > +++ b/drivers/led/led_sw_blink.c
> > @@ -103,8 +103,16 @@ 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);
> > +               enum led_state_t curr_state = led_get_state(dev);
> > +
> > +               curr_state = ops->get_state(dev);
> 
> 
> The led_get_state return  curr_state. You need to use a) led_set/get
> state or them from ops
> 
> if (state == LEDST_BLINK) {
>         const struct led_ops *ops = led_get_ops(dev);
>         enum led_state_t curr_state = ops->get_state(dev);
>         enun led_state_t next_state;
> 
>         switch (curr_state) {
>         case LEDST_ON:
>                 sw_blink->state = LED_SW_BLINK_ST_OFF;
>                 next_state = LEDST_OFF;
>                break;
>         case LEDST_OFF:
>                 sw_blink->state = LED_SW_BLINK_ST_ON;
>                 next_state = LEDST_ON;
>                 break;
>         }
> 
>         ops->set_state(dev, next_state);
>         return true;
> }
>

Hi,

I'm not following how this is different than the proposed code...
Is this a suggestion to make it more readable, I'm a bit confused.

> 
> > +               /* toggle led initially */
> > +               ops->set_state(dev, curr_state == LEDST_ON ? LEDST_OFF :
> > +                              LEDST_ON);
> >                 /* start blinking on next led_sw_blink() call */
> > -               sw_blink->state = LED_SW_BLINK_ST_OFF;
> > +               sw_blink->state = curr_state == LEDST_ON ? LED_SW_BLINK_ST_OFF :
> > +                                 LED_SW_BLINK_ST_ON;
> 
> 
> >                 return true;
> >         }
> >
> > --
> > 2.45.2
> >
> 
> 
> -- 
> Michael Nazzareno Trimarchi
> Co-Founder & Chief Executive Officer
> M. +39 347 913 2170
> michael@amarulasolutions.com
> __________________________________
> 
> Amarula Solutions BV
> Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
> T. +31 (0)85 111 9172
> info@amarulasolutions.com
> www.amarulasolutions.com

-- 
	Ansuel