diff mbox

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

Message ID 1371475468-5351-4-git-send-email-rui.zhang@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Zhang Rui
Headers show

Commit Message

Zhang Rui June 17, 2013, 1:24 p.m. UTC
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(-)

Comments

Eduardo Valentin July 2, 2013, 8:57 p.m. UTC | #1
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:
>
Zhang Rui July 8, 2013, 2:32 a.m. UTC | #2
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:
> > 
> 
> 


--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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: