Hi Niklas,
On 07/07/2022 11:51, Niklas Söderlund wrote:
Hi Daniel,
On 2022-07-06 13:13:44 +0200, Daniel Lezcano wrote:
Hi Niklas,
On 04/08/2021 11:18, Niklas Söderlund wrote:
All supported hardware except V3U is capable of generating interrupts
to the CPU when the temperature go below or above a set value. Use this
to implement support for the set_trip() feature of the thermal core on
supported hardware.
The V3U have its interrupts routed to the ECM module and therefore can
not be used to implement set_trip() as the driver can't be made aware of
when the interrupt triggers.
Each TSC is capable of tracking up-to three different temperatures while
only two are needed to implement the tracking of the thermal window.
Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@xxxxxxxxxxxx>
---
* Changes since v1
- Remove the 'have_irq' flag from the OF match data and auto-detect if
interrupts are available using platform_get_irq_optional().
- Have a non-static thermal_zone_of_device_ops and clear the .set_trips
if interrupts are unavailable.
---
[ ... ]
@@ -401,8 +492,12 @@ static int __maybe_unused rcar_gen3_thermal_resume(struct device *dev)
for (i = 0; i < priv->num_tscs; i++) {
struct rcar_gen3_thermal_tsc *tsc = priv->tscs[i];
+ struct thermal_zone_device *zone = tsc->zone;
priv->thermal_init(tsc);
+ if (zone->ops->set_trips)
+ rcar_gen3_thermal_set_trips(tsc, zone->prev_low_trip,
+ zone->prev_high_trip);
}
While doing a cleanup I lately noticed this change and I've concerns about
it:
- it uses the thermal zone internals
- is it really needed ?
At resume time we have:
thermal_pm_notify()
--> PM_POST_RESTORE
--> thermal_zone_device_update()
--> thermal_zone_set_trips()
In addition, I believe this later call is consistent as it sets the trip
point based on the last temperature update, while the
rcar_gen3_thermal_resume() does not.
Was this function added on purpose because some there is an issue when
resuming the board or just put there assuming it is doing the right thing ?
I would be happy if we can remove this portion of code because it is the
only users of prev_*_trip I would like to replace by prev_trip id.
This looks like something that should never have been submitted
upstream. The usage for this was to restore the trip points in the
hardware registers *after* the hardware have been initialized. However
as far as I can tell from the code this is already done by the thermal
core so no need for the driver to deal with this.
I did a test on a Gen3 board (M3-N) with this code removed and the core
appears to do the right thing so this code in the driver can be removed.
Will you write up a patch as part of your cleanup work or would you
prefer I do it?
Thanks for double checking and confirming. I've a patch removing this
code, no need to send one. I'll submit it along with other changes
around this. Perhaps, I'll try a revert before, it would make more sense.
Do you think the 'revert' should be backported ?
-- 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
