Hi,

Working on drivers/hwmon/sht15.c, I noticed it would return bogus temperatures in my case, where CONFIG_REGULATOR is not set.

This is due to the following section in drivers/hwmon/sht15.c:

/* If a regulator is available, query what the supply voltage actually is!*/
        data->reg = regulator_get(data->dev, "vcc");
        if (!IS_ERR(data->reg)) {
            ...

Looking at consumer.h, it appears that regulator_get() returns a pointer to its second argument when CONFIG_REGULATOR is not set.

What would be the proper way to determine if the returned value is a valid regulator ?
Would it be safe to check it against the 2nd argument ?

Regards
Jerome Oufella
--

[ message continues ]

On Fri, Apr 02, 2010 at 11:47:50AM -0400, Jerome Oufella wrote:

Please fix your mail client to word wrap paragraphs, I've manually fixed


You're asking the wrong question here.  The problem here is not that the
regulator got stubbed out, the problem is that the sht15 driver is not
checking the return value of regulator_get_voltage() and so is trying to
use the error code that was returned as a voltage, with predictably poor
results.  It is this function that the driver needs to check, not
regulator_get().  There are a range of reasons why an error might be
returned when querying the voltage, all of which would cause the same
result.

It is not sensible to check the return code of regulator_get() for
anything other than IS_ERR().
--

[ message continues ]

Hi Mark,


Ugly design. You're casting a random pointer to struct regulator * and
just hope that the user won't make use of it. Right now, you're safe
because the definition of struct regulator is not public, but assuming
it will stay that way forever is somewhat risky.

I've never seen any API doing things that way, and I can't think of a

Error code? regulator_get_voltage() returns 0, how is that supposed to

This goes against the expectations. When I have to get a reference to
something and then use that something, I expect the former to fail is
the something is not available for whatever reason. I don't expect to


Why can't we have the stub form of regulator_get() return NULL or
ERR_PTR(-ENODEV)? This would be a much friendlier API.

-- 
Jean Delvare
--

[ message continues ]

If this changes we can always provide a more complex stub; for now we're

All we're doing is stubbing out the API so that common case clients
(which just want to switch on and off their supplies) don't need to
either depend on it or include lots of conditional code which could
easily end up masking error conditions.  The stub behaves as always
on fixed voltage regulators which matches what most systems that do

It's zero volts which is a reasonable out of range value for a
regulator.  We could change the API to return a signed value but I'm

Well, it did work in the sense that there's something there that
satisfies the expectations of many users.  We could change things so
that the consumers passed in a set of requirements for the regulator but
that'd be a bit mroe work and at the minute we're mostly relying on

Examples include failure to communicate with the hardware (so we don't
know what state it's in), or hardware that isn't actually doing
sufficient regulation to supply a voltage (many regulators support a
more efficient passthrough mode, and sometimes devices are connected
directly to unregulated supplies for efficiency reasons even if the

Not really.  The overwhemling majority of users only do simple power
control, they don't need to get or set voltages and are only interested
in allowing the system to save some power when they're idle.  These
drivers don't even need the power to actually get switched off, they
just want to allow that possibility.  If we returned an error then all
these consumers would need to conditionalise all their regulator API
usage and find it hard to distinguish between not having any power and
normal operation without the regulator API.  With the current approach
these drivers can have a single code path which is used unconditionally
and will do the right thing on systems with and without the regulator
API.

The expectation is that users which have a strong requirement that the 
regulator API does more than this will need to depend on the ...
[ message continues ]

Hi Mark,



Sorry for playing the devil's advocate here, but deciding that 0V is an
error is pretty arbitrary, and deciding that a negative voltage value
is an error is just as arbitrary. Just because these are not common
cases, doesn't mean they can't happen today or tomorrow for very
specific setups. I'd rather see a more robust way to notifier the

OK, I get it. This indeed rules out ERR_PTR(-ENODEV). But what about
NULL? IS_ERR() doesn't catch NULL, so this wouldn't affect the current
users, as you never dereference the struct regulator pointer in the
stubs anyway. And at least it would let drivers that need it cleanly
differentiate between the cases of availability or unavailability of
the real regulator API. Something like (from the hwmon/sht15 driver):

/* If a regulator is available, query what the supply voltage actually is!*/
	data->reg = regulator_get(data->dev, "vcc");
	if (data->reg && !IS_ERR(data->reg)) {
		data->supply_uV = regulator_get_voltage(data->reg);
		regulator_enable(data->reg);
		/* setup a notifier block to update this if another device
		 *  causes the voltage to change */
		data->nb.notifier_call = &sht15_invalidate_voltage;
		ret = regulator_register_notifier(data->reg, &data->nb);
	}

looks much better than

/* If a regulator is available, query what the supply voltage actually is!*/
	data->reg = regulator_get(data->dev, "vcc");
	if (!IS_ERR(data->reg)) {
		int voltage = regulator_get_voltage(data->reg);
		if (voltage) {
			data->supply_uV = voltage;
			regulator_enable(data->reg);
			/* setup a notifier block to update this if
			 *  another device causes the voltage to change */
			data->nb.notifier_call = &sht15_invalidate_voltage;
			ret = regulator_register_notifier(data->reg, &data->nb);
		}
	}

IMHO. One less level of indentation, and one less intermediate

I guess we could have ifdefs in hwmon/sht15, yes. But OTOH it looks
weird to have a complete stub API for the CONFIG_REGULATOR=n case, and
still require ifdefs from times ...
[ message continues ]

Yes, you could pass a pointer to the value or similar.  OTOH that'd mean

I'm not sure there's actually much win from this information since we do
have cases with things like functionally limited regulators and error
conditions which mean that drivers end up having to cope with pretty
much all this stuff anyway.  But yes, NULL should be just as good as a

In this case you don't need the if (voltage) check - the code that uses
supply_uV is going to have to cope with it being set to 0 if the driver
doesn't just give up, and the enable wants to happen anyway (perhaps
we've got a switchable supply we can't read the voltage of).  It should
never make any odds if the notifier never gets called since the supply

The regulator API is kind of odd here in that there's a lot of users
that are able to do things using it but which are largely indifferent to
if they're happening or not since the results are optimisations of
benefit to the system as a whole rather than directly used
functionality.  The stubs are only attempting to cater for them and
don't cater for the other users that do care about what happens.

I agree that it's unusual but don't see any great alternatives that
don't involve things like pushing some stuff for the stub users more
into the core APIs (like how some platforms are integrating their clock
management into the PM framework) and I don't think we're anywhere near

Actually having thought about that one a bit more I'm not so sure for
set_voltage() - the DVFS style drivers are in a similar position to the
basic power switching ones, they'd like to be able to lower the supplies
to save power but don't actually need that to happen.  Needs a bit more
thought.

There are currently some functions that don't get stubbed, like
regulator_get_exclusive().
--

[ message continues ]

Hi Mark,


We still need to check if (voltage) to not overwrite the previous value
of data->supply_uV with 0. We will probably do that as an immediate fix
to the sht15 driver. But yes, the rest doesn't need a condition.

Still, I'd prefer if drivers were just able to check for data->reg ==
NULL and skip the whole thing. Would you apply the following patch?

* * * * *

From: Jean Delvare <khali@linux-fr.org>
Subject: regulator: Let drivers know when they use the stub API

Have the stub variant of regulator_get() return NULL, so that drivers
can (but still don't have to) handle this case specifically.

Signed-off-by: Jean Delvare <khali@linux-fr.org>
Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
Cc: Jerome Oufella <jerome.oufella@savoirfairelinux.com>
---
 include/linux/regulator/consumer.h |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

--- linux-2.6.34-rc3.orig/include/linux/regulator/consumer.h	2010-03-09 08:25:30.000000000 +0100
+++ linux-2.6.34-rc3/include/linux/regulator/consumer.h	2010-04-03 17:21:08.000000000 +0200
@@ -183,9 +183,13 @@ static inline struct regulator *__must_c
 {
 	/* Nothing except the stubbed out regulator API should be
 	 * looking at the value except to check if it is an error
-	 * value so the actual return value doesn't matter.
+	 * value. Drivers are free to handle NULL specifically by
+	 * skipping all regulator API calls, but they don't have to.
+	 * Drivers which don't, should make sure they properly handle
+	 * corner cases of the API, such as regulator_get_voltage()
+	 * returning 0.
 	 */
-	return (struct regulator *)id;
+	return NULL;
 }
 static inline void regulator_put(struct regulator *regulator)
 {


Thanks,
-- 
Jean Delvare
--

[ message continues ]

I was assuming that there wasn't a previous value since this was in

I guess I'll ack it but I'd be suspicous of driver code which actually
makes use of this - there is actual hardware which has the same features
as the regulator that gets stubbed in and ought to be handled.  On the
other hand, perhaps someone will come up with a good use for it.

It also seems a bit odd to return a traditional error value in a success
case but it doesn't actually make much difference.
--

[ message continues ]

The option to supply this voltage via platform data is there because
at the time or writing there were relatively few regulator drivers
in existence at all.  The device really needs to know what this voltage
is to function.

The underlying bug in sht15 was thrown up a while back but we never
worked out how to fix it then (and I'll admit I managed to forget
--

[ message continues ]

Thanks, I've applied this with Mark's Ack.

I suppose this is something we may look into more when we have more
clients.

Liam

-- 
Freelance Developer, SlimLogic Ltd
ASoC and Voltage Regulator Maintainer.
http://www.slimlogic.co.uk

--

[ message continues ]

Makes sense.  There will probably be quite a few IIO drivers over the next
few months doing much the same as sht15, where the voltage ref for devices
may well be fed by a regulator.  In that case, we may only offer the option
of using an external v_ref if the regulator is available.  Many devices have
an internal regulator to provide it so typically we'll start them up using that
and provide an interface to switch to external regulator if one is available.
I haven't thought through exactly how this will work as yet. I'll cc people in
when this comes up.

Thanks,

Jonathan



--

[ message continues ]

TBH this seems like a very vanilla use case - there may be some small  
advantage to representing the internal regulator via the regulator API  
but that's about the only thing I can think might be a bit odd.
--

[ message continues ]

Although may be quite useful here for any mfd devices where the core has
regulators that only supply the other on chip functions.

Liam 

-- 
Freelance Developer, SlimLogic Ltd
ASoC and Voltage Regulator Maintainer.
http://www.slimlogic.co.uk

--

[ message continues ]

I wasn't thinking of representing the internal regulator using the regulator
framework (though if it is externally available I guess that would make sense
though probably only if anyone is actually using this to supply something else
- most likely case I can think of is daisy chaining multiple adc's and ensuring
they have the same reference value).

The case I'm interested in is the externally supplied voltage reference. This
typically comes off a fixed regulator or a spare regulator on a pmic.
The use case is getting this voltage (and indeed buffering it using the
same notifier approach as in sht15). This is needed to provide api compliant info
to userspace (which needs sufficient info to work out what the actual value is).

I'd imagine similar cases occur in some hwmon devices.

Basically all these uses look just like that in sht15.

Nothing new here, but there will be a number of consumers that care about changes
in voltage (rather than typically controlling it.)  Hence I'm welcoming the change
just agreed upon.

Thanks,

Jonathan
--

[ message continues ]

Like I say, I think this is likely to be a small benefit from that.  The

Note that you're not going to see any difference you can actually use
here - you still have to handle the possibility that you've got an
actual regulator but for some reason fail to read a voltage from it
which is the same behaviour that you see from the dummy regulator.
--

[ message continues ]