Re: [PATCH v2] hwmon: (pmbus) Add support for MPS MP2949A

Hi，Guenter
    Thanks for the review. It all sounds reasonable. I'll address it all in v3.
Best regards,
Ainux Wang.


On Sat, 19 Jun 2021 at 03:39, Guenter Roeck [off-list ref] wrote:

On Fri, Jun 18, 2021 at 11:09:34AM +0800, ainux.wang@gmail.com wrote:

quoted

From: "Ainux.Wang" <redacted>

Add support for MP2949A device from Monolithic Power Systems, Inc. (MPS).
This is a triple-loop, digital, multi-phase controller.
This device:
- Supports up to three power rail.
- Provides 6 pulse-width modulations (PWMs), and can be configured up
  to 6-phase operation for Rail A , up to 2-phase operation for Rail B
  and up to 1-phase operation for Rail C.
- The PMBus registers are distributed into three pages: Page 0, Page 1,
  Page 2. Page 0 contains the registers for Rail A and most of the common
  settings for all of the rails. Page 1 contains register information for
  Rail B. Page 2 contains register information for Rail C.
- The MP2949A supports both 5mV VID step and 10mv VID step for IMVP8 and
  IMVP9 with only one DAC for each rail to generate REF.

Signed-off-by: Ainux.Wang <redacted>
---

Change log goes here. My normal response to patches with missing change log is:

<Formletter>
Change log goes here. If it is missing, I won't know what changed.
That means I will have to dig out older patch versions to compare.
That costs time and would hold up both this patch as well as all other
patches which I still have to review.

For this reason, I will not review patches without change log.
</Formletter>

so you are lucky that your previous version is recent enough that I remember
at least some of it. However, please do provide change logs in the future.

quoted

 Documentation/hwmon/index.rst   |   1 +
 Documentation/hwmon/mp2949a.rst |  44 ++++++++++++
 drivers/hwmon/pmbus/Kconfig     |   9 +++
 drivers/hwmon/pmbus/Makefile    |   1 +
 drivers/hwmon/pmbus/mp2949a.c   | 119 ++++++++++++++++++++++++++++++++
 5 files changed, 174 insertions(+)
 create mode 100644 Documentation/hwmon/mp2949a.rst
 create mode 100644 drivers/hwmon/pmbus/mp2949a.c

diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
index 9ed60fa84cbe..56aac3b1678d 100644
--- a/Documentation/hwmon/index.rst
+++ b/Documentation/hwmon/index.rst

@@ -137,6 +137,7 @@ Hardware Monitoring Kernel Drivers
    mcp3021
    menf21bmc
    mlxreg-fan
+   mp2949a
    mp2975
    nct6683
    nct6775

diff --git a/Documentation/hwmon/mp2949a.rst b/Documentation/hwmon/mp2949a.rst
new file mode 100644
index 000000000000..ac4084e067f1
--- /dev/null
+++ b/Documentation/hwmon/mp2949a.rst

@@ -0,0 +1,44 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+Kernel driver mp2949a
+====================
+
+Supported chips:
+
+  * MPS MP2949A
+
+    Prefix: 'mp2949a'
+
+Author:
+
+     Ainux Wang <ainux.wang@gmail.com>
+
+Description
+-----------
+
+This driver implements support for Monolithic Power Systems, Inc. (MPS)
+triple-loop, digital, multi-phase controller MP2949A.
+
+This device:
+
+- Supports up to three power rail.

rails

quoted

+- Provides 6 pulse-width modulations (PWMs), and can be configured up

can be configured for ...

quoted

+  to 6-phase operation for Rail A , up to 2-phase operation for Rail B

Rail B,

quoted

+  and up to 1-phase operation for Rail C.
+- The PMBus registers are distributed into three pages: Page 0, Page 1,
+  Page 2. Page 0 contains the registers for Rail A and most of the common
+  settings for all of the rails. Page 1 contains register information for
+  Rail B. Page 2 contains register information for Rail C.
+- The MP2949A supports both 5mV VID step and 10mv VID step for IMVP8 and
+  IMVP9 with only one DAC for each rail to generate REF.

Please explain REF.

quoted

+
+Device supports:
+
+- SVID interface.
+- PMBus rev 1.2 interface.
+
+Device supports direct format for reading output power.
+Device supports linear format for reading input voltage and output current

input voltage, output current, and temperature.

quoted

+and temperature.
+Device supports VID for reading output voltage.
+The below VID modes are supported: VR12, VR13, IMVP8, IMVP9.

The following

quoted

diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
index 37a5c39784fa..b1344b265976 100644
--- a/drivers/hwmon/pmbus/Kconfig
+++ b/drivers/hwmon/pmbus/Kconfig

@@ -248,6 +248,15 @@ config SENSORS_MAX8688
        This driver can also be built as a module. If so, the module will
        be called max8688.

+config SENSORS_MP2949A
+     tristate "MPS MP2949A"
+     help
+       If you say yes here you get hardware monitoring support for MPS
+       MP2949A Triple Loop Digital Multi-Phase Controller.
+
+       This driver can also be built as a module. If so, the module will
+       be called mp2949a.
+
 config SENSORS_MP2975
      tristate "MPS MP2975"
      help

diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile
index f8dcc27cd56a..bfb55ab12da1 100644
--- a/drivers/hwmon/pmbus/Makefile
+++ b/drivers/hwmon/pmbus/Makefile

