Device tree support for regulators

of_regulator_register_devices() registers all regulators as platform devices. Use this to register all twl regulators from the twl driver probe.

 drivers/mfd/twl-core.c       |    3 +++
 drivers/of/of_regulator.c    |   30 ++++++++++++++++++++++++++++++
 include/linux/of_regulator.h |    5 +++++

drivers/of/of_regulator.c +13 -0

+/**
+ * of_regulator_register_devices - Register regulator devices to platform bus
+ * @np:	Parent device node with regulator child nodes
+ *
+ * Registers all the regulator and regulator-fixed nodes as platform devices
+ *
+ */
+void of_regulator_register_devices(struct device_node *np)
+{
+	struct device_node *child;
+	struct platform_device *dev;
+
+	for_each_child_of_node(np, child) {

drivers/of/of_regulator.c +5 -0

+		}
+	}
+	return;
+}
+

include/linux/of_regulator.h +5 -0

index 5fc7329..38cf7e3 100644
@@ -15,6 +15,7 @@ extern struct fixed_voltage_config
 	*of_get_fixed_voltage_config(struct device_node *np);
 extern struct device_node *of_get_regulator(struct device *dev,
 	const char *id);
+extern void of_regulator_register_devices(struct device_node *np);
 #else
 static inline struct regulator_init_data
 	*of_get_regulator_init_data(struct device_node *np)

