Linux-PM Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 1/6] cpufreq: Use existing stub functions instead of IS_ENABLED macro
@ 2019-06-21 13:22 Daniel Lezcano
  2019-06-21 13:22 ` [PATCH 2/6] thermal/drivers/cpu_cooling: Unregister with the policy Daniel Lezcano
                   ` (6 more replies)
  0 siblings, 7 replies; 23+ messages in thread
From: Daniel Lezcano @ 2019-06-21 13:22 UTC (permalink / raw)
  To: viresh.kumar; +Cc: edubezval, linux-kernel, Rafael J. Wysocki, linux-pm

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(-)

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


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH 2/6] thermal/drivers/cpu_cooling: Unregister with the policy
  2019-06-21 13:22 [PATCH 1/6] cpufreq: Use existing stub functions instead of IS_ENABLED macro Daniel Lezcano
@ 2019-06-21 13:22 ` Daniel Lezcano
  2019-06-24  6:03   ` Viresh Kumar
  2019-06-21 13:22 ` [PATCH 3/6] cpufreq/drivers/arm_big_little: Remove cooling device usage Daniel Lezcano
                   ` (5 subsequent siblings)
  6 siblings, 1 reply; 23+ messages in thread
From: Daniel Lezcano @ 2019-06-21 13:22 UTC (permalink / raw)
  To: viresh.kumar
  Cc: edubezval, linux-kernel, Sudeep Holla, Rafael J. Wysocki,
	Amit Daniel Kachhap, Javi Merino, Zhang Rui, Shawn Guo,
	Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
	NXP Linux Team, Keerthy, linux-pm, linux-arm-kernel, linux-omap

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(-)

diff --git a/drivers/cpufreq/arm_big_little.c b/drivers/cpufreq/arm_big_little.c
index 7fe52fcddcf1..6b243202caa9 100644
--- a/drivers/cpufreq/arm_big_little.c
+++ b/drivers/cpufreq/arm_big_little.c
@@ -502,7 +502,7 @@ static int bL_cpufreq_exit(struct cpufreq_policy *policy)
 	int cur_cluster = cpu_to_cluster(policy->cpu);
 
 	if (cur_cluster < MAX_CLUSTERS) {
-		cpufreq_cooling_unregister(cdev[cur_cluster]);
+		cpufreq_cooling_unregister(policy);
 		cdev[cur_cluster] = NULL;
 	}
 
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 7c72f7d3509c..dfbc9bea606c 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1469,7 +1469,7 @@ static int cpufreq_offline(unsigned int cpu)
 	}
 
 	if (cpufreq_driver->flags & CPUFREQ_IS_COOLING_DEV) {
-		cpufreq_cooling_unregister(policy->cdev);
+		cpufreq_cooling_unregister(policy);
 		policy->cdev = NULL;
 	}
 
diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
index 83486775e593..007c7c6bf845 100644
--- a/drivers/thermal/cpu_cooling.c
+++ b/drivers/thermal/cpu_cooling.c
@@ -78,6 +78,7 @@ struct cpufreq_cooling_device {
 	struct cpufreq_policy *policy;
 	struct list_head node;
 	struct time_in_idle *idle_time;
+	struct thermal_cooling_device *cdev;
 };
 
 static DEFINE_IDA(cpufreq_ida);
