[PATCH] ASoC: Intel: Add defaults for SND_SST_ options and machine drivers

[PATCH] ASoC: Intel: Add defaults for SND_SST_ options and machine drivers

[Pierre-Louis Bossart]

Add 'default m' when sensible, move common dependencies in if/endif block
and clarify which options default to n - distro configurations
should not enable legacy Baytrail stuff and NOCODEC (test only)

Basic tests run with defconfig, oldconfig, olddefconfig, there should be
no functionality change.

Fixes: f6a118a800e3 ("ASoC: Intel: clarify Kconfig dependencies")
Reported-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 sound/soc/intel/Kconfig        |  5 ++++
 sound/soc/intel/boards/Kconfig | 60 ++++++++++++++++++++++++++----------------
 2 files changed, 43 insertions(+), 22 deletions(-)

diff --git a/sound/soc/intel/Kconfig b/sound/soc/intel/Kconfig
index 7b49d04e3c60..b40719396e9d 100644
--- a/sound/soc/intel/Kconfig
+++ b/sound/soc/intel/Kconfig
@@ -31,6 +31,7 @@ config SND_SOC_ACPI_INTEL_MATCH
 
 config SND_SOC_INTEL_SST_TOPLEVEL
 	tristate "Intel ASoC SST drivers"
+	default m
 	depends on X86 || COMPILE_TEST
 	select SND_SOC_INTEL_MACH
 	select SND_SOC_INTEL_COMMON
@@ -42,6 +43,7 @@ config SND_SOC_INTEL_SST_TOPLEVEL
 
 config SND_SOC_INTEL_HASWELL
 	tristate "Intel ASoC SST driver for Haswell/Broadwell"
+	default m
 	depends on SND_SOC_INTEL_SST_TOPLEVEL && SND_DMA_SGBUF
 	depends on DMADEVICES
 	select SND_SOC_INTEL_SST
@@ -49,6 +51,7 @@ config SND_SOC_INTEL_HASWELL
 
 config SND_SOC_INTEL_BAYTRAIL
 	tristate "Intel ASoC SST driver for Baytrail (legacy)"
+	default n
 	depends on SND_SOC_INTEL_SST_TOPLEVEL
 	depends on DMADEVICES
 	select SND_SOC_INTEL_SST
@@ -56,11 +59,13 @@ config SND_SOC_INTEL_BAYTRAIL
 
 config SND_SST_ATOM_HIFI2_PLATFORM
 	tristate "Intel ASoC SST driver for HiFi2 platforms (*field, *trail)"
+	default m
 	depends on SND_SOC_INTEL_SST_TOPLEVEL && X86
 	select SND_SOC_COMPRESS
 
 config SND_SOC_INTEL_SKYLAKE
 	tristate "Intel ASoC SST driver for SKL/BXT/KBL/GLK/CNL"
+	default m
 	depends on SND_SOC_INTEL_SST_TOPLEVEL && PCI && ACPI
 	select SND_HDA_EXT_CORE
 	select SND_HDA_DSP_LOADER
diff --git a/sound/soc/intel/boards/Kconfig b/sound/soc/intel/boards/Kconfig
index 6f754708a48c..6069328c5c28 100644
--- a/sound/soc/intel/boards/Kconfig
+++ b/sound/soc/intel/boards/Kconfig
@@ -17,10 +17,12 @@ config SND_MFLD_MACHINE
           Say Y if you have such a device.
           If unsure select "N".
 
+if SND_SOC_INTEL_HASWELL
+
 config SND_SOC_INTEL_HASWELL_MACH
 	tristate "ASoC Audio DSP support for Intel Haswell Lynxpoint"
+	default m
 	depends on X86_INTEL_LPSS && I2C && I2C_DESIGNWARE_PLATFORM
-	depends on SND_SOC_INTEL_HASWELL
 	select SND_SOC_RT5640
 	help
 	  This adds support for the Lynxpoint Audio DSP on Intel(R) Haswell
