On Fri, 17 Nov 2017 00:01:06 +0100, Pierre-Louis Bossart wrote:
On 11/16/2017 04:24 PM, Linus Torvalds wrote:
On Thu, Nov 16, 2017 at 2:16 PM, Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com wrote:
Add 'default m' when sensible
No.
This is not sensible, and is not at all what I suggested.
Actual new code should *never* be default 'm' or 'y'.
You may think it's supremely important because you maintain it, but so does EVERY other developer for their code, and no, we don'[t want to just default everything on.
So the only thing that should be on by default is stuff that either
(a) is a new option for something that used to not have an option at all and was always enabled
IOW, this is a "it used to always be there, we default it on nowthat we've made it optional".
(b) stuff that doesn't actually generate any code, but is there as a choice.
So this is stuff like "show me the config options for vendor XYZ".(c) stuff that really is so common that it is the majority of users.
This is stuff like USB, or block devices, or "enable networking".But some individual driver for some hardware that nobody has? No.
So things like this is pure garbage:
config SND_SST_ATOM_HIFI2_PLATFORM tristate "Intel ASoC SST driver for HiFi2 platforms(*field, *trail)" default m
because clearly "Intel ASoC SST driver for HiFi2 platforms" should not at all default to being on for everybody.
It's only on for people who select X86 and ASoC (which isn't on by default). With make defconfig, none of this is selected and I *thought* making those suboptions default to m would help distros who don't necessarily know the status of each driver, plus what was selected is known to be conflict-free. I guess I will follow the network example and state 'this is recommended'.
This is also not *new* code, what I tried here is to reverse the selection since the existing Kconfigs selected a target and inferred what SOC options are needed, and it needs to be the other way around to share machine drivers between SST and SOF drivers. Not to mention that there is no reason for machine driver configurations to be dependent on SOC-level configs like LPSS, this should be handled elsewhere.
I will move the TOPLEVEL config to follow what you suggested with the network example, this config does not have any impact on the code. will send a v2 tomorrow. If you want to send me your config i'll be happy to check the changes are ok. Thanks.
Well, think of creating a config from scratch on the tree with your patch and user just selects the default choice. What will happen? Then ASoC Intel drivers appear there. That should be avoided.
Also another problem is that the helper modules are selected by CONFIG_SND_SOC_INTEL_TOPLEVEL. With default on, these modules are also enabled unconditionally. That is, even if the board driver's default is off, you'll still get these modules with the default choice.
Lasts but not least, default=m looks definitely strange. Distros set CONFIG_SND=m, so it'll be module in anyway. That is, you don't have to care about modularity in that level at all.
A usual pattern is something like:
config SOME_TOPLEVEL bool "some toplevel gateway config" default y help ....
config BACKEND_A tristate select MORE_STUFF
config HELPER_DRIVERS tristate select ANOTHER_STUFF ....
if SOME_TOPLEVEL
config DRIVER_A tristate "driver a" depends on PLATFORM_A select BACKEND_A select HELPER_DRIVERS help ... config DRIVER_B tristate "driver b" depends on SOMETHING_ELSE select BACKEND_B select HELPER_DRIVERS help ...
....
endif
Here we set the top-level config default=y, so that you'll go into the board driver selection as default. If you know that you don't need it and are annoyed to deselect all, you can filter out at this point. That's the *only* meaning of the toplevel config: just filtering. (In the above it's bool, but it can be a tristate, too; but the purpose is still only for filtering.)
The rest drivers have no default, thus turned off. So the default are without any drivers. And note that CONFIG_SOME_TOPLEVEL doesn't select any others. Thus it works nothing but a gatekeeper.
Takashi