On Fri, 2023-01-13 at 11:41 +0000, Zhang, Rui wrote:
> On Tue, 2023-01-10 at 16:17 +0100, Daniel Lezcano wrote:
> > The thermal framework gives the possibility to register the trip
> > points with the thermal zone. When that is done, no get_trip_* ops
> > are
> > needed and they can be removed.
> >
> > Convert the ops content logic into generic trip points and register
> > them with the thermal zone.
> >
> > In order to consolidate the code, use the ACPI thermal framework
> > API
> > to fill the generic trip point from the ACPI tables.
> >
> > It has been tested on a Intel i7-8650U - x280 with the INT3400, the
> > PCH, ACPITZ, and x86_pkg_temp. No regression observed so far.
> >
> > Signed-off-by: Daniel Lezcano <daniel.lezcano@xxxxxxxxxx>
> > ---
> > V3:
> > - The driver Kconfig option selects CONFIG_THERMAL_ACPI
> > - Change the initialization to use GTSH for the hysteresis on
> > all the trip points
> >
> > Signed-off-by: Daniel Lezcano <daniel.lezcano@xxxxxxxxxx>
> > ---
> > drivers/thermal/intel/int340x_thermal/Kconfig | 1 +
> > .../int340x_thermal/int340x_thermal_zone.c | 177 ++++----------
> > ----
> > .../int340x_thermal/int340x_thermal_zone.h | 10 +-
> > 3 files changed, 43 insertions(+), 145 deletions(-)
> >
> > diff --git a/drivers/thermal/intel/int340x_thermal/Kconfig
> > b/drivers/thermal/intel/int340x_thermal/Kconfig
> > index 5d046de96a5d..b7072d37101d 100644
> > --- a/drivers/thermal/intel/int340x_thermal/Kconfig
> > +++ b/drivers/thermal/intel/int340x_thermal/Kconfig
> > @@ -9,6 +9,7 @@ config INT340X_THERMAL
> > select THERMAL_GOV_USER_SPACE
> > select ACPI_THERMAL_REL
> > select ACPI_FAN
> > + select THERMAL_ACPI
> > select INTEL_SOC_DTS_IOSF_CORE
> > select PROC_THERMAL_MMIO_RAPL if POWERCAP
> > help
> > diff --git
> > a/drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.c
> > b/drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.c
> > index 228f44260b27..626b33253153 100644
> > --- a/drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.c
> > +++ b/drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.c
> > @@ -37,65 +37,6 @@ static int int340x_thermal_get_zone_temp(struct
> > thermal_zone_device *zone,
> > return 0;
> > }
> >
> > -static int int340x_thermal_get_trip_temp(struct
> > thermal_zone_device
> > *zone,
> > - int trip, int *temp)
> > -{
> > - struct int34x_thermal_zone *d = zone->devdata;
> > - int i;
> > -
> > - if (trip < d->aux_trip_nr)
> > - *temp = d->aux_trips[trip];
> > - else if (trip == d->crt_trip_id)
> > - *temp = d->crt_temp;
> > - else if (trip == d->psv_trip_id)
> > - *temp = d->psv_temp;
> > - else if (trip == d->hot_trip_id)
> > - *temp = d->hot_temp;
> > - else {
> > - for (i = 0; i < INT340X_THERMAL_MAX_ACT_TRIP_COUNT;
> > i++) {
> > - if (d->act_trips[i].valid &&
> > - d->act_trips[i].id == trip) {
> > - *temp = d->act_trips[i].temp;
> > - break;
> > - }
> > - }
> > - if (i == INT340X_THERMAL_MAX_ACT_TRIP_COUNT)
> > - return -EINVAL;
> > - }
> > -
> > - return 0;
> > -}
> > -
> > -static int int340x_thermal_get_trip_type(struct
> > thermal_zone_device
> > *zone,
> > - int trip,
> > - enum thermal_trip_type
> > *type)
> > -{
> > - struct int34x_thermal_zone *d = zone->devdata;
> > - int i;
> > -
> > - if (trip < d->aux_trip_nr)
> > - *type = THERMAL_TRIP_PASSIVE;
> > - else if (trip == d->crt_trip_id)
> > - *type = THERMAL_TRIP_CRITICAL;
> > - else if (trip == d->hot_trip_id)
> > - *type = THERMAL_TRIP_HOT;
> > - else if (trip == d->psv_trip_id)
> > - *type = THERMAL_TRIP_PASSIVE;
> > - else {
> > - for (i = 0; i < INT340X_THERMAL_MAX_ACT_TRIP_COUNT;
> > i++) {
> > - if (d->act_trips[i].valid &&
> > - d->act_trips[i].id == trip) {
> > - *type = THERMAL_TRIP_ACTIVE;
> > - break;
> > - }
> > - }
> > - if (i == INT340X_THERMAL_MAX_ACT_TRIP_COUNT)
> > - return -EINVAL;
> > - }
> > -
> > - return 0;
> > -}
> > -
> > static int int340x_thermal_set_trip_temp(struct
> > thermal_zone_device
> > *zone,
> > int trip, int temp)
> > {
> > @@ -109,25 +50,6 @@ static int int340x_thermal_set_trip_temp(struct
> > thermal_zone_device *zone,
> > if (ACPI_FAILURE(status))
> > return -EIO;
> >
> > - d->aux_trips[trip] = temp;
> > -
> > - return 0;
> > -}
> > -
> > -
> > -static int int340x_thermal_get_trip_hyst(struct
> > thermal_zone_device
> > *zone,
> > - int trip, int *temp)
> > -{
> > - struct int34x_thermal_zone *d = zone->devdata;
> > - acpi_status status;
> > - unsigned long long hyst;
> > -
> > - status = acpi_evaluate_integer(d->adev->handle, "GTSH",
> > NULL,
> > &hyst);
> > - if (ACPI_FAILURE(status))
> > - *temp = 0;
> > - else
> > - *temp = hyst * 100;
>
> The previous code returns hyst * 100.
> But the new API retuurns hyst directly.
>
> -/sys/class/thermal/thermal_zone2/trip_point_4_hyst:2000
> +/sys/class/the
> rmal/thermal_zone2/trip_point_4_hyst:20
>
> Is this done on purpose?
>
> I think this may break userspace tools like thermald.
>
It will.
Thanks,
Srinivas
> Srinivas, can you confirm?
>
> thanks,
> rui
> > -
> > return 0;
> > }
> >
> > @@ -138,58 +60,36 @@ static void int340x_thermal_critical(struct
> > thermal_zone_device *zone)
> >
> > static struct thermal_zone_device_ops int340x_thermal_zone_ops = {
> > .get_temp = int340x_thermal_get_zone_temp,
> > - .get_trip_temp = int340x_thermal_get_trip_temp,
> > - .get_trip_type = int340x_thermal_get_trip_type,
> > .set_trip_temp = int340x_thermal_set_trip_temp,
> > - .get_trip_hyst = int340x_thermal_get_trip_hyst,
> > .critical = int340x_thermal_critical,
> > };
> >
> > -static int int340x_thermal_get_trip_config(acpi_handle handle,
> > char
> > *name,
> > - int *temp)
> > -{
> > - unsigned long long r;
> > - acpi_status status;
> > -
> > - status = acpi_evaluate_integer(handle, name, NULL, &r);
> > - if (ACPI_FAILURE(status))
> > - return -EIO;
> > -
> > - *temp = deci_kelvin_to_millicelsius(r);
> > -
> > - return 0;
> > -}
> > -
> > int int340x_thermal_read_trips(struct int34x_thermal_zone
> > *int34x_zone)
> > {
> > - int trip_cnt = int34x_zone->aux_trip_nr;
> > - int i;
> > + int trip_cnt;
> > + int i, ret;
> > +
> > + trip_cnt = int34x_zone->aux_trip_nr;
> >
> > - int34x_zone->crt_trip_id = -1;
> > - if (!int340x_thermal_get_trip_config(int34x_zone->adev-
> > >handle,
> > "_CRT",
> > - &int34x_zone-
> > >crt_temp))
> > - int34x_zone->crt_trip_id = trip_cnt++;
> > + ret = thermal_acpi_trip_crit(int34x_zone->adev,
> > &int34x_zone-
> > > trips[trip_cnt]);
> > + if (!ret)
> > + trip_cnt++;
> >
> > - int34x_zone->hot_trip_id = -1;
> > - if (!int340x_thermal_get_trip_config(int34x_zone->adev-
> > >handle,
> > "_HOT",
> > - &int34x_zone-
> > >hot_temp))
> > - int34x_zone->hot_trip_id = trip_cnt++;
> > + ret = thermal_acpi_trip_hot(int34x_zone->adev,
> > &int34x_zone-
> > > trips[trip_cnt]);
> > + if (!ret)
> > + trip_cnt++;
> >
> > - int34x_zone->psv_trip_id = -1;
> > - if (!int340x_thermal_get_trip_config(int34x_zone->adev-
> > >handle,
> > "_PSV",
> > - &int34x_zone-
> > >psv_temp))
> > - int34x_zone->psv_trip_id = trip_cnt++;
> > + ret = thermal_acpi_trip_psv(int34x_zone->adev,
> > &int34x_zone-
> > > trips[trip_cnt]);
> > + if (!ret)
> > + trip_cnt++;
> >
> > for (i = 0; i < INT340X_THERMAL_MAX_ACT_TRIP_COUNT; i++) {
> > - char name[5] = { '_', 'A', 'C', '0' + i, '\0' };
> >
> > - if (int340x_thermal_get_trip_config(int34x_zone-
> > >adev-
> > > handle,
> > - name,
> > - &int34x_zone-
> > > act_trips[i].temp))
> > + ret = thermal_acpi_trip_act(int34x_zone->adev,
> > &int34x_zone->trips[trip_cnt], i);
> > + if (ret)
> > break;
> >
> > - int34x_zone->act_trips[i].id = trip_cnt++;
> > - int34x_zone->act_trips[i].valid = true;
> > + trip_cnt++;
> > }
> >
> > return trip_cnt;
> > @@ -208,7 +108,7 @@ struct int34x_thermal_zone
> > *int340x_thermal_zone_add(struct acpi_device *adev,
> > acpi_status status;
> > unsigned long long trip_cnt;
> > int trip_mask = 0;
> > - int ret;
> > + int i, ret;
> >
> > int34x_thermal_zone = kzalloc(sizeof(*int34x_thermal_zone),
> > GFP_KERNEL);
> > @@ -228,32 +128,35 @@ struct int34x_thermal_zone
> > *int340x_thermal_zone_add(struct acpi_device *adev,
> > int34x_thermal_zone->ops->get_temp = get_temp;
> >
> > status = acpi_evaluate_integer(adev->handle, "PATC", NULL,
> > &trip_cnt);
> > - if (ACPI_FAILURE(status))
> > - trip_cnt = 0;
> > - else {
> > - int i;
> > -
> > - int34x_thermal_zone->aux_trips =
> > - kcalloc(trip_cnt,
> > - sizeof(*int34x_thermal_zone-
> > > aux_trips),
> > - GFP_KERNEL);
> > - if (!int34x_thermal_zone->aux_trips) {
> > - ret = -ENOMEM;
> > - goto err_trip_alloc;
> > - }
> > - trip_mask = BIT(trip_cnt) - 1;
> > + if (!ACPI_FAILURE(status)) {
> > int34x_thermal_zone->aux_trip_nr = trip_cnt;
> > - for (i = 0; i < trip_cnt; ++i)
> > - int34x_thermal_zone->aux_trips[i] =
> > THERMAL_TEMP_INVALID;
> > + trip_mask = BIT(trip_cnt) - 1;
> > + }
> > +
> > + int34x_thermal_zone->trips =
> > kzalloc(sizeof(*int34x_thermal_zone->trips) *
> > +
> > (INT340X_THERMAL_MAX_TRIP_
> > COUNT + trip_cnt),
> > + GFP_KERNEL);
> > + if (!int34x_thermal_zone->trips) {
> > + ret = -ENOMEM;
> > + goto err_trips_alloc;
> > }
> >
> > trip_cnt = int340x_thermal_read_trips(int34x_thermal_zone);
> >
> > + for (i = 0; i < trip_cnt; ++i)
> > + int34x_thermal_zone->trips[i].hysteresis =
> > thermal_acpi_trip_gtsh(adev);
> > +
> > + for (i = 0; i < int34x_thermal_zone->aux_trip_nr; i++) {
> > + int34x_thermal_zone->trips[i].type =
> > THERMAL_TRIP_PASSIVE;
> > + int34x_thermal_zone->trips[i].temperature =
> > THERMAL_TEMP_INVALID;
> > + }
> > +
> > int34x_thermal_zone->lpat_table =
> > acpi_lpat_get_conversion_table(
> > ade
> > v-
> > > handle);
> >
> > - int34x_thermal_zone->zone = thermal_zone_device_register(
> > + int34x_thermal_zone->zone =
> > thermal_zone_device_register_with_trips(
> > acpi_device_bid(ade
> > v),
> > + int34x_thermal_zone
> > -
> > > trips,
> > trip_cnt,
> > trip_mask,
> > int34x_thermal_zone,
> > int34x_thermal_zone
> > -
> > > ops,
> > @@ -272,9 +175,9 @@ struct int34x_thermal_zone
> > *int340x_thermal_zone_add(struct acpi_device *adev,
> > err_enable:
> > thermal_zone_device_unregister(int34x_thermal_zone->zone);
> > err_thermal_zone:
> > + kfree(int34x_thermal_zone->trips);
> > acpi_lpat_free_conversion_table(int34x_thermal_zone-
> > > lpat_table);
> > - kfree(int34x_thermal_zone->aux_trips);
> > -err_trip_alloc:
> > +err_trips_alloc:
> > kfree(int34x_thermal_zone->ops);
> > err_ops_alloc:
> > kfree(int34x_thermal_zone);
> > @@ -287,7 +190,7 @@ void int340x_thermal_zone_remove(struct
> > int34x_thermal_zone
> > {
> > thermal_zone_device_unregister(int34x_thermal_zone->zone);
> > acpi_lpat_free_conversion_table(int34x_thermal_zone-
> > > lpat_table);
> > - kfree(int34x_thermal_zone->aux_trips);
> > + kfree(int34x_thermal_zone->trips);
> > kfree(int34x_thermal_zone->ops);
> > kfree(int34x_thermal_zone);
> > }
> > diff --git
> > a/drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.h
> > b/drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.h
> > index e28ab1ba5e06..0c2c8de92014 100644
> > --- a/drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.h
> > +++ b/drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.h
> > @@ -10,6 +10,7 @@
> > #include <acpi/acpi_lpat.h>
> >
> > #define INT340X_THERMAL_MAX_ACT_TRIP_COUNT 10
> > +#define INT340X_THERMAL_MAX_TRIP_COUNT
> > INT340X_THERMAL_MAX_ACT_TRIP_COUNT + 3
> >
> > struct active_trip {
> > int temp;
> > @@ -19,15 +20,8 @@ struct active_trip {
> >
> > struct int34x_thermal_zone {
> > struct acpi_device *adev;
> > - struct active_trip
> > act_trips[INT340X_THERMAL_MAX_ACT_TRIP_COUNT];
> > - unsigned long *aux_trips;
> > + struct thermal_trip *trips;
> > int aux_trip_nr;
> > - int psv_temp;
> > - int psv_trip_id;
> > - int crt_temp;
> > - int crt_trip_id;
> > - int hot_temp;
> > - int hot_trip_id;
> > struct thermal_zone_device *zone;
> > struct thermal_zone_device_ops *ops;
> > void *priv_data;
