[PATCH v5 0/5] regulator: of: Add initial and suspend modes support

[PATCH v5 0/5] regulator: of: Add initial and suspend modes support

[Javier Martinez Canillas]

Hello Mark,

This is the fifth version of the series that adds regulator initial
and suspend operating modes support. It relies on the existing work
that added suspend states bindings. The opmodes are parsed by the
regulator core and drivers should only define a translation function
to map between hardware specific to standard modes.

The series adds a "regulator-initial-mode" property to configure at
startup, the operating mode for the regulators that support changing
its mode during normal operation and a "regulator-mode" property for
the regulators that supports changing its operating mode when the
system enters in a suspend state. These properties were originally
part of Chanwoo Choi's regulator suspend state series [0] but were
removed since there wasn't a way to define the operating modes in a
generic way.

The generic regulator DT binding doc explains that each device has
to document what their valid operating modes are and drivers must
add a translation function so the core knows how to map the opmodes.

Older versions of this series were meant to add initial and suspend
modes for the max77802 regulator driver but the feedback was that
this should had been done in a generic way. The latest version was
"[PATCH v4 00/14] Add Maxim 77802 PMIC support" [1] but that series
mixed core changes, bugfixes and new driver features.

This series instead contains only the patches that add the support
to the regulator core and drivers are only modified when a function
signature is changed to maintain git bisect-ability.

If the patches are merged, following series will change the drivers
using of_regulator_match() to pass the regulator description in the
match table and another series will add the new opmode feature in
the max77802 regulator driver.

The series is composed of the following patches:

Javier Martinez Canillas (5):
  regulator: Document binding for initial and suspend modes
  regulator: Add function to map modes to struct regulator_desc
  regulator: of: Add regulator desc param to
    of_get_regulator_init_data()
  regulator: of: Pass the regulator description in the match table
  regulator: of: Add support for parsing initial and suspend modes

 .../devicetree/bindings/regulator/regulator.txt    | 14 ++++++++++
 drivers/regulator/88pm8607.c                       |  3 +-
 drivers/regulator/anatop-regulator.c               |  4 +--
 drivers/regulator/arizona-ldo1.c                   |  8 ++++--
 drivers/regulator/arizona-micsupp.c                |  8 ++++--
 drivers/regulator/da9052-regulator.c               |  3 +-
 drivers/regulator/da9210-regulator.c               |  2 +-
 drivers/regulator/fan53555.c                       | 17 ++++++------
 drivers/regulator/fixed.c                          | 18 ++++++------
 drivers/regulator/gpio-regulator.c                 | 18 ++++++------
 drivers/regulator/max8952.c                        |  2 +-
 drivers/regulator/max8973-regulator.c              |  3 +-
 drivers/regulator/max8997.c                        |  3 +-
 drivers/regulator/max8998.c                        |  5 ++--
 drivers/regulator/mc13xxx-regulator-core.c         |  3 +-
 drivers/regulator/of_regulator.c                   | 32 ++++++++++++++++++----
 drivers/regulator/pwm-regulator.c                  |  3 +-
 drivers/regulator/qcom_rpm-regulator.c             |  9 +++---
 drivers/regulator/s5m8767.c                        |  3 +-
 drivers/regulator/sky81452-regulator.c             |  2 +-
 drivers/regulator/stw481x-vmmc.c                   |  3 +-
 drivers/regulator/ti-abb-regulator.c               |  3 +-
 drivers/regulator/tps51632-regulator.c             | 16 ++++++-----
 drivers/regulator/tps62360-regulator.c             | 17 +++++++-----
 drivers/regulator/tps65218-regulator.c             |  3 +-
 drivers/regulator/twl-regulator.c                  |  3 +-
 drivers/regulator/vexpress.c                       |  3 +-
 include/linux/regulator/driver.h                   |  8 ++++++
 include/linux/regulator/of_regulator.h             |  9 ++++--
 29 files changed, 151 insertions(+), 74 deletions(-)

Best regards,
Javier

[0]: https://lkml.org/lkml/2014/10/10/161
[1]: https://lkml.org/lkml/2014/6/25/668

[PATCH v5 1/5] regulator: Document binding for initial and suspend modes

[Javier Martinez Canillas]

Some regulators can run on different operating modes (opmodes). This
allows systems to choose the most efficient opmode for each regulator.

This patch builds on top of (291d761 regulator: Document binding for
regulator suspend state for PM state) adding a regulator-initial-mode
DT property to configure at startup the operating mode for regulators
that support changing its mode during normal operation and a property
regulator-mode to be used in the regulator-state-[mem/disk] nodes for
regulators that supports changing its operating mode when the system
enters in a suspend state.

The set of possible modes that a regulator can operate depends on the
hardware capabilities so a list of generic operating modes can't be
provided. Instead, each hardware binding should define the list of
valid operating modes for the regulators found on that device.

Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
---

Changes in v5: None

Changes in v4: None

Changes in v3:
 - Rebased on top of regulator suspend voltage series

 Documentation/devicetree/bindings/regulator/regulator.txt | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/Documentation/devicetree/bindings/regulator/regulator.txt b/Documentation/devicetree/bindings/regulator/regulator.txt
index 4e7ed76..3fffa3b 100644
--- a/Documentation/devicetree/bindings/regulator/regulator.txt
+++ b/Documentation/devicetree/bindings/regulator/regulator.txt
@@ -30,6 +30,20 @@ Optional properties:
 	- regulator-off-in-suspend: regulator should be off in suspend state.
 	- regulator-suspend-microvolt: regulator should be set to this voltage
 	  in suspend.
+	- regulator-mode: operating mode in the given suspend state.
+	  The set of possible operating modes depends on the capabilities of
+	  every hardware so the valid modes are documented on each regulator
+	  device tree binding document.
+	  The "regulator-mode" property only takes effect if the regulator is
+	  enabled for the given suspend state using "regulator-on-in-suspend".
+	  If the regulator has not been explicitly disabled for the given state
+	  with "regulator-off-in-suspend", then setting the operating mode
+	  will also have no effect.
+- regulator-initial-mode: initial operating mode. The set of possible operating
+  modes is the same used for the regulator-mode property and the device binding
+  documentation explains which property each regulator supports.
+If no mode is defined, then the OS will not manage the modes and the hardware
+default values will be used instead.
 
 Deprecated properties:
 - regulator-compatible: If a regulator chip contains multiple
-- 
2.1.0

Re: [PATCH v5 1/5] regulator: Document binding for initial and suspend modes

[Mark Brown]

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

On Fri, Nov 07, 2014 at 02:00:01PM +0100, Javier Martinez Canillas wrote:

> +	  The "regulator-mode" property only takes effect if the regulator is
> +	  enabled for the given suspend state using "regulator-on-in-suspend".

Why?

> +	  If the regulator has not been explicitly disabled for the given state
> +	  with "regulator-off-in-suspend", then setting the operating mode
> +	  will also have no effect.

This seems surprising, I'd expect mode setting to be paid attention to
even if the regulator is off - we may add other ways to control the
enable state in suspend for example.

> +- regulator-initial-mode: initial operating mode. The set of possible operating
> +  modes is the same used for the regulator-mode property and the device binding
> +  documentation explains which property each regulator supports.
> +If no mode is defined, then the OS will not manage the modes and the hardware
> +default values will be used instead.

Again that seems surprising, it precludes any future changes and isn't
going to be true for devices where we can't read the current state.

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

Re: [PATCH v5 1/5] regulator: Document binding for initial and suspend modes

[Javier Martinez Canillas]

Hello Mark,

Thanks a lot for your feedback.

On 11/07/2014 03:58 PM, Mark Brown wrote:
> On Fri, Nov 07, 2014 at 02:00:01PM +0100, Javier Martinez Canillas wrote:
> 
>> +	  The "regulator-mode" property only takes effect if the regulator is
>> +	  enabled for the given suspend state using "regulator-on-in-suspend".
> 
> Why?
> 

I saw that the regulator core only call the .set_suspend_mode callback if
the regulator is either enabled or disabled explicitly...

static int suspend_set_state(struct regulator_dev *rdev,
	struct regulator_state *rstate)
{
	....
	/* If we have no suspend mode configration don't set anything;
	 * only warn if the driver implements set_suspend_voltage or
	 * set_suspend_mode callback.
	 */
	if (!rstate->enabled && !rstate->disabled) {
		if (rdev->desc->ops->set_suspend_voltage ||
		    rdev->desc->ops->set_suspend_mode)
			rdev_warn(rdev, "No configuration\n");
		return 0;
	}
	...
	
};

>> +	  If the regulator has not been explicitly disabled for the given state
>> +	  with "regulator-off-in-suspend", then setting the operating mode
>> +	  will also have no effect.
> 
> This seems surprising, I'd expect mode setting to be paid attention to
> even if the regulator is off - we may add other ways to control the
> enable state in suspend for example.
> 

...and I thought that setting a mode if the regulator was disabled in suspend
was not a possible configuration, that's why I documented that.

I know that there is both a .set_suspend_disable and .set_suspend_mode but
at least in the hardware that I'm interested in (max77802), the same hw
register is used for setting a suspend mode and disable on suspend.

So if this is allowed and a user specifies both a disable on suspend and a
suspend mode, the later will overwrite the configuration of the former.

But now that I think about it, this is only a constraint of the max77802
and probably needs to be specified in the max77802 DT binding and not in
the regulator generic one since otherwise limits what other devices can
do.

>> +- regulator-initial-mode: initial operating mode. The set of possible operating
>> +  modes is the same used for the regulator-mode property and the device binding
>> +  documentation explains which property each regulator supports.
>> +If no mode is defined, then the OS will not manage the modes and the hardware
>> +default values will be used instead.
> 
> Again that seems surprising, it precludes any future changes and isn't
> going to be true for devices where we can't read the current state.
> 

I saw that you asked Chanwoo on an early version of his suspend states
series, to point out that in the absence of a initial state, the state is
the hardware default [0] so I thought that it should be the case for the
regulator suspend mode too.

So should I just explain what each property is about without trying to make
assumptions about the limitations that different devices could have and let
each device DT binding to specify those?

Best regards,
Javier

[0]: https://lkml.org/lkml/2014/9/4/652 

Re: [PATCH v5 1/5] regulator: Document binding for initial and suspend modes

[Mark Brown]

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

On Fri, Nov 07, 2014 at 04:38:04PM +0100, Javier Martinez Canillas wrote:
> On 11/07/2014 03:58 PM, Mark Brown wrote:
> > On Fri, Nov 07, 2014 at 02:00:01PM +0100, Javier Martinez Canillas wrote:

