| Message ID | 20190708141140.24379-2-k.konieczny@partner.samsung.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [1/3] opp: core: add regulators enable and disable | expand |
On 08-07-19, 16:11, k.konieczny@partner.samsung.com wrote: > From: Kamil Konieczny <k.konieczny@partner.samsung.com> > > 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> > --- > drivers/opp/core.c | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/drivers/opp/core.c b/drivers/opp/core.c > index 0e7703fe733f..947cac452854 100644 > --- a/drivers/opp/core.c > +++ b/drivers/opp/core.c > @@ -1580,8 +1580,19 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev, > if (ret) > goto free_regulators; > > + for (i = 0; i < opp_table->regulator_count; i++) { > + ret = regulator_enable(opp_table->regulators[i]); > + if (ret < 0) > + goto disable; > + } I am wondering on why is this really required as this isn't done for any other platform, probably because the regulators are enabled by bootloader and are always on.
On 09.07.2019 07:40, Viresh Kumar wrote: > On 08-07-19, 16:11, k.konieczny@partner.samsung.com wrote: >> From: Kamil Konieczny <k.konieczny@partner.samsung.com> >> >> 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> >> --- >> drivers/opp/core.c | 13 +++++++++++++ >> 1 file changed, 13 insertions(+) >> >> diff --git a/drivers/opp/core.c b/drivers/opp/core.c >> index 0e7703fe733f..947cac452854 100644 >> --- a/drivers/opp/core.c >> +++ b/drivers/opp/core.c >> @@ -1580,8 +1580,19 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev, >> if (ret) >> goto free_regulators; >> >> + for (i = 0; i < opp_table->regulator_count; i++) { >> + ret = regulator_enable(opp_table->regulators[i]); >> + if (ret < 0) >> + goto disable; >> + } > > I am wondering on why is this really required as this isn't done for > any other platform, probably because the regulators are enabled by > bootloader and are always on. It was not enabled for historical reasons, from design point regualtors should be enabled before use.
On 10.07.2019 12:16, Kamil Konieczny wrote: > > > On 09.07.2019 07:40, Viresh Kumar wrote: >> On 08-07-19, 16:11, k.konieczny@partner.samsung.com wrote: >>> From: Kamil Konieczny <k.konieczny@partner.samsung.com> >>> >>> 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> >>> --- >>> drivers/opp/core.c | 13 +++++++++++++ >>> 1 file changed, 13 insertions(+) >>> >>> diff --git a/drivers/opp/core.c b/drivers/opp/core.c >>> index 0e7703fe733f..947cac452854 100644 >>> --- a/drivers/opp/core.c >>> +++ b/drivers/opp/core.c >>> @@ -1580,8 +1580,19 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev, >>> if (ret) >>> goto free_regulators; >>> >>> + for (i = 0; i < opp_table->regulator_count; i++) { >>> + ret = regulator_enable(opp_table->regulators[i]); >>> + if (ret < 0) >>> + goto disable; >>> + } >> >> I am wondering on why is this really required as this isn't done for >> any other platform, probably because the regulators are enabled by >> bootloader and are always on. > > It was not enabled for historical reasons, from design point regualtors > should be enabled before use. On Exynos platform devfreq driver (exynos-bus) always enabled them, so I wanted to preserve the current behaviour. I've also checked the change with cpufreq-dt driver and it doesn't cause issues. Do you find this change acceptable?
On 09.07.2019 07:40, Viresh Kumar wrote: > On 08-07-19, 16:11, k.konieczny@partner.samsung.com wrote: >> From: Kamil Konieczny <k.konieczny@partner.samsung.com> >> >> 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> >> --- >> drivers/opp/core.c | 13 +++++++++++++ >> 1 file changed, 13 insertions(+) >> >> diff --git a/drivers/opp/core.c b/drivers/opp/core.c >> index 0e7703fe733f..947cac452854 100644 >> --- a/drivers/opp/core.c >> +++ b/drivers/opp/core.c >> @@ -1580,8 +1580,19 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev, >> if (ret) >> goto free_regulators; >> >> + for (i = 0; i < opp_table->regulator_count; i++) { >> + ret = regulator_enable(opp_table->regulators[i]); >> + if (ret < 0) >> + goto disable; >> + } > > I am wondering on why is this really required as this isn't done for > any other platform, probably because the regulators are enabled by > bootloader and are always on. It is not ABI break, it should work with existing DTBs
On 10.07.2019 12:43, Kamil Konieczny wrote: > On 09.07.2019 07:40, Viresh Kumar wrote: >> On 08-07-19, 16:11, k.konieczny@partner.samsung.com wrote: >>> From: Kamil Konieczny <k.konieczny@partner.samsung.com> >>> >>> 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> >>> --- >>> drivers/opp/core.c | 13 +++++++++++++ >>> 1 file changed, 13 insertions(+) >>> >>> diff --git a/drivers/opp/core.c b/drivers/opp/core.c >>> index 0e7703fe733f..947cac452854 100644 >>> --- a/drivers/opp/core.c >>> +++ b/drivers/opp/core.c >>> @@ -1580,8 +1580,19 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev, >>> if (ret) >>> goto free_regulators; >>> >>> + for (i = 0; i < opp_table->regulator_count; i++) { >>> + ret = regulator_enable(opp_table->regulators[i]); >>> + if (ret < 0) >>> + goto disable; >>> + } >> >> I am wondering on why is this really required as this isn't done for >> any other platform, probably because the regulators are enabled by >> bootloader and are always on. > > It is not ABI break, it should work with existing DTBs Sorry, this answer should go to question by Krzysztof
On Mon, 8 Jul 2019 at 16:12, <k.konieczny@partner.samsung.com> wrote: > > From: Kamil Konieczny <k.konieczny@partner.samsung.com> > > 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> > --- > drivers/opp/core.c | 13 +++++++++++++ > 1 file changed, 13 insertions(+) Acked-by: Krzysztof Kozlowski <krzk@kernel.org> Best regards, Krzysztof
On 08-07-19, 16:11, k.konieczny@partner.samsung.com wrote: > From: Kamil Konieczny <k.konieczny@partner.samsung.com> > > 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> > --- > drivers/opp/core.c | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/drivers/opp/core.c b/drivers/opp/core.c > index 0e7703fe733f..947cac452854 100644 > --- a/drivers/opp/core.c > +++ b/drivers/opp/core.c > @@ -1580,8 +1580,19 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev, > if (ret) > goto free_regulators; > > + for (i = 0; i < opp_table->regulator_count; i++) { > + ret = regulator_enable(opp_table->regulators[i]); > + if (ret < 0) > + goto disable; > + } What about doing this in the same loop of regulator_get_optional() ? > + > return opp_table; > > +disable: > + while (i != 0) > + regulator_disable(opp_table->regulators[--i]); > + > + i = opp_table->regulator_count; You also need to call _free_set_opp_data() here. > free_regulators: > while (i != 0) > regulator_put(opp_table->regulators[--i]); > @@ -1609,6 +1620,8 @@ 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)); Preserve the blank line here. > + for (i = opp_table->regulator_count - 1; i >= 0; i--) > + regulator_disable(opp_table->regulators[i]); > > for (i = opp_table->regulator_count - 1; i >= 0; i--) > regulator_put(opp_table->regulators[i]); Only single loop should be sufficient for this. > -- > 2.22.0
On 11.07.2019 08:22, Viresh Kumar wrote: > On 08-07-19, 16:11, k.konieczny@partner.samsung.com wrote: >> From: Kamil Konieczny <k.konieczny@partner.samsung.com> >> >> 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> >> --- >> drivers/opp/core.c | 13 +++++++++++++ >> 1 file changed, 13 insertions(+) >> >> diff --git a/drivers/opp/core.c b/drivers/opp/core.c >> index 0e7703fe733f..947cac452854 100644 >> --- a/drivers/opp/core.c >> +++ b/drivers/opp/core.c >> @@ -1580,8 +1580,19 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev, >> if (ret) >> goto free_regulators; >> >> + for (i = 0; i < opp_table->regulator_count; i++) { >> + ret = regulator_enable(opp_table->regulators[i]); >> + if (ret < 0) >> + goto disable; >> + } > > What about doing this in the same loop of regulator_get_optional() ? yes, this is good, it will simplify code >> + >> return opp_table; >> >> +disable: >> + while (i != 0) >> + regulator_disable(opp_table->regulators[--i]); >> + >> + i = opp_table->regulator_count; > > You also need to call _free_set_opp_data() here. good catch if I move enable in loop above, then this will not be needed >> free_regulators: >> while (i != 0) >> regulator_put(opp_table->regulators[--i]); >> @@ -1609,6 +1620,8 @@ 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)); > > Preserve the blank line here. > >> + for (i = opp_table->regulator_count - 1; i >= 0; i--) >> + regulator_disable(opp_table->regulators[i]); >> >> for (i = opp_table->regulator_count - 1; i >= 0; i--) >> regulator_put(opp_table->regulators[i]); > > Only single loop should be sufficient for this. you are right, I will do this in single loop I will send v2
diff --git a/drivers/opp/core.c b/drivers/opp/core.c index 0e7703fe733f..947cac452854 100644 --- a/drivers/opp/core.c +++ b/drivers/opp/core.c @@ -1580,8 +1580,19 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev, if (ret) goto free_regulators; + for (i = 0; i < opp_table->regulator_count; i++) { + ret = regulator_enable(opp_table->regulators[i]); + if (ret < 0) + goto disable; + } + return opp_table; +disable: + while (i != 0) + regulator_disable(opp_table->regulators[--i]); + + i = opp_table->regulator_count; free_regulators: while (i != 0) regulator_put(opp_table->regulators[--i]); @@ -1609,6 +1620,8 @@ 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--) + regulator_disable(opp_table->regulators[i]); for (i = opp_table->regulator_count - 1; i >= 0; i--) regulator_put(opp_table->regulators[i]);