On Tue, 7 Sep 2021 13:41:11 +0200 Arnaud Ferraris <[email protected]> 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 <[email protected]> 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
