[RFC] regulator: Propagate voltage changes to supply regulators

[RFC] regulator: Propagate voltage changes to supply regulators

[Sascha Hauer]

Until now changing the voltage of a regulator only ever effected the
regulator itself, but never its supplies. It's a common pattern though
to put LDO regulators behind switching regulators. The switching
regulators efficiently drop the input voltage but have a high ripple on
their output. The output is then cleaned up by the LDOs. For higher
energy efficiency the voltage drop at the LDOs should be minimized. This
patch adds support for such a scenario.

A new min_dropout_uv field is added to struct regulator_desc. Regulators
can specify the minimun dropout voltage they need for proper function
here. Now when the voltage is changed on a regulator the regulator core
makes sure that
a) before increasing the voltage on the current regulator the supply
   provides at least the desired voltage plus the minimum dropout
b) after decreasing the voltage on the current regulator the supply
   is optimized to the minimum required voltage within the needs of
   the consumers of the supply.

Calculating the optimum voltage for the supply regulator is a bit tricky
since the simple approach of just adding the desired minimum voltage and
the minimum dropout is not enough. It may happen that the current
regulator does not support the desired minimum voltage, but only a
higher one. This means we have to figure out the lowest voltage
supported by the regulator that is higher than the minimum desired
voltage. The regulator_get_voltage_floor introduced with this series
does
exactly that.

This is a first RFC for this series which probably has some rough edges,
but it was already tested successfully on a Phytec PFLA02 i.MX6 board.

Please review, any input welcome.

Sascha

[PATCH 1/6] Revert "regulator: core: Handle full constraints systems when resolving supplies"

[Sascha Hauer]

This reverts commit 9f7e25edb1575a6d2363dc003f9cc09d840657e2.

When a regulator A is registered and is supplied by regulator B which is
not yet registered then a regulator_get on regulator A will set the As
supply to the dummy regulator. This is not correct, we should return
-EPROBE_DEFER instead as done without this patch.
Of course reverting this patch brings back the issue it fixed, so this
is not a solution, but what is the correct solution?

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

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 7849187..bd9db70 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1403,13 +1403,9 @@ static int regulator_resolve_supply(struct regulator_dev *rdev)
 			return 0;
 		}
 
-		if (have_full_constraints()) {
-			r = dummy_regulator_rdev;
-		} else {
-			dev_err(dev, "Failed to resolve %s-supply for %s\n",
-				rdev->supply_name, rdev->desc->name);
-			return -EPROBE_DEFER;
-		}
+		dev_err(dev, "Failed to resolve %s-supply for %s\n",
+			rdev->supply_name, rdev->desc->name);
+		return -EPROBE_DEFER;
 	}
 
 	/* Recursively resolve the supply of the supply */
-- 
2.5.3

[PATCH 2/6] regulator: core: introduce function to lock regulators and its supplies

[Sascha Hauer]

Each regulator_dev is locked with its own mutex. This is fine as long
as only one regulator_dev is locked, but makes lockdep unhappy when we
have to walk up the supply chain like it can happen in
regulator_get_voltage:

regulator_get_voltage ->
 mutex_lock(&regulator->rdev->mutex) ->
_regulator_get_voltage(regulator->rdev) ->
regulator_get_voltage(rdev->supply) ->
mutex_lock(&regulator->rdev->mutex);

This causes lockdep to issue a possible deadlock warning.

There are at least two ways to work around this:

- We can always lock the whole supply chain using the functions
  introduced with this patch.
- We could store the current voltage in struct regulator_rdev so
  that we do not have to walk up the supply chain for the
  _regulator_get_voltage case.

