[PATCH v2 0/7] General regulator and pmic uclass improvements

[PATCH v2 0/7] General regulator and pmic uclass improvements

[Svyatoslav Ryhel]

This patchset derives from discussion of TPS65913 bringup and aims to
cycle all regulator management inside uclass removing need of any board
calls for regulators.

My hw setup is Tegra 3 LG Optimus Vu P895 (PMIC is MAX77663) with native
spl u-boot build.

power: regulator: expand basic reference counter onto all uclass
Commit is basically expansion of 4fcba5d ("regulator: implement basic
reference counter") onto all regulators.
 - my testing on hw has shown no issues so far with both pmic regulators
 and fixed regulators. Counting works as expected, dm test is set in
 test of regulator_set_enable_if_allowed.

power: regulator-uclass: perform regulator setup inside uclass
All self-sufficient regulators are set to probe after bind to perform
initial setup later in post-probe with pdata from device tree.
 - self-sufficient regulator is one that is not dependent on any other
 device (this category does not include pmic regulators for example).
 I have tested fixed regulators and got correct probing and setup
 according to device tree. DM test is set by checking regulators data
 without pre-configuring them manually

power: pmic-uclass: probe every child on pmic_post_probe
All non self-sufficient regulators must be probed after main device is
probed (in this case it is pmic_post_probe). In all other aspects pmic
regulators behave same.
 - tested with MAX77663 ldo and sd regulators, no errors or inconsistencies
 were tracked, regulator props (boot-on, always-on etc) and consumer calls
 work as expected. DM test is set by checking regulators data without pre-
 configuring them manually just after pmic probe.

power: regulator-uclass: remove all deprecated API use
This is where everything gets tricky. All board dedicated API of regulators
has to be removed. System presented above should cover all regulators setup
but non the less this should be disscussed with maintainers and tested on
affected boards. This commit removes and cleans most of those API traces.

---
Changes from v1:
 - adapted description of regulator_set_enable
 - remove uc_pdata->enable_count from post_probe
 - added tests from counter and regulators post_probe
---

Svyatoslav Ryhel (7):
  power: regulator: expand basic reference counter onto all uclass
  test: dm: regulator: test counter in set_enable_if_allowed test
  power: regulator-uclass: perform regulator setup inside uclass
  test: dm: regulator: provide test of auto setup
  power: pmic-uclass: probe every child on pmic_post_probe
  test: dm: pmic: provide test of child autosetup
  power: regulator-uclass: remove all deprecated API use

 arch/arm/mach-rockchip/board.c                |   8 -
 arch/arm/mach-rockchip/rk3399/rk3399.c        |  10 -
 board/Marvell/octeontx2_cn913x/board.c        |   5 -
 .../amlogic/odroid-go-ultra/odroid-go-ultra.c |   2 -
 board/dhelectronics/dh_stm32mp1/board.c       |   2 -
 board/engicam/stm32mp1/stm32mp1.c             |   3 -
 board/google/veyron/veyron.c                  |   6 -
 board/samsung/common/exynos5-dt.c             |   4 -
 board/samsung/odroid/odroid.c                 |  10 -
 board/st/stm32mp1/stm32mp1.c                  |   9 -
 drivers/power/pmic/pmic-uclass.c              |  10 +
 drivers/power/regulator/regulator-uclass.c    | 253 +++++++-----------
 drivers/power/regulator/regulator_common.c    |  22 --
 drivers/power/regulator/regulator_common.h    |  21 --
 drivers/video/bridge/ps862x.c                 |  12 +-
 drivers/video/rockchip/rk_vop.c               |   6 +-
 include/power/regulator.h                     | 121 +--------
 include/power/sandbox_pmic.h                  |   2 +-
 test/dm/pmic.c                                |  34 +++
 test/dm/regulator.c                           | 148 +++-------
 20 files changed, 204 insertions(+), 484 deletions(-)

-- 
2.39.2

[PATCH v2 1/7] power: regulator: expand basic reference counter onto all uclass

[Svyatoslav Ryhel]

Commit is based on 4fcba5d ("regulator: implement basic reference
counter") but expands the idea to all regulators instead of just
fixed/gpio regulators.

Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
---
 drivers/power/regulator/regulator-uclass.c | 41 ++++++++++++++++++++++
 drivers/power/regulator/regulator_common.c | 22 ------------
 drivers/power/regulator/regulator_common.h | 21 -----------
 include/power/regulator.h                  |  2 ++
 4 files changed, 43 insertions(+), 43 deletions(-)

diff --git a/drivers/power/regulator/regulator-uclass.c b/drivers/power/regulator/regulator-uclass.c
index 3a6ba69f6d..fc7a4631b4 100644
--- a/drivers/power/regulator/regulator-uclass.c
+++ b/drivers/power/regulator/regulator-uclass.c
@@ -159,6 +159,25 @@ int regulator_get_enable(struct udevice *dev)
 	return ops->get_enable(dev);
 }
 
+/*
+ * Enable or Disable a regulator
+ *
+ * This is a reentrant function and subsequent calls that enable will
+ * increase an internal counter, and disable calls will decrease the counter.
+ * The actual resource will be enabled when the counter gets to 1 coming from 0,
+ * and disabled when it reaches 0 coming from 1.
+ *
+ * @dev: regulator device
+ * @enable: bool indicating whether to enable or disable the regulator
+ * @return:
+ * 0 on Success
+ * -EBUSY if the regulator cannot be disabled because it's requested by
+ *        another device
+ * -EALREADY if the regulator has already been enabled or has already been
+ *        disabled
+ * -EACCES if there is no possibility to enable/disable the regulator
+ * -ve on different error situation
+ */
 int regulator_set_enable(struct udevice *dev, bool enable)
 {
 	const struct dm_regulator_ops *ops = dev_get_driver_ops(dev);
@@ -172,6 +191,23 @@ int regulator_set_enable(struct udevice *dev, bool enable)
 	if (!enable && uc_pdata->always_on)
 		return -EACCES;
 
+	/* If previously enabled, increase count */
+	if (enable && uc_pdata->enable_count > 0) {
+		uc_pdata->enable_count++;
+		return -EALREADY;
+	}
+
+	if (!enable) {
+		if (uc_pdata->enable_count > 1) {
+			/* If enabled multiple times, decrease count */
+			uc_pdata->enable_count--;
+			return -EBUSY;
+		} else if (!uc_pdata->enable_count) {
+			/* If already disabled, do nothing */
+			return -EALREADY;
+		}
+	}
+
 	if (uc_pdata->ramp_delay)
 		old_enable = regulator_get_enable(dev);
 
@@ -187,6 +223,11 @@ int regulator_set_enable(struct udevice *dev, bool enable)
 		}
 	}
 
+	if (enable)
+		uc_pdata->enable_count++;
+	else
+		uc_pdata->enable_count--;
+
 	return ret;
 }
 
diff --git a/drivers/power/regulator/regulator_common.c b/drivers/power/regulator/regulator_common.c
index e26f5ebec3..d88bc6f6de 100644
--- a/drivers/power/regulator/regulator_common.c
+++ b/drivers/power/regulator/regulator_common.c
@@ -72,23 +72,6 @@ int regulator_common_set_enable(const struct udevice *dev,
 		return 0;
 	}
 
