[RFC PATCH 0/6] Thermal: step_wise governor fixes

[RFC PATCH 0/6] Thermal: step_wise governor fixes

[Zhang Rui]

Hi, all,

There are a lot of step_wise governor problems reported recently,
and plus, there are already several patches avaible, but I'd like to do
a cleanup for this piece of code altogether.

This patch set fixes several problem as addressed in the description
of each patch.
Please kindly review.

Note: I just run build test. It would be great that some of you
can test it.

thanks,
rui

[RFC PATCH 1/6] thermal: step_wise: cdev only needs update on a new target state

[Zhang Rui]

From: Shawn Guo <shawn.guo@linaro.org>

The cooling device only needs update on a new target state.  Since we
already check old target in thermal_zone_trip_update(), we can do one
more check to see if it's a new target state.  If not, we can reasonably
save some uncecesary code execution.

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
Acked-by: Eduardo Valentin <eduardo.valentin@ti.com>
Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/thermal/step_wise.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/thermal/step_wise.c b/drivers/thermal/step_wise.c
index 4d4ddae..0afbd86 100644
--- a/drivers/thermal/step_wise.c
+++ b/drivers/thermal/step_wise.c
@@ -133,6 +133,9 @@ static void thermal_zone_trip_update(struct thermal_zone_device *tz, int trip)
 		old_target = instance->target;
 		instance->target = get_target_state(instance, trend, throttle);
 
+		if (old_target == instance->target)
+			continue;
+
 		/* Activate a passive thermal instance */
 		if (old_target == THERMAL_NO_TARGET &&
 			instance->target != THERMAL_NO_TARGET)
-- 
1.7.9.5

[RFC PATCH 2/6] thermal: step_wise: return instance->target by default

[Zhang Rui]

From: Eduardo Valentin <eduardo.valentin@ti.com>

In case the trend is not changing or when there is no
request for throttling, it is expected that the instance
would not change its requested target. This patch improves
the code implementation to cover for this expected behavior.

With current implementation, the instance will always
reset to cdev.cur_state, even in not expected cases,
like those mentioned above.

This patch changes the step_wise governor implementation
of get_target so that we accomplish:
(a) - default value will be current instance->target, so
we do not change the thermal instance target unnecessarily.
(b) - the code now it is clear about what is the intention.
There is a clear statement of what are the expected outcomes
(c) - removal of hardcoded constants, now it is put in use
the THERMAL_NO_TARGET macro.
(d) - variable names are also improved so that reader can
clearly understand the difference between instance cur target,
next target and cdev cur_state.

Cc: Zhang Rui <rui.zhang@intel.com>
Cc: Durgadoss R <durgadoss.r@intel.com>
Cc: linux-pm@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Reported-by: Ruslan Ruslichenko <ruslan.ruslichenko@ti.com>
Signed-of-by: Eduardo Valentin <eduardo.valentin@ti.com>
Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/thermal/step_wise.c |   29 ++++++++++++++++++-----------
 1 file changed, 18 insertions(+), 11 deletions(-)