@@ -606,6 +607,7 @@ __cpufreq_cooling_register(struct device_node *np,
 		goto remove_ida;
 
 	cpufreq_cdev->clipped_freq = get_state_freq(cpufreq_cdev, 0);
+	cpufreq_cdev->cdev = cdev;
 
 	mutex_lock(&cooling_list_lock);
 	/* Register the notifier for first cpufreq cooling device */
@@ -699,18 +701,18 @@ EXPORT_SYMBOL_GPL(of_cpufreq_cooling_register);
  *
  * This interface function unregisters the "thermal-cpufreq-%x" cooling device.
  */
-void cpufreq_cooling_unregister(struct thermal_cooling_device *cdev)
+void cpufreq_cooling_unregister(struct cpufreq_policy *policy)
 {
 	struct cpufreq_cooling_device *cpufreq_cdev;
 	bool last;
 
-	if (!cdev)
-		return;
-
-	cpufreq_cdev = cdev->devdata;
-
 	mutex_lock(&cooling_list_lock);
-	list_del(&cpufreq_cdev->node);
+	list_for_each_entry(cpufreq_cdev, &cpufreq_cdev_list, node) {
+		if (cpufreq_cdev->policy == policy) {
+			list_del(&cpufreq_cdev->node);
+			break;
+		}
+	}
 	/* Unregister the notifier for the last cpufreq cooling device */
 	last = list_empty(&cpufreq_cdev_list);
 	mutex_unlock(&cooling_list_lock);
@@ -719,7 +721,7 @@ void cpufreq_cooling_unregister(struct thermal_cooling_device *cdev)
 		cpufreq_unregister_notifier(&thermal_cpufreq_notifier_block,
 					    CPUFREQ_POLICY_NOTIFIER);
 
-	thermal_cooling_device_unregister(cdev);
+	thermal_cooling_device_unregister(cpufreq_cdev->cdev);
 	ida_simple_remove(&cpufreq_ida, cpufreq_cdev->id);
 	kfree(cpufreq_cdev->idle_time);
 	kfree(cpufreq_cdev);
diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c
index bb6754a5342c..6746f1b73eb7 100644
--- a/drivers/thermal/imx_thermal.c
+++ b/drivers/thermal/imx_thermal.c
@@ -680,7 +680,7 @@ static int imx_thermal_register_legacy_cooling(struct imx_thermal_data *data)
 
 static void imx_thermal_unregister_legacy_cooling(struct imx_thermal_data *data)
 {
-	cpufreq_cooling_unregister(data->cdev);
+	cpufreq_cooling_unregister(data->policy);
 	cpufreq_cpu_put(data->policy);
 }
 
@@ -872,7 +872,7 @@ static int imx_thermal_remove(struct platform_device *pdev)
 		clk_disable_unprepare(data->thermal_clk);
 
 	thermal_zone_device_unregister(data->tz);
-	cpufreq_cooling_unregister(data->cdev);
+	cpufreq_cooling_unregister(data->policy);
 	cpufreq_cpu_put(data->policy);
 
 	return 0;
diff --git a/drivers/thermal/ti-soc-thermal/ti-thermal-common.c b/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
index b4f981daeaf2..217b1aae8b4f 100644
--- a/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
+++ b/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
@@ -277,7 +277,7 @@ int ti_thermal_unregister_cpu_cooling(struct ti_bandgap *bgp, int id)
 	data = ti_bandgap_get_sensor_data(bgp, id);
 
 	if (data) {
-		cpufreq_cooling_unregister(data->cool_dev);
+		cpufreq_cooling_unregister(data->policy);
 		if (data->policy)
 			cpufreq_cpu_put(data->policy);
 	}
diff --git a/include/linux/cpu_cooling.h b/include/linux/cpu_cooling.h
index bae54bb7c048..89f469ee4be4 100644
--- a/include/linux/cpu_cooling.h
+++ b/include/linux/cpu_cooling.h
@@ -29,9 +29,9 @@ cpufreq_cooling_register(struct cpufreq_policy *policy);
 
 /**
  * cpufreq_cooling_unregister - function to remove cpufreq cooling device.
- * @cdev: thermal cooling device pointer.
+ * @policy: cpufreq policy
  */
-void cpufreq_cooling_unregister(struct thermal_cooling_device *cdev);
+void cpufreq_cooling_unregister(struct cpufreq_policy *policy);
 
 #else /* !CONFIG_CPU_THERMAL */
 static inline struct thermal_cooling_device *
@@ -41,7 +41,7 @@ cpufreq_cooling_register(struct cpufreq_policy *policy)
 }
 
 static inline
-void cpufreq_cooling_unregister(struct thermal_cooling_device *cdev)
+void cpufreq_cooling_unregister(struct cpufreq_policy *policy)
 {
 	return;
 }
-- 
2.17.1


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH 3/6] cpufreq/drivers/arm_big_little: Remove cooling device usage
  2019-06-21 13:22 [PATCH 1/6] cpufreq: Use existing stub functions instead of IS_ENABLED macro Daniel Lezcano
  2019-06-21 13:22 ` [PATCH 2/6] thermal/drivers/cpu_cooling: Unregister with the policy Daniel Lezcano
@ 2019-06-21 13:22 ` Daniel Lezcano
  2019-06-24  6:04   ` Viresh Kumar
  2019-06-21 13:23 ` [PATCH 4/6] cpufreq: " Daniel Lezcano
                   ` (4 subsequent siblings)
  6 siblings, 1 reply; 23+ messages in thread
From: Daniel Lezcano @ 2019-06-21 13:22 UTC (permalink / raw)
  To: viresh.kumar
  Cc: edubezval, linux-kernel, Sudeep Holla, Rafael J. Wysocki, linux-pm

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/cpufreq/arm_big_little.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/cpufreq/arm_big_little.c b/drivers/cpufreq/arm_big_little.c
index 6b243202caa9..718c63231e66 100644
--- a/drivers/cpufreq/arm_big_little.c
+++ b/drivers/cpufreq/arm_big_little.c
@@ -56,7 +56,6 @@ static bool bL_switching_enabled;
 #define ACTUAL_FREQ(cluster, freq)  ((cluster == A7_CLUSTER) ? freq << 1 : freq)
 #define VIRT_FREQ(cluster, freq)    ((cluster == A7_CLUSTER) ? freq >> 1 : freq)
 
-static struct thermal_cooling_device *cdev[MAX_CLUSTERS];
 static const struct cpufreq_arm_bL_ops *arm_bL_ops;
 static struct clk *clk[MAX_CLUSTERS];
 static struct cpufreq_frequency_table *freq_table[MAX_CLUSTERS + 1];
@@ -501,10 +500,8 @@ static int bL_cpufreq_exit(struct cpufreq_policy *policy)
 	struct device *cpu_dev;
 	int cur_cluster = cpu_to_cluster(policy->cpu);
 
-	if (cur_cluster < MAX_CLUSTERS) {
+	if (cur_cluster < MAX_CLUSTERS)
 		cpufreq_cooling_unregister(policy);
-		cdev[cur_cluster] = NULL;
-	}
 
 	cpu_dev = get_cpu_device(policy->cpu);
 	if (!cpu_dev) {
@@ -527,7 +524,7 @@ static void bL_cpufreq_ready(struct cpufreq_policy *policy)
 	if (cur_cluster >= MAX_CLUSTERS)
 		return;
 
-	cdev[cur_cluster] = of_cpufreq_cooling_register(policy);
+	of_cpufreq_cooling_register(policy);
 }
 
 static struct cpufreq_driver bL_cpufreq_driver = {
-- 
2.17.1


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH 4/6] cpufreq:  Remove cooling device usage
  2019-06-21 13:22 [PATCH 1/6] cpufreq: Use existing stub functions instead of IS_ENABLED macro Daniel Lezcano
  2019-06-21 13:22 ` [PATCH 2/6] thermal/drivers/cpu_cooling: Unregister with the policy Daniel Lezcano
  2019-06-21 13:22 ` [PATCH 3/6] cpufreq/drivers/arm_big_little: Remove cooling device usage Daniel Lezcano
@ 2019-06-21 13:23 ` " Daniel Lezcano
  2019-06-24  6:05   ` Viresh Kumar
  2019-06-21 13:23 ` [PATCH 5/6] thermal/drivers/imx: " Daniel Lezcano
                   ` (3 subsequent siblings)
  6 siblings, 1 reply; 23+ messages in thread
From: Daniel Lezcano @ 2019-06-21 13:23 UTC (permalink / raw)
  To: viresh.kumar; +Cc: edubezval, linux-kernel, Rafael J. Wysocki, linux-pm

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/cpufreq/cpufreq.c | 6 ++----
 include/linux/cpufreq.h   | 3 ---
 2 files changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index dfbc9bea606c..1d8f85faeaca 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1379,7 +1379,7 @@ static int cpufreq_online(unsigned int cpu)
 		cpufreq_driver->ready(policy);
 
 	if (cpufreq_driver->flags & CPUFREQ_IS_COOLING_DEV)
-		policy->cdev = of_cpufreq_cooling_register(policy);
+		of_cpufreq_cooling_register(policy);
 
 	pr_debug("initialization complete\n");
 
@@ -1468,10 +1468,8 @@ static int cpufreq_offline(unsigned int cpu)
 		goto unlock;
 	}
 
-	if (cpufreq_driver->flags & CPUFREQ_IS_COOLING_DEV) {
+	if (cpufreq_driver->flags & CPUFREQ_IS_COOLING_DEV)
 		cpufreq_cooling_unregister(policy);
-		policy->cdev = NULL;
-	}
 
 	if (cpufreq_driver->stop_cpu)
 		cpufreq_driver->stop_cpu(policy);
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index d01a74fbc4db..9a42711f338b 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -144,9 +144,6 @@ struct cpufreq_policy {
 
 	/* For cpufreq driver's internal use */
 	void			*driver_data;
-
-	/* Pointer to the cooling device if used for thermal mitigation */
-	struct thermal_cooling_device *cdev;
 };
 
 struct cpufreq_freqs {
-- 
2.17.1


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH 5/6] thermal/drivers/imx: Remove cooling device usage
  2019-06-21 13:22 [PATCH 1/6] cpufreq: Use existing stub functions instead of IS_ENABLED macro Daniel Lezcano
                   ` (2 preceding siblings ...)
  2019-06-21 13:23 ` [PATCH 4/6] cpufreq: " Daniel Lezcano
@ 2019-06-21 13:23 ` " Daniel Lezcano
  2019-06-24  6:06   ` Viresh Kumar
  2019-06-21 13:23 ` [PATCH 6/6] thermal/drivers/ti: " Daniel Lezcano
                   ` (2 subsequent siblings)
  6 siblings, 1 reply; 23+ messages in thread
From: Daniel Lezcano @ 2019-06-21 13:23 UTC (permalink / raw)
  To: viresh.kumar
  Cc: edubezval, linux-kernel, Zhang Rui, Shawn Guo, Sascha Hauer,
	Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team, linux-pm,
	linux-arm-kernel

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/imx_thermal.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c
index 6746f1b73eb7..021c0948b740 100644
--- a/drivers/thermal/imx_thermal.c
+++ b/drivers/thermal/imx_thermal.c
@@ -203,7 +203,6 @@ static struct thermal_soc_data thermal_imx7d_data = {
 struct imx_thermal_data {
 	struct cpufreq_policy *policy;
 	struct thermal_zone_device *tz;
-	struct thermal_cooling_device *cdev;
 	enum thermal_device_mode mode;
 	struct regmap *tempmon;
 	u32 c1, c2; /* See formula in imx_init_calib() */
@@ -656,6 +655,7 @@ MODULE_DEVICE_TABLE(of, of_imx_thermal_match);
 static int imx_thermal_register_legacy_cooling(struct imx_thermal_data *data)
 {
 	struct device_node *np;
+	struct thermal_cooling_device *cdev;
 	int ret;
 
 	data->policy = cpufreq_cpu_get(0);
@@ -667,9 +667,9 @@ static int imx_thermal_register_legacy_cooling(struct imx_thermal_data *data)
 	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);
+		cdev = cpufreq_cooling_register(data->policy);
+		if (IS_ERR(cdev)) {
+			ret = PTR_ERR(cdev);
 			cpufreq_cpu_put(data->policy);
 			return ret;
 		}
-- 
2.17.1


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH 6/6] thermal/drivers/ti: Remove cooling device usage
  2019-06-21 13:22 [PATCH 1/6] cpufreq: Use existing stub functions instead of IS_ENABLED macro Daniel Lezcano
                   ` (3 preceding siblings ...)
  2019-06-21 13:23 ` [PATCH 5/6] thermal/drivers/imx: " Daniel Lezcano
@ 2019-06-21 13:23 ` " Daniel Lezcano
  2019-06-24  6:06   ` Viresh Kumar
  2019-06-22  9:12 ` [PATCH 1/6] cpufreq: Use existing stub functions instead of IS_ENABLED macro Rafael J. Wysocki
  2019-06-24  8:53 ` Daniel Lezcano
  6 siblings, 1 reply; 23+ messages in thread
From: Daniel Lezcano @ 2019-06-21 13:23 UTC (permalink / raw)
  To: viresh.kumar
  Cc: edubezval, linux-kernel, Keerthy, Zhang Rui, linux-pm, linux-omap

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);
-- 
2.17.1


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 1/6] cpufreq: Use existing stub functions instead of IS_ENABLED macro
  2019-06-21 13:22 [PATCH 1/6] cpufreq: Use existing stub functions instead of IS_ENABLED macro Daniel Lezcano
                   ` (4 preceding siblings ...)
  2019-06-21 13:23 ` [PATCH 6/6] thermal/drivers/ti: " Daniel Lezcano
@ 2019-06-22  9:12 ` Rafael J. Wysocki
  2019-06-24  9:22   ` Daniel Lezcano
  2019-06-24  8:53 ` Daniel Lezcano
  6 siblings, 1 reply; 23+ messages in thread
