diff mbox series

[v6,03/12] regulator: Move OF-specific regulator lookup code to of_regulator.c

Message ID 20240904090016.2841572-4-wenst@chromium.org
State Not Applicable
Delegated to: Wolfram Sang
Headers show
Series platform/chrome: Introduce DT hardware prober | expand

Commit Message

Chen-Yu Tsai Sept. 4, 2024, 9 a.m. UTC 
There's still a bit of OF-specific code in the regulator device lookup
function.

Move those bits of code over to of_regulator.c, and create a new
function of_regulator_dev_lookup() to encapsulate the code moved out of
regulator_dev_lookup().

Also mark of_find_regulator_by_node() as static, since there are no
other users in other compile units.

There are no functional changes. A line alignment was also fixed.

Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
---
Changes since v5:
- Fix kerneldoc format of of_regulator_dev_lookup()
- Fix stub compile error for !CONFIG_OF in drivers/regulator/internal.h

Changes since v4:
- New patch
---
 drivers/regulator/core.c         |  87 ++----------------------
 drivers/regulator/internal.h     |   9 +--
 drivers/regulator/of_regulator.c | 110 ++++++++++++++++++++++++++++++-
 3 files changed, 119 insertions(+), 87 deletions(-)

Comments

Andy Shevchenko Sept. 4, 2024, 1:09 p.m. UTC | #1 
On Wed, Sep 04, 2024 at 05:00:05PM +0800, Chen-Yu Tsai wrote:
> There's still a bit of OF-specific code in the regulator device lookup
> function.
> 
> Move those bits of code over to of_regulator.c, and create a new
> function of_regulator_dev_lookup() to encapsulate the code moved out of
> regulator_dev_lookup().
> 
> Also mark of_find_regulator_by_node() as static, since there are no
> other users in other compile units.
> 
> There are no functional changes. A line alignment was also fixed.

...

> +/**
> + * of_get_child_regulator - get a child regulator device node
> + * based on supply name
> + * @parent: Parent device node
> + * @prop_name: Combination regulator supply name and "-supply"
> + *
> + * Traverse all child nodes.
> + * Extract the child regulator device node corresponding to the supply name.
> + *
> + * Return: Pointer to the &struct device_node corresponding to the regulator
> + *	   if found, or %NULL if not found.
> + */
> +static struct device_node *of_get_child_regulator(struct device_node *parent,
> +						  const char *prop_name)
> +{
> +	struct device_node *regnode = NULL;
> +	struct device_node *child = NULL;
> +
> +	for_each_child_of_node(parent, child) {

> +		regnode = of_parse_phandle(child, prop_name, 0);
> +		if (!regnode) {
> +			regnode = of_get_child_regulator(child, prop_name);
> +			if (regnode)
> +				goto err_node_put;
> +		} else {
> +			goto err_node_put;
> +		}

I know this is just a move of the existing code, but consider negating the
conditional and have something like

		regnode = of_parse_phandle(child, prop_name, 0);
		if (regnode)
			goto err_node_put;

		regnode = of_get_child_regulator(child, prop_name);
		if (regnode)
			goto err_node_put;

> +	}
> +	return NULL;
> +
> +err_node_put:
> +	of_node_put(child);
> +	return regnode;
> +}

...

I assume the use of _scoped() macros is in mind for the future changes?

...

> +/**
> + * of_get_regulator - get a regulator device node based on supply name
> + * @dev: Device pointer for the consumer (of regulator) device
> + * @supply: regulator supply name
> + *
> + * Extract the regulator device node corresponding to the supply name.
> + *
> + * Return: Pointer to the &struct device_node corresponding to the regulator
> + *	   if found, or %NULL if not found.
> + */
> +static struct device_node *of_get_regulator(struct device *dev, const char *supply)
> +{
> +	struct device_node *regnode = NULL;
> +	char prop_name[64]; /* 64 is max size of property name */
> +
> +	dev_dbg(dev, "Looking up %s-supply from device tree\n", supply);
> +
> +	snprintf(prop_name, 64, "%s-supply", supply);
> +	regnode = of_parse_phandle(dev->of_node, prop_name, 0);

> +	if (!regnode) {

Similarly here

	snprintf(prop_name, 64, "%s-supply", supply);
	regnode = of_parse_phandle(dev->of_node, prop_name, 0);
	if (regnode)
		return regnode;


> +		regnode = of_get_child_regulator(dev->of_node, prop_name);
> +		if (regnode)
> +			return regnode;
> +
> +		dev_dbg(dev, "Looking up %s property in node %pOF failed\n",
> +			prop_name, dev->of_node);
> +		return NULL;
> +	}
> +	return regnode;

	regnode = of_get_child_regulator(dev->of_node, prop_name);
	if (regnode)
		return regnode;

	dev_dbg(dev, "Looking up %s property in node %pOF failed\n", prop_name, dev->of_node);
	return NULL;

> +}
Andy Shevchenko Sept. 4, 2024, 1:14 p.m. UTC | #2 
On Wed, Sep 04, 2024 at 04:09:26PM +0300, Andy Shevchenko wrote:
> On Wed, Sep 04, 2024 at 05:00:05PM +0800, Chen-Yu Tsai wrote:

> > +static struct device_node *of_get_child_regulator(struct device_node *parent,
> > +						  const char *prop_name)
> > +{
> > +	struct device_node *regnode = NULL;

> > +	struct device_node *child = NULL;

Btw, redundant assignment here, as child will be assigned anyway AFAIR.

> > +	for_each_child_of_node(parent, child) {
> 
> > +		regnode = of_parse_phandle(child, prop_name, 0);
> > +		if (!regnode) {
> > +			regnode = of_get_child_regulator(child, prop_name);
> > +			if (regnode)
> > +				goto err_node_put;
> > +		} else {
> > +			goto err_node_put;
> > +		}
> 
> I know this is just a move of the existing code, but consider negating the
> conditional and have something like
> 
> 		regnode = of_parse_phandle(child, prop_name, 0);
> 		if (regnode)
> 			goto err_node_put;
> 
> 		regnode = of_get_child_regulator(child, prop_name);
> 		if (regnode)
> 			goto err_node_put;
> 
> > +	}
> > +	return NULL;
> > +
> > +err_node_put:
> > +	of_node_put(child);
> > +	return regnode;
> > +}
Chen-Yu Tsai Sept. 5, 2024, 8:11 a.m. UTC | #3 
On Wed, Sep 4, 2024 at 9:16 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Wed, Sep 04, 2024 at 04:09:26PM +0300, Andy Shevchenko wrote:
> > On Wed, Sep 04, 2024 at 05:00:05PM +0800, Chen-Yu Tsai wrote:
>
> > > +static struct device_node *of_get_child_regulator(struct device_node *parent,
> > > +                                             const char *prop_name)
> > > +{
> > > +   struct device_node *regnode = NULL;
>
> > > +   struct device_node *child = NULL;
>
> Btw, redundant assignment here, as child will be assigned anyway AFAIR.
>
> > > +   for_each_child_of_node(parent, child) {
> >
> > > +           regnode = of_parse_phandle(child, prop_name, 0);
> > > +           if (!regnode) {
> > > +                   regnode = of_get_child_regulator(child, prop_name);
> > > +                   if (regnode)
> > > +                           goto err_node_put;
> > > +           } else {
> > > +                   goto err_node_put;
> > > +           }
> >
> > I know this is just a move of the existing code, but consider negating the
> > conditional and have something like
> >
> >               regnode = of_parse_phandle(child, prop_name, 0);
> >               if (regnode)
> >                       goto err_node_put;
> >
> >               regnode = of_get_child_regulator(child, prop_name);
> >               if (regnode)
> >                       goto err_node_put;
> >

Looks like Mark already merged this one. I'll send extra patches to clean
this up later.

ChenYu

> > > +   }
> > > +   return NULL;
> > > +
> > > +err_node_put:
> > > +   of_node_put(child);
> > > +   return regnode;
> > > +}
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
Andy Shevchenko Sept. 5, 2024, 8:29 a.m. UTC | #4 
On Thu, Sep 05, 2024 at 04:11:18PM +0800, Chen-Yu Tsai wrote:
> On Wed, Sep 4, 2024 at 9:16 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:

...

> Looks like Mark already merged this one. I'll send extra patches to clean
> this up later.

I was OOF, haven't read this and I sent already a patch yesterday evening.
diff mbox series

Patch

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index c1d11924d892..835a5531d045 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -421,74 +421,6 @@  static void regulator_lock_dependent(struct regulator_dev *rdev,
 	mutex_unlock(&regulator_list_mutex);
 }
 
