All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] CHROMIUM: ASoC: Intel: boards: Use FS as nau8825 sysclk in nau88l25_max98357a machine
@ 2016-11-17  6:35 Grace Kao
  2016-11-18  5:28 ` Vinod Koul
  0 siblings, 1 reply; 5+ messages in thread
From: Grace Kao @ 2016-11-17  6:35 UTC (permalink / raw)
  To: alsa-devel; +Cc: Vinod.koul, Grace Kao

256FS sysclk gives the best audio quality per nau8825 datasheet.

Loud headphone pop happens if there is no sysclk during nau8825
playback power up sequence. Currently Skylake does not output MCLK/FS
when the back-end DAI op hw_param is called, so we cannot switch to
MCLK/FS in hw_param. This patch reduces pop by leting nau8825 keep
using its internal VCO clock during widget power up sequence, until
SNDRV_PCM_TRIGGER_START when MCLK/FS is available.

This is a temporary fix before Intel ADSP can output MCLK/FS early.

BUG=chrome-os-partner:50471
TEST=`emerge-sentry sys-kernel/chromeos-kernel-3_18`. Verified a local
build that could reduce the pop sound.

Change-Id: I4943fffa8eb1c874cc66b50f0897092bf48dd579
Signed-off-by: Grace Kao <grace.kao@intel.com>
Reviewed-on: https://chromium-review.googlesource.com/333180
Reviewed-by: Ben Zhang <benzh@chromium.org>

Series-to: alsa-devel@alsa-project.org
Series-cc: Vinod.koul@intel.com
---

diff --git a/sound/soc/intel/boards/skl_nau88l25_max98357a.c b/sound/soc/intel/boards/skl_nau88l25_max98357a.c
index 0f89602..6ad8f52 100644
--- a/sound/soc/intel/boards/skl_nau88l25_max98357a.c
+++ b/sound/soc/intel/boards/skl_nau88l25_max98357a.c
@@ -70,14 +70,7 @@
 		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) {
@@ -272,25 +265,7 @@
 	.startup = skl_fe_startup,
 };
 
-static int skylake_nau8825_hw_params(struct snd_pcm_substream *substream,
-	struct snd_pcm_hw_params *params)
-{
-	struct snd_soc_pcm_runtime *rtd = substream->private_data;
-	struct snd_soc_dai *codec_dai = rtd->codec_dai;
-	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);
-
-	return ret;
-}
-
-static struct snd_soc_ops skylake_nau8825_ops = {
-	.hw_params = skylake_nau8825_hw_params,
-};
 
 static int skylake_dmic_fixup(struct snd_soc_pcm_runtime *rtd,
 		struct snd_pcm_hw_params *params)
@@ -306,6 +281,33 @@
 	return 0;
 }
 
+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 = rtd->codec_dai;
+	int ret = 0;
+
+	switch (cmd) {
+	case SNDRV_PCM_TRIGGER_START:
+		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);
+		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 struct snd_soc_ops skylake_nau8825_ops = {
+	.trigger = skylake_nau8825_trigger,
+};
+
 static unsigned int channels_dmic[] = {
 	2, 4,
 };

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] CHROMIUM: ASoC: Intel: boards: Use FS as nau8825 sysclk in nau88l25_max98357a machine
  2016-11-17  6:35 [PATCH] CHROMIUM: ASoC: Intel: boards: Use FS as nau8825 sysclk in nau88l25_max98357a machine Grace Kao
@ 2016-11-18  5:28 ` Vinod Koul
  2016-11-18  9:48   ` Charles Keepax
  0 siblings, 1 reply; 5+ messages in thread
From: Vinod Koul @ 2016-11-18  5:28 UTC (permalink / raw)
  To: Grace Kao; +Cc: alsa-devel

On Thu, Nov 17, 2016 at 02:35:15PM +0800, Grace Kao wrote:
> 256FS sysclk gives the best audio quality per nau8825 datasheet.
> 
> Loud headphone pop happens if there is no sysclk during nau8825
> playback power up sequence. Currently Skylake does not output MCLK/FS
> when the back-end DAI op hw_param is called, so we cannot switch to
> MCLK/FS in hw_param. This patch reduces pop by leting nau8825 keep
> using its internal VCO clock during widget power up sequence, until
> SNDRV_PCM_TRIGGER_START when MCLK/FS is available.

Please CC maintainers Mark and Liam on ASoC patches

The tag CHROMIUM is not fit for upstream please remove

> 
> This is a temporary fix before Intel ADSP can output MCLK/FS early.
> 
> BUG=chrome-os-partner:50471
> TEST=`emerge-sentry sys-kernel/chromeos-kernel-3_18`. Verified a local
> build that could reduce the pop sound.

This is just noise for upstream, please remove

> 
> Change-Id: I4943fffa8eb1c874cc66b50f0897092bf48dd579

This as well

> Signed-off-by: Grace Kao <grace.kao@intel.com>
> Reviewed-on: https://chromium-review.googlesource.com/333180

And this

> Reviewed-by: Ben Zhang <benzh@chromium.org>
> 
> Series-to: alsa-devel@alsa-project.org
> Series-cc: Vinod.koul@intel.com

here too

> ---
> 
> diff --git a/sound/soc/intel/boards/skl_nau88l25_max98357a.c b/sound/soc/intel/boards/skl_nau88l25_max98357a.c
> index 0f89602..6ad8f52 100644
> --- a/sound/soc/intel/boards/skl_nau88l25_max98357a.c
> +++ b/sound/soc/intel/boards/skl_nau88l25_max98357a.c
> @@ -70,14 +70,7 @@
>  		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)) {

SND_SOC_DAPM_EVENT_OFF ??

> +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 = rtd->codec_dai;
> +	int ret = 0;
> +
> +	switch (cmd) {
> +	case SNDRV_PCM_TRIGGER_START:
> +		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);
> +		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;

starting clock on trigger doesnt sound right to me..

-- 
~Vinod

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] CHROMIUM: ASoC: Intel: boards: Use FS as nau8825 sysclk in nau88l25_max98357a machine
  2016-11-18  5:28 ` Vinod Koul
