[v1] thermal: ACPI: Make helpers retrieve temperature only

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

It is slightly better to make the ACPI thermal helper functions retrieve
the trip point temperature only instead of doing the full trip point
initialization, because they are also used for updating some already
registered trip points, in which case initializing a new trip just
in order to update the temperature of an existing one is somewhat
wasteful.

Modify the ACPI thermal helpers accordingly and update their users.

No intentional functional impact.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

I've change my mind regarding what the ACPI thermal helpers should do after
the realization that they can be used for updating an existing trip point
as well as for initializing a new one.  It makes more sense for them to
return the temperature because of that, which is the change made here.

The patch is on top of the current linux-next branch in linux-pm.git.

Thanks!

---
 drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.c |   40 ++--
 drivers/thermal/intel/intel_pch_thermal.c                    |    7 
 drivers/thermal/thermal_acpi.c                               |  108 +++--------
 include/linux/thermal.h                                      |    9 
 4 files changed, 70 insertions(+), 94 deletions(-)

On Fri, Jan 27, 2023 at 7:18 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> It is slightly better to make the ACPI thermal helper functions retrieve
> the trip point temperature only instead of doing the full trip point
> initialization, because they are also used for updating some already
> registered trip points, in which case initializing a new trip just
> in order to update the temperature of an existing one is somewhat
> wasteful.
>
> Modify the ACPI thermal helpers accordingly and update their users.
>
> No intentional functional impact.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>
> I've change my mind regarding what the ACPI thermal helpers should do after
> the realization that they can be used for updating an existing trip point
> as well as for initializing a new one.  It makes more sense for them to
> return the temperature because of that, which is the change made here.
>
> The patch is on top of the current linux-next branch in linux-pm.git.

And I'm assuming no objections here, because patch series depending on
this one have been tested, reviewed and ACKed.

