Re: [PATCH v2 1/2] thermal: rcar_gen3_thermal: Add support for hardware trip points

["Niklas Söderlund" <niklas.soderlund+renesas@ragnatech.se>]

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@ragnatech.se>
> > ---
> > * 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