Messages in this thread | | | Date | Thu, 11 May 2023 21:25:46 +0200 | Subject | Re: [PATCH] cpufreq: imx6q: Disable only available frequencies | From | Marek Vasut <> |
| |
On 5/11/23 11:23, Christoph Niedermaier wrote: > In the example in Documentation/power/opp.rst, an availability check > is present before disabling a specific frequency. If a frequency isn't > available, the warning of a failed disabling of a non-existent > frequency is misleading. Therefore, check the availability of the > frequency in a separate inline function before disabling it.
[...]
> diff --git a/drivers/cpufreq/imx6q-cpufreq.c b/drivers/cpufreq/imx6q-cpufreq.c > index 48e1772e98fd..4e2d2bc47aba 100644 > --- a/drivers/cpufreq/imx6q-cpufreq.c > +++ b/drivers/cpufreq/imx6q-cpufreq.c > @@ -209,6 +209,21 @@ static struct cpufreq_driver imx6q_cpufreq_driver = { > .suspend = cpufreq_generic_suspend, > }; > > +static inline int disable_freq_if_available(struct device *dev,
The inline isn't needed, esp. on static function, let the compiler figure it out.
Also, "disable if available" should rather be "disable if unavailable" I think ?
> + unsigned long freq) > +{ > + struct dev_pm_opp *opp; > + int ret = 0; > + > + opp = dev_pm_opp_find_freq_exact(dev, freq, true); > + if (!IS_ERR(opp)) { > + dev_pm_opp_put(opp); > + ret = dev_pm_opp_disable(dev, freq); > + } > + > + return ret; > +} > + > #define OCOTP_CFG3 0x440 > #define OCOTP_CFG3_SPEED_SHIFT 16 > #define OCOTP_CFG3_SPEED_1P2GHZ 0x3 > @@ -254,16 +269,16 @@ static int imx6q_opp_check_speed_grading(struct device *dev) > val &= 0x3; > > if (val < OCOTP_CFG3_SPEED_996MHZ) > - if (dev_pm_opp_disable(dev, 996000000)) > + if (disable_freq_if_available(dev, 996000000)) > dev_warn(dev, "failed to disable 996MHz OPP\n"); > > if (of_machine_is_compatible("fsl,imx6q") || > of_machine_is_compatible("fsl,imx6qp")) {
Can we introduce a function like:
void imx_disable_freq_if_unavailable(struct device *dev, u32 freq_mhz, u32 val, u32 mask) { if (val == mask) return; if (!disable_freq_if_available(dev, freq_mhz * 1000000)) return; dev_warn(dev, "failed to disable %dMHz OPP\n", mhz); }
And then just call it multiple times in here, to reduce duplication ?
> if (val != OCOTP_CFG3_SPEED_852MHZ) > - if (dev_pm_opp_disable(dev, 852000000)) > + if (disable_freq_if_available(dev, 852000000)) > dev_warn(dev, "failed to disable 852MHz OPP\n");
[...]
| |