diff mbox series

[2/2] ASoC: intel: fix soundwire dependencies

Message ID 20210112203250.2576775-2-arnd@kernel.org (mailing list archive)
State New, archived
Headers show
Series [1/2,v2] ALSA: hda: fix SND_INTEL_DSP_CONFIG dependency | expand

Commit Message

Arnd Bergmann Jan. 12, 2021, 8:32 p.m. UTC
From: Arnd Bergmann <arnd@arndb.de>

The Kconfig logic around SND_SOC_SOF_INTEL_SOUNDWIRE tries to
ensure that all sound modules can be built with the minimal
dependencies, but this fails in some configurations:

x86_64-linux-ld: sound/hda/intel-dsp-config.o: in function `snd_intel_dsp_driver_probe':
intel-dsp-config.c:(.text+0x134): undefined reference to `sdw_intel_acpi_scan'

Specifically, this happens if the dsp-config driver is built-in but does
not need to use soundwire, while CONFIG_SOUNDWIRE_INTEL is enabled as
a loadable module.

An easier and more correct way to do this is to remove
CONFIG_SND_SOC_SOF_INTEL_SOUNDWIRE_LINK and instead have
the two drivers that can link against SOUNDWIRE_INTEL,
i.e. DSP_CONFIG and SND_SOC_SOF_HDA, select that driver whenever
SND_SOC_SOF_INTEL_SOUNDWIRE_LINK is set.

This however means that SND_SOC_SOF_INTEL_SOUNDWIRE cannot be selected
by users when SOUNDWIRE is only usable by loadable modules and one or
more drivers using SND_SOC_SOF_INTEL_SOUNDWIRE_LINK_BASELINE is built-in.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 sound/hda/Kconfig           |  1 +
 sound/soc/sof/intel/Kconfig | 16 ++++++----------
 2 files changed, 7 insertions(+), 10 deletions(-)

Comments

Pierre-Louis Bossart Jan. 12, 2021, 9:03 p.m. UTC | #1
On 1/12/21 2:32 PM, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> The Kconfig logic around SND_SOC_SOF_INTEL_SOUNDWIRE tries to
> ensure that all sound modules can be built with the minimal
> dependencies, but this fails in some configurations:
> 
> x86_64-linux-ld: sound/hda/intel-dsp-config.o: in function `snd_intel_dsp_driver_probe':
> intel-dsp-config.c:(.text+0x134): undefined reference to `sdw_intel_acpi_scan'
> 
> Specifically, this happens if the dsp-config driver is built-in but does
> not need to use soundwire, while CONFIG_SOUNDWIRE_INTEL is enabled as
> a loadable module.
> 
> An easier and more correct way to do this is to remove
> CONFIG_SND_SOC_SOF_INTEL_SOUNDWIRE_LINK and instead have
> the two drivers that can link against SOUNDWIRE_INTEL,
> i.e. DSP_CONFIG and SND_SOC_SOF_HDA, select that driver whenever
> SND_SOC_SOF_INTEL_SOUNDWIRE_LINK is set.
> 
> This however means that SND_SOC_SOF_INTEL_SOUNDWIRE cannot be selected
> by users when SOUNDWIRE is only usable by loadable modules and one or
> more drivers using SND_SOC_SOF_INTEL_SOUNDWIRE_LINK_BASELINE is built-in.

The problem is real, but the proposal isn't completely right, there is 
absolutely no logical link or functional dependency between 
INTEL_DSP_CONFIG and SOUNDWIRE.

We have a similar case with HDaudio, the two buses can be selected as 
tristates, but the SOF configuration needs to match.

In both cases, either we add a 'depends' and users need to make sure the 
configurations match on the two sides. Or we use select but one of the 
selections will be overridden and becomes meaningless.

Arnd, do you mind if I give it a try on my side?

> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>   sound/hda/Kconfig           |  1 +
>   sound/soc/sof/intel/Kconfig | 16 ++++++----------
>   2 files changed, 7 insertions(+), 10 deletions(-)
> 
> diff --git a/sound/hda/Kconfig b/sound/hda/Kconfig
> index 3bc9224d5e4f..ecab814d7698 100644
> --- a/sound/hda/Kconfig
> +++ b/sound/hda/Kconfig
> @@ -44,5 +44,6 @@ config SND_INTEL_NHLT
>   config SND_INTEL_DSP_CONFIG
>   	tristate
>   	select SND_INTEL_NHLT if ACPI
> +	select SOUNDWIRE_INTEL if SND_SOC_SOF_INTEL_SOUNDWIRE
>   	# this config should be selected only for Intel DSP platforms.
>   	# A fallback is provided so that the code compiles in all cases.
> diff --git a/sound/soc/sof/intel/Kconfig b/sound/soc/sof/intel/Kconfig
> index 67365ce0d86d..df8f9760870e 100644
> --- a/sound/soc/sof/intel/Kconfig
> +++ b/sound/soc/sof/intel/Kconfig
> @@ -330,13 +330,17 @@ config SND_SOC_SOF_HDA
>   	tristate
>   	select SND_HDA_EXT_CORE if SND_SOC_SOF_HDA_LINK
>   	select SND_SOC_HDAC_HDA if SND_SOC_SOF_HDA_AUDIO_CODEC
> +	select SOUNDWIRE_INTEL if SND_SOC_SOF_INTEL_SOUNDWIRE
>   	help
>   	  This option is not user-selectable but automagically handled by
>   	  'select' statements at a higher level.
>   
> -config SND_SOC_SOF_INTEL_SOUNDWIRE_LINK
> +config SND_SOC_SOF_INTEL_SOUNDWIRE
>   	bool "SOF support for SoundWire"
> -	depends on SOUNDWIRE && ACPI
> +	depends on SND_SOC_SOF_INTEL_SOUNDWIRE_LINK_BASELINE
> +	depends on SOUNDWIRE
> +	depends on ACPI
> +	depends on !(SOUNDWIRE=m && SND_SOC_SOF_INTEL_SOUNDWIRE_LINK_BASELINE=y)
>   	help
>   	  This adds support for SoundWire with Sound Open Firmware
>   	  for Intel(R) platforms.
> @@ -345,14 +349,6 @@ config SND_SOC_SOF_INTEL_SOUNDWIRE_LINK
>   
>   config SND_SOC_SOF_INTEL_SOUNDWIRE_LINK_BASELINE
>   	tristate
> -	select SND_SOC_SOF_INTEL_SOUNDWIRE if SND_SOC_SOF_INTEL_SOUNDWIRE_LINK
> -	help
> -	  This option is not user-selectable but automagically handled by
> -	  'select' statements at a higher level.
> -
> -config SND_SOC_SOF_INTEL_SOUNDWIRE
> -	tristate
> -	select SOUNDWIRE_INTEL
>   	help
>   	  This option is not user-selectable but automagically handled by
>   	  'select' statements at a higher level.
>
Arnd Bergmann Jan. 12, 2021, 10:36 p.m. UTC | #2
On Tue, Jan 12, 2021 at 10:03 PM Pierre-Louis Bossart
<pierre-louis.bossart@linux.intel.com> wrote:
> On 1/12/21 2:32 PM, Arnd Bergmann wrote:
> > From: Arnd Bergmann <arnd@arndb.de>
> >
> > The Kconfig logic around SND_SOC_SOF_INTEL_SOUNDWIRE tries to
> > ensure that all sound modules can be built with the minimal
> > dependencies, but this fails in some configurations:
> >
> > x86_64-linux-ld: sound/hda/intel-dsp-config.o: in function `snd_intel_dsp_driver_probe':
> > intel-dsp-config.c:(.text+0x134): undefined reference to `sdw_intel_acpi_scan'
> >
> > Specifically, this happens if the dsp-config driver is built-in but does
> > not need to use soundwire, while CONFIG_SOUNDWIRE_INTEL is enabled as
> > a loadable module.
> >
> > An easier and more correct way to do this is to remove
> > CONFIG_SND_SOC_SOF_INTEL_SOUNDWIRE_LINK and instead have
> > the two drivers that can link against SOUNDWIRE_INTEL,
> > i.e. DSP_CONFIG and SND_SOC_SOF_HDA, select that driver whenever
> > SND_SOC_SOF_INTEL_SOUNDWIRE_LINK is set.
> >
> > This however means that SND_SOC_SOF_INTEL_SOUNDWIRE cannot be selected
> > by users when SOUNDWIRE is only usable by loadable modules and one or
> > more drivers using SND_SOC_SOF_INTEL_SOUNDWIRE_LINK_BASELINE is built-in.
>
> The problem is real, but the proposal isn't completely right, there is
> absolutely no logical link or functional dependency between
> INTEL_DSP_CONFIG and SOUNDWIRE.

If that is true, would it be possible to move the call to
sdw_intel_acpi_scan() out of these drivers and one layer
higher where the dependency actually is?

I was indeed wondering whether the intel-dsp-config.c is just
another layering violation: this is another generic piece
of code that seems to contain too much knowledge about
specific hardware implementations.

