[v4,5/7] hwmon: add driver for the hwmon parts of qnap-mcu devices

Commit Message

Heiko Stuebner Aug. 10, 2024, 6:47 p.m. UTC

The MCU can be found on network-attached-storage devices made by QNAP
and provides access to fan control including reading back its RPM as
well as reading the temperature of the NAS case.

Signed-off-by: Heiko Stuebner <[e-mail]>
---
 Documentation/hwmon/index.rst          |   1 +
 Documentation/hwmon/qnap-mcu-hwmon.rst |  27 ++
 MAINTAINERS                            |   1 +
 drivers/hwmon/Kconfig                  |  12 +
 drivers/hwmon/Makefile                 |   1 +
 drivers/hwmon/qnap-mcu-hwmon.c         | 392 +++++++++++++++++++++++++
 6 files changed, 434 insertions(+)
 create mode 100644 Documentation/hwmon/qnap-mcu-hwmon.rst
 create mode 100644 drivers/hwmon/qnap-mcu-hwmon.c

Comments

Guenter Roeck Aug. 13, 2024, 4:03 p.m. UTC | #1

On 8/10/24 11:47, Heiko Stuebner wrote:
> The MCU can be found on network-attached-storage devices made by QNAP
> and provides access to fan control including reading back its RPM as
> well as reading the temperature of the NAS case.
> 
> Signed-off-by: Heiko Stuebner <[e-mail]>
> ---
>   Documentation/hwmon/index.rst          |   1 +
>   Documentation/hwmon/qnap-mcu-hwmon.rst |  27 ++
>   MAINTAINERS                            |   1 +
>   drivers/hwmon/Kconfig                  |  12 +
>   drivers/hwmon/Makefile                 |   1 +
>   drivers/hwmon/qnap-mcu-hwmon.c         | 392 +++++++++++++++++++++++++
>   6 files changed, 434 insertions(+)
>   create mode 100644 Documentation/hwmon/qnap-mcu-hwmon.rst
>   create mode 100644 drivers/hwmon/qnap-mcu-hwmon.c
> 
> diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
> index 913c11390a457..7adbe23f06597 100644
> --- a/Documentation/hwmon/index.rst
> +++ b/Documentation/hwmon/index.rst
> @@ -199,6 +199,7 @@ Hardware Monitoring Kernel Drivers
>      pxe1610
>      pwm-fan
>      q54sj108a2
> +   qnap-mcu-hwmon
>      raspberrypi-hwmon
>      sbrmi
>      sbtsi_temp
> diff --git a/Documentation/hwmon/qnap-mcu-hwmon.rst b/Documentation/hwmon/qnap-mcu-hwmon.rst
> new file mode 100644
> index 0000000000000..83407e3408f2b
> --- /dev/null
> +++ b/Documentation/hwmon/qnap-mcu-hwmon.rst
> @@ -0,0 +1,27 @@
> +.. SPDX-License-Identifier: GPL-2.0-or-later
> +
> +Kernel driver qnap-mcu-hwmon
> +============================
> +
> +This driver enables the use of the hardware monitoring and fan control
> +of the MCU used on some QNAP network attached storage devices.
> +
> +Author: Heiko Stuebner <[e-mail]>
> +
> +Description
> +-----------
> +
> +The driver implements a simple interface for driving the fan controlled by
> +setting its PWM output value and exposes the fan rpm and case-temperature
> +to user space through hwmon's sysfs interface.
> +
> +The fan rotation speed returned via the optional 'fan1_input' is calculated
> +inside the MCU device.
> +
> +The driver provides the following sensor accesses in sysfs:
> +
> +=============== ======= =======================================================
> +fan1_input	ro	fan tachometer speed in RPM
> +pwm1		rw	relative speed (0-255), 255=max. speed.
> +temp1_input	ro	Measured temperature in millicelsius
> +=============== ======= =======================================================
> diff --git a/MAINTAINERS b/MAINTAINERS
> index baaec814bea1b..282042e3d5f80 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -18655,6 +18655,7 @@ F:	drivers/media/tuners/qm1d1c0042*
>   QNAP MCU DRIVER
>   M:	Heiko Stuebner <[e-mail]>
>   S:	Maintained
> +F:	drivers/hwmon/qnap-mcu-hwmon.c
>   F:	drivers/input/misc/qnap-mcu-input.c
>   F:	drivers/leds/leds-qnap-mcu.c
>   F:	drivers/mfd/qnap-mcu.c
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index b60fe2e58ad6c..0118ad91057e0 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -1793,6 +1793,18 @@ config SENSORS_PWM_FAN
>   	  This driver can also be built as a module. If so, the module
>   	  will be called pwm-fan.
>   
> +config SENSORS_QNAP_MCU_HWMON
> +	tristate "QNAP MCU hardware monitoring"
> +	depends on MFD_QNAP_MCU
> +	depends on THERMAL || THERMAL=n
> +	help
> +	  Say yes here to enable support for fan and temperature sensor
> +	  connected to a QNAP MCU, as found in a number of QNAP network
> +	  attached storage devices.
> +
> +	  This driver can also be built as a module. If so, the module
> +	  will be called qnap-mcu-hwmon.
> +
>   config SENSORS_RASPBERRYPI_HWMON
>   	tristate "Raspberry Pi voltage monitor"
>   	depends on RASPBERRYPI_FIRMWARE || (COMPILE_TEST && !RASPBERRYPI_FIRMWARE)
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index b1c7056c37db5..d60f46a03cc96 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -187,6 +187,7 @@ obj-$(CONFIG_SENSORS_POWERZ)	+= powerz.o
>   obj-$(CONFIG_SENSORS_POWR1220)  += powr1220.o
>   obj-$(CONFIG_SENSORS_PT5161L)	+= pt5161l.o
>   obj-$(CONFIG_SENSORS_PWM_FAN)	+= pwm-fan.o
> +obj-$(CONFIG_SENSORS_QNAP_MCU_HWMON)	+= qnap-mcu-hwmon.o
>   obj-$(CONFIG_SENSORS_RASPBERRYPI_HWMON)	+= raspberrypi-hwmon.o
>   obj-$(CONFIG_SENSORS_SBTSI)	+= sbtsi_temp.o
>   obj-$(CONFIG_SENSORS_SBRMI)	+= sbrmi.o
> diff --git a/drivers/hwmon/qnap-mcu-hwmon.c b/drivers/hwmon/qnap-mcu-hwmon.c
> new file mode 100644
> index 0000000000000..f106fe8b012ea
> --- /dev/null
> +++ b/drivers/hwmon/qnap-mcu-hwmon.c
> @@ -0,0 +1,392 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +/*
> + * Driver for hwmon elements found on QNAP-MCU devices
> + *
> + * Copyright (C) 2024 Heiko Stuebner <[e-mail]>
> + */
> +
> +#include <linux/fwnode.h>
> +#include <linux/hwmon.h>
> +#include <linux/mfd/qnap-mcu.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/property.h>
> +#include <linux/thermal.h>
> +
> +struct qnap_mcu_hwmon {
> +	struct qnap_mcu *mcu;
> +	struct device *dev;
> +
> +	unsigned int pwm_min;
> +	unsigned int pwm_max;
> +
> +	struct fwnode_handle *fan_node;
> +	unsigned int fan_state;
> +	unsigned int fan_max_state;
> +	unsigned int *fan_cooling_levels;
> +
> +	struct thermal_cooling_device *cdev;
> +	struct hwmon_chip_info info;
> +};
> +
> +static int qnap_mcu_hwmon_get_rpm(struct qnap_mcu_hwmon *hwm)
> +{
> +	static const u8 cmd[] = {
> +		[0] = 0x40, /* @ */
> +		[1] = 0x46, /* F */
> +		[2] = 0x41  /* A */
> +	};
> +	u8 reply[6];
> +	int ret;
> +
> +	/* poll the fan rpm */
> +	ret = qnap_mcu_exec(hwm->mcu, cmd, sizeof(cmd), reply, sizeof(reply));
> +	if (ret)
> +		return ret;
> +
> +	/* First 2 bytes must mirror the sent command */
> +	if (memcmp(cmd, reply, 2))
> +		return -EIO;
> +
> +	return reply[4] * 30;
> +}
> +
> +static int qnap_mcu_hwmon_get_pwm(struct qnap_mcu_hwmon *hwm)
> +{
> +	static const u8 cmd[] = {
> +		[0] = 0x40, /* @ */
> +		[1] = 0x46, /* F */
> +		[2] = 0x5a, /* Z */
> +		[3] = 0x30  /* 0 ... fan-id? */
> +	};
> +	u8 reply[4];
> +	int ret;
> +
> +	/* poll the fan pwm */
> +	ret = qnap_mcu_exec(hwm->mcu, cmd, sizeof(cmd), reply, sizeof(reply));
> +	if (ret)
> +		return ret;
> +
> +	/* First 3 bytes must mirror the sent command */
> +	if (memcmp(cmd, reply, 3))
> +		return -EIO;
> +
> +	return reply[3];
> +}
> +
> +static int qnap_mcu_hwmon_set_pwm(struct qnap_mcu_hwmon *hwm, u8 pwm)
> +{
> +	const u8 cmd[] = {
> +		[0] = 0x40, /* @ */
> +		[1] = 0x46, /* F */
> +		[2] = 0x57, /* W */
> +		[3] = 0x30, /* 0 ... fan-id? */
> +		[4] = pwm
> +	};
> +
> +	/* set the fan pwm */
> +	return qnap_mcu_exec_with_ack(hwm->mcu, cmd, sizeof(cmd));
> +}
> +
> +static int qnap_mcu_hwmon_get_temp(struct qnap_mcu_hwmon *hwm)
> +{
> +	static const u8 cmd[] = {
> +		[0] = 0x40, /* @ */
> +		[1] = 0x54, /* T */
> +		[2] = 0x33  /* 3 */
> +	};
> +	u8 reply[4];
> +	int ret;
> +
> +	/* poll the fan rpm */
> +	ret = qnap_mcu_exec(hwm->mcu, cmd, sizeof(cmd), reply, sizeof(reply));
> +	if (ret)
> +		return ret;
> +
> +	/* First bytes must mirror the sent command */
> +	if (memcmp(cmd, reply, sizeof(cmd)))
> +		return -EIO;
> +
> +	/*
> +	 * There is an unknown bit set in bit7.
> +	 * Bits [6:0] report the actual temperature as returned by the
> +	 * original qnap firmware-tools, so just drop bit7 for now.
> +	 */
> +	return (reply[3] & 0x7f) * 1000;
> +}
> +
> +static int qnap_mcu_hwmon_write(struct device *dev, enum hwmon_sensor_types type,
> +				u32 attr, int channel, long val)
> +{
> +	struct qnap_mcu_hwmon *hwm = dev_get_drvdata(dev);
> +
> +	switch (attr) {
> +	case hwmon_pwm_input:
> +		if (val < 0 || val > 255)
> +			return -EINVAL;
> +
> +		if (val != 0)
> +			val = clamp_val(val, hwm->pwm_min, hwm->pwm_max);
> +
> +		return qnap_mcu_hwmon_set_pwm(hwm, val);
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	return 0;
> +}
> +
> +static int qnap_mcu_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
> +			       u32 attr, int channel, long *val)
> +{
> +	struct qnap_mcu_hwmon *hwm = dev_get_drvdata(dev);
> +	int ret;
> +
> +	switch (type) {
> +	case hwmon_pwm:
> +		switch (attr) {
> +		case hwmon_pwm_input:
> +			ret = qnap_mcu_hwmon_get_pwm(hwm);
> +			if (ret < 0)
> +				return ret;
> +
> +			*val = ret;
> +			return 0;
> +		default:
> +			return -EOPNOTSUPP;
> +		}
> +	case hwmon_fan:
> +		ret = qnap_mcu_hwmon_get_rpm(hwm);
> +		if (ret < 0)
> +			return ret;
> +
> +		*val = ret;
> +		return 0;
> +	case hwmon_temp:
> +		ret = qnap_mcu_hwmon_get_temp(hwm);
> +		if (ret < 0)
> +			return ret;
> +
> +		*val = ret;
> +		return 0;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +static umode_t qnap_mcu_hwmon_is_visible(const void *data,
> +					 enum hwmon_sensor_types type,
> +					 u32 attr, int channel)
> +{
> +	switch (type) {
> +	case hwmon_temp:
> +		return 0444;
> +
> +	case hwmon_pwm:
> +		return 0644;
> +
> +	case hwmon_fan:
> +		return 0444;
> +
> +	default:
> +		return 0;
> +	}
> +}
> +
> +static const struct hwmon_ops qnap_mcu_hwmon_hwmon_ops = {
> +	.is_visible = qnap_mcu_hwmon_is_visible,
> +	.read = qnap_mcu_hwmon_read,
> +	.write = qnap_mcu_hwmon_write,
> +};
> +
> +/* thermal cooling device callbacks */
> +static int qnap_mcu_hwmon_get_max_state(struct thermal_cooling_device *cdev,
> +					unsigned long *state)
> +{
> +	struct qnap_mcu_hwmon *hwm = cdev->devdata;
> +
> +	if (!hwm)
> +		return -EINVAL;
> +
> +	*state = hwm->fan_max_state;
> +
> +	return 0;
> +}
> +
> +static int qnap_mcu_hwmon_get_cur_state(struct thermal_cooling_device *cdev,
> +					unsigned long *state)
> +{
> +	struct qnap_mcu_hwmon *hwm = cdev->devdata;
> +
> +	if (!hwm)
> +		return -EINVAL;
> +
> +	*state = hwm->fan_state;
> +
> +	return 0;
> +}
> +
> +static int qnap_mcu_hwmon_set_cur_state(struct thermal_cooling_device *cdev,
> +					unsigned long state)
> +{
> +	struct qnap_mcu_hwmon *hwm = cdev->devdata;
> +	int ret;
> +
> +	if (!hwm || state > hwm->fan_max_state)
> +		return -EINVAL;
> +
> +	if (state == hwm->fan_state)
> +		return 0;
> +
> +	ret = qnap_mcu_hwmon_set_pwm(hwm, hwm->fan_cooling_levels[state]);
> +	if (ret)
> +		return ret;
> +
> +	hwm->fan_state = state;
> +
> +	return ret;
> +}
> +
> +static const struct thermal_cooling_device_ops qnap_mcu_hwmon_cooling_ops = {
> +	.get_max_state = qnap_mcu_hwmon_get_max_state,
> +	.get_cur_state = qnap_mcu_hwmon_get_cur_state,
> +	.set_cur_state = qnap_mcu_hwmon_set_cur_state,
> +};
> +
> +static void devm_fan_node_release(void *data)
> +{
> +	struct qnap_mcu_hwmon *hwm = data;
> +
> +	fwnode_handle_put(hwm->fan_node);
> +}
> +
> +static int qnap_mcu_hwmon_get_cooling_data(struct device *dev, struct qnap_mcu_hwmon *hwm)
> +{
> +	struct fwnode_handle *fwnode;
> +	int num, i, ret;
> +
> +	fwnode = device_get_named_child_node(dev->parent, "fan-0");

Is the dummy "-0" index mandated somewhere ?

I don't care either way, it just seems odd. Either case,

Acked-by: Guenter Roeck <[e-mail]>

> +	if (!fwnode)
> +		return 0;
> +
> +	/* if we found the fan-node, we're keeping it until device-unbind */
> +	hwm->fan_node = fwnode;
> +	ret = devm_add_action_or_reset(dev, devm_fan_node_release, hwm);
> +	if (ret)
> +		return ret;
> +
> +	if (!fwnode_property_present(fwnode, "cooling-levels"))
> +		return 0;
> +

Side note: One could argue that a sub-node with no content does not really
make sense and should be invalid.

> +	ret = fwnode_property_count_u32(fwnode, "cooling-levels");
> +	if (ret <= 0) {
> +		dev_err(dev, "Failed to count elements in 'cooling-levels'\n");
> +		return ret ? : -EINVAL;
> +	}
> +
> +	num = ret;

Another side note: Using ret here isn't necessary. I'd just have used
'num' directly.

> +	hwm->fan_cooling_levels = devm_kcalloc(dev, num, sizeof(u32),
> +					       GFP_KERNEL);
> +	if (!hwm->fan_cooling_levels)
> +		return -ENOMEM;
> +
> +	ret = fwnode_property_read_u32_array(fwnode, "cooling-levels",
> +					     hwm->fan_cooling_levels, num);
> +	if (ret) {
> +		dev_err(dev, "Failed to read 'cooling-levels'\n");
> +		return ret;
> +	}
> +
> +	for (i = 0; i < num; i++) {
> +		if (hwm->fan_cooling_levels[i] > hwm->pwm_max) {
> +			dev_err(dev, "fan state[%d]:%d > %d\n", i,
> +				hwm->fan_cooling_levels[i], hwm->pwm_max);
> +			return -EINVAL;

In case you send another version, you might want to consider using dev_err_probe().

> +		}
> +	}
> +
> +	hwm->fan_max_state = num - 1;
> +
> +	return 0;
> +}
> +
> +static const struct hwmon_channel_info * const qnap_mcu_hwmon_channels[] = {
> +	HWMON_CHANNEL_INFO(pwm, HWMON_PWM_INPUT),
> +	HWMON_CHANNEL_INFO(fan, HWMON_F_INPUT),
> +	HWMON_CHANNEL_INFO(temp, HWMON_T_INPUT),
> +	NULL
> +};
> +
> +static int qnap_mcu_hwmon_probe(struct platform_device *pdev)
> +{
> +	struct qnap_mcu *mcu = dev_get_drvdata(pdev->dev.parent);
> +	const struct qnap_mcu_variant *variant = qnap_mcu_get_variant_data(mcu);
> +	struct qnap_mcu_hwmon *hwm;
> +	struct thermal_cooling_device *cdev;
> +	struct device *dev = &pdev->dev;
> +	struct device *hwmon;
> +	int ret;
> +
> +	hwm = devm_kzalloc(dev, sizeof(*hwm), GFP_KERNEL);
> +	if (!hwm)
> +		return -ENOMEM;
> +
> +	hwm->mcu = mcu;
> +	hwm->dev = &pdev->dev;
> +	hwm->pwm_min = variant->fan_pwm_min;
> +	hwm->pwm_max = variant->fan_pwm_max;
> +
> +	platform_set_drvdata(pdev, hwm);
> +
> +	/*
> +	 * Set duty cycle to maximum allowed.
> +	 */
> +	ret = qnap_mcu_hwmon_set_pwm(hwm, hwm->pwm_max);
> +	if (ret)
> +		return ret;
> +
> +	hwm->info.ops = &qnap_mcu_hwmon_hwmon_ops;
> +	hwm->info.info = qnap_mcu_hwmon_channels;
> +
> +	ret = qnap_mcu_hwmon_get_cooling_data(dev, hwm);
> +	if (ret)
> +		return ret;
> +
> +	hwm->fan_state = hwm->fan_max_state;
> +
> +	hwmon = devm_hwmon_device_register_with_info(dev, "qnapmcu",
> +						     hwm, &hwm->info, NULL);
> +	if (IS_ERR(hwmon))
> +		return dev_err_probe(dev, PTR_ERR(hwmon), "Failed to register hwmon device\n");
> +
> +	/*
> +	 * Only register cooling device when we found cooling-levels.
> +	 * qnap_mcu_hwmon_get_cooling_data() will fail when reading malformed
> +	 * levels and only succeed with either no or correct cooling levels.
> +	 */
> +	if (IS_ENABLED(CONFIG_THERMAL) && hwm->fan_cooling_levels) {
> +		cdev = devm_thermal_of_cooling_device_register(dev,
> +					to_of_node(hwm->fan_node), "qnap-mcu-hwmon",
> +					hwm, &qnap_mcu_hwmon_cooling_ops);
> +		if (IS_ERR(cdev))
> +			return dev_err_probe(dev, PTR_ERR(cdev),
> +				"Failed to register qnap-mcu-hwmon as cooling device\n");
> +		hwm->cdev = cdev;
> +	}
> +
> +	return 0;
> +}
> +
> +static struct platform_driver qnap_mcu_hwmon_driver = {
> +	.probe = qnap_mcu_hwmon_probe,
> +	.driver = {
> +		.name = "qnap-mcu-hwmon",
> +	},
> +};
> +module_platform_driver(qnap_mcu_hwmon_driver);
> +
> +MODULE_ALIAS("platform:qnap-mcu-hwmon");
> +MODULE_AUTHOR("Heiko Stuebner <[e-mail]>");
> +MODULE_DESCRIPTION("QNAP MCU hwmon driver");
> +MODULE_LICENSE("GPL");

Heiko Stuebner Aug. 13, 2024, 8:39 p.m. UTC | #2

Hi Guenter,

Am Dienstag, 13. August 2024, 18:03:57 CEST schrieb Guenter Roeck:
> On 8/10/24 11:47, Heiko Stuebner wrote:

> > +static int qnap_mcu_hwmon_set_pwm(struct qnap_mcu_hwmon *hwm, u8 pwm)
> > +{
> > +	const u8 cmd[] = {
> > +		[0] = 0x40, /* @ */
> > +		[1] = 0x46, /* F */
> > +		[2] = 0x57, /* W */
> > +		[3] = 0x30, /* 0 ... fan-id? */
> > +		[4] = pwm
> > +	};
> > +
> > +	/* set the fan pwm */
> > +	return qnap_mcu_exec_with_ack(hwm->mcu, cmd, sizeof(cmd));
> > +}

> > +static int qnap_mcu_hwmon_get_cooling_data(struct device *dev, struct qnap_mcu_hwmon *hwm)
> > +{
> > +	struct fwnode_handle *fwnode;
> > +	int num, i, ret;
> > +
> > +	fwnode = device_get_named_child_node(dev->parent, "fan-0");
> 
> Is the dummy "-0" index mandated somewhere ?

I don't think it is. The node should just begin with "fan" I think.

I've added the -0 because from everything I've seen of the qnap software-
side, the mcu firmware can address multiple fans.

In the original firmware, everything is done in userspace wrt. the MCU,
and the fan commands in their abstraction layer allow for multiple fans.

Also there is that suspicious "0" in the command sequence when
getting/setting the fan pwm. And in the original device config this is
labeled as the first fan.

From everything I've seen, the MCU is not limited to the Rockchip-line
of devices and I do hope others will find this useful in the future,
so adding the "-0" was a better safe than sorry choice.

Because that way adding that theoretical 2nd fan in the future won't
cause too much trouble in the dt-binding.


> I don't care either way, it just seems odd. Either case,
> 
> Acked-by: Guenter Roeck <[e-mail]>
> 
> > +	if (!fwnode)
> > +		return 0;
> > +
> > +	/* if we found the fan-node, we're keeping it until device-unbind */
> > +	hwm->fan_node = fwnode;
> > +	ret = devm_add_action_or_reset(dev, devm_fan_node_release, hwm);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (!fwnode_property_present(fwnode, "cooling-levels"))
> > +		return 0;
> > +
> 
> Side note: One could argue that a sub-node with no content does not really
> make sense and should be invalid.

I remember thinking about that, but didn't come to a real decision on my
own, hence left it as it was. So will follow your suggestion :-)


> > +	ret = fwnode_property_count_u32(fwnode, "cooling-levels");
> > +	if (ret <= 0) {
> > +		dev_err(dev, "Failed to count elements in 'cooling-levels'\n");
> > +		return ret ? : -EINVAL;
> > +	}
> > +
> > +	num = ret;
> 
> Another side note: Using ret here isn't necessary. I'd just have used
> 'num' directly.

will do

> 
> > +	hwm->fan_cooling_levels = devm_kcalloc(dev, num, sizeof(u32),
> > +					       GFP_KERNEL);
> > +	if (!hwm->fan_cooling_levels)
> > +		return -ENOMEM;
> > +
> > +	ret = fwnode_property_read_u32_array(fwnode, "cooling-levels",
> > +					     hwm->fan_cooling_levels, num);
> > +	if (ret) {
> > +		dev_err(dev, "Failed to read 'cooling-levels'\n");
> > +		return ret;
> > +	}
> > +
> > +	for (i = 0; i < num; i++) {
> > +		if (hwm->fan_cooling_levels[i] > hwm->pwm_max) {
> > +			dev_err(dev, "fan state[%d]:%d > %d\n", i,
> > +				hwm->fan_cooling_levels[i], hwm->pwm_max);
> > +			return -EINVAL;
> 
> In case you send another version, you might want to consider using dev_err_probe().

ok will do.

I was probably way overthinking if I should not use dev_err_probe in a
function that is not a probe function (though of course part of the probe
process).


Thanks a lot for looking over the code once again
Heiko

Patch

diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
index 913c11390a457..7adbe23f06597 100644
--- a/Documentation/hwmon/index.rst
+++ b/Documentation/hwmon/index.rst
@@ -199,6 +199,7 @@  Hardware Monitoring Kernel Drivers
    pxe1610
    pwm-fan
    q54sj108a2
+   qnap-mcu-hwmon
    raspberrypi-hwmon
    sbrmi
    sbtsi_temp
diff --git a/Documentation/hwmon/qnap-mcu-hwmon.rst b/Documentation/hwmon/qnap-mcu-hwmon.rst
new file mode 100644
index 0000000000000..83407e3408f2b
--- /dev/null
+++ b/Documentation/hwmon/qnap-mcu-hwmon.rst
@@ -0,0 +1,27 @@ 
+.. SPDX-License-Identifier: GPL-2.0-or-later
+
+Kernel driver qnap-mcu-hwmon
+============================
+
+This driver enables the use of the hardware monitoring and fan control
+of the MCU used on some QNAP network attached storage devices.
+
+Author: Heiko Stuebner <[e-mail]>
+
+Description
+-----------
+
+The driver implements a simple interface for driving the fan controlled by
+setting its PWM output value and exposes the fan rpm and case-temperature
+to user space through hwmon's sysfs interface.
+
+The fan rotation speed returned via the optional 'fan1_input' is calculated
+inside the MCU device.
+
+The driver provides the following sensor accesses in sysfs:
+
+=============== ======= =======================================================
+fan1_input	ro	fan tachometer speed in RPM
+pwm1		rw	relative speed (0-255), 255=max. speed.
+temp1_input	ro	Measured temperature in millicelsius
+=============== ======= =======================================================
diff --git a/MAINTAINERS b/MAINTAINERS
index baaec814bea1b..282042e3d5f80 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -18655,6 +18655,7 @@  F:	drivers/media/tuners/qm1d1c0042*
 QNAP MCU DRIVER
 M:	Heiko Stuebner <[e-mail]>
 S:	Maintained
+F:	drivers/hwmon/qnap-mcu-hwmon.c
 F:	drivers/input/misc/qnap-mcu-input.c
 F:	drivers/leds/leds-qnap-mcu.c
 F:	drivers/mfd/qnap-mcu.c
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index b60fe2e58ad6c..0118ad91057e0 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -1793,6 +1793,18 @@  config SENSORS_PWM_FAN
 	  This driver can also be built as a module. If so, the module
 	  will be called pwm-fan.
 
+config SENSORS_QNAP_MCU_HWMON
+	tristate "QNAP MCU hardware monitoring"
+	depends on MFD_QNAP_MCU
+	depends on THERMAL || THERMAL=n
+	help
+	  Say yes here to enable support for fan and temperature sensor
+	  connected to a QNAP MCU, as found in a number of QNAP network
+	  attached storage devices.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called qnap-mcu-hwmon.
+
 config SENSORS_RASPBERRYPI_HWMON
 	tristate "Raspberry Pi voltage monitor"
 	depends on RASPBERRYPI_FIRMWARE || (COMPILE_TEST && !RASPBERRYPI_FIRMWARE)
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index b1c7056c37db5..d60f46a03cc96 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -187,6 +187,7 @@  obj-$(CONFIG_SENSORS_POWERZ)	+= powerz.o
 obj-$(CONFIG_SENSORS_POWR1220)  += powr1220.o
 obj-$(CONFIG_SENSORS_PT5161L)	+= pt5161l.o
 obj-$(CONFIG_SENSORS_PWM_FAN)	+= pwm-fan.o
+obj-$(CONFIG_SENSORS_QNAP_MCU_HWMON)	+= qnap-mcu-hwmon.o
 obj-$(CONFIG_SENSORS_RASPBERRYPI_HWMON)	+= raspberrypi-hwmon.o
 obj-$(CONFIG_SENSORS_SBTSI)	+= sbtsi_temp.o
 obj-$(CONFIG_SENSORS_SBRMI)	+= sbrmi.o
diff --git a/drivers/hwmon/qnap-mcu-hwmon.c b/drivers/hwmon/qnap-mcu-hwmon.c
new file mode 100644
index 0000000000000..f106fe8b012ea
--- /dev/null
+++ b/drivers/hwmon/qnap-mcu-hwmon.c
@@ -0,0 +1,392 @@ 
+// SPDX-License-Identifier: GPL-2.0-only
+
+/*
+ * Driver for hwmon elements found on QNAP-MCU devices
+ *
+ * Copyright (C) 2024 Heiko Stuebner <[e-mail]>
+ */
+
+#include <linux/fwnode.h>
+#include <linux/hwmon.h>
+#include <linux/mfd/qnap-mcu.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/property.h>
+#include <linux/thermal.h>
+
+struct qnap_mcu_hwmon {
+	struct qnap_mcu *mcu;
+	struct device *dev;
+
+	unsigned int pwm_min;
+	unsigned int pwm_max;
+
+	struct fwnode_handle *fan_node;
+	unsigned int fan_state;
+	unsigned int fan_max_state;
+	unsigned int *fan_cooling_levels;
+
+	struct thermal_cooling_device *cdev;
+	struct hwmon_chip_info info;
+};
+
+static int qnap_mcu_hwmon_get_rpm(struct qnap_mcu_hwmon *hwm)
+{
+	static const u8 cmd[] = {
+		[0] = 0x40, /* @ */
+		[1] = 0x46, /* F */
+		[2] = 0x41  /* A */
+	};
+	u8 reply[6];
+	int ret;
+
+	/* poll the fan rpm */
+	ret = qnap_mcu_exec(hwm->mcu, cmd, sizeof(cmd), reply, sizeof(reply));
+	if (ret)
+		return ret;
+
+	/* First 2 bytes must mirror the sent command */
+	if (memcmp(cmd, reply, 2))
+		return -EIO;
+
+	return reply[4] * 30;
+}
+
+static int qnap_mcu_hwmon_get_pwm(struct qnap_mcu_hwmon *hwm)
+{
+	static const u8 cmd[] = {
+		[0] = 0x40, /* @ */
+		[1] = 0x46, /* F */
+		[2] = 0x5a, /* Z */
+		[3] = 0x30  /* 0 ... fan-id? */
+	};
+	u8 reply[4];
+	int ret;
+
+	/* poll the fan pwm */
+	ret = qnap_mcu_exec(hwm->mcu, cmd, sizeof(cmd), reply, sizeof(reply));
+	if (ret)
+		return ret;
+
+	/* First 3 bytes must mirror the sent command */
+	if (memcmp(cmd, reply, 3))
+		return -EIO;
+
+	return reply[3];
+}
+
+static int qnap_mcu_hwmon_set_pwm(struct qnap_mcu_hwmon *hwm, u8 pwm)
+{
+	const u8 cmd[] = {
+		[0] = 0x40, /* @ */
+		[1] = 0x46, /* F */
+		[2] = 0x57, /* W */
+		[3] = 0x30, /* 0 ... fan-id? */
+		[4] = pwm
+	};
+
+	/* set the fan pwm */
+	return qnap_mcu_exec_with_ack(hwm->mcu, cmd, sizeof(cmd));
+}
+
+static int qnap_mcu_hwmon_get_temp(struct qnap_mcu_hwmon *hwm)
+{
+	static const u8 cmd[] = {
+		[0] = 0x40, /* @ */
+		[1] = 0x54, /* T */
+		[2] = 0x33  /* 3 */
+	};
+	u8 reply[4];
+	int ret;
+
+	/* poll the fan rpm */
+	ret = qnap_mcu_exec(hwm->mcu, cmd, sizeof(cmd), reply, sizeof(reply));
+	if (ret)
+		return ret;
+
+	/* First bytes must mirror the sent command */
+	if (memcmp(cmd, reply, sizeof(cmd)))
+		return -EIO;
+
+	/*
+	 * There is an unknown bit set in bit7.
+	 * Bits [6:0] report the actual temperature as returned by the
+	 * original qnap firmware-tools, so just drop bit7 for now.
+	 */
+	return (reply[3] & 0x7f) * 1000;
+}
+
+static int qnap_mcu_hwmon_write(struct device *dev, enum hwmon_sensor_types type,
+				u32 attr, int channel, long val)
+{
+	struct qnap_mcu_hwmon *hwm = dev_get_drvdata(dev);
+
+	switch (attr) {
+	case hwmon_pwm_input:
+		if (val < 0 || val > 255)
+			return -EINVAL;
+
+		if (val != 0)
+			val = clamp_val(val, hwm->pwm_min, hwm->pwm_max);
+
+		return qnap_mcu_hwmon_set_pwm(hwm, val);
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	return 0;
+}
+
+static int qnap_mcu_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
+			       u32 attr, int channel, long *val)
+{
+	struct qnap_mcu_hwmon *hwm = dev_get_drvdata(dev);
+	int ret;
+
+	switch (type) {
+	case hwmon_pwm:
+		switch (attr) {
+		case hwmon_pwm_input:
+			ret = qnap_mcu_hwmon_get_pwm(hwm);
+			if (ret < 0)
+				return ret;
+
+			*val = ret;
+			return 0;
+		default:
+			return -EOPNOTSUPP;
+		}
+	case hwmon_fan:
+		ret = qnap_mcu_hwmon_get_rpm(hwm);
+		if (ret < 0)
+			return ret;
+
+		*val = ret;
+		return 0;
+	case hwmon_temp:
+		ret = qnap_mcu_hwmon_get_temp(hwm);
+		if (ret < 0)
+			return ret;
+
+		*val = ret;
+		return 0;
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static umode_t qnap_mcu_hwmon_is_visible(const void *data,
+					 enum hwmon_sensor_types type,
+					 u32 attr, int channel)
+{
+	switch (type) {
+	case hwmon_temp:
+		return 0444;
+
+	case hwmon_pwm:
+		return 0644;
+
+	case hwmon_fan:
+		return 0444;
+
+	default:
+		return 0;
+	}
+}
+
+static const struct hwmon_ops qnap_mcu_hwmon_hwmon_ops = {
+	.is_visible = qnap_mcu_hwmon_is_visible,
+	.read = qnap_mcu_hwmon_read,
+	.write = qnap_mcu_hwmon_write,
+};
+
+/* thermal cooling device callbacks */
+static int qnap_mcu_hwmon_get_max_state(struct thermal_cooling_device *cdev,
+					unsigned long *state)
+{
+	struct qnap_mcu_hwmon *hwm = cdev->devdata;
+
+	if (!hwm)
+		return -EINVAL;
+
+	*state = hwm->fan_max_state;
+
+	return 0;
+}
+
+static int qnap_mcu_hwmon_get_cur_state(struct thermal_cooling_device *cdev,
+					unsigned long *state)
+{
+	struct qnap_mcu_hwmon *hwm = cdev->devdata;
+
+	if (!hwm)
+		return -EINVAL;
+
+	*state = hwm->fan_state;
+
+	return 0;
+}
+
+static int qnap_mcu_hwmon_set_cur_state(struct thermal_cooling_device *cdev,
+					unsigned long state)
+{
+	struct qnap_mcu_hwmon *hwm = cdev->devdata;
+	int ret;
+
+	if (!hwm || state > hwm->fan_max_state)
+		return -EINVAL;
+
+	if (state == hwm->fan_state)
+		return 0;
+
+	ret = qnap_mcu_hwmon_set_pwm(hwm, hwm->fan_cooling_levels[state]);
+	if (ret)
+		return ret;
+
+	hwm->fan_state = state;
+
+	return ret;
+}
+
+static const struct thermal_cooling_device_ops qnap_mcu_hwmon_cooling_ops = {
+	.get_max_state = qnap_mcu_hwmon_get_max_state,
+	.get_cur_state = qnap_mcu_hwmon_get_cur_state,
+	.set_cur_state = qnap_mcu_hwmon_set_cur_state,
+};
+
+static void devm_fan_node_release(void *data)
+{
+	struct qnap_mcu_hwmon *hwm = data;
+
+	fwnode_handle_put(hwm->fan_node);
+}
+
+static int qnap_mcu_hwmon_get_cooling_data(struct device *dev, struct qnap_mcu_hwmon *hwm)
+{
+	struct fwnode_handle *fwnode;
+	int num, i, ret;
+
+	fwnode = device_get_named_child_node(dev->parent, "fan-0");
+	if (!fwnode)
+		return 0;
+
+	/* if we found the fan-node, we're keeping it until device-unbind */
+	hwm->fan_node = fwnode;
+	ret = devm_add_action_or_reset(dev, devm_fan_node_release, hwm);
+	if (ret)
+		return ret;
+
+	if (!fwnode_property_present(fwnode, "cooling-levels"))
+		return 0;
+
+	ret = fwnode_property_count_u32(fwnode, "cooling-levels");
+	if (ret <= 0) {
+		dev_err(dev, "Failed to count elements in 'cooling-levels'\n");
+		return ret ? : -EINVAL;
+	}
+
+	num = ret;
+	hwm->fan_cooling_levels = devm_kcalloc(dev, num, sizeof(u32),
+					       GFP_KERNEL);
+	if (!hwm->fan_cooling_levels)
+		return -ENOMEM;
+
+	ret = fwnode_property_read_u32_array(fwnode, "cooling-levels",
+					     hwm->fan_cooling_levels, num);
+	if (ret) {
+		dev_err(dev, "Failed to read 'cooling-levels'\n");
+		return ret;
+	}
+
+	for (i = 0; i < num; i++) {
+		if (hwm->fan_cooling_levels[i] > hwm->pwm_max) {
+			dev_err(dev, "fan state[%d]:%d > %d\n", i,
+				hwm->fan_cooling_levels[i], hwm->pwm_max);
+			return -EINVAL;
+		}
+	}
+
+	hwm->fan_max_state = num - 1;
+
+	return 0;
+}
+
+static const struct hwmon_channel_info * const qnap_mcu_hwmon_channels[] = {
+	HWMON_CHANNEL_INFO(pwm, HWMON_PWM_INPUT),
+	HWMON_CHANNEL_INFO(fan, HWMON_F_INPUT),
+	HWMON_CHANNEL_INFO(temp, HWMON_T_INPUT),
+	NULL
+};
+
+static int qnap_mcu_hwmon_probe(struct platform_device *pdev)
+{
+	struct qnap_mcu *mcu = dev_get_drvdata(pdev->dev.parent);
+	const struct qnap_mcu_variant *variant = qnap_mcu_get_variant_data(mcu);
+	struct qnap_mcu_hwmon *hwm;
+	struct thermal_cooling_device *cdev;
+	struct device *dev = &pdev->dev;
+	struct device *hwmon;
+	int ret;
+
+	hwm = devm_kzalloc(dev, sizeof(*hwm), GFP_KERNEL);
+	if (!hwm)
+		return -ENOMEM;
+
+	hwm->mcu = mcu;
+	hwm->dev = &pdev->dev;
+	hwm->pwm_min = variant->fan_pwm_min;
+	hwm->pwm_max = variant->fan_pwm_max;
+
+	platform_set_drvdata(pdev, hwm);
+
+	/*
+	 * Set duty cycle to maximum allowed.
+	 */
+	ret = qnap_mcu_hwmon_set_pwm(hwm, hwm->pwm_max);
+	if (ret)
+		return ret;
+
+	hwm->info.ops = &qnap_mcu_hwmon_hwmon_ops;
+	hwm->info.info = qnap_mcu_hwmon_channels;
+
+	ret = qnap_mcu_hwmon_get_cooling_data(dev, hwm);
+	if (ret)
+		return ret;
+
+	hwm->fan_state = hwm->fan_max_state;
+
+	hwmon = devm_hwmon_device_register_with_info(dev, "qnapmcu",
+						     hwm, &hwm->info, NULL);
+	if (IS_ERR(hwmon))
+		return dev_err_probe(dev, PTR_ERR(hwmon), "Failed to register hwmon device\n");
+
+	/*
+	 * Only register cooling device when we found cooling-levels.
+	 * qnap_mcu_hwmon_get_cooling_data() will fail when reading malformed
+	 * levels and only succeed with either no or correct cooling levels.
+	 */
+	if (IS_ENABLED(CONFIG_THERMAL) && hwm->fan_cooling_levels) {
+		cdev = devm_thermal_of_cooling_device_register(dev,
+					to_of_node(hwm->fan_node), "qnap-mcu-hwmon",
+					hwm, &qnap_mcu_hwmon_cooling_ops);
+		if (IS_ERR(cdev))
+			return dev_err_probe(dev, PTR_ERR(cdev),
+				"Failed to register qnap-mcu-hwmon as cooling device\n");
+		hwm->cdev = cdev;
+	}
+
+	return 0;
+}
+
+static struct platform_driver qnap_mcu_hwmon_driver = {
+	.probe = qnap_mcu_hwmon_probe,
+	.driver = {
+		.name = "qnap-mcu-hwmon",
+	},
+};
+module_platform_driver(qnap_mcu_hwmon_driver);
+
+MODULE_ALIAS("platform:qnap-mcu-hwmon");
+MODULE_AUTHOR("Heiko Stuebner <[e-mail]>");
+MODULE_DESCRIPTION("QNAP MCU hwmon driver");
+MODULE_LICENSE("GPL");