[PATCH] ASoC: kirkwood: simplify clock handling

[PATCH] ASoC: kirkwood: simplify clock handling

[Uwe Kleine-König]

There is no need to not use extclk if it is identical to the main clk.
The main motivation for this patch is dropping devm_clk_put which is
used in a wrong way by all other users.

While at it also extend the comments.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
Hello,

there might be some further optimisations possible. I only note them
here because I don't have the hardware to test:

 - only enable extclk if it a clock rate used that makes use of the
   external clock. Not sure if that works; hardware docs reading
   necessary.
 - only provide extclk if it's != the internal clock.
 - The code uses:

	priv->clk = devm_clk_get(&pdev->dev, np ? "internal" : NULL);

   i.e. provides a con_id in the dt-case. I think that using NULL
   unconditionally should also work, i.e. return the first clk
   associated to the device. OTOH the current code might make things
   clearer because it's more explicit.

Best regards
Uwe
---
 sound/soc/kirkwood/kirkwood-i2s.c | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/sound/soc/kirkwood/kirkwood-i2s.c b/sound/soc/kirkwood/kirkwood-i2s.c
index 0f3d73d..8224f93 100644
--- a/sound/soc/kirkwood/kirkwood-i2s.c
+++ b/sound/soc/kirkwood/kirkwood-i2s.c
@@ -104,16 +104,25 @@ static void kirkwood_set_rate(struct snd_soc_dai *dai,
 	uint32_t clks_ctrl;
 
 	if (rate == 44100 || rate == 48000 || rate == 96000) {
-		/* use internal dco for the supported rates
-		 * defined in kirkwood_i2s_dai */
+		/*
+		 * use internal dco for the supported rates
+		 * defined in kirkwood_i2s_dai
+		 * Note: For these specific rates the dco is also used if
+		 * kirkwood_i2s_dai_extclk is in use.
+		 */
 		dev_dbg(dai->dev, "%s: dco set rate = %lu\n",
 			__func__, rate);
 		kirkwood_set_dco(priv->io, rate);
 
 		clks_ctrl = KIRKWOOD_MCLK_SOURCE_DCO;
 	} else {
-		/* use the external clock for the other rates
-		 * defined in kirkwood_i2s_dai_extclk */
+		/*
+		 * use the external clock for the other rates
+		 * defined in kirkwood_i2s_dai_extclk
+		 * This case isn't reached if kirkwood_i2s_dai is in use (and so
+		 * extclk isn't valid) because it only supports the three rates
+		 * above.
+		 */
 		dev_dbg(dai->dev, "%s: extclk set rate = %lu -> %lu\n",
 			__func__, rate, 256 * rate);
 		clk_set_rate(priv->extclk, 256 * rate);
@@ -497,12 +506,9 @@ static int kirkwood_i2s_dev_probe(struct platform_device *pdev)
 
 	priv->extclk = devm_clk_get(&pdev->dev, "extclk");
 	if (!IS_ERR(priv->extclk)) {
-		if (priv->extclk == priv->clk) {
-			devm_clk_put(&pdev->dev, priv->extclk);
-			priv->extclk = ERR_PTR(-EINVAL);
-		} else {
+		clk_prepare_enable(priv->extclk);
+		if (priv->extclk != priv->clk) {
 			dev_info(&pdev->dev, "found external clock\n");
-			clk_prepare_enable(priv->extclk);
 			soc_dai = &kirkwood_i2s_dai_extclk;
 		}
 	}
-- 
1.8.4.rc3

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] ASoC: kirkwood: simplify clock handling

[Russell King - ARM Linux]

On Tue, Sep 24, 2013 at 08:12:35PM +0200, Uwe Kleine-König wrote:
> There is no need to not use extclk if it is identical to the main clk.
> The main motivation for this patch is dropping devm_clk_put which is
> used in a wrong way by all other users.

NAK.  There's patches around which switch this driver to always use
the external clock when it's available.  This patch prevents that
from happening because we no longer know whether it is the external
clock or not.

What we could do is just lose the reference to the second clock if
it turns out to be idential to the first - it'll get cleaned up if
the driver is unloaded anyway.  In other words, the call to
devm_clk_put() here can be viewed as merely an optimisation.

Re: [PATCH] ASoC: kirkwood: simplify clock handling

[Jean-Francois Moine]

On Tue, 24 Sep 2013 20:12:35 +0200
Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote:

> There is no need to not use extclk if it is identical to the main clk.
> The main motivation for this patch is dropping devm_clk_put which is
> used in a wrong way by all other users.
> 
> While at it also extend the comments.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
> Hello,
> 
> there might be some further optimisations possible. I only note them
> here because I don't have the hardware to test:
> 
>  - only enable extclk if it a clock rate used that makes use of the
>    external clock. Not sure if that works; hardware docs reading
>    necessary.
>  - only provide extclk if it's != the internal clock.
>  - The code uses:
> 
> 	priv->clk = devm_clk_get(&pdev->dev, np ? "internal" : NULL);
> 
>    i.e. provides a con_id in the dt-case. I think that using NULL
>    unconditionally should also work, i.e. return the first clk
>    associated to the device. OTOH the current code might make things
>    clearer because it's more explicit.
	[snip]

Uwe,

The code around line 104 in kirkwood-i2s.c is not what it should be
(the patch from Russell is lost somewhere in the mailing-list).
Instead of:

	if (rate == 44100 || rate == 48000 || rate == 96000) {
		/* use internal dco for the supported rates
		 * defined in kirkwood_i2s_dai */

it should be:

	if (IS_ERR(priv->extclk)) {	/* no external clock */
		/* use internal dco - the supported rates are
		 * defined in kirkwood_i2s_dai */

That is: if there is an external clock, use it.

In fact, the internal dco is used for two audio devices. When both
devices are used at the same time, at least one of them must always use
an external clock, otherwise, there is a clock rate conflict.

As only one clock is used, there is no need to declare 2 clocks in the
DT, but the driver must know if it uses the internal or external clock
(to set the right clock input and also because their rates are not set
the same way)

So, the probe code should be:

	/* check first if an external clock is declared */
	priv->extclk = devm_clk_get(&pdev->dev, "extclk");
	if (!IS_ERR(priv->extclk)) {
		... use the external clock ...
	} else {

		/* get the first clock which must be the dco */
		priv->clk = devm_clk_get(&pdev->dev, NULL);
		if (IS_ERR(priv->clk))
			.. error, no clock ..
		.. use the internal dco ...
	}

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] ASoC: kirkwood: simplify clock handling

[Russell King - ARM Linux]

On Tue, Sep 24, 2013 at 09:04:42PM +0200, Jean-Francois Moine wrote:
> So, the probe code should be:
> 
> 	/* check first if an external clock is declared */
> 	priv->extclk = devm_clk_get(&pdev->dev, "extclk");
> 	if (!IS_ERR(priv->extclk)) {
> 		... use the external clock ...
> 	} else {
> 
> 		/* get the first clock which must be the dco */
> 		priv->clk = devm_clk_get(&pdev->dev, NULL);
> 		if (IS_ERR(priv->clk))
> 			.. error, no clock ..
> 		.. use the internal dco ...
> 	}

Actually no - we need to get and enable the internal clock so that we can
access the registers - the Dove locks solid if you access the audio block
registers without its internal clock to the audio block enabled.

Re: [PATCH] ASoC: kirkwood: simplify clock handling

[Uwe Kleine-König]

On Tue, Sep 24, 2013 at 08:05:34PM +0100, Russell King - ARM Linux wrote:
> On Tue, Sep 24, 2013 at 09:04:42PM +0200, Jean-Francois Moine wrote:
> > So, the probe code should be:
> > 
> > 	/* check first if an external clock is declared */
> > 	priv->extclk = devm_clk_get(&pdev->dev, "extclk");
> > 	if (!IS_ERR(priv->extclk)) {
> > 		... use the external clock ...
> > 	} else {
> > 
> > 		/* get the first clock which must be the dco */
> > 		priv->clk = devm_clk_get(&pdev->dev, NULL);
> > 		if (IS_ERR(priv->clk))
> > 			.. error, no clock ..
> > 		.. use the internal dco ...
> > 	}
> 
> Actually no - we need to get and enable the internal clock so that we can
> access the registers - the Dove locks solid if you access the audio block
> registers without its internal clock to the audio block enabled.
So what is the plan here? Apply Russell's patch and then just drop the
devm_clk_put? Do you have a pointer to that patch? Then I'd follow up
with a patch for devm_clk_put.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |