| Message ID | 3345fd49f7987d022f4f61edb6c44f230f7354c4.1611227342.git.viresh.kumar@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | viresh kumar |
| Headers | show |
| Series | opp: Implement dev_pm_opp_set_opp() | expand |
21.01.2021 14:17, Viresh Kumar пишет: > dev_pm_opp_set_bw() is getting removed and dev_pm_opp_set_opp() should > be used instead. Migrate to the new API. > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > --- > drivers/devfreq/tegra30-devfreq.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c > index 117cad7968ab..d2477d7d1f66 100644 > --- a/drivers/devfreq/tegra30-devfreq.c > +++ b/drivers/devfreq/tegra30-devfreq.c > @@ -647,7 +647,7 @@ static int tegra_devfreq_target(struct device *dev, unsigned long *freq, > return PTR_ERR(opp); > } > > - ret = dev_pm_opp_set_bw(dev, opp); > + ret = dev_pm_opp_set_opp(dev, opp); > dev_pm_opp_put(opp); > > return ret; > This patch introduces a very serious change that needs to be fixed. Now dev_pm_opp_set_opp() changes both clock rate and bandwidth, this is unacceptable for this driver because it shall not touch the clock rate. I think dev_pm_opp_set_bw() can't be removed.
On 22-01-21, 00:36, Dmitry Osipenko wrote: > 21.01.2021 14:17, Viresh Kumar пишет: > > dev_pm_opp_set_bw() is getting removed and dev_pm_opp_set_opp() should > > be used instead. Migrate to the new API. > > > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > > --- > > drivers/devfreq/tegra30-devfreq.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c > > index 117cad7968ab..d2477d7d1f66 100644 > > --- a/drivers/devfreq/tegra30-devfreq.c > > +++ b/drivers/devfreq/tegra30-devfreq.c > > @@ -647,7 +647,7 @@ static int tegra_devfreq_target(struct device *dev, unsigned long *freq, > > return PTR_ERR(opp); > > } > > > > - ret = dev_pm_opp_set_bw(dev, opp); > > + ret = dev_pm_opp_set_opp(dev, opp); > > dev_pm_opp_put(opp); > > > > return ret; > > > > This patch introduces a very serious change that needs to be fixed. > > Now dev_pm_opp_set_opp() changes both clock rate and bandwidth, this is > unacceptable for this driver because it shall not touch the clock rate. > > I think dev_pm_opp_set_bw() can't be removed. I am wondering here on what would be a better solution, do what you said or introduce another helper like dev_pm_opp_clear_clk(), which will make sure the OPP core doesn't play with device's clk.
22.01.2021 09:26, Viresh Kumar пишет: > On 22-01-21, 00:36, Dmitry Osipenko wrote: >> 21.01.2021 14:17, Viresh Kumar пишет: >>> dev_pm_opp_set_bw() is getting removed and dev_pm_opp_set_opp() should >>> be used instead. Migrate to the new API. >>> >>> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> >>> --- >>> drivers/devfreq/tegra30-devfreq.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c >>> index 117cad7968ab..d2477d7d1f66 100644 >>> --- a/drivers/devfreq/tegra30-devfreq.c >>> +++ b/drivers/devfreq/tegra30-devfreq.c >>> @@ -647,7 +647,7 @@ static int tegra_devfreq_target(struct device *dev, unsigned long *freq, >>> return PTR_ERR(opp); >>> } >>> >>> - ret = dev_pm_opp_set_bw(dev, opp); >>> + ret = dev_pm_opp_set_opp(dev, opp); >>> dev_pm_opp_put(opp); >>> >>> return ret; >>> >> >> This patch introduces a very serious change that needs to be fixed. >> >> Now dev_pm_opp_set_opp() changes both clock rate and bandwidth, this is >> unacceptable for this driver because it shall not touch the clock rate. >> >> I think dev_pm_opp_set_bw() can't be removed. > > I am wondering here on what would be a better solution, do what you > said or introduce another helper like dev_pm_opp_clear_clk(), which > will make sure the OPP core doesn't play with device's clk. > Either way will work, but maybe keeping the dev_pm_opp_set_bw() is a bit more straightforward variant for now since it will avoid the code's changes and it's probably a bit more obvious variant for the OPP users.
On 22-01-21, 18:28, Dmitry Osipenko wrote: > Either way will work, but maybe keeping the dev_pm_opp_set_bw() is a bit > more straightforward variant for now since it will avoid the code's > changes and it's probably a bit more obvious variant for the OPP users. The problem is it creates unnecessary paths which we need to support. For example, in case of bandwidth itself we may want to update regulator/pm domain/required OPPs and this should all be done for everyone. I really do not want to keep separate paths for such stuff, it makes it hard to maintain..
25.01.2021 06:14, Viresh Kumar пишет: > On 22-01-21, 18:28, Dmitry Osipenko wrote: >> Either way will work, but maybe keeping the dev_pm_opp_set_bw() is a bit >> more straightforward variant for now since it will avoid the code's >> changes and it's probably a bit more obvious variant for the OPP users. > > The problem is it creates unnecessary paths which we need to support. For > example, in case of bandwidth itself we may want to update regulator/pm > domain/required OPPs and this should all be done for everyone. I really do not > want to keep separate paths for such stuff, it makes it hard to maintain.. > Maybe we could add dev_pm_opp_of_add_table_without_clock(), at least that should be a bit nicer from a drivers perspective than having to care about dev_pm_opp_clear_clk(), IMO.
diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c index 117cad7968ab..d2477d7d1f66 100644 --- a/drivers/devfreq/tegra30-devfreq.c +++ b/drivers/devfreq/tegra30-devfreq.c @@ -647,7 +647,7 @@ static int tegra_devfreq_target(struct device *dev, unsigned long *freq, return PTR_ERR(opp); } - ret = dev_pm_opp_set_bw(dev, opp); + ret = dev_pm_opp_set_opp(dev, opp); dev_pm_opp_put(opp); return ret;
dev_pm_opp_set_bw() is getting removed and dev_pm_opp_set_opp() should be used instead. Migrate to the new API. Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> --- drivers/devfreq/tegra30-devfreq.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)