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

login
register
mail settings
Submitter Jean-Francois Moine
Date Oct. 18, 2013, 6:35 p.m.
Message ID <20131018203555.4d974251@armhf>
Download mbox | patch
Permalink /patch/284695/
State New
Headers show

Comments

Jean-Francois Moine - Oct. 18, 2013, 6:35 p.m.
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(-)
Uwe Kleine-König - Oct. 18, 2013, 7:12 p.m.
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
Jean-Francois Moine - Oct. 19, 2013, 8:28 a.m.
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...
Uwe Kleine-König - Oct. 20, 2013, 8:03 a.m.
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
Mark Brown - Oct. 20, 2013, 4:33 p.m.
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.
Mark Brown - Oct. 20, 2013, 4:38 p.m.
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.
Jean-Francois Moine - Oct. 21, 2013, 7:24 a.m.
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.

Patch

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 */