[PATCH V3] regulator: disable supply regulator if it is enabled for boot-on

[PATCH V3] regulator: disable supply regulator if it is enabled for boot-on

[Laxman Dewangan]

If supply regulator is enabled because of boot-on (not always-on)
then disable regulator need to be call if regulator have some
user or full constraint has been enabled.
This will make sure that reference count of supply regulator
is in sync with child regulator's state.

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
Reported-by: Rabin Vincent <rabin.vincent@gmail.com>
---
Changes from V1:
Rabin reported that the nested locking trace is getting generated.
This is because the regulator lock mutex and disable himself and then
call for supply disable which again lock the regulator lock.
Probably because both lock is taken from regulator structure in nested
manner, the trace is getting generated.
Rewritten patch to avoid nested locking.

Changes from v2:
The patch 1 does not handle the case where a regulator is not set boot_on
but is considered on (for example, because of the lack of an is_enabled
callback), and is later actually enabled by a consumer before
regulator_init_complete().
So here I added the flag to track if the supply is enabled during registration.


 drivers/regulator/core.c         |   15 ++++++++++++++-
 include/linux/regulator/driver.h |    5 +++++
 2 files changed, 19 insertions(+), 1 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 06186ba..f166a4f 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -3284,6 +3284,7 @@ regulator_register(const struct regulator_desc *regulator_desc,
 			ret = regulator_enable(rdev->supply);
 			if (ret < 0)
 				goto scrub;
+			rdev->need_supply_disable = 1;
 		}
 	}
 
@@ -3312,8 +3313,13 @@ unset_supplies:
 	unset_regulator_supplies(rdev);
 
 scrub:
-	if (rdev->supply)
+	if (rdev->supply) {
+		if (rdev->need_supply_disable) {
+			regulator_disable(rdev->supply);
+			rdev->need_supply_disable = 0;
+		}
 		regulator_put(rdev->supply);
+	}
 	if (rdev->ena_gpio)
 		gpio_free(rdev->ena_gpio);
 	kfree(rdev->constraints);
@@ -3606,6 +3612,8 @@ static int __init regulator_init_complete(void)
 	 * default behaviour in the future.
 	 */
 	list_for_each_entry(rdev, &regulator_list, list) {
+		bool supply_disable = true;
+
 		ops = rdev->desc->ops;
 		c = rdev->constraints;
 
@@ -3641,10 +3649,15 @@ static int __init regulator_init_complete(void)
 			 * anything here.
 			 */
 			rdev_warn(rdev, "incomplete constraints, leaving on\n");
+			supply_disable = false;
 		}
 
 unlock:
 		mutex_unlock(&rdev->mutex);
+		if (rdev->need_supply_disable && supply_disable) {
+			regulator_disable(rdev->supply);
+			rdev->need_supply_disable = 0;
+		}
 	}
 
 	mutex_unlock(&regulator_list_mutex);
diff --git a/include/linux/regulator/driver.h b/include/linux/regulator/driver.h
index c10012f..bf31771 100644
--- a/include/linux/regulator/driver.h
+++ b/include/linux/regulator/driver.h
@@ -279,6 +279,11 @@ struct regulator_dev {
 	int ena_gpio;
 	unsigned int ena_gpio_invert:1;
 	unsigned int ena_gpio_state:1;
+
+	/* If supply enabled during registrtaion then need to disable
+	 * when full constraint enabled.
+	 */
+	unsigned int need_supply_disable:1;
 };
 
 struct regulator_dev *
-- 
1.7.1.1

Re: [PATCH V3] regulator: disable supply regulator if it is enabled for boot-on

[Mark Brown]

On Mon, Sep 03, 2012 at 07:55:39PM +0530, Laxman Dewangan wrote:

> +	/* If supply enabled during registrtaion then need to disable
> +	 * when full constraint enabled.
> +	 */
> +	unsigned int need_supply_disable:1;

I don't understand why we need this flag.  Shouldn't we just be making
the reference counts for the supplies correct and then have the
regulator disabled in the late initcall as a result of its refcount
falling to zero when the children are disabled?  I think what we need to
do here is always take a reference to the supply if the child is enabled
during boot, then in the initcall we can just disable the parent
regulator as normal.

Re: [PATCH V3] regulator: disable supply regulator if it is enabled for boot-on

[Laxman Dewangan]

On Friday 07 September 2012 07:29 AM, Mark Brown wrote:
> On Mon, Sep 03, 2012 at 07:55:39PM +0530, Laxman Dewangan wrote:
>
>> +	/* If supply enabled during registrtaion then need to disable
>> +	 * when full constraint enabled.
>> +	 */
>> +	unsigned int need_supply_disable:1;
> I don't understand why we need this flag.  Shouldn't we just be making
> the reference counts for the supplies correct and then have the
> regulator disabled in the late initcall as a result of its refcount
> falling to zero when the children are disabled?
Here we are trying to call the supply disable if child refcount become 0.
So when child calls disable, it need to call the supply disable also.

>   I think what we need to
> do here is always take a reference to the supply if the child is enabled
> during boot, then in the initcall we can just disable the parent
> regulator as normal.
My second patch was similar but Rabin has some issue to handle the case 
when is_regulator_enable() is not implementd by regulator device driver.

Re: [PATCH V3] regulator: disable supply regulator if it is enabled for boot-on

[Mark Brown]

On Fri, Sep 07, 2012 at 06:21:25PM +0530, Laxman Dewangan wrote:
> On Friday 07 September 2012 07:29 AM, Mark Brown wrote:

> >  I think what we need to
> >do here is always take a reference to the supply if the child is enabled
> >during boot, then in the initcall we can just disable the parent
> >regulator as normal.

> My second patch was similar but Rabin has some issue to handle the
> case when is_regulator_enable() is not implementd by regulator
> device driver.

So let's fix that...  if it's causing an issue in this case it's likely
to affect other areas too, coding in custom hacks in different bits of
the code isn't going to help maintainability.  What was the issue?