[1/6] cpufreq: Use existing stub functions instead of IS_ENABLED macro

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
Related show

Commit Message

Daniel Lezcano June 21, 2019, 1:22 p.m.
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

Comments

Rafael J. Wysocki June 22, 2019, 9:12 a.m. | #1
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

>
Viresh Kumar June 24, 2019, 6:06 a.m. | #2
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
Daniel Lezcano June 24, 2019, 7:45 a.m. | #3
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
Daniel Lezcano June 24, 2019, 8:53 a.m. | #4
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
Rafael J. Wysocki June 24, 2019, 9 a.m. | #5
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?
Daniel Lezcano June 24, 2019, 9:07 a.m. | #6
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
Rafael J. Wysocki June 24, 2019, 9:30 a.m. | #7
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().
Daniel Lezcano June 24, 2019, 9:37 a.m. | #8
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
Viresh Kumar June 24, 2019, 9:39 a.m. | #9
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

Patch

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;
 	}