Messages in this thread | | | Subject | Re: [PATCH 07/13] opp: Allow _generic_set_opp_clk_only() to work for non-freq devices | From | Dmitry Osipenko <> | Date | Tue, 26 Jan 2021 00:09:06 +0300 |
| |
22.01.2021 07:35, Viresh Kumar пишет: > On 21-01-21, 23:26, Dmitry Osipenko wrote: >> 21.01.2021 14:17, Viresh Kumar пишет: >>> In order to avoid conditional statements at the caller site, this patch >>> updates _generic_set_opp_clk_only() to work for devices that don't >>> change frequency (like power domains, etc.). Return 0 if the clk pointer >>> passed to this routine is not valid. >>> >>> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> >>> --- >> ... >> >> Hello Viresh, >> >> Thank you very much for yours effort! I gave a quick test to this series >> and instantly found one small issue in this patch. >> >>> + /* We may reach here for devices which don't change frequency */ >>> + if (unlikely(!clk)) >> >> I replaced dev_pm_opp_set_voltage() with dev_pm_opp_set_opp() in the >> Tegra PD driver and got a crash, which happens because the above line >> should be: >> >> if (IS_ERR(clk)) > > Fixed, thanks. >
Please remove unlikely() around IS_ERR(), it already has the unlikely().
https://elixir.bootlin.com/linux/v5.11-rc4/source/include/linux/err.h#L22
I'd also recommend to remove all the unlikely() from OPP code since it doesn't bring any value if not used in a very performance-critical code path. OPP core doesn't have such code paths. The [un]likely() only make code less readable and may result in a worse assembly.
| |