diff --git a/drivers/thermal/step_wise.c b/drivers/thermal/step_wise.c
index 0afbd86..d89e781 100644
--- a/drivers/thermal/step_wise.c
+++ b/drivers/thermal/step_wise.c
@@ -51,44 +51,51 @@ static unsigned long get_target_state(struct thermal_instance *instance,
 {
 	struct thermal_cooling_device *cdev = instance->cdev;
 	unsigned long cur_state;
+	unsigned long next_target;
 
+	/*
+	 * We keep this instance the way it is by default.
+	 * Otherwise, we use the current state of the
+	 * cdev in use to determine the next_target.
+	 */
 	cdev->ops->get_cur_state(cdev, &cur_state);
+	next_target = instance->target;
 
 	switch (trend) {
 	case THERMAL_TREND_RAISING:
 		if (throttle) {
-			cur_state = cur_state < instance->upper ?
+			next_target = cur_state < instance->upper ?
 				    (cur_state + 1) : instance->upper;
-			if (cur_state < instance->lower)
-				cur_state = instance->lower;
+			if (next_target < instance->lower)
+				next_target = instance->lower;
 		}
 		break;
 	case THERMAL_TREND_RAISE_FULL:
 		if (throttle)
-			cur_state = instance->upper;
+			next_target = instance->upper;
 		break;
 	case THERMAL_TREND_DROPPING:
 		if (cur_state == instance->lower) {
 			if (!throttle)
-				cur_state = -1;
+				next_target = THERMAL_NO_TARGET;
 		} else {
-			cur_state -= 1;
-			if (cur_state > instance->upper)
-				cur_state = instance->upper;
+			next_target = cur_state - 1;
+			if (next_target > instance->upper)
+				next_target = instance->upper;
 		}
 		break;
 	case THERMAL_TREND_DROP_FULL:
 		if (cur_state == instance->lower) {
 			if (!throttle)
-				cur_state = -1;
+				next_target = THERMAL_NO_TARGET;
 		} else
-			cur_state = instance->lower;
+			next_target = instance->lower;
 		break;
 	default:
 		break;
 	}
 
-	return cur_state;
+	return next_target;
 }
 
 static void update_passive_instance(struct thermal_zone_device *tz,
-- 
1.7.9.5

[RFC PATCH 3/6] Thermal: fix step_wise handling of THERMAL_TREND_DROPPING

[Zhang Rui]

Fixes two problems in the THERMAL_TREND_DROPPING
handling code in step_wise governor.
1. When the temperature is higher than the trip point,
we should not deactivate the thermal instance.
2. When the temperature is lower than the trip point,
we should not activate the thermal instance.

Also rephrase the code a bit to make it more readable.

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/thermal/step_wise.c |   18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/thermal/step_wise.c b/drivers/thermal/step_wise.c
index d89e781..f0cc5e5 100644
--- a/drivers/thermal/step_wise.c
+++ b/drivers/thermal/step_wise.c
@@ -75,13 +75,19 @@ static unsigned long get_target_state(struct thermal_instance *instance,
 			next_target = instance->upper;
 		break;
 	case THERMAL_TREND_DROPPING:
-		if (cur_state == instance->lower) {
-			if (!throttle)
-				next_target = THERMAL_NO_TARGET;
+		if (throttle) {
+			if (cur_state <= instance->lower)
+				next_target = instance->lower;
+			else
+				next_target = cur_state > instance->upper ?
+					instance->upper : cur_state - 1;
 		} else {
-			next_target = cur_state - 1;
-			if (next_target > instance->upper)
-				next_target = instance->upper;
+			if (cur_state <= instance->lower ||
+			    instance->target == THERMAL_NO_TARGET)
+				next_target = THERMAL_NO_TARGET;
+			else
+				next_target = cur_state > instance->upper ?
+					instance->upper : cur_state - 1;
 		}
 		break;
 	case THERMAL_TREND_DROP_FULL:
-- 
1.7.9.5

[RFC PATCH 4/6] Thermal: fix step_wise handling of THERMAL_TREND_DROP_EFULL

[Zhang Rui]

Change the step_wise cooling algorithm to handle
THERMAL_TREND_DROP_FULL a bit differently.

When the temperature is higher than the trip point,
we should always use the instance->lower as the next target state.
When the temperature is lower than the trip point,
we should always deactive the thermal instance.

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/thermal/step_wise.c |   12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/thermal/step_wise.c b/drivers/thermal/step_wise.c
index f0cc5e5..84d6e90 100644
--- a/drivers/thermal/step_wise.c
+++ b/drivers/thermal/step_wise.c
@@ -42,9 +42,8 @@
  *       state for this trip point, if the cooling state already
  *       equals lower limit, deactivate the thermal instance
  *    c. if the trend is THERMAL_TREND_RAISE_FULL, do nothing
- *    d. if the trend is THERMAL_TREND_DROP_FULL, use lower limit,
- *       if the cooling state already equals lower limit,
- *       deactive the thermal instance
+ *    d. if the trend is THERMAL_TREND_DROP_FULL, deactive
+ *	 the thermal instance
  */
 static unsigned long get_target_state(struct thermal_instance *instance,
 				enum thermal_trend trend, bool throttle)
@@ -91,11 +90,10 @@ static unsigned long get_target_state(struct thermal_instance *instance,
 		}
 		break;
 	case THERMAL_TREND_DROP_FULL:
-		if (cur_state == instance->lower) {
-			if (!throttle)
-				next_target = THERMAL_NO_TARGET;
-		} else
+		if (throttle)
 			next_target = instance->lower;
+		else
+			next_target = THERMAL_NO_TARGET;
 		break;
 	default:
 		break;
-- 
1.7.9.5

[RFC PATCH 5/6] Thermal: step_wise handle THERMAL_TREND_STABLE explicitly

[Zhang Rui]

The current code does not show the handling of
THERMAL_TREND_STABLE explicitly, which may be confusing.

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/thermal/step_wise.c |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/thermal/step_wise.c b/drivers/thermal/step_wise.c
index 84d6e90..53b56e6 100644
--- a/drivers/thermal/step_wise.c
+++ b/drivers/thermal/step_wise.c
@@ -36,6 +36,7 @@
  *       for this trip point
  *    d. if the trend is THERMAL_TREND_DROP_FULL, use lower limit
  *       for this trip point
+ *    e. if the trend is THERMAL_TREND_STABLE, do nothing.
  * If the temperature is lower than a trip point,
  *    a. if the trend is THERMAL_TREND_RAISING, do nothing
  *    b. if the trend is THERMAL_TREND_DROPPING, use lower cooling
@@ -44,6 +45,7 @@
  *    c. if the trend is THERMAL_TREND_RAISE_FULL, do nothing
  *    d. if the trend is THERMAL_TREND_DROP_FULL, deactive
  *	 the thermal instance
+ *    e. if the trend is THERMAL_TREND_STABLE, do nothing.
  */
 static unsigned long get_target_state(struct thermal_instance *instance,
 				enum thermal_trend trend, bool throttle)
@@ -95,6 +97,8 @@ static unsigned long get_target_state(struct thermal_instance *instance,
 		else
 			next_target = THERMAL_NO_TARGET;
 		break;
+	case THERMAL_TREND_STABLE:
+		/* Do nothing */
 	default:
 		break;
 	}
-- 
1.7.9.5

[RFC PATCH 6/6] Thermal: step_wise: set next cooling target explicitly

[Zhang Rui]

Set the next target state explicitly for each thermal trend,
in step_wise governor, to provide more readability, and to
follow the cooling algorithm description strictly.

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/thermal/step_wise.c |    9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/thermal/step_wise.c b/drivers/thermal/step_wise.c
index 53b56e6..ffa553f 100644
--- a/drivers/thermal/step_wise.c
+++ b/drivers/thermal/step_wise.c
@@ -60,7 +60,6 @@ static unsigned long get_target_state(struct thermal_instance *instance,
 	 * cdev in use to determine the next_target.
 	 */
 	cdev->ops->get_cur_state(cdev, &cur_state);
-	next_target = instance->target;
 
 	switch (trend) {
 	case THERMAL_TREND_RAISING:
@@ -69,11 +68,14 @@ static unsigned long get_target_state(struct thermal_instance *instance,
 				    (cur_state + 1) : instance->upper;
 			if (next_target < instance->lower)
 				next_target = instance->lower;
-		}
+		} else
+			next_target = instance->target;
 		break;
 	case THERMAL_TREND_RAISE_FULL:
 		if (throttle)
 			next_target = instance->upper;
+		else
+			next_target = instance->target;
 		break;
 	case THERMAL_TREND_DROPPING:
 		if (throttle) {
@@ -99,7 +101,10 @@ static unsigned long get_target_state(struct thermal_instance *instance,
 		break;
 	case THERMAL_TREND_STABLE:
 		/* Do nothing */
+		next_target = instance->target;
+		break;
 	default:
+		next_target = instance->target;
 		break;
 	}
 
-- 
1.7.9.5

Re: [RFC PATCH 0/6] Thermal: step_wise governor fixes

[Shawn Guo]

On Mon, Jun 17, 2013 at 09:24:22PM +0800, Zhang Rui wrote:
> Hi, all,
> 
> There are a lot of step_wise governor problems reported recently,
> and plus, there are already several patches avaible, but I'd like to do
> a cleanup for this piece of code altogether.
> 
> This patch set fixes several problem as addressed in the description
> of each patch.
> Please kindly review.
> 
> Note: I just run build test. It would be great that some of you
> can test it.

The series works fine on imx6q with the patch "thermal: add imx thermal
driver support" I submitted a few days ago, so for the series,

Tested-by: Shawn Guo <shawn.guo@linaro.org>

Re: [RFC PATCH 3/6] Thermal: fix step_wise handling of THERMAL_TREND_DROPPING

[Eduardo Valentin]

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

On 17-06-2013 09:24, Zhang Rui wrote:
> Fixes two problems in the THERMAL_TREND_DROPPING
> handling code in step_wise governor.
> 1. When the temperature is higher than the trip point,
> we should not deactivate the thermal instance.

When temperature is higher than trip point, should it then get the
actions of the next trip point?

Does it mean that with this patch, all thermal instances assigned to
trip points below the cur temperature would be activated, right?

> 2. When the temperature is lower than the trip point,
> we should not activate the thermal instance.
> 

When would they get activated then?


> Also rephrase the code a bit to make it more readable.
> 
> Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> ---
>  drivers/thermal/step_wise.c |   18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/thermal/step_wise.c b/drivers/thermal/step_wise.c
> index d89e781..f0cc5e5 100644
> --- a/drivers/thermal/step_wise.c
> +++ b/drivers/thermal/step_wise.c
> @@ -75,13 +75,19 @@ static unsigned long get_target_state(struct thermal_instance *instance,
>  			next_target = instance->upper;
>  		break;
>  	case THERMAL_TREND_DROPPING:
> -		if (cur_state == instance->lower) {
> -			if (!throttle)
> -				next_target = THERMAL_NO_TARGET;
> +		if (throttle) {
> +			if (cur_state <= instance->lower)
> +				next_target = instance->lower;
> +			else
> +				next_target = cur_state > instance->upper ?
> +					instance->upper : cur_state - 1;
>  		} else {
> -			next_target = cur_state - 1;
> -			if (next_target > instance->upper)
> -				next_target = instance->upper;
> +			if (cur_state <= instance->lower ||
> +			    instance->target == THERMAL_NO_TARGET)
> +				next_target = THERMAL_NO_TARGET;
> +			else
> +				next_target = cur_state > instance->upper ?
> +					instance->upper : cur_state - 1;
>  		}
>  		break;
>  	case THERMAL_TREND_DROP_FULL:
> 


-- 
You have got to be excited about what you are doing. (L. Lamport)

Eduardo Valentin


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 295 bytes --]

Re: [RFC PATCH 6/6] Thermal: step_wise: set next cooling target explicitly

[Eduardo Valentin]

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

On 17-06-2013 09:24, Zhang Rui wrote:
> Set the next target state explicitly for each thermal trend,
> in step_wise governor, to provide more readability, and to
> follow the cooling algorithm description strictly.


How about merging this one with patch 05?

> 
> Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> ---
>  drivers/thermal/step_wise.c |    9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/thermal/step_wise.c b/drivers/thermal/step_wise.c
> index 53b56e6..ffa553f 100644
> --- a/drivers/thermal/step_wise.c
> +++ b/drivers/thermal/step_wise.c
> @@ -60,7 +60,6 @@ static unsigned long get_target_state(struct thermal_instance *instance,
>  	 * cdev in use to determine the next_target.
>  	 */
>  	cdev->ops->get_cur_state(cdev, &cur_state);
> -	next_target = instance->target;
>  
>  	switch (trend) {
>  	case THERMAL_TREND_RAISING:
> @@ -69,11 +68,14 @@ static unsigned long get_target_state(struct thermal_instance *instance,
>  				    (cur_state + 1) : instance->upper;
>  			if (next_target < instance->lower)
>  				next_target = instance->lower;
> -		}
> +		} else
> +			next_target = instance->target;
>  		break;
>  	case THERMAL_TREND_RAISE_FULL:
>  		if (throttle)
>  			next_target = instance->upper;
> +		else
> +			next_target = instance->target;
>  		break;
>  	case THERMAL_TREND_DROPPING:
>  		if (throttle) {
> @@ -99,7 +101,10 @@ static unsigned long get_target_state(struct thermal_instance *instance,
>  		break;
>  	case THERMAL_TREND_STABLE:
>  		/* Do nothing */
> +		next_target = instance->target;
> +		break;
>  	default:
> +		next_target = instance->target;
>  		break;


The above could be rewritten like this:
  	case THERMAL_TREND_STABLE:
  		/* Do nothing */
  	default:
 		next_target = instance->target;
  		break;

>  	}
>  
> 


-- 
You have got to be excited about what you are doing. (L. Lamport)

Eduardo Valentin


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 295 bytes --]

Re: [RFC PATCH 6/6] Thermal: step_wise: set next cooling target explicitly

[Eduardo Valentin]

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

On 17-06-2013 09:24, Zhang Rui wrote:
> Set the next target state explicitly for each thermal trend,
> in step_wise governor, to provide more readability, and to
> follow the cooling algorithm description strictly.


How about merging this one with patch 05?

> 
> Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> ---
>  drivers/thermal/step_wise.c |    9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/thermal/step_wise.c b/drivers/thermal/step_wise.c
> index 53b56e6..ffa553f 100644
> --- a/drivers/thermal/step_wise.c
> +++ b/drivers/thermal/step_wise.c
> @@ -60,7 +60,6 @@ static unsigned long get_target_state(struct thermal_instance *instance,
>  	 * cdev in use to determine the next_target.
>  	 */
>  	cdev->ops->get_cur_state(cdev, &cur_state);
> -	next_target = instance->target;
>  
>  	switch (trend) {
>  	case THERMAL_TREND_RAISING:
> @@ -69,11 +68,14 @@ static unsigned long get_target_state(struct thermal_instance *instance,
>  				    (cur_state + 1) : instance->upper;
>  			if (next_target < instance->lower)
>  				next_target = instance->lower;
> -		}
> +		} else
> +			next_target = instance->target;
>  		break;
>  	case THERMAL_TREND_RAISE_FULL:
>  		if (throttle)
>  			next_target = instance->upper;
> +		else
> +			next_target = instance->target;
>  		break;
>  	case THERMAL_TREND_DROPPING:
>  		if (throttle) {
> @@ -99,7 +101,10 @@ static unsigned long get_target_state(struct thermal_instance *instance,
>  		break;
>  	case THERMAL_TREND_STABLE:
>  		/* Do nothing */
> +		next_target = instance->target;
> +		break;
>  	default:
> +		next_target = instance->target;
>  		break;


The above could be rewritten like this:
  	case THERMAL_TREND_STABLE:
  		/* Do nothing */
  	default:
 		next_target = instance->target;
  		break;

>  	}
>  
> 


-- 
You have got to be excited about what you are doing. (L. Lamport)

Eduardo Valentin


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 295 bytes --]

Re: [RFC PATCH 4/6] Thermal: fix step_wise handling of THERMAL_TREND_DROP_EFULL

[Eduardo Valentin]

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

On subject s/THERMAL_TREND_DROP_EFULL/THERMAL_TREND_DROP_FULL/g

On 17-06-2013 09:24, Zhang Rui wrote:
> Change the step_wise cooling algorithm to handle
> THERMAL_TREND_DROP_FULL a bit differently.
> 
> When the temperature is higher than the trip point,
> we should always use the instance->lower as the next target state.
> When the temperature is lower than the trip point,
> we should always deactive the thermal instance.

s/deactive/deactivate/g


Are you expecting to optimize the system in which way by doing this
change? In which situation we would need to set to instance->lower if we
are at a state which trend is dropping fully?

> 
> Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> ---
>  drivers/thermal/step_wise.c |   12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/thermal/step_wise.c b/drivers/thermal/step_wise.c
> index f0cc5e5..84d6e90 100644
> --- a/drivers/thermal/step_wise.c
> +++ b/drivers/thermal/step_wise.c
> @@ -42,9 +42,8 @@
>   *       state for this trip point, if the cooling state already
>   *       equals lower limit, deactivate the thermal instance
>   *    c. if the trend is THERMAL_TREND_RAISE_FULL, do nothing
> - *    d. if the trend is THERMAL_TREND_DROP_FULL, use lower limit,
> - *       if the cooling state already equals lower limit,
> - *       deactive the thermal instance
> + *    d. if the trend is THERMAL_TREND_DROP_FULL, deactive
> + *	 the thermal instance
>   */
>  static unsigned long get_target_state(struct thermal_instance *instance,
>  				enum thermal_trend trend, bool throttle)
> @@ -91,11 +90,10 @@ static unsigned long get_target_state(struct thermal_instance *instance,
>  		}
>  		break;
>  	case THERMAL_TREND_DROP_FULL:
> -		if (cur_state == instance->lower) {
> -			if (!throttle)
> -				next_target = THERMAL_NO_TARGET;
> -		} else
> +		if (throttle)
>  			next_target = instance->lower;
> +		else
> +			next_target = THERMAL_NO_TARGET;
>  		break;
>  	default:
>  		break;
> 


-- 
You have got to be excited about what you are doing. (L. Lamport)

Eduardo Valentin


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 295 bytes --]

Re: [RFC PATCH 3/6] Thermal: fix step_wise handling of THERMAL_TREND_DROPPING

[Zhang Rui]

On Tue, 2013-07-02 at 16:57 -0400, Eduardo Valentin wrote:
> On 17-06-2013 09:24, Zhang Rui wrote:
> > Fixes two problems in the THERMAL_TREND_DROPPING
> > handling code in step_wise governor.
> > 1. When the temperature is higher than the trip point,
> > we should not deactivate the thermal instance.
> 
> When temperature is higher than trip point, should it then get the
> actions of the next trip point?
> 
no, a thermal instance is only activated when the temperature is above
the trip point.
The thermal instance may still be active for a while even if the
temperature gets lower than the trip point, and then be deactivated when
the instance->target is set to THERMAL_NO_TARGET.
Say, instance->lower is 3, upper is 6, and the cooling device is in
cooling state 6. When the temperature drops below the trip point,
step_wise governor should reduce the target to 5, then 4, then 3, and
then THERMAL_NO_TARGET.

> Does it mean that with this patch, all thermal instances assigned to
> trip points below the cur temperature would be activated, right?
> 
No, the instance will keep active only if its target is not
THERMAL_NO_TARGET.

> > 2. When the temperature is lower than the trip point,
> > we should not activate the thermal instance.
> > 
> 
> When would they get activated then?
> 
It is activated when the temperature aboves the trip point, and
deactivated when the instance->target is set to THERMAL_NO_TARGET by the
cooling algorithm.

thanks,
rui
> 
> > Also rephrase the code a bit to make it more readable.
> > 
> > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> > ---
> >  drivers/thermal/step_wise.c |   18 ++++++++++++------
> >  1 file changed, 12 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/thermal/step_wise.c b/drivers/thermal/step_wise.c
> > index d89e781..f0cc5e5 100644
> > --- a/drivers/thermal/step_wise.c
> > +++ b/drivers/thermal/step_wise.c
> > @@ -75,13 +75,19 @@ static unsigned long get_target_state(struct thermal_instance *instance,
> >  			next_target = instance->upper;
> >  		break;
> >  	case THERMAL_TREND_DROPPING:
> > -		if (cur_state == instance->lower) {
> > -			if (!throttle)
> > -				next_target = THERMAL_NO_TARGET;
> > +		if (throttle) {
> > +			if (cur_state <= instance->lower)
> > +				next_target = instance->lower;
> > +			else
> > +				next_target = cur_state > instance->upper ?
> > +					instance->upper : cur_state - 1;
> >  		} else {
> > -			next_target = cur_state - 1;
> > -			if (next_target > instance->upper)
> > -				next_target = instance->upper;
> > +			if (cur_state <= instance->lower ||
> > +			    instance->target == THERMAL_NO_TARGET)
> > +				next_target = THERMAL_NO_TARGET;
> > +			else
> > +				next_target = cur_state > instance->upper ?
> > +					instance->upper : cur_state - 1;
> >  		}
> >  		break;
> >  	case THERMAL_TREND_DROP_FULL:
> > 
> 
> 

Re: [RFC PATCH 6/6] Thermal: step_wise: set next cooling target explicitly

[Zhang Rui]

On Tue, 2013-07-02 at 17:03 -0400, Eduardo Valentin wrote:
> On 17-06-2013 09:24, Zhang Rui wrote:
> > Set the next target state explicitly for each thermal trend,
> > in step_wise governor, to provide more readability, and to
> > follow the cooling algorithm description strictly.
> 
> 
> How about merging this one with patch 05?
> 
> > 
> > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> > ---
> >  drivers/thermal/step_wise.c |    9 +++++++--
> >  1 file changed, 7 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/thermal/step_wise.c b/drivers/thermal/step_wise.c
> > index 53b56e6..ffa553f 100644
> > --- a/drivers/thermal/step_wise.c
> > +++ b/drivers/thermal/step_wise.c
> > @@ -60,7 +60,6 @@ static unsigned long get_target_state(struct thermal_instance *instance,
> >  	 * cdev in use to determine the next_target.
> >  	 */
> >  	cdev->ops->get_cur_state(cdev, &cur_state);
> > -	next_target = instance->target;
> >  
> >  	switch (trend) {
> >  	case THERMAL_TREND_RAISING:
> > @@ -69,11 +68,14 @@ static unsigned long get_target_state(struct thermal_instance *instance,
> >  				    (cur_state + 1) : instance->upper;
> >  			if (next_target < instance->lower)
> >  				next_target = instance->lower;
> > -		}
> > +		} else
> > +			next_target = instance->target;
> >  		break;
> >  	case THERMAL_TREND_RAISE_FULL:
> >  		if (throttle)
> >  			next_target = instance->upper;
> > +		else
> > +			next_target = instance->target;
> >  		break;
> >  	case THERMAL_TREND_DROPPING:
> >  		if (throttle) {
> > @@ -99,7 +101,10 @@ static unsigned long get_target_state(struct thermal_instance *instance,
> >  		break;
> >  	case THERMAL_TREND_STABLE:
> >  		/* Do nothing */
> > +		next_target = instance->target;
> > +		break;
> >  	default:
> > +		next_target = instance->target;
> >  		break;
> 
> 
> The above could be rewritten like this:
>   	case THERMAL_TREND_STABLE:
>   		/* Do nothing */
>   	default:
>  		next_target = instance->target;
>   		break;
> 

sure I'm okay with your suggestion.
I wrote the code in this way because I want to keep the code follows the
description explicitly.

thanks,
rui
> >  	}
> >  
> > 
> 
> 

Re: [RFC PATCH 4/6] Thermal: fix step_wise handling of THERMAL_TREND_DROP_EFULL

[Zhang Rui]

On Tue, 2013-07-02 at 17:11 -0400, Eduardo Valentin wrote:
> On subject s/THERMAL_TREND_DROP_EFULL/THERMAL_TREND_DROP_FULL/g
> 
agreed.

> On 17-06-2013 09:24, Zhang Rui wrote:
> > Change the step_wise cooling algorithm to handle
> > THERMAL_TREND_DROP_FULL a bit differently.
> > 
> > When the temperature is higher than the trip point,
> > we should always use the instance->lower as the next target state.
> > When the temperature is lower than the trip point,
> > we should always deactive the thermal instance.
> 
> s/deactive/deactivate/g
> 
agreed.

thanks,
rui
> 
> Are you expecting to optimize the system in which way by doing this
> change? In which situation we would need to set to instance->lower if we
> are at a state which trend is dropping fully?
> 
> > 
> > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> > ---
> >  drivers/thermal/step_wise.c |   12 +++++-------
> >  1 file changed, 5 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/thermal/step_wise.c b/drivers/thermal/step_wise.c
> > index f0cc5e5..84d6e90 100644
> > --- a/drivers/thermal/step_wise.c
> > +++ b/drivers/thermal/step_wise.c
> > @@ -42,9 +42,8 @@
> >   *       state for this trip point, if the cooling state already
> >   *       equals lower limit, deactivate the thermal instance
> >   *    c. if the trend is THERMAL_TREND_RAISE_FULL, do nothing
> > - *    d. if the trend is THERMAL_TREND_DROP_FULL, use lower limit,
> > - *       if the cooling state already equals lower limit,
> > - *       deactive the thermal instance
> > + *    d. if the trend is THERMAL_TREND_DROP_FULL, deactive
> > + *	 the thermal instance
> >   */
> >  static unsigned long get_target_state(struct thermal_instance *instance,
> >  				enum thermal_trend trend, bool throttle)
> > @@ -91,11 +90,10 @@ static unsigned long get_target_state(struct thermal_instance *instance,
> >  		}
> >  		break;
> >  	case THERMAL_TREND_DROP_FULL:
> > -		if (cur_state == instance->lower) {
> > -			if (!throttle)
> > -				next_target = THERMAL_NO_TARGET;
> > -		} else
> > +		if (throttle)
> >  			next_target = instance->lower;
> > +		else
> > +			next_target = THERMAL_NO_TARGET;
> >  		break;
> >  	default:
> >  		break;
> > 
> 
>