Messages in this thread | | | Date | Wed, 25 Jan 2012 14:35:25 +0100 | Subject | Re: [PATCH/RFC] regulator: Reverse the disable sequence in regulator_bulk_disable() | From | 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |