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

From: Rohit Kumar
Date: Thu Jul 09 2020 - 05:57:57 EST

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_set_sysclk(struct snd_soc_dai *dai, int clk_id,
ÂÂÂÂÂÂÂÂÂ unsigned int freq, int dir)
 {
@@ -79,12 +105,13 @@ static int lpass_cpu_daiops_hw_params(struct snd_pcm_substream *substream,
ÂÂÂÂÂÂÂÂÂ struct snd_pcm_hw_params *params, struct snd_soc_dai *dai)
 {
ÂÂÂÂÂ struct lpass_data *drvdata = snd_soc_dai_get_drvdata(dai);
+ÂÂÂ struct lpaif_i2sctl *i2sctl = drvdata->i2sctl;
+ÂÂÂ unsigned int id = dai->driver->id;
ÂÂÂÂÂ snd_pcm_format_t format = params_format(params);
ÂÂÂÂÂ unsigned int channels = params_channels(params);
ÂÂÂÂÂ unsigned int rate = params_rate(params);
ÂÂÂÂÂ unsigned int mode;
-ÂÂÂ unsigned int regval;
-ÂÂÂ int bitwidth, ret;
+ÂÂÂ int bitwidth, ret, regval;

Why is this change?

Sorry, It came as part of previous patchset. regval was removed in patchv2Â and

bitwidth variable was used to configure both clk and i2sctl register, which was not proper.

Added regval again. I will repost this patch without this change.


 Â bitwidth = snd_pcm_format_width(format);
ÂÂÂÂÂ if (bitwidth < 0) {
@@ -92,28 +119,45 @@ static int lpass_cpu_daiops_hw_params(struct snd_pcm_substream *substream,
ÂÂÂÂÂÂÂÂÂ return bitwidth;
ÂÂÂÂÂ }
 - regval = LPAIF_I2SCTL_LOOPBACK_DISABLE |
-ÂÂÂÂÂÂÂÂÂÂÂ LPAIF_I2SCTL_WSSRC_INTERNAL;
+ÂÂÂ ret = regmap_fields_write(i2sctl->loopback, id,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ LPAIF_I2SCTL_LOOPBACK_DISABLE);
+ÂÂÂ if (ret) {
+ÂÂÂÂÂÂÂ dev_err(dai->dev, "error updating loopback field: %d\n", ret);
+ÂÂÂÂÂÂÂ return ret;
+ÂÂÂ }
+
+ÂÂÂ ret = regmap_fields_write(i2sctl->wssrc, id,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ LPAIF_I2SCTL_WSSRC_INTERNAL);
+ÂÂÂ if (ret) {
+ÂÂÂÂÂÂÂ dev_err(dai->dev, "error updating wssrc field: %d\n", ret);
+ÂÂÂÂÂÂÂ return ret;
+ÂÂÂ }
 Â switch (bitwidth) {
ÂÂÂÂÂ case 16:
-ÂÂÂÂÂÂÂ regval |= LPAIF_I2SCTL_BITWIDTH_16;
+ÂÂÂÂÂÂÂ regval = LPAIF_I2SCTL_BITWIDTH_16;
ÂÂÂÂÂÂÂÂÂ break;
ÂÂÂÂÂ case 24:
-ÂÂÂÂÂÂÂ regval |= LPAIF_I2SCTL_BITWIDTH_24;
+ÂÂÂÂÂÂÂ regval = LPAIF_I2SCTL_BITWIDTH_24;
ÂÂÂÂÂÂÂÂÂ break;
ÂÂÂÂÂ case 32:
-ÂÂÂÂÂÂÂ regval |= LPAIF_I2SCTL_BITWIDTH_32;
+ÂÂÂÂÂÂÂ regval = LPAIF_I2SCTL_BITWIDTH_32;
ÂÂÂÂÂÂÂÂÂ break;
ÂÂÂÂÂ default:
ÂÂÂÂÂÂÂÂÂ dev_err(dai->dev, "invalid bitwidth given: %d\n", bitwidth);
ÂÂÂÂÂÂÂÂÂ return -EINVAL;
ÂÂÂÂÂ }
 + ret = regmap_fields_write(i2sctl->bitwidth, id, regval);
+ÂÂÂ if (ret) {
+ÂÂÂÂÂÂÂ dev_err(dai->dev, "error updating bitwidth field: %d\n", ret);
+ÂÂÂÂÂÂÂ return ret;
+ÂÂÂ }
+
ÂÂÂÂÂ if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
-ÂÂÂÂÂÂÂ mode = drvdata->mi2s_playback_sd_mode[dai->driver->id];
+ÂÂÂÂÂÂÂ mode = drvdata->mi2s_playback_sd_mode[id];
ÂÂÂÂÂ else
-ÂÂÂÂÂÂÂ mode = drvdata->mi2s_capture_sd_mode[dai->driver->id];
+ÂÂÂÂÂÂÂ mode = drvdata->mi2s_capture_sd_mode[id];
 Â if (!mode) {
ÂÂÂÂÂÂÂÂÂ dev_err(dai->dev, "no line is assigned\n");
@@ -175,30 +219,34 @@ static int lpass_cpu_daiops_hw_params(struct snd_pcm_substream *substream,
ÂÂÂÂÂ }
 Â if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
-ÂÂÂÂÂÂÂ regval |= LPAIF_I2SCTL_SPKMODE(mode);
+ÂÂÂÂÂÂÂ ret = regmap_fields_write(i2sctl->spkmode, id,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ LPAIF_I2SCTL_SPKMODE(mode));
 Â if (channels >= 2)
-ÂÂÂÂÂÂÂÂÂÂÂ regval |= LPAIF_I2SCTL_SPKMONO_STEREO;
+ÂÂÂÂÂÂÂÂÂÂÂ ret = regmap_fields_write(i2sctl->spkmono, id,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ LPAIF_I2SCTL_SPKMONO_STEREO);
ÂÂÂÂÂÂÂÂÂ else
-ÂÂÂÂÂÂÂÂÂÂÂ regval |= LPAIF_I2SCTL_SPKMONO_MONO;
+ÂÂÂÂÂÂÂÂÂÂÂ ret = regmap_fields_write(i2sctl->spkmono, id,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ LPAIF_I2SCTL_SPKMONO_MONO);
ÂÂÂÂÂ } else {
-ÂÂÂÂÂÂÂ regval |= LPAIF_I2SCTL_MICMODE(mode);
+ÂÂÂÂÂÂÂ ret = regmap_fields_write(i2sctl->micmode, id,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ LPAIF_I2SCTL_MICMODE(mode));
 Â if (channels >= 2)
-ÂÂÂÂÂÂÂÂÂÂÂ regval |= LPAIF_I2SCTL_MICMONO_STEREO;
+ÂÂÂÂÂÂÂÂÂÂÂ ret = regmap_fields_write(i2sctl->micmono, id,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ LPAIF_I2SCTL_MICMONO_STEREO);
ÂÂÂÂÂÂÂÂÂ else
-ÂÂÂÂÂÂÂÂÂÂÂ regval |= LPAIF_I2SCTL_MICMONO_MONO;
+ÂÂÂÂÂÂÂÂÂÂÂ ret = regmap_fields_write(i2sctl->micmono, id,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ LPAIF_I2SCTL_MICMONO_MONO);
ÂÂÂÂÂ }
 - ret = regmap_write(drvdata->lpaif_map,
-ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ LPAIF_I2SCTL_REG(drvdata->variant, dai->driver->id),
-ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ regval);
ÂÂÂÂÂ if (ret) {
-ÂÂÂÂÂÂÂ dev_err(dai->dev, "error writing to i2sctl reg: %d\n", ret);
+ÂÂÂÂÂÂÂ dev_err(dai->dev, "error writing to i2sctl channels mode: %d\n",
+ÂÂÂÂÂÂÂÂÂÂÂ ret);
ÂÂÂÂÂÂÂÂÂ return ret;
ÂÂÂÂÂ }
 - ret = clk_set_rate(drvdata->mi2s_bit_clk[dai->driver->id],
+ÂÂÂ ret = clk_set_rate(drvdata->mi2s_bit_clk[id],
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ rate * bitwidth * 2);
ÂÂÂÂÂ if (ret) {
ÂÂÂÂÂÂÂÂÂ dev_err(dai->dev, "error setting mi2s bitclk to %u: %d\n",
@@ -209,41 +257,24 @@ static int lpass_cpu_daiops_hw_params(struct snd_pcm_substream *substream,
ÂÂÂÂÂ return 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.

-
 static int lpass_cpu_daiops_prepare(struct snd_pcm_substream *substream,
ÂÂÂÂÂÂÂÂÂ struct snd_soc_dai *dai)
 {
ÂÂÂÂÂ struct lpass_data *drvdata = snd_soc_dai_get_drvdata(dai);
+ÂÂÂ struct lpaif_i2sctl *i2sctl = drvdata->i2sctl;
+ÂÂÂ unsigned int id = dai->driver->id;
ÂÂÂÂÂ int ret;
-ÂÂÂ unsigned int val, mask;
 Â if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
-ÂÂÂÂÂÂÂ val = LPAIF_I2SCTL_SPKEN_ENABLE;
-ÂÂÂÂÂÂÂ mask = LPAIF_I2SCTL_SPKEN_MASK;
- } else {
-ÂÂÂÂÂÂÂ val = LPAIF_I2SCTL_MICEN_ENABLE;
-ÂÂÂÂÂÂÂ mask = LPAIF_I2SCTL_MICEN_MASK;
+ÂÂÂÂÂÂÂ ret = regmap_fields_write(i2sctl->spken, id,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ LPAIF_I2SCTL_SPKEN_ENABLE);
+ÂÂÂ } else {
+ÂÂÂÂÂÂÂ ret = regmap_fields_write(i2sctl->micen, id,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ LPAIF_I2SCTL_MICEN_ENABLE);
ÂÂÂÂÂ }
 - ret = regmap_update_bits(drvdata->lpaif_map,
-ÂÂÂÂÂÂÂÂÂÂÂ LPAIF_I2SCTL_REG(drvdata->variant, dai->driver->id),
-ÂÂÂÂÂÂÂÂÂÂÂ mask, val);
ÂÂÂÂÂ if (ret)
-ÂÂÂÂÂÂÂ dev_err(dai->dev, "error writing to i2sctl reg: %d\n", ret);
+ÂÂÂÂÂÂÂ dev_err(dai->dev, "error writing to i2sctl enable: %d\n", ret);
 Â return ret;
 }
...
@@ -304,7 +326,6 @@ const struct snd_soc_dai_ops asoc_qcom_lpass_cpu_dai_ops = {
ÂÂÂÂÂ .startupÂÂÂ = lpass_cpu_daiops_startup,
ÂÂÂÂÂ .shutdownÂÂÂ = lpass_cpu_daiops_shutdown,
ÂÂÂÂÂ .hw_paramsÂÂÂ = lpass_cpu_daiops_hw_params,
-ÂÂÂ .hw_freeÂÂÂ = lpass_cpu_daiops_hw_free,
ÂÂÂÂÂ .prepareÂÂÂ = lpass_cpu_daiops_prepare,
ÂÂÂÂÂ .triggerÂÂÂ = lpass_cpu_daiops_trigger,
 };
@@ -599,6 +620,18 @@ int asoc_qcom_lpass_cpu_platform_probe(struct platform_device *pdev)
ÂÂÂÂÂÂÂÂÂ }
ÂÂÂÂÂ }
 + /* Allocation for i2sctl regmap fields */
+ÂÂÂ drvdata->i2sctl = devm_kzalloc(&pdev->dev, sizeof(struct lpaif_i2sctl),
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ GFP_KERNEL);
+
+ÂÂÂ /* Initialize bitfields for dai I2SCTL register */
+ÂÂÂ ret = lpass_cpu_init_i2sctl_bitfields(dev, drvdata->i2sctl,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ drvdata->lpaif_map);
+ÂÂÂ if (ret) {
+ÂÂÂÂÂÂÂ dev_err(dev, "error init i2sctl field: %d\n", ret);
+ÂÂÂÂÂÂÂ return ret;
+ÂÂÂ }
+
ÂÂÂÂÂ ret = devm_snd_soc_register_component(dev,
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ &lpass_cpu_comp_driver,
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ variant->dai_driver,

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?


ÂÂÂÂÂ 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;
-ÂÂÂ struct lpass_pcm_data *data;
+ÂÂÂ struct lpass_pcm_data *data = runtime->private_data;
 - data = runtime->private_data;
ÂÂÂÂÂ drvdata->substream[data->dma_ch] = NULL;
ÂÂÂÂÂ if (v->free_dma_channel)
ÂÂÂÂÂÂÂÂÂ v->free_dma_channel(drvdata, data->dma_ch);
 + kfree(data);
ÂÂÂÂÂ return 0;
 }
  Â return devm_snd_soc_register_component(&pdev->dev,
ÂÂÂÂÂÂÂÂÂÂÂÂÂ &lpass_component_driver, NULL, 0);
diff --git a/sound/soc/qcom/lpass.h b/sound/soc/qcom/lpass.h
index 450020e..4294ec2 100644
--- a/sound/soc/qcom/lpass.h
+++ b/sound/soc/qcom/lpass.h
@@ -17,6 +17,28 @@
 #define LPASS_MAX_MI2S_PORTS (8)
 #define LPASS_MAX_DMA_CHANNELS (8)
...

 /* Both the CPU DAI and platform drivers will access this data */
 struct lpass_data {
 @@ -55,6 +77,10 @@ struct lpass_data {
ÂÂÂÂÂ struct clk_bulk_data *clks;
ÂÂÂÂÂ int num_clks;
 + /* Regmap fields of I2SCTL & DMACTL registers bitfields */
+ÂÂÂ struct lpaif_i2sctl *i2sctl;
+ÂÂÂ struct lpaif_dmactl *rd_dmactl;
+ÂÂÂ struct lpaif_dmactl *wr_dmactl;
 };
  /* Vairant data per each SOC */
@@ -72,6 +98,33 @@ struct lpass_variant {
ÂÂÂÂÂ u32ÂÂÂ wrdma_reg_stride;
ÂÂÂÂÂ u32ÂÂÂ wrdma_channels;

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.


+ÂÂÂ /* I2SCTL Register fields */
+ÂÂÂ struct reg_field loopback;
+ÂÂÂ struct reg_field spken;
+ÂÂÂ struct reg_field spkmode;
+ÂÂÂ struct reg_field spkmono;
+ÂÂÂ struct reg_field micen;
+ÂÂÂ struct reg_field micmode;
+ÂÂÂ struct reg_field micmono;
+ÂÂÂ struct reg_field wssrc;
+ÂÂÂ struct reg_field bitwidth;
+
+ÂÂÂ /* RD_DMA Register fields */
+ÂÂÂ struct reg_field rdma_bursten;
+ÂÂÂ struct reg_field rdma_wpscnt;
+ÂÂÂ struct reg_field rdma_intf;
+ÂÂÂ struct reg_field rdma_fifowm;
+ÂÂÂ struct reg_field rdma_enable;
+ÂÂÂ struct reg_field rdma_dyncclk;
+
+ÂÂÂ /* RD_DMA Register fields */
+ÂÂÂ struct reg_field wrdma_bursten;
+ÂÂÂ struct reg_field wrdma_wpscnt;
+ÂÂÂ struct reg_field wrdma_intf;
+ÂÂÂ struct reg_field wrdma_fifowm;
+ÂÂÂ struct reg_field wrdma_enable;
+ÂÂÂ struct reg_field wrdma_dyncclk;
+
ÂÂÂÂÂ /**
ÂÂÂÂÂÂ * on SOCs like APQ8016 the channel control bits start
ÂÂÂÂÂÂ * at different offset to ipq806x

Thanks,

Rohit

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