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

[Daniel Lezcano]

On 07/08/2023 17:40, Rafael J. Wysocki wrote:
> 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.

As there is no user of this function yet, I think you can change that to 
use the trip array instead of the __thermal_zone_get_trip(). This one 
was used to have a compatibility with thermal zones using get_trip_* ops 
but that is not really needed and with your series only one driver will 
remain before dropping these ops.

> > 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().

Yes, __for_each_thermal_trip()

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

--
<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