-/**
- * of_get_child_regulator - get a child regulator device node
- * based on supply name
- * @parent: Parent device node
- * @prop_name: Combination regulator supply name and "-supply"
- *
- * Traverse all child nodes.
- * Extract the child regulator device node corresponding to the supply name.
- *
- * Return: Pointer to the &struct device_node corresponding to the regulator
- *	   if found, or %NULL if not found.
- */
-static struct device_node *of_get_child_regulator(struct device_node *parent,
-						  const char *prop_name)
-{
-	struct device_node *regnode = NULL;
-	struct device_node *child = NULL;
-
-	for_each_child_of_node(parent, child) {
-		regnode = of_parse_phandle(child, prop_name, 0);
-
-		if (!regnode) {
-			regnode = of_get_child_regulator(child, prop_name);
-			if (regnode)
-				goto err_node_put;
-		} else {
-			goto err_node_put;
-		}
-	}
-	return NULL;
-
-err_node_put:
-	of_node_put(child);
-	return regnode;
-}
-
-/**
- * of_get_regulator - get a regulator device node based on supply name
- * @dev: Device pointer for the consumer (of regulator) device
- * @supply: regulator supply name
- *
- * Extract the regulator device node corresponding to the supply name.
- *
- * Return: Pointer to the &struct device_node corresponding to the regulator
- *	   if found, or %NULL if not found.
- */
-static struct device_node *of_get_regulator(struct device *dev, const char *supply)
-{
-	struct device_node *regnode = NULL;
-	char prop_name[64]; /* 64 is max size of property name */
-
-	dev_dbg(dev, "Looking up %s-supply from device tree\n", supply);
-
-	snprintf(prop_name, 64, "%s-supply", supply);
-	regnode = of_parse_phandle(dev->of_node, prop_name, 0);
-
-	if (!regnode) {
-		regnode = of_get_child_regulator(dev->of_node, prop_name);
-		if (regnode)
-			return regnode;
-
-		dev_dbg(dev, "Looking up %s property in node %pOF failed\n",
-				prop_name, dev->of_node);
-		return NULL;
-	}
-	return regnode;
-}
-
 /* Platform voltage constraint check */
 int regulator_check_voltage(struct regulator_dev *rdev,
 			    int *min_uV, int *max_uV)
