Re: [PATCH v2] ASoC: qcom: lpass-cpu: Fix pop noise during audio capture begin

[Stephen Boyd]

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@xxxxxxxxxxxx>
> Signed-off-by: Srinivasa Rao Mandadapu <srivasam@xxxxxxxxxxxxxx>

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

From: Judy Hsiao <judyhsiao@xxxxxxxxxxxx>

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,
>  };