Re: [PATCH 1/3] sunxi: spl: fix SPL_SUNXI_LED active low configuration

[Quentin Schulz <quentin.schulz@cherry.de>]

Hi Paul,

On 4/9/26 5:56 PM, Paul Kocialkowski wrote:
> Hi,
> 
> On Wed 08 Apr 26, 00:34, Andre Przywara wrote:
>> The newly introduced Allwinner SPL LED "framework" defined a
>> SPL_SUNXI_LED_STATUS_STATE Kconfig symbol, that was supposed to denote
>> the active-low vs. active-high polarity of the LED. However this is
>> a bool symbol, so it will simply vanish if not defined, and we cannot use
>> it directly inside a C statement.
>>
>> Filter the symbol through the IS_ENABLED() macro, which will return 0 if
>> the symbol is not defined, which is the intended value here.
>>
>> This fixes configuring LEDs with active-low polarity.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>> ---
>>   board/sunxi/board.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/board/sunxi/board.c b/board/sunxi/board.c
>> index d7722d1858a..80dcae9c1a4 100644
>> --- a/board/sunxi/board.c
>> +++ b/board/sunxi/board.c
>> @@ -563,7 +563,7 @@ static void sunxi_spl_store_dram_size(phys_addr_t dram_size)
>>   static void status_led_init(void)
>>   {
>>   #if CONFIG_IS_ENABLED(SUNXI_LED_STATUS)
>> -	unsigned int state = CONFIG_SPL_SUNXI_LED_STATUS_STATE;
>> +	unsigned int state = IS_ENABLED(CONFIG_SPL_SUNXI_LED_STATUS_STATE);
> 
> Sorry I didn't react to the initial submission, but it feels like the
> CONFIG_SPL_SUNXI_LED_STATUS_STATE symbol really means active-high if enabled
> and active-low if disabled. The name would suggest that it's an int with a value
> of either 0 or 1 instead.
> 

Yeah, I lazily renamed the old CONFIG_LED_STATUS_STATE which used to be 
an int range (0..2) and made it both specific for Allwinner as well as 
changing it into a bool.

> I think it would be less confusing to call the symbol
> CONFIG_SPL_SUNXI_LED_STATUS_ACTIVE_LOW and reverse its meaning, so that we can
> spare defining it in most configs (that will be active-high).

You can also have
default y
in your symbol to not have to reverse the meaning.

> Also the description currently mentions "initial state" which may be confusing
> as it could refer to the state inherited after reset (e.g. due to some pull
> resistor) or the state we do set in the SPL.
> 

It was the prompt for the now removed LED_STATUS_STATE. What are you 
suggesting instead?

>>   	unsigned int gpio = CONFIG_SPL_SUNXI_LED_STATUS_BIT;
> 
> And while at it I would rename this to something like:
> CONFIG_SPL_SUNXI_LED_STATUS_GPIO since it indicates the GPIO number, not a
> specific bit in a sunxi-specific kind of register.
> 

Also a remnant of LED_STATUS_BIT* symbols. Either work for me.

Cheers,
Quentin