Anyway, regulator_lock_supply/regulator_unlock_supply will be needed
once we allow regulator_set_voltage to optimize the supply voltages.

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

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index bd9db70..e5de3d9 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -132,6 +132,45 @@ static bool have_full_constraints(void)
 }
 
 /**
+ * regulator_lock_supply - lock a regulator and its supplies
+ * @rdev:         regulator source
+ */
+static void regulator_lock_supply(struct regulator_dev *rdev)
+{
+	struct regulator *supply;
+	int i = 0;
+
+	while (1) {
+		mutex_lock_nested(&rdev->mutex, i++);
+		supply = rdev->supply;
+
+		if (!rdev->supply)
+			return;
+
+		rdev = supply->rdev;
+	}
+}
+
+/**
+ * regulator_unlock_supply - unlock a regulator and its supplies
+ * @rdev:         regulator source
+ */
+static void regulator_unlock_supply(struct regulator_dev *rdev)
+{
+	struct regulator *supply;
+
+	while (1) {
+		mutex_unlock(&rdev->mutex);
+		supply = rdev->supply;
+
+		if (!rdev->supply)
+			return;
+
+		rdev = supply->rdev;
+	}
+}
+
+/**
  * of_get_regulator - get a regulator device node based on supply name
  * @dev: Device pointer for the consumer (of regulator) device
  * @supply: regulator supply name
-- 
2.5.3

[PATCH 3/6] regulator: core: create unlocked version of regulator_list_voltage

[Sascha Hauer]

The unlocked version will be needed when we start propagating voltage
changes to the supply regulators.

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

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index e5de3d9..d148545 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -2343,6 +2343,40 @@ static int _regulator_is_enabled(struct regulator_dev *rdev)
 	return rdev->desc->ops->is_enabled(rdev);
 }
 
+static int _regulator_list_voltage(struct regulator *regulator,
+				    unsigned selector, int lock)
+{
+	struct regulator_dev *rdev = regulator->rdev;
+	const struct regulator_ops *ops = rdev->desc->ops;
+	int ret;
+
+	if (rdev->desc->fixed_uV && rdev->desc->n_voltages == 1 && !selector)
+		return rdev->desc->fixed_uV;
+
+	if (ops->list_voltage) {
+		if (selector >= rdev->desc->n_voltages)
+			return -EINVAL;
+		if (lock)
+			mutex_lock(&rdev->mutex);
+		ret = ops->list_voltage(rdev, selector);
+		if (lock)
+			mutex_unlock(&rdev->mutex);
+	} else if (rdev->supply) {
+		ret = _regulator_list_voltage(rdev->supply, selector, lock);
+	} else {
+		return -EINVAL;
+	}
+
+	if (ret > 0) {
+		if (ret < rdev->constraints->min_uV)
+			ret = 0;
+		else if (ret > rdev->constraints->max_uV)
+			ret = 0;
+	}
+
+	return ret;
+}
+
 /**
  * regulator_is_enabled - is the regulator output enabled
  * @regulator: regulator source
@@ -2432,33 +2466,7 @@ EXPORT_SYMBOL_GPL(regulator_count_voltages);
  */
 int regulator_list_voltage(struct regulator *regulator, unsigned selector)
 {
-	struct regulator_dev *rdev = regulator->rdev;
-	const struct regulator_ops *ops = rdev->desc->ops;
-	int ret;
-
-	if (rdev->desc->fixed_uV && rdev->desc->n_voltages == 1 && !selector)
-		return rdev->desc->fixed_uV;
-
-	if (ops->list_voltage) {
-		if (selector >= rdev->desc->n_voltages)
-			return -EINVAL;
-		mutex_lock(&rdev->mutex);
-		ret = ops->list_voltage(rdev, selector);
-		mutex_unlock(&rdev->mutex);
-	} else if (rdev->supply) {
-		ret = regulator_list_voltage(rdev->supply, selector);
-	} else {
-		return -EINVAL;
-	}
-
-	if (ret > 0) {
-		if (ret < rdev->constraints->min_uV)
-			ret = 0;
-		else if (ret > rdev->constraints->max_uV)
-			ret = 0;
-	}
-
-	return ret;
+	return _regulator_list_voltage(regulator, selector, 1);
 }
 EXPORT_SYMBOL_GPL(regulator_list_voltage);
 
-- 
2.5.3

[PATCH 4/6] regulator: core: Propagate voltage changes to supply regulators

[Sascha Hauer]

Until now changing the voltage of a regulator only ever effected the
regulator itself, but never its supplies. It's a common pattern though
to put LDO regulators behind switching regulators. The switching
regulators efficiently drop the input voltage but have a high ripple on
their output. The output is then cleaned up by the LDOs. For higher
energy efficiency the voltage drop at the LDOs should be minimized. This
patch adds support for such a scenario.

A new min_dropout_uv field is added to struct regulator_desc. Regulators
can specify the minimun dropout voltage they need for proper function
here. Now when the voltage is changed on a regulator the regulator core
makes sure that
a) before increasing the voltage on the current regulator the supply
   provides at least the desired voltage plus the minimum dropout
b) after decreasing the voltage on the current regulator the supply
   is optimized to the minimum required voltage within the needs of
   the consumers of the supply.

