Re: [patch 2.6.28-rc6+] regulator: bugfixes and messaging cleanup

[David Brownell <david-b@pacbell.net>]

On Monday 01 December 2008, Mark Brown wrote:
> On Mon, Dec 01, 2008 at 01:35:43PM -0800, David Brownell wrote:
> 
> > Regulator core bugfixes:
> 
> ...
> 
> > And messaging updates;
> 
> This could usefuly be split into a patch series, there's rather a lot
> of different changes in here...

Could be ... I was mostly just collecting core updates involved in
making sure I could regulator_enable() on one board.  The messaging
in the current code was decidedly un-helpful for that task.

At most I'd see two patches:  one with bugfixes, one with messaging
fixes that aren't strictly "bug" fixes (i.e. messages worked but
violated normal conventions like driver model).


> On the whole it looks good, a few 
> things, though:
> 
> > @@ -894,7 +915,7 @@ struct regulator *regulator_get(struct d
> >  	struct regulator *regulator = ERR_PTR(-ENODEV);
> >  
> >  	if (id == NULL) {
> > -		printk(KERN_ERR "regulator: get() with no identifier\n");
> > +		dev_dbg(dev, "regulator_get with no identifier\n");
> >  		return regulator;
> >  	}
> 
> dev may legally be NULL here

The kerneldoc doesn't mention that at all ... seems like a pretty
major interface artifact.  Hmm, for that matter I notice that if
you were to run kerneldoc, regulator_register() -- and presumably
other functions -- would get lots of errors.

Allowing it to be NULL seems pretty much contrary to the model that
a supply name is associated with a device.  I'd call that a bug and
fix it ... vs calling it a feature and diverging from standard
framework models.


> which is going to cause upset if the 
> dev_dbg() is hit.  Things like cpufreq which can use regulators don't
> use a struct device (currently!) so it's supported.

Which cpufreq driver(s) in mainline use regulator code?

I had commented not long ago that the regulator framework needs
updates to support cpufreq.  For example, trying to enable hardware
support for DVFS (Dynamic Voltage and Frequency Scaling) on several
platforms requires configuring more than one voltage ... floor and
ceiling, switched automatically by a CPU signal, for starters.

It would be good to overhaul cpufreq before trying to make it use
things like the clock and regulator frameworks.  Fitting into the
driver model seems overdue, for one.


> > @@ -971,19 +991,16 @@ static int _regulator_enable(struct regu
> >  	int ret = -EINVAL;
> >  
> >  	if (!rdev->constraints) {
> > -		printk(KERN_ERR "%s: %s has no constraints\n",
> > -		       __func__, rdev->desc->name);
> > +		dev_err(&rdev->dev, "%s has no constraints\n",
> > +		       rdev->desc->name);
> 
> Hrm.  You've downgraded most of the diagnostics to dev_dbg()

No; just ones in the enable/disable paths, and even there just
ones where the caller has control over the illegal parameters.

Recall that rdev->constraints can be nulled for various reasons
that aren't under control of the regulator client.


> but left 
> this as KERN_ERR.  It'd be nice to be a bit more consistent.  My
> personal preference is to make things in the constraints and machine
> definitions display at dev_err() since they should never happen and a
> plain text error is often helpful for new users but it's not a very
> strong one either way.

I think this was as consistent as possible with the basic rule
that when the caller could have prevented it, it's their job to
handle the fault code they get.


> 
> >  		ret = rdev->desc->ops->enable(rdev);
> > -		if (ret < 0) {
> > -			printk(KERN_ERR "%s: failed to enable %s: %d\n",
> > -			       __func__, rdev->desc->name, ret);
> > -			return ret;
> > +		if (ret >= 0) {
> > +			dev_dbg(&rdev->dev, "enabled %s\n", rdev->desc->name);
> > +			rdev->use_count++;
> 
> I have to say I'd be happier keeping the logic as it was here.  The
> behaviour does stay the same due to fall through but it's not quite so
> clear since it's not consistent with the error handling style for the
> rest of the function.

You mean in this case you'd want to see a (needless) GOTO?  I suppose
it could be done.  If you want consistency, I'd get rid of the unique
message for missing constraints, and rely on unique fault codes ...

- Dave