Device tree support for regulators

Remove the hardcoded .valid_modes_mask and .valid_ops_mask for each regulator from the twl driver and let the boards pass it.

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

Quoted 6 lines · Rajendra Nayak

Remove the hardcoded .valid_modes_mask and .valid_ops_mask for each regulator from the twl driver and let the boards pass it.

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

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

Quoted 8 lines

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

-	/* Constrain board-specific capabilities according to what
-	 * this driver and the chip itself can actually do.
-	 */
-	c =&initdata->constraints;
-	c->valid_modes_mask&= REGULATOR_MODE_NORMAL | REGULATOR_MODE_STANDBY;
-	c->valid_ops_mask&= REGULATOR_CHANGE_VOLTAGE
-				| REGULATOR_CHANGE_MODE
-				| REGULATOR_CHANGE_STATUS;

This isn't actually hard coding constraints, this is restricting the constraints passed in further rather than adding new ones.

However should be fine:

Acked-by Mark Brown<[email protected]>

include/linux/regulator/machine.h +1 -2

 	/* voltage output range (inclusive) - for voltage control */
-	int min_uV;
-	int max_uV;
+	u32 min_uV;

drivers/regulator/of_regulator.c line 82

Quoted 4 lines · Rajendra Nayak

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

+	init_data = devm_kzalloc(dev, sizeof(struct regulator_init_data),
+						 GFP_KERNEL);
+	if (!init_data)
+		return NULL; /* Out of memory? */

This means that the init data will be kept around for the entire lifetime of the device rather than being discarded.

drivers/regulator/of_regulator.c line 85

Quoted 3 lines

drivers/regulator/of_regulator.c new file +2 -0

+	init_data->supply_regulator = (char *)of_get_property(dev->of_node,
+						"regulator-supplies", NULL);

I'd expect that in the device tree world the supply regulator would reference the node for that regulator.

Quoted 10 lines

/* voltage output range (inclusive) - for voltage control */

include/linux/regulator/machine.h +3 -3

-	int min_uV;
-	int max_uV;
+	u32 min_uV;
+	u32 max_uV;
 
-	int uV_offset;
+	u32 uV_offset;

