From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753224AbeBSQ3b (ORCPT ); Mon, 19 Feb 2018 11:29:31 -0500 Received: from usa-sjc-mx-foss1.foss.arm.com ([217.140.101.70]:33352 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752897AbeBSQ31 (ORCPT ); Mon, 19 Feb 2018 11:29:27 -0500 Subject: Re: [PATCH 1/8] clk: Add clk_bulk_alloc functions To: 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 , Marek Szyprowski , 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 References: <1519055046-2399-1-git-send-email-m.purski@samsung.com> <1519055046-2399-2-git-send-email-m.purski@samsung.com> From: Robin Murphy Message-ID: Date: Mon, 19 Feb 2018 16:29:12 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 MIME-Version: 1.0 In-Reply-To: <1519055046-2399-2-git-send-email-m.purski@samsung.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-GB Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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. > 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; > From mboxrd@z Thu Jan 1 00:00:00 1970 From: Robin Murphy Subject: Re: [PATCH 1/8] clk: Add clk_bulk_alloc functions Date: Mon, 19 Feb 2018 16:29:12 +0000 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: <1519055046-2399-2-git-send-email-m.purski@samsung.com> Content-Language: en-GB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: 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 , Marek Szyprowski , 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 SGkgTWFjaWVqLAoKT24gMTkvMDIvMTggMTU6NDMsIE1hY2llaiBQdXJza2kgd3JvdGU6Cj4gV2hl biBhIGRyaXZlciBpcyBnb2luZyB0byB1c2UgY2xrX2J1bGtfZ2V0KCkgZnVuY3Rpb24sIGl0IGhh cyB0bwo+IGluaXRpYWxpemUgYW4gYXJyYXkgb2YgY2xrX2J1bGtfZGF0YSwgYnkgZmlsbGluZyBp dHMgaWQgZmllbGRzLgo+IAo+IEFkZCBhIG5ldyBmdW5jdGlvbiB0byB0aGUgY29yZSwgd2hpY2gg ZHluYW1pY2FsbHkgYWxsb2NhdGVzCj4gY2xrX2J1bGtfZGF0YSBhcnJheSBhbmQgZmlsbHMgaXRz IGlkIGZpZWxkcy4gQWRkIGNsa19idWxrX2ZyZWUoKQo+IGZ1bmN0aW9uLCB3aGljaCBmcmVlcyB0 aGUgYXJyYXkgYWxsb2NhdGVkIGJ5IGNsa19idWxrX2FsbG9jKCkgZnVuY3Rpb24uCj4gQWRkIGEg bWFuYWdlZCB2ZXJzaW9uIG9mIGNsa19idWxrX2FsbG9jKCkuCgpTZWVpbmcgaG93IGV2ZXJ5IHN1 YnNlcXVlbnQgcGF0Y2ggZW5kcyB1cCB3aXRoIHRoZSByb3VnaGx5IHRoaXMgc2FtZSBzdGFuemE6 CgoJeCA9IGRldm1fY2xrX2J1bGtfYWxsb2MoZGV2LCBudW0sIG5hbWVzKTsKCWlmIChJU19FUlIo eCkKCQlyZXR1cm4gUFRSX0VSUih4KTsKCXJldCA9IGRldm1fY2xrX2J1bGtfZ2V0KGRldiwgeCwg bnVtKTsKCkkgd29uZGVyIGlmIGl0IG1pZ2h0IGJlIGJldHRlciB0byBzaW1wbHkgaW1wbGVtZW50 OgoKCWludCBkZXZtX2Nsa19idWxrX2FsbG9jX2dldChkZXYsICZ4LCBudW0sIG5hbWVzKQoKdGhh dCBkb2VzIHRoZSB3aG9sZSBsb3QgaW4gb25lIGdvLCBhbmQgbGV0IGRyaXZlcnMgdGhhdCB3YW50 IHRvIGRvIG1vcmUgCmZpZGRseSB0aGluZ3MgY29udGludWUgdG8gb3Blbi1jb2RlIHRoZSBhbGxv Y2F0aW9uLgoKQnV0IHBlcmhhcHMgdGhhdCdzIGFuIGFic3RyYWN0aW9uIHRvbyBmYXIuLi4gSSdt IG5vdCBhbGwgdGhhdCBmYW1pbGlhciAKd2l0aCB0aGUgbGllIG9mIHRoZSBsYW5kIGhlcmUuCgo+ IFNpZ25lZC1vZmYtYnk6IE1hY2llaiBQdXJza2kgPG0ucHVyc2tpQHNhbXN1bmcuY29tPgo+IC0t LQo+ICAgZHJpdmVycy9jbGsvY2xrLWJ1bGsuYyAgIHwgMTYgKysrKysrKysrKysrCj4gICBkcml2 ZXJzL2Nsay9jbGstZGV2cmVzLmMgfCAzNyArKysrKysrKysrKysrKysrKysrKysrKysrLS0tCj4g ICBpbmNsdWRlL2xpbnV4L2Nsay5oICAgICAgfCA2NCArKysrKysrKysrKysrKysrKysrKysrKysr KysrKysrKysrKysrKysrKysrKysrKysKPiAgIDMgZmlsZXMgY2hhbmdlZCwgMTEzIGluc2VydGlv bnMoKyksIDQgZGVsZXRpb25zKC0pCj4gCgpbLi4uXQo+IEBAIC01OTgsNiArNjQ1LDIzIEBAIHN0 cnVjdCBjbGsgKmNsa19nZXRfc3lzKGNvbnN0IGNoYXIgKmRldl9pZCwgY29uc3QgY2hhciAqY29u X2lkKTsKPiAgIAo+ICAgI2Vsc2UgLyogIUNPTkZJR19IQVZFX0NMSyAqLwo+ICAgCj4gK3N0YXRp YyBpbmxpbmUgc3RydWN0IGNsa19idWxrX2RhdGEgKmNsa19idWxrX2FsbG9jKGludCBudW1fY2xr cywKPiArCQkJCQkJICAgY29uc3QgY2hhciAqKmNsa19pZHMpCj4gK3sKPiArCXJldHVybiBOVUxM OwoKRWl0aGVyIHdheSwgaXMgaXQgaW50ZW50aW9uYWwgbm90IHJldHVybmluZyBhbiBFUlJfUFRS KCkgdmFsdWUgaW4gdGhpcyAKY2FzZT8gU2luY2UgTlVMTCB3aWxsIHBhc3MgYW4gSVNfRVJSKCkg Y2hlY2ssIGl0IHNlZW1zIGEgYml0IGZyYWdpbGUgZm9yIAphbiBhbGxvY2F0aW9uIGNhbGwgdG8g YXBwYXJlbnRseSBzdWNjZWVkIHdoZW4gdGhlIHdob2xlIEFQSSBpcyAKY29uZmlndXJlZCBvdXQg KGFuZCBJIGJlbGlldmUgaW50cm9kdWNpbmcgbmV3IHVzZXMgb2YgSVNfRVJSX09SX05VTEwoKSAK aXMgaW4gZ2VuZXJhbCBzdHJvbmdseSBkaXNjb3VyYWdlZC4pCgpSb2Jpbi4KCj4gK30KPiArCj4g K3N0YXRpYyBpbmxpbmUgc3RydWN0IGNsa19idWxrX2RhdGEgKmRldm1fY2xrX2J1bGtfYWxsb2Mo c3RydWN0IGRldmljZSAqZGV2LAo+ICsJCQkJCQkJaW50IG51bV9jbGtzLAo+ICsJCQkJCQkJY29u c3QgY2hhciAqKmNsa19pZHMpCj4gK3sKPiArCXJldHVybiBOVUxMOwo+ICt9Cj4gKwo+ICtzdGF0 aWMgaW5saW5lIHZvaWQgY2xrX2J1bGtfZnJlZShzdHJ1Y3QgY2xrX2J1bGtfZGF0YSAqY2xrcykK PiArewo+ICt9Cj4gKwo+ICAgc3RhdGljIGlubGluZSBzdHJ1Y3QgY2xrICpjbGtfZ2V0KHN0cnVj dCBkZXZpY2UgKmRldiwgY29uc3QgY2hhciAqaWQpCj4gICB7Cj4gICAJcmV0dXJuIE5VTEw7Cj4g Cl9fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fCmRyaS1kZXZl bCBtYWlsaW5nIGxpc3QKZHJpLWRldmVsQGxpc3RzLmZyZWVkZXNrdG9wLm9yZwpodHRwczovL2xp c3RzLmZyZWVkZXNrdG9wLm9yZy9tYWlsbWFuL2xpc3RpbmZvL2RyaS1kZXZlbAo= From mboxrd@z Thu Jan 1 00:00:00 1970 From: robin.murphy@arm.com (Robin Murphy) Date: Mon, 19 Feb 2018 16:29:12 +0000 Subject: [PATCH 1/8] clk: Add clk_bulk_alloc functions In-Reply-To: <1519055046-2399-2-git-send-email-m.purski@samsung.com> 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 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. > 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; >