Re: [PATCHv5 5/11] REGULATOR: Regulator module of DA9052 device driver

[Mark Brown <broonie@opensource.wolfsonmicro.com>]

On Tue, Dec 21, 2010 at 07:10:12PM +0100, dd diasemi wrote:

> Linux Kernel Version: 2.6.34

Always submit drivers against current development kernels, see the git
trees referenced in MAINTAINERS - linux-next is usually a good
approximation.  Drivers generated against older kernels will frequently
not even build against current kernels.  I'm fairly sure this has been
mentioned to you on previous submissions.

I've *briefly* glanced through the code below but not thoroughly
reviewed due to the old kernel version.

> +struct regulator {
> +	struct device *dev;
> +	struct list_head list;
> +	int uA_load;
> +	int min_uV;
> +	int max_uV;
> +	int enabled;
> +	char *supply_name;
> +	struct device_attribute dev_attr;
> +	struct regulator_dev *rdev;
> +};

Why are you replicating this in your driver?  You should never even look
at the interenals of this structure...

> +	da9052_lock(priv->da9052);
> +	ret = priv->da9052->read(priv->da9052, &ssc_msg);
> +	if (ret) {
> +		da9052_unlock(priv->da9052);
> +		return -EIO;
> +	}
> +
> +	ssc_msg.data = (ssc_msg.data | da9052_regulators[id].en_bit_mask);
> +
> +	ret = priv->da9052->write(priv->da9052, &ssc_msg);
> +	if (ret) {
> +		da9052_unlock(priv->da9052);
> +		return -EIO;
> +	}
> +	da9052_unlock(priv->da9052);

This register manpulation looks like it really should be factored out
into the MFD driver - there's quite a few copies of essentially the same
code in the regulator driver alone.

> +			(DA9052_BUCK_PERI_STEP_ABOVE_3000);
> +	} else{

checkpatch.pl will spot style issues like this for you.

> +	priv->da9052 = da9052;
> +	for (i = 0; i < 14; i++) {

14 is a magic number...  should be something more meaningful.

> +/* PM Device name and static Major number macros */
> +#define DRIVER_NAME				"da9052-regulator"

This shouldn't be in a header where other code might see it.

> +/* PM Device Error Codes */
> +

Remove this empty section.