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

[PATCH v2 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.

Changes from v1 based on feedback from Mark Brown:
* Drop the patch #5 from pervious series.  It worked around a bug created by 
  another patch.
* Rework patch #6 completely.


[1] https://lkml.org/lkml/2011/10/19/182
[2] https://lkml.org/lkml/2011/10/19/191

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: Move regulator defs to header
  mfd: TPS65910: Create an array for reg init data

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

-- 
1.7.5.4

[PATCH v2 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

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

[Mark Brown]

On Mon, Oct 24, 2011 at 12:05:59PM -0500, Kyle Manna wrote:
> Attempt to read the first register of the device, if there is no
> device return -ENODEV

Can you check for a known value in any of the registers?

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

[Kyle Manna]

On 10/25/2011 02:14 AM, Mark Brown wrote:
> On Mon, Oct 24, 2011 at 12:05:59PM -0500, Kyle Manna wrote:
>> Attempt to read the first register of the device, if there is no
>> device return -ENODEV
> Can you check for a known value in any of the registers?
Unfortunately on this chip there aren't any identification or constant 
test registers.  The only register close is the JTAGVERNUM_REG and 
that's will change across silicon revisions.

- Kyle

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

[Mark Brown]

On Tue, Oct 25, 2011 at 10:10:05AM -0500, Kyle Manna wrote:
> On 10/25/2011 02:14 AM, Mark Brown wrote:

> >Can you check for a known value in any of the registers?

> Unfortunately on this chip there aren't any identification or
> constant test registers.  The only register close is the
> JTAGVERNUM_REG and that's will change across silicon revisions.

It seems like a more useful implementation would be to have the driver
read and display this revision number during boot, this will give you
the register readability check and also let us know the device revision
which might be useful for diagnostics.

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

[Kyle Manna]

On 10/25/2011 11:01 AM, Mark Brown wrote:
>> Unfortunately on this chip there aren't any identification or
>> >  constant test registers.  The only register close is the
>> >  JTAGVERNUM_REG and that's will change across silicon revisions.
> It seems like a more useful implementation would be to have the driver
> read and display this revision number during boot, this will give you
> the register readability check and also let us know the device revision
> which might be useful for diagnostics.
I reviewed the register and the lowest 4 bits are the silicon revision 
(my hw reads 0x1) and the upper four bits should always be zero.  I will 
update the patch to silently succeed as long a valid value is read, else 
print a error including that value and return -ENODEV.

Should I submit an entire updated patch series or just submit a single 
patch in reply to this?  Did you have any other comments on the rest of 
the series?

- Kyle

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

[Mark Brown]

On Tue, Oct 25, 2011 at 11:20:06AM -0500, Kyle Manna wrote:

> Should I submit an entire updated patch series or just submit a single  
> patch in reply to this?  Did you have any other comments on the rest of  
> the series?

I commented on some of the other patches at the same time I followed up
to this one...

[PATCH v2 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 v2 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 v2 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 v2 5/6] mfd: TPS65910: Move regulator defs to header

[Kyle Manna]

Move the regulator defintions to the header so that platform board file
can use them to configure specific regulators.

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

diff --git a/drivers/regulator/tps65910-regulator.c b/drivers/regulator/tps65910-regulator.c
index 66d2d60..44b4f22 100644
--- a/drivers/regulator/tps65910-regulator.c
+++ b/drivers/regulator/tps65910-regulator.c
@@ -25,30 +25,6 @@
 #include <linux/gpio.h>
 #include <linux/mfd/tps65910.h>
 
-#define TPS65910_REG_VRTC		0
-#define TPS65910_REG_VIO		1
-#define TPS65910_REG_VDD1		2
-#define TPS65910_REG_VDD2		3
-#define TPS65910_REG_VDD3		4
-#define TPS65910_REG_VDIG1		5
-#define TPS65910_REG_VDIG2		6
-#define TPS65910_REG_VPLL		7
-#define TPS65910_REG_VDAC		8
-#define TPS65910_REG_VAUX1		9
-#define TPS65910_REG_VAUX2		10
-#define TPS65910_REG_VAUX33		11
-#define TPS65910_REG_VMMC		12
-
-#define TPS65911_REG_VDDCTRL		4
-#define TPS65911_REG_LDO1		5
-#define TPS65911_REG_LDO2		6
-#define TPS65911_REG_LDO3		7
-#define TPS65911_REG_LDO4		8
-#define TPS65911_REG_LDO5		9
-#define TPS65911_REG_LDO6		10
-#define TPS65911_REG_LDO7		11
-#define TPS65911_REG_LDO8		12
-
 #define TPS65910_SUPPLY_STATE_ENABLED	0x1
 
 /* supported VIO voltages in milivolts */
diff --git a/include/linux/mfd/tps65910.h b/include/linux/mfd/tps65910.h
index 503ded3..84108bb 100644
--- a/include/linux/mfd/tps65910.h
+++ b/include/linux/mfd/tps65910.h
@@ -746,6 +746,31 @@
 #define TPS65910_GPIO_STS				BIT(1)
 #define TPS65910_GPIO_SET				BIT(0)
 
+/* Regulator Index Definitions */
+#define TPS65910_REG_VRTC				0
+#define TPS65910_REG_VIO				1
+#define TPS65910_REG_VDD1				2
+#define TPS65910_REG_VDD2				3
+#define TPS65910_REG_VDD3				4
+#define TPS65910_REG_VDIG1				5
+#define TPS65910_REG_VDIG2				6
+#define TPS65910_REG_VPLL				7
+#define TPS65910_REG_VDAC				8
+#define TPS65910_REG_VAUX1				9
+#define TPS65910_REG_VAUX2				10
+#define TPS65910_REG_VAUX33				11
+#define TPS65910_REG_VMMC				12
+
+#define TPS65911_REG_VDDCTRL				4
+#define TPS65911_REG_LDO1				5
+#define TPS65911_REG_LDO2				6
+#define TPS65911_REG_LDO3				7
+#define TPS65911_REG_LDO4				8
+#define TPS65911_REG_LDO5				9
+#define TPS65911_REG_LDO6				10
+#define TPS65911_REG_LDO7				11
+#define TPS65911_REG_LDO8				12
+
 /**
  * struct tps65910_board
  * Board platform data may be used to initialize regulators.
-- 
1.7.5.4

Re: [PATCH v2 5/6] mfd: TPS65910: Move regulator defs to header

[Mark Brown]

On Mon, Oct 24, 2011 at 12:06:03PM -0500, Kyle Manna wrote:
> Move the regulator defintions to the header so that platform board file
> can use them to configure specific regulators.
> 
> Signed-off-by: Kyle Manna <kyle.manna@fuel7.com>
> ---
>  drivers/regulator/tps65910-regulator.c |   24 ------------------------
>  include/linux/mfd/tps65910.h           |   25 +++++++++++++++++++++++++

Your subject line claims that this is a patch for the mfd but in fact
this is a patch for the regulator driver.  Please use appropriate
subject lines for your patches.

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

[PATCH v2 6/6] mfd: TPS65910: Create an array for reg init data

[Kyle Manna]

Create an array of fixed size for the platform to pass regulator
initalization data through.

Passing an array of pointers to init data also allows more flexible
definition of init data as well as prevents reading past the end of the
array should the platform define an incorrectly sized array.

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

diff --git a/drivers/regulator/tps65910-regulator.c b/drivers/regulator/tps65910-regulator.c
index 44b4f22..a620e25 100644
--- a/drivers/regulator/tps65910-regulator.c
+++ b/drivers/regulator/tps65910-regulator.c
@@ -861,8 +861,6 @@ static __devinit int tps65910_probe(struct platform_device *pdev)
 	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;
@@ -913,7 +911,16 @@ static __devinit int tps65910_probe(struct platform_device *pdev)
 		goto err_free_info;
 	}
 
-	for (i = 0; i < pmic->num_regulators; i++, info++, reg_data++) {
+	for (i = 0; i < pmic->num_regulators && i < TPS65910_NUM_REGS;
+			i++, info++) {
+
+		reg_data = pmic_plat_data->tps65910_pmic_init_data[i];
+
+		/* Regulator API handles empty constraints but not NULL
+		 * constraints */
+		if (!reg_data)
+			continue;
+
 		/* Register the regulators */
 		pmic->info[i] = info;
 
diff --git a/include/linux/mfd/tps65910.h b/include/linux/mfd/tps65910.h
index 84108bb..207b8b2 100644
--- a/include/linux/mfd/tps65910.h
+++ b/include/linux/mfd/tps65910.h
@@ -771,6 +771,9 @@
 #define TPS65911_REG_LDO7				11
 #define TPS65911_REG_LDO8				12
 
+/* Max number of TPS65910/11 regulators */
+#define TPS65910_NUM_REGS				13
+
 /**
  * struct tps65910_board
  * Board platform data may be used to initialize regulators.
@@ -782,7 +785,7 @@ struct tps65910_board {
 	int irq_base;
 	int vmbch_threshold;
 	int vmbch2_threshold;
-	struct regulator_init_data *tps65910_pmic_init_data;
+	struct regulator_init_data *tps65910_pmic_init_data[TPS65910_NUM_REGS];
 };
 
 /**
-- 
1.7.5.4

Re: [PATCH v2 6/6] mfd: TPS65910: Create an array for reg init data

[Mark Brown]

On Mon, Oct 24, 2011 at 12:06:04PM -0500, Kyle Manna wrote:
> Create an array of fixed size for the platform to pass regulator
> initalization data through.
> 
> Passing an array of pointers to init data also allows more flexible
> definition of init data as well as prevents reading past the end of the
> array should the platform define an incorrectly sized array.

Again, please make sure you use appropriate subject lines for your
patches.

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