Calculating the optimum voltage for the supply regulator is a bit tricky
since the simple approach of just adding the desired minimum voltage and
the minimum dropout is not enough. It may happen that the current
regulator does not support the desired minimum voltage, but only a
higher one. This means we have to figure out the lowest voltage
supported by the regulator that is higher than the minimum desired
voltage. The regulator_get_voltage_floor introduced with this patch does
exactly that.

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

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index d148545..36da924 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -2756,32 +2756,54 @@ static int _regulator_do_set_voltage(struct regulator_dev *rdev,
 	return ret;
 }
 
-/**
- * regulator_set_voltage - set regulator output voltage
- * @regulator: regulator source
- * @min_uV: Minimum required voltage in uV
- * @max_uV: Maximum acceptable voltage in uV
- *
- * Sets a voltage regulator to the desired output voltage. This can be set
- * during any regulator state. IOW, regulator can be disabled or enabled.
- *
- * If the regulator is enabled then the voltage will change to the new value
- * immediately otherwise if the regulator is disabled the regulator will
- * output at the new voltage when enabled.
- *
- * NOTE: If the regulator is shared between several devices then the lowest
- * request voltage that meets the system constraints will be used.
- * Regulator system constraints must be set for this regulator before
- * calling this function otherwise this call will fail.
+/*
+ * Return the minimum voltage supported by a regulator that is higher or equal
+ * to a given voltage.
  */
-int regulator_set_voltage(struct regulator *regulator, int min_uV, int max_uV)
+static int regulator_get_voltage_floor(struct regulator *regulator, int min_uV)
+{
+	struct regulator_dev *rdev = regulator->rdev;
+	int num_voltages;
+	int best = INT_MAX;
+	int max_uV = INT_MAX;
+	int i, now, ret;
+
+	/* constraints check */
+	ret = regulator_check_voltage(rdev, &min_uV, &max_uV);
+	if (ret < 0)
+		return ret;
+
+	ret = regulator_check_consumers(rdev, &min_uV, &max_uV);
+	if (ret < 0)
+		return ret;
+
+	num_voltages = regulator_count_voltages(regulator);
+	if (num_voltages < 0)
+		return num_voltages;
+
+	for (i = 0; i < num_voltages; i++) {
+		now = _regulator_list_voltage(regulator, i, 0);
+		if (now < 0)
+			continue;
+		if (now < best && now >= min_uV)
+			best = now;
+	}
+
+	if (best > max_uV)
+		return -EINVAL;
+
+	return best;
+}
+
+static int regulator_set_voltage_unlocked(struct regulator *regulator,
+					  int min_uV, int max_uV)
 {
 	struct regulator_dev *rdev = regulator->rdev;
 	int ret = 0;
 	int old_min_uV, old_max_uV;
 	int current_uV;
-
-	mutex_lock(&rdev->mutex);
+	int best_supply_uV = 0;
+	int supply_change_uV = 0;
 
 	/* If we're setting the same range as last time the change
 	 * should be a noop (some cpufreq implementations use the same
@@ -2825,17 +2847,87 @@ int regulator_set_voltage(struct regulator *regulator, int min_uV, int max_uV)
 	if (ret < 0)
 		goto out2;
 
+	if (rdev->supply) {
+		int current_supply_uV;
+
+		best_supply_uV = regulator_get_voltage_floor(regulator, min_uV);
+		if (best_supply_uV < 0) {
+			ret = best_supply_uV;
+			goto out2;
+		}
+
+		best_supply_uV += rdev->desc->min_dropout_uv;
+
+		current_supply_uV = _regulator_get_voltage(rdev->supply->rdev);
+		if (current_supply_uV < 0) {
+			ret = current_supply_uV;
+			goto out2;
+		}
+
+		supply_change_uV = best_supply_uV - current_supply_uV;
+	}
+
+	if (supply_change_uV > 0) {
+		ret = regulator_set_voltage_unlocked(rdev->supply,
+				best_supply_uV, INT_MAX);
+		if (ret) {
+			dev_err(&rdev->dev, "Failed to increase supply voltage: %d\n",
+					ret);
+			goto out2;
+		}
+	}
+
 	ret = _regulator_do_set_voltage(rdev, min_uV, max_uV);
 	if (ret < 0)
 		goto out2;
 
+	if (supply_change_uV < 0) {
+		ret = regulator_set_voltage_unlocked(rdev->supply,
+				best_supply_uV, INT_MAX);
+		if (ret)
+			dev_warn(&rdev->dev, "Failed to decrease supply voltage: %d\n",
+					ret);
+		/* No need to fail here */
+		ret = 0;
+	}
+
 out:
