Re: [RFC v3 04/10] OPP: Add and export helper to update voltage

From: Sibi Sankar
Date: Wed Jan 29 2020 - 08:49:53 EST


On 2020-01-29 03:03, Matthias Kaehlcke wrote:
Hi Sibi,

On Tue, Jan 28, 2020 at 01:33:44AM +0530, Sibi Sankar wrote:
Add and export 'dev_pm_opp_update_voltage' to find and update voltage
of an opp for a given frequency. This will be useful to update the opps
with voltages read back from firmware.

Signed-off-by: Sibi Sankar <sibis@xxxxxxxxxxxxxx>
---
drivers/opp/core.c | 55 ++++++++++++++++++++++++++++++++++++++++++
include/linux/pm_opp.h | 10 ++++++++
2 files changed, 65 insertions(+)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 9aa2a44a5d638..f241e83ec926a 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -2503,6 +2503,61 @@ int dev_pm_opp_disable(struct device *dev, unsigned long freq)
}
EXPORT_SYMBOL_GPL(dev_pm_opp_disable);

+/**
+ * dev_pm_opp_update_voltage() - Find and update voltage

The comment should mention that this is done for an OPP.

Maybe omit the 'find' part here and just say 'Update the voltage of
an OPP'?

sure makes sense


+ * @dev: device for which we do this operation
+ * @freq: OPP frequency to update voltage
+ * @u_volt: voltage requested for this opp
+ *
+ * Find and update voltage of a disabled opp corresponding to the given
+ * frequency. This is useful only for devices with single power supply.
+ *
+ * Return: 0 if modification was successful or a negative error value.
+ */
+int dev_pm_opp_update_voltage(struct device *dev, unsigned long freq,
+ unsigned long u_volt)
+{
+ struct dev_pm_opp *opp = ERR_PTR(-ENODEV);

initialization is not needed

+ struct opp_table *opp_table;
+ unsigned long tol;
+ int ret = 0;
+
+ /* Find the opp_table */

Drop the comment, it's obvious from the code.

+ opp_table = _find_opp_table(dev);
+ if (IS_ERR(opp_table)) {
+ ret = PTR_ERR(opp_table);
+ dev_err(dev, "%s: OPP table not found (%d)\n", __func__, ret);
+ return PTR_ERR(opp_table);

return ret;

missed that :(


+ }
+
+ opp = dev_pm_opp_find_freq_exact(dev, freq, false);
+ if (IS_ERR(opp)) {
+ ret = PTR_ERR(opp);
+ goto put_table;
+ }
+
+ mutex_lock(&opp_table->lock);
+
+ /* update only if the opp is disabled */
+ if (opp->available) {
+ ret = -EBUSY;
+ goto unlock;
+ }
+
+ tol = u_volt * opp_table->voltage_tolerance_v1 / 100;
+ opp->supplies[0].u_volt_min = u_volt - tol;
+ opp->supplies[0].u_volt = u_volt;
+ opp->supplies[0].u_volt_min = u_volt + tol;

.u_volt_max =

I suppose the assignments need to be done for all possible supplies,
i.e. 0 to (opp_table->regulator_count - 1).

a single value for all possible
supplies seems wrong. Anyway
will wait to see what Viresh
thinks about it.

--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project.