Re: [PATCH 1/2] board: sunxi: enable status LED early

[Andre Przywara <andre.przywara@arm.com>]

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