[PATCH v3 2/2] ASoC: kirkwood: fix loss of external clock at probe time

[PATCH v3 2/2] ASoC: kirkwood: fix loss of external clock at probe time

[Jean-Francois Moine]

At probe time, when the clock driver is not yet initialized, the
external clock of the kirkwood sound device will not be usable.

This patch fixes this problem defering the device probe.

It also removes the test about same internal and external clocks which
can never occur.

Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
---
v3: remove the test of same internal and external clocks

The associated patch:
[PATCH v3 1/2] ASoC: kirkwood: clk: probe defer when clock not yet ready
is unchanged and not resent
---
 sound/soc/kirkwood/kirkwood-i2s.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/sound/soc/kirkwood/kirkwood-i2s.c b/sound/soc/kirkwood/kirkwood-i2s.c
index 8ac89f5..2bbbab5 100644
--- a/sound/soc/kirkwood/kirkwood-i2s.c
+++ b/sound/soc/kirkwood/kirkwood-i2s.c
@@ -496,15 +496,13 @@ static int kirkwood_i2s_dev_probe(struct platform_device *pdev)
 		return err;
 
 	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 {
-			dev_info(&pdev->dev, "found external clock\n");
-			clk_prepare_enable(priv->extclk);
-			soc_dai = &kirkwood_i2s_dai_extclk;
-		}
+	if (IS_ERR(priv->extclk)) {
+		if (PTR_ERR(priv->extclk) == -EPROBE_DEFER)
+			return -EPROBE_DEFER;
+	} else {
+		dev_info(&pdev->dev, "found external clock\n");
+		clk_prepare_enable(priv->extclk);
+		soc_dai = kirkwood_i2s_dai_extclk;
 	}
 
 	/* Some sensible defaults - this reflects the powerup values */


-- 
Ken ar c'henta?	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

[PATCH v3 2/2] ASoC: kirkwood: fix loss of external clock at probe time

[Uwe Kleine-König]

Hello,

