[PATCH v2 1/3] thermal/core: Encapsulate the trip point crossed function

[PATCH v2 1/3] thermal/core: Encapsulate the trip point crossed function

[Daniel Lezcano]

The routine where the trip point crossed is detected is a strategic
place where different processing will happen. Encapsulate the code in
a function, so all specific actions related with a trip point crossed
can be grouped.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/thermal/thermal_core.c | 30 ++++++++++++++++++++----------
 1 file changed, 20 insertions(+), 10 deletions(-)

diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index cdc0552e8c42..d9f771b15ed8 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -358,6 +358,25 @@ static void handle_critical_trips(struct thermal_zone_device *tz,
 		tz->ops->critical(tz);
 }
 
+static void handle_thermal_trip_crossed(struct thermal_zone_device *tz, int trip,
+					int trip_temp, int trip_hyst, enum thermal_trip_type trip_type)
+{
+	if (tz->last_temperature == THERMAL_TEMP_INVALID)
+		return;
+
+	if (tz->last_temperature < trip_temp &&
+	    tz->temperature >= trip_temp) {
+		thermal_notify_tz_trip_up(tz->id, trip,
+					  tz->temperature);
+	}
+
+	if (tz->last_temperature >= trip_temp &&
+	    tz->temperature < (trip_temp - trip_hyst)) {
+		thermal_notify_tz_trip_down(tz->id, trip,
+					    tz->temperature);
+	}
+}
+
 static void handle_thermal_trip(struct thermal_zone_device *tz, int trip)
 {
 	enum thermal_trip_type type;
@@ -372,16 +391,7 @@ static void handle_thermal_trip(struct thermal_zone_device *tz, int trip)
 	if (tz->ops->get_trip_hyst)
 		tz->ops->get_trip_hyst(tz, trip, &hyst);
 
-	if (tz->last_temperature != THERMAL_TEMP_INVALID) {
-		if (tz->last_temperature < trip_temp &&
-		    tz->temperature >= trip_temp)
-			thermal_notify_tz_trip_up(tz->id, trip,
-						  tz->temperature);
-		if (tz->last_temperature >= trip_temp &&
-		    tz->temperature < (trip_temp - hyst))
-			thermal_notify_tz_trip_down(tz->id, trip,
-						    tz->temperature);
-	}
+	handle_thermal_trip_crossed(tz, trip, trip_temp, hyst, type);
 
 	if (type == THERMAL_TRIP_CRITICAL || type == THERMAL_TRIP_HOT)
 		handle_critical_trips(tz, trip, type);
-- 
2.25.1

[PATCH v2 2/3] thermal/core: Passing a parameter instead of calling the function again

[Daniel Lezcano]

As the trip temperature is already available when calling the
function, pass it as a parameter instead of having the function
calling the ops again to retrieve the same data.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/thermal/thermal_core.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index d9f771b15ed8..f66036b3daae 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -340,12 +340,8 @@ void thermal_zone_device_critical(struct thermal_zone_device *tz)
 EXPORT_SYMBOL(thermal_zone_device_critical);
 
 static void handle_critical_trips(struct thermal_zone_device *tz,
-				  int trip, enum thermal_trip_type trip_type)
+				  int trip, int trip_temp, enum thermal_trip_type trip_type)
 {
-	int trip_temp;
-
-	tz->ops->get_trip_temp(tz, trip, &trip_temp);
-
 	/* If we have not crossed the trip_temp, we do not care. */
 	if (trip_temp <= 0 || tz->temperature < trip_temp)
 		return;
@@ -394,7 +390,7 @@ static void handle_thermal_trip(struct thermal_zone_device *tz, int trip)
 	handle_thermal_trip_crossed(tz, trip, trip_temp, hyst, type);
 
 	if (type == THERMAL_TRIP_CRITICAL || type == THERMAL_TRIP_HOT)
-		handle_critical_trips(tz, trip, type);
+		handle_critical_trips(tz, trip, trip_temp, type);
 	else
 		handle_non_critical_trips(tz, trip);
 	/*
-- 
2.25.1

[PATCH v2 3/3] thermal/core: Fix thermal trip cross point

[Daniel Lezcano]

The routine doing trip point crossing the way up or down is actually
wrong.

A trip point is composed with a trip temperature and a hysteresis.

The trip temperature is used to detect when the trip point is crossed
the way up.

The trip temperature minus the hysteresis is used to detect when the
trip point is crossed the way down.

|-----------low--------high------------|
             |<--------->|
             |    hyst   |
             |           |
             |          -|--> crossed the way up
             |
         <---|-- crossed the way down

For that, there is a two point comparison: the current temperature and
the previous temperature.

The actual code assumes if the current temperature is greater than the
trip temperature and the previous temperature was lesser, then the
trip point is crossed the way up. That is true only if we crossed the
way down the low temperature boundary from the previous temperature or
if the hysteresis is zero. The temperature can decrease between the
low and high, so the trip point is not crossed the way down and then
increase again and cross the high temperature raising a new trip point
crossed detection which is incorrect. The same scenario happens when
crossing the way down.

The trip point crossing the way up and down must act as parenthesis, a
trip point down must close a trip point up. Today we have multiple
trip point up without the corresponding trip point down.

In order to fix that, we store the previous trip point which gives the
information about the previous trip and we change the trip point
browsing order depending on the temperature trend: in the ascending
order when the temperature trend is raising, otherwise in the
descending order.

As a sidenote, the thermal_zone_device structure has already the
prev_trip_low and prev_trip_high information which are used by the
thermal_zone_set_trips() function. This one can be changed to be
triggered by the trip temperature crossing function, which makes more
sense, and the two fields will disappear.

Tested on a rk3399-rock960 with thermal stress and 4 trip points. Also
tested with temperature emulation to create a temperature jump
directly to the second trip point.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
V2:
   - As spotted by Zhang Rui, the trip cross notification does not
  work if the temperature drops and crosses two trip points in the
  same update interval. In order to fix that, we browse the trip point
  in the ascending order when the temperature trend is raising,
  otherwise in the descending order.
---
 drivers/thermal/thermal_core.c | 46 +++++++++++++++++++++++++---------
 include/linux/thermal.h        |  2 ++
 2 files changed, 36 insertions(+), 12 deletions(-)

diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index f66036b3daae..89926e029378 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -357,19 +357,35 @@ static void handle_critical_trips(struct thermal_zone_device *tz,
 static void handle_thermal_trip_crossed(struct thermal_zone_device *tz, int trip,
 					int trip_temp, int trip_hyst, enum thermal_trip_type trip_type)
 {
+	int trip_low_temp = trip_temp - trip_hyst;
+
 	if (tz->last_temperature == THERMAL_TEMP_INVALID)
 		return;
 
-	if (tz->last_temperature < trip_temp &&
-	    tz->temperature >= trip_temp) {
-		thermal_notify_tz_trip_up(tz->id, trip,
-					  tz->temperature);
-	}
-
-	if (tz->last_temperature >= trip_temp &&
-	    tz->temperature < (trip_temp - trip_hyst)) {
-		thermal_notify_tz_trip_down(tz->id, trip,
-					    tz->temperature);
+	/*
+	 * Due to the hysteresis, a third information is needed to
+	 * detect when the temperature is wavering between the
+	 * trip_low_temp and the trip_temp. A trip point is crossed
+	 * the way up only if the temperature is above it while the
+	 * previous temperature was below *and* we crossed the
+	 * trip_temp_low before. The previous trip point give us the
+	 * previous trip point transition. The similar problem exists
+	 * when crossing the way down.
+	 *
+	 * Note the mechanism works only if the caller of the function
+	 * invoke the function with the trip point ascending or
+	 * descending regarding the temperature trend. A temperature
+	 * drop trend will browse the trip point in the descending
+	 * order
+	 */
+	if (tz->last_temperature < trip_temp && tz->temperature >= trip_temp &&
+	    trip != tz->prev_trip) {
+		thermal_notify_tz_trip_up(tz->id, trip, tz->temperature);
+		tz->prev_trip = trip;
+	} else if (tz->last_temperature >= trip_low_temp && tz->temperature < trip_low_temp &&
+		   trip == tz->prev_trip) {
+		thermal_notify_tz_trip_down(tz->id, trip, tz->temperature);
+		tz->prev_trip = trip - 1;
 	}
 }
 
