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

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

[Laxman Dewangan]

Based on the discussion on patch
 [PATCH 2/2] ARM: dt: tegra: cardhu: register core regulator tps65911

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.

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

 Documentation/devicetree/bindings/mfd/tps65910.txt |   54 ++++++--
 .../devicetree/bindings/regulator/tps6586x.txt     |   60 +++++++---
 arch/arm/boot/dts/db8500.dtsi                      |  128 +++++++++++++++-----
 drivers/regulator/of_regulator.c                   |   50 +++++---
 4 files changed, 215 insertions(+), 77 deletions(-)

[PATCH V2 1/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>
---
Changes from V1:
- In place of adding new api, modify the existing api of_regulator_match() to
look regulator-compatible for matching.

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

diff --git a/drivers/regulator/of_regulator.c b/drivers/regulator/of_regulator.c
index e2a7310..7621306 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,39 @@ 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++;
 		}
-
-		match->of_node = child;
-		count++;
 	}
 
 	return count;
-- 
1.7.1.1

Re: [PATCH V2 1/3] regulator: dt: regulator match by regulator-compatible

[Stephen Warren]

On 06/19/2012 08:28 AM, Laxman Dewangan wrote:
> Match the device's regulators with the property of
> "regulator-compatible" of each regulator node.

> diff --git a/drivers/regulator/of_regulator.c b/drivers/regulator/of_regulator.c

>  /**
> - * 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.
>   */

I don't think you actually need to modify any of the documentation; the
function is still doing the exact same thing and it's an implementation
detail really that it's doing it based on the regulator-compatible
property rather than the node name now.

Still, that's just a nit-pick, so I'm OK either way.

