Re: [PATCH] clk: bulk: call of_clk_get() when id is NULL

[Stephen Boyd <sboyd@codeaurora.org>]

On 08/30, A.s. Dong wrote:
> > -----Original Message-----
> > From: Shawn Guo [mailto:shawnguo@kernel.org]
> > Sent: Sunday, August 13, 2017 10:19 PM
> > To: Stephen Boyd
> > Cc: Michael Turquette; A.s. Dong; linux-clk@vger.kernel.org; linux-arm-
> > kernel@lists.infradead.org; Shawn Guo
> > Subject: Re: [PATCH] clk: bulk: call of_clk_get() when id is NULL
> > 
> > Hi Stephen,
> > 
> > On Wed, Aug 09, 2017 at 10:33:18AM -0700, Stephen Boyd wrote:
> > > > diff --git a/drivers/clk/clk-bulk.c b/drivers/clk/clk-bulk.c index
> > > > c834f5abfc49..65cee595a67e 100644
> > > > --- a/drivers/clk/clk-bulk.c
> > > > +++ b/drivers/clk/clk-bulk.c
> > > > @@ -39,7 +39,10 @@ int __must_check clk_bulk_get(struct device *dev,
> > int num_clks,
> > > >  		clks[i].clk = NULL;
> > > >
> > > >  	for (i = 0; i < num_clks; i++) {
> > > > -		clks[i].clk = clk_get(dev, clks[i].id);
> > > > +		if (clks[i].id)
> > > > +			clks[i].clk = clk_get(dev, clks[i].id);
> > > > +		else if (dev->of_node)
> > > > +			clks[i].clk = of_clk_get(dev->of_node, i);
> > >
> > > This seems a little too magical. The omission of an id in an array of
> > > clks would mean that only that one clk is acquired through
> > > of_clk_get(). We could have a mixture of ids and no ids for some
> > > device, and then do very odd things.
> > >
> > > How about we add a flag to clk_bulk_data that indicates we want it to
> > > use of_clk_get() instead of clk_get() for all of the clks?
> > > Then the id is ignored for the entire function.
> > 
> > I have sent v2 to add a flag to clk_bulk_data to address your comments.
> > But I just found it might not be what you want exactly, because the
> > current bulk clk APIs defines clk_bulk_data in a different way from what
> > you are asking here.
> > 
> > It seems that you think clk_bulk_data represents all the clocks that bulk
> > APIs manage, but actually it only represents one of the multiple clocks.
> > Callers need to prepare an array of clk_bulk_data to represent multiple
> > clocks.  That said, the flag we add to clk_bulk_data only affects one
> > clock instead of all clocks.
> > 
> > I feel this is something we can improve.  clk_bulk_data can be used to
> > represent all clocks, something like below.  In this case, bulk clk APIs
> > do not need to take num_clks as a separate parameter, and callers do not
> > need to maintain multiple clk_bulk_data instances.  Since we do not have
> > any in tree users of bulk clk APIs yet, we can make this semantic change
> > quickly.  Thoughts?
> > 
> 
> Flags may be better used for extending per clk feature support in the future.
> 
> IMHO how about implement a of_clk_bulk_get() to handle this issue as we already
> have a of_clk_get?
> e.g.
> 
> int __must_check of_clk_bulk_get(struct device_node *np, int num_clks,
>                               struct clk_bulk_data *clks)
> {
>         for (i = 0; i < num_clks; i++) {
>                 clks[i].clk = of_clk_get(dev, clks[i].id);
> ...
> 
> }
> 
> Then it's less intrusive.
> 

This looks ok too. Go ahead and send this version instead.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project