From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755160AbcJLVaE (ORCPT ); Wed, 12 Oct 2016 17:30:04 -0400 Received: from bear.ext.ti.com ([198.47.19.11]:35201 "EHLO bear.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752586AbcJLVaD (ORCPT ); Wed, 12 Oct 2016 17:30:03 -0400 Subject: Re: [PATCH 3/8] PM / OPP: Manage supply's voltage/current in a separate structure To: Viresh Kumar , Rafael Wysocki , , , Viresh Kumar References: CC: , , , Vincent Guittot , , From: Dave Gerlach Message-ID: <57FEA7F2.3070604@ti.com> Date: Wed, 12 Oct 2016 16:15:30 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.6.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit X-Originating-IP: [128.247.83.19] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi, On 10/04/2016 06:56 AM, Viresh Kumar wrote: > This is a preparatory step for multiple regulator per device support. > Move the voltage/current variables to a new structure. > > Signed-off-by: Viresh Kumar > --- > drivers/base/power/opp/core.c | 44 +++++++++++++++++++++------------------- > drivers/base/power/opp/debugfs.c | 8 ++++---- > drivers/base/power/opp/of.c | 18 ++++++++-------- > drivers/base/power/opp/opp.h | 27 ++++++++++++++++-------- > 4 files changed, 55 insertions(+), 42 deletions(-) > > diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c > index 056527a3fb4e..8d6006151c9a 100644 > --- a/drivers/base/power/opp/core.c > +++ b/drivers/base/power/opp/core.c > @@ -112,7 +112,7 @@ unsigned long dev_pm_opp_get_voltage(struct dev_pm_opp *opp) > if (IS_ERR_OR_NULL(tmp_opp)) > pr_err("%s: Invalid parameters\n", __func__); > else > - v = tmp_opp->u_volt; > + v = tmp_opp->supply.u_volt; > > return v; > } > @@ -246,10 +246,10 @@ unsigned long dev_pm_opp_get_max_volt_latency(struct device *dev) > if (!opp->available) > continue; > > - if (opp->u_volt_min < min_uV) > - min_uV = opp->u_volt_min; > - if (opp->u_volt_max > max_uV) > - max_uV = opp->u_volt_max; > + if (opp->supply.u_volt_min < min_uV) > + min_uV = opp->supply.u_volt_min; > + if (opp->supply.u_volt_max > max_uV) > + max_uV = opp->supply.u_volt_max; > } > > rcu_read_unlock(); > @@ -637,14 +637,14 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq) > if (IS_ERR(old_opp)) { > old_u_volt = 0; > } else { > - old_u_volt = old_opp->u_volt; > - old_u_volt_min = old_opp->u_volt_min; > - old_u_volt_max = old_opp->u_volt_max; > + old_u_volt = old_opp->supply.u_volt; > + old_u_volt_min = old_opp->supply.u_volt_min; > + old_u_volt_max = old_opp->supply.u_volt_max; > } > > - u_volt = opp->u_volt; > - u_volt_min = opp->u_volt_min; > - u_volt_max = opp->u_volt_max; > + u_volt = opp->supply.u_volt; > + u_volt_min = opp->supply.u_volt_min; > + u_volt_max = opp->supply.u_volt_max; > > reg = opp_table->regulator; > > @@ -957,10 +957,11 @@ static bool _opp_supported_by_regulators(struct dev_pm_opp *opp, > struct regulator *reg = opp_table->regulator; > > if (!IS_ERR(reg) && > - !regulator_is_supported_voltage(reg, opp->u_volt_min, > - opp->u_volt_max)) { > + !regulator_is_supported_voltage(reg, opp->supply.u_volt_min, > + opp->supply.u_volt_max)) { > pr_warn("%s: OPP minuV: %lu maxuV: %lu, not supported by regulator\n", > - __func__, opp->u_volt_min, opp->u_volt_max); > + __func__, opp->supply.u_volt_min, > + opp->supply.u_volt_max); > return false; > } > > @@ -993,11 +994,12 @@ int _opp_add(struct device *dev, struct dev_pm_opp *new_opp, > > /* Duplicate OPPs */ > dev_warn(dev, "%s: duplicate OPPs detected. Existing: freq: %lu, volt: %lu, enabled: %d. New: freq: %lu, volt: %lu, enabled: %d\n", > - __func__, opp->rate, opp->u_volt, opp->available, > - new_opp->rate, new_opp->u_volt, new_opp->available); > + __func__, opp->rate, opp->supply.u_volt, > + opp->available, new_opp->rate, new_opp->supply.u_volt, > + new_opp->available); > > - return opp->available && new_opp->u_volt == opp->u_volt ? > - 0 : -EEXIST; > + return opp->available && > + new_opp->supply.u_volt == opp->supply.u_volt ? 0 : -EEXIST; > } > > new_opp->opp_table = opp_table; > @@ -1064,9 +1066,9 @@ int _opp_add_v1(struct device *dev, unsigned long freq, long u_volt, > /* populate the opp table */ > new_opp->rate = freq; > tol = u_volt * opp_table->voltage_tolerance_v1 / 100; > - new_opp->u_volt = u_volt; > - new_opp->u_volt_min = u_volt - tol; > - new_opp->u_volt_max = u_volt + tol; > + new_opp->supply.u_volt = u_volt; > + new_opp->supply.u_volt_min = u_volt - tol; > + new_opp->supply.u_volt_max = u_volt + tol; > new_opp->available = true; > new_opp->dynamic = dynamic; > > diff --git a/drivers/base/power/opp/debugfs.c b/drivers/base/power/opp/debugfs.c > index ef1ae6b52042..c897676ca35f 100644 > --- a/drivers/base/power/opp/debugfs.c > +++ b/drivers/base/power/opp/debugfs.c > @@ -63,16 +63,16 @@ int opp_debug_create_one(struct dev_pm_opp *opp, struct opp_table *opp_table) > if (!debugfs_create_ulong("rate_hz", S_IRUGO, d, &opp->rate)) > return -ENOMEM; > > - if (!debugfs_create_ulong("u_volt_target", S_IRUGO, d, &opp->u_volt)) > + if (!debugfs_create_ulong("u_volt_target", S_IRUGO, d, &opp->supply.u_volt)) > return -ENOMEM; > > - if (!debugfs_create_ulong("u_volt_min", S_IRUGO, d, &opp->u_volt_min)) > + if (!debugfs_create_ulong("u_volt_min", S_IRUGO, d, &opp->supply.u_volt_min)) > return -ENOMEM; > > - if (!debugfs_create_ulong("u_volt_max", S_IRUGO, d, &opp->u_volt_max)) > + if (!debugfs_create_ulong("u_volt_max", S_IRUGO, d, &opp->supply.u_volt_max)) > return -ENOMEM; > > - if (!debugfs_create_ulong("u_amp", S_IRUGO, d, &opp->u_amp)) > + if (!debugfs_create_ulong("u_amp", S_IRUGO, d, &opp->supply.u_amp)) > return -ENOMEM; > > if (!debugfs_create_ulong("clock_latency_ns", S_IRUGO, d, > diff --git a/drivers/base/power/opp/of.c b/drivers/base/power/opp/of.c > index 5552211e6fcd..b7fcd0a1b58b 100644 > --- a/drivers/base/power/opp/of.c > +++ b/drivers/base/power/opp/of.c > @@ -148,14 +148,14 @@ static int opp_parse_supplies(struct dev_pm_opp *opp, struct device *dev, > return -EINVAL; > } > > - opp->u_volt = microvolt[0]; > + opp->supply.u_volt = microvolt[0]; > > if (count == 1) { > - opp->u_volt_min = opp->u_volt; > - opp->u_volt_max = opp->u_volt; > + opp->supply.u_volt_min = opp->supply.u_volt; > + opp->supply.u_volt_max = opp->supply.u_volt; > } else { > - opp->u_volt_min = microvolt[1]; > - opp->u_volt_max = microvolt[2]; > + opp->supply.u_volt_min = microvolt[1]; > + opp->supply.u_volt_max = microvolt[2]; > } > > /* Search for "opp-microamp-" */ > @@ -173,7 +173,7 @@ static int opp_parse_supplies(struct dev_pm_opp *opp, struct device *dev, > } > > if (prop && !of_property_read_u32(opp->np, name, &val)) > - opp->u_amp = val; > + opp->supply.u_amp = val; > > return 0; > } > @@ -303,9 +303,9 @@ static int _opp_add_static_v2(struct device *dev, struct device_node *np) > mutex_unlock(&opp_table_lock); > > pr_debug("%s: turbo:%d rate:%lu uv:%lu uvmin:%lu uvmax:%lu latency:%lu\n", > - __func__, new_opp->turbo, new_opp->rate, new_opp->u_volt, > - new_opp->u_volt_min, new_opp->u_volt_max, > - new_opp->clock_latency_ns); > + __func__, new_opp->turbo, new_opp->rate, > + new_opp->supply.u_volt, new_opp->supply.u_volt_min, > + new_opp->supply.u_volt_max, new_opp->clock_latency_ns); > > /* > * Notify the changes in the availability of the operable > diff --git a/drivers/base/power/opp/opp.h b/drivers/base/power/opp/opp.h > index fabd5ca1a083..1bda0d35c486 100644 > --- a/drivers/base/power/opp/opp.h > +++ b/drivers/base/power/opp/opp.h > @@ -47,6 +47,22 @@ extern struct list_head opp_tables; > */ > > /** > + * struct dev_pm_opp_supply - Power supply voltage/current values > + * @u_volt: Target voltage in microvolts corresponding to this OPP > + * @u_volt_min: Minimum voltage in microvolts corresponding to thisq OPP > + * @u_volt_max: Maximum voltage in microvolts corresponding to this OPP > + * @u_amp: Maximum current drawn by the device in microamperes > + * > + * This structure stores the voltage/current values for a single power supply. > + */ > +struct dev_pm_opp_supply { > + unsigned long u_volt; > + unsigned long u_volt_min; > + unsigned long u_volt_max; > + unsigned long u_amp; > +}; > + > +/** This needs to move to include/linux/pm_opp.h, does it not? We need access to the actual voltage values from outside of the OPP core if we are going to be setting regulators from the platform provided opp_set_rate callback described in patch 7. Regards, Dave > * struct dev_pm_opp - Generic OPP description structure > * @node: opp table node. The nodes are maintained throughout the lifetime > * of boot. It is expected only an optimal set of OPPs are > @@ -61,10 +77,7 @@ extern struct list_head opp_tables; > * @turbo: true if turbo (boost) OPP > * @suspend: true if suspend OPP > * @rate: Frequency in hertz > - * @u_volt: Target voltage in microvolts corresponding to this OPP > - * @u_volt_min: Minimum voltage in microvolts corresponding to this OPP > - * @u_volt_max: Maximum voltage in microvolts corresponding to this OPP > - * @u_amp: Maximum current drawn by the device in microamperes > + * @supply: Power supply voltage/current values > * @clock_latency_ns: Latency (in nanoseconds) of switching to this OPP's > * frequency from any other OPP's frequency. > * @opp_table: points back to the opp_table struct this opp belongs to > @@ -83,10 +96,8 @@ struct dev_pm_opp { > bool suspend; > unsigned long rate; > > - unsigned long u_volt; > - unsigned long u_volt_min; > - unsigned long u_volt_max; > - unsigned long u_amp; > + struct dev_pm_opp_supply supply; > + > unsigned long clock_latency_ns; > > struct opp_table *opp_table; >