@@ -31,6 +32,10 @@ static inline struct device_node *of_get_regulator(struct device *dev,
 {
 	return NULL;
 }
+static inline void of_regulator_register_devices(struct device_node *np)
+{
+	return NULL;
+}
 #endif /* CONFIG_OF_REGULATOR */
 
 #endif /* __LINUX_OF_REG_H */

Signature · 1 line

1.7.1

Quoted 3 lines · Rajendra Nayak

of_regulator_register_devices() registers all regulators as platform devices. Use this to register all twl regulators from the twl driver probe.

Regulators can be devices of any type, not just platform devices.

Quoted 4 lines

  drivers/mfd/twl-core.c       |    3 +++
  drivers/of/of_regulator.c    |   30 ++++++++++++++++++++++++++++++
  include/linux/of_regulator.h |    5 +++++

Again, in the regulator code not hidden away please.

Quoted 13 lines

drivers/of/of_regulator.c +13 -0

+/**
+ * of_regulator_register_devices - Register regulator devices to platform bus
+ * @np:	Parent device node with regulator child nodes
+ *
+ * Registers all the regulator and regulator-fixed nodes as platform devices
+ *
+ */
+void of_regulator_register_devices(struct device_node *np)
+{
+	struct device_node *child;
+	struct platform_device *dev;
+
+	for_each_child_of_node(np, child) {

drivers/of/of_regulator.c struct device_node *of_get_regulator(struct device *dev, const char *supply) · line 178

Quoted 10 lines

drivers/of/of_regulator.c +10 -0

+		if (of_device_is_compatible(child, "regulator")
+			|| of_device_is_compatible(child, "regulator-fixed")) {
+			dev = of_device_alloc(child, NULL, NULL);
+			if (!dev)
+				return;
+			dev->dev.bus =&platform_bus_type;
+			if (of_device_add(dev) != 0) {
+				platform_device_put(dev);
+				return;
+			}

I'm not entirely sure what this is for? Surely we should be instantiating the subdevices of the MFD in the same way we always have done?

Quoted 29 lines

drivers/of/of_regulator.c +5 -0

+		}
+	}
+	return;
+}
+

include/linux/of_regulator.h +5 -1

index 5fc7329..38cf7e3 100644
@@ -15,6 +15,7 @@ extern struct fixed_voltage_config
  	*of_get_fixed_voltage_config(struct device_node *np);
  extern struct device_node *of_get_regulator(struct device *dev,
  	const char *id);
+extern void of_regulator_register_devices(struct device_node *np);
  #else
  static inline struct regulator_init_data
  	*of_get_regulator_init_data(struct device_node *np)

@@ -31,6 +32,10 @@ static inline struct device_node *of_get_regulator(struct device *dev,
  {
  	return NULL;
  }
+static inline void of_regulator_register_devices(struct device_node *np)
+{
+	return NULL;
+}
  #endif /* CONFIG_OF_REGULATOR */
 
  #endif /* __LINUX_OF_REG_H */
--

Quoted 2 lines

1.7.1

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

drivers/regulator/core.c +9 -0

+#ifdef CONFIG_OF
+	struct device_node *node;
+	node = of_get_regulator(dev, id);
+	if (!node)
+		goto out;
+	list_for_each_entry(rdev, &regulator_list, list)
+		if (node == rdev->node)
+			goto found;
+#else
 	list_for_each_entry(map, &regulator_map_list, list) {
 		/* If the mapping has a device set up it must match */
 		if (map->dev_name &&

Quoted 13 lines · Rajendra Nayak

drivers/regulator/core.c +9 -0

+#ifdef CONFIG_OF
+	struct device_node *node;
+	node = of_get_regulator(dev, id);
+	if (!node)
+		goto out;
+	list_for_each_entry(rdev, &regulator_list, list)
+		if (node == rdev->node)
+			goto found;
+#else
  	list_for_each_entry(map,&regulator_map_list, list) {
  		/* If the mapping has a device set up it must match */
  		if (map->dev_name &&

@@ -1178,6 +1189,7 @@ static struct regulator *_regulator_get(struct device *dev, const char *id,

drivers/regulator/core.c static struct regulator *_regulator_get(struct device *dev, const char *id, · line 1192

Quoted 4 lines

drivers/regulator/core.c +1 -0

  			goto found;
  		}
  	}
+#endif

This forces all machines to use device tree when CONFIG_OF is enabled. I'd expect to see both mechanisms supported simultaneously, for example by falling back to the current code if the device tree lookup fails.

Quoted 5 lines

drivers/regulator/core.c +0 -0

@@ -1216,6 +1228,15 @@ found:
  	if (!try_module_get(rdev->owner))
  		goto out;
 

drivers/regulator/core.c found: · line 1239

Quoted 9 lines

drivers/regulator/core.c +9 -0

+#ifdef CONFIG_OF
+	ret = set_consumer_device_supply(rdev, dev, devname, id);
+	if (ret <  0) {
+		dev_err(dev, "Failed to set supply %d\n", ret);
+		unset_regulator_supplies(rdev);
+		goto out;
+	}
+#endif
+

This seems wrong, why are we adding things to the regulator_map which is really only there for lookups when we already did the lookup using the device tree?

drivers/regulator/core.c struct regulator_dev *regulator_register(struct regulator_desc *regulator_desc, · line 2644

Quoted 7 lines

drivers/regulator/core.c +2 -0

@@ -2619,6 +2640,8 @@ struct regulator_dev *regulator_register(struct regulator_desc *regulator_desc,
  	rdev->reg_data = driver_data;
  	rdev->owner = regulator_desc->owner;
  	rdev->desc = regulator_desc;
+	if (dev &&  dev->of_node)
+		rdev->node = dev->of_node;

dev is mandatory.

Quoted 3 lines · Mark Brown

This seems wrong, why are we adding things to the regulator_map which is really only there for lookups when we already did the lookup using the device tree?

I did this only so the mappings show up in sysfs.

		regulator = <&regulator1>,<&regulator2>;
		regulator-names = "supply1","supply2";
	};

Quoted 3 lines · Rajendra Nayak

		regulator = <&regulator1>,<&regulator2>;
		regulator-names = "supply1","supply2";
	};

This syntax is really painful - we're relying on keeping two arrays in sync which isn't good for legibility or robustness. I'd expect something like having a property with the supply name referencing the regulator node concerned like:

	dbvdd = <&regulator1>;
	dcvdd = <&regulator2>;

or something. Keeping the two arrays separate doesn't seem great.

We've got two competing approaches here. For reg and interrupts, the proposal on the table that we talked about at LPC is to do reg-names and interrupts-names so as to preserve the existing semantics of the reg and interrupts properties. For gpios we're using the binding "<name>-gpios" for named gpio references. There isn't the same pressure to preserve existing bindings in that case. I'm okay with either approach, providing that "-regulator" is encoded into the name.

Quoted 7 lines · Grant Likely

We've got two competing approaches here. For reg and interrupts, the proposal on the table that we talked about at LPC is to do reg-names and interrupts-names so as to preserve the existing semantics of the reg and interrupts properties. For gpios we're using the binding "<name>-gpios" for named gpio references. There isn't the same pressure to preserve existing bindings in that case. I'm okay with either approach, providing that "-regulator" is encoded into the name.

I think I prefer the latter but I'm probably not too fussed.

I'd rather use something like -supply for these just on the basis that you don't always connect things to regulators (for example, speaker supplies are generally attached direct to the battery).

Modify the fixed regulator driver to extract fixed_voltage_config from device tree when passed, instead of getting it through platform_data structures (on non-DT builds)

 .../devicetree/bindings/regulator/regulator.txt    |   19 ++++++++++++
 drivers/of/of_regulator.c                          |   31 ++++++++++++++++++++
 include/linux/of_regulator.h                       |    7 ++++
 3 files changed, 57 insertions(+), 0 deletions(-)

Required properties:
- compatible: Must be "regulator";
+or
+- compatible: Must be "regulator-fixed"; /* For Fixed volatge regulator */

+# For fixed voltage regulators

+- supply-name: Name of the regulator supply
+- microvolts: Output voltage of regulator
+- gpio: gpio to use for enable control
+- startup-delay: startup time in microseconds
+- enable-high: Polarity of enable GPIO, 1 = Active High, 0 = Active low
+- enabled-at-boot: 1 = yes, 0 = no

Quoted 4 lines · Rajendra Nayak

  .../devicetree/bindings/regulator/regulator.txt    |   19 ++++++++++++
  drivers/of/of_regulator.c                          |   31 ++++++++++++++++++++
  include/linux/of_regulator.h                       |    7 ++++
  3 files changed, 57 insertions(+), 0 deletions(-)

Again, this should be part of the regulator API code not hidden away where it will only get reviewed by device tree people.

Quoted 5 lines

Required properties:
- compatible: Must be "regulator";
+or
+- compatible: Must be "regulator-fixed"; /* For Fixed volatge regulator */

This seems at best confusing - the fixed voltage regulator is a particular regulator driver, and the general concept of a fixed voltage regulator is still a subset of regulators.

Quoted 8 lines

+# For fixed voltage regulators

+- supply-name: Name of the regulator supply
+- microvolts: Output voltage of regulator
+- gpio: gpio to use for enable control
+- startup-delay: startup time in microseconds
+- enable-high: Polarity of enable GPIO, 1 = Active High, 0 = Active low
+- enabled-at-boot: 1 = yes, 0 = no

Much of this is specific to the Linux fixed voltage regulator driver rather than a generic regulator with a fixed voltage.

Quoted 9 lines · Mark Brown

Quoted 7 lines · Rajendra Nayak

+# For fixed voltage regulators

+- supply-name: Name of the regulator supply
+- microvolts: Output voltage of regulator
+- gpio: gpio to use for enable control
+- startup-delay: startup time in microseconds
+- enable-high: Polarity of enable GPIO, 1 = Active High, 0 = Active low
+- enabled-at-boot: 1 = yes, 0 = no

Quoted 2 lines

Much of this is specific to the Linux fixed voltage regulator driver rather than a generic regulator with a fixed voltage.

So how should these non-generic things be handled through device tree? Should they never be part of dt or should the bindings just be defined such that its clear they are linux specific?

Quoted 13 lines · Rajendra Nayak

Quoted 12 lines · Mark Brown

Quoted 7 lines · Rajendra Nayak

+# For fixed voltage regulators

+- supply-name: Name of the regulator supply
+- microvolts: Output voltage of regulator
+- gpio: gpio to use for enable control
+- startup-delay: startup time in microseconds
+- enable-high: Polarity of enable GPIO, 1 = Active High, 0 = Active low
+- enabled-at-boot: 1 = yes, 0 = no

Much of this is specific to the Linux fixed voltage regulator driver rather than a generic regulator with a fixed voltage.

Quoted 3 lines

So how should these non-generic things be handled through device tree? Should they never be part of dt or should the bindings just be defined such that its clear they are linux specific?

Having them be part of the fixed voltage regulator bindings would be fine, the problem is that you're adding this in framework code not in a driver.

The helper routine defined here (of_get_fixed_voltage_config) can be used to extract information about fixed regulators (which are not software controlable) from device tree.

Add documenation about additional bindings for fixed regulators that can be passed through DT.

Signed-off-by Rajendra Nayak <[email protected]>

---
 .../devicetree/bindings/regulator/regulator.txt    |   19 ++++++++++++
 drivers/of/of_regulator.c                          |   31 ++++++++++++++++++++
 include/linux/of_regulator.h                       |    7 ++++
 3 files changed, 57 insertions(+), 0 deletions(-)

Documentation/devicetree/bindings/regulator/regulator.txt +0 -0

index 001e5ce..f8c51d8 100644

Documentation/devicetree/bindings/regulator/regulator.txt +17 -0

 Optional properties:
 - supply-regulator: Name of the parent regulator

@@ -24,6 +26,13 @@ Optional properties:
 - always-on: boolean, regulator should never be disabled
 - boot-on: bootloader/firmware enabled regulator
 - apply-uV: apply uV constraint if min == max
+# For fixed voltage regulators
+- supply-name: Name of the regulator supply
+- microvolts: Output voltage of regulator
+- gpio: gpio to use for enable control
+- startup-delay: startup time in microseconds
+- enable-high: Polarity of enable GPIO, 1 = Active High, 0 = Active low
+- enabled-at-boot: 1 = yes, 0 = no
 
 Example:
 

@@ -35,3 +44,13 @@ Example:
 		change-voltage;
 		always-on;
 	};
+
+	abc-fixedregulator {
+		compatible = "regulator-fixed";
+		supply-name = "fixed-supply";
+		microvolts = <1800000>;
+		gpio = <43>;
+		startup-delay = <70000>;
+		enable-high;
+		enabled-at-boot;
+	};

drivers/of/of_regulator.c +31 -0

index f01d275..4d7a49d 100644
@@ -13,6 +13,7 @@
 #include <linux/slab.h>
 #include <linux/of.h>
 #include <linux/regulator/machine.h>
+#include <linux/regulator/fixed.h>
 
 static void of_get_regulation_constraints(struct device_node *np,
 					struct regulator_init_data **init_data)

@@ -83,3 +84,33 @@ struct regulator_init_data *of_get_regulator_init_data(struct device_node *np)
 	return init_data;
 }
 EXPORT_SYMBOL(of_get_regulator_init_data);
+
+/**
+ * of_get_fixed_voltage_config - extract fixed_voltage_config structure info
+ * @np: Pointer to fixed-regulator device tree node
+ *
+ * Populates fixed_voltage_config structure by extracting data from device
+ * tree node, returns a pointer to the populated structure of NULL if memory
+ * alloc fails.
+ */
+struct fixed_voltage_config *of_get_fixed_voltage_config(struct device_node *np)
+{
+	struct fixed_voltage_config *config;
+
+	config = kzalloc(sizeof(struct fixed_voltage_config), GFP_KERNEL);
+	if (!config)
+		return NULL;
+
+	config->supply_name = (char *)of_get_property(np, "supply-name", NULL);
+	of_property_read_u32(np, "microvolts", &config->microvolts);
+	of_property_read_u32(np, "gpio", &config->gpio);
+	of_property_read_u32(np, "startup-delay", &config->startup_delay);
+	if (of_find_property(np, "enable-high", NULL))
+		config->enable_high = true;
+	if (of_find_property(np, "enabled-at-boot", NULL))
+		config->enabled_at_boot = true;
+	config->init_data = of_get_regulator_init_data(np);
+
+	return config;
+}
+EXPORT_SYMBOL(of_get_fixed_voltage_config);

include/linux/of_regulator.h +7 -0

index 5eb048c..39860c5 100644
@@ -11,12 +11,19 @@
 #if defined(CONFIG_OF_REGULATOR)
 extern struct regulator_init_data
 	*of_get_regulator_init_data(struct device_node *np);
+extern struct fixed_voltage_config
+	*of_get_fixed_voltage_config(struct device_node *np);
 #else
 static inline struct regulator_init_data
 	*of_get_regulator_init_data(struct device_node *np)
 {
 	return NULL;
 }
+static inline struct fixed_voltage_config
+	*of_get_fixed_voltage_config(struct device_node *np)
+{
+	return NULL;
+}
 #endif /* CONFIG_OF_REGULATOR */
 
 #endif /* __LINUX_OF_REG_H */

Signature · 1 line

1.7.1

Modify the twl regulator driver to extract the regulator_init_data from device tree when passed, instead of getting it through platform_data structures (on non-DT builds)

Signed-off-by Rajendra Nayak <[email protected]>

---
 drivers/regulator/twl-regulator.c |   28 +++++++++++++++++++++++++---
 1 files changed, 25 insertions(+), 3 deletions(-)

drivers/regulator/twl-regulator.c +2 -0

index ee8747f..df1b95a 100644
@@ -17,6 +17,8 @@
 #include <linux/regulator/driver.h>
 #include <linux/regulator/machine.h>
 #include <linux/i2c/twl.h>
+#include <linux/of.h>
+#include <linux/of_regulator.h>
 
 
 /*

drivers/regulator/twl-regulator.c +5 -1

 	for (i = 0, info = NULL; i < ARRAY_SIZE(twl_regs); i++) {
 		if (twl_regs[i].desc.id != pdev->id)
 			continue;

@@ -1020,7 +1025,11 @@ static int __devinit twlreg_probe(struct platform_device *pdev)
 	if (!info)
 		return -ENODEV;
 
-	initdata = pdev->dev.platform_data;
+	if (pdev->dev.of_node)
+		initdata = of_get_regulator_init_data(pdev->dev.of_node);
+	else
+		initdata = pdev->dev.platform_data;
+
 	if (!initdata)
 		return -EINVAL;
 

drivers/regulator/twl-regulator.c +12 -2

+	{},
+};
+MODULE_DEVICE_TABLE(of, twl_of_match);
+#else
+#define twl_of_match NULL
+#endif
+
 static struct platform_driver twlreg_driver = {
 	.probe		= twlreg_probe,
 	.remove		= __devexit_p(twlreg_remove),
 	/* NOTE: short name, to work around driver model truncation of
 	 * "twl_regulator.12" (and friends) to "twl_regulator.1".
 	 */
-	.driver.name	= "twl_reg",
-	.driver.owner	= THIS_MODULE,
+	.driver  = {
+		.name  = "twl_reg",
+		.owner = THIS_MODULE,
+		.of_match_table = twl_of_match,
+	},
 };
 
 static int __init twlreg_init(void)

Signature · 1 line

1.7.1

Quoted 26 lines · Rajendra Nayak

Modify the twl regulator driver to extract the regulator_init_data from device tree when passed, instead of getting it through platform_data structures (on non-DT builds)

Signed-off-by Rajendra Nayak<[email protected]>

---
  drivers/regulator/twl-regulator.c |   28 +++++++++++++++++++++++++---
  1 files changed, 25 insertions(+), 3 deletions(-)

drivers/regulator/twl-regulator.c +2 -0

index ee8747f..df1b95a 100644
@@ -17,6 +17,8 @@
  #include<linux/regulator/driver.h>
  #include<linux/regulator/machine.h>
  #include<linux/i2c/twl.h>
+#include<linux/of.h>
+#include<linux/of_regulator.h>
 
 
  /*

@@ -1011,6 +1013,9 @@ static int __devinit twlreg_probe(struct platform_device *pdev)
  	struct regulation_constraints	*c;
  	struct regulator_dev		*rdev;
 

drivers/regulator/twl-regulator.c static int __devinit twlreg_probe(struct platform_device *pdev) · line 1018

Quoted 3 lines

drivers/regulator/twl-regulator.c +3 -0

+	if (pdev->dev.of_node)
+		of_property_read_u32(pdev->dev.of_node, "ti,reg-id",&pdev->id);
+

Don't do this. As much as possible, don't reply on plaform_device->id when using DT. Plus it is illegal to modify pdev->id after the device is registered.

Quoted 19 lines

drivers/regulator/twl-regulator.c +5 -1

  	for (i = 0, info = NULL; i<  ARRAY_SIZE(twl_regs); i++) {
  		if (twl_regs[i].desc.id != pdev->id)
  			continue;

@@ -1020,7 +1025,11 @@ static int __devinit twlreg_probe(struct platform_device *pdev)
  	if (!info)
  		return -ENODEV;
 
-	initdata = pdev->dev.platform_data;
+	if (pdev->dev.of_node)
+		initdata = of_get_regulator_init_data(pdev->dev.of_node);
+	else
+		initdata = pdev->dev.platform_data;
+
  	if (!initdata)
  		return -EINVAL;
 

@@ -1101,14 +1110,27 @@ static int __devexit twlreg_remove(struct platform_device *pdev)
 

drivers/regulator/twl-regulator.c static int __devexit twlreg_remove(struct platform_device *pdev) · line 1115

Quoted 5 lines

drivers/regulator/twl-regulator.c +3 -0

  MODULE_ALIAS("platform:twl_reg");
 
+#if defined(CONFIG_OF)
+static const struct of_device_id twl_of_match[] __devinitconst = {
+	{ .compatible = "ti,twl-reg", },

This looks rather generic. Is this a specific chip? It should be.

g.

Quoted 26 lines

drivers/regulator/twl-regulator.c +12 -3

+	{},
+};
+MODULE_DEVICE_TABLE(of, twl_of_match);
+#else
+#define twl_of_match NULL
+#endif
+
  static struct platform_driver twlreg_driver = {
  	.probe		= twlreg_probe,
  	.remove		= __devexit_p(twlreg_remove),
  	/* NOTE: short name, to work around driver model truncation of
  	 * "twl_regulator.12" (and friends) to "twl_regulator.1".
  	 */
-	.driver.name	= "twl_reg",
-	.driver.owner	= THIS_MODULE,
+	.driver  = {
+		.name  = "twl_reg",
+		.owner = THIS_MODULE,
+		.of_match_table = twl_of_match,
+	},
  };
 
  static int __init twlreg_init(void)
--

1.7.1

+Required properties:
+- compatible: Must be "regulator","ti,twl-reg";

Quoted 2 lines · Rajendra Nayak

+Required properties:
+- compatible: Must be "regulator","ti,twl-reg";

I'd expect listings for the specific chips too.

Documentation/devicetree/bindings/regulator/twl-regulator.txt line 14

Quoted 3 lines

Documentation/devicetree/bindings/regulator/twl-regulator.txt new file +3 -0

+	xyz-regulator: regulator at 0 {
+		compatible = "regulator","ti,twl-reg";
+		ti,reg-id = <37>; /* TWL6030_REG_VAUX1_6030 */

These magic numbers are *very* Linux specific, we should have a better way of specifying regulators - I'd off the top of my head expect that the compatible property would identify the regulator.

Quoted 2 lines · Rajendra Nayak

+Required properties:
+- compatible: Must be "regulator","ti,twl-reg";

I'd expect listings for the specific chips too.

Documentation/devicetree/bindings/regulator/twl-regulator.txt

Quoted 4 lines

Documentation/devicetree/bindings/regulator/twl-regulator.txt new file +3 -0

+	xyz-regulator: regulator at 0 {
+		compatible = "regulator","ti,twl-reg";
+		ti,reg-id =<37>; /* TWL6030_REG_VAUX1_6030 */

These magic numbers are *very* Linux specific, we should have a better way of specifying regulators - I'd off the top of my head expect that the compatible property would identify the regulator.

Quoted 4 lines · Mark Brown

Quoted 2 lines · Rajendra Nayak

+Required properties:
+- compatible: Must be "regulator","ti,twl-reg";

Quoted 1 line

I'd expect listings for the specific chips too.

I just did'nt do that because we have just one driver for all twl chips (twl4030/twl6030/twl6025) and there seems to be no real need to identify specific chips while we could do knowing just the chip family.

Quoted 7 lines

Documentation/devicetree/bindings/regulator/twl-regulator.txt

Quoted 3 lines

Documentation/devicetree/bindings/regulator/twl-regulator.txt new file +3 -0

+	xyz-regulator: regulator at 0 {
+		compatible = "regulator","ti,twl-reg";
+		ti,reg-id =<37>; /* TWL6030_REG_VAUX1_6030 */

These magic numbers are *very* Linux specific, we should have a better way of specifying regulators - I'd off the top of my head expect that the compatible property would identify the regulator.

The driver seems to use a per-regulator table, and it uses the above id to indexed into it. I could probably do it with the compatible

property, but that would mean I have a compatible for *each* regulator instance, like "ti,twl-reg-vaux1", "ti,twl-reg-vmmc" etc. Does that sound reasonable?

Quoted 5 lines · Rajendra Nayak

Quoted 4 lines · Mark Brown

Quoted 2 lines · Rajendra Nayak

+Required properties:
+- compatible: Must be "regulator","ti,twl-reg";

Quoted 6 lines

Quoted 1 line

I'd expect listings for the specific chips too.

I just did'nt do that because we have just one driver for all twl chips (twl4030/twl6030/twl6025) and there seems to be no real need to identify specific chips while we could do knowing just the chip family.

The driver can bind to as many names as it likes.

Quoted 11 lines

Quoted 8 lines

Documentation/devicetree/bindings/regulator/twl-regulator.txt

Quoted 4 lines

Documentation/devicetree/bindings/regulator/twl-regulator.txt new file +3 -0

+	xyz-regulator: regulator at 0 {
+		compatible = "regulator","ti,twl-reg";
+		ti,reg-id =<37>; /* TWL6030_REG_VAUX1_6030 */

These magic numbers are *very* Linux specific, we should have a better way of specifying regulators - I'd off the top of my head expect that the compatible property would identify the regulator.

The driver seems to use a per-regulator table, and it uses the above id to indexed into it. I could probably do it with the compatible

I know what the driver is doing, the problem is that it's very much specific to Linux (and Linux may change the numbers at some point).

Quoted 3 lines

property, but that would mean I have a compatible for *each* regulator instance, like "ti,twl-reg-vaux1", "ti,twl-reg-vmmc" etc. Does that sound reasonable?

Yes.

Pass regulator_init_data information for omap4sdp from device tree so the regulator driver can then use the regulator helper
routine to extract and use them during the driver probe().

Add documentation for TWL regulator specific bindings.

Signed-off-by Rajendra Nayak <[email protected]>

---
 .../bindings/regulator/twl-regulator.txt           |   18 ++++++++++++++++++
 arch/arm/boot/dts/omap4-sdp.dts                    |   16 ++++++++++++++++
 2 files changed, 34 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/regulator/twl-regulator.txt

Documentation/devicetree/bindings/regulator/twl-regulator.txt new file +0 -0

new file mode 100644
index 0000000..59df44b

Documentation/devicetree/bindings/regulator/twl-regulator.txt new file +10 -0

+
+Example:
+
+	xyz-regulator: regulator at 0 {
+		compatible = "regulator","ti,twl-reg";
+		ti,reg-id = <37>; /* TWL6030_REG_VAUX1_6030 */
+		min-uV  = <1000000>;
+		max-uV  = <3000000>;
+		apply-uV;
+	};

arch/arm/boot/dts/omap4-sdp.dts +16 -0

index 14faf92..2a613e2 100644
@@ -52,6 +52,22 @@
 			interrupts = <11>;
 			reg = <0>;
 		};
+
+		vaux1-regulator: regulator at 0 {
+			compatible = "regulator","ti,twl-reg";
+			ti,reg-id = <37>;  /* TWL6030_REG_VAUX1_6030 */
+			min-uV = <1000000>;
+			max-uV = <3000000>;
+			apply-uV;
+		};
+
+		vusim-regulator: regulator at 1 {
+			compatible = "regulator","ti,twl-reg";
+			ti,reg-id = <42>; /* TWL6030_REG_VUSIM */
+			min-uV = <1200000>;
+			max-uV = <2900000>;
+			apply_uV;
+		};
 	};
 };
 

Signature · 1 line

1.7.1

 .../devicetree/bindings/regulator/regulator.txt    |   37 +++++++++
 drivers/of/Kconfig                                 |    6 ++
 drivers/of/Makefile                                |    1 +
 drivers/of/of_regulator.c                          |   85 ++++++++++++++++++++
 include/linux/of_regulator.h                       |   23 +++++

+Required properties:
+- compatible: Must be "regulator";

+- mode-fast: boolean, Can handle fast changes in its load
+- mode-normal: boolean, Normal regulator power supply mode
+- mode-idle: boolean, Can be more efficient during light loads
+- mode-standby: boolean, Can be most efficient during light loads

+- apply-uV: apply uV constraint if min == max

Quoted 5 lines · Rajendra Nayak

 .../devicetree/bindings/regulator/regulator.txt    |   37 +++++++++
 drivers/of/Kconfig                                 |    6 ++
 drivers/of/Makefile                                |    1 +
 drivers/of/of_regulator.c                          |   85 ++++++++++++++++++++
 include/linux/of_regulator.h                       |   23 +++++

Don't go hiding the bindings for things away from the code they're binding. Bindings for the regualtor API are a regulator API thing and should be part of the regulator API code.

Quoted 2 lines

+Required properties:
+- compatible: Must be "regulator";

Is this idiomatic or should we just have a helper that parses a big list of properties from the device node?

Quoted 4 lines

+- mode-fast: boolean, Can handle fast changes in its load
+- mode-normal: boolean, Normal regulator power supply mode
+- mode-idle: boolean, Can be more efficient during light loads
+- mode-standby: boolean, Can be most efficient during light loads

I guess these are actually permissions to set the given modes? The documentation should be clearer.

Quoted 1 line

+- apply-uV: apply uV constraint if min == max

This seems a bit Linux/runtime policy specific (especially the last bit).
_______________________________________________
devicetree-discuss mailing list
devicetree-discuss at lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Quoted 5 lines · Rajendra Nayak

  .../devicetree/bindings/regulator/regulator.txt    |   37 +++++++++
  drivers/of/Kconfig                                 |    6 ++
  drivers/of/Makefile                                |    1 +
  drivers/of/of_regulator.c                          |   85 ++++++++++++++++++++
  include/linux/of_regulator.h                       |   23 +++++

Don't go hiding the bindings for things away from the code they're binding. Bindings for the regualtor API are a regulator API thing and should be part of the regulator API code.

Quoted 3 lines

+Required properties:
+- compatible: Must be "regulator";

Is this idiomatic or should we just have a helper that parses a big list of properties from the device node?

Quoted 5 lines

+- mode-fast: boolean, Can handle fast changes in its load
+- mode-normal: boolean, Normal regulator power supply mode
+- mode-idle: boolean, Can be more efficient during light loads
+- mode-standby: boolean, Can be most efficient during light loads

I guess these are actually permissions to set the given modes? The documentation should be clearer.

Quoted 2 lines

+- apply-uV: apply uV constraint if min == max

This seems a bit Linux/runtime policy specific (especially the last bit).

Quoted 6 lines · Mark Brown

Quoted 1 line · Rajendra Nayak

+- apply-uV: apply uV constraint if min == max

This seems a bit Linux/runtime policy specific (especially the last bit).

So these bindings should be defined differently?

The helper routine is meant to be used by regulator drivers to extract the regulator_init_data structure passed from device tree. 'consumer_supplies' which is part of regulator_init_data is not extracted as the regulator consumer mappings are passed through DT differently, implemented in subsequent patches.

Also add documentation for regulator bindings to be used to pass regulator_init_data struct information from device tree.

Signed-off-by Rajendra Nayak <[email protected]>

---
 .../devicetree/bindings/regulator/regulator.txt    |   37 +++++++++
 drivers/of/Kconfig                                 |    6 ++
 drivers/of/Makefile                                |    1 +
 drivers/of/of_regulator.c                          |   85 ++++++++++++++++++++
 include/linux/of_regulator.h                       |   23 +++++
 5 files changed, 152 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/regulator/regulator.txt
 create mode 100644 drivers/of/of_regulator.c
 create mode 100644 include/linux/of_regulator.h

Documentation/devicetree/bindings/regulator/regulator.txt new file +0 -0

new file mode 100644
index 0000000..001e5ce

Documentation/devicetree/bindings/regulator/regulator.txt new file +23 -0

+
+Optional properties:
+- supply-regulator: Name of the parent regulator
+- min-uV: smallest voltage consumers may set
+- max-uV: largest voltage consumers may set
+- uV-offset: Offset applied to voltages from consumer to compensate for voltage drops
+- min-uA: smallest current consumers may set
+- max-uA: largest current consumers may set
+- mode-fast: boolean, Can handle fast changes in its load
+- mode-normal: boolean, Normal regulator power supply mode
+- mode-idle: boolean, Can be more efficient during light loads
+- mode-standby: boolean, Can be most efficient during light loads
+- change-voltage: boolean, Output voltage can be changed by software
+- change-current: boolean, Output current can be chnaged by software
+- change-mode: boolean, Operating mode can be changed by software
+- change-status: boolean, Enable/Disable control for regulator exists
+- change-drms: boolean, Dynamic regulator mode switching is enabled
+- input-uV: Input voltage for regulator when supplied by another regulator
+- initial-mode: Mode to set at startup
+- always-on: boolean, regulator should never be disabled
+- boot-on: bootloader/firmware enabled regulator
+- apply-uV: apply uV constraint if min == max
+

drivers/of/Kconfig +6 -0

index b3bfea5..296b96d 100644
@@ -87,4 +87,10 @@ config OF_PCI_IRQ
 	help
 	  OpenFirmware PCI IRQ routing helpers
 
+config OF_REGULATOR
+	def_bool y
+	depends on REGULATOR
+	help
+	  OpenFirmware regulator framework helpers
+
 endmenu # OF

drivers/of/Makefile +1 -0

index 74b959c..bea2852 100644
@@ -12,3 +12,4 @@ obj-$(CONFIG_OF_SPI)	+= of_spi.o
 obj-$(CONFIG_OF_MDIO)	+= of_mdio.o
 obj-$(CONFIG_OF_PCI)	+= of_pci.o
 obj-$(CONFIG_OF_PCI_IRQ)  += of_pci_irq.o
+obj-$(CONFIG_OF_REGULATOR) += of_regulator.o

drivers/of/of_regulator.c new file +85 -0

new file mode 100644
index 0000000..f01d275
@@ -0,0 +1,85 @@
+/*
+ * OF helpers for regulator framework
+ *
+ * Copyright (C) 2011 Texas Instruments, Inc.
+ * Rajendra Nayak <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <linux/slab.h>
+#include <linux/of.h>
+#include <linux/regulator/machine.h>
+
+static void of_get_regulation_constraints(struct device_node *np,
+					struct regulator_init_data **init_data)
+{
+	unsigned int valid_modes_mask = 0, valid_ops_mask = 0;
+
+	of_property_read_u32(np, "min-uV", &(*init_data)->constraints.min_uV);
+	of_property_read_u32(np, "max-uV", &(*init_data)->constraints.max_uV);
+	of_property_read_u32(np, "uV-offset", &(*init_data)->constraints.uV_offset);
+	of_property_read_u32(np, "min-uA", &(*init_data)->constraints.min_uA);
+	of_property_read_u32(np, "max-uA", &(*init_data)->constraints.max_uA);
+
+	/* valid modes mask */
+	if (of_find_property(np, "mode-fast", NULL))
+		valid_modes_mask |= REGULATOR_MODE_FAST;
+	if (of_find_property(np, "mode-normal", NULL))
+		valid_modes_mask |= REGULATOR_MODE_NORMAL;
+	if (of_find_property(np, "mode-idle", NULL))
+		valid_modes_mask |= REGULATOR_MODE_IDLE;
+	if (of_find_property(np, "mode-standby", NULL))
+		valid_modes_mask |= REGULATOR_MODE_STANDBY;
+
+	/* valid ops mask */
+	if (of_find_property(np, "change-voltage", NULL))
+		valid_ops_mask |= REGULATOR_CHANGE_VOLTAGE;
+	if (of_find_property(np, "change-current", NULL))
+		valid_ops_mask |= REGULATOR_CHANGE_CURRENT;
+	if (of_find_property(np, "change-mode", NULL))
+		valid_ops_mask |= REGULATOR_CHANGE_MODE;
+	if (of_find_property(np, "change-status", NULL))
+		valid_ops_mask |= REGULATOR_CHANGE_STATUS;
+
+	(*init_data)->constraints.valid_modes_mask = valid_modes_mask;
+	(*init_data)->constraints.valid_ops_mask = valid_ops_mask;
+
+	of_property_read_u32(np, "input-uV",
+			&(*init_data)->constraints.input_uV);
+	of_property_read_u32(np, "initial-mode",
+			&(*init_data)->constraints.initial_mode);
+
+	if (of_find_property(np, "always-on", NULL))
+			(*init_data)->constraints.always_on = true;
+	if (of_find_property(np, "boot-on", NULL))
+			(*init_data)->constraints.boot_on = true;
+	if (of_find_property(np, "apply-uV", NULL))
+			(*init_data)->constraints.apply_uV = true;
+}
+
+/**
+ * of_get_regulator_init_data - extarct regulator_init_data structure info
+ * @np: Pointer to regulator device tree node
+ *
+ * Populates regulator_init_data structure by extracting data from device
+ * tree node, returns a pointer to the populated struture or NULL if memory
+ * alloc fails.
+ */
+struct regulator_init_data *of_get_regulator_init_data(struct device_node *np)
+{
+	struct regulator_init_data *init_data;
+
+	init_data = kzalloc(sizeof(struct regulator_init_data), GFP_KERNEL);
+	if (!init_data)
+		return NULL; /* Out of memory? */
+
+	init_data->supply_regulator = (char *)of_get_property(np,
+						"supply-regulator", NULL);
+	of_get_regulation_constraints(np, &init_data);
+	return init_data;
+}
+EXPORT_SYMBOL(of_get_regulator_init_data);

include/linux/of_regulator.h new file +23 -0

new file mode 100644
index 0000000..5eb048c
@@ -0,0 +1,23 @@
+/*
+ * OpenFirmware regulator support routines
+ *
+ */
+
+#ifndef __LINUX_OF_REG_H
+#define __LINUX_OF_REG_H
+
+#include <linux/regulator/machine.h>
+
+#if defined(CONFIG_OF_REGULATOR)
+extern struct regulator_init_data
+	*of_get_regulator_init_data(struct device_node *np);
+#else
+static inline struct regulator_init_data
+	*of_get_regulator_init_data(struct device_node *np)
+{
+	return NULL;
+}
+#endif /* CONFIG_OF_REGULATOR */
+
+#endif /* __LINUX_OF_REG_H */
+

The helper routine is meant to be used by regulator drivers to extract the regulator_init_data structure passed from device tree. 'consumer_supplies' which is part of regulator_init_data is not extracted as the regulator consumer mappings are passed through DT differently, implemented in subsequent patches.

Also add documentation for regulator bindings to be used to pass regulator_init_data struct information from device tree.

Signed-off-by Rajendra Nayak <[email protected]>

---
 .../devicetree/bindings/regulator/regulator.txt    |   37 +++++++++
 drivers/of/Kconfig                                 |    6 ++
 drivers/of/Makefile                                |    1 +
 drivers/of/of_regulator.c                          |   85 ++++++++++++++++++++

 include/linux/of_regulator.h                       |   23 +++++
 5 files changed, 152 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/regulator/regulator.txt
 create mode 100644 drivers/of/of_regulator.c
 create mode 100644 include/linux/of_regulator.h

Documentation/devicetree/bindings/regulator/regulator.txt new file +0 -0

new file mode 100644
index 0000000..001e5ce

Documentation/devicetree/bindings/regulator/regulator.txt new file +9 -0

+- min-uV: smallest voltage consumers may set
+- max-uV: largest voltage consumers may set
+- uV-offset: Offset applied to voltages from consumer to compensate for voltage drops
+- min-uA: smallest current consumers may set
+- max-uA: largest current consumers may set
+- mode-fast: boolean, Can handle fast changes in its load
+- mode-normal: boolean, Normal regulator power supply mode
+- mode-idle: boolean, Can be more efficient during light loads
+- mode-standby: boolean, Can be most efficient during light loads

Documentation/devicetree/bindings/regulator/regulator.txt new file +11 -0

+
+Example:
+
+	xyz-regulator: regulator at 0 {
+		compatible = "regulator";
+		min-uV = <1000000>;
+		max-uV = <2500000>;
+		mode-fast;
+		change-voltage;
+		always-on;
+	};

drivers/of/Kconfig +6 -0

index b3bfea5..296b96d 100644
@@ -87,4 +87,10 @@ config OF_PCI_IRQ
 	help
 	  OpenFirmware PCI IRQ routing helpers
 
+config OF_REGULATOR
+	def_bool y
+	depends on REGULATOR
+	help
+	  OpenFirmware regulator framework helpers
+
 endmenu # OF

drivers/of/Makefile +1 -0

index 74b959c..bea2852 100644
@@ -12,3 +12,4 @@ obj-$(CONFIG_OF_SPI)	+= of_spi.o
 obj-$(CONFIG_OF_MDIO)	+= of_mdio.o
 obj-$(CONFIG_OF_PCI)	+= of_pci.o
 obj-$(CONFIG_OF_PCI_IRQ)  += of_pci_irq.o
+obj-$(CONFIG_OF_REGULATOR) += of_regulator.o

drivers/of/of_regulator.c new file +16 -0

new file mode 100644
index 0000000..f01d275
@@ -0,0 +1,85 @@
+/*
+ * OF helpers for regulator framework
+ *
+ * Copyright (C) 2011 Texas Instruments, Inc.
+ * Rajendra Nayak <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <linux/slab.h>
+#include <linux/of.h>
+#include <linux/regulator/machine.h>
+

drivers/of/of_regulator.c new file +49 -0

+
+	/* valid modes mask */
+	if (of_find_property(np, "mode-fast", NULL))
+		valid_modes_mask |= REGULATOR_MODE_FAST;
+	if (of_find_property(np, "mode-normal", NULL))
+		valid_modes_mask |= REGULATOR_MODE_NORMAL;
+	if (of_find_property(np, "mode-idle", NULL))
+		valid_modes_mask |= REGULATOR_MODE_IDLE;
+	if (of_find_property(np, "mode-standby", NULL))
+		valid_modes_mask |= REGULATOR_MODE_STANDBY;
+
+	/* valid ops mask */
+	if (of_find_property(np, "change-voltage", NULL))
+		valid_ops_mask |= REGULATOR_CHANGE_VOLTAGE;
+	if (of_find_property(np, "change-current", NULL))
+		valid_ops_mask |= REGULATOR_CHANGE_CURRENT;
+	if (of_find_property(np, "change-mode", NULL))
+		valid_ops_mask |= REGULATOR_CHANGE_MODE;
+	if (of_find_property(np, "change-status", NULL))
+		valid_ops_mask |= REGULATOR_CHANGE_STATUS;
+
+	(*init_data)->constraints.valid_modes_mask = valid_modes_mask;
+	(*init_data)->constraints.valid_ops_mask = valid_ops_mask;
+
+	of_property_read_u32(np, "input-uV",
+			&(*init_data)->constraints.input_uV);
+	of_property_read_u32(np, "initial-mode",
+			&(*init_data)->constraints.initial_mode);
+
+	if (of_find_property(np, "always-on", NULL))
+			(*init_data)->constraints.always_on = true;
+	if (of_find_property(np, "boot-on", NULL))
+			(*init_data)->constraints.boot_on = true;
+	if (of_find_property(np, "apply-uV", NULL))
+			(*init_data)->constraints.apply_uV = true;
+}
+
+/**
+ * of_get_regulator_init_data - extarct regulator_init_data structure info
+ * @np: Pointer to regulator device tree node
+ *
+ * Populates regulator_init_data structure by extracting data from device
+ * tree node, returns a pointer to the populated struture or NULL if memory
+ * alloc fails.
+ */
+struct regulator_init_data *of_get_regulator_init_data(struct device_node *np)
+{
+	struct regulator_init_data *init_data;
+

include/linux/of_regulator.h new file +23 -0

new file mode 100644
index 0000000..5eb048c
@@ -0,0 +1,23 @@
+/*
+ * OpenFirmware regulator support routines
+ *
+ */
+
+#ifndef __LINUX_OF_REG_H
+#define __LINUX_OF_REG_H
+
+#include <linux/regulator/machine.h>
+
+#if defined(CONFIG_OF_REGULATOR)
+extern struct regulator_init_data
+	*of_get_regulator_init_data(struct device_node *np);
+#else
+static inline struct regulator_init_data
+	*of_get_regulator_init_data(struct device_node *np)
+{
+	return NULL;
+}
+#endif /* CONFIG_OF_REGULATOR */
+
+#endif /* __LINUX_OF_REG_H */
+

Signature · 1 line

1.7.1

Quoted 9 lines · Rajendra Nayak

The helper routine is meant to be used by regulator drivers to extract the regulator_init_data structure passed from device tree. 'consumer_supplies' which is part of regulator_init_data is not extracted as the regulator consumer mappings are passed through DT differently, implemented in subsequent patches.

Also add documentation for regulator bindings to be used to pass regulator_init_data struct information from device tree.

Quoted 6 lines

Signed-off-by Rajendra Nayak<[email protected]>

---
  .../devicetree/bindings/regulator/regulator.txt    |   37 +++++++++
  drivers/of/Kconfig                                 |    6 ++
  drivers/of/Makefile                                |    1 +
  drivers/of/of_regulator.c                          |   85 ++++++++++++++++++++

I originally put the DT stuff under drivers/of for i2c and spi because there was resistance from driver subsystem maintainers to having code for something that was powerpc only. Now that it is no longer the case, I recommend putting this code under drivers/regulator.

Quoted 7 lines

  include/linux/of_regulator.h                       |   23 +++++
  5 files changed, 152 insertions(+), 0 deletions(-)

create mode 100644 Documentation/devicetree/bindings/regulator/regulator.txt create mode 100644 drivers/of/of_regulator.c
create mode 100644 include/linux/of_regulator.h

Documentation/devicetree/bindings/regulator/regulator.txt line 4

Quoted 10 lines

Documentation/devicetree/bindings/regulator/regulator.txt new file +4 -0

new file mode 100644
index 0000000..001e5ce
@@ -0,0 +1,37 @@
+Voltage/Current Regulators
+
+Required properties:
+- compatible: Must be "regulator";

A "regulator" compatible doesn't actually help much. Compatible is for specifying the specific device, not for trying to describe what kind of device it is. Instead, specific regulator bindings can adopt & extend this binding.

Documentation/devicetree/bindings/regulator/regulator.txt line 7

Quoted 4 lines

Documentation/devicetree/bindings/regulator/regulator.txt new file +3 -0

+
+Optional properties:
+- supply-regulator: Name of the parent regulator

If this is a reference to another regulator node then don't use names. Use phandles instead. In which case it should probably be named something like "regulator-parent" (similar to interrupt parent).

However, I can imagine it possible for a regulator to have multiple parents. It may just be better to go with something like the gpio scheme of<phandle gpio-specifier> list of entries. Or maybe just a list of phandles would be sufficient.

Quoted 10 lines

Documentation/devicetree/bindings/regulator/regulator.txt new file +9 -0

+- min-uV: smallest voltage consumers may set
+- max-uV: largest voltage consumers may set
+- uV-offset: Offset applied to voltages from consumer to compensate for voltage drops
+- min-uA: smallest current consumers may set
+- max-uA: largest current consumers may set
+- mode-fast: boolean, Can handle fast changes in its load
+- mode-normal: boolean, Normal regulator power supply mode
+- mode-idle: boolean, Can be more efficient during light loads
+- mode-standby: boolean, Can be most efficient during light loads

Documentation/devicetree/bindings/regulator/regulator.txt line 26

Quoted 10 lines

Documentation/devicetree/bindings/regulator/regulator.txt new file +10 -0

+- change-voltage: boolean, Output voltage can be changed by software
+- change-current: boolean, Output current can be chnaged by software
+- change-mode: boolean, Operating mode can be changed by software
+- change-status: boolean, Enable/Disable control for regulator exists
+- change-drms: boolean, Dynamic regulator mode switching is enabled
+- input-uV: Input voltage for regulator when supplied by another regulator
+- initial-mode: Mode to set at startup
+- always-on: boolean, regulator should never be disabled
+- boot-on: bootloader/firmware enabled regulator
+- apply-uV: apply uV constraint if min == max

To avoid collisions, I'd prefix all of these with a common prefix. Something like regulator-*.

These map 1:1 to how Linux currently implements regulators; which isn't exactly bad, but it means that even if Linux changes, we're still have to support this binding. Does this represent the best layout for high level description of regulators?

Quoted 64 lines

Documentation/devicetree/bindings/regulator/regulator.txt new file +11 -0

+
+Example:
+
+	xyz-regulator: regulator at 0 {
+		compatible = "regulator";
+		min-uV =<1000000>;
+		max-uV =<2500000>;
+		mode-fast;
+		change-voltage;
+		always-on;
+	};

drivers/of/Kconfig +6 -0

index b3bfea5..296b96d 100644
@@ -87,4 +87,10 @@ config OF_PCI_IRQ
  	help
  	  OpenFirmware PCI IRQ routing helpers
 
+config OF_REGULATOR
+	def_bool y
+	depends on REGULATOR
+	help
+	  OpenFirmware regulator framework helpers
+
  endmenu # OF

drivers/of/Makefile +1 -0

index 74b959c..bea2852 100644
@@ -12,3 +12,4 @@ obj-$(CONFIG_OF_SPI)	+= of_spi.o
  obj-$(CONFIG_OF_MDIO)	+= of_mdio.o
  obj-$(CONFIG_OF_PCI)	+= of_pci.o
  obj-$(CONFIG_OF_PCI_IRQ)  += of_pci_irq.o
+obj-$(CONFIG_OF_REGULATOR) += of_regulator.o

drivers/of/of_regulator.c new file +22 -0

new file mode 100644
index 0000000..f01d275
@@ -0,0 +1,85 @@
+/*
+ * OF helpers for regulator framework
+ *
+ * Copyright (C) 2011 Texas Instruments, Inc.
+ * Rajendra Nayak<[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include<linux/slab.h>
+#include<linux/of.h>
+#include<linux/regulator/machine.h>
+
+static void of_get_regulation_constraints(struct device_node *np,
+					struct regulator_init_data **init_data)
+{
+	unsigned int valid_modes_mask = 0, valid_ops_mask = 0;
+
+	of_property_read_u32(np, "min-uV",&(*init_data)->constraints.min_uV);

drivers/of/of_regulator.c line 26

Quoted 4 lines

drivers/of/of_regulator.c new file +4 -0

+	of_property_read_u32(np, "max-uV",&(*init_data)->constraints.max_uV);
+	of_property_read_u32(np, "uV-offset",&(*init_data)->constraints.uV_offset);
+	of_property_read_u32(np, "min-uA",&(*init_data)->constraints.min_uA);
+	of_property_read_u32(np, "max-uA",&(*init_data)->constraints.max_uA);

Are all these structure members are int and unsigned int. They need to be u32 to be used with of_property_read_u32() (which also tells me that the of_property_read_*() api still needs work).

Quoted 56 lines

drivers/of/of_regulator.c new file +55 -0

+
+	/* valid modes mask */
+	if (of_find_property(np, "mode-fast", NULL))
+		valid_modes_mask |= REGULATOR_MODE_FAST;
+	if (of_find_property(np, "mode-normal", NULL))
+		valid_modes_mask |= REGULATOR_MODE_NORMAL;
+	if (of_find_property(np, "mode-idle", NULL))
+		valid_modes_mask |= REGULATOR_MODE_IDLE;
+	if (of_find_property(np, "mode-standby", NULL))
+		valid_modes_mask |= REGULATOR_MODE_STANDBY;
+
+	/* valid ops mask */
+	if (of_find_property(np, "change-voltage", NULL))
+		valid_ops_mask |= REGULATOR_CHANGE_VOLTAGE;
+	if (of_find_property(np, "change-current", NULL))
+		valid_ops_mask |= REGULATOR_CHANGE_CURRENT;
+	if (of_find_property(np, "change-mode", NULL))
+		valid_ops_mask |= REGULATOR_CHANGE_MODE;
+	if (of_find_property(np, "change-status", NULL))
+		valid_ops_mask |= REGULATOR_CHANGE_STATUS;
+
+	(*init_data)->constraints.valid_modes_mask = valid_modes_mask;
+	(*init_data)->constraints.valid_ops_mask = valid_ops_mask;
+
+	of_property_read_u32(np, "input-uV",
+			&(*init_data)->constraints.input_uV);
+	of_property_read_u32(np, "initial-mode",
+			&(*init_data)->constraints.initial_mode);
+
+	if (of_find_property(np, "always-on", NULL))
+			(*init_data)->constraints.always_on = true;
+	if (of_find_property(np, "boot-on", NULL))
+			(*init_data)->constraints.boot_on = true;
+	if (of_find_property(np, "apply-uV", NULL))
+			(*init_data)->constraints.apply_uV = true;
+}
+
+/**
+ * of_get_regulator_init_data - extarct regulator_init_data structure info
+ * @np: Pointer to regulator device tree node
+ *
+ * Populates regulator_init_data structure by extracting data from device
+ * tree node, returns a pointer to the populated struture or NULL if memory
+ * alloc fails.
+ */
+struct regulator_init_data *of_get_regulator_init_data(struct device_node *np)
+{
+	struct regulator_init_data *init_data;
+
+	init_data = kzalloc(sizeof(struct regulator_init_data), GFP_KERNEL);
+	if (!init_data)
+		return NULL; /* Out of memory? */
+
+	init_data->supply_regulator = (char *)of_get_property(np,
+						"supply-regulator", NULL);

drivers/of/of_regulator.c line 85

Quoted 4 lines

drivers/of/of_regulator.c new file +4 -0

+	of_get_regulation_constraints(np,&init_data);
+	return init_data;
+}
+EXPORT_SYMBOL(of_get_regulator_init_data);

Who will call this? If it is done by proper device drivers, it would be better have it do the alloc so that it can do devm_kzalloc(). Or maybe have a devm_kzalloc variant.

Quoted 33 lines

include/linux/of_regulator.h new file +23 -1

new file mode 100644
index 0000000..5eb048c
@@ -0,0 +1,23 @@
+/*
+ * OpenFirmware regulator support routines
+ *
+ */
+
+#ifndef __LINUX_OF_REG_H
+#define __LINUX_OF_REG_H
+
+#include<linux/regulator/machine.h>
+
+#if defined(CONFIG_OF_REGULATOR)
+extern struct regulator_init_data
+	*of_get_regulator_init_data(struct device_node *np);
+#else
+static inline struct regulator_init_data
+	*of_get_regulator_init_data(struct device_node *np)
+{
+	return NULL;
+}
+#endif /* CONFIG_OF_REGULATOR */
+
+#endif /* __LINUX_OF_REG_H */
+
--

1.7.1

Quoted 4 lines · Grant Likely

However, I can imagine it possible for a regulator to have multiple parents. It may just be better to go with something like the gpio scheme of<phandle gpio-specifier> list of entries. Or maybe just a list of phandles would be sufficient.

Ok, I can use phandles instead of the name, and have a list to specify multiple parents.
The linux regulator framework though limits to just one parent I guess.

Quoted 4 lines

These map 1:1 to how Linux currently implements regulators; which isn't exactly bad, but it means that even if Linux changes, we're still have to support this binding. Does this represent the best layout for high level description of regulators?

I guess, except for some like apply-uV, which like Mark pointed out are very linux/runtime policy specific.
Mark, what do you think?

The parameters to set_consumer_device_supply() can be considered invalid (and hence it could return -EINVAL) if nether consumer_dev nor consumer_dev_name are passed, not when *both* are passed.

Signed-off-by Rajendra Nayak <[email protected]>

Quoted 5 lines · Rajendra Nayak

The parameters to set_consumer_device_supply() can be considered invalid (and hence it could return -EINVAL) if nether consumer_dev nor consumer_dev_name are passed, not when *both* are passed.

Signed-off-by Rajendra Nayak<[email protected]>

No, passing both is a clear bug on the part of the caller - the two ways of specifying the device are not supposed to be used together.

The TWL driver seems to set the default .valid_modes_mask to (REGULATOR_MODE_NORMAL | REGULATOR_MODE_STANDBY) and .valid_ops_mask to (REGULATOR_CHANGE_VOLTAGE | REGULATOR_CHANGE_MODE | REGULATOR_CHANGE_STATUS) for all the registered regulators.

There is no need for all the board files to pass this information additionally, when the driver already sets them by default.

Quoted 7 lines · Rajendra Nayak

The TWL driver seems to set the default .valid_modes_mask to (REGULATOR_MODE_NORMAL | REGULATOR_MODE_STANDBY) and .valid_ops_mask to (REGULATOR_CHANGE_VOLTAGE | REGULATOR_CHANGE_MODE | REGULATOR_CHANGE_STATUS) for all the registered regulators.

There is no need for all the board files to pass this information additionally, when the driver already sets them by default.

"the driver"? This sounds very buggy...

Quoted 6 lines · Mark Brown

Quoted 2 lines · Rajendra Nayak

There is no need for all the board files to pass this information additionally, when the driver already sets them by default.

"the driver"? This sounds very buggy...

Yeah, the driver currently sets some default based on what the chip itself supports and completely ignores the board inputs.
How about the driver using the defaults and letting the boards override it (if it ever wants to)?
Currently none of the boards (using any of the twl regulators) seem to override the defaults and that way a lot of this duplication in board files could be avoided.

Quoted 8 lines · Rajendra Nayak

Quoted 6 lines · Mark Brown

Quoted 2 lines · Rajendra Nayak

There is no need for all the board files to pass this information additionally, when the driver already sets them by default.

"the driver"? This sounds very buggy...

Quoted 8 lines

Yeah, the driver currently sets some default based on what the chip itself supports and completely ignores the board inputs.
How about the driver using the defaults and letting the boards override it (if it ever wants to)?
Currently none of the boards (using any of the twl regulators) seem to override the defaults and that way a lot of this duplication in board files could be avoided.

No, this is just plain buggy. It's fine for OMAP to have some common stuff that the board reference but if the drivers are doing this that's bad. The drivers and framework shouldn't touch the hardware unless the board says it's OK.