| Message ID | 20230110151745.2546131-4-daniel.lezcano@linaro.org (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | Thermal ACPI APIs for generic trip points | expand |
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! > >
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; - 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;