[PATCH 0/4] Update TPS65910 to boot using devicetree

[PATCH 0/4] Update TPS65910 to boot using devicetree

[Rhyland Klein]

This patch set updates the tps65910 driver to boot using devicetree.

One thing that is unfavorable is the addition of the use_dt_for_init_data
flag. I added this as I wanted to split the parsing of the of nodes between
the tps65910-regulator code and the main tps65910 code. This split was to
maintain the list of regulator names solely inside the tps65910-regulator
code as it already was.

Rhyland Klein (4):
  regulator: tps65910: update type for regmap
  regulator: tps65910: Add device tree bindings
  mfd: tps65910: Add device-tree support
  ARM: Tegra: Add support for TPS65910 PMIC

 Documentation/devicetree/bindings/mfd/tps65910.txt |  132 ++++++++++++++++++++
 arch/arm/boot/dts/tegra-cardhu.dts                 |   92 ++++++++++++++
 drivers/gpio/gpio-tps65910.c                       |   39 ++++++-
 drivers/mfd/tps65910-irq.c                         |   21 +++-
 drivers/mfd/tps65910.c                             |   78 ++++++++++++-
 drivers/regulator/tps65910-regulator.c             |   86 +++++++++++++-
 include/linux/mfd/tps65910.h                       |    3 +-
 7 files changed, 439 insertions(+), 12 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mfd/tps65910.txt

[PATCH 1/4] regulator: tps65910: update type for regmap

[Rhyland Klein]

When accessing the regmap via the read/write functions, we need to use a
unsigned int * instead of a u8 * otherwise corruption will occur.

Signed-off-by: Rhyland Klein <rklein@nvidia.com>
---
 drivers/regulator/tps65910-regulator.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/regulator/tps65910-regulator.c b/drivers/regulator/tps65910-regulator.c
index 747bf57..9fd0fe1 100644
--- a/drivers/regulator/tps65910-regulator.c
+++ b/drivers/regulator/tps65910-regulator.c
@@ -331,7 +331,7 @@ struct tps65910_reg {
 
 static inline int tps65910_read(struct tps65910_reg *pmic, u8 reg)
 {
-	u8 val;
+	unsigned int val;
 	int err;
 
 	err = pmic->mfd->read(pmic->mfd, reg, 1, &val);
-- 
1.7.0.4

[PATCH 2/4] regulator: tps65910: Add device tree bindings

[Rhyland Klein]

Add device tree bindings for TI's tps65910 pmic.

Signed-off-by: Rhyland Klein <rklein@nvidia.com>
---
 Documentation/devicetree/bindings/mfd/tps65910.txt |  132 ++++++++++++++++++++
 1 files changed, 132 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mfd/tps65910.txt

diff --git a/Documentation/devicetree/bindings/mfd/tps65910.txt b/Documentation/devicetree/bindings/mfd/tps65910.txt
new file mode 100644
index 0000000..4a9f1aa
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/tps65910.txt
@@ -0,0 +1,132 @@
+TPS65910 Power Management Integrated Circuit
+
+Required properties:
+- compatible: "ti,tps65910" or "ti,tps65911"
+- reg: I2C slave address
+- interrupts: the interrupt outputs of the controller
+- #gpio-cells: number of cells to describe a GPIO, this should be 2.
+  The first cell is the GPIO number.
+  The second cell is used to specify additional options <unused>.
+- gpio-controller: mark the device as a GPIO controller
+- #interrupt-cells: the number of cells to describe an IRQ, this should be 2.
+  The first cell is the IRQ number.
+  The second cell is the flags, encoded as the trigger masks from
+  Documentation/devicetree/bindings/interrupts.txt
+- regulators: This is the list of child nodes that specify the regulator
+  initialization data for defined regulators. Not all regulators for the given
+  device need to be present. The definition for each of these nodes is defined
+  using the standard binding for regulators found at
+  Documentation/devicetree/bindings/regulator/regulator.txt.
+
+  The valid names for regulators are:
+  tps65910: vrtc, vio, vdd1, vdd2, vdd3, vdig1, vdig2, vpll, vdac, vaux1,
+            vaux2, vaux33, vmmc
+  tps65911: vrtc, vio, vdd1, vdd3, vddctrl, ldo1, ldo2, ldo3, ldo4, ldo5,
+            ldo6, ldo7, ldo8
+
+Optional properties:
+- ti,vmbch-threshold: (tps65911) main battery charged threshold
+  comparator. (see VMBCH_VSEL in TPS65910 datasheet)
+- ti,vmbch2-threshold: (tps65911) main battery discharged threshold
+  comparator. (see VMBCH_VSEL in TPS65910 datasheet)
+- ti,regulator-ext-sleep-control: enable external sleep
+  control through external inputs [0 (not enabled), 1 (EN1), 2 (EN2) or 4(EN3)]
+  There should be 13 entries here, one for each regulators for the chip.
+- ti,en-gpio-sleep: enable sleep control for gpios
+  There should be 9 entries here, one for each gpio.
+
+Example:
+
+	pmu: tps65910@d2 {
+		compatible = "ti,tps65910";
+		reg = <0xd2>;
+		interrupt-parent = <&intc>;
+		interrupts = < 0 118 0x04 >;
+
+		#gpio-cells = <2>;
+		gpio-controller;
+
+		#interrupt-cells = <2>;
+		interrupt-controller;
+
+		ti,vmbch-threshold = 0;
+		ti,vmbch2-threshold = 0;
+		ti,regulator-ext-sleep-control = <0
+						0
+						0x4   /* input EN3 */
+						0
+						0x1   /* input EN1 */
+						0
+						0
+						0
+						0
+						0
+						0
+						0x1   /* input EN1 */
+						0x1>; /* input EN1 */
+
+		ti,en-gpio-sleep = <0 0 1 0 0 0 0 0 0>;
+
+		regulators {
+			vdd1_reg: vdd1 {
+				regulator-min-microvolt = < 600000>;
+				regulator-max-microvolt = <1500000>;
+				regulator-always-on;
+				regulator-boot-on;
+			};
+			vdd2_reg: vdd2 {
+				regulator-min-microvolt = < 600000>;
+				regulator-max-microvolt = <1500000>;
+				regulator-always-on;
+				regulator-boot-on;
+			};
+			vddctrl_reg: vddctrl {
+				regulator-min-microvolt = < 600000>;
+				regulator-max-microvolt = <1400000>;
+				regulator-always-on;
+				regulator-boot-on;
+			};
+			vio_reg: vio {
+				regulator-min-microvolt = <1500000>;
+				regulator-max-microvolt = <1800000>;
+				regulator-always-on;
+				regulator-boot-on;
+			};
+			ldo1_reg: ldo1 {
+				regulator-min-microvolt = <1000000>;
+				regulator-max-microvolt = <3300000>;
+			};
+			ldo2_reg: ldo2 {
+				regulator-min-microvolt = <1050000>;
+				regulator-max-microvolt = <1050000>;
+			};
+			ldo3_reg: ldo3 {
+				regulator-min-microvolt = <1000000>;
+				regulator-max-microvolt = <3300000>;
+			};
+			ldo4_reg: ldo4 {
+				regulator-min-microvolt = <1000000>;
+				regulator-max-microvolt = <3300000>;
+				regulator-always-on;
+			};
+			ldo5_reg: ldo5 {
+				regulator-min-microvolt = <1000000>;
+				regulator-max-microvolt = <3300000>;
+			};
+			ldo6_reg: ldo6 {
+				regulator-min-microvolt = <1200000>;
+				regulator-max-microvolt = <1200000>;
+			};
+			ldo7_reg: ldo7 {
+				regulator-min-microvolt = <1200000>;
+				regulator-max-microvolt = <1200000>;
+				regulator-always-on;
+				regulator-boot-on;
+			};
+			ldo8_reg: ldo8 {
+				regulator-min-microvolt = <1000000>;
+				regulator-max-microvolt = <3300000>;
+				regulator-always-on;
+			};
+		};
+	};
-- 
1.7.0.4

[PATCH 3/4] mfd: tps65910: Add device-tree support

[Rhyland Klein]

Add device tree based initialization support for TI's tps65910 pmic.

Signed-off-by: Rhyland Klein <rklein@nvidia.com>
---
 drivers/gpio/gpio-tps65910.c           |   39 ++++++++++++++-
 drivers/mfd/tps65910-irq.c             |   21 ++++++--
 drivers/mfd/tps65910.c                 |   78 +++++++++++++++++++++++++++++-
 drivers/regulator/tps65910-regulator.c |   84 +++++++++++++++++++++++++++++++-
 include/linux/mfd/tps65910.h           |    3 +-
 5 files changed, 214 insertions(+), 11 deletions(-)

diff --git a/drivers/gpio/gpio-tps65910.c b/drivers/gpio/gpio-tps65910.c
index 7eef648..079aff1 100644
--- a/drivers/gpio/gpio-tps65910.c
+++ b/drivers/gpio/gpio-tps65910.c
@@ -66,9 +66,38 @@ static int tps65910_gpio_input(struct gpio_chip *gc, unsigned offset)
 						GPIO_CFG_MASK);
 }
 
