diff mbox

[RFC,v2,2/5] tps6586x: Add device tree support

Message ID 1335347102-14905-3-git-send-email-thierry.reding@avionic-design.de
State Not Applicable, archived
Headers show

Commit Message

Thierry Reding April 25, 2012, 9:44 a.m. UTC
This commit adds device tree support for the TPS6586x regulator.

Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
---
 .../devicetree/bindings/regulator/tps6586x.txt     |   97 ++++++++++++++++++
 drivers/mfd/tps6586x.c                             |  105 ++++++++++++++++++++
 drivers/regulator/tps6586x-regulator.c             |    1 +
 include/linux/mfd/tps6586x.h                       |    1 +
 4 files changed, 204 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/regulator/tps6586x.txt

Comments

Mark Brown April 25, 2012, 10:10 a.m. UTC | #1
On Wed, Apr 25, 2012 at 11:44:59AM +0200, Thierry Reding wrote:
> This commit adds device tree support for the TPS6586x regulator.
> 
> Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>

This looks basically good from a quick scan through but the pattern of
looking up regulator nodes by name is very common so should be factored
out - I made a similar comment in response to a recent patch from
Rhyland Klein and earlier today he posted a patch "regulator: add
generic of node parsing for regulators" which does just that.  Can you
please redo this on top of his code?  I'll probably apply it later
today, though I didn't properly read the code yet.

I guess it should be possible to apply this patch independantly of the
rest of the series?  It shouldn't break bisection if it's missing as
it's a new driver that's being added as the consumer.
Thierry Reding April 25, 2012, 10:14 a.m. UTC | #2
* Mark Brown wrote:
> On Wed, Apr 25, 2012 at 11:44:59AM +0200, Thierry Reding wrote:
> > This commit adds device tree support for the TPS6586x regulator.
> > 
> > Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
> 
> This looks basically good from a quick scan through but the pattern of
> looking up regulator nodes by name is very common so should be factored
> out - I made a similar comment in response to a recent patch from
> Rhyland Klein and earlier today he posted a patch "regulator: add
> generic of node parsing for regulators" which does just that.  Can you
> please redo this on top of his code?  I'll probably apply it later
> today, though I didn't properly read the code yet.

I'll take a look.

> I guess it should be possible to apply this patch independantly of the
> rest of the series?  It shouldn't break bisection if it's missing as
> it's a new driver that's being added as the consumer.

Yes, it can be applied independently.

Thierry
Thierry Reding April 25, 2012, 10:41 a.m. UTC | #3
* Thierry Reding wrote:
> * Mark Brown wrote:
> > On Wed, Apr 25, 2012 at 11:44:59AM +0200, Thierry Reding wrote:
> > > This commit adds device tree support for the TPS6586x regulator.
> > > 
> > > Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
> > 
> > This looks basically good from a quick scan through but the pattern of
> > looking up regulator nodes by name is very common so should be factored
> > out - I made a similar comment in response to a recent patch from
> > Rhyland Klein and earlier today he posted a patch "regulator: add
> > generic of node parsing for regulators" which does just that.  Can you
> > please redo this on top of his code?  I'll probably apply it later
> > today, though I didn't properly read the code yet.
> 
> I'll take a look.

After taking a closer look I don't think Rhyland's patch is very useful for
this driver. I need to lookup the platform ID by regulator name anyway so
using the new code is actually more work and requires a second table that
lists the regulator names only.

Thierry
Mark Brown April 25, 2012, 10:47 a.m. UTC | #4
On Wed, Apr 25, 2012 at 12:41:47PM +0200, Thierry Reding wrote:

> After taking a closer look I don't think Rhyland's patch is very useful for
> this driver. I need to lookup the platform ID by regulator name anyway so
> using the new code is actually more work and requires a second table that
> lists the regulator names only.