> @@ -110,27 +112,39 @@ int of_regulator_match(struct device *dev, struct device_node *node,
...
> +	for_each_child_of_node(node, child) {
...
> +		for (i = 0; i < num_matches; i++) {
...
> +			match->of_node = child;
> +			count++;

You may as well "break;" here to avoid checking all the other match
table entries, which hopefully don't have duplicate names...

But, that's also pretty minor, so:
Acked-by: Stephen Warren <swarren@wwwdotorg.org>

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

[Laxman Dewangan]

Add the policy for regulator DT such that 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.

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

diff --git a/Documentation/devicetree/bindings/mfd/tps65910.txt b/Documentation/devicetree/bindings/mfd/tps65910.txt
index 645f5ea..5407a10 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 for regulators 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/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 V2 2/3] regulator: dt: add policy to have property "regulator-compatible"

[Stephen Warren]

On 06/19/2012 08:28 AM, Laxman Dewangan wrote:
> Add the policy for regulator DT such that 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.

> diff --git a/Documentation/devicetree/bindings/mfd/tps65910.txt b/Documentation/devicetree/bindings/mfd/tps65910.txt

>    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.

That last sentence should be true for any chip containing multiple
regulators and using the standard regulator binding. As such, shouldn't
that property be part of regulator.txt, rather than each individual
regulator chip's binding document?

> -  The valid names for regulators are:
> +  The valid regulator-compatible for regulators are:

Perhaps "The valid regulator-compatible values are:"

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

[Mark Brown]

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

On Tue, Jun 19, 2012 at 11:43:56AM -0600, Stephen Warren wrote:
> On 06/19/2012 08:28 AM, Laxman Dewangan wrote:

> > +  The regulator is matched with the regulator-compatible.

> That last sentence should be true for any chip containing multiple
> regulators and using the standard regulator binding. As such, shouldn't
> that property be part of regulator.txt, rather than each individual
> regulator chip's binding document?

No, there's more than one way to skin this cat.  We can either have
something like this where there's a single DT node for all regulators on
the device or we can have an MFD where the regulators all appear
separately.  This is certainly what the former case should be using but
it's less clear for the latter.

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

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

[Stephen Warren]

On 06/19/2012 11:53 AM, Mark Brown wrote:
> On Tue, Jun 19, 2012 at 11:43:56AM -0600, Stephen Warren wrote:
>> On 06/19/2012 08:28 AM, Laxman Dewangan wrote:
> 
>>> +  The regulator is matched with the regulator-compatible.
> 
>> That last sentence should be true for any chip containing
>> multiple regulators and using the standard regulator binding. As
>> such, shouldn't that property be part of regulator.txt, rather
>> than each individual regulator chip's binding document?
> 
> No, there's more than one way to skin this cat.  We can either
> have something like this where there's a single DT node for all
> regulators on the device or we can have an MFD where the regulators
> all appear separately.  This is certainly what the former case
> should be using but it's less clear for the latter.

Well, I expected the language to be something along the lines of:

Optional properties:
...
- 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.

... although I guess you'd need something to differentiate the
MFD-style vs. plain initdata-style mechanisms

So while as you say regulator.txt might not mandate that this be the
only method of handling multiple child nodes, shouldn't it document
this method as /a/ method in a central location to avoid all the
bindings that make use of this feature from duplicating the documentation?

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

[Mark Brown]

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

On Tue, Jun 19, 2012 at 12:03:27PM -0600, Stephen Warren wrote:

> So while as you say regulator.txt might not mandate that this be the
> only method of handling multiple child nodes, shouldn't it document
> this method as /a/ method in a central location to avoid all the
> bindings that make use of this feature from duplicating the documentation?

Sure, but I'd expect some reference in the individual binding documents
to the mechanism being used for a given device (which seems to be what
Laxman has here).

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

[PATCH V2 3/3] ARM: dts: db8500: add node 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.
Add the property "regulator-compatible" for each regulator with
their name.

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
---
Changes from V1:
 - This is new change in V2.

 arch/arm/boot/dts/db8500.dtsi |  128 +++++++++++++++++++++++++++++++----------
 1 files changed, 97 insertions(+), 31 deletions(-)

diff --git a/arch/arm/boot/dts/db8500.dtsi b/arch/arm/boot/dts/db8500.dtsi
index 4ad5160..9548f80 100644
--- a/arch/arm/boot/dts/db8500.dtsi
+++ b/arch/arm/boot/dts/db8500.dtsi
@@ -203,107 +203,149 @@
 
 			db8500-prcmu-regulators {
 				compatible = "stericsson,db8500-prcmu-regulator";
+				#address-cells = <1>;
+				#size-cells = <0>;
 
 				// DB8500_REGULATOR_VAPE
-				db8500_vape_reg: db8500_vape {
+				db8500_vape_reg: regulator@0 {
+					reg = <0>;
+					regulator-compatible = "db8500_vape";
 					regulator-name = "db8500-vape";
 					regulator-always-on;
 				};
 
 				// DB8500_REGULATOR_VARM
-				db8500_varm_reg: db8500_varm {
+				db8500_varm_reg: regulator@1 {
+					reg = <1>;
+					regulator-compatible = "db8500_varm";
 					regulator-name = "db8500-varm";
 				};
 
 				// DB8500_REGULATOR_VMODEM
-				db8500_vmodem_reg: db8500_vmodem {
+				db8500_vmodem_reg: regulator@2 {
+					reg = <2>;
+					regulator-compatible = "db8500_vmodem";
 					regulator-name = "db8500-vmodem";
 				};
 
 				// DB8500_REGULATOR_VPLL
-				db8500_vpll_reg: db8500_vpll {
+				db8500_vpll_reg: regulator@3 {
+					reg = <3>;
+					regulator-compatible = "db8500_vpll";
 					regulator-name = "db8500-vpll";
 				};
 
 				// DB8500_REGULATOR_VSMPS1
-				db8500_vsmps1_reg: db8500_vsmps1 {
+				db8500_vsmps1_reg: regulator@4 {
+					reg = <4>;
+					regulator-compatible = "db8500_vsmps1";
 					regulator-name = "db8500-vsmps1";
 				};
 
 				// DB8500_REGULATOR_VSMPS2
-				db8500_vsmps2_reg: db8500_vsmps2 {
+				db8500_vsmps2_reg: regulator@5 {
+					reg = <5>;
+					regulator-compatible = "db8500_vsmps2";
 					regulator-name = "db8500-vsmps2";
 				};
 
 				// DB8500_REGULATOR_VSMPS3
-				db8500_vsmps3_reg: db8500_vsmps3 {
+				db8500_vsmps3_reg: regulator@6 {
+					reg = <6>;
+					regulator-compatible = "db8500_vsmps3";
 					regulator-name = "db8500-vsmps3";
 				};
 
 				// DB8500_REGULATOR_VRF1
-				db8500_vrf1_reg: db8500_vrf1 {
+				db8500_vrf1_reg: regulator@7 {
+					reg = <7>;
+					regulator-compatible = "db8500_vrf1";
 					regulator-name = "db8500-vrf1";
 				};
 
 				// DB8500_REGULATOR_SWITCH_SVAMMDSP
-				db8500_sva_mmdsp_reg: db8500_sva_mmdsp {
+				db8500_sva_mmdsp_reg: regulator@8 {
+					reg = <8>;
+					regulator-compatible = "db8500_sva_mmdsp";
 					regulator-name = "db8500-sva-mmdsp";
 				};
 
 				// DB8500_REGULATOR_SWITCH_SVAMMDSPRET
-				db8500_sva_mmdsp_ret_reg: db8500_sva_mmdsp_ret {
+				db8500_sva_mmdsp_ret_reg: regulator@9 {
+					reg = <9>;
+					regulator-compatible = "db8500_sva_mmdsp_ret";
 					regulator-name = "db8500-sva-mmdsp-ret";
 				};
 
 				// DB8500_REGULATOR_SWITCH_SVAPIPE
-				db8500_sva_pipe_reg: db8500_sva_pipe {
+				db8500_sva_pipe_reg: regulator@10 {
+					reg = <10>;
+					regulator-compatible = "db8500_sva_pipe";
 					regulator-name = "db8500_sva_pipe";
 				};
 
 				// DB8500_REGULATOR_SWITCH_SIAMMDSP
-				db8500_sia_mmdsp_reg: db8500_sia_mmdsp {
+				db8500_sia_mmdsp_reg: regulator@11 {
+					reg = <11>;
+					regulator-compatible = "db8500_sia_mmdsp";
 					regulator-name = "db8500_sia_mmdsp";
 				};
 
 				// DB8500_REGULATOR_SWITCH_SIAMMDSPRET
-				db8500_sia_mmdsp_ret_reg: db8500_sia_mmdsp_ret {
+				db8500_sia_mmdsp_ret_reg: regulator@12 {
+					reg = <12>;
+					regulator-compatible = "db8500_sia_mmdsp_ret";
 					regulator-name = "db8500-sia-mmdsp-ret";
 				};
 
 				// DB8500_REGULATOR_SWITCH_SIAPIPE
-				db8500_sia_pipe_reg: db8500_sia_pipe {
+				db8500_sia_pipe_reg: regulator@13 {
+					reg = <13>;
+					regulator-compatible = "db8500_sia_pipe";
 					regulator-name = "db8500-sia-pipe";
 				};
 
 				// DB8500_REGULATOR_SWITCH_SGA
-				db8500_sga_reg: db8500_sga {
+				db8500_sga_reg: regulator@14 {
+					reg = <14>;
+					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 {
+				db8500_b2r2_mcde_reg: regulator@15 {
+					reg = <15>;
+					regulator-compatible = "db8500_b2r2_mcde";
 					regulator-name = "db8500-b2r2-mcde";
 					vin-supply = <&db8500_vape_reg>;
 				};
 
 				// DB8500_REGULATOR_SWITCH_ESRAM12
-				db8500_esram12_reg: db8500_esram12 {
+				db8500_esram12_reg: regulator@16 {
+					reg = <16>;
+					regulator-compatible = "db8500_esram12";
 					regulator-name = "db8500-esram12";
 				};
 
 				// DB8500_REGULATOR_SWITCH_ESRAM12RET
-				db8500_esram12_ret_reg: db8500_esram12_ret {
+				db8500_esram12_ret_reg: regulator@17 {
+					reg = <17>;
+					regulator-compatible = "db8500_esram12_ret";
 					regulator-name = "db8500-esram12-ret";
 				};
 
 				// DB8500_REGULATOR_SWITCH_ESRAM34
-				db8500_esram34_reg: db8500_esram34 {
+				db8500_esram34_reg: regulator@18 {
+					reg = <18>;
+					regulator-compatible = "db8500_esram34";
 					regulator-name = "db8500-esram34";
 				};
 
 				// DB8500_REGULATOR_SWITCH_ESRAM34RET
-				db8500_esram34_ret_reg: db8500_esram34_ret {
+				db8500_esram34_ret_reg: regulator@19 {
+					reg = <19>;
+					regulator-compatible = "db8500_esram34_ret";
 					regulator-name = "db8500-esram34-ret";
 				};
 			};
@@ -315,9 +357,13 @@
 
 				ab8500-regulators {
 					compatible = "stericsson,ab8500-regulator";
+					#address-cells = <1>;
+					#size-cells = <0>;
 
 					// supplies to the display/camera
-					ab8500_ldo_aux1_reg: ab8500_ldo_aux1 {
+					ab8500_ldo_aux1_reg: regulator@0 {
+						reg = <0>;
+						regulator-compatible = "ab8500_ldo_aux1";
 						regulator-name = "V-DISPLAY";
 						regulator-min-microvolt = <2500000>;
 						regulator-max-microvolt = <2900000>;
@@ -327,56 +373,76 @@
 					};
 
 					// supplies to the on-board eMMC
-					ab8500_ldo_aux2_reg: ab8500_ldo_aux2 {
+					ab8500_ldo_aux2_reg: regulator@1 {
+						reg = <1>;
+						regulator-compatible = "ab8500_ldo_aux2";
 						regulator-name = "V-eMMC1";
 						regulator-min-microvolt = <1100000>;
 						regulator-max-microvolt = <3300000>;
 					};
 
 					// supply for VAUX3; SDcard slots
-					ab8500_ldo_aux3_reg: ab8500_ldo_aux3 {
+					ab8500_ldo_aux3_reg: regulator@2 {
+						reg = <2>;
+						regulator-compatible = "ab8500_ldo_aux3";
 						regulator-name = "V-MMC-SD";
 						regulator-min-microvolt = <1100000>;
 						regulator-max-microvolt = <3300000>;
 					};
 
 					// supply for v-intcore12; VINTCORE12 LDO
-					ab8500_ldo_initcore_reg: ab8500_ldo_initcore {
+					ab8500_ldo_initcore_reg: regulator@3 {
+						reg = <3>;
+						regulator-compatible = "ab8500_ldo_initcore";
 						regulator-name = "V-INTCORE";
 					};
 
 					// supply for tvout; gpadc; TVOUT LDO
-					ab8500_ldo_tvout_reg: ab8500_ldo_tvout {
+					ab8500_ldo_tvout_reg: regulator@4 {
+						reg = <4>;
+						regulator-compatible = "ab8500_ldo_tvout";
 						regulator-name = "V-TVOUT";
 					};
 
 					// supply for ab8500-usb; USB LDO
-					ab8500_ldo_usb_reg: ab8500_ldo_usb {
+					ab8500_ldo_usb_reg: regulator@5 {
+						reg = <5>;
+						regulator-compatible = "ab8500_ldo_usb";
 						regulator-name = "dummy";
 					};
 
 					// supply for ab8500-vaudio; VAUDIO LDO
-					ab8500_ldo_audio_reg: ab8500_ldo_audio {
+					ab8500_ldo_audio_reg: regulator@6 {
+						reg = <6>;
+						regulator-compatible = "ab8500_ldo_audio";
 						regulator-name = "V-AUD";
 					};
 
 					// supply for v-anamic1 VAMic1-LDO
-					ab8500_ldo_anamic1_reg: ab8500_ldo_anamic1 {
+					ab8500_ldo_anamic1_reg: regulator@7 {
+						reg = <7>;
+						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 {
+					ab8500_ldo_amamic2_reg: regulator@8 {
+						reg = <8>;
+						regulator-compatible = "ab8500_ldo_amamic2";
 						regulator-name = "V-AMIC2";
 					};
 
 					// supply for v-dmic; VDMIC LDO
-					ab8500_ldo_dmic_reg: ab8500_ldo_dmic {
+					ab8500_ldo_dmic_reg: regulator@9 {
+						reg = <9>;
+						regulator-compatible = "ab8500_ldo_dmic";
 						regulator-name = "V-DMIC";
 					};
 
 					// supply for U8500 CSI/DSI; VANA LDO
-					ab8500_ldo_ana_reg: ab8500_ldo_ana {
+					ab8500_ldo_ana_reg: regulator@10 {
+						reg = <10>;
+						regulator-compatible = "ab8500_ldo_ana";
 						regulator-name = "V-CSI/DSI";
 					};
 				};
-- 
1.7.1.1

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

[Lee Jones]

On 19/06/12 15:28, 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.
> Add the property "regulator-compatible" for each regulator with
> their name.
>
> Signed-off-by: Laxman Dewangan<ldewangan@nvidia.com>
> ---
> Changes from V1:
>   - This is new change in V2.
>
>   arch/arm/boot/dts/db8500.dtsi |  128 +++++++++++++++++++++++++++++++----------
>   1 files changed, 97 insertions(+), 31 deletions(-)
>
> diff --git a/arch/arm/boot/dts/db8500.dtsi b/arch/arm/boot/dts/db8500.dtsi
> index 4ad5160..9548f80 100644
> --- a/arch/arm/boot/dts/db8500.dtsi
> +++ b/arch/arm/boot/dts/db8500.dtsi
> @@ -203,107 +203,149 @@
>
>   			db8500-prcmu-regulators {
>   				compatible = "stericsson,db8500-prcmu-regulator";
> +				#address-cells =<1>;
> +				#size-cells =<0>;

Why are these and the reg properties required?

>   				// DB8500_REGULATOR_VAPE
> -				db8500_vape_reg: db8500_vape {
> +				db8500_vape_reg: regulator@0 {
> +					reg =<0>;
> +					regulator-compatible = "db8500_vape";
>   					regulator-name = "db8500-vape";
>   					regulator-always-on;
>   				};

-- 
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 V2 3/3] ARM: dts: db8500: add node property "regulator-compatible" regulator node

[Stephen Warren]

On 06/19/2012 10:13 AM, Lee Jones wrote:
> On 19/06/12 15:28, 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.
>> Add the property "regulator-compatible" for each regulator with
>> their name.
>>
>> Signed-off-by: Laxman Dewangan<ldewangan@nvidia.com>
>> ---
>> Changes from V1:
>>   - This is new change in V2.
>>
>>   arch/arm/boot/dts/db8500.dtsi |  128
>> +++++++++++++++++++++++++++++++----------
>>   1 files changed, 97 insertions(+), 31 deletions(-)
>>
>> diff --git a/arch/arm/boot/dts/db8500.dtsi
>> b/arch/arm/boot/dts/db8500.dtsi
>> index 4ad5160..9548f80 100644
>> --- a/arch/arm/boot/dts/db8500.dtsi
>> +++ b/arch/arm/boot/dts/db8500.dtsi
>> @@ -203,107 +203,149 @@
>>
>>               db8500-prcmu-regulators {
>>                   compatible = "stericsson,db8500-prcmu-regulator";
>> +                #address-cells =<1>;
>> +                #size-cells =<0>;
> 
> Why are these and the reg properties required?

DT nodes should be named after the type of object they describe (e.g.
"regulator") rather than the name of the object they're describing (e.g.
"vape").

Once you've made that change, you end up with many nodes with the same
name in the same parent, so you need to make their names unique. You do
this by adding a "unit address" to each of them - "@0", "@1", ... But,
in order to be "allowed" to use such a unit address, you need a reg
property that matches the unit address, and #address-cells/#size-cells
in the parent node.

>>                   // DB8500_REGULATOR_VAPE
>> -                db8500_vape_reg: db8500_vape {
>> +                db8500_vape_reg: regulator@0 {
>> +                    reg =<0>;

Laxman, you need a space after the = sign for all these added reg
properties, and #address-cells/#size-cells above.

>> +                    regulator-compatible = "db8500_vape";
>>                       regulator-name = "db8500-vape";
>>                       regulator-always-on;
>>                   };

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

[Lee Jones]

On 19/06/12 18:32, Stephen Warren wrote:
> On 06/19/2012 10:13 AM, Lee Jones wrote:
>> >  On 19/06/12 15:28, 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.
>>> >>  Add the property "regulator-compatible" for each regulator with
>>> >>  their name.
>>> >>
>>> >>  Signed-off-by: Laxman Dewangan<ldewangan@nvidia.com>
>>> >>  ---
>>> >>  Changes from V1:
>>> >>     - This is new change in V2.
>>> >>
>>> >>     arch/arm/boot/dts/db8500.dtsi |  128
>>> >>  +++++++++++++++++++++++++++++++----------
>>> >>     1 files changed, 97 insertions(+), 31 deletions(-)
>>> >>
>>> >>  diff --git a/arch/arm/boot/dts/db8500.dtsi
>>> >>  b/arch/arm/boot/dts/db8500.dtsi
>>> >>  index 4ad5160..9548f80 100644
>>> >>  --- a/arch/arm/boot/dts/db8500.dtsi
>>> >>  +++ b/arch/arm/boot/dts/db8500.dtsi
>>> >>  @@ -203,107 +203,149 @@
>>> >>
>>> >>                 db8500-prcmu-regulators {
>>> >>                     compatible = "stericsson,db8500-prcmu-regulator";
>>> >>  +                #address-cells =<1>;
>>> >>  +                #size-cells =<0>;
>> >
>> >  Why are these and the reg properties required?
> DT nodes should be named after the type of object they describe (e.g.
> "regulator") rather than the name of the object they're describing (e.g.
> "vape").
>
> Once you've made that change, you end up with many nodes with the same
> name in the same parent, so you need to make their names unique. You do
> this by adding a "unit address" to each of them - "@0", "@1", ... But,
> in order to be "allowed" to use such a unit address, you need a reg
> property that matches the unit address, and #address-cells/#size-cells
> in the parent node.

I don't like it. By doing this you are preventing any regulator from 
being registered by of_platform_populate(). Also, the nodes are already 
placed under an identifying node "db8500-prcmu-regulators", so we know 
they are regulators, making the regulator@x, the reg property and the 
*-cells properties unnecessary cruft.

I'd prefer to have the second label removed and just to call the 
regulators by their correct name. The property names become functionally 
redundant after the previous patch has been applied in any case.

Something like this:

>  			db8500-prcmu-regulators {
>  				compatible = "stericsson,db8500-prcmu-regulator";
>
>  				// DB8500_REGULATOR_VAPE
> -				db8500_vape_reg: db8500_vape {
> +				db8500_vape {
> +					regulator-compatible = "db8500_vape";
>  					regulator-name = "db8500-vape";
>  					regulator-always-on;
>  				};

It's also a shame we can't do anything about the regulator-name, or 
regulator-compatible property naming conventions. They are almost always 
going to be either extremely similar or even the same. Seems like a bit 
of a wasted property to me at this point.

-- 
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 V2 3/3] ARM: dts: db8500: add node property "regulator-compatible" regulator node

[Laxman Dewangan]

On Wednesday 20 June 2012 12:39 PM, Lee Jones wrote:
> On 19/06/12 18:32, Stephen Warren wrote:
>> On 06/19/2012 10:13 AM, Lee Jones wrote:
>>>>   On 19/06/12 15:28, 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.
>>>>>>   Add the property "regulator-compatible" for each regulator with
>>>>>>   their name.
>>>>>>
>>>>>>   Signed-off-by: Laxman Dewangan<ldewangan@nvidia.com>
>>>>>>   ---
>>>>>>   Changes from V1:
>>>>>>      - This is new change in V2.
>>>>>>
>>>>>>      arch/arm/boot/dts/db8500.dtsi |  128
>>>>>>   +++++++++++++++++++++++++++++++----------
>>>>>>      1 files changed, 97 insertions(+), 31 deletions(-)
>>>>>>
>>>>>>   diff --git a/arch/arm/boot/dts/db8500.dtsi
>>>>>>   b/arch/arm/boot/dts/db8500.dtsi
>>>>>>   index 4ad5160..9548f80 100644
>>>>>>   --- a/arch/arm/boot/dts/db8500.dtsi
>>>>>>   +++ b/arch/arm/boot/dts/db8500.dtsi
>>>>>>   @@ -203,107 +203,149 @@
>>>>>>
>>>>>>                  db8500-prcmu-regulators {
>>>>>>                      compatible = "stericsson,db8500-prcmu-regulator";
>>>>>>   +                #address-cells =<1>;
>>>>>>   +                #size-cells =<0>;
>>>>   Why are these and the reg properties required?
>> DT nodes should be named after the type of object they describe (e.g.
>> "regulator") rather than the name of the object they're describing (e.g.
>> "vape").
>>
>> Once you've made that change, you end up with many nodes with the same
>> name in the same parent, so you need to make their names unique. You do
>> this by adding a "unit address" to each of them - "@0", "@1", ... But,
>> in order to be "allowed" to use such a unit address, you need a reg
>> property that matches the unit address, and #address-cells/#size-cells
>> in the parent node.
> I don't like it. By doing this you are preventing any regulator from
> being registered by of_platform_populate(). Also, the nodes are already
> placed under an identifying node "db8500-prcmu-regulators", so we know
> they are regulators, making the regulator@x, the reg property and the
> *-cells properties unnecessary cruft.
>
> I'd prefer to have the second label removed and just to call the
> regulators by their correct name. The property names become functionally
> redundant after the previous patch has been applied in any case.
>
> Something like this:
>
>>   			db8500-prcmu-regulators {
>>   				compatible = "stericsson,db8500-prcmu-regulator";
>>
>>   				// DB8500_REGULATOR_VAPE
>> -				db8500_vape_reg: db8500_vape {
>> +				db8500_vape {
>> +					regulator-compatible = "db8500_vape";
>>   					regulator-name = "db8500-vape";
>>   					regulator-always-on;
>>   				};
>


You will require a label so that it can refer by the consumer.

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

[Lee Jones]

On 20/06/12 08:39, Laxman Dewangan wrote:
> On Wednesday 20 June 2012 12:39 PM, Lee Jones wrote:
>> On 19/06/12 18:32, Stephen Warren wrote:
>>> On 06/19/2012 10:13 AM, Lee Jones wrote:
>>>>> On 19/06/12 15:28, 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.
>>>>>>> Add the property "regulator-compatible" for each regulator with
>>>>>>> their name.
>>>>>>>
>>>>>>> Signed-off-by: Laxman Dewangan<ldewangan@nvidia.com>
>>>>>>> ---
>>>>>>> Changes from V1:
>>>>>>> - This is new change in V2.
>>>>>>>
>>>>>>> arch/arm/boot/dts/db8500.dtsi | 128
>>>>>>> +++++++++++++++++++++++++++++++----------
>>>>>>> 1 files changed, 97 insertions(+), 31 deletions(-)
>>>>>>>
>>>>>>> diff --git a/arch/arm/boot/dts/db8500.dtsi
>>>>>>> b/arch/arm/boot/dts/db8500.dtsi
>>>>>>> index 4ad5160..9548f80 100644
>>>>>>> --- a/arch/arm/boot/dts/db8500.dtsi
>>>>>>> +++ b/arch/arm/boot/dts/db8500.dtsi
>>>>>>> @@ -203,107 +203,149 @@
>>>>>>>
>>>>>>> db8500-prcmu-regulators {
>>>>>>> compatible = "stericsson,db8500-prcmu-regulator";
>>>>>>> + #address-cells =<1>;
>>>>>>> + #size-cells =<0>;
>>>>> Why are these and the reg properties required?
>>> DT nodes should be named after the type of object they describe (e.g.
>>> "regulator") rather than the name of the object they're describing (e.g.
>>> "vape").
>>>
>>> Once you've made that change, you end up with many nodes with the same
>>> name in the same parent, so you need to make their names unique. You do
>>> this by adding a "unit address" to each of them - "@0", "@1", ... But,
>>> in order to be "allowed" to use such a unit address, you need a reg
>>> property that matches the unit address, and #address-cells/#size-cells
>>> in the parent node.
>> I don't like it. By doing this you are preventing any regulator from
>> being registered by of_platform_populate(). Also, the nodes are already
>> placed under an identifying node "db8500-prcmu-regulators", so we know
>> they are regulators, making the regulator@x, the reg property and the
>> *-cells properties unnecessary cruft.
>>
>> I'd prefer to have the second label removed and just to call the
>> regulators by their correct name. The property names become functionally
>> redundant after the previous patch has been applied in any case.
>>
>> Something like this:
>>
>>> db8500-prcmu-regulators {
>>> compatible = "stericsson,db8500-prcmu-regulator";
>>>
>>> // DB8500_REGULATOR_VAPE
>>> - db8500_vape_reg: db8500_vape {
>>> + db8500_vape {
>>> + regulator-compatible = "db8500_vape";
>>> regulator-name = "db8500-vape";
>>> regulator-always-on;
>>> };
>>
>
>
> You will require a label so that it can refer by the consumer.

Don't they both act as labels? Thus if you removed the second one, the 
phandle will be taken from the remaining label? It's not something I've 
tried, so I'm happy to be wrong here.

If I'm wrong about that, can't we just omit the reg and *-size 
properties? They are meaningless and restrictive.

-- 
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 V2 3/3] ARM: dts: db8500: add node property "regulator-compatible" regulator node

[Laxman Dewangan]

On Wednesday 20 June 2012 01:31 PM, Lee Jones wrote:
> On 20/06/12 08:39, Laxman Dewangan wrote:
>> On Wednesday 20 June 2012 12:39 PM, Lee Jones wrote:
>>> On 19/06/12 18:32, Stephen Warren wrote:
>>>> On 06/19/2012 10:13 AM, Lee Jones wrote:
>>>>>> On 19/06/12 15:28, 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.
>>>>>>>> Add the property "regulator-compatible" for each regulator with
>>>>>>>> their name.
>>>>>>>>
>>>>>>>> Signed-off-by: Laxman Dewangan<ldewangan@nvidia.com>
>>>>>>>> ---
>>>>>>>> Changes from V1:
>>>>>>>> - This is new change in V2.
>>>>>>>>
>>>>>>>> arch/arm/boot/dts/db8500.dtsi | 128
>>>>>>>> +++++++++++++++++++++++++++++++----------
>>>>>>>> 1 files changed, 97 insertions(+), 31 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/arch/arm/boot/dts/db8500.dtsi
>>>>>>>> b/arch/arm/boot/dts/db8500.dtsi
>>>>>>>> index 4ad5160..9548f80 100644
>>>>>>>> --- a/arch/arm/boot/dts/db8500.dtsi
>>>>>>>> +++ b/arch/arm/boot/dts/db8500.dtsi
>>>>>>>> @@ -203,107 +203,149 @@
>>>>>>>>
>>>>>>>> db8500-prcmu-regulators {
>>>>>>>> compatible = "stericsson,db8500-prcmu-regulator";
>>>>>>>> + #address-cells =<1>;
>>>>>>>> + #size-cells =<0>;
>>>>>> Why are these and the reg properties required?
>>>> DT nodes should be named after the type of object they describe (e.g.
>>>> "regulator") rather than the name of the object they're describing (e.g.
>>>> "vape").
>>>>
>>>> Once you've made that change, you end up with many nodes with the same
>>>> name in the same parent, so you need to make their names unique. You do
>>>> this by adding a "unit address" to each of them - "@0", "@1", ... But,
>>>> in order to be "allowed" to use such a unit address, you need a reg
>>>> property that matches the unit address, and #address-cells/#size-cells
>>>> in the parent node.
>>> I don't like it. By doing this you are preventing any regulator from
>>> being registered by of_platform_populate(). Also, the nodes are already
>>> placed under an identifying node "db8500-prcmu-regulators", so we know
>>> they are regulators, making the regulator@x, the reg property and the
>>> *-cells properties unnecessary cruft.
>>>
>>> I'd prefer to have the second label removed and just to call the
>>> regulators by their correct name. The property names become functionally
>>> redundant after the previous patch has been applied in any case.
>>>
>>> Something like this:
>>>
>>>> db8500-prcmu-regulators {
>>>> compatible = "stericsson,db8500-prcmu-regulator";
>>>>
>>>> // DB8500_REGULATOR_VAPE
>>>> - db8500_vape_reg: db8500_vape {
>>>> + db8500_vape {
>>>> + regulator-compatible = "db8500_vape";
>>>> regulator-name = "db8500-vape";
>>>> regulator-always-on;
>>>> };
>>
>> You will require a label so that it can refer by the consumer.
> Don't they both act as labels? Thus if you removed the second one, the
> phandle will be taken from the remaining label? It's not something I've
> tried, so I'm happy to be wrong here.
>
> If I'm wrong about that, can't we just omit the reg and *-size
> properties? They are meaningless and restrictive.
>

We need to have the label. The name can not act as label. I tried 
following and got compilation error for dts file.
Tried following way

                                 reg_vdd1 {
                                         regulator-compatible = "vdd1";
                                         :::::::::::::::::
                                 };

                                 reg_vdd2: regulator@1 {
                                         regulator-compatible = "vdd2";
                                         :::::::::::::::::
                                 };

                                 reg_vddctrl: regulator@2 {
                                         regulator-compatible = "vddctrl";
                                         :::::::::::::::::
                                         vin-supply = <&reg_vdd1>;
                                 };

And got build error as
**********
DTC: dts->dtb  on file "arch/arm/boot/dts/tegra30-cardhu.dts"
ERROR (phandle_references): Reference to non-existent node or label 
"reg_vdd1"

ERROR: Input tree has errors, aborting (use -f to force output)
*******

Are you OK to have the first patch with adding property 
"regulator-compatible" on each of child node so that I can go ahead with 
this patch and regulator core/documentation patch along with changes in 
my board to enable regulators.
Once we will conclude on the child name either like vdd1 or regulator@0, 
we can have modification accordingly.

This will also need to avoid bi-sect issue as Stephen's suggested
patch 1: Add regulator-property on each child node of db8500.
patch 2: modify the regulator/core and documentation.

Patch3 and onwards: Based on discussion, name the child node.

Patch1 and 2 will not break anything and with this I can enable 
regulator on my boards.


Thanks,
Laxman

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

[Lee Jones]

On 20/06/12 09:19, Laxman Dewangan wrote:
> On Wednesday 20 June 2012 01:31 PM, Lee Jones wrote:
>> On 20/06/12 08:39, Laxman Dewangan wrote:
>>> On Wednesday 20 June 2012 12:39 PM, Lee Jones wrote:
>>>> On 19/06/12 18:32, Stephen Warren wrote:
>>>>> On 06/19/2012 10:13 AM, Lee Jones wrote:
>>>>>>> On 19/06/12 15:28, 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.
>>>>>>>>> Add the property "regulator-compatible" for each regulator with
>>>>>>>>> their name.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Laxman Dewangan<ldewangan@nvidia.com>
>>>>>>>>> ---
>>>>>>>>> Changes from V1:
>>>>>>>>> - This is new change in V2.
>>>>>>>>>
>>>>>>>>> arch/arm/boot/dts/db8500.dtsi | 128
>>>>>>>>> +++++++++++++++++++++++++++++++----------
>>>>>>>>> 1 files changed, 97 insertions(+), 31 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/arch/arm/boot/dts/db8500.dtsi
>>>>>>>>> b/arch/arm/boot/dts/db8500.dtsi
>>>>>>>>> index 4ad5160..9548f80 100644
>>>>>>>>> --- a/arch/arm/boot/dts/db8500.dtsi
>>>>>>>>> +++ b/arch/arm/boot/dts/db8500.dtsi
>>>>>>>>> @@ -203,107 +203,149 @@
>>>>>>>>>
>>>>>>>>> db8500-prcmu-regulators {
>>>>>>>>> compatible = "stericsson,db8500-prcmu-regulator";
>>>>>>>>> + #address-cells =<1>;
>>>>>>>>> + #size-cells =<0>;
>>>>>>> Why are these and the reg properties required?
>>>>> DT nodes should be named after the type of object they describe (e.g.
>>>>> "regulator") rather than the name of the object they're describing
>>>>> (e.g.
>>>>> "vape").
>>>>>
>>>>> Once you've made that change, you end up with many nodes with the same
>>>>> name in the same parent, so you need to make their names unique.
>>>>> You do
>>>>> this by adding a "unit address" to each of them - "@0", "@1", ... But,
>>>>> in order to be "allowed" to use such a unit address, you need a reg
>>>>> property that matches the unit address, and #address-cells/#size-cells
>>>>> in the parent node.
>>>> I don't like it. By doing this you are preventing any regulator from
>>>> being registered by of_platform_populate(). Also, the nodes are already
>>>> placed under an identifying node "db8500-prcmu-regulators", so we know
>>>> they are regulators, making the regulator@x, the reg property and the
>>>> *-cells properties unnecessary cruft.
>>>>
>>>> I'd prefer to have the second label removed and just to call the
>>>> regulators by their correct name. The property names become
>>>> functionally
>>>> redundant after the previous patch has been applied in any case.
>>>>
>>>> Something like this:
>>>>
>>>>> db8500-prcmu-regulators {
>>>>> compatible = "stericsson,db8500-prcmu-regulator";
>>>>>
>>>>> // DB8500_REGULATOR_VAPE
>>>>> - db8500_vape_reg: db8500_vape {
>>>>> + db8500_vape {
>>>>> + regulator-compatible = "db8500_vape";
>>>>> regulator-name = "db8500-vape";
>>>>> regulator-always-on;
>>>>> };
>>>
>>> You will require a label so that it can refer by the consumer.
>> Don't they both act as labels? Thus if you removed the second one, the
>> phandle will be taken from the remaining label? It's not something I've
>> tried, so I'm happy to be wrong here.
>>
>> If I'm wrong about that, can't we just omit the reg and *-size
>> properties? They are meaningless and restrictive.
>>
>
> We need to have the label. The name can not act as label. I tried
> following and got compilation error for dts file.
> Tried following way
>
> reg_vdd1 {
> regulator-compatible = "vdd1";
> :::::::::::::::::
> };
>
> reg_vdd2: regulator@1 {
> regulator-compatible = "vdd2";
> :::::::::::::::::
> };
>
> reg_vddctrl: regulator@2 {
> regulator-compatible = "vddctrl";
> :::::::::::::::::
> vin-supply = <&reg_vdd1>;
> };
>
> And got build error as
> **********
> DTC: dts->dtb on file "arch/arm/boot/dts/tegra30-cardhu.dts"
> ERROR (phandle_references): Reference to non-existent node or label
> "reg_vdd1"
>
> ERROR: Input tree has errors, aborting (use -f to force output)
> *******

Yes, I just did the same experiment. Shame. :(

> Are you OK to have the first patch with adding property
> "regulator-compatible" on each of child node so that I can go ahead with
> this patch and regulator core/documentation patch along with changes in
> my board to enable regulators.
> Once we will conclude on the child name either like vdd1 or regulator@0,
> we can have modification accordingly.

Yes I'm fine with it.

It's only the unnecessary reg and *-size properties I'm opposed to.

-- 
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 V2 3/3] ARM: dts: db8500: add node property "regulator-compatible" regulator node

[Mark Brown]

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

On Wed, Jun 20, 2012 at 09:56:22AM +0100, Lee Jones wrote:

> >Are you OK to have the first patch with adding property

...

> Yes I'm fine with it.

So is that an ack?

> It's only the unnecessary reg and *-size properties I'm opposed to.

My understanding is that they're required by DT policies in the same way
that some of the other verbiage is.

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

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

[Lee Jones]

On 20/06/12 11:06, Mark Brown wrote:
> On Wed, Jun 20, 2012 at 09:56:22AM +0100, Lee Jones wrote:
>
>>> Are you OK to have the first patch with adding property
>
> ...
>
>> Yes I'm fine with it.
>
> So is that an ack?

Sure why not. Although, Linus Walleij has the final say.

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

>> It's only the unnecessary reg and *-size properties I'm opposed to.
>
> My understanding is that they're required by DT policies in the same way
> that some of the other verbiage is.

Well I haven't put them in the DB8500 Device Tree if they're not useful 
and nothing untoward has occurred.

-- 
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 V2 3/3] ARM: dts: db8500: add node property "regulator-compatible" regulator node

[Laxman Dewangan]

On Wednesday 20 June 2012 04:55 PM, Lee Jones wrote:
> On 20/06/12 11:06, Mark Brown wrote:
>> On Wed, Jun 20, 2012 at 09:56:22AM +0100, Lee Jones wrote:
>>
>>>> Are you OK to have the first patch with adding property
>> ...
>>
>>> Yes I'm fine with it.
>> So is that an ack?
> Sure why not. Although, Linus Walleij has the final say.
>
> Acked-by: Lee Jones<lee.jones@linaro.org>
>

I am going to send the new patch having "regulator-compatible" on each 
child node only for db8500.dtsi, and removing all other changes.

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

[Lee Jones]

On 20/06/12 12:27, Laxman Dewangan wrote:
> On Wednesday 20 June 2012 04:55 PM, Lee Jones wrote:
>> On 20/06/12 11:06, Mark Brown wrote:
>>> On Wed, Jun 20, 2012 at 09:56:22AM +0100, Lee Jones wrote:
>>>
>>>>> Are you OK to have the first patch with adding property
>>> ...
>>>
>>>> Yes I'm fine with it.
>>> So is that an ack?
>> Sure why not. Although, Linus Walleij has the final say.
>>
>> Acked-by: Lee Jones<lee.jones@linaro.org>
>>
>
> I am going to send the new patch having "regulator-compatible" on each
> child node only for db8500.dtsi, and removing all other changes.

That's fine. Keep me CC'ed on other related correspondence and I'll make 
any other necessary changes with regards to node naming at a later date.

-- 
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 V2 3/3] ARM: dts: db8500: add node property "regulator-compatible" regulator node

[Stephen Warren]

On 06/20/2012 01:09 AM, Lee Jones wrote:
> On 19/06/12 18:32, Stephen Warren wrote:
>> On 06/19/2012 10:13 AM, Lee Jones wrote:
>>> >  On 19/06/12 15:28, 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.
>>>> >>  Add the property "regulator-compatible" for each regulator with
>>>> >>  their name.
>>>> >>
>>>> >>  Signed-off-by: Laxman Dewangan<ldewangan@nvidia.com>
>>>> >>  ---
>>>> >>  Changes from V1:
>>>> >>     - This is new change in V2.
>>>> >>
>>>> >>     arch/arm/boot/dts/db8500.dtsi |  128
>>>> >>  +++++++++++++++++++++++++++++++----------
>>>> >>     1 files changed, 97 insertions(+), 31 deletions(-)
>>>> >>
>>>> >>  diff --git a/arch/arm/boot/dts/db8500.dtsi
>>>> >>  b/arch/arm/boot/dts/db8500.dtsi
>>>> >>  index 4ad5160..9548f80 100644
>>>> >>  --- a/arch/arm/boot/dts/db8500.dtsi
>>>> >>  +++ b/arch/arm/boot/dts/db8500.dtsi
>>>> >>  @@ -203,107 +203,149 @@
>>>> >>
>>>> >>                 db8500-prcmu-regulators {
>>>> >>                     compatible =
>>>> "stericsson,db8500-prcmu-regulator";
>>>> >>  +                #address-cells =<1>;
>>>> >>  +                #size-cells =<0>;
>>> >
>>> >  Why are these and the reg properties required?
>>
>> DT nodes should be named after the type of object they describe (e.g.
>> "regulator") rather than the name of the object they're describing (e.g.
>> "vape").
>>
>> Once you've made that change, you end up with many nodes with the same
>> name in the same parent, so you need to make their names unique. You do
>> this by adding a "unit address" to each of them - "@0", "@1", ... But,
>> in order to be "allowed" to use such a unit address, you need a reg
>> property that matches the unit address, and #address-cells/#size-cells
>> in the parent node.
> 
> I don't like it. By doing this you are preventing any regulator from
> being registered by of_platform_populate().

That isn't true.

The binding for a regulator chip can choose to either use one of two
schemes to identify which regulator the child nodes apply to:

1) Use the regulator-compatible property, and the Linux driver would use
of_regulator_match() to parse the DT.

2) Use the compatible property, and the Linux driver would use
of_platform_populate().

Now, the binding does need to choose the identification scheme up-front,
but simply so that there's a single way to write the binding for the
device; nothing to do with the driver code to parse it.

Note that the way the existing db8500.dtsi regulator binding is defined
already has chosen option (1) above, since there's no compatible
property within each regulator node.

> Also, the nodes are already
> placed under an identifying node "db8500-prcmu-regulators", so we know
> they are regulators, making the regulator@x, the reg property and the
> *-cells properties unnecessary cruft.

What we "know" isn't the issue here; DT is defined such that node names
are supposed to encode the class of device. That's a rule that was
accidentally overlooked here, and we're hoping to correct that.

> I'd prefer to have the second label removed and just to call the
> regulators by their correct name. The property names become functionally
> redundant after the previous patch has been applied in any case.
> 
> Something like this:
> 
>>              db8500-prcmu-regulators {
>>                  compatible = "stericsson,db8500-prcmu-regulator";
>>
>>                  // DB8500_REGULATOR_VAPE
>> -                db8500_vape_reg: db8500_vape {
>> +                db8500_vape {
>> +                    regulator-compatible = "db8500_vape";
>>                      regulator-name = "db8500-vape";
>>                      regulator-always-on;
>>                  };

The node name isn't a label. However, you can in fact remove the label
if you want. Instead of referencing the label in the client node:

	foo = <&db8500_vape_reg ...>;

you can reference the path to the node:

	foo = <&{/soc-u9500/prcmu@80157000/db8500-prcmu-regulators\
/db8500_vape}>;

However, that's quite unwieldy, hence why labels are typically used to
provide a more succinct name. Also, when the nodes are named according
the to DT rules, the last name component is something like
"regulator@0", so isn't very readable.

> It's also a shame we can't do anything about the regulator-name, or
> regulator-compatible property naming conventions. They are almost always
> going to be either extremely similar or even the same. Seems like a bit
> of a wasted property to me at this point.

The two properties name different things:

regulator-compatible names the regulator HW unit within the chip. This
should be invariant across all boards that use this chip; it's something
defined internally to the chip and/or the chip's
documentation/specification. An example might be "LDO1".

regulator-name defines what the regulator is used for in a particular
scenario. This may be the signal name on a particular board's schematic
or similar function name. Examples might be "SDMMC3_VDDIO" or "SD slot
power".

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

[Stephen Warren]

On 06/19/2012 08:28 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.
> Add the property "regulator-compatible" for each regulator with
> their name.

In order to prevent "git bisect" failures, don't you need to squash this
patch into patch 1/3, so that there is no point in git history where the
code expects the regulator-compatible property to exist, yet the .dtsi
file doesn't have it?

Or you could do this:

patch 1: add register-compatible property to db8500.dtsi
patch 2: adjust the code to use register-compatible property
patch 3: rename nodes in db8500.dtsi

but I'm not sure there's any advantage to having multiple patches
(except for patch statistics counters, but we certainly don't want to
have legitimate allegations of patch padding!)

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

[Linus Walleij]

On Tue, Jun 19, 2012 at 4:28 PM, Laxman Dewangan <ldewangan@nvidia.com> wrote:

> 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.

If I had today deployed the device trees generated prior to this patch,
and try to boot kernels after this patch with the same device tree,
would they still work as expected?

I don't know it it's an issue right now, but at some point we need to
realize that people will expect old device trees to work on newer
kernels.

Yours,
Linus Walleij

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

[Mark Brown]

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

On Wed, Jun 20, 2012 at 10:59:32AM +0200, Linus Walleij wrote:

> If I had today deployed the device trees generated prior to this patch,
> and try to boot kernels after this patch with the same device tree,
> would they still work as expected?

> I don't know it it's an issue right now, but at some point we need to
> realize that people will expect old device trees to work on newer
> kernels.

I don't think anyone's taking that idea terribly seriously right now for
ARM, lots of the core SoC bindings are still in flux or not there.

Even for PowerPC where this has apparently been achieved I've still had
to push back on new mandatory properties being added :/

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

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

[Stephen Warren]

On 06/20/2012 04:00 AM, Mark Brown wrote:
> On Wed, Jun 20, 2012 at 10:59:32AM +0200, Linus Walleij wrote:
> 
>> If I had today deployed the device trees generated prior to this
>> patch, and try to boot kernels after this patch with the same
>> device tree, would they still work as expected?
> 
>> I don't know it it's an issue right now, but at some point we
>> need to realize that people will expect old device trees to work
>> on newer kernels.
> 
> I don't think anyone's taking that idea terribly seriously right
> now for ARM, lots of the core SoC bindings are still in flux or not
> there.
> 
> Even for PowerPC where this has apparently been achieved I've still
> had to push back on new mandatory properties being added :/

For Tegra bindings at least, I had historically tried to be extremely
hard-and-fast on binding changes, based on this compatibility requirement.

However, at Linaro Connect Q1 I think, Grant made the point that he
considers the current binding definitions a work-in-progress in
general since as you say, everything is so new. So, I've backed off on
the 100% compatibility requirement until things are more solidified.
It's better to fix broken stuff that force any historical binding bugs
not be fixed.

Related to this, I wonder if we need some scheme to tag individual
bindings as under-development vs. final, and some way to promote
bindings between those states (a bindings review board?!). Do binding
definitions need versions? This perhaps might be coupled with the
binding documentation (even .dts files?) moving out of the kernel tree
to some other repository. To be honest, until/unless the source and
.dts files or binding docs are separated, it's slightly hard to argue
that the bindings ever need to be static, since the idea appears to be
to always use the .dts file that shipped with the kernel you built,
and upgrade them both at once.

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

[Linus Walleij]

On Wed, Jun 20, 2012 at 6:23 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:

> Related to this, I wonder if we need some scheme to tag individual
> bindings as under-development vs. final, and some way to promote
> bindings between those states (a bindings review board?!).

Hm, we just have Documentation/devicetree/bindings and that's it.

But compare to the Documentation/ABI:
$ ls
obsolete  README  removed  stable  testing

I think we might want to mirror this structure for the
bindings, putting all current bindings into testing.

That said, this scheme has not always been very strictly
enforced on sysfs etc. But the ambition is there.

Yours,
Linus Walleij

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

[Mark Brown]

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

On Wed, Jun 20, 2012 at 10:23:52AM -0600, Stephen Warren wrote:

> Related to this, I wonder if we need some scheme to tag individual
> bindings as under-development vs. final, and some way to promote
> bindings between those states (a bindings review board?!). Do binding

Part of the trouble with this, and the reason why it doesn't really work
for sysfs, is that people generally don't think there's a problem with
the bindings until they run into it or until someone new comes along and
reviews them.  Apart from the OMAP hwmod thing generally we aren't doing
anything that's not supposed to last.

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