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

Message ID 1318263578-7407-4-git-send-email-rnayak@ti.com
State New
Headers show

Commit Message

Rajendra Nayak Oct. 10, 2011, 4:19 p.m.
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

Comments

Mark Brown Oct. 10, 2011, 5:22 p.m. | #1
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.
Rajendra Nayak Oct. 11, 2011, 5:59 a.m. | #2
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.
Grant Likely Oct. 13, 2011, 6:38 p.m. | #3
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.
Grant Likely Oct. 13, 2011, 6:40 p.m. | #4
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@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@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
>
Mark Brown Oct. 13, 2011, 10:12 p.m. | #5
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.
Shawn Guo Oct. 16, 2011, 2:55 p.m. | #6
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@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@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 */
};
Rajendra Nayak Oct. 17, 2011, 4:17 a.m. | #7
> 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 */
> };
>
Shawn Guo Oct. 18, 2011, 11:58 a.m. | #8
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?
Shawn Guo Oct. 18, 2011, 1:20 p.m. | #9
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@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@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@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss
>
Mark Brown Oct. 18, 2011, 4 p.m. | #10
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.
Shawn Guo Oct. 19, 2011, 5:33 a.m. | #11
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?
Rajendra Nayak Oct. 19, 2011, 11:35 a.m. | #12
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@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@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@lists.ozlabs.org
>> https://lists.ozlabs.org/listinfo/devicetree-discuss
>>
>
Shawn Guo Oct. 19, 2011, 2:42 p.m. | #13
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@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@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@lists.ozlabs.org
> >>https://lists.ozlabs.org/listinfo/devicetree-discuss
> >>
> >
> 
>
Mark Brown Oct. 19, 2011, 2:47 p.m. | #14
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.
Mark Brown Oct. 19, 2011, 2:50 p.m. | #15
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.
Shawn Guo Oct. 19, 2011, 3:04 p.m. | #16
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'?
Mark Brown Oct. 19, 2011, 3:10 p.m. | #17
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.
Rajendra Nayak Oct. 20, 2011, 3:42 a.m. | #18
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.
Rajendra Nayak Oct. 20, 2011, 5:18 a.m. | #19
>
> 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?
Shawn Guo Oct. 20, 2011, 6:14 a.m. | #20
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.
Mark Brown Oct. 20, 2011, 9:41 a.m. | #21
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.
Rajendra Nayak Oct. 20, 2011, 12:09 p.m. | #22
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?
>
Rajendra Nayak Oct. 20, 2011, 12:10 p.m. | #23
>
> 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.
Tony Lindgren Oct. 20, 2011, 4:27 p.m. | #24
* 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
Mark Brown Oct. 20, 2011, 4:40 p.m. | #25
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.
Tony Lindgren Oct. 20, 2011, 5:05 p.m. | #26
* 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
Tony Lindgren Oct. 20, 2011, 5:22 p.m. | #27
* 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
Mark Brown Oct. 20, 2011, 7:56 p.m. | #28
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.
Mark Brown Oct. 20, 2011, 7:57 p.m. | #29
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?
Tony Lindgren Oct. 20, 2011, 8:10 p.m. | #30
* 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
Mark Brown Oct. 20, 2011, 9:42 p.m. | #31
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.
Tony Lindgren Oct. 20, 2011, 10:09 p.m. | #32
* 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
Grant Likely Oct. 24, 2011, 9:07 a.m. | #33
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 hide | download patch | download mbox

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@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@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 */
+