[patch 2.6.28-rc3] regulator: add REGULATOR_MODE_OFF - Kernel

This is a discussion on [patch 2.6.28-rc3] regulator: add REGULATOR_MODE_OFF - Kernel ; From: David Brownell The regulator framework needs to expose an OFF mode for regulators with a single state machine. Example: TWL4030 regulators each have a status register exposing the current mode, which will be either ACTIVE, STANDBY, or OFF. But ...

+ Reply to Thread
Results 1 to 5 of 5

Thread: [patch 2.6.28-rc3] regulator: add REGULATOR_MODE_OFF

  1. [patch 2.6.28-rc3] regulator: add REGULATOR_MODE_OFF

    From: David Brownell

    The regulator framework needs to expose an OFF mode for regulators
    with a single state machine. Example: TWL4030 regulators each
    have a status register exposing the current mode, which will be
    either ACTIVE, STANDBY, or OFF. But regulator_ops.get_mode()
    currently has no way to report that third (OFF) mode.

    Add such an OFF mode, reporting it in the standard ways.

    In the spirit of the existing interface, disable() operations are
    still used to enter this mode:

    - regulator_set_mode() rejects it; drivers doing runtime power
    management must use regulator_disable().

    - the PM_SUSPEND_* notifier goes down the set_suspend_disable()
    path, not requiring set_suspend_mode() to handle OFF mode.

    This doesn't address any other enable/disable issues... like the
    way regulator_disable() clobbers state even on failure paths, or
    refcounting isssues (e.g. two clients enable, one disables, then
    the regulator should stay active.)

    Signed-off-by: David Brownell
    ---
    drivers/regulator/core.c | 16 +++++++++++++---
    include/linux/regulator/consumer.h | 4 ++++
    2 files changed, 17 insertions(+), 3 deletions(-)

    --- a/drivers/regulator/core.c
    +++ b/drivers/regulator/core.c
    @@ -272,6 +272,8 @@ static ssize_t regulator_opmode_show(str
    return sprintf(buf, "idle\n");
    case REGULATOR_MODE_STANDBY:
    return sprintf(buf, "standby\n");
    + case REGULATOR_MODE_OFF:
    + return sprintf(buf, "off\n");
    }
    return sprintf(buf, "unknown\n");
    }
    @@ -411,6 +413,8 @@ static ssize_t suspend_opmode_show(struc
    return sprintf(buf, "idle\n");
    case REGULATOR_MODE_STANDBY:
    return sprintf(buf, "standby\n");
    + case REGULATOR_MODE_OFF:
    + return sprintf(buf, "off\n");
    }
    return sprintf(buf, "unknown\n");
    }
    @@ -589,7 +593,7 @@ static int suspend_set_state(struct regu
    return -EINVAL;
    }

    - if (rstate->enabled)
    + if (rstate->enabled && rstate->mode != REGULATOR_MODE_OFF)
    ret = rdev->desc->ops->set_suspend_enable(rdev);
    else
    ret = rdev->desc->ops->set_suspend_disable(rdev);
    @@ -668,7 +672,9 @@ static void print_constraints(struct reg
    if (constraints->valid_modes_mask & REGULATOR_MODE_IDLE)
    count += sprintf(buf + count, "idle ");
    if (constraints->valid_modes_mask & REGULATOR_MODE_STANDBY)
    - count += sprintf(buf + count, "standby");
    + count += sprintf(buf + count, "standby ");
    + if (constraints->valid_modes_mask & REGULATOR_MODE_OFF)
    + count += sprintf(buf + count, "off ");

    printk(KERN_INFO "regulator: %s: %s\n", rdev->desc->name, buf);
    }
    @@ -1362,7 +1368,8 @@ EXPORT_SYMBOL_GPL(regulator_get_current_
    * @mode: operating mode - one of the REGULATOR_MODE constants
    *
    * Set regulator operating mode to increase regulator efficiency or improve
    - * regulation performance.
    + * regulation performance. You may not pass REGULATOR_MODE_OFF; to achieve
    + * that effect, call regulator_disable().
    *
    * NOTE: Regulator system constraints must be set for this regulator before
    * calling this function otherwise this call will fail.
    @@ -1372,6 +1379,9 @@ int regulator_set_mode(struct regulator
    struct regulator_dev *rdev = regulator->rdev;
    int ret;

    + if (mode == REGULATOR_MODE_OFF)
    + return -EINVAL;
    +
    mutex_lock(&rdev->mutex);

    /* sanity check */
    --- a/include/linux/regulator/consumer.h
    +++ b/include/linux/regulator/consumer.h
    @@ -68,6 +68,9 @@
    * the most noisy and may not be able to handle fast load
    * switching.
    *
    + * OFF Regulator is disabled. This mode may be observed from
    + * some hardware after invoking disable() primitives.
    + *
    * NOTE: Most regulators will only support a subset of these modes. Some
    * will only just support NORMAL.
    *
    @@ -78,6 +81,7 @@
    #define REGULATOR_MODE_NORMAL 0x2
    #define REGULATOR_MODE_IDLE 0x4
    #define REGULATOR_MODE_STANDBY 0x8
    +#define REGULATOR_MODE_OFF 0x10

    /*
    * Regulator notifier events.
    --
    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/

  2. Re: [patch 2.6.28-rc3] regulator: add REGULATOR_MODE_OFF

    On Sun, 2008-11-09 at 15:31 -0800, David Brownell wrote:
    > From: David Brownell
    >
    > The regulator framework needs to expose an OFF mode for regulators
    > with a single state machine. Example: TWL4030 regulators each
    > have a status register exposing the current mode, which will be
    > either ACTIVE, STANDBY, or OFF. But regulator_ops.get_mode()
    > currently has no way to report that third (OFF) mode.
    >


    OFF is currently not a regulator operating mode but is a regulator
    operating state (e.g. state is either ON or OFF). The modes define the
    ON (supplying power) operating modes supported by a regulator. I should
    probably add some more docs/comments here......

    I assume the TWL4030's ACTIVE and STANDBY modes supply power and
    probably all share the same register/bits with OFF (thus making it more
    tightly coupled in the hardware).

    The other two patches are fine. Would you be able to resend the first
    without the OFF mode patch changes.


    Thanks

    Liam


    --
    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/

  3. Re: [patch 2.6.28-rc3] regulator: add REGULATOR_MODE_OFF

    On Monday 10 November 2008, Liam Girdwood wrote:
    > On Sun, 2008-11-09 at 15:31 -0800, David Brownell wrote:
    > > From: David Brownell
    > >
    > > The regulator framework needs to expose an OFF mode for regulators
    > > with a single state machine. Example: TWL4030 regulators each
    > > have a status register exposing the current mode, which will be
    > > either ACTIVE, STANDBY, or OFF. But regulator_ops.get_mode()
    > > currently has no way to report that third (OFF) mode.

    >
    > OFF is currently not a regulator operating mode but is a regulator
    > operating state (e.g. state is either ON or OFF).


    The regulator itself supports exactly three states/modes.

    You seem to imply that the programming interface should be
    exposing four -- {ACTIVE, STANDBY } x { ON, OFF } -- which
    doesn't reflect how the hardware works.

    See below; the key conceptual problem in this interface is
    probably the assumption that the Linux CPU isn't sharing
    control over the regulator. So regulator_disable() can't
    imply REGULATOR_MODE_OFF ... another CPU may need to keep
    it in some other state.


    > The modes define the
    > ON (supplying power) operating modes supported by a regulator.
    > I should probably add some more docs/comments here......


    Seems to me more like this is a "fix the interface" case
    instead of a "document the problem" one. It's not that
    the implication was unclear ... but that it won't work.


    > I assume the TWL4030's ACTIVE and STANDBY modes supply power and
    > probably all share the same register/bits with OFF (thus making
    > it more tightly coupled in the hardware).


    It's *very* tightly coupled to the hardware. The regulator
    state (active/standby/off) is determined by a vote between
    three hardware request mechanisms ... the CPU running Linux
    only gets one vote. Have a look at the docs[1], if you dare.

    So for example when any of the three requestors asks for the
    regulator to go ACTIVE it will do so. This means you can have
    cases like:

    - One CPU (running Linux, say) asks to disable() the regulator
    * implemented by clearing that CPU's bit in a mask
    * is_enabled() tests that bit and says "no, not enabled"
    - Another CPU needs it active
    * request might be coupled to the nSLEEP2 signal
    * thus get_mode() will say it's ACTIVE

    So you see why enable/disable is orthogonal to MODE_OFF.

    It's true that it won't be OFF unless the Linux CPU is
    not requesting it ("disabled" its request) ... but the
    converse is false, because of the non-Linux requestor(s).


    > The other two patches are fine. Would you be able to resend the first
    > without the OFF mode patch changes.


    I could, but I'd rather get the interface problem resolved
    first. At this point, adding MODE_OFF is the only viable
    option on the table...

    - Dave

    [1] http://focus.ti.com/docs/prod/folder.../tps65950.html

    "TPS65950" is a mouthful, so it's easier to say TWL5030
    (equivalent part) or TWL4030 (predecessor part, which is
    in more developers' hands).

    The most relevant section of the doc seem to be chapter 5,
    pp. 221-390 ... yes, some Linux-capable SOCs are smaller
    and simpler chips; and no, I've not read it all either.
    You'd want the TRM, 9+ MBytes, for programming info.
    --
    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/

  4. Re: [patch 2.6.28-rc3] regulator: add REGULATOR_MODE_OFF

    On Mon, Nov 10, 2008 at 07:43:34AM -0800, David Brownell wrote:

    > The regulator itself supports exactly three states/modes.


    > You seem to imply that the programming interface should be
    > exposing four -- {ACTIVE, STANDBY } x { ON, OFF } -- which


    Yes, that's expected model.

    > doesn't reflect how the hardware works.


    Most hardware has a similar level of squashing in it and certainly the
    functional effect is always the same. It's done this way partly to
    allow a complete configuration to be done before enabling the regulator
    and partly because most clients don't need to worry about the modes used
    and just care about enabling and disabling - for most things the
    operating modes are fixed in the board constraints.

    > See below; the key conceptual problem in this interface is
    > probably the assumption that the Linux CPU isn't sharing
    > control over the regulator. So regulator_disable() can't
    > imply REGULATOR_MODE_OFF ... another CPU may need to keep
    > it in some other state.


    regulator_disable() needn't imply that the regulator will actually be
    off - regulator API already does internal reference counting for shared
    regulators and...

    > So for example when any of the three requestors asks for the
    > regulator to go ACTIVE it will do so. This means you can have
    > cases like:


    ....this sounds like the same thing done in hardware.

    > - One CPU (running Linux, say) asks to disable() the regulator
    > * implemented by clearing that CPU's bit in a mask
    > * is_enabled() tests that bit and says "no, not enabled"
    > - Another CPU needs it active
    > * request might be coupled to the nSLEEP2 signal
    > * thus get_mode() will say it's ACTIVE


    > So you see why enable/disable is orthogonal to MODE_OFF.


    Not really. The mode reflects the characteristics of the regulator when
    it is enabled (things like the quality of the output, efficiency and the
    amount of power that can be delivered) rather than the regulator being
    turned on or off. In both cases I'd expect that get_mode() would report
    ACTIVE.

    The issue you see here is a divergence between the software-requested
    state and the actual physical state of the regulator.

    > It's true that it won't be OFF unless the Linux CPU is
    > not requesting it ("disabled" its request) ... but the
    > converse is false, because of the non-Linux requestor(s).


    My first instinct here would be to have the software simply reflect the
    state requested by the CPU and ignore the other enable sources. My
    second instinct would be to have is_enabled() report the actual state
    and leave it at that. I'm having a hard time thinking of a hardware
    design that could tolerate too much uncoordinated control of the
    regulators.

    I'm also wondering if part of what we need to do is add separate out the
    reporting paths for the actual and requested status? At the minute we
    only report the actual status and there's no indication of the logical
    status which does create some confusion here.
    --
    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/

  5. Re: [patch 2.6.28-rc3] regulator: add REGULATOR_MODE_OFF

    On Monday 10 November 2008, Mark Brown wrote:
    > On Mon, Nov 10, 2008 at 07:43:34AM -0800, David Brownell wrote:
    >
    > > See below; the key conceptual problem in this interface is
    > > probably the assumption that the Linux CPU isn't sharing
    > > control over the regulator. So regulator_disable() can't


    Read that as regulator_ops.disable() ... although there are
    issues with regulator_disable() too. $SUBJECT patch relates
    to regulator_ops semantics, which of course bubble up.


    > > imply REGULATOR_MODE_OFF ... another CPU may need to keep
    > > it in some other state.

    >
    > regulator_disable() needn't imply that the regulator will actually be
    > off -


    Would you say that also for regulator_ops.disable() though?


    > regulator API already does internal reference counting for shared
    > regulators and...


    Very confusing refcounting. The naming convention is unusual too,
    and that's just the start of it:

    struct regulator_dev ... unique handle to the hardware, has
    some real refcounting (use_count), and is what shows
    up in /sys/class/regulator (vs. say .../regulator_dev).
    This class is internal to the regulator framework.

    struct regulator ... opaque client handle, wraps regulator_dev,
    but has no refcounting: just a boolean 'enabled' flag,
    and KERN_CRIT messaging if you try using enable/disable
    like you would with clocks or IRQs (N enables are fine,
    so long as they're eventually paired with N disables).
    Each "consumer" would normally have its own "regulator".

    Less surprising/confusing would be if regulator_{en,dis}able() did
    its own refcounting and called down to regulator_dev when changing
    a per-client refcount to/from zero. (Easy patch, for later.)

    And, hrm, kerneldoc for regulator_is_enabled() says it returns
    a refcount, not boolean; but the refcount is unavailable to the
    regulator_ops method returning that value.


    > > So for example when any of the three requestors asks for the
    > > regulator to go ACTIVE it will do so. This means you can have
    > > cases like:

    >
    > ...this sounds like the same thing done in hardware.


    Seems to me more like a three input OR gate. No counters.
    At least, if you ignore the additional arbitration between
    ACTIVE, STANDBY, and OFF modes. (Highest one wins...)


    > > - One CPU (running Linux, say) asks to disable() the regulator
    > > * implemented by clearing that CPU's bit in a mask
    > > * is_enabled() tests that bit and says "no, not enabled"
    > > - Another CPU needs it active
    > > * request might be coupled to the nSLEEP2 signal
    > > * thus get_mode() will say it's ACTIVE

    >
    > > So you see why enable/disable is orthogonal to MODE_OFF.

    >
    > Not really. The mode reflects the characteristics of the regulator when
    > it is enabled ...


    That answer doesn't make much sense to me; plus, kerneldoc
    for regulator_get_mode() -- which essentially just calls down
    to regulator_ops.get_mode() -- says it returns the "current"
    mode. NOT some potential future mode. Giving the "current"
    mode implies asking the hardware what it's doing right now.

    Consider also the scenario above, where Linux calls enable()
    and has requested STANDBY mode. The regulator will instead
    be in ACTIVE state; answering STANDBY seems quite wrong.
    (Just like answering STANDBY when it's really OFF...)


    > The issue you see here is a divergence between the software-requested
    > state and the actual physical state of the regulator.


    In a general sense, yes ... but this framework splits that
    state into a "mode" and an "enable" flag (called "state" in
    sysfs, which doesn't reduce confusion at all).


    > > It's true that it won't be OFF unless the Linux CPU is
    > > not requesting it ("disabled" its request) ... but the
    > > converse is false, because of the non-Linux requestor(s).

    >
    > My first instinct here would be to have the software simply reflect the
    > state requested by the CPU and ignore the other enable sources.


    Mine is to have get_mode() report the actual hardware state,
    matching kerneldoc ... and thus the $SUBJECT patch. Also, I
    have no set_mode(), and thus no mode "requested by the CPU"
    through this framework.


    > My
    > second instinct would be to have is_enabled() report the actual state
    > and leave it at that.


    But *which* actual state? I'd say that reporting this CPU's
    enable/request bit *is* reporting actual hardware state: the
    request line managed by regulator_ops.disable()/enable().


    > I'm having a hard time thinking of a hardware
    > design that could tolerate too much uncoordinated control of the
    > regulators.


    True; the coordination here is done in hardware. There
    are "scripts" that get downloaded into the hardware, and
    which update things on various hardware state changes.


    > I'm also wondering if part of what we need to do is add separate out the
    > reporting paths for the actual and requested status? At the minute we
    > only report the actual status and there's no indication of the logical
    > status which does create some confusion here.


    Makes sense. Record "requested_mode" in "struct regulator_dev"
    and expose a new sysfs attribute for it. Should I update
    the $SUBJECT patch to do that too?

    - Dave

    --
    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/

+ Reply to Thread