@@ -28,6 +28,7 @@ obj-$(CONFIG_SENSORS_MAX20751)      += max20751.o
 obj-$(CONFIG_SENSORS_MAX31785)       += max31785.o
 obj-$(CONFIG_SENSORS_MAX34440)       += max34440.o
 obj-$(CONFIG_SENSORS_MAX8688)        += max8688.o
+obj-$(CONFIG_SENSORS_MP2949A)        += mp2949a.o
 obj-$(CONFIG_SENSORS_MP2975) += mp2975.o
 obj-$(CONFIG_SENSORS_PM6764TR)       += pm6764tr.o
 obj-$(CONFIG_SENSORS_PXE1610)        += pxe1610.o

diff --git a/drivers/hwmon/pmbus/mp2949a.c b/drivers/hwmon/pmbus/mp2949a.c
new file mode 100644
index 000000000000..d68e8526abe3
--- /dev/null
+++ b/drivers/hwmon/pmbus/mp2949a.c

@@ -0,0 +1,119 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Hardware monitoring driver for Monolithic Power Systems MP2949A
+ *
+ * Copyright (c) 2021 Lemote Technologies. All rights reserved.
+ * Copyright (c) 2021 Ainux <ainux.wang@gmail.com>
+ */
+
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include "pmbus.h"
+
+#define MP2949A_PAGE_NUM             3
+
+static int mp2949a_read_byte_data(struct i2c_client *client, int page, int reg)
+{
+     switch (reg) {
+     case PMBUS_VOUT_MODE:
+             /* This chip do not support the VOUT_MODE command. */

s/do/does/

This warrants a more detailed explanation: What happens if the command
is sent to the chip, and why is it not possible to rely on the standard
response to unsupported commands ?

quoted

+             return -EINVAL;
+     default:
+             return -ENODATA;
+     }
+}
+
+static int mp2949a_identify(struct i2c_client *client,
+                         struct pmbus_driver_info *info)
+{
+     u8 vout_params;
+     int i, ret;
+
+     for (i = 0; i < MP2949A_PAGE_NUM; i++) {
+             /* Read the register with VOUT scaling value.*/
+             ret = pmbus_read_byte_data(client, i, PMBUS_VOUT_MODE);

                                                      ^^^^^^^^^^^^^^^
Should this be MFR_VR_CONFIG ?

quoted

+             if (ret < 0)
+                     return ret;
+
+             /*
+              * Rail A bit 5, Rail B bit 4, Rail C bit 3.
+              * 1'b1: 5mV  (vr12/imvp8)
+              * 1'b0: 10mv (imvp9)
+              */
+             vout_params = ret & ~BIT(5-i);

space before and after '-'. vout_params is really unnecessary; just do
the check directly in the if() statement.

quoted

+             if (vout_params)
+                     info->vrm_version[i] = vr12;
+             else
+                     info->vrm_version[i] = imvp9;
+

Unnecessary empty line.

checkpatch --strict reports this and the above problem, plus another
alignment problem. Please fix everything it reports.

quoted

+     }
+
+     return 0;
+}
+
+static struct pmbus_driver_info mp2949a_info = {
+     .pages = MP2949A_PAGE_NUM,
+     .format[PSC_VOLTAGE_IN] = linear,
+     .format[PSC_VOLTAGE_OUT] = vid,
+     .format[PSC_CURRENT_OUT] = linear,
+     .format[PSC_TEMPERATURE] = linear,
+     .format[PSC_POWER] = direct,
+     .m[PSC_POWER] = 1,
+     .b[PSC_POWER] = 0,
+     .R[PSC_POWER] = 0,
+     .func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT |
+             PMBUS_HAVE_IOUT | PMBUS_HAVE_STATUS_IOUT |
+             PMBUS_HAVE_TEMP | PMBUS_HAVE_POUT,
+     .func[1] = PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT |
+             PMBUS_HAVE_IOUT | PMBUS_HAVE_STATUS_IOUT |
+             PMBUS_HAVE_POUT,
+     .func[2] = PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT |
+             PMBUS_HAVE_IOUT | PMBUS_HAVE_STATUS_IOUT |
+             PMBUS_HAVE_POUT,
+     .identify = mp2949a_identify,
+     .read_byte_data = mp2949a_read_byte_data,
+};
+
+static int mp2949a_probe(struct i2c_client *client,
+                       const struct i2c_device_id *id)
+{
+     struct pmbus_driver_info *info;
+
+     info = devm_kmemdup(&client->dev, &mp2949a_info, sizeof(*info),
+                         GFP_KERNEL);
+     if (!info)
+             return -ENOMEM;
+
+     return pmbus_do_probe(client, info);
+}
+
+static const struct i2c_device_id mp2949a_id[] = {
+     {"mp2949a", 0},
+     {}
+};
+
+MODULE_DEVICE_TABLE(i2c, mp2949a_id);
+
+static const struct of_device_id mp2949a_of_match[] = {
+     {.compatible = "mps,mp2949a"},
+     {}
+};
+MODULE_DEVICE_TABLE(of, mp2949a_of_match);
+
+static struct i2c_driver mp2949a_driver = {
+     .driver = {
+             .name = "mp2949a",
+             .of_match_table = of_match_ptr(mp2949a_of_match),
+     },
+     .probe = mp2949a_probe,
+     .id_table = mp2949a_id,
+};
+
+module_i2c_driver(mp2949a_driver);
+
+MODULE_AUTHOR("Ainux [off-list ref]");
+MODULE_DESCRIPTION("PMBus driver for Monolithic Power Systems MP2949A");
+MODULE_LICENSE("GPL");
--
2.18.1