[PATCH 02/31] kconfig: Add support for conditional values

[Simon Glass]

Hi Tom,

On Wed, 12 Jan 2022 at 14:56, Tom Rini <trini at konsulko.com> wrote:
>
> On Wed, Jan 12, 2022 at 02:28:21PM -0700, Simon Glass wrote:
> > Hi Ilias,
> >
> > On Mon, 1 Nov 2021 at 01:05, Ilias Apalodimas
> > <ilias.apalodimas at linaro.org> wrote:
> > >
> > > On Mon, 1 Nov 2021 at 03:19, Simon Glass <sjg at chromium.org> wrote:
> > > >
> > > > At present if an optional Kconfig value needs to be used it must be
> > > > bracketed by #ifdef. For example, with this Kconfig setup:
> > > >
> > > > config WIBBLE
> > > >         bool "Support wibbles, the world needs more wibbles"
> > > >
> > > > config WIBBLE_ADDR
> > > >         hex "Address of the wibble"
> > > >         depends on WIBBLE
> > > >
> > > > then the following code must be used:
> > > >
> > > >  #ifdef CONFIG_WIBBLE
> > > >  static void handle_wibble(void)
> > > >  {
> > > >         int val = CONFIG_WIBBLE_ADDR;
> > > >
> > > >         ...
> > > >  }
> > > >  #endif
> > > >
> > >
> > > The example here might be a bit off and we might need this for int
> > > related values. Was this function handle_wibble() supposed to return
> > > an int or not?  We could shield the linker easier here without adding
> > > macros. Something along the lines of
> > > static void handle_wibble(void)
> > > {
> > > #ifdef CONFIG_WIBBLE
> > > int val = CONFIG_WIBBLE_ADDR;
> > > #endif
> > > }
> > >
> > > In that case you don't an extra ifdef to call handle_wibble().
> > > Personally I find this easier to read.
> >
> > But how does that help with the problem here? I am trying to avoid
> > using preprocessor macros in this case.
>
> I'm not sure I see a problem here.  A number of the finish-converting-X
> that I did recently had a guard symbol first because usage wasn't fully
> converted but really everyone using that area of code needed to set the
> value, or use the default.
>
> There might be some cases where we do still need a guard symbol because
> usage is in common code and maybe shouldn't be, but instead moved to
> other usage-specific files.
>
> I also think I've seen cases where doing:
> if (CONFIG_EVALUATES_TO_ZERO) {
>   ...
> }
>
> takes more space in the binary than an #ifdef does.

I'd like to see that.

>
> And finally for the moment, we also have many cases where zero is a
> valid value.  That's what leads to potentially harder to read code or
> needing a guard, I think.

The specific case where this is (to be) used is here:

https://patchwork.ozlabs.org/project/uboot/patch/20211101011734.1614781-14-sjg@chromium.org/

addr = IF_ENABLED_INT(CONFIG_BLOBLIST_FIXED, CONFIG_BLOBLIST_ADDR);

This is because BLOBLIST_ADDR depends on BLOBLIST_FIXED (it is
meaningless to have an address if the bloblits is allocated).

One fix is to always have an address, and set it to 0 by default (and
not use it) when BLOBLIST_FIXED is not enabled.

But it does lead to a strange Kconfig since options are present which
are not really used.

Another option is to add an accessor to the header file, as is down
with global_data (e.g. as is done with gd_of_root()).

Thoughts?

Regards,
Simon