Re: [PATCH] regulator: core: Use parent voltage from the supply when bypassed

Re: [PATCH] regulator: core: Use parent voltage from the supply when bypassed

[Jon Hunter]

On 30/03/16 18:32, Mark Brown wrote:
> When a regulator is in bypass mode it is functioning as a switch
> returning the voltage set in the regulator will not give the voltage
> being output by the regulator as it's just passing through its supply.
> This means that when we are getting the voltage from a regulator we
> should check to see if it is in bypass mode and if it is we should
> report the voltage from the supply rather than that which is set on the
> regulator.
> 
> Reported-by: Jon Hunter <jonathanh@nvidia.com>
> Signed-off-by: Mark Brown <broonie@kernel.org>
> ---
> 
> Completely untested.
> 
>  drivers/regulator/core.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index 74e8a7a3b3e8..03042e450399 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -3118,6 +3118,20 @@ EXPORT_SYMBOL_GPL(regulator_sync_voltage);
>  static int _regulator_get_voltage(struct regulator_dev *rdev)
>  {
>  	int sel, ret;
> +	bool bypassed;
> +
> +	if (rdev->desc->ops->get_bypass) {
> +		ret = rdev->desc->ops->get_bypass(rdev, &bypassed);
> +		if (ret < 0)
> +			return ret;
> +		if (bypassed) {
> +			if (rdev->supply) {
> +				ret = _regulator_get_voltage(rdev->supply->rdev);

Should this be a return here?

> +			} else {
> +				return -EINVAL;
> +			}
> +		}
> +	}
>  
>  	if (rdev->desc->ops->get_voltage_sel) {
>  		sel = rdev->desc->ops->get_voltage_sel(rdev);
> 

I gave this a quick test on tegra124 having populated the
get/set_bypass() operators for the as3722. In this case, there is still
a problem because _regulator_get_voltage() is called during regulator
registration when set_machine_constraints() is called, which is before
we have called regulator_register_resolve_supply(). Therefore, it seems
to me that we still need to resolve the supply before we call
set_machine_constraints().

Jon

Re: [PATCH] regulator: core: Use parent voltage from the supply when bypassed

[Mark Brown]

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

On Thu, Mar 31, 2016 at 03:27:58PM +0100, Jon Hunter wrote:
> On 30/03/16 18:32, Mark Brown wrote:

> > +		if (bypassed) {
> > +			if (rdev->supply) {
> > +				ret = _regulator_get_voltage(rdev->supply->rdev);

> Should this be a return here?

Gnargh, yes.

> I gave this a quick test on tegra124 having populated the
> get/set_bypass() operators for the as3722. In this case, there is still
> a problem because _regulator_get_voltage() is called during regulator
> registration when set_machine_constraints() is called, which is before
> we have called regulator_register_resolve_supply(). Therefore, it seems
> to me that we still need to resolve the supply before we call
> set_machine_constraints().

OK, we need to change the code there to defer probe in the case where
the device is in bypass mode as well.

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

Re: [PATCH] regulator: core: Use parent voltage from the supply when bypassed

[Thierry Reding]

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

On Thu, Mar 31, 2016 at 10:55:27AM -0700, Mark Brown wrote:
> On Thu, Mar 31, 2016 at 03:27:58PM +0100, Jon Hunter wrote:
> > On 30/03/16 18:32, Mark Brown wrote:
> 
> > > +		if (bypassed) {
> > > +			if (rdev->supply) {
> > > +				ret = _regulator_get_voltage(rdev->supply->rdev);
> 
> > Should this be a return here?
> 
> Gnargh, yes.
> 
> > I gave this a quick test on tegra124 having populated the
> > get/set_bypass() operators for the as3722. In this case, there is still
> > a problem because _regulator_get_voltage() is called during regulator
> > registration when set_machine_constraints() is called, which is before
> > we have called regulator_register_resolve_supply(). Therefore, it seems
> > to me that we still need to resolve the supply before we call
> > set_machine_constraints().
> 
> OK, we need to change the code there to defer probe in the case where
> the device is in bypass mode as well.

The following on top of your patch here and Jon's two patches to add
bypass support for the LDO6 on AS3722 make Jetson TK1 boot fine for me
again.

I can turn this into a proper commit if you want, or perhaps it might be
worth splitting this into several patches, one rearranging the supply
resolution vs. constraints application and another fixing up the bypass
patch you sent earlier (probably best to fold it into your original).

Thierry

--- >8 ---
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 6e5085034b77..23c8c4c86389 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -3125,11 +3125,11 @@ static int _regulator_get_voltage(struct regulator_dev *rdev)
 		if (ret < 0)
 			return ret;
 		if (bypassed) {
-			if (rdev->supply) {
-				ret = _regulator_get_voltage(rdev->supply->rdev);
-			} else {
+			/* if bypassed the regulator must have a supply */
+			if (!rdev->supply)
 				return -EINVAL;
-			}
+
+			return _regulator_get_voltage(rdev->supply->rdev);
 		}
 	}
 
@@ -3986,18 +3986,27 @@ regulator_register(const struct regulator_desc *regulator_desc,
 
 	dev_set_drvdata(&rdev->dev, rdev);
 
+	if (init_data && init_data->supply_regulator)
+		rdev->supply_name = init_data->supply_regulator;
+	else if (regulator_desc->supply_name)
+		rdev->supply_name = regulator_desc->supply_name;
+
+	/*
+	 * set_machine_constraints() needs the supply to be resolved in order
+	 * to support querying the current voltage in bypass mode. Resolve it
+	 * here to more easily handle deferred probing.
+	 */
+	ret = regulator_resolve_supply(rdev);
+	if (ret < 0)
+		goto scrub;
+
 	/* set regulator constraints */
 	if (init_data)
 		constraints = &init_data->constraints;
 
 	ret = set_machine_constraints(rdev, constraints);
 	if (ret < 0)
-		goto scrub;
-
-	if (init_data && init_data->supply_regulator)
-		rdev->supply_name = init_data->supply_regulator;
-	else if (regulator_desc->supply_name)
-		rdev->supply_name = regulator_desc->supply_name;
+		goto tumble;
 
 	/* add consumers devices */
 	if (init_data) {
@@ -4024,7 +4033,13 @@ regulator_register(const struct regulator_desc *regulator_desc,
 
 unset_supplies:
 	unset_regulator_supplies(rdev);
+tumble:
+	if (rdev->supply) {
+		if (_regulator_is_enabled(rdev))
+			regulator_disable(rdev->supply);
 
+		_regulator_put(rdev->supply);
+	}
 scrub:
 	regulator_ena_gpio_free(rdev);
 	device_unregister(&rdev->dev);

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]