Re: [PATCH 3/3] thermal/drivers/intel: Use generic trip points for intel_soc_dts_iosf

[Rafael J. Wysocki]

On Tue, Jan 31, 2023 at 6:03 PM Daniel Lezcano
<daniel.lezcano@xxxxxxxxxx> wrote:
>
>
> Hi Rafael,
>
>
> On 26/01/2023 17:47, Rafael J. Wysocki wrote:
> > On Wed, Jan 18, 2023 at 7:16 PM Daniel Lezcano
> > <daniel.lezcano@xxxxxxxxxx> wrote:
> >>
> >> From: Daniel Lezcano <daniel.lezcano@xxxxxxxxxx>
> >>
> >> The thermal framework gives the possibility to register the trip
> >> points with the thermal zone. When that is done, no get_trip_* ops are
> >> needed and they can be removed.
> >>
> >> Convert ops content logic into generic trip points and register them with the
> >> thermal zone.
> >>
> >> Signed-off-by: Daniel Lezcano <daniel.lezcano@xxxxxxxxxx>
> >> ---
>
> [ ... ]
>
> >> @@ -173,8 +166,13 @@ static int update_trip_temp(struct intel_soc_dts_sensor_entry *dts,
> >>          if (status)
> >>                  goto err_restore_te_out;
> >>
> >> -       dts->trip_types[thres_index] = trip_type;
> >> -
> >> +       status = get_trip_temp(sensors, thres_index, &temp);
> >> +       if (status)
> >> +               goto err_restore_te_out;
> >> +
> >> +       dts->trips[thres_index].type = trip_type;
> >> +       dts->trips[thres_index].temperature = temp;
> >
> > This change doesn't look correct to me, because this function takes
> > temp as an argument and it is used to populate the trip with it at
> > least in some cases.
> >
> > Why should temp be overwritten here?
>
> You are correct. This is wrong.
>
> I think we should call get_trip_temp() before calling update_trip_temp()
> instead of passing a zero temperature parameter

update_trip_temp() is sort of a misnomer, because it is used for
initializing a trip point for example in
intel_soc_dts_iosf_add_read_only_critical_trip() and in this
particular case get_trip_temp() need not be called before it.

This driver seems to be in need of a cleanup.