@@ -2021,7 +1953,6 @@  static struct regulator_dev *regulator_dev_lookup(struct device *dev,
 						  const char *supply)
 {
 	struct regulator_dev *r = NULL;
-	struct device_node *node;
 	struct regulator_map *map;
 	const char *devname = NULL;
 
@@ -2029,19 +1960,11 @@  static struct regulator_dev *regulator_dev_lookup(struct device *dev,
 
 	/* first do a dt based lookup */
 	if (dev && dev->of_node) {
-		node = of_get_regulator(dev, supply);
-		if (node) {
-			r = of_find_regulator_by_node(node);
-			of_node_put(node);
-			if (r)
-				return r;
-
-			/*
-			 * We have a node, but there is no device.
-			 * assume it has not registered yet.
-			 */
-			return ERR_PTR(-EPROBE_DEFER);
-		}
+		r = of_regulator_dev_lookup(dev, supply);
+		if (!IS_ERR(r))
+			return r;
+		if (PTR_ERR(r) == -EPROBE_DEFER)
+			return r;
 	}
 
 	/* if not found, try doing it non-dt way */
diff --git a/drivers/regulator/internal.h b/drivers/regulator/internal.h
index 77a502141089..8e5506c5ee94 100644
--- a/drivers/regulator/internal.h
+++ b/drivers/regulator/internal.h
@@ -66,7 +66,8 @@  static inline struct regulator_dev *dev_to_rdev(struct device *dev)
 }
 
 #ifdef CONFIG_OF
-struct regulator_dev *of_find_regulator_by_node(struct device_node *np);
+struct regulator_dev *of_regulator_dev_lookup(struct device *dev,
+					      const char *supply);
 struct regulator_init_data *regulator_of_get_init_data(struct device *dev,
 			         const struct regulator_desc *desc,
 				 struct regulator_config *config,
@@ -80,10 +81,10 @@  int of_get_n_coupled(struct regulator_dev *rdev);
 bool of_check_coupling_data(struct regulator_dev *rdev);
 
 #else
-static inline struct regulator_dev *
-of_find_regulator_by_node(struct device_node *np)
+static inline struct regulator_dev *of_regulator_dev_lookup(struct device *dev,
+							    const char *supply)
 {
-	return NULL;
+	return ERR_PTR(-ENODEV);
 }
 
 static inline struct regulator_init_data *
diff --git a/drivers/regulator/of_regulator.c b/drivers/regulator/of_regulator.c
index cec8c3647a00..d5dd7a9e577b 100644
--- a/drivers/regulator/of_regulator.c
+++ b/drivers/regulator/of_regulator.c
@@ -552,7 +552,75 @@  struct regulator_init_data *regulator_of_get_init_data(struct device *dev,
 	return NULL;
 }
 
-struct regulator_dev *of_find_regulator_by_node(struct device_node *np)
+/**
+ * of_get_child_regulator - get a child regulator device node
+ * based on supply name
+ * @parent: Parent device node
+ * @prop_name: Combination regulator supply name and "-supply"
+ *
+ * Traverse all child nodes.
+ * Extract the child regulator device node corresponding to the supply name.
+ *
+ * Return: Pointer to the &struct device_node corresponding to the regulator
+ *	   if found, or %NULL if not found.
+ */
+static struct device_node *of_get_child_regulator(struct device_node *parent,
+						  const char *prop_name)
+{
+	struct device_node *regnode = NULL;
+	struct device_node *child = NULL;
+
+	for_each_child_of_node(parent, child) {
+		regnode = of_parse_phandle(child, prop_name, 0);
+
+		if (!regnode) {
+			regnode = of_get_child_regulator(child, prop_name);
+			if (regnode)
+				goto err_node_put;
+		} else {
+			goto err_node_put;
+		}
+	}
+	return NULL;
+
+err_node_put:
+	of_node_put(child);
+	return regnode;
+}
+
+/**
+ * of_get_regulator - get a regulator device node based on supply name
+ * @dev: Device pointer for the consumer (of regulator) device
+ * @supply: regulator supply name
+ *
+ * Extract the regulator device node corresponding to the supply name.
+ *
+ * Return: Pointer to the &struct device_node corresponding to the regulator
+ *	   if found, or %NULL if not found.
+ */
+static struct device_node *of_get_regulator(struct device *dev, const char *supply)
+{
+	struct device_node *regnode = NULL;
+	char prop_name[64]; /* 64 is max size of property name */
+
+	dev_dbg(dev, "Looking up %s-supply from device tree\n", supply);
+
+	snprintf(prop_name, 64, "%s-supply", supply);
+	regnode = of_parse_phandle(dev->of_node, prop_name, 0);
+
+	if (!regnode) {
+		regnode = of_get_child_regulator(dev->of_node, prop_name);
+		if (regnode)
+			return regnode;
+
+		dev_dbg(dev, "Looking up %s property in node %pOF failed\n",
+			prop_name, dev->of_node);
+		return NULL;
+	}
+	return regnode;
+}
+
+static struct regulator_dev *of_find_regulator_by_node(struct device_node *np)
 {
 	struct device *dev;
 
@@ -561,6 +629,46 @@  struct regulator_dev *of_find_regulator_by_node(struct device_node *np)
 	return dev ? dev_to_rdev(dev) : NULL;
 }
 
+/**
+ * of_regulator_dev_lookup - lookup a regulator device with device tree only
+ * @dev: Device pointer for regulator supply lookup.
+ * @supply: Supply name or regulator ID.
+ *
+ * Return: Pointer to the &struct regulator_dev on success, or ERR_PTR()
+ *	   encoded value on error.
+ *
+ * If successful, returns a pointer to the &struct regulator_dev that
+ * corresponds to the name @supply and with the embedded &struct device
+ * refcount incremented by one. The refcount must be dropped by calling
+ * put_device().
+ *
+ * On failure one of the following ERR_PTR() encoded values is returned:
+ * * -%ENODEV if lookup fails permanently.
+ * * -%EPROBE_DEFER if lookup could succeed in the future.
+ */
+struct regulator_dev *of_regulator_dev_lookup(struct device *dev,
+					      const char *supply)
+{
+	struct regulator_dev *r;
+	struct device_node *node;
+
+	node = of_get_regulator(dev, supply);
+	if (node) {
+		r = of_find_regulator_by_node(node);
+		of_node_put(node);
+		if (r)
+			return r;
+
+		/*
+		 * We have a node, but there is no device.
+		 * assume it has not registered yet.
+		 */
+		return ERR_PTR(-EPROBE_DEFER);
+	}
+
+	return ERR_PTR(-ENODEV);
+}
+
 /*
  * Returns number of regulators coupled with rdev.
  */