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

[Mark Brown <broonie@sirena.org.uk>]

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

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

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