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?
--
Kind Regards,
Niklas Söderlund
