On Sat, May 17, 2014 at 4:01 AM, Rafael J. Wysocki [off-list ref] wrote:

On Thursday, May 15, 2014 02:33:07 PM Eduardo Valentin wrote:

quoted

Hello Amit,


On Thu, May 08, 2014 at 08:07:56PM +0530, Amit Daniel Kachhap wrote:

quoted

This patch is in preparation to add notfication support for cpufrequency
cooling changes. This change also removes the unnecessary exposing of
internal housekeeping structure via thermal_cooling_device->devdata to the
users of cpufreq cooling APIs.

Signed-off-by: Amit Daniel Kachhap <redacted>
---
 drivers/thermal/cpu_cooling.c                      |   79 +++++++++++++++----
 drivers/thermal/db8500_cpufreq_cooling.c           |    2 +-
 drivers/thermal/samsung/exynos_thermal_common.c    |    2 +-
 drivers/thermal/ti-soc-thermal/ti-thermal-common.c |    2 +-
 include/linux/cpu_cooling.h                        |    5 +-
 5 files changed, 68 insertions(+), 22 deletions(-)

diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
index 4246262..21f44d4 100644
--- a/drivers/thermal/cpu_cooling.c
+++ b/drivers/thermal/cpu_cooling.c

@@ -50,16 +50,18 @@ struct cpufreq_cooling_device {
    unsigned int cpufreq_state;
    unsigned int cpufreq_val;
    struct cpumask allowed_cpus;
+   struct list_head node;
 };
 static DEFINE_IDR(cpufreq_idr);
 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;

+/* A list to hold all the cpufreq cooling devices registered */
+static LIST_HEAD(cpufreq_cooling_list);
+
 /**
  * get_idr - function to get a unique id.
  * @idr: struct idr * handle used to create a id.

@@ -357,12 +359,23 @@ static int cpufreq_thermal_notifier(struct notifier_block *nb,
 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;
+   struct cpufreq_cooling_device *cpufreq_device = NULL;
+   struct cpumask *mask = NULL;
    unsigned int cpu;
    unsigned int count = 0;
    int ret;

+   mutex_lock(&cooling_cpufreq_lock);
+   list_for_each_entry(cpufreq_device, &cpufreq_cooling_list, node)
+           if (cpufreq_device->cool_dev == cdev)
+                   break;
+   mutex_unlock(&cooling_cpufreq_lock);
+   if (!cpufreq_device) {
+           /* Cooling device pointer not found */
+           return -EINVAL;
+   }
+
+   mask = &cpufreq_device->allowed_cpus;
    cpu = cpumask_any(mask);

    ret = get_property(cpu, 0, &count, GET_MAXL);