From: Rafael J. Wysocki @ 2019-06-22  9:12 UTC (permalink / raw)
  To: Daniel Lezcano
  Cc: Viresh Kumar, Eduardo Valentin, Linux Kernel Mailing List,
	Rafael J. Wysocki, open list:CPU FREQUENCY SCALING FRAMEWORK

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
>

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 2/6] thermal/drivers/cpu_cooling: Unregister with the policy
  2019-06-21 13:22 ` [PATCH 2/6] thermal/drivers/cpu_cooling: Unregister with the policy Daniel Lezcano
@ 2019-06-24  6:03   ` Viresh Kumar
  2019-06-24  7:30     ` Daniel Lezcano
  0 siblings, 1 reply; 23+ messages in thread
From: Viresh Kumar @ 2019-06-24  6:03 UTC (permalink / raw)
  To: Daniel Lezcano
  Cc: edubezval, linux-kernel, Sudeep Holla, Rafael J. Wysocki,
	Amit Daniel Kachhap, Javi Merino, Zhang Rui, Shawn Guo,
	Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
	NXP Linux Team, Keerthy,
	open list:CPU FREQUENCY DRIVERS - ARM BIG LITTLE,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
	open list:TI BANDGAP AND THERMAL DRIVER

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>

-- 
viresh

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 3/6] cpufreq/drivers/arm_big_little: Remove cooling device usage
  2019-06-21 13:22 ` [PATCH 3/6] cpufreq/drivers/arm_big_little: Remove cooling device usage Daniel Lezcano
