* [PATCH v2] ASoC: qcom: lpass-cpu: Fix pop noise during audio capture begin
@ 2021-05-20 14:24 Srinivasa Rao Mandadapu
2021-05-23 22:54 ` Stephen Boyd
0 siblings, 1 reply; 3+ messages in thread
From: Srinivasa Rao Mandadapu @ 2021-05-20 14:24 UTC (permalink / raw)
To: agross, bjorn.andersson, lgirdwood, broonie, robh+dt, plai,
bgoswami, perex, tiwai, srinivas.kandagatla, rohitkr,
linux-arm-msm, alsa-devel, devicetree, linux-kernel, swboyd,
judyhsiao
Cc: Srinivasa Rao Mandadapu
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 <judyhsiao@chromium.org>
Signed-off-by: Srinivasa Rao Mandadapu <srivasam@codeaurora.org>
---
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(-)
diff --git a/sound/soc/qcom/lpass-cpu.c b/sound/soc/qcom/lpass-cpu.c
index c62d2612e8f5..c5bb3f16d25f 100644
--- a/sound/soc/qcom/lpass-cpu.c
+++ b/sound/soc/qcom/lpass-cpu.c
@@ -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);
--
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.,
is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH v2] ASoC: qcom: lpass-cpu: Fix pop noise during audio capture begin
2021-05-20 14:24 [PATCH v2] ASoC: qcom: lpass-cpu: Fix pop noise during audio capture begin Srinivasa Rao Mandadapu
@ 2021-05-23 22:54 ` Stephen Boyd
0 siblings, 0 replies; 3+ messages in thread
From: Stephen Boyd @ 2021-05-23 22:54 UTC (permalink / raw)
To: Srinivasa Rao Mandadapu, agross, alsa-devel, bgoswami,
bjorn.andersson, broonie, devicetree, judyhsiao, lgirdwood,
linux-arm-msm, linux-kernel, perex, plai, robh+dt, rohitkr,
srinivas.kandagatla, tiwai
Quoting Srinivasa Rao Mandadapu (2021-05-20 07:24:32)
> 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 <judyhsiao@chromium.org>
> Signed-off-by: Srinivasa Rao Mandadapu <srivasam@codeaurora.org>
The SoB chain is weird. If Judy is first then Judy should show up with a
From: Judy Hsiao <judyhsiao@chromium.org>
initially. Otherwise, it's a Co-developed-by: tag that should be after
the SoB line.
> ---
> 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(-)
>
> diff --git a/sound/soc/qcom/lpass-cpu.c b/sound/soc/qcom/lpass-cpu.c
> index c62d2612e8f5..c5bb3f16d25f 100644
> --- a/sound/soc/qcom/lpass-cpu.c
> +++ b/sound/soc/qcom/lpass-cpu.c
> @@ -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()?
> + 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:
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?
> 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?
> + /* 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?
> +
> + 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;
> +}
> +
> +
Nitpick: Why two newlines?
> 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,
> };
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH v2] ASoC: qcom: lpass-cpu: Fix pop noise during audio capture begin
@ 2021-05-23 22:54 ` Stephen Boyd
0 siblings, 0 replies; 3+ messages in thread
From: Stephen Boyd @ 2021-05-23 22:54 UTC (permalink / raw)
To: Srinivasa Rao Mandadapu, agross, alsa-devel, bgoswami,
bjorn.andersson, broonie, devicetree, judyhsiao, lgirdwood,
linux-arm-msm, linux-kernel, perex, plai, robh+dt, rohitkr,
srinivas.kandagatla, tiwai
Quoting Srinivasa Rao Mandadapu (2021-05-20 07:24:32)
> 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 <judyhsiao@chromium.org>
> Signed-off-by: Srinivasa Rao Mandadapu <srivasam@codeaurora.org>
The SoB chain is weird. If Judy is first then Judy should show up with a
From: Judy Hsiao <judyhsiao@chromium.org>
initially. Otherwise, it's a Co-developed-by: tag that should be after
the SoB line.
> ---
> 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(-)
>
> diff --git a/sound/soc/qcom/lpass-cpu.c b/sound/soc/qcom/lpass-cpu.c
> index c62d2612e8f5..c5bb3f16d25f 100644
> --- a/sound/soc/qcom/lpass-cpu.c
> +++ b/sound/soc/qcom/lpass-cpu.c
> @@ -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()?
> + 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:
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?
> 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?
> + /* 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?
> +
> + 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;
> +}
> +
> +
Nitpick: Why two newlines?
> 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,
> };
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-05-23 22:55 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-20 14:24 [PATCH v2] ASoC: qcom: lpass-cpu: Fix pop noise during audio capture begin Srinivasa Rao Mandadapu
2021-05-23 22:54 ` Stephen Boyd
2021-05-23 22:54 ` Stephen Boyd
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.