Messages in this thread | | | Date | Mon, 1 Dec 2008 22:53:13 +0000 | From | Mark Brown <> | Subject | Re: [patch 2.6.28-rc6+] regulator: bugfixes and messaging cleanup |
| |
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.
| |