From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932880AbeCOWzN convert rfc822-to-8bit (ORCPT ); Thu, 15 Mar 2018 18:55:13 -0400 Received: from mail.kernel.org ([198.145.29.99]:37692 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752810AbeCOWzL (ORCPT ); Thu, 15 Mar 2018 18:55:11 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 70D3320685 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=sboyd@kernel.org Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8BIT To: Maciej Purski , Marek Szyprowski , Robin Murphy , dri-devel@lists.freedesktop.org, linux-arm-kernel@lists.infradead.org, linux-clk@vger.kernel.org, linux-kernel@vger.kernel.org, linux-media@vger.kernel.org, linux-samsung-soc@vger.kernel.org From: Stephen Boyd In-Reply-To: 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 , 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> Message-ID: <152115450981.111154.2342657732967302796@swboyd.mtv.corp.google.com> User-Agent: alot/0.7 Subject: Re: [PATCH 1/8] clk: Add clk_bulk_alloc functions Date: Thu, 15 Mar 2018 15:55:09 -0700 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Quoting Marek Szyprowski (2018-02-20 01:36:03) > Hi Robin, > > On 2018-02-19 17:29, Robin Murphy wrote: > > > > 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[]); > ... > If you have an array of pointers to names of clks then we can put the struct clk pointers adjacent to them at the same time. I suppose the problem there is that some drivers want to mark that array of pointers to names as const. And then we can't update the clk pointers next to them. This is the same design that regulators has done so that's why it's written like this for clks. Honestly, we're talking about a handful of pointers here so I'm not sure it really matters much. Just duplicate the pointer and be done if you want to mark the array of names as const or have your const 'setup' structure point to the bulk_data array that you define statically non-const somewhere globally. From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 To: Maciej Purski , Marek Szyprowski , Robin Murphy , dri-devel@lists.freedesktop.org, linux-arm-kernel@lists.infradead.org, linux-clk@vger.kernel.org, linux-kernel@vger.kernel.org, linux-media@vger.kernel.org, linux-samsung-soc@vger.kernel.org From: Stephen Boyd In-Reply-To: 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 , 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> Message-ID: <152115450981.111154.2342657732967302796@swboyd.mtv.corp.google.com> Subject: Re: [PATCH 1/8] clk: Add clk_bulk_alloc functions Date: Thu, 15 Mar 2018 15:55:09 -0700 List-ID: Quoting Marek Szyprowski (2018-02-20 01:36:03) > Hi Robin, > = > On 2018-02-19 17:29, Robin Murphy wrote: > > > > Seeing how every subsequent patch ends up with the roughly this same = > > stanza: > > > > =C2=A0=C2=A0=C2=A0=C2=A0x =3D devm_clk_bulk_alloc(dev, num, names); > > =C2=A0=C2=A0=C2=A0=C2=A0if (IS_ERR(x) > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return PTR_ERR(x); > > =C2=A0=C2=A0=C2=A0=C2=A0ret =3D devm_clk_bulk_get(dev, x, num); > > > > I wonder if it might be better to simply implement: > > > > =C2=A0=C2=A0=C2=A0=C2=A0int 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[], > =C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2= =A0=C2=A0 const char *clk_names[]); > int clk_bulk_prepare(int num_clks, struct clk *clks[]); > int clk_bulk_enable(int num_clks, struct clk *clks[]); > ... > = If you have an array of pointers to names of clks then we can put the struct clk pointers adjacent to them at the same time. I suppose the problem there is that some drivers want to mark that array of pointers to names as const. And then we can't update the clk pointers next to them. This is the same design that regulators has done so that's why it's written like this for clks. Honestly, we're talking about a handful of pointers here so I'm not sure it really matters much. Just duplicate the pointer and be done if you want to mark the array of names as const or have your const 'setup' structure point to the bulk_data array that you define statically non-const somewhere globally. From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stephen Boyd Subject: Re: [PATCH 1/8] clk: Add clk_bulk_alloc functions Date: Thu, 15 Mar 2018 15:55:09 -0700 Message-ID: <152115450981.111154.2342657732967302796@swboyd.mtv.corp.google.com> 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" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Maciej Purski , Marek Szyprowski , Robin Murphy , dri-devel@lists.freedesktop.org, linux-arm-kernel@lists.infradead.org, linux-clk@vger.kernel.org, linux-kernel@vger.kernel.org, linux-media@vger.kernel.org, linux-samsung-soc@vger.kernel.org Cc: Hans Verkuil , Thibault Saunier , Javier Martinez Canillas , David Airlie , Michael Turquette , Bartlomiej Zolnierkiewicz , Seung-Woo Kim , Kamil Debski , Krzysztof Kozlowski , Russell King , Kyungmin Park , Kukjin Kim , Jacek Anaszewski , Sylwester Nawrocki , Andrzej Pietrasiewicz , Mauro Carvalho Chehab , Jeongtae Park , Hoegeun Kwon List-Id: linux-samsung-soc@vger.kernel.org UXVvdGluZyBNYXJlayBTenlwcm93c2tpICgyMDE4LTAyLTIwIDAxOjM2OjAzKQo+IEhpIFJvYmlu LAo+IAo+IE9uIDIwMTgtMDItMTkgMTc6MjksIFJvYmluIE11cnBoeSB3cm90ZToKPiA+Cj4gPiBT ZWVpbmcgaG93IGV2ZXJ5IHN1YnNlcXVlbnQgcGF0Y2ggZW5kcyB1cCB3aXRoIHRoZSByb3VnaGx5 IHRoaXMgc2FtZSAKPiA+IHN0YW56YToKPiA+Cj4gPiDCoMKgwqDCoHggPSBkZXZtX2Nsa19idWxr X2FsbG9jKGRldiwgbnVtLCBuYW1lcyk7Cj4gPiDCoMKgwqDCoGlmIChJU19FUlIoeCkKPiA+IMKg wqDCoMKgwqDCoMKgIHJldHVybiBQVFJfRVJSKHgpOwo+ID4gwqDCoMKgwqByZXQgPSBkZXZtX2Ns a19idWxrX2dldChkZXYsIHgsIG51bSk7Cj4gPgo+ID4gSSB3b25kZXIgaWYgaXQgbWlnaHQgYmUg YmV0dGVyIHRvIHNpbXBseSBpbXBsZW1lbnQ6Cj4gPgo+ID4gwqDCoMKgwqBpbnQgZGV2bV9jbGtf YnVsa19hbGxvY19nZXQoZGV2LCAmeCwgbnVtLCBuYW1lcykKPiA+Cj4gPiB0aGF0IGRvZXMgdGhl IHdob2xlIGxvdCBpbiBvbmUgZ28sIGFuZCBsZXQgZHJpdmVycyB0aGF0IHdhbnQgdG8gZG8gCj4g PiBtb3JlIGZpZGRseSB0aGluZ3MgY29udGludWUgdG8gb3Blbi1jb2RlIHRoZSBhbGxvY2F0aW9u Lgo+ID4KPiA+IEJ1dCBwZXJoYXBzIHRoYXQncyBhbiBhYnN0cmFjdGlvbiB0b28gZmFyLi4uIEkn bSBub3QgYWxsIHRoYXQgZmFtaWxpYXIgCj4gPiB3aXRoIHRoZSBsaWUgb2YgdGhlIGxhbmQgaGVy ZS4KPiAKPiBIbW1tLiBUaGlzIHBhdGNoc2V0IGNsZWFybHkgc2hvd3MsIHRoYXQgaXQgd291bGQg YmUgbXVjaCBzaW1wbGVyIGlmIHdlCj4gZ2V0IHJpZCBvZiBjbGtfYnVsa19kYXRhIHN0cnVjdHVy ZSBhdCBhbGwgYW5kIGxldCBjbGtfYnVsa18qIGZ1bmN0aW9ucwo+IHRvIG9wZXJhdGUgb24gc3Ry dWN0IGNsayAqYXJyYXlbXS4gVHlwaWNhbGx5IHRoZSBsaXN0IG9mIGNsb2NrIG5hbWVzCj4gaXMg YWxyZWFkeSBpbiBzb21lIGtpbmQgb2YgYXJyYXkgKHRha2VuIGZyb20gZHJpdmVyIGRhdGEgb3Ig c3RhdGljYWxseQo+IGVtYmVkZGVkIGludG8gZHJpdmVyKSwgc28gdGhlcmUgaXMgbGl0dGxlIGJl bmVmaXQgZnJvbSBkdXBsaWNhdGluZyBpdAo+IGluIGNsa19idWxrX2RhdGEuIFNhZGx5LCBJIG1p c3NlZCBjbGtfYnVsa18qIGFwaSBkaXNjdXNzaW9uIGFuZCBtYXliZQo+IHRoZXJlIGFyZSBvdGhl ciBiZW5lZml0cyBmcm9tIHRoaXMgYXBwcm9hY2guCj4gCj4gSWYgbm90LCBJIHN1Z2dlc3Qgc2lt cGxpZnlpbmcgY2xrX2J1bGtfKiBhcGkgYnkgZHJvcHBpbmcgY2xrX2J1bGtfZGF0YQo+IHN0cnVj dHVyZSBhbmQgc3dpdGNoaW5nIHRvIGNsb2NrIHB0ciBhcnJheToKPiAKPiBpbnQgY2xrX2J1bGtf Z2V0KHN0cnVjdCBkZXZpY2UgKmRldiwgaW50IG51bV9jbG9jaywgc3RydWN0IGNsayAqY2xvY2tz W10sCj4gIMKgwqDCoCDCoMKgwqAgwqDCoMKgIMKgwqDCoMKgIGNvbnN0IGNoYXIgKmNsa19uYW1l c1tdKTsKPiBpbnQgY2xrX2J1bGtfcHJlcGFyZShpbnQgbnVtX2Nsa3MsIHN0cnVjdCBjbGsgKmNs a3NbXSk7Cj4gaW50IGNsa19idWxrX2VuYWJsZShpbnQgbnVtX2Nsa3MsIHN0cnVjdCBjbGsgKmNs a3NbXSk7Cj4gLi4uCj4gCgpJZiB5b3UgaGF2ZSBhbiBhcnJheSBvZiBwb2ludGVycyB0byBuYW1l cyBvZiBjbGtzIHRoZW4gd2UgY2FuIHB1dCB0aGUKc3RydWN0IGNsayBwb2ludGVycyBhZGphY2Vu dCB0byB0aGVtIGF0IHRoZSBzYW1lIHRpbWUuIEkgc3VwcG9zZSB0aGUKcHJvYmxlbSB0aGVyZSBp cyB0aGF0IHNvbWUgZHJpdmVycyB3YW50IHRvIG1hcmsgdGhhdCBhcnJheSBvZiBwb2ludGVycwp0 byBuYW1lcyBhcyBjb25zdC4gQW5kIHRoZW4gd2UgY2FuJ3QgdXBkYXRlIHRoZSBjbGsgcG9pbnRl cnMgbmV4dCB0bwp0aGVtLgoKVGhpcyBpcyB0aGUgc2FtZSBkZXNpZ24gdGhhdCByZWd1bGF0b3Jz IGhhcyBkb25lIHNvIHRoYXQncyB3aHkgaXQncwp3cml0dGVuIGxpa2UgdGhpcyBmb3IgY2xrcy4g SG9uZXN0bHksIHdlJ3JlIHRhbGtpbmcgYWJvdXQgYSBoYW5kZnVsIG9mCnBvaW50ZXJzIGhlcmUg c28gSSdtIG5vdCBzdXJlIGl0IHJlYWxseSBtYXR0ZXJzIG11Y2guIEp1c3QgZHVwbGljYXRlIHRo ZQpwb2ludGVyIGFuZCBiZSBkb25lIGlmIHlvdSB3YW50IHRvIG1hcmsgdGhlIGFycmF5IG9mIG5h bWVzIGFzIGNvbnN0IG9yCmhhdmUgeW91ciBjb25zdCAnc2V0dXAnIHN0cnVjdHVyZSBwb2ludCB0 byB0aGUgYnVsa19kYXRhIGFycmF5IHRoYXQgeW91CmRlZmluZSBzdGF0aWNhbGx5IG5vbi1jb25z dCBzb21ld2hlcmUgZ2xvYmFsbHkuCl9fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fCmRyaS1kZXZlbCBtYWlsaW5nIGxpc3QKZHJpLWRldmVsQGxpc3RzLmZyZWVk ZXNrdG9wLm9yZwpodHRwczovL2xpc3RzLmZyZWVkZXNrdG9wLm9yZy9tYWlsbWFuL2xpc3RpbmZv L2RyaS1kZXZlbAo= From mboxrd@z Thu Jan 1 00:00:00 1970 From: sboyd@kernel.org (Stephen Boyd) Date: Thu, 15 Mar 2018 15:55:09 -0700 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: <152115450981.111154.2342657732967302796@swboyd.mtv.corp.google.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Quoting Marek Szyprowski (2018-02-20 01:36:03) > Hi Robin, > > On 2018-02-19 17:29, Robin Murphy wrote: > > > > 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[]); > ... > If you have an array of pointers to names of clks then we can put the struct clk pointers adjacent to them at the same time. I suppose the problem there is that some drivers want to mark that array of pointers to names as const. And then we can't update the clk pointers next to them. This is the same design that regulators has done so that's why it's written like this for clks. Honestly, we're talking about a handful of pointers here so I'm not sure it really matters much. Just duplicate the pointer and be done if you want to mark the array of names as const or have your const 'setup' structure point to the bulk_data array that you define statically non-const somewhere globally.