[PATCH/RFC] regulator: Reverse the disable sequence in regulator_bulk_disable()

[PATCH/RFC] regulator: Reverse the disable sequence in regulator_bulk_disable()

[Sylwester Nawrocki]

Often there is a need for disabling a set of regulators in order opposite
to the enable order. Currently the function regulator_bulk_disable() walks
list of regulators in same order as regulator_bulk_enable(). This may cause
trouble, especially for devices with mixed analogue and digital circuits.
So reverse the disabling sequence of regulator_bulk_disable().
While at it, also correct the comment.

Cc: Jaroslav Kysela <perex@perex.cz>
Cc: Takashi Iwai <tiwai@suse.de>
Cc: Samuel Ortiz <sameo@linux.intel.com>
Cc: Steve Glendinning <steve.glendinning@smsc.com>
Cc: Richard Purdie <rpurdie@rpsys.net>
Cc: Timur Tabi <timur@freescale.com>
Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
The alternatives to directly modifying regulator_bulk_disable() could be:

 - re-implement it in modules that need the order reversed; it is not
   really helpful in practice since such code would have to be repeated
   in multiple modules;

 - create new function, e.g. regulator_bulk_disable_reversed() with the
   order reversed - not sure if it is not an overkill though;
---
 drivers/regulator/core.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index e9a83f8..67db4a6 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -2463,8 +2463,8 @@ EXPORT_SYMBOL_GPL(regulator_bulk_enable);
  * @return         0 on success, an errno on failure
  *
  * This convenience API allows consumers to disable multiple regulator