> >> +	  The "regulator-mode" property only takes effect if the regulator is
> >> +	  enabled for the given suspend state using "regulator-on-in-suspend".

> > Why?

> I saw that the regulator core only call the .set_suspend_mode callback if
> the regulator is either enabled or disabled explicitly...

That's the current implementation.  Why are you adding it to a
specification?

> static int suspend_set_state(struct regulator_dev *rdev,
> 	struct regulator_state *rstate)
> {
> 	....
> 	/* If we have no suspend mode configration don't set anything;
> 	 * only warn if the driver implements set_suspend_voltage or
> 	 * set_suspend_mode callback.
> 	 */
> 	if (!rstate->enabled && !rstate->disabled) {

For example why couldn't these get set by reading the current state?

> >> +	  If the regulator has not been explicitly disabled for the given state
> >> +	  with "regulator-off-in-suspend", then setting the operating mode
> >> +	  will also have no effect.

> > This seems surprising, I'd expect mode setting to be paid attention to
> > even if the regulator is off - we may add other ways to control the
> > enable state in suspend for example.

> ...and I thought that setting a mode if the regulator was disabled in suspend
> was not a possible configuration, that's why I documented that.

Like I say this is at the very least making it impossible for us to in
future add other ways of setting if the regulator is enabled or disabled
in suspend.

> I know that there is both a .set_suspend_disable and .set_suspend_mode but
> at least in the hardware that I'm interested in (max77802), the same hw
> register is used for setting a suspend mode and disable on suspend.

That's your device, other hardware exists which uses seprate bitfields.

> >> +If no mode is defined, then the OS will not manage the modes and the hardware
> >> +default values will be used instead.

> > Again that seems surprising, it precludes any future changes and isn't
> > going to be true for devices where we can't read the current state.

> I saw that you asked Chanwoo on an early version of his suspend states
> series, to point out that in the absence of a initial state, the state is
> the hardware default [0] so I thought that it should be the case for the
> regulator suspend mode too.

You're also saying that the OS won't manage the mode here, that's a step
further.

> So should I just explain what each property is about without trying to make
> assumptions about the limitations that different devices could have and let
> each device DT binding to specify those?

More towards that direction, yes.  Don't overspecify.

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

Re: [PATCH v5 1/5] regulator: Document binding for initial and suspend modes

[Javier Martinez Canillas]

Hello Mark,

On 11/07/2014 05:10 PM, Mark Brown wrote:
> 
> You're also saying that the OS won't manage the mode here, that's a step
> further.
>

I see
 
>> So should I just explain what each property is about without trying to make
>> assumptions about the limitations that different devices could have and let
>> each device DT binding to specify those?
> 
> More towards that direction, yes.  Don't overspecify.
> 

Got it, thanks a lot for your valuable feedback.

Best regards,
Javier

[PATCH v5 2/5] regulator: Add function to map modes to struct regulator_desc

[Javier Martinez Canillas]

The "regulator-initial-mode" and "regulator-mode" DT properties allows
to configure the regulator operating modes at startup or when a system
enters into a susend state.

But these properties use as valid values the operating modes supported
by each device while the core deals with the standard operating modes.
So a mapping function is needed to translate from the hardware specific
modes to the standard ones.

This mapping is a non-varying configuration for each regulator, so add
a function pointer to struct regulator_desc that will allow drivers to
define their callback to do the modes translation.

Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
---

Changes in v5:
 - Add a better explanation of what mode is passed as an argument and what
   mode is returned. Suggested by Krzysztof Kozlowski.
 - Explain that this callback is needed to be able to parse the regulator
   initial and suspend state modes. Suggested by Krzysztof Kozlowski.

 include/linux/regulator/driver.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/include/linux/regulator/driver.h b/include/linux/regulator/driver.h
index 28da08e..7966650 100644
--- a/include/linux/regulator/driver.h
+++ b/include/linux/regulator/driver.h
@@ -243,6 +243,12 @@ enum regulator_type {
  *
  * @enable_time: Time taken for initial enable of regulator (in uS).
  * @off_on_delay: guard time (in uS), before re-enabling a regulator
+ *
+ * @map_modes: Callback invoked to translate from hardware to standard modes.
+ *             The function returns the standard REGULATOR_MODE that maps to
+ *             the hardware specific mode passed as an argument.
+ *             If the callback is not defined, the "regulator-initial-mode"
+ *             and suspend states "regulator-mode" properties won't be parsed.
  */
 struct regulator_desc {
 	const char *name;
@@ -285,6 +291,8 @@ struct regulator_desc {
 	unsigned int enable_time;
 
 	unsigned int off_on_delay;
+
+	unsigned int (*map_modes)(unsigned int mode);
 };
 
 /**
-- 
2.1.0

Re: [PATCH v5 2/5] regulator: Add function to map modes to struct regulator_desc

[Mark Brown]

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

On Fri, Nov 07, 2014 at 02:00:02PM +0100, Javier Martinez Canillas wrote:

> + * @map_modes: Callback invoked to translate from hardware to standard modes.
> + *             The function returns the standard REGULATOR_MODE that maps to
> + *             the hardware specific mode passed as an argument.

of_map_mode - it's only one mode and this is mapping to/from DT.

> + *             If the callback is not defined, the "regulator-initial-mode"
> + *             and suspend states "regulator-mode" properties won't be parsed.

Just remove this, it's pretty obvious at best and going to bit rot at
worst.

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

Re: [PATCH v5 2/5] regulator: Add function to map modes to struct regulator_desc

[Javier Martinez Canillas]

On 11/07/2014 04:54 PM, Mark Brown wrote:
> of_map_mode - it's only one mode and this is mapping to/from DT.
>

Ok
 
>> + *             If the callback is not defined, the "regulator-initial-mode"
>> + *             and suspend states "regulator-mode" properties won't be parsed.
> 
> Just remove this, it's pretty obvious at best and going to bit rot at
> worst.
> 

Ok, it was not in a previous version but I added because Krzysztof said
that it may be good to have it documented. I'll remove it.

Best regards,
Javier

[PATCH v5 3/5] regulator: of: Add regulator desc param to of_get_regulator_init_data()

[Javier Martinez Canillas]

The of_get_regulator_init_data() function is used to extract the regulator
init_data but information on how to extract certain data is defined in the
static regulator descriptor (e.g: how to map the hardware operating modes).

Add a const struct regulator_desc * parameter to the function signature so
the parsing logic could use the information in the struct regulator_desc.

of_get_regulator_init_data() relies on of_get_regulation_constraints() to
actually extract the init_data so it has to pass the struct regulator_desc
but that is modified on a later patch.

Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
---

Changes in v5:
 - Pass the descriptor struct regulator_desc to of_get_regulator_init_data()
   for all drivers even if won't be currently used. Suggested by Mark Brown.

of_regulator_match() still passes NULL to of_get_regulator_init_data() but
that is changed on a later patch when a struct regulator_desc pointer is
added to the struct of_regulator_match match table.

 drivers/regulator/88pm8607.c               |  3 ++-
 drivers/regulator/anatop-regulator.c       |  4 ++--
 drivers/regulator/arizona-ldo1.c           |  8 +++++---
 drivers/regulator/arizona-micsupp.c        |  8 +++++---
 drivers/regulator/da9052-regulator.c       |  3 ++-
 drivers/regulator/da9210-regulator.c       |  2 +-
 drivers/regulator/fan53555.c               | 17 +++++++++--------
 drivers/regulator/fixed.c                  | 18 ++++++++++--------
 drivers/regulator/gpio-regulator.c         | 18 ++++++++++--------
 drivers/regulator/max8952.c                |  2 +-
 drivers/regulator/max8973-regulator.c      |  3 ++-
 drivers/regulator/max8997.c                |  3 ++-
 drivers/regulator/max8998.c                |  5 +++--
 drivers/regulator/mc13xxx-regulator-core.c |  3 ++-
 drivers/regulator/of_regulator.c           |  9 ++++++---
 drivers/regulator/pwm-regulator.c          |  3 ++-
 drivers/regulator/qcom_rpm-regulator.c     |  9 +++++----
 drivers/regulator/s5m8767.c                |  3 ++-
 drivers/regulator/sky81452-regulator.c     |  2 +-
 drivers/regulator/stw481x-vmmc.c           |  3 ++-
 drivers/regulator/ti-abb-regulator.c       |  3 ++-
 drivers/regulator/tps51632-regulator.c     | 16 +++++++++-------
 drivers/regulator/tps62360-regulator.c     | 17 ++++++++++-------
 drivers/regulator/tps65218-regulator.c     |  3 ++-
 drivers/regulator/twl-regulator.c          |  3 ++-
 drivers/regulator/vexpress.c               |  3 ++-
 include/linux/regulator/of_regulator.h     |  8 ++++++--
 27 files changed, 107 insertions(+), 72 deletions(-)

diff --git a/drivers/regulator/88pm8607.c b/drivers/regulator/88pm8607.c
index 6d77dcd..3fe47bd 100644
--- a/drivers/regulator/88pm8607.c
+++ b/drivers/regulator/88pm8607.c
@@ -330,7 +330,8 @@ static int pm8607_regulator_dt_init(struct platform_device *pdev,
 	for_each_child_of_node(nproot, np) {
 		if (!of_node_cmp(np->name, info->desc.name)) {
 			config->init_data =
-				of_get_regulator_init_data(&pdev->dev, np);
+				of_get_regulator_init_data(&pdev->dev, np,
+							   &info->desc);
 			config->of_node = np;
 			break;
 		}
diff --git a/drivers/regulator/anatop-regulator.c b/drivers/regulator/anatop-regulator.c
index 542d14e..9c3dc4d 100644
--- a/drivers/regulator/anatop-regulator.c
+++ b/drivers/regulator/anatop-regulator.c
@@ -189,13 +189,13 @@ static int anatop_regulator_probe(struct platform_device *pdev)
 	int ret = 0;
 	u32 val;
 
-	initdata = of_get_regulator_init_data(dev, np);
 	sreg = devm_kzalloc(dev, sizeof(*sreg), GFP_KERNEL);
 	if (!sreg)
 		return -ENOMEM;
-	sreg->initdata = initdata;
 	sreg->name = of_get_property(np, "regulator-name", NULL);
 	rdesc = &sreg->rdesc;
+	initdata = of_get_regulator_init_data(dev, np, rdesc);
+	sreg->initdata = initdata;
 	rdesc->name = sreg->name;
 	rdesc->type = REGULATOR_VOLTAGE;
 	rdesc->owner = THIS_MODULE;
diff --git a/drivers/regulator/arizona-ldo1.c b/drivers/regulator/arizona-ldo1.c
index 4c9db58..b1eea7f 100644
--- a/drivers/regulator/arizona-ldo1.c
+++ b/drivers/regulator/arizona-ldo1.c
@@ -179,7 +179,8 @@ static const struct regulator_init_data arizona_ldo1_default = {
 };
 
 static int arizona_ldo1_of_get_pdata(struct arizona *arizona,
-				     struct regulator_config *config)
+				     struct regulator_config *config,
+				     const struct regulator_desc *desc)
 {
 	struct arizona_pdata *pdata = &arizona->pdata;
 	struct arizona_ldo1 *ldo1 = config->driver_data;
@@ -194,7 +195,8 @@ static int arizona_ldo1_of_get_pdata(struct arizona *arizona,
 	if (init_node) {
 		config->of_node = init_node;
 
-		init_data = of_get_regulator_init_data(arizona->dev, init_node);
+		init_data = of_get_regulator_init_data(arizona->dev, init_node,
+						       desc);
 
 		if (init_data) {
 			init_data->consumer_supplies = &ldo1->supply;
@@ -257,7 +259,7 @@ static int arizona_ldo1_probe(struct platform_device *pdev)
 
 	if (IS_ENABLED(CONFIG_OF)) {
 		if (!dev_get_platdata(arizona->dev)) {
-			ret = arizona_ldo1_of_get_pdata(arizona, &config);
+			ret = arizona_ldo1_of_get_pdata(arizona, &config, desc);
 			if (ret < 0)
 				return ret;
 		}
diff --git a/drivers/regulator/arizona-micsupp.c b/drivers/regulator/arizona-micsupp.c
index ce9aca5..c313ef4 100644
--- a/drivers/regulator/arizona-micsupp.c
+++ b/drivers/regulator/arizona-micsupp.c
@@ -198,7 +198,8 @@ static const struct regulator_init_data arizona_micsupp_ext_default = {
 };
 
 static int arizona_micsupp_of_get_pdata(struct arizona *arizona,
-					struct regulator_config *config)
+					struct regulator_config *config,
+					const struct regulator_desc *desc)
 {
 	struct arizona_pdata *pdata = &arizona->pdata;
 	struct arizona_micsupp *micsupp = config->driver_data;
@@ -210,7 +211,7 @@ static int arizona_micsupp_of_get_pdata(struct arizona *arizona,
 	if (np) {
 		config->of_node = np;
 
-		init_data = of_get_regulator_init_data(arizona->dev, np);
+		init_data = of_get_regulator_init_data(arizona->dev, np, desc);
 
 		if (init_data) {
 			init_data->consumer_supplies = &micsupp->supply;
@@ -264,7 +265,8 @@ static int arizona_micsupp_probe(struct platform_device *pdev)
 
 	if (IS_ENABLED(CONFIG_OF)) {
 		if (!dev_get_platdata(arizona->dev)) {
-			ret = arizona_micsupp_of_get_pdata(arizona, &config);
+			ret = arizona_micsupp_of_get_pdata(arizona, &config,
+							   desc);
 			if (ret < 0)
 				return ret;
 		}
diff --git a/drivers/regulator/da9052-regulator.c b/drivers/regulator/da9052-regulator.c
index 0003362..3945f10 100644
--- a/drivers/regulator/da9052-regulator.c
+++ b/drivers/regulator/da9052-regulator.c
@@ -436,7 +436,8 @@ static int da9052_regulator_probe(struct platform_device *pdev)
 			if (!of_node_cmp(np->name,
 					 regulator->info->reg_desc.name)) {
 				config.init_data = of_get_regulator_init_data(
-					&pdev->dev, np);
+					&pdev->dev, np,
+					&regulator->info->reg_desc);
 				config.of_node = np;
 				break;
 			}
diff --git a/drivers/regulator/da9210-regulator.c b/drivers/regulator/da9210-regulator.c
index 7a320dd..bc61001 100644
--- a/drivers/regulator/da9210-regulator.c
+++ b/drivers/regulator/da9210-regulator.c
@@ -147,7 +147,7 @@ static int da9210_i2c_probe(struct i2c_client *i2c,
 
 	config.dev = &i2c->dev;
 	config.init_data = pdata ? &pdata->da9210_constraints :
-		of_get_regulator_init_data(dev, dev->of_node);
+		of_get_regulator_init_data(dev, dev->of_node, &da9210_reg);
 	config.driver_data = chip;
 	config.regmap = chip->regmap;
 	config.of_node = dev->of_node;
diff --git a/drivers/regulator/fan53555.c b/drivers/regulator/fan53555.c
index f8e4257..37b671c 100644
--- a/drivers/regulator/fan53555.c
+++ b/drivers/regulator/fan53555.c
@@ -302,7 +302,8 @@ static struct regmap_config fan53555_regmap_config = {
 };
 
 static struct fan53555_platform_data *fan53555_parse_dt(struct device *dev,
-							struct device_node *np)
+						struct device_node *np,
+						struct regulator_desc *desc)
 {
 	struct fan53555_platform_data *pdata;
 	int ret;
@@ -312,7 +313,7 @@ static struct fan53555_platform_data *fan53555_parse_dt(struct device *dev,
 	if (!pdata)
 		return NULL;
 
-	pdata->regulator = of_get_regulator_init_data(dev, np);
+	pdata->regulator = of_get_regulator_init_data(dev, np, desc);
 
 	ret = of_property_read_u32(np, "fcs,suspend-voltage-selector",
 				   &tmp);
@@ -347,20 +348,20 @@ static int fan53555_regulator_probe(struct i2c_client *client,
 	unsigned int val;
 	int ret;
 
+	di = devm_kzalloc(&client->dev, sizeof(struct fan53555_device_info),
+					GFP_KERNEL);
+	if (!di)
+		return -ENOMEM;
+
 	pdata = dev_get_platdata(&client->dev);
 	if (!pdata)
-		pdata = fan53555_parse_dt(&client->dev, np);
+		pdata = fan53555_parse_dt(&client->dev, np, &di->desc);
 
 	if (!pdata || !pdata->regulator) {
 		dev_err(&client->dev, "Platform data not found!\n");
 		return -ENODEV;
 	}
 
-	di = devm_kzalloc(&client->dev, sizeof(struct fan53555_device_info),
-					GFP_KERNEL);
-	if (!di)
-		return -ENOMEM;
-
 	di->regulator = pdata->regulator;
 	if (client->dev.of_node) {
 		const struct of_device_id *match;
diff --git a/drivers/regulator/fixed.c b/drivers/regulator/fixed.c
index 354105e..649fece 100644
--- a/drivers/regulator/fixed.c
+++ b/drivers/regulator/fixed.c
@@ -40,13 +40,14 @@ struct fixed_voltage_data {
 /**
  * of_get_fixed_voltage_config - extract fixed_voltage_config structure info
  * @dev: device requesting for fixed_voltage_config
+ * @desc: regulator description
  *
  * Populates fixed_voltage_config structure by extracting data from device
  * tree node, returns a pointer to the populated structure of NULL if memory
  * alloc fails.
  */
 static struct fixed_voltage_config *
-of_get_fixed_voltage_config(struct device *dev)
+of_get_fixed_voltage_config(struct device *dev, struct regulator_desc *desc)
 {
 	struct fixed_voltage_config *config;
 	struct device_node *np = dev->of_node;
@@ -57,7 +58,7 @@ of_get_fixed_voltage_config(struct device *dev)
 	if (!config)
 		return ERR_PTR(-ENOMEM);
 
-	config->init_data = of_get_regulator_init_data(dev, dev->of_node);
+	config->init_data = of_get_regulator_init_data(dev, dev->of_node, desc);
 	if (!config->init_data)
 		return ERR_PTR(-EINVAL);
 
@@ -112,8 +113,14 @@ static int reg_fixed_voltage_probe(struct platform_device *pdev)
 	struct regulator_config cfg = { };
 	int ret;
 
+	drvdata = devm_kzalloc(&pdev->dev, sizeof(struct fixed_voltage_data),
+			       GFP_KERNEL);
+	if (!drvdata)
+		return -ENOMEM;
+
 	if (pdev->dev.of_node) {
-		config = of_get_fixed_voltage_config(&pdev->dev);
+		config = of_get_fixed_voltage_config(&pdev->dev,
+						     &drvdata->desc);
 		if (IS_ERR(config))
 			return PTR_ERR(config);
 	} else {
@@ -123,11 +130,6 @@ static int reg_fixed_voltage_probe(struct platform_device *pdev)
 	if (!config)
 		return -ENOMEM;
 
-	drvdata = devm_kzalloc(&pdev->dev, sizeof(struct fixed_voltage_data),
-			       GFP_KERNEL);
-	if (!drvdata)
-		return -ENOMEM;
-
 	drvdata->desc.name = devm_kstrdup(&pdev->dev,
 					  config->supply_name,
 					  GFP_KERNEL);
diff --git a/drivers/regulator/gpio-regulator.c b/drivers/regulator/gpio-regulator.c
index 989b23b..be0d08e 100644
--- a/drivers/regulator/gpio-regulator.c
+++ b/drivers/regulator/gpio-regulator.c
@@ -133,7 +133,8 @@ static struct regulator_ops gpio_regulator_voltage_ops = {
 };
 
 static struct gpio_regulator_config *
-of_get_gpio_regulator_config(struct device *dev, struct device_node *np)
+of_get_gpio_regulator_config(struct device *dev, struct device_node *np,
+			     struct regulator_desc *desc)
 {
 	struct gpio_regulator_config *config;
 	const char *regtype;
@@ -146,7 +147,7 @@ of_get_gpio_regulator_config(struct device *dev, struct device_node *np)
 	if (!config)
 		return ERR_PTR(-ENOMEM);
 
-	config->init_data = of_get_regulator_init_data(dev, np);
+	config->init_data = of_get_regulator_init_data(dev, np, desc);
 	if (!config->init_data)
 		return ERR_PTR(-EINVAL);
 
@@ -243,17 +244,18 @@ static int gpio_regulator_probe(struct platform_device *pdev)
 	struct regulator_config cfg = { };
 	int ptr, ret, state;
 
-	if (np) {
-		config = of_get_gpio_regulator_config(&pdev->dev, np);
-		if (IS_ERR(config))
-			return PTR_ERR(config);
-	}
-
 	drvdata = devm_kzalloc(&pdev->dev, sizeof(struct gpio_regulator_data),
 			       GFP_KERNEL);
 	if (drvdata == NULL)
 		return -ENOMEM;
 
+	if (np) {
+		config = of_get_gpio_regulator_config(&pdev->dev, np,
+						      &drvdata->desc);
+		if (IS_ERR(config))
+			return PTR_ERR(config);
+	}
+
 	drvdata->desc.name = kstrdup(config->supply_name, GFP_KERNEL);
 	if (drvdata->desc.name == NULL) {
 		dev_err(&pdev->dev, "Failed to allocate supply name\n");
diff --git a/drivers/regulator/max8952.c b/drivers/regulator/max8952.c
index f7f9efc..6e54d78 100644
--- a/drivers/regulator/max8952.c
+++ b/drivers/regulator/max8952.c
@@ -174,7 +174,7 @@ static struct max8952_platform_data *max8952_parse_dt(struct device *dev)
 	if (of_property_read_u32(np, "max8952,ramp-speed", &pd->ramp_speed))
 		dev_warn(dev, "max8952,ramp-speed property not specified, defaulting to 32mV/us\n");
 
-	pd->reg_data = of_get_regulator_init_data(dev, np);
+	pd->reg_data = of_get_regulator_init_data(dev, np, &regulator);
 	if (!pd->reg_data) {
 		dev_err(dev, "Failed to parse regulator init data\n");
 		return NULL;
diff --git a/drivers/regulator/max8973-regulator.c b/drivers/regulator/max8973-regulator.c
index dbedf17..c3d55c2 100644
--- a/drivers/regulator/max8973-regulator.c
+++ b/drivers/regulator/max8973-regulator.c
@@ -458,7 +458,8 @@ static int max8973_probe(struct i2c_client *client,
 
 	config.dev = &client->dev;
 	config.init_data = pdata ? pdata->reg_init_data :
-		of_get_regulator_init_data(&client->dev, client->dev.of_node);
+		of_get_regulator_init_data(&client->dev, client->dev.of_node,
+					   &max->desc);
 	config.driver_data = max;
 	config.of_node = client->dev.of_node;
 	config.regmap = max->regmap;
diff --git a/drivers/regulator/max8997.c b/drivers/regulator/max8997.c
index 9c31e21..726fde1 100644
--- a/drivers/regulator/max8997.c
+++ b/drivers/regulator/max8997.c
@@ -953,7 +953,8 @@ static int max8997_pmic_dt_parse_pdata(struct platform_device *pdev,
 
 		rdata->id = i;
 		rdata->initdata = of_get_regulator_init_data(&pdev->dev,
-							     reg_np);
+							     reg_np,
+							     &regulators[i]);
 		rdata->reg_node = reg_np;
 		rdata++;
 	}
diff --git a/drivers/regulator/max8998.c b/drivers/regulator/max8998.c
index 961091b..59e34a0 100644
--- a/drivers/regulator/max8998.c
+++ b/drivers/regulator/max8998.c
@@ -686,8 +686,9 @@ static int max8998_pmic_dt_parse_pdata(struct max8998_dev *iodev,
 			continue;
 
 		rdata->id = regulators[i].id;
-		rdata->initdata = of_get_regulator_init_data(
-							iodev->dev, reg_np);
+		rdata->initdata = of_get_regulator_init_data(iodev->dev,
+							     reg_np,
+							     &regulators[i]);
 		rdata->reg_node = reg_np;
 		++rdata;
 	}
diff --git a/drivers/regulator/mc13xxx-regulator-core.c b/drivers/regulator/mc13xxx-regulator-core.c
index afba024..0281c31 100644
--- a/drivers/regulator/mc13xxx-regulator-core.c
+++ b/drivers/regulator/mc13xxx-regulator-core.c
@@ -194,7 +194,8 @@ struct mc13xxx_regulator_init_data *mc13xxx_parse_regulators_dt(
 					 regulators[i].desc.name)) {
 				p->id = i;
 				p->init_data = of_get_regulator_init_data(
-							&pdev->dev, child);
+							&pdev->dev, child,
+							&regulators[i].desc);
 				p->node = child;
 				p++;
 
diff --git a/drivers/regulator/of_regulator.c b/drivers/regulator/of_regulator.c
index 03edb17..945486f 100644
--- a/drivers/regulator/of_regulator.c
+++ b/drivers/regulator/of_regulator.c
@@ -120,13 +120,16 @@ static void of_get_regulation_constraints(struct device_node *np,
 /**
  * of_get_regulator_init_data - extract regulator_init_data structure info
  * @dev: device requesting for regulator_init_data
+ * @node: regulator device node
+ * @desc: regulator description
  *
  * Populates regulator_init_data structure by extracting data from device
  * tree node, returns a pointer to the populated struture or NULL if memory
  * alloc fails.
  */
 struct regulator_init_data *of_get_regulator_init_data(struct device *dev,
-						struct device_node *node)
+					  struct device_node *node,
+					  const struct regulator_desc *desc)
 {
 	struct regulator_init_data *init_data;
 
@@ -218,7 +221,7 @@ int of_regulator_match(struct device *dev, struct device_node *node,
 				continue;
 
 			match->init_data =
-				of_get_regulator_init_data(dev, child);
+				of_get_regulator_init_data(dev, child, NULL);
 			if (!match->init_data) {
 				dev_err(dev,
 					"failed to parse DT for regulator %s\n",
@@ -266,7 +269,7 @@ struct regulator_init_data *regulator_of_get_init_data(struct device *dev,
 		if (strcmp(desc->of_match, name))
 			continue;
 
-		init_data = of_get_regulator_init_data(dev, child);
+		init_data = of_get_regulator_init_data(dev, child, desc);
 		if (!init_data) {
 			dev_err(dev,
 				"failed to parse DT for regulator %s\n",
diff --git a/drivers/regulator/pwm-regulator.c b/drivers/regulator/pwm-regulator.c
index d3f55ea..91f34ca 100644
--- a/drivers/regulator/pwm-regulator.c
+++ b/drivers/regulator/pwm-regulator.c
@@ -149,7 +149,8 @@ static int pwm_regulator_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	config.init_data = of_get_regulator_init_data(&pdev->dev, np);
+	config.init_data = of_get_regulator_init_data(&pdev->dev, np,
+						      &drvdata->desc);
 	if (!config.init_data)
 		return -ENOMEM;
 
diff --git a/drivers/regulator/qcom_rpm-regulator.c b/drivers/regulator/qcom_rpm-regulator.c
index b55cd5b..dabd28a 100644
--- a/drivers/regulator/qcom_rpm-regulator.c
+++ b/drivers/regulator/qcom_rpm-regulator.c
@@ -643,10 +643,6 @@ static int rpm_reg_probe(struct platform_device *pdev)
 	match = of_match_device(rpm_of_match, &pdev->dev);
 	template = match->data;
 
-	initdata = of_get_regulator_init_data(&pdev->dev, pdev->dev.of_node);
-	if (!initdata)
-		return -EINVAL;
-
 	vreg = devm_kmalloc(&pdev->dev, sizeof(*vreg), GFP_KERNEL);
 	if (!vreg) {
 		dev_err(&pdev->dev, "failed to allocate vreg\n");
@@ -666,6 +662,11 @@ static int rpm_reg_probe(struct platform_device *pdev)
 		return -ENODEV;
 	}
 
+	initdata = of_get_regulator_init_data(&pdev->dev, pdev->dev.of_node,
+					      &vreg->desc);
+	if (!initdata)
+		return -EINVAL;
+
 	key = "reg";
 	ret = of_property_read_u32(pdev->dev.of_node, key, &val);
 	if (ret) {
diff --git a/drivers/regulator/s5m8767.c b/drivers/regulator/s5m8767.c
index 0ab5cbe..26932fe 100644
--- a/drivers/regulator/s5m8767.c
+++ b/drivers/regulator/s5m8767.c
@@ -581,7 +581,8 @@ static int s5m8767_pmic_dt_parse_pdata(struct platform_device *pdev,
 
 		rdata->id = i;
 		rdata->initdata = of_get_regulator_init_data(
-						&pdev->dev, reg_np);
+						&pdev->dev, reg_np,
+						&regulators[i]);
 		rdata->reg_node = reg_np;
 		rdata++;
 		rmode->id = i;
diff --git a/drivers/regulator/sky81452-regulator.c b/drivers/regulator/sky81452-regulator.c
index 476b80a..e68e13f 100644
--- a/drivers/regulator/sky81452-regulator.c
+++ b/drivers/regulator/sky81452-regulator.c
@@ -76,7 +76,7 @@ static struct regulator_init_data *sky81452_reg_parse_dt(struct device *dev)
 		return NULL;
 	}
 
-	init_data = of_get_regulator_init_data(dev, np);
+	init_data = of_get_regulator_init_data(dev, np, &sky81452_reg);
 
 	of_node_put(np);
 	return init_data;
diff --git a/drivers/regulator/stw481x-vmmc.c b/drivers/regulator/stw481x-vmmc.c
index a7e1526..b4f1696 100644
--- a/drivers/regulator/stw481x-vmmc.c
+++ b/drivers/regulator/stw481x-vmmc.c
@@ -72,7 +72,8 @@ static int stw481x_vmmc_regulator_probe(struct platform_device *pdev)
 	config.regmap = stw481x->map;
 	config.of_node = pdev->dev.of_node;
 	config.init_data = of_get_regulator_init_data(&pdev->dev,
-						      pdev->dev.of_node);
+						      pdev->dev.of_node,
+						      &vmmc_regulator);
 
 	stw481x->vmmc_regulator = devm_regulator_register(&pdev->dev,
 						&vmmc_regulator, &config);
diff --git a/drivers/regulator/ti-abb-regulator.c b/drivers/regulator/ti-abb-regulator.c
index a2dabb5..1ef5aba 100644
--- a/drivers/regulator/ti-abb-regulator.c
+++ b/drivers/regulator/ti-abb-regulator.c
@@ -837,7 +837,8 @@ skip_opt:
 		return -EINVAL;
 	}
 
-	initdata = of_get_regulator_init_data(dev, pdev->dev.of_node);
+	initdata = of_get_regulator_init_data(dev, pdev->dev.of_node,
+					      &abb->rdesc);
 	if (!initdata) {
 		dev_err(dev, "%s: Unable to alloc regulator init data\n",
 			__func__);
diff --git a/drivers/regulator/tps51632-regulator.c b/drivers/regulator/tps51632-regulator.c
index f31f22e..c917b54 100644
--- a/drivers/regulator/tps51632-regulator.c
+++ b/drivers/regulator/tps51632-regulator.c
@@ -221,7 +221,8 @@ static const struct of_device_id tps51632_of_match[] = {
 MODULE_DEVICE_TABLE(of, tps51632_of_match);
 
 static struct tps51632_regulator_platform_data *
-	of_get_tps51632_platform_data(struct device *dev)
+	of_get_tps51632_platform_data(struct device *dev,
+				      struct regulator_desc *desc)
 {
 	struct tps51632_regulator_platform_data *pdata;
 	struct device_node *np = dev->of_node;
@@ -230,7 +231,8 @@ static struct tps51632_regulator_platform_data *
 	if (!pdata)
 		return NULL;
 
-	pdata->reg_init_data = of_get_regulator_init_data(dev, dev->of_node);
+	pdata->reg_init_data = of_get_regulator_init_data(dev, dev->of_node,
+							  desc);
 	if (!pdata->reg_init_data) {
 		dev_err(dev, "Not able to get OF regulator init data\n");
 		return NULL;
@@ -273,9 +275,13 @@ static int tps51632_probe(struct i2c_client *client,
 		}
 	}
 
+	tps = devm_kzalloc(&client->dev, sizeof(*tps), GFP_KERNEL);
+	if (!tps)
+		return -ENOMEM;
+
 	pdata = dev_get_platdata(&client->dev);
 	if (!pdata && client->dev.of_node)
-		pdata = of_get_tps51632_platform_data(&client->dev);
+		pdata = of_get_tps51632_platform_data(&client->dev, &tps->desc);
 	if (!pdata) {
 		dev_err(&client->dev, "No Platform data\n");
 		return -EINVAL;
@@ -296,10 +302,6 @@ static int tps51632_probe(struct i2c_client *client,
 		}
 	}
 
-	tps = devm_kzalloc(&client->dev, sizeof(*tps), GFP_KERNEL);
-	if (!tps)
-		return -ENOMEM;
-
 	tps->dev = &client->dev;
 	tps->desc.name = client->name;
 	tps->desc.id = 0;
diff --git a/drivers/regulator/tps62360-regulator.c b/drivers/regulator/tps62360-regulator.c
index a167204..be1f401 100644
--- a/drivers/regulator/tps62360-regulator.c
+++ b/drivers/regulator/tps62360-regulator.c
@@ -293,7 +293,8 @@ static const struct regmap_config tps62360_regmap_config = {
 };
 
 static struct tps62360_regulator_platform_data *
-	of_get_tps62360_platform_data(struct device *dev)
+	of_get_tps62360_platform_data(struct device *dev,
+				      struct regulator_desc *desc)
 {
 	struct tps62360_regulator_platform_data *pdata;
 	struct device_node *np = dev->of_node;
@@ -302,7 +303,8 @@ static struct tps62360_regulator_platform_data *
 	if (!pdata)
 		return NULL;
 
-	pdata->reg_init_data = of_get_regulator_init_data(dev, dev->of_node);
+	pdata->reg_init_data = of_get_regulator_init_data(dev, dev->of_node,
+							  desc);
 	if (!pdata->reg_init_data) {
 		dev_err(dev, "Not able to get OF regulator init data\n");
 		return NULL;
@@ -350,6 +352,10 @@ static int tps62360_probe(struct i2c_client *client,
 
 	pdata = dev_get_platdata(&client->dev);
 
+	tps = devm_kzalloc(&client->dev, sizeof(*tps), GFP_KERNEL);
+	if (!tps)
+		return -ENOMEM;
+
 	if (client->dev.of_node) {
 		const struct of_device_id *match;
 		match = of_match_device(of_match_ptr(tps62360_of_match),
@@ -360,7 +366,8 @@ static int tps62360_probe(struct i2c_client *client,
 		}
 		chip_id = (int)(long)match->data;
 		if (!pdata)
-			pdata = of_get_tps62360_platform_data(&client->dev);
+			pdata = of_get_tps62360_platform_data(&client->dev,
+							      &tps->desc);
 	} else if (id) {
 		chip_id = id->driver_data;
 	} else {
@@ -374,10 +381,6 @@ static int tps62360_probe(struct i2c_client *client,
 		return -EIO;
 	}
 
-	tps = devm_kzalloc(&client->dev, sizeof(*tps), GFP_KERNEL);
-	if (!tps)
-		return -ENOMEM;
-
 	tps->en_discharge = pdata->en_discharge;
 	tps->en_internal_pulldn = pdata->en_internal_pulldn;
 	tps->vsel0_gpio = pdata->vsel0_gpio;
diff --git a/drivers/regulator/tps65218-regulator.c b/drivers/regulator/tps65218-regulator.c
index f0a4028..263cc85 100644
--- a/drivers/regulator/tps65218-regulator.c
+++ b/drivers/regulator/tps65218-regulator.c
@@ -231,7 +231,8 @@ static int tps65218_regulator_probe(struct platform_device *pdev)
 
 	template = match->data;
 	id = template->id;
-	init_data = of_get_regulator_init_data(&pdev->dev, pdev->dev.of_node);
+	init_data = of_get_regulator_init_data(&pdev->dev, pdev->dev.of_node,
+					       &regulators[id]);
 
 	platform_set_drvdata(pdev, tps);
 
diff --git a/drivers/regulator/twl-regulator.c b/drivers/regulator/twl-regulator.c
index 0b4f866..dd727bc 100644
--- a/drivers/regulator/twl-regulator.c
+++ b/drivers/regulator/twl-regulator.c
@@ -1104,7 +1104,8 @@ static int twlreg_probe(struct platform_device *pdev)
 		template = match->data;
 		id = template->desc.id;
 		initdata = of_get_regulator_init_data(&pdev->dev,
-						      pdev->dev.of_node);
+						      pdev->dev.of_node,
+						      &template->desc);
 		drvdata = NULL;
 	} else {
 		id = pdev->id;
diff --git a/drivers/regulator/vexpress.c b/drivers/regulator/vexpress.c
index 02e7267..5e7c789 100644
--- a/drivers/regulator/vexpress.c
+++ b/drivers/regulator/vexpress.c
@@ -74,7 +74,8 @@ static int vexpress_regulator_probe(struct platform_device *pdev)
 	reg->desc.owner = THIS_MODULE;
 	reg->desc.continuous_voltage_range = true;
 
-	init_data = of_get_regulator_init_data(&pdev->dev, pdev->dev.of_node);
+	init_data = of_get_regulator_init_data(&pdev->dev, pdev->dev.of_node,
+					       &reg->desc);
 	if (!init_data)
 		return -EINVAL;
 
diff --git a/include/linux/regulator/of_regulator.h b/include/linux/regulator/of_regulator.h
index f921796..3bbfb1b 100644
--- a/include/linux/regulator/of_regulator.h
+++ b/include/linux/regulator/of_regulator.h
@@ -6,6 +6,8 @@
 #ifndef __LINUX_OF_REG_H
 #define __LINUX_OF_REG_H
 
+#include <linux/regulator/driver.h>
+
 struct of_regulator_match {
 	const char *name;
 	void *driver_data;
@@ -16,14 +18,16 @@ struct of_regulator_match {
 #if defined(CONFIG_OF)
 extern struct regulator_init_data
 	*of_get_regulator_init_data(struct device *dev,
-				    struct device_node *node);
+				    struct device_node *node,
+				    const struct regulator_desc *desc);
 extern int of_regulator_match(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,
-				    struct device_node *node)
+				    struct device_node *node,
+				    const struct regulator_desc *desc)
 {
 	return NULL;
 }
-- 
2.1.0

Re: [PATCH v5 3/5] regulator: of: Add regulator desc param to of_get_regulator_init_data()

[Mark Brown]

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

On Fri, Nov 07, 2014 at 02:00:03PM +0100, Javier Martinez Canillas wrote:

> -	initdata = of_get_regulator_init_data(dev, np);
>  	sreg = devm_kzalloc(dev, sizeof(*sreg), GFP_KERNEL);
>  	if (!sreg)
>  		return -ENOMEM;
> -	sreg->initdata = initdata;
>  	sreg->name = of_get_property(np, "regulator-name", NULL);
>  	rdesc = &sreg->rdesc;
> +	initdata = of_get_regulator_init_data(dev, np, rdesc);
> +	sreg->initdata = initdata;
>  	rdesc->name = sreg->name;
>  	rdesc->type = REGULATOR_VOLTAGE;
>  	rdesc->owner = THIS_MODULE;

This is using the regulator descriptor before it is initialized which
doesn't seem ideal...

> +++ b/include/linux/regulator/of_regulator.h
> @@ -6,6 +6,8 @@
>  #ifndef __LINUX_OF_REG_H
>  #define __LINUX_OF_REG_H
>  
> +#include <linux/regulator/driver.h>
> +
>  struct of_regulator_match {
>  	const char *name;
>  	void *driver_data;
> @@ -16,14 +18,16 @@ struct of_regulator_match {
>  #if defined(CONFIG_OF)
>  extern struct regulator_init_data
>  	*of_get_regulator_init_data(struct device *dev,
> -				    struct device_node *node);
> +				    struct device_node *node,
> +				    const struct regulator_desc *desc);

This is just adding the include to get the declaration of regulator_desc
as far as I can see, add a forward declaration of it instead.

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

Re: [PATCH v5 3/5] regulator: of: Add regulator desc param to of_get_regulator_init_data()

[Javier Martinez Canillas]

Hello Mark,

On 11/07/2014 04:07 PM, Mark Brown wrote:
> 
> This is using the regulator descriptor before it is initialized which
> doesn't seem ideal...
>

You are right, even if most of them are not used currently, that may
change in the future so is safer to use it after all fields have been
initialized. I'll double check all drivers to be sure that's the case.

> 
> This is just adding the include to get the declaration of regulator_desc
> as far as I can see, add a forward declaration of it instead.
> 

Perfect, I didn't know what you would prefer. I'll change it in the
next version.

Best regards,
Javier
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 3/5] regulator: of: Add regulator desc param to of_get_regulator_init_data()

[Krzysztof Kozlowski]

On pią, 2014-11-07 at 14:00 +0100, Javier Martinez Canillas wrote:
> The of_get_regulator_init_data() function is used to extract the regulator
> init_data but information on how to extract certain data is defined in the
> static regulator descriptor (e.g: how to map the hardware operating modes).
> 
> Add a const struct regulator_desc * parameter to the function signature so
> the parsing logic could use the information in the struct regulator_desc.
> 
> of_get_regulator_init_data() relies on of_get_regulation_constraints() to
> actually extract the init_data so it has to pass the struct regulator_desc
> but that is modified on a later patch.
> 
> Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>

(this looks fine)
(...)

> diff --git a/drivers/regulator/fan53555.c b/drivers/regulator/fan53555.c
> index f8e4257..37b671c 100644
> --- a/drivers/regulator/fan53555.c
> +++ b/drivers/regulator/fan53555.c
> @@ -302,7 +302,8 @@ static struct regmap_config fan53555_regmap_config = {
>  };
>  
>  static struct fan53555_platform_data *fan53555_parse_dt(struct device *dev,
> -							struct device_node *np)
> +						struct device_node *np,
> +						struct regulator_desc *desc)

Not a const? Why not?

>  {
>  	struct fan53555_platform_data *pdata;
>  	int ret;
> @@ -312,7 +313,7 @@ static struct fan53555_platform_data *fan53555_parse_dt(struct device *dev,
>  	if (!pdata)
>  		return NULL;
>  
> -	pdata->regulator = of_get_regulator_init_data(dev, np);
> +	pdata->regulator = of_get_regulator_init_data(dev, np, desc);

Desc would be always NULL here which is safe (check in patch 5/5) but if
someone puts "regulator-initial-mode" in DTS then this will always print
warning. Just wonder - is it actually what you wanted?

This applies also for tps51632 and tps62360.

>  
>  	ret = of_property_read_u32(np, "fcs,suspend-voltage-selector",
>  				   &tmp);
> @@ -347,20 +348,20 @@ static int fan53555_regulator_probe(struct i2c_client *client,
>  	unsigned int val;
>  	int ret;
>  
> +	di = devm_kzalloc(&client->dev, sizeof(struct fan53555_device_info),
> +					GFP_KERNEL);
> +	if (!di)
> +		return -ENOMEM;
> +
>  	pdata = dev_get_platdata(&client->dev);
>  	if (!pdata)
> -		pdata = fan53555_parse_dt(&client->dev, np);
> +		pdata = fan53555_parse_dt(&client->dev, np, &di->desc);
>  
>  	if (!pdata || !pdata->regulator) {
>  		dev_err(&client->dev, "Platform data not found!\n");
>  		return -ENODEV;
>  	}
>  
> -	di = devm_kzalloc(&client->dev, sizeof(struct fan53555_device_info),
> -					GFP_KERNEL);
> -	if (!di)
> -		return -ENOMEM;
> -
>  	di->regulator = pdata->regulator;
>  	if (client->dev.of_node) {
>  		const struct of_device_id *match;
> diff --git a/drivers/regulator/fixed.c b/drivers/regulator/fixed.c
> index 354105e..649fece 100644
> --- a/drivers/regulator/fixed.c
> +++ b/drivers/regulator/fixed.c
> @@ -40,13 +40,14 @@ struct fixed_voltage_data {
>  /**
>   * of_get_fixed_voltage_config - extract fixed_voltage_config structure info
>   * @dev: device requesting for fixed_voltage_config
> + * @desc: regulator description
>   *
>   * Populates fixed_voltage_config structure by extracting data from device
>   * tree node, returns a pointer to the populated structure of NULL if memory
>   * alloc fails.
>   */
>  static struct fixed_voltage_config *
> -of_get_fixed_voltage_config(struct device *dev)
> +of_get_fixed_voltage_config(struct device *dev, struct regulator_desc *desc)

Not const?

>  {
>  	struct fixed_voltage_config *config;
>  	struct device_node *np = dev->of_node;
> @@ -57,7 +58,7 @@ of_get_fixed_voltage_config(struct device *dev)
>  	if (!config)
>  		return ERR_PTR(-ENOMEM);
>  
> -	config->init_data = of_get_regulator_init_data(dev, dev->of_node);
> +	config->init_data = of_get_regulator_init_data(dev, dev->of_node, desc);
>  	if (!config->init_data)
>  		return ERR_PTR(-EINVAL);
>  
> @@ -112,8 +113,14 @@ static int reg_fixed_voltage_probe(struct platform_device *pdev)
>  	struct regulator_config cfg = { };
>  	int ret;
>  
> +	drvdata = devm_kzalloc(&pdev->dev, sizeof(struct fixed_voltage_data),
> +			       GFP_KERNEL);
> +	if (!drvdata)
> +		return -ENOMEM;
> +
>  	if (pdev->dev.of_node) {
> -		config = of_get_fixed_voltage_config(&pdev->dev);
> +		config = of_get_fixed_voltage_config(&pdev->dev,
> +						     &drvdata->desc);
>  		if (IS_ERR(config))
>  			return PTR_ERR(config);
>  	} else {
> @@ -123,11 +130,6 @@ static int reg_fixed_voltage_probe(struct platform_device *pdev)
>  	if (!config)
>  		return -ENOMEM;
>  
> -	drvdata = devm_kzalloc(&pdev->dev, sizeof(struct fixed_voltage_data),
> -			       GFP_KERNEL);
> -	if (!drvdata)
> -		return -ENOMEM;
> -
>  	drvdata->desc.name = devm_kstrdup(&pdev->dev,
>  					  config->supply_name,
>  					  GFP_KERNEL);
> diff --git a/drivers/regulator/gpio-regulator.c b/drivers/regulator/gpio-regulator.c
> index 989b23b..be0d08e 100644
> --- a/drivers/regulator/gpio-regulator.c
> +++ b/drivers/regulator/gpio-regulator.c
> @@ -133,7 +133,8 @@ static struct regulator_ops gpio_regulator_voltage_ops = {
>  };
>  
>  static struct gpio_regulator_config *
> -of_get_gpio_regulator_config(struct device *dev, struct device_node *np)
> +of_get_gpio_regulator_config(struct device *dev, struct device_node *np,
> +			     struct regulator_desc *desc)

Not const?

>  {
>  	struct gpio_regulator_config *config;
>  	const char *regtype;
> @@ -146,7 +147,7 @@ of_get_gpio_regulator_config(struct device *dev, struct device_node *np)
>  	if (!config)
>  		return ERR_PTR(-ENOMEM);
>  
> -	config->init_data = of_get_regulator_init_data(dev, np);
> +	config->init_data = of_get_regulator_init_data(dev, np, desc);
>  	if (!config->init_data)
>  		return ERR_PTR(-EINVAL);
>  
> @@ -243,17 +244,18 @@ static int gpio_regulator_probe(struct platform_device *pdev)
>  	struct regulator_config cfg = { };
>  	int ptr, ret, state;
>  
> -	if (np) {
> -		config = of_get_gpio_regulator_config(&pdev->dev, np);
> -		if (IS_ERR(config))
> -			return PTR_ERR(config);
> -	}
> -
>  	drvdata = devm_kzalloc(&pdev->dev, sizeof(struct gpio_regulator_data),
>  			       GFP_KERNEL);
>  	if (drvdata == NULL)
>  		return -ENOMEM;
>  
> +	if (np) {
> +		config = of_get_gpio_regulator_config(&pdev->dev, np,
> +						      &drvdata->desc);
> +		if (IS_ERR(config))
> +			return PTR_ERR(config);
> +	}
> +
>  	drvdata->desc.name = kstrdup(config->supply_name, GFP_KERNEL);
>  	if (drvdata->desc.name == NULL) {
>  		dev_err(&pdev->dev, "Failed to allocate supply name\n");
> diff --git a/drivers/regulator/max8952.c b/drivers/regulator/max8952.c
> index f7f9efc..6e54d78 100644
> --- a/drivers/regulator/max8952.c
> +++ b/drivers/regulator/max8952.c
> @@ -174,7 +174,7 @@ static struct max8952_platform_data *max8952_parse_dt(struct device *dev)
>  	if (of_property_read_u32(np, "max8952,ramp-speed", &pd->ramp_speed))
>  		dev_warn(dev, "max8952,ramp-speed property not specified, defaulting to 32mV/us\n");
>  
> -	pd->reg_data = of_get_regulator_init_data(dev, np);
> +	pd->reg_data = of_get_regulator_init_data(dev, np, &regulator);
>  	if (!pd->reg_data) {
>  		dev_err(dev, "Failed to parse regulator init data\n");
>  		return NULL;
> diff --git a/drivers/regulator/max8973-regulator.c b/drivers/regulator/max8973-regulator.c
> index dbedf17..c3d55c2 100644
> --- a/drivers/regulator/max8973-regulator.c
> +++ b/drivers/regulator/max8973-regulator.c
> @@ -458,7 +458,8 @@ static int max8973_probe(struct i2c_client *client,
>  
>  	config.dev = &client->dev;
>  	config.init_data = pdata ? pdata->reg_init_data :
> -		of_get_regulator_init_data(&client->dev, client->dev.of_node);
> +		of_get_regulator_init_data(&client->dev, client->dev.of_node,
> +					   &max->desc);
>  	config.driver_data = max;
>  	config.of_node = client->dev.of_node;
>  	config.regmap = max->regmap;
> diff --git a/drivers/regulator/max8997.c b/drivers/regulator/max8997.c
> index 9c31e21..726fde1 100644
> --- a/drivers/regulator/max8997.c
> +++ b/drivers/regulator/max8997.c
> @@ -953,7 +953,8 @@ static int max8997_pmic_dt_parse_pdata(struct platform_device *pdev,
>  
>  		rdata->id = i;
>  		rdata->initdata = of_get_regulator_init_data(&pdev->dev,
> -							     reg_np);
> +							     reg_np,
> +							     &regulators[i]);
>  		rdata->reg_node = reg_np;
>  		rdata++;
>  	}
> diff --git a/drivers/regulator/max8998.c b/drivers/regulator/max8998.c
> index 961091b..59e34a0 100644
> --- a/drivers/regulator/max8998.c
> +++ b/drivers/regulator/max8998.c
> @@ -686,8 +686,9 @@ static int max8998_pmic_dt_parse_pdata(struct max8998_dev *iodev,
>  			continue;
>  
>  		rdata->id = regulators[i].id;
> -		rdata->initdata = of_get_regulator_init_data(
> -							iodev->dev, reg_np);
> +		rdata->initdata = of_get_regulator_init_data(iodev->dev,
> +							     reg_np,
> +							     &regulators[i]);
>  		rdata->reg_node = reg_np;
>  		++rdata;
>  	}
> diff --git a/drivers/regulator/mc13xxx-regulator-core.c b/drivers/regulator/mc13xxx-regulator-core.c
> index afba024..0281c31 100644
> --- a/drivers/regulator/mc13xxx-regulator-core.c
> +++ b/drivers/regulator/mc13xxx-regulator-core.c
> @@ -194,7 +194,8 @@ struct mc13xxx_regulator_init_data *mc13xxx_parse_regulators_dt(
>  					 regulators[i].desc.name)) {
>  				p->id = i;
>  				p->init_data = of_get_regulator_init_data(
> -							&pdev->dev, child);
> +							&pdev->dev, child,
> +							&regulators[i].desc);
>  				p->node = child;
>  				p++;
>  
> diff --git a/drivers/regulator/of_regulator.c b/drivers/regulator/of_regulator.c
> index 03edb17..945486f 100644
> --- a/drivers/regulator/of_regulator.c
> +++ b/drivers/regulator/of_regulator.c
> @@ -120,13 +120,16 @@ static void of_get_regulation_constraints(struct device_node *np,
>  /**
>   * of_get_regulator_init_data - extract regulator_init_data structure info
>   * @dev: device requesting for regulator_init_data
> + * @node: regulator device node
> + * @desc: regulator description
>   *
>   * Populates regulator_init_data structure by extracting data from device
>   * tree node, returns a pointer to the populated struture or NULL if memory
>   * alloc fails.
>   */
>  struct regulator_init_data *of_get_regulator_init_data(struct device *dev,
> -						struct device_node *node)
> +					  struct device_node *node,
> +					  const struct regulator_desc *desc)
>  {
>  	struct regulator_init_data *init_data;
>  
> @@ -218,7 +221,7 @@ int of_regulator_match(struct device *dev, struct device_node *node,
>  				continue;
>  
>  			match->init_data =
> -				of_get_regulator_init_data(dev, child);
> +				of_get_regulator_init_data(dev, child, NULL);
>  			if (!match->init_data) {
>  				dev_err(dev,
>  					"failed to parse DT for regulator %s\n",
> @@ -266,7 +269,7 @@ struct regulator_init_data *regulator_of_get_init_data(struct device *dev,
>  		if (strcmp(desc->of_match, name))
>  			continue;
>  
> -		init_data = of_get_regulator_init_data(dev, child);
> +		init_data = of_get_regulator_init_data(dev, child, desc);
>  		if (!init_data) {
>  			dev_err(dev,
>  				"failed to parse DT for regulator %s\n",
> diff --git a/drivers/regulator/pwm-regulator.c b/drivers/regulator/pwm-regulator.c
> index d3f55ea..91f34ca 100644
> --- a/drivers/regulator/pwm-regulator.c
> +++ b/drivers/regulator/pwm-regulator.c
> @@ -149,7 +149,8 @@ static int pwm_regulator_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> -	config.init_data = of_get_regulator_init_data(&pdev->dev, np);
> +	config.init_data = of_get_regulator_init_data(&pdev->dev, np,
> +						      &drvdata->desc);
>  	if (!config.init_data)
>  		return -ENOMEM;
>  
> diff --git a/drivers/regulator/qcom_rpm-regulator.c b/drivers/regulator/qcom_rpm-regulator.c
> index b55cd5b..dabd28a 100644
> --- a/drivers/regulator/qcom_rpm-regulator.c
> +++ b/drivers/regulator/qcom_rpm-regulator.c
> @@ -643,10 +643,6 @@ static int rpm_reg_probe(struct platform_device *pdev)
>  	match = of_match_device(rpm_of_match, &pdev->dev);
>  	template = match->data;
>  
> -	initdata = of_get_regulator_init_data(&pdev->dev, pdev->dev.of_node);
> -	if (!initdata)
> -		return -EINVAL;
> -
>  	vreg = devm_kmalloc(&pdev->dev, sizeof(*vreg), GFP_KERNEL);
>  	if (!vreg) {
>  		dev_err(&pdev->dev, "failed to allocate vreg\n");
> @@ -666,6 +662,11 @@ static int rpm_reg_probe(struct platform_device *pdev)
>  		return -ENODEV;
>  	}
>  
> +	initdata = of_get_regulator_init_data(&pdev->dev, pdev->dev.of_node,
> +					      &vreg->desc);
> +	if (!initdata)
> +		return -EINVAL;
> +
>  	key = "reg";
>  	ret = of_property_read_u32(pdev->dev.of_node, key, &val);
>  	if (ret) {
> diff --git a/drivers/regulator/s5m8767.c b/drivers/regulator/s5m8767.c
> index 0ab5cbe..26932fe 100644
> --- a/drivers/regulator/s5m8767.c
> +++ b/drivers/regulator/s5m8767.c
> @@ -581,7 +581,8 @@ static int s5m8767_pmic_dt_parse_pdata(struct platform_device *pdev,
>  
>  		rdata->id = i;
>  		rdata->initdata = of_get_regulator_init_data(
> -						&pdev->dev, reg_np);
> +						&pdev->dev, reg_np,
> +						&regulators[i]);
>  		rdata->reg_node = reg_np;
>  		rdata++;
>  		rmode->id = i;
> diff --git a/drivers/regulator/sky81452-regulator.c b/drivers/regulator/sky81452-regulator.c
> index 476b80a..e68e13f 100644
> --- a/drivers/regulator/sky81452-regulator.c
> +++ b/drivers/regulator/sky81452-regulator.c
> @@ -76,7 +76,7 @@ static struct regulator_init_data *sky81452_reg_parse_dt(struct device *dev)
>  		return NULL;
>  	}
>  
> -	init_data = of_get_regulator_init_data(dev, np);
> +	init_data = of_get_regulator_init_data(dev, np, &sky81452_reg);
>  
>  	of_node_put(np);
>  	return init_data;
> diff --git a/drivers/regulator/stw481x-vmmc.c b/drivers/regulator/stw481x-vmmc.c
> index a7e1526..b4f1696 100644
> --- a/drivers/regulator/stw481x-vmmc.c
> +++ b/drivers/regulator/stw481x-vmmc.c
> @@ -72,7 +72,8 @@ static int stw481x_vmmc_regulator_probe(struct platform_device *pdev)
>  	config.regmap = stw481x->map;
>  	config.of_node = pdev->dev.of_node;
>  	config.init_data = of_get_regulator_init_data(&pdev->dev,
> -						      pdev->dev.of_node);
> +						      pdev->dev.of_node,
> +						      &vmmc_regulator);
>  
>  	stw481x->vmmc_regulator = devm_regulator_register(&pdev->dev,
>  						&vmmc_regulator, &config);
> diff --git a/drivers/regulator/ti-abb-regulator.c b/drivers/regulator/ti-abb-regulator.c
> index a2dabb5..1ef5aba 100644
> --- a/drivers/regulator/ti-abb-regulator.c
> +++ b/drivers/regulator/ti-abb-regulator.c
> @@ -837,7 +837,8 @@ skip_opt:
>  		return -EINVAL;
>  	}
>  
> -	initdata = of_get_regulator_init_data(dev, pdev->dev.of_node);
> +	initdata = of_get_regulator_init_data(dev, pdev->dev.of_node,
> +					      &abb->rdesc);
>  	if (!initdata) {
>  		dev_err(dev, "%s: Unable to alloc regulator init data\n",
>  			__func__);
> diff --git a/drivers/regulator/tps51632-regulator.c b/drivers/regulator/tps51632-regulator.c
> index f31f22e..c917b54 100644
> --- a/drivers/regulator/tps51632-regulator.c
> +++ b/drivers/regulator/tps51632-regulator.c
> @@ -221,7 +221,8 @@ static const struct of_device_id tps51632_of_match[] = {
>  MODULE_DEVICE_TABLE(of, tps51632_of_match);
>  
>  static struct tps51632_regulator_platform_data *
> -	of_get_tps51632_platform_data(struct device *dev)
> +	of_get_tps51632_platform_data(struct device *dev,
> +				      struct regulator_desc *desc)

Not const?

>  {
>  	struct tps51632_regulator_platform_data *pdata;
>  	struct device_node *np = dev->of_node;
> @@ -230,7 +231,8 @@ static struct tps51632_regulator_platform_data *
>  	if (!pdata)
>  		return NULL;
>  
> -	pdata->reg_init_data = of_get_regulator_init_data(dev, dev->of_node);
> +	pdata->reg_init_data = of_get_regulator_init_data(dev, dev->of_node,
> +							  desc);
>  	if (!pdata->reg_init_data) {
>  		dev_err(dev, "Not able to get OF regulator init data\n");
>  		return NULL;
> @@ -273,9 +275,13 @@ static int tps51632_probe(struct i2c_client *client,
>  		}
>  	}
>  
> +	tps = devm_kzalloc(&client->dev, sizeof(*tps), GFP_KERNEL);
> +	if (!tps)
> +		return -ENOMEM;
> +
>  	pdata = dev_get_platdata(&client->dev);
>  	if (!pdata && client->dev.of_node)
> -		pdata = of_get_tps51632_platform_data(&client->dev);
> +		pdata = of_get_tps51632_platform_data(&client->dev, &tps->desc);
>  	if (!pdata) {
>  		dev_err(&client->dev, "No Platform data\n");
>  		return -EINVAL;
> @@ -296,10 +302,6 @@ static int tps51632_probe(struct i2c_client *client,
>  		}
>  	}
>  
> -	tps = devm_kzalloc(&client->dev, sizeof(*tps), GFP_KERNEL);
> -	if (!tps)
> -		return -ENOMEM;
> -
>  	tps->dev = &client->dev;
>  	tps->desc.name = client->name;
>  	tps->desc.id = 0;
> diff --git a/drivers/regulator/tps62360-regulator.c b/drivers/regulator/tps62360-regulator.c
> index a167204..be1f401 100644
> --- a/drivers/regulator/tps62360-regulator.c
> +++ b/drivers/regulator/tps62360-regulator.c
> @@ -293,7 +293,8 @@ static const struct regmap_config tps62360_regmap_config = {
>  };
>  
>  static struct tps62360_regulator_platform_data *
> -	of_get_tps62360_platform_data(struct device *dev)
> +	of_get_tps62360_platform_data(struct device *dev,
> +				      struct regulator_desc *desc)

Not const?


(...)

Rest looks fine.

Krzysztof

Re: [PATCH v5 3/5] regulator: of: Add regulator desc param to of_get_regulator_init_data()

[Javier Martinez Canillas]

Hello Krzysztof,

On 11/07/2014 04:23 PM, Krzysztof Kozlowski wrote:
>>  
>>  static struct fan53555_platform_data *fan53555_parse_dt(struct device *dev,
>> -							struct device_node *np)
>> +						struct device_node *np,
>> +						struct regulator_desc *desc)
> 
> Not a const? Why not?
>

Right, I removed all your other mentions to the same issue but I'll double
check all the drivers to be sure that use const.
 
>>  {
>>  	struct fan53555_platform_data *pdata;
>>  	int ret;
>> @@ -312,7 +313,7 @@ static struct fan53555_platform_data *fan53555_parse_dt(struct device *dev,
>>  	if (!pdata)
>>  		return NULL;
>>  
>> -	pdata->regulator = of_get_regulator_init_data(dev, np);
>> +	pdata->regulator = of_get_regulator_init_data(dev, np, desc);
> 
> Desc would be always NULL here which is safe (check in patch 5/5) but if
> someone puts "regulator-initial-mode" in DTS then this will always print
> warning. Just wonder - is it actually what you wanted?
> 
> This applies also for tps51632 and tps62360.
>

In an early version of the series I just passed NULL for all drivers but
Mark said that is more future proof to pass the descriptor and I agree.

I guess I should just remove the warning and only show an error if the
descriptor is not NULL and also the map mode function is defined but
the callback returns an error.
 
> (...)
> 
> Rest looks fine.
>

Thanks a lot for the review.
 
> Krzysztof
> 
> 

Best regards,
Javier

[PATCH v5 4/5] regulator: of: Pass the regulator description in the match table

[Javier Martinez Canillas]

Drivers can use the of_regulator_match() function to parse the regulator
init_data from DT. A match table is used to specify the name of the node
containing the regulators, the device node and to return the init_data
to the caller.

But also the static regulator descriptor is needed to correctly extract
some DT properties like the regulator initial and suspend modes. Use the
match table to pass that information.

Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
---

This patch only adds the struct regulator_desc pointer to the struct
of_regulator_match and does not fill this info in all drivers using
the of_regulator_match() function to keep the series short.
A following series will change all regulator drivers calling this
function so the descriptor is passed even when currently isn't used.

 drivers/regulator/of_regulator.c       | 3 ++-
 include/linux/regulator/of_regulator.h | 1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/regulator/of_regulator.c b/drivers/regulator/of_regulator.c
index 945486f..cbc1d71 100644
--- a/drivers/regulator/of_regulator.c
+++ b/drivers/regulator/of_regulator.c
@@ -221,7 +221,8 @@ int of_regulator_match(struct device *dev, struct device_node *node,
 				continue;
 
 			match->init_data =
-				of_get_regulator_init_data(dev, child, NULL);
+				of_get_regulator_init_data(dev, child,
+							   match->desc);
 			if (!match->init_data) {
 				dev_err(dev,
 					"failed to parse DT for regulator %s\n",
diff --git a/include/linux/regulator/of_regulator.h b/include/linux/regulator/of_regulator.h
index 3bbfb1b..ce0877d 100644
--- a/include/linux/regulator/of_regulator.h
+++ b/include/linux/regulator/of_regulator.h
@@ -13,6 +13,7 @@ struct of_regulator_match {
 	void *driver_data;
 	struct regulator_init_data *init_data;
 	struct device_node *of_node;
+	const struct regulator_desc *desc;
 };
 
 #if defined(CONFIG_OF)
-- 
2.1.0

[PATCH v5 5/5] regulator: of: Add support for parsing initial and suspend modes

[Javier Martinez Canillas]

Some regulators support their operating mode to be changed on startup
or by consumers when the system is running while others only support
their operating mode to be changed while the system has entered in a
suspend state.

The regulator Device Tree binding documents a set of properties to
configure the regulators operating modes from a FDT. This patch builds
on (40e20d6 regulator: of: Add support for parsing regulator_state for
suspend state) and adds support to parse those properties and fill the
regulator constraints so the regulator core can call the right suspend
handlers when the system enters into sleep.

The modes are defined in the Device Tree using the hardware specific
modes supported by the regulators. Regulator drivers have to define a
translation function that is used to map the hardware specific modes
to the standard ones.

Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
---

Changes in v5: None

Changes in v4:
 - Parse the properties in the core and map using driver provided functions.
   Suggested by Mark Brown

Changes in v3:
 - Use the standard suspend states binding instead of custom properties.
   Suggested by Mark Brown

 drivers/regulator/of_regulator.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/drivers/regulator/of_regulator.c b/drivers/regulator/of_regulator.c
index cbc1d71..cd65885 100644
--- a/drivers/regulator/of_regulator.c
+++ b/drivers/regulator/of_regulator.c
@@ -25,7 +25,8 @@ static const char *const regulator_states[PM_SUSPEND_MAX + 1] = {
 };
 
 static void of_get_regulation_constraints(struct device_node *np,
-					struct regulator_init_data **init_data)
+					struct regulator_init_data **init_data,
+					const struct regulator_desc *desc)
 {
 	const __be32 *min_uV, *max_uV;
 	struct regulation_constraints *constraints = &(*init_data)->constraints;
@@ -81,6 +82,14 @@ static void of_get_regulation_constraints(struct device_node *np,
 	if (!ret)
 		constraints->enable_time = pval;
 
+	if (!of_property_read_u32(np, "regulator-initial-mode", &pval)) {
+		if (desc && desc->map_modes)
+			constraints->initial_mode = desc->map_modes(pval);
+		else
+			pr_warn("%s: failed to parse regulator-initial-mode\n",
+				np->name);
+	}
+
 	for (i = 0; i < ARRAY_SIZE(regulator_states); i++) {
 		switch (i) {
 		case PM_SUSPEND_MEM:
@@ -100,6 +109,15 @@ static void of_get_regulation_constraints(struct device_node *np,
 		if (!suspend_np || !suspend_state)
 			continue;
 
+		if (!of_property_read_u32(suspend_np, "regulator-mode",
+					  &pval)) {
+			if (desc && desc->map_modes)
+				suspend_state->mode = desc->map_modes(pval);
+			else
+				pr_warn("%s: failed to parse regulator-mode\n",
+					np->name);
+		}
+
 		if (of_property_read_bool(suspend_np,
 					"regulator-on-in-suspend"))
 			suspend_state->enabled = true;
@@ -140,7 +158,7 @@ struct regulator_init_data *of_get_regulator_init_data(struct device *dev,
 	if (!init_data)
 		return NULL; /* Out of memory? */
 
-	of_get_regulation_constraints(node, &init_data);
+	of_get_regulation_constraints(node, &init_data, desc);
 	return init_data;
 }
 EXPORT_SYMBOL_GPL(of_get_regulator_init_data);
-- 
2.1.0

Re: [PATCH v5 5/5] regulator: of: Add support for parsing initial and suspend modes

[Mark Brown]

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

On Fri, Nov 07, 2014 at 02:00:05PM +0100, Javier Martinez Canillas wrote:

> +	if (!of_property_read_u32(np, "regulator-initial-mode", &pval)) {
> +		if (desc && desc->map_modes)
> +			constraints->initial_mode = desc->map_modes(pval);
> +		else
> +			pr_warn("%s: failed to parse regulator-initial-mode\n",
> +				np->name);
> +	}

This is ignoring any error return from map_modes(), it's possible the DT
might have an invalid value.  The error message could also use some
improvement, it's more that the kernel doesn't understand how to parse
it even if it is valid.

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

Re: [PATCH v5 5/5] regulator: of: Add support for parsing initial and suspend modes

[Javier Martinez Canillas]

On 11/07/2014 04:47 PM, Mark Brown wrote:
> On Fri, Nov 07, 2014 at 02:00:05PM +0100, Javier Martinez Canillas wrote:
> 
>> +	if (!of_property_read_u32(np, "regulator-initial-mode", &pval)) {
>> +		if (desc && desc->map_modes)
>> +			constraints->initial_mode = desc->map_modes(pval);
>> +		else
>> +			pr_warn("%s: failed to parse regulator-initial-mode\n",
>> +				np->name);
>> +	}
> 
> This is ignoring any error return from map_modes(), it's possible the DT
> might have an invalid value.  The error message could also use some
> improvement, it's more that the kernel doesn't understand how to parse
> it even if it is valid.
> 

Right, as I mentioned to Krzysztof in a previous email, I'll remove the
warning message and add an error message instead if the map mode callback
function fails and also show the error code as you suggest.

Best regards,
Javier
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 5/5] regulator: of: Add support for parsing initial and suspend modes

[Mark Brown]

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

On Fri, Nov 07, 2014 at 05:15:38PM +0100, Javier Martinez Canillas wrote:
> On 11/07/2014 04:47 PM, Mark Brown wrote:

> >> +	if (!of_property_read_u32(np, "regulator-initial-mode", &pval)) {
> >> +		if (desc && desc->map_modes)
> >> +			constraints->initial_mode = desc->map_modes(pval);
> >> +		else
> >> +			pr_warn("%s: failed to parse regulator-initial-mode\n",
> >> +				np->name);
> >> +	}

> > This is ignoring any error return from map_modes(), it's possible the DT
> > might have an invalid value.  The error message could also use some
> > improvement, it's more that the kernel doesn't understand how to parse
> > it even if it is valid.

> Right, as I mentioned to Krzysztof in a previous email, I'll remove the
> warning message and add an error message instead if the map mode callback
> function fails and also show the error code as you suggest.

It seems reasonable to at least warn if there's a mode specified and
there's no way of understanding it...

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

Re: [PATCH v5 0/5] regulator: of: Add initial and suspend modes support

[Krzysztof Kozlowski]

On pią, 2014-11-07 at 14:00 +0100, Javier Martinez Canillas wrote:
> Hello Mark,
> 
> This is the fifth version of the series that adds regulator initial
> and suspend operating modes support. It relies on the existing work
> that added suspend states bindings. The opmodes are parsed by the
> regulator core and drivers should only define a translation function
> to map between hardware specific to standard modes.
> 
> The series adds a "regulator-initial-mode" property to configure at
> startup, the operating mode for the regulators that support changing
> its mode during normal operation and a "regulator-mode" property for
> the regulators that supports changing its operating mode when the
> system enters in a suspend state. These properties were originally
> part of Chanwoo Choi's regulator suspend state series [0] but were
> removed since there wasn't a way to define the operating modes in a
> generic way.
> 
> The generic regulator DT binding doc explains that each device has
> to document what their valid operating modes are and drivers must
> add a translation function so the core knows how to map the opmodes.
> 
> Older versions of this series were meant to add initial and suspend
> modes for the max77802 regulator driver but the feedback was that
> this should had been done in a generic way. The latest version was
> "[PATCH v4 00/14] Add Maxim 77802 PMIC support" [1] but that series
> mixed core changes, bugfixes and new driver features.
> 
> This series instead contains only the patches that add the support
> to the regulator core and drivers are only modified when a function
> signature is changed to maintain git bisect-ability.
> 
> If the patches are merged, following series will change the drivers
> using of_regulator_match() to pass the regulator description in the
> match table and another series will add the new opmode feature in
> the max77802 regulator driver.
> 
> The series is composed of the following patches:
> 
> Javier Martinez Canillas (5):
>   regulator: Document binding for initial and suspend modes
>   regulator: Add function to map modes to struct regulator_desc
>   regulator: of: Add regulator desc param to
>     of_get_regulator_init_data()
>   regulator: of: Pass the regulator description in the match table
>   regulator: of: Add support for parsing initial and suspend modes

I see my previous thoughts were addressed. I had few minor questions
about patch 3/5 but still whole patchset looks fine to me.

Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>

Best regards,
Krzysztof