Received-SPF: pass (google.com: domain of linux-kernel+bounces-95970-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Sender: Guenter Roeck <groeck7@gmail.com>
Date: Thu, 7 Mar 2024 09:47:59 -0800
From: Guenter Roeck <linux@roeck-us.net>
To: Jack Doan <me@jackdoan.com>
Cc: Jean Delvare <jdelvare@suse.com>, Jonathan Corbet <corbet@lwn.net>,
	linux-hwmon@vger.kernel.org, linux-doc@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] hwmon: Add MSI PSU HID monitoring driver
Message-ID: <480eaeb9-1f47-414b-aaac-0ddd131f9359@roeck-us.net>
References: <20240108185604.2858930-1-me@jackdoan.com>
 <20240212174859.3321000-1-me@jackdoan.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240212174859.3321000-1-me@jackdoan.com>

On Mon, Feb 12, 2024 at 10:48:57AM -0700, Jack Doan wrote:
> This driver provides a sysfs interface for MSI power supplies with a
> USB-HID monitoring interface.
> 

From Documentation/process/submitting-patches.rst:

"Describe your changes in imperative mood, e.g. "make xyzzy do frotz"
 instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy
 to do frotz", as if you are giving orders to the codebase to change
 its behaviour.
"

> Measurements for the output voltage and current for each rail are provided,
> as well as total output power, temperature, and fan control.
> 
> This patch adds:
> - hwmon driver msi-psu
> - hwmon documentation
> - updates MAINTAINERS
> 
> Signed-off-by: Jack Doan <me@jackdoan.com>

Checkpatch says:

total: 1 errors, 6 warnings, 0 checks, 922 lines checked

Please do run checkpatch.

> ---
> Changelog:
> v2 - Correct tab usage in #defines
>    - Align pwn_enable codes with hwmon standards
>    - Add a length check to the NAK handler
> ---
>  Documentation/hwmon/index.rst   |   1 +
>  Documentation/hwmon/msi-psu.rst |  64 +++
>  MAINTAINERS                     |   7 +
>  drivers/hwmon/Kconfig           |  12 +
>  drivers/hwmon/Makefile          |   1 +
>  drivers/hwmon/msi-psu.c         | 813 ++++++++++++++++++++++++++++++++
>  6 files changed, 898 insertions(+)
>  create mode 100644 Documentation/hwmon/msi-psu.rst
>  create mode 100644 drivers/hwmon/msi-psu.c
> 
> diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
> index 72f4e6065bae..34e4bc086bdb 100644
> --- a/Documentation/hwmon/index.rst
> +++ b/Documentation/hwmon/index.rst
> @@ -159,6 +159,7 @@ Hardware Monitoring Kernel Drivers
>     mp2888
>     mp2975
>     mp5023
> +   msi-psu
>     nct6683
>     nct6775
>     nct7802
> diff --git a/Documentation/hwmon/msi-psu.rst b/Documentation/hwmon/msi-psu.rst
> new file mode 100644
> index 000000000000..c6750713c82c
> --- /dev/null
> +++ b/Documentation/hwmon/msi-psu.rst
> @@ -0,0 +1,64 @@
> +.. SPDX-License-Identifier: GPL-2.0-or-later
> +
> +Kernel driver msi-psu
> +=========================
> +
> +Supported devices:
> +
> +* MSI MEG Ai1300P
> +
> +* MSI MEG Ai1000P
> +
> +Author: Jack Doan
> +
> +Description
> +-----------
> +
> +This driver provides a sysfs interface for MSI PSUs with a HID monitoring
> +interface.

Same as above.

> +
> +Measurements for the output voltage and current for each rail are provided,
> +as well as total output power, temperature, and fan control.
> +
> +Additional properties are available in debugfs, such as an efficiency
> +measurement, and switching to/from 12V mutli-rail mode
> +
> +Sysfs entries
> +-------------
> +
> +======================= ========================================================
> +curr1_input             Current on the 12v psu rail
> +curr2_input             Current on the 5v psu rail
> +curr3_input             Current on the 3.3v psu rail
> +fan1_input              RPM of psu fan
> +in0_input               Voltage of the psu ac input
> +in1_input               Voltage of the 12v psu rail
> +in2_input               Voltage of the 5v psu rail
> +in3_input               Voltage of the 3.3v psu rail
> +power1_input            Total power usage
> +pwm1                    PWM value for fan1. Writes to this file will switch set
> +                        pwm1_enable to manual control mode
> +pwm1_enable             PWM mode for fan1. (2) means "auto", and uses the
> +                        built-in fan curve. (1) means manual control
> +temp1_input             Temperature of the psu
> +======================= ========================================================
> +
> +Usage Notes
> +-----------
> +
> +It is a USB HID device, so it is auto-detected, supports hot-swapping and
> +several devices at once.
> +
> +Debugfs entries
> +---------------
> +
> +======================= ========================================================
> +multi_rail_enabled      Single or multi rail mode of the PCIe power connectors
> +efficiency              An efficiency measurement, expressed as per-ten-thousand
> +(ex: 8512 == 85.12%)

Alignment