-	mutex_unlock(&rdev->mutex);
 	return ret;
 out2:
 	regulator->min_uV = old_min_uV;
 	regulator->max_uV = old_max_uV;
-	mutex_unlock(&rdev->mutex);
+
+	return ret;
+}
+
+/**
+ * regulator_set_voltage - set regulator output voltage
+ * @regulator: regulator source
+ * @min_uV: Minimum required voltage in uV
+ * @max_uV: Maximum acceptable voltage in uV
+ *
+ * Sets a voltage regulator to the desired output voltage. This can be set
+ * during any regulator state. IOW, regulator can be disabled or enabled.
+ *
+ * If the regulator is enabled then the voltage will change to the new value
+ * immediately otherwise if the regulator is disabled the regulator will
+ * output at the new voltage when enabled.
+ *
+ * NOTE: If the regulator is shared between several devices then the lowest
+ * request voltage that meets the system constraints will be used.
+ * Regulator system constraints must be set for this regulator before
+ * calling this function otherwise this call will fail.
+ */
+int regulator_set_voltage(struct regulator *regulator, int min_uV, int max_uV)
+{
+	int ret = 0;
+
+	regulator_lock_supply(regulator->rdev);
+
+	ret = regulator_set_voltage_unlocked(regulator, min_uV, max_uV);
+
+	regulator_unlock_supply(regulator->rdev);
+
 	return ret;
 }
 EXPORT_SYMBOL_GPL(regulator_set_voltage);
diff --git a/include/linux/regulator/driver.h b/include/linux/regulator/driver.h
index 4593222..a3815bd 100644
--- a/include/linux/regulator/driver.h
+++ b/include/linux/regulator/driver.h
@@ -245,6 +245,7 @@ enum regulator_type {
  * @linear_min_sel: Minimal selector for starting linear mapping
  * @fixed_uV: Fixed voltage of rails.
  * @ramp_delay: Time to settle down after voltage change (unit: uV/us)
+ * @min_dropout_uv: The minimum dropout voltage this regulator can handle
  * @linear_ranges: A constant table of possible voltage ranges.
  * @n_linear_ranges: Number of entries in the @linear_ranges table.
  * @volt_table: Voltage mapping table (if table based mapping)
@@ -292,6 +293,7 @@ struct regulator_desc {
 	unsigned int linear_min_sel;
 	int fixed_uV;
 	unsigned int ramp_delay;
+	int min_dropout_uv;
 
 	const struct regulator_linear_range *linear_ranges;
 	int n_linear_ranges;
-- 
2.5.3

[PATCH 5/6] regulator: i.MX anatop: Allow supply regulator

[Sascha Hauer]

The anatop regulators are SoC internal LDO regulators usually supplied
by an external PMIC. This patch adds support for specifying the supply
from the device tree using the vin-supply property.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 Documentation/devicetree/bindings/regulator/anatop-regulator.txt | 1 +
 drivers/regulator/anatop-regulator.c                             | 3 +++
 2 files changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/regulator/anatop-regulator.txt b/Documentation/devicetree/bindings/regulator/anatop-regulator.txt
index 758eae2..37c4ea0 100644
--- a/Documentation/devicetree/bindings/regulator/anatop-regulator.txt
+++ b/Documentation/devicetree/bindings/regulator/anatop-regulator.txt
@@ -13,6 +13,7 @@ Optional properties:
 - anatop-delay-reg-offset: Anatop MFD step time register offset
 - anatop-delay-bit-shift: Bit shift for the step time register
 - anatop-delay-bit-width: Number of bits used in the step time register
+- vin-supply: The supply for this regulator
 
 Any property defined as part of the core regulator
 binding, defined in regulator.txt, can also be used.
diff --git a/drivers/regulator/anatop-regulator.c b/drivers/regulator/anatop-regulator.c
index 52ea605..ba78870 100644
--- a/drivers/regulator/anatop-regulator.c
+++ b/drivers/regulator/anatop-regulator.c
@@ -30,6 +30,7 @@
 #include <linux/regmap.h>
 #include <linux/regulator/driver.h>
 #include <linux/regulator/of_regulator.h>
+#include <linux/regulator/machine.h>
 
 #define LDO_RAMP_UP_UNIT_IN_CYCLES      64 /* 64 cycles per step */
 #define LDO_RAMP_UP_FREQ_IN_MHZ         24 /* cycle based on 24M OSC */
@@ -199,6 +200,7 @@ static int anatop_regulator_probe(struct platform_device *pdev)
 	rdesc->owner = THIS_MODULE;
 
 	initdata = of_get_regulator_init_data(dev, np, rdesc);
+	initdata->supply_regulator = "vin";
 	sreg->initdata = initdata;
 
 	anatop_np = of_get_parent(np);
@@ -262,6 +264,7 @@ static int anatop_regulator_probe(struct platform_device *pdev)
 	rdesc->vsel_reg = sreg->control_reg;
 	rdesc->vsel_mask = ((1 << sreg->vol_bit_width) - 1) <<
 			   sreg->vol_bit_shift;
+	rdesc->min_dropout_uv = 125000;
 
 	config.dev = &pdev->dev;
 	config.init_data = initdata;
-- 
2.5.3

[PATCH 6/6] ARM: i.MX6 Phytec PFLA02: Add supplies for the SoC internal regulators

[Sascha Hauer]

The SoC internal regulators for the CPU and the SoC come from the
DA9063 vdd_core and vdd_soc. Add this relationship to the device tree
so that the voltage drop on the SoC internal LDO regulators can be
minimized.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 arch/arm/boot/dts/imx6qdl-phytec-pfla02.dtsi | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/arch/arm/boot/dts/imx6qdl-phytec-pfla02.dtsi b/arch/arm/boot/dts/imx6qdl-phytec-pfla02.dtsi
index 9e6ecd9..f19c680 100644
--- a/arch/arm/boot/dts/imx6qdl-phytec-pfla02.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-phytec-pfla02.dtsi
@@ -379,6 +379,18 @@
 	status = "disabled";
 };
 
+&reg_arm {
+	vin-supply = <&vddcore_reg>;
+};
+
+&reg_pu {
+	vin-supply = <&vddsoc_reg>;
+};
+
+&reg_soc {
+	vin-supply = <&vddsoc_reg>;
+};
+
 &uart3 {
 	pinctrl-names = "default";
 	pinctrl-0 = <&pinctrl_uart3>;
-- 
2.5.3

[RFC] regulator: Propagate voltage changes to supply regulators

[Sascha Hauer]

Until now changing the voltage of a regulator only ever effected the
regulator itself, but never its supplies. It's a common pattern though
to put LDO regulators behind switching regulators. The switching
regulators efficiently drop the input voltage but have a high ripple on
their output. The output is then cleaned up by the LDOs. For higher
energy efficiency the voltage drop at the LDOs should be minimized. This
patch adds support for such a scenario.

A new min_dropout_uv field is added to struct regulator_desc. Regulators
can specify the minimun dropout voltage they need for proper function
here. Now when the voltage is changed on a regulator the regulator core
makes sure that
a) before increasing the voltage on the current regulator the supply
   provides at least the desired voltage plus the minimum dropout
b) after decreasing the voltage on the current regulator the supply
   is optimized to the minimum required voltage within the needs of
   the consumers of the supply.

