[PATCH V3 0/3] regulator: dt: add policy to match regulator with prop "regulator-compatible"

[PATCH V3 0/3] regulator: dt: add policy to match regulator with prop "regulator-compatible"

[Laxman Dewangan]

and patch V1 of this series:
add policy to match regulator with the property of "regulator-compatible" of each
child regulator node with their hardware counterparts name.
Modify documentation as well to reflect this change and dts files.

Changes from V1: 
- Modify the existing of_regulator_match() to use the property 
  "regulator-compatible" of each child node of regulator for 
   matching rather than child node name.
- Documentation change to reflect this policy who are using the
  of_regulator_match().
- Modify the dts file to absorb this policy.

Changes from V2:
- Making modification of the db8500.dtsi as first patch.
- Add break in te match loop in of_regulator_match() when match found.
- Add the policy in the dt documentation of regulator.txt if the regulators
  nodes are under regulator node.

Laxman Dewangan (3):
  ARM: dts: db8500: add property "regulator-compatible" regulator node
  regulator: dt: regulator match by regulator-compatible
  regulator: dt: add policy to have property "regulator-compatible"

 Documentation/devicetree/bindings/mfd/tps65910.txt |   54 +++++++++++++----
 .../devicetree/bindings/regulator/regulator.txt    |    5 ++
 .../devicetree/bindings/regulator/tps6586x.txt     |   60 +++++++++++++++-----
 arch/arm/boot/dts/db8500.dtsi                      |   30 ++++++++++
 drivers/regulator/of_regulator.c                   |   51 +++++++++++------
 5 files changed, 154 insertions(+), 46 deletions(-)

[PATCH V3 1/3] ARM: dts: db8500: add property "regulator-compatible" regulator node

[Laxman Dewangan]

Device's regulator matches their hardware counterparts with the
property "regulator-compatible" of each child regulator node in
place of the child node name.
Add the property "regulator-compatible" for each regulator with
their hardware counterpart's name.

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
---
Changes from V2:
- Adding the regulator-compatible for each regulator child node.
- Remove the renaming of the child node.

 arch/arm/boot/dts/db8500.dtsi |   30 ++++++++++++++++++++++++++++++
 1 files changed, 30 insertions(+), 0 deletions(-)

diff --git a/arch/arm/boot/dts/db8500.dtsi b/arch/arm/boot/dts/db8500.dtsi
index 4ad5160..ec2be92 100644
--- a/arch/arm/boot/dts/db8500.dtsi
+++ b/arch/arm/boot/dts/db8500.dtsi
@@ -206,62 +206,74 @@
 
 				// DB8500_REGULATOR_VAPE
 				db8500_vape_reg: db8500_vape {
+					regulator-compatible = "db8500_vape";
 					regulator-name = "db8500-vape";
 					regulator-always-on;
 				};
 
 				// DB8500_REGULATOR_VARM
 				db8500_varm_reg: db8500_varm {
+					regulator-compatible = "db8500_varm";
 					regulator-name = "db8500-varm";
 				};
 
 				// DB8500_REGULATOR_VMODEM
 				db8500_vmodem_reg: db8500_vmodem {
+					regulator-compatible = "db8500_vmodem";
 					regulator-name = "db8500-vmodem";
 				};
 
 				// DB8500_REGULATOR_VPLL
 				db8500_vpll_reg: db8500_vpll {
+					regulator-compatible = "db8500_vpll";
 					regulator-name = "db8500-vpll";
 				};
 
 				// DB8500_REGULATOR_VSMPS1
 				db8500_vsmps1_reg: db8500_vsmps1 {
+					regulator-compatible = "db8500_vsmps1";
 					regulator-name = "db8500-vsmps1";
 				};
 
 				// DB8500_REGULATOR_VSMPS2
 				db8500_vsmps2_reg: db8500_vsmps2 {
+					regulator-compatible = "db8500_vsmps2";
 					regulator-name = "db8500-vsmps2";
 				};
 
 				// DB8500_REGULATOR_VSMPS3
 				db8500_vsmps3_reg: db8500_vsmps3 {
+					regulator-compatible = "db8500_vsmps3";
 					regulator-name = "db8500-vsmps3";
 				};
 
 				// DB8500_REGULATOR_VRF1
 				db8500_vrf1_reg: db8500_vrf1 {
+					regulator-compatible = "db8500_vrf1";
 					regulator-name = "db8500-vrf1";
 				};
 
 				// DB8500_REGULATOR_SWITCH_SVAMMDSP
 				db8500_sva_mmdsp_reg: db8500_sva_mmdsp {
+					regulator-compatible = "db8500_sva_mmdsp";
 					regulator-name = "db8500-sva-mmdsp";
 				};
 
 				// DB8500_REGULATOR_SWITCH_SVAMMDSPRET
 				db8500_sva_mmdsp_ret_reg: db8500_sva_mmdsp_ret {
+					regulator-compatible = "db8500_sva_mmdsp_ret";
 					regulator-name = "db8500-sva-mmdsp-ret";
 				};
 
 				// DB8500_REGULATOR_SWITCH_SVAPIPE
 				db8500_sva_pipe_reg: db8500_sva_pipe {
+					regulator-compatible = "db8500_sva_pipe";
 					regulator-name = "db8500_sva_pipe";
 				};
 
 				// DB8500_REGULATOR_SWITCH_SIAMMDSP
 				db8500_sia_mmdsp_reg: db8500_sia_mmdsp {
+					regulator-compatible = "db8500_sia_mmdsp";
 					regulator-name = "db8500_sia_mmdsp";
 				};
 
@@ -272,38 +284,45 @@
 
 				// DB8500_REGULATOR_SWITCH_SIAPIPE
 				db8500_sia_pipe_reg: db8500_sia_pipe {
+					regulator-compatible = "db8500_sia_pipe";
 					regulator-name = "db8500-sia-pipe";
 				};
 
 				// DB8500_REGULATOR_SWITCH_SGA
 				db8500_sga_reg: db8500_sga {
+					regulator-compatible = "db8500_sga";
 					regulator-name = "db8500-sga";
 					vin-supply = <&db8500_vape_reg>;
 				};
 
 				// DB8500_REGULATOR_SWITCH_B2R2_MCDE
 				db8500_b2r2_mcde_reg: db8500_b2r2_mcde {
+					regulator-compatible = "db8500_b2r2_mcde";
 					regulator-name = "db8500-b2r2-mcde";
 					vin-supply = <&db8500_vape_reg>;
 				};
 
 				// DB8500_REGULATOR_SWITCH_ESRAM12
 				db8500_esram12_reg: db8500_esram12 {
+					regulator-compatible = "db8500_esram12";
 					regulator-name = "db8500-esram12";
 				};
 
 				// DB8500_REGULATOR_SWITCH_ESRAM12RET
 				db8500_esram12_ret_reg: db8500_esram12_ret {
+					regulator-compatible = "db8500_esram12_ret";
 					regulator-name = "db8500-esram12-ret";
 				};
 
 				// DB8500_REGULATOR_SWITCH_ESRAM34
 				db8500_esram34_reg: db8500_esram34 {
+					regulator-compatible = "db8500_esram34";
 					regulator-name = "db8500-esram34";
 				};
 
 				// DB8500_REGULATOR_SWITCH_ESRAM34RET
 				db8500_esram34_ret_reg: db8500_esram34_ret {
+					regulator-compatible = "db8500_esram34_ret";
 					regulator-name = "db8500-esram34-ret";
 				};
 			};
