Re: [RFC PATCH] hwmon: Add fan monitoring support for HONOR FMI-XX

On 8/12/26 12:22, Nikita Dubrovskih wrote:
> The HONOR FMI-XX firmware exposes a serialized \\GFNS ACPI method.
> It returns a status byte and a 16-bit fan speed in RPM for either of two
> firmware channels.
> 
> Add a DMI-restricted, read-only hwmon driver using that firmware
> interface. The driver deliberately exposes no fan control or direct
> Embedded Controller access.
> 
> The interface was validated on firmware 1.09 with fan channel 0
> reporting approximately 2500-2800 RPM. Channel 1 is readable and
> remained at 0 RPM during idle and a short CPU load.
> 
> Signed-off-by: Nikita Dubrovskih <[email protected]>
> ---
>   Documentation/hwmon/honor-fmi.rst |  32 +++++
>   Documentation/hwmon/index.rst     |   1 +
>   MAINTAINERS                       |   7 ++
>   drivers/hwmon/Kconfig             |  10 ++
>   drivers/hwmon/Makefile            |   1 +
>   drivers/hwmon/honor-fmi.c         | 195 ++++++++++++++++++++++++++++++
>   6 files changed, 246 insertions(+)
>   create mode 100644 Documentation/hwmon/honor-fmi.rst
>   create mode 100644 drivers/hwmon/honor-fmi.c
> 
> diff --git a/Documentation/hwmon/honor-fmi.rst b/Documentation/hwmon/honor-fmi.rst
> new file mode 100644
> index 0000000..a42a1dd
> --- /dev/null
> +++ b/Documentation/hwmon/honor-fmi.rst
> @@ -0,0 +1,32 @@
> +.. SPDX-License-Identifier: GPL-2.0-only
> +
> +Kernel driver honor-fmi
> +=======================
> +
> +Supported systems:
> +
> +  * HONOR FMI-XX
> +
> +Author: Nikita Dubrovskih <[email protected]>
> +
> +Description
> +-----------
> +
> +The driver provides read-only monitoring of the fan speed on the HONOR FMI-XX.
> +The system firmware implements a ``GFNS`` ACPI method which returns the speed
> +of one of two firmware fan channels in RPM. Embedded Controller access and
> +serialization are handled by the firmware method.
> +
> +The driver does not expose fan control or direct Embedded Controller access.
> +
> +Sysfs entries
> +-------------
> +
> +The following attributes are supported:
> +
> +======================= ======= =============================================
> +Name                    Perm    Description
> +======================= ======= =============================================
> +``fan1_input``          RO      Fan channel 0 speed in RPM
> +``fan2_input``          RO      Fan channel 1 speed in RPM
> +======================= ======= =============================================
> diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
> index 29130df..91052fa 100644
> --- a/Documentation/hwmon/index.rst
> +++ b/Documentation/hwmon/index.rst
> @@ -90,6 +90,7 @@ Hardware Monitoring Kernel Drivers
>      gxp-fan-ctrl
>      hac300s
>      hih6130
> +   honor-fmi
>      hp-wmi-sensors
>      hs3001
>      htu31
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 8014b9f..d7355db 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11854,6 +11854,13 @@ F:	lib/test_hmm*
>   F:	mm/hmm*
>   F:	tools/testing/selftests/mm/*hmm*
>   
> +HONOR FMI-XX HARDWARE MONITOR DRIVER
> +M:	Nikita Dubrovskih <[email protected]>
> +L:	[email protected]
> +S:	Maintained
> +F:	Documentation/hwmon/honor-fmi.rst
> +F:	drivers/hwmon/honor-fmi.c
> +
>   HONEYWELL ABP2030PA PRESSURE SENSOR SERIES IIO DRIVER
>   M:	Petre Rodan <[email protected]>
>   L:	[email protected]
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 2bfbcc0..8a11a30 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -2785,6 +2785,16 @@ config SENSORS_ASUS_EC
>   	  This driver can also be built as a module. If so, the module
>   	  will be called asus_ec_sensors.
>   
> +config SENSORS_HONOR_FMI
> +	tristate "HONOR FMI-XX fan monitor"
> +	depends on X86
> +	help
> +	  If you say yes here, you get support for fan speed monitoring on
> +	  the HONOR FMI-XX laptop through its firmware ACPI method.
> +
> +	  This driver can also be built as a module. If so, the module
> +	  will be called honor-fmi.
> +
>   config SENSORS_HP_WMI
>   	tristate "HP WMI Sensors"
>   	depends on ACPI_WMI
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index 63effc0..e098793 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -11,6 +11,7 @@ obj-$(CONFIG_SENSORS_ACPI_POWER) += acpi_power_meter.o
>   obj-$(CONFIG_SENSORS_ATK0110)	+= asus_atk0110.o
>   obj-$(CONFIG_SENSORS_ASUS_EC)	+= asus-ec-sensors.o
>   obj-$(CONFIG_SENSORS_ASUS_WMI)	+= asus_wmi_sensors.o
> +obj-$(CONFIG_SENSORS_HONOR_FMI)	+= honor-fmi.o
>   obj-$(CONFIG_SENSORS_HP_WMI)	+= hp-wmi-sensors.o
>   
>   # Native drivers
> diff --git a/drivers/hwmon/honor-fmi.c b/drivers/hwmon/honor-fmi.c
> new file mode 100644
> index 0000000..225066e
> --- /dev/null
> +++ b/drivers/hwmon/honor-fmi.c
> @@ -0,0 +1,195 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Read-only fan monitoring for the HONOR FMI-XX.
> + *
> + * The firmware-provided \GFNS ACPI method accepts a three-byte buffer.
> + * Byte 2 selects fan 0 or 1. It returns a status byte followed by a
> + * little-endian 16-bit fan speed in RPM. The method owns all Embedded
> + * Controller access and serialization; this driver deliberately exposes no
> + * fan control interface.
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/dmi.h>
> +#include <linux/err.h>
> +#include <linux/hwmon.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/platform_device.h>
> +
> +#define HONOR_FMI_FAN_COUNT 2
> +#define HONOR_FMI_GFNS_RESULT_SIZE 3

#define<space>NAME<tab>value

and align value, please.

> +
> +struct honor_fmi_data {
> +	acpi_handle gfns;
> +	/* Serialize firmware method evaluation. */
> +	struct mutex lock;

