Re: [PATCH 13/35] ASoC: Intel: Skylake: Reuse sst_dsp_new

[Cezary Rojewski <cezary.rojewski@xxxxxxxxx>]

On 2019-08-23 21:09, Pierre-Louis Bossart wrote:
> On 8/22/19 2:04 PM, Cezary Rojewski wrote:
> > skl_dsp_ctx_init is dumplication of sst_dsp_new and usage of such
>
> typo: duplication.

Ack.

> > bypasses natural DSP framework's flow. Remove it and reuse sst_dsp_new
> > constructor which invokes sst specific init internally so nothing is
> > missed.
> >
> > Skylake does not even define any sst_ops::init so portion of existing
> > skl_dsp_ctx_init can be regarded as DEADCODE.
>
> this is also hard to review, you have lines like
>
>  > -    return skl_dsp_acquire_irq(sst);
>  > +    return 0;
>
> that seem like a different change than what is described here.

Noted, thank you. Same as in sst_dsp_new case, the framework does that 
for us so nothing is missed and thus we remove the _acquire_irq. The 
whole patchset shows how much /skylake quality can be improved with 
_little_ effort granted framework provided in /common is made use of 
properly.

> > Signed-off-by: Cezary Rojewski <cezary.rojewski@xxxxxxxxx>
> > ---
> >   sound/soc/intel/skylake/bxt-sst.c       |  2 +-
> >   sound/soc/intel/skylake/cnl-sst.c       |  2 +-
> >   sound/soc/intel/skylake/skl-sst-dsp.c   | 28 -------------------------
> >   sound/soc/intel/skylake/skl-sst-dsp.h   |  2 --
> >   sound/soc/intel/skylake/skl-sst-utils.c |  6 +++++-
> >   sound/soc/intel/skylake/skl-sst.c       |  2 +-
> >   6 files changed, 8 insertions(+), 34 deletions(-)
> >
> > diff --git a/sound/soc/intel/skylake/bxt-sst.c 
> > b/sound/soc/intel/skylake/bxt-sst.c
> > index e3614acff34d..a8a2783f9b37 100644
> > --- a/sound/soc/intel/skylake/bxt-sst.c
> > +++ b/sound/soc/intel/skylake/bxt-sst.c
> > @@ -588,7 +588,7 @@ int bxt_sst_dsp_init(struct device *dev, void 
> > __iomem *mmio_base, int irq,
> >       INIT_DELAYED_WORK(&skl->d0i3.work, bxt_set_dsp_D0i3);
> >       skl->d0i3.state = SKL_DSP_D0I3_NONE;
> > -    return skl_dsp_acquire_irq(sst);
> > +    return 0;
> >   }
> >   EXPORT_SYMBOL_GPL(bxt_sst_dsp_init);
> > diff --git a/sound/soc/intel/skylake/cnl-sst.c 
> > b/sound/soc/intel/skylake/cnl-sst.c
> > index 84dc6b82831d..0b0337f6ebff 100644
> > --- a/sound/soc/intel/skylake/cnl-sst.c
> > +++ b/sound/soc/intel/skylake/cnl-sst.c
> > @@ -460,7 +460,7 @@ int cnl_sst_dsp_init(struct device *dev, void 
> > __iomem *mmio_base, int irq,
> >       cnl->boot_complete = false;
> >       init_waitqueue_head(&cnl->boot_wait);
> > -    return skl_dsp_acquire_irq(sst);
> > +    return 0;
> >   }
> >   EXPORT_SYMBOL_GPL(cnl_sst_dsp_init);
> > diff --git a/sound/soc/intel/skylake/skl-sst-dsp.c 
> > b/sound/soc/intel/skylake/skl-sst-dsp.c
> > index 1c4ecbcd7db7..9d8eb1af4798 100644
> > --- a/sound/soc/intel/skylake/skl-sst-dsp.c
> > +++ b/sound/soc/intel/skylake/skl-sst-dsp.c
> > @@ -418,34 +418,6 @@ int skl_dsp_sleep(struct sst_dsp *ctx)
> >   }
> >   EXPORT_SYMBOL_GPL(skl_dsp_sleep);
> > -struct sst_dsp *skl_dsp_ctx_init(struct device *dev,
> > -        struct sst_pdata *pdata, int irq)
> > -{
> > -    int ret;
> > -    struct sst_dsp *sst;
> > -
> > -    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->pdata = pdata;
> > -    sst->irq = irq;
> > -    sst->ops = pdata->ops;
> > -    sst->thread_context = pdata->dsp;
> > -
> > -    /* Initialise SST Audio DSP */
> > -    if (sst->ops->init) {
> > -        ret = sst->ops->init(sst, NULL);
> > -        if (ret < 0)
> > -            return NULL;
> > -    }
> > -
> > -    return sst;
> > -}
> > -
> >   int skl_dsp_acquire_irq(struct sst_dsp *sst)
> >   {
> >       int ret;
> > diff --git a/sound/soc/intel/skylake/skl-sst-dsp.h 
> > b/sound/soc/intel/skylake/skl-sst-dsp.h
> > index ba37433e4efa..1d579d59de60 100644
> > --- a/sound/soc/intel/skylake/skl-sst-dsp.h
> > +++ b/sound/soc/intel/skylake/skl-sst-dsp.h
> > @@ -196,8 +196,6 @@ int skl_cldma_prepare(struct sst_dsp *ctx);
> >   int skl_cldma_wait_interruptible(struct sst_dsp *ctx);
> >   void skl_dsp_set_state_locked(struct sst_dsp *ctx, int state);
> > -struct sst_dsp *skl_dsp_ctx_init(struct device *dev,
> > -        struct sst_pdata *pdata, int irq);
> >   int skl_dsp_acquire_irq(struct sst_dsp *sst);
> >   bool is_skl_dsp_running(struct sst_dsp *ctx);
> > diff --git a/sound/soc/intel/skylake/skl-sst-utils.c 
> > b/sound/soc/intel/skylake/skl-sst-utils.c
> > index 9061a9b17ea0..8e03a10855c4 100644
> > --- a/sound/soc/intel/skylake/skl-sst-utils.c
> > +++ b/sound/soc/intel/skylake/skl-sst-utils.c
> > @@ -6,6 +6,7 @@
> >    */
> >   #include <linux/device.h>
> > +#include <linux/pci.h>
> >   #include <linux/slab.h>
> >   #include <linux/uuid.h>
> >   #include "../common/sst-dsp.h"
> > @@ -360,10 +361,13 @@ int skl_sst_ctx_init(struct device *dev, int 
> > irq, const char *fw_name,
> >       struct skl_dev *skl = *dsp;
> >       struct sst_dsp *sst;
> > +    pdata->id = skl->pci->device;
> > +    pdata->irq = irq;
> > +    pdata->resindex_dma_base = -1;
> >       skl->dev = dev;
> >       pdata->dsp = skl;
> >       INIT_LIST_HEAD(&skl->uuid_list);
> > -    skl->dsp = skl_dsp_ctx_init(dev, pdata, irq);
> > +    skl->dsp = sst_dsp_new(dev, pdata);
> >       if (!skl->dsp) {
> >           dev_err(skl->dev, "%s: no device\n", __func__);
> >           return -ENODEV;
> > diff --git a/sound/soc/intel/skylake/skl-sst.c 
> > b/sound/soc/intel/skylake/skl-sst.c
> > index e55523826346..6bb003add9e2 100644
> > --- a/sound/soc/intel/skylake/skl-sst.c
> > +++ b/sound/soc/intel/skylake/skl-sst.c
> > @@ -550,7 +550,7 @@ int skl_sst_dsp_init(struct device *dev, void 
> > __iomem *mmio_base, int irq,
> >       sst->fw_ops = skl_fw_ops;
> > -    return skl_dsp_acquire_irq(sst);
> > +    return 0;
> >   }
> >   EXPORT_SYMBOL_GPL(skl_sst_dsp_init);
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel