Re: [PATCH 1/8] clk: Add clk_bulk_alloc functions

[Marek Szyprowski]

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 <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
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland