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

[Paul Kocialkowski <paulk@sys-base.io>]

[-- Attachment #1: Type: text/plain, Size: 2430 bytes --]

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.

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

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

What do you think?

All the best,

Paul

>  
>  	gpio_request(gpio, "gpio_led");
> -- 
> 2.46.4
> 

-- 
Paul Kocialkowski,

Free software developer - https://www.paulk.fr/
Independent contractor - sys-base - https://www.sys-base.io/

Contributor to fully free software support for selected hardware.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]