@ 2019-06-24  6:04   ` Viresh Kumar
  0 siblings, 0 replies; 23+ messages in thread
From: Viresh Kumar @ 2019-06-24  6:04 UTC (permalink / raw)
  To: Daniel Lezcano
  Cc: edubezval, linux-kernel, Sudeep Holla, Rafael J. Wysocki,
	open list:CPU FREQUENCY DRIVERS - ARM BIG LITTLE

On 21-06-19, 15:22, 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/cpufreq/arm_big_little.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/cpufreq/arm_big_little.c b/drivers/cpufreq/arm_big_little.c
> index 6b243202caa9..718c63231e66 100644
> --- a/drivers/cpufreq/arm_big_little.c
> +++ b/drivers/cpufreq/arm_big_little.c
> @@ -56,7 +56,6 @@ static bool bL_switching_enabled;
>  #define ACTUAL_FREQ(cluster, freq)  ((cluster == A7_CLUSTER) ? freq << 1 : freq)
>  #define VIRT_FREQ(cluster, freq)    ((cluster == A7_CLUSTER) ? freq >> 1 : freq)
>  
> -static struct thermal_cooling_device *cdev[MAX_CLUSTERS];
>  static const struct cpufreq_arm_bL_ops *arm_bL_ops;
>  static struct clk *clk[MAX_CLUSTERS];
>  static struct cpufreq_frequency_table *freq_table[MAX_CLUSTERS + 1];
> @@ -501,10 +500,8 @@ static int bL_cpufreq_exit(struct cpufreq_policy *policy)
>  	struct device *cpu_dev;
>  	int cur_cluster = cpu_to_cluster(policy->cpu);
>  
> -	if (cur_cluster < MAX_CLUSTERS) {
> +	if (cur_cluster < MAX_CLUSTERS)
>  		cpufreq_cooling_unregister(policy);
> -		cdev[cur_cluster] = NULL;
> -	}
>  
>  	cpu_dev = get_cpu_device(policy->cpu);
>  	if (!cpu_dev) {
> @@ -527,7 +524,7 @@ static void bL_cpufreq_ready(struct cpufreq_policy *policy)
>  	if (cur_cluster >= MAX_CLUSTERS)
>  		return;
>  
> -	cdev[cur_cluster] = of_cpufreq_cooling_register(policy);
> +	of_cpufreq_cooling_register(policy);
>  }
>  
>  static struct cpufreq_driver bL_cpufreq_driver = {

I will merge it with the previous commit if I were you.

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

-- 
viresh

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 4/6] cpufreq:  Remove cooling device usage
  2019-06-21 13:23 ` [PATCH 4/6] cpufreq: " Daniel Lezcano
