| Message ID | 1317118372-17052-9-git-send-email-rnayak@ti.com |
|---|---|
| State | New |
| Headers | show |
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.
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.
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.
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
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
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.
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).
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 = <®ulator1>; > vpll-supply = <®ulator1>; > }; > > 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 >
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?
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.
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.
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 */
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 = <®ulator1>; vpll-supply = <®ulator1>; }; 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(-)