Re: [PATCH 1/8] clk: Add clk_bulk_alloc functions
From: Robin Murphy
Date: Tue Feb 20 2018 - 09:19:45 EST
Hi Marek,
On 20/02/18 09:36, Marek Szyprowski wrote:
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[]);
...
Yes, that's certainly a possibility; if on the other hand there are
desirable reasons for the encapsulation (personally, I do think it's
quite neat), then maybe num_clocks should get pushed down into
clk_bulk_data as well - then with dedicated alloc/free functions as
proposed here it could become a simple opaque cookie as far as callers
are concerned.
I also haven't looked into the origins of the bulk API design, though;
I've just been familiarising myself from the perspective of reviewing
general clk API usage in drivers.
Robin.
Signed-off-by: Maciej Purski <m.purski@xxxxxxxxxxx>
---
 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