diff mbox series

[v3,05/12] opp: Add dev_pm_opp_set_voltage()

Message ID 20210118005524.27787-6-digetx@gmail.com (mailing list archive)
State New
Delegated to: viresh kumar
Headers show
Series OPP API fixes and improvements | expand

Commit Message

Dmitry Osipenko Jan. 18, 2021, 12:55 a.m. UTC
Add dev_pm_opp_set_voltage() which allows OPP table users to set voltage
in accordance to a given OPP. In particular this is needed for driving
voltage of a generic power domain which uses OPPs and doesn't have a
clock.

Tested-by: Peter Geis <pgwipeout@gmail.com>
Tested-by: Nicolas Chauvet <kwizart@gmail.com>
Tested-by: Matt Merhar <mattmerhar@protonmail.com>
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/opp/core.c     | 55 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/pm_opp.h |  6 +++++
 2 files changed, 61 insertions(+)

Comments

Viresh Kumar Jan. 18, 2021, 9:52 a.m. UTC | #1
On 18-01-21, 03:55, Dmitry Osipenko wrote:
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 99d18befc209..341484d58e6c 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -2731,3 +2731,58 @@ int dev_pm_opp_sync_regulators(struct device *dev)
>  	return ret;
>  }
>  EXPORT_SYMBOL_GPL(dev_pm_opp_sync_regulators);
> +
> +/**
> + * dev_pm_opp_set_voltage() - Change voltage of regulators
> + * @dev:	device for which we do this operation
> + * @opp:	opp based on which the voltages are to be configured
> + *
> + * Change voltage of the OPP table regulators.
> + *
> + * Return: 0 on success or a negative error value.
> + */
> +int dev_pm_opp_set_voltage(struct device *dev, struct dev_pm_opp *opp)

I think we should do better than this, will require some work from
your part though (or I can do it if you want).

Basically what you wanted to do here is set the OPP for a device and
this means do whatever is required for setting the OPP. It is normally
frequency, which is not your case, but it is other things as well.
Like setting multiple regulators, bandwidth, required-opps, etc.

I feel the right way of doing this would be to do this:

Factor out dev_pm_opp_set_opp() from dev_pm_opp_set_rate() and make
the later call the former. And then we can just call
dev_pm_opp_set_opp() from your usecase. This will make sure we have a
single code path for all the set-opp stuff. What do you think ?
Dmitry Osipenko Jan. 18, 2021, 7:14 p.m. UTC | #2
18.01.2021 12:52, Viresh Kumar пишет:
> On 18-01-21, 03:55, Dmitry Osipenko wrote:
>> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
>> index 99d18befc209..341484d58e6c 100644
>> --- a/drivers/opp/core.c
>> +++ b/drivers/opp/core.c
>> @@ -2731,3 +2731,58 @@ int dev_pm_opp_sync_regulators(struct device *dev)
>>  	return ret;
>>  }
>>  EXPORT_SYMBOL_GPL(dev_pm_opp_sync_regulators);
>> +
>> +/**
>> + * dev_pm_opp_set_voltage() - Change voltage of regulators
>> + * @dev:	device for which we do this operation
>> + * @opp:	opp based on which the voltages are to be configured
>> + *
>> + * Change voltage of the OPP table regulators.
>> + *
>> + * Return: 0 on success or a negative error value.
>> + */
>> +int dev_pm_opp_set_voltage(struct device *dev, struct dev_pm_opp *opp)
> 
> I think we should do better than this, will require some work from
> your part though (or I can do it if you want).
> 
> Basically what you wanted to do here is set the OPP for a device and
> this means do whatever is required for setting the OPP. It is normally
> frequency, which is not your case, but it is other things as well.
> Like setting multiple regulators, bandwidth, required-opps, etc.
> 
> I feel the right way of doing this would be to do this:
> 
> Factor out dev_pm_opp_set_opp() from dev_pm_opp_set_rate() and make
> the later call the former. And then we can just call
> dev_pm_opp_set_opp() from your usecase. This will make sure we have a
> single code path for all the set-opp stuff. What do you think ?
> 

Sounds like it could be a lot of code moving and some extra complexity
will be added to the code. If nobody will ever need the universal
dev_pm_opp_set_opp(), then it could become a wasted effort. I'd choose
the easiest path, i.e. to defer the dev_pm_opp_set_opp() implementation
until somebody will really need it.

But if it looks to you that it won't be a too much effort, then I'll
appreciate if you could type the patch.
Dmitry Osipenko Jan. 20, 2021, 9:57 p.m. UTC | #3
18.01.2021 22:14, Dmitry Osipenko пишет:
> 18.01.2021 12:52, Viresh Kumar пишет:
>> On 18-01-21, 03:55, Dmitry Osipenko wrote:
>>> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
>>> index 99d18befc209..341484d58e6c 100644
>>> --- a/drivers/opp/core.c
>>> +++ b/drivers/opp/core.c
>>> @@ -2731,3 +2731,58 @@ int dev_pm_opp_sync_regulators(struct device *dev)
>>>  	return ret;
>>>  }
>>>  EXPORT_SYMBOL_GPL(dev_pm_opp_sync_regulators);
>>> +
>>> +/**
>>> + * dev_pm_opp_set_voltage() - Change voltage of regulators
>>> + * @dev:	device for which we do this operation
>>> + * @opp:	opp based on which the voltages are to be configured
>>> + *
>>> + * Change voltage of the OPP table regulators.
>>> + *
>>> + * Return: 0 on success or a negative error value.
>>> + */
>>> +int dev_pm_opp_set_voltage(struct device *dev, struct dev_pm_opp *opp)
>>
>> I think we should do better than this, will require some work from
>> your part though (or I can do it if you want).
>>
>> Basically what you wanted to do here is set the OPP for a device and
>> this means do whatever is required for setting the OPP. It is normally
>> frequency, which is not your case, but it is other things as well.
>> Like setting multiple regulators, bandwidth, required-opps, etc.
>>
>> I feel the right way of doing this would be to do this:
>>
>> Factor out dev_pm_opp_set_opp() from dev_pm_opp_set_rate() and make
>> the later call the former. And then we can just call
>> dev_pm_opp_set_opp() from your usecase. This will make sure we have a
>> single code path for all the set-opp stuff. What do you think ?
>>
> 
> Sounds like it could be a lot of code moving and some extra complexity
> will be added to the code. If nobody will ever need the universal
> dev_pm_opp_set_opp(), then it could become a wasted effort. I'd choose
> the easiest path, i.e. to defer the dev_pm_opp_set_opp() implementation
> until somebody will really need it.
> 
> But if it looks to you that it won't be a too much effort, then I'll
> appreciate if you could type the patch.
> 