> We have a similar case with HDaudio, the two buses can be selected as
> tristates, but the SOF configuration needs to match.
>
> In both cases, either we add a 'depends' and users need to make sure the
> configurations match on the two sides. Or we use select but one of the
> selections will be overridden and becomes meaningless.

Maybe something like this:

config SND_SOC_SOF_INTEL_SOUNDWIRE
-        bool "SOF support for SoundWire"
+       tristate "SOF support for SoundWire"
-       depends on SOUNDWIRE && ACPI
+       depends on SND_SOC_SOF_INTEL_SOUNDWIRE_LINK_BASELINE
+       depends on SOUNDWIRE
+       depends on ACPI
+       depends on !(SOUNDWIRE=m && SND_SOC_SOF_INTEL_SOUNDWIRE_LINK_BASELINE=y)
+       select SOUNDWIRE_INTEL

I have not tried it, but that should keep it all in one place.

> Arnd, do you mind if I give it a try on my side?

I have no specific attachment to my patch, this was just what I came up
with to fix the build regression locally.

       Arnd
Mark Brown Jan. 13, 2021, 11:29 a.m. UTC | #3
On Tue, Jan 12, 2021 at 11:36:15PM +0100, Arnd Bergmann wrote:

> I was indeed wondering whether the intel-dsp-config.c is just
> another layering violation: this is another generic piece
> of code that seems to contain too much knowledge about
> specific hardware implementations.

The purpose of that code is to try to figure out which of the multiple
sets of drivers and firmwares available for Intel systems it's best to
run on any given system by default and choose between them at runtime
(or allow that choice to be overridden by users) so it's all about
knowing about specific hardware implementations.
diff mbox series

Patch

diff --git a/sound/hda/Kconfig b/sound/hda/Kconfig
index 3bc9224d5e4f..ecab814d7698 100644
--- a/sound/hda/Kconfig
+++ b/sound/hda/Kconfig
@@ -44,5 +44,6 @@  config SND_INTEL_NHLT
 config SND_INTEL_DSP_CONFIG
 	tristate
 	select SND_INTEL_NHLT if ACPI
+	select SOUNDWIRE_INTEL if SND_SOC_SOF_INTEL_SOUNDWIRE
 	# this config should be selected only for Intel DSP platforms.
 	# A fallback is provided so that the code compiles in all cases.
diff --git a/sound/soc/sof/intel/Kconfig b/sound/soc/sof/intel/Kconfig
index 67365ce0d86d..df8f9760870e 100644
--- a/sound/soc/sof/intel/Kconfig
+++ b/sound/soc/sof/intel/Kconfig
@@ -330,13 +330,17 @@  config SND_SOC_SOF_HDA
 	tristate
 	select SND_HDA_EXT_CORE if SND_SOC_SOF_HDA_LINK
 	select SND_SOC_HDAC_HDA if SND_SOC_SOF_HDA_AUDIO_CODEC
+	select SOUNDWIRE_INTEL if SND_SOC_SOF_INTEL_SOUNDWIRE
 	help
 	  This option is not user-selectable but automagically handled by
 	  'select' statements at a higher level.
 
-config SND_SOC_SOF_INTEL_SOUNDWIRE_LINK
+config SND_SOC_SOF_INTEL_SOUNDWIRE
 	bool "SOF support for SoundWire"
-	depends on SOUNDWIRE && ACPI
+	depends on SND_SOC_SOF_INTEL_SOUNDWIRE_LINK_BASELINE
+	depends on SOUNDWIRE
+	depends on ACPI
+	depends on !(SOUNDWIRE=m && SND_SOC_SOF_INTEL_SOUNDWIRE_LINK_BASELINE=y)
 	help
 	  This adds support for SoundWire with Sound Open Firmware
 	  for Intel(R) platforms.
@@ -345,14 +349,6 @@  config SND_SOC_SOF_INTEL_SOUNDWIRE_LINK
 
 config SND_SOC_SOF_INTEL_SOUNDWIRE_LINK_BASELINE
 	tristate
-	select SND_SOC_SOF_INTEL_SOUNDWIRE if SND_SOC_SOF_INTEL_SOUNDWIRE_LINK
-	help
-	  This option is not user-selectable but automagically handled by
-	  'select' statements at a higher level.
-
-config SND_SOC_SOF_INTEL_SOUNDWIRE
-	tristate
-	select SOUNDWIRE_INTEL
 	help
 	  This option is not user-selectable but automagically handled by
 	  'select' statements at a higher level.