[PATCH 00/26] thermal: cpu_cooling: Fixes and cleanups

[PATCH 00/26] thermal: cpu_cooling: Fixes and cleanups

[Viresh Kumar]

Hi Eduardo,

As you know I got into fixing cpu_cooling.c due to some cpufreq issues you and
Lukasz were struggling with. I found some issues in cpu_cooling then and here
are the fixes/cleanups.

Sorry for the long list. Haven't broken them into smaller sets as most of the
patches are very small, easy to review and inter-dependent. Only few of them
should take more time to review. If this doesn't work out, let me know and I
will try to send separate inter-dependent sets.

Just apply whatever looks fine and I will update/resend the ones left in V2 if
at required.

First few are updates to platform drivers. Exynos fails to register after few
patches in this series as it doesn't handle -EPROBE_DEFER properly (reported
that in reply to your patch as well). Others weren't setting clip_cpus properly
and are fixed.

Next ones are cleanups of cpu_cooling.c to get things properly organized.

Let me know if I screwed it up completely.

Tested-on: Exynos5250 (Dual ARM Cortex A15).
Rebased-over: v3.18-rc6
Pushed here: git://git.kernel.org/pub/scm/linux/kernel/git/vireshk/linux.git
thermal/cpu-cooling-fixes

Cc: Amit Daniel Kachhap <amit.daniel@samsung.com>
Cc: Chanwoo Choi <cw00.choi@samsung.com>
Cc: Hongbo Zhang <hongbo.zhang@linaro.com>
Cc: Kyungmin Park <kyungmin.park@samsung.com>
Cc: Lukasz Majewski <l.majewski@samsung.com>
Cc: Shawn Guo <shawn.guo@linaro.org>

Viresh Kumar (26):
  thermal: db8500: pass cpu_present_mask to cpufreq_cooling_register()
  thermal: imx: pass cpu_present_mask to cpufreq_cooling_register()
  thermal: exynos: pass cpu_present_mask to cpufreq_cooling_register()
  thermal: exynos: Handle -EPROBE_DEFER properly
  cpu_cooling: random comment fixups
  cpu_cooling: fix doc comment over struct cpufreq_cooling_device
  cpu_cooling: Add comment to clarify relation between cooling state and
    frequency
  cpu_cooling: Pass variable instead of its type to sizeof()
  cpu_cooling: no need to set cpufreq_state to zero
  cpu_cooling: no need to set cpufreq_dev to NULL
  cpu_cooling: propagate error returned by idr_alloc()
  cpu_cooling: Don't match min/max frequencies for all CPUs on cooling
    register
  cpu_cooling: don't iterate over all allowed_cpus to update cpufreq
    policy
  cpu_cooling: Don't check is_cpufreq_valid()
  cpu_cooling: do error handling at the bottom in
    __cpufreq_cooling_register()
  cpu_cooling: Drop useless locking around idr_alloc/idr_remove
  cpu_cooling: Merge cpufreq_apply_cooling() into
    cpufreq_set_cur_state()
  cpu_cooling: Merge get_cpu_frequency() into cpufreq_set_cur_state()
  cpu_cooling: find max level during device registration
  cpu_cooling: get_property() doesn't need to support GET_MAXL anymore
  cpu_cooling: create list of cpufreq_cooling_devices
  cpu_cooling: use cpufreq_dev_list instead of cpufreq_dev_count
  cpu_cooling: Pass 'cpufreq_dev' to get_property()
  cpu_cooling: Store frequencies in descending order
  cpu_cooling: Use cpufreq_dev->freq_table for finding level/freq
  cpu_cooling: update copyright tags

 drivers/thermal/cpu_cooling.c                   | 405 +++++++++---------------
 drivers/thermal/db8500_cpufreq_cooling.c        |   5 +-
 drivers/thermal/imx_thermal.c                   |   4 +-
 drivers/thermal/samsung/exynos_thermal_common.c |  11 +-
 drivers/thermal/samsung/exynos_tmu.c            |   4 +-
 5 files changed, 153 insertions(+), 276 deletions(-)

-- 
2.0.3.693.g996b0fd

[PATCH 01/26] thermal: db8500: pass cpu_present_mask to cpufreq_cooling_register()

[Viresh Kumar]

cpufreq_cooling_register() expects mask of all the CPUs where frequency
constraint is applicable.

This platform has more than one CPU to which these constraints will apply and so
passing mask of only CPU0 wouldn't be sufficient. Also, this platform has a
single cluster of CPUs and the constraint applies to all CPUs.

If CPU0 is hoplugged out then we may face strange BUGs as cpu_cooling framework
isn't aware of any siblings sharing clock line.

Fix it by passing cpu_present_mask to cpufreq_cooling_register().

