From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jean-Francois Moine Subject: Re: [PATCH] ASoC: kirkwood: simplify clock handling Date: Tue, 24 Sep 2013 21:04:42 +0200 Message-ID: <20130924210442.6f617798@armhf> References: <1379951159-8294-1-git-send-email-u.kleine-koenig@pengutronix.de> <1380046355-7920-1-git-send-email-u.kleine-koenig@pengutronix.de> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Received: from smtp3-g21.free.fr (smtp3-g21.free.fr [212.27.42.3]) by alsa0.perex.cz (Postfix) with ESMTP id 99FFE261A2C for ; Tue, 24 Sep 2013 21:02:29 +0200 (CEST) In-Reply-To: <1380046355-7920-1-git-send-email-u.kleine-koenig@pengutronix.de> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: alsa-devel-bounces@alsa-project.org Sender: alsa-devel-bounces@alsa-project.org To: Uwe =?UTF-8?B?S2xlaW5lLUvDtm5pZw==?= Cc: Thomas Petazzoni , alsa-devel@alsa-project.org, Liam Girdwood , Mark Brown , kernel@pengutronix.de, Russell King , linux-arm-kernel@lists.infradead.org List-Id: alsa-devel@alsa-project.org T24gVHVlLCAyNCBTZXAgMjAxMyAyMDoxMjozNSArMDIwMApVd2UgS2xlaW5lLUvDtm5pZyA8dS5r bGVpbmUta29lbmlnQHBlbmd1dHJvbml4LmRlPiB3cm90ZToKCj4gVGhlcmUgaXMgbm8gbmVlZCB0 byBub3QgdXNlIGV4dGNsayBpZiBpdCBpcyBpZGVudGljYWwgdG8gdGhlIG1haW4gY2xrLgo+IFRo ZSBtYWluIG1vdGl2YXRpb24gZm9yIHRoaXMgcGF0Y2ggaXMgZHJvcHBpbmcgZGV2bV9jbGtfcHV0 IHdoaWNoIGlzCj4gdXNlZCBpbiBhIHdyb25nIHdheSBieSBhbGwgb3RoZXIgdXNlcnMuCj4gCj4g V2hpbGUgYXQgaXQgYWxzbyBleHRlbmQgdGhlIGNvbW1lbnRzLgo+IAo+IFNpZ25lZC1vZmYtYnk6 IFV3ZSBLbGVpbmUtS8O2bmlnIDx1LmtsZWluZS1rb2VuaWdAcGVuZ3V0cm9uaXguZGU+Cj4gLS0t Cj4gSGVsbG8sCj4gCj4gdGhlcmUgbWlnaHQgYmUgc29tZSBmdXJ0aGVyIG9wdGltaXNhdGlvbnMg cG9zc2libGUuIEkgb25seSBub3RlIHRoZW0KPiBoZXJlIGJlY2F1c2UgSSBkb24ndCBoYXZlIHRo ZSBoYXJkd2FyZSB0byB0ZXN0Ogo+IAo+ICAtIG9ubHkgZW5hYmxlIGV4dGNsayBpZiBpdCBhIGNs b2NrIHJhdGUgdXNlZCB0aGF0IG1ha2VzIHVzZSBvZiB0aGUKPiAgICBleHRlcm5hbCBjbG9jay4g Tm90IHN1cmUgaWYgdGhhdCB3b3JrczsgaGFyZHdhcmUgZG9jcyByZWFkaW5nCj4gICAgbmVjZXNz YXJ5Lgo+ICAtIG9ubHkgcHJvdmlkZSBleHRjbGsgaWYgaXQncyAhPSB0aGUgaW50ZXJuYWwgY2xv Y2suCj4gIC0gVGhlIGNvZGUgdXNlczoKPiAKPiAJcHJpdi0+Y2xrID0gZGV2bV9jbGtfZ2V0KCZw ZGV2LT5kZXYsIG5wID8gImludGVybmFsIiA6IE5VTEwpOwo+IAo+ICAgIGkuZS4gcHJvdmlkZXMg YSBjb25faWQgaW4gdGhlIGR0LWNhc2UuIEkgdGhpbmsgdGhhdCB1c2luZyBOVUxMCj4gICAgdW5j b25kaXRpb25hbGx5IHNob3VsZCBhbHNvIHdvcmssIGkuZS4gcmV0dXJuIHRoZSBmaXJzdCBjbGsK PiAgICBhc3NvY2lhdGVkIHRvIHRoZSBkZXZpY2UuIE9UT0ggdGhlIGN1cnJlbnQgY29kZSBtaWdo dCBtYWtlIHRoaW5ncwo+ICAgIGNsZWFyZXIgYmVjYXVzZSBpdCdzIG1vcmUgZXhwbGljaXQuCglb c25pcF0KClV3ZSwKClRoZSBjb2RlIGFyb3VuZCBsaW5lIDEwNCBpbiBraXJrd29vZC1pMnMuYyBp cyBub3Qgd2hhdCBpdCBzaG91bGQgYmUKKHRoZSBwYXRjaCBmcm9tIFJ1c3NlbGwgaXMgbG9zdCBz b21ld2hlcmUgaW4gdGhlIG1haWxpbmctbGlzdCkuCkluc3RlYWQgb2Y6CgoJaWYgKHJhdGUgPT0g NDQxMDAgfHwgcmF0ZSA9PSA0ODAwMCB8fCByYXRlID09IDk2MDAwKSB7CgkJLyogdXNlIGludGVy bmFsIGRjbyBmb3IgdGhlIHN1cHBvcnRlZCByYXRlcwoJCSAqIGRlZmluZWQgaW4ga2lya3dvb2Rf aTJzX2RhaSAqLwoKaXQgc2hvdWxkIGJlOgoKCWlmIChJU19FUlIocHJpdi0+ZXh0Y2xrKSkgewkv KiBubyBleHRlcm5hbCBjbG9jayAqLwoJCS8qIHVzZSBpbnRlcm5hbCBkY28gLSB0aGUgc3VwcG9y dGVkIHJhdGVzIGFyZQoJCSAqIGRlZmluZWQgaW4ga2lya3dvb2RfaTJzX2RhaSAqLwoKVGhhdCBp czogaWYgdGhlcmUgaXMgYW4gZXh0ZXJuYWwgY2xvY2ssIHVzZSBpdC4KCkluIGZhY3QsIHRoZSBp bnRlcm5hbCBkY28gaXMgdXNlZCBmb3IgdHdvIGF1ZGlvIGRldmljZXMuIFdoZW4gYm90aApkZXZp Y2VzIGFyZSB1c2VkIGF0IHRoZSBzYW1lIHRpbWUsIGF0IGxlYXN0IG9uZSBvZiB0aGVtIG11c3Qg YWx3YXlzIHVzZQphbiBleHRlcm5hbCBjbG9jaywgb3RoZXJ3aXNlLCB0aGVyZSBpcyBhIGNsb2Nr IHJhdGUgY29uZmxpY3QuCgpBcyBvbmx5IG9uZSBjbG9jayBpcyB1c2VkLCB0aGVyZSBpcyBubyBu ZWVkIHRvIGRlY2xhcmUgMiBjbG9ja3MgaW4gdGhlCkRULCBidXQgdGhlIGRyaXZlciBtdXN0IGtu b3cgaWYgaXQgdXNlcyB0aGUgaW50ZXJuYWwgb3IgZXh0ZXJuYWwgY2xvY2sKKHRvIHNldCB0aGUg cmlnaHQgY2xvY2sgaW5wdXQgYW5kIGFsc28gYmVjYXVzZSB0aGVpciByYXRlcyBhcmUgbm90IHNl dAp0aGUgc2FtZSB3YXkpCgpTbywgdGhlIHByb2JlIGNvZGUgc2hvdWxkIGJlOgoKCS8qIGNoZWNr IGZpcnN0IGlmIGFuIGV4dGVybmFsIGNsb2NrIGlzIGRlY2xhcmVkICovCglwcml2LT5leHRjbGsg PSBkZXZtX2Nsa19nZXQoJnBkZXYtPmRldiwgImV4dGNsayIpOwoJaWYgKCFJU19FUlIocHJpdi0+ ZXh0Y2xrKSkgewoJCS4uLiB1c2UgdGhlIGV4dGVybmFsIGNsb2NrIC4uLgoJfSBlbHNlIHsKCgkJ LyogZ2V0IHRoZSBmaXJzdCBjbG9jayB3aGljaCBtdXN0IGJlIHRoZSBkY28gKi8KCQlwcml2LT5j bGsgPSBkZXZtX2Nsa19nZXQoJnBkZXYtPmRldiwgTlVMTCk7CgkJaWYgKElTX0VSUihwcml2LT5j bGspKQoJCQkuLiBlcnJvciwgbm8gY2xvY2sgLi4KCQkuLiB1c2UgdGhlIGludGVybmFsIGRjbyAu Li4KCX0KCi0tIApLZW4gYXIgYydoZW50YcOxCXwJICAgICAgKiogQnJlaXpoIGhhIExpbnV4IGF0 YXYhICoqCkplZgkJfAkJaHR0cDovL21vaW5lamYuZnJlZS5mci8KX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fX18KQWxzYS1kZXZlbCBtYWlsaW5nIGxpc3QKQWxz YS1kZXZlbEBhbHNhLXByb2plY3Qub3JnCmh0dHA6Ly9tYWlsbWFuLmFsc2EtcHJvamVjdC5vcmcv bWFpbG1hbi9saXN0aW5mby9hbHNhLWRldmVsCg== From mboxrd@z Thu Jan 1 00:00:00 1970 From: moinejf@free.fr (Jean-Francois Moine) Date: Tue, 24 Sep 2013 21:04:42 +0200 Subject: [PATCH] ASoC: kirkwood: simplify clock handling In-Reply-To: <1380046355-7920-1-git-send-email-u.kleine-koenig@pengutronix.de> References: <1379951159-8294-1-git-send-email-u.kleine-koenig@pengutronix.de> <1380046355-7920-1-git-send-email-u.kleine-koenig@pengutronix.de> Message-ID: <20130924210442.6f617798@armhf> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Tue, 24 Sep 2013 20:12:35 +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. > > While at it also extend the comments. > > Signed-off-by: Uwe Kleine-K?nig > --- > 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/