Re: [alsa-devel] [PATCH] ASoC: Intel: Add defaults for SND_SST_ options and machine drivers

[Takashi Iwai <tiwai@suse.de>]

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 now
> > that 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