Cc: Hongbo Zhang <hongbo.zhang@linaro.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/thermal/db8500_cpufreq_cooling.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/thermal/db8500_cpufreq_cooling.c b/drivers/thermal/db8500_cpufreq_cooling.c
index 786d192..374ef1e 100644
--- a/drivers/thermal/db8500_cpufreq_cooling.c
+++ b/drivers/thermal/db8500_cpufreq_cooling.c
@@ -28,15 +28,12 @@
 static int db8500_cpufreq_cooling_probe(struct platform_device *pdev)
 {
 	struct thermal_cooling_device *cdev;
-	struct cpumask mask_val;
 
 	/* make sure cpufreq driver has been initialized */
 	if (!cpufreq_frequency_get_table(0))
 		return -EPROBE_DEFER;
 
-	cpumask_set_cpu(0, &mask_val);
-	cdev = cpufreq_cooling_register(&mask_val);
-
+	cdev = cpufreq_cooling_register(cpu_present_mask);
 	if (IS_ERR(cdev)) {
 		dev_err(&pdev->dev, "Failed to register cooling device\n");
 		return PTR_ERR(cdev);
-- 
2.0.3.693.g996b0fd

[PATCH 02/26] thermal: imx: pass cpu_present_mask to cpufreq_cooling_register()

[Viresh Kumar]

cpufreq_cooling_register() expects mask of all the CPUs where frequency
constraint is applicable.

This platform has more than one CPU to which these constraints will apply and so
passing mask of only CPU0 wouldn't be sufficient. Also, this platform has a
single cluster of CPUs and the constraint applies to all CPUs.

If CPU0 is hoplugged out then we may face strange BUGs as cpu_cooling framework
isn't aware of any siblings sharing clock line.

Fix it by passing cpu_present_mask to cpufreq_cooling_register().

Cc: Shawn Guo <shawn.guo@linaro.org>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/thermal/imx_thermal.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c
index 5a1f107..1eefc6d 100644
--- a/drivers/thermal/imx_thermal.c
+++ b/drivers/thermal/imx_thermal.c
@@ -454,7 +454,6 @@ static int imx_thermal_probe(struct platform_device *pdev)
 	const struct of_device_id *of_id =
 		of_match_device(of_imx_thermal_match, &pdev->dev);
 	struct imx_thermal_data *data;
-	struct cpumask clip_cpus;
 	struct regmap *map;
 	int measure_freq;
 	int ret;
@@ -516,8 +515,7 @@ static int imx_thermal_probe(struct platform_device *pdev)
 	regmap_write(map, MISC0 + REG_SET, MISC0_REFTOP_SELBIASOFF);
 	regmap_write(map, TEMPSENSE0 + REG_SET, TEMPSENSE0_POWER_DOWN);
 
-	cpumask_set_cpu(0, &clip_cpus);
-	data->cdev = cpufreq_cooling_register(&clip_cpus);
+	data->cdev = cpufreq_cooling_register(cpu_present_mask);
 	if (IS_ERR(data->cdev)) {
 		ret = PTR_ERR(data->cdev);
 		dev_err(&pdev->dev,
-- 
2.0.3.693.g996b0fd

[PATCH 03/26] thermal: exynos: pass cpu_present_mask to cpufreq_cooling_register()

[Viresh Kumar]

cpufreq_cooling_register() expects mask of all the CPUs where frequency
constraint is applicable.

This platform has more than one CPU to which these constraints will apply and so
passing mask of only CPU0 wouldn't be sufficient. Also, this platform has a
single cluster of CPUs and the constraint applies to all CPUs.

If CPU0 is hoplugged out then we may face strange BUGs as cpu_cooling framework
isn't aware of any siblings sharing clock line.

Fix it by passing cpu_present_mask to cpufreq_cooling_register().

Cc: Chanwoo Choi <cw00.choi@samsung.com>
Cc: Kyungmin Park <kyungmin.park@samsung.com>
Cc: Amit Daniel Kachhap <amit.daniel@samsung.com>
Cc: Lukasz Majewski <l.majewski@samsung.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/thermal/samsung/exynos_thermal_common.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/thermal/samsung/exynos_thermal_common.c b/drivers/thermal/samsung/exynos_thermal_common.c
index 3f5ad25..bf39212 100644
--- a/drivers/thermal/samsung/exynos_thermal_common.c
+++ b/drivers/thermal/samsung/exynos_thermal_common.c
@@ -347,7 +347,6 @@ void exynos_report_trigger(struct thermal_sensor_conf *conf)
 int exynos_register_thermal(struct thermal_sensor_conf *sensor_conf)
 {
 	int ret;
-	struct cpumask mask_val;
 	struct exynos_thermal_zone *th_zone;
 
 	if (!sensor_conf || !sensor_conf->read_temperature) {
@@ -367,9 +366,8 @@ int exynos_register_thermal(struct thermal_sensor_conf *sensor_conf)
 	 *	 sensor
 	 */
 	if (sensor_conf->cooling_data.freq_clip_count > 0) {
-		cpumask_set_cpu(0, &mask_val);
 		th_zone->cool_dev[th_zone->cool_dev_size] =
-					cpufreq_cooling_register(&mask_val);
+				cpufreq_cooling_register(cpu_present_mask);
 		if (IS_ERR(th_zone->cool_dev[th_zone->cool_dev_size])) {
 			dev_err(sensor_conf->dev,
 				"Failed to register cpufreq cooling device\n");
-- 
2.0.3.693.g996b0fd

[PATCH 04/26] thermal: exynos: Handle -EPROBE_DEFER properly

[Viresh Kumar]

cpufreq_cooling_register() can return -EPROBE_DEFER if cpufreq driver isn't
ready yet and so the callers must defer their initialization.

Exynos thermal drivers weren't handling this well and were raising false error
message when -EPROBE_DEFER is returned to them.

Fix them to handle -EPROBE_DEFER.

Cc: Chanwoo Choi <cw00.choi@samsung.com>
Cc: Kyungmin Park <kyungmin.park@samsung.com>
Cc: Amit Daniel Kachhap <amit.daniel@samsung.com>
Cc: Lukasz Majewski <l.majewski@samsung.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/thermal/samsung/exynos_thermal_common.c | 7 ++++---
 drivers/thermal/samsung/exynos_tmu.c            | 4 +++-
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/thermal/samsung/exynos_thermal_common.c b/drivers/thermal/samsung/exynos_thermal_common.c
index bf39212..0be1d54 100644
--- a/drivers/thermal/samsung/exynos_thermal_common.c
+++ b/drivers/thermal/samsung/exynos_thermal_common.c
@@ -369,9 +369,10 @@ int exynos_register_thermal(struct thermal_sensor_conf *sensor_conf)
 		th_zone->cool_dev[th_zone->cool_dev_size] =
 				cpufreq_cooling_register(cpu_present_mask);
 		if (IS_ERR(th_zone->cool_dev[th_zone->cool_dev_size])) {
-			dev_err(sensor_conf->dev,
-				"Failed to register cpufreq cooling device\n");
-			ret = -EINVAL;
+			ret = PTR_ERR(th_zone->cool_dev[th_zone->cool_dev_size]);
+			if (ret != -EPROBE_DEFER)
+				dev_err(sensor_conf->dev,
+					"Failed to register cpufreq cooling device\n");
 			goto err_unregister;
 		}
 		th_zone->cool_dev_size++;
diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
index 49c0924..cc3677f 100644
--- a/drivers/thermal/samsung/exynos_tmu.c
+++ b/drivers/thermal/samsung/exynos_tmu.c
@@ -683,7 +683,9 @@ static int exynos_tmu_probe(struct platform_device *pdev)
 	/* Register the sensor with thermal management interface */
 	ret = exynos_register_thermal(sensor_conf);
 	if (ret) {
-		dev_err(&pdev->dev, "Failed to register thermal interface\n");
+		if (ret != -EPROBE_DEFER)
+			dev_err(&pdev->dev,
+				"Failed to register thermal interface\n");
 		goto err_clk;
 	}
 	data->reg_conf = sensor_conf;
-- 
2.0.3.693.g996b0fd

[PATCH 05/26] cpu_cooling: random comment fixups

[Viresh Kumar]

s/give/given

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/thermal/cpu_cooling.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
index 1ab0018..7c2ba2e 100644
--- a/drivers/thermal/cpu_cooling.c
+++ b/drivers/thermal/cpu_cooling.c
@@ -122,7 +122,7 @@ enum cpufreq_cooling_property {
 };
 
 /**
- * get_property - fetch a property of interest for a give cpu.
+ * get_property - fetch a property of interest for a given cpu.
  * @cpu: cpu for which the property is required
  * @input: query parameter
  * @output: query return
@@ -132,6 +132,7 @@ enum cpufreq_cooling_property {
  * 1. get maximum cpu cooling states
  * 2. translate frequency to cooling state
  * 3. translate cooling state to frequency
+ *
  * Note that the code may be not in good shape
  * but it is written in this way in order to:
  * a) reduce duplicate code as most of the code can be shared.
@@ -212,7 +213,7 @@ static int get_property(unsigned int cpu, unsigned long input,
 }
 
 /**
- * cpufreq_cooling_get_level - for a give cpu, return the cooling level.
+ * cpufreq_cooling_get_level - for a given cpu, return the cooling level.
  * @cpu: cpu for which the level is required
  * @freq: the frequency of interest
  *
-- 
2.0.3.693.g996b0fd

[PATCH 06/26] cpu_cooling: fix doc comment over struct cpufreq_cooling_device

[Viresh Kumar]

cooling_cpufreq_lock isn't used to protect this structure and so the comment
over it is outdated. Fix it.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/thermal/cpu_cooling.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
index 7c2ba2e..3ef9050 100644
--- a/drivers/thermal/cpu_cooling.c
+++ b/drivers/thermal/cpu_cooling.c
@@ -40,9 +40,8 @@
  *	frequency.
  * @allowed_cpus: all the cpus involved for this cpufreq_cooling_device.
  *
- * This structure is required for keeping information of each
- * cpufreq_cooling_device registered. In order to prevent corruption of this a
- * mutex lock cooling_cpufreq_lock is used.
+ * This structure is required for keeping information of each registered
+ * cpufreq_cooling_device.
  */
 struct cpufreq_cooling_device {
 	int id;
-- 
2.0.3.693.g996b0fd

[PATCH 07/26] cpu_cooling: Add comment to clarify relation between cooling state and frequency

[Viresh Kumar]

This wasn't explained well anywhere and should be clearly specified. Lets add a
top level comment for this.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/thermal/cpu_cooling.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
index 3ef9050..def0f21 100644
--- a/drivers/thermal/cpu_cooling.c
+++ b/drivers/thermal/cpu_cooling.c
@@ -28,6 +28,20 @@
 #include <linux/cpu.h>
 #include <linux/cpu_cooling.h>
 
+/*
+ * Cooling state <-> CPUFreq frequency
+ *
+ * Cooling states are translated to frequencies throughout this driver and this
+ * is the relation between them.
+ *
+ * Highest cooling state corresponds to lowest possible frequency.
+ *
+ * i.e.
+ *	level 0 --> 1st Max Freq
+ *	level 1 --> 2nd Max Freq
+ *	...
+ */
+
 /**
  * struct cpufreq_cooling_device - data for cooling device with cpufreq
  * @id: unique integer value corresponding to each cpufreq_cooling_device
-- 
2.0.3.693.g996b0fd

[PATCH 08/26] cpu_cooling: Pass variable instead of its type to sizeof()

[Viresh Kumar]

Just following coding guidelines here.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/thermal/cpu_cooling.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
index def0f21..59725d8 100644
--- a/drivers/thermal/cpu_cooling.c
+++ b/drivers/thermal/cpu_cooling.c
@@ -468,8 +468,7 @@ __cpufreq_cooling_register(struct device_node *np,
 				return ERR_PTR(-EINVAL);
 		}
 	}
-	cpufreq_dev = kzalloc(sizeof(struct cpufreq_cooling_device),
-			      GFP_KERNEL);
+	cpufreq_dev = kzalloc(sizeof(*cool_dev), GFP_KERNEL);
 	if (!cpufreq_dev)
 		return ERR_PTR(-ENOMEM);
 
-- 
2.0.3.693.g996b0fd

[PATCH 09/26] cpu_cooling: no need to set cpufreq_state to zero

[Viresh Kumar]

Its already zero, we allocated cpufreq_dev with kzalloc.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/thermal/cpu_cooling.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
index 59725d8..cf9d1de 100644
--- a/drivers/thermal/cpu_cooling.c
+++ b/drivers/thermal/cpu_cooling.c
@@ -491,7 +491,7 @@ __cpufreq_cooling_register(struct device_node *np,
 		return cool_dev;
 	}
 	cpufreq_dev->cool_dev = cool_dev;
-	cpufreq_dev->cpufreq_state = 0;
+
 	mutex_lock(&cooling_cpufreq_lock);
 
 	/* Register the notifier for first cpufreq cooling device */
-- 
2.0.3.693.g996b0fd

[PATCH 10/26] cpu_cooling: no need to set cpufreq_dev to NULL

[Viresh Kumar]

It will be overwritten soon with return value of kzalloc().

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/thermal/cpu_cooling.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
index cf9d1de..bb11dd4 100644
--- a/drivers/thermal/cpu_cooling.c
+++ b/drivers/thermal/cpu_cooling.c
@@ -448,7 +448,7 @@ __cpufreq_cooling_register(struct device_node *np,
 			   const struct cpumask *clip_cpus)
 {
 	struct thermal_cooling_device *cool_dev;
-	struct cpufreq_cooling_device *cpufreq_dev = NULL;
+	struct cpufreq_cooling_device *cpufreq_dev;
 	unsigned int min = 0, max = 0;
 	char dev_name[THERMAL_NAME_LENGTH];
 	int ret = 0, i;
-- 
2.0.3.693.g996b0fd

[PATCH 11/26] cpu_cooling: propagate error returned by idr_alloc()

[Viresh Kumar]

We aren't supposed to return our own error type here. Return what we got.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/thermal/cpu_cooling.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
index bb11dd4..964586f 100644
--- a/drivers/thermal/cpu_cooling.c
+++ b/drivers/thermal/cpu_cooling.c
@@ -477,7 +477,7 @@ __cpufreq_cooling_register(struct device_node *np,
 	ret = get_idr(&cpufreq_idr, &cpufreq_dev->id);
 	if (ret) {
 		kfree(cpufreq_dev);
-		return ERR_PTR(-EINVAL);
+		return ERR_PTR(cpufreq_dev->id);
 	}
 
 	snprintf(dev_name, sizeof(dev_name), "thermal-cpufreq-%d",
-- 
2.0.3.693.g996b0fd

[PATCH 12/26] cpu_cooling: Don't match min/max frequencies for all CPUs on cooling register

[Viresh Kumar]

In __cpufreq_cooling_register() we try to match min/max frequencies for all CPUs
passed in 'clip_cpus' mask. This mask is the cpumask of cpus where the frequency
constraints will be applied.

Same frequency constraint can be applied only to the CPUs belonging to the same
cluster (i.e. CPUs sharing clock line). For all such CPUs we have a single
'struct cpufreq_policy' structure managing them and so getting policies for all
CPUs wouldn't make any sense as they will all return the same pointer.

So, remove this useless check of checking min/max for all CPUs. Also update doc
comment to make this more obvious that clip_cpus should be same as
policy->related_cpus.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/thermal/cpu_cooling.c | 19 ++-----------------
 1 file changed, 2 insertions(+), 17 deletions(-)

diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
index 964586f..c358ede 100644
--- a/drivers/thermal/cpu_cooling.c
+++ b/drivers/thermal/cpu_cooling.c
@@ -434,6 +434,7 @@ static struct notifier_block thermal_cpufreq_notifier_block = {
  * __cpufreq_cooling_register - helper function to create cpufreq cooling device
  * @np: a valid struct device_node to the cooling device device tree node
  * @clip_cpus: cpumask of cpus where the frequency constraints will happen.
+ * Normally this should be same as cpufreq policy->related_cpus.
  *
  * This interface function registers the cpufreq cooling device with the name
  * "thermal-cpufreq-%x". This api can support multiple instances of cpufreq
@@ -449,25 +450,9 @@ __cpufreq_cooling_register(struct device_node *np,
 {
 	struct thermal_cooling_device *cool_dev;
 	struct cpufreq_cooling_device *cpufreq_dev;
-	unsigned int min = 0, max = 0;
 	char dev_name[THERMAL_NAME_LENGTH];
-	int ret = 0, i;
-	struct cpufreq_policy policy;
+	int ret = 0;
 
-	/* Verify that all the clip cpus have same freq_min, freq_max limit */
-	for_each_cpu(i, clip_cpus) {
-		/* continue if cpufreq policy not found and not return error */
-		if (!cpufreq_get_policy(&policy, i))
-			continue;
-		if (min == 0 && max == 0) {
-			min = policy.cpuinfo.min_freq;
-			max = policy.cpuinfo.max_freq;
-		} else {
-			if (min != policy.cpuinfo.min_freq ||
-			    max != policy.cpuinfo.max_freq)
-				return ERR_PTR(-EINVAL);
-		}
-	}
 	cpufreq_dev = kzalloc(sizeof(*cool_dev), GFP_KERNEL);
 	if (!cpufreq_dev)
 		return ERR_PTR(-ENOMEM);
-- 
2.0.3.693.g996b0fd

[PATCH 13/26] cpu_cooling: don't iterate over all allowed_cpus to update cpufreq policy

[Viresh Kumar]

All CPUs present in 'allowed_cpus' share the same 'struct cpufreq_policy'
structure and so calling cpufreq_update_policy() for each of them doesn't make
sense.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/thermal/cpu_cooling.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
index c358ede..8ca0380 100644
--- a/drivers/thermal/cpu_cooling.c
+++ b/drivers/thermal/cpu_cooling.c
@@ -286,11 +286,10 @@ static unsigned int get_cpu_frequency(unsigned int cpu, unsigned long level)
 static int cpufreq_apply_cooling(struct cpufreq_cooling_device *cpufreq_device,
 				 unsigned long cooling_state)
 {
-	unsigned int cpuid, clip_freq;
+	unsigned int clip_freq;
 	struct cpumask *mask = &cpufreq_device->allowed_cpus;
 	unsigned int cpu = cpumask_any(mask);
 
-
 	/* Check if the old cooling action is same as new cooling action */
 	if (cpufreq_device->cpufreq_state == cooling_state)
 		return 0;
@@ -303,10 +302,8 @@ static int cpufreq_apply_cooling(struct cpufreq_cooling_device *cpufreq_device,
 	cpufreq_device->cpufreq_val = clip_freq;
 	notify_device = cpufreq_device;
 
-	for_each_cpu(cpuid, mask) {
-		if (is_cpufreq_valid(cpuid))
-			cpufreq_update_policy(cpuid);
-	}
+	if (is_cpufreq_valid(cpu))
+		cpufreq_update_policy(cpu);
 
 	notify_device = NOTIFY_INVALID;
 
-- 
2.0.3.693.g996b0fd

[PATCH 14/26] cpu_cooling: Don't check is_cpufreq_valid()

[Viresh Kumar]

Because get_cpu_frequency() has returned a valid frequency, it means that the
cpufreq policy is surely valid and so no point checking that again with
is_cpufreq_valid(). Get rid of the routine as well as there are no more users.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/thermal/cpu_cooling.c | 20 +-------------------
 1 file changed, 1 insertion(+), 19 deletions(-)

diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
index 8ca0380..41aa7d5 100644
--- a/drivers/thermal/cpu_cooling.c
+++ b/drivers/thermal/cpu_cooling.c
@@ -111,23 +111,6 @@ static void release_idr(struct idr *idr, int id)
 
 /* Below code defines functions to be used for cpufreq as cooling device */
 
-/**
- * is_cpufreq_valid - function to check frequency transitioning capability.
- * @cpu: cpu for which check is needed.
- *
- * This function will check the current state of the system if
- * it is capable of changing the frequency for a given @cpu.
- *
- * Return: 0 if the system is not currently capable of changing
- * the frequency of given cpu. !0 in case the frequency is changeable.
- */
-static int is_cpufreq_valid(int cpu)
-{
-	struct cpufreq_policy policy;
-
-	return !cpufreq_get_policy(&policy, cpu);
-}
-
 enum cpufreq_cooling_property {
 	GET_LEVEL,
 	GET_FREQ,
@@ -302,8 +285,7 @@ static int cpufreq_apply_cooling(struct cpufreq_cooling_device *cpufreq_device,
 	cpufreq_device->cpufreq_val = clip_freq;
 	notify_device = cpufreq_device;
 
-	if (is_cpufreq_valid(cpu))
-		cpufreq_update_policy(cpu);
+	cpufreq_update_policy(cpu);
 
 	notify_device = NOTIFY_INVALID;
 
-- 
2.0.3.693.g996b0fd

[PATCH 15/26] cpu_cooling: do error handling at the bottom in __cpufreq_cooling_register()

[Viresh Kumar]

This makes life easy and bug free. And is scalable for future resource
allocations.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/thermal/cpu_cooling.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
index 41aa7d5..032cba3 100644
--- a/drivers/thermal/cpu_cooling.c
+++ b/drivers/thermal/cpu_cooling.c
@@ -440,8 +440,8 @@ __cpufreq_cooling_register(struct device_node *np,
 
 	ret = get_idr(&cpufreq_idr, &cpufreq_dev->id);
 	if (ret) {
-		kfree(cpufreq_dev);
-		return ERR_PTR(cpufreq_dev->id);
+		cool_dev = ERR_PTR(cpufreq_dev->id);
+		goto free_cdev;
 	}
 
 	snprintf(dev_name, sizeof(dev_name), "thermal-cpufreq-%d",
@@ -449,11 +449,9 @@ __cpufreq_cooling_register(struct device_node *np,
 
 	cool_dev = thermal_of_cooling_device_register(np, dev_name, cpufreq_dev,
 						      &cpufreq_cooling_ops);
-	if (IS_ERR(cool_dev)) {
-		release_idr(&cpufreq_idr, cpufreq_dev->id);
-		kfree(cpufreq_dev);
-		return cool_dev;
-	}
+	if (IS_ERR(cool_dev))
+		goto remove_idr;
+
 	cpufreq_dev->cool_dev = cool_dev;
 
 	mutex_lock(&cooling_cpufreq_lock);
@@ -467,6 +465,13 @@ __cpufreq_cooling_register(struct device_node *np,
 	mutex_unlock(&cooling_cpufreq_lock);
 
 	return cool_dev;
+
+remove_idr:
+	release_idr(&cpufreq_idr, cpufreq_dev->id);
+free_cdev:
+	kfree(cpufreq_dev);
+
+	return cool_dev;
 }
 
 /**
-- 
2.0.3.693.g996b0fd

[PATCH 16/26] cpu_cooling: Drop useless locking around idr_alloc/idr_remove

[Viresh Kumar]

Locking around idr_alloc/idr_remove looks rather pointless as there is no race
it is trying to fix. Remove it.

get_idr() and release_idr() aren't much useful now, so get rid of them as well.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
@Eduardo: Same is true for thermal-core as well ?
---
 drivers/thermal/cpu_cooling.c | 45 ++++---------------------------------------
 1 file changed, 4 insertions(+), 41 deletions(-)

diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
index 032cba3..ca8f1bb 100644
--- a/drivers/thermal/cpu_cooling.c
+++ b/drivers/thermal/cpu_cooling.c
@@ -73,42 +73,6 @@ static unsigned int cpufreq_dev_count;
 #define NOTIFY_INVALID NULL
 static struct cpufreq_cooling_device *notify_device;
 
-/**
- * get_idr - function to get a unique id.
- * @idr: struct idr * handle used to create a id.
- * @id: int * value generated by this function.
- *
- * This function will populate @id with an unique
- * id, using the idr API.
- *
- * Return: 0 on success, an error code on failure.
- */
-static int get_idr(struct idr *idr, int *id)
-{
-	int ret;
-
-	mutex_lock(&cooling_cpufreq_lock);
-	ret = idr_alloc(idr, NULL, 0, 0, GFP_KERNEL);
-	mutex_unlock(&cooling_cpufreq_lock);
-	if (unlikely(ret < 0))
-		return ret;
-	*id = ret;
-
-	return 0;
-}
-
-/**
- * release_idr - function to free the unique id.
- * @idr: struct idr * handle used for creating the id.
- * @id: int value representing the unique id.
- */
-static void release_idr(struct idr *idr, int id)
-{
-	mutex_lock(&cooling_cpufreq_lock);
-	idr_remove(idr, id);
-	mutex_unlock(&cooling_cpufreq_lock);
-}
-
 /* Below code defines functions to be used for cpufreq as cooling device */
 
 enum cpufreq_cooling_property {
@@ -430,7 +394,6 @@ __cpufreq_cooling_register(struct device_node *np,
 	struct thermal_cooling_device *cool_dev;
 	struct cpufreq_cooling_device *cpufreq_dev;
 	char dev_name[THERMAL_NAME_LENGTH];
-	int ret = 0;
 
 	cpufreq_dev = kzalloc(sizeof(*cool_dev), GFP_KERNEL);
 	if (!cpufreq_dev)
@@ -438,8 +401,8 @@ __cpufreq_cooling_register(struct device_node *np,
 
 	cpumask_copy(&cpufreq_dev->allowed_cpus, clip_cpus);
 
-	ret = get_idr(&cpufreq_idr, &cpufreq_dev->id);
-	if (ret) {
+	cpufreq_dev->id = idr_alloc(&cpufreq_idr, NULL, 0, 0, GFP_KERNEL);
+	if (unlikely(cpufreq_dev->id < 0)) {
 		cool_dev = ERR_PTR(cpufreq_dev->id);
 		goto free_cdev;
 	}
@@ -467,7 +430,7 @@ __cpufreq_cooling_register(struct device_node *np,
 	return cool_dev;
 
 remove_idr:
-	release_idr(&cpufreq_idr, cpufreq_dev->id);
+	idr_remove(&cpufreq_idr, cpufreq_dev->id);
 free_cdev:
 	kfree(cpufreq_dev);
 
@@ -540,7 +503,7 @@ void cpufreq_cooling_unregister(struct thermal_cooling_device *cdev)
 	mutex_unlock(&cooling_cpufreq_lock);
 
 	thermal_cooling_device_unregister(cpufreq_dev->cool_dev);
-	release_idr(&cpufreq_idr, cpufreq_dev->id);
+	idr_remove(&cpufreq_idr, cpufreq_dev->id);
 	kfree(cpufreq_dev);
 }
 EXPORT_SYMBOL_GPL(cpufreq_cooling_unregister);
-- 
2.0.3.693.g996b0fd

[PATCH 17/26] cpu_cooling: Merge cpufreq_apply_cooling() into cpufreq_set_cur_state()

[Viresh Kumar]

cpufreq_apply_cooling() has a single caller, cpufreq_set_cur_state() and
cpufreq_set_cur_state() is an unnecessary wrapper over cpufreq_apply_cooling().

Get rid of it by merging both routines.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/thermal/cpu_cooling.c | 58 ++++++++++++++-----------------------------
 1 file changed, 19 insertions(+), 39 deletions(-)

diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
index ca8f1bb..9673b48 100644
--- a/drivers/thermal/cpu_cooling.c
+++ b/drivers/thermal/cpu_cooling.c
@@ -219,44 +219,6 @@ static unsigned int get_cpu_frequency(unsigned int cpu, unsigned long level)
 }
 
 /**
- * cpufreq_apply_cooling - function to apply frequency clipping.
- * @cpufreq_device: cpufreq_cooling_device pointer containing frequency
- *	clipping data.
- * @cooling_state: value of the cooling state.
- *
- * Function used to make sure the cpufreq layer is aware of current thermal
- * limits. The limits are applied by updating the cpufreq policy.
- *
- * Return: 0 on success, an error code otherwise (-EINVAL in case wrong
- * cooling state).
- */
-static int cpufreq_apply_cooling(struct cpufreq_cooling_device *cpufreq_device,
-				 unsigned long cooling_state)
-{
-	unsigned int clip_freq;
-	struct cpumask *mask = &cpufreq_device->allowed_cpus;
-	unsigned int cpu = cpumask_any(mask);
-
-	/* Check if the old cooling action is same as new cooling action */
-	if (cpufreq_device->cpufreq_state == cooling_state)
-		return 0;
-
-	clip_freq = get_cpu_frequency(cpu, cooling_state);
-	if (!clip_freq)
-		return -EINVAL;
-
-	cpufreq_device->cpufreq_state = cooling_state;
-	cpufreq_device->cpufreq_val = clip_freq;
-	notify_device = cpufreq_device;
-
-	cpufreq_update_policy(cpu);
-
-	notify_device = NOTIFY_INVALID;
-
-	return 0;
-}
-
-/**
  * cpufreq_thermal_notifier - notifier callback for cpufreq policy change.
  * @nb:	struct notifier_block * with callback info.
  * @event: value showing cpufreq event for which this function invoked.
@@ -357,8 +319,26 @@ static int cpufreq_set_cur_state(struct thermal_cooling_device *cdev,
 				 unsigned long state)
 {
 	struct cpufreq_cooling_device *cpufreq_device = cdev->devdata;
+	unsigned int cpu = cpumask_any(&cpufreq_device->allowed_cpus);
+	unsigned int clip_freq;
 
-	return cpufreq_apply_cooling(cpufreq_device, state);
+	/* Check if the old cooling action is same as new cooling action */
+	if (cpufreq_device->cpufreq_state == state)
+		return 0;
+
+	clip_freq = get_cpu_frequency(cpu, state);
+	if (!clip_freq)
+		return -EINVAL;
+
+	cpufreq_device->cpufreq_state = state;
+	cpufreq_device->cpufreq_val = clip_freq;
+	notify_device = cpufreq_device;
+
+	cpufreq_update_policy(cpu);
+
+	notify_device = NOTIFY_INVALID;
+
+	return 0;
 }
 
 /* Bind cpufreq callbacks to thermal cooling device ops */
-- 
2.0.3.693.g996b0fd

[PATCH 18/26] cpu_cooling: Merge get_cpu_frequency() into cpufreq_set_cur_state()

[Viresh Kumar]

get_cpu_frequency() is used from only one place, i.e. cpufreq_set_cur_state().
And there is no need to have this extra level of function calls. Merge them.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/thermal/cpu_cooling.c | 31 ++++---------------------------
 1 file changed, 4 insertions(+), 27 deletions(-)

diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
index 9673b48..5815abf 100644
--- a/drivers/thermal/cpu_cooling.c
+++ b/drivers/thermal/cpu_cooling.c
@@ -195,30 +195,6 @@ unsigned long cpufreq_cooling_get_level(unsigned int cpu, unsigned int freq)
 EXPORT_SYMBOL_GPL(cpufreq_cooling_get_level);
 
 /**
- * get_cpu_frequency - get the absolute value of frequency from level.
- * @cpu: cpu for which frequency is fetched.
- * @level: cooling level
- *
- * This function matches cooling level with frequency. Based on a cooling level
- * of frequency, equals cooling state of cpu cooling device, it will return
- * the corresponding frequency.
- *	e.g level=0 --> 1st MAX FREQ, level=1 ---> 2nd MAX FREQ, .... etc
- *
- * Return: 0 on error, the corresponding frequency otherwise.
- */
-static unsigned int get_cpu_frequency(unsigned int cpu, unsigned long level)
-{
-	int ret = 0;
-	unsigned int freq;
-
-	ret = get_property(cpu, level, &freq, GET_FREQ);
-	if (ret)
-		return 0;
-
-	return freq;
-}
-
-/**
  * cpufreq_thermal_notifier - notifier callback for cpufreq policy change.
  * @nb:	struct notifier_block * with callback info.
  * @event: value showing cpufreq event for which this function invoked.
@@ -321,14 +297,15 @@ static int cpufreq_set_cur_state(struct thermal_cooling_device *cdev,
 	struct cpufreq_cooling_device *cpufreq_device = cdev->devdata;
 	unsigned int cpu = cpumask_any(&cpufreq_device->allowed_cpus);
 	unsigned int clip_freq;
+	int ret = 0;
 
 	/* Check if the old cooling action is same as new cooling action */
 	if (cpufreq_device->cpufreq_state == state)
 		return 0;
 
-	clip_freq = get_cpu_frequency(cpu, state);
-	if (!clip_freq)
-		return -EINVAL;
+	ret = get_property(cpu, state, &clip_freq, GET_FREQ);
+	if (ret)
+		return ret;
 
 	cpufreq_device->cpufreq_state = state;
 	cpufreq_device->cpufreq_val = clip_freq;
-- 
2.0.3.693.g996b0fd

[PATCH 19/26] cpu_cooling: find max level during device registration

[Viresh Kumar]

CPU frequency tables don't update after the driver is registered and so we don't
need to iterate over them to find total number of states every time
cpufreq_get_max_state() is called. Do it once at boot time.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/thermal/cpu_cooling.c | 31 +++++++++++++++++++------------
 1 file changed, 19 insertions(+), 12 deletions(-)

diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
index 5815abf..05712d5 100644
--- a/drivers/thermal/cpu_cooling.c
+++ b/drivers/thermal/cpu_cooling.c
@@ -52,6 +52,8 @@
  *	cooling	devices.
  * @cpufreq_val: integer value representing the absolute value of the clipped
  *	frequency.
+ * @max_level: maximum cooling level. One less than total number of valid
+ *	cpufreq frequencies.
  * @allowed_cpus: all the cpus involved for this cpufreq_cooling_device.
  *
  * This structure is required for keeping information of each registered
@@ -62,6 +64,7 @@ struct cpufreq_cooling_device {
 	struct thermal_cooling_device *cool_dev;
 	unsigned int cpufreq_state;
 	unsigned int cpufreq_val;
+	unsigned int max_level;
 	struct cpumask allowed_cpus;
 };
 static DEFINE_IDR(cpufreq_idr);
@@ -246,19 +249,9 @@ static int cpufreq_get_max_state(struct thermal_cooling_device *cdev,
 				 unsigned long *state)
 {
 	struct cpufreq_cooling_device *cpufreq_device = cdev->devdata;
-	struct cpumask *mask = &cpufreq_device->allowed_cpus;
-	unsigned int cpu;
-	unsigned int count = 0;
-	int ret;
 
-	cpu = cpumask_any(mask);
-
-	ret = get_property(cpu, 0, &count, GET_MAXL);
-
-	if (count > 0)
-		*state = count;
-
-	return ret;
+	*state = cpufreq_device->max_level;
+	return 0;
 }
 
 /**
@@ -351,11 +344,25 @@ __cpufreq_cooling_register(struct device_node *np,
 	struct thermal_cooling_device *cool_dev;
 	struct cpufreq_cooling_device *cpufreq_dev;
 	char dev_name[THERMAL_NAME_LENGTH];
+	struct cpufreq_frequency_table *pos, *table;
+
+	table = cpufreq_frequency_get_table(cpumask_first(clip_cpus));
+	if (!table) {
+		pr_debug("%s: CPUFreq table not found\n", __func__);
+		return ERR_PTR(-EPROBE_DEFER);
+	}
 
 	cpufreq_dev = kzalloc(sizeof(*cool_dev), GFP_KERNEL);
 	if (!cpufreq_dev)
 		return ERR_PTR(-ENOMEM);
 
+	/* Find max levels */
+	cpufreq_for_each_valid_entry(pos, table)
+		cpufreq_dev->max_level++;
+
+	/* max_level is an index, not a counter */
+	cpufreq_dev->max_level--;
+
 	cpumask_copy(&cpufreq_dev->allowed_cpus, clip_cpus);
 
 	cpufreq_dev->id = idr_alloc(&cpufreq_idr, NULL, 0, 0, GFP_KERNEL);
-- 
2.0.3.693.g996b0fd

[PATCH 20/26] cpu_cooling: get_property() doesn't need to support GET_MAXL anymore

[Viresh Kumar]

We don't use get_property() to find max levels anymore as it is done at boot
now. So, don't support GET_MAXL in get_property().

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/thermal/cpu_cooling.c | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
index 05712d5..ddb97aa 100644
--- a/drivers/thermal/cpu_cooling.c
+++ b/drivers/thermal/cpu_cooling.c
@@ -81,7 +81,6 @@ static struct cpufreq_cooling_device *notify_device;
 enum cpufreq_cooling_property {
 	GET_LEVEL,
 	GET_FREQ,
-	GET_MAXL,
 };
 
 /**
@@ -89,12 +88,11 @@ enum cpufreq_cooling_property {
  * @cpu: cpu for which the property is required
  * @input: query parameter
  * @output: query return
- * @property: type of query (frequency, level, max level)
+ * @property: type of query (frequency, level)
  *
  * This is the common function to
- * 1. get maximum cpu cooling states
- * 2. translate frequency to cooling state
- * 3. translate cooling state to frequency
+ * 1. translate frequency to cooling state
+ * 2. translate cooling state to frequency
  *
  * Note that the code may be not in good shape
  * but it is written in this way in order to:
@@ -141,12 +139,6 @@ static int get_property(unsigned int cpu, unsigned long input,
 	/* max_level is an index, not a counter */
 	max_level--;
 
-	/* get max level */
-	if (property == GET_MAXL) {
-		*output = (unsigned int)max_level;
-		return 0;
-	}
-
 	if (property == GET_FREQ)
 		level = descend ? input : (max_level - input);
 
-- 
2.0.3.693.g996b0fd

[PATCH 21/26] cpu_cooling: create list of cpufreq_cooling_devices

[Viresh Kumar]

That will be used by later patches to iterate over all cpufreq cooling devices.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/thermal/cpu_cooling.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
index ddb97aa..f76a665 100644
--- a/drivers/thermal/cpu_cooling.c
+++ b/drivers/thermal/cpu_cooling.c
@@ -66,8 +66,11 @@ struct cpufreq_cooling_device {
 	unsigned int cpufreq_val;
 	unsigned int max_level;
 	struct cpumask allowed_cpus;
+	struct list_head head;
 };
+
 static DEFINE_IDR(cpufreq_idr);
+static LIST_HEAD(cpufreq_dev_list);
 static DEFINE_MUTEX(cooling_cpufreq_lock);
 
 static unsigned int cpufreq_dev_count;
@@ -372,6 +375,7 @@ __cpufreq_cooling_register(struct device_node *np,
 		goto remove_idr;
 
 	cpufreq_dev->cool_dev = cool_dev;
+	INIT_LIST_HEAD(&cpufreq_dev->head);
 
 	mutex_lock(&cooling_cpufreq_lock);
 
@@ -381,6 +385,7 @@ __cpufreq_cooling_register(struct device_node *np,
 					  CPUFREQ_POLICY_NOTIFIER);
 	cpufreq_dev_count++;
 
+	list_add(&cpufreq_dev->head, &cpufreq_dev_list);
 	mutex_unlock(&cooling_cpufreq_lock);
 
 	return cool_dev;
@@ -451,6 +456,7 @@ void cpufreq_cooling_unregister(struct thermal_cooling_device *cdev)
 	cpufreq_dev = cdev->devdata;
 	mutex_lock(&cooling_cpufreq_lock);
 	cpufreq_dev_count--;
+	list_del(&cpufreq_dev->head);
 
 	/* Unregister the notifier for the last cpufreq cooling device */
 	if (cpufreq_dev_count == 0)
-- 
2.0.3.693.g996b0fd

[PATCH 22/26] cpu_cooling: use cpufreq_dev_list instead of cpufreq_dev_count

[Viresh Kumar]

As we already have a list of cpufreq_cooling_devices now, lets use it instead of
a local counter.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/thermal/cpu_cooling.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
index f76a665..4965a55 100644
--- a/drivers/thermal/cpu_cooling.c
+++ b/drivers/thermal/cpu_cooling.c
@@ -73,8 +73,6 @@ static DEFINE_IDR(cpufreq_idr);
 static LIST_HEAD(cpufreq_dev_list);
 static DEFINE_MUTEX(cooling_cpufreq_lock);
 
-static unsigned int cpufreq_dev_count;
-
 /* notify_table passes value to the CPUFREQ_ADJUST callback function. */
 #define NOTIFY_INVALID NULL
 static struct cpufreq_cooling_device *notify_device;
@@ -380,10 +378,9 @@ __cpufreq_cooling_register(struct device_node *np,
 	mutex_lock(&cooling_cpufreq_lock);
 
 	/* Register the notifier for first cpufreq cooling device */
-	if (cpufreq_dev_count == 0)
+	if (list_empty(&cpufreq_dev_list))
 		cpufreq_register_notifier(&thermal_cpufreq_notifier_block,
 					  CPUFREQ_POLICY_NOTIFIER);
-	cpufreq_dev_count++;
 
 	list_add(&cpufreq_dev->head, &cpufreq_dev_list);
 	mutex_unlock(&cooling_cpufreq_lock);
@@ -455,11 +452,10 @@ void cpufreq_cooling_unregister(struct thermal_cooling_device *cdev)
 
 	cpufreq_dev = cdev->devdata;
 	mutex_lock(&cooling_cpufreq_lock);
-	cpufreq_dev_count--;
 	list_del(&cpufreq_dev->head);
 
 	/* Unregister the notifier for the last cpufreq cooling device */
-	if (cpufreq_dev_count == 0)
+	if (list_empty(&cpufreq_dev_list))
 		cpufreq_unregister_notifier(&thermal_cpufreq_notifier_block,
 					    CPUFREQ_POLICY_NOTIFIER);
 	mutex_unlock(&cooling_cpufreq_lock);
-- 
2.0.3.693.g996b0fd

[PATCH 23/26] cpu_cooling: Pass 'cpufreq_dev' to get_property()

[Viresh Kumar]

We already know the value of 'cpufreq_dev->max_level' and so there is no need
calculating that once again. For this, we need to send 'cpufreq_dev' to
get_property().

Make all necessary changes for this change. Because cpufreq_cooling_get_level()
doesn't have access to 'cpufreq_dev', it is updated to iterate over the list of
cpufreq_cooling_devices to get cooling device for the cpu number passed to it.
This also makes it robust to return levels only for the CPU registered via a
cooling device. We don't have to support anything that isn't registered yet.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/thermal/cpu_cooling.c | 47 ++++++++++++++++++++++++-------------------
 1 file changed, 26 insertions(+), 21 deletions(-)

diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
index 4965a55..07414c5 100644
--- a/drivers/thermal/cpu_cooling.c
+++ b/drivers/thermal/cpu_cooling.c
@@ -86,7 +86,7 @@ enum cpufreq_cooling_property {
 
 /**
  * get_property - fetch a property of interest for a given cpu.
- * @cpu: cpu for which the property is required
+ * @cpufreq_dev: cpufreq_dev for which the property is required
  * @input: query parameter
  * @output: query return
  * @property: type of query (frequency, level)
@@ -103,20 +103,20 @@ enum cpufreq_cooling_property {
  *
  * Return: 0 on success, -EINVAL when invalid parameters are passed.
  */
-static int get_property(unsigned int cpu, unsigned long input,
-			unsigned int *output,
+static int get_property(struct cpufreq_cooling_device *cpufreq_dev,
+			unsigned long input, unsigned int *output,
 			enum cpufreq_cooling_property property)
 {
 	int i;
-	unsigned long max_level = 0, level = 0;
+	unsigned long level = 0;
 	unsigned int freq = CPUFREQ_ENTRY_INVALID;
 	int descend = -1;
-	struct cpufreq_frequency_table *pos, *table =
-					cpufreq_frequency_get_table(cpu);
+	struct cpufreq_frequency_table *pos, *table;
 
 	if (!output)
 		return -EINVAL;
 
+	table = cpufreq_frequency_get_table(cpumask_first(&cpufreq_dev->allowed_cpus));
 	if (!table)
 		return -EINVAL;
 
@@ -130,18 +130,10 @@ static int get_property(unsigned int cpu, unsigned long input,
 			descend = freq > pos->frequency;
 
 		freq = pos->frequency;
-		max_level++;
 	}
 
-	/* No valid cpu frequency entry */
-	if (max_level == 0)
-		return -EINVAL;
-
-	/* max_level is an index, not a counter */
-	max_level--;
-
 	if (property == GET_FREQ)
-		level = descend ? input : (max_level - input);
+		level = descend ? input : (cpufreq_dev->max_level - input);
 
 	i = 0;
 	cpufreq_for_each_valid_entry(pos, table) {
@@ -154,7 +146,7 @@ static int get_property(unsigned int cpu, unsigned long input,
 
 		if (property == GET_LEVEL && (unsigned int)input == freq) {
 			/* get level by frequency */
-			*output = descend ? i : (max_level - i);
+			*output = descend ? i : (cpufreq_dev->max_level - i);
 			return 0;
 		}
 		if (property == GET_FREQ && level == i) {
@@ -181,12 +173,25 @@ static int get_property(unsigned int cpu, unsigned long input,
  */
 unsigned long cpufreq_cooling_get_level(unsigned int cpu, unsigned int freq)
 {
-	unsigned int val;
+	struct cpufreq_cooling_device *cpufreq_dev;
 
-	if (get_property(cpu, (unsigned long)freq, &val, GET_LEVEL))
-		return THERMAL_CSTATE_INVALID;
+	mutex_lock(&cooling_cpufreq_lock);
+	list_for_each_entry(cpufreq_dev, &cpufreq_dev_list, head) {
+		if (cpumask_test_cpu(cpu, &cpufreq_dev->allowed_cpus)) {
+			unsigned int val;
+
+			mutex_unlock(&cooling_cpufreq_lock);
+			if (get_property(cpufreq_dev, (unsigned long)freq, &val,
+					 GET_LEVEL))
+				return THERMAL_CSTATE_INVALID;
+
+			return (unsigned long)val;
+		}
+	}
+	mutex_unlock(&cooling_cpufreq_lock);
 
-	return (unsigned long)val;
+	pr_err("%s: cpu:%d not part of any cooling device\n", __func__, cpu);
+	return THERMAL_CSTATE_INVALID;
 }
 EXPORT_SYMBOL_GPL(cpufreq_cooling_get_level);
 
@@ -289,7 +294,7 @@ static int cpufreq_set_cur_state(struct thermal_cooling_device *cdev,
 	if (cpufreq_device->cpufreq_state == state)
 		return 0;
 
-	ret = get_property(cpu, state, &clip_freq, GET_FREQ);
+	ret = get_property(cpufreq_device, state, &clip_freq, GET_FREQ);
 	if (ret)
 		return ret;
 
-- 
2.0.3.693.g996b0fd

[PATCH 24/26] cpu_cooling: Store frequencies in descending order

[Viresh Kumar]

CPUFreq framework *doesn't* guarantee that frequencies present in cpufreq table
will be in ascending or descending order. But cpu_cooling somehow assumes that.

Probably because most of current users are creating this list from DT, which is
done with the help of OPP layer. And OPP layer creates the list in ascending
order of frequencies.

But cpu_cooling can be used for other platforms too, which don't have
frequencies arranged in any order.

This patch tries to fix this issue by creating another list of valid frequencies
in descending order. Care is also taken to throw warnings for duplicate entries.

Later patches would use it to simplify code.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/thermal/cpu_cooling.c | 41 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 40 insertions(+), 1 deletion(-)

diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
index 07414c5..9a4a323 100644
--- a/drivers/thermal/cpu_cooling.c
+++ b/drivers/thermal/cpu_cooling.c
@@ -65,6 +65,7 @@ struct cpufreq_cooling_device {
 	unsigned int cpufreq_state;
 	unsigned int cpufreq_val;
 	unsigned int max_level;
+	unsigned int *freq_table;	/* In descending order */
 	struct cpumask allowed_cpus;
 	struct list_head head;
 };
@@ -321,6 +322,20 @@ static struct notifier_block thermal_cpufreq_notifier_block = {
 	.notifier_call = cpufreq_thermal_notifier,
 };
 
+static unsigned int find_next_max(struct cpufreq_frequency_table *table,
+				  unsigned int prev_max)
+{
+	struct cpufreq_frequency_table *pos;
+	unsigned int max = 0;
+
+	cpufreq_for_each_valid_entry(pos, table) {
+		if (pos->frequency > max && pos->frequency < prev_max)
+			max = pos->frequency;
+	}
+
+	return max;
+}
+
 /**
  * __cpufreq_cooling_register - helper function to create cpufreq cooling device
  * @np: a valid struct device_node to the cooling device device tree node
@@ -343,6 +358,7 @@ __cpufreq_cooling_register(struct device_node *np,
 	struct cpufreq_cooling_device *cpufreq_dev;
 	char dev_name[THERMAL_NAME_LENGTH];
 	struct cpufreq_frequency_table *pos, *table;
+	unsigned int freq, i;
 
 	table = cpufreq_frequency_get_table(cpumask_first(clip_cpus));
 	if (!table) {
@@ -358,6 +374,14 @@ __cpufreq_cooling_register(struct device_node *np,
 	cpufreq_for_each_valid_entry(pos, table)
 		cpufreq_dev->max_level++;
 
+	cpufreq_dev->freq_table = kmalloc(sizeof(*cpufreq_dev->freq_table) *
+					  cpufreq_dev->max_level, GFP_KERNEL);
+	if (!cpufreq_dev->freq_table) {
+		return ERR_PTR(-ENOMEM);
+		cool_dev = ERR_PTR(-ENOMEM);
+		goto free_cdev;
+	}
+
 	/* max_level is an index, not a counter */
 	cpufreq_dev->max_level--;
 
@@ -366,7 +390,7 @@ __cpufreq_cooling_register(struct device_node *np,
 	cpufreq_dev->id = idr_alloc(&cpufreq_idr, NULL, 0, 0, GFP_KERNEL);
 	if (unlikely(cpufreq_dev->id < 0)) {
 		cool_dev = ERR_PTR(cpufreq_dev->id);
-		goto free_cdev;
+		goto free_table;
 	}
 
 	snprintf(dev_name, sizeof(dev_name), "thermal-cpufreq-%d",
@@ -377,6 +401,18 @@ __cpufreq_cooling_register(struct device_node *np,
 	if (IS_ERR(cool_dev))
 		goto remove_idr;
 
+	/* Fill freq-table in descending order of frequencies */
+	for (i = 0, freq = -1; i <= cpufreq_dev->max_level; i++) {
+		freq = find_next_max(table, freq);
+		cpufreq_dev->freq_table[i] = freq;
+
+		/* Warn for duplicate entries */
+		if (!freq)
+			pr_warn("%s: table has duplicate entries\n", __func__);
+		else
+			pr_debug("%s: freq:%u KHz\n", __func__, freq);
+	}
+
 	cpufreq_dev->cool_dev = cool_dev;
 	INIT_LIST_HEAD(&cpufreq_dev->head);
 
@@ -394,6 +430,8 @@ __cpufreq_cooling_register(struct device_node *np,
 
 remove_idr:
 	idr_remove(&cpufreq_idr, cpufreq_dev->id);
+free_table:
+	kfree(cpufreq_dev->freq_table);
 free_cdev:
 	kfree(cpufreq_dev);
 
@@ -467,6 +505,7 @@ void cpufreq_cooling_unregister(struct thermal_cooling_device *cdev)
 
 	thermal_cooling_device_unregister(cpufreq_dev->cool_dev);
 	idr_remove(&cpufreq_idr, cpufreq_dev->id);
+	kfree(cpufreq_dev->freq_table);
 	kfree(cpufreq_dev);
 }
 EXPORT_SYMBOL_GPL(cpufreq_cooling_unregister);
-- 
2.0.3.693.g996b0fd

[PATCH 25/26] cpu_cooling: Use cpufreq_dev->freq_table for finding level/freq

[Viresh Kumar]

get_property() was an over complicated beast with BUGs. It used to believe that
cpufreq table is present in ascending or descending order, which might not
always be true.

Previous patch has created another freq table in descending order for us and we
better use it now. With that get_property() simply goes away and another helper
get_level() comes in.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/thermal/cpu_cooling.c | 96 +++++++------------------------------------
 1 file changed, 14 insertions(+), 82 deletions(-)

diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
index 9a4a323..db4c001 100644
--- a/drivers/thermal/cpu_cooling.c
+++ b/drivers/thermal/cpu_cooling.c
@@ -80,85 +80,27 @@ static struct cpufreq_cooling_device *notify_device;
 
 /* Below code defines functions to be used for cpufreq as cooling device */
 
-enum cpufreq_cooling_property {
-	GET_LEVEL,
-	GET_FREQ,
-};
-
 /**
- * get_property - fetch a property of interest for a given cpu.
+ * get_level: Find the level for a particular frequency
  * @cpufreq_dev: cpufreq_dev for which the property is required
- * @input: query parameter
- * @output: query return
- * @property: type of query (frequency, level)
- *
- * This is the common function to
- * 1. translate frequency to cooling state
- * 2. translate cooling state to frequency
+ * @freq: Frequency
  *
- * Note that the code may be not in good shape
- * but it is written in this way in order to:
- * a) reduce duplicate code as most of the code can be shared.
- * b) make sure the logic is consistent when translating between
- *    cooling states and frequencies.
- *
- * Return: 0 on success, -EINVAL when invalid parameters are passed.
+ * Returns: level on success, THERMAL_CSTATE_INVALID on error.
  */
-static int get_property(struct cpufreq_cooling_device *cpufreq_dev,
-			unsigned long input, unsigned int *output,
-			enum cpufreq_cooling_property property)
+static unsigned long get_level(struct cpufreq_cooling_device *cpufreq_dev,
+			       unsigned int freq)
 {
-	int i;
-	unsigned long level = 0;
-	unsigned int freq = CPUFREQ_ENTRY_INVALID;
-	int descend = -1;
-	struct cpufreq_frequency_table *pos, *table;
-
-	if (!output)
-		return -EINVAL;
+	unsigned long level;
 
-	table = cpufreq_frequency_get_table(cpumask_first(&cpufreq_dev->allowed_cpus));
-	if (!table)
-		return -EINVAL;
+	for (level = 0; level <= cpufreq_dev->max_level; level++) {
+		if (freq == cpufreq_dev->freq_table[level])
+			return level;
 
-	cpufreq_for_each_valid_entry(pos, table) {
-		/* ignore duplicate entry */
-		if (freq == pos->frequency)
-			continue;
-
-		/* get the frequency order */
-		if (freq != CPUFREQ_ENTRY_INVALID && descend == -1)
-			descend = freq > pos->frequency;
-
-		freq = pos->frequency;
+		if (freq > cpufreq_dev->freq_table[level])
+			break;
 	}
 
-	if (property == GET_FREQ)
-		level = descend ? input : (cpufreq_dev->max_level - input);
-
-	i = 0;
-	cpufreq_for_each_valid_entry(pos, table) {
-		/* ignore duplicate entry */
-		if (freq == pos->frequency)
-			continue;
-
-		/* now we have a valid frequency entry */
-		freq = pos->frequency;
-
-		if (property == GET_LEVEL && (unsigned int)input == freq) {
-			/* get level by frequency */
-			*output = descend ? i : (cpufreq_dev->max_level - i);
-			return 0;
-		}
-		if (property == GET_FREQ && level == i) {
-			/* get frequency by level */
-			*output = freq;
-			return 0;
-		}
-		i++;
-	}
-
-	return -EINVAL;
+	return THERMAL_CSTATE_INVALID;
 }
 
 /**
@@ -179,14 +121,8 @@ unsigned long cpufreq_cooling_get_level(unsigned int cpu, unsigned int freq)
 	mutex_lock(&cooling_cpufreq_lock);
 	list_for_each_entry(cpufreq_dev, &cpufreq_dev_list, head) {
 		if (cpumask_test_cpu(cpu, &cpufreq_dev->allowed_cpus)) {
-			unsigned int val;
-
 			mutex_unlock(&cooling_cpufreq_lock);
-			if (get_property(cpufreq_dev, (unsigned long)freq, &val,
-					 GET_LEVEL))
-				return THERMAL_CSTATE_INVALID;
-
-			return (unsigned long)val;
+			return get_level(cpufreq_dev, freq);
 		}
 	}
 	mutex_unlock(&cooling_cpufreq_lock);
@@ -289,16 +225,12 @@ static int cpufreq_set_cur_state(struct thermal_cooling_device *cdev,
 	struct cpufreq_cooling_device *cpufreq_device = cdev->devdata;
 	unsigned int cpu = cpumask_any(&cpufreq_device->allowed_cpus);
 	unsigned int clip_freq;
-	int ret = 0;
 
 	/* Check if the old cooling action is same as new cooling action */
 	if (cpufreq_device->cpufreq_state == state)
 		return 0;
 
-	ret = get_property(cpufreq_device, state, &clip_freq, GET_FREQ);
-	if (ret)
-		return ret;
-
+	clip_freq = cpufreq_device->freq_table[state];
 	cpufreq_device->cpufreq_state = state;
 	cpufreq_device->cpufreq_val = clip_freq;
 	notify_device = cpufreq_device;
-- 
2.0.3.693.g996b0fd

[PATCH 26/26] cpu_cooling: update copyright tags

[Viresh Kumar]

Adding my copyright information for two purposes:
- To get cc'd for future patches to review (Only if people read this header
  while sending mail)
- Have done enough changes to earn a place here?

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
Drop this patch if you didn't like it Eduardo :)
---
 drivers/thermal/cpu_cooling.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
index db4c001..4369963 100644
--- a/drivers/thermal/cpu_cooling.c
+++ b/drivers/thermal/cpu_cooling.c
@@ -4,6 +4,8 @@
  *  Copyright (C) 2012	Samsung Electronics Co., Ltd(http://www.samsung.com)
  *  Copyright (C) 2012  Amit Daniel <amit.kachhap@linaro.org>
  *
+ *  Copyright (C) 2014  Viresh Kumar <viresh.kumar@linaro.org>
+ *
  * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  *  This program is free software; you can redistribute it and/or modify
  *  it under the terms of the GNU General Public License as published by
-- 
2.0.3.693.g996b0fd

Re: [PATCH 00/26] thermal: cpu_cooling: Fixes and cleanups

[Eduardo Valentin]

[-- Attachment #1: Type: text/plain, Size: 4637 bytes --]


Hello Viresh,

On Fri, Nov 28, 2014 at 03:13:54PM +0530, Viresh Kumar wrote:
> Hi Eduardo,
> 
> As you know I got into fixing cpu_cooling.c due to some cpufreq issues you and
> Lukasz were struggling with. I found some issues in cpu_cooling then and here
> are the fixes/cleanups.
> 

Ok. No problem. As I mentioned before, good to have a review in the code
by someone with extra experience with cpufreq. 

Well, I don't think there was an issue with cpufreq itself. The struggle
is with sequencing, specially during booting. The Linux booting sequencing,
when it comes to builtin module dependency and probing, does not help
much. So, we need to do some tricks with the APIs.


> Sorry for the long list. Haven't broken them into smaller sets as most of the
> patches are very small, easy to review and inter-dependent. Only few of them
> should take more time to review. If this doesn't work out, let me know and I
> will try to send separate inter-dependent sets.
> 

I am fine with this approach, because now you are dealing with a single
target: refactoring cpu cooling code.


> Just apply whatever looks fine and I will update/resend the ones left in V2 if
> at required.

sure, I will have a look.

> 
> First few are updates to platform drivers. Exynos fails to register after few
> patches in this series as it doesn't handle -EPROBE_DEFER properly (reported
> that in reply to your patch as well). Others weren't setting clip_cpus properly
> and are fixed.

OK. 

About Exynos, at which point/patch Exynos starts to fail?

As I mentioned in the thread about cpu cooling vs. cpufreq, I prefer to
update all users of the updated [of_]cpufreq_cooling_register API, if
you don't mind. 

Can you please elaborate a little more about how this failure is
happening?

> 
> Next ones are cleanups of cpu_cooling.c to get things properly organized.
> 
> Let me know if I screwed it up completely.
> 

hehehe.. Ok. I will let you know.

> Tested-on: Exynos5250 (Dual ARM Cortex A15).
> Rebased-over: v3.18-rc6
> Pushed here: git://git.kernel.org/pub/scm/linux/kernel/git/vireshk/linux.git
> thermal/cpu-cooling-fixes
> 
> Cc: Amit Daniel Kachhap <amit.daniel@samsung.com>
> Cc: Chanwoo Choi <cw00.choi@samsung.com>
> Cc: Hongbo Zhang <hongbo.zhang@linaro.com>
> Cc: Kyungmin Park <kyungmin.park@samsung.com>
> Cc: Lukasz Majewski <l.majewski@samsung.com>
> Cc: Shawn Guo <shawn.guo@linaro.org>
> 
> Viresh Kumar (26):
>   thermal: db8500: pass cpu_present_mask to cpufreq_cooling_register()
>   thermal: imx: pass cpu_present_mask to cpufreq_cooling_register()
>   thermal: exynos: pass cpu_present_mask to cpufreq_cooling_register()
>   thermal: exynos: Handle -EPROBE_DEFER properly
>   cpu_cooling: random comment fixups
>   cpu_cooling: fix doc comment over struct cpufreq_cooling_device
>   cpu_cooling: Add comment to clarify relation between cooling state and
>     frequency
>   cpu_cooling: Pass variable instead of its type to sizeof()
>   cpu_cooling: no need to set cpufreq_state to zero
>   cpu_cooling: no need to set cpufreq_dev to NULL
>   cpu_cooling: propagate error returned by idr_alloc()
>   cpu_cooling: Don't match min/max frequencies for all CPUs on cooling
>     register
>   cpu_cooling: don't iterate over all allowed_cpus to update cpufreq
>     policy
>   cpu_cooling: Don't check is_cpufreq_valid()
>   cpu_cooling: do error handling at the bottom in
>     __cpufreq_cooling_register()
>   cpu_cooling: Drop useless locking around idr_alloc/idr_remove
>   cpu_cooling: Merge cpufreq_apply_cooling() into
>     cpufreq_set_cur_state()
>   cpu_cooling: Merge get_cpu_frequency() into cpufreq_set_cur_state()
>   cpu_cooling: find max level during device registration
>   cpu_cooling: get_property() doesn't need to support GET_MAXL anymore
>   cpu_cooling: create list of cpufreq_cooling_devices
>   cpu_cooling: use cpufreq_dev_list instead of cpufreq_dev_count
>   cpu_cooling: Pass 'cpufreq_dev' to get_property()
>   cpu_cooling: Store frequencies in descending order
>   cpu_cooling: Use cpufreq_dev->freq_table for finding level/freq
>   cpu_cooling: update copyright tags
> 
>  drivers/thermal/cpu_cooling.c                   | 405 +++++++++---------------
>  drivers/thermal/db8500_cpufreq_cooling.c        |   5 +-
>  drivers/thermal/imx_thermal.c                   |   4 +-
>  drivers/thermal/samsung/exynos_thermal_common.c |  11 +-
>  drivers/thermal/samsung/exynos_tmu.c            |   4 +-
>  5 files changed, 153 insertions(+), 276 deletions(-)
> 
> -- 
> 2.0.3.693.g996b0fd
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 00/26] thermal: cpu_cooling: Fixes and cleanups

[Viresh Kumar]

On 28 November 2014 at 18:56, Eduardo Valentin <edubezval@gmail.com> wrote:
> About Exynos, at which point/patch Exynos starts to fail?

e2cb864 cpu_cooling: find max level during device registration

> As I mentioned in the thread about cpu cooling vs. cpufreq, I prefer to
> update all users of the updated [of_]cpufreq_cooling_register API, if
> you don't mind.
>
> Can you please elaborate a little more about how this failure is
> happening?

Its the same problem, cpufreq driver isn't ready and
cpufreq_frequency_get_table() starts failing :)

Re: [PATCH 08/26] cpu_cooling: Pass variable instead of its type to sizeof()

[Javi Merino]

Hi Viresh,

On Fri, Nov 28, 2014 at 09:44:02AM +0000, Viresh Kumar wrote:
> Just following coding guidelines here.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/thermal/cpu_cooling.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
> index def0f21..59725d8 100644
> --- a/drivers/thermal/cpu_cooling.c
> +++ b/drivers/thermal/cpu_cooling.c
> @@ -468,8 +468,7 @@ __cpufreq_cooling_register(struct device_node *np,
>  				return ERR_PTR(-EINVAL);
>  		}
>  	}
> -	cpufreq_dev = kzalloc(sizeof(struct cpufreq_cooling_device),
> -			      GFP_KERNEL);
> +	cpufreq_dev = kzalloc(sizeof(*cool_dev), GFP_KERNEL);

This should be:

+	cpufreq_dev = kzalloc(sizeof(*cpufreq_dev), GFP_KERNEL);

Cheers,
Javi

Re: [PATCH 11/26] cpu_cooling: propagate error returned by idr_alloc()

[Javi Merino]

On Fri, Nov 28, 2014 at 09:44:05AM +0000, Viresh Kumar wrote:
> We aren't supposed to return our own error type here. Return what we got.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/thermal/cpu_cooling.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
> index bb11dd4..964586f 100644
> --- a/drivers/thermal/cpu_cooling.c
> +++ b/drivers/thermal/cpu_cooling.c
> @@ -477,7 +477,7 @@ __cpufreq_cooling_register(struct device_node *np,
>  	ret = get_idr(&cpufreq_idr, &cpufreq_dev->id);
>  	if (ret) {
>  		kfree(cpufreq_dev);
> -		return ERR_PTR(-EINVAL);
> +		return ERR_PTR(cpufreq_dev->id);

The error is ret, not the id which is probably 0 if there was an
error. So:

+		return ERR_PTR(ret);

Cheers,
Javi

Re: [PATCH 15/26] cpu_cooling: do error handling at the bottom in __cpufreq_cooling_register()

[Javi Merino]

On Fri, Nov 28, 2014 at 09:44:09AM +0000, Viresh Kumar wrote:
> This makes life easy and bug free. And is scalable for future resource
> allocations.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

Yay!  We have a similar fix as part of our patches for the power
allocator governor (the only difference is that the instead of calling
the label "remove_idr", we call it "release_idr").  So quite happy to
see this going in.  FWIW,

Acked-by: Javi Merino <javi.merino@arm.com>

Re: [PATCH 16/26] cpu_cooling: Drop useless locking around idr_alloc/idr_remove

[Javi Merino]

Hi Viresh,

On Fri, Nov 28, 2014 at 09:44:10AM +0000, Viresh Kumar wrote:
> Locking around idr_alloc/idr_remove looks rather pointless as there is no race
> it is trying to fix. Remove it.

You are assuming that all cpufreq cooling devices are registered
sequentially, one after the other.  That doesn't need be the case.  I
don't think the performance that you get from this patch justifies the
possible races that could be introduced by not having the locking.
Why should we remove this?

> get_idr() and release_idr() aren't much useful now, so get rid of them as well.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
> @Eduardo: Same is true for thermal-core as well ?

I think that my previous concern applies to thermal_core as well,
thermal zones may not be initialised sequentially.

Cheers,
Javi

Re: [PATCH 26/26] cpu_cooling: update copyright tags

[Eduardo Valentin]

[-- Attachment #1: Type: text/plain, Size: 1504 bytes --]

Hello Viresh K,

On Fri, Nov 28, 2014 at 03:14:20PM +0530, Viresh Kumar wrote:
> Adding my copyright information for two purposes:
> - To get cc'd for future patches to review (Only if people read this header
>   while sending mail)

For this item, I get_maintainers might copy you due the amount of
commit hits, no?

> - Have done enough changes to earn a place here?
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
> Drop this patch if you didn't like it Eduardo :)

I am not against your patch, no.

But, I am probably not the correct person to judge this though. If you get a
confirmation from the original copyright holder (amit/samsung), then I
am glad to add this.

> ---
>  drivers/thermal/cpu_cooling.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
> index db4c001..4369963 100644
> --- a/drivers/thermal/cpu_cooling.c
> +++ b/drivers/thermal/cpu_cooling.c
> @@ -4,6 +4,8 @@
>   *  Copyright (C) 2012	Samsung Electronics Co., Ltd(http://www.samsung.com)
>   *  Copyright (C) 2012  Amit Daniel <amit.kachhap@linaro.org>
>   *
> + *  Copyright (C) 2014  Viresh Kumar <viresh.kumar@linaro.org>
> + *
>   * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>   *  This program is free software; you can redistribute it and/or modify
>   *  it under the terms of the GNU General Public License as published by
> -- 
> 2.0.3.693.g996b0fd
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 11/26] cpu_cooling: propagate error returned by idr_alloc()

[Eduardo Valentin]

[-- Attachment #1: Type: text/plain, Size: 950 bytes --]

Hello Viresh,

On Fri, Nov 28, 2014 at 03:14:05PM +0530, Viresh Kumar wrote:
> We aren't supposed to return our own error type here. Return what we got.
> 

OK..

> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/thermal/cpu_cooling.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
> index bb11dd4..964586f 100644
> --- a/drivers/thermal/cpu_cooling.c
> +++ b/drivers/thermal/cpu_cooling.c
> @@ -477,7 +477,7 @@ __cpufreq_cooling_register(struct device_node *np,
>  	ret = get_idr(&cpufreq_idr, &cpufreq_dev->id);
>  	if (ret) {
>  		kfree(cpufreq_dev);
> -		return ERR_PTR(-EINVAL);
> +		return ERR_PTR(cpufreq_dev->id);

cpufreq_dev is an invalid object here. Maybe you want to use ret in your
patch?
>  	}
>  
>  	snprintf(dev_name, sizeof(dev_name), "thermal-cpufreq-%d",
> -- 
> 2.0.3.693.g996b0fd
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 16/26] cpu_cooling: Drop useless locking around idr_alloc/idr_remove

[Eduardo Valentin]

[-- Attachment #1: Type: text/plain, Size: 3473 bytes --]

On Fri, Nov 28, 2014 at 03:14:10PM +0530, Viresh Kumar wrote:
> Locking around idr_alloc/idr_remove looks rather pointless as there is no race
> it is trying to fix. Remove it.
> 

Can you please elaborate on how the idr's are going to be serialize
without the lock?

> get_idr() and release_idr() aren't much useful now, so get rid of them as well.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
> @Eduardo: Same is true for thermal-core as well ?

Probably not ?

> ---
>  drivers/thermal/cpu_cooling.c | 45 ++++---------------------------------------
>  1 file changed, 4 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
> index 032cba3..ca8f1bb 100644
> --- a/drivers/thermal/cpu_cooling.c
> +++ b/drivers/thermal/cpu_cooling.c
> @@ -73,42 +73,6 @@ static unsigned int cpufreq_dev_count;
>  #define NOTIFY_INVALID NULL
>  static struct cpufreq_cooling_device *notify_device;
>  
> -/**
> - * get_idr - function to get a unique id.
> - * @idr: struct idr * handle used to create a id.
> - * @id: int * value generated by this function.
> - *
> - * This function will populate @id with an unique
> - * id, using the idr API.
> - *
> - * Return: 0 on success, an error code on failure.
> - */
> -static int get_idr(struct idr *idr, int *id)
> -{
> -	int ret;
> -
> -	mutex_lock(&cooling_cpufreq_lock);
> -	ret = idr_alloc(idr, NULL, 0, 0, GFP_KERNEL);
> -	mutex_unlock(&cooling_cpufreq_lock);
> -	if (unlikely(ret < 0))
> -		return ret;
> -	*id = ret;
> -
> -	return 0;
> -}
> -
> -/**
> - * release_idr - function to free the unique id.
> - * @idr: struct idr * handle used for creating the id.
> - * @id: int value representing the unique id.
> - */
> -static void release_idr(struct idr *idr, int id)
> -{
> -	mutex_lock(&cooling_cpufreq_lock);
> -	idr_remove(idr, id);
> -	mutex_unlock(&cooling_cpufreq_lock);
> -}
> -
>  /* Below code defines functions to be used for cpufreq as cooling device */
>  
>  enum cpufreq_cooling_property {
> @@ -430,7 +394,6 @@ __cpufreq_cooling_register(struct device_node *np,
>  	struct thermal_cooling_device *cool_dev;
>  	struct cpufreq_cooling_device *cpufreq_dev;
>  	char dev_name[THERMAL_NAME_LENGTH];
> -	int ret = 0;
>  
>  	cpufreq_dev = kzalloc(sizeof(*cool_dev), GFP_KERNEL);
>  	if (!cpufreq_dev)
> @@ -438,8 +401,8 @@ __cpufreq_cooling_register(struct device_node *np,
>  
>  	cpumask_copy(&cpufreq_dev->allowed_cpus, clip_cpus);
>  
> -	ret = get_idr(&cpufreq_idr, &cpufreq_dev->id);
> -	if (ret) {
> +	cpufreq_dev->id = idr_alloc(&cpufreq_idr, NULL, 0, 0, GFP_KERNEL);
> +	if (unlikely(cpufreq_dev->id < 0)) {
>  		cool_dev = ERR_PTR(cpufreq_dev->id);
>  		goto free_cdev;
>  	}
> @@ -467,7 +430,7 @@ __cpufreq_cooling_register(struct device_node *np,
>  	return cool_dev;
>  
>  remove_idr:
> -	release_idr(&cpufreq_idr, cpufreq_dev->id);
> +	idr_remove(&cpufreq_idr, cpufreq_dev->id);
>  free_cdev:
>  	kfree(cpufreq_dev);
>  
> @@ -540,7 +503,7 @@ void cpufreq_cooling_unregister(struct thermal_cooling_device *cdev)
>  	mutex_unlock(&cooling_cpufreq_lock);
>  
>  	thermal_cooling_device_unregister(cpufreq_dev->cool_dev);
> -	release_idr(&cpufreq_idr, cpufreq_dev->id);
> +	idr_remove(&cpufreq_idr, cpufreq_dev->id);
>  	kfree(cpufreq_dev);
>  }
>  EXPORT_SYMBOL_GPL(cpufreq_cooling_unregister);
> -- 
> 2.0.3.693.g996b0fd
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 08/26] cpu_cooling: Pass variable instead of its type to sizeof()

[Eduardo Valentin]

[-- Attachment #1: Type: text/plain, Size: 986 bytes --]

Hi,

On Tue, Dec 02, 2014 at 03:26:24PM +0000, Javi Merino wrote:
> Hi Viresh,
> 
> On Fri, Nov 28, 2014 at 09:44:02AM +0000, Viresh Kumar wrote:
> > Just following coding guidelines here.
> > 
> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> > ---
> >  drivers/thermal/cpu_cooling.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
> > index def0f21..59725d8 100644
> > --- a/drivers/thermal/cpu_cooling.c
> > +++ b/drivers/thermal/cpu_cooling.c
> > @@ -468,8 +468,7 @@ __cpufreq_cooling_register(struct device_node *np,
> >  				return ERR_PTR(-EINVAL);
> >  		}
> >  	}
> > -	cpufreq_dev = kzalloc(sizeof(struct cpufreq_cooling_device),
> > -			      GFP_KERNEL);
> > +	cpufreq_dev = kzalloc(sizeof(*cool_dev), GFP_KERNEL);
> 
> This should be:
> 
> +	cpufreq_dev = kzalloc(sizeof(*cpufreq_dev), GFP_KERNEL);

Agreed.

> 
> Cheers,
> Javi
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 04/26] thermal: exynos: Handle -EPROBE_DEFER properly

[Eduardo Valentin]

[-- Attachment #1: Type: text/plain, Size: 2491 bytes --]

On Fri, Nov 28, 2014 at 03:13:58PM +0530, Viresh Kumar wrote:
> cpufreq_cooling_register() can return -EPROBE_DEFER if cpufreq driver isn't
> ready yet and so the callers must defer their initialization.
> 
> Exynos thermal drivers weren't handling this well and were raising false error
> message when -EPROBE_DEFER is returned to them.
> 
> Fix them to handle -EPROBE_DEFER.

As mentioned in patch 0, this one has been merged to another patch.

> 
> Cc: Chanwoo Choi <cw00.choi@samsung.com>
> Cc: Kyungmin Park <kyungmin.park@samsung.com>
> Cc: Amit Daniel Kachhap <amit.daniel@samsung.com>
> Cc: Lukasz Majewski <l.majewski@samsung.com>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/thermal/samsung/exynos_thermal_common.c | 7 ++++---
>  drivers/thermal/samsung/exynos_tmu.c            | 4 +++-
>  2 files changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/thermal/samsung/exynos_thermal_common.c b/drivers/thermal/samsung/exynos_thermal_common.c
> index bf39212..0be1d54 100644
> --- a/drivers/thermal/samsung/exynos_thermal_common.c
> +++ b/drivers/thermal/samsung/exynos_thermal_common.c
> @@ -369,9 +369,10 @@ int exynos_register_thermal(struct thermal_sensor_conf *sensor_conf)
>  		th_zone->cool_dev[th_zone->cool_dev_size] =
>  				cpufreq_cooling_register(cpu_present_mask);
>  		if (IS_ERR(th_zone->cool_dev[th_zone->cool_dev_size])) {
> -			dev_err(sensor_conf->dev,
> -				"Failed to register cpufreq cooling device\n");
> -			ret = -EINVAL;
> +			ret = PTR_ERR(th_zone->cool_dev[th_zone->cool_dev_size]);
> +			if (ret != -EPROBE_DEFER)
> +				dev_err(sensor_conf->dev,
> +					"Failed to register cpufreq cooling device\n");
>  			goto err_unregister;
>  		}
>  		th_zone->cool_dev_size++;
> diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
> index 49c0924..cc3677f 100644
> --- a/drivers/thermal/samsung/exynos_tmu.c
> +++ b/drivers/thermal/samsung/exynos_tmu.c
> @@ -683,7 +683,9 @@ static int exynos_tmu_probe(struct platform_device *pdev)
>  	/* Register the sensor with thermal management interface */
>  	ret = exynos_register_thermal(sensor_conf);
>  	if (ret) {
> -		dev_err(&pdev->dev, "Failed to register thermal interface\n");
> +		if (ret != -EPROBE_DEFER)
> +			dev_err(&pdev->dev,
> +				"Failed to register thermal interface\n");
>  		goto err_clk;
>  	}
>  	data->reg_conf = sensor_conf;
> -- 
> 2.0.3.693.g996b0fd
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 05/26] cpu_cooling: random comment fixups

[Eduardo Valentin]

[-- Attachment #1: Type: text/plain, Size: 1566 bytes --]

On Fri, Nov 28, 2014 at 03:13:59PM +0530, Viresh Kumar wrote:
> s/give/given
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

Acked-by: Eduardo Valentin <edubezval@gmail.com>

> ---
>  drivers/thermal/cpu_cooling.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
> index 1ab0018..7c2ba2e 100644
> --- a/drivers/thermal/cpu_cooling.c
> +++ b/drivers/thermal/cpu_cooling.c
> @@ -122,7 +122,7 @@ enum cpufreq_cooling_property {
>  };
>  
>  /**
> - * get_property - fetch a property of interest for a give cpu.
> + * get_property - fetch a property of interest for a given cpu.
>   * @cpu: cpu for which the property is required
>   * @input: query parameter
>   * @output: query return
> @@ -132,6 +132,7 @@ enum cpufreq_cooling_property {
>   * 1. get maximum cpu cooling states
>   * 2. translate frequency to cooling state
>   * 3. translate cooling state to frequency
> + *
>   * Note that the code may be not in good shape
>   * but it is written in this way in order to:
>   * a) reduce duplicate code as most of the code can be shared.
> @@ -212,7 +213,7 @@ static int get_property(unsigned int cpu, unsigned long input,
>  }
>  
>  /**
> - * cpufreq_cooling_get_level - for a give cpu, return the cooling level.
> + * cpufreq_cooling_get_level - for a given cpu, return the cooling level.
>   * @cpu: cpu for which the level is required
>   * @freq: the frequency of interest
>   *
> -- 
> 2.0.3.693.g996b0fd
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 21/26] cpu_cooling: create list of cpufreq_cooling_devices

[Eduardo Valentin]

[-- Attachment #1: Type: text/plain, Size: 2124 bytes --]

Viresh,

On Fri, Nov 28, 2014 at 03:14:15PM +0530, Viresh Kumar wrote:
> That will be used by later patches to iterate over all cpufreq cooling devices.
> 

Can you please refresh the series on top of latest Linus tree (material
for 3.18-rc7) ?


> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/thermal/cpu_cooling.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
> index ddb97aa..f76a665 100644
> --- a/drivers/thermal/cpu_cooling.c
> +++ b/drivers/thermal/cpu_cooling.c
> @@ -66,8 +66,11 @@ struct cpufreq_cooling_device {
>  	unsigned int cpufreq_val;
>  	unsigned int max_level;
>  	struct cpumask allowed_cpus;
> +	struct list_head head;

Yadwinder has already introduced a list of cpu cooling devs:
commit 2dcd851fe4b4fe60c2f8520bf7668d7e9b2dd76b
Author: Yadwinder Singh Brar <yadi.brar@samsung.com>
Date:   Fri Nov 7 19:12:29 2014 +0530

    thermal: cpu_cooling: Update always cpufreq policy with thermal
    constraints
        
>  };
> +
>  static DEFINE_IDR(cpufreq_idr);
> +static LIST_HEAD(cpufreq_dev_list);
>  static DEFINE_MUTEX(cooling_cpufreq_lock);
>  
>  static unsigned int cpufreq_dev_count;
> @@ -372,6 +375,7 @@ __cpufreq_cooling_register(struct device_node *np,
>  		goto remove_idr;
>  
>  	cpufreq_dev->cool_dev = cool_dev;
> +	INIT_LIST_HEAD(&cpufreq_dev->head);
>  
>  	mutex_lock(&cooling_cpufreq_lock);
>  
> @@ -381,6 +385,7 @@ __cpufreq_cooling_register(struct device_node *np,
>  					  CPUFREQ_POLICY_NOTIFIER);
>  	cpufreq_dev_count++;
>  
> +	list_add(&cpufreq_dev->head, &cpufreq_dev_list);
>  	mutex_unlock(&cooling_cpufreq_lock);
>  
>  	return cool_dev;
> @@ -451,6 +456,7 @@ void cpufreq_cooling_unregister(struct thermal_cooling_device *cdev)
>  	cpufreq_dev = cdev->devdata;
>  	mutex_lock(&cooling_cpufreq_lock);
>  	cpufreq_dev_count--;
> +	list_del(&cpufreq_dev->head);
>  
>  	/* Unregister the notifier for the last cpufreq cooling device */
>  	if (cpufreq_dev_count == 0)
> -- 
> 2.0.3.693.g996b0fd
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 24/26] cpu_cooling: Store frequencies in descending order

[Eduardo Valentin]

[-- Attachment #1: Type: text/plain, Size: 4720 bytes --]

Viresh,

On Fri, Nov 28, 2014 at 03:14:18PM +0530, Viresh Kumar wrote:
> CPUFreq framework *doesn't* guarantee that frequencies present in cpufreq table
> will be in ascending or descending order. But cpu_cooling somehow assumes that.
> 
> Probably because most of current users are creating this list from DT, which is
> done with the help of OPP layer. And OPP layer creates the list in ascending
> order of frequencies.
> 
> But cpu_cooling can be used for other platforms too, which don't have
> frequencies arranged in any order.
> 
> This patch tries to fix this issue by creating another list of valid frequencies
> in descending order. Care is also taken to throw warnings for duplicate entries.
> 
> Later patches would use it to simplify code.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/thermal/cpu_cooling.c | 41 ++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 40 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
> index 07414c5..9a4a323 100644
> --- a/drivers/thermal/cpu_cooling.c
> +++ b/drivers/thermal/cpu_cooling.c
> @@ -65,6 +65,7 @@ struct cpufreq_cooling_device {
>  	unsigned int cpufreq_state;
>  	unsigned int cpufreq_val;
>  	unsigned int max_level;
> +	unsigned int *freq_table;	/* In descending order */
>  	struct cpumask allowed_cpus;
>  	struct list_head head;
>  };
> @@ -321,6 +322,20 @@ static struct notifier_block thermal_cpufreq_notifier_block = {
>  	.notifier_call = cpufreq_thermal_notifier,
>  };
>  
> +static unsigned int find_next_max(struct cpufreq_frequency_table *table,
> +				  unsigned int prev_max)
> +{
> +	struct cpufreq_frequency_table *pos;
> +	unsigned int max = 0;
> +
> +	cpufreq_for_each_valid_entry(pos, table) {
> +		if (pos->frequency > max && pos->frequency < prev_max)

What happens if, for some random reason, the cpufreq table is in
ascending order and this function is called with prev_max == (unsigned
int) -1 ? What would be the returned max?

> +			max = pos->frequency;
> +	}
> +
> +	return max;
> +}
> +
>  /**
>   * __cpufreq_cooling_register - helper function to create cpufreq cooling device
>   * @np: a valid struct device_node to the cooling device device tree node
> @@ -343,6 +358,7 @@ __cpufreq_cooling_register(struct device_node *np,
>  	struct cpufreq_cooling_device *cpufreq_dev;
>  	char dev_name[THERMAL_NAME_LENGTH];
>  	struct cpufreq_frequency_table *pos, *table;
> +	unsigned int freq, i;
>  
>  	table = cpufreq_frequency_get_table(cpumask_first(clip_cpus));
>  	if (!table) {
> @@ -358,6 +374,14 @@ __cpufreq_cooling_register(struct device_node *np,
>  	cpufreq_for_each_valid_entry(pos, table)
>  		cpufreq_dev->max_level++;
>  
> +	cpufreq_dev->freq_table = kmalloc(sizeof(*cpufreq_dev->freq_table) *
> +					  cpufreq_dev->max_level, GFP_KERNEL);
> +	if (!cpufreq_dev->freq_table) {
> +		return ERR_PTR(-ENOMEM);
> +		cool_dev = ERR_PTR(-ENOMEM);
> +		goto free_cdev;
> +	}
> +
>  	/* max_level is an index, not a counter */
>  	cpufreq_dev->max_level--;
>  
> @@ -366,7 +390,7 @@ __cpufreq_cooling_register(struct device_node *np,
>  	cpufreq_dev->id = idr_alloc(&cpufreq_idr, NULL, 0, 0, GFP_KERNEL);
>  	if (unlikely(cpufreq_dev->id < 0)) {
>  		cool_dev = ERR_PTR(cpufreq_dev->id);
> -		goto free_cdev;
> +		goto free_table;
>  	}
>  
>  	snprintf(dev_name, sizeof(dev_name), "thermal-cpufreq-%d",
> @@ -377,6 +401,18 @@ __cpufreq_cooling_register(struct device_node *np,
>  	if (IS_ERR(cool_dev))
>  		goto remove_idr;
>  
> +	/* Fill freq-table in descending order of frequencies */
> +	for (i = 0, freq = -1; i <= cpufreq_dev->max_level; i++) {
> +		freq = find_next_max(table, freq);
> +		cpufreq_dev->freq_table[i] = freq;
> +
> +		/* Warn for duplicate entries */
> +		if (!freq)
> +			pr_warn("%s: table has duplicate entries\n", __func__);
> +		else
> +			pr_debug("%s: freq:%u KHz\n", __func__, freq);
> +	}
> +
>  	cpufreq_dev->cool_dev = cool_dev;
>  	INIT_LIST_HEAD(&cpufreq_dev->head);
>  
> @@ -394,6 +430,8 @@ __cpufreq_cooling_register(struct device_node *np,
>  
>  remove_idr:
>  	idr_remove(&cpufreq_idr, cpufreq_dev->id);
> +free_table:
> +	kfree(cpufreq_dev->freq_table);
>  free_cdev:
>  	kfree(cpufreq_dev);
>  
> @@ -467,6 +505,7 @@ void cpufreq_cooling_unregister(struct thermal_cooling_device *cdev)
>  
>  	thermal_cooling_device_unregister(cpufreq_dev->cool_dev);
>  	idr_remove(&cpufreq_idr, cpufreq_dev->id);
> +	kfree(cpufreq_dev->freq_table);
>  	kfree(cpufreq_dev);
>  }
>  EXPORT_SYMBOL_GPL(cpufreq_cooling_unregister);
> -- 
> 2.0.3.693.g996b0fd
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 25/26] cpu_cooling: Use cpufreq_dev->freq_table for finding level/freq

[Eduardo Valentin]

[-- Attachment #1: Type: text/plain, Size: 5335 bytes --]

On Fri, Nov 28, 2014 at 03:14:19PM +0530, Viresh Kumar wrote:
> get_property() was an over complicated beast with BUGs. It used to believe that
> cpufreq table is present in ascending or descending order, which might not
> always be true.
> 
> Previous patch has created another freq table in descending order for us and we
> better use it now. With that get_property() simply goes away and another helper
> get_level() comes in.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/thermal/cpu_cooling.c | 96 +++++++------------------------------------
>  1 file changed, 14 insertions(+), 82 deletions(-)
> 
> diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
> index 9a4a323..db4c001 100644
> --- a/drivers/thermal/cpu_cooling.c
> +++ b/drivers/thermal/cpu_cooling.c
> @@ -80,85 +80,27 @@ static struct cpufreq_cooling_device *notify_device;
>  
>  /* Below code defines functions to be used for cpufreq as cooling device */
>  
> -enum cpufreq_cooling_property {
> -	GET_LEVEL,
> -	GET_FREQ,
> -};
> -
>  /**
> - * get_property - fetch a property of interest for a given cpu.
> + * get_level: Find the level for a particular frequency
>   * @cpufreq_dev: cpufreq_dev for which the property is required
> - * @input: query parameter
> - * @output: query return
> - * @property: type of query (frequency, level)
> - *
> - * This is the common function to
> - * 1. translate frequency to cooling state
> - * 2. translate cooling state to frequency
> + * @freq: Frequency
>   *
> - * Note that the code may be not in good shape
> - * but it is written in this way in order to:
> - * a) reduce duplicate code as most of the code can be shared.
> - * b) make sure the logic is consistent when translating between
> - *    cooling states and frequencies.
> - *
> - * Return: 0 on success, -EINVAL when invalid parameters are passed.
> + * Returns: level on success, THERMAL_CSTATE_INVALID on error.
>   */
> -static int get_property(struct cpufreq_cooling_device *cpufreq_dev,
> -			unsigned long input, unsigned int *output,
> -			enum cpufreq_cooling_property property)
> +static unsigned long get_level(struct cpufreq_cooling_device *cpufreq_dev,
> +			       unsigned int freq)
>  {
> -	int i;
> -	unsigned long level = 0;
> -	unsigned int freq = CPUFREQ_ENTRY_INVALID;
> -	int descend = -1;
> -	struct cpufreq_frequency_table *pos, *table;
> -
> -	if (!output)
> -		return -EINVAL;
> +	unsigned long level;
>  
> -	table = cpufreq_frequency_get_table(cpumask_first(&cpufreq_dev->allowed_cpus));
> -	if (!table)
> -		return -EINVAL;
> +	for (level = 0; level <= cpufreq_dev->max_level; level++) {
> +		if (freq == cpufreq_dev->freq_table[level])
> +			return level;
>  
> -	cpufreq_for_each_valid_entry(pos, table) {
> -		/* ignore duplicate entry */
> -		if (freq == pos->frequency)
> -			continue;
> -
> -		/* get the frequency order */
> -		if (freq != CPUFREQ_ENTRY_INVALID && descend == -1)
> -			descend = freq > pos->frequency;
> -
> -		freq = pos->frequency;
> +		if (freq > cpufreq_dev->freq_table[level])
> +			break;
>  	}
>  
> -	if (property == GET_FREQ)
> -		level = descend ? input : (cpufreq_dev->max_level - input);
> -
> -	i = 0;
> -	cpufreq_for_each_valid_entry(pos, table) {
> -		/* ignore duplicate entry */
> -		if (freq == pos->frequency)
> -			continue;
> -
> -		/* now we have a valid frequency entry */
> -		freq = pos->frequency;
> -
> -		if (property == GET_LEVEL && (unsigned int)input == freq) {
> -			/* get level by frequency */
> -			*output = descend ? i : (cpufreq_dev->max_level - i);
> -			return 0;
> -		}
> -		if (property == GET_FREQ && level == i) {
> -			/* get frequency by level */
> -			*output = freq;
> -			return 0;
> -		}
> -		i++;
> -	}
> -
> -	return -EINVAL;
> +	return THERMAL_CSTATE_INVALID;
>  }
>  
>  /**
> @@ -179,14 +121,8 @@ unsigned long cpufreq_cooling_get_level(unsigned int cpu, unsigned int freq)
>  	mutex_lock(&cooling_cpufreq_lock);
>  	list_for_each_entry(cpufreq_dev, &cpufreq_dev_list, head) {
>  		if (cpumask_test_cpu(cpu, &cpufreq_dev->allowed_cpus)) {
> -			unsigned int val;
> -
>  			mutex_unlock(&cooling_cpufreq_lock);
> -			if (get_property(cpufreq_dev, (unsigned long)freq, &val,
> -					 GET_LEVEL))
> -				return THERMAL_CSTATE_INVALID;
> -
> -			return (unsigned long)val;
> +			return get_level(cpufreq_dev, freq);
>  		}
>  	}
>  	mutex_unlock(&cooling_cpufreq_lock);
> @@ -289,16 +225,12 @@ static int cpufreq_set_cur_state(struct thermal_cooling_device *cdev,
>  	struct cpufreq_cooling_device *cpufreq_device = cdev->devdata;
>  	unsigned int cpu = cpumask_any(&cpufreq_device->allowed_cpus);
>  	unsigned int clip_freq;
> -	int ret = 0;
>  
>  	/* Check if the old cooling action is same as new cooling action */
>  	if (cpufreq_device->cpufreq_state == state)
>  		return 0;
>  
> -	ret = get_property(cpufreq_device, state, &clip_freq, GET_FREQ);
> -	if (ret)
> -		return ret;
> -
> +	clip_freq = cpufreq_device->freq_table[state];

There should be some check for valid state here..

>  	cpufreq_device->cpufreq_state = state;
>  	cpufreq_device->cpufreq_val = clip_freq;
>  	notify_device = cpufreq_device;
> -- 
> 2.0.3.693.g996b0fd
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 19/26] cpu_cooling: find max level during device registration

[Eduardo Valentin]

[-- Attachment #1: Type: text/plain, Size: 3015 bytes --]

Viresh

On Fri, Nov 28, 2014 at 03:14:13PM +0530, Viresh Kumar wrote:
> CPU frequency tables don't update after the driver is registered and so we don't
> need to iterate over them to find total number of states every time
> cpufreq_get_max_state() is called. Do it once at boot time.

Could you please update me regarding the story behind the opp del patch
set?

http://permalink.gmane.org/gmane.linux.power-management.general/53348

> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/thermal/cpu_cooling.c | 31 +++++++++++++++++++------------
>  1 file changed, 19 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
> index 5815abf..05712d5 100644
> --- a/drivers/thermal/cpu_cooling.c
> +++ b/drivers/thermal/cpu_cooling.c
> @@ -52,6 +52,8 @@
>   *	cooling	devices.
>   * @cpufreq_val: integer value representing the absolute value of the clipped
>   *	frequency.
> + * @max_level: maximum cooling level. One less than total number of valid
> + *	cpufreq frequencies.
>   * @allowed_cpus: all the cpus involved for this cpufreq_cooling_device.
>   *
>   * This structure is required for keeping information of each registered
> @@ -62,6 +64,7 @@ struct cpufreq_cooling_device {
>  	struct thermal_cooling_device *cool_dev;
>  	unsigned int cpufreq_state;
>  	unsigned int cpufreq_val;
> +	unsigned int max_level;
>  	struct cpumask allowed_cpus;
>  };
>  static DEFINE_IDR(cpufreq_idr);
> @@ -246,19 +249,9 @@ static int cpufreq_get_max_state(struct thermal_cooling_device *cdev,
>  				 unsigned long *state)
>  {
>  	struct cpufreq_cooling_device *cpufreq_device = cdev->devdata;
> -	struct cpumask *mask = &cpufreq_device->allowed_cpus;
> -	unsigned int cpu;
> -	unsigned int count = 0;
> -	int ret;
>  
> -	cpu = cpumask_any(mask);
> -
> -	ret = get_property(cpu, 0, &count, GET_MAXL);
> -
> -	if (count > 0)
> -		*state = count;
> -
> -	return ret;
> +	*state = cpufreq_device->max_level;
> +	return 0;
>  }
>  
>  /**
> @@ -351,11 +344,25 @@ __cpufreq_cooling_register(struct device_node *np,
>  	struct thermal_cooling_device *cool_dev;
>  	struct cpufreq_cooling_device *cpufreq_dev;
>  	char dev_name[THERMAL_NAME_LENGTH];
> +	struct cpufreq_frequency_table *pos, *table;
> +
> +	table = cpufreq_frequency_get_table(cpumask_first(clip_cpus));
> +	if (!table) {
> +		pr_debug("%s: CPUFreq table not found\n", __func__);
> +		return ERR_PTR(-EPROBE_DEFER);
> +	}
>  
>  	cpufreq_dev = kzalloc(sizeof(*cool_dev), GFP_KERNEL);
>  	if (!cpufreq_dev)
>  		return ERR_PTR(-ENOMEM);
>  
> +	/* Find max levels */
> +	cpufreq_for_each_valid_entry(pos, table)
> +		cpufreq_dev->max_level++;
> +
> +	/* max_level is an index, not a counter */
> +	cpufreq_dev->max_level--;
> +
>  	cpumask_copy(&cpufreq_dev->allowed_cpus, clip_cpus);
>  
>  	cpufreq_dev->id = idr_alloc(&cpufreq_idr, NULL, 0, 0, GFP_KERNEL);
> -- 
> 2.0.3.693.g996b0fd
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 26/26] cpu_cooling: update copyright tags

[Viresh Kumar]

On 3 December 2014 at 01:11, Eduardo Valentin <edubezval@gmail.com> wrote:
> Hello Viresh K,
>
> On Fri, Nov 28, 2014 at 03:14:20PM +0530, Viresh Kumar wrote:
>> Adding my copyright information for two purposes:
>> - To get cc'd for future patches to review (Only if people read this header
>>   while sending mail)
>
> For this item, I get_maintainers might copy you due the amount of
> commit hits, no?
>
>> - Have done enough changes to earn a place here?
>>
>> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
>> ---
>> Drop this patch if you didn't like it Eduardo :)
>
> I am not against your patch, no.
>
> But, I am probably not the correct person to judge this though. If you get a
> confirmation from the original copyright holder (amit/samsung), then I
> am glad to add this.

@Amit ?? :)

>> ---
>>  drivers/thermal/cpu_cooling.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
>> index db4c001..4369963 100644
>> --- a/drivers/thermal/cpu_cooling.c
>> +++ b/drivers/thermal/cpu_cooling.c
>> @@ -4,6 +4,8 @@
>>   *  Copyright (C) 2012       Samsung Electronics Co., Ltd(http://www.samsung.com)
>>   *  Copyright (C) 2012  Amit Daniel <amit.kachhap@linaro.org>
>>   *
>> + *  Copyright (C) 2014  Viresh Kumar <viresh.kumar@linaro.org>
>> + *
>>   * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>   *  This program is free software; you can redistribute it and/or modify
>>   *  it under the terms of the GNU General Public License as published by
>> --
>> 2.0.3.693.g996b0fd
>>

Re: [PATCH 11/26] cpu_cooling: propagate error returned by idr_alloc()

[Viresh Kumar]

On 2 December 2014 at 21:05, Javi Merino <javi.merino@arm.com> wrote:
> The error is ret, not the id which is probably 0 if there was an
> error. So:
>
> +               return ERR_PTR(ret);

Where should I hide my face, shameful mistake :(

Re: [PATCH 08/26] cpu_cooling: Pass variable instead of its type to sizeof()

[Viresh Kumar]

On 3 December 2014 at 04:37, Eduardo Valentin <edubezval@gmail.com> wrote:
>> This should be:
>>
>> +     cpufreq_dev = kzalloc(sizeof(*cpufreq_dev), GFP_KERNEL);

This is insulting now. So stupid mistake..

Re: [PATCH 24/26] cpu_cooling: Store frequencies in descending order

[Viresh Kumar]

On 3 December 2014 at 04:51, Eduardo Valentin <edubezval@gmail.com> wrote:

>> +static unsigned int find_next_max(struct cpufreq_frequency_table *table,
>> +                               unsigned int prev_max)
>> +{
>> +     struct cpufreq_frequency_table *pos;
>> +     unsigned int max = 0;
>> +
>> +     cpufreq_for_each_valid_entry(pos, table) {
>> +             if (pos->frequency > max && pos->frequency < prev_max)
>
> What happens if, for some random reason, the cpufreq table is in
> ascending order and this function is called with prev_max == (unsigned
> int) -1 ? What would be the returned max?

The last frequency of the table, i.e. the max value. What bug did you catch,
that I am not able to see..

Re: [PATCH 19/26] cpu_cooling: find max level during device registration

[Viresh Kumar]

On 3 December 2014 at 05:09, Eduardo Valentin <edubezval@gmail.com> wrote:
> Could you please update me regarding the story behind the opp del patch
> set?
>
> http://permalink.gmane.org/gmane.linux.power-management.general/53348

Its merged by Rafael, should be in next now.

Re: [PATCH 25/26] cpu_cooling: Use cpufreq_dev->freq_table for finding level/freq

[Viresh Kumar]

On 3 December 2014 at 05:06, Eduardo Valentin <edubezval@gmail.com> wrote:
>> -     ret = get_property(cpufreq_device, state, &clip_freq, GET_FREQ);
>> -     if (ret)
>> -             return ret;
>> -
>> +     clip_freq = cpufreq_device->freq_table[state];
>
> There should be some check for valid state here..

What about this ??

diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
index 6bd95bc..6e76417 100644
--- a/drivers/thermal/cpu_cooling.c
+++ b/drivers/thermal/cpu_cooling.c
@@ -228,6 +228,10 @@ static int cpufreq_set_cur_state(struct
thermal_cooling_device *cdev,
        unsigned int cpu = cpumask_any(&cpufreq_device->allowed_cpus);
        unsigned int clip_freq;

+       /* Request state should be less than max_level */
+       if (WARN_ON(state > cpufreq_device->max_level))
+               return -EINVAL;
+
        /* Check if the old cooling action is same as new cooling action */
        if (cpufreq_device->cpufreq_state == state)
                return 0;

Re: [PATCH 16/26] cpu_cooling: Drop useless locking around idr_alloc/idr_remove

[Viresh Kumar]

On 3 December 2014 at 04:35, Eduardo Valentin <edubezval@gmail.com> wrote:
> On Fri, Nov 28, 2014 at 03:14:10PM +0530, Viresh Kumar wrote:
>> Locking around idr_alloc/idr_remove looks rather pointless as there is no race
>> it is trying to fix. Remove it.
>>
>
> Can you please elaborate on how the idr's are going to be serialize
> without the lock?

Dropped this patch, I was wrong ..

Re: [PATCH 25/26] cpu_cooling: Use cpufreq_dev->freq_table for finding level/freq

[Eduardo Valentin]

[-- Attachment #1: Type: text/plain, Size: 1174 bytes --]

On Wed, Dec 03, 2014 at 10:40:11AM +0530, Viresh Kumar wrote:
> On 3 December 2014 at 05:06, Eduardo Valentin <edubezval@gmail.com> wrote:
> >> -     ret = get_property(cpufreq_device, state, &clip_freq, GET_FREQ);
> >> -     if (ret)
> >> -             return ret;
> >> -
> >> +     clip_freq = cpufreq_device->freq_table[state];
> >
> > There should be some check for valid state here..
> 
> What about this ??

Sounds good to me.

> 
> diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
> index 6bd95bc..6e76417 100644
> --- a/drivers/thermal/cpu_cooling.c
> +++ b/drivers/thermal/cpu_cooling.c
> @@ -228,6 +228,10 @@ static int cpufreq_set_cur_state(struct
> thermal_cooling_device *cdev,
>         unsigned int cpu = cpumask_any(&cpufreq_device->allowed_cpus);
>         unsigned int clip_freq;
> 
> +       /* Request state should be less than max_level */
> +       if (WARN_ON(state > cpufreq_device->max_level))
> +               return -EINVAL;
> +
>         /* Check if the old cooling action is same as new cooling action */
>         if (cpufreq_device->cpufreq_state == state)
>                 return 0;

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 19/26] cpu_cooling: find max level during device registration

[Eduardo Valentin]

[-- Attachment #1: Type: text/plain, Size: 519 bytes --]

On Wed, Dec 03, 2014 at 10:27:43AM +0530, Viresh Kumar wrote:
> On 3 December 2014 at 05:09, Eduardo Valentin <edubezval@gmail.com> wrote:
> > Could you please update me regarding the story behind the opp del patch
> > set?
> >
> > http://permalink.gmane.org/gmane.linux.power-management.general/53348
> 
> Its merged by Rafael, should be in next now.

OK. Good. But does it mean we can remove OPPs after cpufreq
registration? Meaning, is the cpufreq table changing over time?


BR,

Eduardo Valentin


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 24/26] cpu_cooling: Store frequencies in descending order

[Eduardo Valentin]

[-- Attachment #1: Type: text/plain, Size: 1024 bytes --]

On Wed, Dec 03, 2014 at 10:22:30AM +0530, Viresh Kumar wrote:
> On 3 December 2014 at 04:51, Eduardo Valentin <edubezval@gmail.com> wrote:
> 
> >> +static unsigned int find_next_max(struct cpufreq_frequency_table *table,
> >> +                               unsigned int prev_max)
> >> +{
> >> +     struct cpufreq_frequency_table *pos;
> >> +     unsigned int max = 0;
> >> +
> >> +     cpufreq_for_each_valid_entry(pos, table) {
> >> +             if (pos->frequency > max && pos->frequency < prev_max)
> >
> > What happens if, for some random reason, the cpufreq table is in
> > ascending order and this function is called with prev_max == (unsigned
> > int) -1 ? What would be the returned max?
> 
> The last frequency of the table, i.e. the max value. What bug did you catch,
> that I am not able to see..

Well, passing -1 to unsigned int will make it max unsigned int, your
function will return the first freq, which in the scenario I gave, it
would be the min freq, not the first max, right?


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 24/26] cpu_cooling: Store frequencies in descending order

[Eduardo Valentin]

[-- Attachment #1: Type: text/plain, Size: 554 bytes --]

On Wed, Dec 03, 2014 at 07:26:17PM +0530, Viresh Kumar wrote:
> On Wednesday, December 3, 2014, Eduardo Valentin <edubezval@gmail.com>
> wrote:
> 
> >
> > Well, passing -1 to unsigned int will make it max unsigned int, your
> 
> 
> Yeah, that's what I want.
> 
> 
> > function will return the first freq, which in the scenario I gave, it
> > would be the min freq, not the first max, right?
> >
> 
> Are you sure you are reading the code correctly? Try one more
> time :)

Yeah, the code is correct. I need more coffee probably. :-)


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 26/26] cpu_cooling: update copyright tags

[amit daniel kachhap]

On Wed, Dec 3, 2014 at 10:04 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 3 December 2014 at 01:11, Eduardo Valentin <edubezval@gmail.com> wrote:
>> Hello Viresh K,
>>
>> On Fri, Nov 28, 2014 at 03:14:20PM +0530, Viresh Kumar wrote:
>>> Adding my copyright information for two purposes:
>>> - To get cc'd for future patches to review (Only if people read this header
>>>   while sending mail)
>>
>> For this item, I get_maintainers might copy you due the amount of
>> commit hits, no?
>>
>>> - Have done enough changes to earn a place here?
>>>
>>> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
>>> ---
>>> Drop this patch if you didn't like it Eduardo :)
>>
>> I am not against your patch, no.
>>
>> But, I am probably not the correct person to judge this though. If you get a
>> confirmation from the original copyright holder (amit/samsung), then I
>> am glad to add this.
>
> @Amit ?? :)

Yes its fine with me.

Regards,
Amit
>
>>> ---
>>>  drivers/thermal/cpu_cooling.c | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
>>> index db4c001..4369963 100644
>>> --- a/drivers/thermal/cpu_cooling.c
>>> +++ b/drivers/thermal/cpu_cooling.c
>>> @@ -4,6 +4,8 @@
>>>   *  Copyright (C) 2012       Samsung Electronics Co., Ltd(http://www.samsung.com)
>>>   *  Copyright (C) 2012  Amit Daniel <amit.kachhap@linaro.org>
>>>   *
>>> + *  Copyright (C) 2014  Viresh Kumar <viresh.kumar@linaro.org>
>>> + *
>>>   * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>   *  This program is free software; you can redistribute it and/or modify
>>>   *  it under the terms of the GNU General Public License as published by
>>> --
>>> 2.0.3.693.g996b0fd
>>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html