[PATCH] clk: Use parent node pointer during registration if necessary

[PATCH] clk: Use parent node pointer during registration if necessary

[Stephen Boyd]

Sometimes clk drivers are attached to devices which are children of a
parent device that is connected to a node in DT. This happens when
devices are MFD-ish and the parent device driver mostly registers child
devices to match against drivers placed in their respective subsystem
directories like drivers/clk, drivers/regulator, etc. When the clk
driver calls clk_register() with a device pointer, that struct device
pointer won't have a device_node associated with it because it was
created purely in software as a way to partition logic to a subsystem.

This causes problems for the way we find parent clks for the clks
registered by these child devices because we look at the registering
device's device_node pointer to lookup 'clocks' and 'clock-names'
properties. Let's use the parent device's device_node pointer if the
registering device doesn't have a device_node but the parent does. This
simplifies clk registration code by avoiding the need to assign some
device_node to the device registering the clk.

Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
Reported-by: Niklas Cassel <niklas.cassel@linaro.org>
Signed-off-by: Stephen Boyd <sboyd@kernel.org>
---

I decided to introduce a new function instead of trying to jam it all
in the one line where we assign np. This way the function gets the 
true 'np' as an argument all the time.

 drivers/clk/clk.c | 27 +++++++++++++++++++++++++--
 1 file changed, 25 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index b68e200829f2..a743fffe8e46 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -3719,6 +3719,28 @@ __clk_register(struct device *dev, struct device_node *np, struct clk_hw *hw)
 	return ERR_PTR(ret);
 }
 
+/**
+ * dev_or_parent_of_node - Get device node of @dev or @dev's parent
+ * @dev: Device to get device node of
+ *
+ * Returns: device node pointer of @dev, or the device node pointer of
+ * @dev->parent if dev doesn't have a device node, or NULL if neither
+ * @dev or @dev->parent have a device node.
+ */
+static struct device_node *dev_or_parent_of_node(struct device *dev)
+{
+	struct device_node *np;
+
+	if (!dev)
+		return NULL;
+
+	np = dev_of_node(dev);
+	if (!np)
+		np = dev_of_node(dev->parent);
+
+	return np;
+}
+
 /**
  * clk_register - allocate a new clock, register it and return an opaque cookie
  * @dev: device that is registering this clock
@@ -3734,7 +3756,7 @@ __clk_register(struct device *dev, struct device_node *np, struct clk_hw *hw)
  */
 struct clk *clk_register(struct device *dev, struct clk_hw *hw)
 {
-	return __clk_register(dev, dev_of_node(dev), hw);
+	return __clk_register(dev, dev_or_parent_of_node(dev), hw);
 }
 EXPORT_SYMBOL_GPL(clk_register);
 
@@ -3750,7 +3772,8 @@ EXPORT_SYMBOL_GPL(clk_register);
  */
 int clk_hw_register(struct device *dev, struct clk_hw *hw)
 {
-	return PTR_ERR_OR_ZERO(__clk_register(dev, dev_of_node(dev), hw));
+	return PTR_ERR_OR_ZERO(__clk_register(dev, dev_or_parent_of_node(dev),
+			       hw));
 }
 EXPORT_SYMBOL_GPL(clk_hw_register);
 

base-commit: e42617b825f8073569da76dc4510bfa019b1c35a
-- 
Sent by a computer, using git, on the internet

Re: [PATCH] clk: Use parent node pointer during registration if necessary

[Niklas Cassel]

