| Message ID | 20210118005524.27787-6-digetx@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | OPP API fixes and improvements | expand |
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 ?
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.
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!
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. -- viresh
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)