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);
- 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 ?