On Wed, 2021-03-31 at 12:25 -0700, Stephen Boyd wrote:
> Quoting Geert Uytterhoeven (2021-03-31 00:05:00)
> > On Wed, Mar 31, 2021 at 4:22 AM Stephen Boyd <sboyd@kernel.org> wrote:
> > > > > Does it have any use?
> > > > 
> > > > of_clk_del_provider() removes the first provider found with node == NULL.
> > > > If there are two drivers calling of_clk_add_hw_provider(), and one of
> > > > hem calls of_clk_del_provider() later, the wrong provider may be
> > > > removed from the list.
> > > > 
> > > 
> > > So you're saying we shouldn't add a NULL device node pointer to the list
> > > so that this can't happen? That doesn't mean returning an error from
> > > of_clk_add_hw_provider() would be useful though.
> > > of_clk_add_hw_provider() can return 0 if np == NULL and
> > > of_clk_del_provider() can return early if np == NULL too.
> > 
> > I don't know if I grasp all meanings of the above.
> > 
> > The main question is if it is valid for a driver to call
> > of_clk_add_hw_provider()
> > with np == NULL.
> >   - If yes, should that register the provider?
> 
> No it should not register the provider. That would be bad as you pointed
> out.
> 
> >       - If yes, how to handle two drivers calling of_clk_add_hw_provider()
> >         with np = NULL, as their unregistration order is not guaranteed to
> >         be correct.
> > 
> > If no, is that something to ignore (0), or a bug (error)?
> 
> This is my question above. Is there a use to having
> of_clk_add_hw_provider() return an error value when np == NULL? I doubt
> it.
> 
> Returning 0 would reduce the if conditions in driver code in this case
> and be consistent with the CONFIG_OF=n inline stub that returns 0 when
> CONFIG_OF is disabled. The only case an error would be returned is if we
> couldn't allocate memory or if the assigned clocks code failed. Seems
> sane to me. The downside is that drivers would maybe register clkdev
> lookups when they don't need to and waste some memory. I'm fine with
> that until we have some sort of non-DT based clk provider lookup
> mechanism that could unify the two methods.

What about devm_of_clk_add_hw_provider() users, do we care that a seemingly
empty managed resource will be created?

Regards,
Nicolas