@ 2019-06-24  6:05   ` Viresh Kumar
  0 siblings, 0 replies; 23+ messages in thread
From: Viresh Kumar @ 2019-06-24  6:05 UTC (permalink / raw)
  To: Daniel Lezcano
  Cc: edubezval, linux-kernel, Rafael J. Wysocki,
	open list:CPU FREQUENCY SCALING FRAMEWORK

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/cpufreq/cpufreq.c | 6 ++----
>  include/linux/cpufreq.h   | 3 ---
>  2 files changed, 2 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index dfbc9bea606c..1d8f85faeaca 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -1379,7 +1379,7 @@ static int cpufreq_online(unsigned int cpu)
>  		cpufreq_driver->ready(policy);
>  
>  	if (cpufreq_driver->flags & CPUFREQ_IS_COOLING_DEV)
> -		policy->cdev = of_cpufreq_cooling_register(policy);
> +		of_cpufreq_cooling_register(policy);
>  
>  	pr_debug("initialization complete\n");
>  
> @@ -1468,10 +1468,8 @@ static int cpufreq_offline(unsigned int cpu)
>  		goto unlock;
>  	}
>  
> -	if (cpufreq_driver->flags & CPUFREQ_IS_COOLING_DEV) {
> +	if (cpufreq_driver->flags & CPUFREQ_IS_COOLING_DEV)
>  		cpufreq_cooling_unregister(policy);
> -		policy->cdev = NULL;
> -	}
>  
>  	if (cpufreq_driver->stop_cpu)
>  		cpufreq_driver->stop_cpu(policy);
> diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
> index d01a74fbc4db..9a42711f338b 100644
> --- a/include/linux/cpufreq.h
> +++ b/include/linux/cpufreq.h
> @@ -144,9 +144,6 @@ struct cpufreq_policy {
>  
>  	/* For cpufreq driver's internal use */
>  	void			*driver_data;
> -
> -	/* Pointer to the cooling device if used for thermal mitigation */
> -	struct thermal_cooling_device *cdev;
>  };
>  
>  struct cpufreq_freqs {

This too. In my view this should be merged with 2/6.

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

-- 
viresh

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 5/6] thermal/drivers/imx: Remove cooling device usage
  2019-06-21 13:23 ` [PATCH 5/6] thermal/drivers/imx: " Daniel Lezcano
