[lm-sensors] regulator: regulator_get behaviour without

[lm-sensors] regulator: regulator_get behaviour without

[Jerome Oufella]

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

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

regulator: regulator_get behaviour without CONFIG_REGULATOR set

[Jerome Oufella]

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

Re: [lm-sensors] regulator: regulator_get behaviour without

[Mark Brown]

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
this up here.

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

Right, it's just returning something that won't match IS_ERR().

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

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().

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: regulator: regulator_get behaviour without CONFIG_REGULATOR set

[Mark Brown]

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
this up here.

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

Right, it's just returning something that won't match IS_ERR().

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

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().

Re: [lm-sensors] regulator: regulator_get behaviour without

[Jean Delvare]

Hi Mark,

On Fri, 2 Apr 2010 17:00:59 +0100, Mark Brown wrote:
> 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
> this up here.
> 
> > 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.
> 
> Right, it's just returning something that won't match IS_ERR().

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
sane reason for doing things that way. It's pretty error-prone.

> > 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 ?
> 
> 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,

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

> with predictably poor
> results.  It is this function that the driver needs to check, not
> regulator_get().

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
have to do the latter to realize that the former did not actually work.

> There are a range of reasons why an error might be
> returned when querying the voltage, all of which would cause the same
> result.

I don't quite follow you here.

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

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

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] regulator: regulator_get behaviour without CONFIG_REGULATOR set

[Jean Delvare]

Hi Mark,

On Fri, 2 Apr 2010 17:00:59 +0100, Mark Brown wrote:
> 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
> this up here.
> 
> > 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.
> 
> Right, it's just returning something that won't match IS_ERR().

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
sane reason for doing things that way. It's pretty error-prone.

> > 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 ?
> 
> 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,

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

> with predictably poor
> results.  It is this function that the driver needs to check, not
> regulator_get().

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
have to do the latter to realize that the former did not actually work.

> There are a range of reasons why an error might be
> returned when querying the voltage, all of which would cause the same
> result.

I don't quite follow you here.

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

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

Re: [lm-sensors] regulator: regulator_get behaviour

[Mark Brown]

On Fri, Apr 02, 2010 at 06:44:03PM +0200, Jean Delvare wrote:
> On Fri, 2 Apr 2010 17:00:59 +0100, Mark Brown wrote:

> > Right, it's just returning something that won't match IS_ERR().

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

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

> I've never seen any API doing things that way, and I can't think of a
> sane reason for doing things that way. It's pretty error-prone.

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
not use the regulator API actually have.

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

> Error code? regulator_get_voltage() returns 0, how is that supposed to
> be an error code?

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
having a hard time summoning the enthusiasm to do that myself.

> > with predictably poor
> > results.  It is this function that the driver needs to check, not
> > regulator_get().

> 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
> have to do the latter to realize that the former did not actually work.

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
silly combinations being filtered out at the hardware design stage.

> > There are a range of reasons why an error might be
> > returned when querying the voltage, all of which would cause the same
> > result.

> I don't quite follow you here.

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
system mostly uses regulators).

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

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

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 regulator
API in Kconfig or have ifdefs and so never see the stubs though they
should still error check since individual operations may fail or not be
supported.

Looking again at the stubs we should remove the stubs for at least
setting voltages and current limits from the API since they don't
currently do the right thing and I can't think of any useful stub
behaviour.  The get operations are more useful as stubs since some
analogue parts can usefully have their configuration optimised if we
know their supply voltages but it's just a nice to have and not a
requirement.

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] regulator: regulator_get behaviour without CONFIG_REGULATOR set

[Mark Brown]

On Fri, Apr 02, 2010 at 06:44:03PM +0200, Jean Delvare wrote:
> On Fri, 2 Apr 2010 17:00:59 +0100, Mark Brown wrote:

> > Right, it's just returning something that won't match IS_ERR().

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

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

> I've never seen any API doing things that way, and I can't think of a
> sane reason for doing things that way. It's pretty error-prone.

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
not use the regulator API actually have.

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

> Error code? regulator_get_voltage() returns 0, how is that supposed to
> be an error code?

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
having a hard time summoning the enthusiasm to do that myself.

> > with predictably poor
> > results.  It is this function that the driver needs to check, not
> > regulator_get().

> 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
> have to do the latter to realize that the former did not actually work.

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
silly combinations being filtered out at the hardware design stage.

> > There are a range of reasons why an error might be
> > returned when querying the voltage, all of which would cause the same
> > result.

> I don't quite follow you here.

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
system mostly uses regulators).

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

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

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 regulator
API in Kconfig or have ifdefs and so never see the stubs though they
should still error check since individual operations may fail or not be
supported.

Looking again at the stubs we should remove the stubs for at least
setting voltages and current limits from the API since they don't
currently do the right thing and I can't think of any useful stub
behaviour.  The get operations are more useful as stubs since some
analogue parts can usefully have their configuration optimised if we
know their supply voltages but it's just a nice to have and not a
requirement.

Re: [lm-sensors] regulator: regulator_get behaviour without

[Jean Delvare]

Hi Mark,

On Fri, 2 Apr 2010 19:51:38 +0100, Mark Brown wrote:
> On Fri, Apr 02, 2010 at 06:44:03PM +0200, Jean Delvare wrote:
> > On Fri, 2 Apr 2010 17:00:59 +0100, Mark Brown wrote:
> 
> > > Right, it's just returning something that won't match IS_ERR().
> 
> > 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.
> 
> If this changes we can always provide a more complex stub; for now we're
> OK.
> 
> > I've never seen any API doing things that way, and I can't think of a
> > sane reason for doing things that way. It's pretty error-prone.
> 
> 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
> not use the regulator API actually have.

OK, I get the idea.

> > > 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,
> 
> > Error code? regulator_get_voltage() returns 0, how is that supposed to
> > be an error code?
> 
> 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
> having a hard time summoning the enthusiasm to do that myself.

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
caller that an error happened.

> > > with predictably poor
> > > results.  It is this function that the driver needs to check, not
> > > regulator_get().
> 
> > 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
> > have to do the latter to realize that the former did not actually work.
> 
> 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
> silly combinations being filtered out at the hardware design stage.
> 
> > > There are a range of reasons why an error might be
> > > returned when querying the voltage, all of which would cause the same
> > > result.
> 
> > I don't quite follow you here.
> 
> 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
> system mostly uses regulators).
> 
> > > It is not sensible to check the return code of regulator_get() for
> > > anything other than IS_ERR().
> 
> > Why can't we have the stub form of regulator_get() return NULL or
> > ERR_PTR(-ENODEV)? This would be a much friendlier API.
> 
> 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.

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
variable, too. What do you think?

> The expectation is that users which have a strong requirement that the 
> regulator API does more than this will need to depend on the regulator
> API in Kconfig or have ifdefs and so never see the stubs though they
> should still error check since individual operations may fail or not be
> supported.

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 to times. This is what make me believe
the stub API isn't good enough.

> Looking again at the stubs we should remove the stubs for at least
> setting voltages and current limits from the API since they don't
> currently do the right thing and I can't think of any useful stub
> behaviour.  The get operations are more useful as stubs since some
> analogue parts can usefully have their configuration optimised if we
> know their supply voltages but it's just a nice to have and not a
> requirement.

I second that. The stub API should only contain the minimum set of
functions that is required to keep drivers which don't depend on
CONFIG_REGULATOR ifdef-free. This will make its intended use case
clearer.

Thanks,
-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] regulator: regulator_get behaviour without CONFIG_REGULATOR set

[Jean Delvare]

Hi Mark,

On Fri, 2 Apr 2010 19:51:38 +0100, Mark Brown wrote:
> On Fri, Apr 02, 2010 at 06:44:03PM +0200, Jean Delvare wrote:
> > On Fri, 2 Apr 2010 17:00:59 +0100, Mark Brown wrote:
> 
> > > Right, it's just returning something that won't match IS_ERR().
> 
> > 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.
> 
> If this changes we can always provide a more complex stub; for now we're
> OK.
> 
> > I've never seen any API doing things that way, and I can't think of a
> > sane reason for doing things that way. It's pretty error-prone.
> 
> 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
> not use the regulator API actually have.

OK, I get the idea.

> > > 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,
> 
> > Error code? regulator_get_voltage() returns 0, how is that supposed to
> > be an error code?
> 
> 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
> having a hard time summoning the enthusiasm to do that myself.

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
caller that an error happened.

