RE: [PATCH v2] ACPI: fan: Add hwmon support
From: Mikael Lund Jepsen
Date: Wed Apr 10 2024 - 10:29:16 EST
On 9. april 2024 Armin Wolf wrote:
>To: Mikael Lund Jepsen <mlj@xxxxxxxxxxx>; rafael.j.wysocki@xxxxxxxxx; lenb@xxxxxxxxxx
>Cc: jdelvare@xxxxxxxx; linux@xxxxxxxxxxxx; linux@xxxxxxxxxxxxxx; ilpo.jarvinen@xxxxxxxxxxxxxxx; linux-acpi@xxxxxxxxxxxxxxx; linux-hwmon@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; platform-driver-x86@xxxxxxxxxxxxxxx
>Subject: [PATCH v2] ACPI: fan: Add hwmon support
>
>Caution: External email. Do not click links or open attachments unless you recognize the sender and know the content is safe.
>
>
>Currently, the driver does only support a custom sysfs to allow userspace to read the fan speed.
>Add support for the standard hwmon interface so users can read the fan speed with standard tools like "sensors".
>
>Compile-tested only.
>
>Signed-off-by: Armin Wolf <W_Armin@xxxxxx>
>---
>Changes since v1:
>- fix undefined reference error
>- fix fan speed validation
>- coding style fixes
>- clarify that the changes are compile-tested only
>- add hwmon maintainers to cc list
>
>The changes will be tested by Mikael Lund Jepsen from Danelec and should be merged only after those tests.
>---
> drivers/acpi/Makefile | 1 +
> drivers/acpi/fan.h | 9 +++++
> drivers/acpi/fan_core.c | 4 ++
> drivers/acpi/fan_hwmon.c | 83 ++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 97 insertions(+)
> create mode 100644 drivers/acpi/fan_hwmon.c
>
>diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile index d69d5444acdb..c272ab2c93b9 100644
>--- a/drivers/acpi/Makefile
>+++ b/drivers/acpi/Makefile
>@@ -83,6 +83,7 @@ obj-$(CONFIG_ACPI_TINY_POWER_BUTTON) += tiny-power-button.o
> obj-$(CONFIG_ACPI_FAN) += fan.o
> fan-objs := fan_core.o
> fan-objs += fan_attr.o
>+fan-$(CONFIG_HWMON) += fan_hwmon.o
>
> obj-$(CONFIG_ACPI_VIDEO) += video.o
> obj-$(CONFIG_ACPI_TAD) += acpi_tad.o
>diff --git a/drivers/acpi/fan.h b/drivers/acpi/fan.h index e7b4b4e4a55e..97863bdb6303 100644
>--- a/drivers/acpi/fan.h
>+++ b/drivers/acpi/fan.h
>@@ -10,6 +10,8 @@
> #ifndef _ACPI_FAN_H_
> #define _ACPI_FAN_H_
>
>+#include <linux/kconfig.h>
>+
> #define ACPI_FAN_DEVICE_IDS \
> {"INT3404", }, /* Fan */ \
> {"INTC1044", }, /* Fan for Tiger Lake generation */ \ @@ -56,4 +58,11 @@ struct acpi_fan { int acpi_fan_get_fst(struct acpi_device *device, struct acpi_fan_fst *fst); int acpi_fan_create_attributes(struct acpi_device *device); void acpi_fan_delete_attributes(struct acpi_device *device);
>+
>+#if IS_REACHABLE(CONFIG_HWMON)
>+int devm_acpi_fan_create_hwmon(struct acpi_device *device); #else
>+static inline int devm_acpi_fan_create_hwmon(struct acpi_device
>+*device) { return 0; }; #endif
>+
> #endif
>diff --git a/drivers/acpi/fan_core.c b/drivers/acpi/fan_core.c index ff72e4ef8738..7cea4495f19b 100644
>--- a/drivers/acpi/fan_core.c
>+++ b/drivers/acpi/fan_core.c
>@@ -336,6 +336,10 @@ static int acpi_fan_probe(struct platform_device *pdev)
> if (result)
> return result;
>
>+ result = devm_acpi_fan_create_hwmon(device);
>+ if (result)
>+ return result;
>+
> result = acpi_fan_create_attributes(device);
> if (result)
> return result;
>diff --git a/drivers/acpi/fan_hwmon.c b/drivers/acpi/fan_hwmon.c new file mode 100644 index 000000000000..b01055432ded
>--- /dev/null
>+++ b/drivers/acpi/fan_hwmon.c
>@@ -0,0 +1,83 @@
>+// SPDX-License-Identifier: GPL-2.0-or-later
>+/*
>+ * fan_hwmon.c - hwmon interface for the ACPI Fan driver
>+ *
>+ * Copyright (C) 2024 Armin Wolf <W_Armin@xxxxxx> */
>+
>+#include <linux/acpi.h>
>+#include <linux/hwmon.h>
>+#include <linux/limits.h>
>+
>+#include "fan.h"
>+
>+/* Returned when the ACPI fan does not support speed reporting */
>+#define FAN_SPEED_UNAVAILABLE 0xffffffff
>+
>+static umode_t acpi_fan_is_visible(const void *drvdata, enum hwmon_sensor_types type, u32 attr,
>+ int channel) {
>+ return 0444;
>+}
>+
>+static int acpi_fan_read(struct device *dev, enum hwmon_sensor_types type, u32 attr, int channel,
>+ long *val)
>+{
>+ struct acpi_device *adev = dev_get_drvdata(dev);
>+ struct acpi_fan_fst fst;
>+ int ret;
>+
>+ switch (type) {
>+ case hwmon_fan:
>+ ret = acpi_fan_get_fst(adev, &fst);
>+ if (ret < 0)
>+ return ret;
>+
>+ switch (attr) {
>+ case hwmon_fan_input:
>+ if (fst.speed == FAN_SPEED_UNAVAILABLE)
>+ return -ENODATA;
>+
>+ if (fst.speed > LONG_MAX)
>+ return -EOVERFLOW;
>+
>+ *val = fst.speed;
>+ return 0;
>+ case hwmon_fan_fault:
>+ *val = (fst.speed == FAN_SPEED_UNAVAILABLE);
>+ return 0;
>+ default:
>+ break;
>+ }
>+ break;
>+ default:
>+ break;
>+ }
>+
>+ return -EOPNOTSUPP;
>+}
>+
>+static const struct hwmon_ops acpi_fan_ops = {
>+ .is_visible = acpi_fan_is_visible,
>+ .read = acpi_fan_read,
>+};
>+
>+static const struct hwmon_channel_info * const acpi_fan_info[] = {
>+ HWMON_CHANNEL_INFO(fan, HWMON_F_INPUT | HWMON_F_FAULT),
>+ NULL
>+};
>+
>+static const struct hwmon_chip_info acpi_fan_chip_info = {
>+ .ops = &acpi_fan_ops,
>+ .info = acpi_fan_info,
>+};
>+
>+int devm_acpi_fan_create_hwmon(struct acpi_device *device) {
>+ struct device *hdev;
>+
>+ hdev = devm_hwmon_device_register_with_info(&device->dev, "acpi_fan", device,
>+ &acpi_fan_chip_info,
>+ NULL);
>+
>+ return PTR_ERR_OR_ZERO(hdev);
>+}
>--
>2.39.2
>
Sorry about the delay, new to ACPI and needed to create the missing tables to define the fan as an ACPIv4 fan and make it talk to the pse/eclite firmware.
I've tested patch v2 on kernel 6.6.15 on my Intel ElkhartLake CRB board, and it works fine for me:
Fan tacho value is shown correctly and FAULT is reported if ishtp_eclite driver is not loaded.
For reference, my test setup:
- Intel ElkhartLake CRB board w/ Intel Atom x6425RE
- Slimbootloader: Intel MR7 release, with custom ACPI definitions for the fan, and If I remember correctly, I also needed to correct some TGPIO softstraps compared to release defaults
- PseFW: Intel MR7 release, with fixes to enable TGPIO/Tacho reading (which was not enabled in release defaults) and to make sure tacho value is read even when fan is turning off (default PSE FW skipped reading tacho if control value was 0, so last tacho value read while fan was turned on would persist).
One thing that occurred to me: The fan control value is reported in _FST, but not used in acpi-fan, so how can we flag an error in hwmon if the fan is broken, but shall not always be running?
If the control value was exported, then we can alert if tacho is zero when control > 0. I think I'm missing something here?
Thanks,
Mikael