[PATCH 0/6] mfd: TPS65910 bug fixes and enhancements

[PATCH 0/6] mfd: TPS65910 bug fixes and enhancements

[Kyle Manna]

This series of patches fixes a number of small problems with the TPS65910 that
were discovered on a custom board.

Additionally, the last patch enhances the platform to driver interface using the
TWL4030 series of regulators as an example.

Kyle Manna (6):
  mfd: TPS65910: Handle non-existent devices
  mfd: TPS65910: Add I2C slave address macros
  mfd: TPS65910: Fix typo that clobbers genirq
  mfd: TPS65910: Move linux/gpio.h include to header
  mfd: TPS65910: Fix tps65910_set_voltage
  mfd: TPS65910: Improve regulator init data

 drivers/mfd/tps65910.c                 |   23 ++-
 drivers/regulator/tps65910-regulator.c |  246 +++++++++++++++++++++++++-------
 include/linux/mfd/tps65910.h           |   36 +++++-
 3 files changed, 247 insertions(+), 58 deletions(-)

-- 
1.7.5.4

[PATCH 1/6] mfd: TPS65910: Handle non-existent devices

[Kyle Manna]

Attempt to read the first register of the device, if there is no
device return -ENODEV

Signed-off-by: Kyle Manna <kyle.manna@fuel7.com>
---
 drivers/mfd/tps65910.c |   20 ++++++++++++++------
 1 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/drivers/mfd/tps65910.c b/drivers/mfd/tps65910.c
index 6f5b8cf..03fb4a7 100644
--- a/drivers/mfd/tps65910.c
+++ b/drivers/mfd/tps65910.c
@@ -138,6 +138,7 @@ static int tps65910_i2c_probe(struct i2c_client *i2c,
 	struct tps65910_board *pmic_plat_data;
 	struct tps65910_platform_data *init_data;
 	int ret = 0;
+	char buff;
 
 	pmic_plat_data = dev_get_platdata(&i2c->dev);
 	if (!pmic_plat_data)
@@ -161,11 +162,17 @@ static int tps65910_i2c_probe(struct i2c_client *i2c,
 	tps65910->write = tps65910_i2c_write;
 	mutex_init(&tps65910->io_mutex);
 
-	ret = mfd_add_devices(tps65910->dev, -1,
-			      tps65910s, ARRAY_SIZE(tps65910s),
-			      NULL, 0);
-	if (ret < 0)
+	/* Check that the device is actually there */
+	ret = tps65910_i2c_read(tps65910, 0x0, 1, &buff);
+	if (ret < 0) {
+		ret = -ENODEV;
 		goto err;
+	}
+
+	ret = mfd_add_devices(tps65910->dev, -1, tps65910s,
+			ARRAY_SIZE(tps65910s), NULL, 0);
+	if (ret < 0)
+		goto err2;
 
 	init_data->irq = pmic_plat_data->irq;
 	init_data->irq_base = pmic_plat_data->irq;
@@ -174,13 +181,14 @@ static int tps65910_i2c_probe(struct i2c_client *i2c,
 
 	ret = tps65910_irq_init(tps65910, init_data->irq, init_data);
 	if (ret < 0)
-		goto err;
+		goto err2;
 
 	kfree(init_data);
 	return ret;
 
-err:
+err2:
 	mfd_remove_devices(tps65910->dev);
+err:
 	kfree(tps65910);
 	kfree(init_data);
 	return ret;
-- 
1.7.5.4

[PATCH 2/6] mfd: TPS65910: Add I2C slave address macros

[Kyle Manna]

Add I2C slave addresses to the header file so that platform definitions
can use them.

Signed-off-by: Kyle Manna <kyle.manna@fuel7.com>
---
 include/linux/mfd/tps65910.h |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/include/linux/mfd/tps65910.h b/include/linux/mfd/tps65910.h
index 82b4c88..399d80a 100644
--- a/include/linux/mfd/tps65910.h
+++ b/include/linux/mfd/tps65910.h
@@ -21,6 +21,10 @@
 #define TPS65910			0
 #define TPS65911			1
 
+/* I2C Slave Address 7-bit */
+#define TPS65910_I2C_ID0 0x12 /* Smart Reflex */
+#define TPS65910_I2C_ID1 0x2D /* general-purpose control */
+
 /* TPS regulator type list */
 #define REGULATOR_LDO			0
 #define REGULATOR_DCDC			1
-- 
1.7.5.4

[PATCH 3/6] mfd: TPS65910: Fix typo that clobbers genirq

[Kyle Manna]

Fix a typo that clobbers other interrupts in an unobvious way.

Signed-off-by: Kyle Manna <kyle.manna@fuel7.com>
---
 drivers/mfd/tps65910.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/mfd/tps65910.c b/drivers/mfd/tps65910.c
index 03fb4a7..f4654d5 100644
--- a/drivers/mfd/tps65910.c
+++ b/drivers/mfd/tps65910.c
@@ -175,7 +175,7 @@ static int tps65910_i2c_probe(struct i2c_client *i2c,
 		goto err2;
 
 	init_data->irq = pmic_plat_data->irq;
-	init_data->irq_base = pmic_plat_data->irq;
+	init_data->irq_base = pmic_plat_data->irq_base;
 
 	tps65910_gpio_init(tps65910, pmic_plat_data->gpio_base);
 
-- 
1.7.5.4

[PATCH 4/6] mfd: TPS65910: Move linux/gpio.h include to header

[Kyle Manna]

The tps65910.h file depends on linux/gpio.h.  Move the include from the
source file to the tps65910.h header file.

Signed-off-by: Kyle Manna <kyle.manna@fuel7.com>
---
 drivers/mfd/tps65910.c       |    1 -
 include/linux/mfd/tps65910.h |    3 +++
 2 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/mfd/tps65910.c b/drivers/mfd/tps65910.c
index f4654d5..36bc08c 100644
--- a/drivers/mfd/tps65910.c
+++ b/drivers/mfd/tps65910.c
@@ -18,7 +18,6 @@
 #include <linux/init.h>
 #include <linux/slab.h>
 #include <linux/i2c.h>
-#include <linux/gpio.h>
 #include <linux/mfd/core.h>
 #include <linux/mfd/tps65910.h>
 
diff --git a/include/linux/mfd/tps65910.h b/include/linux/mfd/tps65910.h
index 399d80a..503ded3 100644
--- a/include/linux/mfd/tps65910.h
+++ b/include/linux/mfd/tps65910.h
@@ -17,6 +17,9 @@
 #ifndef __LINUX_MFD_TPS65910_H
 #define __LINUX_MFD_TPS65910_H
 
+#include <linux/gpio.h>
+#include <linux/regulator/machine.h>
+
 /* TPS chip id list */
 #define TPS65910			0
 #define TPS65911			1
-- 
1.7.5.4

[PATCH 5/6] mfd: TPS65910: Fix tps65910_set_voltage

[Kyle Manna]

Previously tps65910_set_voltage() only selected from a fixed number of
voltages.  Rename that function to tps65910_set_voltage_sel(). Do the
same for tps65911_set_voltage().

Also add a tps65910_set_voltage that works with the regulator framework
and applies the correct voltage with apply_uv is set in the regulator's
constraints.

This was tested on a TPS65910.

Signed-off-by: Kyle Manna <kyle.manna@fuel7.com>
---
 drivers/regulator/tps65910-regulator.c |   37 ++++++++++++++++++++++++++++---
 1 files changed, 33 insertions(+), 4 deletions(-)

diff --git a/drivers/regulator/tps65910-regulator.c b/drivers/regulator/tps65910-regulator.c
index 66d2d60..44ce2b0 100644
--- a/drivers/regulator/tps65910-regulator.c
+++ b/drivers/regulator/tps65910-regulator.c
@@ -693,7 +693,8 @@ static int tps65910_set_voltage_dcdc(struct regulator_dev *dev,
 	return 0;
 }
 
-static int tps65910_set_voltage(struct regulator_dev *dev, unsigned selector)
+static int tps65910_set_voltage_sel(struct regulator_dev *dev,
+				    unsigned selector)
 {
 	struct tps65910_reg *pmic = rdev_get_drvdata(dev);
 	int reg, id = rdev_get_id(dev);
@@ -719,7 +720,35 @@ static int tps65910_set_voltage(struct regulator_dev *dev, unsigned selector)
 	return -EINVAL;
 }
 
-static int tps65911_set_voltage(struct regulator_dev *dev, unsigned selector)
+static int tps65910_set_voltage(struct regulator_dev *dev,
+		int min_uV, int max_uV, unsigned *selector)
+{
+	int id = rdev_get_id(dev);
+	int i;
+	int new_uV = 0, selected_uV = 0;
+	int midpoint = (max_uV + min_uV) >> 1;
+
+	/* Pick the nearest selector */
+	for (i = 0; i < tps65910_regs[id].table_len; i++) {
+		new_uV = tps65910_regs[id].table[i] * 1000;
+
+		if (new_uV >= min_uV && new_uV <= max_uV &&
+		   (abs(new_uV - midpoint) < abs(selected_uV - midpoint))) {
+			*selector = i;
+			selected_uV = tps65910_regs[id].table[i] * 1000;
+		}
+	}
+
+	/* If a match was found, set it */
+	if (selected_uV)
+		return tps65910_set_voltage_sel(dev, *selector);
+
+	return -EINVAL;
+}
+
+
+static int tps65911_set_voltage_sel(struct regulator_dev *dev,
+				    unsigned selector)
 {
 	struct tps65910_reg *pmic = rdev_get_drvdata(dev);
 	int reg, id = rdev_get_id(dev);
@@ -856,7 +885,7 @@ static struct regulator_ops tps65910_ops = {
 	.set_mode		= tps65910_set_mode,
 	.get_mode		= tps65910_get_mode,
 	.get_voltage		= tps65910_get_voltage,
-	.set_voltage_sel	= tps65910_set_voltage,
+	.set_voltage		= tps65910_set_voltage,
 	.list_voltage		= tps65910_list_voltage,
 };
 
@@ -867,7 +896,7 @@ static struct regulator_ops tps65911_ops = {
 	.set_mode		= tps65910_set_mode,
 	.get_mode		= tps65910_get_mode,
 	.get_voltage		= tps65911_get_voltage,
-	.set_voltage_sel	= tps65911_set_voltage,
+	.set_voltage_sel	= tps65911_set_voltage_sel,
 	.list_voltage		= tps65911_list_voltage,
 };
 
-- 
1.7.5.4

[PATCH 6/6] mfd: TPS65910: Improve regulator init data

[Kyle Manna]

Improve the interface between platform code/board files to the TPS65910
regulators.  The TWL4030/6030 code was used as an example interface.

This improved interface will allow use of the regulators without
specifying all the constraints. Also gets rid of an assumption that
the platform pass in an array of correct size and was unchecked.

Signed-off-by: Kyle Manna <kyle.manna@fuel7.com>
---
 drivers/regulator/tps65910-regulator.c |  209 +++++++++++++++++++++++++-------
 include/linux/mfd/tps65910.h           |   29 +++++-
 2 files changed, 192 insertions(+), 46 deletions(-)

diff --git a/drivers/regulator/tps65910-regulator.c b/drivers/regulator/tps65910-regulator.c
index 44ce2b0..4c2ae4b 100644
--- a/drivers/regulator/tps65910-regulator.c
+++ b/drivers/regulator/tps65910-regulator.c
@@ -900,22 +900,71 @@ static struct regulator_ops tps65911_ops = {
 	.list_voltage		= tps65911_list_voltage,
 };
 
+static __devinit struct regulator_dev *add_regulator(
+		struct tps65910_reg *pmic,
+		struct tps_info *info,
+		int i,
+		const struct regulator_init_data *init_data)
+{
+	struct tps65910 *tps65910 = pmic->mfd;
+	struct regulator_dev *rdev;
+
+	/* Register the regulators */
+	pmic->info[i] = &info[i];
+
+	if (init_data->constraints.name)
+		pmic->desc[i].name = init_data->constraints.name;
+	else
+		pmic->desc[i].name = info[i].name;
+
+	pmic->desc[i].id = i;
+	pmic->desc[i].n_voltages = info->table_len;
+
+	if (i == TPS65910_REG_VDD1 || i == TPS65910_REG_VDD2) {
+		pmic->desc[i].ops = &tps65910_ops_dcdc;
+	} else if (i == TPS65910_REG_VDD3) {
+		if (tps65910_chip_id(tps65910) == TPS65910)
+			pmic->desc[i].ops = &tps65910_ops_vdd3;
+		else
+			pmic->desc[i].ops = &tps65910_ops_dcdc;
+	} else {
+		if (tps65910_chip_id(tps65910) == TPS65910)
+			pmic->desc[i].ops = &tps65910_ops;
+		else
+			pmic->desc[i].ops = &tps65911_ops;
+	}
+
+	pmic->desc[i].type = REGULATOR_VOLTAGE;
+	pmic->desc[i].owner = THIS_MODULE;
+
+	rdev = regulator_register(&pmic->desc[i],
+			tps65910->dev, init_data, pmic);
+	if (IS_ERR(rdev)) {
+		dev_err(tps65910->dev,
+				"failed to register %s regulator\n",
+				pmic->desc[i].name);
+		rdev = (struct regulator_dev *)PTR_ERR(rdev);
+	} else {
+		/* Save regulator for cleanup */
+		pmic->rdev[i] = rdev;
+	}
+
+	return rdev;
+}
+
 static __devinit int tps65910_probe(struct platform_device *pdev)
 {
 	struct tps65910 *tps65910 = dev_get_drvdata(pdev->dev.parent);
 	struct tps_info *info;
-	struct regulator_init_data *reg_data;
 	struct regulator_dev *rdev;
 	struct tps65910_reg *pmic;
 	struct tps65910_board *pmic_plat_data;
-	int i, err;
+	int err;
 
 	pmic_plat_data = dev_get_platdata(tps65910->dev);
 	if (!pmic_plat_data)
 		return -EINVAL;
 
-	reg_data = pmic_plat_data->tps65910_pmic_init_data;
-
 	pmic = kzalloc(sizeof(*pmic), GFP_KERNEL);
 	if (!pmic)
 		return -ENOMEM;
@@ -966,50 +1015,120 @@ static __devinit int tps65910_probe(struct platform_device *pdev)
 		goto err_free_info;
 	}
 
-	for (i = 0; i < pmic->num_regulators; i++, info++, reg_data++) {
-		/* Register the regulators */
-		pmic->info[i] = info;
-
-		pmic->desc[i].name = info->name;
-		pmic->desc[i].id = i;
-		pmic->desc[i].n_voltages = info->table_len;
-
-		if (i == TPS65910_REG_VDD1 || i == TPS65910_REG_VDD2) {
-			pmic->desc[i].ops = &tps65910_ops_dcdc;
-		} else if (i == TPS65910_REG_VDD3) {
-			if (tps65910_chip_id(tps65910) == TPS65910)
-				pmic->desc[i].ops = &tps65910_ops_vdd3;
-			else
-				pmic->desc[i].ops = &tps65910_ops_dcdc;
-		} else {
-			if (tps65910_chip_id(tps65910) == TPS65910)
-				pmic->desc[i].ops = &tps65910_ops;
-			else
-				pmic->desc[i].ops = &tps65911_ops;
-		}
-
-		pmic->desc[i].type = REGULATOR_VOLTAGE;
-		pmic->desc[i].owner = THIS_MODULE;
-
-		rdev = regulator_register(&pmic->desc[i],
-				tps65910->dev, reg_data, pmic);
-		if (IS_ERR(rdev)) {
-			dev_err(tps65910->dev,
-				"failed to register %s regulator\n",
-				pdev->name);
-			err = PTR_ERR(rdev);
-			goto err_unregister_regulator;
-		}
-
-		/* Save regulator for cleanup */
-		pmic->rdev[i] = rdev;
+	/* TPS65910 and TPS65911 Regulators */
+	rdev = add_regulator(pmic, info, TPS65910_REG_VRTC,
+			pmic_plat_data->vrtc);
+	if (IS_ERR(rdev))
+		return PTR_ERR(rdev);
+	rdev = add_regulator(pmic, info, TPS65910_REG_VIO,
+			pmic_plat_data->vio);
+
+	if (IS_ERR(rdev))
+		return PTR_ERR(rdev);
+
+	rdev = add_regulator(pmic, info, TPS65910_REG_VDD1,
+			pmic_plat_data->vdd1);
+	if (IS_ERR(rdev))
+		return PTR_ERR(rdev);
+
+	rdev = add_regulator(pmic, info, TPS65910_REG_VDD2,
+			pmic_plat_data->vdd2);
+	if (IS_ERR(rdev))
+		return PTR_ERR(rdev);
+
+	if (tps65910_chip_id(tps65910) == TPS65910) {
+		/* TPS65910 Regulators */
+		rdev = add_regulator(pmic, info, TPS65910_REG_VDD3,
+				pmic_plat_data->vdd3);
+		if (IS_ERR(rdev))
+			return PTR_ERR(rdev);
+
+		rdev = add_regulator(pmic, info, TPS65910_REG_VDIG1,
+				pmic_plat_data->vdig1);
+		if (IS_ERR(rdev))
+			return PTR_ERR(rdev);
+
+		rdev = add_regulator(pmic, info, TPS65910_REG_VDIG2,
+				pmic_plat_data->vdig2);
+		if (IS_ERR(rdev))
+			return PTR_ERR(rdev);
+
+		rdev = add_regulator(pmic, info, TPS65910_REG_VDAC,
+				pmic_plat_data->vdac);
+		if (IS_ERR(rdev))
+			return PTR_ERR(rdev);
+
+		rdev = add_regulator(pmic, info, TPS65910_REG_VAUX1,
+				pmic_plat_data->vaux1);
+		if (IS_ERR(rdev))
+			return PTR_ERR(rdev);
+
+		rdev = add_regulator(pmic, info, TPS65910_REG_VAUX2,
+				pmic_plat_data->vaux2);
+		if (IS_ERR(rdev))
+			return PTR_ERR(rdev);
+
+		rdev = add_regulator(pmic, info, TPS65910_REG_VAUX33,
+				pmic_plat_data->vaux33);
+		if (IS_ERR(rdev))
+			return PTR_ERR(rdev);
+
+		rdev = add_regulator(pmic, info, TPS65910_REG_VMMC,
+				pmic_plat_data->vmmc);
+		if (IS_ERR(rdev))
+			return PTR_ERR(rdev);
+
+
+	} else if (tps65910_chip_id(tps65910) == TPS65910) {
+		/* TPS65911 Regulators */
+		rdev = add_regulator(pmic, info, TPS65911_REG_VDDCTRL,
+				pmic_plat_data->vddctrl);
+		if (IS_ERR(rdev))
+			return PTR_ERR(rdev);
+
+		rdev = add_regulator(pmic, info, TPS65911_REG_LDO1,
+				pmic_plat_data->ldo1);
+		if (IS_ERR(rdev))
+			return PTR_ERR(rdev);
+
+		rdev = add_regulator(pmic, info, TPS65911_REG_LDO2,
+				pmic_plat_data->ldo2);
+		if (IS_ERR(rdev))
+			return PTR_ERR(rdev);
+
+		rdev = add_regulator(pmic, info, TPS65911_REG_LDO3,
+				pmic_plat_data->ldo3);
+		if (IS_ERR(rdev))
+			return PTR_ERR(rdev);
+
+		rdev = add_regulator(pmic, info, TPS65911_REG_LDO4,
+				pmic_plat_data->ldo4);
+		if (IS_ERR(rdev))
+			return PTR_ERR(rdev);
+
+		rdev = add_regulator(pmic, info, TPS65911_REG_LDO5,
+				pmic_plat_data->ldo5);
+		if (IS_ERR(rdev))
+			return PTR_ERR(rdev);
+
+		rdev = add_regulator(pmic, info, TPS65911_REG_LDO6,
+				pmic_plat_data->ldo6);
+		if (IS_ERR(rdev))
+			return PTR_ERR(rdev);
+
+		rdev = add_regulator(pmic, info, TPS65911_REG_LDO7,
+				pmic_plat_data->ldo7);
+		if (IS_ERR(rdev))
+			return PTR_ERR(rdev);
+
+		rdev = add_regulator(pmic, info, TPS65911_REG_LDO8,
+				pmic_plat_data->ldo8);
+		if (IS_ERR(rdev))
+			return PTR_ERR(rdev);
 	}
+
 	return 0;
 
-err_unregister_regulator:
-	while (--i >= 0)
-		regulator_unregister(pmic->rdev[i]);
-	kfree(pmic->rdev);
 err_free_info:
 	kfree(pmic->info);
 err_free_desc:
diff --git a/include/linux/mfd/tps65910.h b/include/linux/mfd/tps65910.h
index 503ded3..38d9821 100644
--- a/include/linux/mfd/tps65910.h
+++ b/include/linux/mfd/tps65910.h
@@ -757,7 +757,34 @@ struct tps65910_board {
 	int irq_base;
 	int vmbch_threshold;
 	int vmbch2_threshold;
-	struct regulator_init_data *tps65910_pmic_init_data;
+
+	/* TPS65910 and TPS65911 Regulators */
+	struct regulator_init_data *vrtc;
+	struct regulator_init_data *vio;
+	struct regulator_init_data *vdd1;
+	struct regulator_init_data *vdd2;
+
+	/* TPS65910 Regulators */
+	struct regulator_init_data *vdd3;
+	struct regulator_init_data *vdig1;
+	struct regulator_init_data *vdig2;
+	struct regulator_init_data *vpll;
+	struct regulator_init_data *vdac;
+	struct regulator_init_data *vaux1;
+	struct regulator_init_data *vaux2;
+	struct regulator_init_data *vaux33;
+	struct regulator_init_data *vmmc;
+
+	/* TPS65911 Regulators */
+	struct regulator_init_data *vddctrl;
+	struct regulator_init_data *ldo1;
+	struct regulator_init_data *ldo2;
+	struct regulator_init_data *ldo3;
+	struct regulator_init_data *ldo4;
+	struct regulator_init_data *ldo5;
+	struct regulator_init_data *ldo6;
+	struct regulator_init_data *ldo7;
+	struct regulator_init_data *ldo8;
 };
 
 /**
-- 
1.7.5.4

Re: [PATCH 0/6] mfd: TPS65910 bug fixes and enhancements

[Graeme Gregory]

On 18/10/11 19:26, Kyle Manna wrote:
> This series of patches fixes a number of small problems with the TPS65910 that
> were discovered on a custom board.
>
> Additionally, the last patch enhances the platform to driver interface using the
> TWL4030 series of regulators as an example.
>
> Kyle Manna (6):
>    mfd: TPS65910: Handle non-existent devices
>    mfd: TPS65910: Add I2C slave address macros
>    mfd: TPS65910: Fix typo that clobbers genirq
>    mfd: TPS65910: Move linux/gpio.h include to header
>    mfd: TPS65910: Fix tps65910_set_voltage
>    mfd: TPS65910: Improve regulator init data
>
>   drivers/mfd/tps65910.c                 |   23 ++-
>   drivers/regulator/tps65910-regulator.c |  246 +++++++++++++++++++++++++-------
>   include/linux/mfd/tps65910.h           |   36 +++++-
>   3 files changed, 247 insertions(+), 58 deletions(-)
>
In my opinion patches 5 and 6 are regressions.

set_voltage_sel was added to core api to allow the voltage selection 
code that a lot of drivers implemented identically to be abstracted to 
the regulator core.

The twl4030 driver method of defining platform data has left us with a 
very inflexible way of adding functionality when a new variant of the 
chip is released. This was remarked on by a number of kernel developers 
when I upstreamed the twl6025 support. I feel copying the twl driver is 
the wrong direction to go in in this regard.

Graeme

Re: [PATCH 5/6] mfd: TPS65910: Fix tps65910_set_voltage

[Mark Brown]

On Tue, Oct 18, 2011 at 01:26:27PM -0500, Kyle Manna wrote:

*Always* CC maintainers on patches.

> Previously tps65910_set_voltage() only selected from a fixed number of
> voltages.  Rename that function to tps65910_set_voltage_sel(). Do the
> same for tps65911_set_voltage().

What is the issue being fixed here?  This looks like a stylistic change
rateher than a bug fix.

> Also add a tps65910_set_voltage that works with the regulator framework
> and applies the correct voltage with apply_uv is set in the regulator's
> constraints.

So this is adding support for a new chip?  Whatever the answer it's
clearly a distinct change from the above and should therefore be a
separate patch.

> +	/* Pick the nearest selector */
> +	for (i = 0; i < tps65910_regs[id].table_len; i++) {
> +		new_uV = tps65910_regs[id].table[i] * 1000;
> +
> +		if (new_uV >= min_uV && new_uV <= max_uV &&
> +		   (abs(new_uV - midpoint) < abs(selected_uV - midpoint))) {
> +			*selector = i;
> +			selected_uV = tps65910_regs[id].table[i] * 1000;
> +		}
> +	}

This looks wrong, the expected behaviour for the regulator API is that
the driver will pick the minimum voltage within the range.  Why is this
being done?

Re: [PATCH 6/6] mfd: TPS65910: Improve regulator init data

[Mark Brown]

On Tue, Oct 18, 2011 at 01:26:28PM -0500, Kyle Manna wrote:
> Improve the interface between platform code/board files to the TPS65910

Again, *always* CC maintainers on patches.

> regulators.  The TWL4030/6030 code was used as an example interface.

This isn't a good sign...

> This improved interface will allow use of the regulators without
> specifying all the constraints. Also gets rid of an assumption that
> the platform pass in an array of correct size and was unchecked.

You've not described the changes between the two interfaces.  Note that
empty constraints should be absolutely fine with the API.

> +	if (init_data->constraints.name)
> +		pmic->desc[i].name = init_data->constraints.name;
> +	else
> +		pmic->desc[i].name = info[i].name;

No, this is broken.  The name of the regulator is a fixed property of
the device and isn't something that ought to be overridden per system.

> +	/* TPS65910 and TPS65911 Regulators */
> +	rdev = add_regulator(pmic, info, TPS65910_REG_VRTC,
> +			pmic_plat_data->vrtc);
> +	if (IS_ERR(rdev))
> +		return PTR_ERR(rdev);
> +	rdev = add_regulator(pmic, info, TPS65910_REG_VIO,
> +			pmic_plat_data->vio);
> +
> +	if (IS_ERR(rdev))
> +		return PTR_ERR(rdev);
> +
> +	rdev = add_regulator(pmic, info, TPS65910_REG_VDD1,
> +			pmic_plat_data->vdd1);
> +	if (IS_ERR(rdev))
> +		return PTR_ERR(rdev);

This looks like a regression - we've gone from looping over an array
which is nice and simple to explicit code for each individual regulator
giving us lots of repetitive code...

> -err_unregister_regulator:
> -	while (--i >= 0)
> -		regulator_unregister(pmic->rdev[i]);
> -	kfree(pmic->rdev);

...and loosing all our cleanup if things go wrong which isn't great
either.

Re: [PATCH 5/6] mfd: TPS65910: Fix tps65910_set_voltage

[Kyle Manna]

Hi Mark,

I've reviewed the patch and the core regulator framework after reading 
your comments.  This patch worked around a subtle bug that I didn't 
notice and is hence not needed.  The root cause was fixed and the 
voltage selectors work as desired.

Will update the patch series and resend.

- Kyle

On 10/19/2011 08:32 AM, Mark Brown wrote:
> On Tue, Oct 18, 2011 at 01:26:27PM -0500, Kyle Manna wrote:
>
> *Always* CC maintainers on patches.
>
>> Previously tps65910_set_voltage() only selected from a fixed number of
>> voltages.  Rename that function to tps65910_set_voltage_sel(). Do the
>> same for tps65911_set_voltage().
> What is the issue being fixed here?  This looks like a stylistic change
> rateher than a bug fix.
>
>> Also add a tps65910_set_voltage that works with the regulator framework
>> and applies the correct voltage with apply_uv is set in the regulator's
>> constraints.
> So this is adding support for a new chip?  Whatever the answer it's
> clearly a distinct change from the above and should therefore be a
> separate patch.
>
>> +	/* Pick the nearest selector */
>> +	for (i = 0; i<  tps65910_regs[id].table_len; i++) {
>> +		new_uV = tps65910_regs[id].table[i] * 1000;
>> +
>> +		if (new_uV>= min_uV&&  new_uV<= max_uV&&
>> +		   (abs(new_uV - midpoint)<  abs(selected_uV - midpoint))) {
>> +			*selector = i;
>> +			selected_uV = tps65910_regs[id].table[i] * 1000;
>> +		}
>> +	}
> This looks wrong, the expected behaviour for the regulator API is that
> the driver will pick the minimum voltage within the range.  Why is this
> being done?

Re: [PATCH 6/6] mfd: TPS65910: Improve regulator init data

[Kyle Manna]

On 10/19/2011 09:00 AM, Mark Brown wrote:
> On Tue, Oct 18, 2011 at 01:26:28PM -0500, Kyle Manna wrote:
>> Improve the interface between platform code/board files to the TPS65910
> Again, *always* CC maintainers on patches.

This was an oversight on my part.

>
>> regulators.  The TWL4030/6030 code was used as an example interface.
> This isn't a good sign...

I've reviewed other PMICs (ie Wolfson Micro ;) ) and will post an 
updated series with an interface similar to what is used there.  The new 
approach makes more sense and keeps the code/patch small.

>
>> This improved interface will allow use of the regulators without
>> specifying all the constraints. Also gets rid of an assumption that
>> the platform pass in an array of correct size and was unchecked.
> You've not described the changes between the two interfaces.  Note that
> empty constraints should be absolutely fine with the API.
>
>> +	if (init_data->constraints.name)
>> +		pmic->desc[i].name = init_data->constraints.name;
>> +	else
>> +		pmic->desc[i].name = info[i].name;
> No, this is broken.  The name of the regulator is a fixed property of
> the device and isn't something that ought to be overridden per system.

Understood.

>
>> +	/* TPS65910 and TPS65911 Regulators */
>> +	rdev = add_regulator(pmic, info, TPS65910_REG_VRTC,
>> +			pmic_plat_data->vrtc);
>> +	if (IS_ERR(rdev))
>> +		return PTR_ERR(rdev);
>> +	rdev = add_regulator(pmic, info, TPS65910_REG_VIO,
>> +			pmic_plat_data->vio);
>> +
>> +	if (IS_ERR(rdev))
>> +		return PTR_ERR(rdev);
>> +
>> +	rdev = add_regulator(pmic, info, TPS65910_REG_VDD1,
>> +			pmic_plat_data->vdd1);
>> +	if (IS_ERR(rdev))
>> +		return PTR_ERR(rdev);
> This looks like a regression - we've gone from looping over an array
> which is nice and simple to explicit code for each individual regulator
> giving us lots of repetitive code...

Will be revised.
>> -err_unregister_regulator:
>> -	while (--i>= 0)
>> -		regulator_unregister(pmic->rdev[i]);
>> -	kfree(pmic->rdev);
> ...and loosing all our cleanup if things go wrong which isn't great
> either.