> > > with predictably poor
> > > results.  It is this function that the driver needs to check, not
> > > regulator_get().
> 
> > 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
> > have to do the latter to realize that the former did not actually work.
> 
> 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
> silly combinations being filtered out at the hardware design stage.
> 
> > > There are a range of reasons why an error might be
> > > returned when querying the voltage, all of which would cause the same
> > > result.
> 
> > I don't quite follow you here.
> 
> 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
> system mostly uses regulators).
> 
> > > It is not sensible to check the return code of regulator_get() for
> > > anything other than IS_ERR().
> 
> > Why can't we have the stub form of regulator_get() return NULL or
> > ERR_PTR(-ENODEV)? This would be a much friendlier API.
> 
> 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.

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
variable, too. What do you think?

> The expectation is that users which have a strong requirement that the 
> regulator API does more than this will need to depend on the regulator
> API in Kconfig or have ifdefs and so never see the stubs though they
> should still error check since individual operations may fail or not be
> supported.

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 to times. This is what make me believe
the stub API isn't good enough.

> Looking again at the stubs we should remove the stubs for at least
> setting voltages and current limits from the API since they don't
> currently do the right thing and I can't think of any useful stub
> behaviour.  The get operations are more useful as stubs since some
> analogue parts can usefully have their configuration optimised if we
> know their supply voltages but it's just a nice to have and not a
> requirement.

I second that. The stub API should only contain the minimum set of
functions that is required to keep drivers which don't depend on
CONFIG_REGULATOR ifdef-free. This will make its intended use case
clearer.

Thanks,
-- 
Jean Delvare

Re: [lm-sensors] regulator: regulator_get behaviour

[Mark Brown]

On Fri, Apr 02, 2010 at 09:30:10PM +0200, Jean Delvare wrote:
> On Fri, 2 Apr 2010 19:51:38 +0100, Mark Brown wrote:

> > 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
> > having a hard time summoning the enthusiasm to do that myself.

> 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
> caller that an error happened.

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

> 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):

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
stub.

> 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);
> 		}
> 	}

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
could be invariant.

> > The expectation is that users which have a strong requirement that the 
> > regulator API does more than this will need to depend on the regulator
> > API in Kconfig or have ifdefs and so never see the stubs though they
> > should still error check since individual operations may fail or not be
> > supported.

> 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 to times. This is what make me believe
> the stub API isn't good enough.

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
the point where we know enough to say that's a good idea at the minute.

> > Looking again at the stubs we should remove the stubs for at least
> > setting voltages and current limits from the API since they don't
> > currently do the right thing and I can't think of any useful stub
> > behaviour.  The get operations are more useful as stubs since some
> > analogue parts can usefully have their configuration optimised if we
> > know their supply voltages but it's just a nice to have and not a
> > requirement.

> I second that. The stub API should only contain the minimum set of
> functions that is required to keep drivers which don't depend on
> CONFIG_REGULATOR ifdef-free. This will make its intended use case
> clearer.

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().

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] regulator: regulator_get behaviour without CONFIG_REGULATOR set

[Mark Brown]

On Fri, Apr 02, 2010 at 09:30:10PM +0200, Jean Delvare wrote:
> On Fri, 2 Apr 2010 19:51:38 +0100, Mark Brown wrote:

> > 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
> > having a hard time summoning the enthusiasm to do that myself.

> 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
> caller that an error happened.

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

> 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):

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
stub.

> 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);
> 		}
> 	}

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
could be invariant.

> > The expectation is that users which have a strong requirement that the 
> > regulator API does more than this will need to depend on the regulator
> > API in Kconfig or have ifdefs and so never see the stubs though they
> > should still error check since individual operations may fail or not be
> > supported.

> 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 to times. This is what make me believe
> the stub API isn't good enough.

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
the point where we know enough to say that's a good idea at the minute.

> > Looking again at the stubs we should remove the stubs for at least
> > setting voltages and current limits from the API since they don't
> > currently do the right thing and I can't think of any useful stub
> > behaviour.  The get operations are more useful as stubs since some
> > analogue parts can usefully have their configuration optimised if we
> > know their supply voltages but it's just a nice to have and not a
> > requirement.

> I second that. The stub API should only contain the minimum set of
> functions that is required to keep drivers which don't depend on
> CONFIG_REGULATOR ifdef-free. This will make its intended use case
> clearer.

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().

Re: [lm-sensors] regulator: regulator_get behaviour without

[Jean Delvare]

Hi Mark,

On Fri, 2 Apr 2010 21:45:03 +0100, Mark Brown wrote:
> On Fri, Apr 02, 2010 at 09:30:10PM +0200, Jean Delvare wrote:
> > 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);
> > 		}
> > 	}
> 
> 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
> could be invariant.

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

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] regulator: regulator_get behaviour without CONFIG_REGULATOR set

