[PATCH v2 0/5] Device tree support for regulators

[PATCH v2 0/5] Device tree support for regulators

[Rajendra Nayak]

Hi Mark, Grant,

I have reworked the regulator-dt-suppport series based
on your reviews and also split it so I remove any dependency
with the omap specific dt conversion for i2c/twl.
So this latest series is based on mainline 3.1-rc9.

I will post the twl-regulator driver adaptation to dt and
the omap-panda/omap-sdp board regulator data being
passed from dt as a seperate series.

changes in v2:
-1- removed the int to u32 convertions as -ve voltages do exist
-2- merged patches to support fixed voltage regulator dt adaptation
-3- added support for regulator_get() without a device associated (ex: cpufreq)
-4- used same binding for regulator<->consumer and regulator<->parent mapping

regards,
Rajendra

Rajendra Nayak (5):
  regulator: twl: Remove hardcoded board constraints from driver
  dt: add empty dt helpers for non-dt build
  regulator: helper routine to extract regulator_init_data
  regulator: adapt fixed regulator driver to dt
  regulator: map consumer regulator based on device tree

 .../bindings/regulator/fixed-regulator.txt         |   24 +++++
 .../devicetree/bindings/regulator/regulator.txt    |   44 +++++++++
 drivers/regulator/Kconfig                          |    8 ++
 drivers/regulator/Makefile                         |    1 +
 drivers/regulator/core.c                           |   91 ++++++++++++++++---
 drivers/regulator/fixed.c                          |   58 ++++++++++++
 drivers/regulator/of_regulator.c                   |   93 ++++++++++++++++++++
 drivers/regulator/twl-regulator.c                  |    8 --
 include/linux/of.h                                 |   19 ++++
 include/linux/regulator/driver.h                   |    2 +
 include/linux/regulator/of_regulator.h             |   21 +++++
 11 files changed, 346 insertions(+), 23 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/regulator/fixed-regulator.txt
 create mode 100644 Documentation/devicetree/bindings/regulator/regulator.txt
 create mode 100644 drivers/regulator/of_regulator.c
 create mode 100644 include/linux/regulator/of_regulator.h

[PATCH v2 1/5] regulator: twl: Remove hardcoded board constraints from driver

[Rajendra Nayak]

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

Signed-off-by: Rajendra Nayak <rnayak@ti.com>
Acked-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
---
 drivers/regulator/twl-regulator.c |    8 --------
 1 files changed, 0 insertions(+), 8 deletions(-)

diff --git a/drivers/regulator/twl-regulator.c b/drivers/regulator/twl-regulator.c
index ee8747f..f696287 100644
--- a/drivers/regulator/twl-regulator.c
+++ b/drivers/regulator/twl-regulator.c
@@ -1027,14 +1027,6 @@ static int __devinit twlreg_probe(struct platform_device *pdev)
 	/* copy the features into regulator data */
 	info->features = (unsigned long)initdata->driver_data;
 
-	/* Constrain board-specific capabilities according to what
-	 * this driver and the chip itself can actually do.
-	 */
-	c = &initdata->constraints;
-	c->valid_modes_mask &= REGULATOR_MODE_NORMAL | REGULATOR_MODE_STANDBY;
-	c->valid_ops_mask &= REGULATOR_CHANGE_VOLTAGE
-				| REGULATOR_CHANGE_MODE
-				| REGULATOR_CHANGE_STATUS;
 	switch (pdev->id) {
 	case TWL4030_REG_VIO:
 	case TWL4030_REG_VDD1:
-- 
1.7.1

[PATCH v2 1/5] regulator: twl: Remove hardcoded board constraints from driver

[Mark Brown]

On Mon, Oct 10, 2011 at 09:49:34PM +0530, Rajendra Nayak wrote:
> Remove the hardcoded .valid_modes_mask and .valid_ops_mask for
> each regulator from the twl driver and let the boards pass it.
> 
> Signed-off-by: Rajendra Nayak <rnayak@ti.com>
> Acked-by: Mark Brown <broonie@opensource.wolfsonmicro.com>

Please update this changelog - as I said in reply to your previous
posting this isn't actually hardcoding anything, it's dropping things
from the constraints which the hardware doesn't support.

[PATCH v2 1/5] regulator: twl: Remove hardcoded board constraints from driver

[Rajendra Nayak]

On Monday 10 October 2011 09:55 PM, Mark Brown wrote:
> On Mon, Oct 10, 2011 at 09:49:34PM +0530, Rajendra Nayak wrote:
>> Remove the hardcoded .valid_modes_mask and .valid_ops_mask for
>> each regulator from the twl driver and let the boards pass it.
>>
>> Signed-off-by: Rajendra Nayak<rnayak@ti.com>
>> Acked-by: Mark Brown<broonie@opensource.wolfsonmicro.com>
>
> Please update this changelog - as I said in reply to your previous
> posting this isn't actually hardcoding anything, it's dropping things
> from the constraints which the hardware doesn't support.

Looks like I completely missed the "&=" and your comment
on the previous posting. Looking at it now, seems to me
this patch is just un-necessary and what the driver seems
to be doing (preventing users from setting flags which the
hardware does not support) seems perfectly fine.
I will just go ahead and drop this patch.

[PATCH v2 2/5] dt: add empty dt helpers for non-dt build

[Rajendra Nayak]

Add empty of_device_is_compatible(), of_find_property()
and of_parse_phandle() for non-dt builds to work.

Signed-off-by: Rajendra Nayak <rnayak@ti.com>
---
 include/linux/of.h |   19 +++++++++++++++++++
 1 files changed, 19 insertions(+), 0 deletions(-)

diff --git a/include/linux/of.h b/include/linux/of.h
index 9180dc5..47ce461 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -263,6 +263,25 @@ static inline const void *of_get_property(const struct device_node *node,
 	return NULL;
 }
 
+static inline int of_device_is_compatible(const struct device_node *device,
+				   const char *name)
+{
+	return 0;
+}
+
+static inline struct property *of_find_property(const struct device_node *np,
+					 const char *name,
+					 int *lenp)
+{
+	return NULL;
+}
+
+static inline struct device_node *of_parse_phandle(struct device_node *np,
+					    const char *phandle_name,
+					    int index)
+{
+	return NULL;
+}
 #endif /* CONFIG_OF */
 
 static inline int of_property_read_u32(const struct device_node *np,
-- 
1.7.1

[PATCH v2 2/5] dt: add empty dt helpers for non-dt build

[Grant Likely]

On Mon, Oct 10, 2011 at 09:49:35PM +0530, Rajendra Nayak wrote:
> Add empty of_device_is_compatible(), of_find_property()
> and of_parse_phandle() for non-dt builds to work.
> 
> Signed-off-by: Rajendra Nayak <rnayak@ti.com>

Applied, thanks.  The of_find_property() already had an empty version
in devicetree/next, but I've picked up the other two.

g.

> ---
>  include/linux/of.h |   19 +++++++++++++++++++
>  1 files changed, 19 insertions(+), 0 deletions(-)
> 
> diff --git a/include/linux/of.h b/include/linux/of.h
> index 9180dc5..47ce461 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -263,6 +263,25 @@ static inline const void *of_get_property(const struct device_node *node,
>  	return NULL;
>  }
>  
> +static inline int of_device_is_compatible(const struct device_node *device,
> +				   const char *name)
> +{
> +	return 0;
> +}
> +
> +static inline struct property *of_find_property(const struct device_node *np,
> +					 const char *name,
> +					 int *lenp)
> +{
> +	return NULL;
> +}
> +
> +static inline struct device_node *of_parse_phandle(struct device_node *np,
> +					    const char *phandle_name,
> +					    int index)
> +{
> +	return NULL;
> +}
>  #endif /* CONFIG_OF */
>  
>  static inline int of_property_read_u32(const struct device_node *np,
> -- 
> 1.7.1
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 3/5] regulator: helper routine to extract regulator_init_data

[Rajendra Nayak]

The helper routine is meant to be used by regulator drivers
to extract the regulator_init_data structure from the data
that is passed from device tree.
'consumer_supplies' which is part of regulator_init_data is not extracted
as the regulator consumer mappings are passed through DT differently,
implemented in subsequent patches.
Similarly the regulator<-->parent/supply mapping is handled in
subsequent patches.

Also add documentation for regulator bindings to be used to pass
regulator_init_data struct information from device tree.

Signed-off-by: Rajendra Nayak <rnayak@ti.com>
---
 .../devicetree/bindings/regulator/regulator.txt    |   44 +++++++++
 drivers/regulator/Kconfig                          |    8 ++
 drivers/regulator/Makefile                         |    1 +
 drivers/regulator/of_regulator.c                   |   93 ++++++++++++++++++++
 include/linux/regulator/of_regulator.h             |   21 +++++
 5 files changed, 167 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/regulator/regulator.txt
 create mode 100644 drivers/regulator/of_regulator.c
 create mode 100644 include/linux/regulator/of_regulator.h

diff --git a/Documentation/devicetree/bindings/regulator/regulator.txt b/Documentation/devicetree/bindings/regulator/regulator.txt
new file mode 100644
index 0000000..a623fdd
--- /dev/null
+++ b/Documentation/devicetree/bindings/regulator/regulator.txt
@@ -0,0 +1,44 @@
+Voltage/Current Regulators
+
+Optional properties:
+- regulator-min-uV: smallest voltage consumers may set
+- regulator-max-uV: largest voltage consumers may set
+- regulator-uV-offset: Offset applied to voltages to compensate for voltage drops
+- regulator-min-uA: smallest current consumers may set
+- regulator-max-uA: largest current consumers may set
+- regulator-change-voltage: boolean, Output voltage can be changed by software
+- regulator-change-current: boolean, Output current can be chnaged by software
+- regulator-change-mode: boolean, Operating mode can be changed by software
+- regulator-change-status: boolean, Enable/Disable control for regulator exists
+- regulator-change-drms: boolean, Dynamic regulator mode switching is enabled
+- regulator-mode-fast: boolean, allow/set fast mode for the regulator
+- regulator-mode-normal: boolean, allow/set normal mode for the regulator
+- regulator-mode-idle: boolean, allow/set idle mode for the regulator
+- regulator-mode-standby: boolean, allow/set standby mode for the regulator
+- regulator-input-uV: Input voltage for regulator when supplied by another regulator
+- regulator-always-on: boolean, regulator should never be disabled
+- regulator-boot-on: bootloader/firmware enabled regulator
+- <name>-supply: phandle to the parent supply/regulator node
+
+Example:
+
+	xyzreg: regulator at 0 {
+		regulator-min-uV = <1000000>;
+		regulator-max-uV = <2500000>;
+		regulator-mode-fast;
+		regulator-change-voltage;
+		regulator-always-on;
+		vin-supply = <&vin>;
+	};
+
+The same binding used by a regulator to reference its
+supply can be used by any consumer to reference its
+regulator/supply
+
+Example of a device node referencing a regulator node,
+
+	devicenode: node at 0x0 {
+		...
+		...
+		<name>-supply = <&xyzreg>;
+	};
diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index c7fd2c0..981c92e 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -64,6 +64,14 @@ config REGULATOR_USERSPACE_CONSUMER
 
           If unsure, say no.
 
+config OF_REGULATOR
+	tristate "OF regulator helpers"
+	depends on OF
+	default y if OF
+	help
+	  OpenFirmware regulator framework helper routines that can
+	  used by regulator drivers to extract data from device tree.
+
 config REGULATOR_BQ24022
 	tristate "TI bq24022 Dual Input 1-Cell Li-Ion Charger IC"
 	help
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index 040d5aa..e6bc009 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -7,6 +7,7 @@ obj-$(CONFIG_REGULATOR) += core.o dummy.o
 obj-$(CONFIG_REGULATOR_FIXED_VOLTAGE) += fixed.o
 obj-$(CONFIG_REGULATOR_VIRTUAL_CONSUMER) += virtual.o
 obj-$(CONFIG_REGULATOR_USERSPACE_CONSUMER) += userspace-consumer.o
+obj-$(CONFIG_OF_REGULATOR) += of_regulator.o
 
 obj-$(CONFIG_REGULATOR_AD5398) += ad5398.o
 obj-$(CONFIG_REGULATOR_BQ24022) += bq24022.o
diff --git a/drivers/regulator/of_regulator.c b/drivers/regulator/of_regulator.c
new file mode 100644
index 0000000..9262d06
--- /dev/null
+++ b/drivers/regulator/of_regulator.c
@@ -0,0 +1,93 @@
+/*
+ * OF helpers for regulator framework
+ *
+ * Copyright (C) 2011 Texas Instruments, Inc.
+ * Rajendra Nayak <rnayak@ti.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <linux/slab.h>
+#include <linux/of.h>
+#include <linux/regulator/machine.h>
+
+static void of_get_regulation_constraints(struct device_node *np,
+					struct regulator_init_data **init_data)
+{
+	const __be32 *min_uV, *max_uV, *uV_offset;
+	const __be32 *min_uA, *max_uA, *input_uV;
+	unsigned int valid_modes_mask = 0, valid_ops_mask = 0;
+
+	min_uV = of_get_property(np, "regulator-min-uV", NULL);
+	if (min_uV)
+		(*init_data)->constraints.min_uV = be32_to_cpu(*min_uV);
+	max_uV = of_get_property(np, "regulator-max-uV", NULL);
+	if (max_uV)
+		(*init_data)->constraints.max_uV = be32_to_cpu(*max_uV);
+	uV_offset = of_get_property(np, "regulator-uV-offset", NULL);
+	if (uV_offset)
+		(*init_data)->constraints.uV_offset = be32_to_cpu(*uV_offset);
+	min_uA = of_get_property(np, "regulator-min-uA", NULL);
+	if (min_uA)
+		(*init_data)->constraints.min_uA = be32_to_cpu(*min_uA);
+	max_uA = of_get_property(np, "regulator-max-uA", NULL);
+	if (max_uA)
+		(*init_data)->constraints.max_uA = be32_to_cpu(*max_uA);
+	input_uV = of_get_property(np, "regulator-input-uV", NULL);
+	if (input_uV)
+		(*init_data)->constraints.input_uV = be32_to_cpu(*input_uV);
+
+	/* valid modes mask */
+	if (of_find_property(np, "regulator-mode-fast", NULL))
+				valid_modes_mask |= REGULATOR_MODE_FAST;
+	if (of_find_property(np, "regulator-mode-normal", NULL))
+				valid_modes_mask |= REGULATOR_MODE_NORMAL;
+	if (of_find_property(np, "regulator-mode-idle", NULL))
+				valid_modes_mask |= REGULATOR_MODE_IDLE;
+	if (of_find_property(np, "regulator-mode-standby", NULL))
+				valid_modes_mask |= REGULATOR_MODE_STANDBY;
+
+	/* valid ops mask */
+	if (of_find_property(np, "regulator-change-voltage", NULL))
+				valid_ops_mask |= REGULATOR_CHANGE_VOLTAGE;
+	if (of_find_property(np, "regulator-change-current", NULL))
+				valid_ops_mask |= REGULATOR_CHANGE_CURRENT;
+	if (of_find_property(np, "regulator-change-mode", NULL))
+				valid_ops_mask |= REGULATOR_CHANGE_MODE;
+	if (of_find_property(np, "regulator-change-status", NULL))
+				valid_ops_mask |= REGULATOR_CHANGE_STATUS;
+
+	(*init_data)->constraints.valid_modes_mask = valid_modes_mask;
+	(*init_data)->constraints.valid_ops_mask = valid_ops_mask;
+
+	if (of_find_property(np, "regulator-always-on", NULL))
+				(*init_data)->constraints.always_on = true;
+	if (of_find_property(np, "regulator-boot-on", NULL))
+				(*init_data)->constraints.boot_on = true;
+}
+
+/**
+ * of_get_regulator_init_data - extract regulator_init_data structure info
+ * @dev: device requesting for regulator_init_data
+ *
+ * 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 regulator_init_data *init_data;
+
+	if (!dev->of_node)
+		return NULL;
+
+	init_data = devm_kzalloc(dev, sizeof(*init_data), GFP_KERNEL);
+	if (!init_data)
+		return NULL; /* Out of memory? */
+
+	of_get_regulation_constraints(dev->of_node, &init_data);
+	return init_data;
+}
diff --git a/include/linux/regulator/of_regulator.h b/include/linux/regulator/of_regulator.h
new file mode 100644
index 0000000..c5a1ad6
--- /dev/null
+++ b/include/linux/regulator/of_regulator.h
@@ -0,0 +1,21 @@
+/*
+ * OpenFirmware regulator support routines
+ *
+ */
+
+#ifndef __LINUX_OF_REG_H
+#define __LINUX_OF_REG_H
+
+#if defined(CONFIG_OF_REGULATOR)
+extern struct regulator_init_data
+	*of_get_regulator_init_data(struct device *dev);
+#else
+static inline struct regulator_init_data
+	*of_get_regulator_init_data(struct device *dev)
+{
+	return NULL;
+}
+#endif /* CONFIG_OF_REGULATOR */
+
+#endif /* __LINUX_OF_REG_H */
+
-- 
1.7.1

[PATCH v2 3/5] regulator: helper routine to extract regulator_init_data

[Mark Brown]

On Mon, Oct 10, 2011 at 09:49:36PM +0530, Rajendra Nayak wrote:

> +- regulator-change-voltage: boolean, Output voltage can be changed by software
> +- regulator-change-current: boolean, Output current can be chnaged by software
> +- regulator-change-mode: boolean, Operating mode can be changed by software
> +- regulator-change-status: boolean, Enable/Disable control for regulator exists
> +- regulator-change-drms: boolean, Dynamic regulator mode switching is enabled
> +- regulator-mode-fast: boolean, allow/set fast mode for the regulator
> +- regulator-mode-normal: boolean, allow/set normal mode for the regulator
> +- regulator-mode-idle: boolean, allow/set idle mode for the regulator
> +- regulator-mode-standby: boolean, allow/set standby mode for the regulator
> +- regulator-input-uV: Input voltage for regulator when supplied by another regulator
> +- regulator-always-on: boolean, regulator should never be disabled

Thinking about this I'm not sure that these should go in the device
tree, they're as much talking about what Linux is able to cope with as
they are talking about what the hardware is able to do.  Sometimes
they'll be fixed by the design but they are sometimes also restricted by
what the software is currently capable of.  DRMS is a prime example
here, it depends on how good we are at telling the API about how much
current we're using.

[PATCH v2 3/5] regulator: helper routine to extract regulator_init_data

[Rajendra Nayak]

On Monday 10 October 2011 10:52 PM, Mark Brown wrote:
> On Mon, Oct 10, 2011 at 09:49:36PM +0530, Rajendra Nayak wrote:
>
>> +- regulator-change-voltage: boolean, Output voltage can be changed by software
>> +- regulator-change-current: boolean, Output current can be chnaged by software
>> +- regulator-change-mode: boolean, Operating mode can be changed by software
>> +- regulator-change-status: boolean, Enable/Disable control for regulator exists
>> +- regulator-change-drms: boolean, Dynamic regulator mode switching is enabled
>> +- regulator-mode-fast: boolean, allow/set fast mode for the regulator
>> +- regulator-mode-normal: boolean, allow/set normal mode for the regulator
>> +- regulator-mode-idle: boolean, allow/set idle mode for the regulator
>> +- regulator-mode-standby: boolean, allow/set standby mode for the regulator
>> +- regulator-input-uV: Input voltage for regulator when supplied by another regulator
>> +- regulator-always-on: boolean, regulator should never be disabled
>
> Thinking about this I'm not sure that these should go in the device
> tree, they're as much talking about what Linux is able to cope with as
> they are talking about what the hardware is able to do.  Sometimes
> they'll be fixed by the design but they are sometimes also restricted by
> what the software is currently capable of.  DRMS is a prime example
> here, it depends on how good we are at telling the API about how much
> current we're using.

So is there another way of passing these, if not through device tree?
There are other linux specific things that need to be passed to the
framework as well, like the state of the regulator in the various
linux specific suspend states, like standby/mem/disk.
So if these can't be passed through device tree, should they still
be passed in some way through platform_data? Or is it best to identify
the bindings as being linux specific and not generic by appending a
"linux," to the bindings.

Grant, need some help and advice.

[PATCH v2 3/5] regulator: helper routine to extract regulator_init_data

[Grant Likely]

On Tue, Oct 11, 2011 at 11:29:29AM +0530, Rajendra Nayak wrote:
> On Monday 10 October 2011 10:52 PM, Mark Brown wrote:
> >On Mon, Oct 10, 2011 at 09:49:36PM +0530, Rajendra Nayak wrote:
> >
> >>+- regulator-change-voltage: boolean, Output voltage can be changed by software
> >>+- regulator-change-current: boolean, Output current can be chnaged by software
> >>+- regulator-change-mode: boolean, Operating mode can be changed by software
> >>+- regulator-change-status: boolean, Enable/Disable control for regulator exists
> >>+- regulator-change-drms: boolean, Dynamic regulator mode switching is enabled
> >>+- regulator-mode-fast: boolean, allow/set fast mode for the regulator
> >>+- regulator-mode-normal: boolean, allow/set normal mode for the regulator
> >>+- regulator-mode-idle: boolean, allow/set idle mode for the regulator
> >>+- regulator-mode-standby: boolean, allow/set standby mode for the regulator
> >>+- regulator-input-uV: Input voltage for regulator when supplied by another regulator
> >>+- regulator-always-on: boolean, regulator should never be disabled
> >
> >Thinking about this I'm not sure that these should go in the device
> >tree, they're as much talking about what Linux is able to cope with as
> >they are talking about what the hardware is able to do.  Sometimes
> >they'll be fixed by the design but they are sometimes also restricted by
> >what the software is currently capable of.  DRMS is a prime example
> >here, it depends on how good we are at telling the API about how much
> >current we're using.
> 
> So is there another way of passing these, if not through device tree?
> There are other linux specific things that need to be passed to the
> framework as well, like the state of the regulator in the various
> linux specific suspend states, like standby/mem/disk.
> So if these can't be passed through device tree, should they still
> be passed in some way through platform_data? Or is it best to identify
> the bindings as being linux specific and not generic by appending a
> "linux," to the bindings.
> 
> Grant, need some help and advice.
> 

I can't really help much here.  If it is a property of the hardware,
then it absolutely needs to be in the device tree.  If it is a
limitation of Linux, or the Linux driver, then I would say that it
should be encoded in the driver.  I don't understand enough of the
problem space of regulators to give a good answer about what you
should do.

These *sound* like flags that the driver would set based on its own
capabilities; in which case it is something that would be encoded
directly into the driver.

g.

[PATCH v2 3/5] regulator: helper routine to extract regulator_init_data

[Mark Brown]

On Thu, Oct 13, 2011 at 12:38:21PM -0600, Grant Likely wrote:

> These *sound* like flags that the driver would set based on its own
> capabilities; in which case it is something that would be encoded
> directly into the driver.

They're almost entirely a combination of system specifics and software
capabilities - it's not even reliably one or the other.  For example the
ability to switch regulator modes is going to depend on a combination of
the modes a specific regulator offers, the set of things connected to
the regulator in the current system (not all of which will be software
visible), how good all the connected consumer drivers are at providing
accurate information about what they're going to need from the system in
terms of quality of regulation and current draw and how good the board
design is in making these things reality.

For robustness (for some of these things if you get it wrong we're
talking catastrophic hardware failure) the Linux regulator API is
extremely conservative and won't touch the hardware unless explicitly
told that a specific thing is OK on a given board.

[PATCH v2 3/5] regulator: helper routine to extract regulator_init_data

[Grant Likely]

On Mon, Oct 10, 2011 at 09:49:36PM +0530, Rajendra Nayak wrote:
> The helper routine is meant to be used by regulator drivers
> to extract the regulator_init_data structure from the data
> that is passed from device tree.
> 'consumer_supplies' which is part of regulator_init_data is not extracted
> as the regulator consumer mappings are passed through DT differently,
> implemented in subsequent patches.
> Similarly the regulator<-->parent/supply mapping is handled in
> subsequent patches.
> 
> Also add documentation for regulator bindings to be used to pass
> regulator_init_data struct information from device tree.
> 
> Signed-off-by: Rajendra Nayak <rnayak@ti.com>
> ---
>  .../devicetree/bindings/regulator/regulator.txt    |   44 +++++++++
>  drivers/regulator/Kconfig                          |    8 ++
>  drivers/regulator/Makefile                         |    1 +
>  drivers/regulator/of_regulator.c                   |   93 ++++++++++++++++++++
>  include/linux/regulator/of_regulator.h             |   21 +++++
>  5 files changed, 167 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/regulator/regulator.txt
>  create mode 100644 drivers/regulator/of_regulator.c
>  create mode 100644 include/linux/regulator/of_regulator.h
> 
> diff --git a/Documentation/devicetree/bindings/regulator/regulator.txt b/Documentation/devicetree/bindings/regulator/regulator.txt
> new file mode 100644
> index 0000000..a623fdd
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/regulator/regulator.txt
> @@ -0,0 +1,44 @@
> +Voltage/Current Regulators
> +
> +Optional properties:
> +- regulator-min-uV: smallest voltage consumers may set
> +- regulator-max-uV: largest voltage consumers may set
> +- regulator-uV-offset: Offset applied to voltages to compensate for voltage drops
> +- regulator-min-uA: smallest current consumers may set
> +- regulator-max-uA: largest current consumers may set
> +- regulator-change-voltage: boolean, Output voltage can be changed by software
> +- regulator-change-current: boolean, Output current can be chnaged by software
> +- regulator-change-mode: boolean, Operating mode can be changed by software
> +- regulator-change-status: boolean, Enable/Disable control for regulator exists
> +- regulator-change-drms: boolean, Dynamic regulator mode switching is enabled
> +- regulator-mode-fast: boolean, allow/set fast mode for the regulator
> +- regulator-mode-normal: boolean, allow/set normal mode for the regulator
> +- regulator-mode-idle: boolean, allow/set idle mode for the regulator
> +- regulator-mode-standby: boolean, allow/set standby mode for the regulator
> +- regulator-input-uV: Input voltage for regulator when supplied by another regulator
> +- regulator-always-on: boolean, regulator should never be disabled
> +- regulator-boot-on: bootloader/firmware enabled regulator
> +- <name>-supply: phandle to the parent supply/regulator node
> +
> +Example:
> +
> +	xyzreg: regulator at 0 {
> +		regulator-min-uV = <1000000>;
> +		regulator-max-uV = <2500000>;
> +		regulator-mode-fast;
> +		regulator-change-voltage;
> +		regulator-always-on;
> +		vin-supply = <&vin>;
> +	};
> +
> +The same binding used by a regulator to reference its
> +supply can be used by any consumer to reference its
> +regulator/supply
> +
> +Example of a device node referencing a regulator node,
> +
> +	devicenode: node at 0x0 {
> +		...
> +		...
> +		<name>-supply = <&xyzreg>;
> +	};
> diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
> index c7fd2c0..981c92e 100644
> --- a/drivers/regulator/Kconfig
> +++ b/drivers/regulator/Kconfig
> @@ -64,6 +64,14 @@ config REGULATOR_USERSPACE_CONSUMER
>  
>            If unsure, say no.
>  
> +config OF_REGULATOR
> +	tristate "OF regulator helpers"
> +	depends on OF
> +	default y if OF

drop the default y line.  This probably shouldn't even be a
user-visible option.  Make drivers select OF_REGULATOR if it needs
it.

