ASoC: skl: always select SND_SOC_HDAC_HDA
diff mbox series

Message ID 20181102112456.780127-1-arnd@arndb.de
State New
Headers show
Series
  • ASoC: skl: always select SND_SOC_HDAC_HDA
Related show

Commit Message

Arnd Bergmann Nov. 2, 2018, 11:24 a.m. UTC
The skylake sound support is written to work both with or without
CONFIG_SND_SOC_HDAC_HDA, and uses an #ifdef to decide whether it should
link against that. However, this fails with SND_SOC_ALL_CODECS=m or
SND_SOC_INTEL_SKL_HDA_DSP_GENERIC_MACH=m when the Skylake support itself
is built-in, with this link error:

sound/soc/intel/skylake/skl.o: In function `skl_probe':
skl.c:(.text+0x56c): undefined reference to `snd_soc_hdac_hda_get_ops'

Using an explicit 'select' here simplifies the logic and prevents
it from happening, at the cost of always including the compile
time dependency.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 sound/soc/intel/Kconfig       | 1 +
 sound/soc/intel/skylake/skl.c | 2 --
 2 files changed, 1 insertion(+), 2 deletions(-)

Comments

Pierre-Louis Bossart Nov. 2, 2018, 2:56 p.m. UTC | #1
On 11/2/18 6:24 AM, Arnd Bergmann wrote:
> The skylake sound support is written to work both with or without
> CONFIG_SND_SOC_HDAC_HDA, and uses an #ifdef to decide whether it should
> link against that. However, this fails with SND_SOC_ALL_CODECS=m or
> SND_SOC_INTEL_SKL_HDA_DSP_GENERIC_MACH=m when the Skylake support itself
> is built-in, with this link error:
>
> sound/soc/intel/skylake/skl.o: In function `skl_probe':
> skl.c:(.text+0x56c): undefined reference to `snd_soc_hdac_hda_get_ops'
>
> Using an explicit 'select' here simplifies the logic and prevents
> it from happening, at the cost of always including the compile
> time dependency.

Thanks Arnd for the report. I don't quite agree with the proposal, this 
should be similar to HDAC_HDMI which is not selected by default, and 
there's no reason to force the support for HDAudio when the vast 
majority of Skylake+ devices which enable this driver don't have any 
HDaudio codec.

Also the code is skl.c is already conditional, so this function should 
not be required.

Do you have a config that fails, I'd like to look at this further.