@@ -30,8 +32,8 @@ config SND_SOC_INTEL_HASWELL_MACH
 
 config SND_SOC_INTEL_BDW_RT5677_MACH
 	tristate "ASoC Audio driver for Intel Broadwell with RT5677 codec"
+	default m
 	depends on X86_INTEL_LPSS && GPIOLIB && I2C
-	depends on SND_SOC_INTEL_HASWELL
 	select SND_SOC_RT5677
 	help
 	  This adds support for Intel Broadwell platform based boards with
@@ -39,41 +41,50 @@ config SND_SOC_INTEL_BDW_RT5677_MACH
 
 config SND_SOC_INTEL_BROADWELL_MACH
 	tristate "ASoC Audio DSP support for Intel Broadwell Wildcatpoint"
+	default m
 	depends on X86_INTEL_LPSS && I2C && I2C_DESIGNWARE_PLATFORM
-	depends on SND_SOC_INTEL_HASWELL
 	select SND_SOC_RT286
 	help
 	  This adds support for the Wilcatpoint Audio DSP on Intel(R) Broadwell
 	  Ultrabook platforms.
 	  Say Y if you have such a device.
 	  If unsure select "N".
+endif
+
+if SND_SOC_INTEL_BAYTRAIL
 
 config SND_SOC_INTEL_BYT_MAX98090_MACH
 	tristate "ASoC Audio driver for Intel Baytrail with MAX98090 codec"
+	default n
 	depends on X86_INTEL_LPSS && I2C
 	depends on SND_SST_IPC_ACPI = n
-	depends on SND_SOC_INTEL_BAYTRAIL
 	select SND_SOC_MAX98090
 	help
 	  This adds audio driver for Intel Baytrail platform based boards
-	  with the MAX98090 audio codec.
+	  with the MAX98090 audio codec. This driver is deprecated, use
+	  SND_SOC_INTEL_CHT_BSW_MAX98090_TI_MACH instead for better
+	  functionality.
 
 config SND_SOC_INTEL_BYT_RT5640_MACH
 	tristate "ASoC Audio driver for Intel Baytrail with RT5640 codec"
+	default n
 	depends on X86_INTEL_LPSS && I2C
 	depends on SND_SST_IPC_ACPI = n
-	depends on SND_SOC_INTEL_BAYTRAIL
 	select SND_SOC_RT5640
 	help
 	  This adds audio driver for Intel Baytrail platform based boards
 	  with the RT5640 audio codec. This driver is deprecated, use
 	  SND_SOC_INTEL_BYTCR_RT5640_MACH instead for better functionality.
 
+endif
+
+if SND_SST_ATOM_HIFI2_PLATFORM
+
 config SND_SOC_INTEL_BYTCR_RT5640_MACH
         tristate "ASoC Audio driver for Intel Baytrail and Baytrail-CR with RT5640 codec"
+	default m
 	depends on X86 && I2C && ACPI
 	select SND_SOC_RT5640
-	depends on SND_SST_ATOM_HIFI2_PLATFORM
 	select SND_SST_IPC_ACPI
 	help
           This adds support for ASoC machine driver for Intel(R) Baytrail and Baytrail-CR
@@ -83,9 +94,9 @@ config SND_SOC_INTEL_BYTCR_RT5640_MACH
 
 config SND_SOC_INTEL_BYTCR_RT5651_MACH
         tristate "ASoC Audio driver for Intel Baytrail and Baytrail-CR with RT5651 codec"
+	default m
 	depends on X86 && I2C && ACPI
 	select SND_SOC_RT5651
-	depends on SND_SST_ATOM_HIFI2_PLATFORM
 	select SND_SST_IPC_ACPI
 	help
           This adds support for ASoC machine driver for Intel(R) Baytrail and Baytrail-CR
@@ -95,9 +106,9 @@ config SND_SOC_INTEL_BYTCR_RT5651_MACH
 
 config SND_SOC_INTEL_CHT_BSW_RT5672_MACH
         tristate "ASoC Audio driver for Intel Cherrytrail & Braswell with RT5672 codec"
+	default m
 	depends on X86_INTEL_LPSS && I2C && ACPI
         select SND_SOC_RT5670
