diff mbox series

[v1,02/16] thermal: gov_bang_bang: Use .trip_crossed() instead of .throttle()

Message ID 2289003.iZASKD2KPV@kreacher (mailing list archive)
State In Next
Delegated to: Rafael Wysocki
Headers show
Series thermal: core: Redesign the governor interface | expand

Commit Message

Rafael J. Wysocki April 10, 2024, 4:04 p.m. UTC
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

The Bang-Bang governor really is only concerned about trip point
crossing, so it can use the new .trip_crossed() callback instead of
.throttle() that is not particularly suitable for it.

Modify it to do so which also takes trip hysteresis into account, so the
governor does not need to use it directly any more.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/thermal/gov_bang_bang.c |   31 +++++++++++++------------------
 1 file changed, 13 insertions(+), 18 deletions(-)

Comments

Lukasz Luba April 19, 2024, 9:34 a.m. UTC | #1
On 4/10/24 17:04, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> The Bang-Bang governor really is only concerned about trip point
> crossing, so it can use the new .trip_crossed() callback instead of
> .throttle() that is not particularly suitable for it.
> 
> Modify it to do so which also takes trip hysteresis into account, so the
> governor does not need to use it directly any more.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>   drivers/thermal/gov_bang_bang.c |   31 +++++++++++++------------------
>   1 file changed, 13 insertions(+), 18 deletions(-)
> 
> Index: linux-pm/drivers/thermal/gov_bang_bang.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/gov_bang_bang.c
> +++ linux-pm/drivers/thermal/gov_bang_bang.c
> @@ -13,8 +13,9 @@
>   
>   #include "thermal_core.h"
>   
> -static int thermal_zone_trip_update(struct thermal_zone_device *tz,
> -				    const struct thermal_trip *trip)
> +static void thermal_zone_trip_update(struct thermal_zone_device *tz,
> +				     const struct thermal_trip *trip,
> +				     bool crossed_up)
>   {
>   	int trip_index = thermal_zone_trip_id(tz, trip);
>   	struct thermal_instance *instance;
> @@ -43,13 +44,12 @@ static int thermal_zone_trip_update(stru
>   		}
>   
>   		/*
> -		 * enable fan when temperature exceeds trip_temp and disable
> -		 * the fan in case it falls below trip_temp minus hysteresis
> +		 * Enable the fan when the trip is crossed on the way up and
> +		 * disable it when the trip is crossed on the way down.
>   		 */
> -		if (instance->target == 0 && tz->temperature >= trip->temperature)
> +		if (instance->target == 0 && crossed_up)
>   			instance->target = 1;
> -		else if (instance->target == 1 &&
> -			 tz->temperature < trip->temperature - trip->hysteresis)
> +		else if (instance->target == 1 && !crossed_up)
>   			instance->target = 0;
>   
>   		dev_dbg(&instance->cdev->device, "target=%d\n",
> @@ -59,14 +59,13 @@ static int thermal_zone_trip_update(stru
>   		instance->cdev->updated = false; /* cdev needs update */
>   		mutex_unlock(&instance->cdev->lock);
>   	}
> -
> -	return 0;
>   }
>   
>   /**
>    * bang_bang_control - controls devices associated with the given zone
>    * @tz: thermal_zone_device
>    * @trip: the trip point
> + * @crossed_up: whether or not the trip has been crossed on the way up
>    *
>    * Regulation Logic: a two point regulation, deliver cooling state depending
>    * on the previous state shown in this diagram:
> @@ -90,26 +89,22 @@ static int thermal_zone_trip_update(stru
>    *     (trip_temp - hyst) so that the fan gets turned off again.
>    *
>    */
> -static int bang_bang_control(struct thermal_zone_device *tz,
> -			     const struct thermal_trip *trip)
> +static void bang_bang_control(struct thermal_zone_device *tz,
> +			      const struct thermal_trip *trip,
> +			      bool crossed_up)
>   {
>   	struct thermal_instance *instance;
> -	int ret;
>   
>   	lockdep_assert_held(&tz->lock);
>   
> -	ret = thermal_zone_trip_update(tz, trip);
> -	if (ret)
> -		return ret;
> +	thermal_zone_trip_update(tz, trip, crossed_up);
>   
>   	list_for_each_entry(instance, &tz->thermal_instances, tz_node)
>   		thermal_cdev_update(instance->cdev);
> -
> -	return 0;
>   }
>   
>   static struct thermal_governor thermal_gov_bang_bang = {
>   	.name		= "bang_bang",
> -	.throttle	= bang_bang_control,
> +	.trip_crossed	= bang_bang_control,
>   };
>   THERMAL_GOVERNOR_DECLARE(thermal_gov_bang_bang);
> 
> 
> 

LGTM

Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>
Daniel Lezcano April 23, 2024, 5:04 p.m. UTC | #2
On 10/04/2024 18:04, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> The Bang-Bang governor really is only concerned about trip point
> crossing, so it can use the new .trip_crossed() callback instead of
> .throttle() that is not particularly suitable for it.
> 
> Modify it to do so which also takes trip hysteresis into account, so the
> governor does not need to use it directly any more.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---

Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>
diff mbox series

Patch

Index: linux-pm/drivers/thermal/gov_bang_bang.c
===================================================================
--- linux-pm.orig/drivers/thermal/gov_bang_bang.c
+++ linux-pm/drivers/thermal/gov_bang_bang.c
@@ -13,8 +13,9 @@ 
 
 #include "thermal_core.h"
 
-static int thermal_zone_trip_update(struct thermal_zone_device *tz,
-				    const struct thermal_trip *trip)
+static void thermal_zone_trip_update(struct thermal_zone_device *tz,
+				     const struct thermal_trip *trip,
+				     bool crossed_up)
 {
 	int trip_index = thermal_zone_trip_id(tz, trip);
 	struct thermal_instance *instance;
@@ -43,13 +44,12 @@  static int thermal_zone_trip_update(stru
 		}
 
 		/*
-		 * enable fan when temperature exceeds trip_temp and disable
-		 * the fan in case it falls below trip_temp minus hysteresis
+		 * Enable the fan when the trip is crossed on the way up and
+		 * disable it when the trip is crossed on the way down.
 		 */
-		if (instance->target == 0 && tz->temperature >= trip->temperature)
+		if (instance->target == 0 && crossed_up)
 			instance->target = 1;
-		else if (instance->target == 1 &&
-			 tz->temperature < trip->temperature - trip->hysteresis)
+		else if (instance->target == 1 && !crossed_up)
 			instance->target = 0;
 
 		dev_dbg(&instance->cdev->device, "target=%d\n",
@@ -59,14 +59,13 @@  static int thermal_zone_trip_update(stru
 		instance->cdev->updated = false; /* cdev needs update */
 		mutex_unlock(&instance->cdev->lock);
 	}
-
-	return 0;
 }
 
 /**
  * bang_bang_control - controls devices associated with the given zone
  * @tz: thermal_zone_device
  * @trip: the trip point
+ * @crossed_up: whether or not the trip has been crossed on the way up
  *
  * Regulation Logic: a two point regulation, deliver cooling state depending
  * on the previous state shown in this diagram:
@@ -90,26 +89,22 @@  static int thermal_zone_trip_update(stru
  *     (trip_temp - hyst) so that the fan gets turned off again.
  *
  */
-static int bang_bang_control(struct thermal_zone_device *tz,
-			     const struct thermal_trip *trip)
+static void bang_bang_control(struct thermal_zone_device *tz,
+			      const struct thermal_trip *trip,
+			      bool crossed_up)
 {
 	struct thermal_instance *instance;
-	int ret;
 
 	lockdep_assert_held(&tz->lock);
 
-	ret = thermal_zone_trip_update(tz, trip);
-	if (ret)
-		return ret;
+	thermal_zone_trip_update(tz, trip, crossed_up);
 
 	list_for_each_entry(instance, &tz->thermal_instances, tz_node)
 		thermal_cdev_update(instance->cdev);
-
-	return 0;
 }
 
 static struct thermal_governor thermal_gov_bang_bang = {
 	.name		= "bang_bang",
-	.throttle	= bang_bang_control,
+	.trip_crossed	= bang_bang_control,
 };
 THERMAL_GOVERNOR_DECLARE(thermal_gov_bang_bang);