Received-SPF: pass (google.com: domain of linux-kernel+bounces-197462-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Fri, 31 May 2024 15:42:47 -0500
From: Bjorn Helgaas <helgaas@kernel.org>
To: Noah Wang <noahwang.wang@outlook.com>
Cc: robh@kernel.org, krzk+dt@kernel.org, linux@roeck-us.net,
	conor+dt@kernel.org, jdelvare@suse.com, corbet@lwn.net,
	Delphine_CC_Chiu@wiwynn.com, peteryin.openbmc@gmail.com,
	javier.carrasco.cruz@gmail.com, patrick.rudolph@9elements.com,
	luca.ceresoli@bootlin.com, chou.cosmo@gmail.com,
	bhelgaas@google.com, lukas@wunner.de, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-hwmon@vger.kernel.org,
	linux-doc@vger.kernel.org, linux-i2c@vger.kernel.org
Subject: Re: [v3,1/2] hwmon: add MP2891 driver
Message-ID: <20240531204247.GA608272@bhelgaas>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <SEYPR04MB648253BF01D42B24A72B0027FAFC2@SEYPR04MB6482.apcprd04.prod.outlook.com>

On Fri, May 31, 2024 at 03:26:01PM +0800, Noah Wang wrote:
> Add support for MPS VR controller mp2891. This driver exposes
> telemetry and limit value readings and writings.

> +++ b/Documentation/hwmon/index.rst
> @@ -168,6 +168,7 @@ Hardware Monitoring Kernel Drivers
>     mp2975
>     mp5023
>     mp5990
> +   mp2891

Add in alpha order.

>     mpq8785
>     nct6683
>     nct6775

> +++ b/Documentation/hwmon/mp2891.rst

> +Device supports direct and linear format for reading input voltage,
> +output voltage, input currect, output current, input power, output

s/currect/current/

> +++ b/MAINTAINERS
> @@ -22683,6 +22683,13 @@ S:	Maintained
>  F:	Documentation/hwmon/tps546d24.rst
>  F:	drivers/hwmon/pmbus/tps546d24.c
>  
> ++MPS MP2891 DRIVER

Should be added in alpha order.

> ++M:	Noah Wang <noahwang.wang@outlook.com>
> ++L:	linux-hwmon@vger.kernel.org
> ++S:	Maintained
> ++F:	Documentation/hwmon/mp2891.rst
> ++F:	drivers/hwmon/pmbus/mp2891.c
> +
>  TQ SYSTEMS BOARD & DRIVER SUPPORT

> +++ b/drivers/hwmon/pmbus/Makefile
> @@ -39,6 +39,7 @@ obj-$(CONFIG_SENSORS_MP2888)	+= mp2888.o
>  obj-$(CONFIG_SENSORS_MP2975)	+= mp2975.o
>  obj-$(CONFIG_SENSORS_MP5023)	+= mp5023.o
>  obj-$(CONFIG_SENSORS_MP5990)	+= mp5990.o
> +obj-$(CONFIG_SENSORS_MP2891)	+= mp2891.o

This list as a whole isn't sorted, but I would move this so the MPxxxx
entries remain sorted.

> +++ b/drivers/hwmon/pmbus/mp2891.c

> + * Vender specific registers, the register MFR_SVI3_IOUT_PRT(0x65),
> + * MFR_VOUT_LOOP_CTRL(0xBD), READ_PIN_EST(0x94)and READ_IIN_EST(0x95)
> + * redefine the standard PMBUS register. The MFR_SVI3_IOUT_PRT(0x65)
> + * is used to identify the iout scale and the MFR_VOUT_LOOP_CTRL(0xBD)
> + * is used to identify the vout scale. The READ_PIN_EST(0x94) is used
> + * to read input power of per rail. The MP2891 does not have standard

s/of per rail/per rail/ ?

> +	 * The output voltage is equal to the READ_VOUT(0x8B) register value multiply
> +	 * by vout_scale.

s/multiply by/multiplied by/

> +	 * The output current is equal to the READ_IOUT(0x8C) register value
> +	 * multiply by iout_scale.

s/multiply by/multiplied by/

> +		 * The MP2891 does not follow standard PMBus protocol completely, the
> +		 * PMBUS_VOUT_MODE(0x20) in MP2891 is reserved and 0x00 is always be
> +		 * returned when the register is read. But the calculation of vout in

s/always be/always/

> +		 * The MP2891 has standard PMBUS_READ_PIN register(0x97), but this
> +		 * is not used to read the input power of per rail. The input power

s/of per rail/per rail/ ?

> +		 * of per rail is read through the vender redefined register

s/of per rail/per rail/ ?

> +		 * The MP2891 PMBUS_VIN_OV_FAULT_LIMIT scale is 125mV/Lsb.
> +		 * but the vin scale is set to 31.25mV/Lsb(using r/m/b scale).
> +		 * As a result, the limit value should multiply by 4.

s/multiply by/be multiplied by/

> +		 * The scale of PMBUS_IIN_OC_WARN_LIMIT is 0.5A/Lsb, but the iin scale
> +		 * is set to 1A/Lsb(using r/m/b scale), so the word data should divide
> +		 * by 2.

s/divide by/be divided by/

> +		 * The scale of PMBUS_PIN_OP_WARN_LIMIT is 2W/Lsb, but the pin scale
> +		 * is set to 1W/Lsb(using r/m/b scale), so the word data should multiply
> +		 * by 2.

s/multiply by/be multiplied by/

> +		 * The PMBUS_VIN_OV_FAULT_LIMIT[7:0] is the limit value, and bit8-bit15
> +		 * should not be changed. The scale of PMBUS_VIN_OV_FAULT_LIMIT is 125mV/Lsb,
> +		 * but the vin scale is set to 31.25mV/Lsb(using r/m/b scale), so the word data
> +		 * should divide by 4.

s/divide by/be divided by/

> +		 * The scale of PMBUS_IIN_OC_WARN_LIMIT is 0.5A/Lsb, but the iin scale
> +		 * is set to 1A/Lsb(using r/m/b scale), so the word data should multiply
> +		 * by 2.

s/multiply by/be multiplied by/

> +		 * The scale of PMBUS_PIN_OP_WARN_LIMIT is 2W/Lsb, but the pin scale
> +		 * is set to 1W/Lsb(using r/m/b scale), so the word data should divide
> +		 * by 2.

s/divide by/be divided by/