Re: [PATCH 24/35] ASoC: Intel: Refactor probing of ACPI devices

[Pierre-Louis Bossart <pierre-louis.bossart@xxxxxxxxxxxxxxx>]

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@xxxxxxxxx>
> ---
>   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);
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel