[8/9] regulator: helper to extract regulator node based on supply name

Message ID 1317118372-17052-9-git-send-email-rnayak@ti.com
State New
Headers show

Commit Message

Rajendra Nayak Sept. 27, 2011, 10:12 a.m.
Device nodes in DT can associate themselves with one or more
regulators by providing a list of phandles (to regulator nodes)
and corresponding supply names.

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

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

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

Signed-off-by: Rajendra Nayak <rnayak@ti.com>
---
 drivers/regulator/of_regulator.c       |   39 ++++++++++++++++++++++++++++++++
 include/linux/regulator/of_regulator.h |    7 +++++
 2 files changed, 46 insertions(+), 0 deletions(-)

Comments

Mark Brown Sept. 27, 2011, 12:21 p.m. | #1
On Tue, Sep 27, 2011 at 03:42:51PM +0530, Rajendra Nayak wrote:

> +	if (!dev)
> +		return NULL;

So how do we handle CPUs?  cpufreq is one of the most active users of
regulators...

> +	snprintf(prop_name, 32, "%s-supply", supply);
> +
> +	prop = of_get_property(dev->of_node, prop_name, &sz);
> +	if (!prop || sz < 4)
> +		return NULL;

sz < 4?  Magic!  :)

> +extern struct device_node *of_get_regulator(struct device *dev,
> +	const char *supply);

This shouldn't be part of the public API, it should be transparently
handled within the core.
Rajendra Nayak Sept. 27, 2011, 2:49 p.m. | #2
On Tuesday 27 September 2011 05:51 PM, Mark Brown wrote:
> On Tue, Sep 27, 2011 at 03:42:51PM +0530, Rajendra Nayak wrote:
>
>> +	if (!dev)
>> +		return NULL;
>
> So how do we handle CPUs?  cpufreq is one of the most active users of
> regulators...

Hmm, never thought of it :(
Maybe I should associate a supply name with all
regulators and then lookup from the global registered
list.

>
>> +	snprintf(prop_name, 32, "%s-supply", supply);
>> +
>> +	prop = of_get_property(dev->of_node, prop_name,&sz);
>> +	if (!prop || sz<  4)
>> +		return NULL;
>
> sz<  4?  Magic!  :)

Its the valid phandle size.
I guess I need a sz != 4

>
>> +extern struct device_node *of_get_regulator(struct device *dev,
>> +	const char *supply);
>
> This shouldn't be part of the public API, it should be transparently
> handled within the core.

agreed.
Mark Brown Sept. 27, 2011, 6:59 p.m. | #3
On Tue, Sep 27, 2011 at 08:19:37PM +0530, Rajendra Nayak wrote:
> On Tuesday 27 September 2011 05:51 PM, Mark Brown wrote:
> >On Tue, Sep 27, 2011 at 03:42:51PM +0530, Rajendra Nayak wrote:

> >>+	if (!dev)
> >>+		return NULL;

> >So how do we handle CPUs?  cpufreq is one of the most active users of
> >regulators...

> Hmm, never thought of it :(
> Maybe I should associate a supply name with all
> regulators and then lookup from the global registered
> list.

I'm not sure how this should work in a device tree world, I'd *hope*
we'd get a device tree node for the CPU and could then just make this a
regular consumer thing but then the cpufreq drivers would need to be
updated to make use of it.  The only reason we allow null devices right
now is the fact that cpufreq doesn't have a struct device it can use.

> >>+	snprintf(prop_name, 32, "%s-supply", supply);
> >>+
> >>+	prop = of_get_property(dev->of_node, prop_name,&sz);
> >>+	if (!prop || sz<  4)
> >>+		return NULL;

> >sz<  4?  Magic!  :)

> Its the valid phandle size.
> I guess I need a sz != 4

I think we need an of_get_phandle(), it'd be clearer what the check is,
more type safe and would avoid needing to replicate the check.
Cousson, Benoit Sept. 28, 2011, 8:09 a.m. | #4
On 9/27/2011 8:59 PM, Mark Brown wrote:
> On Tue, Sep 27, 2011 at 08:19:37PM +0530, Rajendra Nayak wrote:
>> On Tuesday 27 September 2011 05:51 PM, Mark Brown wrote:
>>> On Tue, Sep 27, 2011 at 03:42:51PM +0530, Rajendra Nayak wrote:
>
>>>> +	if (!dev)
>>>> +		return NULL;
>
>>> So how do we handle CPUs?  cpufreq is one of the most active users of
>>> regulators...
>
>> Hmm, never thought of it :(
>> Maybe I should associate a supply name with all
>> regulators and then lookup from the global registered
>> list.
>
> I'm not sure how this should work in a device tree world, I'd *hope*
> we'd get a device tree node for the CPU and could then just make this a
> regular consumer thing but then the cpufreq drivers would need to be
> updated to make use of it.  The only reason we allow null devices right
> now is the fact that cpufreq doesn't have a struct device it can use.

That's why we do have a MPU node in OMAP dts, in order to build an 
omap_device that will be mainly used for the DVFS on the MPU.

And even before DT migration, we used to build statically some 
omap_device to represent the various processors in the system (MPU, DSP, 
CortexM3...).

Regards,
Benoit
Rajendra Nayak Sept. 28, 2011, 8:18 a.m. | #5
On Wednesday 28 September 2011 01:39 PM, Cousson, Benoit wrote:
> On 9/27/2011 8:59 PM, Mark Brown wrote:
>> On Tue, Sep 27, 2011 at 08:19:37PM +0530, Rajendra Nayak wrote:
>>> On Tuesday 27 September 2011 05:51 PM, Mark Brown wrote:
>>>> On Tue, Sep 27, 2011 at 03:42:51PM +0530, Rajendra Nayak wrote:
>>
>>>>> + if (!dev)
>>>>> + return NULL;
>>
>>>> So how do we handle CPUs? cpufreq is one of the most active users of
>>>> regulators...
>>
>>> Hmm, never thought of it :(
>>> Maybe I should associate a supply name with all
>>> regulators and then lookup from the global registered
>>> list.
>>
>> I'm not sure how this should work in a device tree world, I'd *hope*
>> we'd get a device tree node for the CPU and could then just make this a
>> regular consumer thing but then the cpufreq drivers would need to be
>> updated to make use of it. The only reason we allow null devices right
>> now is the fact that cpufreq doesn't have a struct device it can use.
>
> That's why we do have a MPU node in OMAP dts, in order to build an
> omap_device that will be mainly used for the DVFS on the MPU.
>
> And even before DT migration, we used to build statically some
> omap_device to represent the various processors in the system (MPU, DSP,
> CortexM3...).

yes, but clearly not everyone seems to do this. and then there are
also these instances of board files requesting regulators without
associating them with any device :(

>
> Regards,
> Benoit
Rajendra Nayak Sept. 28, 2011, 10:56 a.m. | #6
On Wednesday 28 September 2011 12:29 AM, Mark Brown wrote:
> On Tue, Sep 27, 2011 at 08:19:37PM +0530, Rajendra Nayak wrote:
>> On Tuesday 27 September 2011 05:51 PM, Mark Brown wrote:
>>> On Tue, Sep 27, 2011 at 03:42:51PM +0530, Rajendra Nayak wrote:
>
>>>> +	if (!dev)
>>>> +		return NULL;
>
>>> So how do we handle CPUs?  cpufreq is one of the most active users of
>>> regulators...
>
>> Hmm, never thought of it :(
>> Maybe I should associate a supply name with all
>> regulators and then lookup from the global registered
>> list.
>
> I'm not sure how this should work in a device tree world, I'd *hope*
> we'd get a device tree node for the CPU and could then just make this a
> regular consumer thing but then the cpufreq drivers would need to be
> updated to make use of it.  The only reason we allow null devices right
> now is the fact that cpufreq doesn't have a struct device it can use.
>
>>>> +	snprintf(prop_name, 32, "%s-supply", supply);
>>>> +
>>>> +	prop = of_get_property(dev->of_node, prop_name,&sz);
>>>> +	if (!prop || sz<   4)
>>>> +		return NULL;
>
>>> sz<   4?  Magic!  :)
>
>> Its the valid phandle size.
>> I guess I need a sz != 4
>
> I think we need an of_get_phandle(), it'd be clearer what the check is,
> more type safe and would avoid needing to replicate the check.

Yes, there already seems to be one, of_parse_phandle() which I should
have used. thanks.
Mark Brown Sept. 28, 2011, 12:26 p.m. | #7
On Wed, Sep 28, 2011 at 10:09:30AM +0200, Cousson, Benoit wrote:
> On 9/27/2011 8:59 PM, Mark Brown wrote:

> >I'm not sure how this should work in a device tree world, I'd *hope*
> >we'd get a device tree node for the CPU and could then just make this a
> >regular consumer thing but then the cpufreq drivers would need to be
> >updated to make use of it.  The only reason we allow null devices right
> >now is the fact that cpufreq doesn't have a struct device it can use.

> That's why we do have a MPU node in OMAP dts, in order to build an
> omap_device that will be mainly used for the DVFS on the MPU.

> And even before DT migration, we used to build statically some
> omap_device to represent the various processors in the system (MPU,
> DSP, CortexM3...).

Yeah, but that's very OMAP specific - we don't have that in general (in
fact it's the only Linux platform I'm aware of that has a device for the
CPU).
Grant Likely Sept. 30, 2011, 1:36 a.m. | #8
On Tue, Sep 27, 2011 at 03:42:51PM +0530, Rajendra Nayak wrote:
> Device nodes in DT can associate themselves with one or more
> regulators by providing a list of phandles (to regulator nodes)
> and corresponding supply names.
> 
> For Example:
> 	devicenode: node@0x0 {
> 		...
> 		...
> 		vmmc-supply = <&regulator1>;
> 		vpll-supply = <&regulator1>;
> 	};
> 
> The driver would then do a regulator_get(dev, "vmmc"); to get
> regulator1 and do a regulator_get(dev, "vpll"); to get
> regulator2.
> 
> of_get_regulator() extracts the regulator node for a given
> device, based on the supply name.
> 
> Signed-off-by: Rajendra Nayak <rnayak@ti.com>
> ---
>  drivers/regulator/of_regulator.c       |   39 ++++++++++++++++++++++++++++++++
>  include/linux/regulator/of_regulator.h |    7 +++++
>  2 files changed, 46 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/regulator/of_regulator.c b/drivers/regulator/of_regulator.c
> index 7fa63ff..49dd105 100644
> --- a/drivers/regulator/of_regulator.c
> +++ b/drivers/regulator/of_regulator.c
> @@ -14,6 +14,45 @@
>  #include <linux/of.h>
>  #include <linux/regulator/machine.h>
>  
> +
> +/**
> + * of_get_regulator - get a regulator device node based on supply name
> + * @dev: Device pointer for the consumer (of regulator) device
> + * @supply: regulator supply name
> + *
> + * Extract the regulator device node corresponding to the supply name.
> + * retruns the device node corresponding to the regulator if found, else
> + * returns NULL.
> + */
> +struct device_node *of_get_regulator(struct device *dev, const char *supply)
> +{
> +	struct device_node *regnode = NULL;
> +	u32 reghandle;
> +	char prop_name[32]; /* 32 is max size of property name */
> +	const void *prop;
> +	int sz;
> +
> +	if (!dev)
> +		return NULL;
> +
> +	dev_dbg(dev, "Looking up %s-supply from device tree\n", supply);
> +
> +	snprintf(prop_name, 32, "%s-supply", supply);
> +
> +	prop = of_get_property(dev->of_node, prop_name, &sz);
> +	if (!prop || sz < 4)
> +		return NULL;
> +
> +	reghandle = be32_to_cpup(prop);
> +	regnode = of_find_node_by_phandle(reghandle);

of_parse_phandle()

> +	if (!regnode) {
> +		pr_warn("%s: %s property in node %s references invalid phandle",
> +			__func__, prop_name, dev->of_node->full_name);
> +		return NULL;
> +	}
> +	return regnode;
> +}
> +
>  static void of_get_regulation_constraints(struct device_node *np,
>  					struct regulator_init_data **init_data)
>  {
> diff --git a/include/linux/regulator/of_regulator.h b/include/linux/regulator/of_regulator.h
> index 3f63be9..edaba1a 100644
> --- a/include/linux/regulator/of_regulator.h
> +++ b/include/linux/regulator/of_regulator.h
> @@ -9,12 +9,19 @@
>  #if defined(CONFIG_OF_REGULATOR)
>  extern struct regulator_init_data
>  	*of_get_regulator_init_data(struct device *dev);
> +extern struct device_node *of_get_regulator(struct device *dev,
> +	const char *supply);
>  #else
>  static inline struct regulator_init_data
>  	*of_get_regulator_init_data(struct device_node *np)
>  {
>  	return NULL;
>  }
> +static inline struct device_node *of_get_regulator(struct device *dev,
> +	const char *id)
> +{
> +	return NULL;
> +}
>  #endif /* CONFIG_OF_REGULATOR */
>  
>  #endif /* __LINUX_OF_REG_H */
> -- 
> 1.7.1
>
Rajendra Nayak Sept. 30, 2011, 9:34 a.m. | #9
On Wednesday 28 September 2011 05:56 PM, Mark Brown wrote:
> On Wed, Sep 28, 2011 at 10:09:30AM +0200, Cousson, Benoit wrote:
>> On 9/27/2011 8:59 PM, Mark Brown wrote:
>
>>> I'm not sure how this should work in a device tree world, I'd *hope*
>>> we'd get a device tree node for the CPU and could then just make this a
>>> regular consumer thing but then the cpufreq drivers would need to be
>>> updated to make use of it.  The only reason we allow null devices right
>>> now is the fact that cpufreq doesn't have a struct device it can use.
>
>> That's why we do have a MPU node in OMAP dts, in order to build an
>> omap_device that will be mainly used for the DVFS on the MPU.
>
>> And even before DT migration, we used to build statically some
>> omap_device to represent the various processors in the system (MPU,
>> DSP, CortexM3...).
>
> Yeah, but that's very OMAP specific - we don't have that in general (in
> fact it's the only Linux platform I'm aware of that has a device for the
> CPU).

But isn't this the right thing to do for everyone else too?
Mark Brown Sept. 30, 2011, 10:35 a.m. | #10
On Fri, Sep 30, 2011 at 03:04:45PM +0530, Rajendra Nayak wrote:
> On Wednesday 28 September 2011 05:56 PM, Mark Brown wrote:
> >On Wed, Sep 28, 2011 at 10:09:30AM +0200, Cousson, Benoit wrote:

> >>And even before DT migration, we used to build statically some
> >>omap_device to represent the various processors in the system (MPU,
> >>DSP, CortexM3...).

> >Yeah, but that's very OMAP specific - we don't have that in general (in
> >fact it's the only Linux platform I'm aware of that has a device for the
> >CPU).

> But isn't this the right thing to do for everyone else too?

That doesn't really matter so long as nobody else is actually doing it;
you can't make a decision like this in an OMAP-specific fashion, you
need to make sure everyone else is on board with the decision and make
sure we've got at least at a high level way of representing the CPUs and
SoCs in the device tree that people can buy into.
Grant Likely Oct. 4, 2011, 5 p.m. | #11
On Fri, Sep 30, 2011 at 03:04:45PM +0530, Rajendra Nayak wrote:
> On Wednesday 28 September 2011 05:56 PM, Mark Brown wrote:
> >On Wed, Sep 28, 2011 at 10:09:30AM +0200, Cousson, Benoit wrote:
> >>On 9/27/2011 8:59 PM, Mark Brown wrote:
> >
> >>>I'm not sure how this should work in a device tree world, I'd *hope*
> >>>we'd get a device tree node for the CPU and could then just make this a
> >>>regular consumer thing but then the cpufreq drivers would need to be
> >>>updated to make use of it.  The only reason we allow null devices right
> >>>now is the fact that cpufreq doesn't have a struct device it can use.
> >
> >>That's why we do have a MPU node in OMAP dts, in order to build an
> >>omap_device that will be mainly used for the DVFS on the MPU.
> >
> >>And even before DT migration, we used to build statically some
> >>omap_device to represent the various processors in the system (MPU,
> >>DSP, CortexM3...).
> >
> >Yeah, but that's very OMAP specific - we don't have that in general (in
> >fact it's the only Linux platform I'm aware of that has a device for the
> >CPU).
> 
> But isn't this the right thing to do for everyone else too?
> 

It is normal to have nodes for each CPU.  The /cpus/ node normally
contains cpu@* nodes for each logical cpu core, and I would expect
nodes for each additional DSP and MPU core.  Whether or not they
belong in the /cpus/ node is a matter of design (we don't have any
patterns for that yet).

g.

Patch hide | download patch | download mbox

diff --git a/drivers/regulator/of_regulator.c b/drivers/regulator/of_regulator.c
index 7fa63ff..49dd105 100644
--- a/drivers/regulator/of_regulator.c
+++ b/drivers/regulator/of_regulator.c
@@ -14,6 +14,45 @@ 
 #include <linux/of.h>
 #include <linux/regulator/machine.h>
 
+
+/**
+ * of_get_regulator - get a regulator device node based on supply name
+ * @dev: Device pointer for the consumer (of regulator) device
+ * @supply: regulator supply name
+ *
+ * Extract the regulator device node corresponding to the supply name.
+ * retruns the device node corresponding to the regulator if found, else
+ * returns NULL.
+ */
+struct device_node *of_get_regulator(struct device *dev, const char *supply)
+{
+	struct device_node *regnode = NULL;
+	u32 reghandle;
+	char prop_name[32]; /* 32 is max size of property name */
+	const void *prop;
+	int sz;
+
+	if (!dev)
+		return NULL;
+
+	dev_dbg(dev, "Looking up %s-supply from device tree\n", supply);
+
+	snprintf(prop_name, 32, "%s-supply", supply);
+
+	prop = of_get_property(dev->of_node, prop_name, &sz);
+	if (!prop || sz < 4)
+		return NULL;
+
+	reghandle = be32_to_cpup(prop);
+	regnode = of_find_node_by_phandle(reghandle);
+	if (!regnode) {
+		pr_warn("%s: %s property in node %s references invalid phandle",
+			__func__, prop_name, dev->of_node->full_name);
+		return NULL;
+	}
+	return regnode;
+}
+
 static void of_get_regulation_constraints(struct device_node *np,
 					struct regulator_init_data **init_data)
 {
diff --git a/include/linux/regulator/of_regulator.h b/include/linux/regulator/of_regulator.h
index 3f63be9..edaba1a 100644
--- a/include/linux/regulator/of_regulator.h
+++ b/include/linux/regulator/of_regulator.h
@@ -9,12 +9,19 @@ 
 #if defined(CONFIG_OF_REGULATOR)
 extern struct regulator_init_data
 	*of_get_regulator_init_data(struct device *dev);
+extern struct device_node *of_get_regulator(struct device *dev,
+	const char *supply);
 #else
 static inline struct regulator_init_data
 	*of_get_regulator_init_data(struct device_node *np)
 {
 	return NULL;
 }
+static inline struct device_node *of_get_regulator(struct device *dev,
+	const char *id)
+{
+	return NULL;
+}
 #endif /* CONFIG_OF_REGULATOR */
 
 #endif /* __LINUX_OF_REG_H */