* [PATCH] thermal/drivers/armada: Use the thermal_zone_get_crit_temp() @ 2023-01-18 22:26 Daniel Lezcano 2023-01-24 11:04 ` Miquel Raynal 0 siblings, 1 reply; 5+ messages in thread From: Daniel Lezcano @ 2023-01-18 22:26 UTC (permalink / raw) To: rafael, daniel.lezcano Cc: Miquel Raynal, Amit Kucheria, Zhang Rui, open list:THERMAL, open list 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 + */ + 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] thermal/drivers/armada: Use the thermal_zone_get_crit_temp() 2023-01-18 22:26 [PATCH] thermal/drivers/armada: Use the thermal_zone_get_crit_temp() Daniel Lezcano @ 2023-01-24 11:04 ` Miquel Raynal 2023-01-24 11:31 ` Daniel Lezcano 2023-01-24 11:36 ` Daniel Lezcano 0 siblings, 2 replies; 5+ messages in thread From: Miquel Raynal @ 2023-01-24 11:04 UTC (permalink / raw) To: Daniel Lezcano Cc: rafael, Amit Kucheria, Zhang Rui, open list:THERMAL, open list 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. > + 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 <miquel.raynal@bootlin.com> Thanks, Miquèl ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] thermal/drivers/armada: Use the thermal_zone_get_crit_temp() 2023-01-24 11:04 ` Miquel Raynal @ 2023-01-24 11:31 ` Daniel Lezcano 2023-01-24 11:36 ` Daniel Lezcano 1 sibling, 0 replies; 5+ messages in thread From: Daniel Lezcano @ 2023-01-24 11:31 UTC (permalink / raw) To: Miquel Raynal Cc: rafael, Amit Kucheria, Zhang Rui, open list:THERMAL, open list Hi Miquel, 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 +++++++++++++------------------- [ ... ] > > 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 <miquel.raynal@bootlin.com> 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] thermal/drivers/armada: Use the thermal_zone_get_crit_temp() 2023-01-24 11:04 ` Miquel Raynal 2023-01-24 11:31 ` Daniel Lezcano @ 2023-01-24 11:36 ` Daniel Lezcano 2023-01-24 11:50 ` Miquel Raynal 1 sibling, 1 reply; 5+ messages in thread From: Daniel Lezcano @ 2023-01-24 11:36 UTC (permalink / raw) To: Miquel Raynal Cc: rafael, Amit Kucheria, Zhang Rui, open list:THERMAL, open list 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); >> + 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] thermal/drivers/armada: Use the thermal_zone_get_crit_temp() 2023-01-24 11:36 ` Daniel Lezcano @ 2023-01-24 11:50 ` Miquel Raynal 0 siblings, 0 replies; 5+ messages in thread From: Miquel Raynal @ 2023-01-24 11:50 UTC (permalink / raw) To: Daniel Lezcano Cc: rafael, Amit Kucheria, Zhang Rui, open list:THERMAL, open list 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-01-24 11:50 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-01-18 22:26 [PATCH] thermal/drivers/armada: Use the thermal_zone_get_crit_temp() Daniel Lezcano 2023-01-24 11:04 ` Miquel Raynal 2023-01-24 11:31 ` Daniel Lezcano 2023-01-24 11:36 ` Daniel Lezcano 2023-01-24 11:50 ` Miquel Raynal
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.