> +	help
> +	  OpenFirmware regulator framework helper routines that can
> +	  used by regulator drivers to extract data from device tree.
> +
>  config REGULATOR_BQ24022
>  	tristate "TI bq24022 Dual Input 1-Cell Li-Ion Charger IC"
>  	help
> diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
> index 040d5aa..e6bc009 100644
> --- a/drivers/regulator/Makefile
> +++ b/drivers/regulator/Makefile
> @@ -7,6 +7,7 @@ obj-$(CONFIG_REGULATOR) += core.o dummy.o
>  obj-$(CONFIG_REGULATOR_FIXED_VOLTAGE) += fixed.o
>  obj-$(CONFIG_REGULATOR_VIRTUAL_CONSUMER) += virtual.o
>  obj-$(CONFIG_REGULATOR_USERSPACE_CONSUMER) += userspace-consumer.o
> +obj-$(CONFIG_OF_REGULATOR) += of_regulator.o
>  
>  obj-$(CONFIG_REGULATOR_AD5398) += ad5398.o
>  obj-$(CONFIG_REGULATOR_BQ24022) += bq24022.o
> diff --git a/drivers/regulator/of_regulator.c b/drivers/regulator/of_regulator.c
> new file mode 100644
> index 0000000..9262d06
> --- /dev/null
> +++ b/drivers/regulator/of_regulator.c
> @@ -0,0 +1,93 @@
> +/*
> + * OF helpers for regulator framework
> + *
> + * Copyright (C) 2011 Texas Instruments, Inc.
> + * Rajendra Nayak <rnayak@ti.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +
> +#include <linux/slab.h>
> +#include <linux/of.h>
> +#include <linux/regulator/machine.h>
> +
> +static void of_get_regulation_constraints(struct device_node *np,
> +					struct regulator_init_data **init_data)
> +{
> +	const __be32 *min_uV, *max_uV, *uV_offset;
> +	const __be32 *min_uA, *max_uA, *input_uV;
> +	unsigned int valid_modes_mask = 0, valid_ops_mask = 0;
> +
> +	min_uV = of_get_property(np, "regulator-min-uV", NULL);
> +	if (min_uV)
> +		(*init_data)->constraints.min_uV = be32_to_cpu(*min_uV);
> +	max_uV = of_get_property(np, "regulator-max-uV", NULL);
> +	if (max_uV)
> +		(*init_data)->constraints.max_uV = be32_to_cpu(*max_uV);
> +	uV_offset = of_get_property(np, "regulator-uV-offset", NULL);
> +	if (uV_offset)
> +		(*init_data)->constraints.uV_offset = be32_to_cpu(*uV_offset);
> +	min_uA = of_get_property(np, "regulator-min-uA", NULL);
> +	if (min_uA)
> +		(*init_data)->constraints.min_uA = be32_to_cpu(*min_uA);
> +	max_uA = of_get_property(np, "regulator-max-uA", NULL);
> +	if (max_uA)
> +		(*init_data)->constraints.max_uA = be32_to_cpu(*max_uA);
> +	input_uV = of_get_property(np, "regulator-input-uV", NULL);
> +	if (input_uV)
> +		(*init_data)->constraints.input_uV = be32_to_cpu(*input_uV);
> +
> +	/* valid modes mask */
> +	if (of_find_property(np, "regulator-mode-fast", NULL))
> +				valid_modes_mask |= REGULATOR_MODE_FAST;
> +	if (of_find_property(np, "regulator-mode-normal", NULL))
> +				valid_modes_mask |= REGULATOR_MODE_NORMAL;
> +	if (of_find_property(np, "regulator-mode-idle", NULL))
> +				valid_modes_mask |= REGULATOR_MODE_IDLE;
> +	if (of_find_property(np, "regulator-mode-standby", NULL))
> +				valid_modes_mask |= REGULATOR_MODE_STANDBY;
> +
> +	/* valid ops mask */
> +	if (of_find_property(np, "regulator-change-voltage", NULL))
> +				valid_ops_mask |= REGULATOR_CHANGE_VOLTAGE;
> +	if (of_find_property(np, "regulator-change-current", NULL))
> +				valid_ops_mask |= REGULATOR_CHANGE_CURRENT;
> +	if (of_find_property(np, "regulator-change-mode", NULL))
> +				valid_ops_mask |= REGULATOR_CHANGE_MODE;
> +	if (of_find_property(np, "regulator-change-status", NULL))
> +				valid_ops_mask |= REGULATOR_CHANGE_STATUS;
> +
> +	(*init_data)->constraints.valid_modes_mask = valid_modes_mask;
> +	(*init_data)->constraints.valid_ops_mask = valid_ops_mask;
> +
> +	if (of_find_property(np, "regulator-always-on", NULL))
> +				(*init_data)->constraints.always_on = true;
> +	if (of_find_property(np, "regulator-boot-on", NULL))
> +				(*init_data)->constraints.boot_on = true;
> +}
> +
> +/**
> + * of_get_regulator_init_data - extract regulator_init_data structure info
> + * @dev: device requesting for regulator_init_data
> + *
> + * 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 regulator_init_data *init_data;
> +
> +	if (!dev->of_node)
> +		return NULL;
> +
> +	init_data = devm_kzalloc(dev, sizeof(*init_data), GFP_KERNEL);
> +	if (!init_data)
> +		return NULL; /* Out of memory? */
> +
> +	of_get_regulation_constraints(dev->of_node, &init_data);
> +	return init_data;
> +}
> diff --git a/include/linux/regulator/of_regulator.h b/include/linux/regulator/of_regulator.h
> new file mode 100644
> index 0000000..c5a1ad6
> --- /dev/null
> +++ b/include/linux/regulator/of_regulator.h
> @@ -0,0 +1,21 @@
> +/*
> + * OpenFirmware regulator support routines
> + *
> + */
> +
> +#ifndef __LINUX_OF_REG_H
> +#define __LINUX_OF_REG_H
> +
> +#if defined(CONFIG_OF_REGULATOR)
> +extern struct regulator_init_data
> +	*of_get_regulator_init_data(struct device *dev);
> +#else
> +static inline struct regulator_init_data
> +	*of_get_regulator_init_data(struct device *dev)
> +{
> +	return NULL;
> +}
> +#endif /* CONFIG_OF_REGULATOR */
> +
> +#endif /* __LINUX_OF_REG_H */
> +
> -- 
> 1.7.1
> 

[PATCH v2 3/5] regulator: helper routine to extract regulator_init_data

[Shawn Guo]

On Mon, Oct 10, 2011 at 09:49:36PM +0530, Rajendra Nayak wrote:
> The helper routine is meant to be used by regulator drivers
> to extract the regulator_init_data structure from the data
> that is passed from device tree.
> 'consumer_supplies' which is part of regulator_init_data is not extracted
> as the regulator consumer mappings are passed through DT differently,
> implemented in subsequent patches.
> Similarly the regulator<-->parent/supply mapping is handled in
> subsequent patches.
> 
> Also add documentation for regulator bindings to be used to pass
> regulator_init_data struct information from device tree.
> 
> Signed-off-by: Rajendra Nayak <rnayak@ti.com>
> ---
>  .../devicetree/bindings/regulator/regulator.txt    |   44 +++++++++
>  drivers/regulator/Kconfig                          |    8 ++
>  drivers/regulator/Makefile                         |    1 +
>  drivers/regulator/of_regulator.c                   |   93 ++++++++++++++++++++
>  include/linux/regulator/of_regulator.h             |   21 +++++
>  5 files changed, 167 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/regulator/regulator.txt
>  create mode 100644 drivers/regulator/of_regulator.c
>  create mode 100644 include/linux/regulator/of_regulator.h
> 
> diff --git a/Documentation/devicetree/bindings/regulator/regulator.txt b/Documentation/devicetree/bindings/regulator/regulator.txt
> new file mode 100644
> index 0000000..a623fdd
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/regulator/regulator.txt
> @@ -0,0 +1,44 @@
> +Voltage/Current Regulators
> +
> +Optional properties:
> +- regulator-min-uV: smallest voltage consumers may set
> +- regulator-max-uV: largest voltage consumers may set
> +- regulator-uV-offset: Offset applied to voltages to compensate for voltage drops
> +- regulator-min-uA: smallest current consumers may set
> +- regulator-max-uA: largest current consumers may set
> +- regulator-change-voltage: boolean, Output voltage can be changed by software
> +- regulator-change-current: boolean, Output current can be chnaged by software
> +- regulator-change-mode: boolean, Operating mode can be changed by software
> +- regulator-change-status: boolean, Enable/Disable control for regulator exists
> +- regulator-change-drms: boolean, Dynamic regulator mode switching is enabled
> +- regulator-mode-fast: boolean, allow/set fast mode for the regulator
> +- regulator-mode-normal: boolean, allow/set normal mode for the regulator
> +- regulator-mode-idle: boolean, allow/set idle mode for the regulator
> +- regulator-mode-standby: boolean, allow/set standby mode for the regulator
> +- regulator-input-uV: Input voltage for regulator when supplied by another regulator
> +- regulator-always-on: boolean, regulator should never be disabled
> +- regulator-boot-on: bootloader/firmware enabled regulator
> +- <name>-supply: phandle to the parent supply/regulator node
> +
> +Example:
> +
> +	xyzreg: regulator at 0 {
> +		regulator-min-uV = <1000000>;
> +		regulator-max-uV = <2500000>;
> +		regulator-mode-fast;
> +		regulator-change-voltage;
> +		regulator-always-on;
> +		vin-supply = <&vin>;
> +	};
> +
> +The same binding used by a regulator to reference its
> +supply can be used by any consumer to reference its
> +regulator/supply
> +
> +Example of a device node referencing a regulator node,
> +
> +	devicenode: node at 0x0 {
> +		...
> +		...
> +		<name>-supply = <&xyzreg>;
> +	};
> diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
> index c7fd2c0..981c92e 100644
> --- a/drivers/regulator/Kconfig
> +++ b/drivers/regulator/Kconfig
> @@ -64,6 +64,14 @@ config REGULATOR_USERSPACE_CONSUMER
>  
>            If unsure, say no.
>  
> +config OF_REGULATOR
> +	tristate "OF regulator helpers"
> +	depends on OF
> +	default y if OF
> +	help
> +	  OpenFirmware regulator framework helper routines that can
> +	  used by regulator drivers to extract data from device tree.
> +
>  config REGULATOR_BQ24022
>  	tristate "TI bq24022 Dual Input 1-Cell Li-Ion Charger IC"
>  	help
> diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
> index 040d5aa..e6bc009 100644
> --- a/drivers/regulator/Makefile
> +++ b/drivers/regulator/Makefile
> @@ -7,6 +7,7 @@ obj-$(CONFIG_REGULATOR) += core.o dummy.o
>  obj-$(CONFIG_REGULATOR_FIXED_VOLTAGE) += fixed.o
>  obj-$(CONFIG_REGULATOR_VIRTUAL_CONSUMER) += virtual.o
>  obj-$(CONFIG_REGULATOR_USERSPACE_CONSUMER) += userspace-consumer.o
> +obj-$(CONFIG_OF_REGULATOR) += of_regulator.o
>  
>  obj-$(CONFIG_REGULATOR_AD5398) += ad5398.o
>  obj-$(CONFIG_REGULATOR_BQ24022) += bq24022.o
> diff --git a/drivers/regulator/of_regulator.c b/drivers/regulator/of_regulator.c
> new file mode 100644
> index 0000000..9262d06
> --- /dev/null
> +++ b/drivers/regulator/of_regulator.c
> @@ -0,0 +1,93 @@
> +/*
> + * OF helpers for regulator framework
> + *
> + * Copyright (C) 2011 Texas Instruments, Inc.
> + * Rajendra Nayak <rnayak@ti.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +
> +#include <linux/slab.h>
> +#include <linux/of.h>
> +#include <linux/regulator/machine.h>
> +
> +static void of_get_regulation_constraints(struct device_node *np,
> +					struct regulator_init_data **init_data)
> +{
> +	const __be32 *min_uV, *max_uV, *uV_offset;
> +	const __be32 *min_uA, *max_uA, *input_uV;
> +	unsigned int valid_modes_mask = 0, valid_ops_mask = 0;
> +
> +	min_uV = of_get_property(np, "regulator-min-uV", NULL);
> +	if (min_uV)
> +		(*init_data)->constraints.min_uV = be32_to_cpu(*min_uV);
> +	max_uV = of_get_property(np, "regulator-max-uV", NULL);
> +	if (max_uV)
> +		(*init_data)->constraints.max_uV = be32_to_cpu(*max_uV);
> +	uV_offset = of_get_property(np, "regulator-uV-offset", NULL);
> +	if (uV_offset)
> +		(*init_data)->constraints.uV_offset = be32_to_cpu(*uV_offset);
> +	min_uA = of_get_property(np, "regulator-min-uA", NULL);
> +	if (min_uA)
> +		(*init_data)->constraints.min_uA = be32_to_cpu(*min_uA);
> +	max_uA = of_get_property(np, "regulator-max-uA", NULL);
> +	if (max_uA)
> +		(*init_data)->constraints.max_uA = be32_to_cpu(*max_uA);
> +	input_uV = of_get_property(np, "regulator-input-uV", NULL);
> +	if (input_uV)
> +		(*init_data)->constraints.input_uV = be32_to_cpu(*input_uV);
> +
> +	/* valid modes mask */
> +	if (of_find_property(np, "regulator-mode-fast", NULL))
> +				valid_modes_mask |= REGULATOR_MODE_FAST;
> +	if (of_find_property(np, "regulator-mode-normal", NULL))
> +				valid_modes_mask |= REGULATOR_MODE_NORMAL;
> +	if (of_find_property(np, "regulator-mode-idle", NULL))
> +				valid_modes_mask |= REGULATOR_MODE_IDLE;
> +	if (of_find_property(np, "regulator-mode-standby", NULL))
> +				valid_modes_mask |= REGULATOR_MODE_STANDBY;
> +
> +	/* valid ops mask */
> +	if (of_find_property(np, "regulator-change-voltage", NULL))
> +				valid_ops_mask |= REGULATOR_CHANGE_VOLTAGE;
> +	if (of_find_property(np, "regulator-change-current", NULL))
> +				valid_ops_mask |= REGULATOR_CHANGE_CURRENT;
> +	if (of_find_property(np, "regulator-change-mode", NULL))
> +				valid_ops_mask |= REGULATOR_CHANGE_MODE;
> +	if (of_find_property(np, "regulator-change-status", NULL))
> +				valid_ops_mask |= REGULATOR_CHANGE_STATUS;
> +
> +	(*init_data)->constraints.valid_modes_mask = valid_modes_mask;
> +	(*init_data)->constraints.valid_ops_mask = valid_ops_mask;
> +
> +	if (of_find_property(np, "regulator-always-on", NULL))
> +				(*init_data)->constraints.always_on = true;
> +	if (of_find_property(np, "regulator-boot-on", NULL))
> +				(*init_data)->constraints.boot_on = true;
> +}
> +
I do not see that of_get_regulation_constraints() covers the following
fields.  We do not support them for DT probe?

struct regulation_constraints {

        char *name;

	...

        /* regulator suspend states for global PMIC STANDBY/HIBERNATE */
        struct regulator_state state_disk;
        struct regulator_state state_mem;
        struct regulator_state state_standby;
        suspend_state_t initial_state; /* suspend state to set at init */

        /* mode to set on startup */
        unsigned int initial_mode;

        /* constraint flags */
	...
        unsigned apply_uV:1;    /* apply uV constraint if min == max */
};

-- 
Regards,
Shawn

[PATCH v2 3/5] regulator: helper routine to extract regulator_init_data

[Rajendra Nayak]

> I do not see that of_get_regulation_constraints() covers the following
> fields.  We do not support them for DT probe?

I left these out as I wasn't sure how such (if at all) Linux specific 
params should be passed through dt, and I am still not quite sure :(

>
> struct regulation_constraints {
>
>          char *name;
>
> 	...
>
>          /* regulator suspend states for global PMIC STANDBY/HIBERNATE */
>          struct regulator_state state_disk;
>          struct regulator_state state_mem;
>          struct regulator_state state_standby;
>          suspend_state_t initial_state; /* suspend state to set at init */
>
>          /* mode to set on startup */
>          unsigned int initial_mode;
>
>          /* constraint flags */
> 	...
>          unsigned apply_uV:1;    /* apply uV constraint if min == max */
> };
>

[PATCH v2 3/5] regulator: helper routine to extract regulator_init_data

[Shawn Guo]

On Mon, Oct 17, 2011 at 09:47:17AM +0530, Rajendra Nayak wrote:
> 
> >I do not see that of_get_regulation_constraints() covers the following
> >fields.  We do not support them for DT probe?
> 
> I left these out as I wasn't sure how such (if at all) Linux
> specific params should be passed through dt, and I am still not
> quite sure :(
> 
I'm seeing some linux specific parameters encoded in the device tree
below.

  Documentation/devicetree/bindings/gpio/gpio_keys.txt
  Documentation/devicetree/bindings/gpio/led.txt

Mark,

I understand that ideally device tree is supposed to describe pure
hardware configurations.  But practically, when migrating a driver
to device tree probe, we are trying to move the configurations
described by platform_data into device tree to save the use of
platform_data for device tree probe.  Then some of the configuration
may not be so purely hardware related.  But I do not see this is a
critical problem.

What do you think?  Is it what you want that we classify the
configurations clearly and put only strict hardware configurations
into device tree and keep others still in platform_data?

-- 
Regards,
Shawn

[PATCH v2 3/5] regulator: helper routine to extract regulator_init_data

[Mark Brown]

On Tue, Oct 18, 2011 at 07:58:37PM +0800, Shawn Guo wrote:

> I understand that ideally device tree is supposed to describe pure
> hardware configurations.  But practically, when migrating a driver
> to device tree probe, we are trying to move the configurations
> described by platform_data into device tree to save the use of
> platform_data for device tree probe.  Then some of the configuration
> may not be so purely hardware related.  But I do not see this is a
> critical problem.

It's not just Linux-specific stuff, some of this is even specific to
what current Linux drivers can do - updating the kernel could mean a
different set of constraints.

[PATCH v2 3/5] regulator: helper routine to extract regulator_init_data

[Shawn Guo]

On Tue, Oct 18, 2011 at 05:00:46PM +0100, Mark Brown wrote:
> On Tue, Oct 18, 2011 at 07:58:37PM +0800, Shawn Guo wrote:
> 
> > I understand that ideally device tree is supposed to describe pure
> > hardware configurations.  But practically, when migrating a driver
> > to device tree probe, we are trying to move the configurations
> > described by platform_data into device tree to save the use of
> > platform_data for device tree probe.  Then some of the configuration
> > may not be so purely hardware related.  But I do not see this is a
> > critical problem.
> 
> It's not just Linux-specific stuff, some of this is even specific to
> what current Linux drivers can do - updating the kernel could mean a
> different set of constraints.
> 
Well, from what I see, the 'struct regulation_constraints' is defined
in machine.h and meant to be the regulator machine/board interface.
With the example I'm looking at, mc13892, the regulation_constraints
configuration is fully passed from machine/board file.  If there is
something specific to what drivers can do, it probably should be encoded
in regulator driver rather than staying in regulation_constraints.

So how do you think we should proceed here?

-- 
Regards,
Shawn

[PATCH v2 3/5] regulator: helper routine to extract regulator_init_data

[Mark Brown]

On Wed, Oct 19, 2011 at 01:33:55PM +0800, Shawn Guo wrote:
> On Tue, Oct 18, 2011 at 05:00:46PM +0100, Mark Brown wrote:

> > It's not just Linux-specific stuff, some of this is even specific to
> > what current Linux drivers can do - updating the kernel could mean a
> > different set of constraints.

> Well, from what I see, the 'struct regulation_constraints' is defined
> in machine.h and meant to be the regulator machine/board interface.

...which will depend on the system integrator's understanding of what
their system is capable of right now.

> With the example I'm looking at, mc13892, the regulation_constraints
> configuration is fully passed from machine/board file.  If there is
> something specific to what drivers can do, it probably should be encoded
> in regulator driver rather than staying in regulation_constraints.

I don't think you're quite understanding the issue - it's an integration
problem with three different variables.  It's a combination of what the
chips can do, what the drivers can do and if the board design affects
any of this stuff.  Only the board can come to a final decision.

[PATCH v2 3/5] regulator: helper routine to extract regulator_init_data

[Shawn Guo]

On Wed, Oct 19, 2011 at 03:47:34PM +0100, Mark Brown wrote:
> On Wed, Oct 19, 2011 at 01:33:55PM +0800, Shawn Guo wrote:
> > On Tue, Oct 18, 2011 at 05:00:46PM +0100, Mark Brown wrote:
> 
> > > It's not just Linux-specific stuff, some of this is even specific to
> > > what current Linux drivers can do - updating the kernel could mean a
> > > different set of constraints.
> 
> > Well, from what I see, the 'struct regulation_constraints' is defined
> > in machine.h and meant to be the regulator machine/board interface.
> 
> ...which will depend on the system integrator's understanding of what
> their system is capable of right now.
> 
> > With the example I'm looking at, mc13892, the regulation_constraints
> > configuration is fully passed from machine/board file.  If there is
> > something specific to what drivers can do, it probably should be encoded
> > in regulator driver rather than staying in regulation_constraints.
> 
> I don't think you're quite understanding the issue - it's an integration
> problem with three different variables.  It's a combination of what the
> chips can do, what the drivers can do and if the board design affects
> any of this stuff.
>
Honestly, I'm still pretty new to regulator subsystem, so really need
your help to understand the situation.

> Only the board can come to a final decision.
> 
The dts is very capable and suitable to describe the board's decision.
But you disagree that we put all constraints description into DT.  I'm
a pretty confused here.  So again, what is your suggestion to this
'problem'?

-- 
Regards,
Shawn

[PATCH v2 3/5] regulator: helper routine to extract regulator_init_data

[Mark Brown]

On Wed, Oct 19, 2011 at 11:04:49PM +0800, Shawn Guo wrote:
> On Wed, Oct 19, 2011 at 03:47:34PM +0100, Mark Brown wrote:

> > Only the board can come to a final decision.

> The dts is very capable and suitable to describe the board's decision.
> But you disagree that we put all constraints description into DT.  I'm
> a pretty confused here.  So again, what is your suggestion to this
> 'problem'?

The problem is that the DT is supposed to be separate to the kernel and
the decisions can depend on what the kernel is currently capable of.
When the data is embedded in the kernel it's not an issue as the data is
attached to the rest of the code, when the data becomes detatched from
the kernel it becomes an issue.

I don't see any issue with leaving some things out of the DT bindings;
you were the one raising that as a concern.

[PATCH v2 3/5] regulator: helper routine to extract regulator_init_data

[Rajendra Nayak]

On Wednesday 19 October 2011 08:40 PM, Mark Brown wrote:
> The problem is that the DT is supposed to be separate to the kernel and
> the decisions can depend on what the kernel is currently capable of.
> When the data is embedded in the kernel it's not an issue as the data is
> attached to the rest of the code, when the data becomes detatched from
> the kernel it becomes an issue.

completely agreed.

>
> I don't see any issue with leaving some things out of the DT bindings;
> you were the one raising that as a concern.

The problem is, that there doesn't seem to be a clean way to embed
*board data* into the kernel with DT, if left out of the DT bindings.
There is the auxdata way of still attaching platform_data, but that I
thought was a stopgap for just handling function pointers.

[PATCH v2 3/5] regulator: helper routine to extract regulator_init_data

[Mark Brown]

On Thu, Oct 20, 2011 at 09:12:10AM +0530, Rajendra Nayak wrote:
> On Wednesday 19 October 2011 08:40 PM, Mark Brown wrote:

> >I don't see any issue with leaving some things out of the DT bindings;
> >you were the one raising that as a concern.

> The problem is, that there doesn't seem to be a clean way to embed
> *board data* into the kernel with DT, if left out of the DT bindings.
> There is the auxdata way of still attaching platform_data, but that I
> thought was a stopgap for just handling function pointers.

We can always start off just completely omitting the data and then see
how we go from there.  If we only cover 50% of users that's still 50%
more than are currently covered with device tree right now and it means
we can then spin round and look at the bits that are hard again without
review fatigue on the bits that are easy.

[PATCH v2 3/5] regulator: helper routine to extract regulator_init_data

[Rajendra Nayak]

>
> We can always start off just completely omitting the data and then see
> how we go from there.  If we only cover 50% of users that's still 50%
> more than are currently covered with device tree right now and it means
> we can then spin round and look at the bits that are hard again without
> review fatigue on the bits that are easy.

Ok, will drop the problematic bindings for now and repost.

[PATCH v2 3/5] regulator: helper routine to extract regulator_init_data

[Tony Lindgren]

* Mark Brown <broonie@opensource.wolfsonmicro.com> [111020 02:07]:
> On Thu, Oct 20, 2011 at 09:12:10AM +0530, Rajendra Nayak wrote:
> > On Wednesday 19 October 2011 08:40 PM, Mark Brown wrote:
> 
> > >I don't see any issue with leaving some things out of the DT bindings;
> > >you were the one raising that as a concern.
> 
> > The problem is, that there doesn't seem to be a clean way to embed
> > *board data* into the kernel with DT, if left out of the DT bindings.
> > There is the auxdata way of still attaching platform_data, but that I
> > thought was a stopgap for just handling function pointers.
> 
> We can always start off just completely omitting the data and then see
> how we go from there.  If we only cover 50% of users that's still 50%
> more than are currently covered with device tree right now and it means
> we can then spin round and look at the bits that are hard again without
> review fatigue on the bits that are easy.

We still need to pass the board configuration somehow, otherwise we can
never remove all the platform data glue layers. And if we can't do that,
we'll forever have all the nasty merge conflicts when adding new drivers.
And there's an unnecessary dependency between adding drivers and the
core SoC code.

So we really need to remove all the platform data glue layers so driver
probe can initialize things automagically.

Kernel cmdline is of course one way to pass options, but probably won't
scale for all the board wirings.. And the kernel cmdline should really
be for user configurable options.

So should we just have a cmdline string for each DT entry to pass the
board specific options from DT to the driver? Something along the
lines module options?

Regards,

Tony

[PATCH v2 3/5] regulator: helper routine to extract regulator_init_data

[Mark Brown]

On Thu, Oct 20, 2011 at 09:27:43AM -0700, Tony Lindgren wrote:
> * Mark Brown <broonie@opensource.wolfsonmicro.com> [111020 02:07]:

> > We can always start off just completely omitting the data and then see
> > how we go from there.  If we only cover 50% of users that's still 50%
> > more than are currently covered with device tree right now and it means
> > we can then spin round and look at the bits that are hard again without
> > review fatigue on the bits that are easy.

> We still need to pass the board configuration somehow, otherwise we can
> never remove all the platform data glue layers. And if we can't do that,
> we'll forever have all the nasty merge conflicts when adding new drivers.
> And there's an unnecessary dependency between adding drivers and the
> core SoC code.

The current patches cover the overwhelming majority of the existing
board configuration - the stuff that's Linux specific is also relatively
rarely used in actual systems.  For example all the board I work with
regularly would be perfectly happy with the generic stuff - the Linux
specific stuff is relatively rarely used.

[PATCH v2 3/5] regulator: helper routine to extract regulator_init_data

[Tony Lindgren]

* Mark Brown <broonie@opensource.wolfsonmicro.com> [111020 09:05]:
> On Thu, Oct 20, 2011 at 09:27:43AM -0700, Tony Lindgren wrote:
> > * Mark Brown <broonie@opensource.wolfsonmicro.com> [111020 02:07]:
> 
> > > We can always start off just completely omitting the data and then see
> > > how we go from there.  If we only cover 50% of users that's still 50%
> > > more than are currently covered with device tree right now and it means
> > > we can then spin round and look at the bits that are hard again without
> > > review fatigue on the bits that are easy.
> 
> > We still need to pass the board configuration somehow, otherwise we can
> > never remove all the platform data glue layers. And if we can't do that,
> > we'll forever have all the nasty merge conflicts when adding new drivers.
> > And there's an unnecessary dependency between adding drivers and the
> > core SoC code.
> 
> The current patches cover the overwhelming majority of the existing
> board configuration - the stuff that's Linux specific is also relatively
> rarely used in actual systems.  For example all the board I work with
> regularly would be perfectly happy with the generic stuff - the Linux
> specific stuff is relatively rarely used.

Right, but in addition the board specific integration variables still
need to be passed somehow to the driver.

That's where a DT entry specific configuration string might be the best
option as it still allows describing the hardware using DT standards,
while also allowing board specific configuration too.

The issue there is then how do we keep these options from getting out
of control..

BTW, of course this issue is generic to all drivers, not specific to
this patchset. With this patchset leaving out the non-standard entries
is the right way to go to so we can merge it.

Cheers,

Tony

[PATCH v2 3/5] regulator: helper routine to extract regulator_init_data

[Tony Lindgren]

* Tony Lindgren <tony@atomide.com> [111020 09:31]:
> 
> That's where a DT entry specific configuration string might be the best
> option as it still allows describing the hardware using DT standards,
> while also allowing board specific configuration too.

Hmm, actually, can't we just pass the board specific configuration in
the board specific .dts file and then it gets merged in with the
arch independent DT node?

Regards,

Tony

[PATCH v2 3/5] regulator: helper routine to extract regulator_init_data

[Mark Brown]

On Thu, Oct 20, 2011 at 10:22:19AM -0700, Tony Lindgren wrote:

> Hmm, actually, can't we just pass the board specific configuration in
> the board specific .dts file and then it gets merged in with the
> arch independent DT node?

I don't understand what an "arch independent DT node" would be?

[PATCH v2 3/5] regulator: helper routine to extract regulator_init_data

[Tony Lindgren]

* Mark Brown <broonie@opensource.wolfsonmicro.com> [111020 12:23]:
> On Thu, Oct 20, 2011 at 10:22:19AM -0700, Tony Lindgren wrote:
> 
> > Hmm, actually, can't we just pass the board specific configuration in
> > the board specific .dts file and then it gets merged in with the
> > arch independent DT node?
> 
> I don't understand what an "arch independent DT node" would be?

Well that's the standard non-Linux specific parts. Basically what's
in the $Subject patch minus what you commented on earlier:

> +- regulator-change-voltage: boolean, Output voltage can be changed by software
> +- regulator-change-current: boolean, Output current can be chnaged by software
> +- regulator-change-mode: boolean, Operating mode can be changed by software
> +- regulator-change-status: boolean, Enable/Disable control for regulator exists
> +- regulator-change-drms: boolean, Dynamic regulator mode switching is enabled
> +- regulator-mode-fast: boolean, allow/set fast mode for the regulator
> +- regulator-mode-normal: boolean, allow/set normal mode for the regulator
> +- regulator-mode-idle: boolean, allow/set idle mode for the regulator
> +- regulator-mode-standby: boolean, allow/set standby mode for the regulator
> +- regulator-input-uV: Input voltage for regulator when supplied by another regulator
> +- regulator-always-on: boolean, regulator should never be disabled

We still need to figure out how to get the above board specific
data to the device driver probe in a way where we can avoid having
platform glue code. Any thoughts on that?

Tony

[PATCH v2 3/5] regulator: helper routine to extract regulator_init_data

[Mark Brown]

On Thu, Oct 20, 2011 at 01:10:55PM -0700, Tony Lindgren wrote:
> * Mark Brown <broonie@opensource.wolfsonmicro.com> [111020 12:23]:

> > I don't understand what an "arch independent DT node" would be?

> Well that's the standard non-Linux specific parts. Basically what's
> in the $Subject patch minus what you commented on earlier:

Oh, you don't mean arch specific at all then.

> > +- regulator-change-voltage: boolean, Output voltage can be changed by software
> > +- regulator-change-current: boolean, Output current can be chnaged by software

These aren't really Linux specific, and you could probably just infer
them from having a voltage/current range anyway.

> > +- regulator-change-mode: boolean, Operating mode can be changed by software
> > +- regulator-change-status: boolean, Enable/Disable control for regulator exists
> > +- regulator-change-drms: boolean, Dynamic regulator mode switching is enabled
> > +- regulator-mode-fast: boolean, allow/set fast mode for the regulator
> > +- regulator-mode-normal: boolean, allow/set normal mode for the regulator
> > +- regulator-mode-idle: boolean, allow/set idle mode for the regulator
> > +- regulator-mode-standby: boolean, allow/set standby mode for the regulator
> > +- regulator-input-uV: Input voltage for regulator when supplied by another regulator
> > +- regulator-always-on: boolean, regulator should never be disabled

> We still need to figure out how to get the above board specific
> data to the device driver probe in a way where we can avoid having
> platform glue code. Any thoughts on that?

I think we should just not bother with most of the above and see if
anyone notices, with the exception of always on and status changes it's
vanishingly rare for anyone to actually do anything constructive with
them.  

Neither of the two I mentioned are Linux specific either, they mean
somehing useful for any OS it's just that the decision is policy which
is going to depend on the OS version.  Status changes we can probably
allow people to enable, it'll just mean that older device trees won't
get good power use out of newer kernels and if you move to an older
kernel things will start exploding as drivers loose regulator support.
Always on we can probably live without, it's a combination of
overspecification and interaction with _has_full_constraints() which
isn't represented in this binding anyway.

[PATCH v2 3/5] regulator: helper routine to extract regulator_init_data

[Tony Lindgren]

* Mark Brown <broonie@opensource.wolfsonmicro.com> [111020 14:08]:
> On Thu, Oct 20, 2011 at 01:10:55PM -0700, Tony Lindgren wrote:
> 
> > We still need to figure out how to get the above board specific
> > data to the device driver probe in a way where we can avoid having
> > platform glue code. Any thoughts on that?
> 
> I think we should just not bother with most of the above and see if
> anyone notices, with the exception of always on and status changes it's
> vanishingly rare for anyone to actually do anything constructive with
> them.  

OK

> Neither of the two I mentioned are Linux specific either, they mean
> somehing useful for any OS it's just that the decision is policy which
> is going to depend on the OS version.  Status changes we can probably
> allow people to enable, it'll just mean that older device trees won't
> get good power use out of newer kernels and if you move to an older
> kernel things will start exploding as drivers loose regulator support.
> Always on we can probably live without, it's a combination of
> overspecification and interaction with _has_full_constraints() which
> isn't represented in this binding anyway.

Yeah, lot of these are "how the regulator using driver is using
the regulator" or "how the operating system is using the regulator"
type policies.

Regards,

Tony

[PATCH v2 3/5] regulator: helper routine to extract regulator_init_data

[Grant Likely]

On Thu, Oct 20, 2011 at 10:22:19AM -0700, Tony Lindgren wrote:
> * Tony Lindgren <tony@atomide.com> [111020 09:31]:
> > 
> > That's where a DT entry specific configuration string might be the best
> > option as it still allows describing the hardware using DT standards,
> > while also allowing board specific configuration too.
> 
> Hmm, actually, can't we just pass the board specific configuration in
> the board specific .dts file and then it gets merged in with the
> arch independent DT node?

It would be fine to, say, have a omap-specific board configuration
node for collecting things that really don't fit anywhere else.  That
would be in some ways analogous to the way we've decided to use top a
top level node for describing a machines audio complex.

g.

[PATCH v2 3/5] regulator: helper routine to extract regulator_init_data

[Mark Brown]

On Thu, Oct 20, 2011 at 10:05:34AM -0700, Tony Lindgren wrote:

> Right, but in addition the board specific integration variables still
> need to be passed somehow to the driver.

I'm not sure you're aware of what the regulator bindings that are being
discussed are - they're entirely board specific.  The discussion was
about how to handle certain parts of the feature set Linux currently
has.

[PATCH v2 3/5] regulator: helper routine to extract regulator_init_data

[Shawn Guo]

On Mon, Oct 10, 2011 at 09:49:36PM +0530, Rajendra Nayak wrote:
> The helper routine is meant to be used by regulator drivers
> to extract the regulator_init_data structure from the data
> that is passed from device tree.
> 'consumer_supplies' which is part of regulator_init_data is not extracted
> as the regulator consumer mappings are passed through DT differently,
> implemented in subsequent patches.
> Similarly the regulator<-->parent/supply mapping is handled in
> subsequent patches.
> 
> Also add documentation for regulator bindings to be used to pass
> regulator_init_data struct information from device tree.
> 
> Signed-off-by: Rajendra Nayak <rnayak@ti.com>
> ---
>  .../devicetree/bindings/regulator/regulator.txt    |   44 +++++++++
>  drivers/regulator/Kconfig                          |    8 ++
>  drivers/regulator/Makefile                         |    1 +
>  drivers/regulator/of_regulator.c                   |   93 ++++++++++++++++++++
>  include/linux/regulator/of_regulator.h             |   21 +++++
>  5 files changed, 167 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/regulator/regulator.txt
>  create mode 100644 drivers/regulator/of_regulator.c
>  create mode 100644 include/linux/regulator/of_regulator.h
> 
> diff --git a/Documentation/devicetree/bindings/regulator/regulator.txt b/Documentation/devicetree/bindings/regulator/regulator.txt
> new file mode 100644
> index 0000000..a623fdd
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/regulator/regulator.txt
> @@ -0,0 +1,44 @@
> +Voltage/Current Regulators
> +
> +Optional properties:
> +- regulator-min-uV: smallest voltage consumers may set
> +- regulator-max-uV: largest voltage consumers may set
> +- regulator-uV-offset: Offset applied to voltages to compensate for voltage drops
> +- regulator-min-uA: smallest current consumers may set
> +- regulator-max-uA: largest current consumers may set
> +- regulator-change-voltage: boolean, Output voltage can be changed by software
> +- regulator-change-current: boolean, Output current can be chnaged by software
> +- regulator-change-mode: boolean, Operating mode can be changed by software
> +- regulator-change-status: boolean, Enable/Disable control for regulator exists
> +- regulator-change-drms: boolean, Dynamic regulator mode switching is enabled
> +- regulator-mode-fast: boolean, allow/set fast mode for the regulator
> +- regulator-mode-normal: boolean, allow/set normal mode for the regulator
> +- regulator-mode-idle: boolean, allow/set idle mode for the regulator
> +- regulator-mode-standby: boolean, allow/set standby mode for the regulator
> +- regulator-input-uV: Input voltage for regulator when supplied by another regulator
> +- regulator-always-on: boolean, regulator should never be disabled
> +- regulator-boot-on: bootloader/firmware enabled regulator
> +- <name>-supply: phandle to the parent supply/regulator node
> +
> +Example:
> +
> +	xyzreg: regulator at 0 {

Does this node have a compatible string?  With looking at the code,
I guess it has a compatible string in your dts file.

> +		regulator-min-uV = <1000000>;
> +		regulator-max-uV = <2500000>;
> +		regulator-mode-fast;
> +		regulator-change-voltage;
> +		regulator-always-on;
> +		vin-supply = <&vin>;
> +	};
> +
> +The same binding used by a regulator to reference its
> +supply can be used by any consumer to reference its
> +regulator/supply
> +
> +Example of a device node referencing a regulator node,
> +
> +	devicenode: node at 0x0 {
> +		...
> +		...
> +		<name>-supply = <&xyzreg>;
> +	};
> diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
> index c7fd2c0..981c92e 100644
> --- a/drivers/regulator/Kconfig
> +++ b/drivers/regulator/Kconfig
> @@ -64,6 +64,14 @@ config REGULATOR_USERSPACE_CONSUMER
>  
>            If unsure, say no.
>  
> +config OF_REGULATOR
> +	tristate "OF regulator helpers"
> +	depends on OF
> +	default y if OF
> +	help
> +	  OpenFirmware regulator framework helper routines that can
> +	  used by regulator drivers to extract data from device tree.
> +
>  config REGULATOR_BQ24022
>  	tristate "TI bq24022 Dual Input 1-Cell Li-Ion Charger IC"
>  	help
> diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
> index 040d5aa..e6bc009 100644
> --- a/drivers/regulator/Makefile
> +++ b/drivers/regulator/Makefile
> @@ -7,6 +7,7 @@ obj-$(CONFIG_REGULATOR) += core.o dummy.o
>  obj-$(CONFIG_REGULATOR_FIXED_VOLTAGE) += fixed.o
>  obj-$(CONFIG_REGULATOR_VIRTUAL_CONSUMER) += virtual.o
>  obj-$(CONFIG_REGULATOR_USERSPACE_CONSUMER) += userspace-consumer.o
> +obj-$(CONFIG_OF_REGULATOR) += of_regulator.o
>  
>  obj-$(CONFIG_REGULATOR_AD5398) += ad5398.o
>  obj-$(CONFIG_REGULATOR_BQ24022) += bq24022.o
> diff --git a/drivers/regulator/of_regulator.c b/drivers/regulator/of_regulator.c
> new file mode 100644
> index 0000000..9262d06
> --- /dev/null
> +++ b/drivers/regulator/of_regulator.c
> @@ -0,0 +1,93 @@
> +/*
> + * OF helpers for regulator framework
> + *
> + * Copyright (C) 2011 Texas Instruments, Inc.
> + * Rajendra Nayak <rnayak@ti.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +
> +#include <linux/slab.h>
> +#include <linux/of.h>
> +#include <linux/regulator/machine.h>
> +
> +static void of_get_regulation_constraints(struct device_node *np,
> +					struct regulator_init_data **init_data)
> +{
> +	const __be32 *min_uV, *max_uV, *uV_offset;
> +	const __be32 *min_uA, *max_uA, *input_uV;
> +	unsigned int valid_modes_mask = 0, valid_ops_mask = 0;
> +
> +	min_uV = of_get_property(np, "regulator-min-uV", NULL);
> +	if (min_uV)
> +		(*init_data)->constraints.min_uV = be32_to_cpu(*min_uV);
> +	max_uV = of_get_property(np, "regulator-max-uV", NULL);
> +	if (max_uV)
> +		(*init_data)->constraints.max_uV = be32_to_cpu(*max_uV);
> +	uV_offset = of_get_property(np, "regulator-uV-offset", NULL);
> +	if (uV_offset)
> +		(*init_data)->constraints.uV_offset = be32_to_cpu(*uV_offset);
> +	min_uA = of_get_property(np, "regulator-min-uA", NULL);
> +	if (min_uA)
> +		(*init_data)->constraints.min_uA = be32_to_cpu(*min_uA);
> +	max_uA = of_get_property(np, "regulator-max-uA", NULL);
> +	if (max_uA)
> +		(*init_data)->constraints.max_uA = be32_to_cpu(*max_uA);
> +	input_uV = of_get_property(np, "regulator-input-uV", NULL);
> +	if (input_uV)
> +		(*init_data)->constraints.input_uV = be32_to_cpu(*input_uV);
> +
> +	/* valid modes mask */
> +	if (of_find_property(np, "regulator-mode-fast", NULL))
> +				valid_modes_mask |= REGULATOR_MODE_FAST;
> +	if (of_find_property(np, "regulator-mode-normal", NULL))
> +				valid_modes_mask |= REGULATOR_MODE_NORMAL;
> +	if (of_find_property(np, "regulator-mode-idle", NULL))
> +				valid_modes_mask |= REGULATOR_MODE_IDLE;
> +	if (of_find_property(np, "regulator-mode-standby", NULL))
> +				valid_modes_mask |= REGULATOR_MODE_STANDBY;
> +
> +	/* valid ops mask */
> +	if (of_find_property(np, "regulator-change-voltage", NULL))
> +				valid_ops_mask |= REGULATOR_CHANGE_VOLTAGE;
> +	if (of_find_property(np, "regulator-change-current", NULL))
> +				valid_ops_mask |= REGULATOR_CHANGE_CURRENT;
> +	if (of_find_property(np, "regulator-change-mode", NULL))
> +				valid_ops_mask |= REGULATOR_CHANGE_MODE;
> +	if (of_find_property(np, "regulator-change-status", NULL))
> +				valid_ops_mask |= REGULATOR_CHANGE_STATUS;
> +
> +	(*init_data)->constraints.valid_modes_mask = valid_modes_mask;
> +	(*init_data)->constraints.valid_ops_mask = valid_ops_mask;
> +
> +	if (of_find_property(np, "regulator-always-on", NULL))
> +				(*init_data)->constraints.always_on = true;
> +	if (of_find_property(np, "regulator-boot-on", NULL))
> +				(*init_data)->constraints.boot_on = true;
> +}
> +
> +/**
> + * of_get_regulator_init_data - extract regulator_init_data structure info
> + * @dev: device requesting for regulator_init_data
> + *
> + * 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)

I remember that Mark ever had a comment on a previous regulator DT
series from Haojian Zhuang, saying this DT parsing procedure can
probably just be private to regulator core and invisible to regulator
drivers.  That said whenever regulator_register() gets called with
parameter init_data as NULL, it should try to retrieve
regulator_init_data from device tree.  It will ease regulator drivers
a little.  They will only need to call regulator_register() with NULL
init_data for DT case.  More importantly, doing so will help solve the
dual 'dev' problem I see below.

> +{
> +	struct regulator_init_data *init_data;
> +
> +	if (!dev->of_node)
> +		return NULL;
> +
> +	init_data = devm_kzalloc(dev, sizeof(*init_data), GFP_KERNEL);
> +	if (!init_data)
> +		return NULL; /* Out of memory? */
> +
> +	of_get_regulation_constraints(dev->of_node, &init_data);

Beside the 'dev' here with of_node attached, there will be another
'dev' created by regulator core function regulator_register(), which
is wrapped by 'regulator_dev'.

So we have two 'dev'.  One is created by DT core with of_node attached,
and used to retrieve regulator_init_data from device tree.  Another one
is created in regulator_register() and used by regulator core.

IMO, this is not what we want.  Instead of having two 'dev' for given
regulator, we should use the same 'dev' created in regulator_register()
to retrieve regulator_init_data from device tree, so that we do not need
the 'dev' created by DT core.  That said, with DT parsing procedure
moved into regulator core, we can get of_node for given regulator and
attach it to the 'dev' created by regulator core.

Will elaborate it a little more in patch #5.

Regards,
Shawn

> +	return init_data;
> +}
> diff --git a/include/linux/regulator/of_regulator.h b/include/linux/regulator/of_regulator.h
> new file mode 100644
> index 0000000..c5a1ad6
> --- /dev/null
> +++ b/include/linux/regulator/of_regulator.h
> @@ -0,0 +1,21 @@
> +/*
> + * OpenFirmware regulator support routines
> + *
> + */
> +
> +#ifndef __LINUX_OF_REG_H
> +#define __LINUX_OF_REG_H
> +
> +#if defined(CONFIG_OF_REGULATOR)
> +extern struct regulator_init_data
> +	*of_get_regulator_init_data(struct device *dev);
> +#else
> +static inline struct regulator_init_data
> +	*of_get_regulator_init_data(struct device *dev)
> +{
> +	return NULL;
> +}
> +#endif /* CONFIG_OF_REGULATOR */
> +
> +#endif /* __LINUX_OF_REG_H */
> +
> -- 
> 1.7.1
> 
> _______________________________________________
> devicetree-discuss mailing list
> devicetree-discuss at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss
> 

[PATCH v2 3/5] regulator: helper routine to extract regulator_init_data

[Rajendra Nayak]

On Tuesday 18 October 2011 06:50 PM, Shawn Guo wrote:
> On Mon, Oct 10, 2011 at 09:49:36PM +0530, Rajendra Nayak wrote:
>> The helper routine is meant to be used by regulator drivers
>> to extract the regulator_init_data structure from the data
>> that is passed from device tree.
>> 'consumer_supplies' which is part of regulator_init_data is not extracted
>> as the regulator consumer mappings are passed through DT differently,
>> implemented in subsequent patches.
>> Similarly the regulator<-->parent/supply mapping is handled in
>> subsequent patches.
>>
>> Also add documentation for regulator bindings to be used to pass
>> regulator_init_data struct information from device tree.
>>
>> Signed-off-by: Rajendra Nayak<rnayak@ti.com>
>> ---
>>   .../devicetree/bindings/regulator/regulator.txt    |   44 +++++++++
>>   drivers/regulator/Kconfig                          |    8 ++
>>   drivers/regulator/Makefile                         |    1 +
>>   drivers/regulator/of_regulator.c                   |   93 ++++++++++++++++++++
>>   include/linux/regulator/of_regulator.h             |   21 +++++
>>   5 files changed, 167 insertions(+), 0 deletions(-)
>>   create mode 100644 Documentation/devicetree/bindings/regulator/regulator.txt
>>   create mode 100644 drivers/regulator/of_regulator.c
>>   create mode 100644 include/linux/regulator/of_regulator.h
>>
>> diff --git a/Documentation/devicetree/bindings/regulator/regulator.txt b/Documentation/devicetree/bindings/regulator/regulator.txt
>> new file mode 100644
>> index 0000000..a623fdd
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/regulator/regulator.txt
>> @@ -0,0 +1,44 @@
>> +Voltage/Current Regulators
>> +
>> +Optional properties:
>> +- regulator-min-uV: smallest voltage consumers may set
>> +- regulator-max-uV: largest voltage consumers may set
>> +- regulator-uV-offset: Offset applied to voltages to compensate for voltage drops
>> +- regulator-min-uA: smallest current consumers may set
>> +- regulator-max-uA: largest current consumers may set
>> +- regulator-change-voltage: boolean, Output voltage can be changed by software
>> +- regulator-change-current: boolean, Output current can be chnaged by software
>> +- regulator-change-mode: boolean, Operating mode can be changed by software
>> +- regulator-change-status: boolean, Enable/Disable control for regulator exists
>> +- regulator-change-drms: boolean, Dynamic regulator mode switching is enabled
>> +- regulator-mode-fast: boolean, allow/set fast mode for the regulator
>> +- regulator-mode-normal: boolean, allow/set normal mode for the regulator
>> +- regulator-mode-idle: boolean, allow/set idle mode for the regulator
>> +- regulator-mode-standby: boolean, allow/set standby mode for the regulator
>> +- regulator-input-uV: Input voltage for regulator when supplied by another regulator
>> +- regulator-always-on: boolean, regulator should never be disabled
>> +- regulator-boot-on: bootloader/firmware enabled regulator
>> +-<name>-supply: phandle to the parent supply/regulator node
>> +
>> +Example:
>> +
>> +	xyzreg: regulator at 0 {
>
> Does this node have a compatible string?  With looking at the code,
> I guess it has a compatible string in your dts file.

The compatible string depends on the regulator driver used.
I added one for the twl regulators when I added support for
them.

>
>> +		regulator-min-uV =<1000000>;
>> +		regulator-max-uV =<2500000>;
>> +		regulator-mode-fast;
>> +		regulator-change-voltage;
>> +		regulator-always-on;
>> +		vin-supply =<&vin>;
>> +	};
>> +
>> +The same binding used by a regulator to reference its
>> +supply can be used by any consumer to reference its
>> +regulator/supply
>> +
>> +Example of a device node referencing a regulator node,
>> +
>> +	devicenode: node at 0x0 {
>> +		...
>> +		...
>> +		<name>-supply =<&xyzreg>;
>> +	};
>> diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
>> index c7fd2c0..981c92e 100644
>> --- a/drivers/regulator/Kconfig
>> +++ b/drivers/regulator/Kconfig
>> @@ -64,6 +64,14 @@ config REGULATOR_USERSPACE_CONSUMER
>>
>>             If unsure, say no.
>>
>> +config OF_REGULATOR
>> +	tristate "OF regulator helpers"
>> +	depends on OF
>> +	default y if OF
>> +	help
>> +	  OpenFirmware regulator framework helper routines that can
>> +	  used by regulator drivers to extract data from device tree.
>> +
>>   config REGULATOR_BQ24022
>>   	tristate "TI bq24022 Dual Input 1-Cell Li-Ion Charger IC"
>>   	help
>> diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
>> index 040d5aa..e6bc009 100644
>> --- a/drivers/regulator/Makefile
>> +++ b/drivers/regulator/Makefile
>> @@ -7,6 +7,7 @@ obj-$(CONFIG_REGULATOR) += core.o dummy.o
>>   obj-$(CONFIG_REGULATOR_FIXED_VOLTAGE) += fixed.o
>>   obj-$(CONFIG_REGULATOR_VIRTUAL_CONSUMER) += virtual.o
>>   obj-$(CONFIG_REGULATOR_USERSPACE_CONSUMER) += userspace-consumer.o
>> +obj-$(CONFIG_OF_REGULATOR) += of_regulator.o
>>
>>   obj-$(CONFIG_REGULATOR_AD5398) += ad5398.o
>>   obj-$(CONFIG_REGULATOR_BQ24022) += bq24022.o
>> diff --git a/drivers/regulator/of_regulator.c b/drivers/regulator/of_regulator.c
>> new file mode 100644
>> index 0000000..9262d06
>> --- /dev/null
>> +++ b/drivers/regulator/of_regulator.c
>> @@ -0,0 +1,93 @@
>> +/*
>> + * OF helpers for regulator framework
>> + *
>> + * Copyright (C) 2011 Texas Instruments, Inc.
>> + * Rajendra Nayak<rnayak@ti.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + */
>> +
>> +#include<linux/slab.h>
>> +#include<linux/of.h>
>> +#include<linux/regulator/machine.h>
>> +
>> +static void of_get_regulation_constraints(struct device_node *np,
>> +					struct regulator_init_data **init_data)
>> +{
>> +	const __be32 *min_uV, *max_uV, *uV_offset;
>> +	const __be32 *min_uA, *max_uA, *input_uV;
>> +	unsigned int valid_modes_mask = 0, valid_ops_mask = 0;
>> +
>> +	min_uV = of_get_property(np, "regulator-min-uV", NULL);
>> +	if (min_uV)
>> +		(*init_data)->constraints.min_uV = be32_to_cpu(*min_uV);
>> +	max_uV = of_get_property(np, "regulator-max-uV", NULL);
>> +	if (max_uV)
>> +		(*init_data)->constraints.max_uV = be32_to_cpu(*max_uV);
>> +	uV_offset = of_get_property(np, "regulator-uV-offset", NULL);
>> +	if (uV_offset)
>> +		(*init_data)->constraints.uV_offset = be32_to_cpu(*uV_offset);
>> +	min_uA = of_get_property(np, "regulator-min-uA", NULL);
>> +	if (min_uA)
>> +		(*init_data)->constraints.min_uA = be32_to_cpu(*min_uA);
>> +	max_uA = of_get_property(np, "regulator-max-uA", NULL);
>> +	if (max_uA)
>> +		(*init_data)->constraints.max_uA = be32_to_cpu(*max_uA);
>> +	input_uV = of_get_property(np, "regulator-input-uV", NULL);
>> +	if (input_uV)
>> +		(*init_data)->constraints.input_uV = be32_to_cpu(*input_uV);
>> +
>> +	/* valid modes mask */
>> +	if (of_find_property(np, "regulator-mode-fast", NULL))
>> +				valid_modes_mask |= REGULATOR_MODE_FAST;
>> +	if (of_find_property(np, "regulator-mode-normal", NULL))
>> +				valid_modes_mask |= REGULATOR_MODE_NORMAL;
>> +	if (of_find_property(np, "regulator-mode-idle", NULL))
>> +				valid_modes_mask |= REGULATOR_MODE_IDLE;
>> +	if (of_find_property(np, "regulator-mode-standby", NULL))
>> +				valid_modes_mask |= REGULATOR_MODE_STANDBY;
>> +
>> +	/* valid ops mask */
>> +	if (of_find_property(np, "regulator-change-voltage", NULL))
>> +				valid_ops_mask |= REGULATOR_CHANGE_VOLTAGE;
>> +	if (of_find_property(np, "regulator-change-current", NULL))
>> +				valid_ops_mask |= REGULATOR_CHANGE_CURRENT;
>> +	if (of_find_property(np, "regulator-change-mode", NULL))
>> +				valid_ops_mask |= REGULATOR_CHANGE_MODE;
>> +	if (of_find_property(np, "regulator-change-status", NULL))
>> +				valid_ops_mask |= REGULATOR_CHANGE_STATUS;
>> +
>> +	(*init_data)->constraints.valid_modes_mask = valid_modes_mask;
>> +	(*init_data)->constraints.valid_ops_mask = valid_ops_mask;
>> +
>> +	if (of_find_property(np, "regulator-always-on", NULL))
>> +				(*init_data)->constraints.always_on = true;
>> +	if (of_find_property(np, "regulator-boot-on", NULL))
>> +				(*init_data)->constraints.boot_on = true;
>> +}
>> +
>> +/**
>> + * of_get_regulator_init_data - extract regulator_init_data structure info
>> + * @dev: device requesting for regulator_init_data
>> + *
>> + * 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)
>
> I remember that Mark ever had a comment on a previous regulator DT
> series from Haojian Zhuang, saying this DT parsing procedure can
> probably just be private to regulator core and invisible to regulator
> drivers.  That said whenever regulator_register() gets called with
> parameter init_data as NULL, it should try to retrieve
> regulator_init_data from device tree.  It will ease regulator drivers
> a little.  They will only need to call regulator_register() with NULL
> init_data for DT case.  More importantly, doing so will help solve the
> dual 'dev' problem I see below.

Yes, it seems like a good idea given that most drivers seem to blindly
pass the regulator_init_data onto regulator_register, however there
are cases like the twl regulator driver which seems to peek into the
constraints passed from the board to make sure it drops anything that
the hardware does not support, or cases like the db8500 where
regulator_init_data for all regulators are bundled together and the
driver extracts and registers them as separate regulators. Exporting an
api instead to extract regulator_init_data to the driver might help
in those cases.

>
>> +{
>> +	struct regulator_init_data *init_data;
>> +
>> +	if (!dev->of_node)
>> +		return NULL;
>> +
>> +	init_data = devm_kzalloc(dev, sizeof(*init_data), GFP_KERNEL);
>> +	if (!init_data)
>> +		return NULL; /* Out of memory? */
>> +
>> +	of_get_regulation_constraints(dev->of_node,&init_data);
>
> Beside the 'dev' here with of_node attached, there will be another
> 'dev' created by regulator core function regulator_register(), which
> is wrapped by 'regulator_dev'.
>
> So we have two 'dev'.  One is created by DT core with of_node attached,
> and used to retrieve regulator_init_data from device tree.  Another one
> is created in regulator_register() and used by regulator core.

But thats not something newly done now with DT. Thats how it was even
in the non-DT world. There were always two devices with the
regulator_dev device as the child.

>
> IMO, this is not what we want.  Instead of having two 'dev' for given
> regulator, we should use the same 'dev' created in regulator_register()
> to retrieve regulator_init_data from device tree, so that we do not need
> the 'dev' created by DT core.  That said, with DT parsing procedure
> moved into regulator core, we can get of_node for given regulator and
> attach it to the 'dev' created by regulator core.
>
> Will elaborate it a little more in patch #5.
>
> Regards,
> Shawn
>
>> +	return init_data;
>> +}
>> diff --git a/include/linux/regulator/of_regulator.h b/include/linux/regulator/of_regulator.h
>> new file mode 100644
>> index 0000000..c5a1ad6
>> --- /dev/null
>> +++ b/include/linux/regulator/of_regulator.h
>> @@ -0,0 +1,21 @@
>> +/*
>> + * OpenFirmware regulator support routines
>> + *
>> + */
>> +
>> +#ifndef __LINUX_OF_REG_H
>> +#define __LINUX_OF_REG_H
>> +
>> +#if defined(CONFIG_OF_REGULATOR)
>> +extern struct regulator_init_data
>> +	*of_get_regulator_init_data(struct device *dev);
>> +#else
>> +static inline struct regulator_init_data
>> +	*of_get_regulator_init_data(struct device *dev)
>> +{
>> +	return NULL;
>> +}
>> +#endif /* CONFIG_OF_REGULATOR */
>> +
>> +#endif /* __LINUX_OF_REG_H */
>> +
>> --
>> 1.7.1
>>
>> _______________________________________________
>> devicetree-discuss mailing list
>> devicetree-discuss at lists.ozlabs.org
>> https://lists.ozlabs.org/listinfo/devicetree-discuss
>>
>

[PATCH v2 3/5] regulator: helper routine to extract regulator_init_data

[Shawn Guo]

