Message-ID: <9572496f-f6b8-4f66-b084-de7644ea60a8@roeck-us.net>
Date: Fri, 26 Jul 2024 07:15:50 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: Heiko Stuebner <heiko@...ech.de>, lee@...nel.org, jdelvare@...e.com,
 dmitry.torokhov@...il.com, pavel@....cz
Cc: robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org,
 ukleinek@...ian.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
 linux-rockchip@...ts.infradead.org, linux-hwmon@...r.kernel.org,
 linux-input@...r.kernel.org, linux-leds@...r.kernel.org
Subject: Re: [PATCH 5/7] hwmon: add driver for the hwmon parts of qnap-mcu
 devices

On 7/25/24 12:45, 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 <heiko@...ech.de>

checkpatch --strict:

total: 0 errors, 1 warnings, 9 checks, 377 lines checked

Te warning is about the MAINTAINERS file, which is your call,
but please fix the check messages.

That does make me wonder, though. It looks like you don't plan to sign up
to maintain this code. What do you expect to happen with it after it
has been accepted ?

> ---
>   drivers/hwmon/Kconfig          |  12 ++
>   drivers/hwmon/Makefile         |   1 +
>   drivers/hwmon/qnap-mcu-hwmon.c | 352 +++++++++++++++++++++++++++++++++

Documentation (Documentation/hwmon/...) missing.

>   3 files changed, 365 insertions(+)
>   create mode 100644 drivers/hwmon/qnap-mcu-hwmon.c
> 
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index e14ae18a973b3..87b6ee463a967 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -1806,6 +1806,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 fans and temperatures sensors

s/temperatures/temperature/

> +	  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 e3f25475d1f04..b72c6a059e4af 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -188,6 +188,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..c4925ece165e2
> --- /dev/null
> +++ b/drivers/hwmon/qnap-mcu-hwmon.c
> @@ -0,0 +1,352 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +/*
> + * Driver for hwmon elements found on QNAP-MCU devices
> + *
> + * Copyright (C) 2024 Heiko Stuebner <heiko@...ech.de>
> + */
> +
> +#include <linux/module.h>
> +#include <linux/hwmon.h>
> +#include <linux/mfd/qnap-mcu.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;
> +
> +	unsigned int pwm_value;
> +	bool pwm_enabled;
> +
> +	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)
> +{
> +	u8 cmd[] = {
> +		[0] = 0x40, /* @ */
> +		[1] = 0x46, /* F */
> +		[2] = 0x41  /* A */
> +	};

Can this be static ?

> +	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)) {
> +		dev_err(hwm->dev, "malformed data received\n");

Pleas drop those error messages.

> +		return -EIO;
> +	}
> +
> +	return reply[4] * 450;
> +}
> +
> +static int qnap_mcu_hwmon_update_fan(struct qnap_mcu_hwmon *hwm)
> +{
> +	u8 cmd[] = {
> +		[0] = 0x40, /* @ */
> +		[1] = 0x46, /* F */
> +		[2] = 0x57, /* W */
> +		[3] = 0x30, /* 0 ... fan-id? */
> +		[4] = hwm->pwm_enabled ? hwm->pwm_value : 0

Per ABI, pwm_enabled == 0 is
		- 0: no fan speed control (i.e. fan at full speed)
This does the opposite.

I am not sure I understand the intent of pwm_enable.
It seems to me it is supposed to turn off the fan, which
is identical to just setting the pwm value to 0.
If so, why have this attribute in the first place ?

> +	};
> +
> +	/* poll the fan rpm */
> +	return qnap_mcu_exec_with_ack(hwm->mcu, cmd, sizeof(cmd));
> +}
> +
> +static int qnap_mcu_hwmon_get_temp(struct qnap_mcu_hwmon *hwm)
> +{
> +	u8 cmd[] = {
> +		[0] = 0x40, /* @ */
> +		[1] = 0x54, /* T */
> +		[2] = 0x33  /* 3 */
> +	};

static ?

> +	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))) {
> +		dev_err(hwm->dev, "malformed data received\n");

Same as above.

> +		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;
> +}
> +
> +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 < hwm->pwm_min || val > hwm->pwm_max)
> +			return -EINVAL;
> +

I'd suggest to bail out if the value is outside (0, 255) and then
clamp to (pwm_min, pwm_max). Users of the API are expected to know that
pwm values are between 0 and 255, but not what the actual value range is.