@ 2019-06-24  6:06   ` Viresh Kumar
  0 siblings, 0 replies; 23+ messages in thread
From: Viresh Kumar @ 2019-06-24  6:06 UTC (permalink / raw)
  To: Daniel Lezcano
  Cc: edubezval, linux-kernel, Zhang Rui, Shawn Guo, Sascha Hauer,
	Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team,
	open list:THERMAL,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE

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/imx_thermal.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c
> index 6746f1b73eb7..021c0948b740 100644
> --- a/drivers/thermal/imx_thermal.c
> +++ b/drivers/thermal/imx_thermal.c
> @@ -203,7 +203,6 @@ static struct thermal_soc_data thermal_imx7d_data = {
>  struct imx_thermal_data {
>  	struct cpufreq_policy *policy;
>  	struct thermal_zone_device *tz;
> -	struct thermal_cooling_device *cdev;
>  	enum thermal_device_mode mode;
>  	struct regmap *tempmon;
>  	u32 c1, c2; /* See formula in imx_init_calib() */
> @@ -656,6 +655,7 @@ MODULE_DEVICE_TABLE(of, of_imx_thermal_match);
>  static int imx_thermal_register_legacy_cooling(struct imx_thermal_data *data)
>  {
>  	struct device_node *np;
> +	struct thermal_cooling_device *cdev;
>  	int ret;
>  
>  	data->policy = cpufreq_cpu_get(0);
> @@ -667,9 +667,9 @@ static int imx_thermal_register_legacy_cooling(struct imx_thermal_data *data)
>  	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);
> +		cdev = cpufreq_cooling_register(data->policy);
> +		if (IS_ERR(cdev)) {
> +			ret = PTR_ERR(cdev);
>  			cpufreq_cpu_put(data->policy);
>  			return ret;
>  		}

This too..

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

-- 
viresh

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 6/6] thermal/drivers/ti: Remove cooling device usage
  2019-06-21 13:23 ` [PATCH 6/6] thermal/drivers/ti: " Daniel Lezcano
@ 2019-06-24  6:06   ` Viresh Kumar
  0 siblings, 0 replies; 23+ messages in thread
From: Viresh Kumar @ 2019-06-24  6:06 UTC (permalink / raw)
  To: Daniel Lezcano
  Cc: edubezval, linux-kernel, Keerthy, Zhang Rui,
	open list:TI BANDGAP AND THERMAL DRIVER,
	open list:TI BANDGAP AND THERMAL DRIVER

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

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 2/6] thermal/drivers/cpu_cooling: Unregister with the policy
  2019-06-24  6:03   ` Viresh Kumar
@ 2019-06-24  7:30     ` Daniel Lezcano
  2019-06-24  7:37       ` Viresh Kumar
  0 siblings, 1 reply; 23+ messages in thread
From: Daniel Lezcano @ 2019-06-24  7:30 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: edubezval, linux-kernel, Sudeep Holla, Rafael J. Wysocki,
	Amit Daniel Kachhap, Javi Merino, Zhang Rui, Shawn Guo,
	Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
	NXP Linux Team, Keerthy,
	open list:CPU FREQUENCY DRIVERS - ARM BIG LITTLE,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
	open list:TI BANDGAP AND THERMAL DRIVER

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?


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


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 2/6] thermal/drivers/cpu_cooling: Unregister with the policy
  2019-06-24  7:30     ` Daniel Lezcano
@ 2019-06-24  7:37       ` Viresh Kumar
  2019-06-24  7:45         ` Daniel Lezcano
  0 siblings, 1 reply; 23+ messages in thread
From: Viresh Kumar @ 2019-06-24  7:37 UTC (permalink / raw)
  To: Daniel Lezcano
  Cc: edubezval, linux-kernel, Sudeep Holla, Rafael J. Wysocki,
	Amit Daniel Kachhap, Javi Merino, Zhang Rui, Shawn Guo,
	Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
	NXP Linux Team, Keerthy,
	open list:CPU FREQUENCY DRIVERS - ARM BIG LITTLE,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
	open list:TI BANDGAP AND THERMAL DRIVER

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 :)

-- 
viresh

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 2/6] thermal/drivers/cpu_cooling: Unregister with the policy
  2019-06-24  7:37       ` Viresh Kumar