>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>   sound/soc/intel/Kconfig       | 1 +
>   sound/soc/intel/skylake/skl.c | 2 --
>   2 files changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/sound/soc/intel/Kconfig b/sound/soc/intel/Kconfig
> index 0caa1f4eb94d..c21ce7624af1 100644
> --- a/sound/soc/intel/Kconfig
> +++ b/sound/soc/intel/Kconfig
> @@ -109,6 +109,7 @@ config SND_SOC_INTEL_SKYLAKE
>   	depends on PCI && ACPI
>   	select SND_HDA_EXT_CORE
>   	select SND_HDA_DSP_LOADER
> +	select SND_SOC_HDAC_HDA
>   	select SND_SOC_TOPOLOGY
>   	select SND_SOC_INTEL_SST
>   	select SND_SOC_ACPI_INTEL_MATCH
> diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c
> index 29225623b4b4..1069ee265ce5 100644
> --- a/sound/soc/intel/skylake/skl.c
> +++ b/sound/soc/intel/skylake/skl.c
> @@ -870,9 +870,7 @@ static int skl_create(struct pci_dev *pci,
>   	hbus = skl_to_hbus(skl);
>   	bus = skl_to_bus(skl);
>   
> -#if IS_ENABLED(CONFIG_SND_SOC_HDAC_HDA)
>   	ext_ops = snd_soc_hdac_hda_get_ops();
> -#endif
>   	snd_hdac_ext_bus_init(bus, &pci->dev, &bus_core_ops, io_ops, ext_ops);
>   	bus->use_posbuf = 1;
>   	skl->pci = pci;
Arnd Bergmann Nov. 2, 2018, 10:03 p.m. UTC | #2
On 11/2/18, Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> wrote:
>
> On 11/2/18 6:24 AM, Arnd Bergmann wrote:
>> The skylake sound support is written to work both with or without
>> CONFIG_SND_SOC_HDAC_HDA, and uses an #ifdef to decide whether it should
>> link against that. However, this fails with SND_SOC_ALL_CODECS=m or
>> SND_SOC_INTEL_SKL_HDA_DSP_GENERIC_MACH=m when the Skylake support itself
>> is built-in, with this link error:
>>
>> sound/soc/intel/skylake/skl.o: In function `skl_probe':
>> skl.c:(.text+0x56c): undefined reference to `snd_soc_hdac_hda_get_ops'
>>
>> Using an explicit 'select' here simplifies the logic and prevents
>> it from happening, at the cost of always including the compile
>> time dependency.
>
> Thanks Arnd for the report. I don't quite agree with the proposal, this
> should be similar to HDAC_HDMI which is not selected by default, and
> there's no reason to force the support for HDAudio when the vast
> majority of Skylake+ devices which enable this driver don't have any
> HDaudio codec.

Sure, feel free to treat this patch as a bug report and come up with
a better fix.

> Also the code is skl.c is already conditional, so this function should
> not be required.
>
> Do you have a config that fails, I'd like to look at this further.

https://pastebin.com/4iYPGxkU is what first triggered this.

      Arnd
Pierre-Louis Bossart Nov. 4, 2018, 4:45 p.m. UTC | #3
On 11/2/18 5:03 PM, Arnd Bergmann wrote:
> On 11/2/18, Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> wrote:
>> On 11/2/18 6:24 AM, Arnd Bergmann wrote:
>>> The skylake sound support is written to work both with or without
>>> CONFIG_SND_SOC_HDAC_HDA, and uses an #ifdef to decide whether it should
>>> link against that. However, this fails with SND_SOC_ALL_CODECS=m or
>>> SND_SOC_INTEL_SKL_HDA_DSP_GENERIC_MACH=m when the Skylake support itself
>>> is built-in, with this link error:
>>>
>>> sound/soc/intel/skylake/skl.o: In function `skl_probe':
>>> skl.c:(.text+0x56c): undefined reference to `snd_soc_hdac_hda_get_ops'
>>>
>>> Using an explicit 'select' here simplifies the logic and prevents
>>> it from happening, at the cost of always including the compile
>>> time dependency.
>> Thanks Arnd for the report. I don't quite agree with the proposal, this
>> should be similar to HDAC_HDMI which is not selected by default, and
>> there's no reason to force the support for HDAudio when the vast
>> majority of Skylake+ devices which enable this driver don't have any
>> HDaudio codec.
> Sure, feel free to treat this patch as a bug report and come up with
> a better fix.

I have to rewind my statement. Arnd is correct, the config cannot be the 
same as hdac_hdmi since there is a code dependency I completely missed.

So yes indeed we have to add a select HDAC_HDA statement under the 
SKYLAKE config - i just don't know of any other means to say "don't 
build HDAC_HDA as a module when SKYLAKE is buit-in"

That said, we can add a condition that will only select HDAC_HDA if 
required by the machine drivers selected. The patch below doesn't seem 
to have circular dependencies and compiles fine with Arnd's config. I 
thought of it while multitasking with "Home IT" work and haven't done 
any testing beyond compilation.  Arnd, can you give it a spin to see if 
this solves the issues?

diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig
index 9cc4f1848c9b..91000924eb7c 100644
--- a/sound/soc/codecs/Kconfig
+++ b/sound/soc/codecs/Kconfig
@@ -627,6 +627,14 @@ config SND_SOC_HDAC_HDA
         tristate
         select SND_HDA

+config SND_SOC_HDAC_HDA_CODEC
+       bool
+       help
+        This config is set by machine drivers who need SND_SOC_HDAC_HDA,
+        which is selected by the platform drivers (Skylake or SOF) based
+        on the value of this boolean. This indirection is required to deal
+        with a code dependency between platform driver and codec driver.
+
  config SND_SOC_ICS43432
         tristate

diff --git a/sound/soc/intel/Kconfig b/sound/soc/intel/Kconfig
index 0caa1f4eb94d..25f3bca5e56d 100644
--- a/sound/soc/intel/Kconfig
+++ b/sound/soc/intel/Kconfig
@@ -107,6 +107,7 @@ config SND_SOC_INTEL_SKYLAKE_SSP_CLK
  config SND_SOC_INTEL_SKYLAKE
         tristate "SKL/BXT/KBL/GLK/CNL... Platforms"
         depends on PCI && ACPI
+       select SND_SOC_HDAC_HDA if SND_SOC_HDAC_HDA_CODEC
         select SND_HDA_EXT_CORE
         select SND_HDA_DSP_LOADER
         select SND_SOC_TOPOLOGY
diff --git a/sound/soc/intel/boards/Kconfig b/sound/soc/intel/boards/Kconfig
index 73ca1350aa31..5e7d3f4aa3ff 100644
--- a/sound/soc/intel/boards/Kconfig
+++ b/sound/soc/intel/boards/Kconfig
@@ -296,7 +296,7 @@ config SND_SOC_INTEL_KBL_DA7219_MAX98927_MACH
  config SND_SOC_INTEL_SKL_HDA_DSP_GENERIC_MACH
         tristate "SKL/KBL/BXT/APL with HDA Codecs"
         select SND_SOC_HDAC_HDMI
-       select SND_SOC_HDAC_HDA
+       select SND_SOC_HDAC_HDA_CODEC
         help
           This adds support for ASoC machine driver for Intel platforms
           SKL/KBL/BXT/APL with iDisp, HDA audio codecs.
diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c
index 29225623b4b4..e0dda3fd689b 100644
--- a/sound/soc/intel/skylake/skl.c
+++ b/sound/soc/intel/skylake/skl.c
@@ -870,7 +870,7 @@ static int skl_create(struct pci_dev *pci,
         hbus = skl_to_hbus(skl);
         bus = skl_to_bus(skl);

-#if IS_ENABLED(CONFIG_SND_SOC_HDAC_HDA)
+#if IS_ENABLED(CONFIG_SND_SOC_HDAC_HDA_CODEC)
         ext_ops = snd_soc_hdac_hda_get_ops();
  #endif
         snd_hdac_ext_bus_init(bus, &pci->dev, &bus_core_ops, io_ops, 
ext_ops);
Andy Shevchenko Nov. 5, 2018, 1:35 p.m. UTC | #4
On Sun, Nov 04, 2018 at 10:45:17AM -0600, Pierre-Louis Bossart wrote:

> So yes indeed we have to add a select HDAC_HDA statement under the SKYLAKE
> config - i just don't know of any other means to say "don't build HDAC_HDA
> as a module when SKYLAKE is buit-in"

We have this ("strange") lines over the drivers:

config BAR
depends on FOO || FOO=n

which guarantees that FOO will be not module when BAR is built-in.
Arnd Bergmann Nov. 5, 2018, 3:07 p.m. UTC | #5
On 11/5/18, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> On Sun, Nov 04, 2018 at 10:45:17AM -0600, Pierre-Louis Bossart wrote:
>
>> So yes indeed we have to add a select HDAC_HDA statement under the
>> SKYLAKE
>> config - i just don't know of any other means to say "don't build
>> HDAC_HDA
>> as a module when SKYLAKE is buit-in"
>
> We have this ("strange") lines over the drivers:
>
> config BAR
> depends on FOO || FOO=n
>
> which guarantees that FOO will be not module when BAR is built-in.

That's what I normally use, but I could not figure this one out.
One problem is that SND_SOC_ALL_CODECS selects
SND_SOC_HDAC_HDA, and SND_SOC_ALL_CODECS itself
may be =m, causing the failure for
SND_SOC_INTEL_SKL_HDA_DSP_GENERIC_MACH=y.

It might work with a separate dummy symbol:

config SND_SOC_HDAC_HDA_FORCE
         tristate
         depends on SND_SOC_ALL_CODECS != n
         default SND_SOC_INTEL_SKYLAKE
         select SND_SOC_HDAC_HDA

This would make SND_SOC_HDAC_HDA built-in even
with SND_SOC_ALL_CODECS=m and
SND_SOC_INTEL_SKL_HDA_DSP_GENERIC_MACH=n

It seems a bit ugly, and would need some testing.

       Arnd
Takashi Iwai Nov. 5, 2018, 3:25 p.m. UTC | #6
On Mon, 05 Nov 2018 16:07:50 +0100,
Arnd Bergmann wrote:
> 
> On 11/5/18, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > On Sun, Nov 04, 2018 at 10:45:17AM -0600, Pierre-Louis Bossart wrote:
> >
> >> So yes indeed we have to add a select HDAC_HDA statement under the
> >> SKYLAKE
> >> config - i just don't know of any other means to say "don't build
> >> HDAC_HDA
> >> as a module when SKYLAKE is buit-in"
> >
> > We have this ("strange") lines over the drivers:
> >
> > config BAR
> > depends on FOO || FOO=n
> >
> > which guarantees that FOO will be not module when BAR is built-in.
> 
> That's what I normally use, but I could not figure this one out.
> One problem is that SND_SOC_ALL_CODECS selects
> SND_SOC_HDAC_HDA, and SND_SOC_ALL_CODECS itself
> may be =m, causing the failure for
> SND_SOC_INTEL_SKL_HDA_DSP_GENERIC_MACH=y.
> 
> It might work with a separate dummy symbol:
> 
> config SND_SOC_HDAC_HDA_FORCE
>          tristate
>          depends on SND_SOC_ALL_CODECS != n
>          default SND_SOC_INTEL_SKYLAKE
>          select SND_SOC_HDAC_HDA
> 
> This would make SND_SOC_HDAC_HDA built-in even
> with SND_SOC_ALL_CODECS=m and
> SND_SOC_INTEL_SKL_HDA_DSP_GENERIC_MACH=n
> 
> It seems a bit ugly, and would need some testing.

The mixture of depends and select is often more confusing, so IMO, we
should align with only select as Pierre's suggestion, in this
particular case.


thanks,

Takashi
Pierre-Louis Bossart Nov. 5, 2018, 5:18 p.m. UTC | #7
>>> We have this ("strange") lines over the drivers:
>>>
>>> config BAR
>>> depends on FOO || FOO=n
>>>
>>> which guarantees that FOO will be not module when BAR is built-in.
>> That's what I normally use, but I could not figure this one out.
>> One problem is that SND_SOC_ALL_CODECS selects
>> SND_SOC_HDAC_HDA, and SND_SOC_ALL_CODECS itself
>> may be =m, causing the failure for
>> SND_SOC_INTEL_SKL_HDA_DSP_GENERIC_MACH=y.
>>
>> It might work with a separate dummy symbol:
>>
>> config SND_SOC_HDAC_HDA_FORCE
>>           tristate
>>           depends on SND_SOC_ALL_CODECS != n
>>           default SND_SOC_INTEL_SKYLAKE
>>           select SND_SOC_HDAC_HDA
>>
>> This would make SND_SOC_HDAC_HDA built-in even
>> with SND_SOC_ALL_CODECS=m and
>> SND_SOC_INTEL_SKL_HDA_DSP_GENERIC_MACH=n
>>
>> It seems a bit ugly, and would need some testing.
> The mixture of depends and select is often more confusing, so IMO, we
> should align with only select as Pierre's suggestion, in this
> particular case.

I tried to avoid re-adding a "depends on" which we tried to remove last 
year, but there is still a conditional statement which assumes a very 
specific evaluation order.

1. SKYLAKE is enabled as m or y

2. that enables some machine drivers (through an if statement), some of 
which might select HDA audio

3. the selections done within the SKYLAKE config are conditional on the 
outcome of #2.

it seems to work but I don't know if it's by design or an accident :-). 
One could argue it's a hidden "depends on".
Andy Shevchenko Nov. 5, 2018, 8:46 p.m. UTC | #8
On Mon, Nov 5, 2018 at 7:19 PM Pierre-Louis Bossart
<pierre-louis.bossart@linux.intel.com> wrote:
>
>
> >>> We have this ("strange") lines over the drivers:
> >>>
> >>> config BAR
> >>> depends on FOO || FOO=n
> >>>
> >>> which guarantees that FOO will be not module when BAR is built-in.
> >> That's what I normally use, but I could not figure this one out.
> >> One problem is that SND_SOC_ALL_CODECS selects
> >> SND_SOC_HDAC_HDA, and SND_SOC_ALL_CODECS itself
> >> may be =m, causing the failure for
> >> SND_SOC_INTEL_SKL_HDA_DSP_GENERIC_MACH=y.
> >>
> >> It might work with a separate dummy symbol:
> >>
> >> config SND_SOC_HDAC_HDA_FORCE
> >>           tristate

> >>           depends on SND_SOC_ALL_CODECS != n

Forgot to mention that AFAIU above line is no-op in symbols which are hidden.

> >>           default SND_SOC_INTEL_SKYLAKE
> >>           select SND_SOC_HDAC_HDA
Arnd Bergmann Nov. 5, 2018, 9:19 p.m. UTC | #9
On 11/5/18, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> On Mon, Nov 5, 2018 at 7:19 PM Pierre-Louis Bossart
> <pierre-louis.bossart@linux.intel.com> wrote:
>>
>>
>> >>> We have this ("strange") lines over the drivers:
>> >>>
>> >>> config BAR
>> >>> depends on FOO || FOO=n
>> >>>
>> >>> which guarantees that FOO will be not module when BAR is built-in.
>> >> That's what I normally use, but I could not figure this one out.
>> >> One problem is that SND_SOC_ALL_CODECS selects
>> >> SND_SOC_HDAC_HDA, and SND_SOC_ALL_CODECS itself
>> >> may be =m, causing the failure for
>> >> SND_SOC_INTEL_SKL_HDA_DSP_GENERIC_MACH=y.
>> >>
>> >> It might work with a separate dummy symbol:
>> >>
>> >> config SND_SOC_HDAC_HDA_FORCE
>> >>           tristate
>
>> >>           depends on SND_SOC_ALL_CODECS != n
>
> Forgot to mention that AFAIU above line is no-op in symbols which are
> hidden.
>

It's not. In this case, it will prevent SND_SOC_HDAC_HDA_FORCE
from being turned on by the 'default ...' line. In other cases, it can
generate a warning if you have a hidden symbol that is selected
by some other symbol while the dependency is missing.

      Arnd
Andy Shevchenko Nov. 6, 2018, 10:10 a.m. UTC | #10
On Mon, Nov 05, 2018 at 10:19:02PM +0100, Arnd Bergmann wrote:
> On 11/5/18, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > On Mon, Nov 5, 2018 at 7:19 PM Pierre-Louis Bossart
> > <pierre-louis.bossart@linux.intel.com> wrote:

> >> >> config SND_SOC_HDAC_HDA_FORCE
> >> >>           tristate
> >
> >> >>           depends on SND_SOC_ALL_CODECS != n
> >
> > Forgot to mention that AFAIU above line is no-op in symbols which are
> > hidden.
> >
> 
> It's not. In this case, it will prevent SND_SOC_HDAC_HDA_FORCE
> from being turned on by the 'default ...' line. In other cases, it can
> generate a warning if you have a hidden symbol that is selected
> by some other symbol while the dependency is missing.

Something new to learn.
Thanks, Arnd, for describing this case.

Patch
diff mbox series

diff --git a/sound/soc/intel/Kconfig b/sound/soc/intel/Kconfig
index 0caa1f4eb94d..c21ce7624af1 100644
--- a/sound/soc/intel/Kconfig
+++ b/sound/soc/intel/Kconfig
@@ -109,6 +109,7 @@  config SND_SOC_INTEL_SKYLAKE
 	depends on PCI && ACPI
 	select SND_HDA_EXT_CORE
 	select SND_HDA_DSP_LOADER
+	select SND_SOC_HDAC_HDA
 	select SND_SOC_TOPOLOGY
 	select SND_SOC_INTEL_SST
 	select SND_SOC_ACPI_INTEL_MATCH
diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c
index 29225623b4b4..1069ee265ce5 100644
--- a/sound/soc/intel/skylake/skl.c
+++ b/sound/soc/intel/skylake/skl.c
@@ -870,9 +870,7 @@  static int skl_create(struct pci_dev *pci,
 	hbus = skl_to_hbus(skl);
 	bus = skl_to_bus(skl);
 
-#if IS_ENABLED(CONFIG_SND_SOC_HDAC_HDA)
 	ext_ops = snd_soc_hdac_hda_get_ops();
-#endif
 	snd_hdac_ext_bus_init(bus, &pci->dev, &bus_core_ops, io_ops, ext_ops);
 	bus->use_posbuf = 1;
 	skl->pci = pci;