-	/* If previously enabled, increase count */
-	if (enable && plat->enable_count > 0) {
-		plat->enable_count++;
-		return -EALREADY;
-	}
-
-	if (!enable) {
-		if (plat->enable_count > 1) {
-			/* If enabled multiple times, decrease count */
-			plat->enable_count--;
-			return -EBUSY;
-		} else if (!plat->enable_count) {
-			/* If already disabled, do nothing */
-			return -EALREADY;
-		}
-	}
-
 	ret = dm_gpio_set_value(&plat->gpio, enable);
 	if (ret) {
 		pr_err("Can't set regulator : %s gpio to: %d\n", dev->name,
@@ -103,10 +86,5 @@ int regulator_common_set_enable(const struct udevice *dev,
 	if (!enable && plat->off_on_delay_us)
 		udelay(plat->off_on_delay_us);
 
-	if (enable)
-		plat->enable_count++;
-	else
-		plat->enable_count--;
-
 	return 0;
 }
diff --git a/drivers/power/regulator/regulator_common.h b/drivers/power/regulator/regulator_common.h
index d4962899d8..15f1fa4c93 100644
--- a/drivers/power/regulator/regulator_common.h
+++ b/drivers/power/regulator/regulator_common.h
@@ -13,7 +13,6 @@ struct regulator_common_plat {
 	struct gpio_desc gpio; /* GPIO for regulator enable control */
 	unsigned int startup_delay_us;
 	unsigned int off_on_delay_us;
-	unsigned int enable_count;
 };
 
 int regulator_common_of_to_plat(struct udevice *dev,
@@ -21,26 +20,6 @@ int regulator_common_of_to_plat(struct udevice *dev,
 				char *enable_gpio_name);
 int regulator_common_get_enable(const struct udevice *dev,
 	struct regulator_common_plat *plat);
-/*
- * Enable or Disable a regulator
- *
- * This is a reentrant function and subsequent calls that enable will
- * increase an internal counter, and disable calls will decrease the counter.
- * The actual resource will be enabled when the counter gets to 1 coming from 0,
- * and disabled when it reaches 0 coming from 1.
- *
- * @dev: regulator device
- * @plat: Platform data
- * @enable: bool indicating whether to enable or disable the regulator
- * @return:
- * 0 on Success
- * -EBUSY if the regulator cannot be disabled because it's requested by
- *        another device
- * -EALREADY if the regulator has already been enabled or has already been
- *        disabled
- * -EACCES if there is no possibility to enable/disable the regulator
- * -ve on different error situation
- */
 int regulator_common_set_enable(const struct udevice *dev,
 	struct regulator_common_plat *plat, bool enable);
 
diff --git a/include/power/regulator.h b/include/power/regulator.h
index ff1bfc2435..727776a8cf 100644
--- a/include/power/regulator.h
+++ b/include/power/regulator.h
@@ -158,6 +158,7 @@ enum regulator_flag {
  * @name**     - fdt regulator name - should be taken from the device tree
  * ctrl_reg:   - Control register offset used to enable/disable regulator
  * volt_reg:   - register offset for writing voltage vsel values
+ * enable_count - counter of enable calls for this regulator
  *
  * Note:
  * *  - set automatically on device probe by the uclass's '.pre_probe' method.
@@ -184,6 +185,7 @@ struct dm_regulator_uclass_plat {
 	u8 volt_reg;
 	bool suspend_on;
 	u32 suspend_uV;
+	u32 enable_count;
 };
 
 /* Regulator device operations */
-- 
2.39.2

[PATCH v2 2/7] test: dm: regulator: test counter in set_enable_if_allowed test

[Svyatoslav Ryhel]

Emulate use of the regulator by a few consumers with balanced on/off
calls.

Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
---
 test/dm/regulator.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/test/dm/regulator.c b/test/dm/regulator.c
index 86f4862d9d..9b503e4c59 100644
--- a/test/dm/regulator.c
+++ b/test/dm/regulator.c
@@ -194,6 +194,25 @@ int dm_test_power_regulator_set_enable_if_allowed(struct unit_test_state *uts)
 	ut_assertok(regulator_set_enable_if_allowed(dev, val_set));
 	ut_asserteq(regulator_get_enable(dev), !val_set);
 
+	/* Set the Enable of LDO1 - default is disabled */
+	platname = regulator_names[LDO1][PLATNAME];
+	ut_assertok(regulator_autoset_by_name(platname, &dev_autoset));
+	ut_assertok(regulator_get_by_platname(platname, &dev));
+
+	/* Get the Enable state of LDO1 and compare it with the requested one */
+	ut_assertok(regulator_set_enable_if_allowed(dev, true));
+	ut_asserteq(regulator_get_enable(dev), true);
+
+	/* Emulate second consumer */
+	ut_assertok(regulator_set_enable_if_allowed(dev, true));
+	ut_asserteq(regulator_get_enable(dev), true);
+
+	/* Emulate one of counsumers disable call */
+	ut_assertok(regulator_set_enable_if_allowed(dev, false));
+
+	/* Regulator should still be on since counter indicates one consumer active */
+	ut_asserteq(regulator_get_enable(dev), true);
+
 	return 0;
 }
 DM_TEST(dm_test_power_regulator_set_enable_if_allowed, UT_TESTF_SCAN_FDT);
-- 
2.39.2

[PATCH v2 3/7] power: regulator-uclass: perform regulator setup inside uclass

[Svyatoslav Ryhel]

Regulators initial setup was previously dependent on board call.
To move from this behaviour were introduced two steps. First is
that all individual regulators will be probed just after binding
which ensures that regulator pdata is filled and second is setting
up regulator in post probe which enseres that correct regulator
state according to pdata is reached.

Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
---
 drivers/power/regulator/regulator-uclass.c | 212 ++++++---------------
 include/power/regulator.h                  | 119 ------------
 2 files changed, 62 insertions(+), 269 deletions(-)

diff --git a/drivers/power/regulator/regulator-uclass.c b/drivers/power/regulator/regulator-uclass.c
index fc7a4631b4..32f4e64ca4 100644
--- a/drivers/power/regulator/regulator-uclass.c
+++ b/drivers/power/regulator/regulator-uclass.c
@@ -327,112 +327,6 @@ int device_get_supply_regulator(struct udevice *dev, const char *supply_name,
 					    supply_name, devp);
 }
 
-int regulator_autoset(struct udevice *dev)
-{
-	struct dm_regulator_uclass_plat *uc_pdata;
-	int ret = 0;
-
-	uc_pdata = dev_get_uclass_plat(dev);
-
-	ret = regulator_set_suspend_enable(dev, uc_pdata->suspend_on);
-	if (ret == -ENOSYS)
-		ret = 0;
-
-	if (!ret && uc_pdata->suspend_on) {
-		ret = regulator_set_suspend_value(dev, uc_pdata->suspend_uV);
-		if (ret == -ENOSYS)
-			ret = 0;
-
-		if (ret)
-			return ret;
-	}
-
-	if (!uc_pdata->always_on && !uc_pdata->boot_on)
-		return -EMEDIUMTYPE;
-
-	if (uc_pdata->type == REGULATOR_TYPE_FIXED)
-		return regulator_set_enable(dev, true);
-
-	if (uc_pdata->flags & REGULATOR_FLAG_AUTOSET_UV)
-		ret = regulator_set_value(dev, uc_pdata->min_uV);
-	if (uc_pdata->init_uV > 0)
-		ret = regulator_set_value(dev, uc_pdata->init_uV);
-	if (!ret && (uc_pdata->flags & REGULATOR_FLAG_AUTOSET_UA))
-		ret = regulator_set_current(dev, uc_pdata->min_uA);
-
-	if (!ret)
-		ret = regulator_set_enable(dev, true);
-
-	return ret;
-}
-
-int regulator_unset(struct udevice *dev)
-{
-	struct dm_regulator_uclass_plat *uc_pdata;
-
-	uc_pdata = dev_get_uclass_plat(dev);
-	if (uc_pdata && uc_pdata->force_off)
-		return regulator_set_enable(dev, false);
-
-	return -EMEDIUMTYPE;
-}
-
-static void regulator_show(struct udevice *dev, int ret)
-{
-	struct dm_regulator_uclass_plat *uc_pdata;
-
-	uc_pdata = dev_get_uclass_plat(dev);
-
-	printf("%s@%s: ", dev->name, uc_pdata->name);
-	if (uc_pdata->flags & REGULATOR_FLAG_AUTOSET_UV)
-		printf("set %d uV", uc_pdata->min_uV);
-	if (uc_pdata->flags & REGULATOR_FLAG_AUTOSET_UA)
-		printf("; set %d uA", uc_pdata->min_uA);
-	printf("; enabling");
-	if (ret)
-		printf(" (ret: %d)", ret);
-	printf("\n");
-}
-
-int regulator_autoset_by_name(const char *platname, struct udevice **devp)
-{
-	struct udevice *dev;
-	int ret;
-
-	ret = regulator_get_by_platname(platname, &dev);
-	if (devp)
-		*devp = dev;
-	if (ret) {
-		debug("Can get the regulator: %s (err=%d)\n", platname, ret);
-		return ret;
-	}
-
-	return regulator_autoset(dev);
-}
-
-int regulator_list_autoset(const char *list_platname[],
-			   struct udevice *list_devp[],
-			   bool verbose)
-{
-	struct udevice *dev;
-	int error = 0, i = 0, ret;
-
-	while (list_platname[i]) {
-		ret = regulator_autoset_by_name(list_platname[i], &dev);
-		if (ret != -EMEDIUMTYPE && verbose)
-			regulator_show(dev, ret);
-		if (ret & !error)
-			error = ret;
-
-		if (list_devp)
-			list_devp[i] = dev;
-
-		i++;
-	}
-
-	return error;
-}
-
 static bool regulator_name_is_unique(struct udevice *check_dev,
 				     const char *check_name)
 {
@@ -463,6 +357,7 @@ static int regulator_post_bind(struct udevice *dev)
 {
 	struct dm_regulator_uclass_plat *uc_pdata;
 	const char *property = "regulator-name";
+	int ret;
 
 	uc_pdata = dev_get_uclass_plat(dev);
 
@@ -476,13 +371,28 @@ static int regulator_post_bind(struct udevice *dev)
 			return -EINVAL;
 	}
 
-	if (regulator_name_is_unique(dev, uc_pdata->name))
-		return 0;
+	ret = regulator_name_is_unique(dev, uc_pdata->name);
+	if (!ret) {
+		debug("'%s' of dev: '%s', has nonunique value: '%s\n",
+		      property, dev->name, uc_pdata->name);
+		return -EINVAL;
+	}
 
-	debug("'%s' of dev: '%s', has nonunique value: '%s\n",
-	      property, dev->name, uc_pdata->name);
+	switch (device_get_uclass_id(dev->parent)) {
+	/* In case class can have regulator child add it here */
+	case UCLASS_PMIC:
+		break;
+
+	default:
+		/*
+		 * Trigger probe() to configure default
+		 * regulators state during startup.
+		 */
+		dev_or_flags(dev, DM_FLAG_PROBE_AFTER_BIND);
+		break;
+	}
 
-	return -EINVAL;
+	return 0;
 }
 
 static int regulator_pre_probe(struct udevice *dev)
@@ -536,55 +446,56 @@ static int regulator_pre_probe(struct udevice *dev)
 	return 0;
 }
 
-int regulators_enable_boot_on(bool verbose)
+static int regulator_post_probe(struct udevice *dev)
 {
-	struct udevice *dev;
-	struct uclass *uc;
+	struct dm_regulator_uclass_plat *uc_pdata =
+					 dev_get_uclass_plat(dev);
 	int ret;
 
-	ret = uclass_get(UCLASS_REGULATOR, &uc);
-	if (ret)
-		return ret;
-	for (uclass_first_device(UCLASS_REGULATOR, &dev);
-	     dev;
-	     uclass_next_device(&dev)) {
-		ret = regulator_autoset(dev);
-		if (ret == -EMEDIUMTYPE) {
-			ret = 0;
-			continue;
-		}
-		if (verbose)
-			regulator_show(dev, ret);
-		if (ret == -ENOSYS)
-			ret = 0;
-	}
+	/*
+	 * Disable is performed in case regulator was somehow
+	 * active, for example it is active by PMIC design etc.
+	 */
+	uc_pdata->enable_count = 1;
+	regulator_set_enable(dev, false);
 
-	return ret;
-}
+	/*
+	 * Always-on regulator can not be disabled so zero out
+	 * enable_count in case regulator has this property.
+	 */
+	uc_pdata->enable_count = 0;
 
-int regulators_enable_boot_off(bool verbose)
-{
-	struct udevice *dev;
-	struct uclass *uc;
-	int ret;
+	if (uc_pdata->force_off)
+		return 0;
 
-	ret = uclass_get(UCLASS_REGULATOR, &uc);
-	if (ret)
-		return ret;
-	for (uclass_first_device(UCLASS_REGULATOR, &dev);
-	     dev;
-	     uclass_next_device(&dev)) {
-		ret = regulator_unset(dev);
-		if (ret == -EMEDIUMTYPE) {
-			ret = 0;
-			continue;
-		}
-		if (verbose)
-			regulator_show(dev, ret);
+	ret = regulator_set_suspend_enable(dev, uc_pdata->suspend_on);
+	if (ret == -ENOSYS)
+		ret = 0;
+
+	if (!ret && uc_pdata->suspend_on) {
+		ret = regulator_set_suspend_value(dev, uc_pdata->suspend_uV);
 		if (ret == -ENOSYS)
 			ret = 0;
+
+		if (ret)
+			return ret;
+	}
+
+	if (uc_pdata->type != REGULATOR_TYPE_FIXED) {
+		if (uc_pdata->flags & REGULATOR_FLAG_AUTOSET_UV)
+			ret = regulator_set_value(dev, uc_pdata->min_uV);
+		if (uc_pdata->init_uV > 0)
+			ret = regulator_set_value(dev, uc_pdata->init_uV);
+		if (!ret && (uc_pdata->flags & REGULATOR_FLAG_AUTOSET_UA))
+			ret = regulator_set_current(dev, uc_pdata->min_uA);
 	}
 
+	if (!uc_pdata->always_on && !uc_pdata->boot_on)
+		return 0;
+
+	if (!ret)
+		ret = regulator_set_enable(dev, true);
+
 	return ret;
 }
 
@@ -593,5 +504,6 @@ UCLASS_DRIVER(regulator) = {
 	.name		= "regulator",
 	.post_bind	= regulator_post_bind,
 	.pre_probe	= regulator_pre_probe,
+	.post_probe	= regulator_post_probe,
 	.per_device_plat_auto	= sizeof(struct dm_regulator_uclass_plat),
 };
diff --git a/include/power/regulator.h b/include/power/regulator.h
index 727776a8cf..e58e2dee16 100644
--- a/include/power/regulator.h
+++ b/include/power/regulator.h
@@ -413,104 +413,6 @@ int regulator_get_mode(struct udevice *dev);
  */
 int regulator_set_mode(struct udevice *dev, int mode_id);
 
-/**
- * regulators_enable_boot_on() - enable regulators needed for boot
- *
- * This enables all regulators which are marked to be on at boot time. This
- * only works for regulators which don't have a range for voltage/current,
- * since in that case it is not possible to know which value to use.
- *
- * This effectively calls regulator_autoset() for every regulator.
- */
-int regulators_enable_boot_on(bool verbose);
-
-/**
- * regulators_enable_boot_off() - disable regulators needed for boot
- *
- * This disables all regulators which are marked to be off at boot time.
- *
- * This effectively calls regulator_unset() for every regulator.
- */
-int regulators_enable_boot_off(bool verbose);
-
-/**
- * regulator_autoset: setup the voltage/current on a regulator
- *
- * The setup depends on constraints found in device's uclass's platform data
- * (struct dm_regulator_uclass_plat):
- *
- * - Enable - will set - if any of: 'always_on' or 'boot_on' is set to true,
- *   or if both are unset, then the function returns
- * - Voltage value - will set - if '.min_uV' and '.max_uV' values are equal
- * - Current limit - will set - if '.min_uA' and '.max_uA' values are equal
- *
- * The function returns on the first-encountered error.
- *
- * @platname - expected string for dm_regulator_uclass_plat .name field
- * @devp     - returned pointer to the regulator device - if non-NULL passed
- * @return: 0 on success or negative value of errno.
- */
-int regulator_autoset(struct udevice *dev);
-
-/**
- * regulator_unset: turn off a regulator
- *
- * The setup depends on constraints found in device's uclass's platform data
- * (struct dm_regulator_uclass_platdata):
- *
- * - Disable - will set - if  'force_off' is set to true,
- *
- * The function returns on the first-encountered error.
- */
-int regulator_unset(struct udevice *dev);
-
-/**
- * regulator_autoset_by_name: setup the regulator given by its uclass's
- * platform data name field. The setup depends on constraints found in device's
- * uclass's platform data (struct dm_regulator_uclass_plat):
- * - Enable - will set - if any of: 'always_on' or 'boot_on' is set to true,
- *   or if both are unset, then the function returns
- * - Voltage value - will set - if '.min_uV' and '.max_uV' values are equal
- * - Current limit - will set - if '.min_uA' and '.max_uA' values are equal
- *
- * The function returns on first encountered error.
- *
- * @platname - expected string for dm_regulator_uclass_plat .name field
- * @devp     - returned pointer to the regulator device - if non-NULL passed
- * @return: 0 on success or negative value of errno.
- *
- * The returned 'regulator' device can be used with:
- * - regulator_get/set_*
- */
-int regulator_autoset_by_name(const char *platname, struct udevice **devp);
-
-/**
- * regulator_list_autoset: setup the regulators given by list of their uclass's
- * platform data name field. The setup depends on constraints found in device's
- * uclass's platform data. The function loops with calls to:
- * regulator_autoset_by_name() for each name from the list.
- *
- * @list_platname - an array of expected strings for .name field of each
- *                  regulator's uclass plat
- * @list_devp     - an array of returned pointers to the successfully setup
- *                  regulator devices if non-NULL passed
- * @verbose       - (true/false) print each regulator setup info, or be quiet
- * Return: 0 on successfully setup of all list entries, otherwise first error.
- *
- * The returned 'regulator' devices can be used with:
- * - regulator_get/set_*
- *
- * Note: The list must ends with NULL entry, like in the "platname" list below:
- * char *my_regulators[] = {
- *     "VCC_3.3V",
- *     "VCC_1.8V",
- *     NULL,
- * };
- */
-int regulator_list_autoset(const char *list_platname[],
-			   struct udevice *list_devp[],
-			   bool verbose);
-
 /**
  * regulator_get_by_devname: returns the pointer to the pmic regulator device.
  * Search by name, found in regulator device's name.
@@ -628,27 +530,6 @@ static inline int regulator_set_mode(struct udevice *dev, int mode_id)
 	return -ENOSYS;
 }
 
-static inline int regulators_enable_boot_on(bool verbose)
-{
-	return -ENOSYS;
-}
-
-static inline int regulator_autoset(struct udevice *dev)
-{
-	return -ENOSYS;
-}
-
-static inline int regulator_autoset_by_name(const char *platname, struct udevice **devp)
-{
-	return -ENOSYS;
-}
-
-static inline int regulator_list_autoset(const char *list_platname[], struct udevice *list_devp[],
-					 bool verbose)
-{
-	return -ENOSYS;
-}
-
 static inline int regulator_get_by_devname(const char *devname, struct udevice **devp)
 {
 	return -ENOSYS;
-- 
2.39.2

[PATCH v2 4/7] test: dm: regulator: provide test of auto setup

[Svyatoslav Ryhel]

Replace deprecated API test with test of uclass wide autosetup.

LDO2 voltage according to dts should be 3.3v not 3.0v

Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
---
 include/power/sandbox_pmic.h |   2 +-
 test/dm/regulator.c          | 131 +++++++----------------------------
 2 files changed, 27 insertions(+), 106 deletions(-)

diff --git a/include/power/sandbox_pmic.h b/include/power/sandbox_pmic.h
index 1dbd15b523..50fc5881f8 100644
--- a/include/power/sandbox_pmic.h
+++ b/include/power/sandbox_pmic.h
@@ -137,7 +137,7 @@ enum {
 #define SANDBOX_LDO1_AUTOSET_EXPECTED_UA	100000
 #define SANDBOX_LDO1_AUTOSET_EXPECTED_ENABLE	true
 
-#define SANDBOX_LDO2_AUTOSET_EXPECTED_UV	3000000
+#define SANDBOX_LDO2_AUTOSET_EXPECTED_UV	3300000
 #define SANDBOX_LDO2_AUTOSET_EXPECTED_UA	-ENOSYS
 #define SANDBOX_LDO2_AUTOSET_EXPECTED_ENABLE	false
 
diff --git a/test/dm/regulator.c b/test/dm/regulator.c
index 9b503e4c59..eb96ddcdac 100644
--- a/test/dm/regulator.c
+++ b/test/dm/regulator.c
@@ -182,12 +182,11 @@ static
 int dm_test_power_regulator_set_enable_if_allowed(struct unit_test_state *uts)
 {
 	const char *platname;
-	struct udevice *dev, *dev_autoset;
+	struct udevice *dev;
 	bool val_set = false;
 
 	/* Get BUCK1 - always on regulator */
 	platname = regulator_names[BUCK1][PLATNAME];
-	ut_assertok(regulator_autoset_by_name(platname, &dev_autoset));
 	ut_assertok(regulator_get_by_platname(platname, &dev));
 
 	/* Try disabling always-on regulator */
@@ -196,7 +195,6 @@ int dm_test_power_regulator_set_enable_if_allowed(struct unit_test_state *uts)
 
 	/* Set the Enable of LDO1 - default is disabled */
 	platname = regulator_names[LDO1][PLATNAME];
-	ut_assertok(regulator_autoset_by_name(platname, &dev_autoset));
 	ut_assertok(regulator_get_by_platname(platname, &dev));
 
 	/* Get the Enable state of LDO1 and compare it with the requested one */
@@ -293,131 +291,54 @@ static int dm_test_power_regulator_set_get_suspend_enable(struct unit_test_state
 }
 DM_TEST(dm_test_power_regulator_set_get_suspend_enable, UT_TESTF_SCAN_FDT);
 
-/* Test regulator autoset method */
-static int dm_test_power_regulator_autoset(struct unit_test_state *uts)
+/* Test regulator setup inside uclass driver */
+static int dm_test_power_regulator_set(struct unit_test_state *uts)
 {
 	const char *platname;
-	struct udevice *dev, *dev_autoset;
+	struct udevice *dev;
 
 	/*
-	 * Test the BUCK1 with fdt properties
-	 * - min-microvolt = max-microvolt = 1200000
-	 * - min-microamp = max-microamp = 200000
-	 * - always-on = set
-	 * - boot-on = not set
-	 * Expected output state: uV=1200000; uA=200000; output enabled
+	 * LDO1 with fdt properties:
+	 * - min-microvolt = max-microvolt = 1800000
+	 * - min-microamp = max-microamp = 100000
+	 * - always-on = not set
+	 * - boot-on = set
+	 * Expected output state: uV=1800000; uA=100000; output enabled
 	 */
-	platname = regulator_names[BUCK1][PLATNAME];
-	ut_assertok(regulator_autoset_by_name(platname, &dev_autoset));
 
 	/* Check, that the returned device is proper */
+	platname = regulator_names[LDO1][PLATNAME];
 	ut_assertok(regulator_get_by_platname(platname, &dev));
-	ut_asserteq_ptr(dev, dev_autoset);
 
 	/* Check the setup after autoset */
 	ut_asserteq(regulator_get_value(dev),
-		    SANDBOX_BUCK1_AUTOSET_EXPECTED_UV);
+		    SANDBOX_LDO1_AUTOSET_EXPECTED_UV);
 	ut_asserteq(regulator_get_current(dev),
-		    SANDBOX_BUCK1_AUTOSET_EXPECTED_UA);
+		    SANDBOX_LDO1_AUTOSET_EXPECTED_UA);
 	ut_asserteq(regulator_get_enable(dev),
-		    SANDBOX_BUCK1_AUTOSET_EXPECTED_ENABLE);
-
-	return 0;
-}
-DM_TEST(dm_test_power_regulator_autoset, UT_TESTF_SCAN_FDT);
-
-/*
- * Struct setting: to keep the expected output settings.
- * @voltage: Voltage value [uV]
- * @current: Current value [uA]
- * @enable: output enable state: true/false
- */
-struct setting {
-	int voltage;
-	int current;
-	bool enable;
-};
-
-/*
- * platname_list: an array of regulator platform names.
- * For testing regulator_list_autoset() for outputs:
- * - LDO1
- * - LDO2
- */
-static const char *platname_list[] = {
-	SANDBOX_LDO1_PLATNAME,
-	SANDBOX_LDO2_PLATNAME,
-	NULL,
-};
-
-/*
- * expected_setting_list: an array of regulator output setting, expected after
- * call of the regulator_list_autoset() for the "platname_list" array.
- * For testing results of regulator_list_autoset() for outputs:
- * - LDO1
- * - LDO2
- * The settings are defined in: include/power/sandbox_pmic.h
- */
-static const struct setting expected_setting_list[] = {
-	[0] = { /* LDO1 */
-	.voltage = SANDBOX_LDO1_AUTOSET_EXPECTED_UV,
-	.current = SANDBOX_LDO1_AUTOSET_EXPECTED_UA,
-	.enable  = SANDBOX_LDO1_AUTOSET_EXPECTED_ENABLE,
-	},
-	[1] = { /* LDO2 */
-	.voltage = SANDBOX_LDO2_AUTOSET_EXPECTED_UV,
-	.current = SANDBOX_LDO2_AUTOSET_EXPECTED_UA,
-	.enable  = SANDBOX_LDO2_AUTOSET_EXPECTED_ENABLE,
-	},
-};
-
-static int list_count = ARRAY_SIZE(expected_setting_list);
-
-/* Test regulator list autoset method */
-static int dm_test_power_regulator_autoset_list(struct unit_test_state *uts)
-{
-	struct udevice *dev_list[2], *dev;
-	int i;
+		    SANDBOX_LDO1_AUTOSET_EXPECTED_ENABLE);
 
 	/*
-	 * Test the settings of the regulator list:
-	 * LDO1 with fdt properties:
-	 * - min-microvolt = max-microvolt = 1800000
-	 * - min-microamp = max-microamp = 100000
-	 * - always-on = not set
-	 * - boot-on = set
-	 * Expected output state: uV=1800000; uA=100000; output enabled
-	 *
 	 * LDO2 with fdt properties:
 	 * - min-microvolt = max-microvolt = 3300000
 	 * - always-on = not set
 	 * - boot-on = not set
-	 * Expected output state: uV=300000(default); output disabled(default)
+	 * Expected output state: uV=330000; output disabled
 	 * The expected settings are defined in: include/power/sandbox_pmic.h.
 	 */
