Re: [PATCHv3 4/4] TPS65910: Add tps65910 regulator driver

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

On Tue, Apr 19, 2011 at 08:27:35PM -0500, Jorge Eduardo Candelaria wrote:
> From: Graeme Gregory <gg@slimlogic.co.uk>
> 
> The regulator module consists of 3 DCDCs and 8 LDOs. The output
> voltages are configurable and are meant to supply power to the
> main processor and other components
> 
> Signed-off-by: Graeme Gregory <gg@slimlogic.co.uk>
> Signed-off-by: Jorge Eduardo Candelaria <jedu@slimlogic.co.uk>

This looks good except for some small nits:

> +		//voltage = pmic->info[id]->table[srvsel] * 100;
> +		voltage = (srvsel * 125 + 6000) * 100;

> +		//voltage = pmic->info[id]->table[opvsel] * 100;
> +		voltage = (opvsel * 125 + 6000) * 100;

Delete the old code.

> +	value = tps65910_reg_read(pmic, reg);
> +	if (value < 0)
> +		return value;
> +
> +	switch (id) {
> +	case TPS65910_REG_VIO:
> +	case TPS65910_REG_VDIG1:
> +	case TPS65910_REG_VDIG2:
> +	case TPS65910_REG_VPLL:
> +	case TPS65910_REG_VDAC:
> +	case TPS65910_REG_VAUX1:
> +	case TPS65910_REG_VAUX2:
> +	case TPS65910_REG_VAUX33:
> +	case TPS65910_REG_VMMC:
> +		value &= ~(LDO_SEL_MASK);
> +		value |= (selector << LDO_SEL_SHIFT);
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return tps65910_reg_write(pmic, reg, value);

This looks like you need a read/modify/write operation for setting
arbatrary bits rather than specifically setting or clearing.