[PATCH 00/11] regulator updates

[PATCH 00/11] regulator updates

[Sascha Hauer]

This series is the result of a longer debugging session on a phyCORE
STM32MP1 board. It turned out that the 'regulator' command prints the
valid voltage range that a regulator has, but by no means makes sure
that a regulator is set to a voltage inside that range, but instead
happily enables a regulator with whatever default voltage it happens
to have.

On my Phytec board this resulted in the ethernet phy regulator being
enabled with a too low voltage with the effect that ethernet was working
with a very high package loss.

This series fixes that. Another bonus of this series is that the
regulator command now prints the regulators in a tree structure instead
of a plain list.

Sascha

Sascha Hauer (11):
  regulator: rename variable rd to rdev
  regulator: merge struct regulator_internal fields into struct
    regulator_dev
  regulator: introduce regulator logging functions.
  regulator: add regulator_get_voltage_internal()
  regulator: Add missing cases in regulator_map_voltage()
  regulator: stpmic1: add .get_voltage_sel
  regulator: stpmic1: add .supply_name
  regulator: register regulator as last step in of_regulator_register()
  regulator: Set initial voltage
  regulator: drop struct regulator_dev::supply_name
  regulator: print regulator tree

 drivers/regulator/bcm2835.c           |   2 +-
 drivers/regulator/core.c              | 377 +++++++++++++++-----------
 drivers/regulator/stpmic1_regulator.c |  11 +
 include/regulator.h                   |  24 +-
 4 files changed, 254 insertions(+), 160 deletions(-)

-- 
2.39.2

[PATCH 01/11] regulator: rename variable rd to rdev

[Sascha Hauer]

the struct regulator_dev * variable is mostly named 'rdev', but
sometimes 'rd' is used. Rename to 'rdev' consistently.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/regulator/core.c | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 97e76b0db9..e9b5b82dbe 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -166,13 +166,13 @@ static int regulator_resolve_supply(struct regulator_dev *rdev)
 	return 0;
 }
 
-static struct regulator_internal * __regulator_register(struct regulator_dev *rd, const char *name)
+static struct regulator_internal * __regulator_register(struct regulator_dev *rdev, const char *name)
 {
 	struct regulator_internal *ri;
 	int ret;
 
 	ri = xzalloc(sizeof(*ri));
-	ri->rdev = rd;
+	ri->rdev = rdev;
 
 	INIT_LIST_HEAD(&ri->consumer_list);
 
@@ -181,7 +181,7 @@ static struct regulator_internal * __regulator_register(struct regulator_dev *rd
 	if (name)
 		ri->name = xstrdup(name);
 
-	if (rd->boot_on || rd->always_on) {
+	if (rdev->boot_on || rdev->always_on) {
 		ret = regulator_resolve_supply(ri->rdev);
 		if (ret < 0)
 			goto err;
@@ -204,38 +204,38 @@ static struct regulator_internal * __regulator_register(struct regulator_dev *rd
 #ifdef CONFIG_OFDEVICE
 /*
  * of_regulator_register - register a regulator corresponding to a device_node
- * @rd:		the regulator device providing the ops
+ * rdev:		the regulator device providing the ops
  * @node:       the device_node this regulator corresponds to
  *
  * Return: 0 for success or a negative error code
  */
-int of_regulator_register(struct regulator_dev *rd, struct device_node *node)
+int of_regulator_register(struct regulator_dev *rdev, struct device_node *node)
 {
 	struct regulator_internal *ri;
 	const char *name;
 
-	if (!rd || !node)
+	if (!rdev || !node)
 		return -EINVAL;
 
-	rd->boot_on = of_property_read_bool(node, "regulator-boot-on");
-	rd->always_on = of_property_read_bool(node, "regulator-always-on");
+	rdev->boot_on = of_property_read_bool(node, "regulator-boot-on");
+	rdev->always_on = of_property_read_bool(node, "regulator-always-on");
 
 	name = of_get_property(node, "regulator-name", NULL);
 	if (!name)
 		name = node->name;
 
-	ri = __regulator_register(rd, name);
+	ri = __regulator_register(rdev, name);
 	if (IS_ERR(ri))
 		return PTR_ERR(ri);
 
 	ri->node = node;
-	node->dev = rd->dev;
+	node->dev = rdev->dev;
 
-	if (rd->desc->off_on_delay)
-		ri->enable_time_us = rd->desc->off_on_delay;
+	if (rdev->desc->off_on_delay)
+		ri->enable_time_us = rdev->desc->off_on_delay;
 
-	if (rd->desc->fixed_uV && rd->desc->n_voltages == 1)
-		ri->min_uv = ri->max_uv = rd->desc->fixed_uV;
+	if (rdev->desc->fixed_uV && rdev->desc->n_voltages == 1)
+		ri->min_uv = ri->max_uv = rdev->desc->fixed_uV;
 
 	of_property_read_u32(node, "regulator-enable-ramp-delay",
 			&ri->enable_time_us);
@@ -333,11 +333,11 @@ static struct regulator_internal *of_regulator_get(struct device *dev,
 }
 #endif
 
-int dev_regulator_register(struct regulator_dev *rd, const char * name, const char* supply)
+int dev_regulator_register(struct regulator_dev *rdev, const char * name, const char* supply)
 {
 	struct regulator_internal *ri;
 
-	ri = __regulator_register(rd, name);
+	ri = __regulator_register(rdev, name);
 
 	ri->supply = supply;
 
-- 
2.39.2

Re: [PATCH 01/11] regulator: rename variable rd to rdev

[Marco Felsch]

On 23-09-20, Sascha Hauer wrote:
> the struct regulator_dev * variable is mostly named 'rdev', but
> sometimes 'rd' is used. Rename to 'rdev' consistently.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>

Reviewed-by: Marco Felsch <m.felsch@pengutronix.de>

> ---
>  drivers/regulator/core.c | 32 ++++++++++++++++----------------
>  1 file changed, 16 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index 97e76b0db9..e9b5b82dbe 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -166,13 +166,13 @@ static int regulator_resolve_supply(struct regulator_dev *rdev)
>  	return 0;
>  }
>  
> -static struct regulator_internal * __regulator_register(struct regulator_dev *rd, const char *name)
> +static struct regulator_internal * __regulator_register(struct regulator_dev *rdev, const char *name)
>  {
>  	struct regulator_internal *ri;
>  	int ret;
>  
>  	ri = xzalloc(sizeof(*ri));
> -	ri->rdev = rd;
> +	ri->rdev = rdev;
>  
>  	INIT_LIST_HEAD(&ri->consumer_list);
>  
> @@ -181,7 +181,7 @@ static struct regulator_internal * __regulator_register(struct regulator_dev *rd
>  	if (name)
>  		ri->name = xstrdup(name);
>  
> -	if (rd->boot_on || rd->always_on) {
> +	if (rdev->boot_on || rdev->always_on) {
>  		ret = regulator_resolve_supply(ri->rdev);
>  		if (ret < 0)
>  			goto err;
> @@ -204,38 +204,38 @@ static struct regulator_internal * __regulator_register(struct regulator_dev *rd
>  #ifdef CONFIG_OFDEVICE
>  /*
>   * of_regulator_register - register a regulator corresponding to a device_node
> - * @rd:		the regulator device providing the ops
> + * rdev:		the regulator device providing the ops
>   * @node:       the device_node this regulator corresponds to
>   *
>   * Return: 0 for success or a negative error code
>   */
> -int of_regulator_register(struct regulator_dev *rd, struct device_node *node)
> +int of_regulator_register(struct regulator_dev *rdev, struct device_node *node)
>  {
>  	struct regulator_internal *ri;
>  	const char *name;
>  
> -	if (!rd || !node)
> +	if (!rdev || !node)
>  		return -EINVAL;
>  
> -	rd->boot_on = of_property_read_bool(node, "regulator-boot-on");
> -	rd->always_on = of_property_read_bool(node, "regulator-always-on");
> +	rdev->boot_on = of_property_read_bool(node, "regulator-boot-on");
> +	rdev->always_on = of_property_read_bool(node, "regulator-always-on");
>  
>  	name = of_get_property(node, "regulator-name", NULL);
>  	if (!name)
>  		name = node->name;
>  
> -	ri = __regulator_register(rd, name);
> +	ri = __regulator_register(rdev, name);
>  	if (IS_ERR(ri))
>  		return PTR_ERR(ri);
>  
>  	ri->node = node;
> -	node->dev = rd->dev;
> +	node->dev = rdev->dev;
>  
> -	if (rd->desc->off_on_delay)
> -		ri->enable_time_us = rd->desc->off_on_delay;
> +	if (rdev->desc->off_on_delay)
> +		ri->enable_time_us = rdev->desc->off_on_delay;
>  
> -	if (rd->desc->fixed_uV && rd->desc->n_voltages == 1)
> -		ri->min_uv = ri->max_uv = rd->desc->fixed_uV;
> +	if (rdev->desc->fixed_uV && rdev->desc->n_voltages == 1)
> +		ri->min_uv = ri->max_uv = rdev->desc->fixed_uV;
>  
>  	of_property_read_u32(node, "regulator-enable-ramp-delay",
>  			&ri->enable_time_us);
> @@ -333,11 +333,11 @@ static struct regulator_internal *of_regulator_get(struct device *dev,
>  }
>  #endif
>  
> -int dev_regulator_register(struct regulator_dev *rd, const char * name, const char* supply)
> +int dev_regulator_register(struct regulator_dev *rdev, const char * name, const char* supply)
>  {
>  	struct regulator_internal *ri;
>  
> -	ri = __regulator_register(rd, name);
> +	ri = __regulator_register(rdev, name);
>  
>  	ri->supply = supply;
>  
> -- 
> 2.39.2
> 
> 
> 

[PATCH 02/11] regulator: merge struct regulator_internal fields into struct regulator_dev

[Sascha Hauer]

Each struct regulator_dev instance has a struct regulator_internal
associated with it. The idea was that core internal fields are seen
by the core only. In the end this is more confusing than helpful. We
have a ri->rdev link, but no rdev->ri link so that we can't get a
rdev from a ri pointer. We could add that link, but instead make the
whole stuff a bit easier by just merging everything from struct
regulator_internal to struct regulator_dev.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/regulator/core.c | 214 ++++++++++++++++++---------------------
 include/regulator.h      |   9 ++
 2 files changed, 106 insertions(+), 117 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index e9b5b82dbe..41a3378ac8 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -13,21 +13,8 @@
 
 static LIST_HEAD(regulator_list);
 
-struct regulator_internal {
-	struct list_head list;
-	struct device_node *node;
-	struct regulator_dev *rdev;
-	int enable_count;
-	int enable_time_us;
-	int min_uv;
-	int max_uv;
-	char *name;
-	const char *supply;
-	struct list_head consumer_list;
-};
-
 struct regulator {
-	struct regulator_internal *ri;
+	struct regulator_dev *rdev;
 	struct list_head list;
 	struct device *dev;
 };
@@ -41,13 +28,12 @@ static int regulator_map_voltage(struct regulator_dev *rdev, int min_uV,
 	return -ENOSYS;
 }
 
-static int regulator_enable_internal(struct regulator_internal *ri)
+static int regulator_enable_internal(struct regulator_dev *rdev)
 {
-	struct regulator_dev *rdev = ri->rdev;
 	int ret;
 
-	if (ri->enable_count) {
-		ri->enable_count++;
+	if (rdev->enable_count) {
+		rdev->enable_count++;
 		return 0;
 	}
 
@@ -59,30 +45,29 @@ static int regulator_enable_internal(struct regulator_internal *ri)
 	if (ret)
 		return ret;
 
-	ret = rdev->desc->ops->enable(ri->rdev);
+	ret = rdev->desc->ops->enable(rdev);
 	if (ret) {
 		regulator_disable(rdev->supply);
 		return ret;
 	}
 
-	if (ri->enable_time_us)
-		udelay(ri->enable_time_us);
+	if (rdev->enable_time_us)
+		udelay(rdev->enable_time_us);
 
-	ri->enable_count++;
+	rdev->enable_count++;
 
 	return 0;
 }
 
-static int regulator_disable_internal(struct regulator_internal *ri)
+static int regulator_disable_internal(struct regulator_dev *rdev)
 {
-	struct regulator_dev *rdev = ri->rdev;
 	int ret;
 
-	if (!ri->enable_count)
+	if (!rdev->enable_count)
 		return -EINVAL;
 
-	if (ri->enable_count > 1) {
-		ri->enable_count--;
+	if (rdev->enable_count > 1) {
+		rdev->enable_count--;
 		return 0;
 	}
 
@@ -97,15 +82,14 @@ static int regulator_disable_internal(struct regulator_internal *ri)
 	if (ret)
 		return ret;
 
-	ri->enable_count--;
+	rdev->enable_count--;
 
-	return regulator_disable(ri->rdev->supply);
+	return regulator_disable(rdev->supply);
 }
 
-static int regulator_set_voltage_internal(struct regulator_internal *ri,
+static int regulator_set_voltage_internal(struct regulator_dev *rdev,
 					  int min_uV, int max_uV)
 {
-	struct regulator_dev *rdev = ri->rdev;
 	const struct regulator_ops *ops = rdev->desc->ops;
 	unsigned int selector;
 	int best_val = 0;
@@ -166,38 +150,33 @@ static int regulator_resolve_supply(struct regulator_dev *rdev)
 	return 0;
 }
 
-static struct regulator_internal * __regulator_register(struct regulator_dev *rdev, const char *name)
+static int __regulator_register(struct regulator_dev *rdev, const char *name)
 {
-	struct regulator_internal *ri;
 	int ret;
 
-	ri = xzalloc(sizeof(*ri));
-	ri->rdev = rdev;
+	INIT_LIST_HEAD(&rdev->consumer_list);
 
-	INIT_LIST_HEAD(&ri->consumer_list);
-
-	list_add_tail(&ri->list, &regulator_list);
+	list_add_tail(&rdev->list, &regulator_list);
 
 	if (name)
-		ri->name = xstrdup(name);
+		rdev->name = xstrdup(name);
 
 	if (rdev->boot_on || rdev->always_on) {
-		ret = regulator_resolve_supply(ri->rdev);
+		ret = regulator_resolve_supply(rdev);
 		if (ret < 0)
 			goto err;
 
-		ret = regulator_enable_internal(ri);
+		ret = regulator_enable_internal(rdev);
 		if (ret && ret != -ENOSYS)
 			goto err;
 	}
 
-	return ri;
+	return 0;
 err:
-	list_del(&ri->list);
-	free(ri->name);
-	free(ri);
+	list_del(&rdev->list);
+	free((char *)rdev->name);
 
-	return ERR_PTR(ret);
+	return ret;
 }
 
 
@@ -211,8 +190,8 @@ static struct regulator_internal * __regulator_register(struct regulator_dev *rd
  */
 int of_regulator_register(struct regulator_dev *rdev, struct device_node *node)
 {
-	struct regulator_internal *ri;
 	const char *name;
+	int ret;
 
 	if (!rdev || !node)
 		return -EINVAL;
@@ -224,34 +203,34 @@ int of_regulator_register(struct regulator_dev *rdev, struct device_node *node)
 	if (!name)
 		name = node->name;
 
-	ri = __regulator_register(rdev, name);
-	if (IS_ERR(ri))
-		return PTR_ERR(ri);
+	ret = __regulator_register(rdev, name);
+	if (ret)
+		return ret;
 
-	ri->node = node;
+	rdev->node = node;
 	node->dev = rdev->dev;
 
 	if (rdev->desc->off_on_delay)
-		ri->enable_time_us = rdev->desc->off_on_delay;
+		rdev->enable_time_us = rdev->desc->off_on_delay;
 
 	if (rdev->desc->fixed_uV && rdev->desc->n_voltages == 1)
-		ri->min_uv = ri->max_uv = rdev->desc->fixed_uV;
+		rdev->min_uv = rdev->max_uv = rdev->desc->fixed_uV;
 
 	of_property_read_u32(node, "regulator-enable-ramp-delay",
-			&ri->enable_time_us);
+			&rdev->enable_time_us);
 	of_property_read_u32(node, "regulator-min-microvolt",
-			&ri->min_uv);
+			&rdev->min_uv);
 	of_property_read_u32(node, "regulator-max-microvolt",
-			&ri->max_uv);
+			&rdev->max_uv);
 
 	return 0;
 }
 
-static struct regulator_internal *of_regulator_get(struct device *dev,
+static struct regulator_dev *of_regulator_get(struct device *dev,
 						   const char *supply)
 {
 	char *propname;
-	struct regulator_internal *ri;
+	struct regulator_dev *rdev;
 	struct device_node *node, *node_parent;
 	int ret;
 
@@ -270,7 +249,7 @@ static struct regulator_internal *of_regulator_get(struct device *dev,
 	if (!of_get_property(dev->of_node, propname, NULL)) {
 		dev_dbg(dev, "No %s-supply node found, using dummy regulator\n",
 				supply);
-		ri = NULL;
+		rdev = NULL;
 		goto out;
 	}
 
@@ -281,7 +260,7 @@ static struct regulator_internal *of_regulator_get(struct device *dev,
 	node = of_parse_phandle(dev->of_node, propname, 0);
 	if (!node) {
 		dev_dbg(dev, "No %s node found\n", propname);
-		ri = ERR_PTR(-EINVAL);
+		rdev = ERR_PTR(-EINVAL);
 		goto out;
 	}
 
@@ -298,16 +277,16 @@ static struct regulator_internal *of_regulator_get(struct device *dev,
 		    of_get_property(node_parent, "barebox,allow-dummy-supply", NULL)) {
 			dev_dbg(dev, "Allow use of dummy regulator for " \
 				"%s-supply\n", supply);
-			ri = NULL;
+			rdev = NULL;
 			goto out;
 		}
 
-		ri = ERR_PTR(ret);
+		rdev = ERR_PTR(ret);
 		goto out;
 	}
 
-	list_for_each_entry(ri, &regulator_list, list) {
-		if (ri->node == node) {
+	list_for_each_entry(rdev, &regulator_list, list) {
+		if (rdev->node == node) {
 			dev_dbg(dev, "Using %s regulator from %pOF\n",
 					propname, node);
 			goto out;
@@ -319,14 +298,14 @@ static struct regulator_internal *of_regulator_get(struct device *dev,
 	 * added in future initcalls, so, instead of reporting a
 	 * complete failure report probe deferral
 	 */
-	ri = ERR_PTR(-EPROBE_DEFER);
+	rdev = ERR_PTR(-EPROBE_DEFER);
 out:
 	free(propname);
 
-	return ri;
+	return rdev;
 }
 #else
-static struct regulator_internal *of_regulator_get(struct device *dev,
+static struct regulator_dev *of_regulator_get(struct device *dev,
 						   const char *supply)
 {
 	return NULL;
@@ -335,38 +314,40 @@ static struct regulator_internal *of_regulator_get(struct device *dev,
 
 int dev_regulator_register(struct regulator_dev *rdev, const char * name, const char* supply)
 {
-	struct regulator_internal *ri;
+	int ret;
 
-	ri = __regulator_register(rdev, name);
+	ret = __regulator_register(rdev, name);
+	if (ret)
+		return ret;
 
-	ri->supply = supply;
+	rdev->supply_name = supply;
 
 	return 0;
 }
 
-static struct regulator_internal *dev_regulator_get(struct device *dev,
-						    const char *supply)
+static struct regulator_dev *dev_regulator_get(struct device *dev,
+					       const char *supply)
 {
-	struct regulator_internal *ri;
-	struct regulator_internal *ret = NULL;
+	struct regulator_dev *rdev;
+	struct regulator_dev *ret = NULL;
 	int match, best = 0;
 	const char *dev_id = dev ? dev_name(dev) : NULL;
 
-	list_for_each_entry(ri, &regulator_list, list) {
+	list_for_each_entry(rdev, &regulator_list, list) {
 		match = 0;
-		if (ri->name) {
-			if (!dev_id || strcmp(ri->name, dev_id))
+		if (rdev->name) {
+			if (!dev_id || strcmp(rdev->name, dev_id))
 				continue;
 			match += 2;
 		}
-		if (ri->supply) {
-			if (!supply || strcmp(ri->supply, supply))
+		if (rdev->supply_name) {
+			if (!supply || strcmp(rdev->supply_name, supply))
 				continue;
 			match += 1;
 		}
 
 		if (match > best) {
-			ret = ri;
+			ret = rdev;
 			if (match != 3)
 				best = match;
 			else
@@ -389,62 +370,63 @@ static struct regulator_internal *dev_regulator_get(struct device *dev,
  */
 struct regulator *regulator_get(struct device *dev, const char *supply)
 {
-	struct regulator_internal *ri = NULL;
+	struct regulator_dev *rdev = NULL;
 	struct regulator *r;
 	int ret;
 
 	if (dev->of_node && supply) {
-		ri = of_regulator_get(dev, supply);
-		if (IS_ERR(ri))
-			return ERR_CAST(ri);
+		rdev = of_regulator_get(dev, supply);
+		if (IS_ERR(rdev))
+			return ERR_CAST(rdev);
 	}
 
-	if (!ri) {
-		ri = dev_regulator_get(dev, supply);
-		if (IS_ERR(ri))
-			return ERR_CAST(ri);
+	if (!rdev) {
+		rdev = dev_regulator_get(dev, supply);
+		if (IS_ERR(rdev))
+			return ERR_CAST(rdev);
 	}
 
-	if (!ri)
+	if (!rdev)
 		return NULL;
 
-	ret = regulator_resolve_supply(ri->rdev);
+	ret = regulator_resolve_supply(rdev);
 	if (ret < 0)
 		return ERR_PTR(ret);
 
 	r = xzalloc(sizeof(*r));
-	r->ri = ri;
+	r->rdev = rdev;
 	r->dev = dev;
 
-	list_add_tail(&r->list, &ri->consumer_list);
+	list_add_tail(&r->list, &rdev->consumer_list);
 
 	return r;
 }
 
-static struct regulator_internal *regulator_by_name(const char *name)
+static struct regulator_dev *regulator_by_name(const char *name)
 {
-	struct regulator_internal *ri;
+	struct regulator_dev *rdev;
 
-	list_for_each_entry(ri, &regulator_list, list)
-		if (ri->name && !strcmp(ri->name, name))
-			return ri;
+	list_for_each_entry(rdev, &regulator_list, list) {
+		if (rdev->name && !strcmp(rdev->name, name))
+			return rdev;
+	}
 
 	return NULL;
 }
 
 struct regulator *regulator_get_name(const char *name)
 {
-	struct regulator_internal *ri;
+	struct regulator_dev *rdev;
 	struct regulator *r;
 
-	ri = regulator_by_name(name);
-	if (!ri)
+	rdev = regulator_by_name(name);
+	if (!rdev)
 		return ERR_PTR(-ENODEV);
 
 	r = xzalloc(sizeof(*r));
-	r->ri = ri;
+	r->rdev = rdev;
 
-	list_add_tail(&r->list, &ri->consumer_list);
+	list_add_tail(&r->list, &rdev->consumer_list);
 
 	return r;
 }
@@ -463,7 +445,7 @@ int regulator_enable(struct regulator *r)
 	if (!r)
 		return 0;
 
-	return regulator_enable_internal(r->ri);
+	return regulator_enable_internal(r->rdev);
 }
 
 /*
@@ -480,7 +462,7 @@ int regulator_disable(struct regulator *r)
 	if (!r)
 		return 0;
 
-	return regulator_disable_internal(r->ri);
+	return regulator_disable_internal(r->rdev);
 }
 
 int regulator_set_voltage(struct regulator *r, int min_uV, int max_uV)
@@ -488,7 +470,7 @@ int regulator_set_voltage(struct regulator *r, int min_uV, int max_uV)
 	if (!r)
 		return 0;
 
-	return regulator_set_voltage_internal(r->ri, min_uV, max_uV);
+	return regulator_set_voltage_internal(r->rdev, min_uV, max_uV);
 }
 
 /**
@@ -632,15 +614,13 @@ EXPORT_SYMBOL_GPL(regulator_bulk_free);
 
 int regulator_get_voltage(struct regulator *regulator)
 {
-	struct regulator_internal *ri;
 	struct regulator_dev *rdev;
 	int sel, ret;
 
 	if (!regulator)
 		return -EINVAL;
 
-	ri = regulator->ri;
-	rdev = ri->rdev;
+	rdev = regulator->rdev;
 
 	if (rdev->desc->ops->get_voltage_sel) {
 		sel = rdev->desc->ops->get_voltage_sel(rdev);
@@ -653,8 +633,8 @@ int regulator_get_voltage(struct regulator *regulator)
 		ret = rdev->desc->ops->list_voltage(rdev, 0);
 	} else if (rdev->desc->fixed_uV && (rdev->desc->n_voltages == 1)) {
 		ret = rdev->desc->fixed_uV;
-	} else if (ri->min_uv && ri->min_uv == ri->max_uv) {
-		ret = ri->min_uv;
+	} else if (rdev->min_uv && rdev->min_uv == rdev->max_uv) {
+		ret = rdev->min_uv;
 	} else if (rdev->supply) {
 		ret = regulator_get_voltage(rdev->supply);
 	} else {
@@ -665,16 +645,16 @@ int regulator_get_voltage(struct regulator *regulator)
 }
 EXPORT_SYMBOL_GPL(regulator_get_voltage);
 
-static void regulator_print_one(struct regulator_internal *ri)
+static void regulator_print_one(struct regulator_dev *rdev)
 {
 	struct regulator *r;
 
-	printf("%-20s %6d %10d %10d\n", ri->name, ri->enable_count, ri->min_uv, ri->max_uv);
+	printf("%-20s %6d %10d %10d\n", rdev->name, rdev->enable_count, rdev->min_uv, rdev->max_uv);
 
-	if (!list_empty(&ri->consumer_list)) {
+	if (!list_empty(&rdev->consumer_list)) {
 		printf(" consumers:\n");
 
-		list_for_each_entry(r, &ri->consumer_list, list)
+		list_for_each_entry(r, &rdev->consumer_list, list)
 			printf("   %s\n", r->dev ? dev_name(r->dev) : "none");
 	}
 }
@@ -684,9 +664,9 @@ static void regulator_print_one(struct regulator_internal *ri)
  */
 void regulators_print(void)
 {
-	struct regulator_internal *ri;
+	struct regulator_dev *rdev;
 
 	printf("%-20s %6s %10s %10s\n", "name", "enable", "min_uv", "max_uv");
-	list_for_each_entry(ri, &regulator_list, list)
-		regulator_print_one(ri);
+	list_for_each_entry(rdev, &regulator_list, list)
+		regulator_print_one(rdev);
 }
diff --git a/include/regulator.h b/include/regulator.h
index b0a500434c..5eb236e602 100644
--- a/include/regulator.h
+++ b/include/regulator.h
@@ -85,6 +85,15 @@ struct regulator_desc {
 };
 
 struct regulator_dev {
+	const char *name;
+	struct list_head list;
+	struct device_node *node;
+	int enable_count;
+	int enable_time_us;
+	int min_uv;
+	int max_uv;
+	const char *supply_name;
+	struct list_head consumer_list;
 	const struct regulator_desc *desc;
 	struct regmap *regmap;
 	bool boot_on;
-- 
2.39.2

Re: [PATCH 02/11] regulator: merge struct regulator_internal fields into struct regulator_dev

[Marco Felsch]

Hi Sascha,

On 23-09-20, Sascha Hauer wrote:
> Each struct regulator_dev instance has a struct regulator_internal
> associated with it. The idea was that core internal fields are seen
> by the core only. In the end this is more confusing than helpful. We
> have a ri->rdev link, but no rdev->ri link so that we can't get a
> rdev from a ri pointer. We could add that link, but instead make the
> whole stuff a bit easier by just merging everything from struct
> regulator_internal to struct regulator_dev.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  drivers/regulator/core.c | 214 ++++++++++++++++++---------------------
>  include/regulator.h      |   9 ++
>  2 files changed, 106 insertions(+), 117 deletions(-)
> 
> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index e9b5b82dbe..41a3378ac8 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -13,21 +13,8 @@
>  
>  static LIST_HEAD(regulator_list);
>  
> -struct regulator_internal {
> -	struct list_head list;
> -	struct device_node *node;
> -	struct regulator_dev *rdev;
> -	int enable_count;
> -	int enable_time_us;
> -	int min_uv;
> -	int max_uv;
> -	char *name;
> -	const char *supply;
> -	struct list_head consumer_list;
> -};
> -
>  struct regulator {
> -	struct regulator_internal *ri;
> +	struct regulator_dev *rdev;
>  	struct list_head list;
>  	struct device *dev;
>  };
> @@ -41,13 +28,12 @@ static int regulator_map_voltage(struct regulator_dev *rdev, int min_uV,
>  	return -ENOSYS;
>  }
>  
> -static int regulator_enable_internal(struct regulator_internal *ri)
> +static int regulator_enable_internal(struct regulator_dev *rdev)
>  {
> -	struct regulator_dev *rdev = ri->rdev;
>  	int ret;
>  
> -	if (ri->enable_count) {
> -		ri->enable_count++;
> +	if (rdev->enable_count) {
> +		rdev->enable_count++;
>  		return 0;
>  	}
>  
> @@ -59,30 +45,29 @@ static int regulator_enable_internal(struct regulator_internal *ri)
>  	if (ret)
>  		return ret;
>  
> -	ret = rdev->desc->ops->enable(ri->rdev);
> +	ret = rdev->desc->ops->enable(rdev);
>  	if (ret) {
>  		regulator_disable(rdev->supply);
>  		return ret;
>  	}
>  
> -	if (ri->enable_time_us)
> -		udelay(ri->enable_time_us);
> +	if (rdev->enable_time_us)
> +		udelay(rdev->enable_time_us);

regualtor_dev does not have a 'enable_time_us' or do I miss something? 

Regards,
  Marco

> -	ri->enable_count++;
> +	rdev->enable_count++;
>  
>  	return 0;
>  }
>  
> -static int regulator_disable_internal(struct regulator_internal *ri)
> +static int regulator_disable_internal(struct regulator_dev *rdev)
>  {
> -	struct regulator_dev *rdev = ri->rdev;
>  	int ret;
>  
> -	if (!ri->enable_count)
> +	if (!rdev->enable_count)
>  		return -EINVAL;
>  
> -	if (ri->enable_count > 1) {
> -		ri->enable_count--;
> +	if (rdev->enable_count > 1) {
> +		rdev->enable_count--;
>  		return 0;
>  	}
>  
> @@ -97,15 +82,14 @@ static int regulator_disable_internal(struct regulator_internal *ri)
>  	if (ret)
>  		return ret;
>  
> -	ri->enable_count--;
> +	rdev->enable_count--;
>  
> -	return regulator_disable(ri->rdev->supply);
> +	return regulator_disable(rdev->supply);
>  }
>  
> -static int regulator_set_voltage_internal(struct regulator_internal *ri,
> +static int regulator_set_voltage_internal(struct regulator_dev *rdev,
>  					  int min_uV, int max_uV)
>  {
> -	struct regulator_dev *rdev = ri->rdev;
>  	const struct regulator_ops *ops = rdev->desc->ops;
>  	unsigned int selector;
>  	int best_val = 0;
> @@ -166,38 +150,33 @@ static int regulator_resolve_supply(struct regulator_dev *rdev)
>  	return 0;
>  }
>  
> -static struct regulator_internal * __regulator_register(struct regulator_dev *rdev, const char *name)
> +static int __regulator_register(struct regulator_dev *rdev, const char *name)
>  {
> -	struct regulator_internal *ri;
>  	int ret;
>  
> -	ri = xzalloc(sizeof(*ri));
> -	ri->rdev = rdev;
> +	INIT_LIST_HEAD(&rdev->consumer_list);
>  
> -	INIT_LIST_HEAD(&ri->consumer_list);
> -
> -	list_add_tail(&ri->list, &regulator_list);
> +	list_add_tail(&rdev->list, &regulator_list);
>  
>  	if (name)
> -		ri->name = xstrdup(name);
> +		rdev->name = xstrdup(name);
>  
>  	if (rdev->boot_on || rdev->always_on) {
> -		ret = regulator_resolve_supply(ri->rdev);
> +		ret = regulator_resolve_supply(rdev);
>  		if (ret < 0)
>  			goto err;
>  
> -		ret = regulator_enable_internal(ri);
> +		ret = regulator_enable_internal(rdev);
>  		if (ret && ret != -ENOSYS)
>  			goto err;
>  	}
>  
> -	return ri;
> +	return 0;
>  err:
> -	list_del(&ri->list);
> -	free(ri->name);
> -	free(ri);
> +	list_del(&rdev->list);
> +	free((char *)rdev->name);
>  
> -	return ERR_PTR(ret);
> +	return ret;
>  }
>  
>  
> @@ -211,8 +190,8 @@ static struct regulator_internal * __regulator_register(struct regulator_dev *rd
>   */
>  int of_regulator_register(struct regulator_dev *rdev, struct device_node *node)
>  {
> -	struct regulator_internal *ri;
>  	const char *name;
> +	int ret;
>  
>  	if (!rdev || !node)
>  		return -EINVAL;
> @@ -224,34 +203,34 @@ int of_regulator_register(struct regulator_dev *rdev, struct device_node *node)
>  	if (!name)
>  		name = node->name;
>  
> -	ri = __regulator_register(rdev, name);
> -	if (IS_ERR(ri))
> -		return PTR_ERR(ri);
> +	ret = __regulator_register(rdev, name);
> +	if (ret)
> +		return ret;
>  
> -	ri->node = node;
> +	rdev->node = node;
>  	node->dev = rdev->dev;
>  
>  	if (rdev->desc->off_on_delay)
> -		ri->enable_time_us = rdev->desc->off_on_delay;
> +		rdev->enable_time_us = rdev->desc->off_on_delay;
>  
>  	if (rdev->desc->fixed_uV && rdev->desc->n_voltages == 1)
> -		ri->min_uv = ri->max_uv = rdev->desc->fixed_uV;
> +		rdev->min_uv = rdev->max_uv = rdev->desc->fixed_uV;
>  
>  	of_property_read_u32(node, "regulator-enable-ramp-delay",
> -			&ri->enable_time_us);
> +			&rdev->enable_time_us);
>  	of_property_read_u32(node, "regulator-min-microvolt",
> -			&ri->min_uv);
> +			&rdev->min_uv);
>  	of_property_read_u32(node, "regulator-max-microvolt",
> -			&ri->max_uv);
> +			&rdev->max_uv);
>  
>  	return 0;
>  }
>  
> -static struct regulator_internal *of_regulator_get(struct device *dev,
> +static struct regulator_dev *of_regulator_get(struct device *dev,
>  						   const char *supply)
>  {
>  	char *propname;
> -	struct regulator_internal *ri;
> +	struct regulator_dev *rdev;
>  	struct device_node *node, *node_parent;
>  	int ret;
>  
> @@ -270,7 +249,7 @@ static struct regulator_internal *of_regulator_get(struct device *dev,
>  	if (!of_get_property(dev->of_node, propname, NULL)) {
>  		dev_dbg(dev, "No %s-supply node found, using dummy regulator\n",
>  				supply);
> -		ri = NULL;
> +		rdev = NULL;
>  		goto out;
>  	}
>  
> @@ -281,7 +260,7 @@ static struct regulator_internal *of_regulator_get(struct device *dev,
>  	node = of_parse_phandle(dev->of_node, propname, 0);
>  	if (!node) {
>  		dev_dbg(dev, "No %s node found\n", propname);
> -		ri = ERR_PTR(-EINVAL);
> +		rdev = ERR_PTR(-EINVAL);
>  		goto out;
>  	}
>  
> @@ -298,16 +277,16 @@ static struct regulator_internal *of_regulator_get(struct device *dev,
>  		    of_get_property(node_parent, "barebox,allow-dummy-supply", NULL)) {
>  			dev_dbg(dev, "Allow use of dummy regulator for " \
>  				"%s-supply\n", supply);
> -			ri = NULL;
> +			rdev = NULL;
>  			goto out;
>  		}
>  
> -		ri = ERR_PTR(ret);
> +		rdev = ERR_PTR(ret);
>  		goto out;
>  	}
>  
> -	list_for_each_entry(ri, &regulator_list, list) {
> -		if (ri->node == node) {
> +	list_for_each_entry(rdev, &regulator_list, list) {
> +		if (rdev->node == node) {
>  			dev_dbg(dev, "Using %s regulator from %pOF\n",
>  					propname, node);
>  			goto out;
> @@ -319,14 +298,14 @@ static struct regulator_internal *of_regulator_get(struct device *dev,
>  	 * added in future initcalls, so, instead of reporting a
>  	 * complete failure report probe deferral
>  	 */
> -	ri = ERR_PTR(-EPROBE_DEFER);
> +	rdev = ERR_PTR(-EPROBE_DEFER);
>  out:
>  	free(propname);
>  
> -	return ri;
> +	return rdev;
>  }
>  #else
> -static struct regulator_internal *of_regulator_get(struct device *dev,
> +static struct regulator_dev *of_regulator_get(struct device *dev,
>  						   const char *supply)
>  {
>  	return NULL;
> @@ -335,38 +314,40 @@ static struct regulator_internal *of_regulator_get(struct device *dev,
>  
>  int dev_regulator_register(struct regulator_dev *rdev, const char * name, const char* supply)
>  {
> -	struct regulator_internal *ri;
> +	int ret;
>  
> -	ri = __regulator_register(rdev, name);
> +	ret = __regulator_register(rdev, name);
> +	if (ret)
> +		return ret;
>  
> -	ri->supply = supply;
> +	rdev->supply_name = supply;
>  
>  	return 0;
>  }
>  
> -static struct regulator_internal *dev_regulator_get(struct device *dev,
> -						    const char *supply)
> +static struct regulator_dev *dev_regulator_get(struct device *dev,
> +					       const char *supply)
>  {
> -	struct regulator_internal *ri;
> -	struct regulator_internal *ret = NULL;
> +	struct regulator_dev *rdev;
> +	struct regulator_dev *ret = NULL;
>  	int match, best = 0;
>  	const char *dev_id = dev ? dev_name(dev) : NULL;
>  
> -	list_for_each_entry(ri, &regulator_list, list) {
> +	list_for_each_entry(rdev, &regulator_list, list) {
>  		match = 0;
> -		if (ri->name) {
> -			if (!dev_id || strcmp(ri->name, dev_id))
> +		if (rdev->name) {
> +			if (!dev_id || strcmp(rdev->name, dev_id))
>  				continue;
>  			match += 2;
>  		}
> -		if (ri->supply) {
> -			if (!supply || strcmp(ri->supply, supply))
> +		if (rdev->supply_name) {
> +			if (!supply || strcmp(rdev->supply_name, supply))
>  				continue;
>  			match += 1;
>  		}
>  
>  		if (match > best) {
> -			ret = ri;
> +			ret = rdev;
>  			if (match != 3)
>  				best = match;
>  			else
> @@ -389,62 +370,63 @@ static struct regulator_internal *dev_regulator_get(struct device *dev,
>   */
>  struct regulator *regulator_get(struct device *dev, const char *supply)
>  {
> -	struct regulator_internal *ri = NULL;
> +	struct regulator_dev *rdev = NULL;
>  	struct regulator *r;
>  	int ret;
>  
>  	if (dev->of_node && supply) {
> -		ri = of_regulator_get(dev, supply);
> -		if (IS_ERR(ri))
> -			return ERR_CAST(ri);
> +		rdev = of_regulator_get(dev, supply);
> +		if (IS_ERR(rdev))
> +			return ERR_CAST(rdev);
>  	}
>  
> -	if (!ri) {
> -		ri = dev_regulator_get(dev, supply);
> -		if (IS_ERR(ri))
> -			return ERR_CAST(ri);
> +	if (!rdev) {
> +		rdev = dev_regulator_get(dev, supply);
> +		if (IS_ERR(rdev))
> +			return ERR_CAST(rdev);
>  	}
>  
> -	if (!ri)
> +	if (!rdev)
>  		return NULL;
>  
> -	ret = regulator_resolve_supply(ri->rdev);
> +	ret = regulator_resolve_supply(rdev);
>  	if (ret < 0)
>  		return ERR_PTR(ret);
>  
>  	r = xzalloc(sizeof(*r));
> -	r->ri = ri;
> +	r->rdev = rdev;
>  	r->dev = dev;
>  
> -	list_add_tail(&r->list, &ri->consumer_list);
> +	list_add_tail(&r->list, &rdev->consumer_list);
>  
>  	return r;
>  }
>  
> -static struct regulator_internal *regulator_by_name(const char *name)
> +static struct regulator_dev *regulator_by_name(const char *name)
>  {
> -	struct regulator_internal *ri;
> +	struct regulator_dev *rdev;
>  
> -	list_for_each_entry(ri, &regulator_list, list)
> -		if (ri->name && !strcmp(ri->name, name))
> -			return ri;
> +	list_for_each_entry(rdev, &regulator_list, list) {
> +		if (rdev->name && !strcmp(rdev->name, name))
> +			return rdev;
> +	}
>  
>  	return NULL;
>  }
>  
>  struct regulator *regulator_get_name(const char *name)
>  {
> -	struct regulator_internal *ri;
> +	struct regulator_dev *rdev;
>  	struct regulator *r;
>  
> -	ri = regulator_by_name(name);
> -	if (!ri)
> +	rdev = regulator_by_name(name);
> +	if (!rdev)
>  		return ERR_PTR(-ENODEV);
>  
>  	r = xzalloc(sizeof(*r));
> -	r->ri = ri;
> +	r->rdev = rdev;
>  
> -	list_add_tail(&r->list, &ri->consumer_list);
> +	list_add_tail(&r->list, &rdev->consumer_list);
>  
>  	return r;
>  }
> @@ -463,7 +445,7 @@ int regulator_enable(struct regulator *r)
>  	if (!r)
>  		return 0;
>  
> -	return regulator_enable_internal(r->ri);
> +	return regulator_enable_internal(r->rdev);
>  }
>  
>  /*
> @@ -480,7 +462,7 @@ int regulator_disable(struct regulator *r)
>  	if (!r)
>  		return 0;
>  
> -	return regulator_disable_internal(r->ri);
> +	return regulator_disable_internal(r->rdev);
>  }
>  
>  int regulator_set_voltage(struct regulator *r, int min_uV, int max_uV)
> @@ -488,7 +470,7 @@ int regulator_set_voltage(struct regulator *r, int min_uV, int max_uV)
>  	if (!r)
>  		return 0;
>  
> -	return regulator_set_voltage_internal(r->ri, min_uV, max_uV);
> +	return regulator_set_voltage_internal(r->rdev, min_uV, max_uV);
>  }
>  
>  /**
> @@ -632,15 +614,13 @@ EXPORT_SYMBOL_GPL(regulator_bulk_free);
>  
>  int regulator_get_voltage(struct regulator *regulator)
>  {
> -	struct regulator_internal *ri;
>  	struct regulator_dev *rdev;
>  	int sel, ret;
>  
>  	if (!regulator)
>  		return -EINVAL;
>  
> -	ri = regulator->ri;
> -	rdev = ri->rdev;
> +	rdev = regulator->rdev;
>  
>  	if (rdev->desc->ops->get_voltage_sel) {
>  		sel = rdev->desc->ops->get_voltage_sel(rdev);
> @@ -653,8 +633,8 @@ int regulator_get_voltage(struct regulator *regulator)
>  		ret = rdev->desc->ops->list_voltage(rdev, 0);
>  	} else if (rdev->desc->fixed_uV && (rdev->desc->n_voltages == 1)) {
>  		ret = rdev->desc->fixed_uV;
> -	} else if (ri->min_uv && ri->min_uv == ri->max_uv) {
> -		ret = ri->min_uv;
> +	} else if (rdev->min_uv && rdev->min_uv == rdev->max_uv) {
> +		ret = rdev->min_uv;
>  	} else if (rdev->supply) {
>  		ret = regulator_get_voltage(rdev->supply);
>  	} else {
> @@ -665,16 +645,16 @@ int regulator_get_voltage(struct regulator *regulator)
>  }
>  EXPORT_SYMBOL_GPL(regulator_get_voltage);
>  
> -static void regulator_print_one(struct regulator_internal *ri)
> +static void regulator_print_one(struct regulator_dev *rdev)
>  {
>  	struct regulator *r;
>  
> -	printf("%-20s %6d %10d %10d\n", ri->name, ri->enable_count, ri->min_uv, ri->max_uv);
> +	printf("%-20s %6d %10d %10d\n", rdev->name, rdev->enable_count, rdev->min_uv, rdev->max_uv);
>  
> -	if (!list_empty(&ri->consumer_list)) {
> +	if (!list_empty(&rdev->consumer_list)) {
>  		printf(" consumers:\n");
>  
> -		list_for_each_entry(r, &ri->consumer_list, list)
> +		list_for_each_entry(r, &rdev->consumer_list, list)
>  			printf("   %s\n", r->dev ? dev_name(r->dev) : "none");
>  	}
>  }
> @@ -684,9 +664,9 @@ static void regulator_print_one(struct regulator_internal *ri)
>   */
>  void regulators_print(void)
>  {
> -	struct regulator_internal *ri;
> +	struct regulator_dev *rdev;
>  
>  	printf("%-20s %6s %10s %10s\n", "name", "enable", "min_uv", "max_uv");
> -	list_for_each_entry(ri, &regulator_list, list)
> -		regulator_print_one(ri);
> +	list_for_each_entry(rdev, &regulator_list, list)
> +		regulator_print_one(rdev);
>  }
> diff --git a/include/regulator.h b/include/regulator.h
> index b0a500434c..5eb236e602 100644
> --- a/include/regulator.h
> +++ b/include/regulator.h
> @@ -85,6 +85,15 @@ struct regulator_desc {
>  };
>  
>  struct regulator_dev {
> +	const char *name;
> +	struct list_head list;
> +	struct device_node *node;
> +	int enable_count;
> +	int enable_time_us;
> +	int min_uv;
> +	int max_uv;
> +	const char *supply_name;
> +	struct list_head consumer_list;
>  	const struct regulator_desc *desc;
>  	struct regmap *regmap;
>  	bool boot_on;
> -- 
> 2.39.2
> 
> 
> 

Re: [PATCH 02/11] regulator: merge struct regulator_internal fields into struct regulator_dev

[Sascha Hauer]

On Wed, Sep 20, 2023 at 12:52:03PM +0200, Marco Felsch wrote:
> Hi Sascha,
> 
> On 23-09-20, Sascha Hauer wrote:
> > Each struct regulator_dev instance has a struct regulator_internal
> > associated with it. The idea was that core internal fields are seen
> > by the core only. In the end this is more confusing than helpful. We
> > have a ri->rdev link, but no rdev->ri link so that we can't get a
> > rdev from a ri pointer. We could add that link, but instead make the
> > whole stuff a bit easier by just merging everything from struct
> > regulator_internal to struct regulator_dev.
> > 
> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > ---
> >  drivers/regulator/core.c | 214 ++++++++++++++++++---------------------
> >  include/regulator.h      |   9 ++
> >  2 files changed, 106 insertions(+), 117 deletions(-)
> > 
> > diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> > index e9b5b82dbe..41a3378ac8 100644
> > --- a/drivers/regulator/core.c
> > +++ b/drivers/regulator/core.c
> > @@ -13,21 +13,8 @@
> >  
> >  static LIST_HEAD(regulator_list);
> >  
> > -struct regulator_internal {
> > -	struct list_head list;
> > -	struct device_node *node;
> > -	struct regulator_dev *rdev;
> > -	int enable_count;
> > -	int enable_time_us;
> > -	int min_uv;
> > -	int max_uv;
> > -	char *name;
> > -	const char *supply;
> > -	struct list_head consumer_list;
> > -};
> > -
> >  struct regulator {
> > -	struct regulator_internal *ri;
> > +	struct regulator_dev *rdev;
> >  	struct list_head list;
> >  	struct device *dev;
> >  };
> > @@ -41,13 +28,12 @@ static int regulator_map_voltage(struct regulator_dev *rdev, int min_uV,
> >  	return -ENOSYS;
> >  }
> >  
> > -static int regulator_enable_internal(struct regulator_internal *ri)
> > +static int regulator_enable_internal(struct regulator_dev *rdev)
> >  {
> > -	struct regulator_dev *rdev = ri->rdev;
> >  	int ret;
> >  
> > -	if (ri->enable_count) {
> > -		ri->enable_count++;
> > +	if (rdev->enable_count) {
> > +		rdev->enable_count++;
> >  		return 0;
> >  	}
> >  
> > @@ -59,30 +45,29 @@ static int regulator_enable_internal(struct regulator_internal *ri)
> >  	if (ret)
> >  		return ret;
> >  
> > -	ret = rdev->desc->ops->enable(ri->rdev);
> > +	ret = rdev->desc->ops->enable(rdev);
> >  	if (ret) {
> >  		regulator_disable(rdev->supply);
> >  		return ret;
> >  	}
> >  
> > -	if (ri->enable_time_us)
> > -		udelay(ri->enable_time_us);
> > +	if (rdev->enable_time_us)
> > +		udelay(rdev->enable_time_us);
> 
> regualtor_dev does not have a 'enable_time_us' or do I miss something?

Yes, the rest of this patch ;)

> > diff --git a/include/regulator.h b/include/regulator.h
> > index b0a500434c..5eb236e602 100644
> > --- a/include/regulator.h
> > +++ b/include/regulator.h
> > @@ -85,6 +85,15 @@ struct regulator_desc {
> >  };
> >  
> >  struct regulator_dev {
> > +	const char *name;
> > +	struct list_head list;
> > +	struct device_node *node;
> > +	int enable_count;
> > +	int enable_time_us;
> > +	int min_uv;
> > +	int max_uv;
> > +	const char *supply_name;
> > +	struct list_head consumer_list;
> >  	const struct regulator_desc *desc;
> >  	struct regmap *regmap;
> >  	bool boot_on;

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 02/11] regulator: merge struct regulator_internal fields into struct regulator_dev

[Marco Felsch]

On 23-09-20, Sascha Hauer wrote:
> On Wed, Sep 20, 2023 at 12:52:03PM +0200, Marco Felsch wrote:
> > Hi Sascha,
> > 
> > On 23-09-20, Sascha Hauer wrote:
> > > Each struct regulator_dev instance has a struct regulator_internal
> > > associated with it. The idea was that core internal fields are seen
> > > by the core only. In the end this is more confusing than helpful. We
> > > have a ri->rdev link, but no rdev->ri link so that we can't get a
> > > rdev from a ri pointer. We could add that link, but instead make the
> > > whole stuff a bit easier by just merging everything from struct
> > > regulator_internal to struct regulator_dev.
> > > 
> > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > > ---
> > >  drivers/regulator/core.c | 214 ++++++++++++++++++---------------------
> > >  include/regulator.h      |   9 ++
> > >  2 files changed, 106 insertions(+), 117 deletions(-)
> > > 
> > > diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> > > index e9b5b82dbe..41a3378ac8 100644
> > > --- a/drivers/regulator/core.c
> > > +++ b/drivers/regulator/core.c
> > > @@ -13,21 +13,8 @@
> > >  
> > >  static LIST_HEAD(regulator_list);
> > >  
> > > -struct regulator_internal {
> > > -	struct list_head list;
> > > -	struct device_node *node;
> > > -	struct regulator_dev *rdev;
> > > -	int enable_count;
> > > -	int enable_time_us;
> > > -	int min_uv;
> > > -	int max_uv;
> > > -	char *name;
> > > -	const char *supply;
> > > -	struct list_head consumer_list;
> > > -};
> > > -
> > >  struct regulator {
> > > -	struct regulator_internal *ri;
> > > +	struct regulator_dev *rdev;
> > >  	struct list_head list;
> > >  	struct device *dev;
> > >  };
> > > @@ -41,13 +28,12 @@ static int regulator_map_voltage(struct regulator_dev *rdev, int min_uV,
> > >  	return -ENOSYS;
> > >  }
> > >  
> > > -static int regulator_enable_internal(struct regulator_internal *ri)
> > > +static int regulator_enable_internal(struct regulator_dev *rdev)
> > >  {
> > > -	struct regulator_dev *rdev = ri->rdev;
> > >  	int ret;
> > >  
> > > -	if (ri->enable_count) {
> > > -		ri->enable_count++;
> > > +	if (rdev->enable_count) {
> > > +		rdev->enable_count++;
> > >  		return 0;
> > >  	}
> > >  
> > > @@ -59,30 +45,29 @@ static int regulator_enable_internal(struct regulator_internal *ri)
> > >  	if (ret)
> > >  		return ret;
> > >  
> > > -	ret = rdev->desc->ops->enable(ri->rdev);
> > > +	ret = rdev->desc->ops->enable(rdev);
> > >  	if (ret) {
> > >  		regulator_disable(rdev->supply);
> > >  		return ret;
> > >  	}
> > >  
> > > -	if (ri->enable_time_us)
> > > -		udelay(ri->enable_time_us);
> > > +	if (rdev->enable_time_us)
> > > +		udelay(rdev->enable_time_us);
> > 
> > regualtor_dev does not have a 'enable_time_us' or do I miss something?
> 
> Yes, the rest of this patch ;)

Argh.. sorry for the noise.

Regards,
  Marco

> > > diff --git a/include/regulator.h b/include/regulator.h
> > > index b0a500434c..5eb236e602 100644
> > > --- a/include/regulator.h
> > > +++ b/include/regulator.h
> > > @@ -85,6 +85,15 @@ struct regulator_desc {
> > >  };
> > >  
> > >  struct regulator_dev {
> > > +	const char *name;
> > > +	struct list_head list;
> > > +	struct device_node *node;
> > > +	int enable_count;
> > > +	int enable_time_us;
> > > +	int min_uv;
> > > +	int max_uv;
> > > +	const char *supply_name;
> > > +	struct list_head consumer_list;
> > >  	const struct regulator_desc *desc;
> > >  	struct regmap *regmap;
> > >  	bool boot_on;
> 
> Sascha
> 
> -- 
> Pengutronix e.K.                           |                             |
> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
> 

Re: [PATCH 02/11] regulator: merge struct regulator_internal fields into struct regulator_dev

[Marco Felsch]

On 23-09-20, Sascha Hauer wrote:
> Each struct regulator_dev instance has a struct regulator_internal
> associated with it. The idea was that core internal fields are seen
> by the core only. In the end this is more confusing than helpful. We
> have a ri->rdev link, but no rdev->ri link so that we can't get a
> rdev from a ri pointer. We could add that link, but instead make the
> whole stuff a bit easier by just merging everything from struct
> regulator_internal to struct regulator_dev.

This also aligns it with Linux :)

> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>

Reviewed-by: Marco Felsch <m.felsch@pengutronix.de>

> ---
>  drivers/regulator/core.c | 214 ++++++++++++++++++---------------------
>  include/regulator.h      |   9 ++
>  2 files changed, 106 insertions(+), 117 deletions(-)
> 
> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index e9b5b82dbe..41a3378ac8 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -13,21 +13,8 @@
>  
>  static LIST_HEAD(regulator_list);
>  
> -struct regulator_internal {
> -	struct list_head list;
> -	struct device_node *node;
> -	struct regulator_dev *rdev;
> -	int enable_count;
> -	int enable_time_us;
> -	int min_uv;
> -	int max_uv;
> -	char *name;
> -	const char *supply;
> -	struct list_head consumer_list;
> -};
> -
>  struct regulator {
> -	struct regulator_internal *ri;
> +	struct regulator_dev *rdev;
>  	struct list_head list;
>  	struct device *dev;
>  };
> @@ -41,13 +28,12 @@ static int regulator_map_voltage(struct regulator_dev *rdev, int min_uV,
>  	return -ENOSYS;
>  }
>  
> -static int regulator_enable_internal(struct regulator_internal *ri)
> +static int regulator_enable_internal(struct regulator_dev *rdev)
>  {
> -	struct regulator_dev *rdev = ri->rdev;
>  	int ret;
>  
> -	if (ri->enable_count) {
> -		ri->enable_count++;
> +	if (rdev->enable_count) {
> +		rdev->enable_count++;
>  		return 0;
>  	}
>  
> @@ -59,30 +45,29 @@ static int regulator_enable_internal(struct regulator_internal *ri)
>  	if (ret)
>  		return ret;
>  
> -	ret = rdev->desc->ops->enable(ri->rdev);
> +	ret = rdev->desc->ops->enable(rdev);
>  	if (ret) {
>  		regulator_disable(rdev->supply);
>  		return ret;
>  	}
>  
> -	if (ri->enable_time_us)
> -		udelay(ri->enable_time_us);
> +	if (rdev->enable_time_us)
> +		udelay(rdev->enable_time_us);
>  
> -	ri->enable_count++;
> +	rdev->enable_count++;
>  
>  	return 0;
>  }
>  
> -static int regulator_disable_internal(struct regulator_internal *ri)
> +static int regulator_disable_internal(struct regulator_dev *rdev)
>  {
> -	struct regulator_dev *rdev = ri->rdev;
>  	int ret;
>  
> -	if (!ri->enable_count)
> +	if (!rdev->enable_count)
>  		return -EINVAL;
>  
> -	if (ri->enable_count > 1) {
> -		ri->enable_count--;
> +	if (rdev->enable_count > 1) {
> +		rdev->enable_count--;
>  		return 0;
>  	}
>  
> @@ -97,15 +82,14 @@ static int regulator_disable_internal(struct regulator_internal *ri)
>  	if (ret)
>  		return ret;
>  
> -	ri->enable_count--;
> +	rdev->enable_count--;
>  
> -	return regulator_disable(ri->rdev->supply);
> +	return regulator_disable(rdev->supply);
>  }
>  
> -static int regulator_set_voltage_internal(struct regulator_internal *ri,
> +static int regulator_set_voltage_internal(struct regulator_dev *rdev,
>  					  int min_uV, int max_uV)
>  {
> -	struct regulator_dev *rdev = ri->rdev;
>  	const struct regulator_ops *ops = rdev->desc->ops;
>  	unsigned int selector;
>  	int best_val = 0;
> @@ -166,38 +150,33 @@ static int regulator_resolve_supply(struct regulator_dev *rdev)
>  	return 0;
>  }
>  
> -static struct regulator_internal * __regulator_register(struct regulator_dev *rdev, const char *name)
> +static int __regulator_register(struct regulator_dev *rdev, const char *name)
>  {
> -	struct regulator_internal *ri;
>  	int ret;
>  
> -	ri = xzalloc(sizeof(*ri));
> -	ri->rdev = rdev;
> +	INIT_LIST_HEAD(&rdev->consumer_list);
>  
> -	INIT_LIST_HEAD(&ri->consumer_list);
> -
> -	list_add_tail(&ri->list, &regulator_list);
> +	list_add_tail(&rdev->list, &regulator_list);
>  
>  	if (name)
> -		ri->name = xstrdup(name);
> +		rdev->name = xstrdup(name);
>  
>  	if (rdev->boot_on || rdev->always_on) {
> -		ret = regulator_resolve_supply(ri->rdev);
> +		ret = regulator_resolve_supply(rdev);
>  		if (ret < 0)
>  			goto err;
>  
> -		ret = regulator_enable_internal(ri);
> +		ret = regulator_enable_internal(rdev);
>  		if (ret && ret != -ENOSYS)
>  			goto err;
>  	}
>  
> -	return ri;
> +	return 0;
>  err:
> -	list_del(&ri->list);
> -	free(ri->name);
> -	free(ri);
> +	list_del(&rdev->list);
> +	free((char *)rdev->name);
>  
> -	return ERR_PTR(ret);
> +	return ret;
>  }
>  
>  
> @@ -211,8 +190,8 @@ static struct regulator_internal * __regulator_register(struct regulator_dev *rd
>   */
>  int of_regulator_register(struct regulator_dev *rdev, struct device_node *node)
>  {
> -	struct regulator_internal *ri;
>  	const char *name;
> +	int ret;
>  
>  	if (!rdev || !node)
>  		return -EINVAL;
> @@ -224,34 +203,34 @@ int of_regulator_register(struct regulator_dev *rdev, struct device_node *node)
>  	if (!name)
>  		name = node->name;
>  
> -	ri = __regulator_register(rdev, name);
> -	if (IS_ERR(ri))
> -		return PTR_ERR(ri);
> +	ret = __regulator_register(rdev, name);
> +	if (ret)
> +		return ret;
>  
> -	ri->node = node;
> +	rdev->node = node;
>  	node->dev = rdev->dev;
>  
>  	if (rdev->desc->off_on_delay)
> -		ri->enable_time_us = rdev->desc->off_on_delay;
> +		rdev->enable_time_us = rdev->desc->off_on_delay;
>  
>  	if (rdev->desc->fixed_uV && rdev->desc->n_voltages == 1)
> -		ri->min_uv = ri->max_uv = rdev->desc->fixed_uV;
> +		rdev->min_uv = rdev->max_uv = rdev->desc->fixed_uV;
>  
>  	of_property_read_u32(node, "regulator-enable-ramp-delay",
> -			&ri->enable_time_us);
> +			&rdev->enable_time_us);
>  	of_property_read_u32(node, "regulator-min-microvolt",
> -			&ri->min_uv);
> +			&rdev->min_uv);
>  	of_property_read_u32(node, "regulator-max-microvolt",
> -			&ri->max_uv);
> +			&rdev->max_uv);
>  
>  	return 0;
>  }
>  
> -static struct regulator_internal *of_regulator_get(struct device *dev,
> +static struct regulator_dev *of_regulator_get(struct device *dev,
>  						   const char *supply)
>  {
>  	char *propname;
> -	struct regulator_internal *ri;
> +	struct regulator_dev *rdev;
>  	struct device_node *node, *node_parent;
>  	int ret;
>  
> @@ -270,7 +249,7 @@ static struct regulator_internal *of_regulator_get(struct device *dev,
>  	if (!of_get_property(dev->of_node, propname, NULL)) {
>  		dev_dbg(dev, "No %s-supply node found, using dummy regulator\n",
>  				supply);
> -		ri = NULL;
> +		rdev = NULL;
>  		goto out;
>  	}
>  
> @@ -281,7 +260,7 @@ static struct regulator_internal *of_regulator_get(struct device *dev,
>  	node = of_parse_phandle(dev->of_node, propname, 0);
>  	if (!node) {
>  		dev_dbg(dev, "No %s node found\n", propname);
> -		ri = ERR_PTR(-EINVAL);
> +		rdev = ERR_PTR(-EINVAL);
>  		goto out;
>  	}
>  
> @@ -298,16 +277,16 @@ static struct regulator_internal *of_regulator_get(struct device *dev,
>  		    of_get_property(node_parent, "barebox,allow-dummy-supply", NULL)) {
>  			dev_dbg(dev, "Allow use of dummy regulator for " \
>  				"%s-supply\n", supply);
> -			ri = NULL;
> +			rdev = NULL;
>  			goto out;
>  		}
>  
> -		ri = ERR_PTR(ret);
> +		rdev = ERR_PTR(ret);
>  		goto out;
>  	}
>  
> -	list_for_each_entry(ri, &regulator_list, list) {
> -		if (ri->node == node) {
> +	list_for_each_entry(rdev, &regulator_list, list) {
> +		if (rdev->node == node) {
>  			dev_dbg(dev, "Using %s regulator from %pOF\n",
>  					propname, node);
>  			goto out;
> @@ -319,14 +298,14 @@ static struct regulator_internal *of_regulator_get(struct device *dev,
>  	 * added in future initcalls, so, instead of reporting a
>  	 * complete failure report probe deferral
>  	 */
> -	ri = ERR_PTR(-EPROBE_DEFER);
> +	rdev = ERR_PTR(-EPROBE_DEFER);
>  out:
>  	free(propname);
>  
> -	return ri;
> +	return rdev;
>  }
>  #else
> -static struct regulator_internal *of_regulator_get(struct device *dev,
> +static struct regulator_dev *of_regulator_get(struct device *dev,
>  						   const char *supply)
>  {
>  	return NULL;
> @@ -335,38 +314,40 @@ static struct regulator_internal *of_regulator_get(struct device *dev,
>  
>  int dev_regulator_register(struct regulator_dev *rdev, const char * name, const char* supply)
>  {
> -	struct regulator_internal *ri;
> +	int ret;
>  
> -	ri = __regulator_register(rdev, name);
> +	ret = __regulator_register(rdev, name);
> +	if (ret)
> +		return ret;
>  
> -	ri->supply = supply;
> +	rdev->supply_name = supply;
>  
>  	return 0;
>  }
>  
> -static struct regulator_internal *dev_regulator_get(struct device *dev,
> -						    const char *supply)
> +static struct regulator_dev *dev_regulator_get(struct device *dev,
> +					       const char *supply)
>  {
> -	struct regulator_internal *ri;
> -	struct regulator_internal *ret = NULL;
> +	struct regulator_dev *rdev;
> +	struct regulator_dev *ret = NULL;
>  	int match, best = 0;
>  	const char *dev_id = dev ? dev_name(dev) : NULL;
>  
> -	list_for_each_entry(ri, &regulator_list, list) {
> +	list_for_each_entry(rdev, &regulator_list, list) {
>  		match = 0;
> -		if (ri->name) {
> -			if (!dev_id || strcmp(ri->name, dev_id))
> +		if (rdev->name) {
> +			if (!dev_id || strcmp(rdev->name, dev_id))
>  				continue;
>  			match += 2;
>  		}
> -		if (ri->supply) {
> -			if (!supply || strcmp(ri->supply, supply))
> +		if (rdev->supply_name) {
> +			if (!supply || strcmp(rdev->supply_name, supply))
>  				continue;
>  			match += 1;
>  		}
>  
>  		if (match > best) {
> -			ret = ri;
> +			ret = rdev;
>  			if (match != 3)
>  				best = match;
>  			else
> @@ -389,62 +370,63 @@ static struct regulator_internal *dev_regulator_get(struct device *dev,
>   */
>  struct regulator *regulator_get(struct device *dev, const char *supply)
>  {
> -	struct regulator_internal *ri = NULL;
> +	struct regulator_dev *rdev = NULL;
>  	struct regulator *r;
>  	int ret;
>  
>  	if (dev->of_node && supply) {
> -		ri = of_regulator_get(dev, supply);
> -		if (IS_ERR(ri))
> -			return ERR_CAST(ri);
> +		rdev = of_regulator_get(dev, supply);
> +		if (IS_ERR(rdev))
> +			return ERR_CAST(rdev);
>  	}
>  
> -	if (!ri) {
> -		ri = dev_regulator_get(dev, supply);
> -		if (IS_ERR(ri))
> -			return ERR_CAST(ri);
> +	if (!rdev) {
> +		rdev = dev_regulator_get(dev, supply);
> +		if (IS_ERR(rdev))
> +			return ERR_CAST(rdev);
>  	}
>  
> -	if (!ri)
> +	if (!rdev)
>  		return NULL;
>  
> -	ret = regulator_resolve_supply(ri->rdev);
> +	ret = regulator_resolve_supply(rdev);
>  	if (ret < 0)
>  		return ERR_PTR(ret);
>  
>  	r = xzalloc(sizeof(*r));
> -	r->ri = ri;
> +	r->rdev = rdev;
>  	r->dev = dev;
>  
> -	list_add_tail(&r->list, &ri->consumer_list);
> +	list_add_tail(&r->list, &rdev->consumer_list);
>  
>  	return r;
>  }
>  
> -static struct regulator_internal *regulator_by_name(const char *name)
> +static struct regulator_dev *regulator_by_name(const char *name)
>  {
> -	struct regulator_internal *ri;
> +	struct regulator_dev *rdev;
>  
> -	list_for_each_entry(ri, &regulator_list, list)
> -		if (ri->name && !strcmp(ri->name, name))
> -			return ri;
> +	list_for_each_entry(rdev, &regulator_list, list) {
> +		if (rdev->name && !strcmp(rdev->name, name))
> +			return rdev;
> +	}
>  
>  	return NULL;
>  }
>  
>  struct regulator *regulator_get_name(const char *name)
>  {
> -	struct regulator_internal *ri;
> +	struct regulator_dev *rdev;
>  	struct regulator *r;
>  
> -	ri = regulator_by_name(name);
> -	if (!ri)
> +	rdev = regulator_by_name(name);
> +	if (!rdev)
>  		return ERR_PTR(-ENODEV);
>  
>  	r = xzalloc(sizeof(*r));
> -	r->ri = ri;
> +	r->rdev = rdev;
>  
> -	list_add_tail(&r->list, &ri->consumer_list);
> +	list_add_tail(&r->list, &rdev->consumer_list);
>  
>  	return r;
>  }
> @@ -463,7 +445,7 @@ int regulator_enable(struct regulator *r)
>  	if (!r)
>  		return 0;
>  
> -	return regulator_enable_internal(r->ri);
> +	return regulator_enable_internal(r->rdev);
>  }
>  
>  /*
> @@ -480,7 +462,7 @@ int regulator_disable(struct regulator *r)
>  	if (!r)
>  		return 0;
>  
> -	return regulator_disable_internal(r->ri);
> +	return regulator_disable_internal(r->rdev);
>  }
>  
>  int regulator_set_voltage(struct regulator *r, int min_uV, int max_uV)
> @@ -488,7 +470,7 @@ int regulator_set_voltage(struct regulator *r, int min_uV, int max_uV)
>  	if (!r)
>  		return 0;
>  
> -	return regulator_set_voltage_internal(r->ri, min_uV, max_uV);
> +	return regulator_set_voltage_internal(r->rdev, min_uV, max_uV);
>  }
>  
>  /**
> @@ -632,15 +614,13 @@ EXPORT_SYMBOL_GPL(regulator_bulk_free);
>  
>  int regulator_get_voltage(struct regulator *regulator)
>  {
> -	struct regulator_internal *ri;
>  	struct regulator_dev *rdev;
>  	int sel, ret;
>  
>  	if (!regulator)
>  		return -EINVAL;
>  
> -	ri = regulator->ri;
> -	rdev = ri->rdev;
> +	rdev = regulator->rdev;
>  
>  	if (rdev->desc->ops->get_voltage_sel) {
>  		sel = rdev->desc->ops->get_voltage_sel(rdev);
> @@ -653,8 +633,8 @@ int regulator_get_voltage(struct regulator *regulator)
>  		ret = rdev->desc->ops->list_voltage(rdev, 0);
>  	} else if (rdev->desc->fixed_uV && (rdev->desc->n_voltages == 1)) {
>  		ret = rdev->desc->fixed_uV;
> -	} else if (ri->min_uv && ri->min_uv == ri->max_uv) {
> -		ret = ri->min_uv;
> +	} else if (rdev->min_uv && rdev->min_uv == rdev->max_uv) {
> +		ret = rdev->min_uv;
>  	} else if (rdev->supply) {
>  		ret = regulator_get_voltage(rdev->supply);
>  	} else {
> @@ -665,16 +645,16 @@ int regulator_get_voltage(struct regulator *regulator)
>  }
>  EXPORT_SYMBOL_GPL(regulator_get_voltage);
>  
> -static void regulator_print_one(struct regulator_internal *ri)
> +static void regulator_print_one(struct regulator_dev *rdev)
>  {
>  	struct regulator *r;
>  
> -	printf("%-20s %6d %10d %10d\n", ri->name, ri->enable_count, ri->min_uv, ri->max_uv);
> +	printf("%-20s %6d %10d %10d\n", rdev->name, rdev->enable_count, rdev->min_uv, rdev->max_uv);
>  
> -	if (!list_empty(&ri->consumer_list)) {
> +	if (!list_empty(&rdev->consumer_list)) {
>  		printf(" consumers:\n");
>  
> -		list_for_each_entry(r, &ri->consumer_list, list)
> +		list_for_each_entry(r, &rdev->consumer_list, list)
>  			printf("   %s\n", r->dev ? dev_name(r->dev) : "none");
>  	}
>  }
> @@ -684,9 +664,9 @@ static void regulator_print_one(struct regulator_internal *ri)
>   */
>  void regulators_print(void)
>  {
> -	struct regulator_internal *ri;
> +	struct regulator_dev *rdev;
>  
>  	printf("%-20s %6s %10s %10s\n", "name", "enable", "min_uv", "max_uv");
> -	list_for_each_entry(ri, &regulator_list, list)
> -		regulator_print_one(ri);
> +	list_for_each_entry(rdev, &regulator_list, list)
> +		regulator_print_one(rdev);
>  }
> diff --git a/include/regulator.h b/include/regulator.h
> index b0a500434c..5eb236e602 100644
> --- a/include/regulator.h
> +++ b/include/regulator.h
> @@ -85,6 +85,15 @@ struct regulator_desc {
>  };
>  
>  struct regulator_dev {
> +	const char *name;
> +	struct list_head list;
> +	struct device_node *node;
> +	int enable_count;
> +	int enable_time_us;
> +	int min_uv;
> +	int max_uv;
> +	const char *supply_name;
> +	struct list_head consumer_list;
>  	const struct regulator_desc *desc;
>  	struct regmap *regmap;
>  	bool boot_on;
> -- 
> 2.39.2
> 
> 
> 

[PATCH 03/11] regulator: introduce regulator logging functions.

[Sascha Hauer]

dev_* functions only print the struct device * as context, but often
a single struct device * implements multiple regulators. Add rdev_*
logging functions which allow to to print one specific regulator as
context.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/regulator/core.c | 12 ++++++++++--
 include/regulator.h      | 13 +++++++++++++
 2 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 41a3378ac8..8ef5a2372c 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -19,6 +19,14 @@ struct regulator {
 	struct device *dev;
 };
 
+const char *rdev_get_name(struct regulator_dev *rdev)
+{
+	if (rdev->name)
+		return rdev->name;
+
+	return "";
+}
+
 static int regulator_map_voltage(struct regulator_dev *rdev, int min_uV,
 				 int max_uV)
 {
@@ -125,7 +133,7 @@ static int regulator_resolve_supply(struct regulator_dev *rdev)
 	if (!supply_name)
 		return 0;
 
-	dev_dbg(rdev->dev, "resolving %s\n", supply_name);
+	rdev_dbg(rdev, "resolving %s\n", supply_name);
 
 	supply = regulator_get(rdev->dev, supply_name);
 	if (IS_ERR(supply)) {
@@ -141,7 +149,7 @@ static int regulator_resolve_supply(struct regulator_dev *rdev)
 		 * we couldn't. If you want to get rid of this warning, consider
 		 * migrating your platform to have deep probe support.
 		 */
-		dev_warn(rdev->dev, "Failed to get '%s' regulator (ignored).\n",
+		rdev_warn(rdev, "Failed to get '%s' regulator (ignored).\n",
 			 supply_name);
 		return 0;
 	}
diff --git a/include/regulator.h b/include/regulator.h
index 5eb236e602..d02ea8ffd0 100644
--- a/include/regulator.h
+++ b/include/regulator.h
@@ -159,6 +159,19 @@ int dev_regulator_register(struct regulator_dev *rd, const char * name,
 
 void regulators_print(void);
 
+const char *rdev_get_name(struct regulator_dev *rdev);
+
+#define rdev_crit(rdev, fmt, ...)                                       \
+        pr_crit("%s: " fmt, rdev_get_name(rdev), ##__VA_ARGS__)
+#define rdev_err(rdev, fmt, ...)                                        \
+        pr_err("%s: " fmt, rdev_get_name(rdev), ##__VA_ARGS__)
+#define rdev_warn(rdev, fmt, ...)                                       \
+        pr_warn("%s: " fmt, rdev_get_name(rdev), ##__VA_ARGS__)
+#define rdev_info(rdev, fmt, ...)                                       \
+        pr_info("%s: " fmt, rdev_get_name(rdev), ##__VA_ARGS__)
+#define rdev_dbg(rdev, fmt, ...)                                        \
+        pr_debug("%s: " fmt, rdev_get_name(rdev), ##__VA_ARGS__)
+
 #ifdef CONFIG_REGULATOR
 
 struct regulator *regulator_get(struct device *, const char *);
-- 
2.39.2

Re: [PATCH 03/11] regulator: introduce regulator logging functions.

[Marco Felsch]

On 23-09-20, Sascha Hauer wrote:
> dev_* functions only print the struct device * as context, but often
> a single struct device * implements multiple regulators. Add rdev_*
> logging functions which allow to to print one specific regulator as
> context.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>

Reviewed-by: Marco Felsch <m.felsch@pengutronix.de>

> ---
>  drivers/regulator/core.c | 12 ++++++++++--
>  include/regulator.h      | 13 +++++++++++++
>  2 files changed, 23 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index 41a3378ac8..8ef5a2372c 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -19,6 +19,14 @@ struct regulator {
>  	struct device *dev;
>  };
>  
> +const char *rdev_get_name(struct regulator_dev *rdev)
> +{
> +	if (rdev->name)
> +		return rdev->name;
> +
> +	return "";
> +}
> +
>  static int regulator_map_voltage(struct regulator_dev *rdev, int min_uV,
>  				 int max_uV)
>  {
> @@ -125,7 +133,7 @@ static int regulator_resolve_supply(struct regulator_dev *rdev)
>  	if (!supply_name)
>  		return 0;
>  
> -	dev_dbg(rdev->dev, "resolving %s\n", supply_name);
> +	rdev_dbg(rdev, "resolving %s\n", supply_name);
>  
>  	supply = regulator_get(rdev->dev, supply_name);
>  	if (IS_ERR(supply)) {
> @@ -141,7 +149,7 @@ static int regulator_resolve_supply(struct regulator_dev *rdev)
>  		 * we couldn't. If you want to get rid of this warning, consider
>  		 * migrating your platform to have deep probe support.
>  		 */
> -		dev_warn(rdev->dev, "Failed to get '%s' regulator (ignored).\n",
> +		rdev_warn(rdev, "Failed to get '%s' regulator (ignored).\n",
>  			 supply_name);
>  		return 0;
>  	}
> diff --git a/include/regulator.h b/include/regulator.h
> index 5eb236e602..d02ea8ffd0 100644
> --- a/include/regulator.h
> +++ b/include/regulator.h
> @@ -159,6 +159,19 @@ int dev_regulator_register(struct regulator_dev *rd, const char * name,
>  
>  void regulators_print(void);
>  
> +const char *rdev_get_name(struct regulator_dev *rdev);
> +
> +#define rdev_crit(rdev, fmt, ...)                                       \
> +        pr_crit("%s: " fmt, rdev_get_name(rdev), ##__VA_ARGS__)
> +#define rdev_err(rdev, fmt, ...)                                        \
> +        pr_err("%s: " fmt, rdev_get_name(rdev), ##__VA_ARGS__)
> +#define rdev_warn(rdev, fmt, ...)                                       \
> +        pr_warn("%s: " fmt, rdev_get_name(rdev), ##__VA_ARGS__)
> +#define rdev_info(rdev, fmt, ...)                                       \
> +        pr_info("%s: " fmt, rdev_get_name(rdev), ##__VA_ARGS__)
> +#define rdev_dbg(rdev, fmt, ...)                                        \
> +        pr_debug("%s: " fmt, rdev_get_name(rdev), ##__VA_ARGS__)
> +
>  #ifdef CONFIG_REGULATOR
>  
>  struct regulator *regulator_get(struct device *, const char *);
> -- 
> 2.39.2
> 
> 
> 

[PATCH 04/11] regulator: add regulator_get_voltage_internal()

[Sascha Hauer]

regulator_get_voltage() works on struct regulator * which may not always
be available internally, so add a regulator_get_voltage_internal() and
use it from regulator_get_voltage().

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/regulator/core.c | 52 +++++++++++++++++++++-------------------
 1 file changed, 27 insertions(+), 25 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 8ef5a2372c..5693fa9634 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -121,6 +121,32 @@ static int regulator_set_voltage_internal(struct regulator_dev *rdev,
 	return -ENOSYS;
 }
 
+static int regulator_get_voltage_internal(struct regulator_dev *rdev)
+{
+	int sel, ret;
+
+	if (rdev->desc->ops->get_voltage_sel) {
+		sel = rdev->desc->ops->get_voltage_sel(rdev);
+		if (sel < 0)
+			return sel;
+		ret = rdev->desc->ops->list_voltage(rdev, sel);
+	} else if (rdev->desc->ops->get_voltage) {
+		ret = rdev->desc->ops->get_voltage(rdev);
+	} else if (rdev->desc->ops->list_voltage) {
+		ret = rdev->desc->ops->list_voltage(rdev, 0);
+	} else if (rdev->desc->fixed_uV && (rdev->desc->n_voltages == 1)) {
+		ret = rdev->desc->fixed_uV;
+	} else if (rdev->min_uv && rdev->min_uv == rdev->max_uv) {
+		ret = rdev->min_uv;
+	} else if (rdev->supply) {
+		ret = regulator_get_voltage(rdev->supply);
+	} else {
+		return -EINVAL;
+	}
+
+	return ret;
+}
+
 static int regulator_resolve_supply(struct regulator_dev *rdev)
 {
 	struct regulator *supply;
@@ -622,34 +648,10 @@ EXPORT_SYMBOL_GPL(regulator_bulk_free);
 
 int regulator_get_voltage(struct regulator *regulator)
 {
-	struct regulator_dev *rdev;
-	int sel, ret;
-
 	if (!regulator)
 		return -EINVAL;
 
-	rdev = regulator->rdev;
-
-	if (rdev->desc->ops->get_voltage_sel) {
-		sel = rdev->desc->ops->get_voltage_sel(rdev);
-		if (sel < 0)
-			return sel;
-		ret = rdev->desc->ops->list_voltage(rdev, sel);
-	} else if (rdev->desc->ops->get_voltage) {
-		ret = rdev->desc->ops->get_voltage(rdev);
-	} else if (rdev->desc->ops->list_voltage) {
-		ret = rdev->desc->ops->list_voltage(rdev, 0);
-	} else if (rdev->desc->fixed_uV && (rdev->desc->n_voltages == 1)) {
-		ret = rdev->desc->fixed_uV;
-	} else if (rdev->min_uv && rdev->min_uv == rdev->max_uv) {
-		ret = rdev->min_uv;
-	} else if (rdev->supply) {
-		ret = regulator_get_voltage(rdev->supply);
-	} else {
-		return -EINVAL;
-	}
-
-	return ret;
+	return regulator_get_voltage_internal(regulator->rdev);
 }
 EXPORT_SYMBOL_GPL(regulator_get_voltage);
 
-- 
2.39.2

Re: [PATCH 04/11] regulator: add regulator_get_voltage_internal()

[Marco Felsch]

On 23-09-20, Sascha Hauer wrote:
> regulator_get_voltage() works on struct regulator * which may not always
> be available internally, so add a regulator_get_voltage_internal() and
> use it from regulator_get_voltage().
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  drivers/regulator/core.c | 52 +++++++++++++++++++++-------------------
>  1 file changed, 27 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index 8ef5a2372c..5693fa9634 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -121,6 +121,32 @@ static int regulator_set_voltage_internal(struct regulator_dev *rdev,
>  	return -ENOSYS;
>  }
>  
> +static int regulator_get_voltage_internal(struct regulator_dev *rdev)

Can we name this regulator_get_voltage_rdev()? This would align the code
with Linux.

Regards,
  Marco

> +{
> +	int sel, ret;
> +
> +	if (rdev->desc->ops->get_voltage_sel) {
> +		sel = rdev->desc->ops->get_voltage_sel(rdev);
> +		if (sel < 0)
> +			return sel;
> +		ret = rdev->desc->ops->list_voltage(rdev, sel);
> +	} else if (rdev->desc->ops->get_voltage) {
> +		ret = rdev->desc->ops->get_voltage(rdev);
> +	} else if (rdev->desc->ops->list_voltage) {
> +		ret = rdev->desc->ops->list_voltage(rdev, 0);
> +	} else if (rdev->desc->fixed_uV && (rdev->desc->n_voltages == 1)) {
> +		ret = rdev->desc->fixed_uV;
> +	} else if (rdev->min_uv && rdev->min_uv == rdev->max_uv) {
> +		ret = rdev->min_uv;
> +	} else if (rdev->supply) {
> +		ret = regulator_get_voltage(rdev->supply);
> +	} else {
> +		return -EINVAL;
> +	}
> +
> +	return ret;
> +}
> +
>  static int regulator_resolve_supply(struct regulator_dev *rdev)
>  {
>  	struct regulator *supply;
> @@ -622,34 +648,10 @@ EXPORT_SYMBOL_GPL(regulator_bulk_free);
>  
>  int regulator_get_voltage(struct regulator *regulator)
>  {
> -	struct regulator_dev *rdev;
> -	int sel, ret;
> -
>  	if (!regulator)
>  		return -EINVAL;
>  
> -	rdev = regulator->rdev;
> -
> -	if (rdev->desc->ops->get_voltage_sel) {
> -		sel = rdev->desc->ops->get_voltage_sel(rdev);
> -		if (sel < 0)
> -			return sel;
> -		ret = rdev->desc->ops->list_voltage(rdev, sel);
> -	} else if (rdev->desc->ops->get_voltage) {
> -		ret = rdev->desc->ops->get_voltage(rdev);
> -	} else if (rdev->desc->ops->list_voltage) {
> -		ret = rdev->desc->ops->list_voltage(rdev, 0);
> -	} else if (rdev->desc->fixed_uV && (rdev->desc->n_voltages == 1)) {
> -		ret = rdev->desc->fixed_uV;
> -	} else if (rdev->min_uv && rdev->min_uv == rdev->max_uv) {
> -		ret = rdev->min_uv;
> -	} else if (rdev->supply) {
> -		ret = regulator_get_voltage(rdev->supply);
> -	} else {
> -		return -EINVAL;
> -	}
> -
> -	return ret;
> +	return regulator_get_voltage_internal(regulator->rdev);
>  }
>  EXPORT_SYMBOL_GPL(regulator_get_voltage);
>  
> -- 
> 2.39.2
> 
> 
> 

Re: [PATCH 04/11] regulator: add regulator_get_voltage_internal()

[Sascha Hauer]

On Wed, Sep 20, 2023 at 01:25:31PM +0200, Marco Felsch wrote:
> On 23-09-20, Sascha Hauer wrote:
> > regulator_get_voltage() works on struct regulator * which may not always
> > be available internally, so add a regulator_get_voltage_internal() and
> > use it from regulator_get_voltage().
> > 
> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > ---
> >  drivers/regulator/core.c | 52 +++++++++++++++++++++-------------------
> >  1 file changed, 27 insertions(+), 25 deletions(-)
> > 
> > diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> > index 8ef5a2372c..5693fa9634 100644
> > --- a/drivers/regulator/core.c
> > +++ b/drivers/regulator/core.c
> > @@ -121,6 +121,32 @@ static int regulator_set_voltage_internal(struct regulator_dev *rdev,
> >  	return -ENOSYS;
> >  }
> >  
> > +static int regulator_get_voltage_internal(struct regulator_dev *rdev)
> 
> Can we name this regulator_get_voltage_rdev()? This would align the code
> with Linux.

Yes. The _internal suffix currently aligns with the other functions we
have in barebox, so I'll keep it here in this patch and add a patch to
this series changing the names for all functions.

Sascha


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH 05/11] regulator: Add missing cases in regulator_map_voltage()

[Sascha Hauer]

regulator_map_voltage() misses to handle some cases, sync this with the
kernel.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/regulator/core.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 5693fa9634..b9a97a784f 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -33,7 +33,10 @@ static int regulator_map_voltage(struct regulator_dev *rdev, int min_uV,
 	if (rdev->desc->ops->list_voltage == regulator_list_voltage_linear)
 		return regulator_map_voltage_linear(rdev, min_uV, max_uV);
 
-	return -ENOSYS;
+	if (rdev->desc->ops->list_voltage == regulator_list_voltage_linear_range)
+		return regulator_map_voltage_linear_range(rdev, min_uV, max_uV);
+
+	return regulator_map_voltage_iterate(rdev, min_uV, max_uV);
 }
 
 static int regulator_enable_internal(struct regulator_dev *rdev)
-- 
2.39.2

Re: [PATCH 05/11] regulator: Add missing cases in regulator_map_voltage()

[Marco Felsch]

On 23-09-20, Sascha Hauer wrote:
> regulator_map_voltage() misses to handle some cases, sync this with the
> kernel.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>

Reviewed-by: Marco Felsch <m.felsch@pengutronix.de>

[PATCH 06/11] regulator: stpmic1: add .get_voltage_sel

[Sascha Hauer]

To get the current voltage from the regulators the .get_voltage_sel
callback is needed. Initialize it to the correct function.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/regulator/stpmic1_regulator.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/regulator/stpmic1_regulator.c b/drivers/regulator/stpmic1_regulator.c
index c8b56e3a1a..ef8cc7f145 100644
--- a/drivers/regulator/stpmic1_regulator.c
+++ b/drivers/regulator/stpmic1_regulator.c
@@ -126,6 +126,7 @@ static const struct regulator_ops stpmic1_ldo_ops = {
 	.is_enabled = regulator_is_enabled_regmap,
 	.enable = regulator_enable_regmap,
 	.disable = regulator_disable_regmap,
+	.get_voltage_sel = regulator_get_voltage_sel_regmap,
 	.set_voltage_sel = regulator_set_voltage_sel_regmap,
 };
 
@@ -135,6 +136,7 @@ static const struct regulator_ops stpmic1_ldo3_ops = {
 	.is_enabled = regulator_is_enabled_regmap,
 	.enable = regulator_enable_regmap,
 	.disable = regulator_disable_regmap,
+	.get_voltage_sel = regulator_get_voltage_sel_regmap,
 	.set_voltage_sel = regulator_set_voltage_sel_regmap,
 };
 
@@ -150,6 +152,7 @@ static const struct regulator_ops stpmic1_buck_ops = {
 	.is_enabled = regulator_is_enabled_regmap,
 	.enable = regulator_enable_regmap,
 	.disable = regulator_disable_regmap,
+	.get_voltage_sel = regulator_get_voltage_sel_regmap,
 	.set_voltage_sel = regulator_set_voltage_sel_regmap,
 };
 
-- 
2.39.2

Re: [PATCH 06/11] regulator: stpmic1: add .get_voltage_sel

[Marco Felsch]

On 23-09-20, Sascha Hauer wrote:
> To get the current voltage from the regulators the .get_voltage_sel
> callback is needed. Initialize it to the correct function.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>

Reviewed-by: Marco Felsch <m.felsch@pengutronix.de>

[PATCH 07/11] regulator: stpmic1: add .supply_name

[Sascha Hauer]

Add .supply_name to the regulator descriptors, otherwise the supplies
are never enabled.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/regulator/stpmic1_regulator.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/regulator/stpmic1_regulator.c b/drivers/regulator/stpmic1_regulator.c
index ef8cc7f145..1e8c1b23ac 100644
--- a/drivers/regulator/stpmic1_regulator.c
+++ b/drivers/regulator/stpmic1_regulator.c
@@ -185,6 +185,7 @@ static const struct regulator_ops stpmic1_switch_regul_ops = {
 	.enable_mask = LDO_ENABLE_MASK, \
 	.enable_val = 1, \
 	.disable_val = 0, \
+	.supply_name = #base, \
 }
 
 #define REG_LDO3(ids, base) { \
@@ -198,6 +199,7 @@ static const struct regulator_ops stpmic1_switch_regul_ops = {
 	.enable_mask = LDO_ENABLE_MASK, \
 	.enable_val = 1, \
 	.disable_val = 0, \
+	.supply_name = #base, \
 }
 
 #define REG_LDO4(ids, base) { \
@@ -208,6 +210,7 @@ static const struct regulator_ops stpmic1_switch_regul_ops = {
 	.enable_mask = LDO_ENABLE_MASK, \
 	.enable_val = 1, \
 	.disable_val = 0, \
+	.supply_name = #base, \
 }
 
 #define REG_BUCK(ids, base) { \
@@ -221,6 +224,7 @@ static const struct regulator_ops stpmic1_switch_regul_ops = {
 	.enable_mask = BUCK_ENABLE_MASK, \
 	.enable_val = 1, \
 	.disable_val = 0, \
+	.supply_name = #base, \
 }
 
 #define REG_VREF_DDR(ids, base) { \
@@ -231,6 +235,7 @@ static const struct regulator_ops stpmic1_switch_regul_ops = {
 	.enable_mask = BUCK_ENABLE_MASK, \
 	.enable_val = 1, \
 	.disable_val = 0, \
+	.supply_name = #base, \
 }
 
 #define REG_BOOST(ids, base) { \
@@ -241,6 +246,7 @@ static const struct regulator_ops stpmic1_switch_regul_ops = {
 	.enable_mask = BOOST_ENABLED, \
 	.enable_val = BOOST_ENABLED, \
 	.disable_val = 0, \
+	.supply_name = #base, \
 }
 
 #define REG_VBUS_OTG(ids, base) { \
@@ -251,6 +257,7 @@ static const struct regulator_ops stpmic1_switch_regul_ops = {
 	.enable_mask = USBSW_OTG_SWITCH_ENABLED, \
 	.enable_val = USBSW_OTG_SWITCH_ENABLED, \
 	.disable_val = 0, \
+	.supply_name = #base, \
 }
 
 #define REG_SW_OUT(ids, base) { \
@@ -261,6 +268,7 @@ static const struct regulator_ops stpmic1_switch_regul_ops = {
 	.enable_mask = SWIN_SWOUT_ENABLED, \
 	.enable_val = SWIN_SWOUT_ENABLED, \
 	.disable_val = 0, \
+	.supply_name = #base, \
 }
 
 static struct stpmic1_regulator_cfg stpmic1_regulator_cfgs[] = {
-- 
2.39.2

Re: [PATCH 07/11] regulator: stpmic1: add .supply_name

[Marco Felsch]

On 23-09-20, Sascha Hauer wrote:
> Add .supply_name to the regulator descriptors, otherwise the supplies
> are never enabled.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>

Reviewed-by: Marco Felsch <m.felsch@pengutronix.de>

[PATCH 08/11] regulator: register regulator as last step in of_regulator_register()

[Sascha Hauer]

In of_regulator_register() call __regulator_register as last step after
all fields have been initialized. This was not possible before as
__regulator_register() returned the struct regulator_internal * which
contained the remaining fields. Now that struct regulator_internal is
gone we can restore the natural initialization order.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/regulator/core.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index b9a97a784f..d08df1dc68 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -240,10 +240,6 @@ int of_regulator_register(struct regulator_dev *rdev, struct device_node *node)
 	if (!name)
 		name = node->name;
 
-	ret = __regulator_register(rdev, name);
-	if (ret)
-		return ret;
-
 	rdev->node = node;
 	node->dev = rdev->dev;
 
@@ -260,6 +256,10 @@ int of_regulator_register(struct regulator_dev *rdev, struct device_node *node)
 	of_property_read_u32(node, "regulator-max-microvolt",
 			&rdev->max_uv);
 
+	ret = __regulator_register(rdev, name);
+	if (ret)
+		return ret;
+
 	return 0;
 }
 
-- 
2.39.2

Re: [PATCH 08/11] regulator: register regulator as last step in of_regulator_register()

[Marco Felsch]

On 23-09-20, Sascha Hauer wrote:
> In of_regulator_register() call __regulator_register as last step after
> all fields have been initialized. This was not possible before as
> __regulator_register() returned the struct regulator_internal * which
> contained the remaining fields. Now that struct regulator_internal is
> gone we can restore the natural initialization order.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  drivers/regulator/core.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index b9a97a784f..d08df1dc68 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -240,10 +240,6 @@ int of_regulator_register(struct regulator_dev *rdev, struct device_node *node)
>  	if (!name)
>  		name = node->name;
>  
> -	ret = __regulator_register(rdev, name);
> -	if (ret)
> -		return ret;
> -
>  	rdev->node = node;
>  	node->dev = rdev->dev;
>  
> @@ -260,6 +256,10 @@ int of_regulator_register(struct regulator_dev *rdev, struct device_node *node)
>  	of_property_read_u32(node, "regulator-max-microvolt",
>  			&rdev->max_uv);
>  
> +	ret = __regulator_register(rdev, name);

	return __regulator_register(rdev, name);

Regards,
  Marco

> +	if (ret)
> +		return ret;
> +
>  	return 0;
>  }
>  
> -- 
> 2.39.2
> 
> 
> 

[PATCH 09/11] regulator: Set initial voltage

[Sascha Hauer]

When voltage constraints are given in the device tree then we should
set the regulator to a valid voltage before enabling it. Without it
we can end up with invalid voltages.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/regulator/core.c | 60 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 60 insertions(+)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index d08df1dc68..00a2aefce7 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -187,6 +187,62 @@ static int regulator_resolve_supply(struct regulator_dev *rdev)
 	return 0;
 }
 
+static int regulator_init_voltage(struct regulator_dev *rdev)
+{
+	int target_min, target_max, current_uV, ret;
+
+	if (!rdev->min_uv || !rdev->max_uv)
+		return 0;
+
+	current_uV = regulator_get_voltage_internal(rdev);
+	if (current_uV < 0) {
+		/* This regulator can't be read and must be initialized */
+		rdev_info(rdev, "Setting %d-%duV\n", rdev->min_uv, rdev->max_uv);
+		regulator_set_voltage_internal(rdev, rdev->min_uv, rdev->max_uv);
+		current_uV = regulator_get_voltage_internal(rdev);
+	}
+
+	if (current_uV < 0) {
+		if (current_uV != -EPROBE_DEFER)
+			rdev_err(rdev,
+				 "failed to get the current voltage: %pe\n",
+				 ERR_PTR(current_uV));
+		return current_uV;
+	}
+
+	/*
+	 * If we're below the minimum voltage move up to the
+	 * minimum voltage, if we're above the maximum voltage
+	 * then move down to the maximum.
+	 */
+	target_min = current_uV;
+	target_max = current_uV;
+
+	if (current_uV < rdev->min_uv) {
+		target_min = rdev->min_uv;
+		target_max = rdev->min_uv;
+	}
+
+	if (current_uV > rdev->max_uv) {
+		target_min = rdev->max_uv;
+		target_max = rdev->max_uv;
+	}
+
+	if (target_min != current_uV || target_max != current_uV) {
+		rdev_info(rdev, "Bringing %duV into %d-%duV\n",
+			  current_uV, target_min, target_max);
+		ret = regulator_set_voltage_internal(rdev, target_min, target_max);
+		if (ret < 0) {
+			rdev_err(rdev,
+				"failed to apply %d-%duV constraint: %pe\n",
+				target_min, target_max, ERR_PTR(ret));
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
 static int __regulator_register(struct regulator_dev *rdev, const char *name)
 {
 	int ret;
@@ -198,6 +254,10 @@ static int __regulator_register(struct regulator_dev *rdev, const char *name)
 	if (name)
 		rdev->name = xstrdup(name);
 
+	ret = regulator_init_voltage(rdev);
+	if (ret)
+		goto err;
+
 	if (rdev->boot_on || rdev->always_on) {
 		ret = regulator_resolve_supply(rdev);
 		if (ret < 0)
-- 
2.39.2

Re: [PATCH 09/11] regulator: Set initial voltage

[Marco Felsch]

On 23-09-20, Sascha Hauer wrote:
> When voltage constraints are given in the device tree then we should
> set the regulator to a valid voltage before enabling it. Without it
> we can end up with invalid voltages.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>

Reviewed-by: Marco Felsch <m.felsch@pengutronix.de>

[PATCH 10/11] regulator: drop struct regulator_dev::supply_name

[Sascha Hauer]

The supply name can be set in struct regulator_desc, no need to have
it in struct regulator_dev also, so drop it there.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/regulator/bcm2835.c | 2 +-
 drivers/regulator/core.c    | 8 +++-----
 include/regulator.h         | 4 +---
 3 files changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/regulator/bcm2835.c b/drivers/regulator/bcm2835.c
index 34e0429dfd..fa9fc47207 100644
--- a/drivers/regulator/bcm2835.c
+++ b/drivers/regulator/bcm2835.c
@@ -122,7 +122,7 @@ static int regulator_bcm2835_probe(struct device *dev)
 		rb->rdev.desc = &rb->rdesc;
 		rb->rdev.dev = dev;
 
-		ret = dev_regulator_register(&rb->rdev, rb->devname, NULL);
+		ret = dev_regulator_register(&rb->rdev, rb->devname);
 		if (ret)
 			return ret;
 	}
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 00a2aefce7..7fe264a133 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -409,7 +409,7 @@ static struct regulator_dev *of_regulator_get(struct device *dev,
 }
 #endif
 
-int dev_regulator_register(struct regulator_dev *rdev, const char * name, const char* supply)
+int dev_regulator_register(struct regulator_dev *rdev, const char *name)
 {
 	int ret;
 
@@ -417,8 +417,6 @@ int dev_regulator_register(struct regulator_dev *rdev, const char * name, const
 	if (ret)
 		return ret;
 
-	rdev->supply_name = supply;
-
 	return 0;
 }
 
@@ -437,8 +435,8 @@ static struct regulator_dev *dev_regulator_get(struct device *dev,
 				continue;
 			match += 2;
 		}
-		if (rdev->supply_name) {
-			if (!supply || strcmp(rdev->supply_name, supply))
+		if (rdev->desc->supply_name) {
+			if (!supply || strcmp(rdev->desc->supply_name, supply))
 				continue;
 			match += 1;
 		}
diff --git a/include/regulator.h b/include/regulator.h
index d02ea8ffd0..c5b405dfcd 100644
--- a/include/regulator.h
+++ b/include/regulator.h
@@ -92,7 +92,6 @@ struct regulator_dev {
 	int enable_time_us;
 	int min_uv;
 	int max_uv;
-	const char *supply_name;
 	struct list_head consumer_list;
 	const struct regulator_desc *desc;
 	struct regmap *regmap;
@@ -154,8 +153,7 @@ static inline int of_regulator_register(struct regulator_dev *rd,
 	return -ENOSYS;
 }
 #endif
-int dev_regulator_register(struct regulator_dev *rd, const char * name,
-			   const char* supply);
+int dev_regulator_register(struct regulator_dev *rd, const char *name);
 
 void regulators_print(void);
 
-- 
2.39.2

Re: [PATCH 10/11] regulator: drop struct regulator_dev::supply_name

[Marco Felsch]

On 23-09-20, Sascha Hauer wrote:
> The supply name can be set in struct regulator_desc, no need to have
> it in struct regulator_dev also, so drop it there.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>

Reviewed-by: Marco Felsch <m.felsch@pengutronix.de>

[PATCH 11/11] regulator: print regulator tree

[Sascha Hauer]

Regulators have a tree structure, so print them as a tree to show the
users their dependencies.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/regulator/core.c | 30 +++++++++++++++++++++---------
 1 file changed, 21 insertions(+), 9 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 7fe264a133..a861d14cd2 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -15,6 +15,7 @@ static LIST_HEAD(regulator_list);
 
 struct regulator {
 	struct regulator_dev *rdev;
+	struct regulator_dev *rdev_consumer;
 	struct list_head list;
 	struct device *dev;
 };
@@ -183,7 +184,11 @@ static int regulator_resolve_supply(struct regulator_dev *rdev)
 		return 0;
 	}
 
+	if (supply)
+		supply->rdev_consumer = rdev;
+
 	rdev->supply = supply;
+
 	return 0;
 }
 
@@ -716,17 +721,22 @@ int regulator_get_voltage(struct regulator *regulator)
 }
 EXPORT_SYMBOL_GPL(regulator_get_voltage);
 
-static void regulator_print_one(struct regulator_dev *rdev)
+static void regulator_print_one(struct regulator_dev *rdev, int level)
 {
 	struct regulator *r;
 
-	printf("%-20s %6d %10d %10d\n", rdev->name, rdev->enable_count, rdev->min_uv, rdev->max_uv);
+	if (!rdev)
+		return;
 
-	if (!list_empty(&rdev->consumer_list)) {
-		printf(" consumers:\n");
+	printf("%*s%-*s %6d %10d %10d\n", level * 3, "",
+	       30 - level * 3,
+	       rdev->name, rdev->enable_count, rdev->min_uv, rdev->max_uv);
 
-		list_for_each_entry(r, &rdev->consumer_list, list)
-			printf("   %s\n", r->dev ? dev_name(r->dev) : "none");
+	list_for_each_entry(r, &rdev->consumer_list, list) {
+		if (r->rdev_consumer)
+			regulator_print_one(r->rdev_consumer, level + 1);
+		else
+			printf("%*s%s\n", (level + 1) * 3, "", r->dev ? dev_name(r->dev) : "none");
 	}
 }
 
@@ -737,7 +747,9 @@ void regulators_print(void)
 {
 	struct regulator_dev *rdev;
 
-	printf("%-20s %6s %10s %10s\n", "name", "enable", "min_uv", "max_uv");
-	list_for_each_entry(rdev, &regulator_list, list)
-		regulator_print_one(rdev);
+	printf("%-30s %6s %10s %10s\n", "name", "enable", "min_uv", "max_uv");
+	list_for_each_entry(rdev, &regulator_list, list) {
+		if (!rdev->supply)
+			regulator_print_one(rdev, 0);
+	}
 }
-- 
2.39.2