Received-SPF: pass (google.com: domain of linux-kernel+bounces-50477-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Sender: Guenter Roeck <groeck7@gmail.com>
Date: Fri, 2 Feb 2024 11:24:39 -0800
From: Guenter Roeck <linux@roeck-us.net>
To: Jack Doan <me@jackdoan.com>
Cc: jdelvare@suse.com, corbet@lwn.net, linux-hwmon@vger.kernel.org,
	linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] hwmon: Add MSI PSU HID monitoring driver
Message-ID: <4dbe0482-7165-4143-8d76-378ab1b25a0b@roeck-us.net>
References: <20240108185604.2858930-1-me@jackdoan.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240108185604.2858930-1-me@jackdoan.com>

On Mon, Jan 08, 2024 at 11:56:04AM -0700, Jack Doan wrote:
> This driver provides a sysfs interface for MSI power supplies with a
> USB-HID monitoring interface.
> 
> 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>
> ---
>  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         | 801 ++++++++++++++++++++++++++++++++
>  6 files changed, 886 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..3dda7190a627
> --- /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.
> +
> +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 multi-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.

No, that is unexpected and not supposed to happen.

> +pwm1_enable     PWM mode for fan1. (1) means "auto", and uses the built-in fan
> +                curve. (3) means manual control

The ABI (Documentation/ABI/testing/sysfs-class-hwmon) says:

                - 0: no fan speed control (i.e. fan at full speed)
                - 1: manual fan speed control enabled (using `pwmY`)
                - 2+: automatic fan speed control enabled

I really do not see the point of declaring that 1 shall mean automatic,
or to skip 2.

..

> +#define REPLY_SIZE 40 /* max length of a reply to a single command */

#define<space>NAME<tab>value

for all defines, please.

Guenter