| Message ID | 1318263578-7407-6-git-send-email-rnayak@ti.com |
|---|---|
| State | New |
| Headers | show |
On Mon, Oct 10, 2011 at 09:49:38PM +0530, Rajendra Nayak wrote: > Device nodes in DT can associate themselves with one or more > regulators/supply by providing a list of phandles (to regulator nodes) > and corresponding supply names. Mostly looks good. > +/** > + * of_get_regulator - get a regulator device node based on supply name > + * @dev: Device pointer for the consumer (of regulator) device > + * @supply: regulator supply name > + * > + * Extract the regulator device node corresponding to the supply name. > + * retruns the device node corresponding to the regulator if found, else > + * returns NULL. > + */ > +struct device_node *of_get_regulator(struct device *dev, const char *supply) > +{ Should be static. > @@ -1178,6 +1225,10 @@ static struct regulator *_regulator_get(struct device *dev, const char *id, > goto found; > } > } > + if (!dev) > + list_for_each_entry(rdev, ®ulator_list, list) > + if (strcmp(rdev_get_name(rdev), id)) > + goto found; > This looks really strange, we didn't remove any old lookup code and the DT lookup jumps to found by iself so why are we adding code here? > + if (supply) { > + struct regulator_dev *r; > + struct device_node *node; > + > + /* first do a dt based lookup */ > + if (dev) { > + node = of_get_regulator(dev, supply); > + if (node) > + list_for_each_entry(r, ®ulator_list, list) > + if (node == r->dev.parent->of_node) > + goto found; > } > > - if (!found) { > - dev_err(dev, "Failed to find supply %s\n", > - init_data->supply_regulator); > - ret = -ENODEV; > - goto scrub; > - } > + /* next try doing it non-dt way */ > + list_for_each_entry(r, ®ulator_list, list) > + if (strcmp(rdev_get_name(r), supply) == 0) > + goto found; > > + dev_err(dev, "Failed to find supply %s\n", supply); > + ret = -ENODEV; > + goto scrub; > + > +found: This is all getting *really* complicated and illegible. I'm not sure what the best way to structure is but erroring out early looks useful.
> >> @@ -1178,6 +1225,10 @@ static struct regulator *_regulator_get(struct device *dev, const char *id, >> goto found; >> } >> } >> + if (!dev) >> + list_for_each_entry(rdev,®ulator_list, list) >> + if (strcmp(rdev_get_name(rdev), id)) >> + goto found; >> > > This looks really strange, we didn't remove any old lookup code and the > DT lookup jumps to found by iself so why are we adding code here? The old lookup code looks up using the regulator_map_list, and the dt lookup depends on a dev pointer to extract the of_node. This above code was needed for cases when a regulator_get() would be called on dt builds without specifying a device pointer, like the cpufreq implementations you mentioned. This is what I tried putting in the comments above the lookup code. /* * Lookup happens in 3 ways * 1. If a dev and dev->of_node exist, look for a * regulator mapping in dt node. * 2. Check if a match can be found in regulator_map_list * if regulator info is still passed in non-dt way * 3. if !dev, then just look for a matching regulator name. * Useful for dt builds using regulator_get() without specifying * a device. */ I know its quite complicated but thats because we need to support both the legacy and the dt based lookups. > >> + if (supply) { >> + struct regulator_dev *r; >> + struct device_node *node; >> + >> + /* first do a dt based lookup */ >> + if (dev) { >> + node = of_get_regulator(dev, supply); >> + if (node) >> + list_for_each_entry(r,®ulator_list, list) >> + if (node == r->dev.parent->of_node) >> + goto found; >> } >> >> - if (!found) { >> - dev_err(dev, "Failed to find supply %s\n", >> - init_data->supply_regulator); >> - ret = -ENODEV; >> - goto scrub; >> - } >> + /* next try doing it non-dt way */ >> + list_for_each_entry(r,®ulator_list, list) >> + if (strcmp(rdev_get_name(r), supply) == 0) >> + goto found; >> >> + dev_err(dev, "Failed to find supply %s\n", supply); >> + ret = -ENODEV; >> + goto scrub; >> + >> +found: > > This is all getting *really* complicated and illegible. I'm not sure > what the best way to structure is but erroring out early looks useful.
On Mon, Oct 10, 2011 at 09:49:38PM +0530, Rajendra Nayak wrote: > Device nodes in DT can associate themselves with one or more > regulators/supply by providing a list of phandles (to regulator nodes) > and corresponding supply names. > > For Example: > devicenode: node@0x0 { > ... > ... > vmmc-supply = <®ulator1>; > vpll-supply = <®ulator1>; s/regulator1/regulator2 > }; > > The driver would then do a regulator_get(dev, "vmmc"); to get > regulator1 and do a regulator_get(dev, "vpll"); to get > regulator2. > > of_get_regulator() extracts the regulator node for a given > device, based on the supply name. > > Use it to look up the regulator for a given consumer from device tree, during > a regulator_get(). If not found fallback and lookup through > the regulator_map_list instead. > > Also, since the regulator dt nodes can use the same binding to > associate with a parent regulator/supply, allow the drivers to > specify a supply_name, which can then be used to lookup dt > to find the parent phandle. > > Signed-off-by: Rajendra Nayak <rnayak@ti.com> > --- > drivers/regulator/core.c | 91 +++++++++++++++++++++++++++++++------ > include/linux/regulator/driver.h | 2 + > 2 files changed, 78 insertions(+), 15 deletions(-) > > diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c > index d8e6a42..9a5ebbe 100644 > --- a/drivers/regulator/core.c > +++ b/drivers/regulator/core.c > @@ -25,9 +25,11 @@ > #include <linux/mutex.h> > #include <linux/suspend.h> > #include <linux/delay.h> > +#include <linux/of.h> > #include <linux/regulator/consumer.h> > #include <linux/regulator/driver.h> > #include <linux/regulator/machine.h> > +#include <linux/regulator/of_regulator.h> > > #define CREATE_TRACE_POINTS > #include <trace/events/regulator.h> > @@ -131,6 +133,33 @@ static struct regulator *get_device_regulator(struct device *dev) > return NULL; > } > > +/** > + * of_get_regulator - get a regulator device node based on supply name > + * @dev: Device pointer for the consumer (of regulator) device > + * @supply: regulator supply name > + * > + * Extract the regulator device node corresponding to the supply name. > + * retruns the device node corresponding to the regulator if found, else > + * returns NULL. > + */ > +struct device_node *of_get_regulator(struct device *dev, const char *supply) > +{ > + struct device_node *regnode = NULL; > + char prop_name[32]; /* 32 is max size of property name */ > + > + dev_dbg(dev, "Looking up %s-supply from device tree\n", supply); > + > + snprintf(prop_name, 32, "%s-supply", supply); > + regnode = of_parse_phandle(dev->of_node, prop_name, 0); > + > + if (!regnode) { > + pr_warn("%s: %s property in node %s references invalid phandle", > + __func__, prop_name, dev->of_node->full_name); > + return NULL; > + } > + return regnode; > +} > + > /* Platform voltage constraint check */ > static int regulator_check_voltage(struct regulator_dev *rdev, > int *min_uV, int *max_uV) > @@ -1155,6 +1184,7 @@ static struct regulator *_regulator_get(struct device *dev, const char *id, > struct regulator_map *map; > struct regulator *regulator = ERR_PTR(-ENODEV); > const char *devname = NULL; > + struct device_node *node; > int ret; > > if (id == NULL) { > @@ -1167,6 +1197,23 @@ static struct regulator *_regulator_get(struct device *dev, const char *id, > > mutex_lock(®ulator_list_mutex); > > + /* > + * Lookup happens in 3 ways > + * 1. If a dev and dev->of_node exist, look for a > + * regulator mapping in dt node. > + * 2. Check if a match can be found in regulator_map_list > + * if regulator info is still passed in non-dt way > + * 3. if !dev, then just look for a matching regulator name. > + * Useful for dt builds using regulator_get() without specifying > + * a device. > + */ > + if (dev && dev->of_node) { > + node = of_get_regulator(dev, id); > + if (node) > + list_for_each_entry(rdev, ®ulator_list, list) > + if (node == rdev->dev.parent->of_node) If we move DT parsing procedure into regulator core, we will be able to find the of_node for given regulator and attach it to rdev->dev when regulator_register() gets called by regulator drivers. Then we will have 'if (node == rdev->dev.of_node)' here which makes more sense to me. Regards, Shawn > + goto found; > + } > list_for_each_entry(map, ®ulator_map_list, list) { > /* If the mapping has a device set up it must match */ > if (map->dev_name && > @@ -1178,6 +1225,10 @@ static struct regulator *_regulator_get(struct device *dev, const char *id, > goto found; > } > } > + if (!dev) > + list_for_each_entry(rdev, ®ulator_list, list) > + if (strcmp(rdev_get_name(rdev), id)) > + goto found; > > if (board_wants_dummy_regulator) { > rdev = dummy_regulator_rdev; > @@ -2579,6 +2630,7 @@ struct regulator_dev *regulator_register(struct regulator_desc *regulator_desc, > static atomic_t regulator_no = ATOMIC_INIT(0); > struct regulator_dev *rdev; > int ret, i; > + const char *supply; > > if (regulator_desc == NULL) > return ERR_PTR(-EINVAL); > @@ -2653,25 +2705,34 @@ struct regulator_dev *regulator_register(struct regulator_desc *regulator_desc, > if (ret < 0) > goto scrub; > > - if (init_data->supply_regulator) { > - struct regulator_dev *r; > - int found = 0; > + if (init_data->supply_regulator) > + supply = init_data->supply_regulator; > + else if (regulator_desc->supply_name); > + supply = regulator_desc->supply_name; > > - list_for_each_entry(r, ®ulator_list, list) { > - if (strcmp(rdev_get_name(r), > - init_data->supply_regulator) == 0) { > - found = 1; > - break; > - } > + if (supply) { > + struct regulator_dev *r; > + struct device_node *node; > + > + /* first do a dt based lookup */ > + if (dev) { > + node = of_get_regulator(dev, supply); > + if (node) > + list_for_each_entry(r, ®ulator_list, list) > + if (node == r->dev.parent->of_node) > + goto found; > } > > - if (!found) { > - dev_err(dev, "Failed to find supply %s\n", > - init_data->supply_regulator); > - ret = -ENODEV; > - goto scrub; > - } > + /* next try doing it non-dt way */ > + list_for_each_entry(r, ®ulator_list, list) > + if (strcmp(rdev_get_name(r), supply) == 0) > + goto found; > > + dev_err(dev, "Failed to find supply %s\n", supply); > + ret = -ENODEV; > + goto scrub; > + > +found: > ret = set_supply(rdev, r); > if (ret < 0) > goto scrub; > diff --git a/include/linux/regulator/driver.h b/include/linux/regulator/driver.h > index 1a80bc7..65ca5ff 100644 > --- a/include/linux/regulator/driver.h > +++ b/include/linux/regulator/driver.h > @@ -153,6 +153,7 @@ enum regulator_type { > * this type. > * > * @name: Identifying name for the regulator. > + * @supply_name: Identifying the regulator supply > * @id: Numerical identifier for the regulator. > * @n_voltages: Number of selectors available for ops.list_voltage(). > * @ops: Regulator operations table. > @@ -162,6 +163,7 @@ enum regulator_type { > */ > struct regulator_desc { > const char *name; > + const char *supply_name; > int id; > unsigned n_voltages; > struct regulator_ops *ops; > -- > 1.7.1 > > _______________________________________________ > devicetree-discuss mailing list > devicetree-discuss@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/devicetree-discuss >
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c index d8e6a42..9a5ebbe 100644 --- a/drivers/regulator/core.c +++ b/drivers/regulator/core.c @@ -25,9 +25,11 @@ #include <linux/mutex.h> #include <linux/suspend.h> #include <linux/delay.h> +#include <linux/of.h> #include <linux/regulator/consumer.h> #include <linux/regulator/driver.h> #include <linux/regulator/machine.h> +#include <linux/regulator/of_regulator.h> #define CREATE_TRACE_POINTS #include <trace/events/regulator.h> @@ -131,6 +133,33 @@ static struct regulator *get_device_regulator(struct device *dev) return NULL; } +/** + * of_get_regulator - get a regulator device node based on supply name + * @dev: Device pointer for the consumer (of regulator) device + * @supply: regulator supply name + * + * Extract the regulator device node corresponding to the supply name. + * retruns the device node corresponding to the regulator if found, else + * returns NULL. + */ +struct device_node *of_get_regulator(struct device *dev, const char *supply) +{ + struct device_node *regnode = NULL; + char prop_name[32]; /* 32 is max size of property name */ + + dev_dbg(dev, "Looking up %s-supply from device tree\n", supply); + + snprintf(prop_name, 32, "%s-supply", supply); + regnode = of_parse_phandle(dev->of_node, prop_name, 0); + + if (!regnode) { + pr_warn("%s: %s property in node %s references invalid phandle", + __func__, prop_name, dev->of_node->full_name); + return NULL; + } + return regnode; +} + /* Platform voltage constraint check */ static int regulator_check_voltage(struct regulator_dev *rdev, int *min_uV, int *max_uV) @@ -1155,6 +1184,7 @@ static struct regulator *_regulator_get(struct device *dev, const char *id, struct regulator_map *map; struct regulator *regulator = ERR_PTR(-ENODEV); const char *devname = NULL; + struct device_node *node; int ret; if (id == NULL) { @@ -1167,6 +1197,23 @@ static struct regulator *_regulator_get(struct device *dev, const char *id, mutex_lock(®ulator_list_mutex); + /* + * Lookup happens in 3 ways + * 1. If a dev and dev->of_node exist, look for a + * regulator mapping in dt node. + * 2. Check if a match can be found in regulator_map_list + * if regulator info is still passed in non-dt way + * 3. if !dev, then just look for a matching regulator name. + * Useful for dt builds using regulator_get() without specifying + * a device. + */ + if (dev && dev->of_node) { + node = of_get_regulator(dev, id); + if (node) + list_for_each_entry(rdev, ®ulator_list, list) + if (node == rdev->dev.parent->of_node) + goto found; + } list_for_each_entry(map, ®ulator_map_list, list) { /* If the mapping has a device set up it must match */ if (map->dev_name && @@ -1178,6 +1225,10 @@ static struct regulator *_regulator_get(struct device *dev, const char *id, goto found; } } + if (!dev) + list_for_each_entry(rdev, ®ulator_list, list) + if (strcmp(rdev_get_name(rdev), id)) + goto found; if (board_wants_dummy_regulator) { rdev = dummy_regulator_rdev; @@ -2579,6 +2630,7 @@ struct regulator_dev *regulator_register(struct regulator_desc *regulator_desc, static atomic_t regulator_no = ATOMIC_INIT(0); struct regulator_dev *rdev; int ret, i; + const char *supply; if (regulator_desc == NULL) return ERR_PTR(-EINVAL); @@ -2653,25 +2705,34 @@ struct regulator_dev *regulator_register(struct regulator_desc *regulator_desc, if (ret < 0) goto scrub; - if (init_data->supply_regulator) { - struct regulator_dev *r; - int found = 0; + if (init_data->supply_regulator) + supply = init_data->supply_regulator; + else if (regulator_desc->supply_name); + supply = regulator_desc->supply_name; - list_for_each_entry(r, ®ulator_list, list) { - if (strcmp(rdev_get_name(r), - init_data->supply_regulator) == 0) { - found = 1; - break; - } + if (supply) { + struct regulator_dev *r; + struct device_node *node; + + /* first do a dt based lookup */ + if (dev) { + node = of_get_regulator(dev, supply); + if (node) + list_for_each_entry(r, ®ulator_list, list) + if (node == r->dev.parent->of_node) + goto found; } - if (!found) { - dev_err(dev, "Failed to find supply %s\n", - init_data->supply_regulator); - ret = -ENODEV; - goto scrub; - } + /* next try doing it non-dt way */ + list_for_each_entry(r, ®ulator_list, list) + if (strcmp(rdev_get_name(r), supply) == 0) + goto found; + dev_err(dev, "Failed to find supply %s\n", supply); + ret = -ENODEV; + goto scrub; + +found: ret = set_supply(rdev, r); if (ret < 0) goto scrub; diff --git a/include/linux/regulator/driver.h b/include/linux/regulator/driver.h index 1a80bc7..65ca5ff 100644 --- a/include/linux/regulator/driver.h +++ b/include/linux/regulator/driver.h @@ -153,6 +153,7 @@ enum regulator_type { * this type. * * @name: Identifying name for the regulator. + * @supply_name: Identifying the regulator supply * @id: Numerical identifier for the regulator. * @n_voltages: Number of selectors available for ops.list_voltage(). * @ops: Regulator operations table. @@ -162,6 +163,7 @@ enum regulator_type { */ struct regulator_desc { const char *name; + const char *supply_name; int id; unsigned n_voltages; struct regulator_ops *ops;
Device nodes in DT can associate themselves with one or more regulators/supply by providing a list of phandles (to regulator nodes) and corresponding supply names. For Example: devicenode: node@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. Use it to look up the regulator for a given consumer from device tree, during a regulator_get(). If not found fallback and lookup through the regulator_map_list instead. Also, since the regulator dt nodes can use the same binding to associate with a parent regulator/supply, allow the drivers to specify a supply_name, which can then be used to lookup dt to find the parent phandle. Signed-off-by: Rajendra Nayak <rnayak@ti.com> --- drivers/regulator/core.c | 91 +++++++++++++++++++++++++++++++------ include/linux/regulator/driver.h | 2 + 2 files changed, 78 insertions(+), 15 deletions(-)