include/linux/regulator/machine.h struct regulation_constraints { · line 108

Quoted 5 lines

/* current output range (inclusive) - for current control */

include/linux/regulator/machine.h +2 -2

-	int min_uA;
-	int max_uA;
+	u32 min_uA;
+	u32 max_uA;

Hrm, I think loosing the signs here is bad karma - negative voltages do exist after all.

Quoted 9 lines · Mark Brown

drivers/regulator/of_regulator.c line 82

Quoted 4 lines · Rajendra Nayak

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

+	init_data = devm_kzalloc(dev, sizeof(struct regulator_init_data),
+						 GFP_KERNEL);
+	if (!init_data)
+		return NULL; /* Out of memory? */

This means that the init data will be kept around for the entire lifetime of the device rather than being discarded.

Wasn't it the same while this was passed around as platform_data?

Quoted 5 lines

drivers/regulator/of_regulator.c line 85

Quoted 2 lines

drivers/regulator/of_regulator.c new file +2 -0

+	init_data->supply_regulator = (char *)of_get_property(dev->of_node,
+						"regulator-supplies", NULL);

I'd expect that in the device tree world the supply regulator would reference the node for that regulator.

You mean using phandles? Thats what Grant proposed too but I thought you instead had an inclination towards names? Or maybe I misunderstood.

Quoted 2 lines

Hrm, I think loosing the signs here is bad karma - negative voltages do exist after all.

Oops.. they do? didn't know about that.

Quoted 11 lines · Rajendra Nayak

Quoted 9 lines · Mark Brown

drivers/regulator/of_regulator.c line 82

Quoted 4 lines · Rajendra Nayak

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

+	init_data = devm_kzalloc(dev, sizeof(struct regulator_init_data),
+						 GFP_KERNEL);
+	if (!init_data)
+		return NULL; /* Out of memory? */

This means that the init data will be kept around for the entire lifetime of the device rather than being discarded.

Quoted 1 line

Wasn't it the same while this was passed around as platform_data?

It was in the past but I remember fixing it at some point. Perhaps I'm imagining things.

Quoted 9 lines

Quoted 5 lines

drivers/regulator/of_regulator.c line 85

Quoted 2 lines

drivers/regulator/of_regulator.c new file +2 -0

+	init_data->supply_regulator = (char *)of_get_property(dev->of_node,
+						"regulator-supplies", NULL);

I'd expect that in the device tree world the supply regulator would reference the node for that regulator.

You mean using phandles? Thats what Grant proposed too but I thought you instead had an inclination towards names? Or maybe I misunderstood.

They need both. We need to reference the device that provides the supply and use a name to say which of the potentially multiple supplies on the consumer device is which.

Quoted 4 lines

Quoted 2 lines

Hrm, I think loosing the signs here is bad karma - negative voltages do exist after all.

Oops.. they do? didn't know about that.

Yup, ground is just a reference point.

Quoted 6 lines

Quoted 6 lines

drivers/regulator/of_regulator.c line 85

Quoted 3 lines

drivers/regulator/of_regulator.c new file +2 -0

+	init_data->supply_regulator = (char *)of_get_property(dev->of_node,
+						"regulator-supplies", NULL);

I'd expect that in the device tree world the supply regulator would reference the node for that regulator.

Quoted 3 lines

You mean using phandles? Thats what Grant proposed too but I thought you instead had an inclination towards names? Or maybe I misunderstood.

They need both. We need to reference the device that provides the supply and use a name to say which of the potentially multiple supplies on the consumer device is which.

Quoted 5 lines

Quoted 3 lines

Hrm, I think loosing the signs here is bad karma - negative voltages do exist after all.

Oops.. they do? didn't know about that.

Yup, ground is just a reference point.

Quoted 3 lines

They need both. We need to reference the device that provides the supply and use a name to say which of the potentially multiple supplies on the consumer device is which.

Mark, I still seem to be a little confused with this one as to why we would need a phandle *and* a supply-name to reference a parent regulator/supply.
The phandle would point to a regulator dt node and that node internally would have just one name associated with it.

I think as well that we should avoid considering a regulator with several outputs. I saw the same pattern used for the clock binding in DT as well.

Mark, I still seem to be a little confused with this one as to why we would need a phandle *and* a supply-name to reference a parent regulator/supply.
The phandle would point to a regulator dt node and that node internally would have just one name associated with it.

Quoted 8 lines · Rajendra Nayak

Quoted 2 lines

drivers/regulator/of_regulator.c line 85

Quoted 2 lines

drivers/regulator/of_regulator.c new file +2 -0

+  init_data->supply_regulator = (char *)of_get_property(dev->of_node,
+                                          "regulator-supplies", NULL);

Mark, I still seem to be a little confused with this one as to why we would need a phandle *and* a supply-name to reference a parent regulator/supply.
The phandle would point to a regulator dt node and that node internally would have just one name associated with it.

To repeat: the supply name is for the consumer. It is needed so that the consumer can tell which supply is provided by which regulator. Almost all devices have more than one supply and if the device does anything more complicated than just turning on all the supplies when the device is active it's going to need to figure out which supply is which.

Quoted 11 lines · Mark Brown

Quoted 8 lines · Rajendra Nayak

Quoted 2 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+  init_data->supply_regulator = (char *)of_get_property(dev->of_node,
+                                          "regulator-supplies", NULL);

Mark, I still seem to be a little confused with this one as to why we would need a phandle *and* a supply-name to reference a parent regulator/supply.
The phandle would point to a regulator dt node and that node internally would have just one name associated with it.

Quoted 5 lines

To repeat: the supply name is for the consumer. It is needed so that the consumer can tell which supply is provided by which regulator. Almost all devices have more than one supply and if the device does anything more complicated than just turning on all the supplies when the device is active it's going to need to figure out which supply is which.

Hang on, I now have no idea what this is supposed to be doing. Later on in the series you had examples in your commit logs with perfectly sensible bindings for supplies:

                 vmmc-supply =<&regulator1>;
                 vpll-supply =<&regulator1>;

which have both a unique name and a direct reference to the supplying regulator. What are these "regulator-supplies" properties supposed to be?

Quoted 22 lines · Mark Brown

Quoted 11 lines · Mark Brown

Quoted 8 lines · Rajendra Nayak

Quoted 2 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+  init_data->supply_regulator = (char *)of_get_property(dev->of_node,
+                                          "regulator-supplies", NULL);

Mark, I still seem to be a little confused with this one as to why we would need a phandle *and* a supply-name to reference a parent regulator/supply.
The phandle would point to a regulator dt node and that node internally would have just one name associated with it.

Quoted 5 lines

To repeat: the supply name is for the consumer. It is needed so that the consumer can tell which supply is provided by which regulator. Almost all devices have more than one supply and if the device does anything more complicated than just turning on all the supplies when the device is active it's going to need to figure out which supply is which.

Hang on, I now have no idea what this is supposed to be doing. Later on in the series you had examples in your commit logs with perfectly sensible bindings for supplies:

Quoted 6 lines

                  vmmc-supply =<&regulator1>;
                  vpll-supply =<&regulator1>;

which have both a unique name and a direct reference to the supplying regulator. What are these "regulator-supplies" properties supposed to be?

:-), yes, I was confused for a while as well after reading your response.

