| Message ID | 1380046355-7920-1-git-send-email-u.kleine-koenig@pengutronix.de |
|---|---|
| State | New |
| Headers | show |
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.
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 ... }
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.
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
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; } }
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(-)