On Mon, Dec 30, 2019 at 11:04:55AM -0800, Stephen Boyd wrote:
> Sometimes clk drivers are attached to devices which are children of a
> parent device that is connected to a node in DT. This happens when
> devices are MFD-ish and the parent device driver mostly registers child
> devices to match against drivers placed in their respective subsystem
> directories like drivers/clk, drivers/regulator, etc. When the clk
> driver calls clk_register() with a device pointer, that struct device
> pointer won't have a device_node associated with it because it was
> created purely in software as a way to partition logic to a subsystem.
> 
> This causes problems for the way we find parent clks for the clks
> registered by these child devices because we look at the registering
> device's device_node pointer to lookup 'clocks' and 'clock-names'
> properties. Let's use the parent device's device_node pointer if the
> registering device doesn't have a device_node but the parent does. This
> simplifies clk registration code by avoiding the need to assign some
> device_node to the device registering the clk.
> 
> Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> Reported-by: Niklas Cassel <niklas.cassel@linaro.org>
> Signed-off-by: Stephen Boyd <sboyd@kernel.org>
> ---
> 
> I decided to introduce a new function instead of trying to jam it all
> in the one line where we assign np. This way the function gets the 
> true 'np' as an argument all the time.
> 
>  drivers/clk/clk.c | 27 +++++++++++++++++++++++++--
>  1 file changed, 25 insertions(+), 2 deletions(-)
> 
> 
> base-commit: e42617b825f8073569da76dc4510bfa019b1c35a
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index b68e200829f2..a743fffe8e46 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -3719,6 +3719,28 @@ __clk_register(struct device *dev, struct device_node *np, struct clk_hw *hw)
>  	return ERR_PTR(ret);
>  }
>  
> +/**
> + * dev_or_parent_of_node - Get device node of @dev or @dev's parent
> + * @dev: Device to get device node of
> + *
> + * Returns: device node pointer of @dev, or the device node pointer of
> + * @dev->parent if dev doesn't have a device node, or NULL if neither
> + * @dev or @dev->parent have a device node.
> + */
> +static struct device_node *dev_or_parent_of_node(struct device *dev)
> +{
> +	struct device_node *np;
> +
> +	if (!dev)
> +		return NULL;
> +
> +	np = dev_of_node(dev);
> +	if (!np)
> +		np = dev_of_node(dev->parent);
> +
> +	return np;
> +}
> +
>  /**
>   * clk_register - allocate a new clock, register it and return an opaque cookie
>   * @dev: device that is registering this clock
> @@ -3734,7 +3756,7 @@ __clk_register(struct device *dev, struct device_node *np, struct clk_hw *hw)
>   */
>  struct clk *clk_register(struct device *dev, struct clk_hw *hw)
>  {
> -	return __clk_register(dev, dev_of_node(dev), hw);
> +	return __clk_register(dev, dev_or_parent_of_node(dev), hw);
>  }
>  EXPORT_SYMBOL_GPL(clk_register);
>  
> @@ -3750,7 +3772,8 @@ EXPORT_SYMBOL_GPL(clk_register);
>   */
>  int clk_hw_register(struct device *dev, struct clk_hw *hw)
>  {
> -	return PTR_ERR_OR_ZERO(__clk_register(dev, dev_of_node(dev), hw));
> +	return PTR_ERR_OR_ZERO(__clk_register(dev, dev_or_parent_of_node(dev),
> +			       hw));
>  }
>  EXPORT_SYMBOL_GPL(clk_hw_register);
>  

Reviewed-by: Niklas Cassel <nks@flawful.org>

Re: [PATCH] clk: Use parent node pointer during registration if necessary

[Bjorn Andersson]

On Mon 30 Dec 11:04 PST 2019, Stephen Boyd wrote:

> Sometimes clk drivers are attached to devices which are children of a
> parent device that is connected to a node in DT. This happens when
> devices are MFD-ish and the parent device driver mostly registers child
> devices to match against drivers placed in their respective subsystem
> directories like drivers/clk, drivers/regulator, etc. When the clk
> driver calls clk_register() with a device pointer, that struct device
> pointer won't have a device_node associated with it because it was
> created purely in software as a way to partition logic to a subsystem.
> 
> This causes problems for the way we find parent clks for the clks
> registered by these child devices because we look at the registering
> device's device_node pointer to lookup 'clocks' and 'clock-names'
> properties. Let's use the parent device's device_node pointer if the
> registering device doesn't have a device_node but the parent does. This
> simplifies clk registration code by avoiding the need to assign some
> device_node to the device registering the clk.
> 
> Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> Reported-by: Niklas Cassel <niklas.cassel@linaro.org>
> Signed-off-by: Stephen Boyd <sboyd@kernel.org>
> ---
> 
> I decided to introduce a new function instead of trying to jam it all
> in the one line where we assign np. This way the function gets the 
> true 'np' as an argument all the time.
> 

Looks better.

