[PATCH v2 0/2] Enhance imx-wm8962 machine driver

[PATCH v2 0/2] Enhance imx-wm8962 machine driver

[Nicolin Chen]

Although there's no direct relationship between any of these two patches,
I recommend to apply them in order so as to circumvent merging conflicts.

Changlog:
v2:
 * Nothing changed for PATCH-1.
 * Added spinlock potection for boolean fll_locked in PATCH-2
 * Dropped PATCH-3, will try to revise it next time.

Nicolin Chen (2):
  ASoC: fsl: imx-wm8962: Don't update bias_level in machine driver
  ASoC: fsl: imx-wm8962: Grant hw_params/free() permission to control
    FLL

 sound/soc/fsl/imx-wm8962.c | 152 +++++++++++++++++++++++++++++----------------
 1 file changed, 99 insertions(+), 53 deletions(-)

-- 
1.8.4

[PATCH v2 1/2] ASoC: fsl: imx-wm8962: Don't update bias_level in machine driver

[Nicolin Chen]

If we update it here, the set_bias_level() of Codec driver won't be normally
called and we will then miss some essential procedures in set_bias_level() of
the Codec driver. Thus drop it.

Signed-off-by: Nicolin Chen <b42378@freescale.com>
---
 sound/soc/fsl/imx-wm8962.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/sound/soc/fsl/imx-wm8962.c b/sound/soc/fsl/imx-wm8962.c
index 61e4885..3fd76bc 100644
--- a/sound/soc/fsl/imx-wm8962.c
+++ b/sound/soc/fsl/imx-wm8962.c
@@ -130,8 +130,6 @@ static int imx_wm8962_set_bias_level(struct snd_soc_card *card,
 		break;
 	}
 
-	dapm->bias_level = level;
-
 	return 0;
 }
 
-- 
1.8.4

Re: [PATCH v2 1/2] ASoC: fsl: imx-wm8962: Don't update bias_level in machine driver

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 273 bytes --]

On Fri, Dec 06, 2013 at 11:38:28PM +0800, Nicolin Chen wrote:
> If we update it here, the set_bias_level() of Codec driver won't be normally
> called and we will then miss some essential procedures in set_bias_level() of
> the Codec driver. Thus drop it.

Applied, thanks.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH v2 2/2] ASoC: fsl: imx-wm8962: Grant hw_params/free() permission to control FLL

[Nicolin Chen]

Previously, we couldn't use hw_params() and hw_free() to open and close FLL
becuase there might be a race between two simmultaneous substreams so the
FLL configuration would be changed and accordingly mulfunction. Also it
wouldn't make sense for bypass path feature of WM8962. So we adopted DAPM
way to control it. However, if we want to playback a different sample rate
file, we need to wait for DAPM to change its bias_level and reconfigure FLL.

But after we introduced full symmetry protection in the soc-pcm, we don't
need to worry about the race any more. And the instance by using hw_params()
and hw_free() to control FLL will allow us to support flexible use cases,
'aplay -Dhw:0 44k16bit.wav 48k24bit.wav 32k16bit.wav' for example.

Thus this patch mainly adds FLL configuration code to hw_params/hw_free()
so as to enchance the sound card's capability. Meanwhile in order not to
break the bypass path feature, we make both set_bias_level() and hw_xxx()
ways coexist.

Signed-off-by: Nicolin Chen <b42378@freescale.com>
---
 sound/soc/fsl/imx-wm8962.c | 150 ++++++++++++++++++++++++++++++---------------
 1 file changed, 99 insertions(+), 51 deletions(-)

diff --git a/sound/soc/fsl/imx-wm8962.c b/sound/soc/fsl/imx-wm8962.c
index 3fd76bc..ce0578d 100644
--- a/sound/soc/fsl/imx-wm8962.c
+++ b/sound/soc/fsl/imx-wm8962.c
@@ -17,6 +17,7 @@
 #include <linux/of_platform.h>
 #include <linux/i2c.h>
 #include <linux/slab.h>
+#include <linux/spinlock.h>
 #include <linux/clk.h>
 #include <sound/soc.h>
 #include <sound/pcm_params.h>