@ 2016-11-18  9:48   ` Charles Keepax
  2016-11-18 10:25     ` Vinod Koul
  0 siblings, 1 reply; 5+ messages in thread
From: Charles Keepax @ 2016-11-18  9:48 UTC (permalink / raw)
  To: Vinod Koul; +Cc: alsa-devel, Grace Kao

On Fri, Nov 18, 2016 at 10:58:46AM +0530, Vinod Koul wrote:
> On Thu, Nov 17, 2016 at 02:35:15PM +0800, Grace Kao wrote:
> > 256FS sysclk gives the best audio quality per nau8825 datasheet.
> > 
> > Loud headphone pop happens if there is no sysclk during nau8825
> > playback power up sequence. Currently Skylake does not output MCLK/FS
> > when the back-end DAI op hw_param is called, so we cannot switch to
> > MCLK/FS in hw_param. This patch reduces pop by leting nau8825 keep
> > using its internal VCO clock during widget power up sequence, until
> > SNDRV_PCM_TRIGGER_START when MCLK/FS is available.
<snip>
> 
> > +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 = rtd->codec_dai;
> > +	int ret = 0;
> > +
> > +	switch (cmd) {
> > +	case SNDRV_PCM_TRIGGER_START:
> > +		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);
> > +		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;
> 
> starting clock on trigger doesnt sound right to me..
> 

Me neither can't trigger be called from an atomic context? In
which case I would expect operations on an external CODEC to be
very likely to cause issues.

Thanks,
Charles

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] CHROMIUM: ASoC: Intel: boards: Use FS as nau8825 sysclk in nau88l25_max98357a machine
  2016-11-18  9:48   ` Charles Keepax
@ 2016-11-18 10:25     ` Vinod Koul
  0 siblings, 0 replies; 5+ messages in thread
From: Vinod Koul @ 2016-11-18 10:25 UTC (permalink / raw)
  To: Charles Keepax; +Cc: alsa-devel, Grace Kao

