Messages in this thread |  | | Subject | Re: [PATCH v3 3/8] ASoC: qcom: lpass: Use regmap_field for i2sctl and dmactl registers | From | Srinivas Kandagatla <> | Date | Thu, 9 Jul 2020 11:06:52 +0100 |
| |
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@codeaurora.org> >> >> 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
|  |