+#ifdef CONFIG_OF
+static int tps65910_gpio_parse_dt(struct device *dev,
+					struct tps65910_board *board)
+{
+	struct device_node *np = dev->of_node;
+	unsigned int prop_array[TPS6591X_MAX_NUM_GPIO];
+	int idx = 0, ret;
+
+	ret = of_property_read_u32_array(np, "ti,en-gpio-sleep",
+				   prop_array, TPS6591X_MAX_NUM_GPIO);
+	if (!ret)
+		for (idx = 0; idx < ARRAY_SIZE(prop_array); idx++)
+			board->en_gpio_sleep[idx] = (prop_array[idx] != 0);
+	else if (ret != -EINVAL) {
+		dev_err(dev,
+			"error reading property ti,en-gpio-sleep: %d\n.", ret);
+		return ret;
+	}
+
+	return 0;
+}
+#else
+static int tps65910_gpio_parse_dt(struct device *dev,
+					struct tps65910_board *board)
+{
+	return 0;
+}
+#endif
+
 void tps65910_gpio_init(struct tps65910 *tps65910, int gpio_base)
 {
-	int ret;
+	int ret = 0;
 	struct tps65910_board *board_data;
 
 	if (!gpio_base)
@@ -97,7 +126,13 @@ void tps65910_gpio_init(struct tps65910 *tps65910, int gpio_base)
 	tps65910->gpio.get		= tps65910_gpio_get;
 
 	/* Configure sleep control for gpios */
-	board_data = dev_get_platdata(tps65910->dev);
+	board_data = tps65910->board_data;
+	if (board_data->use_dt_for_init_data && tps65910->dev->of_node)
+		ret = tps65910_gpio_parse_dt(tps65910->dev, board_data);
+
+	if (ret)
+		return;
+
 	if (board_data) {
 		int i;
 		for (i = 0; i < tps65910->gpio.ngpio; ++i) {
diff --git a/drivers/mfd/tps65910-irq.c b/drivers/mfd/tps65910-irq.c
index c9ed5c0..5bec078 100644
--- a/drivers/mfd/tps65910-irq.c
+++ b/drivers/mfd/tps65910-irq.c
@@ -180,12 +180,6 @@ int tps65910_irq_init(struct tps65910 *tps65910, int irq,
 		return -EINVAL;
 	}
 
-	tps65910->irq_mask = 0xFFFFFF;
-
-	mutex_init(&tps65910->irq_lock);
-	tps65910->chip_irq = irq;
-	tps65910->irq_base = pdata->irq_base;
-
 	switch (tps65910_chip_id(tps65910)) {
 	case TPS65910:
 		tps65910->irq_num = TPS65910_NUM_IRQ;
@@ -195,6 +189,21 @@ int tps65910_irq_init(struct tps65910 *tps65910, int irq,
 		break;
 	}
 
+	if (pdata->irq_base <= 0)
+		pdata->irq_base = irq_alloc_descs(-1, 0, tps65910->irq_num, -1);
+
+	if (pdata->irq_base <= 0) {
+		dev_err(tps65910->dev, "Failed to allocate irq descs: %d\n",
+			pdata->irq_base);
+		return pdata->irq_base;
+	}
+
+	tps65910->irq_mask = 0xFFFFFF;
+
+	mutex_init(&tps65910->irq_lock);
+	tps65910->chip_irq = irq;
+	tps65910->irq_base = pdata->irq_base;
+
 	/* Register with genirq */
 	for (cur_irq = tps65910->irq_base;
 	     cur_irq < tps65910->irq_num + tps65910->irq_base;
diff --git a/drivers/mfd/tps65910.c b/drivers/mfd/tps65910.c
index bf2b25e..a076715 100644
--- a/drivers/mfd/tps65910.c
+++ b/drivers/mfd/tps65910.c
@@ -23,6 +23,7 @@
 #include <linux/mfd/core.h>
 #include <linux/regmap.h>
 #include <linux/mfd/tps65910.h>
+#include <linux/of_device.h>
 
 static struct mfd_cell tps65910s[] = {
 	{
@@ -90,6 +91,67 @@ static const struct regmap_config tps65910_regmap_config = {
 	.cache_type = REGCACHE_RBTREE,
 };
 
+#ifdef CONFIG_OF
+static struct of_device_id tps65910_of_match[] = {
+	{ .compatible = "ti,tps65910", .data = (void *)TPS65910},
+	{ .compatible = "ti,tps65911", .data = (void *)TPS65911},
+	{ },
+};
+MODULE_DEVICE_TABLE(of, tps65910_of_match);
+
+static struct tps65910_board *tps65910_parse_dt(struct i2c_client *client,
+						int *chip_id)
+{
+	struct device_node *np = client->dev.of_node;
+	struct tps65910_board *pmic_plat_data;
+	unsigned int prop;
+	const struct of_device_id *match;
+	int ret = 0;
+
+	match = of_match_device(tps65910_of_match, &client->dev);
+	if (!match) {
+		dev_err(&client->dev, "Failed to find matching dt id\n");
+		return NULL;
+	}
+
+	*chip_id  = (int)match->data;
+
+	pmic_plat_data = devm_kzalloc(&client->dev, sizeof(*pmic_plat_data),
+			GFP_KERNEL);
+	if (!pmic_plat_data) {
+		dev_err(&client->dev, "Failed to allocate pdata\n");
+		return NULL;
+	}
+
+	ret = of_property_read_u32(np, "ti,vmbch-threshold", &prop);
+	if (!ret)
+		pmic_plat_data->vmbch_threshold = prop;
+	else if (*chip_id == TPS65911)
+		dev_warn(&client->dev, "VMBCH-Threshold not specified");
+
+	ret = of_property_read_u32(np, "ti,vmbch2-threshold", &prop);
+	if (!ret)
+		pmic_plat_data->vmbch2_threshold = prop;
+	else if (*chip_id == TPS65911)
+		dev_warn(&client->dev, "VMBCH2-Threshold not specified");
+
+	pmic_plat_data->use_dt_for_init_data = true;
+
+	pmic_plat_data->irq = client->irq;
+	pmic_plat_data->irq_base = -1;
+
+	pmic_plat_data->gpio_base = -1;
+
+	return pmic_plat_data;
+}
+#else
+static inline struct tps65910_board *tps65910_parse_dt(
+					struct i2c_client *client)
+{
+	return NULL;
+}
+#endif
+
 static int tps65910_i2c_probe(struct i2c_client *i2c,
 			    const struct i2c_device_id *id)
 {
@@ -97,11 +159,23 @@ static int tps65910_i2c_probe(struct i2c_client *i2c,
 	struct tps65910_board *pmic_plat_data;
 	struct tps65910_platform_data *init_data;
 	int ret = 0;
+	int chip_id = id->driver_data;
+	int idx;
 
 	pmic_plat_data = dev_get_platdata(&i2c->dev);
+
+	if (!pmic_plat_data && i2c->dev.of_node)
+		pmic_plat_data = tps65910_parse_dt(i2c, &chip_id);
+
 	if (!pmic_plat_data)
 		return -EINVAL;
 
+	/* Pass of data to child devices */
+	for (idx = 0; idx < ARRAY_SIZE(tps65910s); idx++) {
+		tps65910s[idx].platform_data = pmic_plat_data;
+		tps65910s[idx].pdata_size = sizeof(*pmic_plat_data);
+	}
+
 	init_data = kzalloc(sizeof(struct tps65910_platform_data), GFP_KERNEL);
 	if (init_data == NULL)
 		return -ENOMEM;
@@ -115,7 +189,8 @@ static int tps65910_i2c_probe(struct i2c_client *i2c,
 	i2c_set_clientdata(i2c, tps65910);
 	tps65910->dev = &i2c->dev;
 	tps65910->i2c_client = i2c;
-	tps65910->id = id->driver_data;
+	tps65910->board_data = pmic_plat_data;
+	tps65910->id = chip_id;
 	tps65910->read = tps65910_i2c_read;
 	tps65910->write = tps65910_i2c_write;
 	mutex_init(&tps65910->io_mutex);
@@ -175,6 +250,7 @@ static struct i2c_driver tps65910_i2c_driver = {
 	.driver = {
 		   .name = "tps65910",
 		   .owner = THIS_MODULE,
+		   .of_match_table = of_match_ptr(tps65910_of_match),
 	},
 	.probe = tps65910_i2c_probe,
 	.remove = tps65910_i2c_remove,
diff --git a/drivers/regulator/tps65910-regulator.c b/drivers/regulator/tps65910-regulator.c
index 9fd0fe1..121f202 100644
--- a/drivers/regulator/tps65910-regulator.c
+++ b/drivers/regulator/tps65910-regulator.c
@@ -24,6 +24,7 @@
 #include <linux/slab.h>
 #include <linux/gpio.h>
 #include <linux/mfd/tps65910.h>
+#include <linux/regulator/of_regulator.h>
 
 #define TPS65910_SUPPLY_STATE_ENABLED	0x1
 #define EXT_SLEEP_CONTROL (TPS65910_SLEEP_CONTROL_EXT_INPUT_EN1 |	\
@@ -1094,6 +1095,79 @@ static int tps65910_set_ext_sleep_config(struct tps65910_reg *pmic,
 	return ret;
 }
 
+#ifdef CONFIG_OF
+static int tps65910_parse_dt_reg_data(struct platform_device *pdev,
+		struct tps65910_board *pmic_plat_data, struct tps_info *info)
+{
+	struct device_node *np = pdev->dev.parent->of_node;
+	struct tps65910_reg *pmic = dev_get_drvdata(&pdev->dev);
+	struct device_node *regulators;
+	struct device_node *child;
+	unsigned int prop_array[TPS65910_NUM_REGS];
+	struct regulator_init_data **all_data;
+	int idx = 0, ret;
+
+	all_data = pmic_plat_data->tps65910_pmic_init_data;
+
+	regulators = of_find_node_by_name(np, "regulators");
+
+	for_each_child_of_node(regulators, child) {
+		struct regulator_init_data *init_data;
+
+		init_data = of_get_regulator_init_data(&pdev->dev, child);
+		if (!init_data) {
+			dev_err(&pdev->dev,
+				"failed to parse DT for regulator %s\n",
+				child->name);
+			return -EINVAL;
+		}
+
+		for (idx = 0; idx < pmic->num_regulators; idx++) {
+			if (!strcasecmp(info[idx].name, child->name)) {
+				if (all_data[idx]) {
+					dev_err(&pdev->dev,
+						"Duplicate Regulator Node %s\n",
+						child->name);
+					return -EINVAL;
+				}
+				all_data[idx] = init_data;
+				break;
+			}
+		}
+
+		/* Check to see if we iterated without finding its name */
+		if (idx == pmic->num_regulators) {
+			dev_err(&pdev->dev,
+				"Unknown regulator node found [%s]\n",
+				child->name);
+			return -EINVAL;
+		}
+	}
+
+	ret = of_property_read_u32_array(np, "ti,regulator-ext-sleep-control",
+					 prop_array, TPS65910_NUM_REGS);
+
+	if (!ret)
+		for (idx = 0; idx < ARRAY_SIZE(prop_array); idx++)
+			pmic_plat_data->regulator_ext_sleep_control[idx] =
+				prop_array[idx];
+	else if (ret != -EINVAL) {
+		dev_err(&pdev->dev,
+			"error reading ti,regulator-ext-sleep-control: %d\n",
+			ret);
+		return ret;
+	}
+
+	return 0;
+}
+#else
+static inline int tps65910_parse_dt_reg_data(struct platform_device *pdev,
+		struct tps65910_board *pmic_plat_data, struct tps_info *info)
+{
+	return 0;
+}
+#endif
+
 static __devinit int tps65910_probe(struct platform_device *pdev)
 {
 	struct tps65910 *tps65910 = dev_get_drvdata(pdev->dev.parent);
@@ -1105,7 +1179,7 @@ static __devinit int tps65910_probe(struct platform_device *pdev)
 	struct tps65910_board *pmic_plat_data;
 	int i, err;
 
-	pmic_plat_data = dev_get_platdata(tps65910->dev);
+	pmic_plat_data = dev_get_platdata(&pdev->dev);
 	if (!pmic_plat_data)
 		return -EINVAL;
 
@@ -1139,6 +1213,14 @@ static __devinit int tps65910_probe(struct platform_device *pdev)
 		return -ENODEV;
 	}
 
+	if (pmic_plat_data->use_dt_for_init_data) {
+		err = tps65910_parse_dt_reg_data(pdev, pmic_plat_data, info);
+		if (err) {
+			dev_err(&pdev->dev, "Error Parsing DT Information");
+			goto err_out;
+		}
+	}
+
 	pmic->desc = kcalloc(pmic->num_regulators,
 			sizeof(struct regulator_desc), GFP_KERNEL);
 	if (!pmic->desc) {
diff --git a/include/linux/mfd/tps65910.h b/include/linux/mfd/tps65910.h
index 1c6c286..32b320a 100644
--- a/include/linux/mfd/tps65910.h
+++ b/include/linux/mfd/tps65910.h
@@ -787,13 +787,13 @@
  * struct tps65910_board
  * Board platform data may be used to initialize regulators.
  */
-
 struct tps65910_board {
 	int gpio_base;
 	int irq;
 	int irq_base;
 	int vmbch_threshold;
 	int vmbch2_threshold;
+	bool use_dt_for_init_data; /* signal to parse dt for reg init data*/
 	bool en_gpio_sleep[TPS6591X_MAX_NUM_GPIO];
 	unsigned long regulator_ext_sleep_control[TPS65910_NUM_REGS];
 	struct regulator_init_data *tps65910_pmic_init_data[TPS65910_NUM_REGS];
@@ -807,6 +807,7 @@ struct tps65910 {
 	struct device *dev;
 	struct i2c_client *i2c_client;
 	struct regmap *regmap;
+	struct tps65910_board *board_data;
 	struct mutex io_mutex;
 	unsigned int id;
 	int (*read)(struct tps65910 *tps65910, u8 reg, int size, void *dest);
-- 
1.7.0.4

[PATCH 4/4] ARM: Tegra: Add support for TPS65910 PMIC

[Rhyland Klein]

Add support for the tps65910 pmic on cardhu.

Signed-off-by: Rhyland Klein <rklein@nvidia.com>
---
 arch/arm/boot/dts/tegra-cardhu.dts |   92 ++++++++++++++++++++++++++++++++++++
 1 files changed, 92 insertions(+), 0 deletions(-)

diff --git a/arch/arm/boot/dts/tegra-cardhu.dts b/arch/arm/boot/dts/tegra-cardhu.dts
index ab8d901..7db4fac 100644
--- a/arch/arm/boot/dts/tegra-cardhu.dts
+++ b/arch/arm/boot/dts/tegra-cardhu.dts
@@ -115,6 +115,98 @@
 			micdet-delay = <100>;
 			gpio-cfg = <0xffffffff 0xffffffff 0 0xffffffff 0xffffffff>;
 		};
+
+		pmic: tps65911@2d {
+			compatible = "ti,tps65911";
+			reg = <0x2d>;
+			interrupts = < 0 118 0x04 >;
+
+			#gpio-cells = <2>;
+			gpio-controller;
+
+			#interrupt-cells = <2>;
+			interrupt-controller;
+
+			ti,vmbch-threshold = <0>;
+			ti,vmbch2-threshold = <0>;
+			ti,regulator-ext-sleep-control = < 0
+						0
+						0x4     /* input EN3 */
+						0
+						0x1     /* input EN1 */
+						0
+						0
+						0
+						0
+						0
+						0
+						0x1     /* input EN1 */
+						0x1>;   /* input EN1 */
+			ti,en-gpio-sleep = <0 0 1 0 0 0 0 0 0>;
+
+			regulators {
+				vdd1_reg: vdd1 {
+					regulator-min-microvolt = < 600000>;
+					regulator-max-microvolt = <1500000>;
+					regulator-always-on;
+					regulator-boot-on;
+				};
+				vdd2_reg: vdd2 {
+					regulator-min-microvolt = < 600000>;
+					regulator-max-microvolt = <1500000>;
+					regulator-always-on;
+					regulator-boot-on;
+				};
+				vddctrl_reg: vddctrl {
+					regulator-min-microvolt = < 600000>;
+					regulator-max-microvolt = <1400000>;
+					regulator-always-on;
+					regulator-boot-on;
+				};
+				vio_reg: vio {
+					regulator-min-microvolt = <1500000>;
+					regulator-max-microvolt = <1800000>;
+					regulator-always-on;
+					regulator-boot-on;
+				};
+				ldo1_reg: ldo1 {
+					regulator-min-microvolt = <1000000>;
+					regulator-max-microvolt = <3300000>;
+				};
+				ldo2_reg: ldo2 {
+					regulator-min-microvolt = <1050000>;
+					regulator-max-microvolt = <1050000>;
+				};
+				ldo3_reg: ldo3 {
+					regulator-min-microvolt = <1000000>;
+					regulator-max-microvolt = <3300000>;
+				};
+				ldo4_reg: ldo4 {
+					regulator-min-microvolt = <1000000>;
+					regulator-max-microvolt = <3300000>;
+					regulator-always-on;
+				};
+				ldo5_reg: ldo5 {
+					regulator-min-microvolt = <1000000>;
+					regulator-max-microvolt = <3300000>;
+				};
+				ldo6_reg: ldo6 {
+					regulator-min-microvolt = <1200000>;
+					regulator-max-microvolt = <1200000>;
+				};
+				ldo7_reg: ldo7 {
+					regulator-min-microvolt = <1200000>;
+					regulator-max-microvolt = <1200000>;
+					regulator-always-on;
+					regulator-boot-on;
+				};
+				ldo8_reg: ldo8 {
+					regulator-min-microvolt = <1000000>;
+					regulator-max-microvolt = <3300000>;
+					regulator-always-on;
+				};
+			};
+		};
 	};
 
 	sdhci@78000000 {
-- 
1.7.0.4

Re: [PATCH 2/4] regulator: tps65910: Add device tree bindings

[Mark Brown]

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

On Tue, Apr 17, 2012 at 06:00:27PM -0700, Rhyland Klein wrote:
> Add device tree bindings for TI's tps65910 pmic.
> 
> Signed-off-by: Rhyland Klein <rklein@nvidia.com>
> ---
>  Documentation/devicetree/bindings/mfd/tps65910.txt |  132 ++++++++++++++++++++
>  1 files changed, 132 insertions(+), 0 deletions(-)

There's no actual code changes here, this should be part of the patch
adding the code, especially since there's only one patch for the code
change.  If there had been a series of changes it'd possibly have made
sense to split the binding document but when it's just one patch it's
easier to review together.

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

Re: [PATCH 3/4] mfd: tps65910: Add device-tree support

[Mark Brown]

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

On Tue, Apr 17, 2012 at 06:00:28PM -0700, Rhyland Klein wrote:
> Add device tree based initialization support for TI's tps65910 pmic.

Actually, now I look at the larger patch this probably wants to be split
up by driver and possibly split further within that.

> +	board_data = tps65910->board_data;
> +	if (board_data->use_dt_for_init_data && tps65910->dev->of_node)
> +		ret = tps65910_gpio_parse_dt(tps65910->dev, board_data);
> +

This is a really odd idiom - normally the pattern for device tree
support is to just go and try to use the device tree data if it's there
and there's no need for any flag to say if it should be used.

> +	if (pdata->irq_base <= 0)
> +		pdata->irq_base = irq_alloc_descs(-1, 0, tps65910->irq_num, -1);
> +
> +	if (pdata->irq_base <= 0) {
> +		dev_err(tps65910->dev, "Failed to allocate irq descs: %d\n",
> +			pdata->irq_base);
> +		return pdata->irq_base;
> +	}
> +
> +	tps65910->irq_mask = 0xFFFFFF;
> +
> +	mutex_init(&tps65910->irq_lock);
> +	tps65910->chip_irq = irq;
> +	tps65910->irq_base = pdata->irq_base;

While this is needed for DT support it can be done separately and would
probably be better split out into a separate patch.

> +	/* Pass of data to child devices */
> +	for (idx = 0; idx < ARRAY_SIZE(tps65910s); idx++) {
> +		tps65910s[idx].platform_data = pmic_plat_data;
> +		tps65910s[idx].pdata_size = sizeof(*pmic_plat_data);
> +	}

Why is this needed - can't the DT parsing just be moved where it's used?

> +	for_each_child_of_node(regulators, child) {
> +		struct regulator_init_data *init_data;
> +
> +		init_data = of_get_regulator_init_data(&pdev->dev, child);
> +		if (!init_data) {
> +			dev_err(&pdev->dev,
> +				"failed to parse DT for regulator %s\n",
> +				child->name);
> +			return -EINVAL;
> +		}
> +
> +		for (idx = 0; idx < pmic->num_regulators; idx++) {

Hrm, this iteration over a group of regulators to find the relevant
node by name is going to be a fairly common pattern (there's already
at least one driver doing this IIRC) - we should really factor it out
into common code.  Please consider doing this when you resubmit.

> +			if (!strcasecmp(info[idx].name, child->name)) {
> +				if (all_data[idx]) {
> +					dev_err(&pdev->dev,
> +						"Duplicate Regulator Node %s\n",

Please fix the capitalisation in the error message.

> +		/* Check to see if we iterated without finding its name */
> +		if (idx == pmic->num_regulators) {
> +			dev_err(&pdev->dev,
> +				"Unknown regulator node found [%s]\n",
> +				child->name);
> +			return -EINVAL;
> +		}

It'd seem more robust to only print the warning and not return the
error, that way we don't completely fail the device initialisation if
there's data we don't understand.

I'm also not seeing a change here that passes the DT node to
regulator_register() - you should be doing that, it's needed so
consumers can bind to the regulator.

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

Re: [PATCH 1/4] regulator: tps65910: update type for regmap

[Mark Brown]

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

On Tue, Apr 17, 2012 at 06:00:26PM -0700, Rhyland Klein wrote:
> When accessing the regmap via the read/write functions, we need to use a
> unsigned int * instead of a u8 * otherwise corruption will occur.
> 
> Signed-off-by: Rhyland Klein <rklein@nvidia.com>

>  static inline int tps65910_read(struct tps65910_reg *pmic, u8 reg)
>  {
> -	u8 val;
> +	unsigned int val;
>  	int err;
>  
>  	err = pmic->mfd->read(pmic->mfd, reg, 1, &val);

Ugh, this interface is just broken all round - there's absolutely no
type safety here and all users of these functions will be broken (a
similar issue applies on write).  It's much better to fix this for 3.4
by converting the core code to use regmap_raw_ functions which take
native formatted data for the device like the function driver API
actually expects.

Looking at the MFD code the fix for 3.5 should at the very least involve
making the functions take typed pointers, though given the way they're
implemented right now with direct references in the subdevices I'd also
consider just having the subdevices uses regmap directly as the wrappers
are just adding an opportunity for error (the bit operations could be
converted into static inlines in the header too).

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

Re: [PATCH 3/4] mfd: tps65910: Add device-tree support

[Rhyland Klein]

On Wed, 2012-04-18 at 02:01 -0700, Mark Brown wrote:
> * PGP Signed by an unknown key
> 
> On Tue, Apr 17, 2012 at 06:00:28PM -0700, Rhyland Klein wrote:
> > Add device tree based initialization support for TI's tps65910 pmic.
> 
> Actually, now I look at the larger patch this probably wants to be split
> up by driver and possibly split further within that.
> 
> > +	board_data = tps65910->board_data;
> > +	if (board_data->use_dt_for_init_data && tps65910->dev->of_node)
> > +		ret = tps65910_gpio_parse_dt(tps65910->dev, board_data);
> > +
> 
> This is a really odd idiom - normally the pattern for device tree
> support is to just go and try to use the device tree data if it's there
> and there's no need for any flag to say if it should be used.
> 

I agree its odd. My concern was that the idiom is that is pdata assigned
from board files should override dt data. At this point, we don't know
where the tps65910->board_data is coming from, dt or board files.
Arbitrarily using dt breaks that idiom. We could do a check like this if
you prefer:

if (!(dev_get_platdata(tps65910->dev) && tps65910->dev->of_node)

i.e. if doesn't have pdata supplied from board files, but does have dt
node. 


> > +	if (pdata->irq_base <= 0)
> > +		pdata->irq_base = irq_alloc_descs(-1, 0, tps65910->irq_num, -1);
> > +
> > +	if (pdata->irq_base <= 0) {
> > +		dev_err(tps65910->dev, "Failed to allocate irq descs: %d\n",
> > +			pdata->irq_base);
> > +		return pdata->irq_base;
> > +	}
> > +
> > +	tps65910->irq_mask = 0xFFFFFF;
> > +
> > +	mutex_init(&tps65910->irq_lock);
> > +	tps65910->chip_irq = irq;
> > +	tps65910->irq_base = pdata->irq_base;
> 
> While this is needed for DT support it can be done separately and would
> probably be better split out into a separate patch.
> 

ok.

> > +	/* Pass of data to child devices */
> > +	for (idx = 0; idx < ARRAY_SIZE(tps65910s); idx++) {
> > +		tps65910s[idx].platform_data = pmic_plat_data;
> > +		tps65910s[idx].pdata_size = sizeof(*pmic_plat_data);
> > +	}
> 
> Why is this needed - can't the DT parsing just be moved where it's used?

> 
> > +	for_each_child_of_node(regulators, child) {
> > +		struct regulator_init_data *init_data;
> > +
> > +		init_data = of_get_regulator_init_data(&pdev->dev, child);
> > +		if (!init_data) {
> > +			dev_err(&pdev->dev,
> > +				"failed to parse DT for regulator %s\n",
> > +				child->name);
> > +			return -EINVAL;
> > +		}
> > +
> > +		for (idx = 0; idx < pmic->num_regulators; idx++) {
> 
> Hrm, this iteration over a group of regulators to find the relevant
> node by name is going to be a fairly common pattern (there's already
> at least one driver doing this IIRC) - we should really factor it out
> into common code.  Please consider doing this when you resubmit.

Ok.

> 
> > +			if (!strcasecmp(info[idx].name, child->name)) {
> > +				if (all_data[idx]) {
> > +					dev_err(&pdev->dev,
> > +						"Duplicate Regulator Node %s\n",
> 
> Please fix the capitalisation in the error message.
> 
> > +		/* Check to see if we iterated without finding its name */
> > +		if (idx == pmic->num_regulators) {
> > +			dev_err(&pdev->dev,
> > +				"Unknown regulator node found [%s]\n",
> > +				child->name);
> > +			return -EINVAL;
> > +		}
> 
> It'd seem more robust to only print the warning and not return the
> error, that way we don't completely fail the device initialisation if
> there's data we don't understand.
> 
> I'm also not seeing a change here that passes the DT node to
> regulator_register() - you should be doing that, it's needed so
> consumers can bind to the regulator.

> * Unknown Key
> * 0x6E30FDDD

Thanks,

rhyland

Re: [PATCH 1/4] regulator: tps65910: update type for regmap

[Rhyland Klein]

On Wed, 2012-04-18 at 02:25 -0700, Mark Brown wrote:
> * PGP Signed by an unknown key
> 
> On Tue, Apr 17, 2012 at 06:00:26PM -0700, Rhyland Klein wrote:
> > When accessing the regmap via the read/write functions, we need to use a
> > unsigned int * instead of a u8 * otherwise corruption will occur.
> > 
> > Signed-off-by: Rhyland Klein <rklein@nvidia.com>
> 
> >  static inline int tps65910_read(struct tps65910_reg *pmic, u8 reg)
> >  {
> > -	u8 val;
> > +	unsigned int val;
> >  	int err;
> >  
> >  	err = pmic->mfd->read(pmic->mfd, reg, 1, &val);
> 
> Ugh, this interface is just broken all round - there's absolutely no
> type safety here and all users of these functions will be broken (a
> similar issue applies on write).  It's much better to fix this for 3.4
> by converting the core code to use regmap_raw_ functions which take
> native formatted data for the device like the function driver API
> actually expects.

Which interface are you saying is broken? The regmap interface or the
one internal to the tps65910 code? 

> 
> Looking at the MFD code the fix for 3.5 should at the very least involve
> making the functions take typed pointers, though given the way they're
> implemented right now with direct references in the subdevices I'd also
> consider just having the subdevices uses regmap directly as the wrappers
> are just adding an opportunity for error (the bit operations could be
> converted into static inlines in the header too).
> 

So to be clear... Your recommendation is to change the tps65910 code to
remove the common read/write callbacks and to use regmap directly in
each component, and then when using regmap, do use the regmap raw
functions instead of the bulkread/write?

> * Unknown Key
> * 0x6E30FDDD

Thanks,
rhyland

---
nvpublic

Re: [PATCH 1/4] regulator: tps65910: update type for regmap

[Rhyland Klein]

On Wed, 2012-04-18 at 13:03 -0700, Rhyland Klein wrote:
> On Wed, 2012-04-18 at 02:25 -0700, Mark Brown wrote:
> > * PGP Signed by an unknown key
> > 
> > On Tue, Apr 17, 2012 at 06:00:26PM -0700, Rhyland Klein wrote:
> > > When accessing the regmap via the read/write functions, we need to use a
> > > unsigned int * instead of a u8 * otherwise corruption will occur.
> > > 
> > > Signed-off-by: Rhyland Klein <rklein@nvidia.com>
> > 
> > >  static inline int tps65910_read(struct tps65910_reg *pmic, u8 reg)
> > >  {
> > > -	u8 val;
> > > +	unsigned int val;
> > >  	int err;
> > >  
> > >  	err = pmic->mfd->read(pmic->mfd, reg, 1, &val);
> > 
> > Ugh, this interface is just broken all round - there's absolutely no
> > type safety here and all users of these functions will be broken (a
> > similar issue applies on write).  It's much better to fix this for 3.4
> > by converting the core code to use regmap_raw_ functions which take
> > native formatted data for the device like the function driver API
> > actually expects.
> 
> Which interface are you saying is broken? The regmap interface or the
> one internal to the tps65910 code? 
> 
> > 
> > Looking at the MFD code the fix for 3.5 should at the very least involve
> > making the functions take typed pointers, though given the way they're
> > implemented right now with direct references in the subdevices I'd also
> > consider just having the subdevices uses regmap directly as the wrappers
> > are just adding an opportunity for error (the bit operations could be
> > converted into static inlines in the header too).
> > 
> 
> So to be clear... Your recommendation is to change the tps65910 code to
> remove the common read/write callbacks and to use regmap directly in
> each component, and then when using regmap, do use the regmap raw
> functions instead of the bulkread/write?

Looking at the code, I would think it makes more sense use regmap_read
which enforces types since the tps65910 code only ever seems to use
regmap to access a single register at a time. Do you agree?

-rhyland
--- 
nvpublic

Re: [PATCH 3/4] mfd: tps65910: Add device-tree support

[Mark Brown]

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

On Wed, Apr 18, 2012 at 12:35:58PM -0700, Rhyland Klein wrote:
> On Wed, 2012-04-18 at 02:01 -0700, Mark Brown wrote:

> > This is a really odd idiom - normally the pattern for device tree
> > support is to just go and try to use the device tree data if it's there
> > and there's no need for any flag to say if it should be used.

> I agree its odd. My concern was that the idiom is that is pdata assigned
> from board files should override dt data. At this point, we don't know
> where the tps65910->board_data is coming from, dt or board files.
> Arbitrarily using dt breaks that idiom. We could do a check like this if
> you prefer:

No, just prefer the DT - what you're proposing is exactly the opposite
idiom to what all the other DT drivers do so we'd need to go round
changing the policy globally at which point we probably want to start
having helpers for this.

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

Re: [PATCH 1/4] regulator: tps65910: update type for regmap

[Mark Brown]

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

On Wed, Apr 18, 2012 at 01:19:31PM -0700, Rhyland Klein wrote:
> On Wed, 2012-04-18 at 13:03 -0700, Rhyland Klein wrote:

> > Which interface are you saying is broken? The regmap interface or the
> > one internal to the tps65910 code? 

This driver is broken.

> > So to be clear... Your recommendation is to change the tps65910 code to
> > remove the common read/write callbacks and to use regmap directly in
> > each component, and then when using regmap, do use the regmap raw
> > functions instead of the bulkread/write?

> Looking at the code, I would think it makes more sense use regmap_read
> which enforces types since the tps65910 code only ever seems to use
> regmap to access a single register at a time. Do you agree?

Yes, for single register I/O the drivers should just be using the
per-register APIs regmap has if they're using regmap directly.  Having
wrappers for this in the header would be fine also, the things that are
really problematic about the current code are the void * usage and the
fact that individual drivers are writing register read/write functions.

If the driver wasn't using void * this bug would've been caught by the
compiler.

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

Re: [PATCH 3/4] mfd: tps65910: Add device-tree support

[Stephen Warren]

On 04/19/2012 06:50 AM, Mark Brown wrote:
> On Wed, Apr 18, 2012 at 12:35:58PM -0700, Rhyland Klein wrote:
>> On Wed, 2012-04-18 at 02:01 -0700, Mark Brown wrote:
> 
>>> This is a really odd idiom - normally the pattern for device tree
>>> support is to just go and try to use the device tree data if it's there
>>> and there's no need for any flag to say if it should be used.
> 
>> I agree its odd. My concern was that the idiom is that is pdata assigned
>> from board files should override dt data. At this point, we don't know
>> where the tps65910->board_data is coming from, dt or board files.
>> Arbitrarily using dt breaks that idiom. We could do a check like this if
>> you prefer:
> 
> No, just prefer the DT - what you're proposing is exactly the opposite
> idiom to what all the other DT drivers do so we'd need to go round
> changing the policy globally at which point we probably want to start
> having helpers for this.

That's not right - the idea is that pdata should override device tree so
that if there's a platform where the DT is known to contain incorrect
data, then some early platform code can add pdata to the device to fix
the problem, and that will be used in preference to the DT data.

At least, that's the last I heard Grant say on the subject, and that's
how I've been writing all the Tegra-related drivers, and I've seen
others do the same for other platforms.

Re: [PATCH 3/4] mfd: tps65910: Add device-tree support

[Mark Brown]

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

On Thu, Apr 19, 2012 at 09:35:49AM -0600, Stephen Warren wrote:

> That's not right - the idea is that pdata should override device tree so
> that if there's a platform where the DT is known to contain incorrect
> data, then some early platform code can add pdata to the device to fix
> the problem, and that will be used in preference to the DT data.

> At least, that's the last I heard Grant say on the subject, and that's
> how I've been writing all the Tegra-related drivers, and I've seen
> others do the same for other platforms.

Ugh, this is just leading to horrible code here (and why on earth aren't
we just implementing fixups for DT data?).  Though that said I don't
understand why the code here isn't just checking that there's platform
data rather than setting a flag that says there's platform data, the
driver already needs to use the presence of platform data as a check
anyway.

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