| Message ID | 20210906205753.176175-2-arnaud.ferraris@collabora.com |
|---|---|
| State | Superseded |
| Delegated to: | Andre Przywara |
| Headers | show |
| Series | [1/2] board: sunxi: enable status LED early | expand |
On Mon, 6 Sep 2021 22:57:52 +0200 Arnaud Ferraris <arnaud.ferraris@collabora.com> wrote: Hi Arnaud, > For some systems, such as the PinePhone, there is no way for the end > user to make sure the system is indeed booting before the boot script is > executed, which takes several seconds. Therefore, it can be useful to > provide early visual feedback as soon as possible. > > In order achieve this goal, this patch initializes the status LED (if > configured) in the SPL. Thanks for sending this, it looks much better than the previous attempt on the list from earlier this year. > Signed-off-by: Arnaud Ferraris <arnaud.ferraris@collabora.com> > --- > board/sunxi/board.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/board/sunxi/board.c b/board/sunxi/board.c > index 1a46100e40..6e0bf5fbf9 100644 > --- a/board/sunxi/board.c > +++ b/board/sunxi/board.c > @@ -46,6 +46,9 @@ > #include <spl.h> > #include <sy8106a.h> > #include <asm/setup.h> > +#if defined(CONFIG_LED_STATUS) && defined(CONFIG_SPL_DRIVERS_MISC) > +#include <status_led.h> status_led.h already guards for CONFIG_LED_STATUS, so you can just unconditionally include this here, no #ifdefs needed. > +#endif > > #if defined CONFIG_VIDEO_LCD_PANEL_I2C && !(defined CONFIG_SPL_BUILD) > /* So that we can use pin names in Kconfig and sunxi_name_to_gpio() */ > @@ -672,6 +675,10 @@ void sunxi_board_init(void) > { > int power_failed = 0; > > +#if defined(CONFIG_LED_STATUS) && defined(CONFIG_SPL_DRIVERS_MISC) Unfortunately status_led.h doesn't define a dummy prototype for this, so we need the #ifdef CONFIG_LED_STATUS. But I don't think you need CONFIG_SPL_DRIVERS_MISC here. If that should only trigger in the SPL, use: if (IS_ENABLED(CONFIG_SPL_BUILD)) status_led_init(); Cheers, Andre > + status_led_init(); > +#endif > + > #ifdef CONFIG_SY8106A_POWER > power_failed = sy8106a_set_vout1(CONFIG_SY8106A_VOUT1_VOLT); > #endif
Hi André, Thanks for your feedback! Le 07/09/2021 à 01:46, Andre Przywara a écrit : > On Mon, 6 Sep 2021 22:57:52 +0200 > Arnaud Ferraris <arnaud.ferraris@collabora.com> wrote: > > Hi Arnaud, > >> diff --git a/board/sunxi/board.c b/board/sunxi/board.c >> index 1a46100e40..6e0bf5fbf9 100644 >> --- a/board/sunxi/board.c >> +++ b/board/sunxi/board.c >> @@ -46,6 +46,9 @@ >> #include <spl.h> >> #include <sy8106a.h> >> #include <asm/setup.h> >> +#if defined(CONFIG_LED_STATUS) && defined(CONFIG_SPL_DRIVERS_MISC) >> +#include <status_led.h> > > status_led.h already guards for CONFIG_LED_STATUS, so you can just > unconditionally include this here, no #ifdefs needed. Understood, will do. > >> +#endif >> >> #if defined CONFIG_VIDEO_LCD_PANEL_I2C && !(defined CONFIG_SPL_BUILD) >> /* So that we can use pin names in Kconfig and sunxi_name_to_gpio() */ >> @@ -672,6 +675,10 @@ void sunxi_board_init(void) >> { >> int power_failed = 0; >> >> +#if defined(CONFIG_LED_STATUS) && defined(CONFIG_SPL_DRIVERS_MISC) > > Unfortunately status_led.h doesn't define a dummy prototype for this, > so we need the #ifdef CONFIG_LED_STATUS. But I don't think you need > CONFIG_SPL_DRIVERS_MISC here. If that should only trigger in the SPL, use: > > if (IS_ENABLED(CONFIG_SPL_BUILD)) > status_led_init(); > Actually the status_led driver isn't compiled into SPL unless CONFIG_SPL_DRIVERS_MISC is set, resulting in a link error if that's not the case. We should therefore check for both CONFIG_LED_STATUS and CONFIG_SPL_DRIVERS_MISC to prevent build errors both at compile time and link time. The alternative would be: #ifdef CONFIG_LED_STATUS if (IS_ENABLED(CONFIG_SPL_DRIVERS_MISC)) status_led_init(); #endif Which is more or less the same, except that it relies on the compiler optimizing out this function call if CONFIG_SPL_DRIVERS_MISC isn't defined. That assumption feels less safe to me, but overall I'm fine with both implementations. Please also note the whole sunxi_board_init() function itself is already inside a #ifdef CONFIG_SPL_BUILD block. Cheers, Arnaud > Cheers, > Andre > >> + status_led_init(); >> +#endif >> + >> #ifdef CONFIG_SY8106A_POWER >> power_failed = sy8106a_set_vout1(CONFIG_SY8106A_VOUT1_VOLT); >> #endif
On Tue, 7 Sep 2021 13:41:11 +0200 Arnaud Ferraris <arnaud.ferraris@collabora.com> wrote: Hi Arnaud, > Thanks for your feedback! > > Le 07/09/2021 à 01:46, Andre Przywara a écrit : > > On Mon, 6 Sep 2021 22:57:52 +0200 > > Arnaud Ferraris <arnaud.ferraris@collabora.com> wrote: > > > > Hi Arnaud, > > > >> diff --git a/board/sunxi/board.c b/board/sunxi/board.c > >> index 1a46100e40..6e0bf5fbf9 100644 > >> --- a/board/sunxi/board.c > >> +++ b/board/sunxi/board.c > >> @@ -46,6 +46,9 @@ > >> #include <spl.h> > >> #include <sy8106a.h> > >> #include <asm/setup.h> > >> +#if defined(CONFIG_LED_STATUS) && defined(CONFIG_SPL_DRIVERS_MISC) > >> +#include <status_led.h> > > > > status_led.h already guards for CONFIG_LED_STATUS, so you can just > > unconditionally include this here, no #ifdefs needed. > > Understood, will do. > > > > >> +#endif > >> > >> #if defined CONFIG_VIDEO_LCD_PANEL_I2C && !(defined CONFIG_SPL_BUILD) > >> /* So that we can use pin names in Kconfig and sunxi_name_to_gpio() */ > >> @@ -672,6 +675,10 @@ void sunxi_board_init(void) > >> { > >> int power_failed = 0; > >> > >> +#if defined(CONFIG_LED_STATUS) && defined(CONFIG_SPL_DRIVERS_MISC) > > > > Unfortunately status_led.h doesn't define a dummy prototype for this, > > so we need the #ifdef CONFIG_LED_STATUS. But I don't think you need > > CONFIG_SPL_DRIVERS_MISC here. If that should only trigger in the SPL, use: > > > > if (IS_ENABLED(CONFIG_SPL_BUILD)) > > status_led_init(); > > > > Actually the status_led driver isn't compiled into SPL unless > CONFIG_SPL_DRIVERS_MISC is set, resulting in a link error if that's not > the case. We should therefore check for both CONFIG_LED_STATUS and > CONFIG_SPL_DRIVERS_MISC to prevent build errors both at compile time and > link time. > > The alternative would be: > > #ifdef CONFIG_LED_STATUS > if (IS_ENABLED(CONFIG_SPL_DRIVERS_MISC)) > status_led_init(); > #endif Right, the nasty bit here is that you could just have LED_STATUS for U-Boot proper, but there is no separate CONFIG_SPL_LED_STATUS, so we need both symbols, although it looks weird. > Which is more or less the same, except that it relies on the compiler > optimizing out this function call if CONFIG_SPL_DRIVERS_MISC isn't > defined. That assumption feels less safe to me, but overall I'm fine > with both implementations. Yes, but I still prefer to have as little #ifdef as possible, because ... > Please also note the whole sunxi_board_init() function itself is already > inside a #ifdef CONFIG_SPL_BUILD block. You are right, I missed that. That whole file is an #ifdef nightmare, and especially nested #ifdefs are the worst. So I would like to not *add* more of them, hence asking for IS_ENABLED() wherever possible. Thanks, Andre > Cheers, > Arnaud > > > Cheers, > > Andre > > > >> + status_led_init(); > >> +#endif > >> + > >> #ifdef CONFIG_SY8106A_POWER > >> power_failed = sy8106a_set_vout1(CONFIG_SY8106A_VOUT1_VOLT); > >> #endif
diff --git a/board/sunxi/board.c b/board/sunxi/board.c index 1a46100e40..6e0bf5fbf9 100644 --- a/board/sunxi/board.c +++ b/board/sunxi/board.c @@ -46,6 +46,9 @@ #include <spl.h> #include <sy8106a.h> #include <asm/setup.h> +#if defined(CONFIG_LED_STATUS) && defined(CONFIG_SPL_DRIVERS_MISC) +#include <status_led.h> +#endif #if defined CONFIG_VIDEO_LCD_PANEL_I2C && !(defined CONFIG_SPL_BUILD) /* So that we can use pin names in Kconfig and sunxi_name_to_gpio() */ @@ -672,6 +675,10 @@ void sunxi_board_init(void) { int power_failed = 0; +#if defined(CONFIG_LED_STATUS) && defined(CONFIG_SPL_DRIVERS_MISC) + status_led_init(); +#endif + #ifdef CONFIG_SY8106A_POWER power_failed = sy8106a_set_vout1(CONFIG_SY8106A_VOUT1_VOLT); #endif
For some systems, such as the PinePhone, there is no way for the end user to make sure the system is indeed booting before the boot script is executed, which takes several seconds. Therefore, it can be useful to provide early visual feedback as soon as possible. In order achieve this goal, this patch initializes the status LED (if configured) in the SPL. Signed-off-by: Arnaud Ferraris <arnaud.ferraris@collabora.com> --- board/sunxi/board.c | 7 +++++++ 1 file changed, 7 insertions(+)