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

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

Changes from v2:
* Enhance TPS65910 detection logic by reading a register
* Correct commit subjects to reflect correct subsystems
* Added Mark Brown's ACKs to the regulator patches


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

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

-- 
1.7.5.4

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

[Kyle Manna]

The JTAGREVNUM register contains a silicon revision number in the lower
four bits and the upper four bits are to always read 0.

To detect the presence of the device, attempt to read JTAGREVNUM
register and check that it returns a valid value.  If the I2C device
fails to respond or returns an invalid value, return -ENODEV.

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

diff --git a/drivers/mfd/tps65910.c b/drivers/mfd/tps65910.c
index 6f5b8cf..0d9c3eb 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 reg = 0;
 
 	pmic_plat_data = dev_get_platdata(&i2c->dev);
 	if (!pmic_plat_data)
@@ -161,11 +162,19 @@ 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 there */
+	ret = tps65910_i2c_read(tps65910, TPS65910_JTAGVERNUM, 1, &reg);
+	if (ret < 0 || (reg & ~JTAGVERNUM_VERNUM_MASK)) {
+		dev_err(tps65910->dev, "unknown version: JTAGREVNUM = 0x%x\n",
+				       reg);
+		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 +183,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 v3 1/6] mfd: TPS65910: Handle non-existent devices

[Mohammed, Afzal]

Hi Kyla,

On Thu, Nov 03, 2011 at 22:38:01, Kyle Manna wrote:
> The JTAGREVNUM register contains a silicon revision number in the lower
> four bits and the upper four bits are to always read 0.
> 
> To detect the presence of the device, attempt to read JTAGREVNUM
> register and check that it returns a valid value.  If the I2C device
> fails to respond or returns an invalid value, return -ENODEV.
> 
:
:
> +	/* Check that the device is there */
> +	ret = tps65910_i2c_read(tps65910, TPS65910_JTAGVERNUM, 1, &reg);
> +	if (ret < 0 || (reg & ~JTAGVERNUM_VERNUM_MASK)) {
> +		dev_err(tps65910->dev, "unknown version: JTAGREVNUM = 0x%x\n",
> +				       reg);
> +		ret = -ENODEV;
>  		goto err;
> +	}

If i2c read fails, "0" would get printed as version.

Perhaps it would be better to print version irrespective of whether it
is unknown or not, and return error for unknown value, something like,

        unsigned char buff;

        /* Check that the device is actually there */
        ret = tps65910_i2c_read(tps65910, 0x0, 1, &buff);
        if (ret < 0) {
                dev_err(tps65910->dev, "could not be detected\n");
                ret = -ENODEV;
                goto err;
        }

        dev_info(tps65910->dev, "JTAGREVNUM 0x%x\n", buff);

        if (buff & ~JTAGVERNUM_VERNUM_MASK) {
                dev_err(tps65910->dev, "unknown version\n");
                ret = -ENODEV;
                goto err;
        }



Regards
Afzal

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

[Mohammed, Afzal]

Hi Kyle,

On Thu, Nov 03, 2011 at 22:38:01, Kyle Manna wrote:
> +	ret = mfd_add_devices(tps65910->dev, -1, tps65910s,
> +			ARRAY_SIZE(tps65910s), NULL, 0);
> +	if (ret < 0)
> +		goto err2;

Isn't goto err sufficient

Regards
Afzal

[PATCH v3 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 v3 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 0d9c3eb..87cb3c9 100644
--- a/drivers/mfd/tps65910.c
+++ b/drivers/mfd/tps65910.c
@@ -177,7 +177,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 v3 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 87cb3c9..f48a4ae 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

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

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 389 bytes --]

Hi,

On Thu, Nov 03, 2011 at 12:08:04PM -0500, Kyle Manna wrote:
> The tps65910.h file depends on linux/gpio.h.  Move the include from the
> source file to the tps65910.h header file.

you should never rely on indirect inclusion of header files. If both
header and C source need the header, include on both. The header should
be safe for multiple inclusions anyway.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH v3 5/6] regulator: 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>
Acked-by: Mark Brown <broonie@opensource.wolfsonmicro.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

[PATCH v3 6/6] regulator: TPS65910: Create an array for 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>
Acked-by: Mark Brown <broonie@opensource.wolfsonmicro.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