| Message ID | 20230104222127.2364396-3-daniel.lezcano@kernel.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Thermal ACPI APIs for generic trip points | expand |
On Wed, 2023-01-04 at 23:21 +0100, Daniel Lezcano wrote: > From: Daniel Lezcano <daniel.lezcano@linaro.org> > > 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 > --- > drivers/thermal/intel/Kconfig | 1 + > drivers/thermal/intel/intel_pch_thermal.c | 88 +++++-------------- > ---- > 2 files changed, 20 insertions(+), 69 deletions(-) > > diff --git a/drivers/thermal/intel/Kconfig > b/drivers/thermal/intel/Kconfig > index f0c845679250..738b88b290f4 100644 > --- a/drivers/thermal/intel/Kconfig > +++ b/drivers/thermal/intel/Kconfig > @@ -75,6 +75,7 @@ config INTEL_BXT_PMIC_THERMAL > config INTEL_PCH_THERMAL > tristate "Intel PCH Thermal Reporting Driver" > depends on X86 && PCI > + select THERMAL_ACPI THERMAL_ACPI depends on ACPI but the PCH thermal driver does not. So we will run into "unmet dependencies" issue when CONFIG_ACPI is cleared like below WARNING: unmet direct dependencies detected for THERMAL_ACPI Depends on [n]: THERMAL [=y] && ACPI [=n] Selected by [m]: - INTEL_PCH_THERMAL [=m] && THERMAL [=y] && (X86 [=y] || X86_INTEL_QUARK [=n] || COMPILE_TEST [=n]) && X86 [=y] && PCI [=y] thanks, rui > help > Enable this to support thermal reporting on certain intel > PCHs. > Thermal reporting device will provide temperature reading, > diff --git a/drivers/thermal/intel/intel_pch_thermal.c > b/drivers/thermal/intel/intel_pch_thermal.c > index dabf11a687a1..530fe9b38381 100644 > --- a/drivers/thermal/intel/intel_pch_thermal.c > +++ b/drivers/thermal/intel/intel_pch_thermal.c > @@ -65,6 +65,8 @@ > #define WPT_TEMP_OFFSET (PCH_TEMP_OFFSET * > MILLIDEGREE_PER_DEGREE) > #define GET_PCH_TEMP(x) (((x) / 2) + PCH_TEMP_OFFSET) > > +#define PCH_MAX_TRIPS 3 /* critical, hot, passive */ > + > /* Amount of time for each cooling delay, 100ms by default for now > */ > static unsigned int delay_timeout = 100; > module_param(delay_timeout, int, 0644); > @@ -82,12 +84,7 @@ struct pch_thermal_device { > const struct pch_dev_ops *ops; > struct pci_dev *pdev; > struct thermal_zone_device *tzd; > - int crt_trip_id; > - unsigned long crt_temp; > - int hot_trip_id; > - unsigned long hot_temp; > - int psv_trip_id; > - unsigned long psv_temp; > + struct thermal_trip trips[PCH_MAX_TRIPS]; > bool bios_enabled; > }; > > @@ -102,33 +99,22 @@ static void pch_wpt_add_acpi_psv_trip(struct > pch_thermal_device *ptd, > int *nr_trips) > { > struct acpi_device *adev; > - > - ptd->psv_trip_id = -1; > + int ret; > > adev = ACPI_COMPANION(&ptd->pdev->dev); > - if (adev) { > - unsigned long long r; > - acpi_status status; > - > - status = acpi_evaluate_integer(adev->handle, "_PSV", > NULL, > - &r); > - if (ACPI_SUCCESS(status)) { > - unsigned long trip_temp; > - > - trip_temp = deci_kelvin_to_millicelsius(r); > - if (trip_temp) { > - ptd->psv_temp = trip_temp; > - ptd->psv_trip_id = *nr_trips; > - ++(*nr_trips); > - } > - } > - } > + if (!adev) > + return; > + > + ret = thermal_acpi_trip_psv(adev, &ptd->trips[*nr_trips]); > + if (ret) > + return; > + > + ++(*nr_trips); > } > #else > static void pch_wpt_add_acpi_psv_trip(struct pch_thermal_device > *ptd, > int *nr_trips) > { > - ptd->psv_trip_id = -1; > > } > #endif > @@ -163,21 +149,19 @@ static int pch_wpt_init(struct > pch_thermal_device *ptd, int *nr_trips) > } > > read_trips: > - ptd->crt_trip_id = -1; > trip_temp = readw(ptd->hw_base + WPT_CTT); > trip_temp &= 0x1FF; > if (trip_temp) { > - ptd->crt_temp = GET_WPT_TEMP(trip_temp); > - ptd->crt_trip_id = 0; > + ptd->trips[*nr_trips].temperature = > GET_WPT_TEMP(trip_temp); > + ptd->trips[*nr_trips].type = THERMAL_TRIP_CRITICAL; > ++(*nr_trips); > } > > - ptd->hot_trip_id = -1; > trip_temp = readw(ptd->hw_base + WPT_PHL); > trip_temp &= 0x1FF; > if (trip_temp) { > - ptd->hot_temp = GET_WPT_TEMP(trip_temp); > - ptd->hot_trip_id = *nr_trips; > + ptd->trips[*nr_trips].temperature = > GET_WPT_TEMP(trip_temp); > + ptd->trips[*nr_trips].type = THERMAL_TRIP_HOT; > ++(*nr_trips); > } > > @@ -298,39 +282,6 @@ static int pch_thermal_get_temp(struct > thermal_zone_device *tzd, int *temp) > return ptd->ops->get_temp(ptd, temp); > } > > -static int pch_get_trip_type(struct thermal_zone_device *tzd, int > trip, > - enum thermal_trip_type *type) > -{ > - struct pch_thermal_device *ptd = tzd->devdata; > - > - if (ptd->crt_trip_id == trip) > - *type = THERMAL_TRIP_CRITICAL; > - else if (ptd->hot_trip_id == trip) > - *type = THERMAL_TRIP_HOT; > - else if (ptd->psv_trip_id == trip) > - *type = THERMAL_TRIP_PASSIVE; > - else > - return -EINVAL; > - > - return 0; > -} > - > -static int pch_get_trip_temp(struct thermal_zone_device *tzd, int > trip, int *temp) > -{ > - struct pch_thermal_device *ptd = tzd->devdata; > - > - if (ptd->crt_trip_id == trip) > - *temp = ptd->crt_temp; > - else if (ptd->hot_trip_id == trip) > - *temp = ptd->hot_temp; > - else if (ptd->psv_trip_id == trip) > - *temp = ptd->psv_temp; > - else > - return -EINVAL; > - > - return 0; > -} > - > static void pch_critical(struct thermal_zone_device *tzd) > { > dev_dbg(&tzd->device, "%s: critical temperature reached\n", > tzd->type); > @@ -338,8 +289,6 @@ static void pch_critical(struct > thermal_zone_device *tzd) > > static struct thermal_zone_device_ops tzd_ops = { > .get_temp = pch_thermal_get_temp, > - .get_trip_type = pch_get_trip_type, > - .get_trip_temp = pch_get_trip_temp, > .critical = pch_critical, > }; > > @@ -423,8 +372,9 @@ static int intel_pch_thermal_probe(struct pci_dev > *pdev, > if (err) > goto error_cleanup; > > - ptd->tzd = thermal_zone_device_register(bi->name, nr_trips, 0, > ptd, > - &tzd_ops, NULL, 0, 0); > + ptd->tzd = thermal_zone_device_register_with_trips(bi->name, > ptd->trips, > + nr_trips, 0, > ptd, > + &tzd_ops, > NULL, 0, 0); > if (IS_ERR(ptd->tzd)) { > dev_err(&pdev->dev, "Failed to register thermal zone > %s\n", > bi->name);
Hi Rui, On 06/01/2023 09:32, Zhang, Rui wrote: > On Wed, 2023-01-04 at 23:21 +0100, Daniel Lezcano wrote: >> From: Daniel Lezcano <daniel.lezcano@linaro.org> >> >> 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 >> --- >> drivers/thermal/intel/Kconfig | 1 + >> drivers/thermal/intel/intel_pch_thermal.c | 88 +++++-------------- >> ---- >> 2 files changed, 20 insertions(+), 69 deletions(-) >> >> diff --git a/drivers/thermal/intel/Kconfig >> b/drivers/thermal/intel/Kconfig >> index f0c845679250..738b88b290f4 100644 >> --- a/drivers/thermal/intel/Kconfig >> +++ b/drivers/thermal/intel/Kconfig >> @@ -75,6 +75,7 @@ config INTEL_BXT_PMIC_THERMAL >> config INTEL_PCH_THERMAL >> tristate "Intel PCH Thermal Reporting Driver" >> depends on X86 && PCI >> + select THERMAL_ACPI > > THERMAL_ACPI depends on ACPI but the PCH thermal driver does not. > So we will run into "unmet dependencies" issue when CONFIG_ACPI is > cleared like below > > WARNING: unmet direct dependencies detected for THERMAL_ACPI > Depends on [n]: THERMAL [=y] && ACPI [=n] > Selected by [m]: > - INTEL_PCH_THERMAL [=m] && THERMAL [=y] && (X86 [=y] || > X86_INTEL_QUARK [=n] || COMPILE_TEST [=n]) && X86 [=y] && PCI [=y] > Ah yes, indeed. Thanks for spotting this. Given the code, I think we should do: select THERMAL_ACPI if ACPI it is from my POV semantically correct.
diff --git a/drivers/thermal/intel/Kconfig b/drivers/thermal/intel/Kconfig index f0c845679250..738b88b290f4 100644 --- a/drivers/thermal/intel/Kconfig +++ b/drivers/thermal/intel/Kconfig @@ -75,6 +75,7 @@ config INTEL_BXT_PMIC_THERMAL config INTEL_PCH_THERMAL tristate "Intel PCH Thermal Reporting Driver" depends on X86 && PCI + select THERMAL_ACPI help Enable this to support thermal reporting on certain intel PCHs. Thermal reporting device will provide temperature reading, diff --git a/drivers/thermal/intel/intel_pch_thermal.c b/drivers/thermal/intel/intel_pch_thermal.c index dabf11a687a1..530fe9b38381 100644 --- a/drivers/thermal/intel/intel_pch_thermal.c +++ b/drivers/thermal/intel/intel_pch_thermal.c @@ -65,6 +65,8 @@ #define WPT_TEMP_OFFSET (PCH_TEMP_OFFSET * MILLIDEGREE_PER_DEGREE) #define GET_PCH_TEMP(x) (((x) / 2) + PCH_TEMP_OFFSET) +#define PCH_MAX_TRIPS 3 /* critical, hot, passive */ + /* Amount of time for each cooling delay, 100ms by default for now */ static unsigned int delay_timeout = 100; module_param(delay_timeout, int, 0644); @@ -82,12 +84,7 @@ struct pch_thermal_device { const struct pch_dev_ops *ops; struct pci_dev *pdev; struct thermal_zone_device *tzd; - int crt_trip_id; - unsigned long crt_temp; - int hot_trip_id; - unsigned long hot_temp; - int psv_trip_id; - unsigned long psv_temp; + struct thermal_trip trips[PCH_MAX_TRIPS]; bool bios_enabled; }; @@ -102,33 +99,22 @@ static void pch_wpt_add_acpi_psv_trip(struct pch_thermal_device *ptd, int *nr_trips) { struct acpi_device *adev; - - ptd->psv_trip_id = -1; + int ret; adev = ACPI_COMPANION(&ptd->pdev->dev); - if (adev) { - unsigned long long r; - acpi_status status; - - status = acpi_evaluate_integer(adev->handle, "_PSV", NULL, - &r); - if (ACPI_SUCCESS(status)) { - unsigned long trip_temp; - - trip_temp = deci_kelvin_to_millicelsius(r); - if (trip_temp) { - ptd->psv_temp = trip_temp; - ptd->psv_trip_id = *nr_trips; - ++(*nr_trips); - } - } - } + if (!adev) + return; + + ret = thermal_acpi_trip_psv(adev, &ptd->trips[*nr_trips]); + if (ret) + return; + + ++(*nr_trips); } #else static void pch_wpt_add_acpi_psv_trip(struct pch_thermal_device *ptd, int *nr_trips) { - ptd->psv_trip_id = -1; } #endif @@ -163,21 +149,19 @@ static int pch_wpt_init(struct pch_thermal_device *ptd, int *nr_trips) } read_trips: - ptd->crt_trip_id = -1; trip_temp = readw(ptd->hw_base + WPT_CTT); trip_temp &= 0x1FF; if (trip_temp) { - ptd->crt_temp = GET_WPT_TEMP(trip_temp); - ptd->crt_trip_id = 0; + ptd->trips[*nr_trips].temperature = GET_WPT_TEMP(trip_temp); + ptd->trips[*nr_trips].type = THERMAL_TRIP_CRITICAL; ++(*nr_trips); } - ptd->hot_trip_id = -1; trip_temp = readw(ptd->hw_base + WPT_PHL); trip_temp &= 0x1FF; if (trip_temp) { - ptd->hot_temp = GET_WPT_TEMP(trip_temp); - ptd->hot_trip_id = *nr_trips; + ptd->trips[*nr_trips].temperature = GET_WPT_TEMP(trip_temp); + ptd->trips[*nr_trips].type = THERMAL_TRIP_HOT; ++(*nr_trips); } @@ -298,39 +282,6 @@ static int pch_thermal_get_temp(struct thermal_zone_device *tzd, int *temp) return ptd->ops->get_temp(ptd, temp); } -static int pch_get_trip_type(struct thermal_zone_device *tzd, int trip, - enum thermal_trip_type *type) -{ - struct pch_thermal_device *ptd = tzd->devdata; - - if (ptd->crt_trip_id == trip) - *type = THERMAL_TRIP_CRITICAL; - else if (ptd->hot_trip_id == trip) - *type = THERMAL_TRIP_HOT; - else if (ptd->psv_trip_id == trip) - *type = THERMAL_TRIP_PASSIVE; - else - return -EINVAL; - - return 0; -} - -static int pch_get_trip_temp(struct thermal_zone_device *tzd, int trip, int *temp) -{ - struct pch_thermal_device *ptd = tzd->devdata; - - if (ptd->crt_trip_id == trip) - *temp = ptd->crt_temp; - else if (ptd->hot_trip_id == trip) - *temp = ptd->hot_temp; - else if (ptd->psv_trip_id == trip) - *temp = ptd->psv_temp; - else - return -EINVAL; - - return 0; -} - static void pch_critical(struct thermal_zone_device *tzd) { dev_dbg(&tzd->device, "%s: critical temperature reached\n", tzd->type); @@ -338,8 +289,6 @@ static void pch_critical(struct thermal_zone_device *tzd) static struct thermal_zone_device_ops tzd_ops = { .get_temp = pch_thermal_get_temp, - .get_trip_type = pch_get_trip_type, - .get_trip_temp = pch_get_trip_temp, .critical = pch_critical, }; @@ -423,8 +372,9 @@ static int intel_pch_thermal_probe(struct pci_dev *pdev, if (err) goto error_cleanup; - ptd->tzd = thermal_zone_device_register(bi->name, nr_trips, 0, ptd, - &tzd_ops, NULL, 0, 0); + ptd->tzd = thermal_zone_device_register_with_trips(bi->name, ptd->trips, + nr_trips, 0, ptd, + &tzd_ops, NULL, 0, 0); if (IS_ERR(ptd->tzd)) { dev_err(&pdev->dev, "Failed to register thermal zone %s\n", bi->name);