Why do you need the plaform ID, and if it is needed could we work out a
way to make the generic code do that lookup for you (since presumably
other drivers will have the same requirement)?  If you can't use the
generic code it seems like the fix is to enhance the generic code and
I'd expect that something not requiring the platform ID would just be
able to igore that information if the generic code could look it up.
Thierry Reding April 25, 2012, 11:14 a.m. UTC | #5
* Mark Brown wrote:
> On Wed, Apr 25, 2012 at 12:41:47PM +0200, Thierry Reding wrote:
> 
> > After taking a closer look I don't think Rhyland's patch is very useful for
> > this driver. I need to lookup the platform ID by regulator name anyway so
> > using the new code is actually more work and requires a second table that
> > lists the regulator names only.
> 
> Why do you need the plaform ID, and if it is needed could we work out a
> way to make the generic code do that lookup for you (since presumably
> other drivers will have the same requirement)?  If you can't use the
> generic code it seems like the fix is to enhance the generic code and
> I'd expect that something not requiring the platform ID would just be
> able to igore that information if the generic code could look it up.

Basically the platform ID is used by the regulator driver to differentiate
between the different types of regulators. I could imagine a more advanced
setup to do the matching, along the lines of OF device matching and platform
device matching, where an entry in the table has both a name and an
associated "driver data" field.

For instance we could have a new structure:

	struct regulator_lookup_data {
		const char *name;
		unsigned long driver_data;
		struct regulator_init_data *init_data;
		struct device_node *of_node;
	};

	static struct regulator_lookup_data tps6586x_lookup[] = {
		{ .name = "sm0", .driver_data = TPS6586X_ID_SM_0 },
		{ .name = "sm1", .driver_data = TPS6586X_ID_SM_1 },
		...
	};

And pass that into of_find_regulator_init_data_from_device():

	of_find_regulator_init_data_from_device(&client->dev, regulators,
						tps6586x_lookup,
						ARRAY_SIZE(tps6586x_lookup));

Upon return, the tps6586x_lookup table will have the init_data and of_node
fields filled with the data parsed from the DT. Then we can simply iterate
over all entries and add the corresponding regulators.

This is somewhat ugly because of_find_regulator_init_data_from_device() would
modify the same data structure for potentially different devices. This should
be okay because the driver core takes care of serializing device probing
(unless I am mistaken). Alternatively we could make the function return a
copy of the regulator_lookup_data structure with the additional fields filled
in.