On Fri, Nov 18, 2016 at 09:48:10AM +0000, Charles Keepax wrote:
> > > +
> > > +	switch (cmd) {
> > > +	case SNDRV_PCM_TRIGGER_START:
> > > +		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);
> > > +		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;
> > 
> > starting clock on trigger doesnt sound right to me..
> > 
> 
> Me neither can't trigger be called from an atomic context? In
> which case I would expect operations on an external CODEC to be
> very likely to cause issues.

Not in this case :) We use non atomic triggers.. But that doesnt make this
correct :)

-- 
~Vinod

^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH] CHROMIUM: ASoC: Intel: boards: Use FS as nau8825 sysclk in nau88l25_max98357a machine
@ 2016-11-18  5:16 Grace Kao
  0 siblings, 0 replies; 5+ messages in thread
From: Grace Kao @ 2016-11-18  5:16 UTC (permalink / raw)
  To: alsa-devel; +Cc: Vinod.koul, Grace Kao

256FS sysclk gives the best audio quality per nau8825 datasheet.

Loud headphone pop happens if there is no sysclk during nau8825
playback power up sequence. Currently Skylake does not output MCLK/FS
when the back-end DAI op hw_param is called, so we cannot switch to
MCLK/FS in hw_param. This patch reduces pop by leting nau8825 keep
using its internal VCO clock during widget power up sequence, until
SNDRV_PCM_TRIGGER_START when MCLK/FS is available.

This is a temporary fix before Intel ADSP can output MCLK/FS early.

BUG=chrome-os-partner:50471
TEST=`emerge-sentry sys-kernel/chromeos-kernel-3_18`. Verified a local
build that could reduce the pop sound.

Change-Id: I4943fffa8eb1c874cc66b50f0897092bf48dd579
Signed-off-by: Grace Kao <grace.kao@intel.com>
Reviewed-on: https://chromium-review.googlesource.com/333180
Reviewed-by: Ben Zhang <benzh@chromium.org>

Series-to: alsa-devel@alsa-project.org
Series-cc: Vinod.koul@intel.com
---

diff --git a/sound/soc/intel/boards/skl_nau88l25_max98357a.c b/sound/soc/intel/boards/skl_nau88l25_max98357a.c
index 0f89602..6ad8f52 100644
--- a/sound/soc/intel/boards/skl_nau88l25_max98357a.c
+++ b/sound/soc/intel/boards/skl_nau88l25_max98357a.c
@@ -70,14 +70,7 @@
 		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) {
@@ -272,25 +265,7 @@
 	.startup = skl_fe_startup,
 };
 
-static int skylake_nau8825_hw_params(struct snd_pcm_substream *substream,
-	struct snd_pcm_hw_params *params)
-{
-	struct snd_soc_pcm_runtime *rtd = substream->private_data;
-	struct snd_soc_dai *codec_dai = rtd->codec_dai;
-	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);
-
-	return ret;
-}
-
-static struct snd_soc_ops skylake_nau8825_ops = {
-	.hw_params = skylake_nau8825_hw_params,
-};
 
 static int skylake_dmic_fixup(struct snd_soc_pcm_runtime *rtd,
 		struct snd_pcm_hw_params *params)
@@ -306,6 +281,33 @@
 	return 0;
 }
 
+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 = rtd->codec_dai;
+	int ret = 0;
+
+	switch (cmd) {
+	case SNDRV_PCM_TRIGGER_START:
+		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);
+		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 struct snd_soc_ops skylake_nau8825_ops = {
+	.trigger = skylake_nau8825_trigger,
+};
+
 static unsigned int channels_dmic[] = {
 	2, 4,
 };

^ permalink raw reply related	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2016-11-18 10:15 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-11-17  6:35 [PATCH] CHROMIUM: ASoC: Intel: boards: Use FS as nau8825 sysclk in nau88l25_max98357a machine Grace Kao
2016-11-18  5:28 ` Vinod Koul
2016-11-18  9:48   ` Charles Keepax
2016-11-18 10:25     ` Vinod Koul
2016-11-18  5:16 Grace Kao

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.