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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





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



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux