[PATCH] ALSA: hda: fix SND_INTEL_DSP_CONFIG dependency

[PATCH] ALSA: hda: fix SND_INTEL_DSP_CONFIG dependency

[Arnd Bergmann]

From: Arnd Bergmann <arnd@arndb.de>

The sof-pci-dev driver fails to link when built into the kernel
and CONFIG_SND_INTEL_DSP_CONFIG is set to =m:

arm-linux-gnueabi-ld: sound/soc/sof/sof-pci-dev.o: in function `sof_pci_probe':
sof-pci-dev.c:(.text+0x1c): undefined reference to `snd_intel_dsp_driver_probe'

All other drivers using this interface already use a 'select
SND_INTEL_DSP_CONFIG' statement to force the it to be present, so it
seems reasonable to do the same here.

The stub implementation in the header makes the problem harder to find,
as it avoids the link error when SND_INTEL_DSP_CONFIG is completely
disabled, without any obvious upsides. Remove these stubs to make it
clearer that the driver is in fact needed here.

Fixes: 82d9d54a6c0e ("ALSA: hda: add Intel DSP configuration / probe code")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 include/sound/intel-dsp-config.h | 17 -----------------
 sound/soc/sof/Kconfig            |  2 ++
 2 files changed, 2 insertions(+), 17 deletions(-)

diff --git a/include/sound/intel-dsp-config.h b/include/sound/intel-dsp-config.h
index d4609077c258..94667e870029 100644
--- a/include/sound/intel-dsp-config.h
+++ b/include/sound/intel-dsp-config.h
@@ -18,24 +18,7 @@ enum {
 	SND_INTEL_DSP_DRIVER_LAST = SND_INTEL_DSP_DRIVER_SOF
 };
 
-#if IS_ENABLED(CONFIG_SND_INTEL_DSP_CONFIG)
-
 int snd_intel_dsp_driver_probe(struct pci_dev *pci);
 int snd_intel_acpi_dsp_driver_probe(struct device *dev, const u8 acpi_hid[ACPI_ID_LEN]);
 
-#else
-
-static inline int snd_intel_dsp_driver_probe(struct pci_dev *pci)
-{
-	return SND_INTEL_DSP_DRIVER_ANY;
-}
-
-static inline
-int snd_intel_acpi_dsp_driver_probe(struct device *dev, const u8 acpi_hid[ACPI_ID_LEN])
-{
-	return SND_INTEL_DSP_DRIVER_ANY;
-}
-
-#endif
-
 #endif
diff --git a/sound/soc/sof/Kconfig b/sound/soc/sof/Kconfig
index 031dad5fc4c7..051fd3d27047 100644
--- a/sound/soc/sof/Kconfig
+++ b/sound/soc/sof/Kconfig
@@ -12,6 +12,7 @@ if SND_SOC_SOF_TOPLEVEL
 config SND_SOC_SOF_PCI
 	tristate "SOF PCI enumeration support"
 	depends on PCI
+	select SND_INTEL_DSP_CONFIG
 	select SND_SOC_SOF
 	select SND_SOC_ACPI if ACPI
 	help
@@ -23,6 +24,7 @@ config SND_SOC_SOF_PCI
 config SND_SOC_SOF_ACPI
 	tristate "SOF ACPI enumeration support"
 	depends on ACPI || COMPILE_TEST
+	select SND_INTEL_DSP_CONFIG
 	select SND_SOC_SOF
 	select SND_SOC_ACPI if ACPI
 	select IOSF_MBI if X86 && PCI
-- 
2.29.2

[PATCH] ALSA: hda: fix SND_INTEL_DSP_CONFIG dependency

[Arnd Bergmann]

From: Arnd Bergmann <arnd@arndb.de>

The sof-pci-dev driver fails to link when built into the kernel
and CONFIG_SND_INTEL_DSP_CONFIG is set to =m:

arm-linux-gnueabi-ld: sound/soc/sof/sof-pci-dev.o: in function `sof_pci_probe':
sof-pci-dev.c:(.text+0x1c): undefined reference to `snd_intel_dsp_driver_probe'

All other drivers using this interface already use a 'select
SND_INTEL_DSP_CONFIG' statement to force the it to be present, so it
seems reasonable to do the same here.

The stub implementation in the header makes the problem harder to find,
as it avoids the link error when SND_INTEL_DSP_CONFIG is completely
disabled, without any obvious upsides. Remove these stubs to make it
clearer that the driver is in fact needed here.

Fixes: 82d9d54a6c0e ("ALSA: hda: add Intel DSP configuration / probe code")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 include/sound/intel-dsp-config.h | 17 -----------------
 sound/soc/sof/Kconfig            |  2 ++
 2 files changed, 2 insertions(+), 17 deletions(-)

diff --git a/include/sound/intel-dsp-config.h b/include/sound/intel-dsp-config.h
index d4609077c258..94667e870029 100644
--- a/include/sound/intel-dsp-config.h
+++ b/include/sound/intel-dsp-config.h
@@ -18,24 +18,7 @@ enum {
 	SND_INTEL_DSP_DRIVER_LAST = SND_INTEL_DSP_DRIVER_SOF
 };
 
-#if IS_ENABLED(CONFIG_SND_INTEL_DSP_CONFIG)
-
 int snd_intel_dsp_driver_probe(struct pci_dev *pci);
 int snd_intel_acpi_dsp_driver_probe(struct device *dev, const u8 acpi_hid[ACPI_ID_LEN]);
 
-#else
-
-static inline int snd_intel_dsp_driver_probe(struct pci_dev *pci)
-{
-	return SND_INTEL_DSP_DRIVER_ANY;
-}
-
-static inline
-int snd_intel_acpi_dsp_driver_probe(struct device *dev, const u8 acpi_hid[ACPI_ID_LEN])
-{
-	return SND_INTEL_DSP_DRIVER_ANY;
-}
-
-#endif
-
 #endif
diff --git a/sound/soc/sof/Kconfig b/sound/soc/sof/Kconfig
index 031dad5fc4c7..051fd3d27047 100644
--- a/sound/soc/sof/Kconfig
+++ b/sound/soc/sof/Kconfig
@@ -12,6 +12,7 @@ if SND_SOC_SOF_TOPLEVEL
 config SND_SOC_SOF_PCI
 	tristate "SOF PCI enumeration support"
 	depends on PCI
+	select SND_INTEL_DSP_CONFIG
 	select SND_SOC_SOF
 	select SND_SOC_ACPI if ACPI
 	help
@@ -23,6 +24,7 @@ config SND_SOC_SOF_PCI
 config SND_SOC_SOF_ACPI
 	tristate "SOF ACPI enumeration support"
 	depends on ACPI || COMPILE_TEST
+	select SND_INTEL_DSP_CONFIG
 	select SND_SOC_SOF
 	select SND_SOC_ACPI if ACPI
 	select IOSF_MBI if X86 && PCI
-- 
2.29.2

Re: [PATCH] ALSA: hda: fix SND_INTEL_DSP_CONFIG dependency

[Takashi Iwai]

On Sun, 03 Jan 2021 14:52:32 +0100,
Arnd Bergmann wrote:
> 
> From: Arnd Bergmann <arnd@arndb.de>
> 
> The sof-pci-dev driver fails to link when built into the kernel
> and CONFIG_SND_INTEL_DSP_CONFIG is set to =m:
> 
> arm-linux-gnueabi-ld: sound/soc/sof/sof-pci-dev.o: in function `sof_pci_probe':
> sof-pci-dev.c:(.text+0x1c): undefined reference to `snd_intel_dsp_driver_probe'
> 
> All other drivers using this interface already use a 'select
> SND_INTEL_DSP_CONFIG' statement to force the it to be present, so it
> seems reasonable to do the same here.
> 
> The stub implementation in the header makes the problem harder to find,
> as it avoids the link error when SND_INTEL_DSP_CONFIG is completely
> disabled, without any obvious upsides. Remove these stubs to make it
> clearer that the driver is in fact needed here.
> 
> Fixes: 82d9d54a6c0e ("ALSA: hda: add Intel DSP configuration / probe code")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

IMO, the problem is rather the unconditional calls of
snd_intel_dsp_driver_probe() in snd-soc-sof-pci and snd-soc-sof-acpi
drivers.  Those calls should be done only if Intel drivers are
involved.  So, wrapping the call with ifdef
CONFIG_SND_SOC_SOF_INTEL_PCI or CONFIG_SND_SOC_SOF_INTEL_ACPI would
work better (although those are a bit uglier).


thanks,

Takashi

> ---
>  include/sound/intel-dsp-config.h | 17 -----------------
>  sound/soc/sof/Kconfig            |  2 ++
>  2 files changed, 2 insertions(+), 17 deletions(-)
> 
> diff --git a/include/sound/intel-dsp-config.h b/include/sound/intel-dsp-config.h
> index d4609077c258..94667e870029 100644
> --- a/include/sound/intel-dsp-config.h
> +++ b/include/sound/intel-dsp-config.h
> @@ -18,24 +18,7 @@ enum {
>  	SND_INTEL_DSP_DRIVER_LAST = SND_INTEL_DSP_DRIVER_SOF
>  };
>  
> -#if IS_ENABLED(CONFIG_SND_INTEL_DSP_CONFIG)
> -
>  int snd_intel_dsp_driver_probe(struct pci_dev *pci);
>  int snd_intel_acpi_dsp_driver_probe(struct device *dev, const u8 acpi_hid[ACPI_ID_LEN]);
>  
> -#else
> -
> -static inline int snd_intel_dsp_driver_probe(struct pci_dev *pci)
> -{
> -	return SND_INTEL_DSP_DRIVER_ANY;
> -}
> -
> -static inline
> -int snd_intel_acpi_dsp_driver_probe(struct device *dev, const u8 acpi_hid[ACPI_ID_LEN])
> -{
> -	return SND_INTEL_DSP_DRIVER_ANY;
> -}
> -
> -#endif
> -
>  #endif
> diff --git a/sound/soc/sof/Kconfig b/sound/soc/sof/Kconfig
> index 031dad5fc4c7..051fd3d27047 100644
> --- a/sound/soc/sof/Kconfig
> +++ b/sound/soc/sof/Kconfig
> @@ -12,6 +12,7 @@ if SND_SOC_SOF_TOPLEVEL
>  config SND_SOC_SOF_PCI
>  	tristate "SOF PCI enumeration support"
>  	depends on PCI
> +	select SND_INTEL_DSP_CONFIG
>  	select SND_SOC_SOF
>  	select SND_SOC_ACPI if ACPI
>  	help
> @@ -23,6 +24,7 @@ config SND_SOC_SOF_PCI
>  config SND_SOC_SOF_ACPI
>  	tristate "SOF ACPI enumeration support"
>  	depends on ACPI || COMPILE_TEST
> +	select SND_INTEL_DSP_CONFIG
>  	select SND_SOC_SOF
>  	select SND_SOC_ACPI if ACPI
>  	select IOSF_MBI if X86 && PCI
> -- 
> 2.29.2
> 

Re: [PATCH] ALSA: hda: fix SND_INTEL_DSP_CONFIG dependency

[Takashi Iwai]

On Sun, 03 Jan 2021 14:52:32 +0100,
Arnd Bergmann wrote:
> 
> From: Arnd Bergmann <arnd@arndb.de>
> 
> The sof-pci-dev driver fails to link when built into the kernel
> and CONFIG_SND_INTEL_DSP_CONFIG is set to =m:
> 
> arm-linux-gnueabi-ld: sound/soc/sof/sof-pci-dev.o: in function `sof_pci_probe':
> sof-pci-dev.c:(.text+0x1c): undefined reference to `snd_intel_dsp_driver_probe'
> 
> All other drivers using this interface already use a 'select
> SND_INTEL_DSP_CONFIG' statement to force the it to be present, so it
> seems reasonable to do the same here.
> 
> The stub implementation in the header makes the problem harder to find,
> as it avoids the link error when SND_INTEL_DSP_CONFIG is completely
> disabled, without any obvious upsides. Remove these stubs to make it
> clearer that the driver is in fact needed here.
> 
> Fixes: 82d9d54a6c0e ("ALSA: hda: add Intel DSP configuration / probe code")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

IMO, the problem is rather the unconditional calls of
snd_intel_dsp_driver_probe() in snd-soc-sof-pci and snd-soc-sof-acpi
drivers.  Those calls should be done only if Intel drivers are
involved.  So, wrapping the call with ifdef
CONFIG_SND_SOC_SOF_INTEL_PCI or CONFIG_SND_SOC_SOF_INTEL_ACPI would
work better (although those are a bit uglier).


thanks,

Takashi

> ---
>  include/sound/intel-dsp-config.h | 17 -----------------
>  sound/soc/sof/Kconfig            |  2 ++
>  2 files changed, 2 insertions(+), 17 deletions(-)
> 
> diff --git a/include/sound/intel-dsp-config.h b/include/sound/intel-dsp-config.h
> index d4609077c258..94667e870029 100644
> --- a/include/sound/intel-dsp-config.h
> +++ b/include/sound/intel-dsp-config.h
> @@ -18,24 +18,7 @@ enum {
>  	SND_INTEL_DSP_DRIVER_LAST = SND_INTEL_DSP_DRIVER_SOF
>  };
>  
> -#if IS_ENABLED(CONFIG_SND_INTEL_DSP_CONFIG)
> -
>  int snd_intel_dsp_driver_probe(struct pci_dev *pci);
>  int snd_intel_acpi_dsp_driver_probe(struct device *dev, const u8 acpi_hid[ACPI_ID_LEN]);
>  
> -#else
> -
> -static inline int snd_intel_dsp_driver_probe(struct pci_dev *pci)
> -{
> -	return SND_INTEL_DSP_DRIVER_ANY;
> -}
> -
> -static inline
> -int snd_intel_acpi_dsp_driver_probe(struct device *dev, const u8 acpi_hid[ACPI_ID_LEN])
> -{
> -	return SND_INTEL_DSP_DRIVER_ANY;
> -}
> -
> -#endif
> -
>  #endif
> diff --git a/sound/soc/sof/Kconfig b/sound/soc/sof/Kconfig
> index 031dad5fc4c7..051fd3d27047 100644
> --- a/sound/soc/sof/Kconfig
> +++ b/sound/soc/sof/Kconfig
> @@ -12,6 +12,7 @@ if SND_SOC_SOF_TOPLEVEL
>  config SND_SOC_SOF_PCI
>  	tristate "SOF PCI enumeration support"
>  	depends on PCI
> +	select SND_INTEL_DSP_CONFIG
>  	select SND_SOC_SOF
>  	select SND_SOC_ACPI if ACPI
>  	help
> @@ -23,6 +24,7 @@ config SND_SOC_SOF_PCI
>  config SND_SOC_SOF_ACPI
>  	tristate "SOF ACPI enumeration support"
>  	depends on ACPI || COMPILE_TEST
> +	select SND_INTEL_DSP_CONFIG
>  	select SND_SOC_SOF
>  	select SND_SOC_ACPI if ACPI
>  	select IOSF_MBI if X86 && PCI
> -- 
> 2.29.2
> 

Re: [PATCH] ALSA: hda: fix SND_INTEL_DSP_CONFIG dependency

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 461 bytes --]

On Mon, Jan 04, 2021 at 03:09:24PM +0100, Takashi Iwai wrote:

> IMO, the problem is rather the unconditional calls of
> snd_intel_dsp_driver_probe() in snd-soc-sof-pci and snd-soc-sof-acpi
> drivers.  Those calls should be done only if Intel drivers are
> involved.  So, wrapping the call with ifdef
> CONFIG_SND_SOC_SOF_INTEL_PCI or CONFIG_SND_SOC_SOF_INTEL_ACPI would
> work better (although those are a bit uglier).

Or stubbing them which might be neater.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] ALSA: hda: fix SND_INTEL_DSP_CONFIG dependency

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 461 bytes --]

On Mon, Jan 04, 2021 at 03:09:24PM +0100, Takashi Iwai wrote:

> IMO, the problem is rather the unconditional calls of
> snd_intel_dsp_driver_probe() in snd-soc-sof-pci and snd-soc-sof-acpi
> drivers.  Those calls should be done only if Intel drivers are
> involved.  So, wrapping the call with ifdef
> CONFIG_SND_SOC_SOF_INTEL_PCI or CONFIG_SND_SOC_SOF_INTEL_ACPI would
> work better (although those are a bit uglier).

Or stubbing them which might be neater.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] ALSA: hda: fix SND_INTEL_DSP_CONFIG dependency

[Jaroslav Kysela]

Dne 03. 01. 21 v 14:52 Arnd Bergmann napsal(a):
> From: Arnd Bergmann <arnd@arndb.de>
> 
> The sof-pci-dev driver fails to link when built into the kernel
> and CONFIG_SND_INTEL_DSP_CONFIG is set to =m:
> 
> arm-linux-gnueabi-ld: sound/soc/sof/sof-pci-dev.o: in function `sof_pci_probe':
> sof-pci-dev.c:(.text+0x1c): undefined reference to `snd_intel_dsp_driver_probe'
> 
> All other drivers using this interface already use a 'select
> SND_INTEL_DSP_CONFIG' statement to force the it to be present, so it
> seems reasonable to do the same here.
> 
> The stub implementation in the header makes the problem harder to find,
> as it avoids the link error when SND_INTEL_DSP_CONFIG is completely
> disabled, without any obvious upsides. Remove these stubs to make it
> clearer that the driver is in fact needed here.
> 
> Fixes: 82d9d54a6c0e ("ALSA: hda: add Intel DSP configuration / probe code")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  include/sound/intel-dsp-config.h | 17 -----------------
>  sound/soc/sof/Kconfig            |  2 ++
>  2 files changed, 2 insertions(+), 17 deletions(-)
> 
> diff --git a/include/sound/intel-dsp-config.h b/include/sound/intel-dsp-config.h
> index d4609077c258..94667e870029 100644
> --- a/include/sound/intel-dsp-config.h
> +++ b/include/sound/intel-dsp-config.h
> @@ -18,24 +18,7 @@ enum {
>  	SND_INTEL_DSP_DRIVER_LAST = SND_INTEL_DSP_DRIVER_SOF
>  };
>  
> -#if IS_ENABLED(CONFIG_SND_INTEL_DSP_CONFIG)

The SOF drivers selects the DSP config code only when required (for specific
platforms - see sound/soc/sof/intel/Kconfig).

It seems that the above if should be modified as:

#if IS_BUILDIN(CONFIG_SND_INTEL_DSP_CONFIG) || (defined(MODULE) &&
IS_MODULE(CONFIG_SND_INTEL_DSP_CONFIG))

So the buildin drivers which do not require the DSP config probe can be
compiled without this dependency.

					Jaroslav

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

Re: [PATCH] ALSA: hda: fix SND_INTEL_DSP_CONFIG dependency

[Jaroslav Kysela]

Dne 03. 01. 21 v 14:52 Arnd Bergmann napsal(a):
> From: Arnd Bergmann <arnd@arndb.de>
> 
> The sof-pci-dev driver fails to link when built into the kernel
> and CONFIG_SND_INTEL_DSP_CONFIG is set to =m:
> 
> arm-linux-gnueabi-ld: sound/soc/sof/sof-pci-dev.o: in function `sof_pci_probe':
> sof-pci-dev.c:(.text+0x1c): undefined reference to `snd_intel_dsp_driver_probe'
> 
> All other drivers using this interface already use a 'select
> SND_INTEL_DSP_CONFIG' statement to force the it to be present, so it
> seems reasonable to do the same here.
> 
> The stub implementation in the header makes the problem harder to find,
> as it avoids the link error when SND_INTEL_DSP_CONFIG is completely
> disabled, without any obvious upsides. Remove these stubs to make it
> clearer that the driver is in fact needed here.
> 
> Fixes: 82d9d54a6c0e ("ALSA: hda: add Intel DSP configuration / probe code")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  include/sound/intel-dsp-config.h | 17 -----------------
>  sound/soc/sof/Kconfig            |  2 ++
>  2 files changed, 2 insertions(+), 17 deletions(-)
> 
> diff --git a/include/sound/intel-dsp-config.h b/include/sound/intel-dsp-config.h
> index d4609077c258..94667e870029 100644
> --- a/include/sound/intel-dsp-config.h
> +++ b/include/sound/intel-dsp-config.h
> @@ -18,24 +18,7 @@ enum {
>  	SND_INTEL_DSP_DRIVER_LAST = SND_INTEL_DSP_DRIVER_SOF
>  };
>  
> -#if IS_ENABLED(CONFIG_SND_INTEL_DSP_CONFIG)

