The driver browses the trip point to find out the critical trip
temperature. However the function thermal_zone_get_crit_temp() does
already that, so the routine is pointless in the driver.
Use thermal_zone_get_crit_temp() instead of inspecting all the trip
points.
Signed-off-by: Daniel Lezcano <[email protected]>
---
drivers/thermal/armada_thermal.c | 38 +++++++++++++-------------------
1 file changed, 15 insertions(+), 23 deletions(-)
diff --git a/drivers/thermal/armada_thermal.c b/drivers/thermal/armada_thermal.c
index db040dbdaa0a..c6d51d8acbf0 100644
--- a/drivers/thermal/armada_thermal.c
+++ b/drivers/thermal/armada_thermal.c
@@ -784,34 +784,26 @@ static int armada_configure_overheat_int(struct armada_thermal_priv *priv,
int sensor_id)
{
/* Retrieve the critical trip point to enable the overheat interrupt */
- struct thermal_trip trip;
+ int temperature;
int ret;
- int i;
-
- for (i = 0; i < thermal_zone_get_num_trips(tz); i++) {
-
- ret = thermal_zone_get_trip(tz, i, &trip);
- if (ret)
- return ret;
-
- if (trip.type != THERMAL_TRIP_CRITICAL)
- continue;
-
- ret = armada_select_channel(priv, sensor_id);
- if (ret)
- return ret;
- armada_set_overheat_thresholds(priv, trip.temperature,
- trip.hysteresis);
- priv->overheat_sensor = tz;
- priv->interrupt_source = sensor_id;
+ ret = thermal_zone_get_crit_temp(tz, &temperature);
+ if (ret)
+ return ret;
- armada_enable_overheat_interrupt(priv);
+ ret = armada_select_channel(priv, sensor_id);
+ if (ret)
+ return ret;
- return 0;
- }
+ /*
+ * A critical temperature does not have a hysteresis
+ */
+ armada_set_overheat_thresholds(priv, temperature, 0);
+ priv->overheat_sensor = tz;
+ priv->interrupt_source = sensor_id;
+ armada_enable_overheat_interrupt(priv);
- return -EINVAL;
+ return 0;
}
static int armada_thermal_probe(struct platform_device *pdev)
--
2.34.1
Hi Daniel,
[email protected] wrote on Wed, 18 Jan 2023 23:26:10 +0100:
> The driver browses the trip point to find out the critical trip
> temperature. However the function thermal_zone_get_crit_temp() does
> already that, so the routine is pointless in the driver.
>
> Use thermal_zone_get_crit_temp() instead of inspecting all the trip
> points.
>
> Signed-off-by: Daniel Lezcano <[email protected]>
> ---
> drivers/thermal/armada_thermal.c | 38 +++++++++++++-------------------
> 1 file changed, 15 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/thermal/armada_thermal.c b/drivers/thermal/armada_thermal.c
> index db040dbdaa0a..c6d51d8acbf0 100644
> --- a/drivers/thermal/armada_thermal.c
> +++ b/drivers/thermal/armada_thermal.c
> @@ -784,34 +784,26 @@ static int armada_configure_overheat_int(struct armada_thermal_priv *priv,
> int sensor_id)
> {
> /* Retrieve the critical trip point to enable the overheat interrupt */
> - struct thermal_trip trip;
> + int temperature;
> int ret;
> - int i;
> -
> - for (i = 0; i < thermal_zone_get_num_trips(tz); i++) {
> -
> - ret = thermal_zone_get_trip(tz, i, &trip);
> - if (ret)
> - return ret;
> -
> - if (trip.type != THERMAL_TRIP_CRITICAL)
> - continue;
> -
> - ret = armada_select_channel(priv, sensor_id);
> - if (ret)
> - return ret;
>
> - armada_set_overheat_thresholds(priv, trip.temperature,
> - trip.hysteresis);
> - priv->overheat_sensor = tz;
> - priv->interrupt_source = sensor_id;
> + ret = thermal_zone_get_crit_temp(tz, &temperature);
> + if (ret)
> + return ret;
>
> - armada_enable_overheat_interrupt(priv);
> + ret = armada_select_channel(priv, sensor_id);
> + if (ret)
> + return ret;
>
> - return 0;
> - }
> + /*
> + * A critical temperature does not have a hysteresis
> + */
Makes sense.
Nit: I would actually put that comment in the commit log rather than
keeping it in the code, but whatever, that's a nice simplification.
> + armada_set_overheat_thresholds(priv, temperature, 0);
> + priv->overheat_sensor = tz;
> + priv->interrupt_source = sensor_id;
> + armada_enable_overheat_interrupt(priv);
>
> - return -EINVAL;
> + return 0;
> }
>
> static int armada_thermal_probe(struct platform_device *pdev)
LGTM so,
Reviewed-by: Miquel Raynal <[email protected]>
Thanks,
Miquèl
Hi Miquel,
On 24/01/2023 12:04, Miquel Raynal wrote:
> Hi Daniel,
>
> [email protected] wrote on Wed, 18 Jan 2023 23:26:10 +0100:
>
>> The driver browses the trip point to find out the critical trip
>> temperature. However the function thermal_zone_get_crit_temp() does
>> already that, so the routine is pointless in the driver.
>>
>> Use thermal_zone_get_crit_temp() instead of inspecting all the trip
>> points.
>>
>> Signed-off-by: Daniel Lezcano <[email protected]>
>> ---
>> drivers/thermal/armada_thermal.c | 38 +++++++++++++-------------------
[ ... ]
>
> Makes sense.
>
> Nit: I would actually put that comment in the commit log rather than
> keeping it in the code, but whatever, that's a nice simplification.
Ok, I'll do the change.
>> + armada_set_overheat_thresholds(priv, temperature, 0);
>> + priv->overheat_sensor = tz;
>> + priv->interrupt_source = sensor_id;
>> + armada_enable_overheat_interrupt(priv);
>>
>> - return -EINVAL;
>> + return 0;
>> }
>>
>> static int armada_thermal_probe(struct platform_device *pdev)
>
> LGTM so,
>
> Reviewed-by: Miquel Raynal <[email protected]>
Thanks for the review
-- Daniel
--
<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
On 24/01/2023 12:04, Miquel Raynal wrote:
> Hi Daniel,
>
> [email protected] wrote on Wed, 18 Jan 2023 23:26:10 +0100:
>
>> The driver browses the trip point to find out the critical trip
>> temperature. However the function thermal_zone_get_crit_temp() does
>> already that, so the routine is pointless in the driver.
>>
>> Use thermal_zone_get_crit_temp() instead of inspecting all the trip
>> points.
>>
>> Signed-off-by: Daniel Lezcano <[email protected]>
>> ---
>> drivers/thermal/armada_thermal.c | 38 +++++++++++++-------------------
>> 1 file changed, 15 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/thermal/armada_thermal.c b/drivers/thermal/armada_thermal.c
>> index db040dbdaa0a..c6d51d8acbf0 100644
>> --- a/drivers/thermal/armada_thermal.c
>> +++ b/drivers/thermal/armada_thermal.c
>> @@ -784,34 +784,26 @@ static int armada_configure_overheat_int(struct armada_thermal_priv *priv,
>> int sensor_id)
>> {
>> /* Retrieve the critical trip point to enable the overheat interrupt */
>> - struct thermal_trip trip;
>> + int temperature;
>> int ret;
>> - int i;
>> -
>> - for (i = 0; i < thermal_zone_get_num_trips(tz); i++) {
>> -
>> - ret = thermal_zone_get_trip(tz, i, &trip);
>> - if (ret)
>> - return ret;
>> -
>> - if (trip.type != THERMAL_TRIP_CRITICAL)
>> - continue;
>> -
>> - ret = armada_select_channel(priv, sensor_id);
>> - if (ret)
>> - return ret;
>>
>> - armada_set_overheat_thresholds(priv, trip.temperature,
>> - trip.hysteresis);
>> - priv->overheat_sensor = tz;
>> - priv->interrupt_source = sensor_id;
>> + ret = thermal_zone_get_crit_temp(tz, &temperature);
>> + if (ret)
>> + return ret;
>>
>> - armada_enable_overheat_interrupt(priv);
>> + ret = armada_select_channel(priv, sensor_id);
>> + if (ret)
>> + return ret;
>>
>> - return 0;
>> - }
>> + /*
>> + * A critical temperature does not have a hysteresis
>> + */
>
> Makes sense.
>
> Nit: I would actually put that comment in the commit log rather than
> keeping it in the code, but whatever, that's a nice simplification.
Oh, actually, I added the comment because of the third parameter change
for armada_set_overheat_thresholds(..., 0);
>> + armada_set_overheat_thresholds(priv, temperature, 0);
I think it makes sense to keep the comment to clarify why we pass this
zero temperature argument.
--
<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
Hi Daniel,
[email protected] wrote on Tue, 24 Jan 2023 12:36:22 +0100:
> On 24/01/2023 12:04, Miquel Raynal wrote:
> > Hi Daniel,
> >
> > [email protected] wrote on Wed, 18 Jan 2023 23:26:10 +0100:
> >
> >> The driver browses the trip point to find out the critical trip
> >> temperature. However the function thermal_zone_get_crit_temp() does
> >> already that, so the routine is pointless in the driver.
> >>
> >> Use thermal_zone_get_crit_temp() instead of inspecting all the trip
> >> points.
> >>
> >> Signed-off-by: Daniel Lezcano <[email protected]>
> >> ---
> >> drivers/thermal/armada_thermal.c | 38 +++++++++++++-------------------
> >> 1 file changed, 15 insertions(+), 23 deletions(-)
> >>
> >> diff --git a/drivers/thermal/armada_thermal.c b/drivers/thermal/armada_thermal.c
> >> index db040dbdaa0a..c6d51d8acbf0 100644
> >> --- a/drivers/thermal/armada_thermal.c
> >> +++ b/drivers/thermal/armada_thermal.c
> >> @@ -784,34 +784,26 @@ static int armada_configure_overheat_int(struct armada_thermal_priv *priv,
> >> int sensor_id)
> >> {
> >> /* Retrieve the critical trip point to enable the overheat interrupt */
> >> - struct thermal_trip trip;
> >> + int temperature;
> >> int ret;
> >> - int i;
> >> -
> >> - for (i = 0; i < thermal_zone_get_num_trips(tz); i++) {
> >> -
> >> - ret = thermal_zone_get_trip(tz, i, &trip);
> >> - if (ret)
> >> - return ret;
> >> -
> >> - if (trip.type != THERMAL_TRIP_CRITICAL)
> >> - continue;
> >> -
> >> - ret = armada_select_channel(priv, sensor_id);
> >> - if (ret)
> >> - return ret;
> >> >> - armada_set_overheat_thresholds(priv, trip.temperature,
> >> - trip.hysteresis);
> >> - priv->overheat_sensor = tz;
> >> - priv->interrupt_source = sensor_id;
> >> + ret = thermal_zone_get_crit_temp(tz, &temperature);
> >> + if (ret)
> >> + return ret;
> >> >> - armada_enable_overheat_interrupt(priv);
> >> + ret = armada_select_channel(priv, sensor_id);
> >> + if (ret)
> >> + return ret;
> >> >> - return 0;
> >> - }
> >> + /*
> >> + * A critical temperature does not have a hysteresis
> >> + */
> >
> > Makes sense.
> >
> > Nit: I would actually put that comment in the commit log rather than
> > keeping it in the code, but whatever, that's a nice simplification.
>
> Oh, actually, I added the comment because of the third parameter change for armada_set_overheat_thresholds(..., 0);
Yes, that's why I proposed to mention it in the commit log rather than
in the code. If having no threshold here makes sense (and it does), I
don't see the point in explaining it in a comment. I usually use
comments to explain what could appear strange/unclear when looking at
the code.
Here, while the parameter change is intended and legitimate, I feel
like it is worth justifying, but doing so in the commit logs feels
enough.
But that's minor tbh, so feel free to do it like you prefer.
> >> + armada_set_overheat_thresholds(priv, temperature, 0);
>
> I think it makes sense to keep the comment to clarify why we pass this zero temperature argument.
Thanks,
Miquèl