@@ -318,6 +337,7 @@
 
 					// supplies to the display/camera
 					ab8500_ldo_aux1_reg: ab8500_ldo_aux1 {
+						regulator-compatible = "ab8500_ldo_aux1";
 						regulator-name = "V-DISPLAY";
 						regulator-min-microvolt = <2500000>;
 						regulator-max-microvolt = <2900000>;
@@ -328,6 +348,7 @@
 
 					// supplies to the on-board eMMC
 					ab8500_ldo_aux2_reg: ab8500_ldo_aux2 {
+						regulator-compatible = "ab8500_ldo_aux2";
 						regulator-name = "V-eMMC1";
 						regulator-min-microvolt = <1100000>;
 						regulator-max-microvolt = <3300000>;
@@ -335,6 +356,7 @@
 
 					// supply for VAUX3; SDcard slots
 					ab8500_ldo_aux3_reg: ab8500_ldo_aux3 {
+						regulator-compatible = "ab8500_ldo_aux3";
 						regulator-name = "V-MMC-SD";
 						regulator-min-microvolt = <1100000>;
 						regulator-max-microvolt = <3300000>;
@@ -342,41 +364,49 @@
 
 					// supply for v-intcore12; VINTCORE12 LDO
 					ab8500_ldo_initcore_reg: ab8500_ldo_initcore {
+						regulator-compatible = "ab8500_ldo_initcore";
 						regulator-name = "V-INTCORE";
 					};
 
 					// supply for tvout; gpadc; TVOUT LDO
 					ab8500_ldo_tvout_reg: ab8500_ldo_tvout {
+						regulator-compatible = "ab8500_ldo_tvout";
 						regulator-name = "V-TVOUT";
 					};
 
 					// supply for ab8500-usb; USB LDO
 					ab8500_ldo_usb_reg: ab8500_ldo_usb {
+						regulator-compatible = "ab8500_ldo_usb";
 						regulator-name = "dummy";
 					};
 
 					// supply for ab8500-vaudio; VAUDIO LDO
 					ab8500_ldo_audio_reg: ab8500_ldo_audio {
+						regulator-compatible = "ab8500_ldo_audio";
 						regulator-name = "V-AUD";
 					};
 
 					// supply for v-anamic1 VAMic1-LDO
 					ab8500_ldo_anamic1_reg: ab8500_ldo_anamic1 {
+						regulator-compatible = "ab8500_ldo_anamic1";
 						regulator-name = "V-AMIC1";
 					};
 
 					// supply for v-amic2; VAMIC2 LDO; reuse constants for AMIC1
 					ab8500_ldo_amamic2_reg: ab8500_ldo_amamic2 {
+						regulator-compatible = "ab8500_ldo_amamic2";
 						regulator-name = "V-AMIC2";
 					};
 
 					// supply for v-dmic; VDMIC LDO
 					ab8500_ldo_dmic_reg: ab8500_ldo_dmic {
+						regulator-compatible = "ab8500_ldo_dmic";
 						regulator-name = "V-DMIC";
 					};
 
 					// supply for U8500 CSI/DSI; VANA LDO
 					ab8500_ldo_ana_reg: ab8500_ldo_ana {
+						regulator-compatible = "ab8500_ldo_ana";
 						regulator-name = "V-CSI/DSI";
 					};
 				};
-- 
1.7.1.1

Re: [PATCH V3 1/3] ARM: dts: db8500: add property "regulator-compatible" regulator node

[Stephen Warren]

On 06/20/2012 06:23 AM, Laxman Dewangan wrote:
> Device's regulator matches their hardware counterparts with the
> property "regulator-compatible" of each child regulator node in
> place of the child node name.
> Add the property "regulator-compatible" for each regulator with
> their hardware counterpart's name.
> 
> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>

This change is correct as far as it goes, so:
Acked-by: Stephen Warren <swarren@wwwdotorg.org>

That said, the DT still should be modified to match the rules of correct
DT formatting. That was a pre-existing issue though, so it's OK if that
doesn't happen in this patch, and I'm happy leaving that to the
maintainers of this file.

[PATCH V3 2/3] regulator: dt: regulator match by regulator-compatible

[Laxman Dewangan]

Match the device's regulators with the property of
"regulator-compatible" of each regulator node.

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
Acked-by: Stephen Warren <swarren@wwwdotorg.org>
---
Changes from V1:
- In place of adding new api, modify the existing api of_regulator_match() to
look regulator-compatible for matching.

Changes from V2:
- Add break if the regualtor match found.

 drivers/regulator/of_regulator.c |   51 ++++++++++++++++++++++++-------------
 1 files changed, 33 insertions(+), 18 deletions(-)

diff --git a/drivers/regulator/of_regulator.c b/drivers/regulator/of_regulator.c
index e2a7310..dac7550 100644
--- a/drivers/regulator/of_regulator.c
+++ b/drivers/regulator/of_regulator.c
@@ -92,15 +92,17 @@ struct regulator_init_data *of_get_regulator_init_data(struct device *dev,
 EXPORT_SYMBOL_GPL(of_get_regulator_init_data);
 
 /**
- * of_regulator_match - extract regulator init data
+ * of_regulator_match - extract regulator init data when node
+ * property "regulator-compatible" matches with the regulator name.
  * @dev: device requesting the data
  * @node: parent device node of the regulators
  * @matches: match table for the regulators
  * @num_matches: number of entries in match table
  *
  * This function uses a match table specified by the regulator driver and
- * looks up the corresponding init data in the device tree. Note that the
- * match table is modified in place.
+ * looks up the corresponding init data in the device tree  if
+ * regulator-compatible matches. Note that the match table is modified
+ * in place.
  *
  * Returns the number of matches found or a negative error code on failure.
  */
@@ -110,27 +112,40 @@ int of_regulator_match(struct device *dev, struct device_node *node,
 {
 	unsigned int count = 0;
 	unsigned int i;
+	const char *regulator_comp;
+	struct device_node *child;
 
 	if (!dev || !node)
 		return -EINVAL;
 
-	for (i = 0; i < num_matches; i++) {
-		struct of_regulator_match *match = &matches[i];
-		struct device_node *child;
-
-		child = of_find_node_by_name(node, match->name);
-		if (!child)
-			continue;
-
-		match->init_data = of_get_regulator_init_data(dev, child);
-		if (!match->init_data) {
-			dev_err(dev, "failed to parse DT for regulator %s\n",
+	for_each_child_of_node(node, child) {
+		regulator_comp = of_get_property(child,
+					"regulator-compatible", NULL);
+		if (!regulator_comp) {
+			dev_err(dev, "regulator-compatible is missing for node %s\n",
 				child->name);
-			return -EINVAL;
+			continue;
+		}
+		for (i = 0; i < num_matches; i++) {
+			struct of_regulator_match *match = &matches[i];
+			if (match->of_node)
+				continue;
+
+			if (strcmp(match->name, regulator_comp))
+				continue;
+
+			match->init_data =
+				of_get_regulator_init_data(dev, child);
+			if (!match->init_data) {
+				dev_err(dev,
+					"failed to parse DT for regulator %s\n",
+					child->name);
+				return -EINVAL;
+			}
+			match->of_node = child;
+			count++;
+			break;
 		}
-
-		match->of_node = child;
-		count++;
 	}
 
 	return count;
-- 
1.7.1.1

Re: [PATCH V3 2/3] regulator: dt: regulator match by regulator-compatible

[Arnd Bergmann]

On Wednesday 20 June 2012, Laxman Dewangan wrote:
> Match the device's regulators with the property of
> "regulator-compatible" of each regulator node.
> 
> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
> Acked-by: Stephen Warren <swarren@wwwdotorg.org>
> ---
> Changes from V1:
> - In place of adding new api, modify the existing api of_regulator_match() to
> look regulator-compatible for matching.
> 
> Changes from V2:
> - Add break if the regualtor match found.


I've read the entire series a few times and I still can't figure out
why you're doing it. *Please* describe what the change is good for,
now what you are doing in it.

All I can tell from the patch and/or description right now is that
you are adding a third identifier next the phandle and the
"regulalator-name", but not why this is a good idea.

Specifically, please explain these two things in the changelog:

1. What is it that the new property can express that cannot already
be expressed by using the phandle.

2. Why is it called "regulator-compatible"? If it's similar to the
"compatible" property, don't you have to have a binding for each
possible string? If it's not related to the "compatible" property,
why is it named in a similar way?

	Arnd

Re: [PATCH V3 2/3] regulator: dt: regulator match by regulator-compatible

[Mark Brown]

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

On Wed, Jun 20, 2012 at 07:24:09PM +0000, Arnd Bergmann wrote:

> 1. What is it that the new property can express that cannot already
> be expressed by using the phandle.

As well as being able to refer to the object from within the device tree
we also need to be able to tell what the object represents - we have a
bunch of regulators in an array under a node for a PMIC and we want to
know which regulator on the physical device each array entry corresponds
to.  Previously this was being done by parsing the phandle name but that
means we can't have more than one device with the same set of names.

> 2. Why is it called "regulator-compatible"? If it's similar to the
> "compatible" property, don't you have to have a binding for each
> possible string? If it's not related to the "compatible" property,
> why is it named in a similar way?

Yes, there are bindings defined already for all the relevant devices.
Elsewhere in the binding document you'll see a list of all the
regulators on the PMIC and the names by which the device tree binding
knows them.

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

Re: [PATCH V3 2/3] regulator: dt: regulator match by regulator-compatible

[Stephen Warren]

On 06/20/2012 01:46 PM, Mark Brown wrote:
> On Wed, Jun 20, 2012 at 07:24:09PM +0000, Arnd Bergmann wrote:
> 
>> 1. What is it that the new property can express that cannot
>> already be expressed by using the phandle.
> 
> As well as being able to refer to the object from within the device
> tree we also need to be able to tell what the object represents -
> we have a bunch of regulators in an array under a node for a PMIC
> and we want to know which regulator on the physical device each
> array entry corresponds to.  Previously this was being done by
> parsing the phandle name but that

s/phandle name/node name/ in that line I think.

At least, I don't think it's common to refer to nodes as phandles;
phandles are usually the values used at "client" sites to refer to
nodes IIUC.

> means we can't have more than one device with the same set of
> names.
> 
>> 2. Why is it called "regulator-compatible"? If it's similar to
>> the "compatible" property, don't you have to have a binding for
>> each possible string? If it's not related to the "compatible"
>> property, why is it named in a similar way?
> 
> Yes, there are bindings defined already for all the relevant
> devices. Elsewhere in the binding document you'll see a list of all
> the regulators on the PMIC and the names by which the device tree
> binding knows them.

Re: [PATCH V3 2/3] regulator: dt: regulator match by regulator-compatible

[Mark Brown]

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

On Wed, Jun 20, 2012 at 01:51:27PM -0600, Stephen Warren wrote:

> At least, I don't think it's common to refer to nodes as phandles;
> phandles are usually the values used at "client" sites to refer to
> nodes IIUC.

Right, I always think of the phandle as being the label (which is
present at both ends of the link - the client refers to the handle the
node provdes.  Though now I think about it I suppose it might be
Hungarian with the node having a handle and the client having a
phandle...

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

Re: [PATCH V3 2/3] regulator: dt: regulator match by regulator-compatible

[Arnd Bergmann]

On Wednesday 20 June 2012, Mark Brown wrote:
> On Wed, Jun 20, 2012 at 07:24:09PM +0000, Arnd Bergmann wrote:
> 
> > 1. What is it that the new property can express that cannot already
> > be expressed by using the phandle.
> 
> As well as being able to refer to the object from within the device tree
> we also need to be able to tell what the object represents - we have a
> bunch of regulators in an array under a node for a PMIC and we want to
> know which regulator on the physical device each array entry corresponds
> to.  Previously this was being done by parsing the phandle name but that
> means we can't have more than one device with the same set of names.

I see.

But that sounds more like what the "reg" property does than what
the "compatible" property does for other devices. In other words, you
want to know which instance you are describing, not what kind of
object it is.

For a "compatible" property, I would expect two objects that have
the same register-level interface but are responsible for different
physical objects to have the same "compatible" values, but here
you specifically need distinct "regulator-compatible" values.
 
> > 2. Why is it called "regulator-compatible"? If it's similar to the
> > "compatible" property, don't you have to have a binding for each
> > possible string? If it's not related to the "compatible" property,
> > why is it named in a similar way?
> 
> Yes, there are bindings defined already for all the relevant devices.
> Elsewhere in the binding document you'll see a list of all the
> regulators on the PMIC and the names by which the device tree binding
> knows them.

Ah, so we just change the way how the binding refers to those names,
rather than the individual strings.

It makes much more sense now, so please let's make sure that explanation
ends up in the changelog.

I still have two more questions: 

* In case of tps65910, the individual regulators are all numbered, so
the driver can find them by their "reg" property. Should we still
mandate the use of the "regulator-compatible" property? It sounds
to me that we could better make it an optional property in those
cases.

* What is the difference between "regulator-name" and
"regulator-compatible"? Those are the same most of the time
in the patches, so can't we just make sure they are always the
same, and drop the confusingly named "regulator-compatible" one?

	Arnd

Re: [PATCH V3 2/3] regulator: dt: regulator match by regulator-compatible

[Stephen Warren]

On 06/20/2012 02:40 PM, Arnd Bergmann wrote:
> On Wednesday 20 June 2012, Mark Brown wrote:
>> On Wed, Jun 20, 2012 at 07:24:09PM +0000, Arnd Bergmann wrote:
>>
>>> 1. What is it that the new property can express that cannot already
>>> be expressed by using the phandle.
>>
>> As well as being able to refer to the object from within the device tree
>> we also need to be able to tell what the object represents - we have a
>> bunch of regulators in an array under a node for a PMIC and we want to
>> know which regulator on the physical device each array entry corresponds
>> to.  Previously this was being done by parsing the phandle name but that
>> means we can't have more than one device with the same set of names.
> 
> I see.
> 
> But that sounds more like what the "reg" property does than what
> the "compatible" property does for other devices. In other words, you
> want to know which instance you are describing, not what kind of
> object it is.

Yes.

> For a "compatible" property, I would expect two objects that have
> the same register-level interface but are responsible for different
> physical objects to have the same "compatible" values, but here
> you specifically need distinct "regulator-compatible" values.

I'd originally suggested "regulator-id" instead of
"regulator-compatible" in order to avoid any association with the
pre-existing "compatible" property. Mark didn't like that, although I
don't think I quite understood why.

>>> 2. Why is it called "regulator-compatible"? If it's similar to the
>>> "compatible" property, don't you have to have a binding for each
>>> possible string? If it's not related to the "compatible" property,
>>> why is it named in a similar way?
>>
>> Yes, there are bindings defined already for all the relevant devices.
>> Elsewhere in the binding document you'll see a list of all the
>> regulators on the PMIC and the names by which the device tree binding
>> knows them.
> 
> Ah, so we just change the way how the binding refers to those names,
> rather than the individual strings.
> 
> It makes much more sense now, so please let's make sure that explanation
> ends up in the changelog.
> 
> I still have two more questions: 
> 
> * In case of tps65910, the individual regulators are all numbered, so
> the driver can find them by their "reg" property. Should we still
> mandate the use of the "regulator-compatible" property? It sounds
> to me that we could better make it an optional property in those
> cases.

The problem is that dtc has no named constants. Using raw integers
instead of names would make the .dts file rather unreadable. The issue
is much more accute for regulators than say GPIOs or IRQs because
there's likely no relative order to the set of regulators defined by the
documentation, unlike for GPIOs/IRQs where the integer (often) is the
object's primary ID.

> * What is the difference between "regulator-name" and
> "regulator-compatible"? Those are the same most of the time
> in the patches, so can't we just make sure they are always the
> same, and drop the confusingly named "regulator-compatible" one?

regulator-compatible is the name of the regulator within the chip; the
HW sub-module say.

regulator-name is the board-/system-level name of the regulator. Perhaps
a signal name from the schematic, or maybe function name like "SD card
supply".

Re: [PATCH V3 2/3] regulator: dt: regulator match by regulator-compatible

[Mark Brown]

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

On Wed, Jun 20, 2012 at 03:01:15PM -0600, Stephen Warren wrote:
> On 06/20/2012 02:40 PM, Arnd Bergmann wrote:

> > For a "compatible" property, I would expect two objects that have
> > the same register-level interface but are responsible for different
> > physical objects to have the same "compatible" values, but here
> > you specifically need distinct "regulator-compatible" values.

> I'd originally suggested "regulator-id" instead of
> "regulator-compatible" in order to avoid any association with the
> pre-existing "compatible" property. Mark didn't like that, although I
> don't think I quite understood why.

Like I said we already have names (and we have IDs inside the API as
well) and it also seems better to make the "which thing is this?"
property have the same name as the more generic "which thing is this?"
property.  I did mention the fact that drivers could read additional
properties to further identify the regulator if they were so moved.

Ideally we'd just be able to have either named nodes directly in the
parent or be able to have arrays function as a hash, really this is
trying to simulate a hash.

> The problem is that dtc has no named constants. Using raw integers
> instead of names would make the .dts file rather unreadable. The issue
> is much more accute for regulators than say GPIOs or IRQs because
> there's likely no relative order to the set of regulators defined by the
> documentation, unlike for GPIOs/IRQs where the integer (often) is the
> object's primary ID.

Well, there are actually a lot of chips which do provide useful indexes
- for example the wm831x devices just have a bunch of DCDCs and a bunch
of LDOs which can usefully be referred to as DCDCn or LDOn.  They will
hopefully not need to use this interface.  It's just that there's also a
large class of devices we need to cater for which don't have any such
regularity in their register map, this biding
mechanism is for them.

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

Re: [PATCH V3 2/3] regulator: dt: regulator match by regulator-compatible

[Arnd Bergmann]

On Wednesday 20 June 2012, Mark Brown wrote:
> On Wed, Jun 20, 2012 at 03:01:15PM -0600, Stephen Warren wrote:
> > The problem is that dtc has no named constants. Using raw integers
> > instead of names would make the .dts file rather unreadable. The issue
> > is much more accute for regulators than say GPIOs or IRQs because
> > there's likely no relative order to the set of regulators defined by the
> > documentation, unlike for GPIOs/IRQs where the integer (often) is the
> > object's primary ID.
> 
> Well, there are actually a lot of chips which do provide useful indexes
> - for example the wm831x devices just have a bunch of DCDCs and a bunch
> of LDOs which can usefully be referred to as DCDCn or LDOn.  They will
> hopefully not need to use this interface.  It's just that there's also a
> large class of devices we need to cater for which don't have any such
> regularity in their register map, this biding
> mechanism is for them.

Ok, now it all makes sense. Thanks for bearing with me being a little
slow on this topic.

It seems that the drivers that are changed to use this could also try to
describe the individual regulators completely, by moving the contents
of e.g. ab8500_regulator_info into the device tree, but having the string
identifier with an in-kernel table makes sense when there is only one
such table.

	Arnd

Re: [PATCH V3 2/3] regulator: dt: regulator match by regulator-compatible

[Mark Brown]

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

On Thu, Jun 21, 2012 at 02:50:35PM +0000, Arnd Bergmann wrote:

> It seems that the drivers that are changed to use this could also try to
> describe the individual regulators completely, by moving the contents
> of e.g. ab8500_regulator_info into the device tree, but having the string
> identifier with an in-kernel table makes sense when there is only one
> such table.

I'm not that big a fan of moving all the data into device tree as it
means that you need even more parsing code and you need to update the
device trees for every board out there every time you want to add
support for a new feature which doesn't seem like a win.  Right now with
the DT kept in the kernel it's not so bad but if we ever do start
distributing it separately it becomes more of an issue.

I'm also not sure if the tooling works well for allowing people to
include standard DTs for chips and add new properties to nodes for the
board specific configuration, though I think I've seen a few things
which suggested that was dealt with reasonably well.

It also means that the driver becomes useless on non-DT systems; while
for something like ab85xx which is specific to a SoC that's not a
problem it's a serious drawback for devices which may be used on other
architectures (or older ARM systems for that matter).  I'm quite worried
by this tendency to try to just dump all our data structures into DT -
it's similar to the issues with some of the MFDs just wanting to drop in
the current Linux subdivision of the device even though that's often
just reflecting our current internal structures which do change even
without worrying about other OSs.

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

Re: [PATCH V3 2/3] regulator: dt: regulator match by regulator-compatible

[Arnd Bergmann]

On Thursday 21 June 2012, Mark Brown wrote:
>   Show Details
>   On Thu, Jun 21, 2012 at 02:50:35PM +0000, Arnd Bergmann wrote:
> 
> > It seems that the drivers that are changed to use this could also try to
> > describe the individual regulators completely, by moving the contents
> > of e.g. ab8500_regulator_info into the device tree, but having the string
> > identifier with an in-kernel table makes sense when there is only one
> > such table.
> 
> I'm not that big a fan of moving all the data into device tree as it
> means that you need even more parsing code and you need to update the
> device trees for every board out there every time you want to add
> support for a new feature which doesn't seem like a win.  Right now with
> the DT kept in the kernel it's not so bad but if we ever do start
> distributing it separately it becomes more of an issue.

Right. It's certainly a trade-off. If a company makes 100 SoCs that
all have similar-but-different regulators, then it should be clear
win to have the driver be very abstract and fed with DT data for
configuragtion.

> I'm also not sure if the tooling works well for allowing people to
> include standard DTs for chips and add new properties to nodes for the
> board specific configuration, though I think I've seen a few things
> which suggested that was dealt with reasonably well.

It should never be necessary to add board-specific properties in the
nodes that describe the SoC specific bits. What I was referring to
is just moving the data that currently resides in the regulator
driver into DT.

	Arnd

Re: [PATCH V3 2/3] regulator: dt: regulator match by regulator-compatible

[Stephen Warren]

On 06/21/2012 11:17 AM, Arnd Bergmann wrote:
> On Thursday 21 June 2012, Mark Brown wrote:
...
>> I'm also not sure if the tooling works well for allowing people to
>> include standard DTs for chips and add new properties to nodes for the
>> board specific configuration, though I think I've seen a few things
>> which suggested that was dealt with reasonably well.
> 
> It should never be necessary to add board-specific properties in the
> nodes that describe the SoC specific bits. What I was referring to
> is just moving the data that currently resides in the regulator
> driver into DT.

I guess I must be misunderstanding that comment - there are many many
examples of boards adding properties to nodes that describe the SoC. For
example, the GPIOs used by SDHCI controllers, board-specific max clock
rates for SDHCI or I2C controllers, pinmux configuration properties, to
name just a few.

Re: [PATCH V3 2/3] regulator: dt: regulator match by regulator-compatible

[Arnd Bergmann]

On Thursday 21 June 2012, Stephen Warren wrote:
> > It should never be necessary to add board-specific properties in the
> > nodes that describe the SoC specific bits. What I was referring to
> > is just moving the data that currently resides in the regulator
> > driver into DT.
> 
> I guess I must be misunderstanding that comment - there are many many
> examples of boards adding properties to nodes that describe the SoC. For
> example, the GPIOs used by SDHCI controllers, board-specific max clock
> rates for SDHCI or I2C controllers, pinmux configuration properties, to
> name just a few.

The difference is that configuration is board specific, but capabilities
of the hardware are soc specific. The regulator-compatible property
identifies just the capabilities and the location of one regulator:

Look at struct ab8500_regulator_info, everything in there describes
the soc: min/max voltages, register banks, startup delay. Those are
all things that we could in theory have in the .dtsi file describing
the soc.

The board specific settings on the other hand are about what to
put into those registers and what voltage to use. From all I can
tell, those settings belong into the node that actually uses the
regulator, not the node that defines it.

	Arnd

Re: [PATCH V3 2/3] regulator: dt: regulator match by regulator-compatible

[Mark Brown]

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

On Thu, Jun 21, 2012 at 09:03:29PM +0000, Arnd Bergmann wrote:

> The board specific settings on the other hand are about what to
> put into those registers and what voltage to use. From all I can
> tell, those settings belong into the node that actually uses the
> regulator, not the node that defines it.

This doesn't work.  Each regulator may be used by multiple consumers,
the configuration is for the supply as a whole not for an individual
user.  The main service the API is providing is arbitration between
multiple consumers and the physical capabilities of the board.

Due to the fact that the binding of the device has to be done by the
board you'd need to replace the regulator-compatible value with a
phandle pointing to whatever nodes the .dtsi defines (and which would
need to include some string as well so we can bind code to it).  And of
course if anyone wants to use the device on a non-DT architecture we'd
need to replicate everything in code anyway.

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

Re: [PATCH V3 2/3] regulator: dt: regulator match by regulator-compatible

[Mark Brown]

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

On Thu, Jun 21, 2012 at 05:17:45PM +0000, Arnd Bergmann wrote:
> On Thursday 21 June 2012, Mark Brown wrote:

> > I'm not that big a fan of moving all the data into device tree as it
> > means that you need even more parsing code and you need to update the
> > device trees for every board out there every time you want to add
> > support for a new feature which doesn't seem like a win.  Right now with
> > the DT kept in the kernel it's not so bad but if we ever do start
> > distributing it separately it becomes more of an issue.

> Right. It's certainly a trade-off. If a company makes 100 SoCs that
> all have similar-but-different regulators, then it should be clear
> win to have the driver be very abstract and fed with DT data for
> configuragtion.

Well, nobody does that anyway but even if they were it doesn't help
non-DT systems at all, nor does it help when we need to go and add new
properties to every existing device tree using the device.  We've got
far more architectures don't use DT than do...

> > I'm also not sure if the tooling works well for allowing people to
> > include standard DTs for chips and add new properties to nodes for the
> > board specific configuration, though I think I've seen a few things
> > which suggested that was dealt with reasonably well.

> It should never be necessary to add board-specific properties in the
> nodes that describe the SoC specific bits. What I was referring to
> is just moving the data that currently resides in the regulator
> driver into DT.

How would this work given that we also need to put system specific
configuration for the same devices into DT?  As Stephen says it doesn't
seem to match what we're currently doing.

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

Re: [PATCH V3 2/3] regulator: dt: regulator match by regulator-compatible

[Mitch Bradley]

[-- Attachment #1.1: Type: text/plain, Size: 2315 bytes --]

On 6/21/2012 9:45 AM, Mark Brown wrote:
> On Thu, Jun 21, 2012 at 05:17:45PM +0000, Arnd Bergmann wrote:
>> On Thursday 21 June 2012, Mark Brown wrote:
>
>>> I'm not that big a fan of moving all the data into device tree as it
>>> means that you need even more parsing code and you need to update the
>>> device trees for every board out there every time you want to add
>>> support for a new feature which doesn't seem like a win.


Maybe I'm missing something, but in general it's not necessary to update 
old device
trees to support new features.  The trick is to define a new property 
that describes
the new possibility.  Absence of that property implies that the default 
- the thing
that used to happen across the board, before the feature existed - applies.

>>>   Right now with
>>> the DT kept in the kernel it's not so bad but if we ever do start
>>> distributing it separately it becomes more of an issue.
>
>> Right. It's certainly a trade-off. If a company makes 100 SoCs that
>> all have similar-but-different regulators, then it should be clear
>> win to have the driver be very abstract and fed with DT data for
>> configuragtion.
>
> Well, nobody does that anyway but even if they were it doesn't help
> non-DT systems at all, nor does it help when we need to go and add new
> properties to every existing device tree using the device.  We've got
> far more architectures don't use DT than do...
>
>>> I'm also not sure if the tooling works well for allowing people to
>>> include standard DTs for chips and add new properties to nodes for the
>>> board specific configuration, though I think I've seen a few things
>>> which suggested that was dealt with reasonably well.
>
>> It should never be necessary to add board-specific properties in the
>> nodes that describe the SoC specific bits. What I was referring to
>> is just moving the data that currently resides in the regulator
>> driver into DT.
>
> How would this work given that we also need to put system specific
> configuration for the same devices into DT?  As Stephen says it doesn't
> seem to match what we're currently doing.
>
>
> _______________________________________________
> devicetree-discuss mailing list
> devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss


[-- Attachment #1.2: Type: text/html, Size: 4117 bytes --]

[-- Attachment #2: Type: text/plain, Size: 192 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH V3 2/3] regulator: dt: regulator match by regulator-compatible

[Mark Brown]

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

On Thu, Jun 21, 2012 at 11:53:41AM -1000, Mitch Bradley wrote:

Whatever mail program you're using appears to be generating HTML mails
and isn't doing an awesome job at formatting them, the plain text
version is mangled too with most peculiar word wrapping which is quite
hard to read.  I've reflowed.

> On 6/21/2012 9:45 AM, Mark Brown wrote:
> >On Thu, Jun 21, 2012 at 05:17:45PM +0000, Arnd Bergmann wrote:
> >>On Thursday 21 June 2012, Mark Brown wrote:

> >>>I'm not that big a fan of moving all the data into device tree as it
> >>>means that you need even more parsing code and you need to update the
> >>>device trees for every board out there every time you want to add
> >>>support for a new feature which doesn't seem like a win.

> Maybe I'm missing something, but in general it's not necessary to
> update old device trees to support new features.  The trick is to
> define a new property that describes the new possibility.  Absence of
> that property implies that the default - the thing that used to happen
> across the board, before the feature existed - applies.

Which is not a success in terms of deploying the new support, or indeed
in terms of deploying bugfixes.  It increases the overall complexity of
the system and I'm having a hard time getting at all excited about any
benefits.

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

Re: [PATCH V3 2/3] regulator: dt: regulator match by regulator-compatible

[Mark Brown]

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

On Wed, Jun 20, 2012 at 08:40:52PM +0000, Arnd Bergmann wrote:

> But that sounds more like what the "reg" property does than what
> the "compatible" property does for other devices. In other words, you
> want to know which instance you are describing, not what kind of
> object it is.

No, it's describing the kind of object.  This stuff is for devices which
throw a bunch of irregular regulators down on a chip with no particular
similarity with each other.

> For a "compatible" property, I would expect two objects that have
> the same register-level interface but are responsible for different
> physical objects to have the same "compatible" values, but here
> you specifically need distinct "regulator-compatible" values.

There's nothing stopping a driver drilling down deeper with other data
once it's worked out what it's talking about, if it wants to read an
additional id value or base register that's absolutely fine and probably
even a good idea.  The problem is that for a lot of hardware there's
just a bunch of random irregular devices.

> It makes much more sense now, so please let's make sure that explanation
> ends up in the changelog.

To be honest I just don't care, the old code hasn't been around for long
enough or widely adopted enough for anyone to care and it's just more
brain cycles gone.

> * What is the difference between "regulator-name" and
> "regulator-compatible"? Those are the same most of the time
> in the patches, so can't we just make sure they are always the
> same, and drop the confusingly named "regulator-compatible" one?

As the binding documentation says the former is a display name for use
so people can tell what the software is talking about, for example the
name the supply has in the schematic.  It's completely up to the user
and the regulator driver can't use it for anything except printing so it
needs something else to help it figure out what it's working with.

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

Re: [PATCH V3 2/3] regulator: dt: regulator match by regulator-compatible

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 1003 bytes --]

On Wed, Jun 20, 2012 at 08:46:10PM +0100, Mark Brown wrote:
> On Wed, Jun 20, 2012 at 07:24:09PM +0000, Arnd Bergmann wrote:
> 
> > 1. What is it that the new property can express that cannot already
> > be expressed by using the phandle.
> 
> As well as being able to refer to the object from within the device tree
> we also need to be able to tell what the object represents - we have a
> bunch of regulators in an array under a node for a PMIC and we want to
> know which regulator on the physical device each array entry corresponds
> to.  Previously this was being done by parsing the phandle name but that
> means we can't have more than one device with the same set of names.

I thought the reason was that matching the node names up with the
regulator names was considered bad practice. As Stephen already noticed,
the matching is done by node name, not phandle. Correct me if I'm wrong,
but you can have more than one device with the children have the same
names.

Thierry

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

[-- Attachment #2: Type: text/plain, Size: 192 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH V3 2/3] regulator: dt: regulator match by regulator-compatible

[Mark Brown]

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

On Fri, Jun 22, 2012 at 08:13:14AM +0200, Thierry Reding wrote:
> On Wed, Jun 20, 2012 at 08:46:10PM +0100, Mark Brown wrote:

> > As well as being able to refer to the object from within the device tree
> > we also need to be able to tell what the object represents - we have a
> > bunch of regulators in an array under a node for a PMIC and we want to
> > know which regulator on the physical device each array entry corresponds
> > to.  Previously this was being done by parsing the phandle name but that
> > means we can't have more than one device with the same set of names.

> I thought the reason was that matching the node names up with the
> regulator names was considered bad practice. As Stephen already noticed,
> the matching is done by node name, not phandle. Correct me if I'm wrong,
> but you can have more than one device with the children have the same
> names.

We're talking about consumers here, not the regulator driver finding its
own regulators.

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

Re: [PATCH V3 2/3] regulator: dt: regulator match by regulator-compatible

[Thierry Reding]

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

On Fri, Jun 22, 2012 at 09:42:29AM +0100, Mark Brown wrote:
> On Fri, Jun 22, 2012 at 08:13:14AM +0200, Thierry Reding wrote:
> > On Wed, Jun 20, 2012 at 08:46:10PM +0100, Mark Brown wrote:
> 
> > > As well as being able to refer to the object from within the device tree
> > > we also need to be able to tell what the object represents - we have a
> > > bunch of regulators in an array under a node for a PMIC and we want to
> > > know which regulator on the physical device each array entry corresponds
> > > to.  Previously this was being done by parsing the phandle name but that
> > > means we can't have more than one device with the same set of names.
> 
> > I thought the reason was that matching the node names up with the
> > regulator names was considered bad practice. As Stephen already noticed,
> > the matching is done by node name, not phandle. Correct me if I'm wrong,
> > but you can have more than one device with the children have the same
> > names.
> 
> We're talking about consumers here, not the regulator driver finding its
> own regulators.

No we're not. of_regulator_match() isn't used by consumers but by
regulator drivers to parse the DT.

Thierry

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

Re: [PATCH V3 2/3] regulator: dt: regulator match by regulator-compatible

[Mark Brown]

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

On Fri, Jun 22, 2012 at 10:59:09AM +0200, Thierry Reding wrote:
> On Fri, Jun 22, 2012 at 09:42:29AM +0100, Mark Brown wrote:

> > We're talking about consumers here, not the regulator driver finding its
> > own regulators.

> No we're not. of_regulator_match() isn't used by consumers but by
> regulator drivers to parse the DT.

This subthread has wandered off the topic of the original patch somewhat
though.

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

Re: [PATCH V3 2/3] regulator: dt: regulator match by regulator-compatible

[Laxman Dewangan]

Mark,

On Friday 22 June 2012 02:42 PM, Mark Brown wrote:
> * PGP Signed by an unknown key
>
> On Fri, Jun 22, 2012 at 10:59:09AM +0200, Thierry Reding wrote:
>> On Fri, Jun 22, 2012 at 09:42:29AM +0100, Mark Brown wrote:
>>> We're talking about consumers here, not the regulator driver finding its
>>> own regulators.
>> No we're not. of_regulator_match() isn't used by consumers but by
>> regulator drivers to parse the DT.
> This subthread has wandered off the topic of the original patch somewhat
> though.

I have not seen much oppose on this series as such and if you are OK on 
this then can we apply?
If it need to buy some ACK from different stack holder then we can ask them.

Re: [PATCH V3 2/3] regulator: dt: regulator match by regulator-compatible

[Mark Brown]

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

On Tue, Jun 26, 2012 at 02:32:33PM +0530, Laxman Dewangan wrote:

> I have not seen much oppose on this series as such and if you are OK
> on this then can we apply?
> If it need to buy some ACK from different stack holder then we can ask them.

I've been waiting for the ST Ericsson guys who are already CCed.  I
generally give people who I expect might review at least a week to do so
before I get bored and apply anyway.

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

Re: [PATCH V3 2/3] regulator: dt: regulator match by regulator-compatible

[Lee Jones]

On 26/06/12 10:12, Mark Brown wrote:
> On Tue, Jun 26, 2012 at 02:32:33PM +0530, Laxman Dewangan wrote:
>
>> I have not seen much oppose on this series as such and if you are OK
>> on this then can we apply?
>> If it need to buy some ACK from different stack holder then we can ask them.
>
> I've been waiting for the ST Ericsson guys who are already CCed.  I
> generally give people who I expect might review at least a week to do so
> before I get bored and apply anyway.

You need Linus' ACK, but he is off until Monday.

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
M: +44 77 88 633 515
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH V3 2/3] regulator: dt: regulator match by regulator-compatible

[Linus Walleij]

On Tue, Jun 26, 2012 at 12:13 PM, Lee Jones <lee.jones@linaro.org> wrote:
> On 26/06/12 10:12, Mark Brown wrote:
>>
>> On Tue, Jun 26, 2012 at 02:32:33PM +0530, Laxman Dewangan wrote:
>>
>>> I have not seen much oppose on this series as such and if you are OK
>>> on this then can we apply?
>>> If it need to buy some ACK from different stack holder then we can ask
>>> them.
>>
>>
>> I've been waiting for the ST Ericsson guys who are already CCed.  I
>> generally give people who I expect might review at least a week to do so
>> before I get bored and apply anyway.
>
> You need Linus' ACK, but he is off until Monday.

My ACK on this would be pointless since I barely grasp the issue.

I would much more appreciate your ACK or NACK on it, it's much more
valuable since you've been working on this code... and this very
aspect of it even, I guess?

Yours,
Linus Walleij

Re: [PATCH V3 2/3] regulator: dt: regulator match by regulator-compatible

[Lee Jones]

On 05/07/12 00:48, Linus Walleij wrote:
> On Tue, Jun 26, 2012 at 12:13 PM, Lee Jones <lee.jones@linaro.org> wrote:
>> On 26/06/12 10:12, Mark Brown wrote:
>>>
>>> On Tue, Jun 26, 2012 at 02:32:33PM +0530, Laxman Dewangan wrote:
>>>
>>>> I have not seen much oppose on this series as such and if you are OK
>>>> on this then can we apply?
>>>> If it need to buy some ACK from different stack holder then we can ask
>>>> them.
>>>
>>>
>>> I've been waiting for the ST Ericsson guys who are already CCed.  I
>>> generally give people who I expect might review at least a week to do so
>>> before I get bored and apply anyway.
>>
>> You need Linus' ACK, but he is off until Monday.
>
> My ACK on this would be pointless since I barely grasp the issue.
>
> I would much more appreciate your ACK or NACK on it, it's much more
> valuable since you've been working on this code... and this very
> aspect of it even, I guess?

Very well. For the db8500.dtsi changes you have my:

Acked-by: Lee Jones <lee.jones@linaro.org>


-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
M: +44 77 88 633 515
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH V3 3/3] regulator: dt: add policy to have property "regulator-compatible"

[Laxman Dewangan]

Add the policy for regulator DT such that if device have multiple
regulator and its binding contains a child node that describes each
regulator then each regulator child node must have the property
"regulator-compatible" which matches with regulator name of their
hardware counterparts.
Modify the DT documentation of regulator devices to reflect this
policy.

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
---
Changes from V1:
- Add the doc change for the DT binding of tps6586x.

Changes from V2:
- Add the details of property "regulator-compatible" in regulator.txt. 


 Documentation/devicetree/bindings/mfd/tps65910.txt |   54 +++++++++++++----
 .../devicetree/bindings/regulator/regulator.txt    |    5 ++
 .../devicetree/bindings/regulator/tps6586x.txt     |   60 +++++++++++++++-----
 3 files changed, 91 insertions(+), 28 deletions(-)

diff --git a/Documentation/devicetree/bindings/mfd/tps65910.txt b/Documentation/devicetree/bindings/mfd/tps65910.txt
index 645f5ea..b51d206 100644
--- a/Documentation/devicetree/bindings/mfd/tps65910.txt
+++ b/Documentation/devicetree/bindings/mfd/tps65910.txt
@@ -17,8 +17,9 @@ Required properties:
   device need to be present. The definition for each of these nodes is defined
   using the standard binding for regulators found at
   Documentation/devicetree/bindings/regulator/regulator.txt.
+  The regulator is matched with the regulator-compatible.
 
-  The valid names for regulators are:
+  The valid regulator-compatible values are:
   tps65910: vrtc, vio, vdd1, vdd2, vdd3, vdig1, vdig2, vpll, vdac, vaux1,
             vaux2, vaux33, vmmc
   tps65911: vrtc, vio, vdd1, vdd3, vddctrl, ldo1, ldo2, ldo3, ldo4, ldo5,
@@ -57,73 +58,100 @@ Example:
 		ti,en-gpio-sleep = <0 0 1 0 0 0 0 0 0>;
 
 		regulators {
-			vdd1_reg: vdd1 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			vdd1_reg: regulator@0 {
+				regulator-compatible = "vdd1";
+				reg = <0>;
 				regulator-min-microvolt = < 600000>;
 				regulator-max-microvolt = <1500000>;
 				regulator-always-on;
 				regulator-boot-on;
 				ti,regulator-ext-sleep-control = <0>;
 			};
-			vdd2_reg: vdd2 {
+			vdd2_reg: regulator@1 {
+				regulator-compatible = "vdd2";
+				reg = <1>;
 				regulator-min-microvolt = < 600000>;
 				regulator-max-microvolt = <1500000>;
 				regulator-always-on;
 				regulator-boot-on;
 				ti,regulator-ext-sleep-control = <4>;
 			};
-			vddctrl_reg: vddctrl {
+			vddctrl_reg: regulator@2 {
+				regulator-compatible = "vddctrl";
+				reg = <2>;
 				regulator-min-microvolt = < 600000>;
 				regulator-max-microvolt = <1400000>;
 				regulator-always-on;
 				regulator-boot-on;
 				ti,regulator-ext-sleep-control = <0>;
 			};
-			vio_reg: vio {
+			vio_reg: regulator@3 {
+				regulator-compatible = "vio";
+				reg = <3>;
 				regulator-min-microvolt = <1500000>;
 				regulator-max-microvolt = <1800000>;
 				regulator-always-on;
 				regulator-boot-on;
 				ti,regulator-ext-sleep-control = <1>;
 			};
-			ldo1_reg: ldo1 {
+			ldo1_reg: regulator@4 {
+				regulator-compatible = "ldo1";
+				reg = <4>;
 				regulator-min-microvolt = <1000000>;
 				regulator-max-microvolt = <3300000>;
 				ti,regulator-ext-sleep-control = <0>;
 			};
-			ldo2_reg: ldo2 {
+			ldo2_reg: regulator@5 {
+				regulator-compatible = "ldo2";
+				reg = <5>;
 				regulator-min-microvolt = <1050000>;
 				regulator-max-microvolt = <1050000>;
 				ti,regulator-ext-sleep-control = <0>;
 			};
-			ldo3_reg: ldo3 {
+			ldo3_reg: regulator@6 {
+				regulator-compatible = "ldo3";
+				reg = <6>;
 				regulator-min-microvolt = <1000000>;
 				regulator-max-microvolt = <3300000>;
 				ti,regulator-ext-sleep-control = <0>;
 			};
-			ldo4_reg: ldo4 {
+			ldo4_reg: regulator@7 {
+				regulator-compatible = "ldo4";
+				reg = <7>;
 				regulator-min-microvolt = <1000000>;
 				regulator-max-microvolt = <3300000>;
 				regulator-always-on;
 				ti,regulator-ext-sleep-control = <0>;
 			};
-			ldo5_reg: ldo5 {
+			ldo5_reg: regulator@8 {
+				regulator-compatible = "ldo5";
+				reg = <8>;
 				regulator-min-microvolt = <1000000>;
 				regulator-max-microvolt = <3300000>;
 				ti,regulator-ext-sleep-control = <0>;
 			};
-			ldo6_reg: ldo6 {
+			ldo6_reg: regulator@9 {
+				regulator-compatible = "ldo6";
+				reg = <9>;
 				regulator-min-microvolt = <1200000>;
 				regulator-max-microvolt = <1200000>;
 				ti,regulator-ext-sleep-control = <0>;
 			};
-			ldo7_reg: ldo7 {
+			ldo7_reg: regulator@10 {
+				regulator-compatible = "ldo7";
+				reg = <10>;
 				regulator-min-microvolt = <1200000>;
 				regulator-max-microvolt = <1200000>;
 				regulator-always-on;
 				regulator-boot-on;
 				ti,regulator-ext-sleep-control = <1>;
 			};
-			ldo8_reg: ldo8 {
+			ldo8_reg: regulator@11 {
+				regulator-compatible = "ldo8";
+				reg = <11>;
 				regulator-min-microvolt = <1000000>;
 				regulator-max-microvolt = <3300000>;
 				regulator-always-on;
diff --git a/Documentation/devicetree/bindings/regulator/regulator.txt b/Documentation/devicetree/bindings/regulator/regulator.txt
index d0a7b12..c196ab8 100644
--- a/Documentation/devicetree/bindings/regulator/regulator.txt
+++ b/Documentation/devicetree/bindings/regulator/regulator.txt
@@ -11,6 +11,11 @@ Optional properties:
 - regulator-boot-on: bootloader/firmware enabled regulator
 - <name>-supply: phandle to the parent supply/regulator node
 - regulator-ramp-delay: ramp delay for regulator(in mV/uS)
+- regulator-compatible: If a regulator chip contains multiple
+  regulators, and if the chip's binding contains a child node that
+  describes each regulator, then this property indicates which regulator
+  this child node is intended to configure.
 
 Example:
 
diff --git a/Documentation/devicetree/bindings/regulator/tps6586x.txt b/Documentation/devicetree/bindings/regulator/tps6586x.txt
index 0fcabaa..ab17ef6 100644
--- a/Documentation/devicetree/bindings/regulator/tps6586x.txt
+++ b/Documentation/devicetree/bindings/regulator/tps6586x.txt
@@ -6,8 +6,9 @@ Required properties:
 - interrupts: the interrupt outputs of the controller
 - #gpio-cells: number of cells to describe a GPIO
 - gpio-controller: mark the device as a GPIO controller
-- regulators: list of regulators provided by this controller, must be named
-  after their hardware counterparts: sm[0-2], ldo[0-9] and ldo_rtc
+- regulators: list of regulators provided by this controller, must have
+  property "regulator-compatible" to match their hardware counterparts:
+  sm[0-2], ldo[0-9] and ldo_rtc
 
 Each regulator is defined using the standard binding for regulators.
 
@@ -22,74 +23,103 @@ Example:
 		gpio-controller;
 
 		regulators {
-			sm0_reg: sm0 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			sm0_reg: regulator@0 {
+				reg = <0>;
+				regulator-compatible = "sm0";
 				regulator-min-microvolt = < 725000>;
 				regulator-max-microvolt = <1500000>;
 				regulator-boot-on;
 				regulator-always-on;
 			};
 
-			sm1_reg: sm1 {
+			sm1_reg: regulator@1 {
+				reg = <1>;
+				regulator-compatible = "sm1";
 				regulator-min-microvolt = < 725000>;
 				regulator-max-microvolt = <1500000>;
 				regulator-boot-on;
 				regulator-always-on;
 			};
 
-			sm2_reg: sm2 {
+			sm2_reg: regulator@2 {
+				reg = <2>;
+				regulator-compatible = "sm2";
 				regulator-min-microvolt = <3000000>;
 				regulator-max-microvolt = <4550000>;
 				regulator-boot-on;
 				regulator-always-on;
 			};
 
-			ldo0_reg: ldo0 {
+			ldo0_reg: regulator@3 {
+				reg = <3>;
+				regulator-compatible = "ldo0";
 				regulator-name = "PCIE CLK";
 				regulator-min-microvolt = <3300000>;
 				regulator-max-microvolt = <3300000>;
 			};
 
-			ldo1_reg: ldo1 {
+			ldo1_reg: regulator@4 {
+				reg = <4>;
+				regulator-compatible = "ldo1";
 				regulator-min-microvolt = < 725000>;
 				regulator-max-microvolt = <1500000>;
 			};
 
-			ldo2_reg: ldo2 {
+			ldo2_reg: regulator@5 {
+				reg = <5>;
+				regulator-compatible = "ldo2";
 				regulator-min-microvolt = < 725000>;
 				regulator-max-microvolt = <1500000>;
 			};
 
-			ldo3_reg: ldo3 {
+			ldo3_reg: regulator@6 {
+				reg = <6>;
+				regulator-compatible = "ldo3";
 				regulator-min-microvolt = <1250000>;
 				regulator-max-microvolt = <3300000>;
 			};
 
-			ldo4_reg: ldo4 {
+			ldo4_reg: regulator@7 {
+				reg = <7>;
+				regulator-compatible = "ldo4";
 				regulator-min-microvolt = <1700000>;
 				regulator-max-microvolt = <2475000>;
 			};
 
-			ldo5_reg: ldo5 {
+			ldo5_reg: regulator@8 {
+				reg = <8>;
+				regulator-compatible = "ldo5";
 				regulator-min-microvolt = <1250000>;
 				regulator-max-microvolt = <3300000>;
 			};
 
-			ldo6_reg: ldo6 {
+			ldo6_reg: regulator@9 {
+				reg = <9>;
+				regulator-compatible = "ldo6";
 				regulator-min-microvolt = <1250000>;
 				regulator-max-microvolt = <3300000>;
 			};
 
-			ldo7_reg: ldo7 {
+			ldo7_reg: regulator@10 {
+				reg = <10>;
+				regulator-compatible = "ldo7";
 				regulator-min-microvolt = <1250000>;
 				regulator-max-microvolt = <3300000>;
 			};
 
-			ldo8_reg: ldo8 {
+			ldo8_reg: regulator@11 {
+				reg = <11>;
+				regulator-compatible = "ldo8";
 				regulator-min-microvolt = <1250000>;
 				regulator-max-microvolt = <3300000>;
 			};
 
-			ldo9_reg: ldo9 {
+			ldo9_reg: regulator@12 {
+				reg = <12>;
+				regulator-compatible = "ldo9";
 				regulator-min-microvolt = <1250000>;
 				regulator-max-microvolt = <3300000>;
 			};
-- 
1.7.1.1

Re: [PATCH V3 3/3] regulator: dt: add policy to have property "regulator-compatible"

[Stephen Warren]

On 06/20/2012 06:23 AM, Laxman Dewangan wrote:
> Add the policy for regulator DT such that if device have multiple
> regulator and its binding contains a child node that describes each
> regulator then each regulator child node must have the property
> "regulator-compatible" which matches with regulator name of their
> hardware counterparts.
> Modify the DT documentation of regulator devices to reflect this
> policy.
> 
> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>

I'm personally OK with this change as-is, so
Acked-by: Stephen Warren <swarren@wwwdotorg.org>

I suspect Mark may want the wording in regulators.txt modified a little
to allow each regulator binding to choose whether they want the
regulator child nodes identified using the new regulator-compatible
property, or whether they want each instantiated as an actual device,
using regular device instantiation techniques. That could always be a
later patch though, and I'll leave this aspect up to Mark.

Re: [PATCH V3 3/3] regulator: dt: add policy to have property "regulator-compatible"

[Mark Brown]

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

On Wed, Jun 20, 2012 at 05:53:07PM +0530, Laxman Dewangan wrote:
> Add the policy for regulator DT such that if device have multiple
> regulator and its binding contains a child node that describes each
> regulator then each regulator child node must have the property
> "regulator-compatible" which matches with regulator name of their
> hardware counterparts.

I tried to apply this but both git and patch are upset with the patch at
the line:

diff --git a/Documentation/devicetree/bindings/regulator/tps6586x.txt b/Documentation/devicetree/bindings/regulator/tps6586x.txt

for reasons I'm not immediately able to discern.  Can you please look
into this?

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

Re: [PATCH V3 3/3] regulator: dt: add policy to have property "regulator-compatible"

[Laxman Dewangan]

On Wednesday 04 July 2012 12:55 AM, Mark Brown wrote:
> * PGP Signed by an unknown key
>
> On Wed, Jun 20, 2012 at 05:53:07PM +0530, Laxman Dewangan wrote:
>> Add the policy for regulator DT such that if device have multiple
>> regulator and its binding contains a child node that describes each
>> regulator then each regulator child node must have the property
>> "regulator-compatible" which matches with regulator name of their
>> hardware counterparts.
> I tried to apply this but both git and patch are upset with the patch at
> the line:
>
> diff --git a/Documentation/devicetree/bindings/regulator/tps6586x.txt b/Documentation/devicetree/bindings/regulator/tps6586x.txt
>
> for reasons I'm not immediately able to discern.  Can you please look
> into this?

I will locally try and will resend the patch if anything change.