Calculating the optimum voltage for the supply regulator is a bit tricky
since the simple approach of just adding the desired minimum voltage and
the minimum dropout is not enough. It may happen that the current
regulator does not support the desired minimum voltage, but only a
higher one. This means we have to figure out the lowest voltage
supported by the regulator that is higher than the minimum desired
voltage. The regulator_get_voltage_floor introduced with this series
does
exactly that.

This is a first RFC for this series which probably has some rough edges,
but it was already tested successfully on a Phytec PFLA02 i.MX6 board.

Please review, any input welcome.

Sascha

Re: [RFC] regulator: Propagate voltage changes to supply regulators

[Sascha Hauer]

Please do not answer to this series, it contains a wrong address for the
linux-arm-kernel list. I just resent this with the correct addres. Sorry
for the noise.

Sascha

On Wed, Sep 30, 2015 at 03:57:48PM +0200, Sascha Hauer wrote:
> Until now changing the voltage of a regulator only ever effected the
> regulator itself, but never its supplies. It's a common pattern though
> to put LDO regulators behind switching regulators. The switching
> regulators efficiently drop the input voltage but have a high ripple on
> their output. The output is then cleaned up by the LDOs. For higher
> energy efficiency the voltage drop at the LDOs should be minimized. This
> patch adds support for such a scenario.
> 
> A new min_dropout_uv field is added to struct regulator_desc. Regulators
> can specify the minimun dropout voltage they need for proper function
> here. Now when the voltage is changed on a regulator the regulator core
> makes sure that
> a) before increasing the voltage on the current regulator the supply
>    provides at least the desired voltage plus the minimum dropout
> b) after decreasing the voltage on the current regulator the supply
>    is optimized to the minimum required voltage within the needs of
>    the consumers of the supply.
> 
> Calculating the optimum voltage for the supply regulator is a bit tricky
> since the simple approach of just adding the desired minimum voltage and
> the minimum dropout is not enough. It may happen that the current
> regulator does not support the desired minimum voltage, but only a
> higher one. This means we have to figure out the lowest voltage
> supported by the regulator that is higher than the minimum desired
> voltage. The regulator_get_voltage_floor introduced with this series
> does
> exactly that.
> 
> This is a first RFC for this series which probably has some rough edges,
> but it was already tested successfully on a Phytec PFLA02 i.MX6 board.
> 
> Please review, any input welcome.
> 
> Sascha
> 
> 
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 1/6] Revert "regulator: core: Handle full constraints systems when resolving supplies"

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 667 bytes --]