Thierry
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/regulator/tps6586x.txt b/Documentation/devicetree/bindings/regulator/tps6586x.txt
new file mode 100644
index 0000000..0fcabaa3
--- /dev/null
+++ b/Documentation/devicetree/bindings/regulator/tps6586x.txt
@@ -0,0 +1,97 @@ 
+TPS6586x family of regulators
+
+Required properties:
+- compatible: "ti,tps6586x"
+- reg: I2C slave address
+- interrupts: the interrupt outputs of the controller
+- #gpio-cells: number of cells to describe a GPIO
+- gpio-controller: mark the device as a GPIO controller
+- regulators: list of regulators provided by this controller, must be named
+  after their hardware counterparts: sm[0-2], ldo[0-9] and ldo_rtc
+
+Each regulator is defined using the standard binding for regulators.
+
+Example:
+
+	pmu: tps6586x@34 {
+		compatible = "ti,tps6586x";
+		reg = <0x34>;
+		interrupts = <0 88 0x4>;
+
+		#gpio-cells = <2>;
+		gpio-controller;
+
+		regulators {
+			sm0_reg: sm0 {
+				regulator-min-microvolt = < 725000>;
+				regulator-max-microvolt = <1500000>;
+				regulator-boot-on;
+				regulator-always-on;
+			};
+
+			sm1_reg: sm1 {
+				regulator-min-microvolt = < 725000>;
+				regulator-max-microvolt = <1500000>;
+				regulator-boot-on;
+				regulator-always-on;
+			};
+
+			sm2_reg: sm2 {
+				regulator-min-microvolt = <3000000>;
+				regulator-max-microvolt = <4550000>;
+				regulator-boot-on;
+				regulator-always-on;
+			};
+
+			ldo0_reg: ldo0 {
+				regulator-name = "PCIE CLK";
+				regulator-min-microvolt = <3300000>;
+				regulator-max-microvolt = <3300000>;
+			};
+
+			ldo1_reg: ldo1 {
+				regulator-min-microvolt = < 725000>;
+				regulator-max-microvolt = <1500000>;
+			};
+
+			ldo2_reg: ldo2 {
+				regulator-min-microvolt = < 725000>;
+				regulator-max-microvolt = <1500000>;
+			};
+
+			ldo3_reg: ldo3 {
+				regulator-min-microvolt = <1250000>;
+				regulator-max-microvolt = <3300000>;
+			};
+
+			ldo4_reg: ldo4 {
+				regulator-min-microvolt = <1700000>;
+				regulator-max-microvolt = <2475000>;
+			};
+
+			ldo5_reg: ldo5 {
+				regulator-min-microvolt = <1250000>;
+				regulator-max-microvolt = <3300000>;
+			};
+
+			ldo6_reg: ldo6 {
+				regulator-min-microvolt = <1250000>;
+				regulator-max-microvolt = <3300000>;
+			};
+
+			ldo7_reg: ldo7 {
+				regulator-min-microvolt = <1250000>;
+				regulator-max-microvolt = <3300000>;
+			};
+
+			ldo8_reg: ldo8 {
+				regulator-min-microvolt = <1250000>;
+				regulator-max-microvolt = <3300000>;
+			};
+
+			ldo9_reg: ldo9 {
+				regulator-min-microvolt = <1250000>;
+				regulator-max-microvolt = <3300000>;
+			};
+		};
+	};
diff --git a/drivers/mfd/tps6586x.c b/drivers/mfd/tps6586x.c
index a5ddf31..7614b1b 100644
--- a/drivers/mfd/tps6586x.c
+++ b/drivers/mfd/tps6586x.c
@@ -23,6 +23,7 @@ 
 #include <linux/slab.h>
 #include <linux/gpio.h>
 #include <linux/i2c.h>
+#include <linux/regulator/of_regulator.h>
 
 #include <linux/mfd/core.h>
 #include <linux/mfd/tps6586x.h>
