[prev in list] [next in list] [prev in thread] [next in thread] 

List:       linaro-kernel
Subject:    Re: [PATCH V6 06/10] PM / OPP: Add infrastructure to manage multiple regulators
From:       Stephen Boyd <sboyd () codeaurora ! org>
Date:       2016-11-30 22:37:12
Message-ID: 20161130223712.GM6095 () codeaurora ! org
[Download RAW message or body]

On 11/30, Viresh Kumar wrote:
> @@ -1337,26 +1408,44 @@ struct opp_table *dev_pm_opp_set_regulator(struct device \
> *dev, const char *name)  goto err;
> 	}
> 
> -	/* Already have a regulator set */
> -	if (WARN_ON(!IS_ERR(opp_table->regulator))) {
> +	/* Already have regulators set */
> +	if (WARN_ON(opp_table->regulators)) {
> 		opp_table = ERR_PTR(-EBUSY);
> 		goto err;
> 	}
> -	/* Allocate the regulator */
> -	reg = regulator_get_optional(dev, name);
> -	if (IS_ERR(reg)) {
> -		opp_table = (struct opp_table *)reg;
> -		if (PTR_ERR(reg) != -EPROBE_DEFER)
> -			dev_err(dev, "%s: no regulator (%s) found: %ld\n",
> -				__func__, name, PTR_ERR(reg));
> +
> +	opp_table->regulators = kmalloc_array(count,
> +					      sizeof(*opp_table->regulators),
> +					      GFP_KERNEL);
> +	if (!opp_table->regulators) {
> +		opp_table = ERR_PTR(-ENOMEM);
> 		goto err;
> 	}
> 
> -	opp_table->regulator = reg;
> +	for (i = 0; i < count; i++) {
> +		reg = regulator_get_optional(dev, names[i]);
> +		if (IS_ERR(reg)) {
> +			opp_table = (struct opp_table *)reg;

This should be an ERR_CAST() instead. But this is not based on
the correct patch anyway so this should be rebased.

> +			if (PTR_ERR(reg) != -EPROBE_DEFER)
> +				dev_err(dev, "%s: no regulator (%s) found: %ld\n",
> +					__func__, names[i], PTR_ERR(reg));
> +			goto free_regulators;
> +		}
> +
> +		opp_table->regulators[i] = reg;
> +	}
> +
> +	opp_table->regulator_count = count;
> 
> 	mutex_unlock(&opp_table_lock);
> 	return opp_table;
> 
> +free_regulators:
> +	while (i != 0)
> +		regulator_put(opp_table->regulators[--i]);
> +
> +	kfree(opp_table->regulators);
> +	opp_table->regulators = NULL;
> err:
> 	_remove_opp_table(opp_table);
> unlock:
> @@ -1364,10 +1453,10 @@ struct opp_table *dev_pm_opp_set_regulator(struct device \
> *dev, const char *name) 
> 	return opp_table;
> }
> -EXPORT_SYMBOL_GPL(dev_pm_opp_set_regulator);
> +EXPORT_SYMBOL_GPL(dev_pm_opp_set_regulators);
> 
> /**
> - * dev_pm_opp_put_regulator() - Releases resources blocked for regulator
> + * dev_pm_opp_put_regulators() - Releases resources blocked for regulators
> * @opp_table: opp_table returned from dev_pm_opp_set_regulator

This needs an update too                  ^

> *
> * Locking: The internal opp_table and opp structures are RCU protected.
> diff --git a/drivers/base/power/opp/debugfs.c b/drivers/base/power/opp/debugfs.c
> index c897676ca35f..95f433db4ac7 100644
> --- a/drivers/base/power/opp/debugfs.c
> +++ b/drivers/base/power/opp/debugfs.c
> @@ -34,6 +35,46 @@ void opp_debug_remove_one(struct dev_pm_opp *opp)
> 	debugfs_remove_recursive(opp->dentry);
> }
> 
> +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;
> +
> +	/* Always create at least supply-0 directory */
> +	do {
> +		name = kasprintf(GFP_KERNEL, "supply-%d", i);

Sorry I haven't noticed this before. Wouldn't we want to put the
supply name here instead of a number?

> +
> +		/* Create per-opp directory */
> +		d = debugfs_create_dir(name, pdentry);
> +
> +		kfree(name);
> diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c
> index 4d3ec92cbabf..1cd1fcfe835a 100644
> --- a/drivers/cpufreq/cpufreq-dt.c
> +++ b/drivers/cpufreq/cpufreq-dt.c
> @@ -188,7 +188,10 @@ static int cpufreq_init(struct cpufreq_policy *policy)
> 	 */
> 	name = find_supply_name(cpu_dev);
> 	if (name) {
> -		opp_table = dev_pm_opp_set_regulator(cpu_dev, name);
> +		const char *names[] = {name};
> +
> +		opp_table = dev_pm_opp_set_regulators(cpu_dev, names,
> +						ARRAY_SIZE(names));

This can be simplified to: 

		opp_table = dev_pm_opp_set_regulators(cpu_dev, &name, 1);

> 		if (IS_ERR(opp_table)) {
> 			ret = PTR_ERR(opp_table);
> 			dev_err(cpu_dev, "Failed to set regulator for cpu%d: %d\n",

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
_______________________________________________
linaro-kernel mailing list
linaro-kernel@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/linaro-kernel


[prev in list] [next in list] [prev in thread] [next in thread]