[PATCH v2] ASoC: wm8962: Let CODEC driver enable and disable its own MCLK

[PATCH v2] ASoC: wm8962: Let CODEC driver enable and disable its own MCLK

[Nicolin Chen]

snd_soc_open() will trigger pm_runtime resume() which will then enable
the regulator and initialization. So we should make sure the MCLK is
enabled before this resume().

Previously we let the machine driver get the clock and enable it in
its probe(). However, considering about power saving, it'll be better
to enable it when it's going to be used and disable it after using.

So this patch just simply adds clk_get() and clk_enable() to WM8962
driver. Meanwhile, it marks clock pointer to NULL if no clock assigned
to it so it will not break any current function.

Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com>
---

Changelog:
v2
 * Use wm8962->pdata.mclk instead.

 include/sound/wm8962.h    |  1 +
 sound/soc/codecs/wm8962.c | 11 +++++++++++
 2 files changed, 12 insertions(+)

diff --git a/include/sound/wm8962.h b/include/sound/wm8962.h
index 79e6d42..0af7c16 100644
--- a/include/sound/wm8962.h
+++ b/include/sound/wm8962.h
@@ -37,6 +37,7 @@
 #define WM8962_GPIO_FN_MICSCD          22
 
 struct wm8962_pdata {
+	struct clk *mclk;
 	int gpio_base;
 	u32 gpio_init[WM8962_MAX_GPIO];
 
diff --git a/sound/soc/codecs/wm8962.c b/sound/soc/codecs/wm8962.c
index ca2fda9..28a2c21 100644
--- a/sound/soc/codecs/wm8962.c
+++ b/sound/soc/codecs/wm8962.c
@@ -14,6 +14,7 @@
 #include <linux/module.h>
 #include <linux/moduleparam.h>
 #include <linux/init.h>
+#include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/pm.h>
 #include <linux/gcd.h>
@@ -3541,6 +3542,8 @@ static int wm8962_set_pdata_from_of(struct i2c_client *i2c,
 				pdata->gpio_init[i] = 0x0;
 		}
 
+	pdata->mclk = devm_clk_get(&i2c->dev, NULL);
+
 	return 0;
 }
 
@@ -3572,6 +3575,10 @@ static int wm8962_i2c_probe(struct i2c_client *i2c,
 			return ret;
 	}
 
+	/* Mark the mclk pointer to NULL if no mclk assigned */
+	if (IS_ERR(wm8962->pdata.mclk))
+		wm8962->pdata.mclk = NULL;
+
 	for (i = 0; i < ARRAY_SIZE(wm8962->supplies); i++)
 		wm8962->supplies[i].supply = wm8962_supply_names[i];
 
@@ -3780,6 +3787,8 @@ static int wm8962_runtime_resume(struct device *dev)
 	struct wm8962_priv *wm8962 = dev_get_drvdata(dev);
 	int ret;
 
+	clk_prepare_enable(wm8962->pdata.mclk);
+
 	ret = regulator_bulk_enable(ARRAY_SIZE(wm8962->supplies),
 				    wm8962->supplies);
 	if (ret != 0) {
@@ -3839,6 +3848,8 @@ static int wm8962_runtime_suspend(struct device *dev)
 	regulator_bulk_disable(ARRAY_SIZE(wm8962->supplies),
 			       wm8962->supplies);
 
+	clk_disable_unprepare(wm8962->pdata.mclk);
+
 	return 0;
 }
 #endif
-- 
1.8.4

[PATCH v2] ASoC: wm8962: Let CODEC driver enable and disable its own MCLK

[Nicolin Chen]

snd_soc_open() will trigger pm_runtime resume() which will then enable
the regulator and initialization. So we should make sure the MCLK is
enabled before this resume().

Previously we let the machine driver get the clock and enable it in
its probe(). However, considering about power saving, it'll be better
to enable it when it's going to be used and disable it after using.

So this patch just simply adds clk_get() and clk_enable() to WM8962
driver. Meanwhile, it marks clock pointer to NULL if no clock assigned
to it so it will not break any current function.

Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com>
---

Changelog:
v2
 * Use wm8962->pdata.mclk instead.

 include/sound/wm8962.h    |  1 +
 sound/soc/codecs/wm8962.c | 11 +++++++++++
 2 files changed, 12 insertions(+)

diff --git a/include/sound/wm8962.h b/include/sound/wm8962.h
index 79e6d42..0af7c16 100644
--- a/include/sound/wm8962.h
+++ b/include/sound/wm8962.h
@@ -37,6 +37,7 @@
 #define WM8962_GPIO_FN_MICSCD          22
 
 struct wm8962_pdata {
+	struct clk *mclk;
 	int gpio_base;
 	u32 gpio_init[WM8962_MAX_GPIO];
 
diff --git a/sound/soc/codecs/wm8962.c b/sound/soc/codecs/wm8962.c
index ca2fda9..28a2c21 100644
--- a/sound/soc/codecs/wm8962.c
+++ b/sound/soc/codecs/wm8962.c
@@ -14,6 +14,7 @@
 #include <linux/module.h>
 #include <linux/moduleparam.h>
 #include <linux/init.h>
+#include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/pm.h>
 #include <linux/gcd.h>
@@ -3541,6 +3542,8 @@ static int wm8962_set_pdata_from_of(struct i2c_client *i2c,
 				pdata->gpio_init[i] = 0x0;
 		}
 
+	pdata->mclk = devm_clk_get(&i2c->dev, NULL);
+
 	return 0;
 }
 