- * clients in a single API call.  If any consumers cannot be enabled
- * then any others that were disabled will be disabled again prior to
+ * clients in a single API call.  If any consumers cannot be disabled
+ * then any others that were disabled will be enabled again prior to
  * return.
  */
 int regulator_bulk_disable(int num_consumers,
@@ -2473,7 +2473,7 @@ int regulator_bulk_disable(int num_consumers,
 	int i;
 	int ret;
 
-	for (i = 0; i < num_consumers; i++) {
+	for (i = num_consumers - 1; i >= 0; --i) {
 		ret = regulator_disable(consumers[i].consumer);
 		if (ret != 0)
 			goto err;
@@ -2483,7 +2483,7 @@ int regulator_bulk_disable(int num_consumers,
 
 err:
 	pr_err("Failed to disable %s: %d\n", consumers[i].supply, ret);
-	for (--i; i >= 0; --i)
+	for (++i; i < num_consumers; ++i)
 		regulator_enable(consumers[i].consumer);
 
 	return ret;
-- 
1.7.8.3

Re: [PATCH/RFC] regulator: Reverse the disable sequence in regulator_bulk_disable()

[Mark Brown]

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

On Wed, Jan 25, 2012 at 12:35:38PM +0100, Sylwester Nawrocki wrote:

> Often there is a need for disabling a set of regulators in order opposite
> to the enable order. Currently the function regulator_bulk_disable() walks
> list of regulators in same order as regulator_bulk_enable(). This may cause
> trouble, especially for devices with mixed analogue and digital circuits.
> So reverse the disabling sequence of regulator_bulk_disable().
> While at it, also correct the comment.

So, I've applied this since it shouldn't do any harm and probably is
more what we meant to do but note that the bulk APIs don't make any
guarantees about ordering - in particular when we do the enable we fire
off a bunch of threads to bring the regulators up in parallel so the
ordering really is going to be unreliable as it depends on the scheduler
and the rates at which the various regulators ramp.  This is done so
that we can enable faster as we don't have to wait for each regulator to
ramp in series.

Whatever driver inspired you to submit this change is therefore probably
buggy or fragile at the minute - is it something that's in mainline or
next right now?

At some point I'd like to enhance things further so we can coalesce
register writes where multiple regulators have their enable bits in the
same register but that's a relatively large amount of work for a small
benefit unless we do something cute with regmap (and that is likely to
be too cute).

> The alternatives to directly modifying regulator_bulk_disable() could be:

>  - re-implement it in modules that need the order reversed; it is not
>    really helpful in practice since such code would have to be repeated
>    in multiple modules;

>  - create new function, e.g. regulator_bulk_disable_reversed() with the
>    order reversed - not sure if it is not an overkill though;

The third option is that where devices really care about the power
sequencing they should explicitly write that in code and only use the
bulk APIs where they don't care.  Typically this will mean either a few
sets of bulk supplies or a single set of bulk supplies and then some
number of individual supplies.  An awful lot of devices don't have any
sequencing constraints at all, apparently including most of those using
the API at present.

BTW, your CC list here is *really* random - please think more about who
you're CCing, it looks like you've done something with get_maintainer.

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

Re: [PATCH/RFC] regulator: Reverse the disable sequence in regulator_bulk_disable()

[Bill Gatliff]

Guys:

On Wed, Jan 25, 2012 at 12:57 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Wed, Jan 25, 2012 at 12:35:38PM +0100, Sylwester Nawrocki wrote:
>
>
> So, I've applied this since it shouldn't do any harm and probably is
> more what we meant to do but note that the bulk APIs don't make any
> guarantees about ordering - in particular when we do the enable we fire
> off a bunch of threads to bring the regulators up in parallel so the
> ordering really is going to be unreliable as it depends on the scheduler
> and the rates at which the various regulators ramp.  This is done so
> that we can enable faster as we don't have to wait for each regulator to
> ramp in series.

Presumably, then, the notifier mechanism will remain positive
confirmation that the associated regulator has indeed come up?  And
that multithreading thing will be interesting to implement given
parent-child relationships of regulator sources at times...

> Whatever driver inspired you to submit this change is therefore probably
> buggy or fragile at the minute - is it something that's in mainline or
> next right now?

I think he's probably dealing with a "VLOGIC <= VDD"-type requirement,
which isn't uncommon.  I'm dealing with it myself right now, actually.

In addition to the sequencing that imposes, it also has implications
for recalculating VLOGIC when VDD changes--- which means a notifier
somewhere.  I might be convinced that implementing this logic inside
the API itself might be of benefit, though I don't see right now how
to do it in a clear and generic way.

> At some point I'd like to enhance things further so we can coalesce
> register writes where multiple regulators have their enable bits in the
> same register but that's a relatively large amount of work for a small
> benefit unless we do something cute with regmap (and that is likely to
> be too cute).

That sounds awfully cute for such marginal benefit.  :)  I really like
the correct-by-inspection-ness of the current implementation.

>> The alternatives to directly modifying regulator_bulk_disable() could be:

I really don't have a problem with the disable order being the reverse
of the enable order, as it generally follows common sense for people
who work with e.g. multiple mutexes.  I kind of would have expected it
to be that way, actually.

> The third option is that where devices really care about the power
> sequencing they should explicitly write that in code and only use the
> bulk APIs where they don't care.

This is probably the best way, since it keeps the "care about" part in
front of the author's eyes and implements it in the chip-specific
code, where it is immune from changes in how the regulator API might
behave in the future.  Changes like the one we are discussing, for
example. :)


b.g.
-- 
Bill Gatliff
bgat@billgatliff.com

Re: [PATCH/RFC] regulator: Reverse the disable sequence in regulator_bulk_disable()

[Mark Brown]

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

On Wed, Jan 25, 2012 at 02:35:25PM +0100, Bill Gatliff wrote:
> On Wed, Jan 25, 2012 at 12:57 PM, Mark Brown
> > On Wed, Jan 25, 2012 at 12:35:38PM +0100, Sylwester Nawrocki wrote:

> > guarantees about ordering - in particular when we do the enable we fire
> > off a bunch of threads to bring the regulators up in parallel so the

> Presumably, then, the notifier mechanism will remain positive
> confirmation that the associated regulator has indeed come up?  And

The notifiers are called after the enable has finished, yes.

> that multithreading thing will be interesting to implement given
> parent-child relationships of regulator sources at times...

This multithreading thing is already implemented, and has been since
v3.1.  It's pretty trivial, it just fires up a bunch of threads in
parallel to call the underlying regulator_enable() API which already
needs locking to handle concurrent users anyway.

> > Whatever driver inspired you to submit this change is therefore probably
> > buggy or fragile at the minute - is it something that's in mainline or
> > next right now?

> I think he's probably dealing with a "VLOGIC <= VDD"-type requirement,
> which isn't uncommon.  I'm dealing with it myself right now, actually.

I'd imagine so, that's one of the common cases for requiring sequencing,
but if the driver is using the bulk APIs for this then it's buggy.

> In addition to the sequencing that imposes, it also has implications
> for recalculating VLOGIC when VDD changes--- which means a notifier
> somewhere.  I might be convinced that implementing this logic inside
> the API itself might be of benefit, though I don't see right now how
> to do it in a clear and generic way.

I can imagine a bulk set_voltage() that applied constraints and unwound
things when they went wrong but actually working out the values doesn't
feel generic.

> >> The alternatives to directly modifying regulator_bulk_disable() could be:

> I really don't have a problem with the disable order being the reverse
> of the enable order, as it generally follows common sense for people
> who work with e.g. multiple mutexes.  I kind of would have expected it
> to be that way, actually.

Right, that's why I applied the patch - I'm just saying we might not
actually wind up implementing that ordering due to the concurrency.

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

Re: [PATCH/RFC] regulator: Reverse the disable sequence in regulator_bulk_disable()

[Sylwester Nawrocki]

On 01/25/2012 12:57 PM, Mark Brown wrote:
> So, I've applied this since it shouldn't do any harm and probably is

Thank you!

> more what we meant to do but note that the bulk APIs don't make any
> guarantees about ordering - in particular when we do the enable we fire
> off a bunch of threads to bring the regulators up in parallel so the
> ordering really is going to be unreliable as it depends on the scheduler
> and the rates at which the various regulators ramp.  This is done so
> that we can enable faster as we don't have to wait for each regulator to
> ramp in series.

Yeah, I've noticed this API change recently.

> Whatever driver inspired you to submit this change is therefore probably
> buggy or fragile at the minute - is it something that's in mainline or
> next right now?

Yes, there are some drivers in mainline using the bulk API for which TRMs
recommend specific voltage supply enable/disable order, e.g.
drivers/media/video/s5k6aa.* or drivers/media/m5mols.

In fact I've had this patch for a quite long time hanging around in the
internal trees, long before the commit

 f21e0e81d81b649ad309cedc7226f1bed72982e0
 regulator: Do bulk enables of regulators in parallel

However it clearly indicates the order isn't guaranteed for the bulk APIs.

> At some point I'd like to enhance things further so we can coalesce
> register writes where multiple regulators have their enable bits in the
> same register but that's a relatively large amount of work for a small
> benefit unless we do something cute with regmap (and that is likely to
> be too cute).

Hmm, sounds like a good improvement which could also lead to lower power
consumption (since we reduce number of I2C/SPI transfers, etc.). But indeed
the benefits might hardly justify the amount of work needed :)

>> The alternatives to directly modifying regulator_bulk_disable() could be:
> 
>>  - re-implement it in modules that need the order reversed; it is not
>>    really helpful in practice since such code would have to be repeated
>>    in multiple modules;
> 
>>  - create new function, e.g. regulator_bulk_disable_reversed() with the
>>    order reversed - not sure if it is not an overkill though;
> 
> The third option is that where devices really care about the power
> sequencing they should explicitly write that in code and only use the
> bulk APIs where they don't care.  Typically this will mean either a few
> sets of bulk supplies or a single set of bulk supplies and then some
> number of individual supplies.  An awful lot of devices don't have any
> sequencing constraints at all, apparently including most of those using
> the API at present.

Yeah, I guess that's what I'm going to do - drop the bulk API usage to make
sure the order is right for drivers which really are sensitive.
Some of the devices I used to work with require explicit order of switching
all regulators, while some only care about timing relation of single supply
to a group of the remaining ones.

> BTW, your CC list here is *really* random - please think more about who
> you're CCing, it looks like you've done something with get_maintainer.

My apologies for that, especially to those not really involved..
Indeed, I've used get_maintainer on files which used the regulator API
calls in question. I'll try to do better job next time.


Regards,
--
Sylwester Nawrocki
Samsung Poland R&D Center