-	ut_assertok(regulator_list_autoset(platname_list, dev_list, false));
 
-	for (i = 0; i < list_count; i++) {
-		/* Check, that the returned device is non-NULL */
-		ut_assert(dev_list[i]);
-
-		/* Check, that the returned device is proper */
-		ut_assertok(regulator_get_by_platname(platname_list[i], &dev));
-		ut_asserteq_ptr(dev_list[i], dev);
-
-		/* Check, that regulator output Voltage value is as expected */
-		ut_asserteq(regulator_get_value(dev_list[i]),
-			    expected_setting_list[i].voltage);
-
-		/* Check, that regulator output Current value is as expected */
-		ut_asserteq(regulator_get_current(dev_list[i]),
-			    expected_setting_list[i].current);
+	/* Check, that the returned device is proper */
+	platname = regulator_names[LDO2][PLATNAME];
+	ut_assertok(regulator_get_by_platname(platname, &dev));
 
-		/* Check, that regulator output Enable state is as expected */
-		ut_asserteq(regulator_get_enable(dev_list[i]),
-			    expected_setting_list[i].enable);
-	}
+	/* Check the setup after autoset */
+	ut_asserteq(regulator_get_value(dev),
+		    SANDBOX_LDO2_AUTOSET_EXPECTED_UV);
+	ut_asserteq(regulator_get_current(dev),
+		    SANDBOX_LDO2_AUTOSET_EXPECTED_UA);
+	ut_asserteq(regulator_get_enable(dev),
+		    SANDBOX_LDO2_AUTOSET_EXPECTED_ENABLE);
 
 	return 0;
 }
-DM_TEST(dm_test_power_regulator_autoset_list, UT_TESTF_SCAN_FDT);
+DM_TEST(dm_test_power_regulator_set, UT_TESTF_SCAN_FDT);
-- 
2.39.2

[PATCH v2 5/7] power: pmic-uclass: probe every child on pmic_post_probe

[Svyatoslav Ryhel]

Main goal is to probe all regulator childrens for their
proper setup but if pmic has non regulator children they
should not suffer from this either.

Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
---
 drivers/power/pmic/pmic-uclass.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/power/pmic/pmic-uclass.c b/drivers/power/pmic/pmic-uclass.c
index 0e2f5e1f41..8ca717bd5e 100644
--- a/drivers/power/pmic/pmic-uclass.c
+++ b/drivers/power/pmic/pmic-uclass.c
@@ -16,6 +16,7 @@
 #include <dm/device-internal.h>
 #include <dm/uclass-internal.h>
 #include <power/pmic.h>
+#include <power/regulator.h>
 #include <linux/ctype.h>
 
 #if CONFIG_IS_ENABLED(PMIC_CHILDREN)
@@ -198,9 +199,18 @@ static int pmic_pre_probe(struct udevice *dev)
 	return 0;
 }
 
+static int pmic_post_probe(struct udevice *dev)
+{
+	struct udevice *child;
+
+	device_foreach_child_probe(child, dev);
+	return 0;
+}
+
 UCLASS_DRIVER(pmic) = {
 	.id		= UCLASS_PMIC,
 	.name		= "pmic",
 	.pre_probe	= pmic_pre_probe,
+	.post_probe	= pmic_post_probe,
 	.per_device_auto	= sizeof(struct uc_pmic_priv),
 };
-- 
2.39.2

[PATCH v2 6/7] test: dm: pmic: provide test of child autosetup

[Svyatoslav Ryhel]

Test if regulator uclass children of PMIC are auto probed and set
after PMIC probe.

Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
---
 test/dm/pmic.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/test/dm/pmic.c b/test/dm/pmic.c
index ce671202fb..d0a3c24a54 100644
--- a/test/dm/pmic.c
+++ b/test/dm/pmic.c
@@ -18,6 +18,7 @@
 #include <dm/uclass-internal.h>
 #include <dm/util.h>
 #include <power/pmic.h>
+#include <power/regulator.h>
 #include <power/sandbox_pmic.h>
 #include <test/test.h>
 #include <test/ut.h>
@@ -129,3 +130,36 @@ static int dm_test_power_pmic_mc34708_rw_val(struct unit_test_state *uts)
 }
 
 DM_TEST(dm_test_power_pmic_mc34708_rw_val, UT_TESTF_SCAN_FDT);
+
+static int dm_test_power_pmic_child_probe(struct unit_test_state *uts)
+{
+	const char *name = "sandbox_pmic";
+	const char *devname = "ldo1";
+	struct udevice *dev;
+
+	ut_assertok(pmic_get(name, &dev));
+
+	/*
+	 * LDO1 with fdt properties:
+	 * - min-microvolt = max-microvolt = 1800000
+	 * - min-microamp = max-microamp = 100000
+	 * - always-on = not set
+	 * - boot-on = set
+	 * Expected output state: uV=1800000; uA=100000; output enabled
+	 */
+
+	/* Check, that the returned device is proper */
+	ut_assertok(regulator_get_by_devname(devname, &dev));
+
+	/* Check the setup after autoset */
+	ut_asserteq(regulator_get_value(dev),
+		    SANDBOX_LDO1_AUTOSET_EXPECTED_UV);
+	ut_asserteq(regulator_get_current(dev),
+		    SANDBOX_LDO1_AUTOSET_EXPECTED_UA);
+	ut_asserteq(regulator_get_enable(dev),
+		    SANDBOX_LDO1_AUTOSET_EXPECTED_ENABLE);
+
+	return 0;
+}
+
+DM_TEST(dm_test_power_pmic_child_probe, UT_TESTF_SCAN_FDT);
-- 
2.39.2

[PATCH v2 7/7] power: regulator-uclass: remove all deprecated API use

[Svyatoslav Ryhel]

THIS REQUIRES PRECISE TESTING!

Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
---
 arch/arm/mach-rockchip/board.c                  |  8 --------
 arch/arm/mach-rockchip/rk3399/rk3399.c          | 10 ----------
 board/Marvell/octeontx2_cn913x/board.c          |  5 -----
 board/amlogic/odroid-go-ultra/odroid-go-ultra.c |  2 --
 board/dhelectronics/dh_stm32mp1/board.c         |  2 --
 board/engicam/stm32mp1/stm32mp1.c               |  3 ---
 board/google/veyron/veyron.c                    |  6 ------
 board/samsung/common/exynos5-dt.c               |  4 ----
 board/samsung/odroid/odroid.c                   | 10 ----------
 board/st/stm32mp1/stm32mp1.c                    |  9 ---------
 drivers/video/bridge/ps862x.c                   | 12 ++++++++----
 drivers/video/rockchip/rk_vop.c                 |  6 ++----
 12 files changed, 10 insertions(+), 67 deletions(-)

diff --git a/arch/arm/mach-rockchip/board.c b/arch/arm/mach-rockchip/board.c
index 8d7b39ba15..3eaea4b04a 100644
--- a/arch/arm/mach-rockchip/board.c
+++ b/arch/arm/mach-rockchip/board.c
@@ -189,14 +189,6 @@ int board_late_init(void)
 
 int board_init(void)
 {
-	int ret;
-
-#ifdef CONFIG_DM_REGULATOR
-	ret = regulators_enable_boot_on(false);
-	if (ret)
-		debug("%s: Cannot enable boot on regulator\n", __func__);
-#endif
-
 	return 0;
 }
 
