Re: [PATCH V4] ASoC: Intel: boards: Use FS as nau8825 sysclk in nau88125_* machine

[Radosław Biernacki]

Sorry, V4 got one stray change for SSP0 ops.
Need to send V5.

czw., 10 wrz 2020 o 17:40 Radoslaw Biernacki <rad@xxxxxxxxxxxx> napisał(a):
>
> Since 256xFS clocks cannot be generated by SKL, the NAU8825 is
> configured to re-generate its system clock from the BCLK using the
> FLL. The link is configured to use a 48kHz frame rate, and 24 bits in
> 25-bit slot. The SSP configuration is extracted from NHLT settings and
> not dynamically changed. Listening tests and measurements do not show
> any distortion or issues
>
> Signed-off-by: John Hsu <KCHSU0@xxxxxxxxxxx>
> Signed-off-by: Yong Zhi <yong.zhi@xxxxxxxxx>
> Signed-off-by: Mac Chiang <mac.chiang@xxxxxxxxx>
> Signed-off-by: Ben Zhang <benzh@xxxxxxxxxxxx>
> Signed-off-by: Radoslaw Biernacki <rad@xxxxxxxxxxxx>
> ---
>
> Notes:
>     v1 -> v2:
>     - adding same changes to skl_nau88l25_max98357a.c
>     v2 -> v3:
>     - removing msleep() in SNDRV_PCM_TRIGGER_RESUME as it unnecessarily increase
>       playback/capture latency while actually FLL does not require it.
>     - simplifing commit message
>     v3 -> v4:
>     - simplifing the PM resume callback code for setting the FLL
>     - adding comment for the stream START/RESUME sequence which prevent audio pops
>     - fixing mising var initialization in platform_clock_control()
>
>  .../soc/intel/boards/skl_nau88l25_max98357a.c | 63 ++++++++++++------
>  sound/soc/intel/boards/skl_nau88l25_ssm4567.c | 65 +++++++++++++------
>  2 files changed, 86 insertions(+), 42 deletions(-)
>
> diff --git a/sound/soc/intel/boards/skl_nau88l25_max98357a.c b/sound/soc/intel/boards/skl_nau88l25_max98357a.c
> index d7b8154c43a4..2f0abbd2dd8d 100644
> --- a/sound/soc/intel/boards/skl_nau88l25_max98357a.c
> +++ b/sound/soc/intel/boards/skl_nau88l25_max98357a.c
> @@ -8,6 +8,7 @@
>
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
> +#include <linux/delay.h>
>  #include <sound/core.h>
>  #include <sound/jack.h>
>  #include <sound/pcm.h>
> @@ -47,12 +48,12 @@ enum {
>  };
>
>  static int platform_clock_control(struct snd_soc_dapm_widget *w,
> -       struct snd_kcontrol *k, int  event)
> +       struct snd_kcontrol *k, int event)
>  {
>         struct snd_soc_dapm_context *dapm = w->dapm;
>         struct snd_soc_card *card = dapm->card;
>         struct snd_soc_dai *codec_dai;
> -       int ret;
> +       int ret = 0;
>
>         codec_dai = snd_soc_card_get_codec_dai(card, SKL_NUVOTON_CODEC_DAI);
>         if (!codec_dai) {
> @@ -60,14 +61,7 @@ static int platform_clock_control(struct snd_soc_dapm_widget *w,
>                 return -EIO;
>         }
>
> -       if (SND_SOC_DAPM_EVENT_ON(event)) {
> -               ret = snd_soc_dai_set_sysclk(codec_dai,
> -                               NAU8825_CLK_MCLK, 24000000, SND_SOC_CLOCK_IN);
> -               if (ret < 0) {
> -                       dev_err(card->dev, "set sysclk err = %d\n", ret);
> -                       return -EIO;
> -               }
> -       } else {
> +       if (!SND_SOC_DAPM_EVENT_ON(event)) {
>                 ret = snd_soc_dai_set_sysclk(codec_dai,
>                                 NAU8825_CLK_INTERNAL, 0, SND_SOC_CLOCK_IN);
>                 if (ret < 0) {
> @@ -292,24 +286,51 @@ static const struct snd_soc_ops skylake_nau8825_fe_ops = {
>         .startup = skl_fe_startup,
>  };
>
> -static int skylake_nau8825_hw_params(struct snd_pcm_substream *substream,
> -       struct snd_pcm_hw_params *params)
> +static int skylake_nau8825_trigger(struct snd_pcm_substream *substream, int cmd)
>  {
>         struct snd_soc_pcm_runtime *rtd = substream->private_data;
> +       struct snd_pcm_runtime *runtime = substream->runtime;
>         struct snd_soc_dai *codec_dai = asoc_rtd_to_codec(rtd, 0);
> -       int ret;
> -
> -       ret = snd_soc_dai_set_sysclk(codec_dai,
> -                       NAU8825_CLK_MCLK, 24000000, SND_SOC_CLOCK_IN);
> -
> -       if (ret < 0)
> -               dev_err(rtd->dev, "snd_soc_dai_set_sysclk err = %d\n", ret);
> +       int ret = 0;
> +
> +       switch (cmd) {
> +       case SNDRV_PCM_TRIGGER_START:
> +               /* Since 256xFS clocks cannot be generated by SKL, the NAU8825
> +                * is configured to re-generate its system clock from the BCLK
> +                * using the FLL.
> +                * We must switch system clock (FLL to use BCLK) here as it is
> +                * not given eariler by FW (like in hw_param). We let nau8825 to
> +                * use internal VCO clock till now which reduces the audiable
> +                * pop's. */
> +
> +               /* fall through */
> +
> +       case SNDRV_PCM_TRIGGER_RESUME:
> +               /* Once device resumes, the system will only enable power
> +                * sequence for playback without doing hardware parameter, audio
> +                * format, and PLL configure. In the mean time, the jack
> +                * detecion sequence has changed PLL parameters and switched to
> +                * internal clock. Thus, the playback signal distorted without
> +                * correct PLL parameters. Therefore we need to configure PLL
> +                * again */
> +               ret = snd_soc_dai_set_sysclk(codec_dai, NAU8825_CLK_FLL_FS, 0,
> +                                            SND_SOC_CLOCK_IN);
> +               if (ret < 0) {
> +                       dev_err(codec_dai->dev, "can't set FS clock %d\n", ret);
> +                       break;
> +               }
> +               ret = snd_soc_dai_set_pll(codec_dai, 0, 0, runtime->rate,
> +                                         runtime->rate * 256);
> +               if (ret < 0)
> +                       dev_err(codec_dai->dev, "can't set FLL: %d\n", ret);
> +               break;
> +       }
>
>         return ret;
>  }
>
> -static const struct snd_soc_ops skylake_nau8825_ops = {
> -       .hw_params = skylake_nau8825_hw_params,
> +static struct snd_soc_ops skylake_nau8825_ops = {
> +       .trigger = skylake_nau8825_trigger,
>  };
>
>  static int skylake_dmic_fixup(struct snd_soc_pcm_runtime *rtd,
> diff --git a/sound/soc/intel/boards/skl_nau88l25_ssm4567.c b/sound/soc/intel/boards/skl_nau88l25_ssm4567.c
> index 4b317bcf6ea0..d076f19f9b78 100644
> --- a/sound/soc/intel/boards/skl_nau88l25_ssm4567.c
> +++ b/sound/soc/intel/boards/skl_nau88l25_ssm4567.c
> @@ -12,6 +12,7 @@
>
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
> +#include <linux/delay.h>
>  #include <sound/core.h>
>  #include <sound/pcm.h>
>  #include <sound/soc.h>
> @@ -57,12 +58,12 @@ static const struct snd_kcontrol_new skylake_controls[] = {
>  };
>
>  static int platform_clock_control(struct snd_soc_dapm_widget *w,
> -               struct snd_kcontrol *k, int  event)
> +               struct snd_kcontrol *k, int event)
>  {
>         struct snd_soc_dapm_context *dapm = w->dapm;
>         struct snd_soc_card *card = dapm->card;
>         struct snd_soc_dai *codec_dai;
> -       int ret;
> +       int ret = 0;
>
>         codec_dai = snd_soc_card_get_codec_dai(card, SKL_NUVOTON_CODEC_DAI);
>         if (!codec_dai) {
> @@ -70,14 +71,7 @@ static int platform_clock_control(struct snd_soc_dapm_widget *w,
>                 return -EIO;
>         }
>
> -       if (SND_SOC_DAPM_EVENT_ON(event)) {
> -               ret = snd_soc_dai_set_sysclk(codec_dai,
> -                               NAU8825_CLK_MCLK, 24000000, SND_SOC_CLOCK_IN);
> -               if (ret < 0) {
> -                       dev_err(card->dev, "set sysclk err = %d\n", ret);
> -                       return -EIO;
> -               }
> -       } else {
> +       if (!SND_SOC_DAPM_EVENT_ON(event)) {
>                 ret = snd_soc_dai_set_sysclk(codec_dai,
>                                 NAU8825_CLK_INTERNAL, 0, SND_SOC_CLOCK_IN);
>                 if (ret < 0) {
> @@ -85,6 +79,7 @@ static int platform_clock_control(struct snd_soc_dapm_widget *w,
>                         return -EIO;
>                 }
>         }
> +
>         return ret;
>  }
>
> @@ -344,24 +339,51 @@ static int skylake_dmic_fixup(struct snd_soc_pcm_runtime *rtd,
>         return 0;
>  }
>
> -static int skylake_nau8825_hw_params(struct snd_pcm_substream *substream,
> -       struct snd_pcm_hw_params *params)
> +static int skylake_nau8825_trigger(struct snd_pcm_substream *substream, int cmd)
>  {
>         struct snd_soc_pcm_runtime *rtd = substream->private_data;
> +       struct snd_pcm_runtime *runtime = substream->runtime;
>         struct snd_soc_dai *codec_dai = asoc_rtd_to_codec(rtd, 0);
> -       int ret;
> -
> -       ret = snd_soc_dai_set_sysclk(codec_dai,
> -                       NAU8825_CLK_MCLK, 24000000, SND_SOC_CLOCK_IN);
> -
> -       if (ret < 0)
> -               dev_err(rtd->dev, "snd_soc_dai_set_sysclk err = %d\n", ret);
> +       int ret = 0;
> +
> +       switch (cmd) {
> +       case SNDRV_PCM_TRIGGER_START:
> +               /* Since 256xFS clocks cannot be generated by SKL, the NAU8825
> +                * is configured to re-generate its system clock from the BCLK
> +                * using the FLL.
> +                * We must switch system clock (FLL to use BCLK) here as it is
> +                * not given eariler by FW (like in hw_param). We let nau8825 to
> +                * use internal VCO clock till now which reduces the audiable
> +                * pop's. */
> +
> +               /* fall through */
> +
> +       case SNDRV_PCM_TRIGGER_RESUME:
> +               /* Once device resumes, the system will only enable power
> +                * sequence for playback without doing hardware parameter, audio
> +                * format, and PLL configure. In the mean time, the jack
> +                * detecion sequence has changed PLL parameters and switched to
> +                * internal clock. Thus, the playback signal distorted without
> +                * correct PLL parameters. Therefore we need to configure PLL
> +                * again */
> +               ret = snd_soc_dai_set_sysclk(codec_dai, NAU8825_CLK_FLL_FS, 0,
> +                                            SND_SOC_CLOCK_IN);
> +               if (ret < 0) {
> +                       dev_err(codec_dai->dev, "can't set FS clock %d\n", ret);
> +                       break;
> +               }
> +               ret = snd_soc_dai_set_pll(codec_dai, 0, 0, runtime->rate,
> +                                         runtime->rate * 256);
> +               if (ret < 0)
> +                       dev_err(codec_dai->dev, "can't set FLL: %d\n", ret);
> +               break;
> +       }
>
>         return ret;
>  }
>
> -static const struct snd_soc_ops skylake_nau8825_ops = {
> -       .hw_params = skylake_nau8825_hw_params,
> +static struct snd_soc_ops skylake_nau8825_ops = {
> +       .trigger = skylake_nau8825_trigger,
>  };
>
>  static const unsigned int channels_dmic[] = {
> @@ -582,6 +604,7 @@ static struct snd_soc_dai_link skylake_dais[] = {
>                 .init = skylake_ssm4567_codec_init,
>                 .ignore_pmdown_time = 1,
>                 .be_hw_params_fixup = skylake_ssp_fixup,
> +               .ops = &skylake_nau8825_ops,
>                 .dpcm_playback = 1,
>                 .dpcm_capture = 1,
>                 SND_SOC_DAILINK_REG(ssp0_pin, ssp0_codec, platform),
> --
> 2.17.1
>