| Message ID | 20240812103254.26972-9-ansuelsmth@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | led: introduce LED boot and activity function | expand |
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@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) > bye, Heiko
Hi all On Wed, Aug 14, 2024 at 6:34 AM Heiko Schocher <hs@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@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 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@denx.de
On Wed, Aug 14, 2024 at 06:33:12AM +0200, Heiko Schocher 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@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? > Yes can be done. > > + > > 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) > > > > 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@denx.de
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@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@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? 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. > > > > 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@denx.de > > > > -- > 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
Hi On Sun, Aug 18, 2024 at 6:24 PM Christian Marangi <ansuelsmth@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@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@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@denx.de > > > > > > > > -- > > 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
On Sun, Aug 18, 2024 at 09:32:32PM +0200, Michael Nazzareno Trimarchi wrote: > Hi > > On Sun, Aug 18, 2024 at 6:24 PM Christian Marangi <ansuelsmth@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@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@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 Yes that was suggested and I will change code to use this > > 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. > It's really a choice but currently for the boot led people have to use board code to turn on the LED or use the preboot env to run command... Not very clean. Is it really that bad to have these simple call in these functions? > > > > 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@denx.de > > > > > > > > > > > > -- > > > 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 > > > > -- > 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
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 + 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)
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@gmail.com> --- cmd/ubi.c | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-)