Re: [PATCH v2 1/3] thermal/acpi: Add ACPI trip point routines
From: Daniel Lezcano
Date: Tue Jan 03 2023 - 05:44:59 EST
On 02/01/2023 19:22, Rafael J. Wysocki wrote:
On Mon, Jan 2, 2023 at 7:01 PM Daniel Lezcano <daniel.lezcano@xxxxxxxxxx> wrote:
From: Daniel Lezcano <daniel.lezcano@xxxxxxxxxx>
The ACPI specification describes the trip points, the device tree
bindings as well.
The OF code uses the generic trip point structures.
The ACPI has their own trip points structure and uses the get_trip_*
ops to retrieve them.
We can do the same as the OF code and create a set of ACPI functions
to retrieve a trip point description. Having a common code for ACPI
will help to cleanup the remaining Intel drivers and get rid of the
get_trip_* functions.
These changes add the ACPI thermal calls to retrieve the basic
information we need to be reused in the thermal ACPI and Intel
drivers.
The different ACPI functions have the generic trip point structure
passed as parameter where it is filled.
This structure aims to be the one used by all the thermal drivers and
the thermal framework.
After this series, a couple of Intel drivers and the ACPI thermal
driver will still have their own trip points definition but a new
series on top of this one will finish the conversion to the generic
trip point handling.
This series depends on the generic trip point added to the thermal
framework and available in the thermal/linux-next branch.
https://lkml.org/lkml/2022/10/3/456
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>
---
drivers/thermal/Kconfig | 13 ++
drivers/thermal/Makefile | 1 +
drivers/thermal/thermal_acpi.c | 279 +++++++++++++++++++++++++++++++++
include/linux/thermal.h | 16 ++
4 files changed, 309 insertions(+)
create mode 100644 drivers/thermal/thermal_acpi.c
diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
index e052dae614eb..2c19bccd1223 100644
--- a/drivers/thermal/Kconfig
+++ b/drivers/thermal/Kconfig
@@ -76,6 +76,19 @@ config THERMAL_OF
Say 'Y' here if you need to build thermal infrastructure
based on device tree.
+config THERMAL_ACPI
Not needed.
Or if you want it to be built only if there are any users, call it
ACPI_THERMAL_LIB and do
config ACPI_THERMAL_LIB
depends on ACPI_THERMAL
bool
and let the users select it.
Yes, I think it makes more sense to not provide any option and just
compile the wrappers when ACPI_THERMAL is set.
diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
index 2506c6c8ca83..60f0dfa9aae2 100644
--- a/drivers/thermal/Makefile
+++ b/drivers/thermal/Makefile
@@ -13,6 +13,7 @@ thermal_sys-$(CONFIG_THERMAL_NETLINK) += thermal_netlink.o
# interface to/from other layers providing sensors
thermal_sys-$(CONFIG_THERMAL_HWMON) += thermal_hwmon.o
thermal_sys-$(CONFIG_THERMAL_OF) += thermal_of.o
+thermal_sys-$(CONFIG_THERMAL_ACPI) += thermal_acpi.o
# governors
thermal_sys-$(CONFIG_THERMAL_GOV_FAIR_SHARE) += gov_fair_share.o
diff --git a/drivers/thermal/thermal_acpi.c b/drivers/thermal/thermal_acpi.c
new file mode 100644
index 000000000000..28c629b4d814
--- /dev/null
+++ b/drivers/thermal/thermal_acpi.c
@@ -0,0 +1,279 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 2022 Linaro Limited
+ *
+ * Author: Daniel Lezcano <daniel.lezcano@xxxxxxxxxx>
+ *
+ * ACPI thermal configuration
+ */
+#include <linux/acpi.h>
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/units.h>
+#include <uapi/linux/thermal.h>
+
+#include "thermal_core.h"
+
+int thermal_acpi_trip_gtsh(struct acpi_device *adev)
+{
+ unsigned long long hyst;
+ acpi_status status;
+
+ status = acpi_evaluate_integer(adev->handle, "GTSH", NULL, &hyst);
+ if (ACPI_FAILURE(status))
+ return 0;
+
+ return (int)(hyst * 100);
What if the result is larger than INT_MAX?
That would mean ACPI is returning more than 4 billions degree hysteresis
value.
What strategy should we use in these functions? Trust the values
returned by ACPI or double check if they are consistent ?
+}
+EXPORT_SYMBOL_GPL(thermal_acpi_trip_gtsh);
+
+int thermal_acpi_get_tzd(struct acpi_device *adev, struct acpi_handle_list *devices)
+{
+ acpi_status status;
+
+ /*
+ * _TZD (Thermal zone device): This optional object evaluates
+ * to a package of device names. Each name corresponds to a
+ * device in the ACPI namespace that is associated with the
+ * thermal zone. The temperature reported by the thermal zone
+ * is roughly correspondent to that of each of the devices.
+ */
I don't think that the comment is necessary.
The spec already contains a definition of this object.
Yes, this comment is the description from the spec. I put them there to
save time for those who are reading the code so they don't have to go
back and forth between the documentation and the code.
Do you really want me to remove all of them ?
[ ... ]
Overall, I'm not sure about simple wrappers around
acpi_evaluate_integer/reference() that effectively discard the return
value and don't even bother to sanitize the return value before
returning it to the caller.
Ok, so we don't want to trust the values returned by ACPI.
If all the sanity checks are done in the functions, would it make more
sense then ?
The ones that initialize a trip point make more sense IMO.
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog