Re: [PATCH v6 03/10] clk: of-provider: look at parent if registered device has no provider info

[Matti Vaittinen]

On Wed, Dec 05, 2018 at 08:20:58PM +0200, Matti Vaittinen wrote:
> On Wed, Dec 05, 2018 at 09:19:33AM -0800, Stephen Boyd wrote:
> > Quoting Matti Vaittinen (2018-12-04 23:00:46)
> > > But that won't solve the issue if we don't do "dirty hacks" in driver.
> > > The devm interface still only gets the device-pointer, not the DT node
> > > as argument. And if DT node for device is NULL (like in MFD cases) -
> > > then there is no parent node, only parent device with a node. For plain
> > > of_clk_add_provider() the driver can just give the parent's node pointer
> > > in cases where it knows it is the parent who has the provider data in
> > > DT. But our original problem is in devm interfaces.
> > > 
> > 
> > I was misunderstanding the MFD design. Should still work though, so I
> > squashed this into the patch to clean things up a bit. Does this work
> > for you?
> 
> This looks good to me. Especially changing the of_is_clk_provider to
> get_clk_provider_node which allows to remove some repetition. If you
> apply this then I will drop my patch from the series. Just please let me
> know. I will cook version 7 of the series at Friday - tomorrow is the
> independence day in Finland and I'll be offline =)

I see you already applied the follow-up patches to clk-next. Please note
that most of the clean-ups did require this functionality as they used
the parent DT node.

> 
> Thanks!
> 
> > 
> > ------8<-----
> > 
> > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > index bb689161f0f5..6ff852bda892 100644
> > --- a/drivers/clk/clk.c
> > +++ b/drivers/clk/clk.c
> > @@ -3893,9 +3893,23 @@ static void devm_of_clk_release_provider(struct device *dev, void *res)
> >  	of_clk_del_provider(*(struct device_node **)res);
> >  }
> >  
> > -static int of_is_clk_provider(struct device_node *np)
> > +/*
> > + * We allow a child device to use its parent device as the clock provider node
> > + * for cases like MFD sub-devices where the child device driver wants to use
> > + * devm_*() APIs but not list the device in DT as a sub-node.
> > + */
> > +static struct device_node *get_clk_provider_node(struct device *dev)
> >  {
> > -	return !!of_find_property(np, "#clock-cells", NULL);
> > +	struct device_node *np, *parent_np;
> > +
> > +	np = dev->of_node;
> > +	parent_np = dev->parent ? dev->parent->of_node : NULL;
> > +
> > +	if (!of_find_property(np, "#clock-cells", NULL))
> > +		if (of_find_property(parent_np, "#clock-cells", NULL))
> > +			np = parent_np;
> > +
> > +	return np;
> >  }
> >  
> >  /**
> > @@ -3920,17 +3934,12 @@ int devm_of_clk_add_hw_provider(struct device *dev,
> >  	struct device_node **ptr, *np;
> >  	int ret;
> >  
> > -	np = dev->of_node;
> > -
> > -	if (!of_is_clk_provider(dev->of_node))
> > -		if (of_is_clk_provider(dev->parent->of_node))
> > -			np = dev->parent->of_node;
> > -
> >  	ptr = devres_alloc(devm_of_clk_release_provider, sizeof(*ptr),
> >  			   GFP_KERNEL);
> >  	if (!ptr)
> >  		return -ENOMEM;
> >  
> > +	np = get_clk_provider_node(dev);
> >  	ret = of_clk_add_hw_provider(np, get, data);
> >  	if (!ret) {
> >  		*ptr = np;
> > @@ -3981,13 +3990,8 @@ static int devm_clk_provider_match(struct device *dev, void *res, void *data)
> >  void devm_of_clk_del_provider(struct device *dev)
> >  {
> >  	int ret;
> > -	struct device_node *np;
> > -
> > -	np = dev->of_node;
> > +	struct device_node *np = get_clk_provider_node(dev);
> >  
> > -	if (!of_is_clk_provider(dev->of_node))
> > -		if (of_is_clk_provider(dev->parent->of_node))
> > -			np = dev->parent->of_node;
> >  	ret = devres_release(dev, devm_of_clk_release_provider,
> >  			     devm_clk_provider_match, np);
> >  
> > 
> 
> -- 
> Matti Vaittinen
> ROHM Semiconductors
> 
> ~~~ "I don't think so," said Rene Descartes.  Just then, he vanished ~~~

-- 
Matti Vaittinen
ROHM Semiconductors

~~~ "I don't think so," said Rene Descartes.  Just then, he vanished ~~~