@@ -3572,6 +3575,10 @@ static int wm8962_i2c_probe(struct i2c_client *i2c,
 			return ret;
 	}
 
+	/* Mark the mclk pointer to NULL if no mclk assigned */
+	if (IS_ERR(wm8962->pdata.mclk))
+		wm8962->pdata.mclk = NULL;
+
 	for (i = 0; i < ARRAY_SIZE(wm8962->supplies); i++)
 		wm8962->supplies[i].supply = wm8962_supply_names[i];
 
@@ -3780,6 +3787,8 @@ static int wm8962_runtime_resume(struct device *dev)
 	struct wm8962_priv *wm8962 = dev_get_drvdata(dev);
 	int ret;
 
+	clk_prepare_enable(wm8962->pdata.mclk);
+
 	ret = regulator_bulk_enable(ARRAY_SIZE(wm8962->supplies),
 				    wm8962->supplies);
 	if (ret != 0) {
@@ -3839,6 +3848,8 @@ static int wm8962_runtime_suspend(struct device *dev)
 	regulator_bulk_disable(ARRAY_SIZE(wm8962->supplies),
 			       wm8962->supplies);
 
+	clk_disable_unprepare(wm8962->pdata.mclk);
+
 	return 0;
 }
 #endif
-- 
1.8.4

Re: [PATCH v2] ASoC: wm8962: Let CODEC driver enable and disable its own MCLK

[Mark Brown]

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

On Mon, Jul 28, 2014 at 07:41:58PM +0800, Nicolin Chen wrote:

> +	pdata->mclk = devm_clk_get(&i2c->dev, NULL);
> +

This is broken for deferred probe.

> +	clk_prepare_enable(wm8962->pdata.mclk);
> +

Missing error checking.

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

Re: [alsa-devel] [PATCH v2] ASoC: wm8962: Let CODEC driver enable and disable its own MCLK

[Fabio Estevam]

Hi Nicolin,

On Mon, Jul 28, 2014 at 9:22 AM, Mark Brown <broonie@kernel.org> wrote:
> On Mon, Jul 28, 2014 at 07:41:58PM +0800, Nicolin Chen wrote:
>
>> +     pdata->mclk = devm_clk_get(&i2c->dev, NULL);
>> +
>
> This is broken for deferred probe.

You can take a look at sgtl5000 to see how we handle deferred probe in
devm_clk_get().

Re: [alsa-devel] [PATCH v2] ASoC: wm8962: Let CODEC driver enable and disable its own MCLK

[Nicolin Chen]

On Mon, Jul 28, 2014 at 03:19:43PM -0300, Fabio Estevam wrote:
> Hi Nicolin,
> 
> On Mon, Jul 28, 2014 at 9:22 AM, Mark Brown <broonie@kernel.org> wrote:
> > On Mon, Jul 28, 2014 at 07:41:58PM +0800, Nicolin Chen wrote:
> >
> >> +     pdata->mclk = devm_clk_get(&i2c->dev, NULL);
> >> +
> >
> > This is broken for deferred probe.

Hi Mark,

Sorry. I don't get why deferred probe would be broken by this.
And I just took a test by adding this:

[...]
@@ -3545,10 +3545,11 @@ static int wm8962_set_pdata_from_of(struct i2c_client *i2c,
	}

+bool letstry = true;
static int wm8962_i2c_probe(struct i2c_client *i2c,
			const struct i2c_device_id *id)
@@ -3573,10 +3574,15 @@ static int wm8962_i2c_probe(struct i2c_client *i2c,
		ret = wm8962_set_pdata_from_of(i2c, &wm8962->pdata);
		if (ret != 0)
			return ret;
	}
+       if (letstry) {
+               letstry = false;
+               return -EPROBE_DEFER;
+       }
	/* Mark the mclk pointer to NULL if no mclk assigned */
	if (IS_ERR(wm8962->pdata.mclk))
		wm8962->pdata.mclk = NULL;
[...]

And it still works for me with a deferred probe:

[] i2c 0-001a: Driver wm8962 requests probe deferral
[]...
[]wm8962 0-001a: customer id 0 revision D
[]input: WM8962 Beep Generator as /devices/soc0/soc/2100000......
[]fsl-asoc-card sound: wm8962 <-> 202c000.ssi mapping ok

> 
> You can take a look at sgtl5000 to see how we handle deferred probe in
> devm_clk_get().

Hi Fabio,

Thank you for the hint.

But actually, unlike sgtl5000, this patch doesn't error out for
IS_ERR(mclk) case.

There might be a machine driver that has already got and enabled
the clock while getting the clock from other way or other DT node
rather than WM8962's.

