On Sat, Oct 17, 2009 at 07:58:47AM -0600, Myles Watson wrote:
On Sat, Oct 17, 2009 at 7:47 AM, Uwe Hermann uwe@hermann-uwe.de wrote:
On Sat, Oct 17, 2009 at 03:25:07PM +0200, svn@coreboot.org 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.