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

[Mark Brown <broonie@opensource.wolfsonmicro.com>]

[-- 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 --]