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

[Rohit Kumar]

On 7/9/2020 3:36 PM, Srinivas Kandagatla wrote:
> 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!
Ok. Will create separate patch with fixes tag and post.
> > > > 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!


Sure, Will do that in next spin.

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

--
Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc.is a member
of the Code Aurora Forum, hosted by the Linux Foundation.