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

[Arnaud Ferraris <arnaud.ferraris@collabora.com>]

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