| Message ID | 20180727230554.31027-9-pierre-louis.bossart@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Enable HDA Codec support on Intel Platforms | expand |
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
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@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 >
On Tue, Jul 31, 2018 at 09:06:54AM -0700, Pierre-Louis Bossart wrote: > On 7/31/18 3:48 AM, Takashi Iwai wrote: > > Pierre-Louis Bossart wrote: > > > +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. Debug options like these are really a lot more useful as something runtime selectable with something like a module parameter as Takashi suggests - an innatentive distro maintainer could easily enable it by mistake when going through a bunch of new options in a new kernel release and if someone needs it for debugging purposes in a distro context it's a real pain to have to rebuild the kernel. It's also a bit confusing as the config text says: > > > + 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 which sounds like people should turn it on, the bit about it being a debug option is quite deep in the text.
On 7/31/18 9:20 AM, Mark Brown wrote: > On Tue, Jul 31, 2018 at 09:06:54AM -0700, Pierre-Louis Bossart wrote: >> On 7/31/18 3:48 AM, Takashi Iwai wrote: >>> Pierre-Louis Bossart wrote: > >>>> +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. > > Debug options like these are really a lot more useful as something > runtime selectable with something like a module parameter as Takashi > suggests - an innatentive distro maintainer could easily enable it by > mistake when going through a bunch of new options in a new kernel > release and if someone needs it for debugging purposes in a distro > context it's a real pain to have to rebuild the kernel. Ah yes, I get it now and yes it's probably a better idea to have a kernel parameter. I saw multiple cases of distros such as Arch enabling the baytrail nocodec mode and it's not straightforward to reach all maintainers of all distros. Will fix, thanks for the feedback. > > It's also a bit confusing as the config text says: > >>>> + 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 > > which sounds like people should turn it on, the bit about it being a > debug option is quite deep in the text. >
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). + +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. + config SND_SOC_ACPI_INTEL_MATCH tristate select SND_SOC_ACPI if ACPI diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c index e8bf46cfea45..b14a47e765de 100644 --- a/sound/soc/intel/skylake/skl.c +++ b/sound/soc/intel/skylake/skl.c @@ -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 static int skl_find_machine(struct skl *skl, void *driver_data) { @@ -500,13 +502,16 @@ static int skl_find_machine(struct skl *skl, void *driver_data) struct skl_machine_pdata *pdata; mach = snd_soc_acpi_find_machine(mach); - if (!mach) { + if (!mach || IS_ENABLED(CONFIG_SND_SOC_INTEL_SKYLAKE_FORCE_HDA_DSP)) { dev_dbg(bus->dev, "No matching I2S machine driver found\n"); +#if IS_ENABLED(CONFIG_SND_SOC_INTEL_SKYLAKE_HDA_DSP) mach = skl_find_hda_machine(skl, driver_data); +#endif if (!mach) { dev_err(bus->dev, "No matching machine driver found\n"); return -ENODEV; } + } skl->mach = mach;
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(-)