On Wed, Oct 19, 2011 at 05:05:56PM +0530, Rajendra Nayak wrote:
> On Tuesday 18 October 2011 06:50 PM, Shawn Guo wrote:
> >On Mon, Oct 10, 2011 at 09:49:36PM +0530, Rajendra Nayak wrote:
> >>The helper routine is meant to be used by regulator drivers
> >>to extract the regulator_init_data structure from the data
> >>that is passed from device tree.
> >>'consumer_supplies' which is part of regulator_init_data is not extracted
> >>as the regulator consumer mappings are passed through DT differently,
> >>implemented in subsequent patches.
> >>Similarly the regulator<-->parent/supply mapping is handled in
> >>subsequent patches.
> >>
> >>Also add documentation for regulator bindings to be used to pass
> >>regulator_init_data struct information from device tree.
> >>
> >>Signed-off-by: Rajendra Nayak<rnayak@ti.com>
> >>---
> >>  .../devicetree/bindings/regulator/regulator.txt    |   44 +++++++++
> >>  drivers/regulator/Kconfig                          |    8 ++
> >>  drivers/regulator/Makefile                         |    1 +
> >>  drivers/regulator/of_regulator.c                   |   93 ++++++++++++++++++++
> >>  include/linux/regulator/of_regulator.h             |   21 +++++
> >>  5 files changed, 167 insertions(+), 0 deletions(-)
> >>  create mode 100644 Documentation/devicetree/bindings/regulator/regulator.txt
> >>  create mode 100644 drivers/regulator/of_regulator.c
> >>  create mode 100644 include/linux/regulator/of_regulator.h
> >>
> >>diff --git a/Documentation/devicetree/bindings/regulator/regulator.txt b/Documentation/devicetree/bindings/regulator/regulator.txt
> >>new file mode 100644
> >>index 0000000..a623fdd
> >>--- /dev/null
> >>+++ b/Documentation/devicetree/bindings/regulator/regulator.txt
> >>@@ -0,0 +1,44 @@
> >>+Voltage/Current Regulators
> >>+
> >>+Optional properties:
> >>+- regulator-min-uV: smallest voltage consumers may set
> >>+- regulator-max-uV: largest voltage consumers may set
> >>+- regulator-uV-offset: Offset applied to voltages to compensate for voltage drops
> >>+- regulator-min-uA: smallest current consumers may set
> >>+- regulator-max-uA: largest current consumers may set
> >>+- regulator-change-voltage: boolean, Output voltage can be changed by software
> >>+- regulator-change-current: boolean, Output current can be chnaged by software
> >>+- regulator-change-mode: boolean, Operating mode can be changed by software
> >>+- regulator-change-status: boolean, Enable/Disable control for regulator exists
> >>+- regulator-change-drms: boolean, Dynamic regulator mode switching is enabled
> >>+- regulator-mode-fast: boolean, allow/set fast mode for the regulator
> >>+- regulator-mode-normal: boolean, allow/set normal mode for the regulator
> >>+- regulator-mode-idle: boolean, allow/set idle mode for the regulator
> >>+- regulator-mode-standby: boolean, allow/set standby mode for the regulator
> >>+- regulator-input-uV: Input voltage for regulator when supplied by another regulator
> >>+- regulator-always-on: boolean, regulator should never be disabled
> >>+- regulator-boot-on: bootloader/firmware enabled regulator
> >>+-<name>-supply: phandle to the parent supply/regulator node
> >>+
> >>+Example:
> >>+
> >>+	xyzreg: regulator at 0 {
> >
> >Does this node have a compatible string?  With looking at the code,
> >I guess it has a compatible string in your dts file.
> 
> The compatible string depends on the regulator driver used.
> I added one for the twl regulators when I added support for
> them.
> 
> >
> >>+		regulator-min-uV =<1000000>;
> >>+		regulator-max-uV =<2500000>;
> >>+		regulator-mode-fast;
> >>+		regulator-change-voltage;
> >>+		regulator-always-on;
> >>+		vin-supply =<&vin>;
> >>+	};
> >>+
> >>+The same binding used by a regulator to reference its
> >>+supply can be used by any consumer to reference its
> >>+regulator/supply
> >>+
> >>+Example of a device node referencing a regulator node,
> >>+
> >>+	devicenode: node at 0x0 {
> >>+		...
> >>+		...
> >>+		<name>-supply =<&xyzreg>;
> >>+	};
> >>diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
> >>index c7fd2c0..981c92e 100644
> >>--- a/drivers/regulator/Kconfig
> >>+++ b/drivers/regulator/Kconfig
> >>@@ -64,6 +64,14 @@ config REGULATOR_USERSPACE_CONSUMER
> >>
> >>            If unsure, say no.
> >>
> >>+config OF_REGULATOR
> >>+	tristate "OF regulator helpers"
> >>+	depends on OF
> >>+	default y if OF
> >>+	help
> >>+	  OpenFirmware regulator framework helper routines that can
> >>+	  used by regulator drivers to extract data from device tree.
> >>+
> >>  config REGULATOR_BQ24022
> >>  	tristate "TI bq24022 Dual Input 1-Cell Li-Ion Charger IC"
> >>  	help
> >>diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
> >>index 040d5aa..e6bc009 100644
> >>--- a/drivers/regulator/Makefile
> >>+++ b/drivers/regulator/Makefile
> >>@@ -7,6 +7,7 @@ obj-$(CONFIG_REGULATOR) += core.o dummy.o
> >>  obj-$(CONFIG_REGULATOR_FIXED_VOLTAGE) += fixed.o
> >>  obj-$(CONFIG_REGULATOR_VIRTUAL_CONSUMER) += virtual.o
> >>  obj-$(CONFIG_REGULATOR_USERSPACE_CONSUMER) += userspace-consumer.o
> >>+obj-$(CONFIG_OF_REGULATOR) += of_regulator.o
> >>
> >>  obj-$(CONFIG_REGULATOR_AD5398) += ad5398.o
> >>  obj-$(CONFIG_REGULATOR_BQ24022) += bq24022.o
> >>diff --git a/drivers/regulator/of_regulator.c b/drivers/regulator/of_regulator.c
> >>new file mode 100644
> >>index 0000000..9262d06
> >>--- /dev/null
> >>+++ b/drivers/regulator/of_regulator.c
> >>@@ -0,0 +1,93 @@
> >>+/*
> >>+ * OF helpers for regulator framework
> >>+ *
> >>+ * Copyright (C) 2011 Texas Instruments, Inc.
> >>+ * Rajendra Nayak<rnayak@ti.com>
> >>+ *
> >>+ * This program is free software; you can redistribute it and/or modify
> >>+ * it under the terms of the GNU General Public License as published by
> >>+ * the Free Software Foundation; either version 2 of the License, or
> >>+ * (at your option) any later version.
> >>+ */
> >>+
> >>+#include<linux/slab.h>
> >>+#include<linux/of.h>
> >>+#include<linux/regulator/machine.h>
> >>+
> >>+static void of_get_regulation_constraints(struct device_node *np,
> >>+					struct regulator_init_data **init_data)
> >>+{
> >>+	const __be32 *min_uV, *max_uV, *uV_offset;
> >>+	const __be32 *min_uA, *max_uA, *input_uV;
> >>+	unsigned int valid_modes_mask = 0, valid_ops_mask = 0;
> >>+
> >>+	min_uV = of_get_property(np, "regulator-min-uV", NULL);
> >>+	if (min_uV)
> >>+		(*init_data)->constraints.min_uV = be32_to_cpu(*min_uV);
> >>+	max_uV = of_get_property(np, "regulator-max-uV", NULL);
> >>+	if (max_uV)
> >>+		(*init_data)->constraints.max_uV = be32_to_cpu(*max_uV);
> >>+	uV_offset = of_get_property(np, "regulator-uV-offset", NULL);
> >>+	if (uV_offset)
> >>+		(*init_data)->constraints.uV_offset = be32_to_cpu(*uV_offset);
> >>+	min_uA = of_get_property(np, "regulator-min-uA", NULL);
> >>+	if (min_uA)
> >>+		(*init_data)->constraints.min_uA = be32_to_cpu(*min_uA);
> >>+	max_uA = of_get_property(np, "regulator-max-uA", NULL);
> >>+	if (max_uA)
> >>+		(*init_data)->constraints.max_uA = be32_to_cpu(*max_uA);
> >>+	input_uV = of_get_property(np, "regulator-input-uV", NULL);
> >>+	if (input_uV)
> >>+		(*init_data)->constraints.input_uV = be32_to_cpu(*input_uV);
> >>+
> >>+	/* valid modes mask */
> >>+	if (of_find_property(np, "regulator-mode-fast", NULL))
> >>+				valid_modes_mask |= REGULATOR_MODE_FAST;
> >>+	if (of_find_property(np, "regulator-mode-normal", NULL))
> >>+				valid_modes_mask |= REGULATOR_MODE_NORMAL;
> >>+	if (of_find_property(np, "regulator-mode-idle", NULL))
> >>+				valid_modes_mask |= REGULATOR_MODE_IDLE;
> >>+	if (of_find_property(np, "regulator-mode-standby", NULL))
> >>+				valid_modes_mask |= REGULATOR_MODE_STANDBY;
> >>+
> >>+	/* valid ops mask */
> >>+	if (of_find_property(np, "regulator-change-voltage", NULL))
> >>+				valid_ops_mask |= REGULATOR_CHANGE_VOLTAGE;
> >>+	if (of_find_property(np, "regulator-change-current", NULL))
> >>+				valid_ops_mask |= REGULATOR_CHANGE_CURRENT;
> >>+	if (of_find_property(np, "regulator-change-mode", NULL))
> >>+				valid_ops_mask |= REGULATOR_CHANGE_MODE;
> >>+	if (of_find_property(np, "regulator-change-status", NULL))
> >>+				valid_ops_mask |= REGULATOR_CHANGE_STATUS;
> >>+
> >>+	(*init_data)->constraints.valid_modes_mask = valid_modes_mask;
> >>+	(*init_data)->constraints.valid_ops_mask = valid_ops_mask;
> >>+
> >>+	if (of_find_property(np, "regulator-always-on", NULL))
> >>+				(*init_data)->constraints.always_on = true;
> >>+	if (of_find_property(np, "regulator-boot-on", NULL))
> >>+				(*init_data)->constraints.boot_on = true;
> >>+}
> >>+
> >>+/**
> >>+ * of_get_regulator_init_data - extract regulator_init_data structure info
> >>+ * @dev: device requesting for regulator_init_data
> >>+ *
> >>+ * 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)
> >
> >I remember that Mark ever had a comment on a previous regulator DT
> >series from Haojian Zhuang, saying this DT parsing procedure can
> >probably just be private to regulator core and invisible to regulator
> >drivers.  That said whenever regulator_register() gets called with
> >parameter init_data as NULL, it should try to retrieve
> >regulator_init_data from device tree.  It will ease regulator drivers
> >a little.  They will only need to call regulator_register() with NULL
> >init_data for DT case.  More importantly, doing so will help solve the
> >dual 'dev' problem I see below.
> 
> Yes, it seems like a good idea given that most drivers seem to blindly
> pass the regulator_init_data onto regulator_register, however there
> are cases like the twl regulator driver which seems to peek into the
> constraints passed from the board to make sure it drops anything that
> the hardware does not support,

I'm not sure why twl works in that way.  Is it a sign that those
configuration peeked by twl regulator driver should be encoded in twl
regulator driver itself instead of being passed from the board?  Or
why the board does not pass something matching driver/hardware
capability to save that peek?

> or cases like the db8500 where
> regulator_init_data for all regulators are bundled together and the
> driver extracts and registers them as separate regulators.

For this particular case, we just end up with having some duplicated
constraints description in the dts file.  To me, it's not a problem.

> Exporting an
> api instead to extract regulator_init_data to the driver might help
> in those cases.
> 
IMO, these cases should be generalized to fit the common pattern of
regulator drivers.

> >
> >>+{
> >>+	struct regulator_init_data *init_data;
> >>+
> >>+	if (!dev->of_node)
> >>+		return NULL;
> >>+
> >>+	init_data = devm_kzalloc(dev, sizeof(*init_data), GFP_KERNEL);
> >>+	if (!init_data)
> >>+		return NULL; /* Out of memory? */
> >>+
> >>+	of_get_regulation_constraints(dev->of_node,&init_data);
> >
> >Beside the 'dev' here with of_node attached, there will be another
> >'dev' created by regulator core function regulator_register(), which
> >is wrapped by 'regulator_dev'.
> >
> >So we have two 'dev'.  One is created by DT core with of_node attached,
> >and used to retrieve regulator_init_data from device tree.  Another one
> >is created in regulator_register() and used by regulator core.
> 
> But thats not something newly done now with DT. Thats how it was even
> in the non-DT world. There were always two devices with the
> regulator_dev device as the child.
> 
Let's look at mc13892-regulator driver.  There are 23 regulators defined
in array mc13892_regulators.  Needless to say, there is a dev behind
mc13892-regulator driver.  And when getting probed, this driver will
call regulator_register() to register those 23 regulators individually.
That said, for non-dt world, we have 1 + 23 'dev' with that 1 as the
parent of all other 23 'dev' (wrapped by regulator_dev).  But with the
current DT implementation, we will have at least 1 + 23 * 2 'dev'.
These extra 23 'dev' is totally new with DT.

Regards,
Shawn

> >
> >IMO, this is not what we want.  Instead of having two 'dev' for given
> >regulator, we should use the same 'dev' created in regulator_register()
> >to retrieve regulator_init_data from device tree, so that we do not need
> >the 'dev' created by DT core.  That said, with DT parsing procedure
> >moved into regulator core, we can get of_node for given regulator and
> >attach it to the 'dev' created by regulator core.
> >
> >Will elaborate it a little more in patch #5.
> >
> >Regards,
> >Shawn
> >
> >>+	return init_data;
> >>+}
> >>diff --git a/include/linux/regulator/of_regulator.h b/include/linux/regulator/of_regulator.h
> >>new file mode 100644
> >>index 0000000..c5a1ad6
> >>--- /dev/null
> >>+++ b/include/linux/regulator/of_regulator.h
> >>@@ -0,0 +1,21 @@
> >>+/*
> >>+ * OpenFirmware regulator support routines
> >>+ *
> >>+ */
> >>+
> >>+#ifndef __LINUX_OF_REG_H
> >>+#define __LINUX_OF_REG_H
> >>+
> >>+#if defined(CONFIG_OF_REGULATOR)
> >>+extern struct regulator_init_data
> >>+	*of_get_regulator_init_data(struct device *dev);
> >>+#else
> >>+static inline struct regulator_init_data
> >>+	*of_get_regulator_init_data(struct device *dev)
> >>+{
> >>+	return NULL;
> >>+}
> >>+#endif /* CONFIG_OF_REGULATOR */
> >>+
> >>+#endif /* __LINUX_OF_REG_H */
> >>+
> >>--
> >>1.7.1
> >>
> >>_______________________________________________
> >>devicetree-discuss mailing list
> >>devicetree-discuss at lists.ozlabs.org
> >>https://lists.ozlabs.org/listinfo/devicetree-discuss
> >>
> >
> 
> 

-- 
Regards,
Shawn

[PATCH v2 3/5] regulator: helper routine to extract regulator_init_data

[Mark Brown]

On Wed, Oct 19, 2011 at 10:42:16PM +0800, Shawn Guo wrote:

*Please* delete irrelevant quotes from mails.

> On Wed, Oct 19, 2011 at 05:05:56PM +0530, Rajendra Nayak wrote:

> > Yes, it seems like a good idea given that most drivers seem to blindly
> > pass the regulator_init_data onto regulator_register, however there
> > are cases like the twl regulator driver which seems to peek into the
> > constraints passed from the board to make sure it drops anything that
> > the hardware does not support,

> I'm not sure why twl works in that way.  Is it a sign that those
> configuration peeked by twl regulator driver should be encoded in twl
> regulator driver itself instead of being passed from the board?  Or
> why the board does not pass something matching driver/hardware
> capability to save that peek?

It's completely unneeded but harmless, it's there because there were a
large number of discussions going on with the original author and it was
easier to merge the code.

[PATCH v2 3/5] regulator: helper routine to extract regulator_init_data

[Rajendra Nayak]

>
> I'm not sure why twl works in that way.  Is it a sign that those
> configuration peeked by twl regulator driver should be encoded in twl
> regulator driver itself instead of being passed from the board?  Or

No, the driver is just trying to make sure that nothing invalid (not
supported by hardware) gets passed from the boards.

> why the board does not pass something matching driver/hardware
> capability to save that peek?
>
>> or cases like the db8500 where
>> regulator_init_data for all regulators are bundled together and the
>> driver extracts and registers them as separate regulators.
>
> For this particular case, we just end up with having some duplicated
> constraints description in the dts file.  To me, it's not a problem.
>
>> Exporting an
>> api instead to extract regulator_init_data to the driver might help
>> in those cases.
>>
> IMO, these cases should be generalized to fit the common pattern of
> regulator drivers.
>
>>>
>>>> +{
>>>> +	struct regulator_init_data *init_data;
>>>> +
>>>> +	if (!dev->of_node)
>>>> +		return NULL;
>>>> +
>>>> +	init_data = devm_kzalloc(dev, sizeof(*init_data), GFP_KERNEL);
>>>> +	if (!init_data)
>>>> +		return NULL; /* Out of memory? */
>>>> +
>>>> +	of_get_regulation_constraints(dev->of_node,&init_data);
>>>
>>> Beside the 'dev' here with of_node attached, there will be another
>>> 'dev' created by regulator core function regulator_register(), which
>>> is wrapped by 'regulator_dev'.
>>>
>>> So we have two 'dev'.  One is created by DT core with of_node attached,
>>> and used to retrieve regulator_init_data from device tree.  Another one
>>> is created in regulator_register() and used by regulator core.
>>
>> But thats not something newly done now with DT. Thats how it was even
>> in the non-DT world. There were always two devices with the
>> regulator_dev device as the child.
>>
> Let's look at mc13892-regulator driver.  There are 23 regulators defined
> in array mc13892_regulators.  Needless to say, there is a dev behind
> mc13892-regulator driver.  And when getting probed, this driver will
> call regulator_register() to register those 23 regulators individually.
> That said, for non-dt world, we have 1 + 23 'dev' with that 1 as the
> parent of all other 23 'dev' (wrapped by regulator_dev).  But with the
> current DT implementation, we will have at least 1 + 23 * 2 'dev'.
> These extra 23 'dev' is totally new with DT.
>

but thats only because the mc13892-regulator driver is implemeted in
such a way that all the regulators on the platform are bundled in as
*one* device. It would again depend on how you would pass these from
the DT, if you indeed stick to the same way of bundling all regulators
as one device from DT, the mc13892-regulator probe would just get called
once and there would be one device associated, no?

[PATCH v2 3/5] regulator: helper routine to extract regulator_init_data

[Shawn Guo]

On Thu, Oct 20, 2011 at 10:48:58AM +0530, Rajendra Nayak wrote:
> >Let's look at mc13892-regulator driver.  There are 23 regulators defined
> >in array mc13892_regulators.  Needless to say, there is a dev behind
> >mc13892-regulator driver.  And when getting probed, this driver will
> >call regulator_register() to register those 23 regulators individually.
> >That said, for non-dt world, we have 1 + 23 'dev' with that 1 as the
> >parent of all other 23 'dev' (wrapped by regulator_dev).  But with the
> >current DT implementation, we will have at least 1 + 23 * 2 'dev'.
> >These extra 23 'dev' is totally new with DT.
> >
> 
> but thats only because the mc13892-regulator driver is implemeted in
> such a way that all the regulators on the platform are bundled in as
> *one* device.

I did not look into too many regulator drivers, but I expect this is
way that most regulator drivers are implemented in.  Having
mc13892-regulator being probed 23 times to register these 23 regulators
just makes less sense to me.

> It would again depend on how you would pass these from
> the DT, if you indeed stick to the same way of bundling all regulators
> as one device from DT, the mc13892-regulator probe would just get called
> once and there would be one device associated, no?
> 
Yes, I indeed would stick to the same way of bundling the registration
of all regulators with mc13892-regulator being probed once.  The problem
I have with the current regulator core DT implementation is that it
assumes the device_node of rdev->dev (dev wrapped in regulator_dev) is
being attached to rdev->dev.parent rather than itself.  Back to
mc13892-regulator example, that said, it requires the dev of
mc13892-regulator have the device_node of individual regulator attached
to.  IOW, the current implementation forces mc13892-regulator to be
probed 23 times to register those 23 regulators.  This is wrong to me.

-- 
Regards,
Shawn

[PATCH v2 3/5] regulator: helper routine to extract regulator_init_data

[Rajendra Nayak]

On Thursday 20 October 2011 11:44 AM, Shawn Guo wrote:
> On Thu, Oct 20, 2011 at 10:48:58AM +0530, Rajendra Nayak wrote:
>>> Let's look at mc13892-regulator driver.  There are 23 regulators defined
>>> in array mc13892_regulators.  Needless to say, there is a dev behind
>>> mc13892-regulator driver.  And when getting probed, this driver will
>>> call regulator_register() to register those 23 regulators individually.
>>> That said, for non-dt world, we have 1 + 23 'dev' with that 1 as the
>>> parent of all other 23 'dev' (wrapped by regulator_dev).  But with the
>>> current DT implementation, we will have at least 1 + 23 * 2 'dev'.
>>> These extra 23 'dev' is totally new with DT.
>>>
>>
>> but thats only because the mc13892-regulator driver is implemeted in
>> such a way that all the regulators on the platform are bundled in as
>> *one* device.
>
> I did not look into too many regulator drivers, but I expect this is
> way that most regulator drivers are implemented in.  Having
> mc13892-regulator being probed 23 times to register these 23 regulators
> just makes less sense to me.
>
>> It would again depend on how you would pass these from
>> the DT, if you indeed stick to the same way of bundling all regulators
>> as one device from DT, the mc13892-regulator probe would just get called
>> once and there would be one device associated, no?
>>
> Yes, I indeed would stick to the same way of bundling the registration
> of all regulators with mc13892-regulator being probed once.  The problem
> I have with the current regulator core DT implementation is that it
> assumes the device_node of rdev->dev (dev wrapped in regulator_dev) is
> being attached to rdev->dev.parent rather than itself.  Back to
> mc13892-regulator example, that said, it requires the dev of
> mc13892-regulator have the device_node of individual regulator attached
> to.  IOW, the current implementation forces mc13892-regulator to be
> probed 23 times to register those 23 regulators.  This is wrong to me.

I think I now understand to some extent the problem that you seem to be
reporting. It is mainly with drivers which bundle all regulators and
pass them as one device and would want to do so with DT too.

however I am still not clear on how what you seem to suggest would
solve this problem. Note that not all drivers do it this way, and
there are drivers where each regulator is considered as one device
and I suspect they would remain that way with DT too. And hence we
need to support both.

Do you have any RFC patch/code which could explain better what you are
suggesting we do here?
>

[PATCH v2 3/5] regulator: helper routine to extract regulator_init_data

[Shawn Guo]

On Thu, Oct 20, 2011 at 05:39:32PM +0530, Rajendra Nayak wrote:
> On Thursday 20 October 2011 11:44 AM, Shawn Guo wrote:
> >On Thu, Oct 20, 2011 at 10:48:58AM +0530, Rajendra Nayak wrote:
> >>>Let's look at mc13892-regulator driver.  There are 23 regulators defined
> >>>in array mc13892_regulators.  Needless to say, there is a dev behind
> >>>mc13892-regulator driver.  And when getting probed, this driver will
> >>>call regulator_register() to register those 23 regulators individually.
> >>>That said, for non-dt world, we have 1 + 23 'dev' with that 1 as the
> >>>parent of all other 23 'dev' (wrapped by regulator_dev).  But with the
> >>>current DT implementation, we will have at least 1 + 23 * 2 'dev'.
> >>>These extra 23 'dev' is totally new with DT.
> >>>
> >>
> >>but thats only because the mc13892-regulator driver is implemeted in
> >>such a way that all the regulators on the platform are bundled in as
> >>*one* device.
> >
> >I did not look into too many regulator drivers, but I expect this is
> >way that most regulator drivers are implemented in.  Having
> >mc13892-regulator being probed 23 times to register these 23 regulators
> >just makes less sense to me.
> >
> >>It would again depend on how you would pass these from
> >>the DT, if you indeed stick to the same way of bundling all regulators
> >>as one device from DT, the mc13892-regulator probe would just get called
> >>once and there would be one device associated, no?
> >>
> >Yes, I indeed would stick to the same way of bundling the registration
> >of all regulators with mc13892-regulator being probed once.  The problem
> >I have with the current regulator core DT implementation is that it
> >assumes the device_node of rdev->dev (dev wrapped in regulator_dev) is
> >being attached to rdev->dev.parent rather than itself.  Back to
> >mc13892-regulator example, that said, it requires the dev of
> >mc13892-regulator have the device_node of individual regulator attached
> >to.  IOW, the current implementation forces mc13892-regulator to be
> >probed 23 times to register those 23 regulators.  This is wrong to me.
> 
> I think I now understand to some extent the problem that you seem to be
> reporting. It is mainly with drivers which bundle all regulators and
> pass them as one device and would want to do so with DT too.
> 
> however I am still not clear on how what you seem to suggest would
> solve this problem. Note that not all drivers do it this way, and
> there are drivers where each regulator is considered as one device
> and I suspect they would remain that way with DT too. And hence we
> need to support both.
> 
> Do you have any RFC patch/code which could explain better what you are
> suggesting we do here?
> >
Here is what I changed based on your patches.  It only changes
drivers/regulator/core.c.

---8<-------
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 9a5ebbe..8fe132d 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1211,7 +1211,7 @@ static struct regulator *_regulator_get(struct device *dev, const char *id,
                node = of_get_regulator(dev, id);
                if (node)
                        list_for_each_entry(rdev, &regulator_list, list)
-                               if (node == rdev->dev.parent->of_node)
+                               if (node == rdev->dev.of_node)
                                        goto found;
        }
        list_for_each_entry(map, &regulator_map_list, list) {
@@ -2642,9 +2642,6 @@ struct regulator_dev *regulator_register(struct regulator_desc *regulator_desc,
            regulator_desc->type != REGULATOR_CURRENT)
                return ERR_PTR(-EINVAL);

-       if (!init_data)
-               return ERR_PTR(-EINVAL);
-
        /* Only one of each should be implemented */
        WARN_ON(regulator_desc->ops->get_voltage &&
                regulator_desc->ops->get_voltage_sel);
@@ -2675,12 +2672,8 @@ struct regulator_dev *regulator_register(struct regulator_desc *regulator_desc,
        INIT_LIST_HEAD(&rdev->list);
        BLOCKING_INIT_NOTIFIER_HEAD(&rdev->notifier);

-       /* preform any regulator specific init */
-       if (init_data->regulator_init) {
-               ret = init_data->regulator_init(rdev->reg_data);
-               if (ret < 0)
-                       goto clean;
-       }
+       /* find device_node and attach it */
+       rdev->dev.of_node = of_find_node_by_name(NULL, regulator_desc->name);

        /* register with sysfs */
        rdev->dev.class = &regulator_class;
@@ -2693,6 +2686,20 @@ struct regulator_dev *regulator_register(struct regulator_desc *regulator_desc,
                goto clean;
        }

+       if (!init_data) {
+               /* try to get init_data from device tree */
+               init_data = of_get_regulator_init_data(&rdev->dev);
+               if (!init_data)
+                       return ERR_PTR(-EINVAL);
+       }
+
+       /* preform any regulator specific init */
+       if (init_data->regulator_init) {
+               ret = init_data->regulator_init(rdev->reg_data);
+               if (ret < 0)
+                       goto clean;
+       }
+
        dev_set_drvdata(&rdev->dev, rdev);

        /* set regulator constraints */
@@ -2719,7 +2726,7 @@ struct regulator_dev *regulator_register(struct regulator_desc *regulator_desc,
                        node = of_get_regulator(dev, supply);
                        if (node)
                                list_for_each_entry(r, &regulator_list, list)
-                                       if (node == r->dev.parent->of_node)
+                                       if (node == r->dev.of_node)
                                                goto found;
                }

------->8---

And my dts file looks like something below.

	ecspi at 70010000 { /* ECSPI1 */
		fsl,spi-num-chipselects = <2>;
		cs-gpios = <&gpio3 24 0>, /* GPIO4_24 */
			   <&gpio3 25 0>; /* GPIO4_25 */
		status = "okay";

		pmic: mc13892 at 0 {
			#address-cells = <1>;
			#size-cells = <0>;
			compatible = "fsl,mc13892";
			spi-max-frequency = <6000000>;
			reg = <0>;
			mc13xxx-irq-gpios = <&gpio0 8 0>; /* GPIO1_8 */

			regulators {
				sw1reg: mc13892_sw1 {
					regulator-min-uV = <600000>;
					regulator-max-uV = <1375000>;
					regulator-change-voltage;
					regulator-boot-on;
					regulator-always-on;
				};

				sw2reg: mc13892_sw2 {
					regulator-min-uV = <900000>;
					regulator-max-uV = <1850000>;
					regulator-change-voltage;
					regulator-boot-on;
					regulator-always-on;
				};

				......
			};

			leds {
				......
			};

			buttons {
				......
			};
		};

		flash: at45db321d at 1 {
			......
		};
	};
};

-- 
Regards,
Shawn

[PATCH v2 3/5] regulator: helper routine to extract regulator_init_data

[Rajendra Nayak]

>>
>> Do you have any RFC patch/code which could explain better what you are
>> suggesting we do here?
>>>
> Here is what I changed based on your patches.  It only changes
> drivers/regulator/core.c.
>
> ---8<-------
> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index 9a5ebbe..8fe132d 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -1211,7 +1211,7 @@ static struct regulator *_regulator_get(struct device *dev, const char *id,
>                  node = of_get_regulator(dev, id);
>                  if (node)
>                          list_for_each_entry(rdev,&regulator_list, list)
> -                               if (node == rdev->dev.parent->of_node)
> +                               if (node == rdev->dev.of_node)
>                                          goto found;
>          }
>          list_for_each_entry(map,&regulator_map_list, list) {
> @@ -2642,9 +2642,6 @@ struct regulator_dev *regulator_register(struct regulator_desc *regulator_desc,
>              regulator_desc->type != REGULATOR_CURRENT)
>                  return ERR_PTR(-EINVAL);
>
> -       if (!init_data)
> -               return ERR_PTR(-EINVAL);
> -
>          /* Only one of each should be implemented */
>          WARN_ON(regulator_desc->ops->get_voltage&&
>                  regulator_desc->ops->get_voltage_sel);
> @@ -2675,12 +2672,8 @@ struct regulator_dev *regulator_register(struct regulator_desc *regulator_desc,
>          INIT_LIST_HEAD(&rdev->list);
>          BLOCKING_INIT_NOTIFIER_HEAD(&rdev->notifier);
>
> -       /* preform any regulator specific init */
> -       if (init_data->regulator_init) {
> -               ret = init_data->regulator_init(rdev->reg_data);
> -               if (ret<  0)
> -                       goto clean;
> -       }
> +       /* find device_node and attach it */
> +       rdev->dev.of_node = of_find_node_by_name(NULL, regulator_desc->name);

so would this do a complete dt search for every regulator?
we would also need the driver names and dt names to match for this to
work, right?

The approach otherwise looks fine to me and should work for both cases
of one device per regulator and one device for all regulators.

>
>          /* register with sysfs */
>          rdev->dev.class =&regulator_class;
> @@ -2693,6 +2686,20 @@ struct regulator_dev *regulator_register(struct regulator_desc *regulator_desc,
>                  goto clean;
>          }
>
> +       if (!init_data) {
> +               /* try to get init_data from device tree */
> +               init_data = of_get_regulator_init_data(&rdev->dev);
> +               if (!init_data)
> +                       return ERR_PTR(-EINVAL);
> +       }
> +
> +       /* preform any regulator specific init */
> +       if (init_data->regulator_init) {
> +               ret = init_data->regulator_init(rdev->reg_data);
> +               if (ret<  0)
> +                       goto clean;
> +       }
> +
>          dev_set_drvdata(&rdev->dev, rdev);
>
>          /* set regulator constraints */
> @@ -2719,7 +2726,7 @@ struct regulator_dev *regulator_register(struct regulator_desc *regulator_desc,
>                          node = of_get_regulator(dev, supply);
>                          if (node)
>                                  list_for_each_entry(r,&regulator_list, list)
> -                                       if (node == r->dev.parent->of_node)
> +                                       if (node == r->dev.of_node)
>                                                  goto found;
>                  }
>
> ------->8---
>
> And my dts file looks like something below.
>
> 	ecspi at 70010000 { /* ECSPI1 */
> 		fsl,spi-num-chipselects =<2>;
> 		cs-gpios =<&gpio3 24 0>, /* GPIO4_24 */
> 			<&gpio3 25 0>; /* GPIO4_25 */
> 		status = "okay";
>
> 		pmic: mc13892 at 0 {
> 			#address-cells =<1>;
> 			#size-cells =<0>;
> 			compatible = "fsl,mc13892";
> 			spi-max-frequency =<6000000>;
> 			reg =<0>;
> 			mc13xxx-irq-gpios =<&gpio0 8 0>; /* GPIO1_8 */
>
> 			regulators {
> 				sw1reg: mc13892_sw1 {
> 					regulator-min-uV =<600000>;
> 					regulator-max-uV =<1375000>;
> 					regulator-change-voltage;
> 					regulator-boot-on;
> 					regulator-always-on;
> 				};
>
> 				sw2reg: mc13892_sw2 {
> 					regulator-min-uV =<900000>;
> 					regulator-max-uV =<1850000>;
> 					regulator-change-voltage;
> 					regulator-boot-on;
> 					regulator-always-on;
> 				};
>
> 				......
> 			};
>
> 			leds {
> 				......
> 			};
>
> 			buttons {
> 				......
> 			};
> 		};
>
> 		flash: at45db321d at 1 {
> 			......
> 		};
> 	};
> };
>

[PATCH v2 3/5] regulator: helper routine to extract regulator_init_data

[Shawn Guo]

On Fri, Oct 21, 2011 at 02:11:55PM +0530, Rajendra Nayak wrote:
[...]
> >+       /* find device_node and attach it */
> >+       rdev->dev.of_node = of_find_node_by_name(NULL, regulator_desc->name);
> 
> so would this do a complete dt search for every regulator?

Yes, with the first param being NULL, tthe entire device tree will be
searched.

> we would also need the driver names and dt names to match for this to
> work, right?
> 
Driver name does not matter.  The key for this search to work is having
regulator's name (regulator_desc->name) match device tree node's name,
case ignored.

> The approach otherwise looks fine to me and should work for both cases
> of one device per regulator and one device for all regulators.
> 

-- 
Regards,
Shawn

[PATCH v2 3/5] regulator: helper routine to extract regulator_init_data

[Rajendra Nayak]

On Friday 21 October 2011 05:28 PM, Shawn Guo wrote:
> On Fri, Oct 21, 2011 at 02:11:55PM +0530, Rajendra Nayak wrote:
> [...]
>>> +       /* find device_node and attach it */
>>> +       rdev->dev.of_node = of_find_node_by_name(NULL, regulator_desc->name);
>>
>> so would this do a complete dt search for every regulator?
>
> Yes, with the first param being NULL, tthe entire device tree will be
> searched.
>
>> we would also need the driver names and dt names to match for this to
>> work, right?
>>
> Driver name does not matter.  The key for this search to work is having
> regulator's name (regulator_desc->name) match device tree node's name,
> case ignored.

Mark, whats your take on this? I am somehow not quite sure if we should
have this limitation put in to match DT node names with whats in the
driver structs (regulator_desc).

>
>> The approach otherwise looks fine to me and should work for both cases
>> of one device per regulator and one device for all regulators.
>>
>

[PATCH v2 3/5] regulator: helper routine to extract regulator_init_data

[Mark Brown]

On Mon, Oct 24, 2011 at 11:32:19AM +0530, Rajendra Nayak wrote:
> On Friday 21 October 2011 05:28 PM, Shawn Guo wrote:

> >Driver name does not matter.  The key for this search to work is having
> >regulator's name (regulator_desc->name) match device tree node's name,
> >case ignored.

> Mark, whats your take on this? I am somehow not quite sure if we should
> have this limitation put in to match DT node names with whats in the
> driver structs (regulator_desc).

If we're matching the string in the regulator_desc I'd expect we're
going to run into trouble with systems that have two of the same type of
regulator in the system.  Is that the case?  Since my primary
development system is such a system I care deeply about them :)

[PATCH v2 3/5] regulator: helper routine to extract regulator_init_data

[Grant Likely]

On Mon, Oct 24, 2011 at 11:32:19AM +0530, Rajendra Nayak wrote:
> On Friday 21 October 2011 05:28 PM, Shawn Guo wrote:
> >On Fri, Oct 21, 2011 at 02:11:55PM +0530, Rajendra Nayak wrote:
> >[...]
> >>>+       /* find device_node and attach it */
> >>>+       rdev->dev.of_node = of_find_node_by_name(NULL, regulator_desc->name);
> >>
> >>so would this do a complete dt search for every regulator?
> >
> >Yes, with the first param being NULL, tthe entire device tree will be
> >searched.
> >
> >>we would also need the driver names and dt names to match for this to
> >>work, right?
> >>
> >Driver name does not matter.  The key for this search to work is having
> >regulator's name (regulator_desc->name) match device tree node's name,
> >case ignored.
> 
> Mark, whats your take on this? I am somehow not quite sure if we should
> have this limitation put in to match DT node names with whats in the
> driver structs (regulator_desc).

This looks wrong to me.  Matching based on node /name/, particularly
when searching the entire tree, will cause problems.

g.

[PATCH v2 3/5] regulator: helper routine to extract regulator_init_data

[Rajendra Nayak]

On Monday 24 October 2011 01:47 PM, Grant Likely wrote:
> On Mon, Oct 24, 2011 at 11:32:19AM +0530, Rajendra Nayak wrote:
>> On Friday 21 October 2011 05:28 PM, Shawn Guo wrote:
>>> On Fri, Oct 21, 2011 at 02:11:55PM +0530, Rajendra Nayak wrote:
>>> [...]
>>>>> +       /* find device_node and attach it */
>>>>> +       rdev->dev.of_node = of_find_node_by_name(NULL, regulator_desc->name);

Shawn/Mark,

How about the driver passing the of_node to be associated with the
regulator, as part of regulator_desc, to the regulator core,
instead of this complete DT search and the restriction of having
to match DT node names with names in regulator_desc.

regards,
Rajendra

>>>>
>>>> so would this do a complete dt search for every regulator?
>>>
>>> Yes, with the first param being NULL, tthe entire device tree will be
>>> searched.
>>>
>>>> we would also need the driver names and dt names to match for this to
>>>> work, right?
>>>>
>>> Driver name does not matter.  The key for this search to work is having
>>> regulator's name (regulator_desc->name) match device tree node's name,
>>> case ignored.
>>
>> Mark, whats your take on this? I am somehow not quite sure if we should
>> have this limitation put in to match DT node names with whats in the
>> driver structs (regulator_desc).
>
> This looks wrong to me.  Matching based on node /name/, particularly
> when searching the entire tree, will cause problems.
>
> g.

[PATCH v2 3/5] regulator: helper routine to extract regulator_init_data

[Mark Brown]

On Mon, Oct 24, 2011 at 02:23:50PM +0530, Rajendra Nayak wrote:

> How about the driver passing the of_node to be associated with the
> regulator, as part of regulator_desc, to the regulator core,
> instead of this complete DT search and the restriction of having
> to match DT node names with names in regulator_desc.

regulator_desc should be const so that we handle multiple instances of
the same device nicely.  We could pass the information in as an argument
I guess.

[PATCH v2 3/5] regulator: helper routine to extract regulator_init_data

[Rajendra Nayak]

On Monday 24 October 2011 02:49 PM, Mark Brown wrote:
> On Mon, Oct 24, 2011 at 02:23:50PM +0530, Rajendra Nayak wrote:
>
>> How about the driver passing the of_node to be associated with the
>> regulator, as part of regulator_desc, to the regulator core,
>> instead of this complete DT search and the restriction of having
>> to match DT node names with names in regulator_desc.
>
> regulator_desc should be const so that we handle multiple instances of
> the same device nicely.  We could pass the information in as an argument
> I guess.

Ok, will do. The only downside being that all existing users would
need to be changed.

[PATCH v2 3/5] regulator: helper routine to extract regulator_init_data

[Shawn Guo]

On Mon, Oct 24, 2011 at 02:23:50PM +0530, Rajendra Nayak wrote:
> On Monday 24 October 2011 01:47 PM, Grant Likely wrote:
> >On Mon, Oct 24, 2011 at 11:32:19AM +0530, Rajendra Nayak wrote:
> >>On Friday 21 October 2011 05:28 PM, Shawn Guo wrote:
> >>>On Fri, Oct 21, 2011 at 02:11:55PM +0530, Rajendra Nayak wrote:
> >>>[...]
> >>>>>+       /* find device_node and attach it */
> >>>>>+       rdev->dev.of_node = of_find_node_by_name(NULL, regulator_desc->name);
> 
> Shawn/Mark,
> 
> How about the driver passing the of_node to be associated with the
> regulator, as part of regulator_desc, to the regulator core,
> instead of this complete DT search and the restriction of having
> to match DT node names with names in regulator_desc.
> 
If you agree that we should try to get DT core from creating
'struct dev' for each regulator, the of_node is not being attached
to any 'struct dev' in regulator driver either, so there is no
difference between finding the of_node in regulator driver and in
regulator core.

-- 
Regards,
Shawn

[PATCH v2 3/5] regulator: helper routine to extract regulator_init_data

[Shawn Guo]

On Mon, Oct 24, 2011 at 10:17:06AM +0200, Grant Likely wrote:
> On Mon, Oct 24, 2011 at 11:32:19AM +0530, Rajendra Nayak wrote:
> > On Friday 21 October 2011 05:28 PM, Shawn Guo wrote:
> > >On Fri, Oct 21, 2011 at 02:11:55PM +0530, Rajendra Nayak wrote:
> > >[...]
> > >>>+       /* find device_node and attach it */
> > >>>+       rdev->dev.of_node = of_find_node_by_name(NULL, regulator_desc->name);
> > >>
> > >>so would this do a complete dt search for every regulator?
> > >
> > >Yes, with the first param being NULL, tthe entire device tree will be
> > >searched.
> > >
> > >>we would also need the driver names and dt names to match for this to
> > >>work, right?
> > >>
> > >Driver name does not matter.  The key for this search to work is having
> > >regulator's name (regulator_desc->name) match device tree node's name,
> > >case ignored.
> > 
> > Mark, whats your take on this? I am somehow not quite sure if we should
> > have this limitation put in to match DT node names with whats in the
> > driver structs (regulator_desc).
> 
> This looks wrong to me.  Matching based on node /name/, particularly
> when searching the entire tree, will cause problems.
> 
Okay, it's wrong then since so many people say it's wrong :)  I guess
a quick fix would be adding one property in device tree node for
matching some unique field in regulator_desc, id, maybe?  Mark, any
suggestion?

-- 
Regards,
Shawn

[PATCH v2 3/5] regulator: helper routine to extract regulator_init_data

[Rajendra Nayak]

On Monday 24 October 2011 02:32 PM, Shawn Guo wrote:
> On Mon, Oct 24, 2011 at 10:17:06AM +0200, Grant Likely wrote:
>> On Mon, Oct 24, 2011 at 11:32:19AM +0530, Rajendra Nayak wrote:
>>> On Friday 21 October 2011 05:28 PM, Shawn Guo wrote:
>>>> On Fri, Oct 21, 2011 at 02:11:55PM +0530, Rajendra Nayak wrote:
>>>> [...]
>>>>>> +       /* find device_node and attach it */
>>>>>> +       rdev->dev.of_node = of_find_node_by_name(NULL, regulator_desc->name);
>>>>>
>>>>> so would this do a complete dt search for every regulator?
>>>>
>>>> Yes, with the first param being NULL, tthe entire device tree will be
>>>> searched.
>>>>
>>>>> we would also need the driver names and dt names to match for this to
>>>>> work, right?
>>>>>
>>>> Driver name does not matter.  The key for this search to work is having
>>>> regulator's name (regulator_desc->name) match device tree node's name,
>>>> case ignored.
>>>
>>> Mark, whats your take on this? I am somehow not quite sure if we should
>>> have this limitation put in to match DT node names with whats in the
>>> driver structs (regulator_desc).
>>
>> This looks wrong to me.  Matching based on node /name/, particularly
>> when searching the entire tree, will cause problems.
>>
> Okay, it's wrong then since so many people say it's wrong :)  I guess
> a quick fix would be adding one property in device tree node for
> matching some unique field in regulator_desc, id, maybe?  Mark, any
> suggestion?

Thats basically what the DT compatible property is for :)
>

[PATCH v2 3/5] regulator: helper routine to extract regulator_init_data

[Shawn Guo]

On Mon, Oct 24, 2011 at 02:26:49PM +0530, Rajendra Nayak wrote:
> On Monday 24 October 2011 02:32 PM, Shawn Guo wrote:
> >Okay, it's wrong then since so many people say it's wrong :)  I guess
> >a quick fix would be adding one property in device tree node for
> >matching some unique field in regulator_desc, id, maybe?  Mark, any
> >suggestion?
> 
> Thats basically what the DT compatible property is for :)

But adding compatible property will get DT core create 'struct dev'
for each regulator node, while we are trying to avoid that since
regulator core has been doing this.

-- 
Regards,
Shawn

[PATCH v2 3/5] regulator: helper routine to extract regulator_init_data

[Rajendra Nayak]

On Monday 24 October 2011 02:41 PM, Shawn Guo wrote:
> On Mon, Oct 24, 2011 at 02:26:49PM +0530, Rajendra Nayak wrote:
>> On Monday 24 October 2011 02:32 PM, Shawn Guo wrote:
>>> Okay, it's wrong then since so many people say it's wrong :)  I guess
>>> a quick fix would be adding one property in device tree node for
>>> matching some unique field in regulator_desc, id, maybe?  Mark, any
>>> suggestion?
>>
>> Thats basically what the DT compatible property is for :)
>
> But adding compatible property will get DT core create 'struct dev'
> for each regulator node, while we are trying to avoid that since
> regulator core has been doing this.
>
No it won't.

So here's what I am suggesting.

Case 1:
One device per regulator:

DT nodes look something like this

reg1: reg at 1 {
		...
		...
};

reg2: reg at 2 {
		...
		...
};

The regulator driver probes 2 devices and each time the
dev passed has a of_node associated which the regulator
driver can then pass to the core.

Case 2:
One device for all regulators:

DT nodes look something like this

regulators {
	reg1: reg at 1 {
			...
			...
	};

	reg2: reg at 2 {
			...
			...
	};
};

The regulator driver probes only one device and the dev->of_node
points to the "regulators" node above.
The regulator driver then based on compatible property extracts
and registers all the child nodes of "regulators" (for ex: reg1, reg2
above) with each call to regulator_register and passes the child nodes
as of_node to be associated with the regulator device.

[PATCH v2 3/5] regulator: helper routine to extract regulator_init_data

[Shawn Guo]

On Mon, Oct 24, 2011 at 02:43:58PM +0530, Rajendra Nayak wrote:
> Case 2:
> One device for all regulators:
> 
> DT nodes look something like this
> 
> regulators {
> 	reg1: reg at 1 {
> 			...
> 			...
> 	};
> 
> 	reg2: reg at 2 {
> 			...
> 			...
> 	};
> };
> 
> The regulator driver probes only one device and the dev->of_node
> points to the "regulators" node above.

The mc13892 example I put in the reply to Grant demonstrates that
for some case, dev->of_node is NULL (devices are created by mfd core).

> The regulator driver then based on compatible property extracts
> and registers all the child nodes of "regulators" (for ex: reg1, reg2
> above) with each call to regulator_register and passes the child nodes
> as of_node to be associated with the regulator device.
> 
I still think the discovery of all the child nodes of "regulators" does
not necessarily need to be done in regulator driver.  Instead, it can
be done in regulator core.

-- 
Regards,
Shawn

[PATCH v2 3/5] regulator: helper routine to extract regulator_init_data

[Rajendra Nayak]

On Monday 24 October 2011 07:17 PM, Shawn Guo wrote:
> On Mon, Oct 24, 2011 at 02:43:58PM +0530, Rajendra Nayak wrote:
>> Case 2:
>> One device for all regulators:
>>
>> DT nodes look something like this
>>
>> regulators {
>> 	reg1: reg at 1 {
>> 			...
>> 			...
>> 	};
>>
>> 	reg2: reg at 2 {
>> 			...
>> 			...
>> 	};
>> };
>>
>> The regulator driver probes only one device and the dev->of_node
>> points to the "regulators" node above.
>
> The mc13892 example I put in the reply to Grant demonstrates that
> for some case, dev->of_node is NULL (devices are created by mfd core).

In that case should you not be first converting the mfd driver to
register regulator devices using DT?
Thats what we did for OMAP, and hence we always have the of_node
populated when the regulator devices are probed.
See this patch from Benoit on how thats done for twl devices..
http://marc.info/?l=linux-omap&m=131489864814428&w=2

>
>> The regulator driver then based on compatible property extracts
>> and registers all the child nodes of "regulators" (for ex: reg1, reg2
>> above) with each call to regulator_register and passes the child nodes
>> as of_node to be associated with the regulator device.
>>
> I still think the discovery of all the child nodes of "regulators" does
> not necessarily need to be done in regulator driver.  Instead, it can
> be done in regulator core.
>

[PATCH v2 3/5] regulator: helper routine to extract regulator_init_data

[Rajendra Nayak]

On Tuesday 25 October 2011 11:30 AM, Rajendra Nayak wrote:
>
> In that case should you not be first converting the mfd driver to
> register regulator devices using DT?
> Thats what we did for OMAP, and hence we always have the of_node
> populated when the regulator devices are probed.
> See this patch from Benoit on how thats done for twl devices..
> http://marc.info/?l=linux-omap&m=131489864814428&w=2

Sorry, that was a link to an older version of the patch.
Here's the latest one from Benoit which he did based
on Arnd's suggestion of using of_platform_populate() directly.

http://marc.info/?l=linux-omap&m=131705623822369&w=2

[PATCH v2 3/5] regulator: helper routine to extract regulator_init_data

[Shawn Guo]

On Tue, Oct 25, 2011 at 11:30:19AM +0530, Rajendra Nayak wrote:
> On Monday 24 October 2011 07:17 PM, Shawn Guo wrote:
> >On Mon, Oct 24, 2011 at 02:43:58PM +0530, Rajendra Nayak wrote:
> >>Case 2:
> >>One device for all regulators:
> >>
> >>DT nodes look something like this
> >>
> >>regulators {
> >>	reg1: reg at 1 {
> >>			...
> >>			...
> >>	};
> >>
> >>	reg2: reg at 2 {
> >>			...
> >>			...
> >>	};
> >>};
> >>
> >>The regulator driver probes only one device and the dev->of_node
> >>points to the "regulators" node above.
> >
> >The mc13892 example I put in the reply to Grant demonstrates that
> >for some case, dev->of_node is NULL (devices are created by mfd core).
> 
> In that case should you not be first converting the mfd driver to
> register regulator devices using DT?

The mc13892 mfd driver calls mfd_add_devices() to add device for
mc13892 regulator driver.  Are you suggesting that I should hack
mfd_add_devices() to have device_node of 'regulators' attached?
The mfd is not a bus like i2c and spi, so I'm not sure this is the
right thing to do.

> Thats what we did for OMAP, and hence we always have the of_node
> populated when the regulator devices are probed.
> See this patch from Benoit on how thats done for twl devices..
> http://marc.info/?l=linux-omap&m=131489864814428&w=2
> 
OMAP is "Case 1", and we are talking about "Case 2".

-- 
Regards,
Shawn

[PATCH v2 3/5] regulator: helper routine to extract regulator_init_data

[Rajendra Nayak]

On Tuesday 25 October 2011 12:22 PM, Shawn Guo wrote:
> On Tue, Oct 25, 2011 at 11:30:19AM +0530, Rajendra Nayak wrote:
>> On Monday 24 October 2011 07:17 PM, Shawn Guo wrote:
>>> On Mon, Oct 24, 2011 at 02:43:58PM +0530, Rajendra Nayak wrote:
>>>> Case 2:
>>>> One device for all regulators:
>>>>
>>>> DT nodes look something like this
>>>>
>>>> regulators {
>>>> 	reg1: reg at 1 {
>>>> 			...
>>>> 			...
>>>> 	};
>>>>
>>>> 	reg2: reg at 2 {
>>>> 			...
>>>> 			...
>>>> 	};
>>>> };
>>>>
>>>> The regulator driver probes only one device and the dev->of_node
>>>> points to the "regulators" node above.
>>>
>>> The mc13892 example I put in the reply to Grant demonstrates that
>>> for some case, dev->of_node is NULL (devices are created by mfd core).
>>
>> In that case should you not be first converting the mfd driver to
>> register regulator devices using DT?
>
> The mc13892 mfd driver calls mfd_add_devices() to add device for
> mc13892 regulator driver.  Are you suggesting that I should hack
> mfd_add_devices() to have device_node of 'regulators' attached?
> The mfd is not a bus like i2c and spi, so I'm not sure this is the
> right thing to do.
>
>> Thats what we did for OMAP, and hence we always have the of_node
>> populated when the regulator devices are probed.
>> See this patch from Benoit on how thats done for twl devices..
>> http://marc.info/?l=linux-omap&m=131489864814428&w=2
>>
> OMAP is "Case 1", and we are talking about "Case 2".

I don't see why it wouldn't work for "Case 2". The only difference
is in case of "Case 1", the dev->of_node would already point to
the right regulator node, like 'reg1', 'reg2' above.
In case of "Case 2", the dev->of_node would point to the 'regulators'
node instead, and the driver could then do a for_each_child_of_node()
to iterate over all its children to get 'reg1', 'reg2' etc.

>

[PATCH v2 3/5] regulator: helper routine to extract regulator_init_data

[Shawn Guo]

On Tue, Oct 25, 2011 at 12:26:01PM +0530, Rajendra Nayak wrote:
> On Tuesday 25 October 2011 12:22 PM, Shawn Guo wrote:
> >On Tue, Oct 25, 2011 at 11:30:19AM +0530, Rajendra Nayak wrote:
[...]
> >>Thats what we did for OMAP, and hence we always have the of_node
> >>populated when the regulator devices are probed.
> >>See this patch from Benoit on how thats done for twl devices..
> >>http://marc.info/?l=linux-omap&m=131489864814428&w=2
> >>
> >OMAP is "Case 1", and we are talking about "Case 2".
> 
> I don't see why it wouldn't work for "Case 2".

I did not say it wouldn't work for "Case 2".  I meant they work in
different way.

> The only difference
> is in case of "Case 1", the dev->of_node would already point to
> the right regulator node, like 'reg1', 'reg2' above.
> In case of "Case 2", the dev->of_node would point to the 'regulators'
> node instead, and the driver could then do a for_each_child_of_node()
> to iterate over all its children to get 'reg1', 'reg2' etc.
> 
Yes, that's the difference.  So you will need to distinguish these
two cases in regulator_register()?  How?

-- 
Regards,
Shawn

[PATCH v2 3/5] regulator: helper routine to extract regulator_init_data

[Rajendra Nayak]

On Tuesday 25 October 2011 12:50 PM, Shawn Guo wrote:
> On Tue, Oct 25, 2011 at 12:26:01PM +0530, Rajendra Nayak wrote:
>> On Tuesday 25 October 2011 12:22 PM, Shawn Guo wrote:
>>> On Tue, Oct 25, 2011 at 11:30:19AM +0530, Rajendra Nayak wrote:
> [...]
>>>> Thats what we did for OMAP, and hence we always have the of_node
>>>> populated when the regulator devices are probed.
>>>> See this patch from Benoit on how thats done for twl devices..
>>>> http://marc.info/?l=linux-omap&m=131489864814428&w=2
>>>>
>>> OMAP is "Case 1", and we are talking about "Case 2".
>>
>> I don't see why it wouldn't work for "Case 2".
>
> I did not say it wouldn't work for "Case 2".  I meant they work in
> different way.
>
>> The only difference
>> is in case of "Case 1", the dev->of_node would already point to
>> the right regulator node, like 'reg1', 'reg2' above.
>> In case of "Case 2", the dev->of_node would point to the 'regulators'
>> node instead, and the driver could then do a for_each_child_of_node()
>> to iterate over all its children to get 'reg1', 'reg2' etc.
>>
> Yes, that's the difference.  So you will need to distinguish these
> two cases in regulator_register()?  How?

You don't need to distinguish these cases in regulator_register().
The driver knows if its handling "Case 1" devices or "Case 2" devices
and it needs to handle things accordingly and pass the right of_node
which regulator_register() can then attach to regulator_dev->dev.

>

[PATCH v2 3/5] regulator: helper routine to extract regulator_init_data

[Shawn Guo]

On Tue, Oct 25, 2011 at 12:43:55PM +0530, Rajendra Nayak wrote:
> On Tuesday 25 October 2011 12:50 PM, Shawn Guo wrote:
> >On Tue, Oct 25, 2011 at 12:26:01PM +0530, Rajendra Nayak wrote:
> >>On Tuesday 25 October 2011 12:22 PM, Shawn Guo wrote:
> >>>On Tue, Oct 25, 2011 at 11:30:19AM +0530, Rajendra Nayak wrote:
> >[...]
> >>>>Thats what we did for OMAP, and hence we always have the of_node
> >>>>populated when the regulator devices are probed.
> >>>>See this patch from Benoit on how thats done for twl devices..
> >>>>http://marc.info/?l=linux-omap&m=131489864814428&w=2
> >>>>
> >>>OMAP is "Case 1", and we are talking about "Case 2".
> >>
> >>I don't see why it wouldn't work for "Case 2".
> >
> >I did not say it wouldn't work for "Case 2".  I meant they work in
> >different way.
> >
> >>The only difference
> >>is in case of "Case 1", the dev->of_node would already point to
> >>the right regulator node, like 'reg1', 'reg2' above.
> >>In case of "Case 2", the dev->of_node would point to the 'regulators'
> >>node instead, and the driver could then do a for_each_child_of_node()
> >>to iterate over all its children to get 'reg1', 'reg2' etc.
> >>
> >Yes, that's the difference.  So you will need to distinguish these
> >two cases in regulator_register()?  How?
> 
> You don't need to distinguish these cases in regulator_register().
> The driver knows if its handling "Case 1" devices or "Case 2" devices
> and it needs to handle things accordingly and pass the right of_node
> which regulator_register() can then attach to regulator_dev->dev.
> 
Ah, you are thinking about (for Case 2) calling for_each_child_of_node()
in regulator driver and passing the of_node found for each regulator to
regulator core as a new parameter of regulator_register().  Yes, that
would work, though my thought is to put the device_node discovery in
regulator core and hide it from regulator driver.

Okay, I'm good with this solution.  Grant, Mark, what about you?

-- 
Regards,
Shawn

[PATCH v2 3/5] regulator: helper routine to extract regulator_init_data

[Grant Likely]

On Mon, Oct 24, 2011 at 05:11:58PM +0800, Shawn Guo wrote:
> On Mon, Oct 24, 2011 at 02:26:49PM +0530, Rajendra Nayak wrote:
> > On Monday 24 October 2011 02:32 PM, Shawn Guo wrote:
> > >Okay, it's wrong then since so many people say it's wrong :)  I guess
> > >a quick fix would be adding one property in device tree node for
> > >matching some unique field in regulator_desc, id, maybe?  Mark, any
> > >suggestion?
> > 
> > Thats basically what the DT compatible property is for :)
> 
> But adding compatible property will get DT core create 'struct dev'
> for each regulator node, while we are trying to avoid that since
> regulator core has been doing this.

By design, of_platform_populate() will only go one level deep except
if it matches the passed in list of bus compatible properties.  Having
a compatible property does not necessarily create a struct dev.

That said, I agree with you that a compatible property isn't really
needed in the child nodes **if it is defined as part of the parent
node's binding**.

g.

