Re: [PATCH v3] ASoC: qcom: Fix playback recover problem in suspend resume

[Srinivasa Rao Mandadapu]

Thanks a lot  Srinivas for your valuable review comments and for your 
time!!!

On 11/27/2020 5:39 PM, Srinivas Kandagatla wrote:
> On 27/11/2020 09:56, Srinivasa Rao Mandadapu wrote:
> > To support playback continuation after hard suspend(bypass powerd)
> >   and resume:
> > Prepare device in  platform trigger callback.
> > Make I2s and DMA control registers as non volatile.
> Looks like there are two changes here, One is fixing the volatile 
> registers!
>
>
> Other is preparing device after suspend!
>
> Consider splitting them!
Okay will split and share.
> >
>
> Fixes tag is missing here?
Actually fixing volatile registers is just partial reversal of commit 
b1824968221c ("ASoC: qcom: Fix enabling BCLK and LRCLK in LPAIF invalid 
state")

Will revert and share the patch separately.

> > Signed-off-by: V Sujith Kumar Reddy <vsujithk@xxxxxxxxxxxxxx>
> > Signed-off-by: Srinivasa Rao Mandadapu <srivasam@xxxxxxxxxxxxxx>
> > ---
> > Changes Since v1 and v2:
> >    -- Subject lines changed
> >
> >   sound/soc/qcom/lpass-cpu.c      | 8 ++------
> >   sound/soc/qcom/lpass-platform.c | 5 +++--
> >   2 files changed, 5 insertions(+), 8 deletions(-)
> >
> > diff --git a/sound/soc/qcom/lpass-cpu.c b/sound/soc/qcom/lpass-cpu.c
> > index af684fd..c99be03 100644
> > --- a/sound/soc/qcom/lpass-cpu.c
> > +++ b/sound/soc/qcom/lpass-cpu.c
> > @@ -454,20 +454,16 @@ static bool lpass_cpu_regmap_volatile(struct 
> > device *dev, unsigned int reg)
> >       struct lpass_variant *v = drvdata->variant;
> >       int i;
> >   -    for (i = 0; i < v->i2s_ports; ++i)
> > -        if (reg == LPAIF_I2SCTL_REG(v, i))
> > -            return true;
> >       for (i = 0; i < v->irq_ports; ++i)
> >           if (reg == LPAIF_IRQSTAT_REG(v, i))
> >               return true;
> >         for (i = 0; i < v->rdma_channels; ++i)
> > -        if (reg == LPAIF_RDMACURR_REG(v, i) || reg == 
> > LPAIF_RDMACTL_REG(v, i))
> > +        if (reg == LPAIF_RDMACURR_REG(v, i))
> >               return true;
> >         for (i = 0; i < v->wrdma_channels; ++i)
> > -        if (reg == LPAIF_WRDMACURR_REG(v, i + 
> > v->wrdma_channel_start) ||
> > -            reg == LPAIF_WRDMACTL_REG(v, i + v->wrdma_channel_start))
> > +        if (reg == LPAIF_WRDMACURR_REG(v, i + v->wrdma_channel_start))
> >               return true;
> >         return false;
> > diff --git a/sound/soc/qcom/lpass-platform.c 
> > b/sound/soc/qcom/lpass-platform.c
> > index 80b09de..2b0a7c1 100644
> > --- a/sound/soc/qcom/lpass-platform.c
> > +++ b/sound/soc/qcom/lpass-platform.c
> > @@ -481,8 +481,9 @@ static int lpass_platform_pcmops_trigger(struct 
> > snd_soc_component *component,
> >           return -ENOTRECOVERABLE;
> >       }
> >       switch (cmd) {
> > -    case SNDRV_PCM_TRIGGER_START:
> >       case SNDRV_PCM_TRIGGER_RESUME:
> > +        lpass_platform_pcmops_prepare(component, substream);
>
> Can you elaborate the actual issue here?
>
> Are any other registers needs to re-programmed??
>
> Does it make sense to use
> regcache_mark_dirty()
> regcache_sync() in pm suspend resume path,
> instead of calling prepare explicitly?
>
>
> --srini
Yes it's working fine with regcache APIs. will update the patch.
> > +    case SNDRV_PCM_TRIGGER_START:
> >       case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
> >           ret = regmap_fields_write(dmactl->enable, id,
> >                            LPAIF_DMACTL_ENABLE_ON);
> > @@ -592,7 +593,7 @@ static int lpass_platform_pcmops_trigger(struct 
> > snd_soc_component *component,
> >           break;
> >       }
> >   -    return 0;
> > +    return ret;
> >   }
> >     static snd_pcm_uframes_t lpass_platform_pcmops_pointer(
--
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.,
is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.