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

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

Hi Andre,

On 4/27/26 3:07 PM, Andre Przywara wrote:
> 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.
> 

CONFIG_SPL_SUNXI_LED_STATUS_GPIO I'm assuming?

> Let me know what you think.
> 

Ack to all but I also don't care too much about Allwinner. @Paul?

Cheers,
Quentin