@ 2019-06-24  7:45         ` Daniel Lezcano
  2019-06-24  9:39           ` Viresh Kumar
  0 siblings, 1 reply; 23+ messages in thread
From: Daniel Lezcano @ 2019-06-24  7:45 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: edubezval, linux-kernel, Sudeep Holla, Rafael J. Wysocki,
	Amit Daniel Kachhap, Javi Merino, Zhang Rui, Shawn Guo,
	Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
	NXP Linux Team, Keerthy,
	open list:CPU FREQUENCY DRIVERS - ARM BIG LITTLE,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
	open list:TI BANDGAP AND THERMAL DRIVER

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


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 1/6] cpufreq: Use existing stub functions instead of IS_ENABLED macro
  2019-06-21 13:22 [PATCH 1/6] cpufreq: Use existing stub functions instead of IS_ENABLED macro Daniel Lezcano
                   ` (5 preceding siblings ...)
  2019-06-22  9:12 ` [PATCH 1/6] cpufreq: Use existing stub functions instead of IS_ENABLED macro Rafael J. Wysocki
@ 2019-06-24  8:53 ` Daniel Lezcano
  2019-06-24  9:00   ` Rafael J. Wysocki
  2019-06-24  9:08   ` Viresh Kumar
  6 siblings, 2 replies; 23+ messages in thread
From: Daniel Lezcano @ 2019-06-24  8:53 UTC (permalink / raw)
  To: viresh.kumar
  Cc: edubezval, linux-kernel, Rafael J. Wysocki,
	open list:CPU FREQUENCY SCALING FRAMEWORK


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


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 1/6] cpufreq: Use existing stub functions instead of IS_ENABLED macro
  2019-06-24  8:53 ` Daniel Lezcano
@ 2019-06-24  9:00   ` Rafael J. Wysocki
  2019-06-24  9:07     ` Daniel Lezcano
  2019-06-24  9:08   ` Viresh Kumar
  1 sibling, 1 reply; 23+ messages in thread
From: Rafael J. Wysocki @ 2019-06-24  9:00 UTC (permalink / raw)
  To: Daniel Lezcano
  Cc: Viresh Kumar, Eduardo Valentin, Linux Kernel Mailing List,
	Rafael J. Wysocki, open list:CPU FREQUENCY SCALING FRAMEWORK

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?

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 1/6] cpufreq: Use existing stub functions instead of IS_ENABLED macro
  2019-06-24  9:00   ` Rafael J. Wysocki
@ 2019-06-24  9:07     ` Daniel Lezcano
  0 siblings, 0 replies; 23+ messages in thread
From: Daniel Lezcano @ 2019-06-24  9:07 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Viresh Kumar, Eduardo Valentin, Linux Kernel Mailing List,
	Rafael J. Wysocki, open list:CPU FREQUENCY SCALING FRAMEWORK


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


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 1/6] cpufreq: Use existing stub functions instead of IS_ENABLED macro
  2019-06-24  8:53 ` Daniel Lezcano
  2019-06-24  9:00   ` Rafael J. Wysocki
@ 2019-06-24  9:08   ` Viresh Kumar
  1 sibling, 0 replies; 23+ messages in thread
From: Viresh Kumar @ 2019-06-24  9:08 UTC (permalink / raw)
  To: Daniel Lezcano
  Cc: edubezval, linux-kernel, Rafael J. Wysocki,
	open list:CPU FREQUENCY SCALING FRAMEWORK

On 24-06-19, 10:53, Daniel Lezcano 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?

I didn't say something because Rafael already pointed out something
which he wanted. Actually he was the one who suggested it to Amit
initially.

http://lore.kernel.org/lkml/CAJZ5v0huESFqOADqyym-ci-XcWpL4tbcd9a-jxe_KArXKpHFyw@mail.gmail.com

