Re: [PATCH V2 5/8] PM / OPP: Add infrastructure to manage multiple regulators

[Viresh Kumar <viresh.kumar@linaro.org>]

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