@@ -460,6 +461,7 @@  static int __devinit tps6586x_add_subdevs(struct tps6586x *tps6586x,
 
 		pdev->dev.parent = tps6586x->dev;
 		pdev->dev.platform_data = subdev->platform_data;
+		pdev->dev.of_node = subdev->of_node;
 
 		ret = platform_device_add(pdev);
 		if (ret) {
@@ -474,6 +476,105 @@  failed:
 	return ret;
 }
 
+#ifdef CONFIG_OF
+static struct {
+	const char *name;
+	int id;
+} tps6586x_id_map[] = {
+	{ "sm0", TPS6586X_ID_SM_0 },
+	{ "sm1", TPS6586X_ID_SM_1 },
+	{ "sm2", TPS6586X_ID_SM_2 },
+	{ "ldo0", TPS6586X_ID_LDO_0 },
+	{ "ldo1", TPS6586X_ID_LDO_1 },
+	{ "ldo2", TPS6586X_ID_LDO_2 },
+	{ "ldo3", TPS6586X_ID_LDO_3 },
+	{ "ldo4", TPS6586X_ID_LDO_4 },
+	{ "ldo5", TPS6586X_ID_LDO_5 },
+	{ "ldo6", TPS6586X_ID_LDO_6 },
+	{ "ldo7", TPS6586X_ID_LDO_7 },
+	{ "ldo8", TPS6586X_ID_LDO_8 },
+	{ "ldo9", TPS6586X_ID_LDO_9 },
+	{ "ldo_rtc", TPS6586X_ID_LDO_RTC },
+};
+
+static int tps6586x_lookup_id(const char *name)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(tps6586x_id_map); i++)
+		if (strcmp(name, tps6586x_id_map[i].name) == 0)
+			return tps6586x_id_map[i].id;
+
+	return -1;
+}
+
+static struct tps6586x_platform_data *tps6586x_parse_dt(struct i2c_client *client)
+{
+	struct device_node *np = client->dev.of_node;
+	struct tps6586x_platform_data *pdata;
+	struct tps6586x_subdev_info *devs;
+	struct device_node *regulators;
+	struct device_node *child;
+	unsigned int num = 0;
+
+	regulators = of_find_node_by_name(np, "regulators");
+
+	for_each_child_of_node(regulators, child)
+		num++;
+
+	devs = devm_kzalloc(&client->dev, num * sizeof(*devs), GFP_KERNEL);
+	if (!devs)
+		return NULL;
+
+	num = 0;
+
+	for_each_child_of_node(regulators, child) {
+		struct regulator_init_data *init_data;
+		int id;
+
+		id = tps6586x_lookup_id(child->name);
+		if (id < 0) {
+			dev_err(&client->dev, "invalid regulator name: %s\n",
+				child->name);
+			continue;
+		}
+
+		init_data = of_get_regulator_init_data(&client->dev, child);
+		if (!init_data)
+			dev_err(&client->dev,
+				"failed to parse DT for regulator %s\n",
+				child->name);
+
+		devs[num].name = "tps6586x-regulator";
+		devs[num].platform_data = init_data;
+		devs[num].of_node = child;
+		devs[num].id = id;
+		num++;
+	}
+
+	pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL);
+	if (!pdata)
+		return NULL;
+
+	pdata->num_subdevs = num;
+	pdata->subdevs = devs;
+	pdata->gpio_base = -1;
+	pdata->irq_base = -1;
+
+	return pdata;
+}
+
+static struct of_device_id tps6586x_of_match[] = {
+	{ .compatible = "ti,tps6586x", },
+	{ },
+};
+#else
+static struct tps6586x_platform_data *tps6586x_parse_dt(struct i2c_client *client)
+{
+	return NULL;
+}
+#endif
+
 static int __devinit tps6586x_i2c_probe(struct i2c_client *client,
 					const struct i2c_device_id *id)
 {
@@ -481,6 +582,9 @@  static int __devinit tps6586x_i2c_probe(struct i2c_client *client,
 	struct tps6586x *tps6586x;
 	int ret;
 
+	if (!pdata && client->dev.of_node)
+		pdata = tps6586x_parse_dt(client);
+
 	if (!pdata) {
 		dev_err(&client->dev, "tps6586x requires platform data\n");
 		return -ENOTSUPP;
@@ -573,6 +677,7 @@  static struct i2c_driver tps6586x_driver = {
 	.driver	= {
 		.name	= "tps6586x",
 		.owner	= THIS_MODULE,
+		.of_match_table = of_match_ptr(tps6586x_of_match),
 	},
 	.probe		= tps6586x_i2c_probe,
 	.remove		= __devexit_p(tps6586x_i2c_remove),
diff --git a/drivers/regulator/tps6586x-regulator.c b/drivers/regulator/tps6586x-regulator.c
index 9a4029a..c0a2145 100644
--- a/drivers/regulator/tps6586x-regulator.c
+++ b/drivers/regulator/tps6586x-regulator.c
@@ -363,6 +363,7 @@  static int __devinit tps6586x_regulator_probe(struct platform_device *pdev)
 		return err;
 
 	config.dev = &pdev->dev;
+	config.of_node = pdev->dev.of_node;
 	config.init_data = pdev->dev.platform_data;
 	config.driver_data = ri;
 
diff --git a/include/linux/mfd/tps6586x.h b/include/linux/mfd/tps6586x.h
index b19176e..f350fd0 100644
--- a/include/linux/mfd/tps6586x.h
+++ b/include/linux/mfd/tps6586x.h
@@ -68,6 +68,7 @@  struct tps6586x_subdev_info {
 	int		id;
 	const char	*name;
 	void		*platform_data;
+	struct device_node *of_node;
 };
 
 struct tps6586x_platform_data {