| Message ID | 1406547718-21498-1-git-send-email-nicoleotsuka@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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().
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
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.
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
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
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(+)