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@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!
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