Skip to contentSkip to conversation

ASoC: qcom: lpass-cpu: Fix pop noise during audio capture begin

PATCH Superseded Srinivasa Rao Mandadapu 2 participants 2 messages May 20, 2021 – May 23, 2021

Subsystems (4)
Sound Arm Qualcomm mailing list ASoC Qualcomm audio (ASoC) drivers

v2 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Srinivasa Rao Mandadapu

author
#0 lore raw

PATCH v2 1/1 ASoC: qcom: lpass-cpu: Fix pop noise during audio capture begin

This patch fixes PoP noise of around 15ms observed during audio capture begin. Enables BCLK and LRCLK in snd_soc_dai_ops prepare call for introducing some delay before capture start and clock enable.

Signed-off-by Judy Hsiao <[email protected]>
Signed-off-by Srinivasa Rao Mandadapu <[email protected]>

---
Changes Since V1:
-- Enableed BCLK and LRCLK in dai ops prepare API instead of startup API -- Added comments

 sound/soc/qcom/lpass-cpu.c | 48 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 46 insertions(+), 2 deletions(-)
sound/soc/qcom/lpass-cpu.c +46 -2
index c62d2612e8f5..c5bb3f16d25f 100644
@@ -93,9 +93,18 @@ static void lpass_cpu_daiops_shutdown(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;
 
 	clk_disable_unprepare(drvdata->mi2s_osr_clk[dai->driver->id]);
-	clk_unprepare(drvdata->mi2s_bit_clk[dai->driver->id]);
+	/* To ensure BCLK/LRCLK disabled even in device node validation.
+	  Will not impact if disabled in trigger suspend */
+	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
+		regmap_fields_write(i2sctl->spken, id, LPAIF_I2SCTL_SPKEN_DISABLE);
+	else
+		regmap_fields_write(i2sctl->micen, id, LPAIF_I2SCTL_MICEN_DISABLE);
+
+	clk_disable_unprepare(drvdata->mi2s_bit_clk[dai->driver->id]);
 }
 
 static int lpass_cpu_daiops_hw_params(struct snd_pcm_substream *substream,

@@ -275,6 +284,8 @@ static int lpass_cpu_daiops_trigger(struct snd_pcm_substream *substream,
 	case SNDRV_PCM_TRIGGER_START:
 	case SNDRV_PCM_TRIGGER_RESUME:
 	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
+		/* To ensure lpass BCLK/LRCLK is enabled during
+	   		device resume. Will not impact if enabled in prepare */
 		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
 			ret = regmap_fields_write(i2sctl->spken, id,
 						 LPAIF_I2SCTL_SPKEN_ENABLE);

@@ -296,6 +307,8 @@ static int lpass_cpu_daiops_trigger(struct snd_pcm_substream *substream,
 	case SNDRV_PCM_TRIGGER_STOP:
 	case SNDRV_PCM_TRIGGER_SUSPEND:
 	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
+		/* To ensure lpass BCLK/LRCLK is disabled during
+	   		device suspend */
 		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
 			ret = regmap_fields_write(i2sctl->spken, id,
 						 LPAIF_I2SCTL_SPKEN_DISABLE);

@@ -308,19 +321,50 @@ static int lpass_cpu_daiops_trigger(struct snd_pcm_substream *substream,
 				ret);
 
 		clk_disable(drvdata->mi2s_bit_clk[dai->driver->id]);
-
+		break;
+	default:
 		break;
 	}
 
 	return ret;
 }
 
+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 = -EINVAL;
+	/* To ensure lpass BCLK/LRCLK is enabled bit before
+	   playback/capture data flow starts */
+	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
+		ret = regmap_fields_write(i2sctl->spken, id, LPAIF_I2SCTL_SPKEN_ENABLE);
+	else
+		ret = regmap_fields_write(i2sctl->micen, id, LPAIF_I2SCTL_MICEN_ENABLE);
+
+	if (ret)
+		dev_err(dai->dev, "error writing to i2sctl reg: %d\n",
+	       ret);
+
+	ret = clk_enable(drvdata->mi2s_bit_clk[id]);
+
+	if (ret) {
+		dev_err(dai->dev, "error in enabling mi2s bit clk: %d\n", ret);
+		clk_disable(drvdata->mi2s_osr_clk[id]);
+		return ret;
+	}
+	return 0;
+}
+
+
 const struct snd_soc_dai_ops asoc_qcom_lpass_cpu_dai_ops = {
 	.set_sysclk	= lpass_cpu_daiops_set_sysclk,
 	.startup	= lpass_cpu_daiops_startup,
 	.shutdown	= lpass_cpu_daiops_shutdown,
 	.hw_params	= lpass_cpu_daiops_hw_params,
 	.trigger	= lpass_cpu_daiops_trigger,
+	.prepare	= lpass_cpu_daiops_prepare,
 };
 EXPORT_SYMBOL_GPL(asoc_qcom_lpass_cpu_dai_ops);
