| Message ID | 20240812103254.26972-2-ansuelsmth@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | led: introduce LED boot and activity function | expand |
Am 12. August 2024 12:32:43 MESZ schrieb Christian Marangi <ansuelsmth@gmail.com>: >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. If the the prior state is on, blinking should start with off. If the prior state is off, blinking should start with on. Best regards Heinrich > >Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> >--- > drivers/led/led_sw_blink.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > >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); > /* 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; > } >
On Tue, Aug 13, 2024 at 12:00:59AM +0200, Heinrich Schuchardt wrote: > > > Am 12. August 2024 12:32:43 MESZ schrieb Christian Marangi <ansuelsmth@gmail.com>: > >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. > > If the the prior state is on, blinking should start with off. > > If the prior state is off, blinking should start with on. > A bit confused. You mean I should improve the commit description or the code needs to he changed to reflect this and check the LED status before applying the BLINK? > > > > > >Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> > >--- > > drivers/led/led_sw_blink.c | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > >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); > > /* 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; > > } > >
Hi Christian, On Mon, 12 Aug 2024 at 12:33, 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. > > 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 <ansuelsmth@gmail.com> > --- > drivers/led/led_sw_blink.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) Reviewed-by: Simon Glass <sjg@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. > /* 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
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 <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. > > > > 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 <ansuelsmth@gmail.com> > > --- > > drivers/led/led_sw_blink.c | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > Reviewed-by: Simon Glass <sjg@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
On 22.08.24 12:47, Christian Marangi wrote: > On Tue, Aug 13, 2024 at 12:00:59AM +0200, Heinrich Schuchardt wrote: >> >> >> Am 12. August 2024 12:32:43 MESZ schrieb Christian Marangi <ansuelsmth@gmail.com>: >>> 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. >> >> If the the prior state is on, blinking should start with off. >> >> If the prior state is off, blinking should start with on. >> > > A bit confused. You mean I should improve the commit description or the > code needs to he changed to reflect this and check the LED status before > applying the BLINK? The code should take the prior state of the LED into account. When enabling blinking the on/off state of the LED should change immediately. Best regards Heinrich > >> >> >>> >>> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> >>> --- >>> drivers/led/led_sw_blink.c | 5 ++++- >>> 1 file changed, 4 insertions(+), 1 deletion(-) >>> >>> 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); >>> /* 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; >>> } >>> >
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); /* 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; }
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 <ansuelsmth@gmail.com> --- drivers/led/led_sw_blink.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)