The "regulator-supplies" is used to specific the regulator *parent*. Same as what was earlier passed by using the "supply_regulator" field of regulator_init_data structure. Grant wanted the bindings to support specifying multiple parents and hence I was thinking of either a list of names *or*
a list of phandles to specify multiple parents to a regulator.

The "regulator-supplies" is used to specific the regulator *parent*. Same as what was earlier passed by using the "supply_regulator" field of regulator_init_data structure. Grant wanted the bindings to support specifying multiple parents and hence I was thinking of either a list of names *or*
a list of phandles to specify multiple parents to a regulator.

Quoted 6 lines · Rajendra Nayak

The "regulator-supplies" is used to specific the regulator *parent*. Same as what was earlier passed by using the "supply_regulator" field of regulator_init_data structure. Grant wanted the bindings to support specifying multiple parents and hence I was thinking of either a list of names *or*
a list of phandles to specify multiple parents to a regulator.

So, as I'm fairly sure I said last time these are just standard supplies. It just happens to be that the consumer is a regulator. The fact that Linux chooses to have core framework handling for this is an implementation detail of Linux (and indeed many devices ignore this for their on board regulators).

The problem is I don't know if the property in the regulator dt node is called "vin-supply" or "vxyz-supply" and hence I can parse the property based on a substring alone, which is "-supply" because all I know is the property name is expected to end with a "-supply".

Quoted 5 lines · Rajendra Nayak

The problem is I don't know if the property in the regulator dt node is called "vin-supply" or "vxyz-supply" and hence I can parse the property based on a substring alone, which is "-supply" because all I know is the property name is expected to end with a "-supply".

This seems fairly straightforward though it will require some changes within Linux, all we have to do is have the regulators name their supplies.

Quoted 3 lines · Mark Brown

This seems fairly straightforward though it will require some changes within Linux, all we have to do is have the regulators name their supplies.

through dt? thats what the original bindings was doing.

Quoted 7 lines · Rajendra Nayak

Quoted 3 lines · Mark Brown

This seems fairly straightforward though it will require some changes within Linux, all we have to do is have the regulators name their supplies.

through dt? thats what the original bindings was doing.

I wouldn't expect it done through DT (except for things like the fixed voltage regulator) - the driver should just know.

Quoted 2 lines · Mark Brown

I wouldn't expect it done through DT (except for things like the fixed voltage regulator) - the driver should just know.

something like a
int regulator_set_supply(struct regulator_dev *rdev, char *supply_name); to be called from drivers makes sense?

Quoted 4 lines · Rajendra Nayak

Quoted 2 lines · Mark Brown

I wouldn't expect it done through DT (except for things like the fixed voltage regulator) - the driver should just know.

Quoted 3 lines

something like a
int regulator_set_supply(struct regulator_dev *rdev, char *supply_name); to be called from drivers makes sense?

Just put a member in the driver struct, much simpler that way, no code in the individual drivers.

drivers/regulator/of_regulator.c line 82

Quoted 4 lines · Rajendra Nayak

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

+	init_data = devm_kzalloc(dev, sizeof(struct regulator_init_data),
+						 GFP_KERNEL);
+	if (!init_data)
+		return NULL; /* Out of memory? */

This means that the init data will be kept around for the entire lifetime of the device rather than being discarded.

drivers/regulator/of_regulator.c line 85

Quoted 2 lines

drivers/regulator/of_regulator.c new file +2 -0

+	init_data->supply_regulator = (char *)of_get_property(dev->of_node,
+						"regulator-supplies", NULL);

I'd expect that in the device tree world the supply regulator would reference the node for that regulator.

drivers/regulator/of_regulator.c line 82

Quoted 4 lines · Rajendra Nayak

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

+	init_data = devm_kzalloc(dev, sizeof(struct regulator_init_data),
+						 GFP_KERNEL);
+	if (!init_data)
+		return NULL; /* Out of memory? */

This means that the init data will be kept around for the entire lifetime of the device rather than being discarded.

Quoted 9 lines · Mark Brown

drivers/regulator/of_regulator.c line 82

Quoted 4 lines · Rajendra Nayak

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

