[PATCH v3 8/9] ubi: implement support for LED activity

[Michael Nazzareno Trimarchi]

Hi

On Sun, Aug 18, 2024 at 6:24 PM Christian Marangi <ansuelsmth at gmail.com> wrote:
>
> On Wed, Aug 14, 2024 at 10:17:18AM +0200, Michael Nazzareno Trimarchi wrote:
> > Hi all
> >
> > On Wed, Aug 14, 2024 at 6:34 AM Heiko Schocher <hs at denx.de> wrote:
> > >
> > > Hello Christian,
> > >
> > > On 12.08.24 12:32, Christian Marangi wrote:
> > > > Implement support for LED activity. If the feature is enabled,
> > > > make the defined ACTIVITY LED to signal ubi write operation.
> > > >
> > > > Signed-off-by: Christian Marangi <ansuelsmth at gmail.com>
> > > > ---
> > > >   cmd/ubi.c | 17 +++++++++++++++--
> > > >   1 file changed, 15 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/cmd/ubi.c b/cmd/ubi.c
> > > > index 0e62e449327..6f679eae9c3 100644
> > > > --- a/cmd/ubi.c
> > > > +++ b/cmd/ubi.c
> > > > @@ -14,6 +14,7 @@
> > > >   #include <command.h>
> > > >   #include <env.h>
> > > >   #include <exports.h>
> > > > +#include <led.h>
> > > >   #include <malloc.h>
> > > >   #include <memalign.h>
> > > >   #include <mtd.h>
> > > > @@ -488,10 +489,22 @@ exit:
> > > >
> > > >   int ubi_volume_write(char *volume, void *buf, loff_t offset, size_t size)
> > > >   {
> > > > +     int ret;
> > > > +
> > > > +#ifdef CONFIG_LED_ACTIVITY_ENABLE
> > > > +     led_activity_blink();
> > > > +#endif
> > >
> > > Do we really need ifdef? May it is possible to declare an empty function
> > > when CONFIG_LED_ACTIVITY_ENABLE is not set? May this applies for the whole
> > > series?
> > >
> > > > +
> > > >       if (!offset)
> > > > -             return ubi_volume_begin_write(volume, buf, size, size);
> > > > +             ret = ubi_volume_begin_write(volume, buf, size, size);
> > > > +     else
> > > > +             ret = ubi_volume_offset_write(volume, buf, offset, size);
> > > >
> > > > -     return ubi_volume_offset_write(volume, buf, offset, size);
> > > > +#ifdef CONFIG_LED_ACTIVITY_ENABLE
> > > > +     led_activity_off();
> > > > +#endif
> > > > +
> > > > +     return ret;
> > > >   }
> > > >
> > > >   int ubi_volume_read(char *volume, char *buf, loff_t offset, size_t size)
> > > >
> > >
> > I rather prefer to have some registration of events that need to be executed for
> > a particular i/o activity and then a subscription process from led
> > subsystem if that
> > particular event is connected to the dts or just on a board file
> >
>
> My concern is that it might become too complex just for the sake of
> putting a LED intro a state. Do we have other case where such event
> subsystem might be useful?

I was thinking of reusing the cyclic subsystem that allows you to
subscribe to functions
that are executed periodically. I mean it's not exciting to have
function call everywhere,
and anyway I think that

#if defined(CONFIG_FOO)
foo_activity
#else
foo_activity() { };
#endif

This is my preference to not have it ENABLED everywhere. As I
mentioned I even not
have experience about having such needs in in code. Most can be implemented
in a script except blinking like:

led on; ext4load <> ; led off. We can definitely script most of it.
The only exception can be
led blink; ext4load <>; led off.

>
> Uboot is not really multi thread so we don't expect that much thing to
> happen at the same time. Do we have case where an i/o might happen in
> multiple place? Example transfering data and writing them at the same
> time? The common practice is to first transfer and then handle.
>

Michael

> >
> >
> > > bye,
> > > Heiko
> > > --
> > > DENX Software Engineering GmbH,      Managing Director: Erika Unter
> > > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> > > Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs at denx.de
> >
> >
> >
> > --
> > Michael Nazzareno Trimarchi
> > Co-Founder & Chief Executive Officer
> > M. +39 347 913 2170
> > michael at amarulasolutions.com
> > __________________________________
> >
> > Amarula Solutions BV
> > Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
> > T. +31 (0)85 111 9172
> > info at amarulasolutions.com
> > www.amarulasolutions.com
>
> --
>         Ansuel



-- 
Michael Nazzareno Trimarchi
Co-Founder & Chief Executive Officer
M. +39 347 913 2170
michael at amarulasolutions.com
__________________________________

Amarula Solutions BV
Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
T. +31 (0)85 111 9172
info at amarulasolutions.com
www.amarulasolutions.com