From: David Brownell <[email protected]>
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 <[email protected]>
---
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.
On Sun, 2008-11-09 at 15:31 -0800, David Brownell wrote:
> From: David Brownell <[email protected]>
>
> 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
On Monday 10 November 2008, Liam Girdwood wrote:
> On Sun, 2008-11-09 at 15:31 -0800, David Brownell wrote:
> > From: David Brownell <[email protected]>
> >
> > 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.
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.
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
On Mon, Nov 10, 2008 at 08:56:19PM -0800, David Brownell wrote:
> On Monday 10 November 2008, Mark Brown wrote:
> > > 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?
If we ignore regulator_force_disable() (which isn't used yet) for the
moment I'd actually say that in your case with non-software enables it's
reasonable for it to still be powered by some other part of the system.
Mainly because it's proably easier to just ignore the other enables than
it is to explain to the rest of the system that there are others it
can't know about.
> 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.)
Yeah, either way is fine for me - don't know if Liam has a strong
opinion. The main benefit of not doing it is that encourages people to
avoid consumers sharing the clients which causes problems when clients
share the regulator.
> 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.
Easiest to just update the documentation to reflect reality.
> > > 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...)
Logically that's what the regulator API is doing, it's just that
currently it doesn't support reconfiguration of shared regulators. If
you remember the pre-submission versions you looked at they had support
for computing the most restrictive voltage constraint from multiple
clients, this sounds like the same thing done in hardware for the
regulator mode.
> > > 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.
This is because you are viewing the mode as including the enabled state
while the regulator API views the mode as being largely orthogonal to
the operating mode. This is the key point, most of what you're saying
here seems to spring from this disconnect.
The view of the regulator API is that the operating configuration of the
regulator (including not only the mode but also the voltage or current)
is done separately to enabling or disabling it. For most clients the
configuration will be done statically by the machine driver, they will
never consider the configuration of the regulator at all and will simply
enable or disable it. This is partly because the configuration is often
system specific and partly because a large proportion of consumers
aren't interested in any more detail than that.
It is true that the configuration is largely irrelevant when a regulator
isn't enabled but this isn't unusual.
The kerneldoc should be updated to say "the mode currently configured by
Linux when the regulator is enabled" or similar, I guess. A similar
issue applies to the documentation for all the other configuration
readback functions. The expectation when writing this was that anything
software controlled would be fully software controlled.
> 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.
Depends who's asking, really. If you want to know what Linux is doing
for making operational decisions or software debugging then reporting
standby is reasonable - the fact that it's getting higher quality
regulation shouldn't upset it at all. If you want to know what the
hardware is doing it's less so.
> (Just like answering STANDBY when it's really OFF...)
This is the same issue with enable/mode separation.
> > 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
Right, as discussed previously.
> sysfs, which doesn't reduce confusion at all).
It's in there now, unfortunately.
> > > 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,
Again, this comes back to the mode/enable merging.
> matching kerneldoc ... and thus the $SUBJECT patch. Also, I
> have no set_mode(), and thus no mode "requested by the CPU"
> through this framework.
If you're not going to allow anything in Linux to configure the mode of
the regulator then the mode is only going to be useful for diagnostic
purposes (this is mostly what the readback is useful for anyway) so it's
all relatively academic.
> > 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 had meant the physical state of the regulator since you were very keen
on exposing that (as opposed to simply ignoring the other enables which
had been the first option).
> > 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.
Indeed. That is pretty much standard for these parts - you obviously
need configuration for initial power up at least and normally there will
be a set of alternative settings activated on suspend too. The unusual
thing is having something else controlling the power states while the OS
is active.
> > 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?
It should be a separate patch, I'd say.
Thinking about it I'm not sure if the hardware or logical state should
be the primary. In terms of debugging power consumption and so on the
physical state is probably the more important one but from the point of
view of Linux it's the logical state that matters most since that's what
Linux is actually doing (IYSWIM).
On Wednesday 12 November 2008, Mark Brown wrote:
> On Mon, Nov 10, 2008 at 08:56:19PM -0800, David Brownell wrote:
> > > 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?
>
> It should be a separate patch, I'd say.
So you think I should split my "v2" patch in two chunks?
One distinguishing requested-vs=actual mode, and the other
allowing the actual mode to include OFF. (Possibly by just
reporting mode 0 ...)
> Thinking about it I'm not sure if the hardware or logical state should
> be the primary. In terms of debugging power consumption and so on the
> physical state is probably the more important one but from the point of
> view of Linux it's the logical state that matters most since that's what
> Linux is actually doing (IYSWIM).
If there are both "requested opmode" and "opmode" attributes
in sysfs, I don't see how one would be "primary"!
The difference is that one needs to be reported by hardware,
and the other is trivially remembered by framework software.
- Dave
On Wed, 2008-11-12 at 11:25 +0000, Mark Brown wrote:
> On Mon, Nov 10, 2008 at 08:56:19PM -0800, David Brownell wrote:
> > On Monday 10 November 2008, Mark Brown wrote:
>
> > > > 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?
>
> If we ignore regulator_force_disable() (which isn't used yet) for the
> moment I'd actually say that in your case with non-software enables it's
> reasonable for it to still be powered by some other part of the system.
> Mainly because it's proably easier to just ignore the other enables than
> it is to explain to the rest of the system that there are others it
> can't know about.
>
> > 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.)
>
> Yeah, either way is fine for me - don't know if Liam has a strong
> opinion. The main benefit of not doing it is that encourages people to
> avoid consumers sharing the clients which causes problems when clients
> share the regulator.
>
Fwiw, the main design intention here was to have a 1:1 mapping between a
consumer device and a struct regulator so that we could easily store per
consumer power data (for mode switching, easier debug, sysfs) and avoid
any issues between sharing the clients. I'd be happy for this change as
long as we can keep the per consumer data.
Thanks
Liam
On Wed, Nov 12, 2008 at 01:42:35PM -0800, David Brownell wrote:
> On Wednesday 12 November 2008, Mark Brown wrote:
> > It should be a separate patch, I'd say.
> So you think I should split my "v2" patch in two chunks?
> One distinguishing requested-vs=actual mode, and the other
> allowing the actual mode to include OFF. (Possibly by just
> reporting mode 0 ...)
I would certainly keep OFF as a separate patch, yes.
> > Thinking about it I'm not sure if the hardware or logical state should
> > be the primary. In terms of debugging power consumption and so on the
> If there are both "requested opmode" and "opmode" attributes
> in sysfs, I don't see how one would be "primary"!
The one returned by the in-kernel get_mode() and reported as "opmode" is
what I'd think of as primary.
On Wednesday 12 November 2008, Liam Girdwood wrote:
> On Wed, 2008-11-12 at 11:25 +0000, Mark Brown wrote:
> > On Mon, Nov 10, 2008 at 08:56:19PM -0800, David Brownell wrote:
>
> > > 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.)
> >
> > Yeah, either way is fine for me - don't know if Liam has a strong
> > opinion. The main benefit of not doing it is that encourages people to
> > avoid consumers sharing the clients which causes problems when clients
> > share the regulator.
Not refcounting enables/disables gets to be a PITA though; that's
why the the IRQ and clock frameworks gave up on the not-counted
versions of their enable/disable calls a long time ago.
The typical problem scenario is that two parts of the same driver
have independent needs to make sure something is enabled. If it's
not OK to enable() something that's already enabled(), the driver
itself will need to reinvent a refcounting scheme.
That kind of refcount patch would be a net code shrink anyway. :)
> Fwiw, the main design intention here was to have a 1:1 mapping between a
> consumer device and a struct regulator
As in, an LCD display and its touchscreen might be in
the same power domain (regulator_dev, internal to the
framework) but would have different regulator structs.
In that case the most likely scenario would be two
drivers needing to enable() so it's not trying to
talk to controllers that are powered off. Voltage
would normally be fixed.
> so that we could easily store per
> consumer power data (for mode switching, easier debug, sysfs) and avoid
> any issues between sharing the clients. I'd be happy for this change as
> long as we can keep the per consumer data.
Right, I wasn't talking about changing that model.
- Dave
On Wednesday 12 November 2008, Mark Brown wrote:
> On Mon, Nov 10, 2008 at 08:56:19PM -0800, David Brownell wrote:
> > On Monday 10 November 2008, Mark Brown wrote:
>
> > > regulator_disable() needn't imply that the regulator will actually be
> > > off -
>
> > Would you say that also for regulator_ops.disable() though?
>
> If we ignore regulator_force_disable() (which isn't used yet) for the
> moment I'd actually say that in your case with non-software enables it's
> reasonable for it to still be powered by some other part of the system.
I'll read that as a "yes"...
... but I don't know what you mean by "non-software enables". Maybe you
mean "non-Linux"? One of the other inputs would normally be controlled
by software -- just not from Linux. Maybe it'd help if you think about
a system structured like:
- one ARM running Linux for UI, Internet access, etc
- another one running some realtime cell modem firmware
- both coordinating through a PMIC
So the regulator's hardware ENABLE signal is is fed by an OR gate from
both CPUs. Similarly with a signal that's high to put the regulator
in "normal" mode (else it goes into low-current "standby" when enabled).
Those two signals (enable, normal-mode) would be under software control.
Or hardware ... the key point is that some inputs aren't from Linux.
> Mainly because it's proably easier to just ignore the other enables than
> it is to explain to the rest of the system that there are others it
> can't know about.
My recently posted v2 of this patch solves this easily enough.
It suffices to have one method report the actual hardware state
(if that includes OFF as well as NORMAL, STANDBY, etc) ... that
seems to be what you mean by calling one value "primary". Others
can report what Linux has requested.
And having regulator_ops.get_mode() be the call to report that
hardware state is straightforward. Especially once you consider
that the actual state *will* be what Linux requested, except in
cases like: (a) shared regulators, like the scenario above, for
which I suspect twl4030 is the first case in Linux; (b) hardware
fault handling, like overcurrent/overtemp shutdown; and maybe
(c) "smart" regulators that switch modes automatically.
That seems to be a minimal-disruption change.
> > > > 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...)
>
> Logically that's what the regulator API is doing, it's just that
> currently it doesn't support reconfiguration of shared regulators.
Not entirely true. In this case the issue is exposing regulator
output state ... the regulator_ops suffice for inputs, which would
combine with other inputs to determine the actual regulator state
that's reported using by regulator_ops.get_mode().
> If
> you remember the pre-submission versions you looked at they had support
> for computing the most restrictive voltage constraint from multiple
> clients, this sounds like the same thing done in hardware for the
> regulator mode.
Yes ... the current "Dynamic Regulator Mode Switching" (DRMS) code
does some of that: the point of having different client handles
is that those handles can encapsulate different load constraints.
And then when the load changes, the requested mode can change.
> The view of the regulator API is that the operating configuration of the
> regulator (including not only the mode but also the voltage or current)
> is done separately to enabling or disabling it.
OK, but configuration inputs to the regulator drivers are
a different issue from how to expose regulator state.
It seems we've almost converged on the result of get_mode()
being that regulator state output, which is a function of more
than just the inputs from Linux.
> The expectation when writing this was that anything
> software controlled would be fully software controlled.
The problem is that the current code *also* ignores the fact
that hardware status is a function of more inputs than just
those from Linux. Like thermal shutdown from overcurrent.
The configuration inputs might be fine, but the output of
a regulator necessarily incorporates other inputs.
The top reason to display system state in sysfs is to support
troubleshooting ... and hiding the actual system state makes
that needlessly difficult.
- Dave
On Thu, Nov 13, 2008 at 11:40:15AM -0800, David Brownell wrote:
> ... but I don't know what you mean by "non-software enables". Maybe you
> mean "non-Linux"? One of the other inputs would normally be controlled
> by software -- just not from Linux. Maybe it'd help if you think about
> a system structured like:
Yes, I mean non-Linux. The fact that there may be some other software
running on the other hardware isn't terribly interesting here - it's an
implementation detail the user may not even be aware of.
> And having regulator_ops.get_mode() be the call to report that
> hardware state is straightforward. Especially once you consider
> that the actual state *will* be what Linux requested, except in
> cases like: (a) shared regulators, like the scenario above, for
> which I suspect twl4030 is the first case in Linux; (b) hardware
> fault handling, like overcurrent/overtemp shutdown; and maybe
> (c) "smart" regulators that switch modes automatically.
So. This does rather assume that the mode should be expected to change
in a software visible fashion and that this should be the main thing
reported. What the existing drivers are doing is making get_mode() the
inverse of set_mode().
Of the cases you have above I'd be surprised if there were any devices
that didn't implement (b) and (c) is provided to at least some extent by
the DC-DC convertors in the existing Wolfson drivers (one of the modes
they offer automatically adjusts the regulation mode based on load).
If reporting the full state via get_mode() the error reporting case in
particular would seem to be more involved than adding an off state -
you'd probably want an explicit out of regulation state, for example.
If a regulator goes out of regulation it's clearly neither off nor
functioning properly in the mode the hardware is configured for.
> Not entirely true. In this case the issue is exposing regulator
> output state ... the regulator_ops suffice for inputs, which would
> combine with other inputs to determine the actual regulator state
> that's reported using by regulator_ops.get_mode().
Right, if we assume that it reports the instantaneous hardware state.
> It seems we've almost converged on the result of get_mode()
> being that regulator state output, which is a function of more
> than just the inputs from Linux.
I'm not sure about that to be honest. From that point of view it'd seem
we'd also need to consider all the other configuration that the
regulator might have since there's no reason why that couldn't also be
overridden by other sources too.
> > The expectation when writing this was that anything
> > software controlled would be fully software controlled.
> The problem is that the current code *also* ignores the fact
> that hardware status is a function of more inputs than just
> those from Linux. Like thermal shutdown from overcurrent.
> The configuration inputs might be fine, but the output of
> a regulator necessarily incorporates other inputs.
That's all good and I agree - what you're saying that the only facility
in the existing API for reporting back the current hardware status is
the error notifier callbacks and that this is really rather too limited.
> The top reason to display system state in sysfs is to support
> troubleshooting ... and hiding the actual system state makes
> that needlessly difficult.
No argument here, either. What I'm not so sure about is that get_mode()
alone is the ideal way to report this. It feels to me like we wold be
better off exposing both physical and configured versions of all the
existing status for the regulators (not just mode) plus some additional
information for error conditions. This caters for any configuration
changes outside of software control and would let errors report without
masking any other physical state.
On Thursday 13 November 2008, Mark Brown wrote:
> On Thu, Nov 13, 2008 at 11:40:15AM -0800, David Brownell wrote:
>
> > And having regulator_ops.get_mode() be the call to report that
> > hardware state is straightforward. Especially once you consider
> > that the actual state *will* be what Linux requested, except in
> > cases like: (a) shared regulators, like the scenario above, for
> > which I suspect twl4030 is the first case in Linux; (b) hardware
> > fault handling, like overcurrent/overtemp shutdown; and maybe
> > (c) "smart" regulators that switch modes automatically.
And I almost forgot (d) system startup until set_mode() is called,
if indeed it's ever called.
> So. This does rather assume that the mode should be expected to change
> in a software visible fashion and that this should be the main thing
> reported. What the existing drivers are doing is making get_mode() the
> inverse of set_mode().
I looked at the regulator_ops methods, and what all but one of them
does is look at the hardware ... which isn't "the inverse". (That
exception always returns NORMAL, and isn't paired with a set_mode.)
> Of the cases you have above I'd be surprised if there were any devices
> that didn't implement (b) and (c) is provided to at least some extent by
> the DC-DC convertors in the existing Wolfson drivers (one of the modes
> they offer automatically adjusts the regulation mode based on load).
An autoadjust mode can't be requested using today's regulator calls,
though... REGULATOR_MODE_BEST? :)
> If reporting the full state via get_mode() the error reporting case in
> particular would seem to be more involved than adding an off state -
> you'd probably want an explicit out of regulation state, for example.
> If a regulator goes out of regulation it's clearly neither off nor
> functioning properly in the mode the hardware is configured for.
In that case it would be normal to return some error code. I always
like -ERANGE in such cases -- result is out-of-range. The sysfs code
will already report "unknown".
... though, hmm, I observe that regulator_ops calls returning modes
all return "unsigned". That's clearly broken; it prevents error
reporting. And in fact, the wm8350 get_mode() code returns -EINVAL...
> > Not entirely true. In this case the issue is exposing regulator
> > output state ... the regulator_ops suffice for inputs, which would
> > combine with other inputs to determine the actual regulator state
> > that's reported using by regulator_ops.get_mode().
>
> Right, if we assume that it reports the instantaneous hardware state.
Something sure needs to do that... and there's no point to even
having a get_mode() call if that's not what it does. If the goal
is to remember what Linux requested, the framework should have
been doing it.
> > It seems we've almost converged on the result of get_mode()
> > being that regulator state output, which is a function of more
> > than just the inputs from Linux.
>
> I'm not sure about that to be honest. From that point of view it'd seem
> we'd also need to consider all the other configuration that the
> regulator might have since there's no reason why that couldn't also be
> overridden by other sources too.
I'd be fine with an interface which only exposed hardware state,
and offered ways to change it ... but didn't try to show history
of requested changes. That's a very common interface idiom, and
in fact what most of the regulator stuff already does.
Most of the sysfs attributes now shown are from the "constraints"
structure ... and the nine (!) supporting suspend mode operation
don't relate to things that Linux could examine while suspended.
So your thought couldn't even apply there.
> > > The expectation when writing this was that anything
> > > software controlled would be fully software controlled.
>
> > The problem is that the current code *also* ignores the fact
> > that hardware status is a function of more inputs than just
> > those from Linux. Like thermal shutdown from overcurrent.
> > The configuration inputs might be fine, but the output of
> > a regulator necessarily incorporates other inputs.
>
> That's all good and I agree - what you're saying that the only facility
> in the existing API for reporting back the current hardware status is
> the error notifier callbacks and that this is really rather too limited.
Not exactly. I'm looking at the current methods, transferring
common idioms from other Linux driver contexts, and observing
a lot of get_*() methods, which would normally report hardware
status (else they'd have been part of the framework code, not
methods provided by the hardware-specific code).
And then applying that to some voltage regulators, I observe
no particular issue with get_voltage() or is_enabled() status
methods ... but get_mode() doesn't support relevant answers,
or even error returns. It seems clearly in need of fixes, and
the discussion with you has strengthened that impression.
This is all extremely young code, and barely used yet; this
is a common type of interface bug to find at that stage of any
framework's development. So I'm saying: need fixing soon.
> > The top reason to display system state in sysfs is to support
> > troubleshooting ... and hiding the actual system state makes
> > that needlessly difficult.
>
> No argument here, either. What I'm not so sure about is that get_mode()
> alone is the ideal way to report this.
On the other hand, it's sufficient in typical cases and even
in some not-so-typical ones. And simple. Platonic Ideals
are problematic to apply here, as in many pragmatic contexts.
> It feels to me like we wold be
> better off exposing both physical and configured versions of all the
> existing status for the regulators (not just mode) plus some additional
> information for error conditions.
That's altogether too complex for my taste.
If get_mode() is just used better, more like other hardware status
reporting calls, that will solve most of the real problems I've seen.
Other stuff could be added later of course, if needed.
> This caters for any configuration
> changes outside of software control and would let errors report without
> masking any other physical state.
Of the current set of status reporting calls, get_mode() is the
only one which can't report errors. I don't see any need to cater
any further than letting it report errors, and the actual status.
There may be *other* reasons to expand the framework, but I'm not
seeing the issues I've turned up as including such reasons.
- Dave
On Fri, Nov 14, 2008 at 05:15:14PM -0800, David Brownell wrote:
It looks like we have several issues here which are confusing each
other. From the point of view of control and reporting we have two
cases:
- The system taking non-Linux inputs into account when configuring the
regulators without altering the configuration done by Linux and where
the Linux configuration will take over automatically if the external
configuration is removed. For exammple, a register enable for a
regulator and a separate enable controlled elsewhere which are ored
together (like you've talked about for the TWL4030) or error handling
in the hardware.
- The Linux regulator configuration being changed outside of the
control of Linux. For example, the initial configuration on power on
or another processor in the system sharing access to the regulator
configuration registers.
plus how any divergence between the Linux and hardware state is
reported. In the first case there could two simultaneous states, that
in the configuration and the actual operating state.
Previously you had only talked explicitly about the former case but now
I see that you are talking about the latter as well. For those cases
then I do completely agree that the information returned by the status
calls should just change along with what the hardware is currently doing
(ideally with a notifier callback when it does). It's only in the case
where the Linux configuration will persist with no software intervention
that I have any disagreement.
There's also the issue of how we report a regulator which is turned off
- you wish to report this via adding a REGULATOR_MODE_OFF while I don't
feel that fits well with the model the code has and is not adequate for
reporting conditions that might cause the regulator to become disabled.
> On Thursday 13 November 2008, Mark Brown wrote:
> > On Thu, Nov 13, 2008 at 11:40:15AM -0800, David Brownell wrote:
> And I almost forgot (d) system startup until set_mode() is called,
> if indeed it's ever called.
Right, this is...
> > reported. What the existing drivers are doing is making get_mode() the
> > inverse of set_mode().
> I looked at the regulator_ops methods, and what all but one of them
> does is look at the hardware
...roughly what this is about.
> ... which isn't "the inverse". (That
It's the inverse of set_mode() in the sense that assuming Linux has full
control of the system a calling set_mode() with the result of get_mode()
produces no configuration change.
> > Of the cases you have above I'd be surprised if there were any devices
> > that didn't implement (b) and (c) is provided to at least some extent by
> > the DC-DC convertors in the existing Wolfson drivers (one of the modes
> > they offer automatically adjusts the regulation mode based on load).
> An autoadjust mode can't be requested using today's regulator calls,
> though... REGULATOR_MODE_BEST? :)
REGULATOR_MODE_NORMAL would likely be a good fit here - do something
sensible over a wide range of loads, probably at a bit lower efficiency
when operating at either of the extremes.
Given that the modes are a bitmask oring them together may make some
sense if explicit support were provided though as you say it's not
supported right now.
> > If reporting the full state via get_mode() the error reporting case in
> > particular would seem to be more involved than adding an off state -
> In that case it would be normal to return some error code. I always
> like -ERANGE in such cases -- result is out-of-range. The sysfs code
> will already report "unknown".
I suspect that's due to the use of a bitmask but I do agree that there
should at least be a facility to report a failure to determine the mode.
I'm not sure that regulation failures fit in there, though - I do feel
it's still useful to report the state the hardware is attempting to
operate in since that may well have a bearing on why the error has
occurred (for example, running a regulator in a mode when trying to
supply a load it can't support).
> ... though, hmm, I observe that regulator_ops calls returning modes
> all return "unsigned". That's clearly broken; it prevents error
> reporting. And in fact, the wm8350 get_mode() code returns -EINVAL...
Yes, something more like BUG() would be more appropriate there - that's
handling the case of regulators that just aren't supported.
> > Right, if we assume that it reports the instantaneous hardware state.
> Something sure needs to do that... and there's no point to even
> having a get_mode() call if that's not what it does. If the goal
> is to remember what Linux requested, the framework should have
> been doing it.
When I say the state Linux requested what I really mean is "the state
that is currently being requested in the hardware via the control
interface used by Linux" rather than the last value that was set via the
API. As I said at the head of the mail I really hadn't been considering
changes in this outside of the control of Linux, only changes to the
operating state of the regulator in addition to this. This means that...
> > I'm not sure about that to be honest. From that point of view it'd seem
> > we'd also need to consider all the other configuration that the
> > regulator might have since there's no reason why that couldn't also be
> > overridden by other sources too.
> I'd be fine with an interface which only exposed hardware state,
> and offered ways to change it ... but didn't try to show history
> of requested changes. That's a very common interface idiom, and
> in fact what most of the regulator stuff already does.
...I think we may actually (mostly?) agree here apart from the question
of what exactly a mode is?
> Most of the sysfs attributes now shown are from the "constraints"
> structure ... and the nine (!) supporting suspend mode operation
> don't relate to things that Linux could examine while suspended.
> So your thought couldn't even apply there.
Right, on the currently supported regulators the suspend mode
configuration is visible and configurable at runtime too - the hardware
provides separate registers the configuration from which is used when
suspended (which is then complicated by the framework handling the
multiple suspend modes Linux has). The scripts provided by the TWL4030
are rather different here and don't seem to fit quite so well.
> > That's all good and I agree - what you're saying that the only facility
> > in the existing API for reporting back the current hardware status is
> > the error notifier callbacks and that this is really rather too limited.
> Not exactly. I'm looking at the current methods, transferring
> common idioms from other Linux driver contexts, and observing
> a lot of get_*() methods, which would normally report hardware
> status (else they'd have been part of the framework code, not
> methods provided by the hardware-specific code).
As discussed above I agree with this - when I've been talking about the
configured values I have been talking about the values that are
currently configured in the hardware rather than the last values written
via the API. This can diverge from the physical output that regulator
is producing, which is what I have been referring to as the current
hardware state, but is still hardware status.
> And then applying that to some voltage regulators, I observe
> no particular issue with get_voltage() or is_enabled() status
> methods ... but get_mode() doesn't support relevant answers,
> or even error returns. It seems clearly in need of fixes, and
> the discussion with you has strengthened that impression.
So. Here we're back to the question of what a mode is. Hopefully what
I've said above has made my position clearer here. I really do think
you're trying to push too much into get_mode() and I still feel that
your off mode should be a regulator which has either been disabled or is
reporting an error condition.
> This is all extremely young code, and barely used yet; this
> is a common type of interface bug to find at that stage of any
> framework's development. So I'm saying: need fixing soon.
No argument about there being room for improvement here.
> > > The top reason to display system state in sysfs is to support
> > > troubleshooting ... and hiding the actual system state makes
> > > that needlessly difficult.
> > No argument here, either. What I'm not so sure about is that get_mode()
> > alone is the ideal way to report this.
> On the other hand, it's sufficient in typical cases and even
> in some not-so-typical ones. And simple. Platonic Ideals
> are problematic to apply here, as in many pragmatic contexts.
One other part of this is that modes are something that should be the
concern of machine drivers and a few specialist consumers and it should
be a warning flag if they appear elsewhere. If a standard client has to
consider modes that indicates to me that something is going wrong but
determining if a supply has failed feels like something a normal client
could reasonably want to do.
> > It feels to me like we wold be
> > better off exposing both physical and configured versions of all the
> > existing status for the regulators (not just mode) plus some additional
> > information for error conditions.
> That's altogether too complex for my taste.
Does what I've said above make what I'm saying seem more reasonable
here? As I said above, I think we're talking at cross purposes about
what the physical and configured statuses are and I think I've been
making some incorrect assumptions about what your hardware can do.
I'm not saying we should implement any additional reporting interfaces
without hardware that can use them.
> > This caters for any configuration
> > changes outside of software control and would let errors report without
> > masking any other physical state.
> Of the current set of status reporting calls, get_mode() is the
> only one which can't report errors. I don't see any need to cater
> any further than letting it report errors, and the actual status.
What is actual status, though? Is it what's configured in the hardware,
what the hardware is actually managing to do or something else?
First the easily addressed points:
On Friday 14 November 2008, Mark Brown wrote:
> On Fri, Nov 14, 2008 at 05:15:14PM -0800, David Brownell wrote:
> > On Thursday 13 November 2008, Mark Brown wrote:
> > > On Thu, Nov 13, 2008 at 11:40:15AM -0800, David Brownell wrote:
>
> > > If reporting the full state via get_mode() the error reporting case in
> > > particular would seem to be more involved than adding an off state -
>
> > In that case it would be normal to return some error code. I always
> > like -ERANGE in such cases -- result is out-of-range. The sysfs code
> > will already report "unknown".
>
> I suspect that's due to the use of a bitmask but I do agree that there
> should at least be a facility to report a failure to determine the mode.
>
> I'm not sure that regulation failures fit in there, though - I do feel
> it's still useful to report the state the hardware is attempting to
> operate in since that may well have a bearing on why the error has
> occurred (for example, running a regulator in a mode when trying to
> supply a load it can't support).
That would be an argument for a new "requested_mode" attribute ...
I already sent a patch with that change, since you seemed to like
that model. That argument makes me feel such an attribute might
actually be useful.
> > ... though, hmm, I observe that regulator_ops calls returning modes
> > all return "unsigned". That's clearly broken; it prevents error
> > reporting. And in fact, the wm8350 get_mode() code returns -EINVAL...
>
> Yes, something more like BUG() would be more appropriate there - that's
> handling the case of regulators that just aren't supported.
I'd never advise BUG() for such nonfatal driver-goofed-itself cases!
But that's beside the point.
When there is for example a communication error when talking to the
regulator, it should be easy to report that error. I can imagine for
example a regulator on a device connected via USB, where disconnection
would cause errors on all further requests. Even with I2C, errors are
not unknown.
> > > Right, if we assume that it reports the instantaneous hardware state.
>
> > Something sure needs to do that... and there's no point to even
> > having a get_mode() call if that's not what it does. If the goal
> > is to remember what Linux requested, the framework should have
> > been doing it.
>
> When I say the state Linux requested what I really mean is "the state
> that is currently being requested in the hardware via the control
> interface used by Linux" rather than the last value that was set via the
> API.
I was hoping the interface would become *simpler* (not harder) to
explain and understand ... :(
Why would those values be different, anyway? Shouldn't it be an
error if Linux requests something different from what it accepted
through regulator_ops.set_mode()?
> > This is all extremely young code, and barely used yet; this
> > is a common type of interface bug to find at that stage of any
> > framework's development. So I'm saying: need fixing soon.
>
> No argument about there being room for improvement here.
Good on that! :)
> > > This caters for any configuration
> > > changes outside of software control and would let errors report without
> > > masking any other physical state.
>
> > Of the current set of status reporting calls, get_mode() is the
> > only one which can't report errors. I don't see any need to cater
> > any further than letting it report errors, and the actual status.
>
> What is actual status, though? Is it what's configured in the hardware,
> what the hardware is actually managing to do or something else?
Status would be the output produced by the hardware based
on all its inputs ... including mode and enable requests
from Linux and potentially other agents, and the load that's
currently presented to the regulator hardware.
So with a hypothetical MODE_BEST, status might be "normal"
or "idle" in non-error cases. Or "off"; or "error", and
for now I'm not assuming much effort to report fault details.
- Dave
On Friday 14 November 2008, Mark Brown wrote:
> It looks like we have several issues here which are confusing each
> other. From the point of view of control and reporting we have two
> cases:
Succinctly: (control) inputs, of which Linux is not the exclusive
source; and (reporting) outputs, which for the sake of discussion I
think we're assuming are highly observable by software.
There are of course cases where the software observability is poor.
Consider a four pin regulator with Vin, Vout, GND, and enable ...
it might shut down on overcurrent, with no report to Linux. Even if
there were another pin for an overcurrent signal, it might not get
fed to a Linux IRQ handler. And if it has multiple operational modes,
five pins wouldn't seem to allow reporting which one was current.
> - The system taking non-Linux inputs into account when configuring the
> regulators without altering the configuration done by Linux ...
>
> - The Linux regulator configuration being changed outside of the
> control of Linux. For example, the initial configuration ...
>
> plus how any divergence between the Linux and hardware state is
> reported. In the first case there could two simultaneous states, that
> in the configuration and the actual operating state.
That still doesn't make sense to me. Inputs != outputs; they're
different, so **OF COURSE** they diverge ... inputs from "other"
sources, or observability gaps, just make that more obvious.
> Previously you had only talked explicitly about the former case but now
> I see that you are talking about the latter as well.
Mostly just to highlight that a model based exclusively on
inputs from Linux has deep output-shaped holes.
> There's also the issue of how we report a regulator which is turned off
> - you wish to report this via adding a REGULATOR_MODE_OFF while I don't
> feel that fits well with the model the code has and is not adequate for
> reporting conditions that might cause the regulator to become disabled.
Yet you still haven't suggested a solution you like better than
just having regulator_ops.get_mode() report the regulator's state...
Describing fault conditions is IMO an entirely different issue
from reporting the current regulator state.
> On Fri, Nov 14, 2008 at 05:15:14PM -0800, David Brownell wrote:
> > On Thursday 13 November 2008, Mark Brown wrote:
> > > On Thu, Nov 13, 2008 at 11:40:15AM -0800, David Brownell wrote:
>
> > And I almost forgot (d) system startup until set_mode() is called,
> > if indeed it's ever called.
>
> Right, this is...
>
> > > reported. What the existing drivers are doing is making get_mode() the
> > > inverse of set_mode().
>
> > I looked at the regulator_ops methods, and what all but one of them
> > does is look at the hardware
>
> ...roughly what this is about.
>
> > ... which isn't "the inverse". (That
>
> It's the inverse of set_mode() in the sense that assuming Linux has full
> control of the system a calling set_mode() with the result of get_mode()
> produces no configuration change.
But Linux *never* has full control. Inputs always include at
least pre-Linux hardware and firmware actions, and the load
presented to the regulator. And quite possibly more ... the
set_mode() input is only one factor in the regulator state.
A sensible control model for regulators could be exposed through
the current regulator_ops calls. But it requires a more useful
definition of what ops.get_mode() does ... as an output, not a
mirror to one of the inputs, and with more return value options.
> As I said at the head of the mail I really hadn't been considering
> changes in this outside of the control of Linux, only changes to the
> operating state of the regulator in addition to this. This means that...
>
> > > I'm not sure about that to be honest. From that point of view it'd seem
> > > we'd also need to consider all the other configuration that the
> > > regulator might have since there's no reason why that couldn't also be
> > > overridden by other sources too.
>
> > I'd be fine with an interface which only exposed hardware state,
> > and offered ways to change it ... but didn't try to show history
> > of requested changes. That's a very common interface idiom, and
> > in fact what most of the regulator stuff already does.
>
> ...I think we may actually (mostly?) agree here apart from the question
> of what exactly a mode is?
I hope we converge at some point.
- For ops.set_mode() it's clearly an input from Linux, applying
only when regulators are enabled. One glitch there is that most
of the regulators I happen to have worked with have one mode
configuration register, with OFF (disabled) as an option...
- For ops.get_mode() I'm still not clear whether you agree that
it's reporting an output -- the actual mode -- or else reporting
an input -- some "requested" mode. (OFF is again an issue.)
You seem to be going back and forth about whether get_mode() is
reporting a hardware output, or just a recorded input. (Which
may have been recorded in hardware, but is still just an input.)
I'd like to see agreement that it reports an OUTPUT ...
Minor sidelight: as I mentioned previously, I found out how
to implement ops.set_mode() on twl4030. You can think of it
as accessing one of three write-only mode-request registers
associated with that power resource. So it's not practical
to report that input, using ops.get_mode() or any other call.
However it's trivial to report the mode the hardware computes
from all its inputs (including those three registers).
> > Most of the sysfs attributes now shown are from the "constraints"
> > structure ... and the nine (!) supporting suspend mode operation
> > don't relate to things that Linux could examine while suspended.
> > So your thought couldn't even apply there.
>
> Right, on the currently supported regulators the suspend mode
> configuration is visible and configurable at runtime too - the hardware
> provides separate registers
I'd sure hope the framework isn't trying to be specific to,
say, that Wolfson hardware...
By the way: how have you envisioned putting those mode+enable
inputs into the right part of the PM sequence?
It'd seem to me that having the regulator clients handle this
stuff would always be correct -- and is in any case necessary
for runtime PM -- but if regulators get involved, goofing the
sequence would be easy: regulator off, then driver relying on
it has some work to do ... but it can't, since it's off.
> the configuration from which is used when
> suspended (which is then complicated by the framework handling the
> multiple suspend modes Linux has). The scripts provided by the TWL4030
> are rather different here and don't seem to fit quite so well.
That's putting it mildly. But on the plus side, nobody using
a twl4030 is ever likely to care about "system suspend states"
since that model is a poor match for very low-power hardware.
Truly low power designs leverage runtime power saving, in both
hardware and software, to the extent that the normal idle loop
may use what a PC would call suspend-to-RAM ... but wake into
a mode where pretty much every device is in a low power state,
except if it's in active use.
It's a ways off yet, but I catch myself wondering if we'll be
able to put regulators for MMC cards into low power mode when
they're idle. We'd want to know the card's internal controller
was done working though. If we can't know that, we'll have to
make sure nothing accidentally kicks in that mode...
> > And then applying that to some voltage regulators, I observe
> > no particular issue with get_voltage() or is_enabled() status
> > methods ... but get_mode() doesn't support relevant answers,
> > or even error returns. It seems clearly in need of fixes, and
> > the discussion with you has strengthened that impression.
>
> So. Here we're back to the question of what a mode is. Hopefully what
> I've said above has made my position clearer here. I really do think
> you're trying to push too much into get_mode() and I still feel that
> your off mode should be a regulator which has either been disabled or is
> reporting an error condition.
I already showed why that wouldn't be appropriate. That
"disabled" attribute is an input ... but regulator state
is an output.
Are you arguing that there should be some new regulator_ops
call to expose the actual regulator state? If so, then
what should happen to the get_mode() call? As I've noted,
if that's not reporting an output, it's a superfluous call.
> > > > The top reason to display system state in sysfs is to support
> > > > troubleshooting ... and hiding the actual system state makes
> > > > that needlessly difficult.
>
> > > No argument here, either. What I'm not so sure about is that get_mode()
> > > alone is the ideal way to report this.
>
> > On the other hand, it's sufficient in typical cases and even
> > in some not-so-typical ones. And simple. Platonic Ideals
> > are problematic to apply here, as in many pragmatic contexts.
>
> One other part of this is that modes are something that should be the
> concern of machine drivers and a few specialist consumers
Though it does imply something about what a "standard client"
might be. Something that's barely power-aware will just stick
to enable/disable. Something that's trying to avoid cutting a
day off the use cycle of a 1000 uAh battery might have lots of
reason to care about regulator modes... those clients would be
"standard" on certain kinds of hardware. :)
That said, such power-sensitive drivers will set_mode(), or
maybe set_optimimum_mode(), and not care about get_mode().
The primary use for get_mode() seems to be looking at what a
system is actually doing -- not driver support.
> and it should
> be a warning flag if they appear elsewhere. If a standard client has to
> consider modes that indicates to me that something is going wrong but
> determining if a supply has failed feels like something a normal client
> could reasonably want to do.
Given that's true, why are you pushing so hard against making
the ops.get_mode() reflect the actual regulator state?? (That
is, to reflect regulator output, not be a record of an input.)
> > > It feels to me like we wold be
> > > better off exposing both physical and configured versions of all the
> > > existing status for the regulators (not just mode) plus some additional
> > > information for error conditions.
>
> > That's altogether too complex for my taste.
>
> Does what I've said above make what I'm saying seem more reasonable
> here?
What do you mean by status then? mode, enabled, voltage min/max,
current min/max ... everything that can get into a set() interface?
I'm just trying to make sure the interfaces don't seem broken for
the hardware I'm currently working with. For that purpose, the
only real problem relates to get_mode(): it can't report one of
the three basic hardware output states (OFF), or for that matter
any communication errors talking to the regulator.
> As I said above, I think we're talking at cross purposes about
> what the physical and configured statuses are and I think I've been
> making some incorrect assumptions about what your hardware can do.
>
> I'm not saying we should implement any additional reporting interfaces
> without hardware that can use them.
I'm still trying to determine whether you expect get_mode() to
report a regulator input or output ... then
- if it's an input, why is it querying the hardware vs say just
accessing a regulator_dev field the driver initializes and then
the framework maintains ... and how to get an output;
- else if it's an output, how to report the missing modes like
OFF, error, and maybe FAULT.
It's seeming unduly difficult to get this interface sorted out...
- Dave
On Sun, Nov 16, 2008 at 02:58:16PM -0800, David Brownell wrote:
> On Friday 14 November 2008, Mark Brown wrote:
[Snipping much of this - I think we're in strong agreement except for
the main point left...]
> Describing fault conditions is IMO an entirely different issue from
> reporting the current regulator state.
Interesting... I'd agree that detail can go into a separate function
I think status reporting should at least include an error indication
since error conditions often mean that the operational status isn't
clear (eg, poor regulation could potentially mean that the output has
collapsed and looks a lot like it's disabled).
> - For ops.set_mode() it's clearly an input from Linux, applying only
> when regulators are enabled. One glitch there is that most of the
> regulators I happen to have worked with have one mode configuration
> register, with OFF (disabled) as an option...
My experience has been more that regulators either don't have any mode
configuration or clearly distinguish between mode configuration and
enabling. Clearly we do need to cater for both, though.
> - For ops.get_mode() I'm still not clear whether you agree that it's
> reporting an output -- the actual mode -- or else reporting an input
> -- some "requested" mode. (OFF is again an issue.)
> You seem to be going back and forth about whether get_mode() is
> reporting a hardware output, or just a recorded input. (Which may
> have been recorded in hardware, but is still just an input.) I'd like
An input. I think most of the back and forth comes because it's been
written from that point of view while you were talking about it as an
output so we weren't understanding each other.
> to see agreement that it reports an OUTPUT ...
No.
> > Right, on the currently supported regulators the suspend mode
> > configuration is visible and configurable at runtime too - the
> > hardware provides separate registers
> I'd sure hope the framework isn't trying to be specific to, say, that
> Wolfson hardware...
It was actually added to meet the needs of people working on a part from
another (National, IIRC) manufacturer. I can't say I've actually looked
in detail at what other parts do here.
> By the way: how have you envisioned putting those mode+enable inputs
> into the right part of the PM sequence?
The idea is that suspend states only come into play while software has
stopped running...
> It'd seem to me that having the regulator clients handle this stuff
> would always be correct -- and is in any case necessary for runtime PM
> -- but if regulators get involved, goofing the sequence would be easy:
> regulator off, then driver relying on it has some work to do ... but
> it can't, since it's off.
...and that while software is running the client drivers have full
control (within the constraints that have been set). It's expected that
these settings are activiated by hardware as the CPU stops running and
reset to the normal configuration when the CPU restarts.
> Are you arguing that there should be some new regulator_ops call to
> expose the actual regulator state? If so, then what should happen to
Yes, exactly - possibly multiple calls.
> the get_mode() call? As I've noted, if that's not reporting an
> output, it's a superfluous call.
I don't think so - if nothing else, it's useful for reading back the
status when it has not been configured by Linux (for example, when
constraints don't allow mode configuration so it's up to the bootloader
and hardware). It also allows us to read back what's happened if
something else overwrites the configuration (but I'd expect that to be
rare).
> > One other part of this is that modes are something that should be the
> > concern of machine drivers and a few specialist consumers
> Though it does imply something about what a "standard client"
> might be. Something that's barely power-aware will just stick
> to enable/disable. Something that's trying to avoid cutting a
> day off the use cycle of a 1000 uAh battery might have lots of
> reason to care about regulator modes... those clients would be
> "standard" on certain kinds of hardware. :)
Indeed, though there's quite a bit of fuzz in the mode definitions -
a more beefy regulator will have a different idea about what low power
is to one designed for very low power situations that can't scale up as
far and many regulators just don't have modes.
> That said, such power-sensitive drivers will set_mode(), or
> maybe set_optimimum_mode(), and not care about get_mode().
Yes, I'd expect they'd use get_mode() for at most bootstrapping only.
> > and it should
> > be a warning flag if they appear elsewhere. If a standard client has to
> > consider modes that indicates to me that something is going wrong but
> > determining if a supply has failed feels like something a normal client
> > could reasonably want to do.
> Given that's true, why are you pushing so hard against making
> the ops.get_mode() reflect the actual regulator state?? (That
> is, to reflect regulator output, not be a record of an input.)
A combination of seeing it as an input and not wanting to see clients
have to make decisions about regulator modes being suitable for them -
mode is often not an immediately useful peiece of information since it's
system specific what is appropriate. When the API says "mode" that says
"configuration" to me.
I'd see status reporting as a mostly orthogonal issue to having modes -
there's no need for a regulator to support both (I'd expect most with
modes to have status reporting but there's probably corner cases there)
and I think a useful interface for reporting status would be different
to one reporting modes.
> > Does what I've said above make what I'm saying seem more reasonable
> > here?
> What do you mean by status then? mode, enabled, voltage min/max,
> current min/max ... everything that can get into a set() interface?
For reading back the configuration, yes. For operational status
I guess anything that can be determined (I'm not saying this all has to
be in one call).
> I'm just trying to make sure the interfaces don't seem broken for
> the hardware I'm currently working with. For that purpose, the
> only real problem relates to get_mode(): it can't report one of
> the three basic hardware output states (OFF), or for that matter
> any communication errors talking to the regulator.
For hardware like that I'd have a disabled regulator either report
whatever would be set when the regulator is enabled or report nothing.
The former is more what the interface expects but a bit odd.
Communication errors would go under status reporting - at the minute the
only thing for that is to fail configuration operations and/or raise a
notification.
> I'm still trying to determine whether you expect get_mode() to
> report a regulator input or output ... then
> - if it's an input, why is it querying the hardware vs say just
> accessing a regulator_dev field the driver initializes and then
> the framework maintains ... and how to get an output;
Mostly just because it's been easier to query it when needed. The
boostrapping case is an important one (most regulators will have their
configuration left alone at runtime and just enable/disable) and it
avoids needing to consider if these things may actually be volatile. If
it were causing performance, locking or similar issues we'd probably go
to storing it in the class. The same considerations apply to all the
other configuration readback.
As to how to get an output, we need to add a new API for that.
> It's seeming unduly difficult to get this interface sorted out...
It's the input/output thing which is a massive conceptual mismatch and
wasn't immediately apparent since it was obscured by the focus on shared
control.
On Sunday 16 November 2008, Mark Brown wrote:
> > Are you arguing that there should be some new regulator_ops call to
> > expose the actual regulator state? ?If so, then what should happen to
>
> Yes, exactly - possibly multiple calls.
OK, so I finally got back to this issue. I'll post a patch
with such a separate call in a moment ... just a single status
value, sufficient for the need I observe, and only exposed
through sysfs since it's more useful just now as a kind of
introspection. Example: troubleshooting, as we previously
discussed; or, I can see some regulators that are wrongly
enabled, primarily because of bootloader goofage.
That raises an issue: how can Linux get such regulators to
turn off? Clock frameworks have the same issue, and they
tend to resolve this with a SoC-specific Kconfig option to
disable unused clocks (in a late_initcall, after everthing
has had a chance to start up). That conserves power.
I'm thinking it'd be worth having a similar Kconfig option
for regulators too. It could kick in regulator core code to
call regulator_ops.disable() whenever regulator_dev.use_count
is zero, possibly warning if !regulator_ops.is_disabled().
(Handling constraints.always_on too.)
Comments?
- Dave
From: David Brownell <[email protected]>
Based on previous LKML discussions:
* Update docs for regulator sysfs class attributes to highlight
the fact that all current attributes are intended to be control
inputs, including notably "state" and "opmode" which previously
implied otherwise.
* Define a new regulator driver get_status() method, which is the
first method reporting regulator outputs instead of inputs.
It can report on/off and error status; or instead of simply
"on", report the actual operating mode.
For the moment, this is a sysfs-only interface, not accessible to
regulator clients. Such clients can use the current notification
interfaces to detect errors, if the regulator reports them.
Signed-off-by: David Brownell <[email protected]>
---
Documentation/ABI/testing/sysfs-class-regulator | 57 ++++++++++++++++++----
drivers/regulator/core.c | 46 +++++++++++++++++
include/linux/regulator/driver.h | 17 ++++++
3 files changed, 111 insertions(+), 9 deletions(-)
--- a/Documentation/ABI/testing/sysfs-class-regulator
+++ b/Documentation/ABI/testing/sysfs-class-regulator
@@ -4,8 +4,8 @@ KernelVersion: 2.6.26
Contact: Liam Girdwood <[email protected]>
Description:
Some regulator directories will contain a field called
- state. This reports the regulator enable status, for
- regulators which can report that value.
+ state. This reports the regulator enable control, for
+ regulators which can report that input value.
This will be one of the following strings:
@@ -14,16 +14,54 @@ Description:
'unknown'
'enabled' means the regulator output is ON and is supplying
- power to the system.
+ power to the system (assuming no error prevents it).
'disabled' means the regulator output is OFF and is not
- supplying power to the system..
+ supplying power to the system (unless some non-Linux
+ control has enabled it).
'unknown' means software cannot determine the state, or
the reported state is invalid.
NOTE: this field can be used in conjunction with microvolts
- and microamps to determine regulator output levels.
+ or microamps to determine configured regulator output levels.
+
+
+What: /sys/class/regulator/.../status
+Description:
+ Some regulator directories will contain a field called
+ "status". This reports the current regulator status, for
+ regulators which can report that output value.
+
+ This will be one of the following strings:
+
+ off
+ on
+ error
+ fast
+ normal
+ idle
+ standby
+
+ "off" means the regulator is not supplying power to the
+ system.
+
+ "on" means the regulator is supplying power to the system,
+ and the regulator can't report a detailed operation mode.
+
+ "error" indicates an out-of-regulation status such as being
+ disabled due to thermal shutdown, or voltage being unstable
+ because of problems with the input power supply.
+
+ "fast", "normal", "idle", and "standby" are all detailed
+ regulator operation modes (described elsewhere). They
+ imply "on", but provide more detail.
+
+ Note that regulator status is a function of many inputs,
+ not limited to control inputs from Linux. For example,
+ the actual load presented may trigger "error" status; or
+ a regulator may be enabled by another user, even though
+ Linux did not enable it.
What: /sys/class/regulator/.../type
@@ -58,7 +96,7 @@ Description:
Some regulator directories will contain a field called
microvolts. This holds the regulator output voltage setting
measured in microvolts (i.e. E-6 Volts), for regulators
- which can report that voltage.
+ which can report the control input for voltage.
NOTE: This value should not be used to determine the regulator
output voltage level as this value is the same regardless of
@@ -73,7 +111,7 @@ Description:
Some regulator directories will contain a field called
microamps. This holds the regulator output current limit
setting measured in microamps (i.e. E-6 Amps), for regulators
- which can report that current.
+ which can report the control input for a current limit.
NOTE: This value should not be used to determine the regulator
output current level as this value is the same regardless of
@@ -87,7 +125,7 @@ Contact: Liam Girdwood <[email protected]
Description:
Some regulator directories will contain a field called
opmode. This holds the current regulator operating mode,
- for regulators which can report it.
+ for regulators which can report that control input value.
The opmode value can be one of the following strings:
@@ -101,7 +139,8 @@ Description:
NOTE: This value should not be used to determine the regulator
output operating mode as this value is the same regardless of
- whether the regulator is enabled or disabled.
+ whether the regulator is enabled or disabled. A "status"
+ attribute may be available to determine the actual mode.
What: /sys/class/regulator/.../min_microvolts
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -312,6 +312,47 @@ static ssize_t regulator_state_show(stru
}
static DEVICE_ATTR(state, 0444, regulator_state_show, NULL);
+static ssize_t regulator_status_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct regulator_dev *rdev = dev_get_drvdata(dev);
+ int status;
+ char *label;
+
+ status = rdev->desc->ops->get_status(rdev);
+ if (status < 0)
+ return status;
+
+ switch (status) {
+ case REGULATOR_STATUS_OFF:
+ label = "off";
+ break;
+ case REGULATOR_STATUS_ON:
+ label = "on";
+ break;
+ case REGULATOR_STATUS_ERROR:
+ label = "error";
+ break;
+ case REGULATOR_STATUS_FAST:
+ label = "fast";
+ break;
+ case REGULATOR_STATUS_NORMAL:
+ label = "normal";
+ break;
+ case REGULATOR_STATUS_IDLE:
+ label = "idle";
+ break;
+ case REGULATOR_STATUS_STANDBY:
+ label = "standby";
+ break;
+ default:
+ return -ERANGE;
+ }
+
+ return sprintf(buf, "%s\n", label);
+}
+static DEVICE_ATTR(status, 0444, regulator_status_show, NULL);
+
static ssize_t regulator_min_uA_show(struct device *dev,
struct device_attribute *attr, char *buf)
{
@@ -1744,6 +1785,11 @@ static int add_regulator_attributes(stru
if (status < 0)
return status;
}
+ if (ops->get_status) {
+ status = device_create_file(dev, &dev_attr_status);
+ if (status < 0)
+ return status;
+ }
/* some attributes are type-specific */
if (rdev->desc->type == REGULATOR_CURRENT) {
--- a/include/linux/regulator/driver.h
+++ b/include/linux/regulator/driver.h
@@ -21,6 +21,17 @@
struct regulator_dev;
struct regulator_init_data;
+enum regulator_status {
+ REGULATOR_STATUS_OFF,
+ REGULATOR_STATUS_ON,
+ REGULATOR_STATUS_ERROR,
+ /* fast/normal/idle/standby are flavors of "on" */
+ REGULATOR_STATUS_FAST,
+ REGULATOR_STATUS_NORMAL,
+ REGULATOR_STATUS_IDLE,
+ REGULATOR_STATUS_STANDBY,
+};
+
/**
* struct regulator_ops - regulator operations.
*
@@ -46,6 +57,12 @@ struct regulator_ops {
int (*set_mode) (struct regulator_dev *, unsigned int mode);
unsigned int (*get_mode) (struct regulator_dev *);
+ /* report regulator status ... most other accessors report
+ * control inputs, this reports results of combining inputs
+ * from Linux (and other sources) with the actual load.
+ */
+ int (*get_status)(struct regulator_dev *);
+
/* get most efficient regulator operating mode for load */
unsigned int (*get_optimum_mode) (struct regulator_dev *, int input_uV,
int output_uV, int load_uA);
On Wed, 2009-01-14 at 23:03 -0800, David Brownell wrote:
> From: David Brownell <[email protected]>
>
> Based on previous LKML discussions:
>
> * Update docs for regulator sysfs class attributes to highlight
> the fact that all current attributes are intended to be control
> inputs, including notably "state" and "opmode" which previously
> implied otherwise.
>
> * Define a new regulator driver get_status() method, which is the
> first method reporting regulator outputs instead of inputs.
> It can report on/off and error status; or instead of simply
> "on", report the actual operating mode.
>
> For the moment, this is a sysfs-only interface, not accessible to
> regulator clients. Such clients can use the current notification
> interfaces to detect errors, if the regulator reports them.
>
> Signed-off-by: David Brownell <[email protected]>
> ---
Applied.
Thanks
Liam
")
Fcc: +sent-mail
On Wed, Jan 14, 2009 at 11:03:09PM -0800, David Brownell wrote:
> introspection. Example: troubleshooting, as we previously
> discussed; or, I can see some regulators that are wrongly
> enabled, primarily because of bootloader goofage.
> That raises an issue: how can Linux get such regulators to
> turn off? Clock frameworks have the same issue, and they
> tend to resolve this with a SoC-specific Kconfig option to
> disable unused clocks (in a late_initcall, after everthing
> has had a chance to start up). That conserves power.
That's something that should be done by constraints - add a new flag
that can be set.
> I'm thinking it'd be worth having a similar Kconfig option
> for regulators too. It could kick in regulator core code to
> call regulator_ops.disable() whenever regulator_dev.use_count
> is zero, possibly warning if !regulator_ops.is_disabled().
> (Handling constraints.always_on too.)
> Comments?
Given the model the API has of not doing anything unless explicitly
told to I'd not expect to see this as a Kconfig option but rather as
something enabled per regulator or at least per board.
The warning would be a bit interesting; ideally there should be one
since it's probably an indication that something is wrong but there
are probably going to be use cases for it that can't be covered by
always_on, not that I can think of any right now.
")
Fcc: +sent-mail
On Wed, Jan 14, 2009 at 11:03:17PM -0800, David Brownell wrote:
> * Define a new regulator driver get_status() method, which is the
> first method reporting regulator outputs instead of inputs.
> It can report on/off and error status; or instead of simply
> "on", report the actual operating mode.
> For the moment, this is a sysfs-only interface, not accessible to
> regulator clients. Such clients can use the current notification
> interfaces to detect errors, if the regulator reports them.
It's useful for bootstrapping purposes - the notifiers will only report
changes. Not that this is something that should happen on a regular
basis so I'd be surprised if there were much demand.
> Signed-off-by: David Brownell <[email protected]>
Acked-by: Mark Brown <[email protected]>
but...
> +enum regulator_status {
> + REGULATOR_STATUS_OFF,
> + REGULATOR_STATUS_ON,
> + REGULATOR_STATUS_ERROR,
> + /* fast/normal/idle/standby are flavors of "on" */
> + REGULATOR_STATUS_FAST,
> + REGULATOR_STATUS_NORMAL,
> + REGULATOR_STATUS_IDLE,
> + REGULATOR_STATUS_STANDBY,
> +};
> +
> + /* report regulator status ... most other accessors report
> + * control inputs, this reports results of combining inputs
> + * from Linux (and other sources) with the actual load.
> + */
> + int (*get_status)(struct regulator_dev *);
> +
...this needs kerneldoc adding.
On Thu, 2009-01-15 at 12:40 +0000, Mark Brown wrote:
> but...
>
> > +enum regulator_status {
> > + REGULATOR_STATUS_OFF,
> > + REGULATOR_STATUS_ON,
> > + REGULATOR_STATUS_ERROR,
> > + /* fast/normal/idle/standby are flavors of "on" */
> > + REGULATOR_STATUS_FAST,
> > + REGULATOR_STATUS_NORMAL,
> > + REGULATOR_STATUS_IDLE,
> > + REGULATOR_STATUS_STANDBY,
> > +};
> > +
>
> > + /* report regulator status ... most other accessors report
> > + * control inputs, this reports results of combining inputs
> > + * from Linux (and other sources) with the actual load.
> > + */
> > + int (*get_status)(struct regulator_dev *);
> > +
>
> ...this needs kerneldoc adding.
Please send a separate patch for the kerneldoc as I've already applied.
Thanks
Liam
On Thursday 15 January 2009, Liam Girdwood wrote:
>
> > > +???/* report regulator status ... most other accessors report
> > > +??? * control inputs, this reports results of combining inputs
> > > +??? * from Linux (and other sources) with the actual load.
> > > +??? */
> > > +???int (*get_status)(struct regulator_dev *);
> > > +
> >
> > ...this needs kerneldoc adding.
>
> Please send a separate patch for the kerneldoc as I've already applied.
So that will be the first member of "struct regulator_ops" to
grow kerneldoc ... out of a total of fifteen (now) members.
Hmm...
Maybe I'm not understanding what's meant by "kerneldoc adding".
Perhaps it's nothing more than a sentence resembling "returns
REGULATOR_STATUS_* code or negative errno", and not real
kerneldoc?
- Dave
On Thu, Jan 15, 2009 at 07:35:29AM -0800, David Brownell wrote:
> On Thursday 15 January 2009, Liam Girdwood wrote:
> > Please send a separate patch for the kerneldoc as I've already applied.
> So that will be the first member of "struct regulator_ops" to
> grow kerneldoc ... out of a total of fifteen (now) members.
> Hmm...
> Maybe I'm not understanding what's meant by "kerneldoc adding".
> Perhaps it's nothing more than a sentence resembling "returns
> REGULATOR_STATUS_* code or negative errno", and not real
> kerneldoc?
No, real kerneldoc - this was added recently, check current mainline
(eg, commit c8e7e4640facbe99d10a6e262523b25be129b9b9). I'm not saying
it's great but it's there.
--
"You grabbed my hand and we fell into it, like a daydream - or a fever."
On Thursday 15 January 2009, Mark Brown wrote:
> No, real kerneldoc - this was added recently, check current mainline
> (eg, commit c8e7e4640facbe99d10a6e262523b25be129b9b9). ?I'm not saying
> it's great but it's there.
Ah, I see. Makes sense now -- I just didn't think this routine
should have been the first! ;)
Will do. Gotta update some trees first.
- Dave
On Thursday 15 January 2009, Liam Girdwood wrote:
>
> > ...this needs kerneldoc adding.
>
> Please send a separate patch for the kerneldoc as I've already applied.
Appended....
========== CUT HERE
From: David Brownell <[email protected]>
Subject: regulator: get_status() grows kerneldoc
Add kerneldoc for the new get_status() message. Fix the existing
kerneldoc for that struct in two ways:
(a) Syntax, making sure parameter descriptions immediately
follow the one-line struct description and that the first
blank lines is before any more expansive description;
(b) Presentation for a few points, to highlight the fact that
the previous "get" methods exist only to report the current
configuration, not to display actual status.
Signed-off-by: David Brownell <[email protected]>
---
include/linux/regulator/driver.h | 22 ++++++++++------------
1 file changed, 10 insertions(+), 12 deletions(-)
--- a/include/linux/regulator/driver.h
+++ b/include/linux/regulator/driver.h
@@ -34,26 +34,20 @@ enum regulator_status {
/**
* struct regulator_ops - regulator operations.
- *
- * This struct describes regulator operations which can be implemented by
- * regulator chip drivers.
- *
- * @enable: Enable the regulator.
- * @disable: Disable the regulator.
+ * @enable: Configure the regulator as enabled.
+ * @disable: Configure the regulator as disabled.
* @is_enabled: Return 1 if the regulator is enabled, 0 otherwise.
- *
* @set_voltage: Set the voltage for the regulator within the range specified.
* The driver should select the voltage closest to min_uV.
* @get_voltage: Return the currently configured voltage for the regulator.
- *
* @set_current_limit: Configure a limit for a current-limited regulator.
- * @get_current_limit: Get the limit for a current-limited regulator.
- *
+ * @get_current_limit: Get the configured limit for a current-limited regulator.
* @set_mode: Set the operating mode for the regulator.
- * @get_mode: Get the current operating mode for the regulator.
+ * @get_mode: Get the configured operating mode for the regulator.
+ * @get_status: Return actual (not as-configured) status of regulator, as a
+ * REGULATOR_STATUS value (or negative errno)
* @get_optimum_mode: Get the most efficient operating mode for the regulator
* when running with the specified parameters.
- *
* @set_suspend_voltage: Set the voltage for the regulator when the system
* is suspended.
* @set_suspend_enable: Mark the regulator as enabled when the system is
@@ -62,6 +56,9 @@ enum regulator_status {
* suspended.
* @set_suspend_mode: Set the operating mode for the regulator when the
* system is suspended.
+ *
+ * This struct describes regulator operations which can be implemented by
+ * regulator chip drivers.
*/
struct regulator_ops {
@@ -86,6 +83,7 @@ struct regulator_ops {
/* report regulator status ... most other accessors report
* control inputs, this reports results of combining inputs
* from Linux (and other sources) with the actual load.
+ * returns REGULATOR_STATUS_* or negative errno.
*/
int (*get_status)(struct regulator_dev *);
On Thu, Jan 15, 2009 at 10:11:51AM -0800, David Brownell wrote:
> Appended....
Acked-by: Mark Brown <[email protected]>
On Thursday 15 January 2009, Mark Brown wrote:
> > That raises an issue: ?how can Linux get such regulators to
> > turn off? ?Clock frameworks have the same issue, and they
> > tend to resolve this with a SoC-specific Kconfig option to
> > disable unused clocks (in a late_initcall, after everthing
> > has had a chance to start up). ?That conserves power.
>
> That's something that should be done by constraints - add a new flag
> that can be set.
That doesn't make any sense to me, any more than
adding a per-clock flag would.
> > I'm thinking it'd be worth having a similar Kconfig option
> > for regulators too. ?It could kick in regulator core code to
> > call regulator_ops.disable() whenever regulator_dev.use_count
> > is zero, possibly warning if !regulator_ops.is_disabled().
> > (Handling constraints.always_on too.)
>
> > Comments?
>
> Given the model the API has of not doing anything unless explicitly
> told to
... e.g. by CONFIG_REGULATOR_DISABLE_UNUSED being explicitly
set, indicating that the regulator support for that platform
(and its drivers) is complete enough that it can safely paper
over such early init issues.
> I'd not expect to see this as a Kconfig option but rather as
> something enabled per regulator or at least per board.
Why not, though? That works well with clocks, once the
drivers are debugged. Having dozens of fiddly little knobs
is not an advantage.
> The warning would be a bit interesting; ideally there should be one
> since it's probably an indication that something is wrong but there
> are probably going to be use cases for it that can't be covered by
> always_on, not that I can think of any right now.
The "boot_on" flag seems redundant with given "always_on"; as
a rule you can *tell* if the bootloader enabled a supply, so
the issue is more like whether it should stay on, even when no
driver wants it on. Especially considering that bootloaders
seldom guarantee to enable only a minimal set of supplies.
See the appended patch.
- Dave
=========== CUT HERE
From: David Brownell <[email protected]>
Let the regulator framework disable regulators that have wrongly
been left enabled. In the same manner as various implementations
of the clock framework, this is done by a late_initcall after all
drivers have had a chance to say what should be enabled.
Signed-off-by: David Brownell <[email protected]>
---
drivers/regulator/Kconfig | 11 +++++++++++
drivers/regulator/core.c | 34 ++++++++++++++++++++++++++++++++++
2 files changed, 45 insertions(+)
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -32,6 +32,17 @@ config REGULATOR_FIXED_VOLTAGE
tristate
default n
+config REGULATOR_DISABLE_UNUSED
+ bool "Disable unused regulators"
+ help
+ Board initialization sometimes leaves a few regulators
+ active even though they are not being used. This option
+ disables such regulators after all drivers have had a chance
+ to initialize and enable their regulators, conserving power.
+
+ If you're not sure that all your drivers have been properly
+ integrated with the regulator framework, say no.
+
config REGULATOR_VIRTUAL_CONSUMER
tristate "Virtual regulator consumer support"
default n
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -763,6 +763,7 @@ static int set_machine_constraints(struc
rdev->constraints = NULL;
goto out;
}
+ rdev->use_count = 1;
}
print_constraints(rdev);
@@ -2106,3 +2107,36 @@ static int __init regulator_init(void)
/* init early to allow our consumers to complete system booting */
core_initcall(regulator_init);
+
+static int __init regulator_disable_unused(void)
+{
+ struct regulator_dev *rdev;
+
+ mutex_lock(®ulator_list_mutex);
+ list_for_each_entry(rdev, ®ulator_list, list) {
+ int status;
+
+ if (rdev->use_count > 0)
+ continue;
+
+ status = _regulator_is_enabled(rdev);
+ if (status == 0)
+ continue;
+
+ if (status > 0)
+ dev_warn(&rdev->dev, "%s is unused but enabled\n",
+ rdev->desc->name);
+
+#ifdef CONFIG_REGULATOR_DISABLE_UNUSED
+ if (!rdev->desc->ops->disable)
+ continue;
+ status = rdev->desc->ops->disable(rdev);
+ if (status < 0)
+ dev_err(&rdev->dev, "%s error %d when disabling\n",
+ rdev->desc->name, status);
+#endif
+ }
+ mutex_unlock(®ulator_list_mutex);
+ return 0;
+}
+late_initcall(regulator_disable_unused);
On Thu, Jan 15, 2009 at 02:32:30PM -0800, David Brownell wrote:
> On Thursday 15 January 2009, Mark Brown wrote:
> > > That raises an issue: ?how can Linux get such regulators to
> > > turn off? ?Clock frameworks have the same issue, and they
> > That's something that should be done by constraints - add a new flag
> > that can be set.
> That doesn't make any sense to me, any more than
> adding a per-clock flag would.
Well, the clock API doesn't have the concept of constraints or machine
selectable per-clock options such as those set in constraints - it's the
biggest difference between the two APIs.
> > > I'm thinking it'd be worth having a similar Kconfig option
> > > for regulators too. ?It could kick in regulator core code to
> > > call regulator_ops.disable() whenever regulator_dev.use_count
> > > is zero, possibly warning if !regulator_ops.is_disabled().
> > > (Handling constraints.always_on too.)
> > Given the model the API has of not doing anything unless explicitly
> > told to
> ... e.g. by CONFIG_REGULATOR_DISABLE_UNUSED being explicitly
> set, indicating that the regulator support for that platform
> (and its drivers) is complete enough that it can safely paper
> over such early init issues.
Yeah, but user visible things in Kconfig aren't quite the same thing as
having something in the machine definition set it up. At present
control is very firmly in the hands of the board setup code.
> > I'd not expect to see this as a Kconfig option but rather as
> > something enabled per regulator or at least per board.
> Why not, though? That works well with clocks, once the
> drivers are debugged. Having dozens of fiddly little knobs
> is not an advantage.
There's got to be an explict knob, it's just a question of where it's
exposed.
For machines where it works it's something I'd expect the kernel to just
do without having a Kconfig variable set up. The machine driver is
already going to have to explictly set up all the regulators that are in
use in the system for this to work so the additional cost of requesting
the feature if it were per-board doesn't feel onerous. It also means
that people configuring the kernel don't need to worry about how well
the regulators are mapped out, which would be especially useful to
anyone trying to build kernels that run on multiple boards.
> The "boot_on" flag seems redundant with given "always_on"; as
> a rule you can *tell* if the bootloader enabled a supply, so
> the issue is more like whether it should stay on, even when no
> driver wants it on. Especially considering that bootloaders
> seldom guarantee to enable only a minimal set of supplies.
Yeah, I think that since there's no clean way for clients to disable a
boot_on regulator currently it's only really useful in the very unsual
case where you can't read the regulator state. If it just enabled the
regulator without incrementing the use count it may possibly useful for
during boot. I'd need to have a proper think about it to make sure.