Hi Amit,

-----Original Message----- From: amit kachhap [mailto:amitdanielk@gmail.com] On Behalf Of Amit Daniel Kachhap Sent: Wednesday, February 22, 2012 3:44 PM To: linux-pm@lists.linux-foundation.org Cc: linux-kernel@vger.kernel.org; mjg59@srcf.ucam.org; linux- acpi@vger.kernel.org; lenb@kernel.org; linaro-dev@lists.linaro.org; amit.kachhap@linaro.org; R, Durgadoss; rob.lee@linaro.org; patches@linaro.org Subject: [PATCH 1/4] thermal: Add a new trip type to use cooling device instance number

This patch adds a new trip type THERMAL_TRIP_STATE_ACTIVE. This trip behaves same as THERMAL_TRIP_ACTIVE but also passes the cooling device instance number. This helps the cooling device registered as different instances to perform appropriate cooling action decision in the set_cur_state call back function.

Also since the trip temperature's are in ascending order so some logic is put in place to skip the un-necessary checks.

Signed-off-by: Amit Daniel Kachhap amit.kachhap@linaro.org

Documentation/thermal/sysfs-api.txt | 4 +- drivers/thermal/thermal_sys.c | 45 ++++++++++++++++++++++++++++++++-- include/linux/thermal.h | 1 + 3 files changed, 45 insertions(+), 5 deletions(-)

diff --git a/Documentation/thermal/sysfs-api.txt b/Documentation/thermal/sysfs- api.txt index 1733ab9..7a0c080 100644 --- a/Documentation/thermal/sysfs-api.txt +++ b/Documentation/thermal/sysfs-api.txt @@ -184,8 +184,8 @@ trip_point_[0-*]_temp

trip_point_[0-*]_type Strings which indicate the type of the trip point.

• E.g. it can be one of critical, hot, passive, active[0-*] for ACPI
• thermal zone.

• E.g. it can be one of critical, hot, passive, active[0-1],
• state-active[0-*] for ACPI thermal zone. RO, Optional

cdev[0-*] diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c index 220ce7e..d4c9b20 100644 --- a/drivers/thermal/thermal_sys.c +++ b/drivers/thermal/thermal_sys.c @@ -192,6 +192,8 @@ trip_point_type_show(struct device *dev, struct device_attribute *attr, return sprintf(buf, "passive\n"); case THERMAL_TRIP_ACTIVE: return sprintf(buf, "active\n");

• case THERMAL_TRIP_STATE_ACTIVE:

• return sprintf(buf, "state-active\n");
  

  default: return sprintf(buf, "unknown\n"); }

@@ -1034,10 +1036,10 @@ EXPORT_SYMBOL(thermal_cooling_device_unregister);

void thermal_zone_device_update(struct thermal_zone_device *tz) {

• int count, ret = 0;
• long temp, trip_temp;

• int count, ret = 0, inst_id;
• long temp, trip_temp, max_state, last_trip_change = 0; enum thermal_trip_type trip_type;

• struct thermal_cooling_device_instance *instance;

• struct thermal_cooling_device_instance *instance, *state_instance; struct thermal_cooling_device *cdev;

  mutex_lock(&tz->lock);

@@ -1086,6 +1088,43 @@ void thermal_zone_device_update(struct thermal_zone_device *tz) cdev->ops->set_cur_state(cdev, 0); } break;

• case THERMAL_TRIP_STATE_ACTIVE:
  

• 	list_for_each_entry(instance, &tz->cooling_devices,
  

• 			    node) {
  

• 		if (instance->trip != count)
  

• 			continue;
  

• 		if (temp <= last_trip_change)
  

• 			continue;
  

• 		inst_id = 0;
  

• 		/*
  

• 		*For this instance how many instance of same
  

• 		*cooling device occured before
  

• 		*/
  

• 		list_for_each_entry(state_instance,
  

• 				&tz->cooling_devices, node) {
  

• 			if (instance->cdev ==
  

• 					state_instance->cdev)
  

• 				inst_id++;
  

• 			if (state_instance->trip == count)
  

• 				break;
  

• 		}
  

Can you explain a bit more on this loop and the set_cur_state below ? Sorry, I don't get the logic behind this..

Thanks, Durga

• 		cdev = instance->cdev;
  

• 		cdev->ops->get_max_state(cdev, &max_state);
  

• 		if ((temp >= trip_temp) &&
  

• 				(inst_id <= max_state))
  

• 			cdev->ops->set_cur_state(cdev, inst_id);
  

• 		else if ((temp < trip_temp) &&
  

• 				(--inst_id <= max_state))
  

• 			cdev->ops->set_cur_state(cdev, inst_id);
  

• 		last_trip_change = trip_temp;
  

• 	}
  

