[PATCH] hwmon: add MP2891 driver

[PATCH] hwmon: add MP2891 driver

[Noah Wang]

This driver is designed for MPS VR controller mp2891. The input
voltage, output voltage, input current, output current, input
power, output power and temperature of per rail can be obtained
from hwmon sysfs that the driver provided.

Signed-off-by: Noah Wang <noahwang.wang@outlook.com>
---
 Documentation/hwmon/mp2891.rst |  95 +++++++++++++++
 drivers/hwmon/pmbus/Kconfig    |   9 ++
 drivers/hwmon/pmbus/Makefile   |   1 +
 drivers/hwmon/pmbus/mp2891.c   | 210 +++++++++++++++++++++++++++++++++
 4 files changed, 315 insertions(+)
 create mode 100644 Documentation/hwmon/mp2891.rst
 create mode 100644 drivers/hwmon/pmbus/mp2891.c

diff --git a/Documentation/hwmon/mp2891.rst b/Documentation/hwmon/mp2891.rst
new file mode 100644
index 000000000..eaf73fe60
--- /dev/null
+++ b/Documentation/hwmon/mp2891.rst
@@ -0,0 +1,95 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+Kernel driver mp2891
+====================
+
+Supported chips:
+
+  * MPS mp2891
+
+    Prefix: 'mp2891'
+
+  * Datasheet
+
+    Publicly available at the MPS website : https://www.monolithicpower.com/en/mp2891.html
+
+Author:
+
+	Noah Wang <noahwang.wang@outlook.com>
+
+Description
+-----------
+
+This driver implements support for Monolithic Power Systems, Inc. (MPS)
+MP2891 Multi-phase Digital VR Controller.
+
+Device compliant with:
+
+- PMBus rev 1.3 interface.
+
+Device supports direct and linear format for reading input voltage,
+output voltage, input currect, output current, input power, output
+power, and temperature.
+
+The driver exports the following attributes via the 'sysfs' files
+for input voltage:
+
+**in1_input**
+
+**in1_label**
+
+The driver provides the following attributes for output voltage:
+
+**in2_input**
+
+**in2_label**
+
+**in3_input**
+
+**in3_label**
+
+The driver provides the following attributes for input current:
+
+**curr1_input**
+
+**curr1_label**
+
+**curr2_input**
+
+**curr2_label**
+
+The driver provides the following attributes for output current:
+
+**curr3_input**
+
+**curr3_label**
+
+**curr4_input**
+
+**curr4_label**
+
+The driver provides the following attributes for input power:
+
+**power1_input**
+
+**power1_label**
+
+**power2_input**
+
+**power2_label**
+
+The driver provides the following attributes for output power:
+
+**power3_input**
+
+**power3_label**
+
+**power4_input**
+
+**power4_label**
+
+The driver provides the following attributes for temperature:
+
+**temp1_input**
+
+**temp2_input**
diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
index 557ae0c41..b8b6c7724 100644
--- a/drivers/hwmon/pmbus/Kconfig
+++ b/drivers/hwmon/pmbus/Kconfig
@@ -361,6 +361,15 @@ config SENSORS_MP5990
 	  This driver can also be built as a module. If so, the module will
 	  be called mp5990.

+config SENSORS_MP2891
+    tristate "MPS MP2891"
+    help
+      If you say yes here you get hardware monitoring support for MPS
+      MP2891 Dual Loop Digital Multi-Phase Controller.
+
+      This driver can also be built as a module. If so, the module will
+      be called mp2891.
+
 config SENSORS_MPQ7932_REGULATOR
 	bool "Regulator support for MPQ7932"
 	depends on SENSORS_MPQ7932 && REGULATOR
diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile
index f14ecf03a..57b91c20e 100644
--- a/drivers/hwmon/pmbus/Makefile
+++ b/drivers/hwmon/pmbus/Makefile
@@ -38,6 +38,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
 obj-$(CONFIG_SENSORS_MPQ7932)	+= mpq7932.o
 obj-$(CONFIG_SENSORS_MPQ8785)	+= mpq8785.o
 obj-$(CONFIG_SENSORS_PLI1209BC)	+= pli1209bc.o
