| Message ID | 20190715120416.3561-2-k.konieczny@partner.samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | viresh kumar |
| Headers | show |
| Series | [v2,1/4] opp: core: add regulators enable and disable | expand |
Hi Kamil, On 19. 7. 15. 오후 9:04, Kamil Konieczny wrote: > Add enable regulators to dev_pm_opp_set_regulators() and disable > regulators to dev_pm_opp_put_regulators(). This prepares for > converting exynos-bus devfreq driver to use dev_pm_opp_set_rate(). IMHO, it is not proper to mention the specific driver name. If you explain the reason why enable the regulator before using it, it is enough description. > > Signed-off-by: Kamil Konieczny <k.konieczny@partner.samsung.com> > -- > Changes in v2: > > - move regulator enable and disable into loop > > --- > drivers/opp/core.c | 18 +++++++++++++++--- > 1 file changed, 15 insertions(+), 3 deletions(-) > > diff --git a/drivers/opp/core.c b/drivers/opp/core.c > index 0e7703fe733f..069c5cf8827e 100644 > --- a/drivers/opp/core.c > +++ b/drivers/opp/core.c > @@ -1570,6 +1570,10 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev, > goto free_regulators; > } > > + ret = regulator_enable(reg); > + if (ret < 0) > + goto disable; > + > opp_table->regulators[i] = reg; > } > > @@ -1582,9 +1586,15 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev, > > return opp_table; > > +disable: > + regulator_put(reg); > + --i; > + > free_regulators: > - while (i != 0) > - regulator_put(opp_table->regulators[--i]); > + for (; i >= 0; --i) { > + regulator_disable(opp_table->regulators[i]); > + regulator_put(opp_table->regulators[i]); > + } > > kfree(opp_table->regulators); > opp_table->regulators = NULL; > @@ -1610,8 +1620,10 @@ void dev_pm_opp_put_regulators(struct opp_table *opp_table) > /* Make sure there are no concurrent readers while updating opp_table */ > WARN_ON(!list_empty(&opp_table->opp_list)); > > - for (i = opp_table->regulator_count - 1; i >= 0; i--) > + for (i = opp_table->regulator_count - 1; i >= 0; i--) { > + regulator_disable(opp_table->regulators[i]); > regulator_put(opp_table->regulators[i]); > + } > > _free_set_opp_data(opp_table); > > I agree to enable the regulator before using it. The bootloader might not enable the regulators and the kernel need to enable regulator in order to increase the reference count explicitly event if bootloader enables it. Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
On 15-07-19, 14:04, Kamil Konieczny wrote: > Add enable regulators to dev_pm_opp_set_regulators() and disable > regulators to dev_pm_opp_put_regulators(). This prepares for > converting exynos-bus devfreq driver to use dev_pm_opp_set_rate(). > > Signed-off-by: Kamil Konieczny <k.konieczny@partner.samsung.com> > -- > Changes in v2: > > - move regulator enable and disable into loop > > --- > drivers/opp/core.c | 18 +++++++++++++++--- > 1 file changed, 15 insertions(+), 3 deletions(-) > > diff --git a/drivers/opp/core.c b/drivers/opp/core.c > index 0e7703fe733f..069c5cf8827e 100644 > --- a/drivers/opp/core.c > +++ b/drivers/opp/core.c > @@ -1570,6 +1570,10 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev, > goto free_regulators; > } > > + ret = regulator_enable(reg); > + if (ret < 0) > + goto disable; The name of this label is logically incorrect because we won't disable the regulator from there but put it. Over that, I would rather prefer to remove the label and add regulator_put() here itself. > + > opp_table->regulators[i] = reg; > } > > @@ -1582,9 +1586,15 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev, > > return opp_table; > > +disable: > + regulator_put(reg); > + --i; > + > free_regulators: > - while (i != 0) > - regulator_put(opp_table->regulators[--i]); > + for (; i >= 0; --i) { > + regulator_disable(opp_table->regulators[i]); > + regulator_put(opp_table->regulators[i]); This is incorrect as this will now try to put/disable the regulator which we failed to acquire. As --i happens only after the loop has run once. You can rather do: while (i--) { regulator_disable(opp_table->regulators[i]); regulator_put(opp_table->regulators[i]); } > + } > > kfree(opp_table->regulators); > opp_table->regulators = NULL; > @@ -1610,8 +1620,10 @@ void dev_pm_opp_put_regulators(struct opp_table *opp_table) > /* Make sure there are no concurrent readers while updating opp_table */ > WARN_ON(!list_empty(&opp_table->opp_list)); > > - for (i = opp_table->regulator_count - 1; i >= 0; i--) > + for (i = opp_table->regulator_count - 1; i >= 0; i--) { > + regulator_disable(opp_table->regulators[i]); > regulator_put(opp_table->regulators[i]); > + } > > _free_set_opp_data(opp_table); > > -- > 2.22.0
On 16.07.2019 06:03, Chanwoo Choi wrote: > Hi Kamil, > > On 19. 7. 15. 오후 9:04, Kamil Konieczny wrote: >> Add enable regulators to dev_pm_opp_set_regulators() and disable >> regulators to dev_pm_opp_put_regulators(). This prepares for >> converting exynos-bus devfreq driver to use dev_pm_opp_set_rate(). > > IMHO, it is not proper to mention the specific driver name. > If you explain the reason why enable the regulator before using it, > it is enough description. > >> >> Signed-off-by: Kamil Konieczny <k.konieczny@partner.samsung.com> >> -- >> Changes in v2: >> >> - move regulator enable and disable into loop >> >> --- >> drivers/opp/core.c | 18 +++++++++++++++--- >> 1 file changed, 15 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/opp/core.c b/drivers/opp/core.c >> index 0e7703fe733f..069c5cf8827e 100644 >> --- a/drivers/opp/core.c >> +++ b/drivers/opp/core.c >> @@ -1570,6 +1570,10 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev, >> goto free_regulators; >> } >> >> + ret = regulator_enable(reg); >> + if (ret < 0) >> + goto disable; >> + >> opp_table->regulators[i] = reg; >> } >> >> @@ -1582,9 +1586,15 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev, >> >> return opp_table; >> >> +disable: >> + regulator_put(reg); >> + --i; >> + >> free_regulators: >> - while (i != 0) >> - regulator_put(opp_table->regulators[--i]); >> + for (; i >= 0; --i) { >> + regulator_disable(opp_table->regulators[i]); >> + regulator_put(opp_table->regulators[i]); >> + } >> >> kfree(opp_table->regulators); >> opp_table->regulators = NULL; >> @@ -1610,8 +1620,10 @@ void dev_pm_opp_put_regulators(struct opp_table *opp_table) >> /* Make sure there are no concurrent readers while updating opp_table */ >> WARN_ON(!list_empty(&opp_table->opp_list)); >> >> - for (i = opp_table->regulator_count - 1; i >= 0; i--) >> + for (i = opp_table->regulator_count - 1; i >= 0; i--) { >> + regulator_disable(opp_table->regulators[i]); >> regulator_put(opp_table->regulators[i]); >> + } >> >> _free_set_opp_data(opp_table); >> >> > > I agree to enable the regulator before using it. > The bootloader might not enable the regulators > and the kernel need to enable regulator in order to increase > the reference count explicitly event if bootloader enables it. > > Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com> Thank you, I will change commit description and send v3.
On 16.07.2019 12:05, Viresh Kumar wrote: > On 15-07-19, 14:04, Kamil Konieczny wrote: >> Add enable regulators to dev_pm_opp_set_regulators() and disable >> regulators to dev_pm_opp_put_regulators(). This prepares for >> converting exynos-bus devfreq driver to use dev_pm_opp_set_rate(). >> >> Signed-off-by: Kamil Konieczny <k.konieczny@partner.samsung.com> >> -- >> Changes in v2: >> >> - move regulator enable and disable into loop >> >> --- >> drivers/opp/core.c | 18 +++++++++++++++--- >> 1 file changed, 15 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/opp/core.c b/drivers/opp/core.c >> index 0e7703fe733f..069c5cf8827e 100644 >> --- a/drivers/opp/core.c >> +++ b/drivers/opp/core.c >> @@ -1570,6 +1570,10 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev, >> goto free_regulators; >> } >> >> + ret = regulator_enable(reg); >> + if (ret < 0) >> + goto disable; > > The name of this label is logically incorrect because we won't disable > the regulator from there but put it. Over that, I would rather prefer > to remove the label and add regulator_put() here itself. I will change this and following according to your suggestions and will send v3. >> + >> opp_table->regulators[i] = reg; >> } >> >> @@ -1582,9 +1586,15 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev, >> >> return opp_table; >> >> +disable: >> + regulator_put(reg); >> + --i; >> + >> free_regulators: >> - while (i != 0) >> - regulator_put(opp_table->regulators[--i]); >> + for (; i >= 0; --i) { >> + regulator_disable(opp_table->regulators[i]); >> + regulator_put(opp_table->regulators[i]); > > This is incorrect as this will now try to put/disable the regulator > which we failed to acquire. As --i happens only after the loop has run > once. You can rather do: > > while (i--) { > regulator_disable(opp_table->regulators[i]); > regulator_put(opp_table->regulators[i]); > } > > >> + } >> >> kfree(opp_table->regulators); >> opp_table->regulators = NULL; >> @@ -1610,8 +1620,10 @@ void dev_pm_opp_put_regulators(struct opp_table *opp_table) >> /* Make sure there are no concurrent readers while updating opp_table */ >> WARN_ON(!list_empty(&opp_table->opp_list)); >> >> - for (i = opp_table->regulator_count - 1; i >= 0; i--) >> + for (i = opp_table->regulator_count - 1; i >= 0; i--) { >> + regulator_disable(opp_table->regulators[i]); >> regulator_put(opp_table->regulators[i]); >> + } >> >> _free_set_opp_data(opp_table); >> >> -- >> 2.22.0 >
diff --git a/drivers/opp/core.c b/drivers/opp/core.c index 0e7703fe733f..069c5cf8827e 100644 --- a/drivers/opp/core.c +++ b/drivers/opp/core.c @@ -1570,6 +1570,10 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev, goto free_regulators; } + ret = regulator_enable(reg); + if (ret < 0) + goto disable; + opp_table->regulators[i] = reg; } @@ -1582,9 +1586,15 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev, return opp_table; +disable: + regulator_put(reg); + --i; + free_regulators: - while (i != 0) - regulator_put(opp_table->regulators[--i]); + for (; i >= 0; --i) { + regulator_disable(opp_table->regulators[i]); + regulator_put(opp_table->regulators[i]); + } kfree(opp_table->regulators); opp_table->regulators = NULL; @@ -1610,8 +1620,10 @@ void dev_pm_opp_put_regulators(struct opp_table *opp_table) /* Make sure there are no concurrent readers while updating opp_table */ WARN_ON(!list_empty(&opp_table->opp_list)); - for (i = opp_table->regulator_count - 1; i >= 0; i--) + for (i = opp_table->regulator_count - 1; i >= 0; i--) { + regulator_disable(opp_table->regulators[i]); regulator_put(opp_table->regulators[i]); + } _free_set_opp_data(opp_table);
Add enable regulators to dev_pm_opp_set_regulators() and disable regulators to dev_pm_opp_put_regulators(). This prepares for converting exynos-bus devfreq driver to use dev_pm_opp_set_rate(). Signed-off-by: Kamil Konieczny <k.konieczny@partner.samsung.com> -- Changes in v2: - move regulator enable and disable into loop --- drivers/opp/core.c | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-)