| Message ID | 20190621132302.30414-1-daniel.lezcano@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [1/6] cpufreq: Use existing stub functions instead of IS_ENABLED macro | expand |
On Fri, Jun 21, 2019 at 3:23 PM Daniel Lezcano <daniel.lezcano@linaro.org> wrote: > > The functions stub already exist for the condition the IS_ENABLED > is trying to avoid. > > Remove the IS_ENABLED macros as they are pointless. AFAICS, the IS_ENABLED checks are an optimization to avoid generating pointless code (including a branch) in case CONFIG_CPU_THERMAL is not set. Why do you think that it is not useful? > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> > --- > drivers/cpufreq/cpufreq.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > index 85ff958e01f1..7c72f7d3509c 100644 > --- a/drivers/cpufreq/cpufreq.c > +++ b/drivers/cpufreq/cpufreq.c > @@ -1378,8 +1378,7 @@ static int cpufreq_online(unsigned int cpu) > if (cpufreq_driver->ready) > cpufreq_driver->ready(policy); > > - if (IS_ENABLED(CONFIG_CPU_THERMAL) && > - cpufreq_driver->flags & CPUFREQ_IS_COOLING_DEV) > + if (cpufreq_driver->flags & CPUFREQ_IS_COOLING_DEV) > policy->cdev = of_cpufreq_cooling_register(policy); > > pr_debug("initialization complete\n"); > @@ -1469,8 +1468,7 @@ static int cpufreq_offline(unsigned int cpu) > goto unlock; > } > > - if (IS_ENABLED(CONFIG_CPU_THERMAL) && > - cpufreq_driver->flags & CPUFREQ_IS_COOLING_DEV) { > + if (cpufreq_driver->flags & CPUFREQ_IS_COOLING_DEV) { > cpufreq_cooling_unregister(policy->cdev); > policy->cdev = NULL; > } > -- > 2.17.1 >
On 21-06-19, 15:23, Daniel Lezcano wrote: > The cpufreq_cooling_unregister() function uses now the policy to > unregister itself. The only purpose of the cooling device pointer is > to unregister the cpu cooling device. > > As there is no more need of this pointer, remove it. > > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> > --- > drivers/thermal/ti-soc-thermal/ti-thermal-common.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/thermal/ti-soc-thermal/ti-thermal-common.c b/drivers/thermal/ti-soc-thermal/ti-thermal-common.c > index 217b1aae8b4f..170b70b6ec61 100644 > --- a/drivers/thermal/ti-soc-thermal/ti-thermal-common.c > +++ b/drivers/thermal/ti-soc-thermal/ti-thermal-common.c > @@ -41,7 +41,6 @@ struct ti_thermal_data { > struct cpufreq_policy *policy; > struct thermal_zone_device *ti_thermal; > struct thermal_zone_device *pcb_tz; > - struct thermal_cooling_device *cool_dev; > struct ti_bandgap *bgp; > enum thermal_device_mode mode; > struct work_struct thermal_wq; > @@ -233,6 +232,7 @@ int ti_thermal_register_cpu_cooling(struct ti_bandgap *bgp, int id) > { > struct ti_thermal_data *data; > struct device_node *np = bgp->dev->of_node; > + struct thermal_cooling_device *cdev; > > /* > * We are assuming here that if one deploys the zone > @@ -256,9 +256,9 @@ int ti_thermal_register_cpu_cooling(struct ti_bandgap *bgp, int id) > } > > /* Register cooling device */ > - data->cool_dev = cpufreq_cooling_register(data->policy); > - if (IS_ERR(data->cool_dev)) { > - int ret = PTR_ERR(data->cool_dev); > + cdev = cpufreq_cooling_register(data->policy); > + if (IS_ERR(cdev)) { > + int ret = PTR_ERR(cdev); > dev_err(bgp->dev, "Failed to register cpu cooling device %d\n", > ret); > cpufreq_cpu_put(data->policy); And this too.. Acked-by: Viresh Kumar <viresh.kumar@linaro.org> -- viresh
On 24/06/2019 09:37, Viresh Kumar wrote: > On 24-06-19, 09:30, Daniel Lezcano wrote: >> On 24/06/2019 08:03, Viresh Kumar wrote: >>> On 21-06-19, 15:22, Daniel Lezcano wrote: >>>> Currently the function cpufreq_cooling_register() returns a cooling >>>> device pointer which is used back as a pointer to call the function >>>> cpufreq_cooling_unregister(). Even if it is correct, it would make >>>> sense to not leak the structure inside a cpufreq driver and keep the >>>> code thermal code self-encapsulate. Moreover, that forces to add an >>>> extra variable in each driver using this function. >>>> >>>> Instead of passing the cooling device to unregister, pass the policy. >>>> >>>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> >>>> --- >>>> drivers/cpufreq/arm_big_little.c | 2 +- >>>> drivers/cpufreq/cpufreq.c | 2 +- >>>> drivers/thermal/cpu_cooling.c | 18 ++++++++++-------- >>>> drivers/thermal/imx_thermal.c | 4 ++-- >>>> .../thermal/ti-soc-thermal/ti-thermal-common.c | 2 +- >>>> include/linux/cpu_cooling.h | 6 +++--- >>>> 6 files changed, 18 insertions(+), 16 deletions(-) >>> >>> Acked-by: Viresh Kumar <viresh.kumar@linaro.org> >> >> Just a side note, does it make sense to have the function called from >> imx_thermal.c and ti-thermal-common.c? Sounds like also a leakage from >> cpufreq to thermal drivers, no? > > I am not sure what you are proposing here :) Actually I'm asking your opinion :) The structure in drivers/thermal/imx_thermal.c struct imx_thermal_data { struct cpufreq_policy *policy; <<<< in the thermal data ?! [ ... ] }; And then: #ifdef CONFIG_CPU_FREQ /* * Create cooling device in case no #cooling-cells property is available in * CPU node */ static int imx_thermal_register_legacy_cooling(struct imx_thermal_data *data) { struct device_node *np; int ret; data->policy = cpufreq_cpu_get(0); if (!data->policy) { pr_debug("%s: CPUFreq policy not found\n", __func__); return -EPROBE_DEFER; } np = of_get_cpu_node(data->policy->cpu, NULL); if (!np || !of_find_property(np, "#cooling-cells", NULL)) { data->cdev = cpufreq_cooling_register(data->policy); if (IS_ERR(data->cdev)) { ret = PTR_ERR(data->cdev); cpufreq_cpu_put(data->policy); return ret; } } return 0; } [ ... ] Shouldn't this be move in the drivers/cpufreq/<whatever driver> ? -- <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | <http://twitter.com/#!/linaroorg> Twitter | <http://www.linaro.org/linaro-blog/> Blog
Hi Viresh, On 21/06/2019 15:22, Daniel Lezcano wrote: > The functions stub already exist for the condition the IS_ENABLED > is trying to avoid. > > Remove the IS_ENABLED macros as they are pointless. > > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> what about this one? > --- > drivers/cpufreq/cpufreq.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > index 85ff958e01f1..7c72f7d3509c 100644 > --- a/drivers/cpufreq/cpufreq.c > +++ b/drivers/cpufreq/cpufreq.c > @@ -1378,8 +1378,7 @@ static int cpufreq_online(unsigned int cpu) > if (cpufreq_driver->ready) > cpufreq_driver->ready(policy); > > - if (IS_ENABLED(CONFIG_CPU_THERMAL) && > - cpufreq_driver->flags & CPUFREQ_IS_COOLING_DEV) > + if (cpufreq_driver->flags & CPUFREQ_IS_COOLING_DEV) > policy->cdev = of_cpufreq_cooling_register(policy); > > pr_debug("initialization complete\n"); > @@ -1469,8 +1468,7 @@ static int cpufreq_offline(unsigned int cpu) > goto unlock; > } > > - if (IS_ENABLED(CONFIG_CPU_THERMAL) && > - cpufreq_driver->flags & CPUFREQ_IS_COOLING_DEV) { > + if (cpufreq_driver->flags & CPUFREQ_IS_COOLING_DEV) { > cpufreq_cooling_unregister(policy->cdev); > policy->cdev = NULL; > } > -- <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | <http://twitter.com/#!/linaroorg> Twitter | <http://www.linaro.org/linaro-blog/> Blog
On Mon, Jun 24, 2019 at 10:53 AM Daniel Lezcano <daniel.lezcano@linaro.org> wrote: > > > Hi Viresh, > > On 21/06/2019 15:22, Daniel Lezcano wrote: > > The functions stub already exist for the condition the IS_ENABLED > > is trying to avoid. > > > > Remove the IS_ENABLED macros as they are pointless. > > > > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> > > what about this one? Care to respond to my question regarding this one in the first place, please?
Hi Rafael, On 24/06/2019 11:00, Rafael J. Wysocki wrote: > On Mon, Jun 24, 2019 at 10:53 AM Daniel Lezcano > <daniel.lezcano@linaro.org> wrote: >> >> >> Hi Viresh, >> >> On 21/06/2019 15:22, Daniel Lezcano wrote: >>> The functions stub already exist for the condition the IS_ENABLED >>> is trying to avoid. >>> >>> Remove the IS_ENABLED macros as they are pointless. >>> >>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> >> >> what about this one? > > Care to respond to my question regarding this one in the first place, please? Ah yes, sorry, I missed your email. -- <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | <http://twitter.com/#!/linaroorg> Twitter | <http://www.linaro.org/linaro-blog/> Blog
On Monday, June 24, 2019 11:22:19 AM CEST Daniel Lezcano wrote: > On 22/06/2019 11:12, Rafael J. Wysocki wrote: > > On Fri, Jun 21, 2019 at 3:23 PM Daniel Lezcano > > <daniel.lezcano@linaro.org> wrote: > >> > >> The functions stub already exist for the condition the IS_ENABLED > >> is trying to avoid. > >> > >> Remove the IS_ENABLED macros as they are pointless. > > > > AFAICS, the IS_ENABLED checks are an optimization to avoid generating > > pointless code (including a branch) in case CONFIG_CPU_THERMAL is not > > set. > > > > Why do you think that it is not useful? > > I agree but I'm not a big fan of IS_ENABLED macros in the code when it > is possible to avoid them. > > What about adding a stub for that like: Well, > #ifdef CPU_THERMAL > static inline int cpufreq_is_cooling_dev(struct cpufreq_driver *drv) > { > return drv->flags & CPUFREQ_IS_COOLING_DEV; > } > #else > static inline int cpufreq_is_cooling_dev(struct cpufreq_driver *drv) > { > return 0; > } > #endif This may as well be defined as static inline int cpufreq_is_cooling_dev(struct cpufreq_driver *drv) { return IS_ENABLED(CPU_THERMAL) && drv->flags & CPUFREQ_IS_COOLING_DEV; } which is fewer lines of code. And I would call it something like cpufreq_thermal_control_enabled().
On 24/06/2019 11:30, Rafael J. Wysocki wrote: > On Monday, June 24, 2019 11:22:19 AM CEST Daniel Lezcano wrote: >> On 22/06/2019 11:12, Rafael J. Wysocki wrote: >>> On Fri, Jun 21, 2019 at 3:23 PM Daniel Lezcano >>> <daniel.lezcano@linaro.org> wrote: >>>> >>>> The functions stub already exist for the condition the IS_ENABLED >>>> is trying to avoid. >>>> >>>> Remove the IS_ENABLED macros as they are pointless. >>> >>> AFAICS, the IS_ENABLED checks are an optimization to avoid generating >>> pointless code (including a branch) in case CONFIG_CPU_THERMAL is not >>> set. >>> >>> Why do you think that it is not useful? >> >> I agree but I'm not a big fan of IS_ENABLED macros in the code when it >> is possible to avoid them. >> >> What about adding a stub for that like: > > Well, > >> #ifdef CPU_THERMAL >> static inline int cpufreq_is_cooling_dev(struct cpufreq_driver *drv) >> { >> return drv->flags & CPUFREQ_IS_COOLING_DEV; >> } >> #else >> static inline int cpufreq_is_cooling_dev(struct cpufreq_driver *drv) >> { >> return 0; >> } >> #endif > > This may as well be defined as > > static inline int cpufreq_is_cooling_dev(struct cpufreq_driver *drv) > { > return IS_ENABLED(CPU_THERMAL) && drv->flags & CPUFREQ_IS_COOLING_DEV; > } > > which is fewer lines of code. Ah yes, even better. > And I would call it something like cpufreq_thermal_control_enabled(). Ok, thanks! -- <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | <http://twitter.com/#!/linaroorg> Twitter | <http://www.linaro.org/linaro-blog/> Blog
On 24-06-19, 09:45, Daniel Lezcano wrote: > Actually I'm asking your opinion :) > > The structure in drivers/thermal/imx_thermal.c > > struct imx_thermal_data { > struct cpufreq_policy *policy; <<<< in the thermal data ?! > [ ... ] > }; > > And then: > > #ifdef CONFIG_CPU_FREQ > /* > * Create cooling device in case no #cooling-cells property is available in > * CPU node > */ > static int imx_thermal_register_legacy_cooling(struct imx_thermal_data > *data) > { > struct device_node *np; > int ret; > > data->policy = cpufreq_cpu_get(0); > if (!data->policy) { > pr_debug("%s: CPUFreq policy not found\n", __func__); > return -EPROBE_DEFER; > } > > np = of_get_cpu_node(data->policy->cpu, NULL); > > if (!np || !of_find_property(np, "#cooling-cells", NULL)) { > data->cdev = cpufreq_cooling_register(data->policy); > if (IS_ERR(data->cdev)) { > ret = PTR_ERR(data->cdev); > cpufreq_cpu_put(data->policy); > return ret; > } > } > > return 0; > } > > [ ... ] > > Shouldn't this be move in the drivers/cpufreq/<whatever driver> ? Sure, we have platform specific drivers where this can be moved :) -- viresh
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index 85ff958e01f1..7c72f7d3509c 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -1378,8 +1378,7 @@ static int cpufreq_online(unsigned int cpu) if (cpufreq_driver->ready) cpufreq_driver->ready(policy); - if (IS_ENABLED(CONFIG_CPU_THERMAL) && - cpufreq_driver->flags & CPUFREQ_IS_COOLING_DEV) + if (cpufreq_driver->flags & CPUFREQ_IS_COOLING_DEV) policy->cdev = of_cpufreq_cooling_register(policy); pr_debug("initialization complete\n"); @@ -1469,8 +1468,7 @@ static int cpufreq_offline(unsigned int cpu) goto unlock; } - if (IS_ENABLED(CONFIG_CPU_THERMAL) && - cpufreq_driver->flags & CPUFREQ_IS_COOLING_DEV) { + if (cpufreq_driver->flags & CPUFREQ_IS_COOLING_DEV) { cpufreq_cooling_unregister(policy->cdev); policy->cdev = NULL; }
The functions stub already exist for the condition the IS_ENABLED is trying to avoid. Remove the IS_ENABLED macros as they are pointless. Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> --- drivers/cpufreq/cpufreq.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) -- 2.17.1