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

[Sylwester Nawrocki <s.nawrocki@samsung.com>]

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