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

[Takashi Iwai <tiwai@suse.de>]

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