| Message ID | 1331218291-16119-6-git-send-email-thierry.reding@avionic-design.de |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Thu, Mar 08, 2012 at 03:51:25PM +0100, Thierry Reding wrote: > +- gpio-controller: mark the device as a GPIO controller > +- regulators: list of regulators provided by this controller, must be in the > + following order: > + SM0, SM1, SM2, LDO0, LDO1, LDO2, LDO3, LDO4, LDO5, LDO6, LDO7, LDO8, LDO9 This ordering requirement is fairly sad, if there are unused regulators they still need to be listed even though... > + sm0_reg: sm0 { > + regulator-min-microvolt = < 725000>; > + regulator-max-microvolt = <1500000>; > + regulator-boot-on; > + regulator-always-on; > + }; > + > + sm1_reg: sm1 { ...they all seem to be explicitly named in the device tree so presumably there's enough information in there for the driver to pick any set of regulators in any order. This would be much nicer to use. Otherwise this looks good.
* Mark Brown wrote: > On Thu, Mar 08, 2012 at 03:51:25PM +0100, Thierry Reding wrote: > > > +- gpio-controller: mark the device as a GPIO controller > > +- regulators: list of regulators provided by this controller, must be in the > > + following order: > > + SM0, SM1, SM2, LDO0, LDO1, LDO2, LDO3, LDO4, LDO5, LDO6, LDO7, LDO8, LDO9 > > This ordering requirement is fairly sad, if there are unused regulators > they still need to be listed even though... > > > + sm0_reg: sm0 { > > + regulator-min-microvolt = < 725000>; > > + regulator-max-microvolt = <1500000>; > > + regulator-boot-on; > > + regulator-always-on; > > + }; > > + > > + sm1_reg: sm1 { > > ...they all seem to be explicitly named in the device tree so presumably > there's enough information in there for the driver to pick any set of > regulators in any order. This would be much nicer to use. I don't like it much either. The only reason that requirement exists is because it makes the assignment of the regulator ID (as defined in the include/linux/mfd/tps6586x.h header) very trivial. Would it be better to look up the ID based on the node name (sm0 --> TPS6586X_ID_SM_0, ...)? Then the only requirement would be that the names match. Thierry
On Thu, Mar 08, 2012 at 04:15:46PM +0100, Thierry Reding wrote: > * Mark Brown wrote: > > ...they all seem to be explicitly named in the device tree so presumably > > there's enough information in there for the driver to pick any set of > > regulators in any order. This would be much nicer to use. > I don't like it much either. The only reason that requirement exists is > because it makes the assignment of the regulator ID (as defined in the > include/linux/mfd/tps6586x.h header) very trivial. Would it be better to > look up the ID based on the node name (sm0 --> TPS6586X_ID_SM_0, ...)? > Then the only requirement would be that the names match. Yes, that's going to be more code but much nicer for users.
* Mark Brown wrote: > On Thu, Mar 08, 2012 at 04:15:46PM +0100, Thierry Reding wrote: > > * Mark Brown wrote: > > > > ...they all seem to be explicitly named in the device tree so presumably > > > there's enough information in there for the driver to pick any set of > > > regulators in any order. This would be much nicer to use. > > > I don't like it much either. The only reason that requirement exists is > > because it makes the assignment of the regulator ID (as defined in the > > include/linux/mfd/tps6586x.h header) very trivial. Would it be better to > > look up the ID based on the node name (sm0 --> TPS6586X_ID_SM_0, ...)? > > > Then the only requirement would be that the names match. > > Yes, that's going to be more code but much nicer for users. Okay, I'll add that for the next round. Thierry
On Thu, 8 Mar 2012 16:15:46 +0100, Thierry Reding <thierry.reding@avionic-design.de> wrote: > * Mark Brown wrote: > > On Thu, Mar 08, 2012 at 03:51:25PM +0100, Thierry Reding wrote: > > > > > +- gpio-controller: mark the device as a GPIO controller > > > +- regulators: list of regulators provided by this controller, must be in the > > > + following order: > > > + SM0, SM1, SM2, LDO0, LDO1, LDO2, LDO3, LDO4, LDO5, LDO6, LDO7, LDO8, LDO9 > > > > This ordering requirement is fairly sad, if there are unused regulators > > they still need to be listed even though... It's worse than that; the DT provides absolutely no guarantees about the ordering of either child nodes or properties. > > > > > + sm0_reg: sm0 { > > > + regulator-min-microvolt = < 725000>; > > > + regulator-max-microvolt = <1500000>; > > > + regulator-boot-on; > > > + regulator-always-on; > > > + }; > > > + > > > + sm1_reg: sm1 { > > > > ...they all seem to be explicitly named in the device tree so presumably > > there's enough information in there for the driver to pick any set of > > regulators in any order. This would be much nicer to use. > > I don't like it much either. The only reason that requirement exists is > because it makes the assignment of the regulator ID (as defined in the > include/linux/mfd/tps6586x.h header) very trivial. Would it be better to > look up the ID based on the node name (sm0 --> TPS6586X_ID_SM_0, ...)? > > Then the only requirement would be that the names match. Yes, please look up id via name. Alternately you can give each child node a 'reg' property and put #address-cells = <1>; #size-cells = <0>; in the parent (assuming the regulator number is a documented attribute of the hardware and not just a convenient linux construct). g. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Grant Likely wrote: > On Thu, 8 Mar 2012 16:15:46 +0100, Thierry Reding <thierry.reding@avionic-design.de> wrote: > > * Mark Brown wrote: > > > ...they all seem to be explicitly named in the device tree so presumably > > > there's enough information in there for the driver to pick any set of > > > regulators in any order. This would be much nicer to use. > > > > I don't like it much either. The only reason that requirement exists is > > because it makes the assignment of the regulator ID (as defined in the > > include/linux/mfd/tps6586x.h header) very trivial. Would it be better to > > look up the ID based on the node name (sm0 --> TPS6586X_ID_SM_0, ...)? > > > > Then the only requirement would be that the names match. > > Yes, please look up id via name. Alternately you can give each child node > a 'reg' property and put #address-cells = <1>; #size-cells = <0>; in the > parent (assuming the regulator number is a documented attribute of the > hardware and not just a convenient linux construct). I'll go with the lookup via name then because as far as I can tell the datasheet doesn't list any specific ordering of the regulators. Thierry
diff --git a/Documentation/devicetree/bindings/regulator/tps6586x.txt b/Documentation/devicetree/bindings/regulator/tps6586x.txt new file mode 100644 index 0000000..e8d2888 --- /dev/null +++ b/Documentation/devicetree/bindings/regulator/tps6586x.txt @@ -0,0 +1,98 @@ +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 in the + following order: + SM0, SM1, SM2, LDO0, LDO1, LDO2, LDO3, LDO4, LDO5, LDO6, LDO7, LDO8, LDO9 + +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..71997d2 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,66 @@ failed: return ret; } +#ifdef CONFIG_OF +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; + + 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 = num; + 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 +543,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 +638,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 29b615c..f10a735 100644 --- a/drivers/regulator/tps6586x-regulator.c +++ b/drivers/regulator/tps6586x-regulator.c @@ -396,7 +396,7 @@ static int __devinit tps6586x_regulator_probe(struct platform_device *pdev) return err; rdev = regulator_register(&ri->desc, &pdev->dev, - pdev->dev.platform_data, ri, NULL); + pdev->dev.platform_data, ri, pdev->dev.of_node); if (IS_ERR(rdev)) { dev_err(&pdev->dev, "failed to register regulator %s\n", ri->desc.name); 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 {
Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de> --- .../devicetree/bindings/regulator/tps6586x.txt | 98 ++++++++++++++++++++ drivers/mfd/tps6586x.c | 66 +++++++++++++ drivers/regulator/tps6586x-regulator.c | 2 +- include/linux/mfd/tps6586x.h | 1 + 4 files changed, 166 insertions(+), 1 deletion(-) create mode 100644 Documentation/devicetree/bindings/regulator/tps6586x.txt