Signature · 2 lines
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.,
is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.
#1 lore raw

Quoting Srinivasa Rao Mandadapu (2021-05-20 07:24:32)

Quoted 6 lines

This patch fixes PoP noise of around 15ms observed during audio capture begin. Enables BCLK and LRCLK in snd_soc_dai_ops prepare call for introducing some delay before capture start and clock enable.

Signed-off-by Judy Hsiao <[email protected]>
Signed-off-by Srinivasa Rao Mandadapu <[email protected]>

The SoB chain is weird. If Judy is first then Judy should show up with a

From: Judy Hsiao <[email protected]>

initially. Otherwise, it's a Co-developed-by: tag that should be after the SoB line.

Quoted 13 lines

---
Changes Since V1:
-- Enableed BCLK and LRCLK in dai ops prepare API instead of startup API -- Added comments

 sound/soc/qcom/lpass-cpu.c | 48 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 46 insertions(+), 2 deletions(-)
sound/soc/qcom/lpass-cpu.c +0 -0
index c62d2612e8f5..c5bb3f16d25f 100644

sound/soc/qcom/lpass-cpu.c static void lpass_cpu_daiops_shutdown(struct snd_pcm_substream *substream, · line 101

Quoted 10 lines
sound/soc/qcom/lpass-cpu.c +4 -1
@@ -93,9 +93,18 @@ static void lpass_cpu_daiops_shutdown(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;
 
        clk_disable_unprepare(drvdata->mi2s_osr_clk[dai->driver->id]);
-       clk_unprepare(drvdata->mi2s_bit_clk[dai->driver->id]);
+       /* To ensure BCLK/LRCLK disabled even in device node validation.
+         Will not impact if disabled in trigger suspend */

/*
* Can you use proper kernel doc multi-line notation please? */

And also, can we point to the function that is not impacted by "trigger suspend"? Is that lpass_cpu_daiops_trigger()?

Quoted 28 lines
sound/soc/qcom/lpass-cpu.c +10 -0
+       if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
+               regmap_fields_write(i2sctl->spken, id, LPAIF_I2SCTL_SPKEN_DISABLE);
+       else
+               regmap_fields_write(i2sctl->micen, id, LPAIF_I2SCTL_MICEN_DISABLE);
+
+       clk_disable_unprepare(drvdata->mi2s_bit_clk[dai->driver->id]);
 }
 
 static int lpass_cpu_daiops_hw_params(struct snd_pcm_substream *substream,

@@ -275,6 +284,8 @@ static int lpass_cpu_daiops_trigger(struct snd_pcm_substream *substream,
        case SNDRV_PCM_TRIGGER_START:
        case SNDRV_PCM_TRIGGER_RESUME:
        case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
+               /* To ensure lpass BCLK/LRCLK is enabled during
+                       device resume. Will not impact if enabled in prepare */
                if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
                        ret = regmap_fields_write(i2sctl->spken, id,
                                                 LPAIF_I2SCTL_SPKEN_ENABLE);

@@ -296,6 +307,8 @@ static int lpass_cpu_daiops_trigger(struct snd_pcm_substream *substream,
        case SNDRV_PCM_TRIGGER_STOP:
        case SNDRV_PCM_TRIGGER_SUSPEND:
        case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
+               /* To ensure lpass BCLK/LRCLK is disabled during
+                       device suspend */
                if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
                        ret = regmap_fields_write(i2sctl->spken, id,
                                                 LPAIF_I2SCTL_SPKEN_DISABLE);

sound/soc/qcom/lpass-cpu.c static int lpass_cpu_daiops_trigger(struct snd_pcm_substream *substream, · line 325

Quoted 6 lines
sound/soc/qcom/lpass-cpu.c +2 -1
@@ -308,19 +321,50 @@ static int lpass_cpu_daiops_trigger(struct snd_pcm_substream *substream,
                                ret);
 
                clk_disable(drvdata->mi2s_bit_clk[dai->driver->id]);
-
+               break;
+       default:

It confuses me that we're adding 'default' now. Why? Is some define not handled already? Why not use that define instead of 'default' so it is explicit what is being handled? How is it even related to this patch?

sound/soc/qcom/lpass-cpu.c static int lpass_cpu_daiops_trigger(struct snd_pcm_substream *substream, · line 338

Quoted 13 lines
sound/soc/qcom/lpass-cpu.c +7 -0
                break;
        }
 
        return ret;
 }
 
+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 = -EINVAL;

ret will be assigned in the else though? Why assign it and then reassign it?

sound/soc/qcom/lpass-cpu.c static int lpass_cpu_daiops_trigger(struct snd_pcm_substream *substream, · line 348

Quoted 10 lines
sound/soc/qcom/lpass-cpu.c +10 -0
+       /* To ensure lpass BCLK/LRCLK is enabled bit before
+          playback/capture data flow starts */
+       if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
+               ret = regmap_fields_write(i2sctl->spken, id, LPAIF_I2SCTL_SPKEN_ENABLE);
+       else
+               ret = regmap_fields_write(i2sctl->micen, id, LPAIF_I2SCTL_MICEN_ENABLE);
+
+       if (ret)
+               dev_err(dai->dev, "error writing to i2sctl reg: %d\n",
+              ret);

Why do we keep trying here instead of returning an error?

sound/soc/qcom/lpass-cpu.c static int lpass_cpu_daiops_trigger(struct snd_pcm_substream *substream, · line 357

Quoted 9 lines
sound/soc/qcom/lpass-cpu.c +9 -0
+
+       ret = clk_enable(drvdata->mi2s_bit_clk[id]);
+
+       if (ret) {
+               dev_err(dai->dev, "error in enabling mi2s bit clk: %d\n", ret);
+               clk_disable(drvdata->mi2s_osr_clk[id]);
+               return ret;
+       }
+       return 0;

Could be

	return ret;

sound/soc/qcom/lpass-cpu.c static int lpass_cpu_daiops_trigger(struct snd_pcm_substream *substream, · line 360

Quoted 3 lines
sound/soc/qcom/lpass-cpu.c +3 -0
+}
+
+

Nitpick: Why two newlines?

Quoted 8 lines
sound/soc/qcom/lpass-cpu.c +1 -0
 const struct snd_soc_dai_ops asoc_qcom_lpass_cpu_dai_ops = {
        .set_sysclk     = lpass_cpu_daiops_set_sysclk,
        .startup        = lpass_cpu_daiops_startup,
        .shutdown       = lpass_cpu_daiops_shutdown,
        .hw_params      = lpass_cpu_daiops_hw_params,
        .trigger        = lpass_cpu_daiops_trigger,
+       .prepare        = lpass_cpu_daiops_prepare,
 };