Re: [PATCH] opp: Prepare for ->set_opp() helper to work without regulators

Viresh Kumar
2021-01-21T11:29:58Z

On 20-01-21, 13:38, Viresh Kumar wrote:
> On 19-01-21, 12:05, Viresh Kumar wrote:
> > Until now the ->set_opp() helper (i.e. special implementation for
> > setting the OPPs for platforms) was implemented only to take care of
> > multiple regulators case, but going forward we would need that for other
> > use cases as well.
> >
> > This patch prepares for that by allocating the regulator specific part
> > from dev_pm_opp_set_regulators() and the opp helper part from
> > dev_pm_opp_register_set_opp_helper().
> >
> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> > ---
> > Dmitry,
> >
> > I haven't tested this patch, can you please help with that ?
> >
> > drivers/opp/core.c | 81 ++++++++++++++++++++++++----------------------
> > drivers/opp/opp.h | 2 ++
> > 2 files changed, 45 insertions(+), 38 deletions(-)
>
> Pushed with this diff.
>
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 1ed673334565..1dc5ca3f6d26 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -1939,7 +1939,6 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev,
> unsigned int count)
> {
> struct dev_pm_opp_supply *supplies;
> - struct dev_pm_set_opp_data *data;
> struct opp_table *opp_table;
> struct regulator *reg;
> int ret, i;
> @@ -1990,9 +1989,8 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev,
> mutex_lock(&opp_table->lock);
> opp_table->sod_supplies = supplies;
> if (opp_table->set_opp_data) {
> - data = opp_table->set_opp_data;
> - data->old_opp.supplies = supplies;
> - data->new_opp.supplies = supplies + count;
> + opp_table->set_opp_data->old_opp.supplies = supplies;
> + opp_table->set_opp_data->new_opp.supplies = supplies + count;
> }
> mutex_unlock(&opp_table->lock);
>
> @@ -2038,7 +2036,7 @@ void dev_pm_opp_put_regulators(struct opp_table *opp_table)
> regulator_put(opp_table->regulators[i]);
>
> mutex_lock(&opp_table->lock);
> - if (opp_table->sod_supplies) {
> + if (opp_table->set_opp_data) {
> opp_table->set_opp_data->old_opp.supplies = NULL;
> opp_table->set_opp_data->new_opp.supplies = NULL;
> }

And some more as pointed out by Dmitry (I will resend it again
properly).

t a/drivers/opp/core.c b/drivers/opp/core.c
index d8ca15d96ce9..747bdc4338f3 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -2118,8 +2118,12 @@ void dev_pm_opp_unregister_set_opp_helper(struct opp_table *opp_table)
WARN_ON(!list_empty(&opp_table->opp_list));

opp_table->set_opp = NULL;
+
+ mutex_lock(&opp_table->lock);
kfree(opp_table->set_opp_data);
opp_table->set_opp_data = NULL;
+ mutex_unlock(&opp_table->lock);
+
dev_pm_opp_put_opp_table(opp_table);
}
EXPORT_SYMBOL_GPL(dev_pm_opp_unregister_set_opp_helper);

--
viresh