+	init_data = devm_kzalloc(dev, sizeof(struct regulator_init_data),
+						 GFP_KERNEL);
+	if (!init_data)
+		return NULL; /* Out of memory? */

This means that the init data will be kept around for the entire lifetime of the device rather than being discarded.

May I remind you that devm_* lifetime expires whenever the associated driver is unbound, which can be much shorter than the lifetime of the struct device.

It expires when any of the following occurs:
1. userspace asks the associated driver to be unbound
2. the driver is removed
3. any driver probe for this struct device fails
4. the struct device is unregistered.

So: don't use devm_* for anything other than stuff inside a driver being associated with the struct device itself. Other uses are a bug waiting to happen.

drivers/regulator/twl-regulator.c struct twlreg_info { · line 65

Quoted 3 lines · Rajendra Nayak

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

+#ifdef CONFIG_OF
+	char			compatible[128];
+#endif

Might it not be better to just make this a pointer to const char?

The helper routine of_get_fixed_voltage_config() extracts fixed_voltage_config structure contents from device tree.

Also add documenation for additional bindings for fixed regulators that can be passed through dt.

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

+- regulator-fixed-enable-high: Polarity of enable GPIO, 1 = Active High, 0 = Active low

Quoted 5 lines · Rajendra Nayak

The helper routine of_get_fixed_voltage_config() extracts fixed_voltage_config structure contents from device tree.

Also add documenation for additional bindings for fixed regulators that can be passed through dt.

Why not just add device tree support into the driver directly?

Quoted 3 lines

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

This is going to be confusing with respect to the generic regulator supply property.

Quoted 2 lines

+- regulator-fixed-enable-high: Polarity of enable GPIO, 1 = Active High, 0 = Active low

Word wrap for legibility please.

Quoted 7 lines · Mark Brown

Quoted 2 lines · Rajendra Nayak

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

This is going to be confusing with respect to the generic regulator supply property.

Yes, I guess. Any suggestions on how to make it less confusing? :-)

The helper routine of_get_fixed_voltage_config() extracts fixed_voltage_config structure contents from device tree.

Also add documenation for additional bindings for fixed regulators that can be passed through dt.

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

---
 .../bindings/regulator/fixed-regulator.txt         |   24 +++++++++++++
 drivers/regulator/fixed.c                          |   36 ++++++++++++++++++++
 include/linux/regulator/fixed.h                    |    6 ++--
 3 files changed, 63 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/regulator/fixed-regulator.txt

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

new file mode 100644
index 0000000..a204cbd
@@ -0,0 +1,24 @@
+Fixed Voltage regulators
+
+Required properties:
+- compatible: Must be "regulator-fixed";
+
+Optional properties:
+- regulator-fixed-supply: Name of the regulator supply
+- regulator-fixed-microvolts: Output voltage of regulator
+- regulator-fixed-gpio: gpio to use for enable control
+- regulator-fixed-startup-delay: startup time in microseconds
+- regulator-fixed-enable-high: Polarity of enable GPIO, 1 = Active High, 0 = Active low
+- regulator-fixed-enabled-at-boot: 1 = yes, 0 = no
+
+Example:
+
+	abc: fixedregulator at 0 {
+		compatible = "regulator-fixed";
+		regulator-fixed-supply = "fixed-supply";
+		regulator-fixed-microvolts = <1800000>;
+		regulator-fixed-gpio = <43>;
+		regulator-fixed-startup-delay = <70000>;
+		regulator-fixed-enable-high;
+		regulator-fixed-enabled-at-boot;
+	};

drivers/regulator/fixed.c +7 -0

index 2fe9d99..c09f791 100644
@@ -26,6 +26,8 @@
 #include <linux/gpio.h>
 #include <linux/delay.h>
 #include <linux/slab.h>
