[PATCH 0/2] regulator: tps65910: dt: identify regulator by prop regulator-compatible.

[PATCH 0/2] regulator: tps65910: dt: identify regulator by prop regulator-compatible.

[Laxman Dewangan]

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

adding the  property "regulator-compatible" for each regulator of device.
Modifying documentation as well to reflect this change.
Adding common API in regulators/of_regualator to match regulator
by their id. 

Laxman Dewangan (2):
  regulator: support for regulator match by regulator-compatible
  regulator: tps65910: dt: identify regulator by regulator-compatible

 Documentation/devicetree/bindings/mfd/tps65910.txt |   54 +++++++++++++----
 drivers/regulator/of_regulator.c                   |   61 ++++++++++++++++++++
 drivers/regulator/tps65910-regulator.c             |    3 +-
 include/linux/regulator/of_regulator.h             |   13 ++++
 4 files changed, 117 insertions(+), 14 deletions(-)

[PATCH 1/2] regulator: support for regulator match by regulator-compatible

[Laxman Dewangan]

Add API to match the chip regulator's id by the
property of "regulator-compatible" on regulator node.

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
---
 drivers/regulator/of_regulator.c       |   61 ++++++++++++++++++++++++++++++++
 include/linux/regulator/of_regulator.h |   13 +++++++
 2 files changed, 74 insertions(+), 0 deletions(-)

diff --git a/drivers/regulator/of_regulator.c b/drivers/regulator/of_regulator.c
index e2a7310..3dc9be1 100644
--- a/drivers/regulator/of_regulator.c
+++ b/drivers/regulator/of_regulator.c
@@ -136,3 +136,64 @@ int of_regulator_match(struct device *dev, struct device_node *node,
 	return count;
 }
 EXPORT_SYMBOL_GPL(of_regulator_match);
+
+/**
+ * of_regulator_match_by_compatible - extract regulator init data when
+ * regulator-compatible matches.
+ * @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 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.
+ */
+int of_regulator_match_by_compatible(struct device *dev,
+			struct device_node *node,
+			struct of_regulator_match *matches,
+			unsigned int num_matches)
+{
+	unsigned int count = 0;
+	unsigned int i;
+	const char *regulator_comp;
+	struct device_node *child;
+
+	if (!dev || !node)
+		return -EINVAL;
+
+	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);
+			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++;
+		}
+	}
+
+	return count;
+}
+EXPORT_SYMBOL_GPL(of_regulator_match_by_compatible);
diff --git a/include/linux/regulator/of_regulator.h b/include/linux/regulator/of_regulator.h
index f921796..e3469a0 100644
--- a/include/linux/regulator/of_regulator.h
+++ b/include/linux/regulator/of_regulator.h
@@ -20,6 +20,11 @@ extern struct regulator_init_data
 extern int of_regulator_match(struct device *dev, struct device_node *node,
 			      struct of_regulator_match *matches,
 			      unsigned int num_matches);
