Re: [PATCH V6 16/30] thermal: exynos: Make the zone handling use trip information

2013-06-21 Thread Eduardo Valentin
On 21-06-2013 08:30, amit daniel kachhap wrote:
> Hi
> 
> On Thu, Jun 20, 2013 at 4:33 AM, Eduardo Valentin
>  wrote:
>> On 17-06-2013 02:46, Amit Daniel Kachhap wrote:
>>> This code simplifies the zone handling to use the trip information passed
>>> by the TMU driver and not the hardcoded macros. This also helps in adding
>>> more zone support.
>>>
>>> Acked-by: Kukjin Kim 
>>> Acked-by: Jonghwa Lee 
>>> Signed-off-by: Amit Daniel Kachhap 
>>> ---
>>>  drivers/thermal/samsung/exynos_thermal_common.c |   61 
>>> +--
>>>  drivers/thermal/samsung/exynos_thermal_common.h |3 +-
>>>  drivers/thermal/samsung/exynos_tmu.c|5 ++-
>>>  3 files changed, 40 insertions(+), 29 deletions(-)
>>>
>>> diff --git a/drivers/thermal/samsung/exynos_thermal_common.c 
>>> b/drivers/thermal/samsung/exynos_thermal_common.c
>>> index 86d39aa..2873ca3 100644
>>> --- a/drivers/thermal/samsung/exynos_thermal_common.c
>>> +++ b/drivers/thermal/samsung/exynos_thermal_common.c
>>> @@ -78,17 +78,22 @@ static int exynos_set_mode(struct thermal_zone_device 
>>> *thermal,
>>>  static int exynos_get_trip_type(struct thermal_zone_device *thermal, int 
>>> trip,
>>>enum thermal_trip_type *type)
>>>  {
>>> - switch (GET_ZONE(trip)) {
>>> - case MONITOR_ZONE:
>>> - case WARN_ZONE:
>>> - *type = THERMAL_TRIP_ACTIVE;
>>> - break;
>>> - case PANIC_ZONE:
>>> - *type = THERMAL_TRIP_CRITICAL;
>>> - break;
>>> - default:
>>> + struct exynos_thermal_zone *th_zone = thermal->devdata;
>>> + int max_trip = th_zone->sensor_conf->trip_data.trip_count;
>>> + int trip_type;
>>> +
>>> + if (trip < 0 || trip >= max_trip)
>>>   return -EINVAL;
>>> - }
>>> +
>>> + trip_type = th_zone->sensor_conf->trip_data.trip_type[trip];
>>> +
>>> + if (trip_type == SW_TRIP)
>>> + *type = THERMAL_TRIP_CRITICAL;
>>> + else if (trip_type == THROTTLE_ACTIVE)
>>> + *type = THERMAL_TRIP_ACTIVE;
>>> + else if (trip_type == THROTTLE_PASSIVE)
>>> + *type = THERMAL_TRIP_PASSIVE;
>>> +
>>>   return 0;
>>>  }
>>>
>>> @@ -97,8 +102,9 @@ static int exynos_get_trip_temp(struct 
>>> thermal_zone_device *thermal, int trip,
>>>   unsigned long *temp)
>>>  {
>>>   struct exynos_thermal_zone *th_zone = thermal->devdata;
>>> + int max_trip = th_zone->sensor_conf->trip_data.trip_count;
>>>
>>> - if (trip < GET_TRIP(MONITOR_ZONE) || trip > GET_TRIP(PANIC_ZONE))
>>> + if (trip < 0 || trip >= max_trip)
>>>   return -EINVAL;
>>>
>>>   *temp = th_zone->sensor_conf->trip_data.trip_val[trip];
>>> @@ -112,10 +118,10 @@ static int exynos_get_trip_temp(struct 
>>> thermal_zone_device *thermal, int trip,
>>>  static int exynos_get_crit_temp(struct thermal_zone_device *thermal,
>>>   unsigned long *temp)
>>>  {
>>> - int ret;
>>> - /* Panic zone */
>>> - ret = exynos_get_trip_temp(thermal, GET_TRIP(PANIC_ZONE), temp);
>>> - return ret;
>>> + struct exynos_thermal_zone *th_zone = thermal->devdata;
>>> + int max_trip = th_zone->sensor_conf->trip_data.trip_count;
>>> + /* Get the temp of highest trip*/
>>> + return exynos_get_trip_temp(thermal, max_trip - 1, temp);
>>>  }
>>>
>>>  /* Bind callback functions for thermal zone */
>>> @@ -340,19 +346,22 @@ int exynos_register_thermal(struct 
>>> thermal_sensor_conf *sensor_conf)
>>>   return -ENOMEM;
>>>
>>>   th_zone->sensor_conf = sensor_conf;
>>> - cpumask_set_cpu(0, _val);
>>> - th_zone->cool_dev[0] = cpufreq_cooling_register(_val);
>>> - if (IS_ERR(th_zone->cool_dev[0])) {
>>> - pr_err("Failed to register cpufreq cooling device\n");
>>> - ret = -EINVAL;
>>> - goto err_unregister;
>>> + if (sensor_conf->cooling_data.freq_clip_count > 0) {
>>> + cpumask_set_cpu(0, _val);
>>> + th_zone->cool_dev[0] = cpufreq_cooling_register(_val);
>>
>> Did you mean th_zone->cool_dev[th_zone->cool_dev_size] ?
> Yes.
>>
>> I think the logic behind the allocation of these cpufreq cooling devices
>> needs to be revisited. You always assigned to cool_dev[0], but you
>> always iterate the array until cool_dev_size. Remember that
>> cool_dev_size now is per th_zone, not global.
> I have kept an array of cooling devices to add other type of cooling
> devices in the same thermal zone in future. For current use case
> although it does not make sense and only one cpufreq cooling device is
> present

I would at least put a TODO mark. This code can easily evolve to a bug.
What it would be better is to have a proper flag/way to say how you want
to do the cooling on a specific TMU. Now you rely on the fact that you
either cool with cpufreq if freq table is present or you don't cool.
This is not scalable to what you are planing to do.

>>
>>> + if 

Re: [PATCH V6 16/30] thermal: exynos: Make the zone handling use trip information

2013-06-21 Thread amit daniel kachhap
Hi

On Thu, Jun 20, 2013 at 4:33 AM, Eduardo Valentin
 wrote:
> On 17-06-2013 02:46, Amit Daniel Kachhap wrote:
>> This code simplifies the zone handling to use the trip information passed
>> by the TMU driver and not the hardcoded macros. This also helps in adding
>> more zone support.
>>
>> Acked-by: Kukjin Kim 
>> Acked-by: Jonghwa Lee 
>> Signed-off-by: Amit Daniel Kachhap 
>> ---
>>  drivers/thermal/samsung/exynos_thermal_common.c |   61 
>> +--
>>  drivers/thermal/samsung/exynos_thermal_common.h |3 +-
>>  drivers/thermal/samsung/exynos_tmu.c|5 ++-
>>  3 files changed, 40 insertions(+), 29 deletions(-)
>>
>> diff --git a/drivers/thermal/samsung/exynos_thermal_common.c 
>> b/drivers/thermal/samsung/exynos_thermal_common.c
>> index 86d39aa..2873ca3 100644
>> --- a/drivers/thermal/samsung/exynos_thermal_common.c
>> +++ b/drivers/thermal/samsung/exynos_thermal_common.c
>> @@ -78,17 +78,22 @@ static int exynos_set_mode(struct thermal_zone_device 
>> *thermal,
>>  static int exynos_get_trip_type(struct thermal_zone_device *thermal, int 
>> trip,
>>enum thermal_trip_type *type)
>>  {
>> - switch (GET_ZONE(trip)) {
>> - case MONITOR_ZONE:
>> - case WARN_ZONE:
>> - *type = THERMAL_TRIP_ACTIVE;
>> - break;
>> - case PANIC_ZONE:
>> - *type = THERMAL_TRIP_CRITICAL;
>> - break;
>> - default:
>> + struct exynos_thermal_zone *th_zone = thermal->devdata;
>> + int max_trip = th_zone->sensor_conf->trip_data.trip_count;
>> + int trip_type;
>> +
>> + if (trip < 0 || trip >= max_trip)
>>   return -EINVAL;
>> - }
>> +
>> + trip_type = th_zone->sensor_conf->trip_data.trip_type[trip];
>> +
>> + if (trip_type == SW_TRIP)
>> + *type = THERMAL_TRIP_CRITICAL;
>> + else if (trip_type == THROTTLE_ACTIVE)
>> + *type = THERMAL_TRIP_ACTIVE;
>> + else if (trip_type == THROTTLE_PASSIVE)
>> + *type = THERMAL_TRIP_PASSIVE;
>> +
>>   return 0;
>>  }
>>
>> @@ -97,8 +102,9 @@ static int exynos_get_trip_temp(struct 
>> thermal_zone_device *thermal, int trip,
>>   unsigned long *temp)
>>  {
>>   struct exynos_thermal_zone *th_zone = thermal->devdata;
>> + int max_trip = th_zone->sensor_conf->trip_data.trip_count;
>>
>> - if (trip < GET_TRIP(MONITOR_ZONE) || trip > GET_TRIP(PANIC_ZONE))
>> + if (trip < 0 || trip >= max_trip)
>>   return -EINVAL;
>>
>>   *temp = th_zone->sensor_conf->trip_data.trip_val[trip];
>> @@ -112,10 +118,10 @@ static int exynos_get_trip_temp(struct 
>> thermal_zone_device *thermal, int trip,
>>  static int exynos_get_crit_temp(struct thermal_zone_device *thermal,
>>   unsigned long *temp)
>>  {
>> - int ret;
>> - /* Panic zone */
>> - ret = exynos_get_trip_temp(thermal, GET_TRIP(PANIC_ZONE), temp);
>> - return ret;
>> + struct exynos_thermal_zone *th_zone = thermal->devdata;
>> + int max_trip = th_zone->sensor_conf->trip_data.trip_count;
>> + /* Get the temp of highest trip*/
>> + return exynos_get_trip_temp(thermal, max_trip - 1, temp);
>>  }
>>
>>  /* Bind callback functions for thermal zone */
>> @@ -340,19 +346,22 @@ int exynos_register_thermal(struct thermal_sensor_conf 
>> *sensor_conf)
>>   return -ENOMEM;
>>
>>   th_zone->sensor_conf = sensor_conf;
>> - cpumask_set_cpu(0, _val);
>> - th_zone->cool_dev[0] = cpufreq_cooling_register(_val);
>> - if (IS_ERR(th_zone->cool_dev[0])) {
>> - pr_err("Failed to register cpufreq cooling device\n");
>> - ret = -EINVAL;
>> - goto err_unregister;
>> + if (sensor_conf->cooling_data.freq_clip_count > 0) {
>> + cpumask_set_cpu(0, _val);
>> + th_zone->cool_dev[0] = cpufreq_cooling_register(_val);
>
> Did you mean th_zone->cool_dev[th_zone->cool_dev_size] ?
Yes.
>
> I think the logic behind the allocation of these cpufreq cooling devices
> needs to be revisited. You always assigned to cool_dev[0], but you
> always iterate the array until cool_dev_size. Remember that
> cool_dev_size now is per th_zone, not global.
I have kept an array of cooling devices to add other type of cooling
devices in the same thermal zone in future. For current use case
although it does not make sense and only one cpufreq cooling device is
present
>
>> + if (IS_ERR(th_zone->cool_dev[0])) {
>> + pr_err("Failed to register cpufreq cooling device\n");
>> + ret = -EINVAL;
>> + goto err_unregister;
>> + }
>> + th_zone->cool_dev_size++;
>>   }
>> - th_zone->cool_dev_size++;
>>
>> - th_zone->therm_dev = thermal_zone_device_register(sensor_conf->name,
>> - EXYNOS_ZONE_COUNT, 0, th_zone, _dev_ops, NULL, 
>> 0,
>> - 

Re: [PATCH V6 16/30] thermal: exynos: Make the zone handling use trip information

2013-06-21 Thread amit daniel kachhap
Hi

On Thu, Jun 20, 2013 at 4:33 AM, Eduardo Valentin
eduardo.valen...@ti.com wrote:
 On 17-06-2013 02:46, Amit Daniel Kachhap wrote:
 This code simplifies the zone handling to use the trip information passed
 by the TMU driver and not the hardcoded macros. This also helps in adding
 more zone support.

 Acked-by: Kukjin Kim kgene@samsung.com
 Acked-by: Jonghwa Lee jonghwa3@samsung.com
 Signed-off-by: Amit Daniel Kachhap amit.dan...@samsung.com
 ---
  drivers/thermal/samsung/exynos_thermal_common.c |   61 
 +--
  drivers/thermal/samsung/exynos_thermal_common.h |3 +-
  drivers/thermal/samsung/exynos_tmu.c|5 ++-
  3 files changed, 40 insertions(+), 29 deletions(-)

 diff --git a/drivers/thermal/samsung/exynos_thermal_common.c 
 b/drivers/thermal/samsung/exynos_thermal_common.c
 index 86d39aa..2873ca3 100644
 --- a/drivers/thermal/samsung/exynos_thermal_common.c
 +++ b/drivers/thermal/samsung/exynos_thermal_common.c
 @@ -78,17 +78,22 @@ static int exynos_set_mode(struct thermal_zone_device 
 *thermal,
  static int exynos_get_trip_type(struct thermal_zone_device *thermal, int 
 trip,
enum thermal_trip_type *type)
  {
 - switch (GET_ZONE(trip)) {
 - case MONITOR_ZONE:
 - case WARN_ZONE:
 - *type = THERMAL_TRIP_ACTIVE;
 - break;
 - case PANIC_ZONE:
 - *type = THERMAL_TRIP_CRITICAL;
 - break;
 - default:
 + struct exynos_thermal_zone *th_zone = thermal-devdata;
 + int max_trip = th_zone-sensor_conf-trip_data.trip_count;
 + int trip_type;
 +
 + if (trip  0 || trip = max_trip)
   return -EINVAL;
 - }
 +
 + trip_type = th_zone-sensor_conf-trip_data.trip_type[trip];
 +
 + if (trip_type == SW_TRIP)
 + *type = THERMAL_TRIP_CRITICAL;
 + else if (trip_type == THROTTLE_ACTIVE)
 + *type = THERMAL_TRIP_ACTIVE;
 + else if (trip_type == THROTTLE_PASSIVE)
 + *type = THERMAL_TRIP_PASSIVE;
 +
   return 0;
  }

 @@ -97,8 +102,9 @@ static int exynos_get_trip_temp(struct 
 thermal_zone_device *thermal, int trip,
   unsigned long *temp)
  {
   struct exynos_thermal_zone *th_zone = thermal-devdata;
 + int max_trip = th_zone-sensor_conf-trip_data.trip_count;

 - if (trip  GET_TRIP(MONITOR_ZONE) || trip  GET_TRIP(PANIC_ZONE))
 + if (trip  0 || trip = max_trip)
   return -EINVAL;

   *temp = th_zone-sensor_conf-trip_data.trip_val[trip];
 @@ -112,10 +118,10 @@ static int exynos_get_trip_temp(struct 
 thermal_zone_device *thermal, int trip,
  static int exynos_get_crit_temp(struct thermal_zone_device *thermal,
   unsigned long *temp)
  {
 - int ret;
 - /* Panic zone */
 - ret = exynos_get_trip_temp(thermal, GET_TRIP(PANIC_ZONE), temp);
 - return ret;
 + struct exynos_thermal_zone *th_zone = thermal-devdata;
 + int max_trip = th_zone-sensor_conf-trip_data.trip_count;
 + /* Get the temp of highest trip*/
 + return exynos_get_trip_temp(thermal, max_trip - 1, temp);
  }

  /* Bind callback functions for thermal zone */
 @@ -340,19 +346,22 @@ int exynos_register_thermal(struct thermal_sensor_conf 
 *sensor_conf)
   return -ENOMEM;

   th_zone-sensor_conf = sensor_conf;
 - cpumask_set_cpu(0, mask_val);
 - th_zone-cool_dev[0] = cpufreq_cooling_register(mask_val);
 - if (IS_ERR(th_zone-cool_dev[0])) {
 - pr_err(Failed to register cpufreq cooling device\n);
 - ret = -EINVAL;
 - goto err_unregister;
 + if (sensor_conf-cooling_data.freq_clip_count  0) {
 + cpumask_set_cpu(0, mask_val);
 + th_zone-cool_dev[0] = cpufreq_cooling_register(mask_val);

 Did you mean th_zone-cool_dev[th_zone-cool_dev_size] ?
Yes.

 I think the logic behind the allocation of these cpufreq cooling devices
 needs to be revisited. You always assigned to cool_dev[0], but you
 always iterate the array until cool_dev_size. Remember that
 cool_dev_size now is per th_zone, not global.
I have kept an array of cooling devices to add other type of cooling
devices in the same thermal zone in future. For current use case
although it does not make sense and only one cpufreq cooling device is
present

 + if (IS_ERR(th_zone-cool_dev[0])) {
 + pr_err(Failed to register cpufreq cooling device\n);
 + ret = -EINVAL;
 + goto err_unregister;
 + }
 + th_zone-cool_dev_size++;
   }
 - th_zone-cool_dev_size++;

 - th_zone-therm_dev = thermal_zone_device_register(sensor_conf-name,
 - EXYNOS_ZONE_COUNT, 0, th_zone, exynos_dev_ops, NULL, 
 0,
 - sensor_conf-trip_data.trigger_falling ?
 - 0 : IDLE_INTERVAL);
 + th_zone-therm_dev = thermal_zone_device_register(
 + 

Re: [PATCH V6 16/30] thermal: exynos: Make the zone handling use trip information

2013-06-21 Thread Eduardo Valentin
On 21-06-2013 08:30, amit daniel kachhap wrote:
 Hi
 
 On Thu, Jun 20, 2013 at 4:33 AM, Eduardo Valentin
 eduardo.valen...@ti.com wrote:
 On 17-06-2013 02:46, Amit Daniel Kachhap wrote:
 This code simplifies the zone handling to use the trip information passed
 by the TMU driver and not the hardcoded macros. This also helps in adding
 more zone support.

 Acked-by: Kukjin Kim kgene@samsung.com
 Acked-by: Jonghwa Lee jonghwa3@samsung.com
 Signed-off-by: Amit Daniel Kachhap amit.dan...@samsung.com
 ---
  drivers/thermal/samsung/exynos_thermal_common.c |   61 
 +--
  drivers/thermal/samsung/exynos_thermal_common.h |3 +-
  drivers/thermal/samsung/exynos_tmu.c|5 ++-
  3 files changed, 40 insertions(+), 29 deletions(-)

 diff --git a/drivers/thermal/samsung/exynos_thermal_common.c 
 b/drivers/thermal/samsung/exynos_thermal_common.c
 index 86d39aa..2873ca3 100644
 --- a/drivers/thermal/samsung/exynos_thermal_common.c
 +++ b/drivers/thermal/samsung/exynos_thermal_common.c
 @@ -78,17 +78,22 @@ static int exynos_set_mode(struct thermal_zone_device 
 *thermal,
  static int exynos_get_trip_type(struct thermal_zone_device *thermal, int 
 trip,
enum thermal_trip_type *type)
  {
 - switch (GET_ZONE(trip)) {
 - case MONITOR_ZONE:
 - case WARN_ZONE:
 - *type = THERMAL_TRIP_ACTIVE;
 - break;
 - case PANIC_ZONE:
 - *type = THERMAL_TRIP_CRITICAL;
 - break;
 - default:
 + struct exynos_thermal_zone *th_zone = thermal-devdata;
 + int max_trip = th_zone-sensor_conf-trip_data.trip_count;
 + int trip_type;
 +
 + if (trip  0 || trip = max_trip)
   return -EINVAL;
 - }
 +
 + trip_type = th_zone-sensor_conf-trip_data.trip_type[trip];
 +
 + if (trip_type == SW_TRIP)
 + *type = THERMAL_TRIP_CRITICAL;
 + else if (trip_type == THROTTLE_ACTIVE)
 + *type = THERMAL_TRIP_ACTIVE;
 + else if (trip_type == THROTTLE_PASSIVE)
 + *type = THERMAL_TRIP_PASSIVE;
 +
   return 0;
  }

 @@ -97,8 +102,9 @@ static int exynos_get_trip_temp(struct 
 thermal_zone_device *thermal, int trip,
   unsigned long *temp)
  {
   struct exynos_thermal_zone *th_zone = thermal-devdata;
 + int max_trip = th_zone-sensor_conf-trip_data.trip_count;

 - if (trip  GET_TRIP(MONITOR_ZONE) || trip  GET_TRIP(PANIC_ZONE))
 + if (trip  0 || trip = max_trip)
   return -EINVAL;

   *temp = th_zone-sensor_conf-trip_data.trip_val[trip];
 @@ -112,10 +118,10 @@ static int exynos_get_trip_temp(struct 
 thermal_zone_device *thermal, int trip,
  static int exynos_get_crit_temp(struct thermal_zone_device *thermal,
   unsigned long *temp)
  {
 - int ret;
 - /* Panic zone */
 - ret = exynos_get_trip_temp(thermal, GET_TRIP(PANIC_ZONE), temp);
 - return ret;
 + struct exynos_thermal_zone *th_zone = thermal-devdata;
 + int max_trip = th_zone-sensor_conf-trip_data.trip_count;
 + /* Get the temp of highest trip*/
 + return exynos_get_trip_temp(thermal, max_trip - 1, temp);
  }

  /* Bind callback functions for thermal zone */
 @@ -340,19 +346,22 @@ int exynos_register_thermal(struct 
 thermal_sensor_conf *sensor_conf)
   return -ENOMEM;

   th_zone-sensor_conf = sensor_conf;
 - cpumask_set_cpu(0, mask_val);
 - th_zone-cool_dev[0] = cpufreq_cooling_register(mask_val);
 - if (IS_ERR(th_zone-cool_dev[0])) {
 - pr_err(Failed to register cpufreq cooling device\n);
 - ret = -EINVAL;
 - goto err_unregister;
 + if (sensor_conf-cooling_data.freq_clip_count  0) {
 + cpumask_set_cpu(0, mask_val);
 + th_zone-cool_dev[0] = cpufreq_cooling_register(mask_val);

 Did you mean th_zone-cool_dev[th_zone-cool_dev_size] ?
 Yes.

 I think the logic behind the allocation of these cpufreq cooling devices
 needs to be revisited. You always assigned to cool_dev[0], but you
 always iterate the array until cool_dev_size. Remember that
 cool_dev_size now is per th_zone, not global.
 I have kept an array of cooling devices to add other type of cooling
 devices in the same thermal zone in future. For current use case
 although it does not make sense and only one cpufreq cooling device is
 present

I would at least put a TODO mark. This code can easily evolve to a bug.
What it would be better is to have a proper flag/way to say how you want
to do the cooling on a specific TMU. Now you rely on the fact that you
either cool with cpufreq if freq table is present or you don't cool.
This is not scalable to what you are planing to do.


 + if (IS_ERR(th_zone-cool_dev[0])) {
 + pr_err(Failed to register cpufreq cooling device\n);
 + ret = -EINVAL;
 + goto err_unregister;
 + }
 + 

Re: [PATCH V6 16/30] thermal: exynos: Make the zone handling use trip information

2013-06-19 Thread Eduardo Valentin
On 17-06-2013 02:46, Amit Daniel Kachhap wrote:
> This code simplifies the zone handling to use the trip information passed
> by the TMU driver and not the hardcoded macros. This also helps in adding
> more zone support.
> 
> Acked-by: Kukjin Kim 
> Acked-by: Jonghwa Lee 
> Signed-off-by: Amit Daniel Kachhap 
> ---
>  drivers/thermal/samsung/exynos_thermal_common.c |   61 
> +--
>  drivers/thermal/samsung/exynos_thermal_common.h |3 +-
>  drivers/thermal/samsung/exynos_tmu.c|5 ++-
>  3 files changed, 40 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/thermal/samsung/exynos_thermal_common.c 
> b/drivers/thermal/samsung/exynos_thermal_common.c
> index 86d39aa..2873ca3 100644
> --- a/drivers/thermal/samsung/exynos_thermal_common.c
> +++ b/drivers/thermal/samsung/exynos_thermal_common.c
> @@ -78,17 +78,22 @@ static int exynos_set_mode(struct thermal_zone_device 
> *thermal,
>  static int exynos_get_trip_type(struct thermal_zone_device *thermal, int 
> trip,
>enum thermal_trip_type *type)
>  {
> - switch (GET_ZONE(trip)) {
> - case MONITOR_ZONE:
> - case WARN_ZONE:
> - *type = THERMAL_TRIP_ACTIVE;
> - break;
> - case PANIC_ZONE:
> - *type = THERMAL_TRIP_CRITICAL;
> - break;
> - default:
> + struct exynos_thermal_zone *th_zone = thermal->devdata;
> + int max_trip = th_zone->sensor_conf->trip_data.trip_count;
> + int trip_type;
> +
> + if (trip < 0 || trip >= max_trip)
>   return -EINVAL;
> - }
> +
> + trip_type = th_zone->sensor_conf->trip_data.trip_type[trip];
> +
> + if (trip_type == SW_TRIP)
> + *type = THERMAL_TRIP_CRITICAL;
> + else if (trip_type == THROTTLE_ACTIVE)
> + *type = THERMAL_TRIP_ACTIVE;
> + else if (trip_type == THROTTLE_PASSIVE)
> + *type = THERMAL_TRIP_PASSIVE;
> +
>   return 0;
>  }
>  
> @@ -97,8 +102,9 @@ static int exynos_get_trip_temp(struct thermal_zone_device 
> *thermal, int trip,
>   unsigned long *temp)
>  {
>   struct exynos_thermal_zone *th_zone = thermal->devdata;
> + int max_trip = th_zone->sensor_conf->trip_data.trip_count;
>  
> - if (trip < GET_TRIP(MONITOR_ZONE) || trip > GET_TRIP(PANIC_ZONE))
> + if (trip < 0 || trip >= max_trip)
>   return -EINVAL;
>  
>   *temp = th_zone->sensor_conf->trip_data.trip_val[trip];
> @@ -112,10 +118,10 @@ static int exynos_get_trip_temp(struct 
> thermal_zone_device *thermal, int trip,
>  static int exynos_get_crit_temp(struct thermal_zone_device *thermal,
>   unsigned long *temp)
>  {
> - int ret;
> - /* Panic zone */
> - ret = exynos_get_trip_temp(thermal, GET_TRIP(PANIC_ZONE), temp);
> - return ret;
> + struct exynos_thermal_zone *th_zone = thermal->devdata;
> + int max_trip = th_zone->sensor_conf->trip_data.trip_count;
> + /* Get the temp of highest trip*/
> + return exynos_get_trip_temp(thermal, max_trip - 1, temp);
>  }
>  
>  /* Bind callback functions for thermal zone */
> @@ -340,19 +346,22 @@ int exynos_register_thermal(struct thermal_sensor_conf 
> *sensor_conf)
>   return -ENOMEM;
>  
>   th_zone->sensor_conf = sensor_conf;
> - cpumask_set_cpu(0, _val);
> - th_zone->cool_dev[0] = cpufreq_cooling_register(_val);
> - if (IS_ERR(th_zone->cool_dev[0])) {
> - pr_err("Failed to register cpufreq cooling device\n");
> - ret = -EINVAL;
> - goto err_unregister;
> + if (sensor_conf->cooling_data.freq_clip_count > 0) {
> + cpumask_set_cpu(0, _val);
> + th_zone->cool_dev[0] = cpufreq_cooling_register(_val);

Did you mean th_zone->cool_dev[th_zone->cool_dev_size] ?

I think the logic behind the allocation of these cpufreq cooling devices
needs to be revisited. You always assigned to cool_dev[0], but you
always iterate the array until cool_dev_size. Remember that
cool_dev_size now is per th_zone, not global.

> + if (IS_ERR(th_zone->cool_dev[0])) {
> + pr_err("Failed to register cpufreq cooling device\n");
> + ret = -EINVAL;
> + goto err_unregister;
> + }
> + th_zone->cool_dev_size++;
>   }
> - th_zone->cool_dev_size++;
>  
> - th_zone->therm_dev = thermal_zone_device_register(sensor_conf->name,
> - EXYNOS_ZONE_COUNT, 0, th_zone, _dev_ops, NULL, 0,
> - sensor_conf->trip_data.trigger_falling ?
> - 0 : IDLE_INTERVAL);
> + th_zone->therm_dev = thermal_zone_device_register(
> + sensor_conf->name, sensor_conf->trip_data.trip_count,
> + 0, th_zone, _dev_ops, NULL, 0,
> + sensor_conf->trip_data.trigger_falling ? 0 :
> + IDLE_INTERVAL);
>  
>   if 

Re: [PATCH V6 16/30] thermal: exynos: Make the zone handling use trip information

2013-06-19 Thread Eduardo Valentin
On 17-06-2013 02:46, Amit Daniel Kachhap wrote:
 This code simplifies the zone handling to use the trip information passed
 by the TMU driver and not the hardcoded macros. This also helps in adding
 more zone support.
 
 Acked-by: Kukjin Kim kgene@samsung.com
 Acked-by: Jonghwa Lee jonghwa3@samsung.com
 Signed-off-by: Amit Daniel Kachhap amit.dan...@samsung.com
 ---
  drivers/thermal/samsung/exynos_thermal_common.c |   61 
 +--
  drivers/thermal/samsung/exynos_thermal_common.h |3 +-
  drivers/thermal/samsung/exynos_tmu.c|5 ++-
  3 files changed, 40 insertions(+), 29 deletions(-)
 
 diff --git a/drivers/thermal/samsung/exynos_thermal_common.c 
 b/drivers/thermal/samsung/exynos_thermal_common.c
 index 86d39aa..2873ca3 100644
 --- a/drivers/thermal/samsung/exynos_thermal_common.c
 +++ b/drivers/thermal/samsung/exynos_thermal_common.c
 @@ -78,17 +78,22 @@ static int exynos_set_mode(struct thermal_zone_device 
 *thermal,
  static int exynos_get_trip_type(struct thermal_zone_device *thermal, int 
 trip,
enum thermal_trip_type *type)
  {
 - switch (GET_ZONE(trip)) {
 - case MONITOR_ZONE:
 - case WARN_ZONE:
 - *type = THERMAL_TRIP_ACTIVE;
 - break;
 - case PANIC_ZONE:
 - *type = THERMAL_TRIP_CRITICAL;
 - break;
 - default:
 + struct exynos_thermal_zone *th_zone = thermal-devdata;
 + int max_trip = th_zone-sensor_conf-trip_data.trip_count;
 + int trip_type;
 +
 + if (trip  0 || trip = max_trip)
   return -EINVAL;
 - }
 +
 + trip_type = th_zone-sensor_conf-trip_data.trip_type[trip];
 +
 + if (trip_type == SW_TRIP)
 + *type = THERMAL_TRIP_CRITICAL;
 + else if (trip_type == THROTTLE_ACTIVE)
 + *type = THERMAL_TRIP_ACTIVE;
 + else if (trip_type == THROTTLE_PASSIVE)
 + *type = THERMAL_TRIP_PASSIVE;
 +
   return 0;
  }
  
 @@ -97,8 +102,9 @@ static int exynos_get_trip_temp(struct thermal_zone_device 
 *thermal, int trip,
   unsigned long *temp)
  {
   struct exynos_thermal_zone *th_zone = thermal-devdata;
 + int max_trip = th_zone-sensor_conf-trip_data.trip_count;
  
 - if (trip  GET_TRIP(MONITOR_ZONE) || trip  GET_TRIP(PANIC_ZONE))
 + if (trip  0 || trip = max_trip)
   return -EINVAL;
  
   *temp = th_zone-sensor_conf-trip_data.trip_val[trip];
 @@ -112,10 +118,10 @@ static int exynos_get_trip_temp(struct 
 thermal_zone_device *thermal, int trip,
  static int exynos_get_crit_temp(struct thermal_zone_device *thermal,
   unsigned long *temp)
  {
 - int ret;
 - /* Panic zone */
 - ret = exynos_get_trip_temp(thermal, GET_TRIP(PANIC_ZONE), temp);
 - return ret;
 + struct exynos_thermal_zone *th_zone = thermal-devdata;
 + int max_trip = th_zone-sensor_conf-trip_data.trip_count;
 + /* Get the temp of highest trip*/
 + return exynos_get_trip_temp(thermal, max_trip - 1, temp);
  }
  
  /* Bind callback functions for thermal zone */
 @@ -340,19 +346,22 @@ int exynos_register_thermal(struct thermal_sensor_conf 
 *sensor_conf)
   return -ENOMEM;
  
   th_zone-sensor_conf = sensor_conf;
 - cpumask_set_cpu(0, mask_val);
 - th_zone-cool_dev[0] = cpufreq_cooling_register(mask_val);
 - if (IS_ERR(th_zone-cool_dev[0])) {
 - pr_err(Failed to register cpufreq cooling device\n);
 - ret = -EINVAL;
 - goto err_unregister;
 + if (sensor_conf-cooling_data.freq_clip_count  0) {
 + cpumask_set_cpu(0, mask_val);
 + th_zone-cool_dev[0] = cpufreq_cooling_register(mask_val);

Did you mean th_zone-cool_dev[th_zone-cool_dev_size] ?

I think the logic behind the allocation of these cpufreq cooling devices
needs to be revisited. You always assigned to cool_dev[0], but you
always iterate the array until cool_dev_size. Remember that
cool_dev_size now is per th_zone, not global.

 + if (IS_ERR(th_zone-cool_dev[0])) {
 + pr_err(Failed to register cpufreq cooling device\n);
 + ret = -EINVAL;
 + goto err_unregister;
 + }
 + th_zone-cool_dev_size++;
   }
 - th_zone-cool_dev_size++;
  
 - th_zone-therm_dev = thermal_zone_device_register(sensor_conf-name,
 - EXYNOS_ZONE_COUNT, 0, th_zone, exynos_dev_ops, NULL, 0,
 - sensor_conf-trip_data.trigger_falling ?
 - 0 : IDLE_INTERVAL);
 + th_zone-therm_dev = thermal_zone_device_register(
 + sensor_conf-name, sensor_conf-trip_data.trip_count,
 + 0, th_zone, exynos_dev_ops, NULL, 0,
 + sensor_conf-trip_data.trigger_falling ? 0 :
 + IDLE_INTERVAL);
  
   if (IS_ERR(th_zone-therm_dev)) {
   pr_err(Failed to register 

[PATCH V6 16/30] thermal: exynos: Make the zone handling use trip information

2013-06-17 Thread Amit Daniel Kachhap
This code simplifies the zone handling to use the trip information passed
by the TMU driver and not the hardcoded macros. This also helps in adding
more zone support.

Acked-by: Kukjin Kim 
Acked-by: Jonghwa Lee 
Signed-off-by: Amit Daniel Kachhap 
---
 drivers/thermal/samsung/exynos_thermal_common.c |   61 +--
 drivers/thermal/samsung/exynos_thermal_common.h |3 +-
 drivers/thermal/samsung/exynos_tmu.c|5 ++-
 3 files changed, 40 insertions(+), 29 deletions(-)

diff --git a/drivers/thermal/samsung/exynos_thermal_common.c 
b/drivers/thermal/samsung/exynos_thermal_common.c
index 86d39aa..2873ca3 100644
--- a/drivers/thermal/samsung/exynos_thermal_common.c
+++ b/drivers/thermal/samsung/exynos_thermal_common.c
@@ -78,17 +78,22 @@ static int exynos_set_mode(struct thermal_zone_device 
*thermal,
 static int exynos_get_trip_type(struct thermal_zone_device *thermal, int trip,
 enum thermal_trip_type *type)
 {
-   switch (GET_ZONE(trip)) {
-   case MONITOR_ZONE:
-   case WARN_ZONE:
-   *type = THERMAL_TRIP_ACTIVE;
-   break;
-   case PANIC_ZONE:
-   *type = THERMAL_TRIP_CRITICAL;
-   break;
-   default:
+   struct exynos_thermal_zone *th_zone = thermal->devdata;
+   int max_trip = th_zone->sensor_conf->trip_data.trip_count;
+   int trip_type;
+
+   if (trip < 0 || trip >= max_trip)
return -EINVAL;
-   }
+
+   trip_type = th_zone->sensor_conf->trip_data.trip_type[trip];
+
+   if (trip_type == SW_TRIP)
+   *type = THERMAL_TRIP_CRITICAL;
+   else if (trip_type == THROTTLE_ACTIVE)
+   *type = THERMAL_TRIP_ACTIVE;
+   else if (trip_type == THROTTLE_PASSIVE)
+   *type = THERMAL_TRIP_PASSIVE;
+
return 0;
 }
 
@@ -97,8 +102,9 @@ static int exynos_get_trip_temp(struct thermal_zone_device 
*thermal, int trip,
unsigned long *temp)
 {
struct exynos_thermal_zone *th_zone = thermal->devdata;
+   int max_trip = th_zone->sensor_conf->trip_data.trip_count;
 
-   if (trip < GET_TRIP(MONITOR_ZONE) || trip > GET_TRIP(PANIC_ZONE))
+   if (trip < 0 || trip >= max_trip)
return -EINVAL;
 
*temp = th_zone->sensor_conf->trip_data.trip_val[trip];
@@ -112,10 +118,10 @@ static int exynos_get_trip_temp(struct 
thermal_zone_device *thermal, int trip,
 static int exynos_get_crit_temp(struct thermal_zone_device *thermal,
unsigned long *temp)
 {
-   int ret;
-   /* Panic zone */
-   ret = exynos_get_trip_temp(thermal, GET_TRIP(PANIC_ZONE), temp);
-   return ret;
+   struct exynos_thermal_zone *th_zone = thermal->devdata;
+   int max_trip = th_zone->sensor_conf->trip_data.trip_count;
+   /* Get the temp of highest trip*/
+   return exynos_get_trip_temp(thermal, max_trip - 1, temp);
 }
 
 /* Bind callback functions for thermal zone */
@@ -340,19 +346,22 @@ int exynos_register_thermal(struct thermal_sensor_conf 
*sensor_conf)
return -ENOMEM;
 
th_zone->sensor_conf = sensor_conf;
-   cpumask_set_cpu(0, _val);
-   th_zone->cool_dev[0] = cpufreq_cooling_register(_val);
-   if (IS_ERR(th_zone->cool_dev[0])) {
-   pr_err("Failed to register cpufreq cooling device\n");
-   ret = -EINVAL;
-   goto err_unregister;
+   if (sensor_conf->cooling_data.freq_clip_count > 0) {
+   cpumask_set_cpu(0, _val);
+   th_zone->cool_dev[0] = cpufreq_cooling_register(_val);
+   if (IS_ERR(th_zone->cool_dev[0])) {
+   pr_err("Failed to register cpufreq cooling device\n");
+   ret = -EINVAL;
+   goto err_unregister;
+   }
+   th_zone->cool_dev_size++;
}
-   th_zone->cool_dev_size++;
 
-   th_zone->therm_dev = thermal_zone_device_register(sensor_conf->name,
-   EXYNOS_ZONE_COUNT, 0, th_zone, _dev_ops, NULL, 0,
-   sensor_conf->trip_data.trigger_falling ?
-   0 : IDLE_INTERVAL);
+   th_zone->therm_dev = thermal_zone_device_register(
+   sensor_conf->name, sensor_conf->trip_data.trip_count,
+   0, th_zone, _dev_ops, NULL, 0,
+   sensor_conf->trip_data.trigger_falling ? 0 :
+   IDLE_INTERVAL);
 
if (IS_ERR(th_zone->therm_dev)) {
pr_err("Failed to register thermal zone device\n");
diff --git a/drivers/thermal/samsung/exynos_thermal_common.h 
b/drivers/thermal/samsung/exynos_thermal_common.h
index 1e9a326..dd0077e 100644
--- a/drivers/thermal/samsung/exynos_thermal_common.h
+++ b/drivers/thermal/samsung/exynos_thermal_common.h
@@ -42,8 +42,6 @@
 #define GET_ZONE(trip) (trip + 2)
 #define GET_TRIP(zone) (zone - 2)
 
-#define EXYNOS_ZONE_COUNT  

[PATCH V6 16/30] thermal: exynos: Make the zone handling use trip information

2013-06-17 Thread Amit Daniel Kachhap
This code simplifies the zone handling to use the trip information passed
by the TMU driver and not the hardcoded macros. This also helps in adding
more zone support.

Acked-by: Kukjin Kim kgene@samsung.com
Acked-by: Jonghwa Lee jonghwa3@samsung.com
Signed-off-by: Amit Daniel Kachhap amit.dan...@samsung.com
---
 drivers/thermal/samsung/exynos_thermal_common.c |   61 +--
 drivers/thermal/samsung/exynos_thermal_common.h |3 +-
 drivers/thermal/samsung/exynos_tmu.c|5 ++-
 3 files changed, 40 insertions(+), 29 deletions(-)

diff --git a/drivers/thermal/samsung/exynos_thermal_common.c 
b/drivers/thermal/samsung/exynos_thermal_common.c
index 86d39aa..2873ca3 100644
--- a/drivers/thermal/samsung/exynos_thermal_common.c
+++ b/drivers/thermal/samsung/exynos_thermal_common.c
@@ -78,17 +78,22 @@ static int exynos_set_mode(struct thermal_zone_device 
*thermal,
 static int exynos_get_trip_type(struct thermal_zone_device *thermal, int trip,
 enum thermal_trip_type *type)
 {
-   switch (GET_ZONE(trip)) {
-   case MONITOR_ZONE:
-   case WARN_ZONE:
-   *type = THERMAL_TRIP_ACTIVE;
-   break;
-   case PANIC_ZONE:
-   *type = THERMAL_TRIP_CRITICAL;
-   break;
-   default:
+   struct exynos_thermal_zone *th_zone = thermal-devdata;
+   int max_trip = th_zone-sensor_conf-trip_data.trip_count;
+   int trip_type;
+
+   if (trip  0 || trip = max_trip)
return -EINVAL;
-   }
+
+   trip_type = th_zone-sensor_conf-trip_data.trip_type[trip];
+
+   if (trip_type == SW_TRIP)
+   *type = THERMAL_TRIP_CRITICAL;
+   else if (trip_type == THROTTLE_ACTIVE)
+   *type = THERMAL_TRIP_ACTIVE;
+   else if (trip_type == THROTTLE_PASSIVE)
+   *type = THERMAL_TRIP_PASSIVE;
+
return 0;
 }
 
@@ -97,8 +102,9 @@ static int exynos_get_trip_temp(struct thermal_zone_device 
*thermal, int trip,
unsigned long *temp)
 {
struct exynos_thermal_zone *th_zone = thermal-devdata;
+   int max_trip = th_zone-sensor_conf-trip_data.trip_count;
 
-   if (trip  GET_TRIP(MONITOR_ZONE) || trip  GET_TRIP(PANIC_ZONE))
+   if (trip  0 || trip = max_trip)
return -EINVAL;
 
*temp = th_zone-sensor_conf-trip_data.trip_val[trip];
@@ -112,10 +118,10 @@ static int exynos_get_trip_temp(struct 
thermal_zone_device *thermal, int trip,
 static int exynos_get_crit_temp(struct thermal_zone_device *thermal,
unsigned long *temp)
 {
-   int ret;
-   /* Panic zone */
-   ret = exynos_get_trip_temp(thermal, GET_TRIP(PANIC_ZONE), temp);
-   return ret;
+   struct exynos_thermal_zone *th_zone = thermal-devdata;
+   int max_trip = th_zone-sensor_conf-trip_data.trip_count;
+   /* Get the temp of highest trip*/
+   return exynos_get_trip_temp(thermal, max_trip - 1, temp);
 }
 
 /* Bind callback functions for thermal zone */
@@ -340,19 +346,22 @@ int exynos_register_thermal(struct thermal_sensor_conf 
*sensor_conf)
return -ENOMEM;
 
th_zone-sensor_conf = sensor_conf;
-   cpumask_set_cpu(0, mask_val);
-   th_zone-cool_dev[0] = cpufreq_cooling_register(mask_val);
-   if (IS_ERR(th_zone-cool_dev[0])) {
-   pr_err(Failed to register cpufreq cooling device\n);
-   ret = -EINVAL;
-   goto err_unregister;
+   if (sensor_conf-cooling_data.freq_clip_count  0) {
+   cpumask_set_cpu(0, mask_val);
+   th_zone-cool_dev[0] = cpufreq_cooling_register(mask_val);
+   if (IS_ERR(th_zone-cool_dev[0])) {
+   pr_err(Failed to register cpufreq cooling device\n);
+   ret = -EINVAL;
+   goto err_unregister;
+   }
+   th_zone-cool_dev_size++;
}
-   th_zone-cool_dev_size++;
 
-   th_zone-therm_dev = thermal_zone_device_register(sensor_conf-name,
-   EXYNOS_ZONE_COUNT, 0, th_zone, exynos_dev_ops, NULL, 0,
-   sensor_conf-trip_data.trigger_falling ?
-   0 : IDLE_INTERVAL);
+   th_zone-therm_dev = thermal_zone_device_register(
+   sensor_conf-name, sensor_conf-trip_data.trip_count,
+   0, th_zone, exynos_dev_ops, NULL, 0,
+   sensor_conf-trip_data.trigger_falling ? 0 :
+   IDLE_INTERVAL);
 
if (IS_ERR(th_zone-therm_dev)) {
pr_err(Failed to register thermal zone device\n);
diff --git a/drivers/thermal/samsung/exynos_thermal_common.h 
b/drivers/thermal/samsung/exynos_thermal_common.h
index 1e9a326..dd0077e 100644
--- a/drivers/thermal/samsung/exynos_thermal_common.h
+++ b/drivers/thermal/samsung/exynos_thermal_common.h
@@ -42,8 +42,6 @@
 #define GET_ZONE(trip) (trip + 2)
 #define