> +		hwm->pwm_value = val;
> +		return qnap_mcu_hwmon_update_fan(hwm);
> +	case hwmon_pwm_enable:
> +		if (val < 0 || val > 1)
> +			return -EINVAL;
> +
> +		hwm->pwm_enabled = val;
> +		return qnap_mcu_hwmon_update_fan(hwm);
> +	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);
> +
> +	switch (type) {
> +	case hwmon_pwm:
> +		switch (attr) {
> +		case hwmon_pwm_input:
> +			*val = hwm->pwm_value;

This doesn't return a real pwm value. It returns the most recently configured
value, or 0 if no value was ever set. That makes me really unhappy. At the very least,
if it is indeed impossible to get the actual pwm value from the device, it needs to be
very clearly documented.

> +			return 0;
> +		case hwmon_pwm_enable:
> +			*val = hwm->pwm_enabled;
> +			return 0;

		default:
missing

> +		}
> +		return -EOPNOTSUPP;
> +	case hwmon_fan:
> +		*val = qnap_mcu_hwmon_get_rpm(hwm);

This returns errors as negative rpm values.

> +		return 0;
> +	case hwmon_temp:
> +		*val = qnap_mcu_hwmon_get_temp(hwm);

hwmon ABI expects the temperature in degrees C. This returns
a value between 0 and 0x7f, or in other words temperatures
between 0 and 0.127 degrees C. On top of that, it returns errors
as negative temperatures.

Your call how to fix this. Either
		ret = qnap_mcu_hwmon_get_temp(hwm);
		if (ret < 0)
			return ret;
		*val = ret * 1000;
		return 0;
or
		return qnap_mcu_hwmon_get_temp(hwm, val);

and implement the conversion and return value assignment in
qnap_mcu_hwmon_get_temp().

> +		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))

Extra ( )

> +		return -EINVAL;
> +
> +	if (state == hwm->fan_state)
> +		return 0;
> +
> +	if (hwm->fan_cooling_levels[state]) {
> +		hwm->pwm_enabled = 1;
> +		hwm->pwm_value = hwm->fan_cooling_levels[state];
> +	} else {
> +		hwm->pwm_enabled = 0;
> +	}
> +
> +	ret = qnap_mcu_hwmon_update_fan(hwm);
> +	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 int qnap_mcu_hwmon_get_cooling_data(struct device *dev, struct qnap_mcu_hwmon *hwm)
> +{
> +	int num, i, ret;
> +
> +	if (!device_property_present(dev->parent, "cooling-levels"))
> +		return 0;
> +
> +	ret = device_property_count_u32(dev->parent, "cooling-levels");
> +	if (ret <= 0) {
> +		dev_err(dev, "Wrong data!\n");

That doesn't really mean anything to the user. Please refer to
'cooling-levels' like below.

> +		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 = device_property_read_u32_array(dev->parent, "cooling-levels",
> +					     hwm->fan_cooling_levels, num);
> +	if (ret) {
> +		dev_err(dev, "Property 'cooling-levels' cannot be read!\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_PWM_ENABLE),
> +	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 and enable PWM output as well as
> +	 * the regulator. In case of error nothing is changed
> +	 */
> +	hwm->pwm_value = hwm->pwm_max;
> +	hwm->pwm_enabled = 1;
> +	qnap_mcu_hwmon_update_fan(hwm);
> +
> +	hwm->info.ops = &qnap_mcu_hwmon_hwmon_ops;
> +	hwm->info.info = qnap_mcu_hwmon_channels;
> +
> +	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");
> +
> +	ret = qnap_mcu_hwmon_get_cooling_data(dev, hwm);
> +	if (ret)
> +		return ret;
> +

Please move ahead of hwmon device registration. It doesn't make sense to register
the hwmon device only to unregister it because of bad devicetree data.

> +	hwm->fan_state = hwm->fan_max_state;
> +
> +	if (IS_ENABLED(CONFIG_THERMAL)) {
> +		cdev = devm_thermal_of_cooling_device_register(dev,
> +				dev->parent->of_node, "qnap-mcu-hwmon",
> +				hwm, &qnap_mcu_hwmon_cooling_ops);

Does it make sense to register the cooling device even if there is no
cooling device data ?

> +		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 <heiko@...ech.de>");
> +MODULE_DESCRIPTION("QNAP MCU hwmon driver");
> +MODULE_LICENSE("GPL");

Powered by blists - more mailing lists