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 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 =)

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 ~~~