| Message ID | 20131018203555.4d974251@armhf |
|---|---|
| State | New |
| Headers | show |
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
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...
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
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.
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.
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.
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 */
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(-)