[Jean Delvare]

Hi Mark,

On Fri, 2 Apr 2010 21:45:03 +0100, Mark Brown wrote:
> On Fri, Apr 02, 2010 at 09:30:10PM +0200, Jean Delvare wrote:
> > 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);
> > 		}
> > 	}
> 
> 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
> could be invariant.

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

Re: [lm-sensors] regulator: regulator_get behaviour without

[Mark Brown]

On Sat, Apr 03, 2010 at 05:37:45PM +0200, Jean Delvare wrote:
> On Fri, 2 Apr 2010 21:45:03 +0100, Mark Brown wrote:

> > 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
> > could be invariant.

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

I was assuming that there wasn't a previous value since this was in
probe(), sorry.

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

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.

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] regulator: regulator_get behaviour without CONFIG_REGULATOR set

[Mark Brown]

On Sat, Apr 03, 2010 at 05:37:45PM +0200, Jean Delvare wrote:
> On Fri, 2 Apr 2010 21:45:03 +0100, Mark Brown wrote:

> > 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
> > could be invariant.

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

I was assuming that there wasn't a previous value since this was in
probe(), sorry.

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

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.

Re: [lm-sensors] regulator: regulator_get behaviour without

[Jonathan Cameron]

On 04/05/10 14:23, Mark Brown wrote:
> On Sat, Apr 03, 2010 at 05:37:45PM +0200, Jean Delvare wrote:
>> On Fri, 2 Apr 2010 21:45:03 +0100, Mark Brown wrote:
> 
>>> 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
>>> could be invariant.
> 
>> 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.
> 
> I was assuming that there wasn't a previous value since this was in
> probe(), sorry.
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
it existed) - sorry about that.
> 
>> 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.
> 
> 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.

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] regulator: regulator_get behaviour without CONFIG_REGULATOR set

[Jonathan Cameron]

On 04/05/10 14:23, Mark Brown wrote:
> On Sat, Apr 03, 2010 at 05:37:45PM +0200, Jean Delvare wrote:
>> On Fri, 2 Apr 2010 21:45:03 +0100, Mark Brown wrote:
> 
>>> 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
>>> could be invariant.
> 
>> 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.
> 
> I was assuming that there wasn't a previous value since this was in
> probe(), sorry.
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
it existed) - sorry about that.
> 
>> 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.
> 
> 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.

Re: [lm-sensors] regulator: regulator_get behaviour without

[Liam Girdwood]

On Mon, 2010-04-05 at 14:23 +0100, Mark Brown wrote:
> On Sat, Apr 03, 2010 at 05:37:45PM +0200, Jean Delvare wrote:
> > On Fri, 2 Apr 2010 21:45:03 +0100, Mark Brown wrote:
> 
> > > 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
> > > could be invariant.
> 
> > 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.
> 
> I was assuming that there wasn't a previous value since this was in
> probe(), sorry.
> 
> > 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.
> 
> 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.

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


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] regulator: regulator_get behaviour without CONFIG_REGULATOR set

[Liam Girdwood]

On Mon, 2010-04-05 at 14:23 +0100, Mark Brown wrote:
> On Sat, Apr 03, 2010 at 05:37:45PM +0200, Jean Delvare wrote:
> > On Fri, 2 Apr 2010 21:45:03 +0100, Mark Brown wrote:
> 
> > > 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
> > > could be invariant.
> 
> > 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.
> 
> I was assuming that there wasn't a previous value since this was in
> probe(), sorry.
> 
> > 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.
> 
> 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.

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

Re: [lm-sensors] regulator: regulator_get behaviour without

[Jonathan Cameron]

On 04/06/10 16:27, Liam Girdwood wrote:
> On Mon, 2010-04-05 at 14:23 +0100, Mark Brown wrote:
>> On Sat, Apr 03, 2010 at 05:37:45PM +0200, Jean Delvare wrote:
>>> On Fri, 2 Apr 2010 21:45:03 +0100, Mark Brown wrote:
>>
>>>> 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
>>>> could be invariant.
>>
>>> 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.
>>
>> I was assuming that there wasn't a previous value since this was in
>> probe(), sorry.
>>
>>> 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.
>>
>> 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.
> 
> Thanks, I've applied this with Mark's Ack.
> 
> I suppose this is something we may look into more when we have more
> clients.
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




_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] regulator: regulator_get behaviour without CONFIG_REGULATOR set

