[3/3] thermal/drivers/intel: Use generic trip points int340x
Commit Message
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@kernel.org>
---
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@linaro.org>
---
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(-)
Comments
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@kernel.org>
> ---
> 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@linaro.org>
> ---
> 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.
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(
> adev-
> >handle);
>
> - int34x_thermal_zone->zone = thermal_zone_device_register(
> + int34x_thermal_zone->zone =
> thermal_zone_device_register_with_trips(
> acpi_device_bid(adev),
> + 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;
Hi Rui;
thanks for testing and your comments
On 13/01/2023 12:41, 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@kernel.org>
>> ---
>> 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@linaro.org>
>> ---
>> drivers/thermal/intel/int340x_thermal/Kconfig | 1 +
>> .../int340x_thermal/int340x_thermal_zone.c | 177 ++++----------
>> ----
[ ... ]
>> -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?
No, it is an error. The function thermal_acpi_trip_gtsh() should do:
return deci_kelvin_to_millicelsius(hyst);
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@kernel.org>
> > ---
> > 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@linaro.org>
> > ---
> > 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;
Hi Daniel,
> > >
[...]
> > > - 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?
>
> No, it is an error. The function thermal_acpi_trip_gtsh() should do:
>
> return deci_kelvin_to_millicelsius(hyst);
>
>
GTSH returns here in tenths of degree Kelvin. For example 15 means 1.5
degree K.
I would like to test your next series with thermald. If there is a
problem, it will break every distro.
Thanks,
Srinivas
>
>
Hi Srinivas,
On 13/01/2023 16:48, srinivas pandruvada wrote:
> Hi Daniel,
>
>>>>
>
> [...]
>
>>>> - 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?
>>
>> No, it is an error. The function thermal_acpi_trip_gtsh() should do:
>>
>> return deci_kelvin_to_millicelsius(hyst);
>>
>>
>
> GTSH returns here in tenths of degree Kelvin. For example 15 means 1.5
> degree K.
Yes, so the above conversion is correct, right ?
> I would like to test your next series with thermald. If there is a
> problem, it will break every distro.
Great, thanks!
On Fri, 2023-01-13 at 18:21 +0100, Daniel Lezcano wrote:
>
> Hi Srinivas,
>
> On 13/01/2023 16:48, srinivas pandruvada wrote:
> > Hi Daniel,
> >
> > > > >
> >
> > [...]
> >
> > > > > - 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?
> > >
> > > No, it is an error. The function thermal_acpi_trip_gtsh() should
> > > do:
> > >
> > > return deci_kelvin_to_millicelsius(hyst);
> > >
> > >
> >
> > GTSH returns here in tenths of degree Kelvin. For example 15 means
> > 1.5
> > degree K.
>
> Yes, so the above conversion is correct, right ?
Correct.
Thanks,
Srinivas
>
> > I would like to test your next series with thermald. If there is a
> > problem, it will break every distro.
>
> Great, thanks!
>
>
@@ -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
@@ -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;
-
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(
adev->handle);
- int34x_thermal_zone->zone = thermal_zone_device_register(
+ int34x_thermal_zone->zone = thermal_zone_device_register_with_trips(
acpi_device_bid(adev),
+ 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);
}
@@ -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;