> ---
>  drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.c |   40 ++--
>  drivers/thermal/intel/intel_pch_thermal.c                    |    7
>  drivers/thermal/thermal_acpi.c                               |  108 +++--------
>  include/linux/thermal.h                                      |    9
>  4 files changed, 70 insertions(+), 94 deletions(-)
>
> Index: linux-pm/drivers/thermal/thermal_acpi.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_acpi.c
> +++ linux-pm/drivers/thermal/thermal_acpi.c
> @@ -21,42 +21,11 @@
>  #define TEMP_MIN_DECIK 2180
>  #define TEMP_MAX_DECIK 4480
>
> -static int thermal_acpi_trip_init(struct acpi_device *adev,
> -                                 enum thermal_trip_type type, int id,
> -                                 struct thermal_trip *trip)
> +static int thermal_acpi_trip_temp(struct acpi_device *adev, char *obj_name,
> +                                 int *ret_temp)
>  {
>         unsigned long long temp;
>         acpi_status status;
> -       char obj_name[5];
> -
> -       switch (type) {
> -       case THERMAL_TRIP_ACTIVE:
> -               if (id < 0 || id > 9)
> -                       return -EINVAL;
> -
> -               obj_name[1] = 'A';
> -               obj_name[2] = 'C';
> -               obj_name[3] = '0' + id;
> -               break;
> -       case THERMAL_TRIP_PASSIVE:
> -               obj_name[1] = 'P';
> -               obj_name[2] = 'S';
> -               obj_name[3] = 'V';
> -               break;
> -       case THERMAL_TRIP_HOT:
> -               obj_name[1] = 'H';
> -               obj_name[2] = 'O';
> -               obj_name[3] = 'T';
> -               break;
> -       case THERMAL_TRIP_CRITICAL:
> -               obj_name[1] = 'C';
> -               obj_name[2] = 'R';
> -               obj_name[3] = 'T';
> -               break;
> -       }
> -
> -       obj_name[0] = '_';
> -       obj_name[4] = '\0';
>
>         status = acpi_evaluate_integer(adev->handle, obj_name, NULL, &temp);
>         if (ACPI_FAILURE(status)) {
> @@ -65,87 +34,84 @@ static int thermal_acpi_trip_init(struct
>         }
>
>         if (temp >= TEMP_MIN_DECIK && temp <= TEMP_MAX_DECIK) {
> -               trip->temperature = deci_kelvin_to_millicelsius(temp);
> +               *ret_temp = deci_kelvin_to_millicelsius(temp);
>         } else {
>                 acpi_handle_debug(adev->handle, "%s result %llu out of range\n",
>                                   obj_name, temp);
> -               trip->temperature = THERMAL_TEMP_INVALID;
> +               *ret_temp = THERMAL_TEMP_INVALID;
>         }
>
> -       trip->hysteresis = 0;
> -       trip->type = type;
> -
>         return 0;
>  }
>
>  /**
> - * thermal_acpi_trip_active - Get the specified active trip point
> - * @adev: Thermal zone ACPI device object to get the description from.
> + * thermal_acpi_active_trip_temp - Retrieve active trip point temperature
> + * @adev: Target thermal zone ACPI device object.
>   * @id: Active cooling level (0 - 9).
> - * @trip: Trip point structure to be populated on success.
> + * @ret_temp: Address to store the retrieved temperature value on success.
>   *
>   * Evaluate the _ACx object for the thermal zone represented by @adev to obtain
>   * the temperature of the active cooling trip point corresponding to the active
> - * cooling level given by @id and initialize @trip as an active trip point using
> - * that temperature value.
> + * cooling level given by @id.
>   *
>   * Return 0 on success or a negative error value on failure.
>   */
> -int thermal_acpi_trip_active(struct acpi_device *adev, int id,
> -                            struct thermal_trip *trip)
> +int thermal_acpi_active_trip_temp(struct acpi_device *adev, int id, int *ret_temp)
>  {
> -       return thermal_acpi_trip_init(adev, THERMAL_TRIP_ACTIVE, id, trip);
> +       char obj_name[] = {'_', 'A', 'C', '0' + id, '\0'};
> +
> +       if (id < 0 || id > 9)
> +               return -EINVAL;
> +
> +       return thermal_acpi_trip_temp(adev, obj_name, ret_temp);
>  }
> -EXPORT_SYMBOL_GPL(thermal_acpi_trip_active);
> +EXPORT_SYMBOL_GPL(thermal_acpi_active_trip_temp);
>
>  /**
> - * thermal_acpi_trip_passive - Get the passive trip point
> - * @adev: Thermal zone ACPI device object to get the description from.
> - * @trip: Trip point structure to be populated on success.
> + * thermal_acpi_passive_trip_temp - Retrieve passive trip point temperature
> + * @adev: Target thermal zone ACPI device object.
> + * @ret_temp: Address to store the retrieved temperature value on success.
>   *
>   * Evaluate the _PSV object for the thermal zone represented by @adev to obtain
> - * the temperature of the passive cooling trip point and initialize @trip as a
> - * passive trip point using that temperature value.
> + * the temperature of the passive cooling trip point.
>   *
>   * Return 0 on success or -ENODATA on failure.
>   */
> -int thermal_acpi_trip_passive(struct acpi_device *adev, struct thermal_trip *trip)
> +int thermal_acpi_passive_trip_temp(struct acpi_device *adev, int *ret_temp)
>  {
> -       return thermal_acpi_trip_init(adev, THERMAL_TRIP_PASSIVE, INT_MAX, trip);
> +       return thermal_acpi_trip_temp(adev, "_PSV", ret_temp);
>  }
> -EXPORT_SYMBOL_GPL(thermal_acpi_trip_passive);
> +EXPORT_SYMBOL_GPL(thermal_acpi_passive_trip_temp);
>
>  /**
> - * thermal_acpi_trip_hot - Get the near critical trip point
> - * @adev: the ACPI device to get the description from.
> - * @trip: a &struct thermal_trip to be filled if the function succeed.
> + * thermal_acpi_hot_trip_temp - Retrieve hot trip point temperature
> + * @adev: Target thermal zone ACPI device object.
> + * @ret_temp: Address to store the retrieved temperature value on success.
>   *
>   * Evaluate the _HOT object for the thermal zone represented by @adev to obtain
>   * the temperature of the trip point at which the system is expected to be put
> - * into the S4 sleep state and initialize @trip as a hot trip point using that
> - * temperature value.
> + * into the S4 sleep state.
>   *
>   * Return 0 on success or -ENODATA on failure.
>   */
> -int thermal_acpi_trip_hot(struct acpi_device *adev, struct thermal_trip *trip)
> +int thermal_acpi_hot_trip_temp(struct acpi_device *adev, int *ret_temp)
>  {
> -       return thermal_acpi_trip_init(adev, THERMAL_TRIP_HOT, INT_MAX, trip);
> +       return thermal_acpi_trip_temp(adev, "_HOT", ret_temp);
>  }
> -EXPORT_SYMBOL_GPL(thermal_acpi_trip_hot);
> +EXPORT_SYMBOL_GPL(thermal_acpi_hot_trip_temp);
>
>  /**
> - * thermal_acpi_trip_critical - Get the critical trip point
> - * @adev: the ACPI device to get the description from.
> - * @trip: a &struct thermal_trip to be filled if the function succeed.
> + * thermal_acpi_critical_trip_temp - Retrieve critical trip point temperature
> + * @adev: Target thermal zone ACPI device object.
> + * @ret_temp: Address to store the retrieved temperature value on success.
>   *
>   * Evaluate the _CRT object for the thermal zone represented by @adev to obtain
> - * the temperature of the critical cooling trip point and initialize @trip as a
> - * critical trip point using that temperature value.
> + * the temperature of the critical cooling trip point.
>   *
>   * Return 0 on success or -ENODATA on failure.
>   */
> -int thermal_acpi_trip_critical(struct acpi_device *adev, struct thermal_trip *trip)
> +int thermal_acpi_critical_trip_temp(struct acpi_device *adev, int *ret_temp)
>  {
> -       return thermal_acpi_trip_init(adev, THERMAL_TRIP_CRITICAL, INT_MAX, trip);
> +       return thermal_acpi_trip_temp(adev, "_CRT", ret_temp);
>  }
> -EXPORT_SYMBOL_GPL(thermal_acpi_trip_critical);
> +EXPORT_SYMBOL_GPL(thermal_acpi_critical_trip_temp);
> Index: linux-pm/include/linux/thermal.h
> ===================================================================
> --- linux-pm.orig/include/linux/thermal.h
> +++ linux-pm/include/linux/thermal.h
> @@ -347,11 +347,10 @@ int thermal_zone_get_num_trips(struct th
>  int thermal_zone_get_crit_temp(struct thermal_zone_device *tz, int *temp);
>
>  #ifdef CONFIG_THERMAL_ACPI
> -int thermal_acpi_trip_active(struct acpi_device *adev, int id,
> -                            struct thermal_trip *trip);
> -int thermal_acpi_trip_passive(struct acpi_device *adev, struct thermal_trip *trip);
> -int thermal_acpi_trip_hot(struct acpi_device *adev, struct thermal_trip *trip);
> -int thermal_acpi_trip_critical(struct acpi_device *adev, struct thermal_trip *trip);
> +int thermal_acpi_active_trip_temp(struct acpi_device *adev, int id, int *ret_temp);
> +int thermal_acpi_passive_trip_temp(struct acpi_device *adev, int *ret_temp);
> +int thermal_acpi_hot_trip_temp(struct acpi_device *adev, int *ret_temp);
> +int thermal_acpi_critical_trip_temp(struct acpi_device *adev, int *ret_temp);
>  #endif
>
>  #ifdef CONFIG_THERMAL
> Index: linux-pm/drivers/thermal/intel/intel_pch_thermal.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/intel/intel_pch_thermal.c
> +++ linux-pm/drivers/thermal/intel/intel_pch_thermal.c
> @@ -100,16 +100,17 @@ static void pch_wpt_add_acpi_psv_trip(st
>                                       int *nr_trips)
>  {
>         struct acpi_device *adev;
> -       int ret;
> +       int temp;
>
>         adev = ACPI_COMPANION(&ptd->pdev->dev);
>         if (!adev)
>                 return;
>
> -       ret = thermal_acpi_trip_passive(adev, &ptd->trips[*nr_trips]);
> -       if (ret || ptd->trips[*nr_trips].temperature <= 0)
> +       if (thermal_acpi_passive_trip_temp(adev, &temp) || temp <= 0)
>                 return;
>
> +       ptd->trips[*nr_trips].type = THERMAL_TRIP_PASSIVE;
> +       ptd->trips[*nr_trips].temperature = temp;
>         ++(*nr_trips);
>  }
>  #else
> Index: linux-pm/drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.c
> +++ linux-pm/drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.c
> @@ -70,24 +70,35 @@ static int int340x_thermal_read_trips(st
>  {
>         int i, ret;
>
> -       ret = thermal_acpi_trip_critical(zone_adev, &zone_trips[trip_cnt]);
> -       if (!ret)
> +       ret = thermal_acpi_critical_trip_temp(zone_adev,
> +                                             &zone_trips[trip_cnt].temperature);
> +       if (!ret) {
> +               zone_trips[trip_cnt].type = THERMAL_TRIP_CRITICAL;
>                 trip_cnt++;
> +       }
>
> -       ret = thermal_acpi_trip_hot(zone_adev, &zone_trips[trip_cnt]);
> -       if (!ret)
> +       ret = thermal_acpi_hot_trip_temp(zone_adev,
> +                                        &zone_trips[trip_cnt].temperature);
> +       if (!ret) {
> +               zone_trips[trip_cnt].type = THERMAL_TRIP_HOT;
>                 trip_cnt++;
> +       }
>
> -       ret = thermal_acpi_trip_passive(zone_adev, &zone_trips[trip_cnt]);
> -       if (!ret)
> +       ret = thermal_acpi_passive_trip_temp(zone_adev,
> +                                            &zone_trips[trip_cnt].temperature);
> +       if (!ret) {
> +               zone_trips[trip_cnt].type = THERMAL_TRIP_PASSIVE;
>                 trip_cnt++;
> +       }
>
>         for (i = 0; i < INT340X_THERMAL_MAX_ACT_TRIP_COUNT; i++) {
>
> -               ret = thermal_acpi_trip_active(zone_adev, i, &zone_trips[trip_cnt]);
> +               ret = thermal_acpi_active_trip_temp(zone_adev, i,
> +                                                   &zone_trips[trip_cnt].temperature);
>                 if (ret)
>                         break;
>
> +               zone_trips[trip_cnt].type = THERMAL_TRIP_ACTIVE;
>                 trip_cnt++;
>         }
>
> @@ -213,22 +224,21 @@ void int340x_thermal_update_trips(struct
>         mutex_lock(&int34x_zone->zone->lock);
>
>         for (i = int34x_zone->aux_trip_nr; i < trip_cnt; i++) {
> -               struct thermal_trip trip;
> -               int err;
> +               int temp, err;
>
>                 switch (zone_trips[i].type) {
>                 case THERMAL_TRIP_CRITICAL:
> -                       err = thermal_acpi_trip_critical(zone_adev, &trip);
> +                       err = thermal_acpi_critical_trip_temp(zone_adev, &temp);
>                         break;
>                 case THERMAL_TRIP_HOT:
> -                       err = thermal_acpi_trip_hot(zone_adev, &trip);
> +                       err = thermal_acpi_hot_trip_temp(zone_adev, &temp);
>                         break;
>                 case THERMAL_TRIP_PASSIVE:
> -                       err = thermal_acpi_trip_passive(zone_adev, &trip);
> +                       err = thermal_acpi_passive_trip_temp(zone_adev, &temp);
>                         break;
>                 case THERMAL_TRIP_ACTIVE:
> -                       err = thermal_acpi_trip_active(zone_adev, act_trip_nr++,
> -                                                      &trip);
> +                       err = thermal_acpi_active_trip_temp(zone_adev, act_trip_nr++,
> +                                                           &temp);
>                         break;
>                 default:
>                         err = -ENODEV;
> @@ -238,7 +248,7 @@ void int340x_thermal_update_trips(struct
>                         continue;
>                 }
>
> -               zone_trips[i].temperature = trip.temperature;
> +               zone_trips[i].temperature = temp;
>         }
>
>         mutex_unlock(&int34x_zone->zone->lock);
>
>
>

On 27/01/2023 19:17, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> It is slightly better to make the ACPI thermal helper functions retrieve
> the trip point temperature only instead of doing the full trip point
> initialization, because they are also used for updating some already
> registered trip points, in which case initializing a new trip just
> in order to update the temperature of an existing one is somewhat
> wasteful.
> 
> Modify the ACPI thermal helpers accordingly and update their users.
> 
> No intentional functional impact.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---

Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>