Re: [alsa-devel] [PATCH v5 8/8] ASoC: Intel: Skylake: add option to control HDAudio + DSP usage

On Sat, 28 Jul 2018 01:05:54 +0200, Pierre-Louis Bossart wrote:

Add two options to explicitly enable HDAudio + DSP usage and force its use

Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com

sound/soc/intel/Kconfig | 19 +++++++++++++++++++ sound/soc/intel/skylake/skl.c | 7 ++++++- 2 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/sound/soc/intel/Kconfig b/sound/soc/intel/Kconfig index 0caa1f4eb94d..93c3693cdf13 100644 --- a/sound/soc/intel/Kconfig +++ b/sound/soc/intel/Kconfig @@ -117,6 +117,25 @@ config SND_SOC_INTEL_SKYLAKE GeminiLake or CannonLake platform with the DSP enabled in the BIOS then enable this option by saying Y or m.

+config SND_SOC_INTEL_SKYLAKE_HDA_DSP

• bool "Enable HDAudio Codec + DSP support"
• depends on SND_SOC_INTEL_SKYLAKE
• help

•  If you have a Intel Skylake+ platform with the DSP enabled in
  

•  the BIOS and an HDAudio codec, then enable this option by saying Y
  

•  This option will only have an effect if there are no ACPI-enumerated
  

•  audio devices (I2C, SoundWire).
  

IMO, the change needed for this config should be folded into the patch 4 "ASoC: Intel: Skylake: use HDAudio if ACPI enumeration fails").

+config SND_SOC_INTEL_SKYLAKE_FORCE_HDA_DSP

• bool "Force HDAudio Codec + DSP support"
• depends on SND_SOC_INTEL_SKYLAKE_HDA_DSP
• help

•  If you have a Intel Skylake+ platform with the DSP enabled in
  

•  the BIOS and an HDAudio codec, then enable this option by saying Y
  

•  This option will ignore information from the BIOS and force the use
  

•  of the HDaudio codec, if present.
  

•  This is a debug option not recommended for distros.
  

... and this one is better to be a module option. Distros tend to enable all possible options, and this may be set unnecessarily. If any, this kconfig should be only toggling the default option value.

Just another nitpicking:

@@ -474,6 +474,7 @@ static struct skl_ssp_clk skl_ssp_clks[] = { {.name = "ssp5_sclkfs"}, };

+#if IS_ENABLED(CONFIG_SND_SOC_INTEL_SKYLAKE_HDA_DSP) static struct snd_soc_acpi_mach *skl_find_hda_machine(struct skl *skl, struct snd_soc_acpi_mach *machines) { @@ -492,6 +493,7 @@ static struct snd_soc_acpi_mach *skl_find_hda_machine(struct skl *skl,

return mach; } +#endif

Defining a dumb skl_find_hda_machine() returning NULL as #else would reduce one more ifdef. Also, CONFIG_SND_SOC_INTEL_SKYLAKE_HDA_DSP is a bool, so it can be a simple ifdef without IS_ENABLED().

#ifdef CONFIG_SND_SOC_INTEL_SKYLAKE_HDA_DSP static struct snd_soc_acpi_mach *skl_find_hda_machine(struct skl *skl, struct snd_soc_acpi_mach *machines) { .... } #else static struct snd_soc_acpi_mach *skl_find_hda_machine(struct skl *skl, struct snd_soc_acpi_mach *machines) { return NULL; } #endif

thanks,

Takashi