> +product                 Product name of the psu
> +revision                Revision number of the psu
> +uptime      Session uptime of the psu
> +uptime_total    Total uptime of the psu
> +vendor      Vendor name of the psu
> +======================= ========================================================
> \ No newline at end of file
> diff --git a/MAINTAINERS b/MAINTAINERS
> index a7c4cf8201e0..246519f9079d 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -14632,6 +14632,13 @@ L:	platform-driver-x86@vger.kernel.org
>  S:	Maintained
>  F:	drivers/platform/x86/msi-laptop.c
>  
> +MSI PSU HARDWARE MONITOR DRIVER
> +M:	Jack Doan <me@jackdoan.com>
> +L:	linux-hwmon@vger.kernel.org
> +S:	Maintained
> +F:	Documentation/hwmon/msi-psu.rst
> +F:	drivers/hwmon/msi-psu.c
> +
>  MSI WMI SUPPORT
>  L:	platform-driver-x86@vger.kernel.org
>  S:	Orphan
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index cf27523eed5a..2870673b56a5 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -1248,6 +1248,18 @@ config SENSORS_MLXREG_FAN
>  	  driver as a module, choose 'M' here: the module will be called
>  	  mlxreg-fan.
>  
> +config SENSORS_MSI_PSU
> +	tristate "MSI PSU HID controller"
> +	depends on HID
> +	help
> +	  If you say yes here you get support for MSI power supplies with an
> +	  HID monitoring interface.
> +
> +	  Currently this driver supports the MEG Ai1300P and MEG Ai1000P PSUs.
> +
> +	  This driver can also be built as a module. If so, the module
> +	  will be called msi-psu.
> +
>  config SENSORS_TC654
>  	tristate "Microchip TC654/TC655 and compatibles"
>  	depends on I2C
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index e84bd9685b5c..c08268073ca0 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -156,6 +156,7 @@ obj-$(CONFIG_MAX31827) += max31827.o
>  obj-$(CONFIG_SENSORS_MC13783_ADC)+= mc13783-adc.o
>  obj-$(CONFIG_SENSORS_MC34VR500)	+= mc34vr500.o
>  obj-$(CONFIG_SENSORS_MCP3021)	+= mcp3021.o
> +obj-$(CONFIG_SENSORS_MSI_PSU)	+= msi-psu.o
>  obj-$(CONFIG_SENSORS_TC654)	+= tc654.o
>  obj-$(CONFIG_SENSORS_TPS23861)	+= tps23861.o
>  obj-$(CONFIG_SENSORS_MLXREG_FAN) += mlxreg-fan.o
> diff --git a/drivers/hwmon/msi-psu.c b/drivers/hwmon/msi-psu.c
> new file mode 100644
> index 000000000000..13ffb8444893
> --- /dev/null
> +++ b/drivers/hwmon/msi-psu.c
> @@ -0,0 +1,813 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Driver for MSI power supplies with USB-HID interfaces
> + * Heavily derived from the corsair-psu and corsair-cpro drivers,
> + * but different enough to be incompatible
> + *
> + * Copyright (C) 2023 Jack Doan <me@jackdoan.com>
> + */
> +
> +#include <linux/completion.h>
> +#include <linux/debugfs.h>
> +#include <linux/errno.h>
> +#include <linux/hid.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>

Does not appear to be needed.

