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.ferra...@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