On Fri, Oct 18, 2013 at 08:35:55PM +0200, Jean-Francois Moine wrote:
> diff --git a/sound/soc/kirkwood/kirkwood-i2s.c b/sound/soc/kirkwood/kirkwood-i2s.c
> index 8ac89f5..2bbbab5 100644
> --- a/sound/soc/kirkwood/kirkwood-i2s.c
> +++ b/sound/soc/kirkwood/kirkwood-i2s.c
> @@ -496,15 +496,13 @@ static int kirkwood_i2s_dev_probe(struct platform_device *pdev)
>  		return err;
>  
>  	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 {
> -			dev_info(&pdev->dev, "found external clock\n");
> -			clk_prepare_enable(priv->extclk);
> -			soc_dai = &kirkwood_i2s_dai_extclk;
> -		}
> +	if (IS_ERR(priv->extclk)) {
> +		if (PTR_ERR(priv->extclk) == -EPROBE_DEFER)
> +			return -EPROBE_DEFER;
Maybe the better logic here is:
		if (!PTR_ERR(priv->extclk) == -ENOENT)
			return PTR_ERR(priv->extclk);

?

> +	} else {
> +		dev_info(&pdev->dev, "found external clock\n");
> +		clk_prepare_enable(priv->extclk);
> +		soc_dai = kirkwood_i2s_dai_extclk;
Another todo (that is already in the patched code) is that you should
check the return value of clk_prepare_enable.

Best regards
Uwe

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

[PATCH v3 2/2] ASoC: kirkwood: fix loss of external clock at probe time

[Jean-Francois Moine]

On Fri, 18 Oct 2013 21:12:59 +0200
Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de> wrote:

> > +	if (IS_ERR(priv->extclk)) {
> > +		if (PTR_ERR(priv->extclk) == -EPROBE_DEFER)
> > +			return -EPROBE_DEFER;  
> Maybe the better logic here is:
> 		if (!PTR_ERR(priv->extclk) == -ENOENT)
> 			return PTR_ERR(priv->extclk);
> 
> ?

No. This patch is associated with an other one which returns
-EPROBE_DEFER when the external clock is declared in the DT and when
the clock driver is not yet initialized. Then, the kirkwood modules
must be probed later.

For any other error, the external clock is not used, and it is not easy
to know if the error is due to the absence of external clock in the DT
or to a DT parsing error or to anything else.

> > +	} else {
> > +		dev_info(&pdev->dev, "found external clock\n");
> > +		clk_prepare_enable(priv->extclk);
> > +		soc_dai = kirkwood_i2s_dai_extclk;  
> Another todo (that is already in the patched code) is that you should
> check the return value of clk_prepare_enable.

You are right, but an error at this level should not often occur...

-- 
Ken ar c'henta?	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

[PATCH v3 2/2] ASoC: kirkwood: fix loss of external clock at probe time

[Uwe Kleine-König]

On Sat, Oct 19, 2013 at 10:28:09AM +0200, Jean-Francois Moine wrote:
> On Fri, 18 Oct 2013 21:12:59 +0200
> Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de> wrote:
> 
> > > +	if (IS_ERR(priv->extclk)) {
> > > +		if (PTR_ERR(priv->extclk) == -EPROBE_DEFER)
> > > +			return -EPROBE_DEFER;  
> > Maybe the better logic here is:
> > 		if (!PTR_ERR(priv->extclk) == -ENOENT)
> > 			return PTR_ERR(priv->extclk);
> > 
> > ?
> 
> No. This patch is associated with an other one which returns
> -EPROBE_DEFER when the external clock is declared in the DT and when
> the clock driver is not yet initialized. Then, the kirkwood modules
> must be probed later.
Yes, that's understood. My suggestion behaves as your's for the return
values -EPROBE_DEFER and -ENOENT, so the deferred probe should work,
too. The question is only what you want to do for other errors (don't
know if they can happen at this stage). I'd say, on a dt parsing error
bail out, too.

Best regards
Uwe

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

[PATCH v3 2/2] ASoC: kirkwood: fix loss of external clock at probe time

[Mark Brown]

On Sun, Oct 20, 2013 at 10:03:33AM +0200, Uwe Kleine-K?nig wrote:

> Yes, that's understood. My suggestion behaves as your's for the return
> values -EPROBE_DEFER and -ENOENT, so the deferred probe should work,
> too. The question is only what you want to do for other errors (don't
> know if they can happen at this stage). I'd say, on a dt parsing error
> bail out, too.

It seems like the best thing to do is defer if the error might resolve
itself but otherwise carry on with the reduced functionality from the
internal clocks.  I'd expect most errors to be permanently fatal though,
certainly a failure to parse the DT is unlikely to get fixed at runtime.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20131020/00801e71/attachment.sig>

[PATCH v3 2/2] ASoC: kirkwood: fix loss of external clock at probe time

[Mark Brown]

On Fri, Oct 18, 2013 at 08:35:55PM +0200, Jean-Francois Moine wrote:
> At probe time, when the clock driver is not yet initialized, the
> external clock of the kirkwood sound device will not be usable.

This doesn't apply against -next, could you please check and resend?

  git://git.kernel.org/pub/scm/linux/kernel/git/broone/sound.git topic/kirkwood

or the for-next branch.

> It also removes the test about same internal and external clocks which
> can never occur.

It seems like reasonable defensiveness to have the test there in case
someone has a broken DT, though it'd be better to complain about the DT.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20131020/3521b125/attachment.sig>

[PATCH v3 2/2] ASoC: kirkwood: fix loss of external clock at probe time

[Jean-Francois Moine]

On Sun, 20 Oct 2013 17:38:19 +0100
Mark Brown <broonie@kernel.org> wrote:

> On Fri, Oct 18, 2013 at 08:35:55PM +0200, Jean-Francois Moine wrote:
> > At probe time, when the clock driver is not yet initialized, the
> > external clock of the kirkwood sound device will not be usable.  
> 
> This doesn't apply against -next, could you please check and resend?
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/broone/sound.git topic/kirkwood
> 
> or the for-next branch.

Not easy to find :)

> > It also removes the test about same internal and external clocks which
> > can never occur.
> 
> It seems like reasonable defensiveness to have the test there in case
> someone has a broken DT, though it'd be better to complain about the DT.

The removed test run into:

	clocks = <&si5351 2>, <&si5351 2>;
	clock-names = "internal", "extclk";

where it seems that the user really wanted to make a mistake.
Then, the actual code just wants the si5351 2 to work as the internal
clock, and that cannot be.

Also, about errors, you may see that a `normal` error as:

	clocks = <&si5351 2>, <&gate_clk 13>;
	clock-names = "internal", "extclk";

i.e. inversion of the internal and external clock phandle's, cannot be
detected...

So, it is better to remove the useless test.

-- 
Ken ar c'henta?	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/