Re: [Patch v3 2/7] Regulator: DA9055 Regulator driver

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

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

On Thu, Oct 11, 2012 at 03:39:24PM +0530, Ashish Jangam wrote:

> This is the Regulator patch for the DA9055 PMIC and has got dependency on
> the DA9055 MFD core.

Always submit patches with subject lines appropriate for the subsystem,
this helps get your patch noticed.  People do things like search their
mailboxes for subsystem prefixes when looking for things they need to
review.

> This patch support all of the DA9055 regulators. The output voltages are
> fully programmable through I2C interface only. The platform data with regulation
> constraints is passed down from the board to the regulator.
> 
> +	switch (mode) {
> +	case REGULATOR_MODE_FAST:
> +		val = DA9055_BUCK_MODE_SYNC << info->mode.shift;
> +		break;
> +	case REGULATOR_MODE_NORMAL:
> +		val = DA9055_BUCK_MODE_AUTO << info->mode.shift;
> +		break;
> +	case REGULATOR_MODE_IDLE:
> +	case REGULATOR_MODE_STANDBY:
> +		val = DA9055_BUCK_MODE_SLEEP << info->mode.shift;
> +		break;

_IDLE and _STANDBY should have different effects if they're both
implemented; pick one.  From the rest of the code it looks like it
should be _STANDBY.

> +	switch (mode) {
> +	case REGULATOR_MODE_NORMAL:
> +	case REGULATOR_MODE_FAST:
> +		val = DA9055_LDO_MODE_SYNC;
> +		break;
> +	case REGULATOR_MODE_IDLE:
> +	case REGULATOR_MODE_STANDBY:
> +		val = DA9055_LDO_MODE_SLEEP;
> +	}

Similarly here.  You're also missing a break;

> +	/* Get the voltage for the activer register set A/B */
> +	if (ret == DA9055_REGUALTOR_SET_A)
> +		ret = da9055_reg_read(regulator->da9055, volt.reg_a);
> +	else
> +		ret = da9055_reg_read(regulator->da9055, volt.reg_b);
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	sel = ((ret & volt.v_mask) - volt.v_offset);

Why not just use the register values directly and refuse to write ones
That are too low?  This would simplify things a little as you'd only
need to check 

> +	/* Set the voltage */
> +	if (ret == DA9055_REGUALTOR_SET_A)
> +		return da9055_regulator_set_voltage_bits(rdev, info->volt.reg_a,
> +							 selector);
> +
> +	return da9055_regulator_set_voltage_bits(rdev, info->volt.reg_b,
> +						 selector);

This is confusingly written - it should be either a switch or an if/else
really.

> +	/* Select register set B for suspend voltage ramping. */
> +	ret = da9055_reg_update(regulator->da9055, info->conf.reg,
> +				info->conf.sel_mask, DA9055_SEL_REG_B);
> +	if (ret < 0)
> +		return ret;

This doesn't seem like it plays nicely with the GPIO selection in normal
set_voltage() - does it need to check to see if register set B might be
used in normal operation and refuse to run if it could?

> +	for (i = 0; i < ARRAY_SIZE(da9055_regulator_info); i++) {
> +		info = &da9055_regulator_info[i];
> +		if (info->reg_desc.id == id)
> +			return info;
> +		}
> +

The indentation here is *very* messed up.  I'd suggest not omitting any
braces.

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