-        depends on SND_SST_ATOM_HIFI2_PLATFORM
         select SND_SST_IPC_ACPI
         help
           This adds support for ASoC machine driver for Intel(R) Cherrytrail & Braswell
@@ -107,9 +118,9 @@ config SND_SOC_INTEL_CHT_BSW_RT5672_MACH
 
 config SND_SOC_INTEL_CHT_BSW_RT5645_MACH
 	tristate "ASoC Audio driver for Intel Cherrytrail & Braswell with RT5645/5650 codec"
+	default m
 	depends on X86_INTEL_LPSS && I2C && ACPI
 	select SND_SOC_RT5645
-	depends on SND_SST_ATOM_HIFI2_PLATFORM
 	select SND_SST_IPC_ACPI
 	help
 	  This adds support for ASoC machine driver for Intel(R) Cherrytrail & Braswell
@@ -118,10 +129,10 @@ config SND_SOC_INTEL_CHT_BSW_RT5645_MACH
 
 config SND_SOC_INTEL_CHT_BSW_MAX98090_TI_MACH
 	tristate "ASoC Audio driver for Intel Cherrytrail & Braswell with MAX98090 & TI codec"
+	default m
 	depends on X86_INTEL_LPSS && I2C && ACPI
 	select SND_SOC_MAX98090
 	select SND_SOC_TS3A227E
-	depends on SND_SST_ATOM_HIFI2_PLATFORM
 	select SND_SST_IPC_ACPI
 	help
 	  This adds support for ASoC machine driver for Intel(R) Cherrytrail & Braswell
@@ -130,9 +141,9 @@ config SND_SOC_INTEL_CHT_BSW_MAX98090_TI_MACH
 
 config SND_SOC_INTEL_BYT_CHT_DA7213_MACH
 	tristate "ASoC Audio driver for Intel Baytrail & Cherrytrail with DA7212/7213 codec"
+	default m
 	depends on X86_INTEL_LPSS && I2C && ACPI
 	select SND_SOC_DA7213
-	depends on SND_SST_ATOM_HIFI2_PLATFORM
 	select SND_SST_IPC_ACPI
 	help
 	  This adds support for ASoC machine driver for Intel(R) Baytrail & CherryTrail
@@ -141,9 +152,9 @@ config SND_SOC_INTEL_BYT_CHT_DA7213_MACH
 
 config SND_SOC_INTEL_BYT_CHT_ES8316_MACH
 	tristate "ASoC Audio driver for Intel Baytrail & Cherrytrail with ES8316 codec"
+	default m
 	depends on X86_INTEL_LPSS && I2C && ACPI
 	select SND_SOC_ES8316
-	depends on SND_SST_ATOM_HIFI2_PLATFORM
 	select SND_SST_IPC_ACPI
 	help
 	  This adds support for ASoC machine driver for Intel(R) Baytrail &
@@ -152,19 +163,23 @@ config SND_SOC_INTEL_BYT_CHT_ES8316_MACH
 
 config SND_SOC_INTEL_BYT_CHT_NOCODEC_MACH
 	tristate "ASoC Audio driver for Intel Baytrail & Cherrytrail platform with no codec (MinnowBoard MAX, Up)"
+	default n
 	depends on X86_INTEL_LPSS && I2C && ACPI
-	depends on SND_SST_ATOM_HIFI2_PLATFORM
 	select SND_SST_IPC_ACPI
 	help
 	  This adds support for ASoC machine driver for the MinnowBoard Max or
 	  Up boards and provides access to I2S signals on the Low-Speed
-	  connector
+	  connector. It is not intended to be enabled by distros by default.
 	  If unsure select "N".
 
+endif
+
+if SND_SOC_INTEL_SKYLAKE
+
 config SND_SOC_INTEL_SKL_RT286_MACH
 	tristate "ASoC Audio driver for SKL with RT286 I2S mode"
+	default m
 	depends on X86 && ACPI && I2C
-	depends on SND_SOC_INTEL_SKYLAKE
 	select SND_SOC_RT286
 	select SND_SOC_DMIC
 	select SND_SOC_HDAC_HDMI
