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

[Stephen Boyd <sboyd@kernel.org>]

Quoting Matti Vaittinen (2018-12-04 23:00:46)
> Hello Stephen,
> 
> I copied some parts of the v4 discussion here as well. Let's continue
> them under this one email thread. (and yep, this is my bad we now have
> multiple email threads - I did these new patches without waiting for
> the final conclusion. I should try to be more patient in the future...)
> 
> > > > I think we should use parent device's node, not the paren node in DT,        
> > > > right? But I agree, we should only look "one level up in the chain".         
>                                                                                  
> > > Are these two things different? I'm suggesting looking at                      
> > > device_node::parent and trying to find a #clock-cells property.                
>                                                                                  
> > I thought that MFD sub-devices may completely lack the DT node but I
> > will verify this tomorrow.
> 
> So yep. It appears that the DT node for MFD sub-device is left NULL.
> This is quite logical as there really is no clk sub-node in MFD (pmic)
> node. The option to "hack around" this would be setting the of_node to
> parent node in driver code. But this feels wrong. Drivers should not
> mess with the "dt node ownership" - and it also feels a bit odd when
> many devices use same DT node. I think we may hit in problems when
> obtaining resources or doing reference counting. Hence I think we should
> keep the of_node NULL for sub-device if the sub-device does not have own
> node inside the main devie node. And I think Rob was not a fan of having
> own nodes for sub-devices inside the MFD node (AFAIR my first driver
> draft for this device had it but Rob and you thought that was not correct).

Yes let's not change this.

> > > @@ -3901,8 +3906,11 @@ static void devm_of_clk_release_provider(struct device *dev, void *res)
> > >   *
> > >   * Returns 0 on success or an errno on failure.
> > >   *
> > > - * Registers clock provider for given device's node. Provider is automatically
> > > - * released at device exit.
> > > + * Registers clock provider for given device's node. If the device has no DT
> > > + * node or if the device node lacks of clock provider information (#clock-cells)
> > > + * then the parent device's node is scanned for this information. If parent node
> > > + * has the #clock-cells then it is used in registration. Provider is
> > > + * automatically released at device exit.
> > >   */
> > >  int devm_of_clk_add_hw_provider(struct device *dev,
> > >                         struct clk_hw *(*get)(struct of_phandle_args *clkspec,
> > > @@ -3912,12 +3920,17 @@ 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;
> > 
> > As said on v5, let's just modify of_clk_add_provider() to do the parent
> > search.
> 
> 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?

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