[PATCH] Regulator: lp3972 cleanup

[PATCH] Regulator: lp3972 cleanup

[Axel Lin]

This patch includes below fixes based on Mark's comment.
 - Return actual error if i2c_smbus_read_byte_data() fail
 - Add spaces around bitwise AND operator(&) to improve readability
 - Add comment to explain why we need to update voltage change control register
   for LDO1 and LDO5
 - Logging the value for diagnostics if chip reported incorrect voltage value
 - Add __devinit annotation for setup_regulators()
 - Logging the value for diagnostics if failed to detect device

Signed-off-by: Axel Lin <axel.lin@gmail.com>
---

Each fix item is trivial, so I sent the fixes in one patch.

 drivers/regulator/lp3972.c |   20 ++++++++++++++------
 1 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/drivers/regulator/lp3972.c b/drivers/regulator/lp3972.c
index 0d9b475..c22650a 100644
--- a/drivers/regulator/lp3972.c
+++ b/drivers/regulator/lp3972.c
@@ -192,7 +192,7 @@ static int lp3972_i2c_read(struct i2c_client *i2c, char reg, int count,
 		return -EIO;
 	ret = i2c_smbus_read_byte_data(i2c, reg);
 	if (ret < 0)
-		return -EIO;
+		return ret;
 
 	*dest = ret;
 	return 0;
@@ -215,7 +215,7 @@ static u8 lp3972_reg_read(struct lp3972 *lp3972, u8 reg)
 	lp3972_i2c_read(lp3972->i2c, reg, 1, &val);
 
 	dev_dbg(lp3972->dev, "reg read 0x%02x -> 0x%02x\n", (int)reg,
-		(unsigned)val&0xff);
+		(unsigned)val & 0xff);
 
 	mutex_unlock(&lp3972->io_lock);
 
@@ -234,7 +234,7 @@ static int lp3972_set_bits(struct lp3972 *lp3972, u8 reg, u16 mask, u16 val)
 	if (ret == 0) {
 		ret = lp3972_i2c_write(lp3972->i2c, reg, 1, &tmp);
 		dev_dbg(lp3972->dev, "reg write 0x%02x -> 0x%02x\n", (int)reg,
-			(unsigned)val&0xff);
+			(unsigned)val & 0xff);
 	}
 	mutex_unlock(&lp3972->io_lock);
 
@@ -320,6 +320,13 @@ static int lp3972_ldo_set_voltage(struct regulator_dev *dev,
 	if (ret)
 		return ret;
 
+	/*
+	 * LDO1 and LDO5 support voltage control by either target voltage1
+	 * or target voltage2 register.
+	 * We use target voltage1 register for LDO1 and LDO5 in this driver.
+	 * We need to update voltage change control register(0x20) to enable
+	 * LDO1 and LDO5 to change to their programmed target values.
+	 */
 	switch (ldo) {
 	case LP3972_LDO1:
 	case LP3972_LDO5:
@@ -401,7 +408,8 @@ static int lp3972_dcdc_get_voltage(struct regulator_dev *dev)
 		val = 1000 * buck_voltage_map[buck][reg];
 	else {
 		val = 0;
-		dev_warn(&dev->dev, "chip reported incorrect voltage value.\n");
+		dev_warn(&dev->dev, "chip reported incorrect voltage value."
+				    " reg = %d\n", reg);
 	}
 
 	return val;
@@ -523,7 +531,7 @@ static struct regulator_desc regulators[] = {
 	},
 };
 
-static int setup_regulators(struct lp3972 *lp3972,
+static int __devinit setup_regulators(struct lp3972 *lp3972,
 	struct lp3972_platform_data *pdata)
 {
 	int i, err;
@@ -587,7 +595,7 @@ static int __devinit lp3972_i2c_probe(struct i2c_client *i2c,
 	if (ret == 0 && (val & SYS_CONTROL1_INIT_MASK) != SYS_CONTROL1_INIT_VAL)
 		ret = -ENODEV;
 	if (ret < 0) {
-		dev_err(&i2c->dev, "failed to detect device\n");
+		dev_err(&i2c->dev, "failed to detect device: %d\n", ret);
 		goto err_detect;
 	}
 
-- 
1.7.2

Re: [PATCH] Regulator: lp3972 cleanup

[Mark Brown]

On Fri, Sep 17, 2010 at 01:44:17PM +0800, Axel Lin wrote:

>  - Logging the value for diagnostics if failed to detect device

>  	if (ret == 0 && (val & SYS_CONTROL1_INIT_MASK) != SYS_CONTROL1_INIT_VAL)
>  		ret = -ENODEV;
>  	if (ret < 0) {
> -		dev_err(&i2c->dev, "failed to detect device\n");
> +		dev_err(&i2c->dev, "failed to detect device: %d\n", ret);

This misses part of the point of my suggestion - if you don't match the
ID register then you'll log -ENODEV rather than the device ID that you
read back which would presumably be more useful for someone trying to
work out why the check triggered.

Lots of other good stuff in there, though:

Acked-by: Mark Brown <broonie@opensource.wolfsonmicro.com>

Re: [PATCH] Regulator: lp3972 cleanup

[Liam Girdwood]

On Fri, 2010-09-17 at 13:28 +0100, Mark Brown wrote:
> On Fri, Sep 17, 2010 at 01:44:17PM +0800, Axel Lin wrote:
> 
> >  - Logging the value for diagnostics if failed to detect device
> 
> >  	if (ret == 0 && (val & SYS_CONTROL1_INIT_MASK) != SYS_CONTROL1_INIT_VAL)
> >  		ret = -ENODEV;
> >  	if (ret < 0) {
> > -		dev_err(&i2c->dev, "failed to detect device\n");
> > +		dev_err(&i2c->dev, "failed to detect device: %d\n", ret);
> 
> This misses part of the point of my suggestion - if you don't match the
> ID register then you'll log -ENODEV rather than the device ID that you
> read back which would presumably be more useful for someone trying to
> work out why the check triggered.
> 
> Lots of other good stuff in there, though:
> 
> Acked-by: Mark Brown <broonie@opensource.wolfsonmicro.com>

Applied.

Thanks

Liam
-- 
Freelance Developer, SlimLogic Ltd
ASoC and Voltage Regulator Maintainer.
http://www.slimlogic.co.uk