Re: [PATCH v3 3/8] ASoC: qcom: lpass: Use regmap_field for i2sctl and dmactl registers

[Srinivas Kandagatla]

On 09/07/2020 10:57, Rohit Kumar wrote:
> Thanks Srini for reviewing.
>
> On 7/9/2020 2:56 PM, Srinivas Kandagatla wrote:
> > On 08/07/2020 06:08, Rohit kumar wrote:
> > > I2SCTL and DMACTL registers has different bits alignment for newer
> > > LPASS variants of SC7180 soc. Use REG_FIELD_ID() to define the
> > > reg_fields in platform specific file and removed shifts and mask
> > > macros for such registers from header file.
> > >
> > > Signed-off-by: Rohit kumar <rohitkr@xxxxxxxxxxxxxx>
> >
> > Thanks Rohit for doing this, this looks much better now!
> > I have few minor comments..
> >
> > > ---
> > > Â sound/soc/qcom/lpass-apq8016.cÂÂ |Â 24 ++++++
> > > Â sound/soc/qcom/lpass-cpu.cÂÂÂÂÂÂ | 163 
> > > +++++++++++++++++++++++----------------
> > > Â sound/soc/qcom/lpass-ipq806x.cÂÂ |Â 24 ++++++
> > > Â sound/soc/qcom/lpass-lpaif-reg.h | 157 
> > > +++++++++++++++++++------------------
> > >  sound/soc/qcom/lpass-platform.c | 151 
> > > +++++++++++++++++++++++++++---------
> > > Â sound/soc/qcom/lpass.hÂÂÂÂÂÂÂÂÂÂ |Â 53 +++++++++++++
> > > Â 6 files changed, 398 insertions(+), 174 deletions(-)
> >
> > index f0c7e93..f358d12 100644
> > > --- a/sound/soc/qcom/lpass-cpu.c
> > > +++ b/sound/soc/qcom/lpass-cpu.c
> > > @@ -29,6 +29,32 @@
> > > Â #define LPASS_CPU_I2S_SD0_1_2_MASKÂÂÂ GENMASK(2, 0)
> > > Â #define LPASS_CPU_I2S_SD0_1_2_3_MASKÂÂÂ GENMASK(3, 0)
> >
> >
> > > Â }
> > > Â -static int lpass_cpu_daiops_hw_free(struct snd_pcm_substream 
> > > *substream,
> > > -ÂÂÂÂÂÂÂ struct snd_soc_dai *dai)
> > > -{
> > > -ÂÂÂ struct lpass_data *drvdata = snd_soc_dai_get_drvdata(dai);
> > > -ÂÂÂ int ret;
> > > -
> > > -ÂÂÂ ret = regmap_write(drvdata->lpaif_map,
> > > -ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ LPAIF_I2SCTL_REG(drvdata->variant, dai->driver->id),
> > > -ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ 0);
> > > -ÂÂÂ if (ret)
> > > -ÂÂÂÂÂÂÂ dev_err(dai->dev, "error writing to i2sctl reg: %d\n", ret);
> > > -
> > > -ÂÂÂ return ret;
> > > -}
> >
> > Any particular reason why this function remove
>
> This was causing issue in playback/capture concurrency. It sets I2SCTL 
> register value to 0
>
> when usecase ends. However, playback/capture specific bits are already 
> cleared during trigger() stop
>
> function. So, this is not needed.

This should be sent as separate fix with fixes tag!

> > > diff --git a/sound/soc/qcom/lpass-lpaif-reg.h 
> > > b/sound/soc/qcom/lpass-lpaif-reg.h
> > > index 72a3e2f..5258e60 100644
> > > --- a/sound/soc/qcom/lpass-lpaif-reg.h
> > > +++ b/sound/soc/qcom/lpass-lpaif-reg.h
> > > @@ -12,15 +12,12 @@
> > ...
> > > Â #endif /* __LPASS_LPAIF_REG_H__ */
> > > diff --git a/sound/soc/qcom/lpass-platform.c 
> > > b/sound/soc/qcom/lpass-platform.c
> > > index 34f7fd1..445ca193 100644
> > > --- a/sound/soc/qcom/lpass-platform.c
> > > +++ b/sound/soc/qcom/lpass-platform.c
> > > @@ -50,6 +50,53 @@ static const struct snd_pcm_hardware 
> > > lpass_platform_pcm_hardware = {
> > > ÂÂÂÂÂ .fifo_sizeÂÂÂÂÂÂÂ =ÂÂÂ 0,
> > > Â };
> > ...
> > > Â static int lpass_platform_pcmops_open(struct snd_soc_component 
> > > *component,
> > > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct snd_pcm_substream *substream)
> > > Â {
> > > @@ -59,9 +106,9 @@ static int lpass_platform_pcmops_open(struct 
> > > snd_soc_component *component,
> > > ÂÂÂÂÂ struct lpass_data *drvdata = 
> > > snd_soc_component_get_drvdata(component);
> > > ÂÂÂÂÂ struct lpass_variant *v = drvdata->variant;
> > > ÂÂÂÂÂ int ret, dma_ch, dir = substream->stream;
> > > -ÂÂÂ struct lpass_pcm_data *data;
> > > +ÂÂÂ struct lpass_pcm_data *data = NULL;
> > > Â -ÂÂÂ data = devm_kzalloc(soc_runtime->dev, sizeof(*data), GFP_KERNEL);
> > > +ÂÂÂ data = kzalloc(sizeof(*data), GFP_KERNEL);
> >
> > Does this change belong in this patch?
>
>
> As part of this change, I fixed memory leak too by adding kfree() in 
> close()
>
> However, this was causing issue as memory was allocated using 
> devm_kzalloc().
>
> Should I move it to different patch?

That would be the right thing to do, can also add fixes tag!


> > > ÂÂÂÂÂ if (!data)
> > > ÂÂÂÂÂÂÂÂÂ return -ENOMEM;
> > > Â @@ -111,13 +158,13 @@ static int lpass_platform_pcmops_close(struct 
> > > snd_soc_component *component,
> > > ÂÂÂÂÂ struct snd_pcm_runtime *runtime = substream->runtime;
> > > ÂÂÂÂÂ struct lpass_data *drvdata = 
> > > snd_soc_component_get_drvdata(component);
> > > ÂÂÂÂÂ struct lpass_variant *v = drvdata->variant;

> > Above two along with rddma members can be removed, these become 
> > redundant after adding regmap field!
> wrdma_channels is used in alloc_dma_channel() to get the channel id.
>
> Also, both are used for other DMA registers such as LPAIF_RDMABASE_REG,
>
> LPAIF_RDMABUFF_REG, LPAIF_RDMACURR_REG, etc.
Ah I see we are still using this in lpass_cpu_regmap_writeable!
ignore my previous comments about removing them!

--srini