@@ -176,8 +191,8 @@ config SND_SOC_INTEL_SKL_RT286_MACH
 
 config SND_SOC_INTEL_SKL_NAU88L25_SSM4567_MACH
 	tristate "ASoC Audio driver for SKL with NAU88L25 and SSM4567 in I2S Mode"
+	default m
 	depends on X86_INTEL_LPSS && I2C
-	depends on SND_SOC_INTEL_SKYLAKE
 	select SND_SOC_NAU8825
 	select SND_SOC_SSM4567
 	select SND_SOC_DMIC
@@ -190,8 +205,8 @@ config SND_SOC_INTEL_SKL_NAU88L25_SSM4567_MACH
 
 config SND_SOC_INTEL_SKL_NAU88L25_MAX98357A_MACH
 	tristate "ASoC Audio driver for SKL with NAU88L25 and MAX98357A in I2S Mode"
+	default m
 	depends on X86_INTEL_LPSS && I2C
-	depends on SND_SOC_INTEL_SKYLAKE
 	select SND_SOC_NAU8825
 	select SND_SOC_MAX98357A
 	select SND_SOC_DMIC
@@ -204,8 +219,8 @@ config SND_SOC_INTEL_SKL_NAU88L25_MAX98357A_MACH
 
 config SND_SOC_INTEL_BXT_DA7219_MAX98357A_MACH
 	tristate "ASoC Audio driver for Broxton with DA7219 and MAX98357A in I2S Mode"
+	default m
 	depends on X86 && ACPI && I2C
-	depends on SND_SOC_INTEL_SKYLAKE
 	select SND_SOC_DA7219
 	select SND_SOC_MAX98357A
 	select SND_SOC_DMIC
@@ -219,8 +234,8 @@ config SND_SOC_INTEL_BXT_DA7219_MAX98357A_MACH
 
 config SND_SOC_INTEL_BXT_RT298_MACH
 	tristate "ASoC Audio driver for Broxton with RT298 I2S mode"
+	default m
 	depends on X86 && ACPI && I2C
-	depends on SND_SOC_INTEL_SKYLAKE
 	select SND_SOC_RT298
 	select SND_SOC_DMIC
 	select SND_SOC_HDAC_HDMI
@@ -233,9 +248,9 @@ config SND_SOC_INTEL_BXT_RT298_MACH
 
 config SND_SOC_INTEL_KBL_RT5663_MAX98927_MACH
 	tristate "ASoC Audio driver for KBL with RT5663 and MAX98927 in I2S Mode"
+	default m
 	depends on X86_INTEL_LPSS && I2C
 	select SND_SOC_INTEL_SST
-	depends on SND_SOC_INTEL_SKYLAKE
 	select SND_SOC_RT5663
 	select SND_SOC_MAX98927
 	select SND_SOC_DMIC
@@ -248,9 +263,9 @@ config SND_SOC_INTEL_KBL_RT5663_MAX98927_MACH
 
 config SND_SOC_INTEL_KBL_RT5663_RT5514_MAX98927_MACH
         tristate "ASoC Audio driver for KBL with RT5663, RT5514 and MAX98927 in I2S Mode"
+	default m
         depends on X86_INTEL_LPSS && I2C && SPI
         select SND_SOC_INTEL_SST
-        depends on SND_SOC_INTEL_SKYLAKE
         select SND_SOC_RT5663
         select SND_SOC_RT5514
         select SND_SOC_RT5514_SPI
@@ -261,5 +276,6 @@ config SND_SOC_INTEL_KBL_RT5663_RT5514_MAX98927_MACH
           create an alsa sound card for RT5663 + RT5514 + MAX98927.
           Say Y if you have such a device.
           If unsure select "N".
+endif
 
 endif
-- 
2.14.1

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

[Linus Torvalds]

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.

So NAK NAK NAK on stuff like this.

                    Linus

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

[Pierre-Louis Bossart]

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.

>
> So NAK NAK NAK on stuff like this.
>
>                      Linus
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

[Takashi Iwai]

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