regulator: adding constraints to regulator_desc?

regulator: adding constraints to regulator_desc?

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1204 bytes --]

Hi,

I have a question regarding the constraints of the regulator-framework: If I
understood correctly, there is a struct in regulator_dev which is describing
the constraints on the power-domain-level. As we also have constraints on the
regulator-level, all the regulator drivers do sanity checks in their
set_voltage()-functions. These checks differ and it is not always clear to me
if additional checks other drivers have were forgotten or intentionally omitted
for a specific driver. I also wondered about code-duplication and if it was
maybe worthwhile to simply add 'min' and 'max' members to struct regulator_desc
and let the core do sanity checks (like it does for the constraints on the
power-domain-level). Has this been considered already?

One thing which also raised my attention was the beginning of
regulator_check_voltage(). It starts with

	BUG_ON(*min_uV > *max_uV);

Is it really necessary to halt the kernel? Wouldn't a big warning and -EINVAL
do like at the end of the function?

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: regulator: adding constraints to regulator_desc?

[Mark Brown]

On Mon, Sep 14, 2009 at 12:41:01PM +0200, Wolfram Sang wrote:
> the constraints on the power-domain-level. As we also have constraints on the
> regulator-level, all the regulator drivers do sanity checks in their
> set_voltage()-functions. These checks differ and it is not always clear to me
> if additional checks other drivers have were forgotten or intentionally omitted
> for a specific driver. I also wondered about code-duplication and if it was
> maybe worthwhile to simply add 'min' and 'max' members to struct regulator_desc
> and let the core do sanity checks (like it does for the constraints on the
> power-domain-level). Has this been considered already?

The checks at the regulator level should be checking more than can be
specified in the constraints - they should also be checking that the
regulator can actually deliver the requested voltage.  It is possible
that a request could be within the constraints but in between two steps
that the regulator can deliver and therefore not supportable.  Drivers
that don't do the additional checks should really be doing them.

These checks could be factored out if we change the API for setting
voltage to work in terms of voltage selectors and force the
implementation of list_voltage() but it's never seemed worth the hassle
yet since we've got cross-tree merge issues caused by the fact that half
the regulator drivers end up getting merged via the MFD tree.  Those
could be dealt with by supporting both methods for a release cycle.  I
might have a look again today, I'm going to be looking at some regulator
drivers anyway.

> One thing which also raised my attention was the beginning of
> regulator_check_voltage(). It starts with

> 	BUG_ON(*min_uV > *max_uV);

> Is it really necessary to halt the kernel? Wouldn't a big warning and -EINVAL
> do like at the end of the function?

That should also be OK, yes.  The risk is that once you start loosing
the plot on regulator stuff the system will often die anyway due to
power problems but WARN_ON and an error would cover the diagnostics just
as well.

Re: regulator: adding constraints to regulator_desc?

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1655 bytes --]


> The checks at the regulator level should be checking more than can be
> specified in the constraints - they should also be checking that the
> regulator can actually deliver the requested voltage.  It is possible
> that a request could be within the constraints but in between two steps
> that the regulator can deliver and therefore not supportable.  Drivers
> that don't do the additional checks should really be doing them.

I thought about the core just doing the basic checks (inbetween min and max),
so the regulator-drivers could still add more checks if needed. Though...

> These checks could be factored out if we change the API for setting
> voltage to work in terms of voltage selectors and force the
> implementation of list_voltage() but it's never seemed worth the hassle

...if that works out and more checks could be done by the core, this would be
great, I think.

> > One thing which also raised my attention was the beginning of
> > regulator_check_voltage(). It starts with
> 
> > 	BUG_ON(*min_uV > *max_uV);
> 
> > Is it really necessary to halt the kernel? Wouldn't a big warning and -EINVAL
> > do like at the end of the function?
> 
> That should also be OK, yes.  The risk is that once you start loosing
> the plot on regulator stuff the system will often die anyway due to
> power problems but WARN_ON and an error would cover the diagnostics just
> as well.

Okay. Will try to prepare a patch this evening.

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]