From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751413AbeBTJgS (ORCPT ); Tue, 20 Feb 2018 04:36:18 -0500 Received: from mailout2.w1.samsung.com ([210.118.77.12]:57021 "EHLO mailout2.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751129AbeBTJgL (ORCPT ); Tue, 20 Feb 2018 04:36:11 -0500 DKIM-Filter: OpenDKIM Filter v2.11.0 mailout2.w1.samsung.com 20180220093608euoutp021628ce46f866b9a615342745c81292e8~U-wRJSmXJ0545405454euoutp02f X-AuditID: cbfec7f5-b5fff700000028a9-6c-5a8bec0654b5 MIME-version: 1.0 Content-type: text/plain; charset="utf-8"; format="flowed" Subject: Re: [PATCH 1/8] clk: Add clk_bulk_alloc functions To: Robin Murphy , Maciej Purski , linux-media@vger.kernel.org, linux-samsung-soc@vger.kernel.org, linux-arm-kernel@lists.infradead.org, dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, linux-clk@vger.kernel.org Cc: David Airlie , Michael Turquette , Kamil Debski , Andrzej Hajda , Sylwester Nawrocki , Thibault Saunier , Joonyoung Shim , Russell King , Krzysztof Kozlowski , Javier Martinez Canillas , Kukjin Kim , Hoegeun Kwon , Bartlomiej Zolnierkiewicz , Inki Dae , Jeongtae Park , Jacek Anaszewski , Andrzej Pietrasiewicz , Mauro Carvalho Chehab , Stephen Boyd , Seung-Woo Kim , Hans Verkuil , Kyungmin Park From: Marek Szyprowski Message-id: Date: Tue, 20 Feb 2018 10:36:03 +0100 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 In-reply-to: Content-transfer-encoding: 8bit Content-language: en-US X-Brightmail-Tracker: H4sIAAAAAAAAA01Sa0hTYRjmO3eXi9My9lGhMRAkKJOKPiqitOJARUF/YkS59GSiTtnULj/K S6bO+7U5pDRSSyx1qZWR5ZSWZZuXZbPQIi2bl5x5Kcusnc4C/z3P+zzf+7zvy8fgslpqNROu juU1alWkgpIQTc/mLRuo8QzlJsfXVejtXQuJsiwdGDI4UglUr68lkW12kkLOvBKAJqt0FMp/ n0ugd411BBqfqMFQ+53XGPoy2E2gkuepJMoZGsOR1VpHo1dJ4zQyDvWRaCpzkES9zaUUyqxr JJHe2oIhU9FjgMocbwlUaVzAUPeLfajVOUyitnFXk8W+egLpCxwUmn6aj+/25mqu1QCut68b 5ybtKTRX+Kue5B4aBmjOWJ1OcffnPpDc+wwzxt27eYlryHHVsxuqATdt9OZs7Yn0EalSsjOU jwyP5zX+u4IlZx5/OxeTuvbcUJcNTwCJch3wYCC7BWaOFQMdkDAy9haAi0k2N5kGsOJ7M/nf lT12GROFSgBrRsy4IEjZFfBHwSAhYJzdDj8vppCiacRlchbSgrBSEDrncEHwYoswaP9toASC s+U0fN53+5+LYgOgbkJHiW13weTi6/8wwfrC0k69K5thVrHH4djCfqHswe6AvR9nMDHZB7ba RtxTyGFySj8h9IdsOQOTfzkpcYe9MHH2ixuvhKPmBlrEa2F6WismPshxXeCPw030ANZOpbhd O2CbuZsUI5bD/KaruDARZKUw7YpMtHAwO+sTIeI9sCS1CghYxiZhsPzWulzgbVhyMsOSkxmW LGFYskQZIKqBnI/TRoXx2s1q/uxGrSpKG6cO2xgSHWUErm/8ctE8+wC0LJwyAZYBCk/pigGd Ukaq4rXno0wAMrjCSxpYkaGUSUNV5y/wmuiTmrhIXmsCaxhCIZee8LuolLFhqlg+gudjeM1/ FWM8VieAnTMHYsCTooMhzd9NzraC201HsZ+nY+3zXjc6ZSdDa03pgT1Oqx9GeZbZR/2HfSRp ni/3d01Y9M6qZe+ONXZt69lteeRXmvNZERwblJCFd5RfvVaoxuu3llmHmDcBn3qYiez54ROH k7y/5s3a+w9FkNhDtXzGQktH9b5BA8FbFIT2jCpgPa7Rqv4Cxbh008IDAAA= X-Brightmail-Tracker: H4sIAAAAAAAAA01Sa0hTYRjmO7cdrdFxGX6YKa0UlPJSER8rLYLghBBB0I9R2dKDWk5tR03D SGdqTVPzxjasjLyAzXSbdkVp09S8TVNXKV5Q08SJmiZJITkt8N/D+1x/vDQuUpKudGR0HKeI lkWJKUeiY63VepC0ZUn9izPc0OCLbhI96P6IIe1MJoH06hoS9f+cp9DCQw1A85UqCuWP5hFo qL6WQLY5HYaaqwcw9H2kl0CatkwS5U7M4shiqRWgLqVNgAwTVhItZo+QqO9tCYWya+tJpLY0 Yshc1ABQ6cwggSoMfzDU234amRYmSdRkWw9Zs+oJpC6YodDS+3z8pDure6wDbJ+1F2fnv6QL 2MLfepJ9ox0WsIaq+xT7amWMZEezWjHWWHaHrctdv+fUVQF2yeDO9jenCs4JpY7Hw7ioyARO 4Rd0xTGi4UdibKZb4kRPP54CUl1UwIGGzBGYM3sXUwFHWsSUAdjQPkDaCSHjBH8VjBB2jDNH 4fJME7kpmgbQMJ6/IdrJSOBU5wpuJ5yZAgx2rCo3onCmXAC1BVpq06LE4FTzO9xuoZgAqJpT UZsdQTCt+MkGJhhPWNKpxux4F3MRFi3qNyocmGOwb3wZ29zhAU390/82ucC09K9EHmC0W+Zq t8zVbrFot1hKAVEFnLl4Xh4u5wN8eZmcj48O9w2NkRvA+me8bFk1vgaf9OfNgKGBeLuQGVZJ RaQsgU+SmwGkcbGz8FR5llQkDJMl3eIUMSGK+CiON4PdNCF2EVr8k6QiJlwWx13nuFhO8Z/F aAfXFHDGJBiadPlQXSqpDNyjvN3V6EmGDIZs0934Zl6bs7qLQcUJnbfY4ul1sz1euOPy1bHP QUOHY70uPPc2GoP3zXaI69sPPNub4KdRmvDC0OSFjP0elcGX9EUeLV6tAWclEm7p6aGaaxc0 gTVtucu+kmA/zqkv+d6jpM7ekJ6MoTAfMcFHyAJ8cAUv+wtuTUaRFQMAAA== X-CMS-MailID: 20180220093605eucas1p1ecfc5f4919bd01ec1b3768f63d71d008 X-Msg-Generator: CA CMS-TYPE: 201P X-CMS-RootMailID: 20180219154456eucas1p15f4073beaf61312238f142f217a8bb3c X-RootMTR: 20180219154456eucas1p15f4073beaf61312238f142f217a8bb3c References: <1519055046-2399-1-git-send-email-m.purski@samsung.com> <1519055046-2399-2-git-send-email-m.purski@samsung.com> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Robin, On 2018-02-19 17:29, Robin Murphy wrote: > Hi Maciej, > > On 19/02/18 15:43, Maciej Purski wrote: >> When a driver is going to use clk_bulk_get() function, it has to >> initialize an array of clk_bulk_data, by filling its id fields. >> >> Add a new function to the core, which dynamically allocates >> clk_bulk_data array and fills its id fields. Add clk_bulk_free() >> function, which frees the array allocated by clk_bulk_alloc() function. >> Add a managed version of clk_bulk_alloc(). > > Seeing how every subsequent patch ends up with the roughly this same > stanza: > >     x = devm_clk_bulk_alloc(dev, num, names); >     if (IS_ERR(x) >         return PTR_ERR(x); >     ret = devm_clk_bulk_get(dev, x, num); > > I wonder if it might be better to simply implement: > >     int devm_clk_bulk_alloc_get(dev, &x, num, names) > > that does the whole lot in one go, and let drivers that want to do > more fiddly things continue to open-code the allocation. > > But perhaps that's an abstraction too far... I'm not all that familiar > with the lie of the land here. Hmmm. This patchset clearly shows, that it would be much simpler if we get rid of clk_bulk_data structure at all and let clk_bulk_* functions to operate on struct clk *array[]. Typically the list of clock names is already in some kind of array (taken from driver data or statically embedded into driver), so there is little benefit from duplicating it in clk_bulk_data. Sadly, I missed clk_bulk_* api discussion and maybe there are other benefits from this approach. If not, I suggest simplifying clk_bulk_* api by dropping clk_bulk_data structure and switching to clock ptr array: int clk_bulk_get(struct device *dev, int num_clock, struct clk *clocks[],                  const char *clk_names[]); int clk_bulk_prepare(int num_clks, struct clk *clks[]); int clk_bulk_enable(int num_clks, struct clk *clks[]); ... > >> Signed-off-by: Maciej Purski >> --- >>   drivers/clk/clk-bulk.c   | 16 ++++++++++++ >>   drivers/clk/clk-devres.c | 37 +++++++++++++++++++++++++--- >>   include/linux/clk.h      | 64 >> ++++++++++++++++++++++++++++++++++++++++++++++++ >>   3 files changed, 113 insertions(+), 4 deletions(-) >> > > [...] >> @@ -598,6 +645,23 @@ struct clk *clk_get_sys(const char *dev_id, >> const char *con_id); >>     #else /* !CONFIG_HAVE_CLK */ >>   +static inline struct clk_bulk_data *clk_bulk_alloc(int num_clks, >> +                           const char **clk_ids) >> +{ >> +    return NULL; > > Either way, is it intentional not returning an ERR_PTR() value in this > case? Since NULL will pass an IS_ERR() check, it seems a bit fragile > for an allocation call to apparently succeed when the whole API is > configured out (and I believe introducing new uses of IS_ERR_OR_NULL() > is in general strongly discouraged.) > > Robin. > >> +} >> + >> +static inline struct clk_bulk_data *devm_clk_bulk_alloc(struct >> device *dev, >> +                            int num_clks, >> +                            const char **clk_ids) >> +{ >> +    return NULL; >> +} >> + >> +static inline void clk_bulk_free(struct clk_bulk_data *clks) >> +{ >> +} >> + >>   static inline struct clk *clk_get(struct device *dev, const char *id) >>   { >>       return NULL; > Best regards -- Marek Szyprowski, PhD Samsung R&D Institute Poland From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marek Szyprowski Subject: Re: [PATCH 1/8] clk: Add clk_bulk_alloc functions Date: Tue, 20 Feb 2018 10:36:03 +0100 Message-ID: References: <1519055046-2399-1-git-send-email-m.purski@samsung.com> <1519055046-2399-2-git-send-email-m.purski@samsung.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8"; Format="flowed" Content-Transfer-Encoding: base64 Return-path: In-reply-to: Content-language: en-US List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Robin Murphy , Maciej Purski , linux-media@vger.kernel.org, linux-samsung-soc@vger.kernel.org, linux-arm-kernel@lists.infradead.org, dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, linux-clk@vger.kernel.org Cc: David Airlie , Michael Turquette , Kamil Debski , Sylwester Nawrocki , Thibault Saunier , Russell King , Krzysztof Kozlowski , Javier Martinez Canillas , Kukjin Kim , Hoegeun Kwon , Bartlomiej Zolnierkiewicz , Jeongtae Park , Jacek Anaszewski , Andrzej Pietrasiewicz , Mauro Carvalho Chehab , Stephen Boyd , Seung-Woo Kim , Hans Verkuil , Kyungmin Park List-Id: linux-samsung-soc@vger.kernel.org SGkgUm9iaW4sCgpPbiAyMDE4LTAyLTE5IDE3OjI5LCBSb2JpbiBNdXJwaHkgd3JvdGU6Cj4gSGkg TWFjaWVqLAo+Cj4gT24gMTkvMDIvMTggMTU6NDMsIE1hY2llaiBQdXJza2kgd3JvdGU6Cj4+IFdo ZW4gYSBkcml2ZXIgaXMgZ29pbmcgdG8gdXNlIGNsa19idWxrX2dldCgpIGZ1bmN0aW9uLCBpdCBo YXMgdG8KPj4gaW5pdGlhbGl6ZSBhbiBhcnJheSBvZiBjbGtfYnVsa19kYXRhLCBieSBmaWxsaW5n IGl0cyBpZCBmaWVsZHMuCj4+Cj4+IEFkZCBhIG5ldyBmdW5jdGlvbiB0byB0aGUgY29yZSwgd2hp Y2ggZHluYW1pY2FsbHkgYWxsb2NhdGVzCj4+IGNsa19idWxrX2RhdGEgYXJyYXkgYW5kIGZpbGxz IGl0cyBpZCBmaWVsZHMuIEFkZCBjbGtfYnVsa19mcmVlKCkKPj4gZnVuY3Rpb24sIHdoaWNoIGZy ZWVzIHRoZSBhcnJheSBhbGxvY2F0ZWQgYnkgY2xrX2J1bGtfYWxsb2MoKSBmdW5jdGlvbi4KPj4g QWRkIGEgbWFuYWdlZCB2ZXJzaW9uIG9mIGNsa19idWxrX2FsbG9jKCkuCj4KPiBTZWVpbmcgaG93 IGV2ZXJ5IHN1YnNlcXVlbnQgcGF0Y2ggZW5kcyB1cCB3aXRoIHRoZSByb3VnaGx5IHRoaXMgc2Ft ZSAKPiBzdGFuemE6Cj4KPiDCoMKgwqDCoHggPSBkZXZtX2Nsa19idWxrX2FsbG9jKGRldiwgbnVt LCBuYW1lcyk7Cj4gwqDCoMKgwqBpZiAoSVNfRVJSKHgpCj4gwqDCoMKgwqDCoMKgwqAgcmV0dXJu IFBUUl9FUlIoeCk7Cj4gwqDCoMKgwqByZXQgPSBkZXZtX2Nsa19idWxrX2dldChkZXYsIHgsIG51 bSk7Cj4KPiBJIHdvbmRlciBpZiBpdCBtaWdodCBiZSBiZXR0ZXIgdG8gc2ltcGx5IGltcGxlbWVu dDoKPgo+IMKgwqDCoMKgaW50IGRldm1fY2xrX2J1bGtfYWxsb2NfZ2V0KGRldiwgJngsIG51bSwg bmFtZXMpCj4KPiB0aGF0IGRvZXMgdGhlIHdob2xlIGxvdCBpbiBvbmUgZ28sIGFuZCBsZXQgZHJp dmVycyB0aGF0IHdhbnQgdG8gZG8gCj4gbW9yZSBmaWRkbHkgdGhpbmdzIGNvbnRpbnVlIHRvIG9w ZW4tY29kZSB0aGUgYWxsb2NhdGlvbi4KPgo+IEJ1dCBwZXJoYXBzIHRoYXQncyBhbiBhYnN0cmFj dGlvbiB0b28gZmFyLi4uIEknbSBub3QgYWxsIHRoYXQgZmFtaWxpYXIgCj4gd2l0aCB0aGUgbGll IG9mIHRoZSBsYW5kIGhlcmUuCgpIbW1tLiBUaGlzIHBhdGNoc2V0IGNsZWFybHkgc2hvd3MsIHRo YXQgaXQgd291bGQgYmUgbXVjaCBzaW1wbGVyIGlmIHdlCmdldCByaWQgb2YgY2xrX2J1bGtfZGF0 YSBzdHJ1Y3R1cmUgYXQgYWxsIGFuZCBsZXQgY2xrX2J1bGtfKiBmdW5jdGlvbnMKdG8gb3BlcmF0 ZSBvbiBzdHJ1Y3QgY2xrICphcnJheVtdLiBUeXBpY2FsbHkgdGhlIGxpc3Qgb2YgY2xvY2sgbmFt ZXMKaXMgYWxyZWFkeSBpbiBzb21lIGtpbmQgb2YgYXJyYXkgKHRha2VuIGZyb20gZHJpdmVyIGRh dGEgb3Igc3RhdGljYWxseQplbWJlZGRlZCBpbnRvIGRyaXZlciksIHNvIHRoZXJlIGlzIGxpdHRs ZSBiZW5lZml0IGZyb20gZHVwbGljYXRpbmcgaXQKaW4gY2xrX2J1bGtfZGF0YS4gU2FkbHksIEkg bWlzc2VkIGNsa19idWxrXyogYXBpIGRpc2N1c3Npb24gYW5kIG1heWJlCnRoZXJlIGFyZSBvdGhl ciBiZW5lZml0cyBmcm9tIHRoaXMgYXBwcm9hY2guCgpJZiBub3QsIEkgc3VnZ2VzdCBzaW1wbGlm eWluZyBjbGtfYnVsa18qIGFwaSBieSBkcm9wcGluZyBjbGtfYnVsa19kYXRhCnN0cnVjdHVyZSBh bmQgc3dpdGNoaW5nIHRvIGNsb2NrIHB0ciBhcnJheToKCmludCBjbGtfYnVsa19nZXQoc3RydWN0 IGRldmljZSAqZGV2LCBpbnQgbnVtX2Nsb2NrLCBzdHJ1Y3QgY2xrICpjbG9ja3NbXSwKIMKgwqDC oCDCoMKgwqAgwqDCoMKgIMKgwqDCoMKgIGNvbnN0IGNoYXIgKmNsa19uYW1lc1tdKTsKaW50IGNs a19idWxrX3ByZXBhcmUoaW50IG51bV9jbGtzLCBzdHJ1Y3QgY2xrICpjbGtzW10pOwppbnQgY2xr X2J1bGtfZW5hYmxlKGludCBudW1fY2xrcywgc3RydWN0IGNsayAqY2xrc1tdKTsKLi4uCgoKCj4K Pj4gU2lnbmVkLW9mZi1ieTogTWFjaWVqIFB1cnNraSA8bS5wdXJza2lAc2Ftc3VuZy5jb20+Cj4+ IC0tLQo+PiDCoCBkcml2ZXJzL2Nsay9jbGstYnVsay5jwqDCoCB8IDE2ICsrKysrKysrKysrKwo+ PiDCoCBkcml2ZXJzL2Nsay9jbGstZGV2cmVzLmMgfCAzNyArKysrKysrKysrKysrKysrKysrKysr KysrLS0tCj4+IMKgIGluY2x1ZGUvbGludXgvY2xrLmjCoMKgwqDCoMKgIHwgNjQgCj4+ICsrKysr KysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKwo+PiDCoCAzIGZpbGVz IGNoYW5nZWQsIDExMyBpbnNlcnRpb25zKCspLCA0IGRlbGV0aW9ucygtKQo+Pgo+Cj4gWy4uLl0K Pj4gQEAgLTU5OCw2ICs2NDUsMjMgQEAgc3RydWN0IGNsayAqY2xrX2dldF9zeXMoY29uc3QgY2hh ciAqZGV2X2lkLCAKPj4gY29uc3QgY2hhciAqY29uX2lkKTsKPj4gwqAgwqAgI2Vsc2UgLyogIUNP TkZJR19IQVZFX0NMSyAqLwo+PiDCoCArc3RhdGljIGlubGluZSBzdHJ1Y3QgY2xrX2J1bGtfZGF0 YSAqY2xrX2J1bGtfYWxsb2MoaW50IG51bV9jbGtzLAo+PiArwqDCoMKgwqDCoMKgwqDCoMKgwqDC oMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoCBjb25zdCBjaGFyICoqY2xrX2lkcykKPj4g K3sKPj4gK8KgwqDCoCByZXR1cm4gTlVMTDsKPgo+IEVpdGhlciB3YXksIGlzIGl0IGludGVudGlv bmFsIG5vdCByZXR1cm5pbmcgYW4gRVJSX1BUUigpIHZhbHVlIGluIHRoaXMgCj4gY2FzZT8gU2lu Y2UgTlVMTCB3aWxsIHBhc3MgYW4gSVNfRVJSKCkgY2hlY2ssIGl0IHNlZW1zIGEgYml0IGZyYWdp bGUgCj4gZm9yIGFuIGFsbG9jYXRpb24gY2FsbCB0byBhcHBhcmVudGx5IHN1Y2NlZWQgd2hlbiB0 aGUgd2hvbGUgQVBJIGlzIAo+IGNvbmZpZ3VyZWQgb3V0IChhbmQgSSBiZWxpZXZlIGludHJvZHVj aW5nIG5ldyB1c2VzIG9mIElTX0VSUl9PUl9OVUxMKCkgCj4gaXMgaW4gZ2VuZXJhbCBzdHJvbmds eSBkaXNjb3VyYWdlZC4pCj4KPiBSb2Jpbi4KPgo+PiArfQo+PiArCj4+ICtzdGF0aWMgaW5saW5l IHN0cnVjdCBjbGtfYnVsa19kYXRhICpkZXZtX2Nsa19idWxrX2FsbG9jKHN0cnVjdCAKPj4gZGV2 aWNlICpkZXYsCj4+ICvCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDC oMKgwqDCoMKgwqAgaW50IG51bV9jbGtzLAo+PiArwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDC oMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgIGNvbnN0IGNoYXIgKipjbGtfaWRzKQo+PiArewo+ PiArwqDCoMKgIHJldHVybiBOVUxMOwo+PiArfQo+PiArCj4+ICtzdGF0aWMgaW5saW5lIHZvaWQg Y2xrX2J1bGtfZnJlZShzdHJ1Y3QgY2xrX2J1bGtfZGF0YSAqY2xrcykKPj4gK3sKPj4gK30KPj4g Kwo+PiDCoCBzdGF0aWMgaW5saW5lIHN0cnVjdCBjbGsgKmNsa19nZXQoc3RydWN0IGRldmljZSAq ZGV2LCBjb25zdCBjaGFyICppZCkKPj4gwqAgewo+PiDCoMKgwqDCoMKgIHJldHVybiBOVUxMOwo+ CkJlc3QgcmVnYXJkcwotLSAKTWFyZWsgU3p5cHJvd3NraSwgUGhEClNhbXN1bmcgUiZEIEluc3Rp dHV0ZSBQb2xhbmQKCl9fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fCmRyaS1kZXZlbCBtYWlsaW5nIGxpc3QKZHJpLWRldmVsQGxpc3RzLmZyZWVkZXNrdG9wLm9y ZwpodHRwczovL2xpc3RzLmZyZWVkZXNrdG9wLm9yZy9tYWlsbWFuL2xpc3RpbmZvL2RyaS1kZXZl bAo= From mboxrd@z Thu Jan 1 00:00:00 1970 From: m.szyprowski@samsung.com (Marek Szyprowski) Date: Tue, 20 Feb 2018 10:36:03 +0100 Subject: [PATCH 1/8] clk: Add clk_bulk_alloc functions In-Reply-To: References: <1519055046-2399-1-git-send-email-m.purski@samsung.com> <1519055046-2399-2-git-send-email-m.purski@samsung.com> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hi Robin, On 2018-02-19 17:29, Robin Murphy wrote: > Hi Maciej, > > On 19/02/18 15:43, Maciej Purski wrote: >> When a driver is going to use clk_bulk_get() function, it has to >> initialize an array of clk_bulk_data, by filling its id fields. >> >> Add a new function to the core, which dynamically allocates >> clk_bulk_data array and fills its id fields. Add clk_bulk_free() >> function, which frees the array allocated by clk_bulk_alloc() function. >> Add a managed version of clk_bulk_alloc(). > > Seeing how every subsequent patch ends up with the roughly this same > stanza: > > ????x = devm_clk_bulk_alloc(dev, num, names); > ????if (IS_ERR(x) > ??????? return PTR_ERR(x); > ????ret = devm_clk_bulk_get(dev, x, num); > > I wonder if it might be better to simply implement: > > ????int devm_clk_bulk_alloc_get(dev, &x, num, names) > > that does the whole lot in one go, and let drivers that want to do > more fiddly things continue to open-code the allocation. > > But perhaps that's an abstraction too far... I'm not all that familiar > with the lie of the land here. Hmmm. This patchset clearly shows, that it would be much simpler if we get rid of clk_bulk_data structure at all and let clk_bulk_* functions to operate on struct clk *array[]. Typically the list of clock names is already in some kind of array (taken from driver data or statically embedded into driver), so there is little benefit from duplicating it in clk_bulk_data. Sadly, I missed clk_bulk_* api discussion and maybe there are other benefits from this approach. If not, I suggest simplifying clk_bulk_* api by dropping clk_bulk_data structure and switching to clock ptr array: int clk_bulk_get(struct device *dev, int num_clock, struct clk *clocks[], ??? ??? ??? ???? const char *clk_names[]); int clk_bulk_prepare(int num_clks, struct clk *clks[]); int clk_bulk_enable(int num_clks, struct clk *clks[]); ... > >> Signed-off-by: Maciej Purski >> --- >> ? drivers/clk/clk-bulk.c?? | 16 ++++++++++++ >> ? drivers/clk/clk-devres.c | 37 +++++++++++++++++++++++++--- >> ? include/linux/clk.h????? | 64 >> ++++++++++++++++++++++++++++++++++++++++++++++++ >> ? 3 files changed, 113 insertions(+), 4 deletions(-) >> > > [...] >> @@ -598,6 +645,23 @@ struct clk *clk_get_sys(const char *dev_id, >> const char *con_id); >> ? ? #else /* !CONFIG_HAVE_CLK */ >> ? +static inline struct clk_bulk_data *clk_bulk_alloc(int num_clks, >> +?????????????????????????? const char **clk_ids) >> +{ >> +??? return NULL; > > Either way, is it intentional not returning an ERR_PTR() value in this > case? Since NULL will pass an IS_ERR() check, it seems a bit fragile > for an allocation call to apparently succeed when the whole API is > configured out (and I believe introducing new uses of IS_ERR_OR_NULL() > is in general strongly discouraged.) > > Robin. > >> +} >> + >> +static inline struct clk_bulk_data *devm_clk_bulk_alloc(struct >> device *dev, >> +??????????????????????????? int num_clks, >> +??????????????????????????? const char **clk_ids) >> +{ >> +??? return NULL; >> +} >> + >> +static inline void clk_bulk_free(struct clk_bulk_data *clks) >> +{ >> +} >> + >> ? static inline struct clk *clk_get(struct device *dev, const char *id) >> ? { >> ????? return NULL; > Best regards -- Marek Szyprowski, PhD Samsung R&D Institute Poland