ASoC: SOF: Intel: avoid reverse module dependency

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

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

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

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

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

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

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