diff --git a/arch/arm/mach-rockchip/rk3399/rk3399.c b/arch/arm/mach-rockchip/rk3399/rk3399.c
index a7cc91a952..cbd2ea047d 100644
--- a/arch/arm/mach-rockchip/rk3399/rk3399.c
+++ b/arch/arm/mach-rockchip/rk3399/rk3399.c
@@ -280,15 +280,5 @@ void spl_board_init(void)
 		if (cru->glb_rst_st != 0)
 			rk3399_force_power_on_reset();
 	}
-
-	if (IS_ENABLED(CONFIG_SPL_DM_REGULATOR)) {
-		/*
-		 * Turning the eMMC and SPI back on (if disabled via the Qseven
-		 * BIOS_ENABLE) signal is done through a always-on regulator).
-		 */
-		if (regulators_enable_boot_on(false))
-			debug("%s: Cannot enable boot on regulator\n",
-			      __func__);
-	}
 }
 #endif
diff --git a/board/Marvell/octeontx2_cn913x/board.c b/board/Marvell/octeontx2_cn913x/board.c
index 3d20cfb2fa..3ffe15d42b 100644
--- a/board/Marvell/octeontx2_cn913x/board.c
+++ b/board/Marvell/octeontx2_cn913x/board.c
@@ -23,11 +23,6 @@ int board_early_init_f(void)
 
 int board_early_init_r(void)
 {
-	if (CONFIG_IS_ENABLED(DM_REGULATOR)) {
-		/* Check if any existing regulator should be turned down */
-		regulators_enable_boot_off(false);
-	}
-
 	return 0;
 }
 
diff --git a/board/amlogic/odroid-go-ultra/odroid-go-ultra.c b/board/amlogic/odroid-go-ultra/odroid-go-ultra.c
index bbd23e20fc..fa6105a071 100644
--- a/board/amlogic/odroid-go-ultra/odroid-go-ultra.c
+++ b/board/amlogic/odroid-go-ultra/odroid-go-ultra.c
@@ -16,7 +16,5 @@ int mmc_get_env_dev(void)
 
 int board_init(void)
 {
-	regulators_enable_boot_on(_DEBUG);
-
 	return 0;
 }
diff --git a/board/dhelectronics/dh_stm32mp1/board.c b/board/dhelectronics/dh_stm32mp1/board.c
index f9cfabe242..a8ad5a1620 100644
--- a/board/dhelectronics/dh_stm32mp1/board.c
+++ b/board/dhelectronics/dh_stm32mp1/board.c
@@ -615,8 +615,6 @@ static void board_init_regulator_av96(void)
 static void board_init_regulator(void)
 {
 	board_init_regulator_av96();
-
-	regulators_enable_boot_on(_DEBUG);
 }
 #else
 static inline int board_get_regulator_buck3_nvm_uv_av96(int *uv)
diff --git a/board/engicam/stm32mp1/stm32mp1.c b/board/engicam/stm32mp1/stm32mp1.c
index 5223e9bae8..c98bbdc71b 100644
--- a/board/engicam/stm32mp1/stm32mp1.c
+++ b/board/engicam/stm32mp1/stm32mp1.c
@@ -38,9 +38,6 @@ int checkboard(void)
 /* board dependent setup after realloc */
 int board_init(void)
 {
-	if (IS_ENABLED(CONFIG_DM_REGULATOR))
-		regulators_enable_boot_on(_DEBUG);
-
 	return 0;
 }
 
diff --git a/board/google/veyron/veyron.c b/board/google/veyron/veyron.c
index 32dbcdc4d1..527e9d4b0e 100644
--- a/board/google/veyron/veyron.c
+++ b/board/google/veyron/veyron.c
@@ -62,12 +62,6 @@ static int veyron_init(void)
 	if (ret)
 		return ret;
 
-	ret = regulators_enable_boot_on(false);
-	if (ret) {
-		debug("%s: Cannot enable boot on regulators\n", __func__);
-		return ret;
-	}
-
 	return 0;
 }
 #endif
diff --git a/board/samsung/common/exynos5-dt.c b/board/samsung/common/exynos5-dt.c
index cde77d79a0..45d34d838d 100644
--- a/board/samsung/common/exynos5-dt.c
+++ b/board/samsung/common/exynos5-dt.c
@@ -92,10 +92,6 @@ int exynos_power_init(void)
 	if (ret == -ENODEV)
 		return 0;
 
-	ret = regulators_enable_boot_on(false);
-	if (ret)
-		return ret;
-
 	ret = exynos_set_regulator("vdd_mif", 1100000);
 	if (ret)
 		return ret;
diff --git a/board/samsung/odroid/odroid.c b/board/samsung/odroid/odroid.c
index 39a60e4ad2..28086ee92e 100644
--- a/board/samsung/odroid/odroid.c
+++ b/board/samsung/odroid/odroid.c
@@ -431,16 +431,6 @@ int exynos_init(void)
 
 int exynos_power_init(void)
 {
-	const char *mmc_regulators[] = {
-		"VDDQ_EMMC_1.8V",
-		"VDDQ_EMMC_2.8V",
-		"TFLASH_2.8V",
-		NULL,
-	};
-
-	if (regulator_list_autoset(mmc_regulators, NULL, true))
-		pr_err("Unable to init all mmc regulators\n");
-
 	return 0;
 }
 
diff --git a/board/st/stm32mp1/stm32mp1.c b/board/st/stm32mp1/stm32mp1.c
index 3205a31c6d..7e425edefd 100644
--- a/board/st/stm32mp1/stm32mp1.c
+++ b/board/st/stm32mp1/stm32mp1.c
@@ -602,13 +602,6 @@ static int board_stm32mp15x_dk2_init(void)
 		goto error;
 	}
 
-	/* power-up audio IC */
-	regulator_autoset_by_name("v1v8_audio", NULL);
-
-	/* power-up HDMI IC */
-	regulator_autoset_by_name("v1v2_hdmi", NULL);
-	regulator_autoset_by_name("v3v3_hdmi", NULL);
-
 error:
 	return ret;
 }
@@ -665,8 +658,6 @@ int board_init(void)
 	if (board_is_stm32mp15x_dk2())
 		board_stm32mp15x_dk2_init();
 
-	regulators_enable_boot_on(_DEBUG);
-
 	/*
 	 * sysconf initialisation done only when U-Boot is running in secure
 	 * done in TF-A for TFABOOT.
diff --git a/drivers/video/bridge/ps862x.c b/drivers/video/bridge/ps862x.c
index d1d22a6e23..52a343bde2 100644
--- a/drivers/video/bridge/ps862x.c
+++ b/drivers/video/bridge/ps862x.c
@@ -77,13 +77,17 @@ static int ps8622_attach(struct udevice *dev)
 	/* set the LDO providing the 1.2V rail to the Parade bridge */
 	ret = uclass_get_device_by_phandle(UCLASS_REGULATOR, dev,
 					   "power-supply", &reg);
