From mboxrd@z Thu Jan 1 00:00:00 1970 From: Amit Kucheria Subject: Re: [PATCH v4 3/6] clk: introduce the common clock framework Date: Thu, 12 Jan 2012 15:13:01 +0200 Message-ID: <20120112131301.GA3478@matterhorn1> References: <1323834838-2206-1-git-send-email-mturquette@linaro.org> <1323834838-2206-4-git-send-email-mturquette@linaro.org> <20120104021541.GI2414@b20223-02.ap.freescale.net> <4F0462FF.1000308@gmail.com> <4F0506D0.30109@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Content-Disposition: inline In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: linaro-dev-bounces-cunTk1MwBs8s++Sfvej+rw@public.gmane.org Errors-To: linaro-dev-bounces-cunTk1MwBs8s++Sfvej+rw@public.gmane.org To: "Turquette, Mike" , Thomas Gleixner Cc: andrew-g2DYL2Zd6BY@public.gmane.org, linaro-dev-cunTk1MwBs8s++Sfvej+rw@public.gmane.org, eric.miao-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org, jeremy.kerr-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org, linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org, sboyd-jfJNa2p1gH1BDgjK7y7TUQ@public.gmane.org, magnus.damm-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org, arnd.bergmann-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org, patches-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org, linux-omap-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, paul-DWxLp4Yu+b8AvxtiuMwx3w@public.gmane.org, linus.walleij-0IS4wlFg1OjSUeElwK9/Pw@public.gmane.org, broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Colin Cross , Richard Zhao , skannan-jfJNa2p1gH1BDgjK7y7TUQ@public.gmane.org List-Id: linux-omap@vger.kernel.org T24gMTIgSmFuIDA0LCBUdXJxdWV0dGUsIE1pa2Ugd3JvdGU6Cj4gT24gV2VkLCBKYW4gNCwgMjAx MiBhdCA2OjExIFBNLCBSb2IgSGVycmluZyA8cm9iaGVycmluZzJAZ21haWwuY29tPiB3cm90ZToK PiA+IE9uIDAxLzA0LzIwMTIgMDc6MDEgUE0sIFR1cnF1ZXR0ZSwgTWlrZSB3cm90ZToKPiA+PiBP biBXZWQsIEphbiA0LCAyMDEyIGF0IDY6MzIgQU0sIFJvYiBIZXJyaW5nIDxyb2JoZXJyaW5nMkBn bWFpbC5jb20+IHdyb3RlOgo+ID4+PiBPbiAwMS8wMy8yMDEyIDA4OjE1IFBNLCBSaWNoYXJkIFpo YW8gd3JvdGU6Cj4gPj4+PiBPbiBGcmksIERlYyAxNiwgMjAxMSBhdCAwNDo0NTo0OFBNIC0wODAw LCBUdXJxdWV0dGUsIE1pa2Ugd3JvdGU6Cj4gPj4+Pj4gT24gV2VkLCBEZWMgMTQsIDIwMTEgYXQg NToxOCBBTSwgVGhvbWFzIEdsZWl4bmVyIDx0Z2x4QGxpbnV0cm9uaXguZGU+IHdyb3RlOgo+ID4+ Pj4+PiBPbiBUdWUsIDEzIERlYyAyMDExLCBNaWtlIFR1cnF1ZXR0ZSB3cm90ZToKPiA+Pj4KPiA+ Pj4gc25pcAo+ID4+Pgo+ID4+Pj4+Pj4gKy8qKgo+ID4+Pj4+Pj4gKyAqIGNsa19pbml0IC0gaW5p dGlhbGl6ZSB0aGUgZGF0YSBzdHJ1Y3R1cmVzIGluIGEgc3RydWN0IGNsawo+ID4+Pj4+Pj4gKyAq IEBkZXY6IGRldmljZSBpbml0aWFsaXppbmcgdGhpcyBjbGssIHBsYWNlaG9sZGVyIGZvciBub3cK PiA+Pj4+Pj4+ICsgKiBAY2xrOiBjbGsgYmVpbmcgaW5pdGlhbGl6ZWQKPiA+Pj4+Pj4+ICsgKgo+ ID4+Pj4+Pj4gKyAqIEluaXRpYWxpemVzIHRoZSBsaXN0cyBpbiBzdHJ1Y3QgY2xrLCBxdWVyaWVz IHRoZSBoYXJkd2FyZSBmb3IgdGhlCj4gPj4+Pj4+PiArICogcGFyZW50IGFuZCByYXRlIGFuZCBz ZXRzIHRoZW0gYm90aC4gwqBBZGRzIHRoZSBjbGsgdG8gdGhlIHN5c2ZzIHRyZWUKPiA+Pj4+Pj4+ ICsgKiB0b3BvbG9neS4KPiA+Pj4+Pj4+ICsgKgo+ID4+Pj4+Pj4gKyAqIENhbGxlciBtdXN0IHBv cHVsYXRlIGNsay0+bmFtZSBhbmQgY2xrLT5mbGFncyBiZWZvcmUgY2FsbGluZwo+ID4+Pj4+Pgo+ ID4+Pj4+PiBJJ20gbm90IHRvbyBoYXBweSBhYm91dCB0aGlzIGNvbnN0cnVjdC4gVGhhdCBsZWF2 ZXMgc3RydWN0IGNsayBhbmQgaXRzCj4gPj4+Pj4+IG1lbWJlcnMgZXhwb3NlZCB0byB0aGUgd29y bGQgaW5zdGVhZCBvZiBtYWtpbmcgaXQgYSByZWFsIG9wYXF1ZQo+ID4+Pj4+PiBjb29raWUuIEkg a25vdyBmcm9tIG15IG93biBwYWluZnVsIGV4cGVyaWVuY2UsIHRoYXQgdGhpcyB3aWxsIGxlYWQg dG8KPiA+Pj4+Pj4gcmFuZG9tIGZpZGRsaW5nIGluIHRoYXQgZGF0YSBzdHJ1Y3R1cmUgaW4gZHJp dmVycyBhbmQgYXJjaCBjb2RlIGp1c3QKPiA+Pj4+Pj4gYmVjYXVzZSB0aGUgY29yZSBjb2RlIGhh cyBhIHNob3J0Y29taW5nLgo+ID4+Pj4+Pgo+ID4+Pj4+PiBXaHkgY2FuJ3Qgd2UgbWFrZSBzdHJ1 Y3QgY2xrIGEgcmVhbCBjb29raWUgYW5kIGNvbmZpbmUgdGhlIGRhdGEKPiA+Pj4+Pj4gc3RydWN0 dXJlIHRvIHRoZSBjb3JlIGNvZGUgPwo+ID4+Pj4+Pgo+ID4+Pj4+PiBUaGF0IHdvdWxkIGNoYW5n ZSB0aGUgaW5pdCBjYWxsIHRvIHNvbWV0aGluZyBsaWtlOgo+ID4+Pj4+Pgo+ID4+Pj4+PiBzdHJ1 Y3QgY2xrICpjbGtfaW5pdChzdHJ1Y3QgZGV2aWNlICpkZXYsIGNvbnN0IHN0cnVjdCBjbGtfaHcg Kmh3LAo+ID4+Pj4+PiDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCBzdHJ1Y3QgY2xrICpw YXJlbnQpCj4gPj4+Pj4+Cj4gPj4+Pj4+IEFuZCBoYXZlOgo+ID4+Pj4+PiBzdHJ1Y3QgY2xrX2h3 IHsKPiA+Pj4+Pj4gwqAgwqAgwqAgc3RydWN0IGNsa19od19vcHMgKm9wczsKPiA+Pj4+Pj4gwqAg wqAgwqAgY29uc3QgY2hhciDCoCDCoCDCoCDCoCpuYW1lOwo+ID4+Pj4+PiDCoCDCoCDCoCB1bnNp Z25lZCBsb25nIMKgIMKgIGZsYWdzOwo+ID4+Pj4+PiB9Owo+ID4+Pj4+Pgo+ID4+Pj4+PiBJbXBs ZW1lbnRlcnMgY2FuIGRvOgo+ID4+Pj4+PiBzdHJ1Y3QgbXlfY2xrX2h3IHsKPiA+Pj4+Pj4gwqAg wqAgwqAgc3RydWN0IGNsa19odyDCoCDCoGh3Owo+ID4+Pj4+PiDCoCDCoCDCoCBteWRhdGE7Cj4g Pj4+Pj4+IH07Cj4gPj4+Pj4+Cj4gPj4+Pj4+IEFuZCB0aGVuIGNoYW5nZSB0aGUgY2xrIG9wcyBj YWxsYmFja3MgdG8gdGFrZSBzdHJ1Y3QgY2xrX2h3ICogYXMgYW4KPiA+Pj4+Pj4gYXJndW1lbnQu Cj4gPj4+PiBXZSBoYXZlIHRvIGRlZmluZSBzdGF0aWMgY2xvY2tzIGJlZm9yZSB3ZSBhZG9wdCBE VCBiaW5kaW5nLgo+ID4+Pj4gSWYgY2xrIGlzIG9wYXF1ZSBhbmQgYWxsb2NhdGUgbWVtb3J5IGlu IGNsayBjb3JlLCBpdCdsbCBtYWtlIGhhcmQKPiA+Pj4+IHRvIGRlZmluZSBzdGF0aWMgY2xvY2tz LiBBbmQgcmVnaXN0ZXIvaW5pdCB3aWxsIHBhc3MgYSBsb25nIHBhcmFtZXRlcgo+ID4+Pj4gbGlz dC4KPiA+Pj4KPiA+Pj4gRFQgaXMgbm90IGEgcHJlcmVxdWlzaXRlIGZvciBoYXZpbmcgZHluYW1p Y2FsbHkgY3JlYXRlZCBjbG9ja3MuIFlvdSBjYW4KPiA+Pj4gbWFrZSBjbG9jayBpbml0IGR5bmFt aWMgd2l0aG91dCBEVC4KPiA+Pgo+ID4+IEFncmVlZC4KPiA+Pgo+ID4+PiBXaGF0IGRhdGEgZ29l cyBpbiBzdHJ1Y3QgY2xrIHZzLiBzdHJ1Y3QgY2xrX2h3IGNvdWxkIGNoYW5nZSBvdmVyIHRpbWUu Cj4gPj4+IFNvIHBlcmhhcHMgd2UgY2FuIHN0YXJ0IHdpdGggc29tZSBkYXRhIGluIGNsa19odyBh bmQgcGxhbiB0byBtb3ZlIGl0IHRvCj4gPj4+IHN0cnVjdCBjbGsgbGF0ZXIuIEV2ZW4gaWYgYWxt b3N0IGV2ZXJ5dGhpbmcgZW5kcyB1cCBpbiBjbGtfaHcgaW5pdGlhbGx5LAo+ID4+PiBhdCBsZWFz dCB0aGUgc3RydWN0dXJlIGlzIGluIHBsYWNlIHRvIGhhdmUgY29tbW9uLCBjb3JlLW9ubHkgZGF0 YQo+ID4+PiBzZXBhcmF0ZSBmcm9tIHBsYXRmb3JtIGRhdGEuCj4gPj4KPiA+PiBXaGF0IGlzIHRo ZSBwb2ludCBvZiB0aGlzPwo+ID4KPiA+IFRvIGhhdmUgYSB3YXkgZm9yd2FyZC4gSXQgd291bGQg YmUgbmljZSB0byBoYXZlIGEgY2xrIGluZnJhc3RydWN0dXJlCj4gPiBiZWZvcmUgSSByZXRpcmUu Li4KPiAKPiBIYWhhLCBhZ3JlZWQuCj4gCj4gPgo+ID4+IFRoZSBvcmlnaW5hbCBjbGtfaHcgd2Fz IGRlZmluZWQgc2ltcGx5IGFzOgo+ID4+Cj4gPj4gc3RydWN0IGNsa19odyB7Cj4gPj4gwqAgwqAg wqAgwqAgc3RydWN0IGNsayAqY2xrOwo+ID4+IH07Cj4gPj4KPiA+PiBJdCdzIG9ubHkgcHVycG9z ZSBpbiBsaWZlIHdhcyBhcyBhIGhhbmRsZSBmb3IgbmF2aWdhdGlvbiBiZXR3ZWVuIHRoZQo+ID4+ IG9wYXF1ZSBzdHJ1Y3QgY2xrIGFuZCB0aGUgaGFyZHdhcmUtc3BlY2lmaWMgc3RydWN0IG15X2Ns a19ody4gwqBzdHJ1Y3QKPiA+PiBjbGtfaHcgaXMgZGVmaW5lZCBpbiBjbGsuaCBhbmQgZXZlcnlv bmUgY2FuIHNlZSBpdC4gwqBJZiB3ZSdyZSBzdWRkZW5seQo+ID4+IE9LIHB1dHRpbmcgY2xrIGRh dGEgaW4gdGhpcyBzdHJ1Y3R1cmUgdGhlbiB3aHkgYm90aGVyIHdpdGggYW4gb3BhcXVlCj4gPj4g c3RydWN0IGNsayBhdCBhbGw/Cj4gPj4KPiA+Pj4gV2hhdCBpcyB0aGUgYWN0dWFsIGRhdGEgeW91 IG5lZWQgdG8gYmUgc3RhdGljIGFuZCBhY2Nlc3NpYmxlIHRvIHRoZQo+ID4+PiBwbGF0Zm9ybSBj b2RlPyBBIHB0ciB0byBwYXJlbnQgY2xrIGlzIHRoZSBtYWluIHRoaW5nIEkndmUgc2VlbiBmb3IK PiA+Pj4gc3RhdGljIGluaXRpYWxpemF0aW9uLiBTbyBtYWtlIHRoZSBwYXJlbnQgcHRyIGJlIHN0 cnVjdCBjbGtfaHcqIGFuZAo+ID4+PiBhbGxvdyB0aGUgcGxhdGZvcm1zIHRvIGFjY2Vzcy4KPiA+ Pgo+ID4+IFRvIGFuc3dlciB5b3VyIHF1ZXN0aW9uIG9uIHdoYXQgZGF0YSB3ZSdyZSB0cnlpbmcg dG8gZXhwb3NlOiBwbGF0Zm9ybQo+ID4+IGNvZGUgY29tbW9ubHkgbmVlZHMgdGhlIHBhcmVudCBw b2ludGVyIGFuZCB0aGUgY2xrIHJhdGUgKGFuZCBieQo+ID4+IGV4dGVuc2lvbiwgdGhlIHJhdGUg b2YgdGhlIHBhcmVudCkuIMKgRm9yIGRlYnVnL2Vycm9yIHByaW50cyBpdCBpcyBhbHNvCj4gPj4g bmljZSB0byBoYXZlIHRoZSBjbGsgbmFtZS4gwqBHZW5lcmljIGNsayBmbGFncyBhcmUgYWxzbyBj b25jZWl2YWJseQo+ID4+IHNvbWV0aGluZyB0aGF0IHBsYXRmb3JtIGNvZGUgbWlnaHQgd2FudC4K PiA+Cj4gPiBJIGFncmVlIHdpdGggdGhlIG5lZWQgdG8gaGF2ZSB0aGUgcGFyZW50IGFuZCBmbGFn cyBmcm9tIGEgc3RhdGljIGluaXQKPiA+IHBlcnNwZWN0aXZlLiBUaGVyZSdzIG5vdCByZWFsbHkg YSBnb29kIHJlYXNvbiB0aGUgb3RoZXJzIGNhbid0IGJlCj4gPiBhY2Nlc3NlZCB0aHJ1IGFjY2Vz c29ycyB0aG91Z2guCj4gPgo+ID4+IEknZCBsaWtlIHRvIHNwaW4gdGhlIHF1ZXN0aW9uIGFyb3Vu ZDogaWYgd2UncmUgT0sgZXhwb3Npbmcgc29tZSBzdHVmZgo+ID4+IChpbiB5b3VyIGV4YW1wbGUg YWJvdmUsIHRoZSBwYXJlbnQgcG9pbnRlciksIHRoZW4gd2hhdCBjbGsgZGF0YSBhcmUKPiA+PiB5 b3UgdHJ5aW5nIHRvIGhpZGU/Cj4gPgo+ID4gV2VsbCwgZXZlcnl0aGluZyBmcm9tIGRyaXZlcnMg d2hpY2ggdGhlIGN1cnJlbnQgY2xrIGltcGxlbWVudGF0aW9ucyBkbwo+ID4gaGlkZS4gQ2F0Y2hp bmcgYWJ1c2UgaW4gd2l0aCBkcml2ZXJzIGNvbWluZyBpbiBmcm9tIGFsbCBkaWZmZXJlbnQgdHJl ZXMKPiA+IGFuZCBsaXN0cyB3aWxsIGJlIGltcG9zc2libGUuCj4gPgo+ID4gRm9yIHBsYXRmb3Jt IGNvZGUgaXQgaXMgbW9yZSBmdXp6eS4gSSBkb24ndCB0aGluayBwbGF0Zm9ybSBjb2RlIHNob3Vs ZAo+ID4gYmUgYWxsb3dlZCB0byBtdWNrIHdpdGggcHJlcGFyZS9lbmFibGUgY291bnRzIGZvciBl eGFtcGxlLgo+IAo+IFNvIHRoZXJlIGlzIGEgY2xlYXIgZGljaG90b215OiBkcml2ZXJzIHNob3Vs ZG4ndCBiZSBleHBvc2VkIHRvIGFueSBvZgo+IGl0IGFuZCBwbGF0Zm9ybSBjb2RlIHNob3VsZCBi ZSBleHBvc2VkIHRvIHNvbWUgb2YgaXQuCj4gCj4gSG93IGFib3V0IGEgZHJpdmVycy9jbGsvY2xr LXByaXZhdGUuaCB3aGljaCB3aWxsIGRlZmluZSBzdHJ1Y3QgY2xrIGFuZAo+IG11c3Qgb25seSBi ZSBpbmNsdWRlZCBieSBjbGsgZHJpdmVycyBpbiBkcml2ZXJzL2Nsay8qPwo+IAo+IFRoaXMgZXN0 YWJsaXNoZXMgYSBicmlnaHQgbGluZSBiZXR3ZWVuIHRob3NlIHRoaW5ncyB3aGljaCBhcmUgYWxs b3dlZAo+IHRvIGtub3cgdGhlIGRldGFpbHMgb2Ygc3RydWN0IGNsayBhbmQgdGhvc2UgdGhhdCBh cmUgbm90OiBuYW1lbHkgdGhhdAo+IGNsayBkcml2ZXJzIGluIGRyaXZlcnMvY2xrLyBtYXkgdXNl ICcjaW5jbHVkZSAiY2xrLXByaXZhdGUuaCInLgo+IE9idmlvdXNseSBzdHJ1Y3QgY2xrIGlzIG9w YXF1ZSB0byB0aGUgcmVzdCBvZiB0aGUga2VybmVsIChpbiB0aGUgc2FtZQo+IHdheSBpdCBoYXMg YmVlbiBwcmlvciB0byB0aGUgY29tbW9uIGNsayBwYXRjaGVzKSBhbmQgdGhlcmUgaXMgbm8gbmVl ZAo+IGZvciBzdHJ1Y3QgY2xrX2h3IGFueW1vcmUuICBBbHNvIGhlbHBlciBmdW5jdGlvbnMgYXJl IG5vIGxvbmdlciBuZWVkZWQKPiBmb3IgY2xvY2sgZHJpdmVyIGNvZGUsIHdoaWNoIEkgdGhpbmsg KmlzKiBhIG1hbmFnZWFibGUgc2V0IG9mIGNvZGUgdG8KPiByZXZpZXcuICBBbHNvIGNsayBkcml2 ZXJzIG11c3QgbGl2ZSBpbiBkcml2ZXJzL2Nsay8gZm9yIHRoaXMgdG8gd29yawo+ICh3aXRob3V0 IGEgYmlnIHVnbHkgcGF0aCBpbiBzb21lb25lJ3MgI2luY2x1ZGUgZGlyZWN0aXZlIHNvbWV3aGVy ZSkuCj4gCj4gVGhvdWdodHM/Cj4gCj4gUmVnYXJkcywKPiBNaWtlCgpUaG9tYXM/IAoKV2UncmUg c3R1Y2sgb24gdGhpcyBmdW5kYW1lbnRhbCBwb2ludCBmb3IgYSB3aGlsZSBub3cuIEFuZCB2NSBv ZiB0aGUKcGF0Y2hzZXQgZG9lc24ndCBtYWtlIG11Y2ggc2Vuc2Ugd2l0aG91dCByZXNvbHZpbmcg aXQuCgovQW1pdAoKX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f X18KbGluYXJvLWRldiBtYWlsaW5nIGxpc3QKbGluYXJvLWRldkBsaXN0cy5saW5hcm8ub3JnCmh0 dHA6Ly9saXN0cy5saW5hcm8ub3JnL21haWxtYW4vbGlzdGluZm8vbGluYXJvLWRldgo= From mboxrd@z Thu Jan 1 00:00:00 1970 From: amit.kucheria@linaro.org (Amit Kucheria) Date: Thu, 12 Jan 2012 15:13:01 +0200 Subject: [PATCH v4 3/6] clk: introduce the common clock framework In-Reply-To: References: <1323834838-2206-1-git-send-email-mturquette@linaro.org> <1323834838-2206-4-git-send-email-mturquette@linaro.org> <20120104021541.GI2414@b20223-02.ap.freescale.net> <4F0462FF.1000308@gmail.com> <4F0506D0.30109@gmail.com> Message-ID: <20120112131301.GA3478@matterhorn1> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 12 Jan 04, Turquette, Mike wrote: > On Wed, Jan 4, 2012 at 6:11 PM, Rob Herring wrote: > > On 01/04/2012 07:01 PM, Turquette, Mike wrote: > >> On Wed, Jan 4, 2012 at 6:32 AM, Rob Herring wrote: > >>> On 01/03/2012 08:15 PM, Richard Zhao wrote: > >>>> On Fri, Dec 16, 2011 at 04:45:48PM -0800, Turquette, Mike wrote: > >>>>> On Wed, Dec 14, 2011 at 5:18 AM, Thomas Gleixner wrote: > >>>>>> On Tue, 13 Dec 2011, Mike Turquette wrote: > >>> > >>> snip > >>> > >>>>>>> +/** > >>>>>>> + * clk_init - initialize the data structures in a struct clk > >>>>>>> + * @dev: device initializing this clk, placeholder for now > >>>>>>> + * @clk: clk being initialized > >>>>>>> + * > >>>>>>> + * Initializes the lists in struct clk, queries the hardware for the > >>>>>>> + * parent and rate and sets them both. ?Adds the clk to the sysfs tree > >>>>>>> + * topology. > >>>>>>> + * > >>>>>>> + * Caller must populate clk->name and clk->flags before calling > >>>>>> > >>>>>> I'm not too happy about this construct. That leaves struct clk and its > >>>>>> members exposed to the world instead of making it a real opaque > >>>>>> cookie. I know from my own painful experience, that this will lead to > >>>>>> random fiddling in that data structure in drivers and arch code just > >>>>>> because the core code has a shortcoming. > >>>>>> > >>>>>> Why can't we make struct clk a real cookie and confine the data > >>>>>> structure to the core code ? > >>>>>> > >>>>>> That would change the init call to something like: > >>>>>> > >>>>>> struct clk *clk_init(struct device *dev, const struct clk_hw *hw, > >>>>>> ? ? ? ? ? ? ? ? ? ? struct clk *parent) > >>>>>> > >>>>>> And have: > >>>>>> struct clk_hw { > >>>>>> ? ? ? struct clk_hw_ops *ops; > >>>>>> ? ? ? const char ? ? ? ?*name; > >>>>>> ? ? ? unsigned long ? ? flags; > >>>>>> }; > >>>>>> > >>>>>> Implementers can do: > >>>>>> struct my_clk_hw { > >>>>>> ? ? ? struct clk_hw ? ?hw; > >>>>>> ? ? ? mydata; > >>>>>> }; > >>>>>> > >>>>>> And then change the clk ops callbacks to take struct clk_hw * as an > >>>>>> argument. > >>>> We have to define static clocks before we adopt DT binding. > >>>> If clk is opaque and allocate memory in clk core, it'll make hard > >>>> to define static clocks. And register/init will pass a long parameter > >>>> list. > >>> > >>> DT is not a prerequisite for having dynamically created clocks. You can > >>> make clock init dynamic without DT. > >> > >> Agreed. > >> > >>> What data goes in struct clk vs. struct clk_hw could change over time. > >>> So perhaps we can start with some data in clk_hw and plan to move it to > >>> struct clk later. Even if almost everything ends up in clk_hw initially, > >>> at least the structure is in place to have common, core-only data > >>> separate from platform data. > >> > >> What is the point of this? > > > > To have a way forward. It would be nice to have a clk infrastructure > > before I retire... > > Haha, agreed. > > > > >> The original clk_hw was defined simply as: > >> > >> struct clk_hw { > >> ? ? ? ? struct clk *clk; > >> }; > >> > >> It's only purpose in life was as a handle for navigation between the > >> opaque struct clk and the hardware-specific struct my_clk_hw. ?struct > >> clk_hw is defined in clk.h and everyone can see it. ?If we're suddenly > >> OK putting clk data in this structure then why bother with an opaque > >> struct clk at all? > >> > >>> What is the actual data you need to be static and accessible to the > >>> platform code? A ptr to parent clk is the main thing I've seen for > >>> static initialization. So make the parent ptr be struct clk_hw* and > >>> allow the platforms to access. > >> > >> To answer your question on what data we're trying to expose: platform > >> code commonly needs the parent pointer and the clk rate (and by > >> extension, the rate of the parent). ?For debug/error prints it is also > >> nice to have the clk name. ?Generic clk flags are also conceivably > >> something that platform code might want. > > > > I agree with the need to have the parent and flags from a static init > > perspective. There's not really a good reason the others can't be > > accessed thru accessors though. > > > >> I'd like to spin the question around: if we're OK exposing some stuff > >> (in your example above, the parent pointer), then what clk data are > >> you trying to hide? > > > > Well, everything from drivers which the current clk implementations do > > hide. Catching abuse in with drivers coming in from all different trees > > and lists will be impossible. > > > > For platform code it is more fuzzy. I don't think platform code should > > be allowed to muck with prepare/enable counts for example. > > So there is a clear dichotomy: drivers shouldn't be exposed to any of > it and platform code should be exposed to some of it. > > How about a drivers/clk/clk-private.h which will define struct clk and > must only be included by clk drivers in drivers/clk/*? > > This establishes a bright line between those things which are allowed > to know the details of struct clk and those that are not: namely that > clk drivers in drivers/clk/ may use '#include "clk-private.h"'. > Obviously struct clk is opaque to the rest of the kernel (in the same > way it has been prior to the common clk patches) and there is no need > for struct clk_hw anymore. Also helper functions are no longer needed > for clock driver code, which I think *is* a manageable set of code to > review. Also clk drivers must live in drivers/clk/ for this to work > (without a big ugly path in someone's #include directive somewhere). > > Thoughts? > > Regards, > Mike Thomas? We're stuck on this fundamental point for a while now. And v5 of the patchset doesn't make much sense without resolving it. /Amit From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753552Ab2ALNNZ (ORCPT ); Thu, 12 Jan 2012 08:13:25 -0500 Received: from mail-lpp01m010-f46.google.com ([209.85.215.46]:38104 "EHLO mail-lpp01m010-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752992Ab2ALNNI (ORCPT ); Thu, 12 Jan 2012 08:13:08 -0500 Date: Thu, 12 Jan 2012 15:13:01 +0200 From: Amit Kucheria To: "Turquette, Mike" , Thomas Gleixner Cc: Rob Herring , Richard Zhao , andrew@lunn.ch, linaro-dev@lists.linaro.org, eric.miao@linaro.org, grant.likely@secretlab.ca, Colin Cross , jeremy.kerr@canonical.com, linux@arm.linux.org.uk, sboyd@quicinc.com, magnus.damm@gmail.com, dsaxena@linaro.org, linux-arm-kernel@lists.infradead.org, arnd.bergmann@linaro.org, patches@linaro.org, linux-omap@vger.kernel.org, richard.zhao@linaro.org, shawn.guo@freescale.com, paul@pwsan.com, linus.walleij@stericsson.com, broonie@opensource.wolfsonmicro.com, linux-kernel@vger.kernel.org, skannan@quicinc.com Subject: Re: [PATCH v4 3/6] clk: introduce the common clock framework Message-ID: <20120112131301.GA3478@matterhorn1> Mail-Followup-To: "Turquette, Mike" , Thomas Gleixner , Rob Herring , Richard Zhao , andrew@lunn.ch, linaro-dev@lists.linaro.org, eric.miao@linaro.org, grant.likely@secretlab.ca, Colin Cross , jeremy.kerr@canonical.com, linux@arm.linux.org.uk, sboyd@quicinc.com, magnus.damm@gmail.com, dsaxena@linaro.org, linux-arm-kernel@lists.infradead.org, arnd.bergmann@linaro.org, patches@linaro.org, linux-omap@vger.kernel.org, richard.zhao@linaro.org, shawn.guo@freescale.com, paul@pwsan.com, linus.walleij@stericsson.com, broonie@opensource.wolfsonmicro.com, linux-kernel@vger.kernel.org, skannan@quicinc.com References: <1323834838-2206-1-git-send-email-mturquette@linaro.org> <1323834838-2206-4-git-send-email-mturquette@linaro.org> <20120104021541.GI2414@b20223-02.ap.freescale.net> <4F0462FF.1000308@gmail.com> <4F0506D0.30109@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: X-URL: http://www.verdurent.com/ User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 12 Jan 04, Turquette, Mike wrote: > On Wed, Jan 4, 2012 at 6:11 PM, Rob Herring wrote: > > On 01/04/2012 07:01 PM, Turquette, Mike wrote: > >> On Wed, Jan 4, 2012 at 6:32 AM, Rob Herring wrote: > >>> On 01/03/2012 08:15 PM, Richard Zhao wrote: > >>>> On Fri, Dec 16, 2011 at 04:45:48PM -0800, Turquette, Mike wrote: > >>>>> On Wed, Dec 14, 2011 at 5:18 AM, Thomas Gleixner wrote: > >>>>>> On Tue, 13 Dec 2011, Mike Turquette wrote: > >>> > >>> snip > >>> > >>>>>>> +/** > >>>>>>> + * clk_init - initialize the data structures in a struct clk > >>>>>>> + * @dev: device initializing this clk, placeholder for now > >>>>>>> + * @clk: clk being initialized > >>>>>>> + * > >>>>>>> + * Initializes the lists in struct clk, queries the hardware for the > >>>>>>> + * parent and rate and sets them both.  Adds the clk to the sysfs tree > >>>>>>> + * topology. > >>>>>>> + * > >>>>>>> + * Caller must populate clk->name and clk->flags before calling > >>>>>> > >>>>>> I'm not too happy about this construct. That leaves struct clk and its > >>>>>> members exposed to the world instead of making it a real opaque > >>>>>> cookie. I know from my own painful experience, that this will lead to > >>>>>> random fiddling in that data structure in drivers and arch code just > >>>>>> because the core code has a shortcoming. > >>>>>> > >>>>>> Why can't we make struct clk a real cookie and confine the data > >>>>>> structure to the core code ? > >>>>>> > >>>>>> That would change the init call to something like: > >>>>>> > >>>>>> struct clk *clk_init(struct device *dev, const struct clk_hw *hw, > >>>>>>                     struct clk *parent) > >>>>>> > >>>>>> And have: > >>>>>> struct clk_hw { > >>>>>>       struct clk_hw_ops *ops; > >>>>>>       const char        *name; > >>>>>>       unsigned long     flags; > >>>>>> }; > >>>>>> > >>>>>> Implementers can do: > >>>>>> struct my_clk_hw { > >>>>>>       struct clk_hw    hw; > >>>>>>       mydata; > >>>>>> }; > >>>>>> > >>>>>> And then change the clk ops callbacks to take struct clk_hw * as an > >>>>>> argument. > >>>> We have to define static clocks before we adopt DT binding. > >>>> If clk is opaque and allocate memory in clk core, it'll make hard > >>>> to define static clocks. And register/init will pass a long parameter > >>>> list. > >>> > >>> DT is not a prerequisite for having dynamically created clocks. You can > >>> make clock init dynamic without DT. > >> > >> Agreed. > >> > >>> What data goes in struct clk vs. struct clk_hw could change over time. > >>> So perhaps we can start with some data in clk_hw and plan to move it to > >>> struct clk later. Even if almost everything ends up in clk_hw initially, > >>> at least the structure is in place to have common, core-only data > >>> separate from platform data. > >> > >> What is the point of this? > > > > To have a way forward. It would be nice to have a clk infrastructure > > before I retire... > > Haha, agreed. > > > > >> The original clk_hw was defined simply as: > >> > >> struct clk_hw { > >>         struct clk *clk; > >> }; > >> > >> It's only purpose in life was as a handle for navigation between the > >> opaque struct clk and the hardware-specific struct my_clk_hw.  struct > >> clk_hw is defined in clk.h and everyone can see it.  If we're suddenly > >> OK putting clk data in this structure then why bother with an opaque > >> struct clk at all? > >> > >>> What is the actual data you need to be static and accessible to the > >>> platform code? A ptr to parent clk is the main thing I've seen for > >>> static initialization. So make the parent ptr be struct clk_hw* and > >>> allow the platforms to access. > >> > >> To answer your question on what data we're trying to expose: platform > >> code commonly needs the parent pointer and the clk rate (and by > >> extension, the rate of the parent).  For debug/error prints it is also > >> nice to have the clk name.  Generic clk flags are also conceivably > >> something that platform code might want. > > > > I agree with the need to have the parent and flags from a static init > > perspective. There's not really a good reason the others can't be > > accessed thru accessors though. > > > >> I'd like to spin the question around: if we're OK exposing some stuff > >> (in your example above, the parent pointer), then what clk data are > >> you trying to hide? > > > > Well, everything from drivers which the current clk implementations do > > hide. Catching abuse in with drivers coming in from all different trees > > and lists will be impossible. > > > > For platform code it is more fuzzy. I don't think platform code should > > be allowed to muck with prepare/enable counts for example. > > So there is a clear dichotomy: drivers shouldn't be exposed to any of > it and platform code should be exposed to some of it. > > How about a drivers/clk/clk-private.h which will define struct clk and > must only be included by clk drivers in drivers/clk/*? > > This establishes a bright line between those things which are allowed > to know the details of struct clk and those that are not: namely that > clk drivers in drivers/clk/ may use '#include "clk-private.h"'. > Obviously struct clk is opaque to the rest of the kernel (in the same > way it has been prior to the common clk patches) and there is no need > for struct clk_hw anymore. Also helper functions are no longer needed > for clock driver code, which I think *is* a manageable set of code to > review. Also clk drivers must live in drivers/clk/ for this to work > (without a big ugly path in someone's #include directive somewhere). > > Thoughts? > > Regards, > Mike Thomas? We're stuck on this fundamental point for a while now. And v5 of the patchset doesn't make much sense without resolving it. /Amit