-- 
viresh

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 1/6] cpufreq: Use existing stub functions instead of IS_ENABLED macro
  2019-06-22  9:12 ` [PATCH 1/6] cpufreq: Use existing stub functions instead of IS_ENABLED macro Rafael J. Wysocki
@ 2019-06-24  9:22   ` Daniel Lezcano
  2019-06-24  9:30     ` Rafael J. Wysocki
  0 siblings, 1 reply; 23+ messages in thread
From: Daniel Lezcano @ 2019-06-24  9:22 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Viresh Kumar, Eduardo Valentin, Linux Kernel Mailing List,
	Rafael J. Wysocki, open list:CPU FREQUENCY SCALING FRAMEWORK

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:

#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

?

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


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


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 1/6] cpufreq: Use existing stub functions instead of IS_ENABLED macro
  2019-06-24  9:22   ` Daniel Lezcano
@ 2019-06-24  9:30     ` Rafael J. Wysocki
  2019-06-24  9:37       ` Daniel Lezcano
  0 siblings, 1 reply; 23+ messages in thread
From: Rafael J. Wysocki @ 2019-06-24  9:30 UTC (permalink / raw)
  To: Daniel Lezcano
  Cc: Rafael J. Wysocki, Viresh Kumar, Eduardo Valentin,
	Linux Kernel Mailing List,
	open list:CPU FREQUENCY SCALING FRAMEWORK

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().




^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 1/6] cpufreq: Use existing stub functions instead of IS_ENABLED macro
  2019-06-24  9:30     ` Rafael J. Wysocki
@ 2019-06-24  9:37       ` Daniel Lezcano
  0 siblings, 0 replies; 23+ messages in thread
From: Daniel Lezcano @ 2019-06-24  9:37 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Rafael J. Wysocki, Viresh Kumar, Eduardo Valentin,
	Linux Kernel Mailing List,
	open list:CPU FREQUENCY SCALING FRAMEWORK

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


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 2/6] thermal/drivers/cpu_cooling: Unregister with the policy
  2019-06-24  7:45         ` Daniel Lezcano
@ 2019-06-24  9:39           ` Viresh Kumar
  0 siblings, 0 replies; 23+ messages in thread
From: Viresh Kumar @ 2019-06-24  9:39 UTC (permalink / raw)
  To: Daniel Lezcano
  Cc: edubezval, linux-kernel, Sudeep Holla, Rafael J. Wysocki,
	Amit Daniel Kachhap, Javi Merino, Zhang Rui, Shawn Guo,
	Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
	NXP Linux Team, Keerthy,
	open list:CPU FREQUENCY DRIVERS - ARM BIG LITTLE,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
	open list:TI BANDGAP AND THERMAL DRIVER

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

^ permalink raw reply	[flat|nested] 23+ messages in thread

end of thread, back to index

Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-21 13:22 [PATCH 1/6] cpufreq: Use existing stub functions instead of IS_ENABLED macro Daniel Lezcano
2019-06-21 13:22 ` [PATCH 2/6] thermal/drivers/cpu_cooling: Unregister with the policy Daniel Lezcano
2019-06-24  6:03   ` Viresh Kumar
2019-06-24  7:30     ` Daniel Lezcano
2019-06-24  7:37       ` Viresh Kumar
2019-06-24  7:45         ` Daniel Lezcano
2019-06-24  9:39           ` Viresh Kumar
2019-06-21 13:22 ` [PATCH 3/6] cpufreq/drivers/arm_big_little: Remove cooling device usage Daniel Lezcano
2019-06-24  6:04   ` Viresh Kumar
2019-06-21 13:23 ` [PATCH 4/6] cpufreq: " Daniel Lezcano
2019-06-24  6:05   ` Viresh Kumar
2019-06-21 13:23 ` [PATCH 5/6] thermal/drivers/imx: " Daniel Lezcano
2019-06-24  6:06   ` Viresh Kumar
2019-06-21 13:23 ` [PATCH 6/6] thermal/drivers/ti: " Daniel Lezcano
2019-06-24  6:06   ` Viresh Kumar
2019-06-22  9:12 ` [PATCH 1/6] cpufreq: Use existing stub functions instead of IS_ENABLED macro Rafael J. Wysocki
2019-06-24  9:22   ` Daniel Lezcano
2019-06-24  9:30     ` Rafael J. Wysocki
2019-06-24  9:37       ` Daniel Lezcano
2019-06-24  8:53 ` Daniel Lezcano
2019-06-24  9:00   ` Rafael J. Wysocki
2019-06-24  9:07     ` Daniel Lezcano
2019-06-24  9:08   ` Viresh Kumar

Linux-PM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-pm/0 linux-pm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-pm linux-pm/ https://lore.kernel.org/linux-pm \
		linux-pm@vger.kernel.org linux-pm@archiver.kernel.org
	public-inbox-index linux-pm

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-pm


AGPL code for this site: git clone https://public-inbox.org/ public-inbox