Re: [PATCH v2] ACPI: fan: Add hwmon support
From: Armin Wolf
Date: Thu Apr 11 2024 - 19:38:54 EST
Am 11.04.24 um 22:30 schrieb Armin Wolf:
Am 10.04.24 um 16:29 schrieb Mikael Lund Jepsen:
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
You are right, i can expose the target RPM as fan1_target when the fan
is not in fine-grained control mode (otherwise there is no guarantee
that each
control value has an associated fan state entry).
This way, userspace can detect when the fan is stuck. I will send a v3
soon.
Armin Wolf
I just noticed that the drvdata argument of the is_visible callback is marked as const, so i cannot use dev_get_drvdata() on the resulting ACPI device.
Guenter, would it be ok to make drvdata non-const in a separate patch series?
Thanks,
Armin Wolf