diff --git a/drivers/hwmon/pmbus/mp2891.c b/drivers/hwmon/pmbus/mp2891.c
new file mode 100644
index 000000000..c98d9ec6b
--- /dev/null
+++ b/drivers/hwmon/pmbus/mp2891.c
@@ -0,0 +1,210 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Hardware monitoring driver for MPS Multi-phase Digital VR Controllers(MP2891)
+ */
+
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include "pmbus.h"
+
+/* Vendor specific registers, the register READ_PIN_EST(0x94),
+ * MFR_VOUT_LOOP_CTRL(0xBD) and READ_IIN_EST(0x95) redefine
+ * the standard PMBUS register.
+ */
+#define MFR_VOUT_LOOP_CTRL      0xBD
+#define READ_PIN_EST            0x94
+#define READ_IIN_EST            0x95
+
+#define MP2891_PAGE_NUM			2
+
+#define MP2891_RAIL1_FUNC	(PMBUS_HAVE_VIN | PMBUS_HAVE_VOUT | \
+							PMBUS_HAVE_IOUT | PMBUS_HAVE_TEMP | \
+							PMBUS_HAVE_POUT | PMBUS_HAVE_PIN | \
+							PMBUS_HAVE_IIN | PMBUS_PHASE_VIRTUAL)
+
+#define MP2891_RAIL2_FUNC	(PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT | \
+							PMBUS_HAVE_TEMP | PMBUS_HAVE_POUT | \
+							PMBUS_HAVE_IIN | PMBUS_PHASE_VIRTUAL)
+
+struct mp2891_data {
+	struct pmbus_driver_info info;
+};
+
+#define to_mp2891_data(x) container_of(x, struct mp2891_data, info)
+
+static int mp2891_read_byte_data(struct i2c_client *client, int page, int reg)
+{
+	int ret;
+
+	switch (reg) {
+	case PMBUS_VOUT_MODE:
+		ret = PB_VOUT_MODE_DIRECT;
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+
+	return ret;
+}
+
+static int mp2891_read_word_data(struct i2c_client *client, int page, int phase,
+			      int reg)
+{
+	int ret;
+
+	switch (reg) {
+	case PMBUS_READ_VIN:
+	case PMBUS_READ_IOUT:
+	case PMBUS_READ_POUT:
+	case PMBUS_READ_VOUT:
+	case PMBUS_READ_TEMPERATURE_1:
+		ret = pmbus_read_word_data(client, page, phase, reg);
+		break;
+	case PMBUS_READ_IIN:
+		ret = pmbus_read_word_data(client, page, phase, READ_IIN_EST);
+		break;
+	case PMBUS_READ_PIN:
+		ret = pmbus_read_word_data(client, page, phase, READ_PIN_EST);
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+
+	return ret;
+}
+
+static int
+mp2891_identify_vout_scale(struct i2c_client *client, struct mp2891_data *data,
+							u32 reg, int page)
+{
+	int ret;
+
+	ret = i2c_smbus_write_byte_data(client, PMBUS_PAGE, page);
+	if (ret < 0)
+		return ret;
+
+	ret = i2c_smbus_read_word_data(client, reg);
+	if (ret < 0)
+		return ret;
+
+	/*
+	 * Obtain vout scale from the register MFR_VOUT_LOOP_CTRL, bits 15-14,bit 13.
+	 * If MFR_VOUT_LOOP_CTRL[13] = 1, the vout scale is below:
+	 * 2.5mV/LSB
+	 * If MFR_VOUT_LOOP_CTRL[13] = 0, the vout scale is decided by
+	 * MFR_VOUT_LOOP_CTRL[15:14]:
+	 * 00b - 6.25mV/LSB, 01b - 5mV/LSB, 10b - 2mV/LSB, 11b - 1mV
+	 */
+	if (ret & GENMASK(13, 13)) {
+		data->info.m[PSC_VOLTAGE_OUT] = 4;
+		data->info.R[PSC_VOLTAGE_OUT] = -1;
+		data->info.b[PSC_VOLTAGE_OUT] = 0;
+	} else {
+		ret = (ret & GENMASK(15, 14)) >> 14;
+		if (ret == 0) {
+			data->info.m[PSC_VOLTAGE_OUT] = 16;
+			data->info.R[PSC_VOLTAGE_OUT] = -2;
+			data->info.b[PSC_VOLTAGE_OUT] = 0;
+		} else if (ret == 1) {
+			data->info.m[PSC_VOLTAGE_OUT] = 2;
+			data->info.R[PSC_VOLTAGE_OUT] = -1;
+			data->info.b[PSC_VOLTAGE_OUT] = 0;
+		} else if (ret == 2) {
+			data->info.m[PSC_VOLTAGE_OUT] = 5;
+			data->info.R[PSC_VOLTAGE_OUT] = -1;
+			data->info.b[PSC_VOLTAGE_OUT] = 0;
+		} else {
+			data->info.m[PSC_VOLTAGE_OUT] = 1;
+			data->info.R[PSC_VOLTAGE_OUT] = 0;
+			data->info.b[PSC_VOLTAGE_OUT] = 0;
+		}
+	}
+
+	return 0;
+}
+
+static int
+mp2891_identify_rails_vout_scale(struct i2c_client *client, struct mp2891_data *data)
+{
+	int ret;
+
+	/* Identify vout scale from register  MFR_VOUT_LOOP_CTRL. */
+	/* Identify vout scale for rail 1. */
+	ret = mp2891_identify_vout_scale(client, data, MFR_VOUT_LOOP_CTRL, 0);
+	if (ret < 0)
+		return ret;
+
+	/* Identify vout scale for rail 2. */
+	ret = mp2891_identify_vout_scale(client, data, MFR_VOUT_LOOP_CTRL, 1);
+
+	return ret;
+}
+
+static struct pmbus_driver_info mp2891_info = {
+	.pages = MP2891_PAGE_NUM,
+	.format[PSC_VOLTAGE_IN] = linear,
+	.format[PSC_CURRENT_IN] = linear,
+	.format[PSC_CURRENT_OUT] = linear,
+	.format[PSC_TEMPERATURE] = linear,
+	.format[PSC_POWER] = linear,
+	.format[PSC_VOLTAGE_OUT] = direct,
+
+	.func[0] = MP2891_RAIL1_FUNC,
+	.func[1] = MP2891_RAIL2_FUNC,
+	.read_word_data = mp2891_read_word_data,
+	.read_byte_data = mp2891_read_byte_data,
+};
+
+static int mp2891_probe(struct i2c_client *client)
+{
+	struct pmbus_driver_info *info;
+	struct mp2891_data *data;
+	int ret;
+
+	data = devm_kzalloc(&client->dev, sizeof(struct mp2891_data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	memcpy(&data->info, &mp2891_info, sizeof(*info));
+	info = &data->info;
+
+	/* Identify vout scale per rail. */
+	ret = mp2891_identify_rails_vout_scale(client, data);
+	if (ret < 0)
+		return ret;
+
+	return pmbus_do_probe(client, info);
+}
+
+static const struct i2c_device_id mp2891_id[] = {
+	{"mp2891", 0},
+	{}
+};
+MODULE_DEVICE_TABLE(i2c, mp2891_id);
+
+static const struct of_device_id __maybe_unused mp2891_of_match[] = {
+	{.compatible = "mps,mp2891"},
+	{}
+};
+MODULE_DEVICE_TABLE(of, mp2891_of_match);
+
+static struct i2c_driver mp2891_driver = {
+	.driver = {
+		.name = "mp2891",
+		.of_match_table = mp2891_of_match,
+	},
+	.probe = mp2891_probe,
+	.id_table = mp2891_id,
+};
+
+module_i2c_driver(mp2891_driver);
+
+MODULE_AUTHOR("Noah Wang <noahwang.wang@outlook.com>");
+MODULE_DESCRIPTION("PMBus driver for MPS MP2891 device");
+MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS(PMBUS);
--
2.25.1

Re: [PATCH] hwmon: add MP2891 driver

[Bagas Sanjaya]

[-- Attachment #1: Type: text/plain, Size: 888 bytes --]

On Mon, May 13, 2024 at 01:52:25PM +0800, Noah Wang wrote:
>  Documentation/hwmon/mp2891.rst |  95 +++++++++++++++
>  drivers/hwmon/pmbus/Kconfig    |   9 ++
>  drivers/hwmon/pmbus/Makefile   |   1 +
>  drivers/hwmon/pmbus/mp2891.c   | 210 +++++++++++++++++++++++++++++++++
>  4 files changed, 315 insertions(+)
>  create mode 100644 Documentation/hwmon/mp2891.rst
>  create mode 100644 drivers/hwmon/pmbus/mp2891.c

You forget to add toctree entry:

---- >8 ----
diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
index 1ca7a4fe1f8f57..5825a056ad054f 100644
--- a/Documentation/hwmon/index.rst
+++ b/Documentation/hwmon/index.rst
@@ -164,6 +164,7 @@ Hardware Monitoring Kernel Drivers
    mlxreg-fan
    mp2856
    mp2888
+   mp2891
    mp2975
    mp5023
    mp5990

Thanks.

-- 
An old man doll... just what I always wanted! - Clara

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH] hwmon: add MP2891 driver

[Guenter Roeck]

On 5/12/24 22:52, Noah Wang wrote:
> This driver is designed for MPS VR controller mp2891. The input
> voltage, output voltage, input current, output current, input
> power, output power and temperature of per rail can be obtained
> from hwmon sysfs that the driver provided.
> 
> Signed-off-by: Noah Wang <noahwang.wang@outlook.com>
> ---
>   Documentation/hwmon/mp2891.rst |  95 +++++++++++++++

Needs to be added to index.rst.

>   drivers/hwmon/pmbus/Kconfig    |   9 ++
>   drivers/hwmon/pmbus/Makefile   |   1 +
>   drivers/hwmon/pmbus/mp2891.c   | 210 +++++++++++++++++++++++++++++++++
>   4 files changed, 315 insertions(+)
>   create mode 100644 Documentation/hwmon/mp2891.rst
>   create mode 100644 drivers/hwmon/pmbus/mp2891.c
> 
> diff --git a/Documentation/hwmon/mp2891.rst b/Documentation/hwmon/mp2891.rst
> new file mode 100644
> index 000000000..eaf73fe60
> --- /dev/null
> +++ b/Documentation/hwmon/mp2891.rst
> @@ -0,0 +1,95 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +Kernel driver mp2891
> +====================
> +
> +Supported chips:
> +
> +  * MPS mp2891
> +
> +    Prefix: 'mp2891'
> +
> +  * Datasheet
> +
> +    Publicly available at the MPS website : https://www.monolithicpower.com/en/mp2891.html

No, it isn't. It is not even accessible with an account (the provided "datasheet"
is a useless one-pager).

> +
> +Author:
> +
> +	Noah Wang <noahwang.wang@outlook.com>
> +
> +Description
> +-----------
> +
> +This driver implements support for Monolithic Power Systems, Inc. (MPS)
> +MP2891 Multi-phase Digital VR Controller.
> +
> +Device compliant with:
> +
> +- PMBus rev 1.3 interface.
> +
> +Device supports direct and linear format for reading input voltage,
> +output voltage, input currect, output current, input power, output
> +power, and temperature.
> +
> +The driver exports the following attributes via the 'sysfs' files
> +for input voltage:
> +
> +**in1_input**
> +
> +**in1_label**
> +
> +The driver provides the following attributes for output voltage:
> +
> +**in2_input**
> +
> +**in2_label**
> +
> +**in3_input**
> +
> +**in3_label**
> +
> +The driver provides the following attributes for input current:
> +
> +**curr1_input**
> +
> +**curr1_label**
> +
> +**curr2_input**
> +
> +**curr2_label**
> +
> +The driver provides the following attributes for output current:
> +
> +**curr3_input**
> +
> +**curr3_label**
> +
> +**curr4_input**
> +
> +**curr4_label**
> +
> +The driver provides the following attributes for input power:
> +
> +**power1_input**
> +
> +**power1_label**
> +
> +**power2_input**
> +
> +**power2_label**
> +
> +The driver provides the following attributes for output power:
> +
> +**power3_input**
> +
> +**power3_label**
> +
> +**power4_input**
> +
> +**power4_label**
> +
> +The driver provides the following attributes for temperature:
> +
> +**temp1_input**
> +
> +**temp2_input**
> diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
> index 557ae0c41..b8b6c7724 100644
> --- a/drivers/hwmon/pmbus/Kconfig
> +++ b/drivers/hwmon/pmbus/Kconfig
> @@ -361,6 +361,15 @@ config SENSORS_MP5990
>   	  This driver can also be built as a module. If so, the module will
>   	  be called mp5990.
> 
> +config SENSORS_MP2891
> +    tristate "MPS MP2891"
> +    help
> +      If you say yes here you get hardware monitoring support for MPS
> +      MP2891 Dual Loop Digital Multi-Phase Controller.
> +
> +      This driver can also be built as a module. If so, the module will
> +      be called mp2891.
> +
>   config SENSORS_MPQ7932_REGULATOR
>   	bool "Regulator support for MPQ7932"
>   	depends on SENSORS_MPQ7932 && REGULATOR
> diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile
> index f14ecf03a..57b91c20e 100644
> --- a/drivers/hwmon/pmbus/Makefile
> +++ b/drivers/hwmon/pmbus/Makefile
> @@ -38,6 +38,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
>   obj-$(CONFIG_SENSORS_MPQ7932)	+= mpq7932.o
>   obj-$(CONFIG_SENSORS_MPQ8785)	+= mpq8785.o
>   obj-$(CONFIG_SENSORS_PLI1209BC)	+= pli1209bc.o
> diff --git a/drivers/hwmon/pmbus/mp2891.c b/drivers/hwmon/pmbus/mp2891.c
> new file mode 100644
> index 000000000..c98d9ec6b
> --- /dev/null
> +++ b/drivers/hwmon/pmbus/mp2891.c
> @@ -0,0 +1,210 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Hardware monitoring driver for MPS Multi-phase Digital VR Controllers(MP2891)
> + */
> +
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include "pmbus.h"
> +
> +/* Vendor specific registers, the register READ_PIN_EST(0x94),

Please use standard multi-line comments.

> + * MFR_VOUT_LOOP_CTRL(0xBD) and READ_IIN_EST(0x95) redefine
> + * the standard PMBUS register.

That doesn't explain the reason for using the _EST registers
instead of the standard registers.

> + */
> +#define MFR_VOUT_LOOP_CTRL      0xBD
> +#define READ_PIN_EST            0x94
> +#define READ_IIN_EST            0x95
> +
> +#define MP2891_PAGE_NUM			2
> +
> +#define MP2891_RAIL1_FUNC	(PMBUS_HAVE_VIN | PMBUS_HAVE_VOUT | \
> +							PMBUS_HAVE_IOUT | PMBUS_HAVE_TEMP | \
> +							PMBUS_HAVE_POUT | PMBUS_HAVE_PIN | \
> +							PMBUS_HAVE_IIN | PMBUS_PHASE_VIRTUAL)
> +
> +#define MP2891_RAIL2_FUNC	(PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT | \
> +							PMBUS_HAVE_TEMP | PMBUS_HAVE_POUT | \
> +							PMBUS_HAVE_IIN | PMBUS_PHASE_VIRTUAL)

What is the point of PMBUS_PHASE_VIRTUAL ?

> +
> +struct mp2891_data {
> +	struct pmbus_driver_info info;
> +};
> +
> +#define to_mp2891_data(x) container_of(x, struct mp2891_data, info)
> +
Unused.

> +static int mp2891_read_byte_data(struct i2c_client *client, int page, int reg)
> +{
> +	int ret;
> +
> +	switch (reg) {
> +	case PMBUS_VOUT_MODE:
> +		ret = PB_VOUT_MODE_DIRECT;

Needs explanation.

> +		break;
> +	default:
> +		ret = -EINVAL;

No status registers ? I do not believe this is correct.

> +		break;
> +	}
> +
> +	return ret;
> +}
> +
> +static int mp2891_read_word_data(struct i2c_client *client, int page, int phase,
> +			      int reg)
> +{
> +	int ret;
> +
> +	switch (reg) {
> +	case PMBUS_READ_VIN:
> +	case PMBUS_READ_IOUT:
> +	case PMBUS_READ_POUT:
> +	case PMBUS_READ_VOUT:
> +	case PMBUS_READ_TEMPERATURE_1:
> +		ret = pmbus_read_word_data(client, page, phase, reg);

Use
		return -ENODATA;
instead.

> +		break;
> +	case PMBUS_READ_IIN:
> +		ret = pmbus_read_word_data(client, page, phase, READ_IIN_EST);
> +		break;
> +	case PMBUS_READ_PIN:
> +		ret = pmbus_read_word_data(client, page, phase, READ_PIN_EST);

This needs an explanation. Why not use standard READ_IIN and READ_PIN ?

> +		break;
> +	default:
> +		ret = -EINVAL;

No limit registers ? That seems extremely unlikely.


Given that you are essentially claiming that the chip would support
almost no standard registers, I'll need to see the datasheet to confirm.

> +		break;
> +	}
> +
> +	return ret;
> +}
> +
> +static int
> +mp2891_identify_vout_scale(struct i2c_client *client, struct mp2891_data *data,
> +							u32 reg, int page)
> +{
> +	int ret;
> +
> +	ret = i2c_smbus_write_byte_data(client, PMBUS_PAGE, page);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = i2c_smbus_read_word_data(client, reg);
> +	if (ret < 0)
> +		return ret;
> +
> +	/*
> +	 * Obtain vout scale from the register MFR_VOUT_LOOP_CTRL, bits 15-14,bit 13.
> +	 * If MFR_VOUT_LOOP_CTRL[13] = 1, the vout scale is below:
> +	 * 2.5mV/LSB
> +	 * If MFR_VOUT_LOOP_CTRL[13] = 0, the vout scale is decided by
> +	 * MFR_VOUT_LOOP_CTRL[15:14]:
> +	 * 00b - 6.25mV/LSB, 01b - 5mV/LSB, 10b - 2mV/LSB, 11b - 1mV
> +	 */
> +	if (ret & GENMASK(13, 13)) {
> +		data->info.m[PSC_VOLTAGE_OUT] = 4;
> +		data->info.R[PSC_VOLTAGE_OUT] = -1;
> +		data->info.b[PSC_VOLTAGE_OUT] = 0;
> +	} else {
> +		ret = (ret & GENMASK(15, 14)) >> 14;

Why not FIELD_GET() ?

> +		if (ret == 0) {
> +			data->info.m[PSC_VOLTAGE_OUT] = 16;
> +			data->info.R[PSC_VOLTAGE_OUT] = -2;
> +			data->info.b[PSC_VOLTAGE_OUT] = 0;
> +		} else if (ret == 1) {
> +			data->info.m[PSC_VOLTAGE_OUT] = 2;
> +			data->info.R[PSC_VOLTAGE_OUT] = -1;
> +			data->info.b[PSC_VOLTAGE_OUT] = 0;
> +		} else if (ret == 2) {
> +			data->info.m[PSC_VOLTAGE_OUT] = 5;
> +			data->info.R[PSC_VOLTAGE_OUT] = -1;
> +			data->info.b[PSC_VOLTAGE_OUT] = 0;
> +		} else {
> +			data->info.m[PSC_VOLTAGE_OUT] = 1;
> +			data->info.R[PSC_VOLTAGE_OUT] = 0;
> +			data->info.b[PSC_VOLTAGE_OUT] = 0;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int
> +mp2891_identify_rails_vout_scale(struct i2c_client *client, struct mp2891_data *data)
> +{
> +	int ret;
> +
> +	/* Identify vout scale from register  MFR_VOUT_LOOP_CTRL. */

Useless comment.

> +	/* Identify vout scale for rail 1. */
> +	ret = mp2891_identify_vout_scale(client, data, MFR_VOUT_LOOP_CTRL, 0);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Identify vout scale for rail 2. */
> +	ret = mp2891_identify_vout_scale(client, data, MFR_VOUT_LOOP_CTRL, 1);

Passing the register to this function is useless. Just use the register directly
in mp2891_identify_vout_scale().

> +
> +	return ret;
> +}
> +
> +static struct pmbus_driver_info mp2891_info = {
> +	.pages = MP2891_PAGE_NUM,
> +	.format[PSC_VOLTAGE_IN] = linear,
> +	.format[PSC_CURRENT_IN] = linear,
> +	.format[PSC_CURRENT_OUT] = linear,
> +	.format[PSC_TEMPERATURE] = linear,
> +	.format[PSC_POWER] = linear,
> +	.format[PSC_VOLTAGE_OUT] = direct,
> +
> +	.func[0] = MP2891_RAIL1_FUNC,
> +	.func[1] = MP2891_RAIL2_FUNC,
> +	.read_word_data = mp2891_read_word_data,
> +	.read_byte_data = mp2891_read_byte_data,
> +};
> +
> +static int mp2891_probe(struct i2c_client *client)
> +{
> +	struct pmbus_driver_info *info;
> +	struct mp2891_data *data;
> +	int ret;
> +
> +	data = devm_kzalloc(&client->dev, sizeof(struct mp2891_data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	memcpy(&data->info, &mp2891_info, sizeof(*info));
> +	info = &data->info;
> +
> +	/* Identify vout scale per rail. */
> +	ret = mp2891_identify_rails_vout_scale(client, data);
> +	if (ret < 0)
> +		return ret;
> +

Why not use the identify() callback instead ?

> +	return pmbus_do_probe(client, info);
> +}
> +
> +static const struct i2c_device_id mp2891_id[] = {
> +	{"mp2891", 0},
> +	{}
> +};
> +MODULE_DEVICE_TABLE(i2c, mp2891_id);
> +
> +static const struct of_device_id __maybe_unused mp2891_of_match[] = {
> +	{.compatible = "mps,mp2891"},
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, mp2891_of_match);
> +
> +static struct i2c_driver mp2891_driver = {
> +	.driver = {
> +		.name = "mp2891",
> +		.of_match_table = mp2891_of_match,
> +	},
> +	.probe = mp2891_probe,
> +	.id_table = mp2891_id,
> +};
> +
> +module_i2c_driver(mp2891_driver);
> +
> +MODULE_AUTHOR("Noah Wang <noahwang.wang@outlook.com>");
> +MODULE_DESCRIPTION("PMBus driver for MPS MP2891 device");

drop "device"

> +MODULE_LICENSE("GPL");
> +MODULE_IMPORT_NS(PMBUS);
> --
> 2.25.1
> 

Re: [PATCH] hwmon: add MP2891 driver

[Guenter Roeck]

On 5/12/24 22:52, Noah Wang wrote:
> This driver is designed for MPS VR controller mp2891. The input
> voltage, output voltage, input current, output current, input
> power, output power and temperature of per rail can be obtained
> from hwmon sysfs that the driver provided.
> 
> Signed-off-by: Noah Wang <noahwang.wang@outlook.com>

The chip name looked vaguely familiar, and indeed I found:

https://patchwork.kernel.org/project/linux-hwmon/patch/TYZPR03MB7130A7F41D61BFB611DDF0C7FA479@TYZPR03MB7130.apcprd03.prod.outlook.com/

So this is really v2 of your patch without saying so,
it pretty much ignores all feedback I sent as response to v1,
and it does drop support for limit and status attributes for no
reason.

Please don't do that.

Guenter