The SOF drivers selects the DSP config code only when required (for specific
platforms - see sound/soc/sof/intel/Kconfig).

It seems that the above if should be modified as:

#if IS_BUILDIN(CONFIG_SND_INTEL_DSP_CONFIG) || (defined(MODULE) &&
IS_MODULE(CONFIG_SND_INTEL_DSP_CONFIG))

So the buildin drivers which do not require the DSP config probe can be
compiled without this dependency.

					Jaroslav

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

Re: [PATCH] ALSA: hda: fix SND_INTEL_DSP_CONFIG dependency

[Takashi Iwai]

On Mon, 04 Jan 2021 16:00:05 +0100,
Jaroslav Kysela wrote:
> 
> Dne 03. 01. 21 v 14:52 Arnd Bergmann napsal(a):
> > From: Arnd Bergmann <arnd@arndb.de>
> > 
> > The sof-pci-dev driver fails to link when built into the kernel
> > and CONFIG_SND_INTEL_DSP_CONFIG is set to =m:
> > 
> > arm-linux-gnueabi-ld: sound/soc/sof/sof-pci-dev.o: in function `sof_pci_probe':
> > sof-pci-dev.c:(.text+0x1c): undefined reference to `snd_intel_dsp_driver_probe'
> > 
> > All other drivers using this interface already use a 'select
> > SND_INTEL_DSP_CONFIG' statement to force the it to be present, so it
> > seems reasonable to do the same here.
> > 
> > The stub implementation in the header makes the problem harder to find,
> > as it avoids the link error when SND_INTEL_DSP_CONFIG is completely
> > disabled, without any obvious upsides. Remove these stubs to make it
> > clearer that the driver is in fact needed here.
> > 
> > Fixes: 82d9d54a6c0e ("ALSA: hda: add Intel DSP configuration / probe code")
> > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> > ---
> >  include/sound/intel-dsp-config.h | 17 -----------------
> >  sound/soc/sof/Kconfig            |  2 ++
> >  2 files changed, 2 insertions(+), 17 deletions(-)
> > 
> > diff --git a/include/sound/intel-dsp-config.h b/include/sound/intel-dsp-config.h
> > index d4609077c258..94667e870029 100644
> > --- a/include/sound/intel-dsp-config.h
> > +++ b/include/sound/intel-dsp-config.h
> > @@ -18,24 +18,7 @@ enum {
> >  	SND_INTEL_DSP_DRIVER_LAST = SND_INTEL_DSP_DRIVER_SOF
> >  };
> >  
> > -#if IS_ENABLED(CONFIG_SND_INTEL_DSP_CONFIG)
> 
> The SOF drivers selects the DSP config code only when required (for specific
> platforms - see sound/soc/sof/intel/Kconfig).
> 
> It seems that the above if should be modified as:
> 
> #if IS_BUILDIN(CONFIG_SND_INTEL_DSP_CONFIG) || (defined(MODULE) &&
> IS_MODULE(CONFIG_SND_INTEL_DSP_CONFIG))
> 
> So the buildin drivers which do not require the DSP config probe can be
> compiled without this dependency.

As I wrote in another post, a part of the problem is that SOF PCI and
ACPI drivers call snd_intel_dsp_driver_probe() unconditionally, even
if no Intel driver is bound.  So even if changing like the above (or
better to use IS_REACHABLE() macro) works around the issue, the call
pattern needs to be reconsidered.


thanks,

Takashi

Re: [PATCH] ALSA: hda: fix SND_INTEL_DSP_CONFIG dependency

[Takashi Iwai]

On Mon, 04 Jan 2021 16:00:05 +0100,
Jaroslav Kysela wrote:
> 
> Dne 03. 01. 21 v 14:52 Arnd Bergmann napsal(a):
> > From: Arnd Bergmann <arnd@arndb.de>
> > 
> > The sof-pci-dev driver fails to link when built into the kernel
> > and CONFIG_SND_INTEL_DSP_CONFIG is set to =m:
> > 
> > arm-linux-gnueabi-ld: sound/soc/sof/sof-pci-dev.o: in function `sof_pci_probe':
> > sof-pci-dev.c:(.text+0x1c): undefined reference to `snd_intel_dsp_driver_probe'
> > 
> > All other drivers using this interface already use a 'select
> > SND_INTEL_DSP_CONFIG' statement to force the it to be present, so it
> > seems reasonable to do the same here.
> > 
> > The stub implementation in the header makes the problem harder to find,
> > as it avoids the link error when SND_INTEL_DSP_CONFIG is completely
> > disabled, without any obvious upsides. Remove these stubs to make it
> > clearer that the driver is in fact needed here.
> > 
> > Fixes: 82d9d54a6c0e ("ALSA: hda: add Intel DSP configuration / probe code")
> > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> > ---
> >  include/sound/intel-dsp-config.h | 17 -----------------
> >  sound/soc/sof/Kconfig            |  2 ++
> >  2 files changed, 2 insertions(+), 17 deletions(-)
> > 
> > diff --git a/include/sound/intel-dsp-config.h b/include/sound/intel-dsp-config.h
> > index d4609077c258..94667e870029 100644
> > --- a/include/sound/intel-dsp-config.h
> > +++ b/include/sound/intel-dsp-config.h
> > @@ -18,24 +18,7 @@ enum {
> >  	SND_INTEL_DSP_DRIVER_LAST = SND_INTEL_DSP_DRIVER_SOF
> >  };
> >  
> > -#if IS_ENABLED(CONFIG_SND_INTEL_DSP_CONFIG)
> 
> The SOF drivers selects the DSP config code only when required (for specific
> platforms - see sound/soc/sof/intel/Kconfig).
> 
> It seems that the above if should be modified as:
> 
> #if IS_BUILDIN(CONFIG_SND_INTEL_DSP_CONFIG) || (defined(MODULE) &&
> IS_MODULE(CONFIG_SND_INTEL_DSP_CONFIG))
> 
> So the buildin drivers which do not require the DSP config probe can be
> compiled without this dependency.

As I wrote in another post, a part of the problem is that SOF PCI and
ACPI drivers call snd_intel_dsp_driver_probe() unconditionally, even
if no Intel driver is bound.  So even if changing like the above (or
better to use IS_REACHABLE() macro) works around the issue, the call
pattern needs to be reconsidered.


thanks,

Takashi

Re: [PATCH] ALSA: hda: fix SND_INTEL_DSP_CONFIG dependency

[Arnd Bergmann]

On Mon, Jan 4, 2021 at 4:05 PM Takashi Iwai <tiwai@suse.de> wrote:
> On Mon, 04 Jan 2021 16:00:05 +0100, Jaroslav Kysela wrote:
> >
> > Dne 03. 01. 21 v 14:52 Arnd Bergmann napsal(a):
> > > From: Arnd Bergmann <arnd@arndb.de>
> > >
> > > The sof-pci-dev driver fails to link when built into the kernel
> > > and CONFIG_SND_INTEL_DSP_CONFIG is set to =m:
> > >
> > > arm-linux-gnueabi-ld: sound/soc/sof/sof-pci-dev.o: in function `sof_pci_probe':
> > > sof-pci-dev.c:(.text+0x1c): undefined reference to `snd_intel_dsp_driver_probe'
> > >
> > > All other drivers using this interface already use a 'select
> > > SND_INTEL_DSP_CONFIG' statement to force the it to be present, so it
> > > seems reasonable to do the same here.
> > >
> > > The stub implementation in the header makes the problem harder to find,
> > > as it avoids the link error when SND_INTEL_DSP_CONFIG is completely
> > > disabled, without any obvious upsides. Remove these stubs to make it
> > > clearer that the driver is in fact needed here.
> > >
> > > Fixes: 82d9d54a6c0e ("ALSA: hda: add Intel DSP configuration / probe code")
> > > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> > > ---
> > >  include/sound/intel-dsp-config.h | 17 -----------------
> > >  sound/soc/sof/Kconfig            |  2 ++
> > >  2 files changed, 2 insertions(+), 17 deletions(-)
> > >
> > > diff --git a/include/sound/intel-dsp-config.h b/include/sound/intel-dsp-config.h
> > > index d4609077c258..94667e870029 100644
> > > --- a/include/sound/intel-dsp-config.h
> > > +++ b/include/sound/intel-dsp-config.h
> > > @@ -18,24 +18,7 @@ enum {
> > >     SND_INTEL_DSP_DRIVER_LAST = SND_INTEL_DSP_DRIVER_SOF
> > >  };
> > >
> > > -#if IS_ENABLED(CONFIG_SND_INTEL_DSP_CONFIG)
> >
> > The SOF drivers selects the DSP config code only when required (for specific
> > platforms - see sound/soc/sof/intel/Kconfig).
> >
> > It seems that the above if should be modified as:
> >
> > #if IS_BUILDIN(CONFIG_SND_INTEL_DSP_CONFIG) || (defined(MODULE) &&
> > IS_MODULE(CONFIG_SND_INTEL_DSP_CONFIG))
> >
> > So the buildin drivers which do not require the DSP config probe can be
> > compiled without this dependency.
>
> As I wrote in another post, a part of the problem is that SOF PCI and
> ACPI drivers call snd_intel_dsp_driver_probe() unconditionally, even
> if no Intel driver is bound.

Makes sense. Is there an existing Kconfig that could be used to
decide whether the drivers use SND_INTEL_DSP_CONFIG or not?
Could it be part of the device specific driver_data?

According to sof_pci_ids[], all PCI IDs are Intel specific, but I can't
tell which ones need the DSP config.

> So even if changing like the above (or
> better to use IS_REACHABLE() macro) works around the issue, the call
> pattern needs to be reconsidered.

If the callers are fixed to address this, then I would expect the
IS_REACHABLE() or IS_ENABLED() to no longer be needed
either.

       Arnd

Re: [PATCH] ALSA: hda: fix SND_INTEL_DSP_CONFIG dependency

[Arnd Bergmann]

On Mon, Jan 4, 2021 at 4:05 PM Takashi Iwai <tiwai@suse.de> wrote:
> On Mon, 04 Jan 2021 16:00:05 +0100, Jaroslav Kysela wrote:
> >
> > Dne 03. 01. 21 v 14:52 Arnd Bergmann napsal(a):
> > > From: Arnd Bergmann <arnd@arndb.de>
> > >
> > > The sof-pci-dev driver fails to link when built into the kernel
> > > and CONFIG_SND_INTEL_DSP_CONFIG is set to =m:
> > >
> > > arm-linux-gnueabi-ld: sound/soc/sof/sof-pci-dev.o: in function `sof_pci_probe':
> > > sof-pci-dev.c:(.text+0x1c): undefined reference to `snd_intel_dsp_driver_probe'
> > >
> > > All other drivers using this interface already use a 'select
> > > SND_INTEL_DSP_CONFIG' statement to force the it to be present, so it
> > > seems reasonable to do the same here.
> > >
> > > The stub implementation in the header makes the problem harder to find,
> > > as it avoids the link error when SND_INTEL_DSP_CONFIG is completely
> > > disabled, without any obvious upsides. Remove these stubs to make it
> > > clearer that the driver is in fact needed here.
> > >
> > > Fixes: 82d9d54a6c0e ("ALSA: hda: add Intel DSP configuration / probe code")
> > > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> > > ---
> > >  include/sound/intel-dsp-config.h | 17 -----------------
> > >  sound/soc/sof/Kconfig            |  2 ++
> > >  2 files changed, 2 insertions(+), 17 deletions(-)
> > >
> > > diff --git a/include/sound/intel-dsp-config.h b/include/sound/intel-dsp-config.h
> > > index d4609077c258..94667e870029 100644
> > > --- a/include/sound/intel-dsp-config.h
> > > +++ b/include/sound/intel-dsp-config.h
> > > @@ -18,24 +18,7 @@ enum {
> > >     SND_INTEL_DSP_DRIVER_LAST = SND_INTEL_DSP_DRIVER_SOF
> > >  };
> > >
> > > -#if IS_ENABLED(CONFIG_SND_INTEL_DSP_CONFIG)
> >
> > The SOF drivers selects the DSP config code only when required (for specific
> > platforms - see sound/soc/sof/intel/Kconfig).
> >
> > It seems that the above if should be modified as:
> >
> > #if IS_BUILDIN(CONFIG_SND_INTEL_DSP_CONFIG) || (defined(MODULE) &&
> > IS_MODULE(CONFIG_SND_INTEL_DSP_CONFIG))
> >
> > So the buildin drivers which do not require the DSP config probe can be
> > compiled without this dependency.
>
> As I wrote in another post, a part of the problem is that SOF PCI and
> ACPI drivers call snd_intel_dsp_driver_probe() unconditionally, even
> if no Intel driver is bound.

Makes sense. Is there an existing Kconfig that could be used to
decide whether the drivers use SND_INTEL_DSP_CONFIG or not?
Could it be part of the device specific driver_data?

According to sof_pci_ids[], all PCI IDs are Intel specific, but I can't
tell which ones need the DSP config.

> So even if changing like the above (or
> better to use IS_REACHABLE() macro) works around the issue, the call
> pattern needs to be reconsidered.

If the callers are fixed to address this, then I would expect the
IS_REACHABLE() or IS_ENABLED() to no longer be needed
either.

       Arnd

Re: [PATCH] ALSA: hda: fix SND_INTEL_DSP_CONFIG dependency

[Kai Vehmanen]

Hey,

On Tue, 5 Jan 2021, Arnd Bergmann wrote:

> On Mon, Jan 4, 2021 at 4:05 PM Takashi Iwai <tiwai@suse.de> wrote:
> > As I wrote in another post, a part of the problem is that SOF PCI and
> > ACPI drivers call snd_intel_dsp_driver_probe() unconditionally, even
> > if no Intel driver is bound.
> 
> Makes sense. Is there an existing Kconfig that could be used to
> decide whether the drivers use SND_INTEL_DSP_CONFIG or not?

no, unfortunately not. This is selected per platform in 
sound/soc/sof/intel/Kconfig. CONFIG_SND_SOC_SOF_INTEL_PCI is close, but 
there is at least one platform that does not use SND_INTEL_DSP_CONFIG.

> According to sof_pci_ids[], all PCI IDs are Intel specific, but I can't
> tell which ones need the DSP config.

Indeed currently all the ids are Intel ones (and with exception of old 
Merrifield, all use DSP config). But that's just how it is now.

> Could it be part of the device specific driver_data? 

This would certainly be a clean way and allow to remove the Intel-specific 
calls from sof_pci_probe(). As a short-term solution, IS_REACHABLE() 
seems ok as well.

Br, Kai

Re: [PATCH] ALSA: hda: fix SND_INTEL_DSP_CONFIG dependency

[Kai Vehmanen]

Hey,

On Tue, 5 Jan 2021, Arnd Bergmann wrote:

> On Mon, Jan 4, 2021 at 4:05 PM Takashi Iwai <tiwai@suse.de> wrote:
> > As I wrote in another post, a part of the problem is that SOF PCI and
> > ACPI drivers call snd_intel_dsp_driver_probe() unconditionally, even
> > if no Intel driver is bound.
> 
> Makes sense. Is there an existing Kconfig that could be used to
> decide whether the drivers use SND_INTEL_DSP_CONFIG or not?

no, unfortunately not. This is selected per platform in 
sound/soc/sof/intel/Kconfig. CONFIG_SND_SOC_SOF_INTEL_PCI is close, but 
there is at least one platform that does not use SND_INTEL_DSP_CONFIG.

> According to sof_pci_ids[], all PCI IDs are Intel specific, but I can't
> tell which ones need the DSP config.

Indeed currently all the ids are Intel ones (and with exception of old 
Merrifield, all use DSP config). But that's just how it is now.

> Could it be part of the device specific driver_data? 

This would certainly be a clean way and allow to remove the Intel-specific 
calls from sof_pci_probe(). As a short-term solution, IS_REACHABLE() 
seems ok as well.

Br, Kai

Re: [PATCH] ALSA: hda: fix SND_INTEL_DSP_CONFIG dependency

[Arnd Bergmann]

On Tue, Jan 5, 2021 at 4:39 PM Kai Vehmanen
<kai.vehmanen@linux.intel.com> wrote:

> > Could it be part of the device specific driver_data?
>
> This would certainly be a clean way and allow to remove the Intel-specific
> calls from sof_pci_probe(). As a short-term solution, IS_REACHABLE()
> seems ok as well.

I looked at it some more and my conclusion is that the problem is
the way the drivers mix device specific and generic data: The generic
acpi or pci driver should never need to know about individual devices
and their dependencies. Instead of just exporting some generic
helper functions, these are the top-level drivers and the device
specific drivers are the ones exporting the data.

It's a common mistake,  but it always leads to complexity like this
and we tend to end up having to undo it all.

I prototyped a patch to do this for the acpi driver, and it seems
much more straightforward this way, please have a look.

commit a83ecfed5b31dfc862e04c9bf77d2107a1047c9b
Author: Arnd Bergmann <arnd@arndb.de>
Date:   Tue Jan 5 19:47:35 2021 +0100

    ASoC: SOF: Intel: avoid reverse module dependency

    The SOF-ACPI driver is backwards from the normal Linux model, it has a
    generic driver that knows about all the specific drivers, as opposed to
    having hardware specific drivers that link against a common framework.

    This requires ugly Kconfig magic and leads to missed dependencies as
    seen in this link error:

    arm-linux-gnueabi-ld: sound/soc/sof/sof-pci-dev.o: in function
`sof_acpi_probe':
    sof-pci-dev.c:(.text+0x1c): undefined reference to
`snd_intel_dsp_driver_probe'

    Change it to use the normal probe order of starting with a specific
    device in a driver, turning the sof-acpi-dev.c driver into a library.

    Signed-off-by: Arnd Bergmann <arnd@arndb.de>

 sound/soc/sof/intel/Kconfig  |  34 +++---------
 sound/soc/sof/intel/bdw.c    |  51 ++++++++++++++++--
 sound/soc/sof/intel/byt.c    | 104 ++++++++++++++++++++++++++++++++----
 sound/soc/sof/intel/shim.h   |  10 ++--
 sound/soc/sof/sof-acpi-dev.c | 122 ++-----------------------------------------
 5 files changed, 156 insertions(+), 165 deletions(-)

The PCI driver is left as an exercise to the reader.

      Arnd

Re: [PATCH] ALSA: hda: fix SND_INTEL_DSP_CONFIG dependency

[Arnd Bergmann]

On Tue, Jan 5, 2021 at 4:39 PM Kai Vehmanen
<kai.vehmanen@linux.intel.com> wrote:

> > Could it be part of the device specific driver_data?
>
> This would certainly be a clean way and allow to remove the Intel-specific
> calls from sof_pci_probe(). As a short-term solution, IS_REACHABLE()
> seems ok as well.

I looked at it some more and my conclusion is that the problem is
the way the drivers mix device specific and generic data: The generic
acpi or pci driver should never need to know about individual devices
and their dependencies. Instead of just exporting some generic
helper functions, these are the top-level drivers and the device
specific drivers are the ones exporting the data.

It's a common mistake,  but it always leads to complexity like this
and we tend to end up having to undo it all.

I prototyped a patch to do this for the acpi driver, and it seems
much more straightforward this way, please have a look.

commit a83ecfed5b31dfc862e04c9bf77d2107a1047c9b
Author: Arnd Bergmann <arnd@arndb.de>
Date:   Tue Jan 5 19:47:35 2021 +0100

    ASoC: SOF: Intel: avoid reverse module dependency

    The SOF-ACPI driver is backwards from the normal Linux model, it has a
    generic driver that knows about all the specific drivers, as opposed to
    having hardware specific drivers that link against a common framework.

    This requires ugly Kconfig magic and leads to missed dependencies as
    seen in this link error:

    arm-linux-gnueabi-ld: sound/soc/sof/sof-pci-dev.o: in function