Let's start with dev_pm_opp_set_voltage() for now. It shouldn't be a
problem at all to upgrade it to dev_pm_opp_set_opp() later on.

I'll make a v4 with the dev_pm_opp_set_voltage(), please let me know if
you have objections or more suggestions!
Viresh Kumar Jan. 21, 2021, 11:20 a.m. UTC | #4
On 21-01-21, 00:57, Dmitry Osipenko wrote:
> 18.01.2021 22:14, Dmitry Osipenko пишет:
> > Sounds like it could be a lot of code moving and some extra complexity
> > will be added to the code. If nobody will ever need the universal
> > dev_pm_opp_set_opp(), then it could become a wasted effort. I'd choose
> > the easiest path, i.e. to defer the dev_pm_opp_set_opp() implementation
> > until somebody will really need it.
> > 
> > But if it looks to you that it won't be a too much effort, then I'll
> > appreciate if you could type the patch.

Yes.
 
> Let's start with dev_pm_opp_set_voltage() for now. It shouldn't be a
> problem at all to upgrade it to dev_pm_opp_set_opp() later on.
> 
> I'll make a v4 with the dev_pm_opp_set_voltage(), please let me know if
> you have objections or more suggestions!

Sorry about this, I have been working on this stuff for last 2 days. I
didn't reply earlier as I thought I would be able to finish this
earlier. Once you see the patches you will see it was a significant
change :)

I have cc'd you to the relevant patches now. Please see if they work
fine for you or not.
diff mbox series

Patch

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 99d18befc209..341484d58e6c 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -2731,3 +2731,58 @@  int dev_pm_opp_sync_regulators(struct device *dev)
 	return ret;
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_sync_regulators);
+
+/**
+ * dev_pm_opp_set_voltage() - Change voltage of regulators
+ * @dev:	device for which we do this operation
+ * @opp:	opp based on which the voltages are to be configured
+ *
+ * Change voltage of the OPP table regulators.
+ *
+ * Return: 0 on success or a negative error value.
+ */
+int dev_pm_opp_set_voltage(struct device *dev, struct dev_pm_opp *opp)
+{
+	struct opp_table *opp_table;
+	struct regulator *reg;
+	int ret = 0;
+
+	/* Device may not have OPP table */
+	opp_table = _find_opp_table(dev);
+	if (IS_ERR(opp_table))
+		return 0;
+
+	/* Regulator may not be required for the device */
+	if (!opp_table->regulators)
+		goto put_table;
+
+	/* This function only supports single regulator per device */
+	if (WARN_ON(opp_table->regulator_count > 1)) {
+		dev_err(dev, "multiple regulators are not supported\n");
+		ret = -EINVAL;
+		goto put_table;
+	}
+
+	mutex_lock(&opp_table->lock);
+
+	reg = opp_table->regulators[0];
+	ret = _set_opp_voltage(dev, reg, opp->supplies);
+
+	if (!opp_table->enabled) {
+		ret = regulator_enable(reg);
+		if (ret < 0) {
+			dev_warn(dev, "Failed to enable regulator: %d", ret);
+			goto unlock;
+		}
+
+		opp_table->enabled = true;
+	}
+unlock:
+	mutex_unlock(&opp_table->lock);
+put_table:
+	/* Drop reference taken by _find_opp_table() */
+	dev_pm_opp_put_opp_table(opp_table);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(dev_pm_opp_set_voltage);
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index 1c3a09cc8dcd..f344be844bde 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -163,6 +163,7 @@  int dev_pm_opp_get_sharing_cpus(struct device *cpu_dev, struct cpumask *cpumask)
 void dev_pm_opp_remove_table(struct device *dev);
 void dev_pm_opp_cpumask_remove_table(const struct cpumask *cpumask);
 int dev_pm_opp_sync_regulators(struct device *dev);
+int dev_pm_opp_set_voltage(struct device *dev, struct dev_pm_opp *opp);
 #else
 static inline struct opp_table *dev_pm_opp_get_opp_table(struct device *dev)
 {
@@ -404,6 +405,11 @@  static inline int dev_pm_opp_sync_regulators(struct device *dev)
 	return -ENOTSUPP;
 }
 
+static inline int dev_pm_opp_set_voltage(struct device *dev, struct dev_pm_opp *opp)
+{
+	return -ENOTSUPP;
+}
+
 #endif		/* CONFIG_PM_OPP */
 
 #if defined(CONFIG_PM_OPP) && defined(CONFIG_OF)