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

[Daniel Lezcano <daniel.lezcano@linaro.org>]

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.

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