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: 4343 bytes --]

Hi,

Le Mon 27 Apr 26, 19:27, Quentin Schulz a écrit :
> 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?

Sounds good to me!

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

Looks good.

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

Yes Andre's proposal sounds fine, I'm all for it.

Thanks for looking at this.

Paul

-- 
Paul Kocialkowski,

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

Expert in multimedia, graphics and embedded hardware support with Linux.

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