`sof_acpi_probe':
    sof-pci-dev.c:(.text+0x1c): undefined reference to
`snd_intel_dsp_driver_probe'

    Change it to use the normal probe order of starting with a specific
    device in a driver, turning the sof-acpi-dev.c driver into a library.

    Signed-off-by: Arnd Bergmann <arnd@arndb.de>

 sound/soc/sof/intel/Kconfig  |  34 +++---------
 sound/soc/sof/intel/bdw.c    |  51 ++++++++++++++++--
 sound/soc/sof/intel/byt.c    | 104 ++++++++++++++++++++++++++++++++----
 sound/soc/sof/intel/shim.h   |  10 ++--
 sound/soc/sof/sof-acpi-dev.c | 122 ++-----------------------------------------
 5 files changed, 156 insertions(+), 165 deletions(-)

The PCI driver is left as an exercise to the reader.

      Arnd

[PATCH] ASoC: SOF: Intel: avoid reverse module dependency

[Arnd Bergmann]

From: Arnd Bergmann <arnd@arndb.de>

The SOF-ACPI driver is backwards from the normal Linux model, it has a
generic driver that knows about all the specific drivers, as opposed to
having hardware specific drivers that link against a common framework.

This requires ugly Kconfig magic and leads to missed dependencies as
seen in this link error:

arm-linux-gnueabi-ld: sound/soc/sof/sof-pci-dev.o: in function `sof_acpi_probe':
sof-pci-dev.c:(.text+0x1c): undefined reference to `snd_intel_dsp_driver_probe'

Change it to use the normal probe order of starting with a specific
device in a driver, turning the sof-acpi-dev.c driver into a library.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 sound/soc/sof/intel/Kconfig  |  34 ++--------
 sound/soc/sof/intel/bdw.c    |  51 +++++++++++++--
 sound/soc/sof/intel/byt.c    | 104 +++++++++++++++++++++++++----
 sound/soc/sof/intel/shim.h   |  10 ++-
 sound/soc/sof/sof-acpi-dev.c | 122 ++---------------------------------
 5 files changed, 156 insertions(+), 165 deletions(-)

diff --git a/sound/soc/sof/intel/Kconfig b/sound/soc/sof/intel/Kconfig
index ae9ba834814e..ff9266413a06 100644
--- a/sound/soc/sof/intel/Kconfig
+++ b/sound/soc/sof/intel/Kconfig
@@ -9,14 +9,6 @@ config SND_SOC_SOF_INTEL_TOPLEVEL
 
 if SND_SOC_SOF_INTEL_TOPLEVEL
 
-config SND_SOC_SOF_INTEL_ACPI
-	def_tristate SND_SOC_SOF_ACPI
-	select SND_SOC_SOF_BAYTRAIL  if SND_SOC_SOF_BAYTRAIL_SUPPORT
-	select SND_SOC_SOF_BROADWELL if SND_SOC_SOF_BROADWELL_SUPPORT
-	help
-	  This option is not user-selectable but automagically handled by
-	  'select' statements at a higher level.
-
 config SND_SOC_SOF_INTEL_PCI
 	def_tristate SND_SOC_SOF_PCI
 	select SND_SOC_SOF_MERRIFIELD  if SND_SOC_SOF_MERRIFIELD_SUPPORT
@@ -58,10 +50,12 @@ config SND_SOC_SOF_INTEL_COMMON
 	  This option is not user-selectable but automagically handled by
 	  'select' statements at a higher level.
 
-if SND_SOC_SOF_INTEL_ACPI
+if SND_SOC_SOF_ACPI
 
-config SND_SOC_SOF_BAYTRAIL_SUPPORT
+config SND_SOC_SOF_BAYTRAIL
 	bool "SOF support for Baytrail, Braswell and Cherrytrail"
+	select SND_SOC_SOF_INTEL_ATOM_HIFI_EP
+	select SND_INTEL_DSP_CONFIG
 	help
 	  This adds support for Sound Open Firmware for Intel(R) platforms
 	  using the Baytrail, Braswell or Cherrytrail processors.
@@ -75,17 +69,11 @@ config SND_SOC_SOF_BAYTRAIL_SUPPORT
 	  Say Y if you want to enable SOF on Baytrail/Cherrytrail.
 	  If unsure select "N".
 
-config SND_SOC_SOF_BAYTRAIL
-	tristate
-	select SND_SOC_SOF_INTEL_ATOM_HIFI_EP
-	select SND_INTEL_DSP_CONFIG
-	help
-	  This option is not user-selectable but automagically handled by
-	  'select' statements at a higher level.
-
-config SND_SOC_SOF_BROADWELL_SUPPORT
+config SND_SOC_SOF_BROADWELL
 	bool "SOF support for Broadwell"
 	select SND_INTEL_DSP_CONFIG
+	select SND_SOC_SOF_INTEL_COMMON
+	select SND_SOC_SOF_INTEL_HIFI_EP_IPC
 	help
 	  This adds support for Sound Open Firmware for Intel(R) platforms
 	  using the Broadwell processors.
@@ -100,14 +88,6 @@ config SND_SOC_SOF_BROADWELL_SUPPORT
 	  Say Y if you want to enable SOF on Broadwell.
 	  If unsure select "N".
 
-config SND_SOC_SOF_BROADWELL
-	tristate
-	select SND_SOC_SOF_INTEL_COMMON
-	select SND_SOC_SOF_INTEL_HIFI_EP_IPC
-	help
-	  This option is not user-selectable but automagically handled by
-	  'select' statements at a higher level.
-
 endif ## SND_SOC_SOF_INTEL_ACPI
 
 if SND_SOC_SOF_INTEL_PCI
diff --git a/sound/soc/sof/intel/bdw.c b/sound/soc/sof/intel/bdw.c
index 50a4a73e6b9f..542e885733a2 100644
--- a/sound/soc/sof/intel/bdw.c
+++ b/sound/soc/sof/intel/bdw.c
@@ -15,6 +15,8 @@
 #include <linux/module.h>
 #include <sound/sof.h>
 #include <sound/sof/xtensa.h>
+#include <sound/soc-acpi.h>
+#include <sound/soc-acpi-intel-match.h>
 #include "../ops.h"
 #include "shim.h"
 #include "../sof-audio.h"
@@ -590,7 +592,7 @@ static struct snd_soc_dai_driver bdw_dai[] = {
 };
 
 /* broadwell ops */
