kconfig options

[Uwe Hermann]

On Sat, Oct 17, 2009 at 03:25:07PM +0200, [e-mail] wrote:
> Remove CONFIG_ from #defines that aren't config variables.  Trivial.

Why this? Shouldn't they have been added to src/Kconfig (for example)
instead?
> Modified: trunk/coreboot-v2/src/arch/i386/init/car.S
>
> --- trunk/coreboot-v2/src/arch/i386/init/car.S	2009-10-17 03:00:04 UTC (rev 4801)
> +++ trunk/coreboot-v2/src/arch/i386/init/car.S	2009-10-17 13:25:07 UTC (rev 4802)
> @@ -269,7 +269,7 @@
>
> /* TODO: make this a config variable */
> -#if CONFIG_CARTEST
> +#if CARTEST

This one, for example, should likely be an option in the "Debugging"
menu, maybe only visible if EXPERT is enabled (will post EXPERT patch
soon).

I personally think that _all_ options should be kconfig options (i.e.
listed in a Kconfig file with some default). Whether or not they should
also be visible in menuconfig is another issue and depends on the
option, though.

Uwe.

[Myles Watson]

On Sat, Oct 17, 2009 at 7:47 AM, Uwe Hermann  wrote:
> On Sat, Oct 17, 2009 at 03:25:07PM +0200, [e-mail] wrote:
> > Remove CONFIG_ from #defines that aren't config variables.  Trivial.
>
> Why this? Shouldn't they have been added to src/Kconfig (for example)
> instead?

No.  Many of them are only mentioned in one place.  They probably should be
removed completely.  I think it will improve the quality of Coreboot to
limit configuration options to ones that actually do something.
> > Modified: trunk/coreboot-v2/src/arch/i386/init/car.S
> >
> > --- trunk/coreboot-v2/src/arch/i386/init/car.S        2009-10-17 03:00:04
>
> UTC (rev 4801)
> > +++ trunk/coreboot-v2/src/arch/i386/init/car.S        2009-10-17 13:25:07
>
> UTC (rev 4802)
> > @@ -269,7 +269,7 @@
> >   /* TODO: make this a config variable */
> >
> > -#if CONFIG_CARTEST
> > +#if CARTEST
>
> This one, for example, should likely be an option in the "Debugging"
> menu, maybe only visible if EXPERT is enabled (will post EXPERT patch
> soon).

I think there's too little gain for something this trivial.

I personally think that _all_ options should be kconfig options (i.e.
> listed in a Kconfig file with some default). Whether or not they should
> also be visible in menuconfig is another issue and depends on the
> option, though.

These aren't really options.  They really are just #ifdefs.  They were never
in Options.lb.

Thanks,
Myles

[Uwe Hermann]

On Sat, Oct 17, 2009 at 07:58:47AM -0600, Myles Watson wrote:
> On Sat, Oct 17, 2009 at 7:47 AM, Uwe Hermann  wrote:
> > On Sat, Oct 17, 2009 at 03:25:07PM +0200, [e-mail] wrote:
> > > Remove CONFIG_ from #defines that aren't config variables.  Trivial.
> >
> > Why this? Shouldn't they have been added to src/Kconfig (for example)
> > instead?
>
> No.  Many of them are only mentioned in one place.  They probably should be
> removed completely.

They should be removed if they are useless. But if they are useful they
should stay and be turned into kconfig options instead. I think
CONFIG_CARTEST for example is useful to have.
> > > Modified: trunk/coreboot-v2/src/arch/i386/init/car.S
> > >
> > > --- trunk/coreboot-v2/src/arch/i386/init/car.S        2009-10-17 03:00:04
> >
> > UTC (rev 4801)
> > > +++ trunk/coreboot-v2/src/arch/i386/init/car.S        2009-10-17 13:25:07
> >
> > UTC (rev 4802)
> > > @@ -269,7 +269,7 @@
> > >   /* TODO: make this a config variable */
> > >
> > > -#if CONFIG_CARTEST
> > > +#if CARTEST
> >
> > This one, for example, should likely be an option in the "Debugging"
> > menu, maybe only visible if EXPERT is enabled (will post EXPERT patch
> > soon).
>
> I think there's too little gain for something this trivial.

Someone did obviously use it to enable/disable some CAR test stuff in
the past, just that they defined CARTEST locally/temporarily somewhere
and the #define was not committed (at least that's how it looks like).

I do think we should definately make all "DEBUG" and "ENABLE_FOO"
#defines which are not yet oldconfig or kconfig options into actual
options. There are lots of them spread over tons of files, having them
all accessible in the "Debugging" menu (or at least in a Kconfig file
if they should not be user-visible) is a major improvement.
> I personally think that _all_ options should be kconfig options (i.e.
> > listed in a Kconfig file with some default). Whether or not they should
> > also be visible in menuconfig is another issue and depends on the
> > option, though.
>
> These aren't really options.  They really are just #ifdefs.  They were never
> in Options.lb.

Yes. My point is that they should become options. They should have been
in Options.lb from the beginning.

Will post patches for discussion later if nobody beats me to it.

Uwe.