Thank you.
Nicolin

Re: [alsa-devel] [PATCH v2] ASoC: wm8962: Let CODEC driver enable and disable its own MCLK

[Nicolin Chen]

On Mon, Jul 28, 2014 at 03:19:43PM -0300, Fabio Estevam wrote:
> Hi Nicolin,
> 
> On Mon, Jul 28, 2014 at 9:22 AM, Mark Brown <broonie@kernel.org> wrote:
> > On Mon, Jul 28, 2014 at 07:41:58PM +0800, Nicolin Chen wrote:
> >
> >> +     pdata->mclk = devm_clk_get(&i2c->dev, NULL);
> >> +
> >
> > This is broken for deferred probe.

Hi Mark,

Sorry. I don't get why deferred probe would be broken by this.
And I just took a test by adding this:

[...]
@@ -3545,10 +3545,11 @@ static int wm8962_set_pdata_from_of(struct i2c_client *i2c,
	}

+bool letstry = true;
static int wm8962_i2c_probe(struct i2c_client *i2c,
			const struct i2c_device_id *id)
@@ -3573,10 +3574,15 @@ static int wm8962_i2c_probe(struct i2c_client *i2c,
		ret = wm8962_set_pdata_from_of(i2c, &wm8962->pdata);
		if (ret != 0)
			return ret;
	}
+       if (letstry) {
+               letstry = false;
+               return -EPROBE_DEFER;
+       }
	/* Mark the mclk pointer to NULL if no mclk assigned */
	if (IS_ERR(wm8962->pdata.mclk))
		wm8962->pdata.mclk = NULL;
[...]

And it still works for me with a deferred probe:

[] i2c 0-001a: Driver wm8962 requests probe deferral
[]...
[]wm8962 0-001a: customer id 0 revision D
[]input: WM8962 Beep Generator as /devices/soc0/soc/2100000......
[]fsl-asoc-card sound: wm8962 <-> 202c000.ssi mapping ok

> 
> You can take a look at sgtl5000 to see how we handle deferred probe in
> devm_clk_get().

Hi Fabio,

Thank you for the hint.

But actually, unlike sgtl5000, this patch doesn't error out for
IS_ERR(mclk) case.

There might be a machine driver that has already got and enabled
the clock while getting the clock from other way or other DT node
rather than WM8962's.

Thank you.
Nicolin

Re: [alsa-devel] [PATCH v2] ASoC: wm8962: Let CODEC driver enable and disable its own MCLK

[Mark Brown]

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

On Tue, Jul 29, 2014 at 10:55:19AM +0800, Nicolin Chen wrote:
> On Mon, Jul 28, 2014 at 03:19:43PM -0300, Fabio Estevam wrote:

> > >> +     pdata->mclk = devm_clk_get(&i2c->dev, NULL);

> > > This is broken for deferred probe.

> Sorry. I don't get why deferred probe would be broken by this.
> And I just took a test by adding this:

I don't see the code that actually pays attention to the return value
from devm_clk_get() - in the main probe function the code just treats
all errors as a missing MCLK.

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

Re: [alsa-devel] [PATCH v2] ASoC: wm8962: Let CODEC driver enable and disable its own MCLK

[Nicolin Chen]

On Tue, Jul 29, 2014 at 10:52:03AM +0100, Mark Brown wrote:
> On Tue, Jul 29, 2014 at 10:55:19AM +0800, Nicolin Chen wrote:
> > On Mon, Jul 28, 2014 at 03:19:43PM -0300, Fabio Estevam wrote:
> 
> > > >> +     pdata->mclk = devm_clk_get(&i2c->dev, NULL);
> 
> > > > This is broken for deferred probe.
> 
> > Sorry. I don't get why deferred probe would be broken by this.
> > And I just took a test by adding this:
> 
> I don't see the code that actually pays attention to the return value
> from devm_clk_get() - in the main probe function the code just treats
> all errors as a missing MCLK.

Just checked the clk_get() code and got the point.

I will add a check to it and return for EPROBE_DEFER.

Thank you,
Nicolin

Re: [alsa-devel] [PATCH v2] ASoC: wm8962: Let CODEC driver enable and disable its own MCLK

[Nicolin Chen]

On Tue, Jul 29, 2014 at 10:52:03AM +0100, Mark Brown wrote:
> On Tue, Jul 29, 2014 at 10:55:19AM +0800, Nicolin Chen wrote:
> > On Mon, Jul 28, 2014 at 03:19:43PM -0300, Fabio Estevam wrote:
> 
> > > >> +     pdata->mclk = devm_clk_get(&i2c->dev, NULL);
> 
> > > > This is broken for deferred probe.
> 
> > Sorry. I don't get why deferred probe would be broken by this.
> > And I just took a test by adding this:
> 
> I don't see the code that actually pays attention to the return value
> from devm_clk_get() - in the main probe function the code just treats
> all errors as a missing MCLK.

Just checked the clk_get() code and got the point.

I will add a check to it and return for EPROBE_DEFER.

Thank you,
Nicolin