@@ -35,6 +36,8 @@ struct imx_wm8962_data {
 	char platform_name[DAI_NAME_SIZE];
 	struct clk *codec_clk;
 	unsigned int clk_frequency;
+	spinlock_t fll_lock;
+	bool fll_locked;
 };
 
 struct imx_priv {
@@ -49,20 +52,103 @@ static const struct snd_soc_dapm_widget imx_wm8962_dapm_widgets[] = {
 	SND_SOC_DAPM_MIC("DMIC", NULL),
 };
 
-static int sample_rate = 44100;
-static snd_pcm_format_t sample_format = SNDRV_PCM_FORMAT_S16_LE;
+static int imx_wm8962_enable_fll(struct snd_soc_dai *codec_dai, u32 sample_rate,
+				snd_pcm_format_t sample_format)
+{
+	struct imx_priv *priv = &card_priv;
+	struct device *dev = &priv->pdev->dev;
+	struct imx_wm8962_data *data = platform_get_drvdata(priv->pdev);
+	unsigned long flags;
+	u32 freq, ret;
+
+	spin_lock_irqsave(&data->fll_lock, flags);
+	if (data->fll_locked) {
+		spin_unlock_irqrestore(&data->fll_lock, flags);
+		return 0;
+	}
+
+	data->fll_locked = true;
+	spin_unlock_irqrestore(&data->fll_lock, flags);
+
+	if (sample_format == SNDRV_PCM_FORMAT_S24_LE)
+		freq = sample_rate * 384;
+	else
+		freq = sample_rate * 256;
+
+	ret = snd_soc_dai_set_pll(codec_dai, WM8962_FLL, WM8962_FLL_MCLK,
+				  data->clk_frequency, freq);
+	if (ret) {
+		dev_err(dev, "failed to start FLL: %d\n", ret);
+		return ret;
+	}
+
+	ret = snd_soc_dai_set_sysclk(codec_dai, WM8962_SYSCLK_FLL,
+				     freq, SND_SOC_CLOCK_IN);
+	if (ret)
+		dev_err(dev, "failed to set SYSCLK: %d\n", ret);
+
+	return ret;
+}
+
+static int imx_wm8962_disable_fll(struct snd_soc_dai *codec_dai)
+{
+	struct imx_priv *priv = &card_priv;
+	struct device *dev = &priv->pdev->dev;
+	struct imx_wm8962_data *data = platform_get_drvdata(priv->pdev);
+	unsigned long flags;
+	int ret;
+
+	spin_lock_irqsave(&data->fll_lock, flags);
+	if (!data->fll_locked) {
+		spin_unlock_irqrestore(&data->fll_lock, flags);
+		return 0;
+	}
+
+	data->fll_locked = false;
+	spin_unlock_irqrestore(&data->fll_lock, flags);
+
+	/* Switch to MCLK as sysclk once so as to disable FLL */
+	ret = snd_soc_dai_set_sysclk(codec_dai, WM8962_SYSCLK_MCLK,
+				     0, SND_SOC_CLOCK_IN);
+	if (ret) {
+		dev_err(dev, "failed to switch away from FLL: %d\n", ret);
+		return ret;
+	}
+
+	/* Disable FLL so that we can reset its output freq later */
+	ret = snd_soc_dai_set_pll(codec_dai, WM8962_FLL,
+				  WM8962_FLL_MCLK, 0, 0);
+	if (ret)
+		dev_err(dev, "failed to stop FLL: %d\n", ret);
+
+	return ret;
+}
 
 static int imx_hifi_hw_params(struct snd_pcm_substream *substream,
 		struct snd_pcm_hw_params *params)
 {
-	sample_rate = params_rate(params);
-	sample_format = params_format(params);
+	struct snd_soc_pcm_runtime *rtd = substream->private_data;
+	struct snd_soc_dai *codec_dai = rtd->codec_dai;
 
-	return 0;
+	return imx_wm8962_enable_fll(codec_dai, params_rate(params),
+				     params_format(params));
+}
+
+static int imx_hifi_hw_free(struct snd_pcm_substream *substream)
+{
+	struct snd_soc_pcm_runtime *rtd = substream->private_data;
+	struct snd_soc_dai *codec_dai = rtd->codec_dai;
+
+	/* Don't diable FLL if still having multiple substreams running */
+	if (codec_dai->active != 1)
+		return 0;
+
+	return imx_wm8962_disable_fll(codec_dai);
 }
 
 static struct snd_soc_ops imx_hifi_ops = {
 	.hw_params = imx_hifi_hw_params,
+	.hw_free = imx_hifi_hw_free,
 };
 
 static int imx_wm8962_set_bias_level(struct snd_soc_card *card,
@@ -70,60 +156,20 @@ static int imx_wm8962_set_bias_level(struct snd_soc_card *card,
 					enum snd_soc_bias_level level)
 {
 	struct snd_soc_dai *codec_dai = card->rtd[0].codec_dai;
-	struct imx_priv *priv = &card_priv;
-	struct imx_wm8962_data *data = platform_get_drvdata(priv->pdev);
-	struct device *dev = &priv->pdev->dev;
-	unsigned int pll_out;
-	int ret;
 
 	if (dapm->dev != codec_dai->dev)
 		return 0;
 
 	switch (level) {
 	case SND_SOC_BIAS_PREPARE:
-		if (dapm->bias_level == SND_SOC_BIAS_STANDBY) {
-			if (sample_format == SNDRV_PCM_FORMAT_S24_LE)
-				pll_out = sample_rate * 384;
-			else
-				pll_out = sample_rate * 256;
-
-			ret = snd_soc_dai_set_pll(codec_dai, WM8962_FLL,
-					WM8962_FLL_MCLK, data->clk_frequency,
-					pll_out);
-			if (ret < 0) {
-				dev_err(dev, "failed to start FLL: %d\n", ret);
-				return ret;
-			}
-
-			ret = snd_soc_dai_set_sysclk(codec_dai,
-					WM8962_SYSCLK_FLL, pll_out,
-					SND_SOC_CLOCK_IN);
-			if (ret < 0) {
-				dev_err(dev, "failed to set SYSCLK: %d\n", ret);
-				return ret;
-			}
-		}
+		if (dapm->bias_level == SND_SOC_BIAS_STANDBY)
+			return imx_wm8962_enable_fll(codec_dai, 44100,
+						SNDRV_PCM_FORMAT_S16_LE);
 		break;
 
 	case SND_SOC_BIAS_STANDBY:
-		if (dapm->bias_level == SND_SOC_BIAS_PREPARE) {
-			ret = snd_soc_dai_set_sysclk(codec_dai,
-					WM8962_SYSCLK_MCLK, data->clk_frequency,
-					SND_SOC_CLOCK_IN);
-			if (ret < 0) {
-				dev_err(dev,
-					"failed to switch away from FLL: %d\n",
-					ret);
-				return ret;
-			}
-
-			ret = snd_soc_dai_set_pll(codec_dai, WM8962_FLL,
-					0, 0, 0);
-			if (ret < 0) {
-				dev_err(dev, "failed to stop FLL: %d\n", ret);
-				return ret;
-			}
-		}
+		if (dapm->bias_level == SND_SOC_BIAS_PREPARE)
+			return imx_wm8962_disable_fll(codec_dai);
 		break;
 
 	default:
@@ -225,6 +271,9 @@ static int imx_wm8962_probe(struct platform_device *pdev)
 		goto fail;
 	}
 
+	data->fll_locked = false;
+	spin_lock_init(&data->fll_lock);
+
 	data->codec_clk = devm_clk_get(&codec_dev->dev, NULL);
 	if (IS_ERR(data->codec_clk)) {
 		ret = PTR_ERR(data->codec_clk);
@@ -260,7 +309,6 @@ static int imx_wm8962_probe(struct platform_device *pdev)
 	data->card.dai_link = &data->dai;
 	data->card.dapm_widgets = imx_wm8962_dapm_widgets;
 	data->card.num_dapm_widgets = ARRAY_SIZE(imx_wm8962_dapm_widgets);
-
 	data->card.late_probe = imx_wm8962_late_probe;
 	data->card.set_bias_level = imx_wm8962_set_bias_level;
 
-- 
1.8.4

Re: [PATCH v2 2/2] ASoC: fsl: imx-wm8962: Grant hw_params/free() permission to control FLL

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 971 bytes --]

On Fri, Dec 06, 2013 at 11:38:29PM +0800, Nicolin Chen wrote:

> +static int imx_hifi_hw_free(struct snd_pcm_substream *substream)
> +{
> +	struct snd_soc_pcm_runtime *rtd = substream->private_data;
> +	struct snd_soc_dai *codec_dai = rtd->codec_dai;
> +
> +	/* Don't diable FLL if still having multiple substreams running */
> +	if (codec_dai->active != 1)
> +		return 0;
> +
> +	return imx_wm8962_disable_fll(codec_dai);

This will still disable the FLL if there's an analogue bypass path
active.  I'd suggest changing enable() and disable() to reference count.

> +		if (dapm->bias_level == SND_SOC_BIAS_STANDBY)
> +			return imx_wm8962_enable_fll(codec_dai, 44100,
> +						SNDRV_PCM_FORMAT_S16_LE);

It might be slightly nicer to keep the static variable for the frequency
- that way if we start up due to set_bias_level() we'll keep the last
rate played which might be more likely to avoid locking playbacks out.
Very minor thing though, I wouldn't worry about it.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v2 2/2] ASoC: fsl: imx-wm8962: Grant hw_params/free() permission to control FLL

[Nicolin Chen]

On Mon, Dec 09, 2013 at 05:56:40PM +0000, Mark Brown wrote:
> On Fri, Dec 06, 2013 at 11:38:29PM +0800, Nicolin Chen wrote:
> 
> > +static int imx_hifi_hw_free(struct snd_pcm_substream *substream)
> > +{
> > +	struct snd_soc_pcm_runtime *rtd = substream->private_data;
> > +	struct snd_soc_dai *codec_dai = rtd->codec_dai;
> > +
> > +	/* Don't diable FLL if still having multiple substreams running */
> > +	if (codec_dai->active != 1)
> > +		return 0;
> > +
> > +	return imx_wm8962_disable_fll(codec_dai);
> 
> This will still disable the FLL if there's an analogue bypass path
> active.  I'd suggest changing enable() and disable() to reference count.

I was expecting this would disable it for further reconfiguration. But it
seems I should also considerate the case using bypass path and normal PCM
playback simultaneously, which looks a bit complex.

> 
> > +		if (dapm->bias_level == SND_SOC_BIAS_STANDBY)
> > +			return imx_wm8962_enable_fll(codec_dai, 44100,
> > +						SNDRV_PCM_FORMAT_S16_LE);
> 
> It might be slightly nicer to keep the static variable for the frequency
> - that way if we start up due to set_bias_level() we'll keep the last
> rate played which might be more likely to avoid locking playbacks out.
> Very minor thing though, I wouldn't worry about it.

I'd like to follow this way.

Thank you,
Nicolin Chen

Re: [PATCH v2 2/2] ASoC: fsl: imx-wm8962: Grant hw_params/free() permission to control FLL

[Nicolin Chen]

On Tue, Dec 10, 2013 at 01:18:42PM +0800, Nicolin Chen wrote:
> On Mon, Dec 09, 2013 at 05:56:40PM +0000, Mark Brown wrote:
> > On Fri, Dec 06, 2013 at 11:38:29PM +0800, Nicolin Chen wrote:
> > 
> > > +static int imx_hifi_hw_free(struct snd_pcm_substream *substream)
> > > +{
> > > +	struct snd_soc_pcm_runtime *rtd = substream->private_data;
> > > +	struct snd_soc_dai *codec_dai = rtd->codec_dai;
> > > +
> > > +	/* Don't diable FLL if still having multiple substreams running */
> > > +	if (codec_dai->active != 1)
> > > +		return 0;
> > > +
> > > +	return imx_wm8962_disable_fll(codec_dai);
> > 
> > This will still disable the FLL if there's an analogue bypass path
> > active.  I'd suggest changing enable() and disable() to reference count.
> 
> I was expecting this would disable it for further reconfiguration. But it
> seems I should also considerate the case using bypass path and normal PCM
> playback simultaneously, which looks a bit complex.

Hmm...Sorry for asking a stupid question that how to test bypass path with
WM8962? I've tried some amixer commands to enable the bypass-path switches.
But we still need to power the Codec up like using arecord or aplay to call
wm8962_resume() and DAPM-related, right?

Sir, could you please share a simple flow for user space so that I can test
and make the patch more comprehensive?

Thank you,
Nicolin Chen

Re: [PATCH v2 2/2] ASoC: fsl: imx-wm8962: Grant hw_params/free() permission to control FLL

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 562 bytes --]

On Tue, Dec 10, 2013 at 06:25:24PM +0800, Nicolin Chen wrote:

> Hmm...Sorry for asking a stupid question that how to test bypass path with
> WM8962? I've tried some amixer commands to enable the bypass-path switches.
> But we still need to power the Codec up like using arecord or aplay to call
> wm8962_resume() and DAPM-related, right?

For this sort of question you should follow the DAPM graph...  the
headphone and speaker mixers have IN4 and MIXIN as inputs for analogue
bypass and the STL and STR paths provide digital sidetones through the
ADC and DAC.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v2 2/2] ASoC: fsl: imx-wm8962: Grant hw_params/free() permission to control FLL

[Nicolin Chen]

On Tue, Dec 10, 2013 at 10:47:45AM +0000, Mark Brown wrote:
> On Tue, Dec 10, 2013 at 06:25:24PM +0800, Nicolin Chen wrote:
> 
> > Hmm...Sorry for asking a stupid question that how to test bypass path with
> > WM8962? I've tried some amixer commands to enable the bypass-path switches.
> > But we still need to power the Codec up like using arecord or aplay to call
> > wm8962_resume() and DAPM-related, right?
> 
> For this sort of question you should follow the DAPM graph...  the
> headphone and speaker mixers have IN4 and MIXIN as inputs for analogue
> bypass and the STL and STR paths provide digital sidetones through the
> ADC and DAC.

So I could test it merely by using 'amixer cset' to enable those switches
without doing anything to power WM8962 up as long as my operation follows
the DAPM graph so that the DPAM would automatically turn on the regulator
for me?

Thank you,
Nicolin Chen

Re: [PATCH v2 2/2] ASoC: fsl: imx-wm8962: Grant hw_params/free() permission to control FLL

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 717 bytes --]

On Tue, Dec 10, 2013 at 06:51:36PM +0800, Nicolin Chen wrote:
> On Tue, Dec 10, 2013 at 10:47:45AM +0000, Mark Brown wrote:

> > For this sort of question you should follow the DAPM graph...  the
> > headphone and speaker mixers have IN4 and MIXIN as inputs for analogue
> > bypass and the STL and STR paths provide digital sidetones through the
> > ADC and DAC.

> So I could test it merely by using 'amixer cset' to enable those switches
> without doing anything to power WM8962 up as long as my operation follows
> the DAPM graph so that the DPAM would automatically turn on the regulator
> for me?

Yes, as soon as you have a complete path from an active input to an
active output the path should get powered up.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH v2 2/2] ASoC: fsl: imx-wm8962: Grant hw_params/free() permission to control FLL

[Nicolin Chen]

On Tue, Dec 10, 2013 at 11:15:05AM +0000, Mark Brown wrote:
> On Tue, Dec 10, 2013 at 06:51:36PM +0800, Nicolin Chen wrote:
> > On Tue, Dec 10, 2013 at 10:47:45AM +0000, Mark Brown wrote:
> 
> > > For this sort of question you should follow the DAPM graph...  the
> > > headphone and speaker mixers have IN4 and MIXIN as inputs for analogue
> > > bypass and the STL and STR paths provide digital sidetones through the
> > > ADC and DAC.
> 
> > So I could test it merely by using 'amixer cset' to enable those switches
> > without doing anything to power WM8962 up as long as my operation follows
> > the DAPM graph so that the DPAM would automatically turn on the regulator
> > for me?
> 
> Yes, as soon as you have a complete path from an active input to an
> active output the path should get powered up.

I'll try again tomorrow. Thank you sir.