[Jonathan Cameron]

On 04/06/10 16:27, Liam Girdwood wrote:
> On Mon, 2010-04-05 at 14:23 +0100, Mark Brown wrote:
>> On Sat, Apr 03, 2010 at 05:37:45PM +0200, Jean Delvare wrote:
>>> On Fri, 2 Apr 2010 21:45:03 +0100, Mark Brown wrote:
>>
>>>> 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
>>>> could be invariant.
>>
>>> 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.
>>
>> I was assuming that there wasn't a previous value since this was in
>> probe(), sorry.
>>
>>> 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.
>>
>> 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.
> 
> Thanks, I've applied this with Mark's Ack.
> 
> I suppose this is something we may look into more when we have more
> clients.
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

Re: [lm-sensors] regulator: regulator_get behaviour without

[Mark Brown]

On 6 Apr 2010, at 17:25, Jonathan Cameron <jic23@cam.ac.uk> wrote:

> On 04/06/10 16:27, Liam Girdwood wrote:
>>>
>>>
>>>
>>
>> I suppose this is something we may look into more when we have more
>> clients.
> 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.

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.

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] regulator: regulator_get behaviour without CONFIG_REGULATOR set

[Mark Brown]

On 6 Apr 2010, at 17:25, Jonathan Cameron <jic23@cam.ac.uk> wrote:

> On 04/06/10 16:27, Liam Girdwood wrote:
>>>
>>>
>>>
>>
>> I suppose this is something we may look into more when we have more
>> clients.
> 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.

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.

Re: [lm-sensors] regulator: regulator_get behaviour without

[Liam Girdwood]

On Tue, 2010-04-06 at 19:19 +0100, Mark Brown wrote:
> On 6 Apr 2010, at 17:25, Jonathan Cameron <jic23@cam.ac.uk> wrote:
> 
> > On 04/06/10 16:27, Liam Girdwood wrote:
> >>>
> >>>
> >>>
> >>
> >> I suppose this is something we may look into more when we have more
> >> clients.
> > 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.
> 
> 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.
> --

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


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] regulator: regulator_get behaviour without CONFIG_REGULATOR set

[Liam Girdwood]

On Tue, 2010-04-06 at 19:19 +0100, Mark Brown wrote:
> On 6 Apr 2010, at 17:25, Jonathan Cameron <jic23@cam.ac.uk> wrote:
> 
> > On 04/06/10 16:27, Liam Girdwood wrote:
> >>>
> >>>
> >>>
> >>
> >> I suppose this is something we may look into more when we have more
> >> clients.
> > 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.
> 
> 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.
> --

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

Re: [lm-sensors] regulator: regulator_get behaviour without

[Jonathan Cameron]

On 04/06/10 19:19, Mark Brown wrote:
> On 6 Apr 2010, at 17:25, Jonathan Cameron <jic23@cam.ac.uk> wrote:
> 
>> On 04/06/10 16:27, Liam Girdwood wrote:
>>>>
>>>>
>>>>
>>>
>>> I suppose this is something we may look into more when we have more
>>> clients.
>> 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.
> 
> 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.
> 
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

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] regulator: regulator_get behaviour without CONFIG_REGULATOR set

[Jonathan Cameron]

On 04/06/10 19:19, Mark Brown wrote:
> On 6 Apr 2010, at 17:25, Jonathan Cameron <jic23@cam.ac.uk> wrote:
> 
>> On 04/06/10 16:27, Liam Girdwood wrote:
>>>>
>>>>
>>>>
>>>
>>> I suppose this is something we may look into more when we have more
>>> clients.
>> 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.
> 
> 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.
> 
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

Re: [lm-sensors] regulator: regulator_get behaviour without

[Mark Brown]

On Wed, Apr 07, 2010 at 12:24:02PM +0100, Jonathan Cameron wrote:
> On 04/06/10 19:19, Mark Brown wrote:

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

> 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).

Like I say, I think this is likely to be a small benefit from that.  The
rest of what you're doing seems very vanilla.

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

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.

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] regulator: regulator_get behaviour without CONFIG_REGULATOR set

[Mark Brown]

On Wed, Apr 07, 2010 at 12:24:02PM +0100, Jonathan Cameron wrote:
> On 04/06/10 19:19, Mark Brown wrote:

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

> 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).

Like I say, I think this is likely to be a small benefit from that.  The
rest of what you're doing seems very vanilla.

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

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.