@@ -386,7 +399,17 @@ static int cpufreq_get_max_state(struct thermal_cooling_device *cdev,
 static int cpufreq_get_cur_state(struct thermal_cooling_device *cdev,
                             unsigned long *state)
 {
-   struct cpufreq_cooling_device *cpufreq_device = cdev->devdata;
+   struct cpufreq_cooling_device *cpufreq_device = NULL;
+
+   mutex_lock(&cooling_cpufreq_lock);
+   list_for_each_entry(cpufreq_device, &cpufreq_cooling_list, node)
+           if (cpufreq_device->cool_dev == cdev)
+                   break;
+   mutex_unlock(&cooling_cpufreq_lock);
+   if (!cpufreq_device) {
+           /* Cooling device pointer not found */
+           return -EINVAL;
+   }

    *state = cpufreq_device->cpufreq_state;

@@ -406,7 +429,17 @@ static int cpufreq_get_cur_state(struct thermal_cooling_device *cdev,
 static int cpufreq_set_cur_state(struct thermal_cooling_device *cdev,
                             unsigned long state)
 {
-   struct cpufreq_cooling_device *cpufreq_device = cdev->devdata;
+   struct cpufreq_cooling_device *cpufreq_device = NULL;
+
+   mutex_lock(&cooling_cpufreq_lock);
+   list_for_each_entry(cpufreq_device, &cpufreq_cooling_list, node)
+           if (cpufreq_device->cool_dev == cdev)
+                   break;
+   mutex_unlock(&cooling_cpufreq_lock);
+   if (!cpufreq_device) {
+           /* Cooling device pointer not found */
+           return -EINVAL;
+   }

    return cpufreq_apply_cooling(cpufreq_device, state);
 }

@@ -427,6 +460,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.
+ * @devdata: driver data pointer
  *
  * This interface function registers the cpufreq cooling device with the name
  * "thermal-cpufreq-%x". This api can support multiple instances of cpufreq

@@ -438,7 +472,7 @@ static struct notifier_block thermal_cpufreq_notifier_block = {
  */
 static struct thermal_cooling_device *
 __cpufreq_cooling_register(struct device_node *np,
-                      const struct cpumask *clip_cpus)
+                      const struct cpumask *clip_cpus, void *devdata)
 {
    struct thermal_cooling_device *cool_dev;
    struct cpufreq_cooling_device *cpufreq_dev = NULL;

@@ -477,7 +511,7 @@ __cpufreq_cooling_register(struct device_node *np,
    snprintf(dev_name, sizeof(dev_name), "thermal-cpufreq-%d",
             cpufreq_dev->id);

-   cool_dev = thermal_of_cooling_device_register(np, dev_name, cpufreq_dev,
+   cool_dev = thermal_of_cooling_device_register(np, dev_name, devdata,
                                                  &cpufreq_cooling_ops);
    if (IS_ERR(cool_dev)) {
            release_idr(&cpufreq_idr, cpufreq_dev->id);

@@ -489,10 +523,11 @@ __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_cooling_list))
            cpufreq_register_notifier(&thermal_cpufreq_notifier_block,
                                      CPUFREQ_POLICY_NOTIFIER);
-   cpufreq_dev_count++;
+
+   list_add(&cpufreq_dev->node, &cpufreq_cooling_list);

    mutex_unlock(&cooling_cpufreq_lock);

@@ -502,6 +537,7 @@ __cpufreq_cooling_register(struct device_node *np,
 /**
  * cpufreq_cooling_register - function to create cpufreq cooling device.
  * @clip_cpus: cpumask of cpus where the frequency constraints will happen.
+ * @devdata: driver data pointer
  *
  * This interface function registers the cpufreq cooling device with the name
  * "thermal-cpufreq-%x". This api can support multiple instances of cpufreq

@@ -511,9 +547,9 @@ __cpufreq_cooling_register(struct device_node *np,
  * on failure, it returns a corresponding ERR_PTR().
  */
 struct thermal_cooling_device *
-cpufreq_cooling_register(const struct cpumask *clip_cpus)
+cpufreq_cooling_register(const struct cpumask *clip_cpus, void *devdata)
 {
-   return __cpufreq_cooling_register(NULL, clip_cpus);
+   return __cpufreq_cooling_register(NULL, clip_cpus, devdata);
 }
 EXPORT_SYMBOL_GPL(cpufreq_cooling_register);

@@ -537,7 +573,7 @@ of_cpufreq_cooling_register(struct device_node *np,
    if (!np)
            return ERR_PTR(-EINVAL);

-   return __cpufreq_cooling_register(np, clip_cpus);
+   return __cpufreq_cooling_register(np, clip_cpus, NULL);
 }
 EXPORT_SYMBOL_GPL(of_cpufreq_cooling_register);

@@ -554,17 +590,26 @@ void cpufreq_cooling_unregister(struct thermal_cooling_device *cdev)
    if (!cdev)
            return;

-   cpufreq_dev = cdev->devdata;
    mutex_lock(&cooling_cpufreq_lock);
-   cpufreq_dev_count--;
+
+   list_for_each_entry(cpufreq_dev, &cpufreq_cooling_list, node)
+           if (cpufreq_dev->cool_dev == cdev)
+                   break;
+
+   if (!cpufreq_dev) {
+           /* Cooling device pointer not found */
+           mutex_unlock(&cooling_cpufreq_lock);
+           return;
+   }

Because you are adding several times the same code to search through
your list of cdevs, I recommend adding at least a helper function to
perform this recurring operation.

Violently agreed.

ok will use wrapper function in next iteration.

--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html