• 	break;
  

  case THERMAL_TRIP_PASSIVE: if (temp >= trip_temp || tz->passive) thermal_zone_device_passive(tz, temp,

diff --git a/include/linux/thermal.h b/include/linux/thermal.h index 796f1ff..8df901f 100644 --- a/include/linux/thermal.h +++ b/include/linux/thermal.h @@ -42,6 +42,7 @@ enum thermal_trip_type { THERMAL_TRIP_PASSIVE, THERMAL_TRIP_HOT, THERMAL_TRIP_CRITICAL,

• THERMAL_TRIP_STATE_ACTIVE,

};

struct thermal_zone_device_ops {

1.7.1

Hello,

On Thu, Feb 23, 2012 at 04:50:14PM +0530, Amit Kachhap wrote:

On 23 February 2012 12:16, R, Durgadoss durgadoss.r@intel.com wrote:

...

Hi Amit,

...

-----Original Message----- From: amit kachhap [mailto:amitdanielk@gmail.com] On Behalf Of Amit Daniel Kachhap Sent: Wednesday, February 22, 2012 3:44 PM To: linux-pm@lists.linux-foundation.org Cc: linux-kernel@vger.kernel.org; mjg59@srcf.ucam.org; linux- acpi@vger.kernel.org; lenb@kernel.org; linaro-dev@lists.linaro.org; amit.kachhap@linaro.org; R, Durgadoss; rob.lee@linaro.org; patches@linaro.org Subject: [PATCH 1/4] thermal: Add a new trip type to use cooling device instance number

This patch adds a new trip type THERMAL_TRIP_STATE_ACTIVE. This trip behaves same as THERMAL_TRIP_ACTIVE but also passes the cooling device instance number. This helps the cooling device registered as different instances to perform appropriate cooling action decision in the set_cur_state call back function.

Also since the trip temperature's are in ascending order so some logic is put in place to skip the un-necessary checks.

Signed-off-by: Amit Daniel Kachhap amit.kachhap@linaro.org

Documentation/thermal/sysfs-api.txt |    4 +-  drivers/thermal/thermal_sys.c       |   45 ++++++++++++++++++++++++++++++++--  include/linux/thermal.h             |    1 +  3 files changed, 45 insertions(+), 5 deletions(-)

diff --git a/Documentation/thermal/sysfs-api.txt b/Documentation/thermal/sysfs- api.txt index 1733ab9..7a0c080 100644 --- a/Documentation/thermal/sysfs-api.txt +++ b/Documentation/thermal/sysfs-api.txt @@ -184,8 +184,8 @@ trip_point_[0-*]_temp

trip_point_[0-*]_type       Strings which indicate the type of the trip point.

• E.g. it can be one of critical, hot, passive, active[0-*] for ACPI
• thermal zone.

• E.g. it can be one of critical, hot, passive, active[0-1],
• state-active[0-*] for ACPI thermal zone.

RO, Optional

cdev[0-*] diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c index 220ce7e..d4c9b20 100644 --- a/drivers/thermal/thermal_sys.c +++ b/drivers/thermal/thermal_sys.c @@ -192,6 +192,8 @@ trip_point_type_show(struct device *dev, struct device_attribute *attr,               return sprintf(buf, "passive\n");       case THERMAL_TRIP_ACTIVE:               return sprintf(buf, "active\n");

• case THERMAL_TRIP_STATE_ACTIVE:
• return sprintf(buf, "state-active\n");

default:               return sprintf(buf, "unknown\n");       } @@ -1034,10 +1036,10 @@ EXPORT_SYMBOL(thermal_cooling_device_unregister);

void thermal_zone_device_update(struct thermal_zone_device *tz)  {

• int count, ret = 0;
• long temp, trip_temp;

• int count, ret = 0, inst_id;
• long temp, trip_temp, max_state, last_trip_change = 0;

enum thermal_trip_type trip_type;

• struct thermal_cooling_device_instance *instance;

• struct thermal_cooling_device_instance *instance, *state_instance;

struct thermal_cooling_device *cdev;

mutex_lock(&tz->lock); @@ -1086,6 +1088,43 @@ void thermal_zone_device_update(struct thermal_zone_device *tz)                                       cdev->ops->set_cur_state(cdev, 0);                       }                       break;

• case THERMAL_TRIP_STATE_ACTIVE:
• list_for_each_entry(instance, &tz->cooling_devices,
• node) {
• if (instance->trip != count)
• continue;
• if (temp <= last_trip_change)
• continue;
• inst_id = 0;
• /*
• *For this instance how many instance of same
• *cooling device occured before
• */
• list_for_each_entry(state_instance,
• &tz->cooling_devices, node) {
• if (instance->cdev ==
• state_instance->cdev)
• inst_id++;
• if (state_instance->trip == count)
• break;
• }

Can you explain a bit more on this loop and the set_cur_state below ? Sorry, I don't get the logic behind this..

This loop is basically finding the instance id of the same cooling device. Say we have done like this, thermal_zone_bind_cooling_device(thermal, 2, cdev); thermal_zone_bind_cooling_device(thermal, 3, cdev); thermal_zone_bind_cooling_device(thermal, 4, cdev);

In above same cooling device cdev is binded to trip no 2,3 and 4 with inst_id generated as 1,2,3 respectively. so set_cur_state for those trip reached will be called as, set_cur_state(cdev, 1); set_cur_state(cdev, 2); set_cur_state(cdev, 3);

In this case, why a simple state = get_cur_state() followed by a set_cur_state(++state) / set_cur_state(--state) is not enough?

Another thing is if we want to do jumps in the sequence?

set_cur_state(cdev, 1); set_cur_state(cdev, 3); set_cur_state(cdev, 6);

But for that we need a table mapping, trip vs. state.

What do you think?

Thanks, Amit D

...

Thanks, Durga

...

• cdev = instance->cdev;
• cdev->ops->get_max_state(cdev, &max_state);
• if ((temp >= trip_temp) &&
• (inst_id <= max_state))
• cdev->ops->set_cur_state(cdev, inst_id);
• else if ((temp < trip_temp) &&
• (--inst_id <= max_state))
• cdev->ops->set_cur_state(cdev, inst_id);
• last_trip_change = trip_temp;
• }
• break;

case THERMAL_TRIP_PASSIVE:                       if (temp >= trip_temp || tz->passive)                               thermal_zone_device_passive(tz, temp, diff --git a/include/linux/thermal.h b/include/linux/thermal.h index 796f1ff..8df901f 100644 --- a/include/linux/thermal.h +++ b/include/linux/thermal.h @@ -42,6 +42,7 @@ enum thermal_trip_type {       THERMAL_TRIP_PASSIVE,       THERMAL_TRIP_HOT,       THERMAL_TRIP_CRITICAL,

• THERMAL_TRIP_STATE_ACTIVE,

};

struct thermal_zone_device_ops {

1.7.1

---

linux-pm mailing list linux-pm@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-pm

Hello,

On Wed, Apr 04, 2012 at 09:53:15AM +0530, Amit Kachhap wrote:

Hi Eduardo,

On 3 April 2012 19:45, Eduardo Valentin eduardo.valentin@ti.com wrote:

...

Hello,

On Thu, Feb 23, 2012 at 04:50:14PM +0530, Amit Kachhap wrote:

...

On 23 February 2012 12:16, R, Durgadoss durgadoss.r@intel.com wrote:

...

Hi Amit,

...

-----Original Message----- From: amit kachhap [mailto:amitdanielk@gmail.com] On Behalf Of Amit Daniel Kachhap Sent: Wednesday, February 22, 2012 3:44 PM To: linux-pm@lists.linux-foundation.org Cc: linux-kernel@vger.kernel.org; mjg59@srcf.ucam.org; linux- acpi@vger.kernel.org; lenb@kernel.org; linaro-dev@lists.linaro.org; amit.kachhap@linaro.org; R, Durgadoss; rob.lee@linaro.org; patches@linaro.org Subject: [PATCH 1/4] thermal: Add a new trip type to use cooling device instance number

This patch adds a new trip type THERMAL_TRIP_STATE_ACTIVE. This trip behaves same as THERMAL_TRIP_ACTIVE but also passes the cooling device instance number. This helps the cooling device registered as different instances to perform appropriate cooling action decision in the set_cur_state call back function.

Also since the trip temperature's are in ascending order so some logic is put in place to skip the un-necessary checks.

Signed-off-by: Amit Daniel Kachhap amit.kachhap@linaro.org

Documentation/thermal/sysfs-api.txt |    4 +-  drivers/thermal/thermal_sys.c       |   45 ++++++++++++++++++++++++++++++++--  include/linux/thermal.h             |    1 +  3 files changed, 45 insertions(+), 5 deletions(-)

diff --git a/Documentation/thermal/sysfs-api.txt b/Documentation/thermal/sysfs- api.txt index 1733ab9..7a0c080 100644 --- a/Documentation/thermal/sysfs-api.txt +++ b/Documentation/thermal/sysfs-api.txt @@ -184,8 +184,8 @@ trip_point_[0-*]_temp

trip_point_[0-*]_type       Strings which indicate the type of the trip point.

• E.g. it can be one of critical, hot, passive, active[0-*] for ACPI
• thermal zone.

• E.g. it can be one of critical, hot, passive, active[0-1],
• state-active[0-*] for ACPI thermal zone.

RO, Optional

cdev[0-*] diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c index 220ce7e..d4c9b20 100644 --- a/drivers/thermal/thermal_sys.c +++ b/drivers/thermal/thermal_sys.c @@ -192,6 +192,8 @@ trip_point_type_show(struct device *dev, struct device_attribute *attr,               return sprintf(buf, "passive\n");       case THERMAL_TRIP_ACTIVE:               return sprintf(buf, "active\n");

• case THERMAL_TRIP_STATE_ACTIVE:
• return sprintf(buf, "state-active\n");

default:               return sprintf(buf, "unknown\n");       } @@ -1034,10 +1036,10 @@ EXPORT_SYMBOL(thermal_cooling_device_unregister);

void thermal_zone_device_update(struct thermal_zone_device *tz)  {

• int count, ret = 0;
• long temp, trip_temp;

• int count, ret = 0, inst_id;
• long temp, trip_temp, max_state, last_trip_change = 0;

enum thermal_trip_type trip_type;

• struct thermal_cooling_device_instance *instance;

• struct thermal_cooling_device_instance *instance, *state_instance;

struct thermal_cooling_device *cdev;

mutex_lock(&tz->lock); @@ -1086,6 +1088,43 @@ void thermal_zone_device_update(struct thermal_zone_device *tz)                                       cdev->ops->set_cur_state(cdev, 0);                       }                       break;

• case THERMAL_TRIP_STATE_ACTIVE:
• list_for_each_entry(instance, &tz->cooling_devices,
• node) {
• if (instance->trip != count)
• continue;
• if (temp <= last_trip_change)
• continue;
• inst_id = 0;
• /*
• *For this instance how many instance of same
• *cooling device occured before
• */
• list_for_each_entry(state_instance,
• &tz->cooling_devices, node) {
• if (instance->cdev ==
• state_instance->cdev)
• inst_id++;
• if (state_instance->trip == count)
• break;
• }

Can you explain a bit more on this loop and the set_cur_state below ? Sorry, I don't get the logic behind this..

This loop is basically finding the instance id of the same cooling device. Say we have done like this, thermal_zone_bind_cooling_device(thermal, 2, cdev); thermal_zone_bind_cooling_device(thermal, 3, cdev); thermal_zone_bind_cooling_device(thermal, 4, cdev);

In above same cooling device cdev is binded to trip no 2,3 and 4 with inst_id generated as 1,2,3 respectively. so set_cur_state for those trip reached will be called as, set_cur_state(cdev, 1); set_cur_state(cdev, 2); set_cur_state(cdev, 3);

In this case, why a simple state = get_cur_state() followed by a set_cur_state(++state) / set_cur_state(--state) is not enough?

Thanks for looking into the patch. Well actually what you are suggesting is exactly happening in PASSIVE trip types where the states are incremented or decremented based on thermal trend. On the contrary what this part of code is doing is to jump to a fixed state as and when a trip point is reached. The cooling effect of a frequency level is known beforehand and hence jumping into that is safe and also this does not cause performance degradation by going into a much lower frequency state just for temperature stablization.

Yeah, I see that you want to match an instance of the cooling device with a cooling state for that cooling device.

...

Another thing is if we want to do jumps in the sequence?

set_cur_state(cdev, 1);  set_cur_state(cdev, 3);  set_cur_state(cdev, 6);

But for that we need a table mapping, trip vs. state.

What do you think?

In the current thermal_zone_device_update implementation all the checks are currently based on increase in temperature. So even though we want to go to set_cur_state(cdev, 6) we have to follow set_cur_state(cdev, 1), set_cur_state(cdev, 2) ,..... and finally set_cur_state(cdev, 6). So mapping table may not be needed as state transition is one by one. This a type of limitation but I think there can be some kind of modification in the way the thermal_zone_device_update calls all the lower temperature cooling devices instead of jumping directly to the final trip point cooling devices.

But, because you also force the thing to be increasing / decreasing 1 by 1, I don't understand why you don't have a similar implementation of the PASSIVE type. Just get its cur state and set the next state based on the curr. How that would be different than what you are currently doing?

If you want to have a 1 to 1 relation between cooling device instance and cooling device state, then you could do the jumps I mentioned, but only if you would have the cooling state reference stored somewhere.

What bugs me is the fact that you have to be iterating in the cooling device list to determine the next cooling state.

...

...

Thanks, Amit D

...

Thanks, Durga

...

• cdev = instance->cdev;
• cdev->ops->get_max_state(cdev, &max_state);
• if ((temp >= trip_temp) &&
• (inst_id <= max_state))
• cdev->ops->set_cur_state(cdev, inst_id);
• else if ((temp < trip_temp) &&
• (--inst_id <= max_state))
• cdev->ops->set_cur_state(cdev, inst_id);
• last_trip_change = trip_temp;
• }
• break;

case THERMAL_TRIP_PASSIVE:                       if (temp >= trip_temp || tz->passive)                               thermal_zone_device_passive(tz, temp, diff --git a/include/linux/thermal.h b/include/linux/thermal.h index 796f1ff..8df901f 100644 --- a/include/linux/thermal.h +++ b/include/linux/thermal.h @@ -42,6 +42,7 @@ enum thermal_trip_type {       THERMAL_TRIP_PASSIVE,       THERMAL_TRIP_HOT,       THERMAL_TRIP_CRITICAL,

• THERMAL_TRIP_STATE_ACTIVE,

};

struct thermal_zone_device_ops {

1.7.1

---

linux-pm mailing list linux-pm@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-pm

Hi Durgadoss,

Thanks for the detailed review comments.

On 24 February 2012 16:34, R, Durgadoss durgadoss.r@intel.com wrote:

Hi Amit,

...

-----Original Message----- From: amit kachhap [mailto:amitdanielk@gmail.com] On Behalf Of Amit Daniel Kachhap Sent: Wednesday, February 22, 2012 3:44 PM To: linux-pm@lists.linux-foundation.org Cc: linux-kernel@vger.kernel.org; mjg59@srcf.ucam.org; linux- acpi@vger.kernel.org; lenb@kernel.org; linaro-dev@lists.linaro.org; amit.kachhap@linaro.org; R, Durgadoss; rob.lee@linaro.org; patches@linaro.org Subject: [PATCH 2/4] thermal: Add generic cpufreq cooling implementation

This patch adds support for generic cpu thermal cooling low level implementations using frequency scaling up/down based on the request from user. Different cpu related cooling devices can be registered by the

I believe what you mean by 'user' is another Driver using this code.. right ??

Yes.

...

user and the binding of these cooling devices to the corresponding trip points can be easily done as the registration API's return the cooling device pointer. The user of these api's are responsible for passing clipping frequency in percentages.

Signed-off-by: Amit Daniel Kachhap amit.kachhap@linaro.org

Documentation/thermal/cpu-cooling-api.txt |   40 ++++  drivers/thermal/Kconfig                   |   11 +  drivers/thermal/Makefile                  |    1 +  drivers/thermal/cpu_cooling.c             |  310 +++++++++++++++++++++++++++++  include/linux/cpu_cooling.h               |   54 +++++  5 files changed, 416 insertions(+), 0 deletions(-)  create mode 100644 Documentation/thermal/cpu-cooling-api.txt  create mode 100644 drivers/thermal/cpu_cooling.c  create mode 100644 include/linux/cpu_cooling.h

diff --git a/Documentation/thermal/cpu-cooling-api.txt b/Documentation/thermal/cpu-cooling-api.txt new file mode 100644 index 0000000..04de67c --- /dev/null +++ b/Documentation/thermal/cpu-cooling-api.txt @@ -0,0 +1,40 @@ +CPU cooling api's How To +===================================

+Written by Amit Daniel Kachhap amit.kachhap@linaro.org

+Updated: 13 Dec 2011

+Copyright (c)  2011 Samsung Electronics Co., Ltd(http://www.samsung.com)

+0. Introduction

+The generic cpu cooling(freq clipping, cpuhotplug) provides +registration/unregistration api's to the user. The binding of the cooling +devices to the trip point is left for the user. The registration api's returns +the cooling device pointer.

+1. cpufreq cooling api's

+1.1 cpufreq registration api's +1.1.1 struct thermal_cooling_device *cpufreq_cooling_register(

• struct freq_pctg_table *tab_ptr, unsigned int tab_size,
• const struct cpumask *mask_val)
• This interface function registers the cpufreq cooling device with the name
• "thermal-cpufreq-%x". This api can support multiple instance of cpufreq

cooling

• devices.
• tab_ptr: The table containing the percentage of frequency to be clipped

for

• each cooling state.
• .freq_clip_pctg: Percentage of frequency to be clipped for each allowed
• cpus.
• .polling_interval: polling interval for this cooling state.
• tab_size: the total number of cooling state.

Although I can understand that the table size is equal to the total number of cooling states, it is better to name it 'num_cooling_states' (or something) that means what it is..

Yes your idea makes more sense. Will accept it in the next version.

...

• mask_val: all the allowed cpu's where frequency clipping can happen.

+1.1.2 void cpufreq_cooling_unregister(struct thermal_cooling_device *cdev)

• This interface function unregisters the "thermal-cpufreq-%x" cooling

device.

• cdev: Cooling device pointer which has to be unregistered.

diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig index f7f71b2..298c1cd 100644 --- a/drivers/thermal/Kconfig +++ b/drivers/thermal/Kconfig @@ -18,3 +18,14 @@ config THERMAL_HWMON       depends on THERMAL       depends on HWMON=y || HWMON=THERMAL       default y

+config CPU_THERMAL

• bool "generic cpu cooling support"
• depends on THERMAL
• help
• This implements the generic cpu cooling mechanism through frequency
• reduction, cpu hotplug and any other ways of reducing temperature. An
• ACPI version of this already exists(drivers/acpi/processor_thermal.c).
• This will be useful for platforms using the generic thermal interface
• and not the ACPI interface.
• If you want this support, you should say Y or M here.

diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile index 31108a0..655cbc4 100644 --- a/drivers/thermal/Makefile +++ b/drivers/thermal/Makefile @@ -3,3 +3,4 @@  #

obj-$(CONFIG_THERMAL)                += thermal_sys.o +obj-$(CONFIG_CPU_THERMAL)    += cpu_cooling.o diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c new file mode 100644 index 0000000..298f550 --- /dev/null +++ b/drivers/thermal/cpu_cooling.c @@ -0,0 +1,310 @@ +/*

• • linux/drivers/thermal/cpu_cooling.c
• • Copyright (C) 2011       Samsung Electronics Co., Ltd(http://www.samsung.com)
• • Copyright (C) 2011  Amit Daniel amit.kachhap@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
• • the Free Software Foundation; version 2 of the License.
• • This program is distributed in the hope that it will be useful, but
• • WITHOUT ANY WARRANTY; without even the implied warranty of
• • MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
• • General Public License for more details.
• • You should have received a copy of the GNU General Public License along
• • with this program; if not, write to the Free Software Foundation, Inc.,
• • 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA.
• */

+#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/thermal.h> +#include <linux/platform_device.h> +#include <linux/cpufreq.h> +#include <linux/err.h> +#include <linux/slab.h> +#include <linux/cpu.h> +#include <linux/cpu_cooling.h>

+#ifdef CONFIG_CPU_FREQ

Except the _idr methods, all the code is inside this #ifdef. So, I think it is better to add this dependency in Kconfig, and leave this code clean w/o many #ifdef's.

ok

...

+struct cpufreq_cooling_device {

• int id;
• struct thermal_cooling_device *cool_dev;
• struct freq_pctg_table *tab_ptr;
• unsigned int tab_size;
• unsigned int cpufreq_state;
• const struct cpumask *allowed_cpus;
• struct list_head node;

+};

+static LIST_HEAD(cooling_cpufreq_list); +static DEFINE_MUTEX(cooling_cpufreq_lock); +static DEFINE_IDR(cpufreq_idr); +static struct cpufreq_cooling_device *notify_cpufreq;

Please move this after the DEFINE_PER_CPU. Hard to notice here..

ok

...

+static DEFINE_PER_CPU(unsigned int, max_policy_freq); +#endif /*CONFIG_CPU_FREQ*/

+static int get_idr(struct idr *idr, struct mutex *lock, int *id) +{

• int err;

+again:

• if (unlikely(idr_pre_get(idr, GFP_KERNEL) == 0))
• return -ENOMEM;
• if (lock)
• mutex_lock(lock);
• err = idr_get_new(idr, NULL, id);
• if (lock)
• mutex_unlock(lock);
• if (unlikely(err == -EAGAIN))
• goto again;
• else if (unlikely(err))
• return err;
• *id = *id & MAX_ID_MASK;
• return 0;

+}

+static void release_idr(struct idr *idr, struct mutex *lock, int id) +{

• if (lock)
• mutex_lock(lock);
• idr_remove(idr, id);
• if (lock)
• mutex_unlock(lock);

+}

+#ifdef CONFIG_CPU_FREQ +/*Below codes defines functions to be used for cpufreq as cooling device*/ +static bool is_cpufreq_valid(int cpu) +{

• struct cpufreq_policy policy;
• if (!cpufreq_get_policy(&policy, cpu))
• return true;
• return false;

Why not just do a return !cpufreq_get_policy(&policy, cpu);

ok

...

+}

+static int cpufreq_apply_cooling(struct cpufreq_cooling_device *cpufreq_device,

• unsigned long cooling_state)

+{

• int cpuid, this_cpu = smp_processor_id();
• if (!is_cpufreq_valid(this_cpu))

You are not using this_cpu anywhere else..so, directly use Smp_processor_id() here..

Ok agreed.

...

• return 0;
• if (cooling_state > cpufreq_device->tab_size)
• return -EINVAL;
• /*Check if last cooling level is same as current cooling level*/

Use either 'state' or 'level' in comments as well as the variable name Makes it easy to read..

Ok, will use state.

...

• if (cpufreq_device->cpufreq_state == cooling_state)
• return 0;
• cpufreq_device->cpufreq_state = cooling_state;
• /*cpufreq thermal notifier uses this cpufreq device pointer*/
• notify_cpufreq = cpufreq_device;
• for_each_cpu(cpuid, cpufreq_device->allowed_cpus) {
• if (is_cpufreq_valid(cpuid))
• cpufreq_update_policy(cpuid);
• }
• return 0;

+}

+static int thermal_cpufreq_notifier(struct notifier_block *nb,

• unsigned long event, void *data)

+{

• struct cpufreq_policy *policy = data;
• struct freq_pctg_table *th_table;
• unsigned long max_freq = 0;
• unsigned int th_pctg = 0, level;
• if (event != CPUFREQ_ADJUST)
• return 0;
• if (!notify_cpufreq)
• return 0;

Why not combine both if's with an || ?

Ok

...

• level = notify_cpufreq->cpufreq_state;

Yes..here it is..please use level/state..

Ok, will use state

...

• if (level > 0) {
• th_table =
• &(notify_cpufreq->tab_ptr[level - 1]);
• th_pctg = th_table->freq_clip_pctg;
• max_freq =
• (policy->cpuinfo.max_freq * (100 - th_pctg)) / 100;
• if (per_cpu(max_policy_freq, policy->cpu) == 0)
• per_cpu(max_policy_freq, policy->cpu) = policy->max;
• } else {
• if (per_cpu(max_policy_freq, policy->cpu) != 0) {
• max_freq = per_cpu(max_policy_freq, policy->cpu);
• per_cpu(max_policy_freq, policy->cpu) = 0;
• } else {
• max_freq = policy->max;
• }
• }
• cpufreq_verify_within_limits(policy, 0, max_freq);
• return 0;

+}

+/*

• • cpufreq cooling device callback functions
• */

+static int cpufreq_get_max_state(struct thermal_cooling_device *cdev,

• unsigned long *state)

+{

• struct cpufreq_cooling_device *cpufreq_device = NULL;

Why assigning NULL ?

yes it is not needed.

...

• mutex_lock(&cooling_cpufreq_lock);
• list_for_each_entry(cpufreq_device, &cooling_cpufreq_list, node)
• if (cpufreq_device && cpufreq_device->cool_dev == cdev)
• break;
• mutex_unlock(&cooling_cpufreq_lock);
• if (!cpufreq_device || cpufreq_device->cool_dev != cdev)
• return -EINVAL;
• *state = cpufreq_device->tab_size;
• return 0;

+}

The above can be simplified this way:        int ret = -EINVAL;        mutex_lock(&cooling_cpufreq_lock);        list_for_each_entry(cpufreq_device, &cooling_cpufreq_list, node)                if (cpufreq_device->cool_dev == cdev) {                        *state = cpufreq_device->tab_size;                        ret = 0;                        break;                }

mutex_unlock(&cooling_cpufreq_lock);        return ret;

I think the same can be done for the get_ function below..and similar ones in the patch 3/4.

Ok accepted.

...

+static int cpufreq_get_cur_state(struct thermal_cooling_device *cdev,

• unsigned long *state)

+{

• struct cpufreq_cooling_device *cpufreq_device = NULL;
• mutex_lock(&cooling_cpufreq_lock);
• list_for_each_entry(cpufreq_device, &cooling_cpufreq_list, node)
• if (cpufreq_device && cpufreq_device->cool_dev == cdev)
• break;
• mutex_unlock(&cooling_cpufreq_lock);
• if (!cpufreq_device || cpufreq_device->cool_dev != cdev)
• return -EINVAL;
• *state = cpufreq_device->cpufreq_state;
• return 0;

+}

+/*This cooling may be as PASSIVE/ACTIVE type*/ +static int cpufreq_set_cur_state(struct thermal_cooling_device *cdev,

• unsigned long state)

+{

• struct cpufreq_cooling_device *cpufreq_device = NULL;
• mutex_lock(&cooling_cpufreq_lock);
• list_for_each_entry(cpufreq_device, &cooling_cpufreq_list, node)
• if (cpufreq_device && cpufreq_device->cool_dev == cdev)
• break;
• mutex_unlock(&cooling_cpufreq_lock);
• if (!cpufreq_device || cpufreq_device->cool_dev != cdev)
• return -EINVAL;
• cpufreq_apply_cooling(cpufreq_device, state);
• return 0;

+}

+/* bind cpufreq callbacks to cpufreq cooling device */ +static struct thermal_cooling_device_ops cpufreq_cooling_ops = {

• .get_max_state = cpufreq_get_max_state,
• .get_cur_state = cpufreq_get_cur_state,
• .set_cur_state = cpufreq_set_cur_state,

+};

+static struct notifier_block thermal_cpufreq_notifier_block = {

• .notifier_call = thermal_cpufreq_notifier,

+};

+struct thermal_cooling_device *cpufreq_cooling_register(

• struct freq_pctg_table *tab_ptr, unsigned int tab_size,
• const struct cpumask *mask_val)

+{

• struct thermal_cooling_device *cool_dev;
• struct cpufreq_cooling_device *cpufreq_dev = NULL;
• unsigned int count = 0;
• char dev_name[THERMAL_NAME_LENGTH];
• int ret = 0, id = 0;
• if (tab_ptr == NULL || tab_size == 0)
• return ERR_PTR(-EINVAL);
• list_for_each_entry(cpufreq_dev, &cooling_cpufreq_list, node)
• count++;
• cpufreq_dev =
• kzalloc(sizeof(struct cpufreq_cooling_device), GFP_KERNEL);
• if (!cpufreq_dev)
• return ERR_PTR(-ENOMEM);
• cpufreq_dev->tab_ptr = tab_ptr;
• cpufreq_dev->tab_size = tab_size;
• cpufreq_dev->allowed_cpus = mask_val;
• ret = get_idr(&cpufreq_idr, &cooling_cpufreq_lock, &cpufreq_dev->id);
• if (ret) {
• kfree(cpufreq_dev);
• return ERR_PTR(-EINVAL);
• }
• sprintf(dev_name, "thermal-cpufreq-%d", cpufreq_dev->id);
• cool_dev = thermal_cooling_device_register(dev_name, cpufreq_dev,
• &cpufreq_cooling_ops);
• if (!cool_dev) {
• release_idr(&cpufreq_idr, &cooling_cpufreq_lock,
• cpufreq_dev->id);
• kfree(cpufreq_dev);
• return ERR_PTR(-EINVAL);
• }
• cpufreq_dev->id = id;
• cpufreq_dev->cool_dev = cool_dev;
• mutex_lock(&cooling_cpufreq_lock);
• list_add_tail(&cpufreq_dev->node, &cooling_cpufreq_list);
• mutex_unlock(&cooling_cpufreq_lock);
• if (count == 0)
• cpufreq_register_notifier(&thermal_cpufreq_notifier_block,
• CPUFREQ_POLICY_NOTIFIER);

Why do we register only when count is 0 ? Or should this be 'if (count > 0)' ?

Count represents the number of cpufreq type cooling devices. So for the first call of this API , cpufreq_register_notifier is called. Other call to cpufreq_cooling_register will use the same notifier. May be using bool flag instead of count variable is better.

...

• return cool_dev;

+} +EXPORT_SYMBOL(cpufreq_cooling_register);

+void cpufreq_cooling_unregister(struct thermal_cooling_device *cdev) +{

• struct cpufreq_cooling_device *cpufreq_dev = NULL;
• unsigned int count = 0;
• mutex_lock(&cooling_cpufreq_lock);
• list_for_each_entry(cpufreq_dev, &cooling_cpufreq_list, node) {
• if (cpufreq_dev && cpufreq_dev->cool_dev == cdev)
• break;
• count++;
• }
• if (!cpufreq_dev || cpufreq_dev->cool_dev != cdev) {
• mutex_unlock(&cooling_cpufreq_lock);
• return;
• }
• list_del(&cpufreq_dev->node);
• mutex_unlock(&cooling_cpufreq_lock);
• if (count == 1)

Same here..I do not get the idea behind this.. Shouldn't this be 'if (count > 0)' ?

Same as above.

In general, I would like to see a real driver using these API's. This will help everybody to understand the working of these API's much better.

Actually RFC version of the driver is already posted which uses these api's. (https://lkml.org/lkml/2011/12/21/169). I forgot to add this link in this patchset. I will post the new version of the driver shortly.

Thanks, Durga

...

• cpufreq_unregister_notifier(&thermal_cpufreq_notifier_block,
• CPUFREQ_POLICY_NOTIFIER);
• thermal_cooling_device_unregister(cpufreq_dev->cool_dev);
• release_idr(&cpufreq_idr, &cooling_cpufreq_lock, cpufreq_dev->id);
• kfree(cpufreq_dev);

+} +EXPORT_SYMBOL(cpufreq_cooling_unregister); +#endif /*CONFIG_CPU_FREQ*/ diff --git a/include/linux/cpu_cooling.h b/include/linux/cpu_cooling.h new file mode 100644 index 0000000..5dc5632 --- /dev/null +++ b/include/linux/cpu_cooling.h @@ -0,0 +1,54 @@ +/*

• • linux/include/linux/cpu_cooling.h
• • Copyright (C) 2011       Samsung Electronics Co., Ltd(http://www.samsung.com)
• • Copyright (C) 2011  Amit Daniel amit.kachhap@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
• • the Free Software Foundation; version 2 of the License.
• • This program is distributed in the hope that it will be useful, but
• • WITHOUT ANY WARRANTY; without even the implied warranty of
• • MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
• • General Public License for more details.
• • You should have received a copy of the GNU General Public License along
• • with this program; if not, write to the Free Software Foundation, Inc.,
• • 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA.
• */

+#ifndef __CPU_COOLING_H__ +#define __CPU_COOLING_H__

+#include <linux/thermal.h>

+struct freq_pctg_table {

• unsigned int freq_clip_pctg;
• unsigned int polling_interval;

+};

+#ifdef CONFIG_CPU_FREQ +struct thermal_cooling_device *cpufreq_cooling_register(

• struct freq_pctg_table *tab_ptr, unsigned int tab_size,
• const struct cpumask *mask_val);

+void cpufreq_cooling_unregister(struct thermal_cooling_device *cdev); +#else /*!CONFIG_CPU_FREQ*/ +static inline struct thermal_cooling_device *cpufreq_cooling_register(

• struct freq_pctg_table *tab_ptr, unsigned int tab_size,
• const struct cpumask *mask_val)

+{

• return NULL;

+} +static inline void cpufreq_cooling_unregister(

• struct thermal_cooling_device *cdev)

+{

• return;

+} +#endif       /*CONFIG_CPU_FREQ*/

+#endif /* __CPU_COOLING_H__ */

1.7.1