[PATCH v2 3/5] regulator: helper routine to extract regulator_init_data

[Grant Likely]

On Fri, Oct 21, 2011 at 04:23:12PM +0800, Shawn Guo wrote:
> On Thu, Oct 20, 2011 at 05:39:32PM +0530, Rajendra Nayak wrote:
> > On Thursday 20 October 2011 11:44 AM, Shawn Guo wrote:
> > >On Thu, Oct 20, 2011 at 10:48:58AM +0530, Rajendra Nayak wrote:
> > >>>Let's look at mc13892-regulator driver.  There are 23 regulators defined
> > >>>in array mc13892_regulators.  Needless to say, there is a dev behind
> > >>>mc13892-regulator driver.  And when getting probed, this driver will
> > >>>call regulator_register() to register those 23 regulators individually.
> > >>>That said, for non-dt world, we have 1 + 23 'dev' with that 1 as the
> > >>>parent of all other 23 'dev' (wrapped by regulator_dev).  But with the
> > >>>current DT implementation, we will have at least 1 + 23 * 2 'dev'.
> > >>>These extra 23 'dev' is totally new with DT.
> > >>>
> > >>
> > >>but thats only because the mc13892-regulator driver is implemeted in
> > >>such a way that all the regulators on the platform are bundled in as
> > >>*one* device.
> > >
> > >I did not look into too many regulator drivers, but I expect this is
> > >way that most regulator drivers are implemented in.  Having
> > >mc13892-regulator being probed 23 times to register these 23 regulators
> > >just makes less sense to me.
> > >
> > >>It would again depend on how you would pass these from
> > >>the DT, if you indeed stick to the same way of bundling all regulators
> > >>as one device from DT, the mc13892-regulator probe would just get called
> > >>once and there would be one device associated, no?
> > >>
> > >Yes, I indeed would stick to the same way of bundling the registration
> > >of all regulators with mc13892-regulator being probed once.  The problem
> > >I have with the current regulator core DT implementation is that it
> > >assumes the device_node of rdev->dev (dev wrapped in regulator_dev) is
> > >being attached to rdev->dev.parent rather than itself.  Back to
> > >mc13892-regulator example, that said, it requires the dev of
> > >mc13892-regulator have the device_node of individual regulator attached
> > >to.  IOW, the current implementation forces mc13892-regulator to be
> > >probed 23 times to register those 23 regulators.  This is wrong to me.
> > 
> > I think I now understand to some extent the problem that you seem to be
> > reporting. It is mainly with drivers which bundle all regulators and
> > pass them as one device and would want to do so with DT too.
> > 
> > however I am still not clear on how what you seem to suggest would
> > solve this problem. Note that not all drivers do it this way, and
> > there are drivers where each regulator is considered as one device
> > and I suspect they would remain that way with DT too. And hence we
> > need to support both.
> > 
> > Do you have any RFC patch/code which could explain better what you are
> > suggesting we do here?
> > >
> Here is what I changed based on your patches.  It only changes
> drivers/regulator/core.c.
> 
> ---8<-------
> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index 9a5ebbe..8fe132d 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -1211,7 +1211,7 @@ static struct regulator *_regulator_get(struct device *dev, const char *id,
>                 node = of_get_regulator(dev, id);
>                 if (node)
>                         list_for_each_entry(rdev, &regulator_list, list)
> -                               if (node == rdev->dev.parent->of_node)
> +                               if (node == rdev->dev.of_node)
>                                         goto found;
>         }
>         list_for_each_entry(map, &regulator_map_list, list) {
> @@ -2642,9 +2642,6 @@ struct regulator_dev *regulator_register(struct regulator_desc *regulator_desc,
>             regulator_desc->type != REGULATOR_CURRENT)
>                 return ERR_PTR(-EINVAL);
> 
> -       if (!init_data)
> -               return ERR_PTR(-EINVAL);
> -
>         /* Only one of each should be implemented */
>         WARN_ON(regulator_desc->ops->get_voltage &&
>                 regulator_desc->ops->get_voltage_sel);
> @@ -2675,12 +2672,8 @@ struct regulator_dev *regulator_register(struct regulator_desc *regulator_desc,
>         INIT_LIST_HEAD(&rdev->list);
>         BLOCKING_INIT_NOTIFIER_HEAD(&rdev->notifier);
> 
> -       /* preform any regulator specific init */
> -       if (init_data->regulator_init) {
> -               ret = init_data->regulator_init(rdev->reg_data);
> -               if (ret < 0)
> -                       goto clean;
> -       }
> +       /* find device_node and attach it */
> +       rdev->dev.of_node = of_find_node_by_name(NULL, regulator_desc->name);
> 
>         /* register with sysfs */
>         rdev->dev.class = &regulator_class;
> @@ -2693,6 +2686,20 @@ struct regulator_dev *regulator_register(struct regulator_desc *regulator_desc,
>                 goto clean;
>         }
> 
> +       if (!init_data) {
> +               /* try to get init_data from device tree */
> +               init_data = of_get_regulator_init_data(&rdev->dev);
> +               if (!init_data)
> +                       return ERR_PTR(-EINVAL);
> +       }
> +
> +       /* preform any regulator specific init */
> +       if (init_data->regulator_init) {
> +               ret = init_data->regulator_init(rdev->reg_data);
> +               if (ret < 0)
> +                       goto clean;
> +       }
> +
>         dev_set_drvdata(&rdev->dev, rdev);
> 
>         /* set regulator constraints */
> @@ -2719,7 +2726,7 @@ struct regulator_dev *regulator_register(struct regulator_desc *regulator_desc,
>                         node = of_get_regulator(dev, supply);
>                         if (node)
>                                 list_for_each_entry(r, &regulator_list, list)
> -                                       if (node == r->dev.parent->of_node)
> +                                       if (node == r->dev.of_node)
>                                                 goto found;
>                 }
> 
> ------->8---
> 
> And my dts file looks like something below.
> 
> 	ecspi at 70010000 { /* ECSPI1 */
> 		fsl,spi-num-chipselects = <2>;
> 		cs-gpios = <&gpio3 24 0>, /* GPIO4_24 */
> 			   <&gpio3 25 0>; /* GPIO4_25 */
> 		status = "okay";
> 
> 		pmic: mc13892 at 0 {
> 			#address-cells = <1>;
> 			#size-cells = <0>;
> 			compatible = "fsl,mc13892";
> 			spi-max-frequency = <6000000>;
> 			reg = <0>;
> 			mc13xxx-irq-gpios = <&gpio0 8 0>; /* GPIO1_8 */
> 
> 			regulators {
> 				sw1reg: mc13892_sw1 {
> 					regulator-min-uV = <600000>;
> 					regulator-max-uV = <1375000>;
> 					regulator-change-voltage;
> 					regulator-boot-on;
> 					regulator-always-on;
> 				};
> 
> 				sw2reg: mc13892_sw2 {
> 					regulator-min-uV = <900000>;
> 					regulator-max-uV = <1850000>;
> 					regulator-change-voltage;
> 					regulator-boot-on;
> 					regulator-always-on;
> 				};
> 
> 				......
> 			};

To follow up from my earlier comment, this .dts structure looks fine
and reasonable to me, and it would also be fine for the mc13892 driver
to use for_each_child_of_node() to find all the children of the
regulators node.  Even finding the 'regulators' node by name from the
mc13892 driver is perfectly fine provided for_each_child_of_node is
used to find it.  All of this is okay because it is under the umbrella
of the "fsl,mc13892" binding.

What isn't okay is doing a whole tree search for nodes by name because
there is a high likelyhood of getting a conflict (and yes, I realized
that this example has the device name encoded into the node name, but
that doesn't change the fact that deep searches by name are bad
practice).

g.

[PATCH v2 3/5] regulator: helper routine to extract regulator_init_data

[Mark Brown]

On Mon, Oct 24, 2011 at 11:24:11AM +0200, Grant Likely wrote:

> To follow up from my earlier comment, this .dts structure looks fine
> and reasonable to me, and it would also be fine for the mc13892 driver
> to use for_each_child_of_node() to find all the children of the
> regulators node.  Even finding the 'regulators' node by name from the
> mc13892 driver is perfectly fine provided for_each_child_of_node is
> used to find it.  All of this is okay because it is under the umbrella
> of the "fsl,mc13892" binding.

Yes, a search of children of the device node that the driver is probing
off seems like a sensible approach.

[PATCH v2 3/5] regulator: helper routine to extract regulator_init_data

[Shawn Guo]

On Mon, Oct 24, 2011 at 11:24:11AM +0200, Grant Likely wrote:
> On Fri, Oct 21, 2011 at 04:23:12PM +0800, Shawn Guo wrote:
> > On Thu, Oct 20, 2011 at 05:39:32PM +0530, Rajendra Nayak wrote:
> > > On Thursday 20 October 2011 11:44 AM, Shawn Guo wrote:
> > > >On Thu, Oct 20, 2011 at 10:48:58AM +0530, Rajendra Nayak wrote:
> > > >>>Let's look at mc13892-regulator driver.  There are 23 regulators defined
> > > >>>in array mc13892_regulators.  Needless to say, there is a dev behind
> > > >>>mc13892-regulator driver.  And when getting probed, this driver will
> > > >>>call regulator_register() to register those 23 regulators individually.
> > > >>>That said, for non-dt world, we have 1 + 23 'dev' with that 1 as the
> > > >>>parent of all other 23 'dev' (wrapped by regulator_dev).  But with the
> > > >>>current DT implementation, we will have at least 1 + 23 * 2 'dev'.
> > > >>>These extra 23 'dev' is totally new with DT.
> > > >>>
> > > >>
> > > >>but thats only because the mc13892-regulator driver is implemeted in
> > > >>such a way that all the regulators on the platform are bundled in as
> > > >>*one* device.
> > > >
> > > >I did not look into too many regulator drivers, but I expect this is
> > > >way that most regulator drivers are implemented in.  Having
> > > >mc13892-regulator being probed 23 times to register these 23 regulators
> > > >just makes less sense to me.
> > > >
> > > >>It would again depend on how you would pass these from
> > > >>the DT, if you indeed stick to the same way of bundling all regulators
> > > >>as one device from DT, the mc13892-regulator probe would just get called
> > > >>once and there would be one device associated, no?
> > > >>
> > > >Yes, I indeed would stick to the same way of bundling the registration
> > > >of all regulators with mc13892-regulator being probed once.  The problem
> > > >I have with the current regulator core DT implementation is that it
> > > >assumes the device_node of rdev->dev (dev wrapped in regulator_dev) is
> > > >being attached to rdev->dev.parent rather than itself.  Back to
> > > >mc13892-regulator example, that said, it requires the dev of
> > > >mc13892-regulator have the device_node of individual regulator attached
> > > >to.  IOW, the current implementation forces mc13892-regulator to be
> > > >probed 23 times to register those 23 regulators.  This is wrong to me.
> > > 
> > > I think I now understand to some extent the problem that you seem to be
> > > reporting. It is mainly with drivers which bundle all regulators and
> > > pass them as one device and would want to do so with DT too.
> > > 
> > > however I am still not clear on how what you seem to suggest would
> > > solve this problem. Note that not all drivers do it this way, and
> > > there are drivers where each regulator is considered as one device
> > > and I suspect they would remain that way with DT too. And hence we
> > > need to support both.
> > > 
> > > Do you have any RFC patch/code which could explain better what you are
> > > suggesting we do here?
> > > >
> > Here is what I changed based on your patches.  It only changes
> > drivers/regulator/core.c.
> > 
> > ---8<-------
> > diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> > index 9a5ebbe..8fe132d 100644
> > --- a/drivers/regulator/core.c
> > +++ b/drivers/regulator/core.c
> > @@ -1211,7 +1211,7 @@ static struct regulator *_regulator_get(struct device *dev, const char *id,
> >                 node = of_get_regulator(dev, id);
> >                 if (node)
> >                         list_for_each_entry(rdev, &regulator_list, list)
> > -                               if (node == rdev->dev.parent->of_node)
> > +                               if (node == rdev->dev.of_node)
> >                                         goto found;
> >         }
> >         list_for_each_entry(map, &regulator_map_list, list) {
> > @@ -2642,9 +2642,6 @@ struct regulator_dev *regulator_register(struct regulator_desc *regulator_desc,
> >             regulator_desc->type != REGULATOR_CURRENT)
> >                 return ERR_PTR(-EINVAL);
> > 
> > -       if (!init_data)
> > -               return ERR_PTR(-EINVAL);
> > -
> >         /* Only one of each should be implemented */
> >         WARN_ON(regulator_desc->ops->get_voltage &&
> >                 regulator_desc->ops->get_voltage_sel);
> > @@ -2675,12 +2672,8 @@ struct regulator_dev *regulator_register(struct regulator_desc *regulator_desc,
> >         INIT_LIST_HEAD(&rdev->list);
> >         BLOCKING_INIT_NOTIFIER_HEAD(&rdev->notifier);
> > 
> > -       /* preform any regulator specific init */
> > -       if (init_data->regulator_init) {
> > -               ret = init_data->regulator_init(rdev->reg_data);
> > -               if (ret < 0)
> > -                       goto clean;
> > -       }
> > +       /* find device_node and attach it */
> > +       rdev->dev.of_node = of_find_node_by_name(NULL, regulator_desc->name);
> > 
> >         /* register with sysfs */
> >         rdev->dev.class = &regulator_class;
> > @@ -2693,6 +2686,20 @@ struct regulator_dev *regulator_register(struct regulator_desc *regulator_desc,
> >                 goto clean;
> >         }
> > 
> > +       if (!init_data) {
> > +               /* try to get init_data from device tree */
> > +               init_data = of_get_regulator_init_data(&rdev->dev);
> > +               if (!init_data)
> > +                       return ERR_PTR(-EINVAL);
> > +       }
> > +
> > +       /* preform any regulator specific init */
> > +       if (init_data->regulator_init) {
> > +               ret = init_data->regulator_init(rdev->reg_data);
> > +               if (ret < 0)
> > +                       goto clean;
> > +       }
> > +
> >         dev_set_drvdata(&rdev->dev, rdev);
> > 
> >         /* set regulator constraints */
> > @@ -2719,7 +2726,7 @@ struct regulator_dev *regulator_register(struct regulator_desc *regulator_desc,
> >                         node = of_get_regulator(dev, supply);
> >                         if (node)
> >                                 list_for_each_entry(r, &regulator_list, list)
> > -                                       if (node == r->dev.parent->of_node)
> > +                                       if (node == r->dev.of_node)
> >                                                 goto found;
> >                 }
> > 
> > ------->8---
> > 
> > And my dts file looks like something below.
> > 
> > 	ecspi at 70010000 { /* ECSPI1 */
> > 		fsl,spi-num-chipselects = <2>;
> > 		cs-gpios = <&gpio3 24 0>, /* GPIO4_24 */
> > 			   <&gpio3 25 0>; /* GPIO4_25 */
> > 		status = "okay";
> > 
> > 		pmic: mc13892 at 0 {
> > 			#address-cells = <1>;
> > 			#size-cells = <0>;
> > 			compatible = "fsl,mc13892";
> > 			spi-max-frequency = <6000000>;
> > 			reg = <0>;
> > 			mc13xxx-irq-gpios = <&gpio0 8 0>; /* GPIO1_8 */
> > 
> > 			regulators {
> > 				sw1reg: mc13892_sw1 {
> > 					regulator-min-uV = <600000>;
> > 					regulator-max-uV = <1375000>;
> > 					regulator-change-voltage;
> > 					regulator-boot-on;
> > 					regulator-always-on;
> > 				};
> > 
> > 				sw2reg: mc13892_sw2 {
> > 					regulator-min-uV = <900000>;
> > 					regulator-max-uV = <1850000>;
> > 					regulator-change-voltage;
> > 					regulator-boot-on;
> > 					regulator-always-on;
> > 				};
> > 
> > 				......
> > 			};
> 
> To follow up from my earlier comment, this .dts structure looks fine
> and reasonable to me, and it would also be fine for the mc13892 driver
> to use for_each_child_of_node() to find all the children of the
> regulators node.  Even finding the 'regulators' node by name from the
> mc13892 driver is perfectly fine provided for_each_child_of_node is
> used to find it.  All of this is okay because it is under the umbrella
> of the "fsl,mc13892" binding.
> 
For mc13892 regulator example, there are 3 levels 'struct dev'.

1. drivers/mfd/mc13xxx-core.c
   The "fsl,mc13892" binding is used in this mfd driver to get
   mc13xxx-core device probed.  And this mfd driver will in turn
   call mfd_add_devices() to add the second level device below.

2. drivers/regulator/mc13892-regulator.c
   As this device is created by mfd_add_devices() above, there is no
   device_node attached to its of_node, though we would hope that node
   'regulators' is attached there.  And the driver will call
   regulator_register() to have each regulator device created and
   registered in regulator core below.

3. drivers/regulator/core.c
   regulator_register() called above will create device (rdev->dev)
   for each regulator.

I am thinking about hiding the device_node discovering for each
regulator from the second level (regulator driver) and make it
internal to the third level (regulator core), as it seems to me
that regulator driver does not need to necessarily know about this.

If we can attach the device_node of 'regulators' node to dev->of_node
when calling regulator_register(regulator_desc, dev, ...) from
regulator driver, the regulator core will be able to find all nodes under
'regulators' using for_each_child_of_node(dev->of_node, child).

Then the question would be how we attach the device_node of 'regulators'
to dev->of_node where 'dev' is the second level device above.  I somehow
hesitate to hack this into mfd_add_devices(), so I would like to add
compatible string "fsl,mc13892-regulators" to node 'regulators' and
find the node using of_find_compatible_node(dev->parent, NULL,
"fsl,mc13892-regulators").

I'm not sure this is exactly same as what your comment suggests above.
So please let me know if it's appropriate.

-- 
Regards,
Shawn

[PATCH v2 3/5] regulator: helper routine to extract regulator_init_data

[Mark Brown]

On Mon, Oct 24, 2011 at 09:04:31PM +0800, Shawn Guo wrote:

> If we can attach the device_node of 'regulators' node to dev->of_node
> when calling regulator_register(regulator_desc, dev, ...) from
> regulator driver, the regulator core will be able to find all nodes under
> 'regulators' using for_each_child_of_node(dev->of_node, child).

Please provide concrete examples of the bindings you're talking about,
the really important thing here is how sane the bindings look and I've
really got no idea what any of what you're talking about will look like
or if they make sense.

> hesitate to hack this into mfd_add_devices(), so I would like to add
> compatible string "fsl,mc13892-regulators" to node 'regulators' and
> find the node using of_find_compatible_node(dev->parent, NULL,
> "fsl,mc13892-regulators").

It's not immediately obvious to me that having a binding for the
regulators separately makes sense, it's not a usefully distinct device.

[PATCH v2 3/5] regulator: helper routine to extract regulator_init_data

[Shawn Guo]

On Mon, Oct 24, 2011 at 03:06:37PM +0200, Mark Brown wrote:
> On Mon, Oct 24, 2011 at 09:04:31PM +0800, Shawn Guo wrote:
> 
> > If we can attach the device_node of 'regulators' node to dev->of_node
> > when calling regulator_register(regulator_desc, dev, ...) from
> > regulator driver, the regulator core will be able to find all nodes under
> > 'regulators' using for_each_child_of_node(dev->of_node, child).
> 
> Please provide concrete examples of the bindings you're talking about,
> the really important thing here is how sane the bindings look and I've
> really got no idea what any of what you're talking about will look like
> or if they make sense.
> 
The only thing different from what I attached last time is the
compatible string added to 'regulators' node.

        ecspi at 70010000 { /* ECSPI1 */
                fsl,spi-num-chipselects = <2>;
                cs-gpios = <&gpio3 24 0>, /* GPIO4_24 */
                           <&gpio3 25 0>; /* GPIO4_25 */
                status = "okay";

                pmic: mc13892 at 0 {
                        #address-cells = <1>;
                        #size-cells = <0>;
                        compatible = "fsl,mc13892";
                        spi-max-frequency = <6000000>;
                        reg = <0>;
                        mc13xxx-irq-gpios = <&gpio0 8 0>; /* GPIO1_8 */

                        regulators {
                        	compatible = "fsl,mc13892-regulator";

                                sw1reg: mc13892_sw1 {
                                        regulator-min-uV = <600000>;
                                        regulator-max-uV = <1375000>;
                                        regulator-change-voltage;
                                        regulator-boot-on;
                                        regulator-always-on;
                                };

                                sw2reg: mc13892_sw2 {
                                        regulator-min-uV = <900000>;
                                        regulator-max-uV = <1850000>;
                                        regulator-change-voltage;
                                        regulator-boot-on;
                                        regulator-always-on;
                                };

                                ......
                        };

                        leds {
                                ......
                        };

                        buttons {
                                ......
                        };
                };

                flash: at45db321d at 1 {
                        ......
                };
        };

> > hesitate to hack this into mfd_add_devices(), so I would like to add
> > compatible string "fsl,mc13892-regulators" to node 'regulators' and
> > find the node using of_find_compatible_node(dev->parent, NULL,
> > "fsl,mc13892-regulators").
> 
> It's not immediately obvious to me that having a binding for the
> regulators separately makes sense, it's not a usefully distinct device.
> 
Fair point.  Actually, I also hate to have the finding of node
'regulators' plugged into regulator driver.  What about following
change to address Grant's concern on global device tree search?
 
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 8fe132d..29dcf90 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -2673,7 +2673,8 @@ struct regulator_dev *regulator_register(struct regulator_desc *regulator_desc,
        BLOCKING_INIT_NOTIFIER_HEAD(&rdev->notifier);

        /* find device_node and attach it */
-       rdev->dev.of_node = of_find_node_by_name(NULL, regulator_desc->name);
+       rdev->dev.of_node = of_find_node_by_name(dev->parent->of_node,
+                                                regulator_desc->name);

        /* register with sysfs */
        rdev->dev.class = &regulator_class;

-- 
Regards,
Shawn

[PATCH v2 3/5] regulator: helper routine to extract regulator_init_data

[Mark Brown]

On Mon, Oct 24, 2011 at 09:40:26PM +0800, Shawn Guo wrote:

> +++ b/drivers/regulator/core.c
> @@ -2673,7 +2673,8 @@ struct regulator_dev *regulator_register(struct regulator_desc *regulator_desc,
>         BLOCKING_INIT_NOTIFIER_HEAD(&rdev->notifier);
> 
>         /* find device_node and attach it */
> -       rdev->dev.of_node = of_find_node_by_name(NULL, regulator_desc->name);
> +       rdev->dev.of_node = of_find_node_by_name(dev->parent->of_node,
> +                                                regulator_desc->name);
> 

Is that going to do the right thing if you've got a MFD which does
register each regulator as a separate device?  Might be best to just
search within dev and get drivers to pass the "real" device in when
registering the regulator rather than the virtual device.

[PATCH v2 3/5] regulator: helper routine to extract regulator_init_data

[Shawn Guo]

On Mon, Oct 24, 2011 at 03:49:30PM +0200, Mark Brown wrote:
> On Mon, Oct 24, 2011 at 09:40:26PM +0800, Shawn Guo wrote:
> 
> > +++ b/drivers/regulator/core.c
> > @@ -2673,7 +2673,8 @@ struct regulator_dev *regulator_register(struct regulator_desc *regulator_desc,
> >         BLOCKING_INIT_NOTIFIER_HEAD(&rdev->notifier);
> > 
> >         /* find device_node and attach it */
> > -       rdev->dev.of_node = of_find_node_by_name(NULL, regulator_desc->name);
> > +       rdev->dev.of_node = of_find_node_by_name(dev->parent->of_node,
> > +                                                regulator_desc->name);
> > 
> 
> Is that going to do the right thing if you've got a MFD which does
> register each regulator as a separate device?

Based on my understanding, 1:1 is just a special case of N:1.  I
failed to see any problem having it work with registering each
regulator as a separate device.

> Might be best to just
> search within dev and get drivers to pass the "real" device in when
> registering the regulator rather than the virtual device.
> 
It should also work, but it will also change the API slightly for
non-dt users.  I'm not sure it's something we want.

-- 
Regards,
Shawn

[PATCH v2 3/5] regulator: helper routine to extract regulator_init_data

[Mark Brown]

On Mon, Oct 24, 2011 at 10:47:17PM +0800, Shawn Guo wrote:
> On Mon, Oct 24, 2011 at 03:49:30PM +0200, Mark Brown wrote:

> > Might be best to just
> > search within dev and get drivers to pass the "real" device in when
> > registering the regulator rather than the virtual device.

> It should also work, but it will also change the API slightly for
> non-dt users.  I'm not sure it's something we want.

I don't think that's an issue at all.

[PATCH v2 3/5] regulator: helper routine to extract regulator_init_data

[Grant Likely]

On Mon, Oct 24, 2011 at 09:40:26PM +0800, Shawn Guo wrote:
> On Mon, Oct 24, 2011 at 03:06:37PM +0200, Mark Brown wrote:
> > On Mon, Oct 24, 2011 at 09:04:31PM +0800, Shawn Guo wrote:
> > 
> > > If we can attach the device_node of 'regulators' node to dev->of_node
> > > when calling regulator_register(regulator_desc, dev, ...) from
> > > regulator driver, the regulator core will be able to find all nodes under
> > > 'regulators' using for_each_child_of_node(dev->of_node, child).
> > 
> > Please provide concrete examples of the bindings you're talking about,
> > the really important thing here is how sane the bindings look and I've
> > really got no idea what any of what you're talking about will look like
> > or if they make sense.
> > 
> The only thing different from what I attached last time is the
> compatible string added to 'regulators' node.
> 
>         ecspi at 70010000 { /* ECSPI1 */
>                 fsl,spi-num-chipselects = <2>;
>                 cs-gpios = <&gpio3 24 0>, /* GPIO4_24 */
>                            <&gpio3 25 0>; /* GPIO4_25 */
>                 status = "okay";
> 
>                 pmic: mc13892 at 0 {
>                         #address-cells = <1>;
>                         #size-cells = <0>;
>                         compatible = "fsl,mc13892";
>                         spi-max-frequency = <6000000>;
>                         reg = <0>;
>                         mc13xxx-irq-gpios = <&gpio0 8 0>; /* GPIO1_8 */
> 
>                         regulators {
>                         	compatible = "fsl,mc13892-regulator";
> 
>                                 sw1reg: mc13892_sw1 {
>                                         regulator-min-uV = <600000>;
>                                         regulator-max-uV = <1375000>;
>                                         regulator-change-voltage;
>                                         regulator-boot-on;
>                                         regulator-always-on;
>                                 };
> 
>                                 sw2reg: mc13892_sw2 {
>                                         regulator-min-uV = <900000>;
>                                         regulator-max-uV = <1850000>;
>                                         regulator-change-voltage;
>                                         regulator-boot-on;
>                                         regulator-always-on;
>                                 };
> 
>                                 ......
>                         };
> 
>                         leds {
>                                 ......
>                         };
> 
>                         buttons {
>                                 ......
>                         };
>                 };
> 
>                 flash: at45db321d at 1 {
>                         ......
>                 };
>         };
> 
> > > hesitate to hack this into mfd_add_devices(), so I would like to add
> > > compatible string "fsl,mc13892-regulators" to node 'regulators' and
> > > find the node using of_find_compatible_node(dev->parent, NULL,
> > > "fsl,mc13892-regulators").
> > 
> > It's not immediately obvious to me that having a binding for the
> > regulators separately makes sense, it's not a usefully distinct device.
> > 
> Fair point.  Actually, I also hate to have the finding of node
> 'regulators' plugged into regulator driver.  What about following
> change to address Grant's concern on global device tree search?
>  
> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index 8fe132d..29dcf90 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -2673,7 +2673,8 @@ struct regulator_dev *regulator_register(struct regulator_desc *regulator_desc,
>         BLOCKING_INIT_NOTIFIER_HEAD(&rdev->notifier);
> 
>         /* find device_node and attach it */
> -       rdev->dev.of_node = of_find_node_by_name(NULL, regulator_desc->name);
> +       rdev->dev.of_node = of_find_node_by_name(dev->parent->of_node,
> +                                                regulator_desc->name);

of_find_node_by_name() doesn't work that way.  The first argument is a
starting point, but it doesn't restrict the search to children of a
node.

for_each_child_of_node() is what you want to use when iterating over
the children which unfortunately changes the structure of this
function.

g.

[PATCH v2 3/5] regulator: helper routine to extract regulator_init_data

[Shawn Guo]

On Mon, Oct 24, 2011 at 03:59:50PM +0200, Grant Likely wrote:
> On Mon, Oct 24, 2011 at 09:40:26PM +0800, Shawn Guo wrote:
> > On Mon, Oct 24, 2011 at 03:06:37PM +0200, Mark Brown wrote:
> > > On Mon, Oct 24, 2011 at 09:04:31PM +0800, Shawn Guo wrote:
> > > 
> > > > If we can attach the device_node of 'regulators' node to dev->of_node
> > > > when calling regulator_register(regulator_desc, dev, ...) from
> > > > regulator driver, the regulator core will be able to find all nodes under
> > > > 'regulators' using for_each_child_of_node(dev->of_node, child).
> > > 
> > > Please provide concrete examples of the bindings you're talking about,
> > > the really important thing here is how sane the bindings look and I've
> > > really got no idea what any of what you're talking about will look like
> > > or if they make sense.
> > > 
> > The only thing different from what I attached last time is the
> > compatible string added to 'regulators' node.
> > 
> >         ecspi at 70010000 { /* ECSPI1 */
> >                 fsl,spi-num-chipselects = <2>;
> >                 cs-gpios = <&gpio3 24 0>, /* GPIO4_24 */
> >                            <&gpio3 25 0>; /* GPIO4_25 */
> >                 status = "okay";
> > 
> >                 pmic: mc13892 at 0 {
> >                         #address-cells = <1>;
> >                         #size-cells = <0>;
> >                         compatible = "fsl,mc13892";
> >                         spi-max-frequency = <6000000>;
> >                         reg = <0>;
> >                         mc13xxx-irq-gpios = <&gpio0 8 0>; /* GPIO1_8 */
> > 
> >                         regulators {
> >                         	compatible = "fsl,mc13892-regulator";
> > 
> >                                 sw1reg: mc13892_sw1 {
> >                                         regulator-min-uV = <600000>;
> >                                         regulator-max-uV = <1375000>;
> >                                         regulator-change-voltage;
> >                                         regulator-boot-on;
> >                                         regulator-always-on;
> >                                 };
> > 
> >                                 sw2reg: mc13892_sw2 {
> >                                         regulator-min-uV = <900000>;
> >                                         regulator-max-uV = <1850000>;
> >                                         regulator-change-voltage;
> >                                         regulator-boot-on;
> >                                         regulator-always-on;
> >                                 };
> > 
> >                                 ......
> >                         };
> > 
> >                         leds {
> >                                 ......
> >                         };
> > 
> >                         buttons {
> >                                 ......
> >                         };
> >                 };
> > 
> >                 flash: at45db321d at 1 {
> >                         ......
> >                 };
> >         };
> > 
> > > > hesitate to hack this into mfd_add_devices(), so I would like to add
> > > > compatible string "fsl,mc13892-regulators" to node 'regulators' and
> > > > find the node using of_find_compatible_node(dev->parent, NULL,
> > > > "fsl,mc13892-regulators").
> > > 
> > > It's not immediately obvious to me that having a binding for the
> > > regulators separately makes sense, it's not a usefully distinct device.
> > > 
> > Fair point.  Actually, I also hate to have the finding of node
> > 'regulators' plugged into regulator driver.  What about following
> > change to address Grant's concern on global device tree search?
> >  
> > diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> > index 8fe132d..29dcf90 100644
> > --- a/drivers/regulator/core.c
> > +++ b/drivers/regulator/core.c
> > @@ -2673,7 +2673,8 @@ struct regulator_dev *regulator_register(struct regulator_desc *regulator_desc,
> >         BLOCKING_INIT_NOTIFIER_HEAD(&rdev->notifier);
> > 
> >         /* find device_node and attach it */
> > -       rdev->dev.of_node = of_find_node_by_name(NULL, regulator_desc->name);
> > +       rdev->dev.of_node = of_find_node_by_name(dev->parent->of_node,
> > +                                                regulator_desc->name);
> 
> of_find_node_by_name() doesn't work that way.  The first argument is a
> starting point, but it doesn't restrict the search to children of a
> node.
> 
> for_each_child_of_node() is what you want to use when iterating over
> the children which unfortunately changes the structure of this
> function.
> 
The dev->parent->of_node is meant to point to node 'pmic: mc13892 at 0'.
And the intention here is not to iterate over the children, but to
start a search from a reasonable point rather than the top root node.

-- 
Regards,
Shawn

[PATCH v2 3/5] regulator: helper routine to extract regulator_init_data

[Grant Likely]

On Mon, Oct 24, 2011 at 10:51:40PM +0800, Shawn Guo wrote:
> On Mon, Oct 24, 2011 at 03:59:50PM +0200, Grant Likely wrote:
> > On Mon, Oct 24, 2011 at 09:40:26PM +0800, Shawn Guo wrote:
> > > diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> > > index 8fe132d..29dcf90 100644
> > > --- a/drivers/regulator/core.c
> > > +++ b/drivers/regulator/core.c
> > > @@ -2673,7 +2673,8 @@ struct regulator_dev *regulator_register(struct regulator_desc *regulator_desc,
> > >         BLOCKING_INIT_NOTIFIER_HEAD(&rdev->notifier);
> > > 
> > >         /* find device_node and attach it */
> > > -       rdev->dev.of_node = of_find_node_by_name(NULL, regulator_desc->name);
> > > +       rdev->dev.of_node = of_find_node_by_name(dev->parent->of_node,
> > > +                                                regulator_desc->name);
> > 
> > of_find_node_by_name() doesn't work that way.  The first argument is a
> > starting point, but it doesn't restrict the search to children of a
> > node.
> > 
> > for_each_child_of_node() is what you want to use when iterating over
> > the children which unfortunately changes the structure of this
> > function.
> > 
> The dev->parent->of_node is meant to point to node 'pmic: mc13892 at 0'.
> And the intention here is not to iterate over the children, but to
> start a search from a reasonable point rather than the top root node.

It is always better to attach the of_node at struct device
registration time instead of searching the tree in common code.  The
of_node should already be assigned by the time regulator_register() is
called.  The caller should already have access to all that information
before the call anyway, especially since it is not strictly manditory
for all regulators to use the common binding.  It is entirely
conceivable that the proposed binding won't work for some regulators.

g.

[PATCH v2 3/5] regulator: helper routine to extract regulator_init_data

[Shawn Guo]

On Mon, Oct 24, 2011 at 04:56:31PM +0200, Grant Likely wrote:
> On Mon, Oct 24, 2011 at 10:51:40PM +0800, Shawn Guo wrote:
> > On Mon, Oct 24, 2011 at 03:59:50PM +0200, Grant Likely wrote:
> > > On Mon, Oct 24, 2011 at 09:40:26PM +0800, Shawn Guo wrote:
> > > > diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> > > > index 8fe132d..29dcf90 100644
> > > > --- a/drivers/regulator/core.c
> > > > +++ b/drivers/regulator/core.c
> > > > @@ -2673,7 +2673,8 @@ struct regulator_dev *regulator_register(struct regulator_desc *regulator_desc,
> > > >         BLOCKING_INIT_NOTIFIER_HEAD(&rdev->notifier);
> > > > 
> > > >         /* find device_node and attach it */
> > > > -       rdev->dev.of_node = of_find_node_by_name(NULL, regulator_desc->name);
> > > > +       rdev->dev.of_node = of_find_node_by_name(dev->parent->of_node,
> > > > +                                                regulator_desc->name);
> > > 
> > > of_find_node_by_name() doesn't work that way.  The first argument is a
> > > starting point, but it doesn't restrict the search to children of a
> > > node.
> > > 
> > > for_each_child_of_node() is what you want to use when iterating over
> > > the children which unfortunately changes the structure of this
> > > function.
> > > 
> > The dev->parent->of_node is meant to point to node 'pmic: mc13892 at 0'.
> > And the intention here is not to iterate over the children, but to
> > start a search from a reasonable point rather than the top root node.
> 
> It is always better to attach the of_node at struct device
> registration time instead of searching the tree in common code.  The
> of_node should already be assigned by the time regulator_register() is
> called.

That's the problem we have.  There is no 'struct dev' to attach of_node
for each regulator by the time regulator_register() is called, because
the 'struct dev' for each regulator is created inside
regulator_register() as wrapped by 'struct regulator_dev'.

-- 
Regards,
Shawn

[PATCH v2 3/5] regulator: helper routine to extract regulator_init_data

[Grant Likely]

On Mon, Oct 24, 2011 at 11:51:33PM +0800, Shawn Guo wrote:
> On Mon, Oct 24, 2011 at 04:56:31PM +0200, Grant Likely wrote:
> > On Mon, Oct 24, 2011 at 10:51:40PM +0800, Shawn Guo wrote:
> > > On Mon, Oct 24, 2011 at 03:59:50PM +0200, Grant Likely wrote:
> > > > On Mon, Oct 24, 2011 at 09:40:26PM +0800, Shawn Guo wrote:
> > > > > diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> > > > > index 8fe132d..29dcf90 100644
> > > > > --- a/drivers/regulator/core.c
> > > > > +++ b/drivers/regulator/core.c
> > > > > @@ -2673,7 +2673,8 @@ struct regulator_dev *regulator_register(struct regulator_desc *regulator_desc,
> > > > >         BLOCKING_INIT_NOTIFIER_HEAD(&rdev->notifier);
> > > > > 
> > > > >         /* find device_node and attach it */
> > > > > -       rdev->dev.of_node = of_find_node_by_name(NULL, regulator_desc->name);
> > > > > +       rdev->dev.of_node = of_find_node_by_name(dev->parent->of_node,
> > > > > +                                                regulator_desc->name);
> > > > 
> > > > of_find_node_by_name() doesn't work that way.  The first argument is a
> > > > starting point, but it doesn't restrict the search to children of a
> > > > node.
> > > > 
> > > > for_each_child_of_node() is what you want to use when iterating over
> > > > the children which unfortunately changes the structure of this
> > > > function.
> > > > 
> > > The dev->parent->of_node is meant to point to node 'pmic: mc13892 at 0'.
> > > And the intention here is not to iterate over the children, but to
> > > start a search from a reasonable point rather than the top root node.
> > 
> > It is always better to attach the of_node at struct device
> > registration time instead of searching the tree in common code.  The
> > of_node should already be assigned by the time regulator_register() is
> > called.
> 
> That's the problem we have.  There is no 'struct dev' to attach of_node
> for each regulator by the time regulator_register() is called, because
> the 'struct dev' for each regulator is created inside
> regulator_register() as wrapped by 'struct regulator_dev'.

Then regulator_register must be split in half.  This is a common use
case, and other libraries have had to do the same.  One half to
allocate the structure, and then a second half for registration, and
regulator_register() becomes a simpler helper that calls both.  That
way a caller can insert extra initialization between the two steps.

g.

[PATCH v2 3/5] regulator: helper routine to extract regulator_init_data

[Rajendra Nayak]

On Monday 24 October 2011 09:21 PM, Shawn Guo wrote:
> On Mon, Oct 24, 2011 at 04:56:31PM +0200, Grant Likely wrote:
>> On Mon, Oct 24, 2011 at 10:51:40PM +0800, Shawn Guo wrote:
>>> On Mon, Oct 24, 2011 at 03:59:50PM +0200, Grant Likely wrote:
>>>> On Mon, Oct 24, 2011 at 09:40:26PM +0800, Shawn Guo wrote:
>>>>> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
>>>>> index 8fe132d..29dcf90 100644
>>>>> --- a/drivers/regulator/core.c
>>>>> +++ b/drivers/regulator/core.c
>>>>> @@ -2673,7 +2673,8 @@ struct regulator_dev *regulator_register(struct regulator_desc *regulator_desc,
>>>>>          BLOCKING_INIT_NOTIFIER_HEAD(&rdev->notifier);
>>>>>
>>>>>          /* find device_node and attach it */
>>>>> -       rdev->dev.of_node = of_find_node_by_name(NULL, regulator_desc->name);
>>>>> +       rdev->dev.of_node = of_find_node_by_name(dev->parent->of_node,
>>>>> +                                                regulator_desc->name);
>>>>
>>>> of_find_node_by_name() doesn't work that way.  The first argument is a
>>>> starting point, but it doesn't restrict the search to children of a
>>>> node.
>>>>
>>>> for_each_child_of_node() is what you want to use when iterating over
>>>> the children which unfortunately changes the structure of this
>>>> function.
>>>>
>>> The dev->parent->of_node is meant to point to node 'pmic: mc13892 at 0'.
>>> And the intention here is not to iterate over the children, but to
>>> start a search from a reasonable point rather than the top root node.
>>
>> It is always better to attach the of_node at struct device
>> registration time instead of searching the tree in common code.  The
>> of_node should already be assigned by the time regulator_register() is
>> called.
>
> That's the problem we have.  There is no 'struct dev' to attach of_node
> for each regulator by the time regulator_register() is called, because
> the 'struct dev' for each regulator is created inside
> regulator_register() as wrapped by 'struct regulator_dev'.

The root of your problem seems to be that your pmic driver isn't
registering regulator devices from DT, and if it did, you wouldn't
need to do a search in dev->parent->of_node and instead the driver
would have the right dev->of_node populated.

>

[PATCH v2 3/5] regulator: helper routine to extract regulator_init_data

[Shawn Guo]

On Tue, Oct 25, 2011 at 11:40:34AM +0530, Rajendra Nayak wrote:
> On Monday 24 October 2011 09:21 PM, Shawn Guo wrote:
> >On Mon, Oct 24, 2011 at 04:56:31PM +0200, Grant Likely wrote:
[...]
> >>It is always better to attach the of_node at struct device
> >>registration time instead of searching the tree in common code.  The
> >>of_node should already be assigned by the time regulator_register() is
> >>called.
> >
> >That's the problem we have.  There is no 'struct dev' to attach of_node
> >for each regulator by the time regulator_register() is called, because
> >the 'struct dev' for each regulator is created inside
> >regulator_register() as wrapped by 'struct regulator_dev'.
> 
> The root of your problem seems to be that your pmic driver isn't
> registering regulator devices from DT, and if it did, you wouldn't
> need to do a search in dev->parent->of_node and instead the driver
> would have the right dev->of_node populated.
> 
No, it's not the root of my problem.  Again, we are talking about
'Case 2', where multiple regulator devices are registered to
regulator core with regulator driver being probed once, where each
regulator node is taken as the child of 'regulators' node.  Having
device_node of 'regulators' attached to dev->of_node does not help
at all.  What we need is to have each child node attached to
regulator_dev->dev.of_node.

-- 
Regards,
Shawn

[PATCH v2 3/5] regulator: helper routine to extract regulator_init_data

[Rajendra Nayak]

On Tuesday 25 October 2011 12:38 PM, Shawn Guo wrote:
> On Tue, Oct 25, 2011 at 11:40:34AM +0530, Rajendra Nayak wrote:
>> On Monday 24 October 2011 09:21 PM, Shawn Guo wrote:
>>> On Mon, Oct 24, 2011 at 04:56:31PM +0200, Grant Likely wrote:
> [...]
>>>> It is always better to attach the of_node at struct device
>>>> registration time instead of searching the tree in common code.  The
>>>> of_node should already be assigned by the time regulator_register() is
>>>> called.
>>>
>>> That's the problem we have.  There is no 'struct dev' to attach of_node
>>> for each regulator by the time regulator_register() is called, because
>>> the 'struct dev' for each regulator is created inside
>>> regulator_register() as wrapped by 'struct regulator_dev'.
>>
>> The root of your problem seems to be that your pmic driver isn't
>> registering regulator devices from DT, and if it did, you wouldn't
>> need to do a search in dev->parent->of_node and instead the driver
>> would have the right dev->of_node populated.
>>
> No, it's not the root of my problem.  Again, we are talking about
> 'Case 2', where multiple regulator devices are registered to
> regulator core with regulator driver being probed once, where each
> regulator node is taken as the child of 'regulators' node.  Having
> device_node of 'regulators' attached to dev->of_node does not help
> at all.  What we need is to have each child node attached to
> regulator_dev->dev.of_node.

It certainly helps if dev->of_node has the 'regulators' node attached.
The driver can very easily then do a for_each_child_of_node() to extract
and register individual regulators passing an additional of_node param.

>

[PATCH v2 3/5] regulator: helper routine to extract regulator_init_data

[Shawn Guo]

On Tue, Oct 25, 2011 at 12:31:51PM +0530, Rajendra Nayak wrote:
> On Tuesday 25 October 2011 12:38 PM, Shawn Guo wrote:
> >On Tue, Oct 25, 2011 at 11:40:34AM +0530, Rajendra Nayak wrote:
> >>On Monday 24 October 2011 09:21 PM, Shawn Guo wrote:
> >>>On Mon, Oct 24, 2011 at 04:56:31PM +0200, Grant Likely wrote:
> >[...]
> >>>>It is always better to attach the of_node at struct device
> >>>>registration time instead of searching the tree in common code.  The
> >>>>of_node should already be assigned by the time regulator_register() is
> >>>>called.
> >>>
> >>>That's the problem we have.  There is no 'struct dev' to attach of_node
> >>>for each regulator by the time regulator_register() is called, because
> >>>the 'struct dev' for each regulator is created inside
> >>>regulator_register() as wrapped by 'struct regulator_dev'.
> >>
> >>The root of your problem seems to be that your pmic driver isn't
> >>registering regulator devices from DT, and if it did, you wouldn't
> >>need to do a search in dev->parent->of_node and instead the driver
> >>would have the right dev->of_node populated.
> >>
> >No, it's not the root of my problem.  Again, we are talking about
> >'Case 2', where multiple regulator devices are registered to
> >regulator core with regulator driver being probed once, where each
> >regulator node is taken as the child of 'regulators' node.  Having
> >device_node of 'regulators' attached to dev->of_node does not help
> >at all.  What we need is to have each child node attached to
> >regulator_dev->dev.of_node.
> 
> It certainly helps if dev->of_node has the 'regulators' node attached.
> The driver can very easily then do a for_each_child_of_node() to extract
> and register individual regulators passing an additional of_node param.
> 
Well, let's look at what Grant is asking for.  He is asking for that
each child node should be attached to regulator_dev->dev.of_node by
the time regulator_register() is called.  How can this be helping that?

-- 
Regards,
Shawn

[PATCH v2 4/5] regulator: adapt fixed regulator driver to dt

[Rajendra Nayak]

The fixed regulator driver uses of_get_fixed_voltage_config()
to extract fixed_voltage_config structure contents from device tree.

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

Signed-off-by: Rajendra Nayak <rnayak@ti.com>
---
 .../bindings/regulator/fixed-regulator.txt         |   25 +++++++++
 drivers/regulator/fixed.c                          |   58 ++++++++++++++++++++
 2 files changed, 83 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/regulator/fixed-regulator.txt

diff --git a/Documentation/devicetree/bindings/regulator/fixed-regulator.txt b/Documentation/devicetree/bindings/regulator/fixed-regulator.txt
new file mode 100644
index 0000000..049df3d
--- /dev/null
+++ b/Documentation/devicetree/bindings/regulator/fixed-regulator.txt
@@ -0,0 +1,25 @@
+Fixed Voltage regulators
+
+Required properties:
+- compatible: Must be "regulator-fixed";
+
+Optional properties:
+- regulator-fixed-supply: Name of the regulator supply
+- regulator-fixed-microvolts: Output voltage of regulator
+- regulator-fixed-gpio: gpio to use for enable control
+- regulator-fixed-startup-delay: startup time in microseconds
+- regulator-fixed-enable-high: Polarity of enable GPIO,
+  1 = Active High, 0 = Active low
+- regulator-fixed-enabled-at-boot: 1 = yes, 0 = no
+
+Example:
+
+	abc: fixedregulator at 0 {
+		compatible = "regulator-fixed";
+		regulator-fixed-supply = "fixed-supply";
+		regulator-fixed-microvolts = <1800000>;
+		regulator-fixed-gpio = <43>;
+		regulator-fixed-startup-delay = <70000>;
+		regulator-fixed-enable-high;
+		regulator-fixed-enabled-at-boot;
+	};
diff --git a/drivers/regulator/fixed.c b/drivers/regulator/fixed.c
index 2fe9d99..eb37eb6 100644
--- a/drivers/regulator/fixed.c
+++ b/drivers/regulator/fixed.c
@@ -23,9 +23,12 @@
 #include <linux/platform_device.h>
 #include <linux/regulator/driver.h>
 #include <linux/regulator/fixed.h>
+#include <linux/regulator/machine.h>
 #include <linux/gpio.h>
 #include <linux/delay.h>
 #include <linux/slab.h>
+#include <linux/of.h>
+#include <linux/regulator/of_regulator.h>
 
 struct fixed_voltage_data {
 	struct regulator_desc desc;
@@ -37,6 +40,47 @@ struct fixed_voltage_data {
 	bool is_enabled;
 };
 
+
+/**
+ * of_get_fixed_voltage_config - extract fixed_voltage_config structure info
+ * @dev: device requesting for fixed_voltage_config
+ *
+ * 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.
+ */
+struct fixed_voltage_config *of_get_fixed_voltage_config(struct device *dev)
+{
+	struct fixed_voltage_config *config;
+	struct device_node *np = dev->of_node;
+	const __be32 *microvolts, *gpio, *delay;
+
+	config = devm_kzalloc(dev, sizeof(struct fixed_voltage_config), GFP_KERNEL);
+	if (!config)
+		return NULL;
+
+	config->supply_name = (char *)of_get_property(np,
+					"regulator-fixed-supply", NULL);
+	microvolts = of_get_property(np, "regulator-fixed-microvolts", NULL);
+	if (microvolts)
+		config->microvolts = be32_to_cpu(*microvolts);
+	gpio = of_get_property(np, "regulator-fixed-gpio", NULL);
+	if (gpio)
+		config->gpio = be32_to_cpu(*gpio);
+	delay = of_get_property(np, "regulator-fixed-startup-delay", NULL);
+	if (delay)
+		config->startup_delay = be32_to_cpu(*delay);
+
+	if (of_find_property(np, "regulator-fixed-enable-high", NULL))
+		config->enable_high = true;
+	if (of_find_property(np, "regulator-fixed-enabled-at-boot", NULL))
+		config->enabled_at_boot = true;
+
+	config->init_data = of_get_regulator_init_data(dev);
+
+	return config;
+}
+
 static int fixed_voltage_is_enabled(struct regulator_dev *dev)
 {
 	struct fixed_voltage_data *data = rdev_get_drvdata(dev);
@@ -108,6 +152,9 @@ static int __devinit reg_fixed_voltage_probe(struct platform_device *pdev)
 	struct fixed_voltage_data *drvdata;
 	int ret;
 
+	if (pdev->dev.of_node)
+		config = of_get_fixed_voltage_config(&pdev->dev);
+
 	drvdata = kzalloc(sizeof(struct fixed_voltage_data), GFP_KERNEL);
 	if (drvdata == NULL) {
 		dev_err(&pdev->dev, "Failed to allocate device data\n");
@@ -216,12 +263,23 @@ static int __devexit reg_fixed_voltage_remove(struct platform_device *pdev)
 	return 0;
 }
 
+#if defined(CONFIG_OF)
+static const struct of_device_id fixed_of_match[] __devinitconst = {
+	{ .compatible = "regulator-fixed", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, fixed_of_match);
+#else
+#define fixed_of_match NULL
+#endif
+
 static struct platform_driver regulator_fixed_voltage_driver = {
 	.probe		= reg_fixed_voltage_probe,
 	.remove		= __devexit_p(reg_fixed_voltage_remove),
 	.driver		= {
 		.name		= "reg-fixed-voltage",
 		.owner		= THIS_MODULE,
+		.of_match_table = fixed_of_match,
 	},
 };
 
-- 
1.7.1

[PATCH v2 4/5] regulator: adapt fixed regulator driver to dt

[Grant Likely]

On Mon, Oct 10, 2011 at 09:49:37PM +0530, Rajendra Nayak wrote:
> The fixed regulator driver uses of_get_fixed_voltage_config()
> to extract fixed_voltage_config structure contents from device tree.
> 
> Also add documenation for additional bindings for fixed
> regulators that can be passed through dt.
> 
> Signed-off-by: Rajendra Nayak <rnayak@ti.com>
> ---
>  .../bindings/regulator/fixed-regulator.txt         |   25 +++++++++
>  drivers/regulator/fixed.c                          |   58 ++++++++++++++++++++
>  2 files changed, 83 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/regulator/fixed-regulator.txt
> 
> diff --git a/Documentation/devicetree/bindings/regulator/fixed-regulator.txt b/Documentation/devicetree/bindings/regulator/fixed-regulator.txt
> new file mode 100644
> index 0000000..049df3d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/regulator/fixed-regulator.txt
> @@ -0,0 +1,25 @@
> +Fixed Voltage regulators
> +
> +Required properties:
> +- compatible: Must be "regulator-fixed";
> +
> +Optional properties:
> +- regulator-fixed-supply: Name of the regulator supply
> +- regulator-fixed-microvolts: Output voltage of regulator
> +- regulator-fixed-gpio: gpio to use for enable control
> +- regulator-fixed-startup-delay: startup time in microseconds
> +- regulator-fixed-enable-high: Polarity of enable GPIO,
> +  1 = Active High, 0 = Active low
> +- regulator-fixed-enabled-at-boot: 1 = yes, 0 = no
> +
> +Example:
> +
> +	abc: fixedregulator at 0 {
> +		compatible = "regulator-fixed";
> +		regulator-fixed-supply = "fixed-supply";
> +		regulator-fixed-microvolts = <1800000>;
> +		regulator-fixed-gpio = <43>;
> +		regulator-fixed-startup-delay = <70000>;
> +		regulator-fixed-enable-high;
> +		regulator-fixed-enabled-at-boot;
> +	};
> diff --git a/drivers/regulator/fixed.c b/drivers/regulator/fixed.c
> index 2fe9d99..eb37eb6 100644
> --- a/drivers/regulator/fixed.c
> +++ b/drivers/regulator/fixed.c
> @@ -23,9 +23,12 @@
>  #include <linux/platform_device.h>
>  #include <linux/regulator/driver.h>
>  #include <linux/regulator/fixed.h>
> +#include <linux/regulator/machine.h>
>  #include <linux/gpio.h>
>  #include <linux/delay.h>
>  #include <linux/slab.h>
> +#include <linux/of.h>
> +#include <linux/regulator/of_regulator.h>
>  
>  struct fixed_voltage_data {
>  	struct regulator_desc desc;
> @@ -37,6 +40,47 @@ struct fixed_voltage_data {
>  	bool is_enabled;
>  };
>  
> +
> +/**
> + * of_get_fixed_voltage_config - extract fixed_voltage_config structure info
> + * @dev: device requesting for fixed_voltage_config
> + *
> + * 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.
> + */
> +struct fixed_voltage_config *of_get_fixed_voltage_config(struct device *dev)
> +{
> +	struct fixed_voltage_config *config;
> +	struct device_node *np = dev->of_node;
> +	const __be32 *microvolts, *gpio, *delay;
> +
> +	config = devm_kzalloc(dev, sizeof(struct fixed_voltage_config), GFP_KERNEL);
> +	if (!config)
> +		return NULL;
> +
> +	config->supply_name = (char *)of_get_property(np,
> +					"regulator-fixed-supply", NULL);

of_get_property() returns a (const void *).  Casting drops the const
and should not be used.  Either make supply_name a (const char *), or
copy the string into a (char *) buffer.

> +	microvolts = of_get_property(np, "regulator-fixed-microvolts", NULL);
> +	if (microvolts)
> +		config->microvolts = be32_to_cpu(*microvolts);
> +	gpio = of_get_property(np, "regulator-fixed-gpio", NULL);
> +	if (gpio)
> +		config->gpio = be32_to_cpu(*gpio);

of_property_read_u32() may help here.

> +	delay = of_get_property(np, "regulator-fixed-startup-delay", NULL);
> +	if (delay)
> +		config->startup_delay = be32_to_cpu(*delay);
> +
> +	if (of_find_property(np, "regulator-fixed-enable-high", NULL))
> +		config->enable_high = true;
> +	if (of_find_property(np, "regulator-fixed-enabled-at-boot", NULL))
> +		config->enabled_at_boot = true;
> +
> +	config->init_data = of_get_regulator_init_data(dev);
> +
> +	return config;
> +}
> +
>  static int fixed_voltage_is_enabled(struct regulator_dev *dev)
>  {
>  	struct fixed_voltage_data *data = rdev_get_drvdata(dev);
> @@ -108,6 +152,9 @@ static int __devinit reg_fixed_voltage_probe(struct platform_device *pdev)
>  	struct fixed_voltage_data *drvdata;
>  	int ret;
>  
> +	if (pdev->dev.of_node)
> +		config = of_get_fixed_voltage_config(&pdev->dev);
> +
>  	drvdata = kzalloc(sizeof(struct fixed_voltage_data), GFP_KERNEL);
>  	if (drvdata == NULL) {
>  		dev_err(&pdev->dev, "Failed to allocate device data\n");
> @@ -216,12 +263,23 @@ static int __devexit reg_fixed_voltage_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +#if defined(CONFIG_OF)
> +static const struct of_device_id fixed_of_match[] __devinitconst = {
> +	{ .compatible = "regulator-fixed", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, fixed_of_match);
> +#else
> +#define fixed_of_match NULL
> +#endif
> +
>  static struct platform_driver regulator_fixed_voltage_driver = {
>  	.probe		= reg_fixed_voltage_probe,
>  	.remove		= __devexit_p(reg_fixed_voltage_remove),
>  	.driver		= {
>  		.name		= "reg-fixed-voltage",
>  		.owner		= THIS_MODULE,
> +		.of_match_table = fixed_of_match,
>  	},
>  };
>  
> -- 
> 1.7.1
> 

[PATCH v2 5/5] regulator: map consumer regulator based on device tree

[Rajendra Nayak]

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

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

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

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

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

Also, since the regulator dt nodes can use the same binding to
associate with a parent regulator/supply, allow the drivers to
specify a supply_name, which can then be used to lookup dt
to find the parent phandle.

Signed-off-by: Rajendra Nayak <rnayak@ti.com>
---
 drivers/regulator/core.c         |   91 +++++++++++++++++++++++++++++++------
 include/linux/regulator/driver.h |    2 +
 2 files changed, 78 insertions(+), 15 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index d8e6a42..9a5ebbe 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -25,9 +25,11 @@
 #include <linux/mutex.h>
 #include <linux/suspend.h>
 #include <linux/delay.h>
+#include <linux/of.h>
 #include <linux/regulator/consumer.h>
 #include <linux/regulator/driver.h>
 #include <linux/regulator/machine.h>
+#include <linux/regulator/of_regulator.h>
 
 #define CREATE_TRACE_POINTS
 #include <trace/events/regulator.h>
@@ -131,6 +133,33 @@ static struct regulator *get_device_regulator(struct device *dev)
 	return NULL;
 }
 
+/**
+ * of_get_regulator - get a regulator device node based on supply name
+ * @dev: Device pointer for the consumer (of regulator) device
+ * @supply: regulator supply name
+ *
+ * Extract the regulator device node corresponding to the supply name.
+ * retruns the device node corresponding to the regulator if found, else
+ * returns NULL.
+ */
+struct device_node *of_get_regulator(struct device *dev, const char *supply)
+{
+	struct device_node *regnode = NULL;
+	char prop_name[32]; /* 32 is max size of property name */
+
+	dev_dbg(dev, "Looking up %s-supply from device tree\n", supply);
+
+	snprintf(prop_name, 32, "%s-supply", supply);
+	regnode = of_parse_phandle(dev->of_node, prop_name, 0);
+
+	if (!regnode) {
+		pr_warn("%s: %s property in node %s references invalid phandle",
+			__func__, prop_name, dev->of_node->full_name);
+		return NULL;
+	}
+	return regnode;
+}
+
 /* Platform voltage constraint check */
 static int regulator_check_voltage(struct regulator_dev *rdev,
 				   int *min_uV, int *max_uV)
@@ -1155,6 +1184,7 @@ static struct regulator *_regulator_get(struct device *dev, const char *id,
 	struct regulator_map *map;
 	struct regulator *regulator = ERR_PTR(-ENODEV);
 	const char *devname = NULL;
+	struct device_node *node;
 	int ret;
 
 	if (id == NULL) {
@@ -1167,6 +1197,23 @@ static struct regulator *_regulator_get(struct device *dev, const char *id,
 
 	mutex_lock(&regulator_list_mutex);
 
+	/*
+	 * Lookup happens in 3 ways
+	 * 1. If a dev and dev->of_node exist, look for a
+	 * regulator mapping in dt node.
+	 * 2. Check if a match can be found in regulator_map_list
+	 * if regulator info is still passed in non-dt way
+	 * 3. if !dev, then just look for a matching regulator name.
+	 * Useful for dt builds using regulator_get() without specifying
+	 * a device.
+	 */
+	if (dev && dev->of_node) {
+		node = of_get_regulator(dev, id);
+		if (node)
+			list_for_each_entry(rdev, &regulator_list, list)
+				if (node == rdev->dev.parent->of_node)
+					goto found;
+	}
 	list_for_each_entry(map, &regulator_map_list, list) {
 		/* If the mapping has a device set up it must match */
 		if (map->dev_name &&
@@ -1178,6 +1225,10 @@ static struct regulator *_regulator_get(struct device *dev, const char *id,
 			goto found;
 		}
 	}
+	if (!dev)
+		list_for_each_entry(rdev, &regulator_list, list)
+			if (strcmp(rdev_get_name(rdev), id))
+				goto found;
 
 	if (board_wants_dummy_regulator) {
 		rdev = dummy_regulator_rdev;
@@ -2579,6 +2630,7 @@ struct regulator_dev *regulator_register(struct regulator_desc *regulator_desc,
 	static atomic_t regulator_no = ATOMIC_INIT(0);
 	struct regulator_dev *rdev;
 	int ret, i;
+	const char *supply;
 
 	if (regulator_desc == NULL)
 		return ERR_PTR(-EINVAL);
@@ -2653,25 +2705,34 @@ struct regulator_dev *regulator_register(struct regulator_desc *regulator_desc,
 	if (ret < 0)
 		goto scrub;
 
-	if (init_data->supply_regulator) {
-		struct regulator_dev *r;
-		int found = 0;
+	if (init_data->supply_regulator)
+		supply = init_data->supply_regulator;
+	else if (regulator_desc->supply_name);
+		supply = regulator_desc->supply_name;
 
-		list_for_each_entry(r, &regulator_list, list) {
-			if (strcmp(rdev_get_name(r),
-				   init_data->supply_regulator) == 0) {
-				found = 1;
-				break;
-			}
+	if (supply) {
+		struct regulator_dev *r;
+		struct device_node *node;
+
+		/* first do a dt based lookup */
+		if (dev) {
+			node = of_get_regulator(dev, supply);
+			if (node)
+				list_for_each_entry(r, &regulator_list, list)
+					if (node == r->dev.parent->of_node)
+						goto found;
 		}
 
-		if (!found) {
-			dev_err(dev, "Failed to find supply %s\n",
-				init_data->supply_regulator);
-			ret = -ENODEV;
-			goto scrub;
-		}
+		/* next try doing it non-dt way */
+		list_for_each_entry(r, &regulator_list, list)
+			if (strcmp(rdev_get_name(r), supply) == 0)
+				goto found;
 
+		dev_err(dev, "Failed to find supply %s\n", supply);
+		ret = -ENODEV;
+		goto scrub;
+
+found:
 		ret = set_supply(rdev, r);
 		if (ret < 0)
 			goto scrub;
diff --git a/include/linux/regulator/driver.h b/include/linux/regulator/driver.h
index 1a80bc7..65ca5ff 100644
--- a/include/linux/regulator/driver.h
+++ b/include/linux/regulator/driver.h
@@ -153,6 +153,7 @@ enum regulator_type {
  * this type.
  *
  * @name: Identifying name for the regulator.
+ * @supply_name: Identifying the regulator supply
  * @id: Numerical identifier for the regulator.
  * @n_voltages: Number of selectors available for ops.list_voltage().
  * @ops: Regulator operations table.
@@ -162,6 +163,7 @@ enum regulator_type {
  */
 struct regulator_desc {
 	const char *name;
+	const char *supply_name;
 	int id;
 	unsigned n_voltages;
 	struct regulator_ops *ops;
-- 
1.7.1

[PATCH v2 5/5] regulator: map consumer regulator based on device tree

[Mark Brown]

On Mon, Oct 10, 2011 at 09:49:38PM +0530, Rajendra Nayak wrote:

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

Mostly looks good.

> +/**
> + * of_get_regulator - get a regulator device node based on supply name
> + * @dev: Device pointer for the consumer (of regulator) device
> + * @supply: regulator supply name
> + *
> + * Extract the regulator device node corresponding to the supply name.
> + * retruns the device node corresponding to the regulator if found, else
> + * returns NULL.
> + */
> +struct device_node *of_get_regulator(struct device *dev, const char *supply)
> +{

Should be static.

> @@ -1178,6 +1225,10 @@ static struct regulator *_regulator_get(struct device *dev, const char *id,
>  			goto found;
>  		}
>  	}
> +	if (!dev)
> +		list_for_each_entry(rdev, &regulator_list, list)
> +			if (strcmp(rdev_get_name(rdev), id))
> +				goto found;
>  

This looks really strange, we didn't remove any old lookup code and the
DT lookup jumps to found by iself so why are we adding code here?

> +	if (supply) {
> +		struct regulator_dev *r;
> +		struct device_node *node;
> +
> +		/* first do a dt based lookup */
> +		if (dev) {
> +			node = of_get_regulator(dev, supply);
> +			if (node)
> +				list_for_each_entry(r, &regulator_list, list)
> +					if (node == r->dev.parent->of_node)
> +						goto found;
>  		}
>  
> -		if (!found) {
> -			dev_err(dev, "Failed to find supply %s\n",
> -				init_data->supply_regulator);
> -			ret = -ENODEV;
> -			goto scrub;
> -		}
> +		/* next try doing it non-dt way */
> +		list_for_each_entry(r, &regulator_list, list)
> +			if (strcmp(rdev_get_name(r), supply) == 0)
> +				goto found;
>  
> +		dev_err(dev, "Failed to find supply %s\n", supply);
> +		ret = -ENODEV;
> +		goto scrub;
> +
> +found:

This is all getting *really* complicated and illegible.  I'm not sure
what the best way to structure is but erroring out early looks useful.

[PATCH v2 5/5] regulator: map consumer regulator based on device tree

[Rajendra Nayak]

>
>> @@ -1178,6 +1225,10 @@ static struct regulator *_regulator_get(struct device *dev, const char *id,
>>   			goto found;
>>   		}
>>   	}
>> +	if (!dev)
>> +		list_for_each_entry(rdev,&regulator_list, list)
>> +			if (strcmp(rdev_get_name(rdev), id))
>> +				goto found;
>>
>
> This looks really strange, we didn't remove any old lookup code and the
> DT lookup jumps to found by iself so why are we adding code here?

The old lookup code looks up using the regulator_map_list, and the dt
lookup depends on a dev pointer to extract the of_node.
This above code was needed for cases when a regulator_get() would be
called on dt builds without specifying a device pointer, like the
cpufreq implementations you mentioned.
This is what I tried putting in the comments above the lookup code.

	/*
          * Lookup happens in 3 ways
          * 1. If a dev and dev->of_node exist, look for a
          * regulator mapping in dt node.
          * 2. Check if a match can be found in regulator_map_list
          * if regulator info is still passed in non-dt way
          * 3. if !dev, then just look for a matching regulator name.
          * Useful for dt builds using regulator_get() without specifying
          * a device.
          */

I know its quite complicated but thats because we need to support both
the legacy and the dt based lookups.

>
>> +	if (supply) {
>> +		struct regulator_dev *r;
>> +		struct device_node *node;
>> +
>> +		/* first do a dt based lookup */
>> +		if (dev) {
>> +			node = of_get_regulator(dev, supply);
>> +			if (node)
>> +				list_for_each_entry(r,&regulator_list, list)
>> +					if (node == r->dev.parent->of_node)
>> +						goto found;
>>   		}
>>
>> -		if (!found) {
>> -			dev_err(dev, "Failed to find supply %s\n",
>> -				init_data->supply_regulator);
>> -			ret = -ENODEV;
>> -			goto scrub;
>> -		}
>> +		/* next try doing it non-dt way */
>> +		list_for_each_entry(r,&regulator_list, list)
>> +			if (strcmp(rdev_get_name(r), supply) == 0)
>> +				goto found;
>>
>> +		dev_err(dev, "Failed to find supply %s\n", supply);
>> +		ret = -ENODEV;
>> +		goto scrub;
>> +
>> +found:
>
> This is all getting *really* complicated and illegible.  I'm not sure
> what the best way to structure is but erroring out early looks useful.

[PATCH v2 5/5] regulator: map consumer regulator based on device tree

[Nayak, Rajendra]

>
>> + ? ? if (supply) {
>> + ? ? ? ? ? ? struct regulator_dev *r;
>> + ? ? ? ? ? ? struct device_node *node;
>> +
>> + ? ? ? ? ? ? /* first do a dt based lookup */
>> + ? ? ? ? ? ? if (dev) {
>> + ? ? ? ? ? ? ? ? ? ? node = of_get_regulator(dev, supply);
>> + ? ? ? ? ? ? ? ? ? ? if (node)
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? list_for_each_entry(r, &regulator_list, list)
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? if (node == r->dev.parent->of_node)
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? goto found;
>> ? ? ? ? ? ? ? }
>>
>> - ? ? ? ? ? ? if (!found) {
>> - ? ? ? ? ? ? ? ? ? ? dev_err(dev, "Failed to find supply %s\n",
>> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? init_data->supply_regulator);
>> - ? ? ? ? ? ? ? ? ? ? ret = -ENODEV;
>> - ? ? ? ? ? ? ? ? ? ? goto scrub;
>> - ? ? ? ? ? ? }
>> + ? ? ? ? ? ? /* next try doing it non-dt way */
>> + ? ? ? ? ? ? list_for_each_entry(r, &regulator_list, list)
>> + ? ? ? ? ? ? ? ? ? ? if (strcmp(rdev_get_name(r), supply) == 0)
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? goto found;
>>
>> + ? ? ? ? ? ? dev_err(dev, "Failed to find supply %s\n", supply);
>> + ? ? ? ? ? ? ret = -ENODEV;
>> + ? ? ? ? ? ? goto scrub;
>> +
>> +found:
>
> This is all getting *really* complicated and illegible. ?I'm not sure
> what the best way to structure is but erroring out early looks useful.
>

How about this updated patch?

[PATCH v2 5/5] regulator: map consumer regulator based on device tree

[Mark Brown]

On Tue, Oct 11, 2011 at 12:38:53PM +0530, Nayak, Rajendra wrote:

> How about this updated patch?

Yes, I think that looks broadly sensible.

[PATCH v2 5/5] regulator: map consumer regulator based on device tree

[Grant Likely]

On Tue, Oct 11, 2011 at 12:38:53PM +0530, Nayak, Rajendra wrote:
> >
> >> + ? ? if (supply) {
> >> + ? ? ? ? ? ? struct regulator_dev *r;
> >> + ? ? ? ? ? ? struct device_node *node;
> >> +
> >> + ? ? ? ? ? ? /* first do a dt based lookup */
> >> + ? ? ? ? ? ? if (dev) {
> >> + ? ? ? ? ? ? ? ? ? ? node = of_get_regulator(dev, supply);
> >> + ? ? ? ? ? ? ? ? ? ? if (node)
> >> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? list_for_each_entry(r, &regulator_list, list)
> >> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? if (node == r->dev.parent->of_node)
> >> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? goto found;
> >> ? ? ? ? ? ? ? }
> >>
> >> - ? ? ? ? ? ? if (!found) {
> >> - ? ? ? ? ? ? ? ? ? ? dev_err(dev, "Failed to find supply %s\n",
> >> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? init_data->supply_regulator);
> >> - ? ? ? ? ? ? ? ? ? ? ret = -ENODEV;
> >> - ? ? ? ? ? ? ? ? ? ? goto scrub;
> >> - ? ? ? ? ? ? }
> >> + ? ? ? ? ? ? /* next try doing it non-dt way */
> >> + ? ? ? ? ? ? list_for_each_entry(r, &regulator_list, list)
> >> + ? ? ? ? ? ? ? ? ? ? if (strcmp(rdev_get_name(r), supply) == 0)
> >> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? goto found;
> >>
> >> + ? ? ? ? ? ? dev_err(dev, "Failed to find supply %s\n", supply);
> >> + ? ? ? ? ? ? ret = -ENODEV;
> >> + ? ? ? ? ? ? goto scrub;
> >> +
> >> +found:
> >
> > This is all getting *really* complicated and illegible. ?I'm not sure
> > what the best way to structure is but erroring out early looks useful.
> >
> 
> How about this updated patch?
> 
> From f4c7d2b283740382c272c7971665375067d26dbe Mon Sep 17 00:00:00 2001
> From: Rajendra Nayak <rnayak@ti.com>
> Date: Thu, 22 Sep 2011 17:51:10 +0530
> Subject: [PATCH 5/5] regulator: map consumer regulator based on device tree
> 
> Device nodes in DT can associate themselves with one or more
> regulators/supply by providing a list of phandles (to regulator nodes)
> and corresponding supply names.
> 
> For Example:
> 	devicenode: node at 0x0 {
> 		...
> 		...
> 		vmmc-supply = <&regulator1>;
> 		vpll-supply = <&regulator1>;
> 	};
> 
> The driver would then do a regulator_get(dev, "vmmc"); to get
> regulator1 and do a regulator_get(dev, "vpll"); to get
> regulator2.
> 
> of_get_regulator() extracts the regulator node for a given
> device, based on the supply name.
> 
> Use it to look up the regulator for a given consumer from device tree, during
> a regulator_get(). If not found fallback and lookup through
> the regulator_map_list instead.
> 
> Also, since the regulator dt nodes can use the same binding to
> associate with a parent regulator/supply, allow the drivers to
> specify a supply_name, which can then be used to lookup dt
> to find the parent phandle.
> 
> Signed-off-by: Rajendra Nayak <rnayak@ti.com>

Acked-by: Grant Likely <grant.likely@secretlab.ca>

> ---
>  drivers/regulator/core.c         |   91 +++++++++++++++++++++++++++++++------
>  include/linux/regulator/driver.h |    2 +
>  2 files changed, 78 insertions(+), 15 deletions(-)
> 
> Index: linux/drivers/regulator/core.c
> ===================================================================
> --- linux.orig/drivers/regulator/core.c	2011-10-11 11:43:27.944140106 +0530
> +++ linux/drivers/regulator/core.c	2011-10-11 12:25:53.540418748 +0530
> @@ -25,9 +25,11 @@
>  #include <linux/mutex.h>
>  #include <linux/suspend.h>
>  #include <linux/delay.h>
> +#include <linux/of.h>
>  #include <linux/regulator/consumer.h>
>  #include <linux/regulator/driver.h>
>  #include <linux/regulator/machine.h>
> +#include <linux/regulator/of_regulator.h>
> 
>  #define CREATE_TRACE_POINTS
>  #include <trace/events/regulator.h>
> @@ -131,6 +133,33 @@
>  	return NULL;
>  }
> 
> +/**
> + * of_get_regulator - get a regulator device node based on supply name
> + * @dev: Device pointer for the consumer (of regulator) device
> + * @supply: regulator supply name
> + *
> + * Extract the regulator device node corresponding to the supply name.
> + * retruns the device node corresponding to the regulator if found, else
> + * returns NULL.
> + */
> +static struct device_node *of_get_regulator(struct device *dev, const
> char *supply)
> +{
> +	struct device_node *regnode = NULL;
> +	char prop_name[32]; /* 32 is max size of property name */
> +
> +	dev_dbg(dev, "Looking up %s-supply from device tree\n", supply);
> +
> +	snprintf(prop_name, 32, "%s-supply", supply);
> +	regnode = of_parse_phandle(dev->of_node, prop_name, 0);
> +
> +	if (!regnode) {
> +		pr_warn("%s: %s property in node %s references invalid phandle",
> +			__func__, prop_name, dev->of_node->full_name);
> +		return NULL;
> +	}
> +	return regnode;
> +}
> +
>  /* Platform voltage constraint check */
>  static int regulator_check_voltage(struct regulator_dev *rdev,
>  				   int *min_uV, int *max_uV)
> @@ -1147,6 +1176,28 @@
>  	return rdev->desc->ops->enable_time(rdev);
>  }
> 
> +static struct regulator_dev *regulator_dev_lookup(struct device *dev,
> +							 const char *supply)
> +{
> +	struct regulator_dev *r;
> +	struct device_node *node;
> +
> +	/* first do a dt based lookup */
> +	if (dev && dev->of_node)
> +		node = of_get_regulator(dev, supply);
> +		if (node)
> +			list_for_each_entry(r, &regulator_list, list)
> +				if (node == r->dev.parent->of_node)
> +					return r;
> +
> +	/* if not found, try doing it non-dt way */
> +	list_for_each_entry(r, &regulator_list, list)
> +		if (strcmp(rdev_get_name(r), supply) == 0)
> +			return r;
> +
> +	return NULL;
> +}
> +
>  /* Internal regulator request function */
>  static struct regulator *_regulator_get(struct device *dev, const char *id,
>  					int exclusive)
> @@ -1155,6 +1206,7 @@
>  	struct regulator_map *map;
>  	struct regulator *regulator = ERR_PTR(-ENODEV);
>  	const char *devname = NULL;
> +	struct device_node *node;
>  	int ret;
> 
>  	if (id == NULL) {
> @@ -1167,6 +1219,10 @@
> 
>  	mutex_lock(&regulator_list_mutex);
> 
> +	rdev = regulator_dev_lookup(dev, id);
> +	if (rdev)
> +		goto found;
> +
>  	list_for_each_entry(map, &regulator_map_list, list) {
>  		/* If the mapping has a device set up it must match */
>  		if (map->dev_name &&
> @@ -2579,6 +2635,7 @@
>  	static atomic_t regulator_no = ATOMIC_INIT(0);
>  	struct regulator_dev *rdev;
>  	int ret, i;
> +	const char *supply;
> 
>  	if (regulator_desc == NULL)
>  		return ERR_PTR(-EINVAL);
> @@ -2653,21 +2710,18 @@
>  	if (ret < 0)
>  		goto scrub;
> 
> -	if (init_data->supply_regulator) {
> +	if (init_data->supply_regulator)
> +		supply = init_data->supply_regulator;
> +	else if (regulator_desc->supply_name);
> +		supply = regulator_desc->supply_name;
> +
> +	if (supply) {
>  		struct regulator_dev *r;
> -		int found = 0;
> 
> -		list_for_each_entry(r, &regulator_list, list) {
> -			if (strcmp(rdev_get_name(r),
> -				   init_data->supply_regulator) == 0) {
> -				found = 1;
> -				break;
> -			}
> -		}
> +		r = regulator_dev_lookup(dev, supply);
> 
> -		if (!found) {
> -			dev_err(dev, "Failed to find supply %s\n",
> -				init_data->supply_regulator);
> +		if (!r) {
> +			dev_err(dev, "Failed to find supply %s\n", supply);
>  			ret = -ENODEV;
>  			goto scrub;
>  		}
> Index: linux/include/linux/regulator/driver.h
> ===================================================================
> --- linux.orig/include/linux/regulator/driver.h	2011-10-11
> 11:43:27.936139476 +0530
> +++ linux/include/linux/regulator/driver.h	2011-10-11 11:43:43.485351850 +0530
> @@ -153,6 +153,7 @@
>   * this type.
>   *
>   * @name: Identifying name for the regulator.
> + * @supply_name: Identifying the regulator supply
>   * @id: Numerical identifier for the regulator.
>   * @n_voltages: Number of selectors available for ops.list_voltage().
>   * @ops: Regulator operations table.
> @@ -162,6 +163,7 @@
>   */
>  struct regulator_desc {
>  	const char *name;
> +	const char *supply_name;
>  	int id;
>  	unsigned n_voltages;
>  	struct regulator_ops *ops;

[PATCH v2 5/5] regulator: map consumer regulator based on device tree

[Shawn Guo]

On Mon, Oct 10, 2011 at 09:49:38PM +0530, Rajendra Nayak wrote:
> Device nodes in DT can associate themselves with one or more
> regulators/supply by providing a list of phandles (to regulator nodes)
> and corresponding supply names.
> 
> For Example:
> 	devicenode: node at 0x0 {
> 		...
> 		...
> 		vmmc-supply = <&regulator1>;
> 		vpll-supply = <&regulator1>;

s/regulator1/regulator2

> 	};
> 
> The driver would then do a regulator_get(dev, "vmmc"); to get
> regulator1 and do a regulator_get(dev, "vpll"); to get
> regulator2.
> 
> of_get_regulator() extracts the regulator node for a given
> device, based on the supply name.
> 
> Use it to look up the regulator for a given consumer from device tree, during
> a regulator_get(). If not found fallback and lookup through
> the regulator_map_list instead.
> 
> Also, since the regulator dt nodes can use the same binding to
> associate with a parent regulator/supply, allow the drivers to
> specify a supply_name, which can then be used to lookup dt
> to find the parent phandle.
> 
> Signed-off-by: Rajendra Nayak <rnayak@ti.com>
> ---
>  drivers/regulator/core.c         |   91 +++++++++++++++++++++++++++++++------
>  include/linux/regulator/driver.h |    2 +
>  2 files changed, 78 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index d8e6a42..9a5ebbe 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -25,9 +25,11 @@
>  #include <linux/mutex.h>
>  #include <linux/suspend.h>
>  #include <linux/delay.h>
> +#include <linux/of.h>
>  #include <linux/regulator/consumer.h>
>  #include <linux/regulator/driver.h>
>  #include <linux/regulator/machine.h>
> +#include <linux/regulator/of_regulator.h>
>  
>  #define CREATE_TRACE_POINTS
>  #include <trace/events/regulator.h>
> @@ -131,6 +133,33 @@ static struct regulator *get_device_regulator(struct device *dev)
>  	return NULL;
>  }
>  
> +/**
> + * of_get_regulator - get a regulator device node based on supply name
> + * @dev: Device pointer for the consumer (of regulator) device
> + * @supply: regulator supply name
> + *
> + * Extract the regulator device node corresponding to the supply name.
> + * retruns the device node corresponding to the regulator if found, else
> + * returns NULL.
> + */
> +struct device_node *of_get_regulator(struct device *dev, const char *supply)
> +{
> +	struct device_node *regnode = NULL;
> +	char prop_name[32]; /* 32 is max size of property name */
> +
> +	dev_dbg(dev, "Looking up %s-supply from device tree\n", supply);
> +
> +	snprintf(prop_name, 32, "%s-supply", supply);
> +	regnode = of_parse_phandle(dev->of_node, prop_name, 0);
> +
> +	if (!regnode) {
> +		pr_warn("%s: %s property in node %s references invalid phandle",
> +			__func__, prop_name, dev->of_node->full_name);
> +		return NULL;
> +	}
> +	return regnode;
> +}
> +
>  /* Platform voltage constraint check */
>  static int regulator_check_voltage(struct regulator_dev *rdev,
>  				   int *min_uV, int *max_uV)
> @@ -1155,6 +1184,7 @@ static struct regulator *_regulator_get(struct device *dev, const char *id,
>  	struct regulator_map *map;
>  	struct regulator *regulator = ERR_PTR(-ENODEV);
>  	const char *devname = NULL;
> +	struct device_node *node;
>  	int ret;
>  
>  	if (id == NULL) {
> @@ -1167,6 +1197,23 @@ static struct regulator *_regulator_get(struct device *dev, const char *id,
>  
>  	mutex_lock(&regulator_list_mutex);
>  
> +	/*
> +	 * Lookup happens in 3 ways
> +	 * 1. If a dev and dev->of_node exist, look for a
> +	 * regulator mapping in dt node.
> +	 * 2. Check if a match can be found in regulator_map_list
> +	 * if regulator info is still passed in non-dt way
> +	 * 3. if !dev, then just look for a matching regulator name.
> +	 * Useful for dt builds using regulator_get() without specifying
> +	 * a device.
> +	 */
> +	if (dev && dev->of_node) {
> +		node = of_get_regulator(dev, id);
> +		if (node)
> +			list_for_each_entry(rdev, &regulator_list, list)
> +				if (node == rdev->dev.parent->of_node)

If we move DT parsing procedure into regulator core, we will be able to
find the of_node for given regulator and attach it to rdev->dev when
regulator_register() gets called by regulator drivers.  Then we will
have 'if (node == rdev->dev.of_node)' here which makes more sense to me.

Regards,
Shawn

> +					goto found;
> +	}
>  	list_for_each_entry(map, &regulator_map_list, list) {
>  		/* If the mapping has a device set up it must match */
>  		if (map->dev_name &&
> @@ -1178,6 +1225,10 @@ static struct regulator *_regulator_get(struct device *dev, const char *id,
>  			goto found;
>  		}
>  	}
> +	if (!dev)
> +		list_for_each_entry(rdev, &regulator_list, list)
> +			if (strcmp(rdev_get_name(rdev), id))
> +				goto found;
>  
>  	if (board_wants_dummy_regulator) {
>  		rdev = dummy_regulator_rdev;
> @@ -2579,6 +2630,7 @@ struct regulator_dev *regulator_register(struct regulator_desc *regulator_desc,
>  	static atomic_t regulator_no = ATOMIC_INIT(0);
>  	struct regulator_dev *rdev;
>  	int ret, i;
> +	const char *supply;
>  
>  	if (regulator_desc == NULL)
>  		return ERR_PTR(-EINVAL);
> @@ -2653,25 +2705,34 @@ struct regulator_dev *regulator_register(struct regulator_desc *regulator_desc,
>  	if (ret < 0)
>  		goto scrub;
>  
> -	if (init_data->supply_regulator) {
> -		struct regulator_dev *r;
> -		int found = 0;
> +	if (init_data->supply_regulator)
> +		supply = init_data->supply_regulator;
> +	else if (regulator_desc->supply_name);
> +		supply = regulator_desc->supply_name;
>  
> -		list_for_each_entry(r, &regulator_list, list) {
> -			if (strcmp(rdev_get_name(r),
> -				   init_data->supply_regulator) == 0) {
> -				found = 1;
> -				break;
> -			}
> +	if (supply) {
> +		struct regulator_dev *r;
> +		struct device_node *node;
> +
> +		/* first do a dt based lookup */
> +		if (dev) {
> +			node = of_get_regulator(dev, supply);
> +			if (node)
> +				list_for_each_entry(r, &regulator_list, list)
> +					if (node == r->dev.parent->of_node)
> +						goto found;
>  		}
>  
> -		if (!found) {
> -			dev_err(dev, "Failed to find supply %s\n",
> -				init_data->supply_regulator);
> -			ret = -ENODEV;
> -			goto scrub;
> -		}
> +		/* next try doing it non-dt way */
> +		list_for_each_entry(r, &regulator_list, list)
> +			if (strcmp(rdev_get_name(r), supply) == 0)
> +				goto found;
>  
> +		dev_err(dev, "Failed to find supply %s\n", supply);
> +		ret = -ENODEV;
> +		goto scrub;
> +
> +found:
>  		ret = set_supply(rdev, r);
>  		if (ret < 0)
>  			goto scrub;
> diff --git a/include/linux/regulator/driver.h b/include/linux/regulator/driver.h
> index 1a80bc7..65ca5ff 100644
> --- a/include/linux/regulator/driver.h
> +++ b/include/linux/regulator/driver.h
> @@ -153,6 +153,7 @@ enum regulator_type {
>   * this type.
>   *
>   * @name: Identifying name for the regulator.
> + * @supply_name: Identifying the regulator supply
>   * @id: Numerical identifier for the regulator.
>   * @n_voltages: Number of selectors available for ops.list_voltage().
>   * @ops: Regulator operations table.
> @@ -162,6 +163,7 @@ enum regulator_type {
>   */
>  struct regulator_desc {
>  	const char *name;
> +	const char *supply_name;
>  	int id;
>  	unsigned n_voltages;
>  	struct regulator_ops *ops;
> -- 
> 1.7.1
> 
> _______________________________________________
> devicetree-discuss mailing list
> devicetree-discuss at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss
>