> +#include <linux/jiffies.h>
> +#include <linux/minmax.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/slab.h>
> +#include <linux/types.h>
> +
> +/*
> + * MSI protocol for PSUs
> + *
> + * message size = 64 bytes (request and response, little endian)
> + * request:
> + *	[command][register][param0][param1][paramX]...
> + * reply:
> + *	[echo of command][echo of register][data0][data1][dataX]...
> + *
> + *	- the driver uses raw events to be accessible from userspace (though this is not really
> + *	  supported, it is just there for convenience, may be removed in the future)
> + *	- a successful reply always starts with the address and command in the same order the
> + *	  request used it
> + *	- length of the reply data is specific to the command used.
> + *	- The replies to most reads are pmbus linear11 encoded
> + *	- the PSU expects a "handshake" init command before all other commands will work
> + *	- send the handshake by sending 0x51 to the address 0xfa (packet will be [0xfa 0x51])
> + *	- the driver supports debugfs for values not fitting into the hwmon class
> + */
> +
> +#define DRIVER_NAME "msi-psu"
> +
> +#define REPLY_SIZE	40 /* max length of a reply to a single command */
> +#define SAMPLE_INTERVAL_MS	50
> +#define CMD_BUFFER_SIZE	64
> +#define CMD_TIMEOUT_MS	250
> +#define SECONDS_PER_HOUR	(60 * 60)
> +#define SECONDS_PER_DAY	(SECONDS_PER_HOUR * 24)
> +
> +#define PSU_NAK	0xFE
> +#define PSU_REG_VEND_STR	0x10
> +#define PSU_REG_PROD_STR	0x11
> +#define PSU_REG_REVISION	0x12
> +#define PSU_REG_SERIAL	0x13 /* accepted, but returns all zeros */
> +
> +#define PSU_REG_IN_VOLTS	0x20
> +#define PSU_REG_VOUT_12V_EACH_RAIL	0x22
> +#define PSU_REG_IOUT_12V_EACH_RAIL	0x23
> +#define PSU_REG_VOUT_12V	0x24
> +#define PSU_REG_IOUT_12V	0x25
> +#define PSU_REG_VOUT_5V	0x26
> +#define PSU_REG_IOUT_5V	0x27
> +#define PSU_REG_VOUT_3V	0x28
> +#define PSU_REG_IOUT_3V	0x29
> +#define PSU_REG_TOTAL_WATTS	0x2A
> +#define PSU_REG_EFFICIENCY	0x2B
> +#define PSU_REG_TEMP0	0x30
> +#define PSU_REG_FAN_RPM	0x40
> +#define PSU_REG_FAN_MODE	0x41
> +#define PSU_REG_FAN_DUTY_CYCLE	0x42
> +
> +#define PSU_REG_MULTIRAIL	0xC0
> +#define PSU_REG_UNKNOWN_C4	0xC4
> +#define PSU_REG_UNKNOWN_C6	0xC6
> +
> +#define PSU_REG_UPTIME	0xD0
> +#define PSU_REG_TOTAL_UPTIME	0xD1
> +
> +#define PSU_REG_READ_EVERYTHING	0xE0
> +#define PSU_REG_SAVE_SETTINGS	0xF1
> +
> +#define PSU_INIT	0xFA
> +#define PSU_READ	0x51
> +#define PSU_WRITE	0x50
> +
> +#define PSU_MULTI_RAIL_ENABLED	2
> +#define PSU_MULTI_RAIL_DISABLED	1
> +
> +#define COMBINED_12V	5
> +
> +#define FAN_MODE_MANUAL	3
> +#define FAN_MODE_AUTO	1
> +#define FAN_SPEED_MAX	100
> +#define FAN_SPEED_MIN	0
> +
> +struct volt_amp_pair {
> +	u16 volts;
> +	u16 amps;
> +} __packed;
> +
> +struct msipsu_all {
> +	struct volt_amp_pair v12[6];
> +	struct volt_amp_pair v5;
> +	struct volt_amp_pair v3;
> +	u16 watts;
> +	u16 eff;
> +	u16 temp;
> +	u16 fan_speed;
> +} __packed;
> +
> +struct msipsu_data {
> +	struct hid_device *hdev;
> +	struct device *hwmon_dev;
> +	struct dentry *debugfs;
> +	struct completion wait_completion;
> +	struct mutex lock; /* for locking access to cmd_buffer */
> +	u8 *cmd_buffer;
> +	char vendor[REPLY_SIZE];
> +	char product[REPLY_SIZE];
> +	struct debugfs_blob_wrapper vendor_blob;
> +	struct debugfs_blob_wrapper product_blob;
> +	struct msipsu_all data;
> +	ktime_t last_read_all;
> +};
> +
> +/* some values are SMBus LINEAR11 data which need a conversion */
> +static int msipsu_lin11_to_int(u16 v16, int scale, bool is_signed)
> +{
> +	s32 exponent;
> +	s32 mantissa;
> +	int val;
> +
> +	exponent = ((s16)v16) >> 11;
> +	if (is_signed)
> +		mantissa = ((s16)((v16 & 0x7ff) << 5)) >> 5;
> +	else
> +		mantissa = (u16)(v16 & 0x7ff); /* deliberately not sign-extending here */
> +	val = mantissa * scale;
> +
> +	if (exponent >= 0)
> +		val <<= exponent;
> +	else
> +		val >>= -exponent;
> +
> +	return val;
> +}
> +
> +static int msipsu_percent_to_pwm(u16 val)
> +{
> +	return DIV_ROUND_CLOSEST(val * 255, 100);
> +}
> +
> +static int msipsu_pwm_to_percent(long val)
> +{
> +	if (val < 0 || val > 255)
> +		return -EINVAL;
> +
> +	return DIV_ROUND_CLOSEST(val * 100, 255);
> +}
> +
> +static int msipsu_usb_cmd(struct msipsu_data *priv, const u8 *in, size_t in_len, void *data)
> +{
> +	unsigned long time;
> +	int ret;
> +
> +	if (in_len > CMD_BUFFER_SIZE)
> +		return -ENOBUFS;
> +
> +	memset(priv->cmd_buffer, 0, CMD_BUFFER_SIZE);
> +	memcpy(priv->cmd_buffer, in, in_len);
> +
> +	reinit_completion(&priv->wait_completion);
> +
> +	ret = hid_hw_output_report(priv->hdev, priv->cmd_buffer, CMD_BUFFER_SIZE);
> +	if (ret < 0)
> +		return ret;
> +
> +	time = wait_for_completion_timeout(&priv->wait_completion,
> +					   msecs_to_jiffies(CMD_TIMEOUT_MS));
> +	if (!time)
> +		return -ETIMEDOUT;
> +
> +	/*
> +	 * at the start of the reply is an echo of the send command/address in the same order it
> +	 * was sent. In the event a command/address is not supported, the PSU will reply with a NAK.
> +	 * For an invalid read, it will look like this:
> +	 *   Out -> [PSU_READ(0x51) A_FAKE_REGISTER(0x69)]
> +	 *   In  <- [PSU_READ(0x51) PSU_NAK(0xFE)]
> +	 * Writes differ slightly, this is the sequence for a write to a non-existent register:
> +	 *   Out -> [PSU_WRITE(0x50) A_FAKE_REGISTER(0x69)]
> +	 *   In  <- [PSU_WRITE(0x50) PSU_NAK(0xFE)]
> +	 * And this is a rejected write (of the correct length) to a real register:
> +	 *   Out -> [PSU_WRITE(0x50) PSU_REG_FAN_MODE(0x41) DATA(0x55 0x55 0x55)]
> +	 *   In  <- [PSU_WRITE(0x50) PSU_REG_FAN_MODE(0x41) PSU_NAK(0xFE)]
> +	 */
> +	if (in[0] != priv->cmd_buffer[0] || in[1] != priv->cmd_buffer[1])
> +		return -EOPNOTSUPP;
> +	else if (in_len >= 3 && in[2] != PSU_NAK && priv->cmd_buffer[2] == PSU_NAK)
> +		return -EINVAL;
> +
> +	if (data)
> +		memcpy(data, priv->cmd_buffer + 2, REPLY_SIZE);
> +
> +	return 0;
> +}
> +
> +static int msipsu_usb_cmd_locked(struct msipsu_data *priv, const u8 *in, size_t in_len, void *data)
> +{
> +	int ret;
> +
> +	mutex_lock(&priv->lock);
> +	ret = msipsu_usb_cmd(priv, in, in_len, data);
> +	mutex_unlock(&priv->lock);
> +	return ret;
> +}
> +
> +static int msipsu_fw_init(struct msipsu_data *priv)
> +{
> +	/*
> +	 * Vendor SW always begins with a message of [0xfa, 0x51]
> +	 * This init message is replied to with the model name of the PSU.
> +	 */
> +	const u8 init[] = {PSU_INIT, 0x51};
> +	const u8 read_vendor[] = {PSU_READ, PSU_REG_VEND_STR};
> +	int ret;
> +
> +	ret = msipsu_usb_cmd(priv, init, sizeof(init), priv->product);
> +	if (ret < 0)
> +		return ret;
> +	ret = msipsu_usb_cmd(priv, read_vendor, sizeof(read_vendor), priv->vendor);
> +	if (ret < 0)
> +		return ret;
> +	priv->vendor_blob.data = priv->vendor;
> +	priv->product_blob.data = priv->product;
> +	priv->vendor_blob.size = strnlen(priv->vendor, sizeof(priv->vendor));
> +	priv->product_blob.size = strnlen(priv->product, sizeof(priv->product));
> +	return ret;
> +}
> +
> +static int msipsu_save_settings(struct msipsu_data *priv)
> +{
> +	const u8 cmd[] = {PSU_WRITE, PSU_REG_SAVE_SETTINGS, 0x0};
> +
> +	return msipsu_usb_cmd_locked(priv, cmd, sizeof(cmd), NULL);
> +}
> +
> +static int msipsu_write_fan_settings(struct msipsu_data *priv, u8 mode, long pwm_duty)
> +{
> +	int ret;
> +	int percent_duty = 0;
> +
> +	if (mode == FAN_MODE_AUTO) {
> +		/* vendor SW masks off the duty cycle for auto-mode, we should do the same */
> +		percent_duty = FAN_SPEED_MIN;
> +	} else if (pwm_duty != 0) {
> +		/*
> +		 * it is an error to write duty to non-zero while the device is not in
> +		 * manual-fan-mode, so switch mode automatically
> +		 */
> +		mode = FAN_MODE_MANUAL;
> +		percent_duty = msipsu_pwm_to_percent(pwm_duty);
> +		if (percent_duty < 0) /* return -EINVAL if we got a value we couldn't convert */
> +			return percent_duty;
> +	}
> +	const u8 cmd[] = {PSU_WRITE, PSU_REG_FAN_MODE, mode, 0, percent_duty};
> +
> +	ret = msipsu_usb_cmd_locked(priv, cmd, sizeof(cmd), NULL);
> +	if (ret < 0)
> +		return ret;
> +	return msipsu_save_settings(priv);

Locking twice seems racy. A second process could step in and execute the
first part of this command again (with different values) while the thread
executing this sequence is waiting for the lock in msipsu_usb_cmd_locked().

> +}
> +
> +static int msipsu_get_all_values(struct msipsu_data *priv)
> +{
> +	const u8 read_cmd[] = {PSU_READ, PSU_REG_READ_EVERYTHING};
> +	ktime_t t = ktime_get();
> +	s64 delta = ktime_ms_delta(t, priv->last_read_all);
> +
> +	if (delta < SAMPLE_INTERVAL_MS)
> +		return 0;
> +
> +	priv->last_read_all = t;

A second caller could step in here, assume that values are current, and return
old data (or in-progress data) to the user while the data is being updated.
This may result in inconsistent or outdated information to be reported to
userspace.

> +	return msipsu_usb_cmd_locked(priv, read_cmd, sizeof(read_cmd), &priv->data);
> +}
> +
> +static int msipsu_get_value(struct msipsu_data *priv, u8 cmd, long *val)
> +{
> +	u8 data[REPLY_SIZE];
> +	const u8 read_cmd[] = {PSU_READ, cmd};
> +	int ret = msipsu_usb_cmd_locked(priv, read_cmd, sizeof(read_cmd), data);
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	u16 tmp16 = (data[1] << 8) + data[0];
> +
> +	switch (cmd) {
> +	case PSU_REG_IN_VOLTS:
> +	case PSU_REG_VOUT_12V:
> +	case PSU_REG_IOUT_12V:
> +	case PSU_REG_VOUT_5V:
> +	case PSU_REG_IOUT_5V:
> +	case PSU_REG_VOUT_3V:
> +	case PSU_REG_IOUT_3V:
> +	case PSU_REG_TEMP0:
> +		*val = msipsu_lin11_to_int(tmp16, 1000, true);
> +		break;
> +	case PSU_REG_EFFICIENCY:
> +		*val = msipsu_lin11_to_int(tmp16, 100, true);
> +		break;
> +	case PSU_REG_FAN_RPM:
> +		*val = msipsu_lin11_to_int(tmp16, 1, false);
> +		break;
> +	case PSU_REG_TOTAL_WATTS:
> +		*val = msipsu_lin11_to_int(tmp16, 1000000, true);
> +		break;
> +	case PSU_REG_FAN_MODE:
> +	    switch (data[0]) {
> +		case FAN_MODE_AUTO:
> +		    *val = 2;
> +		    break;
> +		case FAN_MODE_MANUAL:
> +		    *val = 1;
> +		    break;
> +		default:
> +		    *val = 0xff;

What is this supposed to be ? This is not a documented or sensible
value to report to userspace as pwm_enable value.

> +		    break;
> +	    }
> +	    break;
> +	case PSU_REG_FAN_DUTY_CYCLE:
> +	case PSU_REG_TOTAL_UPTIME:
> +	case PSU_REG_UPTIME:
> +	case PSU_REG_REVISION:
> +		*val = ((long)data[3] << 24) + (data[2] << 16) + tmp16;
> +		break;
> +	case PSU_REG_MULTIRAIL:
> +		switch (data[0]) {
> +		case PSU_MULTI_RAIL_DISABLED:
> +			*val = 0;
> +			break;
> +		case PSU_MULTI_RAIL_ENABLED:
> +			*val = 1;
> +			break;
> +		default:
> +			*val = 0xff;

Same as above. Reporting such undefined values to userspace doesn't really
make sense.

> +			break;
> +		}
> +		break;
> +	default:
> +		ret = -EOPNOTSUPP;
> +		break;
> +	}
> +
> +	return ret;
> +}
> +
> +static umode_t msipsu_hwmon_ops_is_visible(const void *data, enum hwmon_sensor_types type,
> +					   u32 attr, int channel)
> +{
> +	if (type == hwmon_temp && (attr == hwmon_temp_input || attr == hwmon_temp_label))
> +		return 0444;
> +	else if (type == hwmon_fan && (attr == hwmon_fan_input || attr == hwmon_fan_label))
> +		return 0444;
> +	else if (type == hwmon_pwm && (attr == hwmon_pwm_enable || attr == hwmon_pwm_input))
> +		return 0644;
> +	else if (type == hwmon_power && (attr == hwmon_power_input || attr == hwmon_power_label))
> +		return 0444;
> +	else if (type == hwmon_in && (attr == hwmon_in_input || attr == hwmon_in_label))
> +		return 0444;
> +	else if (type == hwmon_curr && (attr == hwmon_curr_input || attr == hwmon_curr_label))
> +		return 0444;
> +
> +	return 0;
> +}
> +
> +static int msipsu_hwmon_ops_read(struct device *dev, enum hwmon_sensor_types type, u32 attr,
> +				 int channel, long *val)
> +{
> +	struct msipsu_data *priv = dev_get_drvdata(dev);
> +	int ret;
> +
> +	if (type != hwmon_pwm) {
> +		/* PWM controls don't use the "all" packet, so don't read it */
> +		ret = msipsu_get_all_values(priv);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	if (type == hwmon_temp && attr == hwmon_temp_input) {
> +		*val = msipsu_lin11_to_int(priv->data.temp, 1000, true);
> +	} else if (type == hwmon_fan && attr == hwmon_fan_input) {
> +		*val = msipsu_lin11_to_int(priv->data.fan_speed, 1, false);
> +	} else if (type == hwmon_pwm) {
> +		switch (attr) {
> +		case hwmon_pwm_enable:
> +			ret = msipsu_get_value(priv, PSU_REG_FAN_MODE, val);

This should break if ret < 0.

> +			*val &= 0xff;

msipsu_get_value() never sets *val to anything above 0xff, so this is a bit
pointless.

> +			break;
> +		case hwmon_pwm_input:
> +			ret = msipsu_get_value(priv, PSU_REG_FAN_DUTY_CYCLE, val);

This should break if ret < 0.

> +			*val &= 0xff; /* there's a "calculated" DC in the next byte up */

What is the point of returning the calculated DC from msipsu_get_value()
if it is never used ?

> +			*val = msipsu_percent_to_pwm(*val);
> +			break;
> +		default:
> +			ret = -EOPNOTSUPP;
> +			break;
> +		}
> +	} else if (type == hwmon_power && attr == hwmon_power_input) {
> +		*val = msipsu_lin11_to_int(priv->data.watts, 1000000, true);
> +	} else if (type == hwmon_in && attr == hwmon_in_input) {
> +		switch (channel) {
> +		case 0:
> +			ret = msipsu_get_value(priv, PSU_REG_IN_VOLTS, val);
> +			break;
> +		case 1:
> +			*val = msipsu_lin11_to_int(priv->data.v12[COMBINED_12V].volts, 1000, true);
> +			break;
> +		case 2:
> +			*val = msipsu_lin11_to_int(priv->data.v5.volts, 1000, true);
> +			break;
> +		case 3:
> +			*val = msipsu_lin11_to_int(priv->data.v3.volts, 1000, true);
> +			break;
> +		default:
> +			return -EOPNOTSUPP;
> +		}
> +	} else if (type == hwmon_curr && attr == hwmon_curr_input) {
> +		switch (channel) {
> +		case 0:
> +			*val = msipsu_lin11_to_int(priv->data.v12[COMBINED_12V].amps, 1000, true);
> +			break;
> +		case 1:
> +			*val = msipsu_lin11_to_int(priv->data.v5.amps, 1000, true);
> +			break;
> +		case 2:
> +			*val = msipsu_lin11_to_int(priv->data.v3.amps, 1000, true);
> +			break;
> +		default:
> +			return -EOPNOTSUPP;
> +		}
> +	} else {
> +		return -EOPNOTSUPP;
> +	}
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;

Why not just
	return ret;
?

> +}
> +
> +static int msipsu_hwmon_ops_read_string(struct device *dev, enum hwmon_sensor_types type, u32 attr,
> +					int channel, const char **str)
> +{
> +	static const char *const label_volts[] = {
> +		"v_in",
> +		"v_out +12v",
> +		"v_out +5v",
> +		"v_out +3.3v"
> +	};
> +
> +	static const char *const label_amps[] = {
> +		"curr +12v",
> +		"curr +5v",
> +		"curr +3.3v"
> +	};
> +
> +	if (type == hwmon_temp && attr == hwmon_temp_label) {
> +		*str = "psu temp";
> +		return 0;
> +	} else if (type == hwmon_fan && attr == hwmon_fan_label) {
> +		*str = "psu fan";
> +		return 0;
> +	} else if (type == hwmon_power && attr == hwmon_power_label) {
> +		*str = "power total";
> +		return 0;
> +	} else if (type == hwmon_in && attr == hwmon_in_label && channel < 4) {
> +		*str = label_volts[channel];
> +		return 0;
> +	} else if (type == hwmon_curr && attr == hwmon_curr_label && channel < 3) {
> +		*str = label_amps[channel];
> +		return 0;
> +	}
> +
> +	return -EOPNOTSUPP;
> +}
> +
> +static int msipsu_hwmon_ops_write(struct device *dev, enum hwmon_sensor_types type, u32 attr,
> +				  int channel, long val)
> +{
> +	struct msipsu_data *priv = dev_get_drvdata(dev);
> +
> +	if (type != hwmon_pwm)
> +		return -EOPNOTSUPP;
> +
> +	switch (attr) {
> +	case hwmon_pwm_enable:
> +		switch (val) {
> +		case 1: /* manual mode */
> +			return msipsu_write_fan_settings(priv, FAN_MODE_MANUAL, FAN_SPEED_MAX);
> +		case 2: /* auto mode */
> +			return msipsu_write_fan_settings(priv, FAN_MODE_AUTO, FAN_SPEED_MIN);
> +		default:
> +			return -EINVAL;
> +		}
> +	case hwmon_pwm_input:
> +		return msipsu_write_fan_settings(priv, FAN_MODE_MANUAL, val);

Unless I am missing something, this has the side effect of setting
the fan mode to manual. This is not supposed to happen.

> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +static const struct hwmon_ops msipsu_hwmon_ops = {
> +	.is_visible = msipsu_hwmon_ops_is_visible,
> +	.read = msipsu_hwmon_ops_read,
> +	.write = msipsu_hwmon_ops_write,
> +	.read_string = msipsu_hwmon_ops_read_string,
> +};
> +
> +static const struct hwmon_channel_info *msipsu_info[] = {
> +	HWMON_CHANNEL_INFO(chip,
> +			   HWMON_C_REGISTER_TZ),
> +	HWMON_CHANNEL_INFO(temp,
> +			   HWMON_T_INPUT | HWMON_T_LABEL),
> +	HWMON_CHANNEL_INFO(fan,
> +			   HWMON_F_INPUT | HWMON_F_LABEL),
> +	HWMON_CHANNEL_INFO(pwm,
> +			   HWMON_PWM_INPUT | HWMON_PWM_ENABLE),
> +	HWMON_CHANNEL_INFO(power,
> +			   HWMON_P_INPUT | HWMON_P_LABEL),
> +	HWMON_CHANNEL_INFO(in,
> +			   HWMON_I_INPUT | HWMON_I_LABEL,
> +			   HWMON_I_INPUT | HWMON_I_LABEL,
> +			   HWMON_I_INPUT | HWMON_I_LABEL,
> +			   HWMON_I_INPUT | HWMON_I_LABEL),
> +	HWMON_CHANNEL_INFO(curr,
> +			   HWMON_C_INPUT | HWMON_C_LABEL,
> +			   HWMON_C_INPUT | HWMON_C_LABEL,
> +			   HWMON_C_INPUT | HWMON_C_LABEL),
> +	NULL
> +};
> +
> +static const struct hwmon_chip_info msipsu_chip_info = {
> +	.ops	= &msipsu_hwmon_ops,
> +	.info	= msipsu_info,
> +};
> +
> +#ifdef CONFIG_DEBUG_FS
> +
> +static void print_uptime(struct seq_file *seqf, u8 cmd)
> +{
> +	struct msipsu_data *priv = seqf->private;
> +	long val = 0;
> +	int ret = msipsu_get_value(priv, cmd, &val);
> +
> +	if (ret < 0) {
> +		seq_puts(seqf, "N/A\n");
> +		return;
> +	}
> +
> +	if (val > SECONDS_PER_DAY) {
> +		seq_printf(seqf, "%ld day(s), %02ld:%02ld:%02ld\n", val / SECONDS_PER_DAY,
> +			   val % SECONDS_PER_DAY / SECONDS_PER_HOUR, val % SECONDS_PER_HOUR / 60,
> +			   val % 60);
> +		return;
> +	}
> +
> +	seq_printf(seqf, "%02ld:%02ld:%02ld\n", val % SECONDS_PER_DAY / SECONDS_PER_HOUR,
> +		   val % SECONDS_PER_HOUR / 60, val % 60);
> +}
> +
> +static int uptime_show(struct seq_file *seqf, void *unused)
> +{
> +	print_uptime(seqf, PSU_REG_UPTIME);
> +	return 0;
> +}
> +DEFINE_SHOW_ATTRIBUTE(uptime);
> +
> +static int uptime_total_show(struct seq_file *seqf, void *unused)
> +{
> +	print_uptime(seqf, PSU_REG_TOTAL_UPTIME);
> +	return 0;
> +}
> +DEFINE_SHOW_ATTRIBUTE(uptime_total);
> +
> +static int revision_show(struct seq_file *seqf, void *unused)
> +{
> +	struct msipsu_data *priv = seqf->private;
> +	long val = 0;
> +	int ret = msipsu_get_value(priv, PSU_REG_REVISION, &val);
> +
> +	if (ret < 0) {
> +		seq_puts(seqf, "N/A\n");
> +		return 0;
> +	}
> +
> +	seq_printf(seqf, "%c.%c\n", (char)(val & 0xff), (char)((val >> 8) & 0xff));
> +	return 0;
> +}
> +DEFINE_SHOW_ATTRIBUTE(revision);
> +
> +static int efficiency_show(struct seq_file *seqf, void *unused)
> +{
> +	struct msipsu_data *priv = seqf->private;
> +	int ret = msipsu_get_all_values(priv);
> +
> +	if (ret < 0) {
> +		seq_puts(seqf, "N/A\n");
> +		return 0;
> +	}
> +
> +	seq_printf(seqf, "%d\n", msipsu_lin11_to_int(priv->data.eff, 100, true));
> +	return 0;
> +}
> +DEFINE_SHOW_ATTRIBUTE(efficiency);
> +
> +static int multi_rail_read(void *data, u64 *val)
> +{
> +	return msipsu_get_value((struct msipsu_data *)data, PSU_REG_MULTIRAIL, (long *)val);
> +}
> +
> +static int msipsu_write_rail_setting(struct msipsu_data *priv, bool multi_enabled)
> +{
> +	long currently_enabled;
> +	int ret = msipsu_get_value(priv, PSU_REG_MULTIRAIL, &currently_enabled);
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	/* hardware returns an error if we try to set the mode to the current mode. Avoid this. */
> +	if (currently_enabled == multi_enabled)
> +		return 0;
> +
> +	u8 to_write = multi_enabled ? PSU_MULTI_RAIL_ENABLED : PSU_MULTI_RAIL_DISABLED;
> +	const u8 cmd[] = {PSU_WRITE, PSU_REG_MULTIRAIL, to_write};
> +
> +	ret = msipsu_usb_cmd_locked(priv, cmd, sizeof(cmd), NULL);
> +	if (ret < 0)
> +		return ret;
> +	return msipsu_save_settings(priv);
> +}
> +
> +static int multi_rail_write(void *data, u64 val)
> +{
> +	struct msipsu_data *priv = data;
> +
> +	switch (val) {
> +	case 0:
> +		return msipsu_write_rail_setting(priv, false);
> +	case 1:
> +		return msipsu_write_rail_setting(priv, true);
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +DEFINE_SIMPLE_ATTRIBUTE(multi_rail_fops, multi_rail_read, multi_rail_write, "%llu\n");
> +
> +static void msipsu_debugfs_init(struct msipsu_data *priv)
> +{
> +	char name[32];
> +
> +	scnprintf(name, sizeof(name), "%s-%s", DRIVER_NAME, dev_name(&priv->hdev->dev));
> +
> +	priv->debugfs = debugfs_create_dir(name, NULL);
> +	debugfs_create_file("uptime", 0444, priv->debugfs, priv, &uptime_fops);
> +	debugfs_create_file("uptime_total", 0444, priv->debugfs, priv, &uptime_total_fops);
> +	debugfs_create_blob("vendor", 0444, priv->debugfs, &priv->vendor_blob);
> +	debugfs_create_blob("product", 0444, priv->debugfs, &priv->product_blob);
> +	debugfs_create_file("revision", 0444, priv->debugfs, priv, &revision_fops);
> +	debugfs_create_file("efficiency", 0444, priv->debugfs, priv, &efficiency_fops);
> +	debugfs_create_file("multi_rail_enabled", 0644, priv->debugfs, priv, &multi_rail_fops);
> +}
> +
> +#else
> +
> +static void msipsu_debugfs_init(struct msipsu_data *priv)
> +{
> +}
> +
> +#endif
> +
> +static int msipsu_probe(struct hid_device *hdev, const struct hid_device_id *id)
> +{
> +	struct msipsu_data *priv;
> +	int ret;
> +
> +	priv = devm_kzalloc(&hdev->dev, sizeof(struct msipsu_data), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->cmd_buffer = devm_kmalloc(&hdev->dev, CMD_BUFFER_SIZE, GFP_KERNEL);
> +	if (!priv->cmd_buffer)
> +		return -ENOMEM;
> +
> +	ret = hid_parse(hdev);
> +	if (ret)
> +		return ret;
> +
> +	ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
> +	if (ret)
> +		return ret;
> +
> +	ret = hid_hw_open(hdev);
> +	if (ret)
> +		goto fail_and_stop;
> +
> +	priv->hdev = hdev;
> +	hid_set_drvdata(hdev, priv);
> +	mutex_init(&priv->lock);
> +	init_completion(&priv->wait_completion);
> +
> +	hid_device_io_start(hdev);
> +
> +	ret = msipsu_fw_init(priv);
> +	if (ret < 0) {
> +		dev_err(&hdev->dev, "unable to initialize device (%d)\n", ret);
> +		goto fail_and_close;
> +	}
> +
> +	priv->hwmon_dev = hwmon_device_register_with_info(&hdev->dev, "msipsu", priv,
> +							  &msipsu_chip_info, NULL);
> +
> +	if (IS_ERR(priv->hwmon_dev)) {
> +		ret = PTR_ERR(priv->hwmon_dev);
> +		goto fail_and_close;
> +	}
> +
> +	msipsu_debugfs_init(priv);
> +
> +	return 0;
> +
> +fail_and_close:
> +	hid_hw_close(hdev);
> +fail_and_stop:
> +	hid_hw_stop(hdev);
> +	return ret;
> +}
> +
> +static void msipsu_remove(struct hid_device *hdev)
> +{
> +	struct msipsu_data *priv = hid_get_drvdata(hdev);
> +
> +	debugfs_remove_recursive(priv->debugfs);
> +	hwmon_device_unregister(priv->hwmon_dev);
> +	hid_hw_close(hdev);
> +	hid_hw_stop(hdev);
> +}
> +
> +static int msipsu_raw_event(struct hid_device *hdev, struct hid_report *report, u8 *data, int size)
> +{
> +	struct msipsu_data *priv = hid_get_drvdata(hdev);
> +
> +	if (completion_done(&priv->wait_completion))
> +		return 0;
> +
> +	memcpy(priv->cmd_buffer, data, min(CMD_BUFFER_SIZE, size));
> +	complete(&priv->wait_completion);
> +
> +	return 0;
> +}
> +
> +#ifdef CONFIG_PM
> +static int msipsu_resume(struct hid_device *hdev)
> +{
> +	struct msipsu_data *priv = hid_get_drvdata(hdev);
> +
> +	/* some PSUs turn off the microcontroller during standby, so a reinit is required */
> +	return msipsu_fw_init(priv);
> +}
> +#endif
> +
> +static const struct hid_device_id msipsu_idtable[] = {
> +	{ HID_USB_DEVICE(0xdb0, 0x56d4) }, /* MEG Ai1300P */
> +	{ HID_USB_DEVICE(0xdb0, 0xe749) }, /* MEG Ai1000P */
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(hid, msipsu_idtable);
> +
> +static struct hid_driver msipsu_driver = {
> +	.name		= DRIVER_NAME,
> +	.id_table	= msipsu_idtable,
> +	.probe		= msipsu_probe,
> +	.remove		= msipsu_remove,
> +	.raw_event	= msipsu_raw_event,
> +#ifdef CONFIG_PM
> +	.resume		= msipsu_resume,
> +	.reset_resume	= msipsu_resume,
> +#endif
> +};
> +
> +static int __init msipsu_init(void)
> +{
> +	return hid_register_driver(&msipsu_driver);
> +}
> +
> +static void __exit msipsu_exit(void)
> +{
> +	hid_unregister_driver(&msipsu_driver);
> +}
> +
> +/*
> + * With module_init() the driver would load before the HID bus when
> + * built-in, so use late_initcall() instead.
> + */
> +late_initcall(msipsu_init);
> +module_exit(msipsu_exit);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Jack Doan <me@jackdoan.com>");
> +MODULE_DESCRIPTION("Driver for MSI power supplies with HID sensor interface");