[PATCH 2/3] regulator-core: update all enable GPIO state in _enable/disable

[PATCH 2/3] regulator-core: update all enable GPIO state in _enable/disable

[Kim, Milo]

 When a regulator is enabled or disabled, enable GPIO state is changed if it is
 used.
 If multiple regulators are controlled by one shared GPIO, pin states of other
 GPIOs should be updated also.

 Look up the list of regulators and change the enable pin state if same GPIO
 is used. It guarantees exact value of regulator_is_enabled().

Signed-off-by: Milo(Woogyom) Kim <milo.kim@ti.com>
---
 drivers/regulator/core.c |   16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 21b4247..44b2d61 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1473,6 +1473,17 @@ static int regulator_enable_gpio_request(struct regulator_dev *rdev,
 				rdev_get_name(rdev));
 }
 
+static void update_enable_gpio_state(int gpio, bool enable)
+{
+	struct regulator_dev *r;
+
+	/* Update shared enable pin state */
+	list_for_each_entry(r, &regulator_list, list) {
+		if (r->ena_gpio == gpio)
+			r->ena_gpio_state = enable ? 1 : 0;
+	}
+}
+
 static int _regulator_do_enable(struct regulator_dev *rdev)
 {
 	int ret, delay;
@@ -1491,7 +1502,7 @@ static int _regulator_do_enable(struct regulator_dev *rdev)
 	if (rdev->ena_gpio) {
 		gpio_set_value_cansleep(rdev->ena_gpio,
 					!rdev->ena_gpio_invert);
-		rdev->ena_gpio_state = 1;
+		update_enable_gpio_state(rdev->ena_gpio, true);
 	} else if (rdev->desc->ops->enable) {
 		ret = rdev->desc->ops->enable(rdev);
 		if (ret < 0)
@@ -1595,8 +1606,7 @@ static int _regulator_do_disable(struct regulator_dev *rdev)
 	if (rdev->ena_gpio) {
 		gpio_set_value_cansleep(rdev->ena_gpio,
 					rdev->ena_gpio_invert);
-		rdev->ena_gpio_state = 0;
-
+		update_enable_gpio_state(rdev->ena_gpio, false);
 	} else if (rdev->desc->ops->disable) {
 		ret = rdev->desc->ops->disable(rdev);
 		if (ret != 0)
-- 
1.7.9.5


Best Regards,
Milo

Re: [PATCH 2/3] regulator-core: update all enable GPIO state in _enable/disable

[Mark Brown]

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

On Thu, Jan 10, 2013 at 09:46:12AM +0000, Kim, Milo wrote:

> +	/* Update shared enable pin state */
> +	list_for_each_entry(r, &regulator_list, list) {
> +		if (r->ena_gpio == gpio)
> +			r->ena_gpio_state = enable ? 1 : 0;
> +	}
> +}

This isn't quite going to work properly - we need to actually move the
state (and also a reference count) into some sort of shared state.
Otherwise consider what happens in this scenario:

	1. regulator 1 enables
	2. regulator 2 enables
	3. regulator 1 disables

Since regulator 1 doesn't know anything about regulator 2 it'll not know
that regulator 2 should still be enabled so it'll put the GPIO into the
disabled state.  It'll be OK if regulator 1 and regulator 2 always have
the same state but if they ever differ then we'll not do the right thing.

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

RE: [PATCH 2/3] regulator-core: update all enable GPIO state in _enable/disable

[Kim, Milo]

> From: Mark Brown [mailto:broonie@opensource.wolfsonmicro.com]
> 
> This isn't quite going to work properly - we need to actually move the
> state (and also a reference count) into some sort of shared state.
> Otherwise consider what happens in this scenario:
> 
> 	1. regulator 1 enables
> 	2. regulator 2 enables
> 	3. regulator 1 disables
> 
> Since regulator 1 doesn't know anything about regulator 2 it'll not
> know
> that regulator 2 should still be enabled so it'll put the GPIO into the
> disabled state.  It'll be OK if regulator 1 and regulator 2 always have
> the same state but if they ever differ then we'll not do the right
> thing.

Thanks for your opinion.
My understanding is 'shared' enable pin means regulators are enabled/disabled
at the same time.
Any other case is possible?

Best Regards,
Milo

Re: [PATCH 2/3] regulator-core: update all enable GPIO state in _enable/disable

[Mark Brown]

On Sun, Jan 13, 2013 at 11:35:03PM +0000, Kim, Milo wrote:

> Thanks for your opinion.
> My understanding is 'shared' enable pin means regulators are enabled/disabled
> at the same time.
> Any other case is possible?

So, clearly that's going to be the behaviour at the system level but the
consumers aren't going to know that.  If the consumer supports some of
the supplies being enabled and disabled separately then it will rely on
the regulator core reference counting to keep the supply enabled if
there are other reasons to do so.  This is how things would work if both
supplies came from the same regulator so I'd expect us to preserve the
same behaviour.

RE: [PATCH 2/3] regulator-core: update all enable GPIO state in _enable/disable

[Kim, Milo]

> From: Mark Brown [mailto:broonie@opensource.wolfsonmicro.com]
> 
> So, clearly that's going to be the behaviour at the system level but
> the
> consumers aren't going to know that.  If the consumer supports some of
> the supplies being enabled and disabled separately then it will rely on
> the regulator core reference counting to keep the supply enabled if
> there are other reasons to do so.  This is how things would work if
> both
> supplies came from the same regulator so I'd expect us to preserve the
> same behaviour.

OK, the consumer just cares about the regulator(s) which is(are) obtained.
However it doesn't show exact state of regulator in case that enable GPIOs are
shared.
For example, LDO1 and LDO3 can be enabled by same GPIO pin.
But there are two separate regulator consumers, CA and CB.

(1) CA gets LDO1 regulator.
(2) CB gets LDO3 regulator.
(3) CA enables LDO1 (the GPIO goes to HIGH state)
    : Both LDO1 and LDO3 are enabled physically.
(4) CB also enables LDO3
    : But no GPIO state is changed because GPIO is already HIGH state.
(5) CA disables LDO1. LDO3 also is turned off eventually.
(6) CB tries to get the state of LDO3 before disabling it,
regulator_is_enabled() returns 1, but real state is 0.

	if(!regulator_is_enabled(rdev))
		regulator_disable(rdev);

	Therefore CB never disables LDO3, use_count is always 1.

This patch solves this unmatched situation.

But based on regulator behavior, I don't need to care this situation either.
A regulator consumer handles their *own* regulator(s).
Regulator APIs may show unmatched value, but it can be handled in each consumer 
driver *separately*.
Therefore, we should not update 'ena_gpio_state' of other regulators.
This patch should be ignored.
Is that correct?

Best Regards,
Milo

Re: [PATCH 2/3] regulator-core: update all enable GPIO state in _enable/disable

[Mark Brown]

On Mon, Jan 14, 2013 at 02:44:08AM +0000, Kim, Milo wrote:

> > So, clearly that's going to be the behaviour at the system level but
> > the
> > consumers aren't going to know that.  If the consumer supports some of
> > the supplies being enabled and disabled separately then it will rely on
> > the regulator core reference counting to keep the supply enabled if
> > there are other reasons to do so.  This is how things would work if
> > both
> > supplies came from the same regulator so I'd expect us to preserve the
> > same behaviour.

> OK, the consumer just cares about the regulator(s) which is(are) obtained.
> However it doesn't show exact state of regulator in case that enable GPIOs are
> shared.

It would do if my suggestion that we use a shared state for the GPIO
were implemented!

> (6) CB tries to get the state of LDO3 before disabling it,
> regulator_is_enabled() returns 1, but real state is 0.

> 	if(!regulator_is_enabled(rdev))
> 		regulator_disable(rdev);

> 	Therefore CB never disables LDO3, use_count is always 1.

> This patch solves this unmatched situation.

This is just a buggy consumer - the consumer would also break if there
were a single regulator shared by two consumers.  There's almost no case
outside of bootstrapping where it makes sense to check if the regulator
is enabled for anything other than diagnostics.

> Regulator APIs may show unmatched value, but it can be handled in each consumer 
> driver *separately*.
> Therefore, we should not update 'ena_gpio_state' of other regulators.
> This patch should be ignored.
> Is that correct?

No, we need to reference count over all the regualtors sharing the
enable line.  Otherwise we won't pay attention to references held by
anything other than the regulator currently being enabled or disabled.

RE: [PATCH 2/3] regulator-core: update all enable GPIO state in _enable/disable

[Kim, Milo]

> > OK, the consumer just cares about the regulator(s) which is(are)
> obtained.
> > However it doesn't show exact state of regulator in case that enable
> GPIOs are
> > shared.
> 
> It would do if my suggestion that we use a shared state for the GPIO
> were implemented!
> 
> > (6) CB tries to get the state of LDO3 before disabling it,
> > regulator_is_enabled() returns 1, but real state is 0.
> 
> > 	if(!regulator_is_enabled(rdev))
> > 		regulator_disable(rdev);
> 
> > 	Therefore CB never disables LDO3, use_count is always 1.
> 
> > This patch solves this unmatched situation.
> 
> This is just a buggy consumer - the consumer would also break if there
> were a single regulator shared by two consumers.  There's almost no
> case
> outside of bootstrapping where it makes sense to check if the regulator
> is enabled for anything other than diagnostics.
> 
> > Regulator APIs may show unmatched value, but it can be handled in
> each consumer
> > driver *separately*.
> > Therefore, we should not update 'ena_gpio_state' of other regulators.
> > This patch should be ignored.
> > Is that correct?
> 
> No, we need to reference count over all the regualtors sharing the
> enable line.  Otherwise we won't pay attention to references held by
> anything other than the regulator currently being enabled or disabled.

Updated patch-set was sent recently.

  [PATCH v2 1/4] regulator-core: support shared enable GPIO concept
  https://lkml.org/lkml/2013/1/14/564

  [PATCH v2 2/4] regulator-core: manage enable GPIO list
  https://lkml.org/lkml/2013/1/14/565

  [PATCH v2 3/4] regulator-core: free requested GPIOs and manage the enable list
  https://lkml.org/lkml/2013/1/14/567

  [PATCH v2 4/4] lp8788-ldo: use ena_pin of regulator-core for external control
  https://lkml.org/lkml/2013/1/14/566

I'd like to get some feedback from you.
Thank you.

Best Regards,
Milo