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

[Stephen Boyd]

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.