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

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

Hi Quentin, Paul,

ah, it seems like LEDs are the "embedded bike shed", let me add my shade 
of blue ;-)

On 4/21/26 19:12, Quentin Schulz wrote:
> 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.

So what about:
config CONFIG_SPL_SUNXI_LED_STATUS_ACTIVE_HIGH
	default y
then?

> 
>> 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?

I came up with:
Whether the GPIO of the status LED must be set high or low to turn the 
LED on.
Thoughts?

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

Yes, indeed CONFIG_LED_STATUS_GPIO makes much more sense.

Let me know what you think.

Cheers,
Andre


> 
> Cheers,
> Quentin