[alsa-devel][PATCH v4] ASoC: wm8960: Let wm8960 codec driver manage its own MCLK

[alsa-devel][PATCH v4] ASoC: wm8960: Let wm8960 codec driver manage its own MCLK

[Zidan Wang]

When we want to use wm8960 codec, we should enable its MCLK in machine
driver. It's reasonable for wm8960 codec driver to manage its own MCLK to
save power.

Enable runtime power management, and auto enable/disable MCLK in pm_runtime
resume and suspend. When wm8960 codec is being used, it will triger resume()
to enable MCLK. When codec is not being used, it will triger suspend() to
disable MCLK.

Signed-off-by: Zidan Wang <b50113@freescale.com>
---
 sound/soc/codecs/wm8960.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/sound/soc/codecs/wm8960.c b/sound/soc/codecs/wm8960.c
index 031a1ae..aff24a7 100644
--- a/sound/soc/codecs/wm8960.c
+++ b/sound/soc/codecs/wm8960.c
@@ -15,6 +15,8 @@
 #include <linux/init.h>
 #include <linux/delay.h>
 #include <linux/pm.h>
+#include <linux/clk.h>
+#include <linux/pm_runtime.h>
 #include <linux/i2c.h>
 #include <linux/slab.h>
 #include <sound/core.h>
