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

  From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

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@intel.com>
---

New patch in v4.

---
 drivers/thermal/thermal_trip.c |   37 +++++++++++++++++++++++++++++++++++++
 include/linux/thermal.h        |    4 ++++
 2 files changed, 41 insertions(+)
  

On 04/08/2023 23:05, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> 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@intel.com>
> ---
> 
> 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.

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



> Index: linux-pm/include/linux/thermal.h
> ===================================================================
> --- linux-pm.orig/include/linux/thermal.h
> +++ linux-pm/include/linux/thermal.h
> @@ -286,9 +286,13 @@ int __thermal_zone_get_trip(struct therm
>   			    struct thermal_trip *trip);
>   int thermal_zone_get_trip(struct thermal_zone_device *tz, int trip_id,
>   			  struct thermal_trip *trip);
> +void thermal_zone_update_trip_temp(struct thermal_zone_device *tz,
> +				   void *trip_priv, int temperature);
>   
>   int thermal_zone_set_trip(struct thermal_zone_device *tz, int trip_id,
>   			  const struct thermal_trip *trip);
> +void thermal_zone_update_trip_temp(struct thermal_zone_device *tz,
> +				   void *trip_priv, int temperature);
>   
>   int thermal_zone_get_num_trips(struct thermal_zone_device *tz);
>   
> 
> 
>
  

On Mon, Aug 7, 2023 at 1:34 PM Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>
> On 04/08/2023 23:05, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > 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@intel.com>
> > ---
> >
> > 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.
  

On 07/08/2023 17:40, Rafael J. Wysocki wrote:
> On Mon, Aug 7, 2023 at 1:34 PM Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>>
>> On 04/08/2023 23:05, Rafael J. Wysocki wrote:
>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>
>>> 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@intel.com>
>>> ---
>>>
>>> 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.
  

On Mon, Aug 7, 2023 at 6:17 PM Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>
> On 07/08/2023 17:40, Rafael J. Wysocki wrote:
> > On Mon, Aug 7, 2023 at 1:34 PM Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
> >>
> >> On 04/08/2023 23:05, Rafael J. Wysocki wrote:
> >>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >>>
> >>> 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@intel.com>
> >>> ---
> >>>
> >>> 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.

Sounds good.

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

OK