I do not see why this lock would be needed on top of the hardware
monitoring subsystem lock.

> +};
> +
> +static const struct dmi_system_id honor_fmi_dmi_table[] = {
> +	{
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "HONOR"),
> +			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "FMI-XX"),
> +		},
> +	},
> +	{}
> +};
> +MODULE_DEVICE_TABLE(dmi, honor_fmi_dmi_table);
> +
> +static int honor_fmi_read_rpm(struct honor_fmi_data *data, int channel,
> +			      long *rpm)
> +{
> +	union acpi_object input = {
> +		.buffer = {
> +			.type = ACPI_TYPE_BUFFER,
> +			.length = 3,
> +		},
> +	};
> +	struct acpi_object_list arguments = {
> +		.count = 1,
> +		.pointer = &input,
> +	};
> +	struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL };
> +	union acpi_object *result;
> +	u8 input_bytes[3] = { 0, 0, channel };
> +	acpi_status status;
> +	int ret = 0;
> +
> +	input.buffer.pointer = input_bytes;
> +
> +	guard(mutex)(&data->lock);
> +	status = acpi_evaluate_object(data->gfns, NULL, &arguments, &output);
> +	if (ACPI_FAILURE(status))
> +		return -EIO;
> +
> +	result = output.pointer;
> +	if (!result || result->type != ACPI_TYPE_BUFFER ||
> +	    result->buffer.length < HONOR_FMI_GFNS_RESULT_SIZE) {
> +		ret = -EPROTO;
> +		goto out_free;
> +	}
> +
> +	if (result->buffer.pointer[0]) {
> +		ret = -EIO;
> +		goto out_free;
> +	}
> +
> +	*rpm = result->buffer.pointer[1] |
> +	       (result->buffer.pointer[2] << 8);
> +
> +out_free:
> +	kfree(output.pointer);
> +	return ret;
> +}
> +
> +static umode_t honor_fmi_is_visible(const void *data,
> +				    enum hwmon_sensor_types type, u32 attr,
> +				    int channel)
> +{
> +	if (type == hwmon_fan && attr == hwmon_fan_input &&
> +	    channel < HONOR_FMI_FAN_COUNT)
> +		return 0444;

Unnecessary check. Just return 0444.

> +
> +	return 0;
> +}
> +
> +static int honor_fmi_read(struct device *dev, enum hwmon_sensor_types type,
> +			  u32 attr, int channel, long *value)
> +{
> +	struct honor_fmi_data *data = dev_get_drvdata(dev);
> +
> +	if (type != hwmon_fan || attr != hwmon_fan_input ||
> +	    channel >= HONOR_FMI_FAN_COUNT)
> +		return -EOPNOTSUPP;

Unnecessary check.

> +
> +	return honor_fmi_read_rpm(data, channel, value);
> +}
> +
> +static const struct hwmon_ops honor_fmi_hwmon_ops = {
> +	.is_visible = honor_fmi_is_visible,
> +	.read = honor_fmi_read,
> +};
> +
> +static const struct hwmon_channel_info * const honor_fmi_hwmon_info[] = {
> +	HWMON_CHANNEL_INFO(fan, HWMON_F_INPUT, HWMON_F_INPUT),
> +	NULL
> +};
> +
> +static const struct hwmon_chip_info honor_fmi_chip_info = {
> +	.ops = &honor_fmi_hwmon_ops,
> +	.info = honor_fmi_hwmon_info,
> +};
> +
> +static int honor_fmi_probe(struct platform_device *pdev)
> +{
> +	struct honor_fmi_data *data;
> +	struct device *hwmon_dev;
> +	acpi_status status;
> +
> +	if (!dmi_check_system(honor_fmi_dmi_table))
> +		return -ENODEV;
> +
> +	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	status = acpi_get_handle(NULL, "\\GFNS", &data->gfns);
> +	if (ACPI_FAILURE(status))
> +		return dev_err_probe(&pdev->dev, -ENODEV,
> +				     "firmware does not provide \\GFNS\n");
> +
> +	mutex_init(&data->lock);
> +	platform_set_drvdata(pdev, data);

I do not see where this is used or needed.

> +
> +	hwmon_dev = devm_hwmon_device_register_with_info(&pdev->dev, "honor_fmi",
> +							 data,
> +							 &honor_fmi_chip_info,
> +							 NULL);
> +	return PTR_ERR_OR_ZERO(hwmon_dev);
> +}
> +
> +static struct platform_driver honor_fmi_driver = {
> +	.probe = honor_fmi_probe,
> +	.driver = {
> +		.name = "honor-fmi-hwmon",
> +	},
> +};
> +
> +static struct platform_device *honor_fmi_device;
> +
> +static int __init honor_fmi_init(void)
> +{
> +	int ret;
> +
> +	if (!dmi_check_system(honor_fmi_dmi_table))
> +		return -ENODEV;
> +
> +	ret = platform_driver_register(&honor_fmi_driver);
> +	if (ret)
> +		return ret;
> +
> +	honor_fmi_device = platform_device_register_simple("honor-fmi-hwmon",
> +							   PLATFORM_DEVID_NONE,
> +							   NULL, 0);
> +	if (IS_ERR(honor_fmi_device)) {
> +		ret = PTR_ERR(honor_fmi_device);
> +		platform_driver_unregister(&honor_fmi_driver);
> +		return ret;

No need to assign the error to ret.

		return PTR_ERR(honor_fmi_device);> +	}
> +
> +	return 0;
> +}
> +
> +static void __exit honor_fmi_exit(void)
> +{
> +	platform_device_unregister(honor_fmi_device);
> +	platform_driver_unregister(&honor_fmi_driver);
> +}
> +
> +module_init(honor_fmi_init);
> +module_exit(honor_fmi_exit);
> +
> +MODULE_AUTHOR("Nikita Dubrovskih <[email protected]>");
> +MODULE_DESCRIPTION("HONOR FMI-XX fan speed monitor");
> +MODULE_LICENSE("GPL");