Re: [PATCHv2 4/4] tps65912: add regulator driver

[Mark Brown]

On Thu, May 12, 2011 at 01:44:05PM -0500, Margarita Olaya wrote:
> The tps65912 consist of 4 DCDCs and 10 LDOs. The output voltages can be
> configured by the SPI or I2C interface, they are meant to supply power
> to the main processor and other components.
> 
> Signed-off-by: Margarita Olaya Cabrera <magi@xxxxxxxxxxxxxxx>

A few fairly minor issues, and one thing that I just want you to
confirm...

> +static int tps65912_set_mode(struct regulator_dev *dev, unsigned int mode)
> +{
> +	struct tps65912_reg *pmic = rdev_get_drvdata(dev);
> +	struct tps65912 *mfd = pmic->mfd;
> +	int pwm_mode, eco, reg1, reg2, reg1_val, reg2_val;
> +	int id = rdev_get_id(dev);
> +
> +	switch (id) {
> +	case TPS65912_REG_DCDC1:
> +		reg1 = TPS65912_DCDC1_CTRL;
> +		reg2 = TPS65912_DCDC1_AVS;
> +		break;

A helper function for this switch statement might be useful, it's used
by both get and set mode calls.

> +static int tps65912_set_voltage_ldo(struct regulator_dev *dev,
> +						unsigned selector)
> +{
> +	struct tps65912_reg *pmic = rdev_get_drvdata(dev);
> +	struct tps65912 *mfd = pmic->mfd;
> +	int id = rdev_get_id(dev), reg, value;
> +
> +	reg = tps65912_get_ldo_sel_register(pmic, id);
> +	value = tps65912_reg_read(mfd, reg);
> +	value &= 0xC0;
> +	return tps65912_reg_write(mfd, reg, selector | value);
> +}

I *think* the register only contains things for this regulator so you
don't need locking beyond the lock the regulator API guarantees you but
if that's not the case you'd need to ensure nothing could jump into the
middle of the read/modify/write cycle.

A comment would help.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/