[lkml]   [2012]   [Jan]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
    SubjectRe: [PATCH/RFC] regulator: Reverse the disable sequence in regulator_bulk_disable()
    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

    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.

    Sylwester Nawrocki
    Samsung Poland R&D Center

     \ /
      Last update: 2012-01-25 18:23    [W:0.025 / U:11.056 seconds]
    ©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site