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

[David Brownell]

On Monday 10 November 2008, Liam Girdwood wrote:
> On Sun, 2008-11-09 at 15:31 -0800, David Brownell wrote:
> > From: David Brownell <dbrownell@xxxxxxxxxxxxxxxxxxxxx>
> > 
> > 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/folders/print/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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/