[alsa-devel] [PATCH] ASoC: intel: Fix sst-dsp dependency on dw stuff
Yang Jie
yang.jie at linux.intel.com
Tue Jul 12 04:02:25 CEST 2016
On 2016年07月11日 16:39, Takashi Iwai wrote:
> The recent commit [a92ea59b74e2: ASoC: Intel: sst: only select
> sst-firmware when DW DMAC is built-in] introduced more strict kconfig
> dependency (depends on DW_DMAC_CORE=y) for avoiding the build failures
> due to dependency messes in intel-sst. This makes, however, it
> impossible to use this driver with the modularized systems,
> i.e. typically on Linux distros.
>
> The problem addressed in the commit above is that sst_dsp_new() and
> sst_dsp_free() includes the firmware init / finish that call dw_*()
> functions. Thus building it as built-in with DW_DMAC_CORE module
> results in the missing symbols.
>
> However, these sst_dsp functions are basically called only from the
> drivers that depend on DW_DMAC_CORE already. That is, once when these
> functions are split out, the rest can be independent from dw stuff.
>
> This patch attempts to solve the issue by the following:
> - Split sst-dsp stuff into two modules: snd-soc-sst-dsp and
> snd-soc-sst-firmware.
> - Move sst_dsp_new() and sst_dsp_free() to the latter module so that
> the former module can be independent from DW_DMAC_CORE.
> - Add a new kconfig SND_SOC_INTEL_SST_FIRMWARE to select the latter
> module by machine drivers.
>
> One only remaining pitfall is that each machine driver has to select
> SND_SOC_INTEL_SST_FIRMWARE carefully depending on DW_DMAC_CORE.
> This can't be done cleanly due to the restriction of the current
> kbuild.
is it good to let SND_SOC_INTEL_SST_FIRMWARE select DW_DMAC_CORE in your
opinion? That may fix this pitfall, but I am not sure if it is comply
with convention -- a module should not select another upper layer one?
>
> Bugzilla: https://bugzilla.opensuse.org/show_bug.cgi?id=988117
> Fixes: a92ea59b74e2 ('ASoC: Intel: sst: only select sst-firmware when DW DMAC is built-in')
> Signed-off-by: Takashi Iwai <tiwai at suse.de>
Acked-by: Jie Yang <yang.jie at intel.com>
thanks,
~Keyon
> ---
> sound/soc/intel/Kconfig | 18 +++++++---
> sound/soc/intel/common/Makefile | 4 +--
> sound/soc/intel/common/sst-dsp-priv.h | 4 ---
> sound/soc/intel/common/sst-dsp.c | 67 ----------------------------------
> sound/soc/intel/common/sst-dsp.h | 2 +-
> sound/soc/intel/common/sst-firmware.c | 68 +++++++++++++++++++++++++++++++++++
> 6 files changed, 85 insertions(+), 78 deletions(-)
>
> diff --git a/sound/soc/intel/Kconfig b/sound/soc/intel/Kconfig
> index 9c86459d0fc3..a20c3dfbcb5d 100644
> --- a/sound/soc/intel/Kconfig
> +++ b/sound/soc/intel/Kconfig
> @@ -32,6 +32,12 @@ config SND_SOC_INTEL_SST
> select SND_SOC_INTEL_SST_MATCH if ACPI
> depends on (X86 || COMPILE_TEST)
>
> +# firmware stuff depends DW_DMAC_CORE; since there is no depends-on from
> +# the reverse selection, each machine driver needs to select
> +# SND_SOC_INTEL_SST_FIRMWARE carefully depending on DW_DMAC_CORE
> +config SND_SOC_INTEL_SST_FIRMWARE
> + tristate
> +
> config SND_SOC_INTEL_SST_ACPI
> tristate
>
> @@ -47,8 +53,9 @@ config SND_SOC_INTEL_BAYTRAIL
> config SND_SOC_INTEL_HASWELL_MACH
> tristate "ASoC Audio DSP support for Intel Haswell Lynxpoint"
> depends on X86_INTEL_LPSS && I2C && I2C_DESIGNWARE_PLATFORM
> - depends on DW_DMAC_CORE=y
> + depends on DW_DMAC_CORE
> select SND_SOC_INTEL_SST
> + select SND_SOC_INTEL_SST_FIRMWARE
> select SND_SOC_INTEL_HASWELL
> select SND_SOC_RT5640
> help
> @@ -91,8 +98,9 @@ config SND_SOC_INTEL_BXT_RT298_MACH
> config SND_SOC_INTEL_BYT_RT5640_MACH
> tristate "ASoC Audio driver for Intel Baytrail with RT5640 codec"
> depends on X86_INTEL_LPSS && I2C
> - depends on DW_DMAC_CORE=y && (SND_SST_IPC_ACPI = n)
> + depends on DW_DMAC_CORE && (SND_SST_IPC_ACPI = n)
> select SND_SOC_INTEL_SST
> + select SND_SOC_INTEL_SST_FIRMWARE
> select SND_SOC_INTEL_BAYTRAIL
> select SND_SOC_RT5640
> help
> @@ -103,8 +111,9 @@ config SND_SOC_INTEL_BYT_RT5640_MACH
> config SND_SOC_INTEL_BYT_MAX98090_MACH
> tristate "ASoC Audio driver for Intel Baytrail with MAX98090 codec"
> depends on X86_INTEL_LPSS && I2C
> - depends on DW_DMAC_CORE=y && (SND_SST_IPC_ACPI = n)
> + depends on DW_DMAC_CORE && (SND_SST_IPC_ACPI = n)
> select SND_SOC_INTEL_SST
> + select SND_SOC_INTEL_SST_FIRMWARE
> select SND_SOC_INTEL_BAYTRAIL
> select SND_SOC_MAX98090
> help
> @@ -115,8 +124,9 @@ config SND_SOC_INTEL_BROADWELL_MACH
> tristate "ASoC Audio DSP support for Intel Broadwell Wildcatpoint"
> depends on X86_INTEL_LPSS && I2C && DW_DMAC && \
> I2C_DESIGNWARE_PLATFORM
> - depends on DW_DMAC_CORE=y
> + depends on DW_DMAC_CORE
> select SND_SOC_INTEL_SST
> + select SND_SOC_INTEL_SST_FIRMWARE
> select SND_SOC_INTEL_HASWELL
> select SND_SOC_RT286
> help
> diff --git a/sound/soc/intel/common/Makefile b/sound/soc/intel/common/Makefile
> index fbbb25c2ceed..1a35149bcad7 100644
> --- a/sound/soc/intel/common/Makefile
> +++ b/sound/soc/intel/common/Makefile
> @@ -2,9 +2,9 @@ snd-soc-sst-dsp-objs := sst-dsp.o
> snd-soc-sst-acpi-objs := sst-acpi.o
> snd-soc-sst-match-objs := sst-match-acpi.o
> snd-soc-sst-ipc-objs := sst-ipc.o
> -
> -snd-soc-sst-dsp-$(CONFIG_DW_DMAC_CORE) += sst-firmware.o
> +snd-soc-sst-firmware-objs := sst-firmware.o
>
> obj-$(CONFIG_SND_SOC_INTEL_SST) += snd-soc-sst-dsp.o snd-soc-sst-ipc.o
> obj-$(CONFIG_SND_SOC_INTEL_SST_ACPI) += snd-soc-sst-acpi.o
> obj-$(CONFIG_SND_SOC_INTEL_SST_MATCH) += snd-soc-sst-match.o
> +obj-$(CONFIG_SND_SOC_INTEL_SST_FIRMWARE) += snd-soc-sst-firmware.o
> diff --git a/sound/soc/intel/common/sst-dsp-priv.h b/sound/soc/intel/common/sst-dsp-priv.h
> index 97dc1ae05e69..d13c84364c3c 100644
> --- a/sound/soc/intel/common/sst-dsp-priv.h
> +++ b/sound/soc/intel/common/sst-dsp-priv.h
> @@ -383,10 +383,6 @@ struct sst_mem_block *sst_mem_block_register(struct sst_dsp *dsp, u32 offset,
> u32 index, void *private);
> void sst_mem_block_unregister_all(struct sst_dsp *dsp);
>
> -/* Create/Free DMA resources */
> -int sst_dma_new(struct sst_dsp *sst);
> -void sst_dma_free(struct sst_dma *dma);
> -
> u32 sst_dsp_get_offset(struct sst_dsp *dsp, u32 offset,
> enum sst_mem_type type);
> #endif
> diff --git a/sound/soc/intel/common/sst-dsp.c b/sound/soc/intel/common/sst-dsp.c
> index ff2196ef359f..c00ede4ea4d7 100644
> --- a/sound/soc/intel/common/sst-dsp.c
> +++ b/sound/soc/intel/common/sst-dsp.c
> @@ -420,73 +420,6 @@ void sst_dsp_inbox_read(struct sst_dsp *sst, void *message, size_t bytes)
> }
> EXPORT_SYMBOL_GPL(sst_dsp_inbox_read);
>
> -#ifdef CONFIG_DW_DMAC_CORE
> -struct sst_dsp *sst_dsp_new(struct device *dev,
> - struct sst_dsp_device *sst_dev, struct sst_pdata *pdata)
> -{
> - struct sst_dsp *sst;
> - int err;
> -
> - dev_dbg(dev, "initialising audio DSP id 0x%x\n", pdata->id);
> -
> - sst = devm_kzalloc(dev, sizeof(*sst), GFP_KERNEL);
> - if (sst == NULL)
> - return NULL;
> -
> - spin_lock_init(&sst->spinlock);
> - mutex_init(&sst->mutex);
> - sst->dev = dev;
> - sst->dma_dev = pdata->dma_dev;
> - sst->thread_context = sst_dev->thread_context;
> - sst->sst_dev = sst_dev;
> - sst->id = pdata->id;
> - sst->irq = pdata->irq;
> - sst->ops = sst_dev->ops;
> - sst->pdata = pdata;
> - INIT_LIST_HEAD(&sst->used_block_list);
> - INIT_LIST_HEAD(&sst->free_block_list);
> - INIT_LIST_HEAD(&sst->module_list);
> - INIT_LIST_HEAD(&sst->fw_list);
> - INIT_LIST_HEAD(&sst->scratch_block_list);
> -
> - /* Initialise SST Audio DSP */
> - if (sst->ops->init) {
> - err = sst->ops->init(sst, pdata);
> - if (err < 0)
> - return NULL;
> - }
> -
> - /* Register the ISR */
> - err = request_threaded_irq(sst->irq, sst->ops->irq_handler,
> - sst_dev->thread, IRQF_SHARED, "AudioDSP", sst);
> - if (err)
> - goto irq_err;
> -
> - err = sst_dma_new(sst);
> - if (err)
> - dev_warn(dev, "sst_dma_new failed %d\n", err);
> -
> - return sst;
> -
> -irq_err:
> - if (sst->ops->free)
> - sst->ops->free(sst);
> -
> - return NULL;
> -}
> -EXPORT_SYMBOL_GPL(sst_dsp_new);
> -
> -void sst_dsp_free(struct sst_dsp *sst)
> -{
> - free_irq(sst->irq, sst);
> - if (sst->ops->free)
> - sst->ops->free(sst);
> -
> - sst_dma_free(sst->dma);
> -}
> -EXPORT_SYMBOL_GPL(sst_dsp_free);
> -#endif
> -
> /* Module information */
> MODULE_AUTHOR("Liam Girdwood");
> MODULE_DESCRIPTION("Intel SST Core");
> diff --git a/sound/soc/intel/common/sst-dsp.h b/sound/soc/intel/common/sst-dsp.h
> index 0b84c719ec48..859f0de00339 100644
> --- a/sound/soc/intel/common/sst-dsp.h
> +++ b/sound/soc/intel/common/sst-dsp.h
> @@ -216,7 +216,7 @@ struct sst_pdata {
> void *dsp;
> };
>
> -#ifdef CONFIG_DW_DMAC_CORE
> +#if IS_ENABLED(CONFIG_DW_DMAC_CORE)
> /* Initialization */
> struct sst_dsp *sst_dsp_new(struct device *dev,
> struct sst_dsp_device *sst_dev, struct sst_pdata *pdata);
> diff --git a/sound/soc/intel/common/sst-firmware.c b/sound/soc/intel/common/sst-firmware.c
> index 25993527370b..a086c35f91bb 100644
> --- a/sound/soc/intel/common/sst-firmware.c
> +++ b/sound/soc/intel/common/sst-firmware.c
> @@ -1211,3 +1211,71 @@ u32 sst_dsp_get_offset(struct sst_dsp *dsp, u32 offset,
> }
> }
> EXPORT_SYMBOL_GPL(sst_dsp_get_offset);
> +
> +struct sst_dsp *sst_dsp_new(struct device *dev,
> + struct sst_dsp_device *sst_dev, struct sst_pdata *pdata)
> +{
> + struct sst_dsp *sst;
> + int err;
> +
> + dev_dbg(dev, "initialising audio DSP id 0x%x\n", pdata->id);
> +
> + sst = devm_kzalloc(dev, sizeof(*sst), GFP_KERNEL);
> + if (sst == NULL)
> + return NULL;
> +
> + spin_lock_init(&sst->spinlock);
> + mutex_init(&sst->mutex);
> + sst->dev = dev;
> + sst->dma_dev = pdata->dma_dev;
> + sst->thread_context = sst_dev->thread_context;
> + sst->sst_dev = sst_dev;
> + sst->id = pdata->id;
> + sst->irq = pdata->irq;
> + sst->ops = sst_dev->ops;
> + sst->pdata = pdata;
> + INIT_LIST_HEAD(&sst->used_block_list);
> + INIT_LIST_HEAD(&sst->free_block_list);
> + INIT_LIST_HEAD(&sst->module_list);
> + INIT_LIST_HEAD(&sst->fw_list);
> + INIT_LIST_HEAD(&sst->scratch_block_list);
> +
> + /* Initialise SST Audio DSP */
> + if (sst->ops->init) {
> + err = sst->ops->init(sst, pdata);
> + if (err < 0)
> + return NULL;
> + }
> +
> + /* Register the ISR */
> + err = request_threaded_irq(sst->irq, sst->ops->irq_handler,
> + sst_dev->thread, IRQF_SHARED, "AudioDSP", sst);
> + if (err)
> + goto irq_err;
> +
> + err = sst_dma_new(sst);
> + if (err)
> + dev_warn(dev, "sst_dma_new failed %d\n", err);
> +
> + return sst;
> +
> +irq_err:
> + if (sst->ops->free)
> + sst->ops->free(sst);
> +
> + return NULL;
> +}
> +EXPORT_SYMBOL_GPL(sst_dsp_new);
> +
> +void sst_dsp_free(struct sst_dsp *sst)
> +{
> + free_irq(sst->irq, sst);
> + if (sst->ops->free)
> + sst->ops->free(sst);
> +
> + sst_dma_free(sst->dma);
> +}
> +EXPORT_SYMBOL_GPL(sst_dsp_free);
> +
> +MODULE_DESCRIPTION("Intel SST Firmware Loader");
> +MODULE_LICENSE("GPL v2");
>
More information about the Alsa-devel
mailing list