On Wed, Sep 30, 2015 at 03:57:49PM +0200, Sascha Hauer wrote:
> This reverts commit 9f7e25edb1575a6d2363dc003f9cc09d840657e2.
> 
> When a regulator A is registered and is supplied by regulator B which is
> not yet registered then a regulator_get on regulator A will set the As
> supply to the dummy regulator. This is not correct, we should return
> -EPROBE_DEFER instead as done without this patch.

What makes you say this is not correct?  In a system with fully
specified supplies if we fail to resolve the supply we know that no
supply will ever appear and so substitute in the dummy on the assumption
that there is a supply with no software control.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 1/6] Revert "regulator: core: Handle full constraints systems when resolving supplies"

[Sascha Hauer]

On Wed, Sep 30, 2015 at 07:02:13PM +0100, Mark Brown wrote:
> On Wed, Sep 30, 2015 at 03:57:49PM +0200, Sascha Hauer wrote:
> > This reverts commit 9f7e25edb1575a6d2363dc003f9cc09d840657e2.
> > 
> > When a regulator A is registered and is supplied by regulator B which is
> > not yet registered then a regulator_get on regulator A will set the As
> > supply to the dummy regulator. This is not correct, we should return
> > -EPROBE_DEFER instead as done without this patch.
> 
> What makes you say this is not correct?  In a system with fully
> specified supplies if we fail to resolve the supply we know that no
> supply will ever appear and so substitute in the dummy on the assumption
> that there is a supply with no software control.

The situation I'm in is:

		gpc: gpc@020dc000 {
			compatible = "fsl,imx6q-gpc";
			pu-supply = <&reg_pu>;
		};

		reg_pu: regulator-vddpu@140 {
			compatible = "fsl,anatop-regulator";
			vin-supply = <&vddsoc_reg>;
		};

		...

		pmic@58 {
			compatible = "dlg,da9063";
			regulators {
				...
				vddsoc_reg: bcore2 {
					regulator-min-microvolt = <730000>;
					regulator-max-microvolt = <1380000>;
					regulator-always-on;
				};
			};
		};

During boot the fsl,imx6q-gpc is probed and tries to get its regulator.
fsl,anatop-regulator is already registered, the dlg,da9063 is not.  The
call to regulator_get tries to resolve the supply using
regulator_resolve_supply. regulator_resolve_supply then calls
regulator_dev_lookup which returns NULL because the supply (vddsoc_reg)
is specified in the device tree but not yet present. &ret is correctly
filled with -EPROBE_DEFER in this case, but this is ignored. Instead due
to 9f7e25edb157 the dummy regulator is used. This case worked before
9f7e25edb157.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 1/6] Revert "regulator: core: Handle full constraints systems when resolving supplies"

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 726 bytes --]

On Wed, Sep 30, 2015 at 08:48:12PM +0200, Sascha Hauer wrote:

> During boot the fsl,imx6q-gpc is probed and tries to get its regulator.
> fsl,anatop-regulator is already registered, the dlg,da9063 is not.  The
> call to regulator_get tries to resolve the supply using
> regulator_resolve_supply. regulator_resolve_supply then calls
> regulator_dev_lookup which returns NULL because the supply (vddsoc_reg)
> is specified in the device tree but not yet present. &ret is correctly
> filled with -EPROBE_DEFER in this case, but this is ignored. Instead due
> to 9f7e25edb157 the dummy regulator is used. This case worked before
> 9f7e25edb157.

Ah, I see - yes that's a bug.  Untested patch incoming, let me know if
that works.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]