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

[Cezary Rojewski <cezary.rojewski@xxxxxxxxx>]

On 2019-08-23 21:43, Pierre-Louis Bossart wrote:
> 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?

As basefw file name is never tied to specific board, 
snd_soc_acpi_mach::fw_filename existence ain't a good thing. It's 
misleading and can catch inexperienced developer off guard.

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

Goal is to elevate single struct - here sst_pdata - to be the unified 
platform descriptor. sst_acpi_desc "extends" sst_pdata. Baytrail _probe 
method mirrors Haswell to make both look cohesive. Some code duplication 
removal is also present.

Value? Since when we abandoned the quality principle?
Some fields in these structs are redundant and have been removed. 
Haswell/ Baytrail -specific code has been separated from sst framework 
to make it unaware of its descendants data. I call framework which knows 
its children secrets upfront a bad design.

Code does not even change much, it's simply cleaned up.

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