[alsa-devel] [PATCH 24/35] ASoC: Intel: Refactor probing of ACPI devices
Pierre-Louis Bossart
pierre-louis.bossart at linux.intel.com
Fri Aug 23 21:43:33 CEST 2019
On 8/22/19 2:04 PM, Cezary Rojewski wrote:
> Baytrail and Haswell ACPI loading is now separated and no longer
> clutters common code. Let's improve the loading procedure and remove
> some superfluous members.
>
> This change removes sst_pdata::resindex_dma_base as it is a duplication
> of dma_base. dma_base field has had it's type changed to allow for -1
> (not used) value.
>
> ACPI descriptor: sst_acpi_desc loses machines field and sst_id - now
> accessed via sst_pdata::boards and sst_pdata::id respectively.
> Cleanup consists mainly of legacy platform-specific probe routines
> being provided for each descendant. Prevents code duplications,
> especially for HSW/ BDW case while not losing any readability.
>
> Signed-off-by: Cezary Rojewski <cezary.rojewski at intel.com>
> ---
> sound/soc/intel/baytrail/acpi.c | 43 +++++++++++-----
> sound/soc/intel/common/sst-acpi.c | 21 +++-----
> sound/soc/intel/common/sst-dsp.h | 8 +--
> sound/soc/intel/common/sst-firmware.c | 2 +-
> sound/soc/intel/haswell/acpi.c | 65 +++++++++++++++----------
> sound/soc/intel/skylake/skl-sst-utils.c | 2 +-
> 6 files changed, 82 insertions(+), 59 deletions(-)
>
> diff --git a/sound/soc/intel/baytrail/acpi.c b/sound/soc/intel/baytrail/acpi.c
> index 57d10a6e3be2..bf2560a8f3e2 100644
> --- a/sound/soc/intel/baytrail/acpi.c
> +++ b/sound/soc/intel/baytrail/acpi.c
> @@ -11,25 +11,46 @@
> #include <sound/soc-acpi-intel-match.h>
> #include "../common/sst-dsp.h"
>
> -static struct sst_acpi_desc byt_acpi_desc = {
> - .drv_name = "baytrail-pcm-audio",
> - .machines = snd_soc_acpi_intel_baytrail_legacy_machines,
> - .resindex_lpe_base = 0,
> - .resindex_pcicfg_base = 1,
> - .resindex_fw_base = 2,
> - .irqindex_host_ipc = 5,
> - .sst_id = SST_DEV_ID_BYT,
> - .resindex_dma_base = -1,
> +static struct sst_pdata byt_desc = {
> + .id = SST_DEV_ID_BYT,
> + .fw_name = "intel/fw_sst_0f28.bin-48kHz_i2s_master",
> + .boards = snd_soc_acpi_intel_baytrail_legacy_machines,
So instead of simplifying you are duplicating the fw_name here.
struct snd_soc_acpi_mach snd_soc_acpi_intel_baytrail_legacy_machines[] = {
{
.id = "10EC5640",
.drv_name = "byt-rt5640",
.fw_filename = "intel/fw_sst_0f28.bin-48kHz_i2s_master",
},
{
.id = "193C9890",
.drv_name = "byt-max98090",
.fw_filename = "intel/fw_sst_0f28.bin-48kHz_i2s_master",
},
{}
};
What is the value of all this?
> + .dma_base = -1,
> };
>
> static const struct acpi_device_id byt_acpi_ids[] = {
> - { "80860F28", (unsigned long)&byt_acpi_desc },
> + { "80860F28", (unsigned long)&byt_desc },
> { }
> };
> MODULE_DEVICE_TABLE(acpi, byt_acpi_ids);
>
> +static int byt_acpi_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct sst_acpi_desc *acpi_desc;
> + const struct acpi_device_id *id;
> +
> + id = acpi_match_device(dev->driver->acpi_match_table, dev);
> + if (!id)
> + return -ENODEV;
> +
> + acpi_desc = devm_kzalloc(dev, sizeof(*acpi_desc), GFP_KERNEL);
> + if (!acpi_desc)
> + return -ENOMEM;
> +
> + acpi_desc->drv_name = "baytrail-pcm-audio";
> + acpi_desc->pdata = (struct sst_pdata *)id->driver_data;
> + acpi_desc->resindex_lpe_base = 0;
> + acpi_desc->resindex_pcicfg_base = 1;
> + acpi_desc->resindex_fw_base = 2;
> + acpi_desc->irqindex_host_ipc = 5;
so we had a nice table and now it's inlined in the code. Yuk.
I really don't see the point of these 'simplifications'
And what do they have to do with Skylake anyways, it's PCI enumerated so
why should we change this?
> + platform_set_drvdata(pdev, acpi_desc);
> +
> + return sst_acpi_probe(pdev);
> +}
> +
> static struct platform_driver byt_acpi_driver = {
> - .probe = sst_acpi_probe,
> + .probe = byt_acpi_probe,
> .remove = sst_acpi_remove,
> .driver = {
> .name = "byt-acpi",
> diff --git a/sound/soc/intel/common/sst-acpi.c b/sound/soc/intel/common/sst-acpi.c
> index 8e75126106ea..53ac23f05966 100644
> --- a/sound/soc/intel/common/sst-acpi.c
> +++ b/sound/soc/intel/common/sst-acpi.c
> @@ -17,7 +17,6 @@
> struct sst_acpi_priv {
> struct platform_device *pdev_mach;
> struct platform_device *pdev_pcm;
> - struct sst_pdata sst_pdata;
> struct sst_acpi_desc *desc;
> struct snd_soc_acpi_mach *mach;
> };
> @@ -27,8 +26,8 @@ static void sst_acpi_fw_cb(const struct firmware *fw, void *context)
> struct platform_device *pdev = context;
> struct device *dev = &pdev->dev;
> struct sst_acpi_priv *sst_acpi = platform_get_drvdata(pdev);
> - struct sst_pdata *sst_pdata = &sst_acpi->sst_pdata;
> struct sst_acpi_desc *desc = sst_acpi->desc;
> + struct sst_pdata *sst_pdata = desc->pdata;
> struct snd_soc_acpi_mach *mach = sst_acpi->mach;
>
> sst_pdata->fw = fw;
> @@ -51,7 +50,6 @@ static void sst_acpi_fw_cb(const struct firmware *fw, void *context)
>
> int sst_acpi_probe(struct platform_device *pdev)
> {
> - const struct acpi_device_id *id;
> struct device *dev = &pdev->dev;
> struct sst_acpi_priv *sst_acpi;
> struct sst_pdata *sst_pdata;
> @@ -64,27 +62,20 @@ int sst_acpi_probe(struct platform_device *pdev)
> if (sst_acpi == NULL)
> return -ENOMEM;
>
> - id = acpi_match_device(dev->driver->acpi_match_table, dev);
> - if (!id)
> - return -ENODEV;
> -
> - desc = (struct sst_acpi_desc *)id->driver_data;
> - mach = snd_soc_acpi_find_machine(desc->machines);
> + desc = platform_get_drvdata(pdev);
> + sst_pdata = desc->pdata;
> + mach = snd_soc_acpi_find_machine(sst_pdata->boards);
> if (mach == NULL) {
> dev_err(dev, "No matching ASoC machine driver found\n");
> return -ENODEV;
> }
>
> - sst_pdata = &sst_acpi->sst_pdata;
> - sst_pdata->id = desc->sst_id;
> sst_pdata->dma_dev = dev;
> sst_acpi->desc = desc;
> sst_acpi->mach = mach;
>
> - sst_pdata->resindex_dma_base = desc->resindex_dma_base;
> - if (desc->resindex_dma_base >= 0) {
> + if (sst_pdata->dma_base >= 0) {
> sst_pdata->dma_engine = desc->dma_engine;
> - sst_pdata->dma_base = desc->resindex_dma_base;
> sst_pdata->dma_size = desc->dma_size;
> }
>
> @@ -140,7 +131,7 @@ EXPORT_SYMBOL_GPL(sst_acpi_probe);
> int sst_acpi_remove(struct platform_device *pdev)
> {
> struct sst_acpi_priv *sst_acpi = platform_get_drvdata(pdev);
> - struct sst_pdata *sst_pdata = &sst_acpi->sst_pdata;
> + struct sst_pdata *sst_pdata = sst_acpi->desc->pdata;
>
> platform_device_unregister(sst_acpi->pdev_mach);
> if (!IS_ERR_OR_NULL(sst_acpi->pdev_pcm))
> diff --git a/sound/soc/intel/common/sst-dsp.h b/sound/soc/intel/common/sst-dsp.h
> index a2ac7998fbdb..87d39b0e79c0 100644
> --- a/sound/soc/intel/common/sst-dsp.h
> +++ b/sound/soc/intel/common/sst-dsp.h
> @@ -171,16 +171,13 @@ struct platform_device;
> /* Descriptor for setting up SST platform data */
> struct sst_acpi_desc {
> const char *drv_name;
> - struct snd_soc_acpi_mach *machines;
> + struct sst_pdata *pdata;
> /* Platform resource indexes. Must set to -1 if not used */
> int resindex_lpe_base;
> int resindex_pcicfg_base;
> int resindex_fw_base;
> int irqindex_host_ipc;
> int resindex_dma_base;
> - /* Unique number identifying the SST core on platform */
> - int sst_id;
> - /* DMA only valid when resindex_dma_base != -1*/
> int dma_engine;
> int dma_size;
> };
> @@ -205,8 +202,7 @@ struct sst_pdata {
> const struct firmware *fw;
>
> /* DMA */
> - int resindex_dma_base; /* other fields invalid if equals to -1 */
> - u32 dma_base;
> + int dma_base; /* other fields invalid if equals to -1 */
> u32 dma_size;
> int dma_engine;
> struct device *dma_dev;
> diff --git a/sound/soc/intel/common/sst-firmware.c b/sound/soc/intel/common/sst-firmware.c
> index 6b6af11c32c3..61d3e6e46b98 100644
> --- a/sound/soc/intel/common/sst-firmware.c
> +++ b/sound/soc/intel/common/sst-firmware.c
> @@ -268,7 +268,7 @@ static int sst_dma_new(struct sst_dsp *sst)
> struct resource mem;
> int ret = 0;
>
> - if (sst->pdata->resindex_dma_base == -1)
> + if (sst->pdata->dma_base == -1)
> /* DMA is not used, return and squelsh error messages */
> return 0;
>
> diff --git a/sound/soc/intel/haswell/acpi.c b/sound/soc/intel/haswell/acpi.c
> index 7bd8b03851c4..3c49ec257e56 100644
> --- a/sound/soc/intel/haswell/acpi.c
> +++ b/sound/soc/intel/haswell/acpi.c
> @@ -15,41 +15,56 @@
> #define SST_WPT_DSP_DMA_ADDR_OFFSET 0x0FE000
> #define SST_LPT_DSP_DMA_SIZE (1024 - 1)
>
> -static struct sst_acpi_desc hsw_acpi_desc = {
> - .drv_name = "haswell-pcm-audio",
> - .machines = snd_soc_acpi_intel_haswell_machines,
> - .resindex_lpe_base = 0,
> - .resindex_pcicfg_base = 1,
> - .resindex_fw_base = -1,
> - .irqindex_host_ipc = 0,
> - .sst_id = SST_DEV_ID_LYNX_POINT,
> - .dma_engine = SST_DMA_TYPE_DW,
> - .resindex_dma_base = SST_LPT_DSP_DMA_ADDR_OFFSET,
> - .dma_size = SST_LPT_DSP_DMA_SIZE,
> +static struct sst_pdata hsw_desc = {
> + .id = SST_DEV_ID_LYNX_POINT,
> + .fw_name = "intel/IntcSST1.bin",
> + .boards = snd_soc_acpi_intel_haswell_machines,
> + .dma_base = SST_LPT_DSP_DMA_ADDR_OFFSET,
> };
>
> -static struct sst_acpi_desc bdw_acpi_desc = {
> - .drv_name = "haswell-pcm-audio",
> - .machines = snd_soc_acpi_intel_broadwell_machines,
> - .resindex_lpe_base = 0,
> - .resindex_pcicfg_base = 1,
> - .resindex_fw_base = -1,
> - .irqindex_host_ipc = 0,
> - .sst_id = SST_DEV_ID_WILDCAT_POINT,
> - .dma_engine = SST_DMA_TYPE_DW,
> - .resindex_dma_base = SST_WPT_DSP_DMA_ADDR_OFFSET,
> - .dma_size = SST_LPT_DSP_DMA_SIZE,
> +static struct sst_pdata bdw_desc = {
> + .id = SST_DEV_ID_WILDCAT_POINT,
> + .fw_name = "intel/IntcSST2.bin",
> + .boards = snd_soc_acpi_intel_broadwell_machines,
> + .dma_base = SST_WPT_DSP_DMA_ADDR_OFFSET,
> };
>
> static const struct acpi_device_id hsw_acpi_ids[] = {
> - { "INT33C8", (unsigned long)&hsw_acpi_desc },
> - { "INT3438", (unsigned long)&bdw_acpi_desc },
> + { "INT33C8", (unsigned long)&hsw_desc },
> + { "INT3438", (unsigned long)&bdw_desc },
> { }
> };
> MODULE_DEVICE_TABLE(acpi, hsw_acpi_ids);
>
> +static int hsw_acpi_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct sst_acpi_desc *acpi_desc;
> + const struct acpi_device_id *id;
> +
> + id = acpi_match_device(dev->driver->acpi_match_table, dev);
> + if (!id)
> + return -ENODEV;
> +
> + acpi_desc = devm_kzalloc(dev, sizeof(*acpi_desc), GFP_KERNEL);
> + if (!acpi_desc)
> + return -ENOMEM;
> +
> + acpi_desc->drv_name = "haswell-pcm-audio";
> + acpi_desc->pdata = (struct sst_pdata *)id->driver_data;
> + acpi_desc->resindex_lpe_base = 0;
> + acpi_desc->resindex_pcicfg_base = 1;
> + acpi_desc->resindex_fw_base = -1;
> + acpi_desc->irqindex_host_ipc = 0;
> + acpi_desc->dma_engine = SST_DMA_TYPE_DW;
> + acpi_desc->dma_size = SST_LPT_DSP_DMA_SIZE;
> + platform_set_drvdata(pdev, acpi_desc);
> +
> + return sst_acpi_probe(pdev);
> +}
> +
> static struct platform_driver hsw_acpi_driver = {
> - .probe = sst_acpi_probe,
> + .probe = hsw_acpi_probe,
> .remove = sst_acpi_remove,
> .driver = {
> .name = "hsw-acpi",
> diff --git a/sound/soc/intel/skylake/skl-sst-utils.c b/sound/soc/intel/skylake/skl-sst-utils.c
> index bbe67e298efe..ac0a0e4c2d68 100644
> --- a/sound/soc/intel/skylake/skl-sst-utils.c
> +++ b/sound/soc/intel/skylake/skl-sst-utils.c
> @@ -363,7 +363,7 @@ int skl_sst_ctx_init(struct device *dev, int irq, const char *fw_name,
>
> pdata->id = skl->pci->device;
> pdata->irq = irq;
> - pdata->resindex_dma_base = -1;
> + pdata->dma_base = -1;
> skl->dev = dev;
> pdata->dsp = skl;
> INIT_LIST_HEAD(&skl->uuid_list);
>
More information about the Alsa-devel
mailing list