From: Armin Wolf <W_Armin@gmx.de>
To: Mikael Lund Jepsen <mlj@danelec.com>,
"rafael.j.wysocki@intel.com" <rafael.j.wysocki@intel.com>,
"lenb@kernel.org" <lenb@kernel.org>
Cc: "jdelvare@suse.com" <jdelvare@suse.com>,
"linux@roeck-us.net" <linux@roeck-us.net>,
"linux@weissschuh.net" <linux@weissschuh.net>,
"ilpo.jarvinen@linux.intel.com" <ilpo.jarvinen@linux.intel.com>,
"linux-acpi@vger.kernel.org" <linux-acpi@vger.kernel.org>,
"linux-hwmon@vger.kernel.org" <linux-hwmon@vger.kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"platform-driver-x86@vger.kernel.org"
<platform-driver-x86@vger.kernel.org>
Subject: Re: [PATCH v2] ACPI: fan: Add hwmon support
Date: Fri, 12 Apr 2024 01:38:11 +0200 [thread overview]
Message-ID: <b0899d74-79d7-459c-8f2d-17a17a0f58d7@gmx.de> (raw)
In-Reply-To: <e6798f0a-5e50-41df-ae3e-0069c16abec3@gmx.de>
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@danelec.com>;
>>> rafael.j.wysocki@intel.com; lenb@kernel.org
>>> Cc: jdelvare@suse.com; linux@roeck-us.net; linux@weissschuh.net;
>>> ilpo.jarvinen@linux.intel.com; linux-acpi@vger.kernel.org;
>>> linux-hwmon@vger.kernel.org; linux-kernel@vger.kernel.org;
>>> platform-driver-x86@vger.kernel.org
>>> 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@gmx.de>
>>> ---
>>> 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@gmx.de> */
>>> +
>>> +#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
next prev parent reply other threads:[~2024-04-11 23:38 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-09 21:33 [PATCH v2] ACPI: fan: Add hwmon support Armin Wolf
2024-04-10 14:29 ` Mikael Lund Jepsen
2024-04-11 20:30 ` Armin Wolf
2024-04-11 23:38 ` Armin Wolf [this message]
2024-04-12 0:57 ` Guenter Roeck
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=b0899d74-79d7-459c-8f2d-17a17a0f58d7@gmx.de \
--to=w_armin@gmx.de \
--cc=ilpo.jarvinen@linux.intel.com \
--cc=jdelvare@suse.com \
--cc=lenb@kernel.org \
--cc=linux-acpi@vger.kernel.org \
--cc=linux-hwmon@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux@roeck-us.net \
--cc=linux@weissschuh.net \
--cc=mlj@danelec.com \
--cc=platform-driver-x86@vger.kernel.org \
--cc=rafael.j.wysocki@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).