-const struct snd_sof_dsp_ops sof_bdw_ops = {
+static const struct snd_sof_dsp_ops sof_bdw_ops = {
 	/*Device init */
 	.probe          = bdw_probe,
 
@@ -651,13 +653,54 @@ const struct snd_sof_dsp_ops sof_bdw_ops = {
 
 	.arch_ops = &sof_xtensa_arch_ops,
 };
-EXPORT_SYMBOL_NS(sof_bdw_ops, SND_SOC_SOF_BROADWELL);
 
-const struct sof_intel_dsp_desc bdw_chip_info = {
+static const struct sof_intel_dsp_desc bdw_chip_info = {
 	.cores_num = 1,
 	.host_managed_cores_mask = 1,
 };
-EXPORT_SYMBOL_NS(bdw_chip_info, SND_SOC_SOF_BROADWELL);
+
+static const struct sof_dev_desc sof_acpi_broadwell_desc = {
+	.machines = snd_soc_acpi_intel_broadwell_machines,
+	.resindex_lpe_base = 0,
+	.resindex_pcicfg_base = 1,
+	.resindex_imr_base = -1,
+	.irqindex_host_ipc = 0,
+	.chip_info = &bdw_chip_info,
+	.default_fw_path = "intel/sof",
+	.default_tplg_path = "intel/sof-tplg",
+	.default_fw_filename = "sof-bdw.ri",
+	.nocodec_tplg_filename = "sof-bdw-nocodec.tplg",
+	.ops = &sof_bdw_ops,
+};
+
+static const struct acpi_device_id sof_broadwell_match[] = {
+	{ "INT3438", (unsigned long)&sof_acpi_broadwell_desc },
+	{ }
+};
+MODULE_DEVICE_TABLE(acpi, sof_broadwell_match);
+
+static int sof_broadwell_probe(struct platform_device *dev)
+{
+	int ret = snd_intel_acpi_dsp_driver_probe(dev, "INT3438");
+	if (ret != SND_INTEL_DSP_DRIVER_SOF) {
+		dev_dbg(dev, "SOF ACPI driver not selected, aborting probe\n");
+		return -ENODEV;
+	}
+
+	return sof_acpi_probe(dev, &sof_acpi_broadwell_desc);
+}
+
+/* acpi_driver definition */
+static struct platform_driver snd_sof_acpi_driver = {
+	.probe = sof_broadwell_probe,
+	.remove = sof_acpi_remove,
+	.driver = {
+		.name = "sof-audio-broadwell",
+		.pm = &sof_acpi_pm,
+		.acpi_match_table = sof_broadwell_match,
+	},
+};
+module_platform_driver(snd_sof_acpi_driver);
 
 MODULE_LICENSE("Dual BSD/GPL");
 MODULE_IMPORT_NS(SND_SOC_SOF_INTEL_HIFI_EP_IPC);
diff --git a/sound/soc/sof/intel/byt.c b/sound/soc/sof/intel/byt.c
index 19260dbecac5..9fa688c7711e 100644
--- a/sound/soc/sof/intel/byt.c
+++ b/sound/soc/sof/intel/byt.c
@@ -15,6 +15,8 @@
 #include <linux/module.h>
 #include <sound/sof.h>
 #include <sound/sof/xtensa.h>
+#include <sound/soc-acpi.h>
+#include <sound/soc-acpi-intel-match.h>
 #include "../ops.h"
 #include "shim.h"
 #include "../sof-audio.h"
@@ -657,8 +659,6 @@ EXPORT_SYMBOL_NS(tng_chip_info, SND_SOC_SOF_MERRIFIELD);
 
 #endif /* CONFIG_SND_SOC_SOF_MERRIFIELD */
 
-#if IS_ENABLED(CONFIG_SND_SOC_SOF_BAYTRAIL)
-
 static void byt_reset_dsp_disable_int(struct snd_sof_dev *sdev)
 {
 	/* Disable Interrupt from both sides */
@@ -822,7 +822,7 @@ static int byt_acpi_probe(struct snd_sof_dev *sdev)
 }
 
 /* baytrail ops */
-const struct snd_sof_dsp_ops sof_byt_ops = {
+static const struct snd_sof_dsp_ops sof_byt_ops = {
 	/* device init */
 	.probe		= byt_acpi_probe,
 	.remove		= byt_remove,
@@ -892,16 +892,14 @@ const struct snd_sof_dsp_ops sof_byt_ops = {
 
 	.arch_ops = &sof_xtensa_arch_ops,
 };
-EXPORT_SYMBOL_NS(sof_byt_ops, SND_SOC_SOF_BAYTRAIL);
 
-const struct sof_intel_dsp_desc byt_chip_info = {
+static const struct sof_intel_dsp_desc byt_chip_info = {
 	.cores_num = 1,
 	.host_managed_cores_mask = 1,
 };
-EXPORT_SYMBOL_NS(byt_chip_info, SND_SOC_SOF_BAYTRAIL);
 
 /* cherrytrail and braswell ops */
-const struct snd_sof_dsp_ops sof_cht_ops = {
+static const struct snd_sof_dsp_ops sof_cht_ops = {
 	/* device init */
 	.probe		= byt_acpi_probe,
 	.remove		= byt_remove,
@@ -972,15 +970,99 @@ const struct snd_sof_dsp_ops sof_cht_ops = {
 
 	.arch_ops = &sof_xtensa_arch_ops,
 };
-EXPORT_SYMBOL_NS(sof_cht_ops, SND_SOC_SOF_BAYTRAIL);
 
-const struct sof_intel_dsp_desc cht_chip_info = {
+static const struct sof_intel_dsp_desc cht_chip_info = {
 	.cores_num = 1,
 	.host_managed_cores_mask = 1,
 };
-EXPORT_SYMBOL_NS(cht_chip_info, SND_SOC_SOF_BAYTRAIL);
 
-#endif /* CONFIG_SND_SOC_SOF_BAYTRAIL */
+/* BYTCR uses different IRQ index */
+static const struct sof_dev_desc sof_acpi_baytrailcr_desc = {
+	.machines = snd_soc_acpi_intel_baytrail_machines,
+	.resindex_lpe_base = 0,
+	.resindex_pcicfg_base = 1,
+	.resindex_imr_base = 2,
+	.irqindex_host_ipc = 0,
+	.chip_info = &byt_chip_info,
+	.default_fw_path = "intel/sof",
+	.default_tplg_path = "intel/sof-tplg",
+	.default_fw_filename = "sof-byt.ri",
+	.nocodec_tplg_filename = "sof-byt-nocodec.tplg",
+	.ops = &sof_byt_ops,
+};
+
+static const struct sof_dev_desc sof_acpi_baytrail_desc = {
+	.machines = snd_soc_acpi_intel_baytrail_machines,
+	.resindex_lpe_base = 0,
+	.resindex_pcicfg_base = 1,
+	.resindex_imr_base = 2,
+	.irqindex_host_ipc = 5,
+	.chip_info = &byt_chip_info,
+	.default_fw_path = "intel/sof",
+	.default_tplg_path = "intel/sof-tplg",
+	.default_fw_filename = "sof-byt.ri",
+	.nocodec_tplg_filename = "sof-byt-nocodec.tplg",
+	.ops = &sof_byt_ops,
+};
+
+static const struct sof_dev_desc sof_acpi_cherrytrail_desc = {
+	.machines = snd_soc_acpi_intel_cherrytrail_machines,
+	.resindex_lpe_base = 0,
+	.resindex_pcicfg_base = 1,
+	.resindex_imr_base = 2,
+	.irqindex_host_ipc = 5,
+	.chip_info = &cht_chip_info,
+	.default_fw_path = "intel/sof",
+	.default_tplg_path = "intel/sof-tplg",
+	.default_fw_filename = "sof-cht.ri",
+	.nocodec_tplg_filename = "sof-cht-nocodec.tplg",
+	.ops = &sof_cht_ops,
+};
+
+static const struct acpi_device_id sof_baytrail_match[] = {
+	{ "80860F28", (unsigned long)&sof_acpi_baytrail_desc },
+	{ "808622A8", (unsigned long)&sof_acpi_cherrytrail_desc },
+	{ }
+};
+MODULE_DEVICE_TABLE(acpi, sof_baytrail_match);
+
+static int sof_baytrail_probe(struct platform_device *pdev)
+{
+	const struct sof_dev_desc *desc = device_get_match_data(&pdev->dev);
+	const struct acpi_device_id *id;
+
+	id = acpi_match_device(dev->driver->acpi_match_table, dev);
+	if (!id)
+		return -ENODEV;
+
+	ret = snd_intel_acpi_dsp_driver_probe(dev, id->id);
+	if (ret != SND_INTEL_DSP_DRIVER_SOF) {
+		dev_dbg(dev, "SOF ACPI driver not selected, aborting probe\n");
+		return -ENODEV;
+	}
+
+	dev_dbg(dev, "ACPI DSP detected");
+
+	if (!desc)
+		return -ENODEV;
+
+	if (desc == &sof_acpi_baytrail_desc && soc_intel_is_byt_cr(pdev))
+		desc = &sof_acpi_baytrailcr_desc;
+
+	return sof_acpi_probe(pdev, desc);
+}
+
+/* acpi_driver definition */
+static struct platform_driver snd_sof_baytrail_driver = {
+	.probe = sof_baytrail_probe,
+	.remove = sof_acpi_remove,
+	.driver = {
+		.name = "sof-audio-baytrail",
+		.pm = &sof_acpi_pm,
+		.acpi_match_table = sof_baytrail_match,
+	},
+};
+module_platform_driver(snd_sof_baytrail_driver);
 
 MODULE_LICENSE("Dual BSD/GPL");
 MODULE_IMPORT_NS(SND_SOC_SOF_INTEL_HIFI_EP_IPC);
diff --git a/sound/soc/sof/intel/shim.h b/sound/soc/sof/intel/shim.h
index 1e0afb5c8720..872116ee622b 100644
--- a/sound/soc/sof/intel/shim.h
+++ b/sound/soc/sof/intel/shim.h
@@ -166,14 +166,12 @@ struct sof_intel_dsp_desc {
 	int ssp_base_offset;		/* base address of the SSPs */
 };
 
+extern const struct dev_pm_ops sof_acpi_pm;
+extern int sof_acpi_probe(struct platform_device *pdev, const struct sof_dev_desc *desc);
+extern int sof_acpi_remove(struct platform_device *pdev);
+
 extern const struct snd_sof_dsp_ops sof_tng_ops;
-extern const struct snd_sof_dsp_ops sof_byt_ops;
-extern const struct snd_sof_dsp_ops sof_cht_ops;
-extern const struct snd_sof_dsp_ops sof_bdw_ops;
 
-extern const struct sof_intel_dsp_desc byt_chip_info;
-extern const struct sof_intel_dsp_desc cht_chip_info;
-extern const struct sof_intel_dsp_desc bdw_chip_info;
 extern const struct sof_intel_dsp_desc tng_chip_info;
 
 struct sof_intel_stream {
diff --git a/sound/soc/sof/sof-acpi-dev.c b/sound/soc/sof/sof-acpi-dev.c
index 2a369c2c6551..32654e042ec4 100644
--- a/sound/soc/sof/sof-acpi-dev.c
+++ b/sound/soc/sof/sof-acpi-dev.c
@@ -36,70 +36,7 @@ MODULE_PARM_DESC(sof_acpi_debug, "SOF ACPI debug options (0x0 all off)");
 
 #define SOF_ACPI_DISABLE_PM_RUNTIME BIT(0)
 
-#if IS_ENABLED(CONFIG_ACPI) && IS_ENABLED(CONFIG_SND_SOC_SOF_BROADWELL)
-static const struct sof_dev_desc sof_acpi_broadwell_desc = {
-	.machines = snd_soc_acpi_intel_broadwell_machines,
-	.resindex_lpe_base = 0,
-	.resindex_pcicfg_base = 1,
-	.resindex_imr_base = -1,
-	.irqindex_host_ipc = 0,
-	.chip_info = &bdw_chip_info,
-	.default_fw_path = "intel/sof",
-	.default_tplg_path = "intel/sof-tplg",
-	.default_fw_filename = "sof-bdw.ri",
-	.nocodec_tplg_filename = "sof-bdw-nocodec.tplg",
-	.ops = &sof_bdw_ops,
-};
-#endif
-
-#if IS_ENABLED(CONFIG_ACPI) && IS_ENABLED(CONFIG_SND_SOC_SOF_BAYTRAIL)
-
-/* BYTCR uses different IRQ index */
-static const struct sof_dev_desc sof_acpi_baytrailcr_desc = {
-	.machines = snd_soc_acpi_intel_baytrail_machines,
-	.resindex_lpe_base = 0,
-	.resindex_pcicfg_base = 1,
-	.resindex_imr_base = 2,
-	.irqindex_host_ipc = 0,
-	.chip_info = &byt_chip_info,
-	.default_fw_path = "intel/sof",
-	.default_tplg_path = "intel/sof-tplg",
-	.default_fw_filename = "sof-byt.ri",
-	.nocodec_tplg_filename = "sof-byt-nocodec.tplg",
-	.ops = &sof_byt_ops,
-};
-
-static const struct sof_dev_desc sof_acpi_baytrail_desc = {
-	.machines = snd_soc_acpi_intel_baytrail_machines,
-	.resindex_lpe_base = 0,
-	.resindex_pcicfg_base = 1,
-	.resindex_imr_base = 2,
-	.irqindex_host_ipc = 5,
-	.chip_info = &byt_chip_info,
-	.default_fw_path = "intel/sof",
-	.default_tplg_path = "intel/sof-tplg",
-	.default_fw_filename = "sof-byt.ri",
-	.nocodec_tplg_filename = "sof-byt-nocodec.tplg",
-	.ops = &sof_byt_ops,
-};
-
-static const struct sof_dev_desc sof_acpi_cherrytrail_desc = {
-	.machines = snd_soc_acpi_intel_cherrytrail_machines,
-	.resindex_lpe_base = 0,
-	.resindex_pcicfg_base = 1,
-	.resindex_imr_base = 2,
-	.irqindex_host_ipc = 5,
-	.chip_info = &cht_chip_info,
-	.default_fw_path = "intel/sof",
-	.default_tplg_path = "intel/sof-tplg",
-	.default_fw_filename = "sof-cht.ri",
-	.nocodec_tplg_filename = "sof-cht-nocodec.tplg",
-	.ops = &sof_cht_ops,
-};
-
-#endif
-
-static const struct dev_pm_ops sof_acpi_pm = {
+const struct dev_pm_ops sof_acpi_pm = {
 	SET_SYSTEM_SLEEP_PM_OPS(snd_sof_suspend, snd_sof_resume)
 	SET_RUNTIME_PM_OPS(snd_sof_runtime_suspend, snd_sof_runtime_resume,
 			   snd_sof_runtime_idle)
@@ -118,40 +55,17 @@ static void sof_acpi_probe_complete(struct device *dev)
 	pm_runtime_enable(dev);
 }
 
-static int sof_acpi_probe(struct platform_device *pdev)
+int sof_acpi_probe(struct platform_device *pdev, const struct sof_dev_desc *desc)
 {
 	struct device *dev = &pdev->dev;
-	const struct acpi_device_id *id;
-	const struct sof_dev_desc *desc;
 	struct snd_sof_pdata *sof_pdata;
 	const struct snd_sof_dsp_ops *ops;
 	int ret;
 
-	id = acpi_match_device(dev->driver->acpi_match_table, dev);
-	if (!id)
-		return -ENODEV;
-
-	ret = snd_intel_acpi_dsp_driver_probe(dev, id->id);
-	if (ret != SND_INTEL_DSP_DRIVER_ANY && ret != SND_INTEL_DSP_DRIVER_SOF) {
-		dev_dbg(dev, "SOF ACPI driver not selected, aborting probe\n");
-		return -ENODEV;
-	}
-
-	dev_dbg(dev, "ACPI DSP detected");
-
 	sof_pdata = devm_kzalloc(dev, sizeof(*sof_pdata), GFP_KERNEL);
 	if (!sof_pdata)
 		return -ENOMEM;
 
-	desc = device_get_match_data(dev);
-	if (!desc)
-		return -ENODEV;
-
-#if IS_ENABLED(CONFIG_ACPI) && IS_ENABLED(CONFIG_SND_SOC_SOF_BAYTRAIL)
-	if (desc == &sof_acpi_baytrail_desc && soc_intel_is_byt_cr(pdev))
-		desc = &sof_acpi_baytrailcr_desc;
-#endif
-
 	/* get ops for platform */
 	ops = desc->ops;
 	if (!ops) {
@@ -193,8 +107,9 @@ static int sof_acpi_probe(struct platform_device *pdev)
 
 	return ret;
 }
+EXPORT_SYMBOL_GPL(sof_acpi_probe);
 
-static int sof_acpi_remove(struct platform_device *pdev)
+int sof_acpi_remove(struct platform_device *pdev)
 {
 	if (!(sof_acpi_debug & SOF_ACPI_DISABLE_PM_RUNTIME))
 		pm_runtime_disable(&pdev->dev);
@@ -204,33 +119,6 @@ static int sof_acpi_remove(struct platform_device *pdev)
 
 	return 0;
 }
-
-#ifdef CONFIG_ACPI
-static const struct acpi_device_id sof_acpi_match[] = {
-#if IS_ENABLED(CONFIG_SND_SOC_SOF_BROADWELL)
-	{ "INT3438", (unsigned long)&sof_acpi_broadwell_desc },
-#endif
-#if IS_ENABLED(CONFIG_SND_SOC_SOF_BAYTRAIL)
-	{ "80860F28", (unsigned long)&sof_acpi_baytrail_desc },
-	{ "808622A8", (unsigned long)&sof_acpi_cherrytrail_desc },
-#endif
-	{ }
-};
-MODULE_DEVICE_TABLE(acpi, sof_acpi_match);
-#endif
-
-/* acpi_driver definition */
-static struct platform_driver snd_sof_acpi_driver = {
-	.probe = sof_acpi_probe,
-	.remove = sof_acpi_remove,
-	.driver = {
-		.name = "sof-audio-acpi",
-		.pm = &sof_acpi_pm,
-		.acpi_match_table = ACPI_PTR(sof_acpi_match),
-	},
-};
-module_platform_driver(snd_sof_acpi_driver);
+EXPORT_SYMBOL_GPL(sof_acpi_remove);
 
 MODULE_LICENSE("Dual BSD/GPL");
-MODULE_IMPORT_NS(SND_SOC_SOF_BAYTRAIL);
-MODULE_IMPORT_NS(SND_SOC_SOF_BROADWELL);
-- 
2.29.2

[PATCH] ASoC: SOF: Intel: avoid reverse module dependency

[Arnd Bergmann]

From: Arnd Bergmann <arnd@arndb.de>

The SOF-ACPI driver is backwards from the normal Linux model, it has a
generic driver that knows about all the specific drivers, as opposed to
having hardware specific drivers that link against a common framework.

This requires ugly Kconfig magic and leads to missed dependencies as
seen in this link error:

arm-linux-gnueabi-ld: sound/soc/sof/sof-pci-dev.o: in function `sof_acpi_probe':
sof-pci-dev.c:(.text+0x1c): undefined reference to `snd_intel_dsp_driver_probe'

Change it to use the normal probe order of starting with a specific
device in a driver, turning the sof-acpi-dev.c driver into a library.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 sound/soc/sof/intel/Kconfig  |  34 ++--------
 sound/soc/sof/intel/bdw.c    |  51 +++++++++++++--
 sound/soc/sof/intel/byt.c    | 104 +++++++++++++++++++++++++----
 sound/soc/sof/intel/shim.h   |  10 ++-
 sound/soc/sof/sof-acpi-dev.c | 122 ++---------------------------------
 5 files changed, 156 insertions(+), 165 deletions(-)

diff --git a/sound/soc/sof/intel/Kconfig b/sound/soc/sof/intel/Kconfig
index ae9ba834814e..ff9266413a06 100644
--- a/sound/soc/sof/intel/Kconfig
+++ b/sound/soc/sof/intel/Kconfig
@@ -9,14 +9,6 @@ config SND_SOC_SOF_INTEL_TOPLEVEL
 
 if SND_SOC_SOF_INTEL_TOPLEVEL
 
-config SND_SOC_SOF_INTEL_ACPI
-	def_tristate SND_SOC_SOF_ACPI
-	select SND_SOC_SOF_BAYTRAIL  if SND_SOC_SOF_BAYTRAIL_SUPPORT
-	select SND_SOC_SOF_BROADWELL if SND_SOC_SOF_BROADWELL_SUPPORT
-	help
-	  This option is not user-selectable but automagically handled by
-	  'select' statements at a higher level.
-
 config SND_SOC_SOF_INTEL_PCI
 	def_tristate SND_SOC_SOF_PCI
 	select SND_SOC_SOF_MERRIFIELD  if SND_SOC_SOF_MERRIFIELD_SUPPORT
@@ -58,10 +50,12 @@ config SND_SOC_SOF_INTEL_COMMON
 	  This option is not user-selectable but automagically handled by
 	  'select' statements at a higher level.
 
-if SND_SOC_SOF_INTEL_ACPI
+if SND_SOC_SOF_ACPI
 
-config SND_SOC_SOF_BAYTRAIL_SUPPORT
+config SND_SOC_SOF_BAYTRAIL
 	bool "SOF support for Baytrail, Braswell and Cherrytrail"
+	select SND_SOC_SOF_INTEL_ATOM_HIFI_EP
+	select SND_INTEL_DSP_CONFIG
 	help
 	  This adds support for Sound Open Firmware for Intel(R) platforms
 	  using the Baytrail, Braswell or Cherrytrail processors.
@@ -75,17 +69,11 @@ config SND_SOC_SOF_BAYTRAIL_SUPPORT
 	  Say Y if you want to enable SOF on Baytrail/Cherrytrail.
 	  If unsure select "N".
 
-config SND_SOC_SOF_BAYTRAIL
-	tristate
-	select SND_SOC_SOF_INTEL_ATOM_HIFI_EP
-	select SND_INTEL_DSP_CONFIG
-	help
-	  This option is not user-selectable but automagically handled by
-	  'select' statements at a higher level.
-
-config SND_SOC_SOF_BROADWELL_SUPPORT
+config SND_SOC_SOF_BROADWELL
 	bool "SOF support for Broadwell"
 	select SND_INTEL_DSP_CONFIG
+	select SND_SOC_SOF_INTEL_COMMON
+	select SND_SOC_SOF_INTEL_HIFI_EP_IPC
 	help
 	  This adds support for Sound Open Firmware for Intel(R) platforms
 	  using the Broadwell processors.
@@ -100,14 +88,6 @@ config SND_SOC_SOF_BROADWELL_SUPPORT
 	  Say Y if you want to enable SOF on Broadwell.
 	  If unsure select "N".
 
-config SND_SOC_SOF_BROADWELL
-	tristate
-	select SND_SOC_SOF_INTEL_COMMON
-	select SND_SOC_SOF_INTEL_HIFI_EP_IPC
-	help
-	  This option is not user-selectable but automagically handled by
-	  'select' statements at a higher level.
-
 endif ## SND_SOC_SOF_INTEL_ACPI
 
 if SND_SOC_SOF_INTEL_PCI
diff --git a/sound/soc/sof/intel/bdw.c b/sound/soc/sof/intel/bdw.c
index 50a4a73e6b9f..542e885733a2 100644
--- a/sound/soc/sof/intel/bdw.c
+++ b/sound/soc/sof/intel/bdw.c
@@ -15,6 +15,8 @@
 #include <linux/module.h>
 #include <sound/sof.h>
 #include <sound/sof/xtensa.h>
+#include <sound/soc-acpi.h>
+#include <sound/soc-acpi-intel-match.h>
 #include "../ops.h"
 #include "shim.h"
 #include "../sof-audio.h"
@@ -590,7 +592,7 @@ static struct snd_soc_dai_driver bdw_dai[] = {
 };
 
 /* broadwell ops */
-const struct snd_sof_dsp_ops sof_bdw_ops = {
+static const struct snd_sof_dsp_ops sof_bdw_ops = {
 	/*Device init */
 	.probe          = bdw_probe,
 
@@ -651,13 +653,54 @@ const struct snd_sof_dsp_ops sof_bdw_ops = {
 
 	.arch_ops = &sof_xtensa_arch_ops,
 };
-EXPORT_SYMBOL_NS(sof_bdw_ops, SND_SOC_SOF_BROADWELL);
 
-const struct sof_intel_dsp_desc bdw_chip_info = {
+static const struct sof_intel_dsp_desc bdw_chip_info = {
 	.cores_num = 1,
 	.host_managed_cores_mask = 1,
 };
-EXPORT_SYMBOL_NS(bdw_chip_info, SND_SOC_SOF_BROADWELL);
+
+static const struct sof_dev_desc sof_acpi_broadwell_desc = {
+	.machines = snd_soc_acpi_intel_broadwell_machines,
+	.resindex_lpe_base = 0,
+	.resindex_pcicfg_base = 1,
+	.resindex_imr_base = -1,
+	.irqindex_host_ipc = 0,
+	.chip_info = &bdw_chip_info,
+	.default_fw_path = "intel/sof",
+	.default_tplg_path = "intel/sof-tplg",
+	.default_fw_filename = "sof-bdw.ri",
+	.nocodec_tplg_filename = "sof-bdw-nocodec.tplg",
+	.ops = &sof_bdw_ops,
+};
+
+static const struct acpi_device_id sof_broadwell_match[] = {
+	{ "INT3438", (unsigned long)&sof_acpi_broadwell_desc },
+	{ }
+};
+MODULE_DEVICE_TABLE(acpi, sof_broadwell_match);
+
+static int sof_broadwell_probe(struct platform_device *dev)
+{
+	int ret = snd_intel_acpi_dsp_driver_probe(dev, "INT3438");
+	if (ret != SND_INTEL_DSP_DRIVER_SOF) {
+		dev_dbg(dev, "SOF ACPI driver not selected, aborting probe\n");
+		return -ENODEV;
+	}
+
+	return sof_acpi_probe(dev, &sof_acpi_broadwell_desc);
+}
+
+/* acpi_driver definition */
+static struct platform_driver snd_sof_acpi_driver = {
+	.probe = sof_broadwell_probe,
+	.remove = sof_acpi_remove,
+	.driver = {
+		.name = "sof-audio-broadwell",
+		.pm = &sof_acpi_pm,
+		.acpi_match_table = sof_broadwell_match,
+	},
+};
+module_platform_driver(snd_sof_acpi_driver);
 
 MODULE_LICENSE("Dual BSD/GPL");
 MODULE_IMPORT_NS(SND_SOC_SOF_INTEL_HIFI_EP_IPC);
diff --git a/sound/soc/sof/intel/byt.c b/sound/soc/sof/intel/byt.c
index 19260dbecac5..9fa688c7711e 100644
--- a/sound/soc/sof/intel/byt.c
+++ b/sound/soc/sof/intel/byt.c
@@ -15,6 +15,8 @@
 #include <linux/module.h>
 #include <sound/sof.h>
 #include <sound/sof/xtensa.h>
+#include <sound/soc-acpi.h>
+#include <sound/soc-acpi-intel-match.h>
 #include "../ops.h"
 #include "shim.h"
 #include "../sof-audio.h"
@@ -657,8 +659,6 @@ EXPORT_SYMBOL_NS(tng_chip_info, SND_SOC_SOF_MERRIFIELD);
 
 #endif /* CONFIG_SND_SOC_SOF_MERRIFIELD */
 
-#if IS_ENABLED(CONFIG_SND_SOC_SOF_BAYTRAIL)
-
 static void byt_reset_dsp_disable_int(struct snd_sof_dev *sdev)
 {
 	/* Disable Interrupt from both sides */
@@ -822,7 +822,7 @@ static int byt_acpi_probe(struct snd_sof_dev *sdev)
 }
 
 /* baytrail ops */
-const struct snd_sof_dsp_ops sof_byt_ops = {
+static const struct snd_sof_dsp_ops sof_byt_ops = {
 	/* device init */
 	.probe		= byt_acpi_probe,
 	.remove		= byt_remove,
@@ -892,16 +892,14 @@ const struct snd_sof_dsp_ops sof_byt_ops = {
 
 	.arch_ops = &sof_xtensa_arch_ops,
 };
-EXPORT_SYMBOL_NS(sof_byt_ops, SND_SOC_SOF_BAYTRAIL);
 
-const struct sof_intel_dsp_desc byt_chip_info = {
+static const struct sof_intel_dsp_desc byt_chip_info = {
 	.cores_num = 1,
 	.host_managed_cores_mask = 1,
 };
-EXPORT_SYMBOL_NS(byt_chip_info, SND_SOC_SOF_BAYTRAIL);
 
 /* cherrytrail and braswell ops */
-const struct snd_sof_dsp_ops sof_cht_ops = {
+static const struct snd_sof_dsp_ops sof_cht_ops = {
 	/* device init */
 	.probe		= byt_acpi_probe,
 	.remove		= byt_remove,
@@ -972,15 +970,99 @@ const struct snd_sof_dsp_ops sof_cht_ops = {
 
 	.arch_ops = &sof_xtensa_arch_ops,
 };
-EXPORT_SYMBOL_NS(sof_cht_ops, SND_SOC_SOF_BAYTRAIL);
 
-const struct sof_intel_dsp_desc cht_chip_info = {
+static const struct sof_intel_dsp_desc cht_chip_info = {
 	.cores_num = 1,
 	.host_managed_cores_mask = 1,
 };
-EXPORT_SYMBOL_NS(cht_chip_info, SND_SOC_SOF_BAYTRAIL);
 
-#endif /* CONFIG_SND_SOC_SOF_BAYTRAIL */
+/* BYTCR uses different IRQ index */
+static const struct sof_dev_desc sof_acpi_baytrailcr_desc = {
+	.machines = snd_soc_acpi_intel_baytrail_machines,
+	.resindex_lpe_base = 0,
+	.resindex_pcicfg_base = 1,
+	.resindex_imr_base = 2,
+	.irqindex_host_ipc = 0,
+	.chip_info = &byt_chip_info,
+	.default_fw_path = "intel/sof",
+	.default_tplg_path = "intel/sof-tplg",
+	.default_fw_filename = "sof-byt.ri",
+	.nocodec_tplg_filename = "sof-byt-nocodec.tplg",
+	.ops = &sof_byt_ops,
+};
+
+static const struct sof_dev_desc sof_acpi_baytrail_desc = {
+	.machines = snd_soc_acpi_intel_baytrail_machines,
+	.resindex_lpe_base = 0,
+	.resindex_pcicfg_base = 1,
+	.resindex_imr_base = 2,
+	.irqindex_host_ipc = 5,
+	.chip_info = &byt_chip_info,
+	.default_fw_path = "intel/sof",
+	.default_tplg_path = "intel/sof-tplg",
+	.default_fw_filename = "sof-byt.ri",
+	.nocodec_tplg_filename = "sof-byt-nocodec.tplg",
+	.ops = &sof_byt_ops,
+};
+
+static const struct sof_dev_desc sof_acpi_cherrytrail_desc = {
+	.machines = snd_soc_acpi_intel_cherrytrail_machines,
+	.resindex_lpe_base = 0,
+	.resindex_pcicfg_base = 1,
+	.resindex_imr_base = 2,
+	.irqindex_host_ipc = 5,
+	.chip_info = &cht_chip_info,
+	.default_fw_path = "intel/sof",
+	.default_tplg_path = "intel/sof-tplg",
+	.default_fw_filename = "sof-cht.ri",
+	.nocodec_tplg_filename = "sof-cht-nocodec.tplg",
+	.ops = &sof_cht_ops,
+};
+
+static const struct acpi_device_id sof_baytrail_match[] = {
+	{ "80860F28", (unsigned long)&sof_acpi_baytrail_desc },
+	{ "808622A8", (unsigned long)&sof_acpi_cherrytrail_desc },
+	{ }
+};
+MODULE_DEVICE_TABLE(acpi, sof_baytrail_match);
+
+static int sof_baytrail_probe(struct platform_device *pdev)
+{
+	const struct sof_dev_desc *desc = device_get_match_data(&pdev->dev);
+	const struct acpi_device_id *id;
+
+	id = acpi_match_device(dev->driver->acpi_match_table, dev);
+	if (!id)
+		return -ENODEV;
+
+	ret = snd_intel_acpi_dsp_driver_probe(dev, id->id);
+	if (ret != SND_INTEL_DSP_DRIVER_SOF) {
+		dev_dbg(dev, "SOF ACPI driver not selected, aborting probe\n");
+		return -ENODEV;
+	}
+
+	dev_dbg(dev, "ACPI DSP detected");
+
+	if (!desc)
+		return -ENODEV;
+
+	if (desc == &sof_acpi_baytrail_desc && soc_intel_is_byt_cr(pdev))
+		desc = &sof_acpi_baytrailcr_desc;
+
+	return sof_acpi_probe(pdev, desc);
+}
+
+/* acpi_driver definition */
+static struct platform_driver snd_sof_baytrail_driver = {
+	.probe = sof_baytrail_probe,
+	.remove = sof_acpi_remove,
+	.driver = {
+		.name = "sof-audio-baytrail",
+		.pm = &sof_acpi_pm,
+		.acpi_match_table = sof_baytrail_match,
+	},
+};
+module_platform_driver(snd_sof_baytrail_driver);
 
 MODULE_LICENSE("Dual BSD/GPL");
 MODULE_IMPORT_NS(SND_SOC_SOF_INTEL_HIFI_EP_IPC);
diff --git a/sound/soc/sof/intel/shim.h b/sound/soc/sof/intel/shim.h
index 1e0afb5c8720..872116ee622b 100644
--- a/sound/soc/sof/intel/shim.h
+++ b/sound/soc/sof/intel/shim.h
@@ -166,14 +166,12 @@ struct sof_intel_dsp_desc {
 	int ssp_base_offset;		/* base address of the SSPs */
 };
 
+extern const struct dev_pm_ops sof_acpi_pm;
+extern int sof_acpi_probe(struct platform_device *pdev, const struct sof_dev_desc *desc);
+extern int sof_acpi_remove(struct platform_device *pdev);
+
 extern const struct snd_sof_dsp_ops sof_tng_ops;
-extern const struct snd_sof_dsp_ops sof_byt_ops;
-extern const struct snd_sof_dsp_ops sof_cht_ops;
-extern const struct snd_sof_dsp_ops sof_bdw_ops;
 
-extern const struct sof_intel_dsp_desc byt_chip_info;
-extern const struct sof_intel_dsp_desc cht_chip_info;
-extern const struct sof_intel_dsp_desc bdw_chip_info;
 extern const struct sof_intel_dsp_desc tng_chip_info;
 
 struct sof_intel_stream {
diff --git a/sound/soc/sof/sof-acpi-dev.c b/sound/soc/sof/sof-acpi-dev.c
index 2a369c2c6551..32654e042ec4 100644
--- a/sound/soc/sof/sof-acpi-dev.c
+++ b/sound/soc/sof/sof-acpi-dev.c
@@ -36,70 +36,7 @@ MODULE_PARM_DESC(sof_acpi_debug, "SOF ACPI debug options (0x0 all off)");
 
 #define SOF_ACPI_DISABLE_PM_RUNTIME BIT(0)
 
-#if IS_ENABLED(CONFIG_ACPI) && IS_ENABLED(CONFIG_SND_SOC_SOF_BROADWELL)
-static const struct sof_dev_desc sof_acpi_broadwell_desc = {
-	.machines = snd_soc_acpi_intel_broadwell_machines,
-	.resindex_lpe_base = 0,
-	.resindex_pcicfg_base = 1,
-	.resindex_imr_base = -1,
-	.irqindex_host_ipc = 0,
-	.chip_info = &bdw_chip_info,
-	.default_fw_path = "intel/sof",
-	.default_tplg_path = "intel/sof-tplg",
-	.default_fw_filename = "sof-bdw.ri",
-	.nocodec_tplg_filename = "sof-bdw-nocodec.tplg",
-	.ops = &sof_bdw_ops,
-};
-#endif
-
-#if IS_ENABLED(CONFIG_ACPI) && IS_ENABLED(CONFIG_SND_SOC_SOF_BAYTRAIL)
-
-/* BYTCR uses different IRQ index */
-static const struct sof_dev_desc sof_acpi_baytrailcr_desc = {
-	.machines = snd_soc_acpi_intel_baytrail_machines,
-	.resindex_lpe_base = 0,
-	.resindex_pcicfg_base = 1,
-	.resindex_imr_base = 2,
-	.irqindex_host_ipc = 0,
-	.chip_info = &byt_chip_info,
-	.default_fw_path = "intel/sof",
-	.default_tplg_path = "intel/sof-tplg",
-	.default_fw_filename = "sof-byt.ri",
-	.nocodec_tplg_filename = "sof-byt-nocodec.tplg",
-	.ops = &sof_byt_ops,
-};
-
-static const struct sof_dev_desc sof_acpi_baytrail_desc = {
-	.machines = snd_soc_acpi_intel_baytrail_machines,
-	.resindex_lpe_base = 0,
-	.resindex_pcicfg_base = 1,
-	.resindex_imr_base = 2,
-	.irqindex_host_ipc = 5,
-	.chip_info = &byt_chip_info,
-	.default_fw_path = "intel/sof",
-	.default_tplg_path = "intel/sof-tplg",
-	.default_fw_filename = "sof-byt.ri",
-	.nocodec_tplg_filename = "sof-byt-nocodec.tplg",
-	.ops = &sof_byt_ops,
-};
-
-static const struct sof_dev_desc sof_acpi_cherrytrail_desc = {
-	.machines = snd_soc_acpi_intel_cherrytrail_machines,
-	.resindex_lpe_base = 0,
-	.resindex_pcicfg_base = 1,
-	.resindex_imr_base = 2,
-	.irqindex_host_ipc = 5,
-	.chip_info = &cht_chip_info,
-	.default_fw_path = "intel/sof",
-	.default_tplg_path = "intel/sof-tplg",
-	.default_fw_filename = "sof-cht.ri",
-	.nocodec_tplg_filename = "sof-cht-nocodec.tplg",
-	.ops = &sof_cht_ops,
-};
-
-#endif
-
-static const struct dev_pm_ops sof_acpi_pm = {
+const struct dev_pm_ops sof_acpi_pm = {
 	SET_SYSTEM_SLEEP_PM_OPS(snd_sof_suspend, snd_sof_resume)
 	SET_RUNTIME_PM_OPS(snd_sof_runtime_suspend, snd_sof_runtime_resume,
 			   snd_sof_runtime_idle)
@@ -118,40 +55,17 @@ static void sof_acpi_probe_complete(struct device *dev)
 	pm_runtime_enable(dev);
 }
 
-static int sof_acpi_probe(struct platform_device *pdev)
+int sof_acpi_probe(struct platform_device *pdev, const struct sof_dev_desc *desc)
 {
 	struct device *dev = &pdev->dev;
-	const struct acpi_device_id *id;
-	const struct sof_dev_desc *desc;
 	struct snd_sof_pdata *sof_pdata;
 	const struct snd_sof_dsp_ops *ops;
 	int ret;
 
-	id = acpi_match_device(dev->driver->acpi_match_table, dev);
-	if (!id)
-		return -ENODEV;
-
-	ret = snd_intel_acpi_dsp_driver_probe(dev, id->id);
-	if (ret != SND_INTEL_DSP_DRIVER_ANY && ret != SND_INTEL_DSP_DRIVER_SOF) {
-		dev_dbg(dev, "SOF ACPI driver not selected, aborting probe\n");
-		return -ENODEV;
-	}
-
-	dev_dbg(dev, "ACPI DSP detected");
-
 	sof_pdata = devm_kzalloc(dev, sizeof(*sof_pdata), GFP_KERNEL);
 	if (!sof_pdata)
 		return -ENOMEM;
 
-	desc = device_get_match_data(dev);
-	if (!desc)
-		return -ENODEV;
-
-#if IS_ENABLED(CONFIG_ACPI) && IS_ENABLED(CONFIG_SND_SOC_SOF_BAYTRAIL)
-	if (desc == &sof_acpi_baytrail_desc && soc_intel_is_byt_cr(pdev))
-		desc = &sof_acpi_baytrailcr_desc;
-#endif
-
 	/* get ops for platform */
 	ops = desc->ops;
 	if (!ops) {
@@ -193,8 +107,9 @@ static int sof_acpi_probe(struct platform_device *pdev)
 
 	return ret;
 }
+EXPORT_SYMBOL_GPL(sof_acpi_probe);
 
-static int sof_acpi_remove(struct platform_device *pdev)
+int sof_acpi_remove(struct platform_device *pdev)
 {
 	if (!(sof_acpi_debug & SOF_ACPI_DISABLE_PM_RUNTIME))
 		pm_runtime_disable(&pdev->dev);
@@ -204,33 +119,6 @@ static int sof_acpi_remove(struct platform_device *pdev)
 
 	return 0;
 }
-
-#ifdef CONFIG_ACPI
-static const struct acpi_device_id sof_acpi_match[] = {
-#if IS_ENABLED(CONFIG_SND_SOC_SOF_BROADWELL)
-	{ "INT3438", (unsigned long)&sof_acpi_broadwell_desc },
-#endif
-#if IS_ENABLED(CONFIG_SND_SOC_SOF_BAYTRAIL)
-	{ "80860F28", (unsigned long)&sof_acpi_baytrail_desc },
-	{ "808622A8", (unsigned long)&sof_acpi_cherrytrail_desc },
-#endif
-	{ }
-};
-MODULE_DEVICE_TABLE(acpi, sof_acpi_match);
-#endif
-
-/* acpi_driver definition */
-static struct platform_driver snd_sof_acpi_driver = {
-	.probe = sof_acpi_probe,
-	.remove = sof_acpi_remove,
-	.driver = {
-		.name = "sof-audio-acpi",
-		.pm = &sof_acpi_pm,
-		.acpi_match_table = ACPI_PTR(sof_acpi_match),
-	},
-};
-module_platform_driver(snd_sof_acpi_driver);
+EXPORT_SYMBOL_GPL(sof_acpi_remove);
 
 MODULE_LICENSE("Dual BSD/GPL");
-MODULE_IMPORT_NS(SND_SOC_SOF_BAYTRAIL);
-MODULE_IMPORT_NS(SND_SOC_SOF_BROADWELL);
-- 
2.29.2

Re: [PATCH] ASoC: SOF: Intel: avoid reverse module dependency

[Arnd Bergmann]

On Tue, Jan 5, 2021 at 8:07 PM Arnd Bergmann <arnd@kernel.org> wrote:
>
> From: Arnd Bergmann <arnd@arndb.de>
>
> The SOF-ACPI driver is backwards from the normal Linux model, it has a
> generic driver that knows about all the specific drivers, as opposed to
> having hardware specific drivers that link against a common framework.
>
> This requires ugly Kconfig magic and leads to missed dependencies as
> seen in this link error:
>
> arm-linux-gnueabi-ld: sound/soc/sof/sof-pci-dev.o: in function `sof_acpi_probe':
> sof-pci-dev.c:(.text+0x1c): undefined reference to `snd_intel_dsp_driver_probe'
>
> Change it to use the normal probe order of starting with a specific
> device in a driver, turning the sof-acpi-dev.c driver into a library.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

There were a couple of build failures introduced by this version. I have
one that does build now, and can post that if others think this is the
direction they want to go.

      Arnd

Re: [PATCH] ASoC: SOF: Intel: avoid reverse module dependency

[Arnd Bergmann]

On Tue, Jan 5, 2021 at 8:07 PM Arnd Bergmann <arnd@kernel.org> wrote:
>
> From: Arnd Bergmann <arnd@arndb.de>
>
> The SOF-ACPI driver is backwards from the normal Linux model, it has a
> generic driver that knows about all the specific drivers, as opposed to
> having hardware specific drivers that link against a common framework.
>
> This requires ugly Kconfig magic and leads to missed dependencies as
> seen in this link error:
>
> arm-linux-gnueabi-ld: sound/soc/sof/sof-pci-dev.o: in function `sof_acpi_probe':
> sof-pci-dev.c:(.text+0x1c): undefined reference to `snd_intel_dsp_driver_probe'
>
> Change it to use the normal probe order of starting with a specific
> device in a driver, turning the sof-acpi-dev.c driver into a library.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

There were a couple of build failures introduced by this version. I have
one that does build now, and can post that if others think this is the
direction they want to go.

      Arnd

Re: [PATCH] ASoC: SOF: Intel: avoid reverse module dependency

[Kai Vehmanen]

Hi Arnd,

On Wed, 6 Jan 2021, Arnd Bergmann wrote:

> On Tue, Jan 5, 2021 at 8:07 PM Arnd Bergmann <arnd@kernel.org> wrote:
> > Change it to use the normal probe order of starting with a specific
> > device in a driver, turning the sof-acpi-dev.c driver into a library.
> 
> There were a couple of build failures introduced by this version. I have
> one that does build now, and can post that if others think this is the
> direction they want to go.

thanks for the follow-up. We have many SOF maintainers still out on 
holidays this week, so let's give some time for people to digest. This is 
certainly a big change. The version you posted is already sufficient to 
describe the idea for sure.

Br, Kai

Re: [PATCH] ASoC: SOF: Intel: avoid reverse module dependency

[Kai Vehmanen]

Hi Arnd,

On Wed, 6 Jan 2021, Arnd Bergmann wrote:

> On Tue, Jan 5, 2021 at 8:07 PM Arnd Bergmann <arnd@kernel.org> wrote:
> > Change it to use the normal probe order of starting with a specific
> > device in a driver, turning the sof-acpi-dev.c driver into a library.
> 
> There were a couple of build failures introduced by this version. I have
> one that does build now, and can post that if others think this is the
> direction they want to go.

thanks for the follow-up. We have many SOF maintainers still out on 
holidays this week, so let's give some time for people to digest. This is 
certainly a big change. The version you posted is already sufficient to 
describe the idea for sure.

Br, Kai

Re: [PATCH] ASoC: SOF: Intel: avoid reverse module dependency

[Pierre-Louis Bossart]

On 1/5/21 1:07 PM, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> The SOF-ACPI driver is backwards from the normal Linux model, it has a
> generic driver that knows about all the specific drivers, as opposed to
> having hardware specific drivers that link against a common framework.
> 
> This requires ugly Kconfig magic and leads to missed dependencies as
> seen in this link error:
> 
> arm-linux-gnueabi-ld: sound/soc/sof/sof-pci-dev.o: in function `sof_acpi_probe':
> sof-pci-dev.c:(.text+0x1c): undefined reference to `snd_intel_dsp_driver_probe'
> 
> Change it to use the normal probe order of starting with a specific
> device in a driver, turning the sof-acpi-dev.c driver into a library.

Thanks Arnd for reporting all this, much appreciated.

The initial design was that we would have one generic platform_driver 
(ACPI) and one generic PCI driver that would deal with all known IDs, 
with descriptors that would point ops and callbacks defined in 
device-specific drivers. It's how all Intel drivers worked so far, from 
HDaudio to Atom/SST and Skylake.

It's not that ugly, but to Arnd's point we do have a lot of #if 
IS_ENABLED at the top level with a larger and larger table of IDs, along 
with Kconfig magic indeed to propagate constraints from top-level to 
device-specific drivers. The error with DSP_CONFIG comes from the fact 
that this never belonged at the top-level, or should have been 
conditionally invoked, as noted by Takashi.

That said, the initial design which dates from 2017 can be revisited now 
that we start having quite a few platforms and more coming. What Arnd 
suggests isn't without merits, it would indeed turn the generic code 
into generic helpers, and have all the platform IDs maintained in 
device-specific drivers. It's a more distributed/scalable solution, the 
only minor drawback I see is that it would require multiple instances of 
the 'platform_driver' and 'pci_driver' structures.

I would also want to keep the top-level selection so that ACPI/PCI/DT 
modules can be disabled in one shot, that would mean an additional 
change to the Makefiles since e.g.
obj-$(CONFIG_SND_SOC_SOF_ACPI) += snd-sof-acpi.o
would need to be set somehow.

Since this is going to be a really invasive change, and past experience 
shows that mucking with Kconfigs will invariably raise a number of 
broken corner cases, if there is support from Mark/Takashi/Jaroslav on 
this idea, we should first test it in the SOF tree so that we get a good 
test coverage and don't break too many eggs in Mark's tree. We would 
also need to concurrently change our CI scripts which are dependent on 
module names.

Also maybe in a first pass we can remove the compilation error with 
IS_REACHABLE and in a second pass do more invasive surgery?

> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>   sound/soc/sof/intel/Kconfig  |  34 ++--------
>   sound/soc/sof/intel/bdw.c    |  51 +++++++++++++--
>   sound/soc/sof/intel/byt.c    | 104 +++++++++++++++++++++++++----
>   sound/soc/sof/intel/shim.h   |  10 ++-
>   sound/soc/sof/sof-acpi-dev.c | 122 ++---------------------------------
>   5 files changed, 156 insertions(+), 165 deletions(-)
> 
> diff --git a/sound/soc/sof/intel/Kconfig b/sound/soc/sof/intel/Kconfig
> index ae9ba834814e..ff9266413a06 100644
> --- a/sound/soc/sof/intel/Kconfig
> +++ b/sound/soc/sof/intel/Kconfig
> @@ -9,14 +9,6 @@ config SND_SOC_SOF_INTEL_TOPLEVEL
>   
>   if SND_SOC_SOF_INTEL_TOPLEVEL
>   
> -config SND_SOC_SOF_INTEL_ACPI
> -	def_tristate SND_SOC_SOF_ACPI
> -	select SND_SOC_SOF_BAYTRAIL  if SND_SOC_SOF_BAYTRAIL_SUPPORT
> -	select SND_SOC_SOF_BROADWELL if SND_SOC_SOF_BROADWELL_SUPPORT
> -	help
> -	  This option is not user-selectable but automagically handled by
> -	  'select' statements at a higher level.
> -
>   config SND_SOC_SOF_INTEL_PCI
>   	def_tristate SND_SOC_SOF_PCI
>   	select SND_SOC_SOF_MERRIFIELD  if SND_SOC_SOF_MERRIFIELD_SUPPORT
> @@ -58,10 +50,12 @@ config SND_SOC_SOF_INTEL_COMMON
>   	  This option is not user-selectable but automagically handled by
>   	  'select' statements at a higher level.
>   
> -if SND_SOC_SOF_INTEL_ACPI
> +if SND_SOC_SOF_ACPI
>   
> -config SND_SOC_SOF_BAYTRAIL_SUPPORT
> +config SND_SOC_SOF_BAYTRAIL
>   	bool "SOF support for Baytrail, Braswell and Cherrytrail"
> +	select SND_SOC_SOF_INTEL_ATOM_HIFI_EP
> +	select SND_INTEL_DSP_CONFIG
>   	help
>   	  This adds support for Sound Open Firmware for Intel(R) platforms
>   	  using the Baytrail, Braswell or Cherrytrail processors.
> @@ -75,17 +69,11 @@ config SND_SOC_SOF_BAYTRAIL_SUPPORT
>   	  Say Y if you want to enable SOF on Baytrail/Cherrytrail.
>   	  If unsure select "N".
>   
> -config SND_SOC_SOF_BAYTRAIL
> -	tristate
> -	select SND_SOC_SOF_INTEL_ATOM_HIFI_EP
> -	select SND_INTEL_DSP_CONFIG
> -	help
> -	  This option is not user-selectable but automagically handled by
> -	  'select' statements at a higher level.
> -
> -config SND_SOC_SOF_BROADWELL_SUPPORT
> +config SND_SOC_SOF_BROADWELL
>   	bool "SOF support for Broadwell"
>   	select SND_INTEL_DSP_CONFIG
> +	select SND_SOC_SOF_INTEL_COMMON
> +	select SND_SOC_SOF_INTEL_HIFI_EP_IPC
>   	help
>   	  This adds support for Sound Open Firmware for Intel(R) platforms
>   	  using the Broadwell processors.
> @@ -100,14 +88,6 @@ config SND_SOC_SOF_BROADWELL_SUPPORT
>   	  Say Y if you want to enable SOF on Broadwell.
>   	  If unsure select "N".
>   
> -config SND_SOC_SOF_BROADWELL
> -	tristate
> -	select SND_SOC_SOF_INTEL_COMMON
> -	select SND_SOC_SOF_INTEL_HIFI_EP_IPC
> -	help
> -	  This option is not user-selectable but automagically handled by
> -	  'select' statements at a higher level.
> -
>   endif ## SND_SOC_SOF_INTEL_ACPI
>   
>   if SND_SOC_SOF_INTEL_PCI
> diff --git a/sound/soc/sof/intel/bdw.c b/sound/soc/sof/intel/bdw.c
> index 50a4a73e6b9f..542e885733a2 100644
> --- a/sound/soc/sof/intel/bdw.c
> +++ b/sound/soc/sof/intel/bdw.c
> @@ -15,6 +15,8 @@
>   #include <linux/module.h>
>   #include <sound/sof.h>
>   #include <sound/sof/xtensa.h>
> +#include <sound/soc-acpi.h>
> +#include <sound/soc-acpi-intel-match.h>
>   #include "../ops.h"
>   #include "shim.h"
>   #include "../sof-audio.h"
> @@ -590,7 +592,7 @@ static struct snd_soc_dai_driver bdw_dai[] = {
>   };
>   
>   /* broadwell ops */
> -const struct snd_sof_dsp_ops sof_bdw_ops = {
> +static const struct snd_sof_dsp_ops sof_bdw_ops = {
>   	/*Device init */
>   	.probe          = bdw_probe,
>   
> @@ -651,13 +653,54 @@ const struct snd_sof_dsp_ops sof_bdw_ops = {
>   
>   	.arch_ops = &sof_xtensa_arch_ops,
>   };
> -EXPORT_SYMBOL_NS(sof_bdw_ops, SND_SOC_SOF_BROADWELL);
>   
> -const struct sof_intel_dsp_desc bdw_chip_info = {
> +static const struct sof_intel_dsp_desc bdw_chip_info = {
>   	.cores_num = 1,
>   	.host_managed_cores_mask = 1,
>   };
> -EXPORT_SYMBOL_NS(bdw_chip_info, SND_SOC_SOF_BROADWELL);
> +
> +static const struct sof_dev_desc sof_acpi_broadwell_desc = {
> +	.machines = snd_soc_acpi_intel_broadwell_machines,
> +	.resindex_lpe_base = 0,
> +	.resindex_pcicfg_base = 1,
> +	.resindex_imr_base = -1,
> +	.irqindex_host_ipc = 0,
> +	.chip_info = &bdw_chip_info,
> +	.default_fw_path = "intel/sof",
> +	.default_tplg_path = "intel/sof-tplg",
> +	.default_fw_filename = "sof-bdw.ri",
> +	.nocodec_tplg_filename = "sof-bdw-nocodec.tplg",
> +	.ops = &sof_bdw_ops,
> +};
> +
> +static const struct acpi_device_id sof_broadwell_match[] = {
> +	{ "INT3438", (unsigned long)&sof_acpi_broadwell_desc },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(acpi, sof_broadwell_match);
> +
> +static int sof_broadwell_probe(struct platform_device *dev)
> +{
> +	int ret = snd_intel_acpi_dsp_driver_probe(dev, "INT3438");
> +	if (ret != SND_INTEL_DSP_DRIVER_SOF) {
> +		dev_dbg(dev, "SOF ACPI driver not selected, aborting probe\n");
> +		return -ENODEV;
> +	}
> +
> +	return sof_acpi_probe(dev, &sof_acpi_broadwell_desc);
> +}
> +
> +/* acpi_driver definition */
> +static struct platform_driver snd_sof_acpi_driver = {
> +	.probe = sof_broadwell_probe,
> +	.remove = sof_acpi_remove,
> +	.driver = {
> +		.name = "sof-audio-broadwell",
> +		.pm = &sof_acpi_pm,
> +		.acpi_match_table = sof_broadwell_match,
> +	},
> +};
> +module_platform_driver(snd_sof_acpi_driver);
>   
>   MODULE_LICENSE("Dual BSD/GPL");
>   MODULE_IMPORT_NS(SND_SOC_SOF_INTEL_HIFI_EP_IPC);
> diff --git a/sound/soc/sof/intel/byt.c b/sound/soc/sof/intel/byt.c
> index 19260dbecac5..9fa688c7711e 100644
> --- a/sound/soc/sof/intel/byt.c
> +++ b/sound/soc/sof/intel/byt.c
> @@ -15,6 +15,8 @@
>   #include <linux/module.h>
>   #include <sound/sof.h>
>   #include <sound/sof/xtensa.h>
> +#include <sound/soc-acpi.h>
> +#include <sound/soc-acpi-intel-match.h>
>   #include "../ops.h"
>   #include "shim.h"
>   #include "../sof-audio.h"
> @@ -657,8 +659,6 @@ EXPORT_SYMBOL_NS(tng_chip_info, SND_SOC_SOF_MERRIFIELD);
>   
>   #endif /* CONFIG_SND_SOC_SOF_MERRIFIELD */
>   
> -#if IS_ENABLED(CONFIG_SND_SOC_SOF_BAYTRAIL)
> -
>   static void byt_reset_dsp_disable_int(struct snd_sof_dev *sdev)
>   {
>   	/* Disable Interrupt from both sides */
> @@ -822,7 +822,7 @@ static int byt_acpi_probe(struct snd_sof_dev *sdev)
>   }
>   
>   /* baytrail ops */
> -const struct snd_sof_dsp_ops sof_byt_ops = {
> +static const struct snd_sof_dsp_ops sof_byt_ops = {
>   	/* device init */
>   	.probe		= byt_acpi_probe,
>   	.remove		= byt_remove,
> @@ -892,16 +892,14 @@ const struct snd_sof_dsp_ops sof_byt_ops = {
>   
>   	.arch_ops = &sof_xtensa_arch_ops,
>   };
> -EXPORT_SYMBOL_NS(sof_byt_ops, SND_SOC_SOF_BAYTRAIL);
>   
> -const struct sof_intel_dsp_desc byt_chip_info = {
> +static const struct sof_intel_dsp_desc byt_chip_info = {
>   	.cores_num = 1,
>   	.host_managed_cores_mask = 1,
>   };
> -EXPORT_SYMBOL_NS(byt_chip_info, SND_SOC_SOF_BAYTRAIL);
>   
>   /* cherrytrail and braswell ops */
> -const struct snd_sof_dsp_ops sof_cht_ops = {
> +static const struct snd_sof_dsp_ops sof_cht_ops = {
>   	/* device init */
>   	.probe		= byt_acpi_probe,
>   	.remove		= byt_remove,
> @@ -972,15 +970,99 @@ const struct snd_sof_dsp_ops sof_cht_ops = {
>   
>   	.arch_ops = &sof_xtensa_arch_ops,
>   };
> -EXPORT_SYMBOL_NS(sof_cht_ops, SND_SOC_SOF_BAYTRAIL);
>   
> -const struct sof_intel_dsp_desc cht_chip_info = {
> +static const struct sof_intel_dsp_desc cht_chip_info = {
>   	.cores_num = 1,
>   	.host_managed_cores_mask = 1,
>   };
> -EXPORT_SYMBOL_NS(cht_chip_info, SND_SOC_SOF_BAYTRAIL);
>   
> -#endif /* CONFIG_SND_SOC_SOF_BAYTRAIL */
> +/* BYTCR uses different IRQ index */
> +static const struct sof_dev_desc sof_acpi_baytrailcr_desc = {
> +	.machines = snd_soc_acpi_intel_baytrail_machines,
> +	.resindex_lpe_base = 0,
> +	.resindex_pcicfg_base = 1,
> +	.resindex_imr_base = 2,
> +	.irqindex_host_ipc = 0,
> +	.chip_info = &byt_chip_info,
> +	.default_fw_path = "intel/sof",
> +	.default_tplg_path = "intel/sof-tplg",
> +	.default_fw_filename = "sof-byt.ri",
> +	.nocodec_tplg_filename = "sof-byt-nocodec.tplg",
> +	.ops = &sof_byt_ops,
> +};
> +
> +static const struct sof_dev_desc sof_acpi_baytrail_desc = {
> +	.machines = snd_soc_acpi_intel_baytrail_machines,
> +	.resindex_lpe_base = 0,
> +	.resindex_pcicfg_base = 1,
> +	.resindex_imr_base = 2,
> +	.irqindex_host_ipc = 5,
> +	.chip_info = &byt_chip_info,
> +	.default_fw_path = "intel/sof",
> +	.default_tplg_path = "intel/sof-tplg",
> +	.default_fw_filename = "sof-byt.ri",
> +	.nocodec_tplg_filename = "sof-byt-nocodec.tplg",
> +	.ops = &sof_byt_ops,
> +};
> +
> +static const struct sof_dev_desc sof_acpi_cherrytrail_desc = {
> +	.machines = snd_soc_acpi_intel_cherrytrail_machines,
> +	.resindex_lpe_base = 0,
> +	.resindex_pcicfg_base = 1,
> +	.resindex_imr_base = 2,
> +	.irqindex_host_ipc = 5,
> +	.chip_info = &cht_chip_info,
> +	.default_fw_path = "intel/sof",
> +	.default_tplg_path = "intel/sof-tplg",
> +	.default_fw_filename = "sof-cht.ri",
> +	.nocodec_tplg_filename = "sof-cht-nocodec.tplg",
> +	.ops = &sof_cht_ops,
> +};
> +
> +static const struct acpi_device_id sof_baytrail_match[] = {
> +	{ "80860F28", (unsigned long)&sof_acpi_baytrail_desc },
> +	{ "808622A8", (unsigned long)&sof_acpi_cherrytrail_desc },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(acpi, sof_baytrail_match);
> +
> +static int sof_baytrail_probe(struct platform_device *pdev)
> +{
> +	const struct sof_dev_desc *desc = device_get_match_data(&pdev->dev);
> +	const struct acpi_device_id *id;
> +
> +	id = acpi_match_device(dev->driver->acpi_match_table, dev);
> +	if (!id)
> +		return -ENODEV;
> +
> +	ret = snd_intel_acpi_dsp_driver_probe(dev, id->id);
> +	if (ret != SND_INTEL_DSP_DRIVER_SOF) {
> +		dev_dbg(dev, "SOF ACPI driver not selected, aborting probe\n");
> +		return -ENODEV;
> +	}
> +
> +	dev_dbg(dev, "ACPI DSP detected");
> +
> +	if (!desc)
> +		return -ENODEV;
> +
> +	if (desc == &sof_acpi_baytrail_desc && soc_intel_is_byt_cr(pdev))
> +		desc = &sof_acpi_baytrailcr_desc;
> +
> +	return sof_acpi_probe(pdev, desc);
> +}
> +
> +/* acpi_driver definition */
> +static struct platform_driver snd_sof_baytrail_driver = {
> +	.probe = sof_baytrail_probe,
> +	.remove = sof_acpi_remove,
> +	.driver = {
> +		.name = "sof-audio-baytrail",
> +		.pm = &sof_acpi_pm,
> +		.acpi_match_table = sof_baytrail_match,
> +	},
> +};
> +module_platform_driver(snd_sof_baytrail_driver);
>   
>   MODULE_LICENSE("Dual BSD/GPL");
>   MODULE_IMPORT_NS(SND_SOC_SOF_INTEL_HIFI_EP_IPC);
> diff --git a/sound/soc/sof/intel/shim.h b/sound/soc/sof/intel/shim.h
> index 1e0afb5c8720..872116ee622b 100644
> --- a/sound/soc/sof/intel/shim.h
> +++ b/sound/soc/sof/intel/shim.h
> @@ -166,14 +166,12 @@ struct sof_intel_dsp_desc {
>   	int ssp_base_offset;		/* base address of the SSPs */
>   };
>   
> +extern const struct dev_pm_ops sof_acpi_pm;
> +extern int sof_acpi_probe(struct platform_device *pdev, const struct sof_dev_desc *desc);
> +extern int sof_acpi_remove(struct platform_device *pdev);
> +
>   extern const struct snd_sof_dsp_ops sof_tng_ops;
> -extern const struct snd_sof_dsp_ops sof_byt_ops;
> -extern const struct snd_sof_dsp_ops sof_cht_ops;
> -extern const struct snd_sof_dsp_ops sof_bdw_ops;
>   
> -extern const struct sof_intel_dsp_desc byt_chip_info;
> -extern const struct sof_intel_dsp_desc cht_chip_info;
> -extern const struct sof_intel_dsp_desc bdw_chip_info;
>   extern const struct sof_intel_dsp_desc tng_chip_info;
>   
>   struct sof_intel_stream {
> diff --git a/sound/soc/sof/sof-acpi-dev.c b/sound/soc/sof/sof-acpi-dev.c
> index 2a369c2c6551..32654e042ec4 100644
> --- a/sound/soc/sof/sof-acpi-dev.c
> +++ b/sound/soc/sof/sof-acpi-dev.c
> @@ -36,70 +36,7 @@ MODULE_PARM_DESC(sof_acpi_debug, "SOF ACPI debug options (0x0 all off)");
>   
>   #define SOF_ACPI_DISABLE_PM_RUNTIME BIT(0)
>   
> -#if IS_ENABLED(CONFIG_ACPI) && IS_ENABLED(CONFIG_SND_SOC_SOF_BROADWELL)
> -static const struct sof_dev_desc sof_acpi_broadwell_desc = {
> -	.machines = snd_soc_acpi_intel_broadwell_machines,
> -	.resindex_lpe_base = 0,
> -	.resindex_pcicfg_base = 1,
> -	.resindex_imr_base = -1,
> -	.irqindex_host_ipc = 0,
> -	.chip_info = &bdw_chip_info,
> -	.default_fw_path = "intel/sof",
> -	.default_tplg_path = "intel/sof-tplg",
> -	.default_fw_filename = "sof-bdw.ri",
> -	.nocodec_tplg_filename = "sof-bdw-nocodec.tplg",
> -	.ops = &sof_bdw_ops,
> -};
> -#endif
> -
> -#if IS_ENABLED(CONFIG_ACPI) && IS_ENABLED(CONFIG_SND_SOC_SOF_BAYTRAIL)
> -
> -/* BYTCR uses different IRQ index */
> -static const struct sof_dev_desc sof_acpi_baytrailcr_desc = {
> -	.machines = snd_soc_acpi_intel_baytrail_machines,
> -	.resindex_lpe_base = 0,
> -	.resindex_pcicfg_base = 1,
> -	.resindex_imr_base = 2,
> -	.irqindex_host_ipc = 0,
> -	.chip_info = &byt_chip_info,
> -	.default_fw_path = "intel/sof",
> -	.default_tplg_path = "intel/sof-tplg",
> -	.default_fw_filename = "sof-byt.ri",
> -	.nocodec_tplg_filename = "sof-byt-nocodec.tplg",
> -	.ops = &sof_byt_ops,
> -};
> -
> -static const struct sof_dev_desc sof_acpi_baytrail_desc = {
> -	.machines = snd_soc_acpi_intel_baytrail_machines,
> -	.resindex_lpe_base = 0,
> -	.resindex_pcicfg_base = 1,
> -	.resindex_imr_base = 2,
> -	.irqindex_host_ipc = 5,
> -	.chip_info = &byt_chip_info,
> -	.default_fw_path = "intel/sof",
> -	.default_tplg_path = "intel/sof-tplg",
> -	.default_fw_filename = "sof-byt.ri",
> -	.nocodec_tplg_filename = "sof-byt-nocodec.tplg",
> -	.ops = &sof_byt_ops,
> -};
> -
> -static const struct sof_dev_desc sof_acpi_cherrytrail_desc = {
> -	.machines = snd_soc_acpi_intel_cherrytrail_machines,
> -	.resindex_lpe_base = 0,
> -	.resindex_pcicfg_base = 1,
> -	.resindex_imr_base = 2,
> -	.irqindex_host_ipc = 5,
> -	.chip_info = &cht_chip_info,
> -	.default_fw_path = "intel/sof",
> -	.default_tplg_path = "intel/sof-tplg",
> -	.default_fw_filename = "sof-cht.ri",
> -	.nocodec_tplg_filename = "sof-cht-nocodec.tplg",
> -	.ops = &sof_cht_ops,
> -};
> -
> -#endif
> -
> -static const struct dev_pm_ops sof_acpi_pm = {
> +const struct dev_pm_ops sof_acpi_pm = {
>   	SET_SYSTEM_SLEEP_PM_OPS(snd_sof_suspend, snd_sof_resume)
>   	SET_RUNTIME_PM_OPS(snd_sof_runtime_suspend, snd_sof_runtime_resume,
>   			   snd_sof_runtime_idle)
> @@ -118,40 +55,17 @@ static void sof_acpi_probe_complete(struct device *dev)
>   	pm_runtime_enable(dev);
>   }
>   
> -static int sof_acpi_probe(struct platform_device *pdev)
> +int sof_acpi_probe(struct platform_device *pdev, const struct sof_dev_desc *desc)
>   {
>   	struct device *dev = &pdev->dev;
> -	const struct acpi_device_id *id;
> -	const struct sof_dev_desc *desc;
>   	struct snd_sof_pdata *sof_pdata;
>   	const struct snd_sof_dsp_ops *ops;
>   	int ret;
>   
> -	id = acpi_match_device(dev->driver->acpi_match_table, dev);
> -	if (!id)
> -		return -ENODEV;
> -
> -	ret = snd_intel_acpi_dsp_driver_probe(dev, id->id);
> -	if (ret != SND_INTEL_DSP_DRIVER_ANY && ret != SND_INTEL_DSP_DRIVER_SOF) {
> -		dev_dbg(dev, "SOF ACPI driver not selected, aborting probe\n");
> -		return -ENODEV;
> -	}
> -
> -	dev_dbg(dev, "ACPI DSP detected");
> -
>   	sof_pdata = devm_kzalloc(dev, sizeof(*sof_pdata), GFP_KERNEL);
>   	if (!sof_pdata)
>   		return -ENOMEM;
>   
> -	desc = device_get_match_data(dev);
> -	if (!desc)
> -		return -ENODEV;
> -
> -#if IS_ENABLED(CONFIG_ACPI) && IS_ENABLED(CONFIG_SND_SOC_SOF_BAYTRAIL)
> -	if (desc == &sof_acpi_baytrail_desc && soc_intel_is_byt_cr(pdev))
> -		desc = &sof_acpi_baytrailcr_desc;
> -#endif
> -
>   	/* get ops for platform */
>   	ops = desc->ops;
>   	if (!ops) {
> @@ -193,8 +107,9 @@ static int sof_acpi_probe(struct platform_device *pdev)
>   
>   	return ret;
>   }
> +EXPORT_SYMBOL_GPL(sof_acpi_probe);
>   
> -static int sof_acpi_remove(struct platform_device *pdev)
> +int sof_acpi_remove(struct platform_device *pdev)
>   {
>   	if (!(sof_acpi_debug & SOF_ACPI_DISABLE_PM_RUNTIME))
>   		pm_runtime_disable(&pdev->dev);
> @@ -204,33 +119,6 @@ static int sof_acpi_remove(struct platform_device *pdev)
>   
>   	return 0;
>   }
> -
> -#ifdef CONFIG_ACPI
> -static const struct acpi_device_id sof_acpi_match[] = {
> -#if IS_ENABLED(CONFIG_SND_SOC_SOF_BROADWELL)
> -	{ "INT3438", (unsigned long)&sof_acpi_broadwell_desc },
> -#endif
> -#if IS_ENABLED(CONFIG_SND_SOC_SOF_BAYTRAIL)
> -	{ "80860F28", (unsigned long)&sof_acpi_baytrail_desc },
> -	{ "808622A8", (unsigned long)&sof_acpi_cherrytrail_desc },
> -#endif
> -	{ }
> -};
> -MODULE_DEVICE_TABLE(acpi, sof_acpi_match);
> -#endif
> -
> -/* acpi_driver definition */
> -static struct platform_driver snd_sof_acpi_driver = {
> -	.probe = sof_acpi_probe,
> -	.remove = sof_acpi_remove,
> -	.driver = {
> -		.name = "sof-audio-acpi",
> -		.pm = &sof_acpi_pm,
> -		.acpi_match_table = ACPI_PTR(sof_acpi_match),
> -	},
> -};
> -module_platform_driver(snd_sof_acpi_driver);
> +EXPORT_SYMBOL_GPL(sof_acpi_remove);
>   
>   MODULE_LICENSE("Dual BSD/GPL");
> -MODULE_IMPORT_NS(SND_SOC_SOF_BAYTRAIL);
> -MODULE_IMPORT_NS(SND_SOC_SOF_BROADWELL);
> 

Re: [PATCH] ASoC: SOF: Intel: avoid reverse module dependency

[Pierre-Louis Bossart]

On 1/5/21 1:07 PM, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> The SOF-ACPI driver is backwards from the normal Linux model, it has a
> generic driver that knows about all the specific drivers, as opposed to
> having hardware specific drivers that link against a common framework.
> 
> This requires ugly Kconfig magic and leads to missed dependencies as
> seen in this link error:
> 
> arm-linux-gnueabi-ld: sound/soc/sof/sof-pci-dev.o: in function `sof_acpi_probe':
> sof-pci-dev.c:(.text+0x1c): undefined reference to `snd_intel_dsp_driver_probe'
> 
> Change it to use the normal probe order of starting with a specific
> device in a driver, turning the sof-acpi-dev.c driver into a library.

Thanks Arnd for reporting all this, much appreciated.

The initial design was that we would have one generic platform_driver 
(ACPI) and one generic PCI driver that would deal with all known IDs, 
with descriptors that would point ops and callbacks defined in 
device-specific drivers. It's how all Intel drivers worked so far, from 
HDaudio to Atom/SST and Skylake.

It's not that ugly, but to Arnd's point we do have a lot of #if 
IS_ENABLED at the top level with a larger and larger table of IDs, along 
with Kconfig magic indeed to propagate constraints from top-level to 
device-specific drivers. The error with DSP_CONFIG comes from the fact 
that this never belonged at the top-level, or should have been 
conditionally invoked, as noted by Takashi.

That said, the initial design which dates from 2017 can be revisited now 
that we start having quite a few platforms and more coming. What Arnd 
suggests isn't without merits, it would indeed turn the generic code 
into generic helpers, and have all the platform IDs maintained in 
device-specific drivers. It's a more distributed/scalable solution, the 
only minor drawback I see is that it would require multiple instances of 
the 'platform_driver' and 'pci_driver' structures.

I would also want to keep the top-level selection so that ACPI/PCI/DT 
modules can be disabled in one shot, that would mean an additional 
change to the Makefiles since e.g.
obj-$(CONFIG_SND_SOC_SOF_ACPI) += snd-sof-acpi.o
would need to be set somehow.

Since this is going to be a really invasive change, and past experience 
shows that mucking with Kconfigs will invariably raise a number of 
broken corner cases, if there is support from Mark/Takashi/Jaroslav on 
this idea, we should first test it in the SOF tree so that we get a good 
test coverage and don't break too many eggs in Mark's tree. We would 
also need to concurrently change our CI scripts which are dependent on 
module names.

Also maybe in a first pass we can remove the compilation error with 
IS_REACHABLE and in a second pass do more invasive surgery?

> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>   sound/soc/sof/intel/Kconfig  |  34 ++--------
>   sound/soc/sof/intel/bdw.c    |  51 +++++++++++++--
>   sound/soc/sof/intel/byt.c    | 104 +++++++++++++++++++++++++----
>   sound/soc/sof/intel/shim.h   |  10 ++-
>   sound/soc/sof/sof-acpi-dev.c | 122 ++---------------------------------
>   5 files changed, 156 insertions(+), 165 deletions(-)
> 
> diff --git a/sound/soc/sof/intel/Kconfig b/sound/soc/sof/intel/Kconfig
> index ae9ba834814e..ff9266413a06 100644
> --- a/sound/soc/sof/intel/Kconfig
> +++ b/sound/soc/sof/intel/Kconfig
> @@ -9,14 +9,6 @@ config SND_SOC_SOF_INTEL_TOPLEVEL
>   
>   if SND_SOC_SOF_INTEL_TOPLEVEL
>   
> -config SND_SOC_SOF_INTEL_ACPI
> -	def_tristate SND_SOC_SOF_ACPI
> -	select SND_SOC_SOF_BAYTRAIL  if SND_SOC_SOF_BAYTRAIL_SUPPORT
> -	select SND_SOC_SOF_BROADWELL if SND_SOC_SOF_BROADWELL_SUPPORT
> -	help
> -	  This option is not user-selectable but automagically handled by
> -	  'select' statements at a higher level.
> -
>   config SND_SOC_SOF_INTEL_PCI
>   	def_tristate SND_SOC_SOF_PCI
>   	select SND_SOC_SOF_MERRIFIELD  if SND_SOC_SOF_MERRIFIELD_SUPPORT
> @@ -58,10 +50,12 @@ config SND_SOC_SOF_INTEL_COMMON
>   	  This option is not user-selectable but automagically handled by
>   	  'select' statements at a higher level.
>   
> -if SND_SOC_SOF_INTEL_ACPI
> +if SND_SOC_SOF_ACPI
>   
> -config SND_SOC_SOF_BAYTRAIL_SUPPORT
> +config SND_SOC_SOF_BAYTRAIL
>   	bool "SOF support for Baytrail, Braswell and Cherrytrail"
> +	select SND_SOC_SOF_INTEL_ATOM_HIFI_EP
> +	select SND_INTEL_DSP_CONFIG
>   	help
>   	  This adds support for Sound Open Firmware for Intel(R) platforms
>   	  using the Baytrail, Braswell or Cherrytrail processors.
> @@ -75,17 +69,11 @@ config SND_SOC_SOF_BAYTRAIL_SUPPORT
>   	  Say Y if you want to enable SOF on Baytrail/Cherrytrail.
>   	  If unsure select "N".
>   
> -config SND_SOC_SOF_BAYTRAIL
> -	tristate
> -	select SND_SOC_SOF_INTEL_ATOM_HIFI_EP
> -	select SND_INTEL_DSP_CONFIG
> -	help
> -	  This option is not user-selectable but automagically handled by
> -	  'select' statements at a higher level.
> -
> -config SND_SOC_SOF_BROADWELL_SUPPORT
> +config SND_SOC_SOF_BROADWELL
>   	bool "SOF support for Broadwell"
>   	select SND_INTEL_DSP_CONFIG
> +	select SND_SOC_SOF_INTEL_COMMON
> +	select SND_SOC_SOF_INTEL_HIFI_EP_IPC
>   	help
>   	  This adds support for Sound Open Firmware for Intel(R) platforms
>   	  using the Broadwell processors.
> @@ -100,14 +88,6 @@ config SND_SOC_SOF_BROADWELL_SUPPORT
>   	  Say Y if you want to enable SOF on Broadwell.
>   	  If unsure select "N".
>   
> -config SND_SOC_SOF_BROADWELL
> -	tristate
> -	select SND_SOC_SOF_INTEL_COMMON
> -	select SND_SOC_SOF_INTEL_HIFI_EP_IPC
> -	help
> -	  This option is not user-selectable but automagically handled by
> -	  'select' statements at a higher level.
> -
>   endif ## SND_SOC_SOF_INTEL_ACPI
>   
>   if SND_SOC_SOF_INTEL_PCI
> diff --git a/sound/soc/sof/intel/bdw.c b/sound/soc/sof/intel/bdw.c
> index 50a4a73e6b9f..542e885733a2 100644
> --- a/sound/soc/sof/intel/bdw.c
> +++ b/sound/soc/sof/intel/bdw.c
> @@ -15,6 +15,8 @@
>   #include <linux/module.h>
>   #include <sound/sof.h>
>   #include <sound/sof/xtensa.h>
> +#include <sound/soc-acpi.h>
> +#include <sound/soc-acpi-intel-match.h>
>   #include "../ops.h"
>   #include "shim.h"
>   #include "../sof-audio.h"
> @@ -590,7 +592,7 @@ static struct snd_soc_dai_driver bdw_dai[] = {
>   };
>   
>   /* broadwell ops */
> -const struct snd_sof_dsp_ops sof_bdw_ops = {
> +static const struct snd_sof_dsp_ops sof_bdw_ops = {
>   	/*Device init */
>   	.probe          = bdw_probe,
>   
> @@ -651,13 +653,54 @@ const struct snd_sof_dsp_ops sof_bdw_ops = {
>   
>   	.arch_ops = &sof_xtensa_arch_ops,
>   };
> -EXPORT_SYMBOL_NS(sof_bdw_ops, SND_SOC_SOF_BROADWELL);
>   
> -const struct sof_intel_dsp_desc bdw_chip_info = {
> +static const struct sof_intel_dsp_desc bdw_chip_info = {
>   	.cores_num = 1,
>   	.host_managed_cores_mask = 1,
>   };
> -EXPORT_SYMBOL_NS(bdw_chip_info, SND_SOC_SOF_BROADWELL);
> +
> +static const struct sof_dev_desc sof_acpi_broadwell_desc = {
> +	.machines = snd_soc_acpi_intel_broadwell_machines,
> +	.resindex_lpe_base = 0,
> +	.resindex_pcicfg_base = 1,
> +	.resindex_imr_base = -1,
> +	.irqindex_host_ipc = 0,
> +	.chip_info = &bdw_chip_info,
> +	.default_fw_path = "intel/sof",
> +	.default_tplg_path = "intel/sof-tplg",
> +	.default_fw_filename = "sof-bdw.ri",
> +	.nocodec_tplg_filename = "sof-bdw-nocodec.tplg",
> +	.ops = &sof_bdw_ops,
> +};
> +
> +static const struct acpi_device_id sof_broadwell_match[] = {
> +	{ "INT3438", (unsigned long)&sof_acpi_broadwell_desc },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(acpi, sof_broadwell_match);
> +
> +static int sof_broadwell_probe(struct platform_device *dev)
> +{
> +	int ret = snd_intel_acpi_dsp_driver_probe(dev, "INT3438");
> +	if (ret != SND_INTEL_DSP_DRIVER_SOF) {
> +		dev_dbg(dev, "SOF ACPI driver not selected, aborting probe\n");
> +		return -ENODEV;
> +	}
> +
> +	return sof_acpi_probe(dev, &sof_acpi_broadwell_desc);
> +}
> +
> +/* acpi_driver definition */
> +static struct platform_driver snd_sof_acpi_driver = {
> +	.probe = sof_broadwell_probe,
> +	.remove = sof_acpi_remove,
> +	.driver = {
> +		.name = "sof-audio-broadwell",
> +		.pm = &sof_acpi_pm,
> +		.acpi_match_table = sof_broadwell_match,
> +	},
> +};
> +module_platform_driver(snd_sof_acpi_driver);
>   
>   MODULE_LICENSE("Dual BSD/GPL");
>   MODULE_IMPORT_NS(SND_SOC_SOF_INTEL_HIFI_EP_IPC);
> diff --git a/sound/soc/sof/intel/byt.c b/sound/soc/sof/intel/byt.c
> index 19260dbecac5..9fa688c7711e 100644
> --- a/sound/soc/sof/intel/byt.c
> +++ b/sound/soc/sof/intel/byt.c
> @@ -15,6 +15,8 @@
>   #include <linux/module.h>
>   #include <sound/sof.h>
>   #include <sound/sof/xtensa.h>
> +#include <sound/soc-acpi.h>
> +#include <sound/soc-acpi-intel-match.h>
>   #include "../ops.h"
>   #include "shim.h"
>   #include "../sof-audio.h"
> @@ -657,8 +659,6 @@ EXPORT_SYMBOL_NS(tng_chip_info, SND_SOC_SOF_MERRIFIELD);
>   
>   #endif /* CONFIG_SND_SOC_SOF_MERRIFIELD */
>   
> -#if IS_ENABLED(CONFIG_SND_SOC_SOF_BAYTRAIL)
> -
>   static void byt_reset_dsp_disable_int(struct snd_sof_dev *sdev)
>   {
>   	/* Disable Interrupt from both sides */
> @@ -822,7 +822,7 @@ static int byt_acpi_probe(struct snd_sof_dev *sdev)
>   }
>   
>   /* baytrail ops */
> -const struct snd_sof_dsp_ops sof_byt_ops = {
> +static const struct snd_sof_dsp_ops sof_byt_ops = {
>   	/* device init */
>   	.probe		= byt_acpi_probe,
>   	.remove		= byt_remove,
> @@ -892,16 +892,14 @@ const struct snd_sof_dsp_ops sof_byt_ops = {
>   
>   	.arch_ops = &sof_xtensa_arch_ops,
>   };
> -EXPORT_SYMBOL_NS(sof_byt_ops, SND_SOC_SOF_BAYTRAIL);
>   
> -const struct sof_intel_dsp_desc byt_chip_info = {
> +static const struct sof_intel_dsp_desc byt_chip_info = {
>   	.cores_num = 1,
>   	.host_managed_cores_mask = 1,
>   };
> -EXPORT_SYMBOL_NS(byt_chip_info, SND_SOC_SOF_BAYTRAIL);
>   
>   /* cherrytrail and braswell ops */
> -const struct snd_sof_dsp_ops sof_cht_ops = {
> +static const struct snd_sof_dsp_ops sof_cht_ops = {
>   	/* device init */
>   	.probe		= byt_acpi_probe,
>   	.remove		= byt_remove,
> @@ -972,15 +970,99 @@ const struct snd_sof_dsp_ops sof_cht_ops = {
>   
>   	.arch_ops = &sof_xtensa_arch_ops,
>   };
> -EXPORT_SYMBOL_NS(sof_cht_ops, SND_SOC_SOF_BAYTRAIL);
>   
> -const struct sof_intel_dsp_desc cht_chip_info = {
> +static const struct sof_intel_dsp_desc cht_chip_info = {
>   	.cores_num = 1,
>   	.host_managed_cores_mask = 1,
>   };
> -EXPORT_SYMBOL_NS(cht_chip_info, SND_SOC_SOF_BAYTRAIL);
>   
> -#endif /* CONFIG_SND_SOC_SOF_BAYTRAIL */
> +/* BYTCR uses different IRQ index */
> +static const struct sof_dev_desc sof_acpi_baytrailcr_desc = {
> +	.machines = snd_soc_acpi_intel_baytrail_machines,
> +	.resindex_lpe_base = 0,
> +	.resindex_pcicfg_base = 1,
> +	.resindex_imr_base = 2,
> +	.irqindex_host_ipc = 0,
> +	.chip_info = &byt_chip_info,
> +	.default_fw_path = "intel/sof",
> +	.default_tplg_path = "intel/sof-tplg",
> +	.default_fw_filename = "sof-byt.ri",
> +	.nocodec_tplg_filename = "sof-byt-nocodec.tplg",
> +	.ops = &sof_byt_ops,
> +};
> +
> +static const struct sof_dev_desc sof_acpi_baytrail_desc = {
> +	.machines = snd_soc_acpi_intel_baytrail_machines,
> +	.resindex_lpe_base = 0,
> +	.resindex_pcicfg_base = 1,
> +	.resindex_imr_base = 2,
> +	.irqindex_host_ipc = 5,
> +	.chip_info = &byt_chip_info,
> +	.default_fw_path = "intel/sof",
> +	.default_tplg_path = "intel/sof-tplg",
> +	.default_fw_filename = "sof-byt.ri",
> +	.nocodec_tplg_filename = "sof-byt-nocodec.tplg",
> +	.ops = &sof_byt_ops,
> +};
> +
> +static const struct sof_dev_desc sof_acpi_cherrytrail_desc = {
> +	.machines = snd_soc_acpi_intel_cherrytrail_machines,
> +	.resindex_lpe_base = 0,
> +	.resindex_pcicfg_base = 1,
> +	.resindex_imr_base = 2,
> +	.irqindex_host_ipc = 5,
> +	.chip_info = &cht_chip_info,
> +	.default_fw_path = "intel/sof",
> +	.default_tplg_path = "intel/sof-tplg",
> +	.default_fw_filename = "sof-cht.ri",
> +	.nocodec_tplg_filename = "sof-cht-nocodec.tplg",
> +	.ops = &sof_cht_ops,
> +};
> +
> +static const struct acpi_device_id sof_baytrail_match[] = {
> +	{ "80860F28", (unsigned long)&sof_acpi_baytrail_desc },
> +	{ "808622A8", (unsigned long)&sof_acpi_cherrytrail_desc },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(acpi, sof_baytrail_match);
> +
> +static int sof_baytrail_probe(struct platform_device *pdev)
> +{
> +	const struct sof_dev_desc *desc = device_get_match_data(&pdev->dev);
> +	const struct acpi_device_id *id;
> +
> +	id = acpi_match_device(dev->driver->acpi_match_table, dev);
> +	if (!id)
> +		return -ENODEV;
> +
> +	ret = snd_intel_acpi_dsp_driver_probe(dev, id->id);
> +	if (ret != SND_INTEL_DSP_DRIVER_SOF) {
> +		dev_dbg(dev, "SOF ACPI driver not selected, aborting probe\n");
> +		return -ENODEV;
> +	}
> +
> +	dev_dbg(dev, "ACPI DSP detected");
> +
> +	if (!desc)
> +		return -ENODEV;
> +
> +	if (desc == &sof_acpi_baytrail_desc && soc_intel_is_byt_cr(pdev))
> +		desc = &sof_acpi_baytrailcr_desc;
> +
> +	return sof_acpi_probe(pdev, desc);
> +}
> +
> +/* acpi_driver definition */
> +static struct platform_driver snd_sof_baytrail_driver = {
> +	.probe = sof_baytrail_probe,
> +	.remove = sof_acpi_remove,
> +	.driver = {
> +		.name = "sof-audio-baytrail",
> +		.pm = &sof_acpi_pm,
> +		.acpi_match_table = sof_baytrail_match,
> +	},
> +};
> +module_platform_driver(snd_sof_baytrail_driver);
>   
>   MODULE_LICENSE("Dual BSD/GPL");
>   MODULE_IMPORT_NS(SND_SOC_SOF_INTEL_HIFI_EP_IPC);
> diff --git a/sound/soc/sof/intel/shim.h b/sound/soc/sof/intel/shim.h
> index 1e0afb5c8720..872116ee622b 100644
> --- a/sound/soc/sof/intel/shim.h
> +++ b/sound/soc/sof/intel/shim.h
> @@ -166,14 +166,12 @@ struct sof_intel_dsp_desc {
>   	int ssp_base_offset;		/* base address of the SSPs */
>   };
>   
> +extern const struct dev_pm_ops sof_acpi_pm;
> +extern int sof_acpi_probe(struct platform_device *pdev, const struct sof_dev_desc *desc);
> +extern int sof_acpi_remove(struct platform_device *pdev);
> +
>   extern const struct snd_sof_dsp_ops sof_tng_ops;
> -extern const struct snd_sof_dsp_ops sof_byt_ops;
> -extern const struct snd_sof_dsp_ops sof_cht_ops;
> -extern const struct snd_sof_dsp_ops sof_bdw_ops;
>   
> -extern const struct sof_intel_dsp_desc byt_chip_info;
> -extern const struct sof_intel_dsp_desc cht_chip_info;
> -extern const struct sof_intel_dsp_desc bdw_chip_info;
>   extern const struct sof_intel_dsp_desc tng_chip_info;
>   
>   struct sof_intel_stream {
> diff --git a/sound/soc/sof/sof-acpi-dev.c b/sound/soc/sof/sof-acpi-dev.c
> index 2a369c2c6551..32654e042ec4 100644
> --- a/sound/soc/sof/sof-acpi-dev.c
> +++ b/sound/soc/sof/sof-acpi-dev.c
> @@ -36,70 +36,7 @@ MODULE_PARM_DESC(sof_acpi_debug, "SOF ACPI debug options (0x0 all off)");
>   
>   #define SOF_ACPI_DISABLE_PM_RUNTIME BIT(0)
>   
> -#if IS_ENABLED(CONFIG_ACPI) && IS_ENABLED(CONFIG_SND_SOC_SOF_BROADWELL)
> -static const struct sof_dev_desc sof_acpi_broadwell_desc = {
> -	.machines = snd_soc_acpi_intel_broadwell_machines,
> -	.resindex_lpe_base = 0,
> -	.resindex_pcicfg_base = 1,
> -	.resindex_imr_base = -1,
> -	.irqindex_host_ipc = 0,
> -	.chip_info = &bdw_chip_info,
> -	.default_fw_path = "intel/sof",
> -	.default_tplg_path = "intel/sof-tplg",
> -	.default_fw_filename = "sof-bdw.ri",
> -	.nocodec_tplg_filename = "sof-bdw-nocodec.tplg",
> -	.ops = &sof_bdw_ops,
> -};
> -#endif
> -
> -#if IS_ENABLED(CONFIG_ACPI) && IS_ENABLED(CONFIG_SND_SOC_SOF_BAYTRAIL)
> -
> -/* BYTCR uses different IRQ index */
> -static const struct sof_dev_desc sof_acpi_baytrailcr_desc = {
> -	.machines = snd_soc_acpi_intel_baytrail_machines,
> -	.resindex_lpe_base = 0,
> -	.resindex_pcicfg_base = 1,
> -	.resindex_imr_base = 2,
> -	.irqindex_host_ipc = 0,
> -	.chip_info = &byt_chip_info,
> -	.default_fw_path = "intel/sof",
> -	.default_tplg_path = "intel/sof-tplg",
> -	.default_fw_filename = "sof-byt.ri",
> -	.nocodec_tplg_filename = "sof-byt-nocodec.tplg",
> -	.ops = &sof_byt_ops,
> -};
> -
> -static const struct sof_dev_desc sof_acpi_baytrail_desc = {
> -	.machines = snd_soc_acpi_intel_baytrail_machines,
> -	.resindex_lpe_base = 0,
> -	.resindex_pcicfg_base = 1,
> -	.resindex_imr_base = 2,
> -	.irqindex_host_ipc = 5,
> -	.chip_info = &byt_chip_info,
> -	.default_fw_path = "intel/sof",
> -	.default_tplg_path = "intel/sof-tplg",
> -	.default_fw_filename = "sof-byt.ri",
> -	.nocodec_tplg_filename = "sof-byt-nocodec.tplg",
> -	.ops = &sof_byt_ops,
> -};
> -
> -static const struct sof_dev_desc sof_acpi_cherrytrail_desc = {
> -	.machines = snd_soc_acpi_intel_cherrytrail_machines,
> -	.resindex_lpe_base = 0,
> -	.resindex_pcicfg_base = 1,
> -	.resindex_imr_base = 2,
> -	.irqindex_host_ipc = 5,
> -	.chip_info = &cht_chip_info,
> -	.default_fw_path = "intel/sof",
> -	.default_tplg_path = "intel/sof-tplg",
> -	.default_fw_filename = "sof-cht.ri",
> -	.nocodec_tplg_filename = "sof-cht-nocodec.tplg",
> -	.ops = &sof_cht_ops,
> -};
> -
> -#endif
> -
> -static const struct dev_pm_ops sof_acpi_pm = {
> +const struct dev_pm_ops sof_acpi_pm = {
>   	SET_SYSTEM_SLEEP_PM_OPS(snd_sof_suspend, snd_sof_resume)
>   	SET_RUNTIME_PM_OPS(snd_sof_runtime_suspend, snd_sof_runtime_resume,
>   			   snd_sof_runtime_idle)
> @@ -118,40 +55,17 @@ static void sof_acpi_probe_complete(struct device *dev)
>   	pm_runtime_enable(dev);
>   }
>   
> -static int sof_acpi_probe(struct platform_device *pdev)
> +int sof_acpi_probe(struct platform_device *pdev, const struct sof_dev_desc *desc)
>   {
>   	struct device *dev = &pdev->dev;
> -	const struct acpi_device_id *id;
> -	const struct sof_dev_desc *desc;
>   	struct snd_sof_pdata *sof_pdata;
>   	const struct snd_sof_dsp_ops *ops;
>   	int ret;
>   
> -	id = acpi_match_device(dev->driver->acpi_match_table, dev);
> -	if (!id)
> -		return -ENODEV;
> -
> -	ret = snd_intel_acpi_dsp_driver_probe(dev, id->id);
> -	if (ret != SND_INTEL_DSP_DRIVER_ANY && ret != SND_INTEL_DSP_DRIVER_SOF) {
> -		dev_dbg(dev, "SOF ACPI driver not selected, aborting probe\n");
> -		return -ENODEV;
> -	}
> -
> -	dev_dbg(dev, "ACPI DSP detected");
> -
>   	sof_pdata = devm_kzalloc(dev, sizeof(*sof_pdata), GFP_KERNEL);
>   	if (!sof_pdata)
>   		return -ENOMEM;
>   
> -	desc = device_get_match_data(dev);
> -	if (!desc)
> -		return -ENODEV;
> -
> -#if IS_ENABLED(CONFIG_ACPI) && IS_ENABLED(CONFIG_SND_SOC_SOF_BAYTRAIL)
> -	if (desc == &sof_acpi_baytrail_desc && soc_intel_is_byt_cr(pdev))
> -		desc = &sof_acpi_baytrailcr_desc;
> -#endif
> -
>   	/* get ops for platform */
>   	ops = desc->ops;
>   	if (!ops) {
> @@ -193,8 +107,9 @@ static int sof_acpi_probe(struct platform_device *pdev)
>   
>   	return ret;
>   }
> +EXPORT_SYMBOL_GPL(sof_acpi_probe);
>   
> -static int sof_acpi_remove(struct platform_device *pdev)
> +int sof_acpi_remove(struct platform_device *pdev)
>   {
>   	if (!(sof_acpi_debug & SOF_ACPI_DISABLE_PM_RUNTIME))
>   		pm_runtime_disable(&pdev->dev);
> @@ -204,33 +119,6 @@ static int sof_acpi_remove(struct platform_device *pdev)
>   
>   	return 0;
>   }
> -
> -#ifdef CONFIG_ACPI
> -static const struct acpi_device_id sof_acpi_match[] = {
> -#if IS_ENABLED(CONFIG_SND_SOC_SOF_BROADWELL)
> -	{ "INT3438", (unsigned long)&sof_acpi_broadwell_desc },
> -#endif
> -#if IS_ENABLED(CONFIG_SND_SOC_SOF_BAYTRAIL)
> -	{ "80860F28", (unsigned long)&sof_acpi_baytrail_desc },
> -	{ "808622A8", (unsigned long)&sof_acpi_cherrytrail_desc },
> -#endif
> -	{ }
> -};
> -MODULE_DEVICE_TABLE(acpi, sof_acpi_match);
> -#endif
> -
> -/* acpi_driver definition */
> -static struct platform_driver snd_sof_acpi_driver = {
> -	.probe = sof_acpi_probe,
> -	.remove = sof_acpi_remove,
> -	.driver = {
> -		.name = "sof-audio-acpi",
> -		.pm = &sof_acpi_pm,
> -		.acpi_match_table = ACPI_PTR(sof_acpi_match),
> -	},
> -};
> -module_platform_driver(snd_sof_acpi_driver);
> +EXPORT_SYMBOL_GPL(sof_acpi_remove);
>   
>   MODULE_LICENSE("Dual BSD/GPL");
> -MODULE_IMPORT_NS(SND_SOC_SOF_BAYTRAIL);
> -MODULE_IMPORT_NS(SND_SOC_SOF_BROADWELL);
> 

Re: [PATCH] ASoC: SOF: Intel: avoid reverse module dependency

[Takashi Iwai]

On Mon, 11 Jan 2021 20:54:17 +0100,
Pierre-Louis Bossart wrote:
> 
> 
> 
> On 1/5/21 1:07 PM, Arnd Bergmann wrote:
> > From: Arnd Bergmann <arnd@arndb.de>
> >
> > The SOF-ACPI driver is backwards from the normal Linux model, it has a
> > generic driver that knows about all the specific drivers, as opposed to
> > having hardware specific drivers that link against a common framework.
> >
> > This requires ugly Kconfig magic and leads to missed dependencies as
> > seen in this link error:
> >
> > arm-linux-gnueabi-ld: sound/soc/sof/sof-pci-dev.o: in function `sof_acpi_probe':
> > sof-pci-dev.c:(.text+0x1c): undefined reference to `snd_intel_dsp_driver_probe'
> >
> > Change it to use the normal probe order of starting with a specific
> > device in a driver, turning the sof-acpi-dev.c driver into a library.
> 
> Thanks Arnd for reporting all this, much appreciated.
> 
> The initial design was that we would have one generic platform_driver
> (ACPI) and one generic PCI driver that would deal with all known IDs,
> with descriptors that would point ops and callbacks defined in
> device-specific drivers. It's how all Intel drivers worked so far,
> from HDaudio to Atom/SST and Skylake.
> 
> It's not that ugly, but to Arnd's point we do have a lot of #if
> IS_ENABLED at the top level with a larger and larger table of IDs,
> along with Kconfig magic indeed to propagate constraints from
> top-level to device-specific drivers. The error with DSP_CONFIG comes
> from the fact that this never belonged at the top-level, or should
> have been conditionally invoked, as noted by Takashi.
> 
> That said, the initial design which dates from 2017 can be revisited
> now that we start having quite a few platforms and more coming. What
> Arnd suggests isn't without merits, it would indeed turn the generic
> code into generic helpers, and have all the platform IDs maintained in
> device-specific drivers. It's a more distributed/scalable solution,
> the only minor drawback I see is that it would require multiple
> instances of the 'platform_driver' and 'pci_driver' structures.
> 
> I would also want to keep the top-level selection so that ACPI/PCI/DT
> modules can be disabled in one shot, that would mean an additional
> change to the Makefiles since e.g.
> obj-$(CONFIG_SND_SOC_SOF_ACPI) += snd-sof-acpi.o
> would need to be set somehow.
> 
> Since this is going to be a really invasive change, and past
> experience shows that mucking with Kconfigs will invariably raise a
> number of broken corner cases, if there is support from
> Mark/Takashi/Jaroslav on this idea, we should first test it in the SOF
> tree so that we get a good test coverage and don't break too many eggs
> in Mark's tree. We would also need to concurrently change our CI
> scripts which are dependent on module names.

I'm in favor of the way Arnd proposed.  It's more straightforward and
less code.

If you find the number of modules or the too much cutting out being
problematic, you can create a module snd-sof-intel-acpi and
snd-sof-intel-pci containing the driver table entries for all Intel
devices, too.  In the case, you'll still need some conditional calls
of intel-dsp-config there, but it's a good step for reducing the
Kconfig complexity.

> Also maybe in a first pass we can remove the compilation error with
> IS_REACHABLE and in a second pass do more invasive surgery?

Agreed, we'd like to keep less changes for 5.11 for now.


thanks,

Takashi

Re: [PATCH] ASoC: SOF: Intel: avoid reverse module dependency

[Takashi Iwai]

On Mon, 11 Jan 2021 20:54:17 +0100,
Pierre-Louis Bossart wrote:
> 
> 
> 
> On 1/5/21 1:07 PM, Arnd Bergmann wrote:
> > From: Arnd Bergmann <arnd@arndb.de>
> >
> > The SOF-ACPI driver is backwards from the normal Linux model, it has a
> > generic driver that knows about all the specific drivers, as opposed to
> > having hardware specific drivers that link against a common framework.
> >
> > This requires ugly Kconfig magic and leads to missed dependencies as
> > seen in this link error:
> >
> > arm-linux-gnueabi-ld: sound/soc/sof/sof-pci-dev.o: in function `sof_acpi_probe':
> > sof-pci-dev.c:(.text+0x1c): undefined reference to `snd_intel_dsp_driver_probe'
> >
> > Change it to use the normal probe order of starting with a specific
> > device in a driver, turning the sof-acpi-dev.c driver into a library.
> 
> Thanks Arnd for reporting all this, much appreciated.
> 
> The initial design was that we would have one generic platform_driver
> (ACPI) and one generic PCI driver that would deal with all known IDs,
> with descriptors that would point ops and callbacks defined in
> device-specific drivers. It's how all Intel drivers worked so far,
> from HDaudio to Atom/SST and Skylake.
> 
> It's not that ugly, but to Arnd's point we do have a lot of #if
> IS_ENABLED at the top level with a larger and larger table of IDs,
> along with Kconfig magic indeed to propagate constraints from
> top-level to device-specific drivers. The error with DSP_CONFIG comes
> from the fact that this never belonged at the top-level, or should
> have been conditionally invoked, as noted by Takashi.
> 
> That said, the initial design which dates from 2017 can be revisited
> now that we start having quite a few platforms and more coming. What
> Arnd suggests isn't without merits, it would indeed turn the generic
> code into generic helpers, and have all the platform IDs maintained in
> device-specific drivers. It's a more distributed/scalable solution,
> the only minor drawback I see is that it would require multiple
> instances of the 'platform_driver' and 'pci_driver' structures.
> 
> I would also want to keep the top-level selection so that ACPI/PCI/DT
> modules can be disabled in one shot, that would mean an additional
> change to the Makefiles since e.g.
> obj-$(CONFIG_SND_SOC_SOF_ACPI) += snd-sof-acpi.o
> would need to be set somehow.
> 
> Since this is going to be a really invasive change, and past
> experience shows that mucking with Kconfigs will invariably raise a
> number of broken corner cases, if there is support from
> Mark/Takashi/Jaroslav on this idea, we should first test it in the SOF
> tree so that we get a good test coverage and don't break too many eggs
> in Mark's tree. We would also need to concurrently change our CI
> scripts which are dependent on module names.

I'm in favor of the way Arnd proposed.  It's more straightforward and
less code.

If you find the number of modules or the too much cutting out being
problematic, you can create a module snd-sof-intel-acpi and
snd-sof-intel-pci containing the driver table entries for all Intel
devices, too.  In the case, you'll still need some conditional calls
of intel-dsp-config there, but it's a good step for reducing the
Kconfig complexity.

> Also maybe in a first pass we can remove the compilation error with
> IS_REACHABLE and in a second pass do more invasive surgery?

Agreed, we'd like to keep less changes for 5.11 for now.


thanks,

Takashi

Re: [Sound-open-firmware] [PATCH] ASoC: SOF: Intel: avoid reverse module dependency

[Pierre-Louis Bossart]

>> Since this is going to be a really invasive change, and past
>> experience shows that mucking with Kconfigs will invariably raise a
>> number of broken corner cases, if there is support from
>> Mark/Takashi/Jaroslav on this idea, we should first test it in the SOF
>> tree so that we get a good test coverage and don't break too many eggs
>> in Mark's tree. We would also need to concurrently change our CI
>> scripts which are dependent on module names.
> 
> I'm in favor of the way Arnd proposed.  It's more straightforward and
> less code.

Thanks Takashi for the feedback.

Since yesterday I looked at another problem where we can have unmet 
dependencies between SoundWire (m) and SOF (y), so we probably need to 
rethink all this. We had similar issue with SOF and HDaudio before, it's 
time to revisit all this.

> 
> If you find the number of modules or the too much cutting out being
> problematic, you can create a module snd-sof-intel-acpi and
> snd-sof-intel-pci containing the driver table entries for all Intel
> devices, too.  In the case, you'll still need some conditional calls
> of intel-dsp-config there, but it's a good step for reducing the
> Kconfig complexity.
> 
>> Also maybe in a first pass we can remove the compilation error with
>> IS_REACHABLE and in a second pass do more invasive surgery?
> 
> Agreed, we'd like to keep less changes for 5.11 for now.

Ack, we'll send smaller changes first.

Re: [Sound-open-firmware] [PATCH] ASoC: SOF: Intel: avoid reverse module dependency

[Pierre-Louis Bossart]

>> Since this is going to be a really invasive change, and past
>> experience shows that mucking with Kconfigs will invariably raise a
>> number of broken corner cases, if there is support from
>> Mark/Takashi/Jaroslav on this idea, we should first test it in the SOF
>> tree so that we get a good test coverage and don't break too many eggs
>> in Mark's tree. We would also need to concurrently change our CI
>> scripts which are dependent on module names.
> 
> I'm in favor of the way Arnd proposed.  It's more straightforward and
> less code.

Thanks Takashi for the feedback.

Since yesterday I looked at another problem where we can have unmet 
dependencies between SoundWire (m) and SOF (y), so we probably need to 
rethink all this. We had similar issue with SOF and HDaudio before, it's 
time to revisit all this.

> 
> If you find the number of modules or the too much cutting out being
> problematic, you can create a module snd-sof-intel-acpi and
> snd-sof-intel-pci containing the driver table entries for all Intel
> devices, too.  In the case, you'll still need some conditional calls
> of intel-dsp-config there, but it's a good step for reducing the
> Kconfig complexity.
> 
>> Also maybe in a first pass we can remove the compilation error with
>> IS_REACHABLE and in a second pass do more invasive surgery?
> 
> Agreed, we'd like to keep less changes for 5.11 for now.

Ack, we'll send smaller changes first.

Re: [Sound-open-firmware] [PATCH] ASoC: SOF: Intel: avoid reverse module dependency

[Arnd Bergmann]

On Tue, Jan 12, 2021 at 9:17 PM Pierre-Louis Bossart
<pierre-louis.bossart@linux.intel.com> wrote:
>
>
> >> Since this is going to be a really invasive change, and past
> >> experience shows that mucking with Kconfigs will invariably raise a
> >> number of broken corner cases, if there is support from
> >> Mark/Takashi/Jaroslav on this idea, we should first test it in the SOF
> >> tree so that we get a good test coverage and don't break too many eggs
> >> in Mark's tree. We would also need to concurrently change our CI
> >> scripts which are dependent on module names.
> >
> > I'm in favor of the way Arnd proposed.  It's more straightforward and
> > less code.
>
> Thanks Takashi for the feedback.
>
> Since yesterday I looked at another problem where we can have unmet
> dependencies between SoundWire (m) and SOF (y), so we probably need to
> rethink all this. We had similar issue with SOF and HDaudio before, it's
> time to revisit all this.

I think I ran into the same thing yesterday, and came up with a patch for
that one as well. I think it should be independent of the other one but I did
not try it by itself.

I'll send it along with a fixed version of the one in this thread, together the
have now survived a few hundred randconfig builds.

       Arnd

Re: [Sound-open-firmware] [PATCH] ASoC: SOF: Intel: avoid reverse module dependency

[Arnd Bergmann]

On Tue, Jan 12, 2021 at 9:17 PM Pierre-Louis Bossart
<pierre-louis.bossart@linux.intel.com> wrote:
>
>
> >> Since this is going to be a really invasive change, and past
> >> experience shows that mucking with Kconfigs will invariably raise a
> >> number of broken corner cases, if there is support from
> >> Mark/Takashi/Jaroslav on this idea, we should first test it in the SOF
> >> tree so that we get a good test coverage and don't break too many eggs
> >> in Mark's tree. We would also need to concurrently change our CI
> >> scripts which are dependent on module names.
> >
> > I'm in favor of the way Arnd proposed.  It's more straightforward and
> > less code.
>
> Thanks Takashi for the feedback.
>
> Since yesterday I looked at another problem where we can have unmet
> dependencies between SoundWire (m) and SOF (y), so we probably need to
> rethink all this. We had similar issue with SOF and HDaudio before, it's
> time to revisit all this.

I think I ran into the same thing yesterday, and came up with a patch for
that one as well. I think it should be independent of the other one but I did
not try it by itself.

I'll send it along with a fixed version of the one in this thread, together the
have now survived a few hundred randconfig builds.

       Arnd

Re: [PATCH] ALSA: hda: fix SND_INTEL_DSP_CONFIG dependency

[Arnd Bergmann]

On Mon, Jan 4, 2021 at 4:00 PM Jaroslav Kysela <perex@perex.cz> wrote:
> Dne 03. 01. 21 v 14:52 Arnd Bergmann napsal(a):

> > diff --git a/include/sound/intel-dsp-config.h b/include/sound/intel-dsp-config.h
> > index d4609077c258..94667e870029 100644
> > --- a/include/sound/intel-dsp-config.h
> > +++ b/include/sound/intel-dsp-config.h
> > @@ -18,24 +18,7 @@ enum {
> >       SND_INTEL_DSP_DRIVER_LAST = SND_INTEL_DSP_DRIVER_SOF
> >  };
> >
> > -#if IS_ENABLED(CONFIG_SND_INTEL_DSP_CONFIG)
>
> The SOF drivers selects the DSP config code only when required (for specific
> platforms - see sound/soc/sof/intel/Kconfig).
>
> It seems that the above if should be modified as:
>
> #if IS_BUILDIN(CONFIG_SND_INTEL_DSP_CONFIG) || (defined(MODULE) &&
> IS_MODULE(CONFIG_SND_INTEL_DSP_CONFIG))
>
> So the buildin drivers which do not require the DSP config probe can be
> compiled without this dependency.

This would be the same as

#if IS_REACHABLE(CONFIG_SND_INTEL_DSP_CONFIG)

but using that macro is almost always a bad idea, as it tends to hide
dependency problems and causes things to silently not work right
when the Kconfig rules are incorrect.

      Arnd

Re: [PATCH] ALSA: hda: fix SND_INTEL_DSP_CONFIG dependency

[Arnd Bergmann]

On Mon, Jan 4, 2021 at 4:00 PM Jaroslav Kysela <perex@perex.cz> wrote:
> Dne 03. 01. 21 v 14:52 Arnd Bergmann napsal(a):

> > diff --git a/include/sound/intel-dsp-config.h b/include/sound/intel-dsp-config.h
> > index d4609077c258..94667e870029 100644
> > --- a/include/sound/intel-dsp-config.h
> > +++ b/include/sound/intel-dsp-config.h
> > @@ -18,24 +18,7 @@ enum {
> >       SND_INTEL_DSP_DRIVER_LAST = SND_INTEL_DSP_DRIVER_SOF
> >  };
> >
> > -#if IS_ENABLED(CONFIG_SND_INTEL_DSP_CONFIG)
>
> The SOF drivers selects the DSP config code only when required (for specific
> platforms - see sound/soc/sof/intel/Kconfig).
>
> It seems that the above if should be modified as:
>
> #if IS_BUILDIN(CONFIG_SND_INTEL_DSP_CONFIG) || (defined(MODULE) &&
> IS_MODULE(CONFIG_SND_INTEL_DSP_CONFIG))
>
> So the buildin drivers which do not require the DSP config probe can be
> compiled without this dependency.

This would be the same as

#if IS_REACHABLE(CONFIG_SND_INTEL_DSP_CONFIG)

but using that macro is almost always a bad idea, as it tends to hide
dependency problems and causes things to silently not work right
when the Kconfig rules are incorrect.

      Arnd