>  drivers/clk/clk.c | 27 +++++++++++++++++++++++++--
>  1 file changed, 25 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index b68e200829f2..a743fffe8e46 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -3719,6 +3719,28 @@ __clk_register(struct device *dev, struct device_node *np, struct clk_hw *hw)
>  	return ERR_PTR(ret);
>  }
>  
> +/**
> + * dev_or_parent_of_node - Get device node of @dev or @dev's parent

()

> + * @dev: Device to get device node of
> + *
> + * Returns: device node pointer of @dev, or the device node pointer of

Return: (no 's')


With that,

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Tested-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Regards,
Bjorn

> + * @dev->parent if dev doesn't have a device node, or NULL if neither
> + * @dev or @dev->parent have a device node.
> + */
> +static struct device_node *dev_or_parent_of_node(struct device *dev)
> +{
> +	struct device_node *np;
> +
> +	if (!dev)
> +		return NULL;
> +
> +	np = dev_of_node(dev);
> +	if (!np)
> +		np = dev_of_node(dev->parent);
> +
> +	return np;
> +}
> +
>  /**
>   * clk_register - allocate a new clock, register it and return an opaque cookie
>   * @dev: device that is registering this clock
> @@ -3734,7 +3756,7 @@ __clk_register(struct device *dev, struct device_node *np, struct clk_hw *hw)
>   */
>  struct clk *clk_register(struct device *dev, struct clk_hw *hw)
>  {
> -	return __clk_register(dev, dev_of_node(dev), hw);
> +	return __clk_register(dev, dev_or_parent_of_node(dev), hw);
>  }
>  EXPORT_SYMBOL_GPL(clk_register);
>  
> @@ -3750,7 +3772,8 @@ EXPORT_SYMBOL_GPL(clk_register);
>   */
>  int clk_hw_register(struct device *dev, struct clk_hw *hw)
>  {
> -	return PTR_ERR_OR_ZERO(__clk_register(dev, dev_of_node(dev), hw));
> +	return PTR_ERR_OR_ZERO(__clk_register(dev, dev_or_parent_of_node(dev),
> +			       hw));
>  }
>  EXPORT_SYMBOL_GPL(clk_hw_register);
>  
> 
> base-commit: e42617b825f8073569da76dc4510bfa019b1c35a
> -- 
> Sent by a computer, using git, on the internet
> 

Re: [PATCH] clk: Use parent node pointer during registration if necessary

[Stephen Boyd]

Quoting Bjorn Andersson (2020-01-02 23:39:26)
> On Mon 30 Dec 11:04 PST 2019, Stephen Boyd wrote:
> 
> > Sometimes clk drivers are attached to devices which are children of a
> > parent device that is connected to a node in DT. This happens when
> > devices are MFD-ish and the parent device driver mostly registers child
> > devices to match against drivers placed in their respective subsystem
> > directories like drivers/clk, drivers/regulator, etc. When the clk
> > driver calls clk_register() with a device pointer, that struct device
> > pointer won't have a device_node associated with it because it was
> > created purely in software as a way to partition logic to a subsystem.
> > 
> > This causes problems for the way we find parent clks for the clks
> > registered by these child devices because we look at the registering
> > device's device_node pointer to lookup 'clocks' and 'clock-names'
> > properties. Let's use the parent device's device_node pointer if the
> > registering device doesn't have a device_node but the parent does. This
> > simplifies clk registration code by avoiding the need to assign some
> > device_node to the device registering the clk.
> > 
> > Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> > Reported-by: Niklas Cassel <niklas.cassel@linaro.org>
> > Signed-off-by: Stephen Boyd <sboyd@kernel.org>
> > ---
> > 
> > I decided to introduce a new function instead of trying to jam it all
> > in the one line where we assign np. This way the function gets the 
> > true 'np' as an argument all the time.
> > 
> 
> Looks better.
> 
> >  drivers/clk/clk.c | 27 +++++++++++++++++++++++++--
> >  1 file changed, 25 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > index b68e200829f2..a743fffe8e46 100644
> > --- a/drivers/clk/clk.c
> > +++ b/drivers/clk/clk.c
> > @@ -3719,6 +3719,28 @@ __clk_register(struct device *dev, struct device_node *np, struct clk_hw *hw)
> >       return ERR_PTR(ret);
> >  }
> >  
> > +/**
> > + * dev_or_parent_of_node - Get device node of @dev or @dev's parent
> 
> ()

This has been left out historically in this file. I'll add it but I
guess I should really take a pass at updating the docs in the whole
file!

> 
> > + * @dev: Device to get device node of
> > + *
> > + * Returns: device node pointer of @dev, or the device node pointer of
> 
> Return: (no 's')

Thanks.

> 
> 
> With that,
> 
> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> Tested-by: Bjorn Andersson <bjorn.andersson@linaro.org>
>