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

[Pierre-Louis Bossart]

On 7/31/18 3:48 AM, Takashi Iwai wrote:
> 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 at 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").

Yes, agreed.
> 
> 
>> +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.

Sorry, I don't get this one. this wouldn't change anything between 
built-in or module, it's just a yes/no answer to ignore ACPI stuff. the 
default is also no and there is a clear statement not to include it 
except for debug.
We have a similar option for SOF to bypass all ACPI information and go 
straight to 'nocodec' mode.

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

yes, thanks for the suggestion.

> 	
> 
> thanks,
> 
> Takashi
>