+extern int of_regulator_match_by_compatible(struct device *dev,
+			struct device_node *node,
+			struct of_regulator_match *matches,
+			unsigned int num_matches);
+
 #else
 static inline struct regulator_init_data
 	*of_get_regulator_init_data(struct device *dev,
@@ -35,6 +40,14 @@ static inline int of_regulator_match(struct device *dev,
 {
 	return 0;
 }
+
+static inline int of_regulator_match_by_compatible(struct device *dev,
+			struct device_node *node,
+			struct of_regulator_match *matches,
+			unsigned int num_matches)
+{
+	return 0;
+}
 #endif /* CONFIG_OF */
 
 #endif /* __LINUX_OF_REG_H */
-- 
1.7.1.1

Re: [PATCH 1/2] regulator: support for regulator match by regulator-compatible

[Mark Brown]

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

On Tue, Jun 19, 2012 at 11:21:26AM +0530, Laxman Dewangan wrote:
> Add API to match the chip regulator's id by the
> property of "regulator-compatible" on regulator node.

We shouldn't have two different methods for doing this, we should
standardise this over all regulator drivers (or at the very least those
using the existing framework code) - having the old framework in place
will at best be confusing for authors of new drivers.  We either need to
make it clear which framework to use or (ideally) remove the old
framework.

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

Re: [PATCH 1/2] regulator: support for regulator match by regulator-compatible

[Laxman Dewangan]

On Tuesday 19 June 2012 03:02 PM, Mark Brown wrote:
> * PGP Signed by an unknown key
>
> On Tue, Jun 19, 2012 at 11:21:26AM +0530, Laxman Dewangan wrote:
>> Add API to match the chip regulator's id by the
>> property of "regulator-compatible" on regulator node.
> We shouldn't have two different methods for doing this, we should
> standardise this over all regulator drivers (or at the very least those
> using the existing framework code) - having the old framework in place
> will at best be confusing for authors of new drivers.  We either need to
> make it clear which framework to use or (ideally) remove the old
> framework.

Agree. Currently following regulators are using this (based on grep)
mfd/tps6586x.c
regulator/ab8500.c
regulator/db8500-prcmu.c
and regulator/tps65910.

If the related change in tps65910 is fine and there is no more 
concern/feedback on the approach then I can create a series of patch to 
modify the other regulator and related dts file to follow the same.

If you want, I can make the new changes as part of this series or can 
start with new series.

I like to go with new series patch for other change once this is accepted.

Re: [PATCH 1/2] regulator: support for regulator match by regulator-compatible

[Mark Brown]

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

On Tue, Jun 19, 2012 at 03:04:00PM +0530, Laxman Dewangan wrote:

> Agree. Currently following regulators are using this (based on grep)
> mfd/tps6586x.c
> regulator/ab8500.c
> regulator/db8500-prcmu.c
> and regulator/tps65910.

That looks about right.

> If the related change in tps65910 is fine and there is no more
> concern/feedback on the approach then I can create a series of patch
> to modify the other regulator and related dts file to follow the
> same.

> If you want, I can make the new changes as part of this series or
> can start with new series.

> I like to go with new series patch for other change once this is accepted.

I've no concerns with the interface or the code itself other than the
usability issue with partial deployment so if you could do the updates
for the other drivers that'd be great.  I'll hold off on applying this
until we've got everything else updated.

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

Re: [PATCH 1/2] regulator: support for regulator match by regulator-compatible

[Laxman Dewangan]

On Tuesday 19 June 2012 04:13 PM, Mark Brown wrote:
> * PGP Signed by an unknown key
>
> On Tue, Jun 19, 2012 at 03:04:00PM +0530, Laxman Dewangan wrote:
>
>> I like to go with new series patch for other change once this is accepted.
> I've no concerns with the interface or the code itself other than the
> usability issue with partial deployment so if you could do the updates
> for the other drivers that'd be great.  I'll hold off on applying this
> until we've got everything else updated.

Great. I will send the patch  to reflect this policy for remaining 
regulators.

Re: [PATCH 1/2] regulator: support for regulator match by regulator-compatible

[Laxman Dewangan]

On Tuesday 19 June 2012 04:13 PM, Mark Brown wrote:
> * PGP Signed by an unknown key
>
> On Tue, Jun 19, 2012 at 03:04:00PM +0530, Laxman Dewangan wrote:
>
>> Agree. Currently following regulators are using this (based on grep)
>> mfd/tps6586x.c
>> regulator/ab8500.c
>> regulator/db8500-prcmu.c
>> and regulator/tps65910.
>
> I've no concerns with the interface or the code itself other than the
> usability issue with partial deployment so if you could do the updates
> for the other drivers that'd be great.  I'll hold off on applying this
> until we've got everything else updated.

Mark,
Just thinking on optimizing the changes require, we can do it in 3 
patches only:
- In place of adding new api of_regulator_match_by_compatible(), we can 
modify the existing of_regulator_match() itself  to look for 
regulator-compatible for matching. This can be done in first patch.
- Need not to change the .c files, here just change the documentation of 
these regulator dt binding. All can be done in second patch.
- Change the dts file to have this policy and seeing only on 
arm/boot/dts/db8500.dtsi for ab8500 and db8500-prcmu. So it will be 
third patch .

We will not need any change on above c files as the api is not going to 
change.

Require your opinion.

Re: [PATCH 1/2] regulator: support for regulator match by regulator-compatible

[Mark Brown]

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

On Tue, Jun 19, 2012 at 05:36:09PM +0530, Laxman Dewangan wrote:

> Just thinking on optimizing the changes require, we can do it in 3
> patches only:
> - In place of adding new api of_regulator_match_by_compatible(), we
> can modify the existing of_regulator_match() itself  to look for
> regulator-compatible for matching. This can be done in first patch.
> - Need not to change the .c files, here just change the
> documentation of these regulator dt binding. All can be done in
> second patch.
> - Change the dts file to have this policy and seeing only on
> arm/boot/dts/db8500.dtsi for ab8500 and db8500-prcmu. So it will be
> third patch .

This seems fine.

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

[PATCH 2/2] regulator: tps65910: dt: identify regulator by regulator-compatible

[Laxman Dewangan]

Add the property of "regulator-compatible" for each
regulator to match with the device regulators.

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
---
 Documentation/devicetree/bindings/mfd/tps65910.txt |   54 +++++++++++++++-----
 drivers/regulator/tps65910-regulator.c             |    3 +-
 2 files changed, 43 insertions(+), 14 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/drivers/regulator/tps65910-regulator.c b/drivers/regulator/tps65910-regulator.c
index 6bf864b..c2ee450 100644
--- a/drivers/regulator/tps65910-regulator.c
+++ b/drivers/regulator/tps65910-regulator.c
@@ -1106,7 +1106,8 @@ static struct tps65910_board *tps65910_parse_dt_reg_data(
 		return NULL;
 	}
 
-	ret = of_regulator_match(pdev->dev.parent, regulators, matches, count);
+	ret = of_regulator_match_by_compatible(pdev->dev.parent, regulators,
+			matches, count);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "Error parsing regulator init data: %d\n",
 			ret);
-- 
1.7.1.1