Messages in this thread | | | Date | Tue, 24 Jan 2023 12:50:27 +0100 | From | Miquel Raynal <> | Subject | Re: [PATCH] thermal/drivers/armada: Use the thermal_zone_get_crit_temp() |
| |
Hi Daniel,
daniel.lezcano@linaro.org wrote on Tue, 24 Jan 2023 12:36:22 +0100:
> On 24/01/2023 12:04, Miquel Raynal wrote: > > Hi Daniel, > > > > daniel.lezcano@linaro.org 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 <daniel.lezcano@linaro.org> > >> --- > >> 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
| |