-	if (!ret) {
-		ret = regulator_autoset(reg);
-	} else if (ret != -ENOENT) {
-		debug("%s: Failed to enable power: ret=%d\n", __func__, ret);
+	if (ret) {
+		debug("%s: Failed to get power: ret=%d\n", __func__, ret);
 		return ret;
 	}
 
+	if (reg) {
+		ret = regulator_set_enable(reg, true);
+		if (ret)
+			return ret;
+	}
+
 	ret = video_bridge_set_active(dev, true);
 	if (ret)
 		return ret;
diff --git a/drivers/video/rockchip/rk_vop.c b/drivers/video/rockchip/rk_vop.c
index c514e2a0e4..e81eed5ffa 100644
--- a/drivers/video/rockchip/rk_vop.c
+++ b/drivers/video/rockchip/rk_vop.c
@@ -397,7 +397,7 @@ static int rk_display_init(struct udevice *dev, ulong fbbase, ofnode ep_node)
 void rk_vop_probe_regulators(struct udevice *dev,
 			     const char * const *names, int cnt)
 {
-	int i, ret;
+	int i;
 	const char *name;
 	struct udevice *reg;
 
@@ -405,9 +405,7 @@ void rk_vop_probe_regulators(struct udevice *dev,
 		name = names[i];
 		debug("%s: probing regulator '%s'\n", dev->name, name);
 
-		ret = regulator_autoset_by_name(name, &reg);
-		if (!ret)
-			ret = regulator_set_enable(reg, true);
+		regulator_set_enable(reg, true);
 	}
 }
 
-- 
2.39.2

Re: [PATCH v2 1/7] power: regulator: expand basic reference counter onto all uclass

[Simon Glass]

Hi Svyatoslav,

On Thu, 20 Jul 2023 at 06:38, Svyatoslav Ryhel <clamor95@gmail.com> wrote:
>
> Commit is based on 4fcba5d ("regulator: implement basic reference
> counter") but expands the idea to all regulators instead of just
> fixed/gpio regulators.
>
> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> ---
>  drivers/power/regulator/regulator-uclass.c | 41 ++++++++++++++++++++++
>  drivers/power/regulator/regulator_common.c | 22 ------------
>  drivers/power/regulator/regulator_common.h | 21 -----------
>  include/power/regulator.h                  |  2 ++
>  4 files changed, 43 insertions(+), 43 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

nit below

>
> diff --git a/drivers/power/regulator/regulator-uclass.c b/drivers/power/regulator/regulator-uclass.c
> index 3a6ba69f6d..fc7a4631b4 100644
> --- a/drivers/power/regulator/regulator-uclass.c
> +++ b/drivers/power/regulator/regulator-uclass.c
> @@ -159,6 +159,25 @@ int regulator_get_enable(struct udevice *dev)
>         return ops->get_enable(dev);
>  }
>
> +/*
> + * Enable or Disable a regulator
> + *
> + * This is a reentrant function and subsequent calls that enable will
> + * increase an internal counter, and disable calls will decrease the counter.
> + * The actual resource will be enabled when the counter gets to 1 coming from 0,
> + * and disabled when it reaches 0 coming from 1.
> + *
> + * @dev: regulator device
> + * @enable: bool indicating whether to enable or disable the regulator
> + * @return:
> + * 0 on Success
> + * -EBUSY if the regulator cannot be disabled because it's requested by
> + *        another device
> + * -EALREADY if the regulator has already been enabled or has already been
> + *        disabled
> + * -EACCES if there is no possibility to enable/disable the regulator
> + * -ve on different error situation
> + */
>  int regulator_set_enable(struct udevice *dev, bool enable)
>  {
>         const struct dm_regulator_ops *ops = dev_get_driver_ops(dev);
> @@ -172,6 +191,23 @@ int regulator_set_enable(struct udevice *dev, bool enable)
>         if (!enable && uc_pdata->always_on)
>                 return -EACCES;
>
> +       /* If previously enabled, increase count */
> +       if (enable && uc_pdata->enable_count > 0) {
> +               uc_pdata->enable_count++;
> +               return -EALREADY;
> +       }
> +
> +       if (!enable) {
> +               if (uc_pdata->enable_count > 1) {
> +                       /* If enabled multiple times, decrease count */
> +                       uc_pdata->enable_count--;
> +                       return -EBUSY;
> +               } else if (!uc_pdata->enable_count) {
> +                       /* If already disabled, do nothing */
> +                       return -EALREADY;
> +               }
> +       }
> +
>         if (uc_pdata->ramp_delay)
>                 old_enable = regulator_get_enable(dev);
>
> @@ -187,6 +223,11 @@ int regulator_set_enable(struct udevice *dev, bool enable)
>                 }
>         }
>
> +       if (enable)
> +               uc_pdata->enable_count++;
> +       else
> +               uc_pdata->enable_count--;
> +
>         return ret;
>  }
>
> diff --git a/drivers/power/regulator/regulator_common.c b/drivers/power/regulator/regulator_common.c
> index e26f5ebec3..d88bc6f6de 100644
> --- a/drivers/power/regulator/regulator_common.c
> +++ b/drivers/power/regulator/regulator_common.c
> @@ -72,23 +72,6 @@ int regulator_common_set_enable(const struct udevice *dev,
>                 return 0;
>         }
>
> -       /* If previously enabled, increase count */
> -       if (enable && plat->enable_count > 0) {
> -               plat->enable_count++;
> -               return -EALREADY;
> -       }
> -
> -       if (!enable) {
> -               if (plat->enable_count > 1) {
> -                       /* If enabled multiple times, decrease count */
> -                       plat->enable_count--;
> -                       return -EBUSY;
> -               } else if (!plat->enable_count) {
> -                       /* If already disabled, do nothing */
> -                       return -EALREADY;
> -               }
> -       }
> -
>         ret = dm_gpio_set_value(&plat->gpio, enable);
>         if (ret) {
>                 pr_err("Can't set regulator : %s gpio to: %d\n", dev->name,
> @@ -103,10 +86,5 @@ int regulator_common_set_enable(const struct udevice *dev,
>         if (!enable && plat->off_on_delay_us)
>                 udelay(plat->off_on_delay_us);
>
> -       if (enable)
> -               plat->enable_count++;
> -       else
> -               plat->enable_count--;
> -
>         return 0;
>  }
> diff --git a/drivers/power/regulator/regulator_common.h b/drivers/power/regulator/regulator_common.h
> index d4962899d8..15f1fa4c93 100644
> --- a/drivers/power/regulator/regulator_common.h
> +++ b/drivers/power/regulator/regulator_common.h
> @@ -13,7 +13,6 @@ struct regulator_common_plat {
>         struct gpio_desc gpio; /* GPIO for regulator enable control */
>         unsigned int startup_delay_us;
>         unsigned int off_on_delay_us;
> -       unsigned int enable_count;
>  };
>
>  int regulator_common_of_to_plat(struct udevice *dev,
> @@ -21,26 +20,6 @@ int regulator_common_of_to_plat(struct udevice *dev,
>                                 char *enable_gpio_name);
>  int regulator_common_get_enable(const struct udevice *dev,
>         struct regulator_common_plat *plat);
> -/*
> - * Enable or Disable a regulator
> - *
> - * This is a reentrant function and subsequent calls that enable will
> - * increase an internal counter, and disable calls will decrease the counter.
> - * The actual resource will be enabled when the counter gets to 1 coming from 0,
> - * and disabled when it reaches 0 coming from 1.
> - *
> - * @dev: regulator device
> - * @plat: Platform data
> - * @enable: bool indicating whether to enable or disable the regulator
> - * @return:
> - * 0 on Success
> - * -EBUSY if the regulator cannot be disabled because it's requested by
> - *        another device
> - * -EALREADY if the regulator has already been enabled or has already been
> - *        disabled
> - * -EACCES if there is no possibility to enable/disable the regulator
> - * -ve on different error situation
> - */
>  int regulator_common_set_enable(const struct udevice *dev,
>         struct regulator_common_plat *plat, bool enable);
>
> diff --git a/include/power/regulator.h b/include/power/regulator.h
> index ff1bfc2435..727776a8cf 100644
> --- a/include/power/regulator.h
> +++ b/include/power/regulator.h
> @@ -158,6 +158,7 @@ enum regulator_flag {
>   * @name**     - fdt regulator name - should be taken from the device tree
>   * ctrl_reg:   - Control register offset used to enable/disable regulator
>   * volt_reg:   - register offset for writing voltage vsel values
> + * enable_count - counter of enable calls for this regulator
>   *
>   * Note:
>   * *  - set automatically on device probe by the uclass's '.pre_probe' method.
> @@ -184,6 +185,7 @@ struct dm_regulator_uclass_plat {
>         u8 volt_reg;
>         bool suspend_on;
>         u32 suspend_uV;
> +       u32 enable_count;

Please add a comment for this

>  };
>
>  /* Regulator device operations */
> --
> 2.39.2
>

Regards,
Simon

Re: [PATCH v2 3/7] power: regulator-uclass: perform regulator setup inside uclass

[Simon Glass]

Hi Svyatoslav,

On Thu, 20 Jul 2023 at 06:38, Svyatoslav Ryhel <clamor95@gmail.com> wrote:
>
> Regulators initial setup was previously dependent on board call.
> To move from this behaviour were introduced two steps. First is
> that all individual regulators will be probed just after binding

We must not probe devices automatically when bound. The i2c interface
may not be available, etc. Do a probe afterwards.

Perhaps I am misunderstanding this, iwc please reword this commit message.

> which ensures that regulator pdata is filled and second is setting
> up regulator in post probe which enseres that correct regulator
> state according to pdata is reached.
>
> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> ---
>  drivers/power/regulator/regulator-uclass.c | 212 ++++++---------------
>  include/power/regulator.h                  | 119 ------------
>  2 files changed, 62 insertions(+), 269 deletions(-)

Regards,
SImon

Re: [PATCH v2 5/7] power: pmic-uclass: probe every child on pmic_post_probe

[Simon Glass]

Hi Svyatoslav,

On Thu, 20 Jul 2023 at 06:38, Svyatoslav Ryhel <clamor95@gmail.com> wrote:
>
> Main goal is to probe all regulator childrens for their
> proper setup but if pmic has non regulator children they
> should not suffer from this either.
>
> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> ---
>  drivers/power/pmic/pmic-uclass.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/drivers/power/pmic/pmic-uclass.c b/drivers/power/pmic/pmic-uclass.c
> index 0e2f5e1f41..8ca717bd5e 100644
> --- a/drivers/power/pmic/pmic-uclass.c
> +++ b/drivers/power/pmic/pmic-uclass.c
> @@ -16,6 +16,7 @@
>  #include <dm/device-internal.h>
>  #include <dm/uclass-internal.h>
>  #include <power/pmic.h>
> +#include <power/regulator.h>
>  #include <linux/ctype.h>

I'm not sure about this.

The idea is that power is handling automatically, e.g. a device is
probed and so its power is enabled. If you do everything at the start,
doesn't that violate the 'lazy' init side of U-Boot?

>
>  #if CONFIG_IS_ENABLED(PMIC_CHILDREN)
> @@ -198,9 +199,18 @@ static int pmic_pre_probe(struct udevice *dev)
>         return 0;
>  }
>
> +static int pmic_post_probe(struct udevice *dev)
> +{
> +       struct udevice *child;
> +
> +       device_foreach_child_probe(child, dev);
> +       return 0;
> +}
> +
>  UCLASS_DRIVER(pmic) = {
>         .id             = UCLASS_PMIC,
>         .name           = "pmic",
>         .pre_probe      = pmic_pre_probe,
> +       .post_probe     = pmic_post_probe,
>         .per_device_auto        = sizeof(struct uc_pmic_priv),
>  };
> --
> 2.39.2
>

Regards,
Simon

Re: [PATCH v2 1/7] power: regulator: expand basic reference counter onto all uclass

[Svyatoslav Ryhel]

чт, 20 лип. 2023 р. о 22:43 Simon Glass <sjg@chromium.org> пише:
>
> Hi Svyatoslav,
>
> On Thu, 20 Jul 2023 at 06:38, Svyatoslav Ryhel <clamor95@gmail.com> wrote:
> >
> > Commit is based on 4fcba5d ("regulator: implement basic reference
> > counter") but expands the idea to all regulators instead of just
> > fixed/gpio regulators.
> >
> > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> > ---
> >  drivers/power/regulator/regulator-uclass.c | 41 ++++++++++++++++++++++
> >  drivers/power/regulator/regulator_common.c | 22 ------------
> >  drivers/power/regulator/regulator_common.h | 21 -----------
> >  include/power/regulator.h                  |  2 ++
> >  4 files changed, 43 insertions(+), 43 deletions(-)
>
> Reviewed-by: Simon Glass <sjg@chromium.org>
>
> nit below
>
> >
> > diff --git a/drivers/power/regulator/regulator-uclass.c b/drivers/power/regulator/regulator-uclass.c
> > index 3a6ba69f6d..fc7a4631b4 100644
> > --- a/drivers/power/regulator/regulator-uclass.c
> > +++ b/drivers/power/regulator/regulator-uclass.c
> > @@ -159,6 +159,25 @@ int regulator_get_enable(struct udevice *dev)
> >         return ops->get_enable(dev);
> >  }
> >
> > +/*
> > + * Enable or Disable a regulator
> > + *
> > + * This is a reentrant function and subsequent calls that enable will
> > + * increase an internal counter, and disable calls will decrease the counter.
> > + * The actual resource will be enabled when the counter gets to 1 coming from 0,
> > + * and disabled when it reaches 0 coming from 1.
> > + *
> > + * @dev: regulator device
> > + * @enable: bool indicating whether to enable or disable the regulator
> > + * @return:
> > + * 0 on Success
> > + * -EBUSY if the regulator cannot be disabled because it's requested by
> > + *        another device
> > + * -EALREADY if the regulator has already been enabled or has already been
> > + *        disabled
> > + * -EACCES if there is no possibility to enable/disable the regulator
> > + * -ve on different error situation
> > + */
> >  int regulator_set_enable(struct udevice *dev, bool enable)
> >  {
> >         const struct dm_regulator_ops *ops = dev_get_driver_ops(dev);
> > @@ -172,6 +191,23 @@ int regulator_set_enable(struct udevice *dev, bool enable)
> >         if (!enable && uc_pdata->always_on)
> >                 return -EACCES;
> >
> > +       /* If previously enabled, increase count */
> > +       if (enable && uc_pdata->enable_count > 0) {
> > +               uc_pdata->enable_count++;
> > +               return -EALREADY;
> > +       }
> > +
> > +       if (!enable) {
> > +               if (uc_pdata->enable_count > 1) {
> > +                       /* If enabled multiple times, decrease count */
> > +                       uc_pdata->enable_count--;
> > +                       return -EBUSY;
> > +               } else if (!uc_pdata->enable_count) {
> > +                       /* If already disabled, do nothing */
> > +                       return -EALREADY;
> > +               }
> > +       }
> > +
> >         if (uc_pdata->ramp_delay)
> >                 old_enable = regulator_get_enable(dev);
> >
> > @@ -187,6 +223,11 @@ int regulator_set_enable(struct udevice *dev, bool enable)
> >                 }
> >         }
> >
> > +       if (enable)
> > +               uc_pdata->enable_count++;
> > +       else
> > +               uc_pdata->enable_count--;
> > +
> >         return ret;
> >  }
> >
> > diff --git a/drivers/power/regulator/regulator_common.c b/drivers/power/regulator/regulator_common.c
> > index e26f5ebec3..d88bc6f6de 100644
> > --- a/drivers/power/regulator/regulator_common.c
> > +++ b/drivers/power/regulator/regulator_common.c
> > @@ -72,23 +72,6 @@ int regulator_common_set_enable(const struct udevice *dev,
> >                 return 0;
> >         }
> >
> > -       /* If previously enabled, increase count */
> > -       if (enable && plat->enable_count > 0) {
> > -               plat->enable_count++;
> > -               return -EALREADY;
> > -       }
> > -
> > -       if (!enable) {
> > -               if (plat->enable_count > 1) {
> > -                       /* If enabled multiple times, decrease count */
> > -                       plat->enable_count--;
> > -                       return -EBUSY;
> > -               } else if (!plat->enable_count) {
> > -                       /* If already disabled, do nothing */
> > -                       return -EALREADY;
> > -               }
> > -       }
> > -
> >         ret = dm_gpio_set_value(&plat->gpio, enable);
> >         if (ret) {
> >                 pr_err("Can't set regulator : %s gpio to: %d\n", dev->name,
> > @@ -103,10 +86,5 @@ int regulator_common_set_enable(const struct udevice *dev,
> >         if (!enable && plat->off_on_delay_us)
> >                 udelay(plat->off_on_delay_us);
> >
> > -       if (enable)
> > -               plat->enable_count++;
> > -       else
> > -               plat->enable_count--;
> > -
> >         return 0;
> >  }
> > diff --git a/drivers/power/regulator/regulator_common.h b/drivers/power/regulator/regulator_common.h
> > index d4962899d8..15f1fa4c93 100644
> > --- a/drivers/power/regulator/regulator_common.h
> > +++ b/drivers/power/regulator/regulator_common.h
> > @@ -13,7 +13,6 @@ struct regulator_common_plat {
> >         struct gpio_desc gpio; /* GPIO for regulator enable control */
> >         unsigned int startup_delay_us;
> >         unsigned int off_on_delay_us;
> > -       unsigned int enable_count;
> >  };
> >
> >  int regulator_common_of_to_plat(struct udevice *dev,
> > @@ -21,26 +20,6 @@ int regulator_common_of_to_plat(struct udevice *dev,
> >                                 char *enable_gpio_name);
> >  int regulator_common_get_enable(const struct udevice *dev,
> >         struct regulator_common_plat *plat);
> > -/*
> > - * Enable or Disable a regulator
> > - *
> > - * This is a reentrant function and subsequent calls that enable will
> > - * increase an internal counter, and disable calls will decrease the counter.
> > - * The actual resource will be enabled when the counter gets to 1 coming from 0,
> > - * and disabled when it reaches 0 coming from 1.
> > - *
> > - * @dev: regulator device
> > - * @plat: Platform data
> > - * @enable: bool indicating whether to enable or disable the regulator
> > - * @return:
> > - * 0 on Success
> > - * -EBUSY if the regulator cannot be disabled because it's requested by
> > - *        another device
> > - * -EALREADY if the regulator has already been enabled or has already been
> > - *        disabled
> > - * -EACCES if there is no possibility to enable/disable the regulator
> > - * -ve on different error situation
> > - */
> >  int regulator_common_set_enable(const struct udevice *dev,
> >         struct regulator_common_plat *plat, bool enable);
> >
> > diff --git a/include/power/regulator.h b/include/power/regulator.h
> > index ff1bfc2435..727776a8cf 100644
> > --- a/include/power/regulator.h
> > +++ b/include/power/regulator.h
> > @@ -158,6 +158,7 @@ enum regulator_flag {
> >   * @name**     - fdt regulator name - should be taken from the device tree
> >   * ctrl_reg:   - Control register offset used to enable/disable regulator
> >   * volt_reg:   - register offset for writing voltage vsel values
> > + * enable_count - counter of enable calls for this regulator
> >   *
> >   * Note:
> >   * *  - set automatically on device probe by the uclass's '.pre_probe' method.
> > @@ -184,6 +185,7 @@ struct dm_regulator_uclass_plat {
> >         u8 volt_reg;
> >         bool suspend_on;
> >         u32 suspend_uV;
> > +       u32 enable_count;
>
> Please add a comment for this
>

Description included in struct description here:
+ * enable_count - counter of enable calls for this regulator

> >  };
> >
> >  /* Regulator device operations */
> > --
> > 2.39.2
> >
>
> Regards,
> Simon

Re: [PATCH v2 3/7] power: regulator-uclass: perform regulator setup inside uclass

[Svyatoslav Ryhel]

чт, 20 лип. 2023 р. о 22:43 Simon Glass <sjg@chromium.org> пише:
>
> Hi Svyatoslav,
>
> On Thu, 20 Jul 2023 at 06:38, Svyatoslav Ryhel <clamor95@gmail.com> wrote:
> >
> > Regulators initial setup was previously dependent on board call.
> > To move from this behaviour were introduced two steps. First is
> > that all individual regulators will be probed just after binding
>
> We must not probe devices automatically when bound. The i2c interface
> may not be available, etc. Do a probe afterwards.
>
> Perhaps I am misunderstanding this, iwc please reword this commit message.

After bound. If the regulator is a self-sufficient i2c device then it
will be bound
after i2c is available by code design so i2c interface should be
available at that
moment. At least led and gpio uclasses perform this for initial setup
of devices.

Platform regulators (aka fixed/gpio regulators) work perfectly fine. I
have no i2c
regulators to test deeply.

As for now only one uclass is not compatible with this system - PMIC which has
strong dependency between regulator and main mfd. This is why probing after
bind for pmic regulators is disabled and pmic regulators are probed on pmic core
post_probe.

> > which ensures that regulator pdata is filled and second is setting
> > up regulator in post probe which enseres that correct regulator
> > state according to pdata is reached.
> >
> > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> > ---
> >  drivers/power/regulator/regulator-uclass.c | 212 ++++++---------------
> >  include/power/regulator.h                  | 119 ------------
> >  2 files changed, 62 insertions(+), 269 deletions(-)
>
> Regards,
> SImon

Re: [PATCH v2 5/7] power: pmic-uclass: probe every child on pmic_post_probe

[Svyatoslav Ryhel]

чт, 20 лип. 2023 р. о 22:43 Simon Glass <sjg@chromium.org> пише:
>
> Hi Svyatoslav,
>
> On Thu, 20 Jul 2023 at 06:38, Svyatoslav Ryhel <clamor95@gmail.com> wrote:
> >
> > Main goal is to probe all regulator childrens for their
> > proper setup but if pmic has non regulator children they
> > should not suffer from this either.
> >
> > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> > ---
> >  drivers/power/pmic/pmic-uclass.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> >
> > diff --git a/drivers/power/pmic/pmic-uclass.c b/drivers/power/pmic/pmic-uclass.c
> > index 0e2f5e1f41..8ca717bd5e 100644
> > --- a/drivers/power/pmic/pmic-uclass.c
> > +++ b/drivers/power/pmic/pmic-uclass.c
> > @@ -16,6 +16,7 @@
> >  #include <dm/device-internal.h>
> >  #include <dm/uclass-internal.h>
> >  #include <power/pmic.h>
> > +#include <power/regulator.h>
> >  #include <linux/ctype.h>
>
> I'm not sure about this.
>
> The idea is that power is handling automatically, e.g. a device is
> probed and so its power is enabled. If you do everything at the start,

Why do you think probe == power on? As for now in u-boot pmic childrens are
mostly regulators with few minor exceptions. Unlike any other devices
regulators have special properties like boot-on or always-on and are
required by other devices to operate correctly.

Before this patch special properties were either neglected or their
establishment was performed with a board call, both ways are bad
solutions. With this patch all pmic regulators are probed and set
according to their dts properties (yes, if the regulator is not in use
it will be turned off so no power consumption issue there) which is
desired behavior.

> doesn't that violate the 'lazy' init side of U-Boot?
>

That 'lazy' init resulted in more issues for me (half working devices,
broken usb) then if it was handled actively. I am not against this
approach, but pmic and regulators is not the device which should
embrace it.

> >
> >  #if CONFIG_IS_ENABLED(PMIC_CHILDREN)
> > @@ -198,9 +199,18 @@ static int pmic_pre_probe(struct udevice *dev)
> >         return 0;
> >  }
> >
> > +static int pmic_post_probe(struct udevice *dev)
> > +{
> > +       struct udevice *child;
> > +
> > +       device_foreach_child_probe(child, dev);
> > +       return 0;
> > +}
> > +
> >  UCLASS_DRIVER(pmic) = {
> >         .id             = UCLASS_PMIC,
> >         .name           = "pmic",
> >         .pre_probe      = pmic_pre_probe,
> > +       .post_probe     = pmic_post_probe,
> >         .per_device_auto        = sizeof(struct uc_pmic_priv),
> >  };
> > --
> > 2.39.2
> >
>
> Regards,
> Simon

Re: [PATCH v2 1/7] power: regulator: expand basic reference counter onto all uclass

[Simon Glass]

Hi Svyatoslav,

On Thu, 20 Jul 2023 at 23:23, Svyatoslav Ryhel <clamor95@gmail.com> wrote:
>
> чт, 20 лип. 2023 р. о 22:43 Simon Glass <sjg@chromium.org> пише:
> >
> > Hi Svyatoslav,
> >
> > On Thu, 20 Jul 2023 at 06:38, Svyatoslav Ryhel <clamor95@gmail.com> wrote:
> > >
> > > Commit is based on 4fcba5d ("regulator: implement basic reference
> > > counter") but expands the idea to all regulators instead of just
> > > fixed/gpio regulators.
> > >
> > > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> > > ---
> > >  drivers/power/regulator/regulator-uclass.c | 41 ++++++++++++++++++++++
> > >  drivers/power/regulator/regulator_common.c | 22 ------------
> > >  drivers/power/regulator/regulator_common.h | 21 -----------
> > >  include/power/regulator.h                  |  2 ++
> > >  4 files changed, 43 insertions(+), 43 deletions(-)
> >
> > Reviewed-by: Simon Glass <sjg@chromium.org>
> >
> > nit below
> >
> > >
> > > diff --git a/drivers/power/regulator/regulator-uclass.c b/drivers/power/regulator/regulator-uclass.c
> > > index 3a6ba69f6d..fc7a4631b4 100644
> > > --- a/drivers/power/regulator/regulator-uclass.c
> > > +++ b/drivers/power/regulator/regulator-uclass.c
> > > @@ -159,6 +159,25 @@ int regulator_get_enable(struct udevice *dev)
> > >         return ops->get_enable(dev);
> > >  }
> > >
> > > +/*
> > > + * Enable or Disable a regulator
> > > + *
> > > + * This is a reentrant function and subsequent calls that enable will
> > > + * increase an internal counter, and disable calls will decrease the counter.
> > > + * The actual resource will be enabled when the counter gets to 1 coming from 0,
> > > + * and disabled when it reaches 0 coming from 1.
> > > + *
> > > + * @dev: regulator device
> > > + * @enable: bool indicating whether to enable or disable the regulator
> > > + * @return:
> > > + * 0 on Success
> > > + * -EBUSY if the regulator cannot be disabled because it's requested by
> > > + *        another device
> > > + * -EALREADY if the regulator has already been enabled or has already been
> > > + *        disabled
> > > + * -EACCES if there is no possibility to enable/disable the regulator
> > > + * -ve on different error situation
> > > + */
> > >  int regulator_set_enable(struct udevice *dev, bool enable)
> > >  {
> > >         const struct dm_regulator_ops *ops = dev_get_driver_ops(dev);
> > > @@ -172,6 +191,23 @@ int regulator_set_enable(struct udevice *dev, bool enable)
> > >         if (!enable && uc_pdata->always_on)
> > >                 return -EACCES;
> > >
> > > +       /* If previously enabled, increase count */
> > > +       if (enable && uc_pdata->enable_count > 0) {
> > > +               uc_pdata->enable_count++;
> > > +               return -EALREADY;
> > > +       }
> > > +
> > > +       if (!enable) {
> > > +               if (uc_pdata->enable_count > 1) {
> > > +                       /* If enabled multiple times, decrease count */
> > > +                       uc_pdata->enable_count--;
> > > +                       return -EBUSY;
> > > +               } else if (!uc_pdata->enable_count) {
> > > +                       /* If already disabled, do nothing */
> > > +                       return -EALREADY;
> > > +               }
> > > +       }
> > > +
> > >         if (uc_pdata->ramp_delay)
> > >                 old_enable = regulator_get_enable(dev);
> > >
> > > @@ -187,6 +223,11 @@ int regulator_set_enable(struct udevice *dev, bool enable)
> > >                 }
> > >         }
> > >
> > > +       if (enable)
> > > +               uc_pdata->enable_count++;
> > > +       else
> > > +               uc_pdata->enable_count--;
> > > +
> > >         return ret;
> > >  }
> > >
> > > diff --git a/drivers/power/regulator/regulator_common.c b/drivers/power/regulator/regulator_common.c
> > > index e26f5ebec3..d88bc6f6de 100644
> > > --- a/drivers/power/regulator/regulator_common.c
> > > +++ b/drivers/power/regulator/regulator_common.c
> > > @@ -72,23 +72,6 @@ int regulator_common_set_enable(const struct udevice *dev,
> > >                 return 0;
> > >         }
> > >
> > > -       /* If previously enabled, increase count */
> > > -       if (enable && plat->enable_count > 0) {
> > > -               plat->enable_count++;
> > > -               return -EALREADY;
> > > -       }
> > > -
> > > -       if (!enable) {
> > > -               if (plat->enable_count > 1) {
> > > -                       /* If enabled multiple times, decrease count */
> > > -                       plat->enable_count--;
> > > -                       return -EBUSY;
> > > -               } else if (!plat->enable_count) {
> > > -                       /* If already disabled, do nothing */
> > > -                       return -EALREADY;
> > > -               }
> > > -       }
> > > -
> > >         ret = dm_gpio_set_value(&plat->gpio, enable);
> > >         if (ret) {
> > >                 pr_err("Can't set regulator : %s gpio to: %d\n", dev->name,
> > > @@ -103,10 +86,5 @@ int regulator_common_set_enable(const struct udevice *dev,
> > >         if (!enable && plat->off_on_delay_us)
> > >                 udelay(plat->off_on_delay_us);
> > >
> > > -       if (enable)
> > > -               plat->enable_count++;
> > > -       else
> > > -               plat->enable_count--;
> > > -
> > >         return 0;
> > >  }
> > > diff --git a/drivers/power/regulator/regulator_common.h b/drivers/power/regulator/regulator_common.h
> > > index d4962899d8..15f1fa4c93 100644
> > > --- a/drivers/power/regulator/regulator_common.h
> > > +++ b/drivers/power/regulator/regulator_common.h
> > > @@ -13,7 +13,6 @@ struct regulator_common_plat {
> > >         struct gpio_desc gpio; /* GPIO for regulator enable control */
> > >         unsigned int startup_delay_us;
> > >         unsigned int off_on_delay_us;
> > > -       unsigned int enable_count;
> > >  };
> > >
> > >  int regulator_common_of_to_plat(struct udevice *dev,
> > > @@ -21,26 +20,6 @@ int regulator_common_of_to_plat(struct udevice *dev,
> > >                                 char *enable_gpio_name);
> > >  int regulator_common_get_enable(const struct udevice *dev,
> > >         struct regulator_common_plat *plat);
> > > -/*
> > > - * Enable or Disable a regulator
> > > - *
> > > - * This is a reentrant function and subsequent calls that enable will
> > > - * increase an internal counter, and disable calls will decrease the counter.
> > > - * The actual resource will be enabled when the counter gets to 1 coming from 0,
> > > - * and disabled when it reaches 0 coming from 1.
> > > - *
> > > - * @dev: regulator device
> > > - * @plat: Platform data
> > > - * @enable: bool indicating whether to enable or disable the regulator
> > > - * @return:
> > > - * 0 on Success
> > > - * -EBUSY if the regulator cannot be disabled because it's requested by
> > > - *        another device
> > > - * -EALREADY if the regulator has already been enabled or has already been
> > > - *        disabled
> > > - * -EACCES if there is no possibility to enable/disable the regulator
> > > - * -ve on different error situation
> > > - */
> > >  int regulator_common_set_enable(const struct udevice *dev,
> > >         struct regulator_common_plat *plat, bool enable);
> > >
> > > diff --git a/include/power/regulator.h b/include/power/regulator.h
> > > index ff1bfc2435..727776a8cf 100644
> > > --- a/include/power/regulator.h
> > > +++ b/include/power/regulator.h
> > > @@ -158,6 +158,7 @@ enum regulator_flag {
> > >   * @name**     - fdt regulator name - should be taken from the device tree
> > >   * ctrl_reg:   - Control register offset used to enable/disable regulator
> > >   * volt_reg:   - register offset for writing voltage vsel values
> > > + * enable_count - counter of enable calls for this regulator
> > >   *
> > >   * Note:
> > >   * *  - set automatically on device probe by the uclass's '.pre_probe' method.
> > > @@ -184,6 +185,7 @@ struct dm_regulator_uclass_plat {
> > >         u8 volt_reg;
> > >         bool suspend_on;
> > >         u32 suspend_uV;
> > > +       u32 enable_count;
> >
> > Please add a comment for this
> >
>
> Description included in struct description here:
> + * enable_count - counter of enable calls for this regulator
>

OK thanks, I wonder why it says it is part of 'enum regulator_flag' ?

Regards,
Simon

Re: [PATCH v2 3/7] power: regulator-uclass: perform regulator setup inside uclass

[Jonas Karlman]

Hi,

On 2023-07-21 07:34, Svyatoslav Ryhel wrote:
> чт, 20 лип. 2023 р. о 22:43 Simon Glass <sjg@chromium.org> пише:
>>
>> Hi Svyatoslav,
>>
>> On Thu, 20 Jul 2023 at 06:38, Svyatoslav Ryhel <clamor95@gmail.com> wrote:
>>>
>>> Regulators initial setup was previously dependent on board call.
>>> To move from this behaviour were introduced two steps. First is
>>> that all individual regulators will be probed just after binding
>>
>> We must not probe devices automatically when bound. The i2c interface
>> may not be available, etc. Do a probe afterwards.
>>
>> Perhaps I am misunderstanding this, iwc please reword this commit message.
> 
> After bound. If the regulator is a self-sufficient i2c device then it
> will be bound
> after i2c is available by code design so i2c interface should be
> available at that
> moment. At least led and gpio uclasses perform this for initial setup
> of devices.
> 
> Platform regulators (aka fixed/gpio regulators) work perfectly fine. I
> have no i2c
> regulators to test deeply.
> 
> As for now only one uclass is not compatible with this system - PMIC which has
> strong dependency between regulator and main mfd. This is why probing after
> bind for pmic regulators is disabled and pmic regulators are probed on pmic core
> post_probe.

This sounds more like a possible bug of some dependency not being
probed in correct order or not leaving the device in a ready state
after probe.

If pmic regulators are bind in pmic bind with the pmic as parent, then
at regulator probe the driver core ensure that pmic has probed before
regulator use.

This works perfect fine with the RK8xx I2C PMIC and its regulators.
Wich a call to device_get_supply_regulator(dev, "test-supply", &reg)
probe happens in i2c, pmic and regulator order.

> 
>>> which ensures that regulator pdata is filled and second is setting
>>> up regulator in post probe which enseres that correct regulator
>>> state according to pdata is reached.

I think that the approach in this patch is trying to change too much all
at once.

Have made some adjustments that I think should help with transition.
- Added a flag to protect regulator_autoset from being called more then
  once for each regulator, in a separate patch.
- Changed to only probe-after-bind on regulators marked as
  always-on/boot-on.

And it works something like this:

static int regulator_post_bind(struct udevice *dev)
{
	[...]

	if (uc_pdata->always_on || uc_pdata->boot_on)
		dev_or_flags(dev, DM_FLAG_PROBE_AFTER_BIND);
}

static int regulator_post_probe(struct udevice *dev)
{
	ret = regulator_autoset(dev);
}

With that any always-on/boot-on regulator would automatically probe and
autoset after bind, remaining would only probe and autoset if they are
used.

This approach should also prevent having to change existing code that
may call autoset, and cleanup can be done in follow-up patches/series.

Probe-after-bind and call to autoset could also be protected with a new
Kconfig to help with transition.

See following for a working example using a new driver that depends
on that regulators have been autoset prior to regulator_get_value.
https://github.com/Kwiboo/u-boot-rockchip/compare/master...rk3568-io-domain

Or the two commits separate:

power: regulator: Only run autoset once for each regulator
https://github.com/Kwiboo/u-boot-rockchip/commit/05db4dbcb8f908b417ed5cae8a7a325c82112d75

power: regulator: Perform regulator setup inside uclass
https://github.com/Kwiboo/u-boot-rockchip/commit/489bd5d2c1a2a33824eac4f2d54399ef5dff4fdf

Regards,
Jonas

>>>
>>> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
>>> ---
>>>  drivers/power/regulator/regulator-uclass.c | 212 ++++++---------------
>>>  include/power/regulator.h                  | 119 ------------
>>>  2 files changed, 62 insertions(+), 269 deletions(-)
>>
>> Regards,
>> SImon

Re: [PATCH v2 3/7] power: regulator-uclass: perform regulator setup inside uclass

[Svyatoslav Ryhel]

5 серпня 2023 р. 15:49:32 GMT+03:00, Jonas Karlman <jonas@kwiboo.se> написав(-ла):
>Hi,
>
>On 2023-07-21 07:34, Svyatoslav Ryhel wrote:
>> чт, 20 лип. 2023 р. о 22:43 Simon Glass <sjg@chromium.org> пише:
>>>
>>> Hi Svyatoslav,
>>>
>>> On Thu, 20 Jul 2023 at 06:38, Svyatoslav Ryhel <clamor95@gmail.com> wrote:
>>>>
>>>> Regulators initial setup was previously dependent on board call.
>>>> To move from this behaviour were introduced two steps. First is
>>>> that all individual regulators will be probed just after binding
>>>
>>> We must not probe devices automatically when bound. The i2c interface
>>> may not be available, etc. Do a probe afterwards.
>>>
>>> Perhaps I am misunderstanding this, iwc please reword this commit message.
>> 
>> After bound. If the regulator is a self-sufficient i2c device then it
>> will be bound
>> after i2c is available by code design so i2c interface should be
>> available at that
>> moment. At least led and gpio uclasses perform this for initial setup
>> of devices.
>> 
>> Platform regulators (aka fixed/gpio regulators) work perfectly fine. I
>> have no i2c
>> regulators to test deeply.
>> 
>> As for now only one uclass is not compatible with this system - PMIC which has
>> strong dependency between regulator and main mfd. This is why probing after
>> bind for pmic regulators is disabled and pmic regulators are probed on pmic core
>> post_probe.
>
>This sounds more like a possible bug of some dependency not being
>probed in correct order or not leaving the device in a ready state
>after probe.
>
>If pmic regulators are bind in pmic bind with the pmic as parent, then
>at regulator probe the driver core ensure that pmic has probed before
>regulator use.
>
>This works perfect fine with the RK8xx I2C PMIC and its regulators.
>Wich a call to device_get_supply_regulator(dev, "test-supply", &reg)
>probe happens in i2c, pmic and regulator order.
>

I will check and re-test drivers I have ASAP. 

>> 
>>>> which ensures that regulator pdata is filled and second is setting
>>>> up regulator in post probe which enseres that correct regulator
>>>> state according to pdata is reached.
>
>I think that the approach in this patch is trying to change too much all
>at once.
>
>Have made some adjustments that I think should help with transition.
>- Added a flag to protect regulator_autoset from being called more then
>  once for each regulator, in a separate patch.

It is not a good decision in the long run and you should keep in mind that there is nothing more constant than a temporary solution.

>- Changed to only probe-after-bind on regulators marked as
>  always-on/boot-on.
>
>And it works something like this:
>
>static int regulator_post_bind(struct udevice *dev)
>{
>	[...]
>
>	if (uc_pdata->always_on || uc_pdata->boot_on)
>		dev_or_flags(dev, DM_FLAG_PROBE_AFTER_BIND);
>}
>
>static int regulator_post_probe(struct udevice *dev)
>{
>	ret = regulator_autoset(dev);
>}
>
>With that any always-on/boot-on regulator would automatically probe and
>autoset after bind, remaining would only probe and autoset if they are
>used.

uc_pdata is filled in pre_probe, while you are runing check in post_bind.

>This approach should also prevent having to change existing code that
>may call autoset, and cleanup can be done in follow-up patches/series.
>
>Probe-after-bind and call to autoset could also be protected with a new
>Kconfig to help with transition.
>
>See following for a working example using a new driver that depends
>on that regulators have been autoset prior to regulator_get_value.
>https://github.com/Kwiboo/u-boot-rockchip/compare/master...rk3568-io-domain
>
>Or the two commits separate:
>
>power: regulator: Only run autoset once for each regulator
>https://github.com/Kwiboo/u-boot-rockchip/commit/05db4dbcb8f908b417ed5cae8a7a325c82112d75
>
>power: regulator: Perform regulator setup inside uclass
>https://github.com/Kwiboo/u-boot-rockchip/commit/489bd5d2c1a2a33824eac4f2d54399ef5dff4fdf
>
>Regards,
>Jonas
>
>>>>
>>>> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
>>>> ---
>>>>  drivers/power/regulator/regulator-uclass.c | 212 ++++++---------------
>>>>  include/power/regulator.h                  | 119 ------------
>>>>  2 files changed, 62 insertions(+), 269 deletions(-)
>>>
>>> Regards,
>>> SImon
>

Re: [PATCH v2 3/7] power: regulator-uclass: perform regulator setup inside uclass

[Jonas Karlman]

On 2023-08-05 21:58, Svyatoslav Ryhel wrote:
> 
> 
> 5 серпня 2023 р. 15:49:32 GMT+03:00, Jonas Karlman <jonas@kwiboo.se> написав(-ла):
>> Hi,
>>
>> On 2023-07-21 07:34, Svyatoslav Ryhel wrote:
>>> чт, 20 лип. 2023 р. о 22:43 Simon Glass <sjg@chromium.org> пише:
>>>>
>>>> Hi Svyatoslav,
>>>>
>>>> On Thu, 20 Jul 2023 at 06:38, Svyatoslav Ryhel <clamor95@gmail.com> wrote:
>>>>>
>>>>> Regulators initial setup was previously dependent on board call.
>>>>> To move from this behaviour were introduced two steps. First is
>>>>> that all individual regulators will be probed just after binding
>>>>
>>>> We must not probe devices automatically when bound. The i2c interface
>>>> may not be available, etc. Do a probe afterwards.
>>>>
>>>> Perhaps I am misunderstanding this, iwc please reword this commit message.
>>>
>>> After bound. If the regulator is a self-sufficient i2c device then it
>>> will be bound
>>> after i2c is available by code design so i2c interface should be
>>> available at that
>>> moment. At least led and gpio uclasses perform this for initial setup
>>> of devices.
>>>
>>> Platform regulators (aka fixed/gpio regulators) work perfectly fine. I
>>> have no i2c
>>> regulators to test deeply.
>>>
>>> As for now only one uclass is not compatible with this system - PMIC which has
>>> strong dependency between regulator and main mfd. This is why probing after
>>> bind for pmic regulators is disabled and pmic regulators are probed on pmic core
>>> post_probe.
>>
>> This sounds more like a possible bug of some dependency not being
>> probed in correct order or not leaving the device in a ready state
>> after probe.
>>
>> If pmic regulators are bind in pmic bind with the pmic as parent, then
>> at regulator probe the driver core ensure that pmic has probed before
>> regulator use.
>>
>> This works perfect fine with the RK8xx I2C PMIC and its regulators.
>> Wich a call to device_get_supply_regulator(dev, "test-supply", &reg)
>> probe happens in i2c, pmic and regulator order.
>>
> 
> I will check and re-test drivers I have ASAP. 
> 
>>>
>>>>> which ensures that regulator pdata is filled and second is setting
>>>>> up regulator in post probe which enseres that correct regulator
>>>>> state according to pdata is reached.
>>
>> I think that the approach in this patch is trying to change too much all
>> at once.
>>
>> Have made some adjustments that I think should help with transition.
>> - Added a flag to protect regulator_autoset from being called more then
>>  once for each regulator, in a separate patch.
> 
> It is not a good decision in the long run and you should keep in mind that there is nothing more constant than a temporary solution.

Nor do I propose this to be a long-term solution, only that it is more
reviewable to see changes in non-breaking smaller steps. In current
state this patch breaks building U-Boot and requires the last patch to
fix build again.

Anyway, I will be posting a similar patch for autoset as linked below as
part of a series to add a Rockchip IO-domain driver shortly. In current
state autoset cannot safely be called multiple times, and such patch
should not have an impact on the direction of this series.

> 
>> - Changed to only probe-after-bind on regulators marked as
>>  always-on/boot-on.
>>
>> And it works something like this:
>>
>> static int regulator_post_bind(struct udevice *dev)
>> {
>> 	[...]
>>
>> 	if (uc_pdata->always_on || uc_pdata->boot_on)
>> 		dev_or_flags(dev, DM_FLAG_PROBE_AFTER_BIND);
>> }
>>
>> static int regulator_post_probe(struct udevice *dev)
>> {
>> 	ret = regulator_autoset(dev);
>> }
>>
>> With that any always-on/boot-on regulator would automatically probe and
>> autoset after bind, remaining would only probe and autoset if they are
>> used.
> 
> uc_pdata is filled in pre_probe, while you are runing check in post_bind.

Please look closer at the code and not the snippet above, they are
filled in post_bind or the code would not have worked :-)

Regards,
Jonas

> 
>> This approach should also prevent having to change existing code that
>> may call autoset, and cleanup can be done in follow-up patches/series.
>>
>> Probe-after-bind and call to autoset could also be protected with a new
>> Kconfig to help with transition.
>>
>> See following for a working example using a new driver that depends
>> on that regulators have been autoset prior to regulator_get_value.
>> https://github.com/Kwiboo/u-boot-rockchip/compare/master...rk3568-io-domain
>>
>> Or the two commits separate:
>>
>> power: regulator: Only run autoset once for each regulator
>> https://github.com/Kwiboo/u-boot-rockchip/commit/05db4dbcb8f908b417ed5cae8a7a325c82112d75
>>
>> power: regulator: Perform regulator setup inside uclass
>> https://github.com/Kwiboo/u-boot-rockchip/commit/489bd5d2c1a2a33824eac4f2d54399ef5dff4fdf
>>
>> Regards,
>> Jonas
>>
>>>>>
>>>>> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
>>>>> ---
>>>>>  drivers/power/regulator/regulator-uclass.c | 212 ++++++---------------
>>>>>  include/power/regulator.h                  | 119 ------------
>>>>>  2 files changed, 62 insertions(+), 269 deletions(-)
>>>>
>>>> Regards,
>>>> SImon
>>

Re: [PATCH v2 3/7] power: regulator-uclass: perform regulator setup inside uclass

[Svyatoslav Ryhel]

5 серпня 2023 р. 23:46:27 GMT+03:00, Jonas Karlman <jonas@kwiboo.se> написав(-ла):
>On 2023-08-05 21:58, Svyatoslav Ryhel wrote:
>> 
>> 
>> 5 серпня 2023 р. 15:49:32 GMT+03:00, Jonas Karlman <jonas@kwiboo.se> написав(-ла):
>>> Hi,
>>>
>>> On 2023-07-21 07:34, Svyatoslav Ryhel wrote:
>>>> чт, 20 лип. 2023 р. о 22:43 Simon Glass <sjg@chromium.org> пише:
>>>>>
>>>>> Hi Svyatoslav,
>>>>>
>>>>> On Thu, 20 Jul 2023 at 06:38, Svyatoslav Ryhel <clamor95@gmail.com> wrote:
>>>>>>
>>>>>> Regulators initial setup was previously dependent on board call.
>>>>>> To move from this behaviour were introduced two steps. First is
>>>>>> that all individual regulators will be probed just after binding
>>>>>
>>>>> We must not probe devices automatically when bound. The i2c interface
>>>>> may not be available, etc. Do a probe afterwards.
>>>>>
>>>>> Perhaps I am misunderstanding this, iwc please reword this commit message.
>>>>
>>>> After bound. If the regulator is a self-sufficient i2c device then it
>>>> will be bound
>>>> after i2c is available by code design so i2c interface should be
>>>> available at that
>>>> moment. At least led and gpio uclasses perform this for initial setup
>>>> of devices.
>>>>
>>>> Platform regulators (aka fixed/gpio regulators) work perfectly fine. I
>>>> have no i2c
>>>> regulators to test deeply.
>>>>
>>>> As for now only one uclass is not compatible with this system - PMIC which has
>>>> strong dependency between regulator and main mfd. This is why probing after
>>>> bind for pmic regulators is disabled and pmic regulators are probed on pmic core
>>>> post_probe.
>>>
>>> This sounds more like a possible bug of some dependency not being
>>> probed in correct order or not leaving the device in a ready state
>>> after probe.
>>>
>>> If pmic regulators are bind in pmic bind with the pmic as parent, then
>>> at regulator probe the driver core ensure that pmic has probed before
>>> regulator use.
>>>
>>> This works perfect fine with the RK8xx I2C PMIC and its regulators.
>>> Wich a call to device_get_supply_regulator(dev, "test-supply", &reg)
>>> probe happens in i2c, pmic and regulator order.
>>>
>> 
>> I will check and re-test drivers I have ASAP. 
>> 
>>>>
>>>>>> which ensures that regulator pdata is filled and second is setting
>>>>>> up regulator in post probe which enseres that correct regulator
>>>>>> state according to pdata is reached.
>>>
>>> I think that the approach in this patch is trying to change too much all
>>> at once.
>>>
>>> Have made some adjustments that I think should help with transition.
>>> - Added a flag to protect regulator_autoset from being called more then
>>>  once for each regulator, in a separate patch.
>> 
>> It is not a good decision in the long run and you should keep in mind that there is nothing more constant than a temporary solution.
>
>Nor do I propose this to be a long-term solution, only that it is more
>reviewable to see changes in non-breaking smaller steps. In current
>state this patch breaks building U-Boot and requires the last patch to
>fix build again.
>
>Anyway, I will be posting a similar patch for autoset as linked below as
>part of a series to add a Rockchip IO-domain driver shortly. In current
>state autoset cannot safely be called multiple times, and such patch
>should not have an impact on the direction of this series.
>
>> 
>>> - Changed to only probe-after-bind on regulators marked as
>>>  always-on/boot-on.
>>>
>>> And it works something like this:
>>>
>>> static int regulator_post_bind(struct udevice *dev)
>>> {
>>> 	[...]
>>>
>>> 	if (uc_pdata->always_on || uc_pdata->boot_on)
>>> 		dev_or_flags(dev, DM_FLAG_PROBE_AFTER_BIND);
>>> }
>>>
>>> static int regulator_post_probe(struct udevice *dev)
>>> {
>>> 	ret = regulator_autoset(dev);
>>> }
>>>
>>> With that any always-on/boot-on regulator would automatically probe and
>>> autoset after bind, remaining would only probe and autoset if they are
>>> used.
>> 
>> uc_pdata is filled in pre_probe, while you are runing check in post_bind.
>
>Please look closer at the code and not the snippet above, they are
>filled in post_bind or the code would not have worked :-)

So you have moved that, alright. Well, I am fine with less invasive patch as long as my devices continue to work as intended. I will test your suggestions and reload patchset. Thanks.

Best regards,
Svyatoslav R.

>Regards,
>Jonas
>
>> 
>>> This approach should also prevent having to change existing code that
>>> may call autoset, and cleanup can be done in follow-up patches/series.
>>>
>>> Probe-after-bind and call to autoset could also be protected with a new
>>> Kconfig to help with transition.
>>>
>>> See following for a working example using a new driver that depends
>>> on that regulators have been autoset prior to regulator_get_value.
>>> https://github.com/Kwiboo/u-boot-rockchip/compare/master...rk3568-io-domain
>>>
>>> Or the two commits separate:
>>>
>>> power: regulator: Only run autoset once for each regulator
>>> https://github.com/Kwiboo/u-boot-rockchip/commit/05db4dbcb8f908b417ed5cae8a7a325c82112d75
>>>
>>> power: regulator: Perform regulator setup inside uclass
>>> https://github.com/Kwiboo/u-boot-rockchip/commit/489bd5d2c1a2a33824eac4f2d54399ef5dff4fdf
>>>
>>> Regards,
>>> Jonas
>>>
>>>>>>
>>>>>> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
>>>>>> ---
>>>>>>  drivers/power/regulator/regulator-uclass.c | 212 ++++++---------------
>>>>>>  include/power/regulator.h                  | 119 ------------
>>>>>>  2 files changed, 62 insertions(+), 269 deletions(-)
>>>>>
>>>>> Regards,
>>>>> SImon
>>>
>

Re: [PATCH v2 3/7] power: regulator-uclass: perform regulator setup inside uclass

[Svyatoslav Ryhel]

сб, 5 серп. 2023 р. о 15:49 Jonas Karlman <jonas@kwiboo.se> пише:
>
> Hi,
>
> On 2023-07-21 07:34, Svyatoslav Ryhel wrote:
> > чт, 20 лип. 2023 р. о 22:43 Simon Glass <sjg@chromium.org> пише:
> >>
> >> Hi Svyatoslav,
> >>
> >> On Thu, 20 Jul 2023 at 06:38, Svyatoslav Ryhel <clamor95@gmail.com> wrote:
> >>>
> >>> Regulators initial setup was previously dependent on board call.
> >>> To move from this behaviour were introduced two steps. First is
> >>> that all individual regulators will be probed just after binding
> >>
> >> We must not probe devices automatically when bound. The i2c interface
> >> may not be available, etc. Do a probe afterwards.
> >>
> >> Perhaps I am misunderstanding this, iwc please reword this commit message.
> >
> > After bound. If the regulator is a self-sufficient i2c device then it
> > will be bound
> > after i2c is available by code design so i2c interface should be
> > available at that
> > moment. At least led and gpio uclasses perform this for initial setup
> > of devices.
> >
> > Platform regulators (aka fixed/gpio regulators) work perfectly fine. I
> > have no i2c
> > regulators to test deeply.
> >
> > As for now only one uclass is not compatible with this system - PMIC which has
> > strong dependency between regulator and main mfd. This is why probing after
> > bind for pmic regulators is disabled and pmic regulators are probed on pmic core
> > post_probe.
>
> This sounds more like a possible bug of some dependency not being
> probed in correct order or not leaving the device in a ready state
> after probe.
>
> If pmic regulators are bind in pmic bind with the pmic as parent, then
> at regulator probe the driver core ensure that pmic has probed before
> regulator use.

I have found why this occurs. With this patchset always/boot-on regulators
are probed immediately after bind, since pmic regulators are bound on pmics
bind it causes a situation when regulator is probed before pmic and pmics ops
are not available. Moving pmic children bind to pmics probe fixed this issue.

> This works perfect fine with the RK8xx I2C PMIC and its regulators.
> Wich a call to device_get_supply_regulator(dev, "test-supply", &reg)
> probe happens in i2c, pmic and regulator order.
>
> >
> >>> which ensures that regulator pdata is filled and second is setting
> >>> up regulator in post probe which enseres that correct regulator
> >>> state according to pdata is reached.
>
> I think that the approach in this patch is trying to change too much all
> at once.
>
> Have made some adjustments that I think should help with transition.
> - Added a flag to protect regulator_autoset from being called more then
>   once for each regulator, in a separate patch.
> - Changed to only probe-after-bind on regulators marked as
>   always-on/boot-on.
>
> And it works something like this:
>
> static int regulator_post_bind(struct udevice *dev)
> {
>         [...]
>
>         if (uc_pdata->always_on || uc_pdata->boot_on)
>                 dev_or_flags(dev, DM_FLAG_PROBE_AFTER_BIND);
> }
>
> static int regulator_post_probe(struct udevice *dev)
> {
>         ret = regulator_autoset(dev);
> }
>
> With that any always-on/boot-on regulator would automatically probe and
> autoset after bind, remaining would only probe and autoset if they are
> used.
>
> This approach should also prevent having to change existing code that
> may call autoset, and cleanup can be done in follow-up patches/series.
>
> Probe-after-bind and call to autoset could also be protected with a new
> Kconfig to help with transition.
>
> See following for a working example using a new driver that depends
> on that regulators have been autoset prior to regulator_get_value.
> https://github.com/Kwiboo/u-boot-rockchip/compare/master...rk3568-io-domain
>
> Or the two commits separate:
>
> power: regulator: Only run autoset once for each regulator
> https://github.com/Kwiboo/u-boot-rockchip/commit/05db4dbcb8f908b417ed5cae8a7a325c82112d75
>
> power: regulator: Perform regulator setup inside uclass
> https://github.com/Kwiboo/u-boot-rockchip/commit/489bd5d2c1a2a33824eac4f2d54399ef5dff4fdf
>
> Regards,
> Jonas
>
> >>>
> >>> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> >>> ---
> >>>  drivers/power/regulator/regulator-uclass.c | 212 ++++++---------------
> >>>  include/power/regulator.h                  | 119 ------------
> >>>  2 files changed, 62 insertions(+), 269 deletions(-)
> >>
> >> Regards,
> >> SImon
>

Re: [PATCH v2 3/7] power: regulator-uclass: perform regulator setup inside uclass

[Svyatoslav Ryhel]

нд, 6 серп. 2023 р. о 19:21 Svyatoslav Ryhel <clamor95@gmail.com> пише:
>
> сб, 5 серп. 2023 р. о 15:49 Jonas Karlman <jonas@kwiboo.se> пише:
> >
> > Hi,
> >
> > On 2023-07-21 07:34, Svyatoslav Ryhel wrote:
> > > чт, 20 лип. 2023 р. о 22:43 Simon Glass <sjg@chromium.org> пише:
> > >>
> > >> Hi Svyatoslav,
> > >>
> > >> On Thu, 20 Jul 2023 at 06:38, Svyatoslav Ryhel <clamor95@gmail.com> wrote:
> > >>>
> > >>> Regulators initial setup was previously dependent on board call.
> > >>> To move from this behaviour were introduced two steps. First is
> > >>> that all individual regulators will be probed just after binding
> > >>
> > >> We must not probe devices automatically when bound. The i2c interface
> > >> may not be available, etc. Do a probe afterwards.
> > >>
> > >> Perhaps I am misunderstanding this, iwc please reword this commit message.
> > >
> > > After bound. If the regulator is a self-sufficient i2c device then it
> > > will be bound
> > > after i2c is available by code design so i2c interface should be
> > > available at that
> > > moment. At least led and gpio uclasses perform this for initial setup
> > > of devices.
> > >
> > > Platform regulators (aka fixed/gpio regulators) work perfectly fine. I
> > > have no i2c
> > > regulators to test deeply.
> > >
> > > As for now only one uclass is not compatible with this system - PMIC which has
> > > strong dependency between regulator and main mfd. This is why probing after
> > > bind for pmic regulators is disabled and pmic regulators are probed on pmic core
> > > post_probe.
> >
> > This sounds more like a possible bug of some dependency not being
> > probed in correct order or not leaving the device in a ready state
> > after probe.
> >
> > If pmic regulators are bind in pmic bind with the pmic as parent, then
> > at regulator probe the driver core ensure that pmic has probed before
> > regulator use.
>
> I have found why this occurs. With this patchset always/boot-on regulators
> are probed immediately after bind, since pmic regulators are bound on pmics
> bind it causes a situation when regulator is probed before pmic and pmics ops
> are not available. Moving pmic children bind to pmics probe fixed this issue.
>
I have made a mailing list reduction. This issue goes deeper, both pmic and its
regulator children work as expected with no issues there. I get error from i2c
driver directly that it fails on xfer. This function

https://github.com/clamor-s/u-boot/blob/master/drivers/i2c/tegra_i2c.c#L477

returns me EREMOTEIO. At the same time if the regulator is autoset on pmics
post_probe I do not have this error. Maybe you have any suggestions on how to
handle this situation?

Best regards,
Svyatoslav R.

> > This works perfect fine with the RK8xx I2C PMIC and its regulators.
> > Wich a call to device_get_supply_regulator(dev, "test-supply", &reg)
> > probe happens in i2c, pmic and regulator order.
> >
> > >
> > >>> which ensures that regulator pdata is filled and second is setting
> > >>> up regulator in post probe which enseres that correct regulator
> > >>> state according to pdata is reached.
> >
> > I think that the approach in this patch is trying to change too much all
> > at once.
> >
> > Have made some adjustments that I think should help with transition.
> > - Added a flag to protect regulator_autoset from being called more then
> >   once for each regulator, in a separate patch.
> > - Changed to only probe-after-bind on regulators marked as
> >   always-on/boot-on.
> >
> > And it works something like this:
> >
> > static int regulator_post_bind(struct udevice *dev)
> > {
> >         [...]
> >
> >         if (uc_pdata->always_on || uc_pdata->boot_on)
> >                 dev_or_flags(dev, DM_FLAG_PROBE_AFTER_BIND);
> > }
> >
> > static int regulator_post_probe(struct udevice *dev)
> > {
> >         ret = regulator_autoset(dev);
> > }
> >
> > With that any always-on/boot-on regulator would automatically probe and
> > autoset after bind, remaining would only probe and autoset if they are
> > used.
> >
> > This approach should also prevent having to change existing code that
> > may call autoset, and cleanup can be done in follow-up patches/series.
> >
> > Probe-after-bind and call to autoset could also be protected with a new
> > Kconfig to help with transition.
> >
> > See following for a working example using a new driver that depends
> > on that regulators have been autoset prior to regulator_get_value.
> > https://github.com/Kwiboo/u-boot-rockchip/compare/master...rk3568-io-domain
> >
> > Or the two commits separate:
> >
> > power: regulator: Only run autoset once for each regulator
> > https://github.com/Kwiboo/u-boot-rockchip/commit/05db4dbcb8f908b417ed5cae8a7a325c82112d75
> >
> > power: regulator: Perform regulator setup inside uclass
> > https://github.com/Kwiboo/u-boot-rockchip/commit/489bd5d2c1a2a33824eac4f2d54399ef5dff4fdf
> >
> > Regards,
> > Jonas
> >
> > >>>
> > >>> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> > >>> ---
> > >>>  drivers/power/regulator/regulator-uclass.c | 212 ++++++---------------
> > >>>  include/power/regulator.h                  | 119 ------------
> > >>>  2 files changed, 62 insertions(+), 269 deletions(-)
> > >>
> > >> Regards,
> > >> SImon
> >