Re: [PATCH v4 04/10] thermal: core: Add thermal_zone_update_trip_temp() helper routine

[Rafael J. Wysocki]

On Mon, Aug 7, 2023 at 1:34 PM Daniel Lezcano <daniel.lezcano@xxxxxxxxxx> wrote:
>
> On 04/08/2023 23:05, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> >
> > Introduce a helper routine called thermal_zone_update_trip_temp() that
> > can be used to update a trip point's temperature with the help of a
> > pointer to local data associated with that trip point provided by
> > the thermal driver that created it.
> >
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> > ---
> >
> > New patch in v4.
> >
> > ---
> >   drivers/thermal/thermal_trip.c |   37 +++++++++++++++++++++++++++++++++++++
> >   include/linux/thermal.h        |    4 ++++
> >   2 files changed, 41 insertions(+)
> >
> > Index: linux-pm/drivers/thermal/thermal_trip.c
> > ===================================================================
> > --- linux-pm.orig/drivers/thermal/thermal_trip.c
> > +++ linux-pm/drivers/thermal/thermal_trip.c
> > @@ -180,3 +180,40 @@ int thermal_zone_set_trip(struct thermal
> >
> >       return 0;
> >   }
> > +
> > +/**
> > + * thermal_zone_update_trip_temp - Update the trip point temperature.
> > + * @tz: Thermal zone.
> > + * @trip_priv: Trip tag.
> > + * @temp: New trip temperature.
> > + *
> > + * This only works for thermal zones using trip tables and its caller must
> > + * ensure that the zone lock is held before using it.
> > + *
> > + * @trip_priv is expected to be the value that has been stored by the driver
> > + * in the struct thermal_trip representing the trip point in question, so it
> > + * can be matched against the value of the priv field in that structure.
> > + *
> > + * If @trip_priv does not match any trip point in the trip table of @tz,
> > + * nothing happens.
> > + */
> > +void thermal_zone_update_trip_temp(struct thermal_zone_device *tz,
> > +                                void *trip_priv, int temperature)
> > +{
> > +     int i;
> > +
> > +     lockdep_assert_held(&tz->lock);
> > +
> > +     if (!tz->trips || !trip_priv)
> > +             return;
> > +
> > +     for (i = 0; i < tz->num_trips; i++) {
> > +             struct thermal_trip *trip = &tz->trips[i];
> > +
> > +             if (trip->priv == trip_priv) {
> > +                     trip->temperature = temperature;
> > +                     return;
> > +             }
> > +     }
> > +}
> > +EXPORT_SYMBOL_GPL(thermal_zone_update_trip_temp);
>
> This function would imply the comparator is always trip->priv but if we
> want another comparison eg. trip->priv->id, that won't be possible.
>
> Actually, I think you can reuse an existing function with a simple
> change, for_each_thermal_trip() located in thermal_core.h.

for_each_thermal_trip() is only defined in tools/lib/thermal/thermal.c
AFAICS, but this one could actually work, so I can copy that
definition to somewhere else.

But I suppose that you mean __for_each_thermal_trip() which won't
work, because it makes a copy of the trip and passes that to the
callback, but the callback would need to update the temperature of the
original trip.

It would work if it passed the original trip to the caller, so I can
add something like that.

> The changes would be renaming it without the '__' prefix and moving it
> in include/linux/thermal.h.
>
> Then the comparison function and the temperature change can be an ACPI
> driver specific callback passed as parameter to for_each_thermal_zone

I guess you mean for_each_thermal_trip().

As per the above, not really, but I can do something along these lines.