Messages in this thread | | | Date | Wed, 26 Oct 2016 11:35:12 +0530 | From | Viresh Kumar <> | Subject | Re: [PATCH V2 5/8] PM / OPP: Add infrastructure to manage multiple regulators |
| |
On 25-10-16, 09:49, Stephen Boyd wrote: > On 10/20, Viresh Kumar wrote: > > diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c > > index 37fad2eb0f47..45c70ce07864 100644 > > --- a/drivers/base/power/opp/core.c > > +++ b/drivers/base/power/opp/core.c > > @@ -235,21 +237,44 @@ unsigned long dev_pm_opp_get_max_volt_latency(struct device *dev) > > return 0; > > } > > > > - reg = opp_table->regulator; > > - if (IS_ERR(reg)) { > > + count = opp_table->regulator_count; > > + > > + if (!count) { > > /* Regulator may not be required for device */ > > rcu_read_unlock(); > > return 0; > > } > > > > - list_for_each_entry_rcu(opp, &opp_table->opp_list, node) { > > - if (!opp->available) > > - continue; > > + size = count * sizeof(*regulators); > > + regulators = kmemdup(opp_table->regulators, size, GFP_KERNEL); > > How do we allocate under RCU? Doesn't that spit out big warnings?
That doesn't. I even tried enabling several locking debug config options.
> > + if (!regulators) { > > + rcu_read_unlock(); > > + return 0; > > + } > > + > > + min_uV = kmalloc(count * (sizeof(*min_uV) + sizeof(*max_uV)), > > Do we imagine min_uV is going to be a different size from max_uV? > It may be better to have a struct for min/max pair and then > stride them. Then the kmalloc() can become a kmalloc_array().
done.
> > - *opp_table = _add_opp_table(dev); > > - if (!*opp_table) { > > - kfree(opp); > > + /* allocate new OPP node + and supplies structures */ > > + opp = kzalloc(sizeof(*opp) + supply_size, GFP_KERNEL); > > + if (!opp) { > > + kfree(table); > > return NULL; > > } > > > > + opp->supplies = (struct dev_pm_opp_supply *)(opp + 1); > > So put the supplies at the end of the OPP structure as an empty > array?
Yes. Added a comment to clarify as well.
> > -int dev_pm_opp_set_regulator(struct device *dev, const char *name) > > +int dev_pm_opp_set_regulators(struct device *dev, const char *names[], > > Make names a const char * const *?
Done.
> I seem to recall arrays as > function arguments has some problem but my C mastery is failing > right now.
I am not sure why it would be a problem, and of course what gets passed is the address and not the array.
> > + for (i = 0; i < count; i++) { > > + reg = regulator_get_optional(dev, names[i]); > > + pr_info("%s: %d: %p: %s\n", __func__, __LINE__, reg, names[i]); > > Debug noise?
Yes.
> > +static bool opp_debug_create_supplies(struct dev_pm_opp *opp, > > + struct opp_table *opp_table, > > + struct dentry *pdentry) > > +{ > > + struct dentry *d; > > + int i = 0; > > + char name[] = "supply-X"; /* support only 0-9 supplies */ > > But we don't verify that's the case? Why not use kasprintf() and > free() and then there isn't any limit?
Done.
> > diff --git a/drivers/base/power/opp/of.c b/drivers/base/power/opp/of.c > > index b7fcd0a1b58b..c857fb07a5bc 100644 > > --- a/drivers/base/power/opp/of.c > > +++ b/drivers/base/power/opp/of.c > > @@ -105,12 +106,13 @@ static bool _opp_is_supported(struct device *dev, struct opp_table *opp_table, > > static int opp_parse_supplies(struct dev_pm_opp *opp, struct device *dev, > > struct opp_table *opp_table) > > { > > - u32 microvolt[3] = {0}; > > - u32 val; > > - int count, ret; > > + u32 *microvolt, *microamp = NULL; > > + int supplies, vcount, icount, ret, i, j; > > struct property *prop = NULL; > > char name[NAME_MAX]; > > > > + supplies = opp_table->regulator_count ? opp_table->regulator_count : 1; > > We can't have regulator_count == 1 by default?
It is used at various places to distinguish if regulators are set by platform code or not. The OPP core can still be used just for DT data, i.e. no opp-set. And so it is important to support cases where the regulators aren't set.
> > @@ -155,7 +155,8 @@ enum opp_table_access { > > * @supported_hw_count: Number of elements in supported_hw array. > > * @prop_name: A name to postfix to many DT properties, while parsing them. > > * @clk: Device's clock handle > > - * @regulator: Supply regulator > > + * @regulators: Supply regulators > > + * @regulator_count: Number of Power Supply regulators > > Lowercase Power Supply please.
Done.
> > * @dentry: debugfs dentry pointer of the real device directory (not links). > > * @dentry_name: Name of the real dentry. > > * > > diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c > > index 5c07ae05d69a..15cb26118dc7 100644 > > --- a/drivers/cpufreq/cpufreq-dt.c > > +++ b/drivers/cpufreq/cpufreq-dt.c > > @@ -186,7 +186,10 @@ static int cpufreq_init(struct cpufreq_policy *policy) > > */ > > name = find_supply_name(cpu_dev); > > if (name) { > > - ret = dev_pm_opp_set_regulator(cpu_dev, name); > > + const char *names[] = {name}; > > + > > + ret = dev_pm_opp_set_regulators(cpu_dev, names, > > We can't just do &names instead here? Hmm...
I don't think so. You sure we can do it ?
-- viresh
| |