+#include <linux/of.h>
+#include <linux/regulator/of_regulator.h>
 
 struct fixed_voltage_data {
 	struct regulator_desc desc;

@@ -37,6 +39,40 @@ struct fixed_voltage_data {
 	bool is_enabled;
 };
 
+
+/**
+ * of_get_fixed_voltage_config - extract fixed_voltage_config structure info
+ * @dev: device requesting for fixed_voltage_config
+ *

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)

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

---
 drivers/regulator/fixed.c |   18 +++++++++++++++++-
 1 files changed, 17 insertions(+), 1 deletions(-)

drivers/regulator/fixed.c +0 -0

index c09f791..0a9a10d 100644

drivers/regulator/fixed.c +16 -0

 	struct fixed_voltage_data *drvdata;
 	int ret;
 
+	if (pdev->dev.of_node)
+		config = of_get_fixed_voltage_config(&pdev->dev);
+	else
+		config = pdev->dev.platform_data;
+
 	drvdata = kzalloc(sizeof(struct fixed_voltage_data), GFP_KERNEL);
 	if (drvdata == NULL) {
 		dev_err(&pdev->dev, "Failed to allocate device data\n");

@@ -252,12 +257,23 @@ static int __devexit reg_fixed_voltage_remove(struct platform_device *pdev)
 	return 0;
 }
 
+#if defined(CONFIG_OF)
+static const struct of_device_id fixed_of_match[] __devinitconst = {
+	{ .compatible = "regulator-fixed", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, fixed_of_match);
+#else
+#define fixed_of_match NULL
+#endif
+
 static struct platform_driver regulator_fixed_voltage_driver = {
 	.probe		= reg_fixed_voltage_probe,
 	.remove		= __devexit_p(reg_fixed_voltage_remove),
 	.driver		= {
 		.name		= "reg-fixed-voltage",
 		.owner		= THIS_MODULE,
+		.of_match_table = fixed_of_match,
 
 
 	},

};

Signature · 1 line

1.7.1

Review

Quoted 2 lines · Rajendra Nayak

(quoted diff) +2 -0

+	if (!dev)
+		return NULL;

So how do we handle CPUs? cpufreq is one of the most active users of regulators...

drivers/regulator/of_regulator.c line 44

Quoted 6 lines

drivers/regulator/of_regulator.c +5 -0

+	snprintf(prop_name, 32, "%s-supply", supply);
+
+	prop = of_get_property(dev->of_node, prop_name,&sz);
+	if (!prop || sz<  4)
+		return NULL;

sz< 4? Magic! :)

include/linux/regulator/of_regulator.h line 13

Quoted 3 lines

include/linux/regulator/of_regulator.h +2 -0

+extern struct device_node *of_get_regulator(struct device *dev,
+	const char *supply);

This shouldn't be part of the public API, it should be transparently handled within the core.

Quoted 7 lines · Mark Brown

Review

Quoted 2 lines · Rajendra Nayak

(quoted diff) +2 -0

+	if (!dev)
+		return NULL;

So how do we handle CPUs? cpufreq is one of the most active users of regulators...

Hmm, never thought of it :(
Maybe I should associate a supply name with all regulators and then lookup from the global registered list.

Quoted 7 lines

drivers/regulator/of_regulator.c line 44

Quoted 5 lines

drivers/regulator/of_regulator.c +5 -0

+	snprintf(prop_name, 32, "%s-supply", supply);
+
+	prop = of_get_property(dev->of_node, prop_name,&sz);
+	if (!prop || sz<  4)
+		return NULL;

sz< 4? Magic! :)

Its the valid phandle size.
I guess I need a sz != 4

Quoted 9 lines · Rajendra Nayak

Quoted 7 lines · Mark Brown

Review

Quoted 2 lines · Rajendra Nayak

(quoted diff) +2 -0

+	if (!dev)
+		return NULL;

So how do we handle CPUs? cpufreq is one of the most active users of regulators...

Quoted 4 lines

Hmm, never thought of it :(
Maybe I should associate a supply name with all regulators and then lookup from the global registered list.

I'm not sure how this should work in a device tree world, I'd *hope* we'd get a device tree node for the CPU and could then just make this a regular consumer thing but then the cpufreq drivers would need to be updated to make use of it. The only reason we allow null devices right now is the fact that cpufreq doesn't have a struct device it can use.

Quoted 15 lines · Mark Brown

Quoted 6 lines · Rajendra Nayak

Quoted 4 lines · Mark Brown

Quoted 2 lines · Rajendra Nayak

+ if (!dev)
+ return NULL;

Quoted 7 lines

Quoted 2 lines

So how do we handle CPUs? cpufreq is one of the most active users of regulators...

Hmm, never thought of it :(
Maybe I should associate a supply name with all regulators and then lookup from the global registered list.

Quoted 5 lines

I'm not sure how this should work in a device tree world, I'd *hope* we'd get a device tree node for the CPU and could then just make this a regular consumer thing but then the cpufreq drivers would need to be updated to make use of it. The only reason we allow null devices right now is the fact that cpufreq doesn't have a struct device it can use.

That's why we do have a MPU node in OMAP dts, in order to build an omap_device that will be mainly used for the DVFS on the MPU.

And even before DT migration, we used to build statically some omap_device to represent the various processors in the system (MPU, DSP, CortexM3...).

Regards,
Benoit

Quoted 5 lines · Mark Brown

I'm not sure how this should work in a device tree world, I'd *hope* we'd get a device tree node for the CPU and could then just make this a regular consumer thing but then the cpufreq drivers would need to be updated to make use of it. The only reason we allow null devices right now is the fact that cpufreq doesn't have a struct device it can use.

That's why we do have a MPU node in OMAP dts, in order to build an omap_device that will be mainly used for the DVFS on the MPU.

And even before DT migration, we used to build statically some omap_device to represent the various processors in the system (MPU, DSP, CortexM3...).

Quoted 10 lines · Benoit

Quoted 5 lines · Mark Brown

I'm not sure how this should work in a device tree world, I'd *hope* we'd get a device tree node for the CPU and could then just make this a regular consumer thing but then the cpufreq drivers would need to be updated to make use of it. The only reason we allow null devices right now is the fact that cpufreq doesn't have a struct device it can use.

That's why we do have a MPU node in OMAP dts, in order to build an omap_device that will be mainly used for the DVFS on the MPU.

Quoted 3 lines

And even before DT migration, we used to build statically some omap_device to represent the various processors in the system (MPU, DSP, CortexM3...).

Yeah, but that's very OMAP specific - we don't have that in general (in fact it's the only Linux platform I'm aware of that has a device for the CPU).

Quoted 5 lines · Mark Brown

Quoted 3 lines · Benoit

And even before DT migration, we used to build statically some omap_device to represent the various processors in the system (MPU, DSP, CortexM3...).

Quoted 3 lines

Yeah, but that's very OMAP specific - we don't have that in general (in fact it's the only Linux platform I'm aware of that has a device for the CPU).

But isn't this the right thing to do for everyone else too?

Quoted 16 lines · Mark Brown

Quoted 14 lines · Benoit

Quoted 5 lines · Mark Brown

I'm not sure how this should work in a device tree world, I'd *hope* we'd get a device tree node for the CPU and could then just make this a regular consumer thing but then the cpufreq drivers would need to be updated to make use of it. The only reason we allow null devices right now is the fact that cpufreq doesn't have a struct device it can use.

That's why we do have a MPU node in OMAP dts, in order to build an omap_device that will be mainly used for the DVFS on the MPU.

And even before DT migration, we used to build statically some omap_device to represent the various processors in the system (MPU, DSP, CortexM3...).

Quoted 3 lines

Yeah, but that's very OMAP specific - we don't have that in general (in fact it's the only Linux platform I'm aware of that has a device for the CPU).

But isn't this the right thing to do for everyone else too?

Quoted 9 lines · Rajendra Nayak

Quoted 7 lines · Mark Brown

Review

Quoted 2 lines · Rajendra Nayak

(quoted diff) +2 -0

+	if (!dev)
+		return NULL;

So how do we handle CPUs? cpufreq is one of the most active users of regulators...

Quoted 4 lines

Hmm, never thought of it :(
Maybe I should associate a supply name with all regulators and then lookup from the global registered list.

I'm not sure how this should work in a device tree world, I'd *hope* we'd get a device tree node for the CPU and could then just make this a regular consumer thing but then the cpufreq drivers would need to be updated to make use of it. The only reason we allow null devices right now is the fact that cpufreq doesn't have a struct device it can use.

Quoted 7 lines

Quoted 7 lines

drivers/regulator/of_regulator.c line 44

Quoted 5 lines

drivers/regulator/of_regulator.c +5 -0

+	snprintf(prop_name, 32, "%s-supply", supply);
+
+	prop = of_get_property(dev->of_node, prop_name,&sz);
+	if (!prop || sz<   4)
+		return NULL;

sz< 4? Magic! :)

Quoted 2 lines

Its the valid phandle size.
I guess I need a sz != 4

I think we need an of_get_phandle(), it'd be clearer what the check is, more type safe and would avoid needing to replicate the check.

Device nodes in DT can associate themselves with one or more regulators by providing a list of phandles (to regulator nodes) and corresponding supply names.

For Example:

	devicenode: node at 0x0 {
		...
		...
		vmmc-supply = <&regulator1>;
		vpll-supply = <&regulator1>;
	};

The driver would then do a regulator_get(dev, "vmmc"); to get regulator1 and do a regulator_get(dev, "vpll"); to get regulator2.

of_get_regulator() extracts the regulator node for a given device, based on the supply name.

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

---
 drivers/regulator/of_regulator.c       |   39 ++++++++++++++++++++++++++++++++
 include/linux/regulator/of_regulator.h |    7 +++++
 2 files changed, 46 insertions(+), 0 deletions(-)

drivers/regulator/of_regulator.c +21 -0

index 7fa63ff..49dd105 100644
@@ -14,6 +14,45 @@
 #include <linux/of.h>
 #include <linux/regulator/machine.h>
 
+
+/**
+ * of_get_regulator - get a regulator device node based on supply name
+ * @dev: Device pointer for the consumer (of regulator) device
+ * @supply: regulator supply name
+ *
+ * Extract the regulator device node corresponding to the supply name.
+ * retruns the device node corresponding to the regulator if found, else
+ * returns NULL.
+ */
+struct device_node *of_get_regulator(struct device *dev, const char *supply)
+{
+	struct device_node *regnode = NULL;
+	u32 reghandle;
+	char prop_name[32]; /* 32 is max size of property name */
+	const void *prop;
+	int sz;
+
+	if (!dev)
+		return NULL;
+

drivers/regulator/of_regulator.c +8 -0

+	if (!regnode) {
+		pr_warn("%s: %s property in node %s references invalid phandle",
+			__func__, prop_name, dev->of_node->full_name);
+		return NULL;
+	}
+	return regnode;
+}
+
 static void of_get_regulation_constraints(struct device_node *np,
 					struct regulator_init_data **init_data)
 {

include/linux/regulator/of_regulator.h +7 -0

index 3f63be9..edaba1a 100644
@@ -9,12 +9,19 @@
 #if defined(CONFIG_OF_REGULATOR)
 extern struct regulator_init_data
 	*of_get_regulator_init_data(struct device *dev);
+extern struct device_node *of_get_regulator(struct device *dev,
+	const char *supply);
 #else
 static inline struct regulator_init_data
 	*of_get_regulator_init_data(struct device_node *np)
 {
 	return NULL;
 }
+static inline struct device_node *of_get_regulator(struct device *dev,
+	const char *id)
+{
+	return NULL;
+}
 #endif /* CONFIG_OF_REGULATOR */
 
 #endif /* __LINUX_OF_REG_H */

Signature · 1 line

1.7.1

Look up the regulator for a given consumer from device tree, during a regulator_get(). If not found fallback and lookup through the regulator_map_list instead.

Quoted 3 lines · Rajendra Nayak

Look up the regulator for a given consumer from device tree, during a regulator_get(). If not found fallback and lookup through the regulator_map_list instead.

As with the fixed voltage regulator patch just use the code along with adding it, no need to split it just makes it harder to review.

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

Quoted 10 lines

drivers/regulator/core.c +9 -0

+	if (dev->of_node) {
+		node = of_get_regulator(dev, id);
+		if (!node)
+			goto retry; /* fallback and chk regulator_map_list */
+		list_for_each_entry(rdev,&regulator_list, list)
+			if (node == rdev->node)
+				goto found;
+	}
+retry:

retry is a confusing name for the target, we don't ever actually retry using it. Given the simplicity of the code I'd be inclined to just intert the if (!node) check.

Look up the regulator for a given consumer from device tree, during a regulator_get(). If not found fallback and lookup through the regulator_map_list instead.

Devices can associate with one or more regulators by providing a list of phandles and supply names.

For Example:

        devicenode: node at 0x0 {
                ...
                ...
                vmmc-supply = <&regulator1>;
                vpll-supply = <&regulator2>;
        };

When a device driver calls a regulator_get, specifying the supply name, the phandle and eventually the regulator node is extracted from the device node.

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

---
 drivers/regulator/core.c         |   14 ++++++++++++++
 include/linux/regulator/driver.h |    3 +++
 2 files changed, 17 insertions(+), 0 deletions(-)

drivers/regulator/core.c +14 -0

index d8e6a42..47b851c 100644
@@ -25,9 +25,11 @@
 #include <linux/mutex.h>
 #include <linux/suspend.h>
 #include <linux/delay.h>
+#include <linux/of.h>
 #include <linux/regulator/consumer.h>
 #include <linux/regulator/driver.h>
 #include <linux/regulator/machine.h>
+#include <linux/regulator/of_regulator.h>
 
 #define CREATE_TRACE_POINTS
 #include <trace/events/regulator.h>

@@ -1155,6 +1157,7 @@ static struct regulator *_regulator_get(struct device *dev, const char *id,
 	struct regulator_map *map;
 	struct regulator *regulator = ERR_PTR(-ENODEV);
 	const char *devname = NULL;
+	struct device_node *node;
 	int ret;
 
 	if (id == NULL) {

@@ -1167,6 +1170,15 @@ static struct regulator *_regulator_get(struct device *dev, const char *id,
 
 	mutex_lock(&regulator_list_mutex);
 
+	if (dev->of_node) {
+		node = of_get_regulator(dev, id);
+		if (!node)
+			goto retry; /* fallback and chk regulator_map_list */
+		list_for_each_entry(rdev, &regulator_list, list)
+			if (node == rdev->node)
+				goto found;
+	}
+retry:
 	list_for_each_entry(map, &regulator_map_list, list) {
 		/* If the mapping has a device set up it must match */
 		if (map->dev_name &&

@@ -2619,6 +2631,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;
 	INIT_LIST_HEAD(&rdev->consumer_list);
 	INIT_LIST_HEAD(&rdev->list);
 	BLOCKING_INIT_NOTIFIER_HEAD(&rdev->notifier);

include/linux/regulator/driver.h +0 -0

index 1a80bc7..4aebbf5 100644

Quoted 80 lines · Rajendra Nayak

Look up the regulator for a given consumer from device tree, during a regulator_get(). If not found fallback and lookup through the regulator_map_list instead.

Devices can associate with one or more regulators by providing a list of phandles and supply names.

For Example:

         devicenode: node at 0x0 {
                 ...
                 ...
                 vmmc-supply =<&regulator1>;
                 vpll-supply =<&regulator2>;
         };

When a device driver calls a regulator_get, specifying the supply name, the phandle and eventually the regulator node is extracted from the device node.

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

---
  drivers/regulator/core.c         |   14 ++++++++++++++
  include/linux/regulator/driver.h |    3 +++
  2 files changed, 17 insertions(+), 0 deletions(-)

drivers/regulator/core.c +14 -0

index d8e6a42..47b851c 100644
@@ -25,9 +25,11 @@
  #include<linux/mutex.h>
  #include<linux/suspend.h>
  #include<linux/delay.h>
+#include<linux/of.h>
  #include<linux/regulator/consumer.h>
  #include<linux/regulator/driver.h>
  #include<linux/regulator/machine.h>
+#include<linux/regulator/of_regulator.h>
 
  #define CREATE_TRACE_POINTS
  #include<trace/events/regulator.h>

@@ -1155,6 +1157,7 @@ static struct regulator *_regulator_get(struct device *dev, const char *id,
  	struct regulator_map *map;
  	struct regulator *regulator = ERR_PTR(-ENODEV);
  	const char *devname = NULL;
+	struct device_node *node;
  	int ret;
 
  	if (id == NULL) {

@@ -1167,6 +1170,15 @@ static struct regulator *_regulator_get(struct device *dev, const char *id,
 
  	mutex_lock(&regulator_list_mutex);
 
+	if (dev->of_node) {
+		node = of_get_regulator(dev, id);
+		if (!node)
+			goto retry; /* fallback and chk regulator_map_list */
+		list_for_each_entry(rdev,&regulator_list, list)
+			if (node == rdev->node)
+				goto found;
+	}
+retry:
  	list_for_each_entry(map,&regulator_map_list, list) {
  		/* If the mapping has a device set up it must match */
  		if (map->dev_name&&

@@ -2619,6 +2631,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;
  	INIT_LIST_HEAD(&rdev->consumer_list);
  	INIT_LIST_HEAD(&rdev->list);
  	BLOCKING_INIT_NOTIFIER_HEAD(&rdev->notifier);

include/linux/regulator/driver.h +0 -0

index 1a80bc7..4aebbf5 100644
@@ -196,6 +196,9 @@ struct regulator_dev {
  	struct mutex mutex; /* consumer lock */

include/linux/regulator/driver.h struct regulator_dev { · line 201

Quoted 5 lines

include/linux/regulator/driver.h +3 -0

  	struct module *owner;
  	struct device dev;
+#ifdef CONFIG_OF
+	struct device_node *node;
+#endif

There is already an of_node pointer in regulator_dev->dev.of_node. Why does another need to be added here?

g.

Hi Mark, Grant,

This is a respin of my RFC series I posted sometime back and is now based on top of the latest omap i2c-twl support series posted by Benoit
git://gitorious.org/omap-pm/linux.git for_3.2/4_omap_dt_i2c_twl

some changes done since the RFC:
1. twl driver fixed to remove hardcoded board params
2. regulator helpers moved from drivers/of to drivers/regulator
3. Better compatible definitions for specific device type
4. twl regulator driver doing internal table lookup based on compatible rather then pdev->id
5. Seperate fixed voltage regulator bindings defined
6. Changed the way devices associate with regulators i.e using <name>-supply = <&regulator-phandle>;