@@ -427,6 +443,7 @@ static void thermal_zone_device_init(struct thermal_zone_device *tz)
 {
 	struct thermal_instance *pos;
 	tz->temperature = THERMAL_TEMP_INVALID;
+	tz->prev_trip = -1;
 	tz->prev_low_trip = -INT_MAX;
 	tz->prev_high_trip = INT_MAX;
 	list_for_each_entry(pos, &tz->thermal_instances, tz_node)
@@ -511,8 +528,13 @@ void thermal_zone_device_update(struct thermal_zone_device *tz,
 
 	tz->notify_event = event;
 
-	for (count = 0; count < tz->trips; count++)
-		handle_thermal_trip(tz, count);
+	if (tz->last_temperature <= tz->temperature) {
+		for (count = 0; count < tz->trips; count++)
+			handle_thermal_trip(tz, count);
+	} else {
+		for (count = tz->prev_trip; count >= 0; count--)
+			handle_thermal_trip(tz, count);
+	}
 }
 EXPORT_SYMBOL_GPL(thermal_zone_device_update);
 
diff --git a/include/linux/thermal.h b/include/linux/thermal.h
index 231bac2768fb..5b3bfb902d10 100644
--- a/include/linux/thermal.h
+++ b/include/linux/thermal.h
@@ -124,6 +124,7 @@ struct thermal_cooling_device {
  * @last_temperature:	previous temperature read
  * @emul_temperature:	emulated temperature when using CONFIG_THERMAL_EMULATION
  * @passive:		1 if you've crossed a passive trip point, 0 otherwise.
+ * @prev_trip:		previous trip point the thermal zone was, -1 if below all of them
  * @prev_low_trip:	the low current temperature if you've crossed a passive
 			trip point.
  * @prev_high_trip:	the above current temperature if you've crossed a
@@ -159,6 +160,7 @@ struct thermal_zone_device {
 	int last_temperature;
 	int emul_temperature;
 	int passive;
+	int prev_trip;
 	int prev_low_trip;
 	int prev_high_trip;
 	atomic_t need_update;
-- 
2.25.1

Re: [PATCH v2 1/3] thermal/core: Encapsulate the trip point crossed function

[Daniel Lezcano]

On 08/07/2022 20:32, Daniel Lezcano wrote:
> The routine where the trip point crossed is detected is a strategic
> place where different processing will happen. Encapsulate the code in
> a function, so all specific actions related with a trip point crossed
> can be grouped.
> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---

Is it ok if I pick this series?


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH v2 1/3] thermal/core: Encapsulate the trip point crossed function

[Lukasz Luba]

On 7/8/22 19:32, Daniel Lezcano wrote:
> The routine where the trip point crossed is detected is a strategic
> place where different processing will happen. Encapsulate the code in
> a function, so all specific actions related with a trip point crossed
> can be grouped.
> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>   drivers/thermal/thermal_core.c | 30 ++++++++++++++++++++----------
>   1 file changed, 20 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> index cdc0552e8c42..d9f771b15ed8 100644
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -358,6 +358,25 @@ static void handle_critical_trips(struct thermal_zone_device *tz,
>   		tz->ops->critical(tz);
>   }
>   
> +static void handle_thermal_trip_crossed(struct thermal_zone_device *tz, int trip,
> +					int trip_temp, int trip_hyst, enum thermal_trip_type trip_type)
> +{
> +	if (tz->last_temperature == THERMAL_TEMP_INVALID)
> +		return;
> +
> +	if (tz->last_temperature < trip_temp &&
> +	    tz->temperature >= trip_temp) {
> +		thermal_notify_tz_trip_up(tz->id, trip,
> +					  tz->temperature);
> +	}
> +
> +	if (tz->last_temperature >= trip_temp &&
> +	    tz->temperature < (trip_temp - trip_hyst)) {
> +		thermal_notify_tz_trip_down(tz->id, trip,
> +					    tz->temperature);
> +	}
> +}
> +
>   static void handle_thermal_trip(struct thermal_zone_device *tz, int trip)
>   {
>   	enum thermal_trip_type type;
> @@ -372,16 +391,7 @@ static void handle_thermal_trip(struct thermal_zone_device *tz, int trip)
>   	if (tz->ops->get_trip_hyst)
>   		tz->ops->get_trip_hyst(tz, trip, &hyst);
>   
> -	if (tz->last_temperature != THERMAL_TEMP_INVALID) {
> -		if (tz->last_temperature < trip_temp &&
> -		    tz->temperature >= trip_temp)
> -			thermal_notify_tz_trip_up(tz->id, trip,
> -						  tz->temperature);
> -		if (tz->last_temperature >= trip_temp &&
> -		    tz->temperature < (trip_temp - hyst))
> -			thermal_notify_tz_trip_down(tz->id, trip,
> -						    tz->temperature);
> -	}
> +	handle_thermal_trip_crossed(tz, trip, trip_temp, hyst, type);
>   
>   	if (type == THERMAL_TRIP_CRITICAL || type == THERMAL_TRIP_HOT)
>   		handle_critical_trips(tz, trip, type);


LGTM

Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>

Re: [PATCH v2 2/3] thermal/core: Passing a parameter instead of calling the function again

[Lukasz Luba]

On 7/8/22 19:32, Daniel Lezcano wrote:
> As the trip temperature is already available when calling the
> function, pass it as a parameter instead of having the function
> calling the ops again to retrieve the same data.
> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>   drivers/thermal/thermal_core.c | 8 ++------
>   1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> index d9f771b15ed8..f66036b3daae 100644
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -340,12 +340,8 @@ void thermal_zone_device_critical(struct thermal_zone_device *tz)
>   EXPORT_SYMBOL(thermal_zone_device_critical);
>   
>   static void handle_critical_trips(struct thermal_zone_device *tz,
> -				  int trip, enum thermal_trip_type trip_type)
> +				  int trip, int trip_temp, enum thermal_trip_type trip_type)
>   {
> -	int trip_temp;
> -
> -	tz->ops->get_trip_temp(tz, trip, &trip_temp);
> -
>   	/* If we have not crossed the trip_temp, we do not care. */
>   	if (trip_temp <= 0 || tz->temperature < trip_temp)
>   		return;
> @@ -394,7 +390,7 @@ static void handle_thermal_trip(struct thermal_zone_device *tz, int trip)
>   	handle_thermal_trip_crossed(tz, trip, trip_temp, hyst, type);
>   
>   	if (type == THERMAL_TRIP_CRITICAL || type == THERMAL_TRIP_HOT)
> -		handle_critical_trips(tz, trip, type);
> +		handle_critical_trips(tz, trip, trip_temp, type);
>   	else
>   		handle_non_critical_trips(tz, trip);
>   	/*

Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>

Re: [PATCH v2 3/3] thermal/core: Fix thermal trip cross point

[Lukasz Luba]

On 7/8/22 19:32, Daniel Lezcano wrote:
> The routine doing trip point crossing the way up or down is actually
> wrong.
> 
> A trip point is composed with a trip temperature and a hysteresis.
> 
> The trip temperature is used to detect when the trip point is crossed
> the way up.
> 
> The trip temperature minus the hysteresis is used to detect when the
> trip point is crossed the way down.
> 
> |-----------low--------high------------|
>               |<--------->|
>               |    hyst   |
>               |           |
>               |          -|--> crossed the way up
>               |
>           <---|-- crossed the way down
> 
> For that, there is a two point comparison: the current temperature and
> the previous temperature.
> 
> The actual code assumes if the current temperature is greater than the
> trip temperature and the previous temperature was lesser, then the
> trip point is crossed the way up. That is true only if we crossed the
> way down the low temperature boundary from the previous temperature or
> if the hysteresis is zero. The temperature can decrease between the
> low and high, so the trip point is not crossed the way down and then
> increase again and cross the high temperature raising a new trip point
> crossed detection which is incorrect. The same scenario happens when
> crossing the way down.
> 
> The trip point crossing the way up and down must act as parenthesis, a
> trip point down must close a trip point up. Today we have multiple
> trip point up without the corresponding trip point down.
> 
> In order to fix that, we store the previous trip point which gives the
> information about the previous trip and we change the trip point
> browsing order depending on the temperature trend: in the ascending
> order when the temperature trend is raising, otherwise in the
> descending order.
> 
> As a sidenote, the thermal_zone_device structure has already the
> prev_trip_low and prev_trip_high information which are used by the
> thermal_zone_set_trips() function. This one can be changed to be
> triggered by the trip temperature crossing function, which makes more
> sense, and the two fields will disappear.
> 
> Tested on a rk3399-rock960 with thermal stress and 4 trip points. Also
> tested with temperature emulation to create a temperature jump
> directly to the second trip point.
> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
> V2:
>     - As spotted by Zhang Rui, the trip cross notification does not
>    work if the temperature drops and crosses two trip points in the
>    same update interval. In order to fix that, we browse the trip point
>    in the ascending order when the temperature trend is raising,
>    otherwise in the descending order.
> ---
>   drivers/thermal/thermal_core.c | 46 +++++++++++++++++++++++++---------
>   include/linux/thermal.h        |  2 ++
>   2 files changed, 36 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> index f66036b3daae..89926e029378 100644
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -357,19 +357,35 @@ static void handle_critical_trips(struct thermal_zone_device *tz,
>   static void handle_thermal_trip_crossed(struct thermal_zone_device *tz, int trip,
>   					int trip_temp, int trip_hyst, enum thermal_trip_type trip_type)
>   {
> +	int trip_low_temp = trip_temp - trip_hyst;
> +
>   	if (tz->last_temperature == THERMAL_TEMP_INVALID)
>   		return;
>   
> -	if (tz->last_temperature < trip_temp &&
> -	    tz->temperature >= trip_temp) {
> -		thermal_notify_tz_trip_up(tz->id, trip,
> -					  tz->temperature);
> -	}
> -
> -	if (tz->last_temperature >= trip_temp &&
> -	    tz->temperature < (trip_temp - trip_hyst)) {
> -		thermal_notify_tz_trip_down(tz->id, trip,
> -					    tz->temperature);
> +	/*
> +	 * Due to the hysteresis, a third information is needed to
> +	 * detect when the temperature is wavering between the
> +	 * trip_low_temp and the trip_temp. A trip point is crossed
> +	 * the way up only if the temperature is above it while the
> +	 * previous temperature was below *and* we crossed the
> +	 * trip_temp_low before. The previous trip point give us the
> +	 * previous trip point transition. The similar problem exists
> +	 * when crossing the way down.
> +	 *
> +	 * Note the mechanism works only if the caller of the function
> +	 * invoke the function with the trip point ascending or
> +	 * descending regarding the temperature trend. A temperature
> +	 * drop trend will browse the trip point in the descending
> +	 * order
> +	 */
> +	if (tz->last_temperature < trip_temp && tz->temperature >= trip_temp &&
> +	    trip != tz->prev_trip) {
> +		thermal_notify_tz_trip_up(tz->id, trip, tz->temperature);
> +		tz->prev_trip = trip;
> +	} else if (tz->last_temperature >= trip_low_temp && tz->temperature < trip_low_temp &&
> +		   trip == tz->prev_trip) {
> +		thermal_notify_tz_trip_down(tz->id, trip, tz->temperature);
> +		tz->prev_trip = trip - 1;
>   	}
>   }
>   
> @@ -427,6 +443,7 @@ static void thermal_zone_device_init(struct thermal_zone_device *tz)
>   {
>   	struct thermal_instance *pos;
>   	tz->temperature = THERMAL_TEMP_INVALID;
> +	tz->prev_trip = -1;
>   	tz->prev_low_trip = -INT_MAX;
>   	tz->prev_high_trip = INT_MAX;
>   	list_for_each_entry(pos, &tz->thermal_instances, tz_node)
> @@ -511,8 +528,13 @@ void thermal_zone_device_update(struct thermal_zone_device *tz,
>   
>   	tz->notify_event = event;
>   
> -	for (count = 0; count < tz->trips; count++)
> -		handle_thermal_trip(tz, count);
> +	if (tz->last_temperature <= tz->temperature) {
> +		for (count = 0; count < tz->trips; count++)
> +			handle_thermal_trip(tz, count);
> +	} else {
> +		for (count = tz->prev_trip; count >= 0; count--)
> +			handle_thermal_trip(tz, count);
> +	}

In general the code look good. I have one question, though:
Is it always true that these trip points coming from the DT
and parsed in thermal_of_build_thermal_zone() populated by
     for_each_child_of_node(child, gchild) {
          thermal_of_populate_trip(gchild, &tz->trips[i++]);

are always defined in right order in DT?

Re: [PATCH v2 3/3] thermal/core: Fix thermal trip cross point

[Daniel Lezcano]

On 12/07/2022 13:29, Lukasz Luba wrote:

[ ... ]

>> @@ -511,8 +528,13 @@ void thermal_zone_device_update(struct 
>> thermal_zone_device *tz,
>>       tz->notify_event = event;
>> -    for (count = 0; count < tz->trips; count++)
>> -        handle_thermal_trip(tz, count);
>> +    if (tz->last_temperature <= tz->temperature) {
>> +        for (count = 0; count < tz->trips; count++)
>> +            handle_thermal_trip(tz, count);
>> +    } else {
>> +        for (count = tz->prev_trip; count >= 0; count--)
>> +            handle_thermal_trip(tz, count);
>> +    }
> 
> In general the code look good. I have one question, though:
> Is it always true that these trip points coming from the DT
> and parsed in thermal_of_build_thermal_zone() populated by
>      for_each_child_of_node(child, gchild) {
>           thermal_of_populate_trip(gchild, &tz->trips[i++]);
> 
> are always defined in right order in DT?

Hmm, that is a good question. Even if the convention is to put the trip 
point in the ascending order, I don't find any documentation telling it 
is mandatory. Given that I don't feel particularly comfortable to assume 
that is the case.

Perhaps, it would make more sense to build a map of indexes telling the 
order in the trip points and work with it instead.


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH v2 3/3] thermal/core: Fix thermal trip cross point

[Lukasz Luba]

On 7/12/22 13:30, Daniel Lezcano wrote:
> On 12/07/2022 13:29, Lukasz Luba wrote:
> 
> [ ... ]
> 
>>> @@ -511,8 +528,13 @@ void thermal_zone_device_update(struct 
>>> thermal_zone_device *tz,
>>>       tz->notify_event = event;
>>> -    for (count = 0; count < tz->trips; count++)
>>> -        handle_thermal_trip(tz, count);
>>> +    if (tz->last_temperature <= tz->temperature) {
>>> +        for (count = 0; count < tz->trips; count++)
>>> +            handle_thermal_trip(tz, count);
>>> +    } else {
>>> +        for (count = tz->prev_trip; count >= 0; count--)
>>> +            handle_thermal_trip(tz, count);
>>> +    }
>>
>> In general the code look good. I have one question, though:
>> Is it always true that these trip points coming from the DT
>> and parsed in thermal_of_build_thermal_zone() populated by
>>      for_each_child_of_node(child, gchild) {
>>           thermal_of_populate_trip(gchild, &tz->trips[i++]);
>>
>> are always defined in right order in DT?
> 
> Hmm, that is a good question. Even if the convention is to put the trip 
> point in the ascending order, I don't find any documentation telling it 
> is mandatory. Given that I don't feel particularly comfortable to assume 
> that is the case.
> 
> Perhaps, it would make more sense to build a map of indexes telling the 
> order in the trip points and work with it instead.
> 
> 

Sounds a reliable way to move forward. Maybe you could just sort in the
right order those trip points in the thermal_of_build_thermal_zone()
in an additional patch to this series?
Than this patch could stay as is, because it looks good.

Re: [PATCH v2 3/3] thermal/core: Fix thermal trip cross point

[Daniel Lezcano]

On 12/07/2022 14:40, Lukasz Luba wrote:
> 
> 
> On 7/12/22 13:30, Daniel Lezcano wrote:
>> On 12/07/2022 13:29, Lukasz Luba wrote:
>>
>> [ ... ]
>>
>>>> @@ -511,8 +528,13 @@ void thermal_zone_device_update(struct 
>>>> thermal_zone_device *tz,
>>>>       tz->notify_event = event;
>>>> -    for (count = 0; count < tz->trips; count++)
>>>> -        handle_thermal_trip(tz, count);
>>>> +    if (tz->last_temperature <= tz->temperature) {
>>>> +        for (count = 0; count < tz->trips; count++)
>>>> +            handle_thermal_trip(tz, count);
>>>> +    } else {
>>>> +        for (count = tz->prev_trip; count >= 0; count--)
>>>> +            handle_thermal_trip(tz, count);
>>>> +    }
>>>
>>> In general the code look good. I have one question, though:
>>> Is it always true that these trip points coming from the DT
>>> and parsed in thermal_of_build_thermal_zone() populated by
>>>      for_each_child_of_node(child, gchild) {
>>>           thermal_of_populate_trip(gchild, &tz->trips[i++]);
>>>
>>> are always defined in right order in DT?
>>
>> Hmm, that is a good question. Even if the convention is to put the 
>> trip point in the ascending order, I don't find any documentation 
>> telling it is mandatory. Given that I don't feel particularly 
>> comfortable to assume that is the case.
>>
>> Perhaps, it would make more sense to build a map of indexes telling 
>> the order in the trip points and work with it instead.
>>
>>
> 
> Sounds a reliable way to move forward. Maybe you could just sort in the
> right order those trip points in the thermal_of_build_thermal_zone()
> in an additional patch to this series?
> Than this patch could stay as is, because it looks go

Unfortunately, there is the manual setup as well as the ACPI.



-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH v2 3/3] thermal/core: Fix thermal trip cross point

[Lukasz Luba]

On 7/12/22 14:06, Daniel Lezcano wrote:
> On 12/07/2022 14:40, Lukasz Luba wrote:
>>
>>
>> On 7/12/22 13:30, Daniel Lezcano wrote:
>>> On 12/07/2022 13:29, Lukasz Luba wrote:
>>>
>>> [ ... ]
>>>
>>>>> @@ -511,8 +528,13 @@ void thermal_zone_device_update(struct 
>>>>> thermal_zone_device *tz,
>>>>>       tz->notify_event = event;
>>>>> -    for (count = 0; count < tz->trips; count++)
>>>>> -        handle_thermal_trip(tz, count);
>>>>> +    if (tz->last_temperature <= tz->temperature) {
>>>>> +        for (count = 0; count < tz->trips; count++)
>>>>> +            handle_thermal_trip(tz, count);
>>>>> +    } else {
>>>>> +        for (count = tz->prev_trip; count >= 0; count--)
>>>>> +            handle_thermal_trip(tz, count);
>>>>> +    }
>>>>
>>>> In general the code look good. I have one question, though:
>>>> Is it always true that these trip points coming from the DT
>>>> and parsed in thermal_of_build_thermal_zone() populated by
>>>>      for_each_child_of_node(child, gchild) {
>>>>           thermal_of_populate_trip(gchild, &tz->trips[i++]);
>>>>
>>>> are always defined in right order in DT?
>>>
>>> Hmm, that is a good question. Even if the convention is to put the 
>>> trip point in the ascending order, I don't find any documentation 
>>> telling it is mandatory. Given that I don't feel particularly 
>>> comfortable to assume that is the case.
>>>
>>> Perhaps, it would make more sense to build a map of indexes telling 
>>> the order in the trip points and work with it instead.
>>>
>>>
>>
>> Sounds a reliable way to move forward. Maybe you could just sort in the
>> right order those trip points in the thermal_of_build_thermal_zone()
>> in an additional patch to this series?
>> Than this patch could stay as is, because it looks go
> 
> Unfortunately, there is the manual setup as well as the ACPI.
> 
> 
> 

I see. OK, so continue to solve it completely. I can review your next
version.

Regards,
Lukasz

Re: [PATCH v2 2/3] thermal/core: Passing a parameter instead of calling the function again

[Rafael J. Wysocki]

On Fri, Jul 8, 2022 at 8:32 PM Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>
> As the trip temperature is already available when calling the
> function,

Which function?

I would change the subject to something like "Avoid calling
->get_trip_temp() unnecessarily" and then use the specific function
name in the changelog.

The changes themselves LGTM.

> pass it as a parameter instead of having the function
> calling the ops again to retrieve the same data.
>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  drivers/thermal/thermal_core.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> index d9f771b15ed8..f66036b3daae 100644
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -340,12 +340,8 @@ void thermal_zone_device_critical(struct thermal_zone_device *tz)
>  EXPORT_SYMBOL(thermal_zone_device_critical);
>
>  static void handle_critical_trips(struct thermal_zone_device *tz,
> -                                 int trip, enum thermal_trip_type trip_type)
> +                                 int trip, int trip_temp, enum thermal_trip_type trip_type)
>  {
> -       int trip_temp;
> -
> -       tz->ops->get_trip_temp(tz, trip, &trip_temp);
> -
>         /* If we have not crossed the trip_temp, we do not care. */
>         if (trip_temp <= 0 || tz->temperature < trip_temp)
>                 return;
> @@ -394,7 +390,7 @@ static void handle_thermal_trip(struct thermal_zone_device *tz, int trip)
>         handle_thermal_trip_crossed(tz, trip, trip_temp, hyst, type);
>
>         if (type == THERMAL_TRIP_CRITICAL || type == THERMAL_TRIP_HOT)
> -               handle_critical_trips(tz, trip, type);
> +               handle_critical_trips(tz, trip, trip_temp, type);
>         else
>                 handle_non_critical_trips(tz, trip);
>         /*
> --
> 2.25.1
>