@@ -117,6 +119,7 @@ static bool wm8960_volatile(struct device *dev, unsigned int reg)
 }
 
 struct wm8960_priv {
+	struct clk *mclk;
 	struct regmap *regmap;
 	int (*set_bias_level)(struct snd_soc_codec *,
 			      enum snd_soc_bias_level level);
@@ -1002,6 +1005,13 @@ static int wm8960_i2c_probe(struct i2c_client *i2c,
 	if (wm8960 == NULL)
 		return -ENOMEM;
 
+	wm8960->mclk = devm_clk_get(&i2c->dev, "codec_mclk");
+
+	if (IS_ERR(wm8960->mclk)) {
+		if (PTR_ERR(wm8960->mclk) == -EPROBE_DEFER)
+			return -EPROBE_DEFER;
+	}
+
 	wm8960->regmap = devm_regmap_init_i2c(i2c, &wm8960_regmap);
 	if (IS_ERR(wm8960->regmap))
 		return PTR_ERR(wm8960->regmap);
@@ -1041,6 +1051,9 @@ static int wm8960_i2c_probe(struct i2c_client *i2c,
 
 	i2c_set_clientdata(i2c, wm8960);
 
+	pm_runtime_enable(&i2c->dev);
+	pm_request_idle(&i2c->dev);
+
 	ret = snd_soc_register_codec(&i2c->dev,
 			&soc_codec_dev_wm8960, &wm8960_dai, 1);
 
@@ -1053,6 +1066,38 @@ static int wm8960_i2c_remove(struct i2c_client *client)
 	return 0;
 }
 
+#ifdef CONFIG_PM_RUNTIME
+static int wm8960_runtime_resume(struct device *dev)
+{
+	struct wm8960_priv *wm8960 = dev_get_drvdata(dev);
+	int ret;
+
+	if (!IS_ERR(wm8960->mclk)) {
+		ret = clk_prepare_enable(wm8960->mclk);
+		if (ret) {
+			dev_err(dev, "Failed to enable MCLK: %d\n", ret);
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
+static int wm8960_runtime_suspend(struct device *dev)
+{
+	struct wm8960_priv *wm8960 = dev_get_drvdata(dev);
+
+	if (!IS_ERR(wm8960->mclk))
+		clk_disable_unprepare(wm8960->mclk);
+
+	return 0;
+}
+#endif
+
+static const struct dev_pm_ops wm8960_pm = {
+	SET_RUNTIME_PM_OPS(wm8960_runtime_suspend, wm8960_runtime_resume, NULL)
+};
+
 static const struct i2c_device_id wm8960_i2c_id[] = {
 	{ "wm8960", 0 },
 	{ }
@@ -1070,6 +1115,7 @@ static struct i2c_driver wm8960_i2c_driver = {
 		.name = "wm8960",
 		.owner = THIS_MODULE,
 		.of_match_table = wm8960_of_match,
+		.pm = &wm8960_pm,
 	},
 	.probe =    wm8960_i2c_probe,
 	.remove =   wm8960_i2c_remove,
-- 
1.9.1

Re: [alsa-devel][PATCH v4] ASoC: wm8960: Let wm8960 codec driver manage its own MCLK

[Charles Keepax]

On Tue, Dec 09, 2014 at 01:45:16PM +0800, Zidan Wang wrote:
> When we want to use wm8960 codec, we should enable its MCLK in machine
> driver. It's reasonable for wm8960 codec driver to manage its own MCLK to
> save power.
> 
> Enable runtime power management, and auto enable/disable MCLK in pm_runtime
> resume and suspend. When wm8960 codec is being used, it will triger resume()
> to enable MCLK. When codec is not being used, it will triger suspend() to
> disable MCLK.
> 
> Signed-off-by: Zidan Wang <b50113@freescale.com>
> ---

Acked-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>

Thanks,
Charles

Re: [alsa-devel][PATCH v4] ASoC: wm8960: Let wm8960 codec driver manage its own MCLK

[Mark Brown]

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

On Tue, Dec 09, 2014 at 01:45:16PM +0800, Zidan Wang wrote:

> @@ -1002,6 +1005,13 @@ static int wm8960_i2c_probe(struct i2c_client *i2c,
>  	if (wm8960 == NULL)
>  		return -ENOMEM;
>  
> +	wm8960->mclk = devm_clk_get(&i2c->dev, "codec_mclk");
> +
> +	if (IS_ERR(wm8960->mclk)) {
> +		if (PTR_ERR(wm8960->mclk) == -EPROBE_DEFER)
> +			return -EPROBE_DEFER;
> +	}
> +
>  	wm8960->regmap = devm_regmap_init_i2c(i2c, &wm8960_regmap);
>  	if (IS_ERR(wm8960->regmap))
>  		return PTR_ERR(wm8960->regmap);
> @@ -1041,6 +1051,9 @@ static int wm8960_i2c_probe(struct i2c_client *i2c,
>  
>  	i2c_set_clientdata(i2c, wm8960);
>  
> +	pm_runtime_enable(&i2c->dev);
> +	pm_request_idle(&i2c->dev);
> +
>  	ret = snd_soc_register_codec(&i2c->dev,
>  			&soc_codec_dev_wm8960, &wm8960_dai, 1);
>  

This isn't going to work if PM is disabled (which is still a valid
configuration).  The general idiom for this is that the driver should
start up with everything powered up then let runtime idle turn things
off if they're not required.  That way if runtime PM is disabled then
the system will still work as everything will just stay powered on all
the time.

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

RE: [alsa-devel][PATCH v4] ASoC: wm8960: Let wm8960 codec driver manage its own MCLK

[Zidan Wang]

Hi Mark,

I am so sorry for missing this letter. 
You are right. When PM is disabled, the codec will not work. But there are also some codecs enable mclk in PM, such as wm8962, cs42xx8.
And some codecs enable codec mclk in i2c probe(), startup() and set_bias_level(). It makes me confused.	
Can you tell me what's the general idiom to enable mclk. Thanks.


Best Regards,
Zidan

-----Original Message-----
From: Mark Brown [mailto:broonie@kernel.org] 
Sent: Tuesday, December 23, 2014 2:11 AM
To: Wang Zidan-B50113
Cc: lgirdwood@gmail.com; perex@perex.cz; tiwai@suse.de; lars@metafoo.de; ckeepax@opensource.wolfsonmicro.com; Xiubo Li-B47053; patches@opensource.wolfsonmicro.com; alsa-devel@alsa-project.org; linux-kernel@vger.kernel.org
Subject: Re: [alsa-devel][PATCH v4] ASoC: wm8960: Let wm8960 codec driver manage its own MCLK

On Tue, Dec 09, 2014 at 01:45:16PM +0800, Zidan Wang wrote:

> @@ -1002,6 +1005,13 @@ static int wm8960_i2c_probe(struct i2c_client *i2c,
>  	if (wm8960 == NULL)
>  		return -ENOMEM;
>  
> +	wm8960->mclk = devm_clk_get(&i2c->dev, "codec_mclk");
> +
> +	if (IS_ERR(wm8960->mclk)) {
> +		if (PTR_ERR(wm8960->mclk) == -EPROBE_DEFER)
> +			return -EPROBE_DEFER;
> +	}
> +
>  	wm8960->regmap = devm_regmap_init_i2c(i2c, &wm8960_regmap);
>  	if (IS_ERR(wm8960->regmap))
>  		return PTR_ERR(wm8960->regmap);
> @@ -1041,6 +1051,9 @@ static int wm8960_i2c_probe(struct i2c_client 
> *i2c,
>  
>  	i2c_set_clientdata(i2c, wm8960);
>  
> +	pm_runtime_enable(&i2c->dev);
> +	pm_request_idle(&i2c->dev);
> +
>  	ret = snd_soc_register_codec(&i2c->dev,
>  			&soc_codec_dev_wm8960, &wm8960_dai, 1);
>  

This isn't going to work if PM is disabled (which is still a valid configuration).  The general idiom for this is that the driver should start up with everything powered up then let runtime idle turn things off if they're not required.  That way if runtime PM is disabled then the system will still work as everything will just stay powered on all the time.

Re: [alsa-devel][PATCH v4] ASoC: wm8960: Let wm8960 codec driver manage its own MCLK

[Mark Brown]

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

On Mon, Dec 29, 2014 at 10:59:08AM +0000, Zidan Wang wrote:
> Hi Mark,

Don't top post and please fix your mailer to word wrap within paragraphs
and avoid corrupting the mail it's quoting.

> You are right. When PM is disabled, the codec will not work. But there are also some codecs enable mclk in PM, such as wm8962, cs42xx8.
> And some codecs enable codec mclk in i2c probe(), startup() and set_bias_level(). It makes me confused.	
> Can you tell me what's the general idiom to enable mclk. Thanks.

Like I said in the mail to which you replied:

> > This isn't going to work if PM is disabled (which is still a valid configuration).  The general idiom for this is that the driver should start up with everything powered up then let runtime idle turn things off if they're not required.  That way if runtime PM is disabled then the system will still work as everything will just stay powered on all the time.

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

Re: [alsa-devel][PATCH v4] ASoC: wm8960: Let wm8960 codec driver manage its own MCLK

[Zidan Wang]

On Mon, Dec 29, 2014 at 04:05:14PM +0000, Mark Brown wrote:
> On Mon, Dec 29, 2014 at 10:59:08AM +0000, Zidan Wang wrote:
> > Hi Mark,
> 
> Don't top post and please fix your mailer to word wrap within paragraphs
> and avoid corrupting the mail it's quoting.
> 
> > You are right. When PM is disabled, the codec will not work. But there are also some codecs enable mclk in PM, such as wm8962, cs42xx8.
> > And some codecs enable codec mclk in i2c probe(), startup() and set_bias_level(). It makes me confused.	
> > Can you tell me what's the general idiom to enable mclk. Thanks.
> 
> Like I said in the mail to which you replied:
> 
> > > This isn't going to work if PM is disabled (which is still a valid configuration).  The general idiom for this is that the driver should start up with everything powered up then let runtime idle turn things off if they're not required.  That way if runtime PM is disabled then the system will still work as everything will just stay powered on all the time.

I want put mclk enable to set_bias_level. Do you thinks it make sense?

Re: [alsa-devel][PATCH v4] ASoC: wm8960: Let wm8960 codec driver manage its own MCLK

[Mark Brown]

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

On Tue, Dec 30, 2014 at 10:29:18AM +0800, Zidan Wang wrote:

> I want put mclk enable to set_bias_level. Do you thinks it make sense?

That should work as well, yes.

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