[PATCH v2 0/4] Add support for mpq7932 PMIC IC

[PATCH v2 0/4] Add support for mpq7932 PMIC IC

[Saravanan Sekar]

changes in v2:
  - add new patch to include min_uV in PMBUS_REGULATOR_STEP
  - fix review comments mpq7932 driver, bindings document and Maintiners file

Saravanan Sekar (4):
  hwmon: pm_bus: core: Add min_uV in pmbus regulator helper macro
  dt-bindings: hwmon/pmbus: Add mps,mpq7932 power-management IC
  hwmon: (pmbus/mpq7932) Add a support for mpq7932 Power Management IC
  MAINTAINERS: Update the entry for MPQ7932 PMIC driver

 .../bindings/hwmon/pmbus/mps,mpq7932.yaml     |  63 ++++++++
 MAINTAINERS                                   |   2 +
 drivers/hwmon/pmbus/Kconfig                   |  10 ++
 drivers/hwmon/pmbus/Makefile                  |   1 +
 drivers/hwmon/pmbus/ltc2978.c                 |  16 +-
 drivers/hwmon/pmbus/mpq7932.c                 | 144 ++++++++++++++++++
 drivers/hwmon/pmbus/pmbus.h                   |   5 +-
 7 files changed, 231 insertions(+), 10 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/hwmon/pmbus/mps,mpq7932.yaml
 create mode 100644 drivers/hwmon/pmbus/mpq7932.c

-- 
2.34.1

[PATCH v2 1/4] hwmon: pm_bus: core: Add min_uV in pmbus regulator helper macro

[Saravanan Sekar]

Some regulator operates in a range of voltage which should not allow
below the lower threshold.

Signed-off-by: Saravanan Sekar <saravanan@linumiz.com>
---
 drivers/hwmon/pmbus/ltc2978.c | 16 ++++++++--------
 drivers/hwmon/pmbus/pmbus.h   |  5 +++--
 2 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/drivers/hwmon/pmbus/ltc2978.c b/drivers/hwmon/pmbus/ltc2978.c
index 6d2592731ba3..406a36885ef3 100644
--- a/drivers/hwmon/pmbus/ltc2978.c
+++ b/drivers/hwmon/pmbus/ltc2978.c
@@ -569,14 +569,14 @@ MODULE_DEVICE_TABLE(i2c, ltc2978_id);
 #define LTC2978_N_VOLTAGES	((LTC2978_MAX_UV / LTC2978_UV_STEP) + 1)
 
 static const struct regulator_desc ltc2978_reg_desc[] = {
-	PMBUS_REGULATOR_STEP("vout", 0, LTC2978_N_VOLTAGES, LTC2978_UV_STEP),
-	PMBUS_REGULATOR_STEP("vout", 1, LTC2978_N_VOLTAGES, LTC2978_UV_STEP),
-	PMBUS_REGULATOR_STEP("vout", 2, LTC2978_N_VOLTAGES, LTC2978_UV_STEP),
-	PMBUS_REGULATOR_STEP("vout", 3, LTC2978_N_VOLTAGES, LTC2978_UV_STEP),
-	PMBUS_REGULATOR_STEP("vout", 4, LTC2978_N_VOLTAGES, LTC2978_UV_STEP),
-	PMBUS_REGULATOR_STEP("vout", 5, LTC2978_N_VOLTAGES, LTC2978_UV_STEP),
-	PMBUS_REGULATOR_STEP("vout", 6, LTC2978_N_VOLTAGES, LTC2978_UV_STEP),
-	PMBUS_REGULATOR_STEP("vout", 7, LTC2978_N_VOLTAGES, LTC2978_UV_STEP),
+	PMBUS_REGULATOR_STEP("vout", 0, LTC2978_N_VOLTAGES, LTC2978_UV_STEP, 0),
+	PMBUS_REGULATOR_STEP("vout", 1, LTC2978_N_VOLTAGES, LTC2978_UV_STEP, 0),
+	PMBUS_REGULATOR_STEP("vout", 2, LTC2978_N_VOLTAGES, LTC2978_UV_STEP, 0),
+	PMBUS_REGULATOR_STEP("vout", 3, LTC2978_N_VOLTAGES, LTC2978_UV_STEP, 0),
+	PMBUS_REGULATOR_STEP("vout", 4, LTC2978_N_VOLTAGES, LTC2978_UV_STEP, 0),
+	PMBUS_REGULATOR_STEP("vout", 5, LTC2978_N_VOLTAGES, LTC2978_UV_STEP, 0),
+	PMBUS_REGULATOR_STEP("vout", 6, LTC2978_N_VOLTAGES, LTC2978_UV_STEP, 0),
+	PMBUS_REGULATOR_STEP("vout", 7, LTC2978_N_VOLTAGES, LTC2978_UV_STEP, 0),
 };
 
 static const struct regulator_desc ltc2978_reg_desc_default[] = {
diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
index 7daaf0caf4d3..b8c7810c812a 100644
--- a/drivers/hwmon/pmbus/pmbus.h
+++ b/drivers/hwmon/pmbus/pmbus.h
@@ -464,7 +464,7 @@ struct pmbus_driver_info {
 extern const struct regulator_ops pmbus_regulator_ops;
 
 /* Macros for filling in array of struct regulator_desc */
-#define PMBUS_REGULATOR_STEP(_name, _id, _voltages, _step)  \
+#define PMBUS_REGULATOR_STEP(_name, _id, _voltages, _step, _min_uV)  \
 	[_id] = {						\
 		.name = (_name # _id),				\
 		.supply_name = "vin",				\
@@ -476,9 +476,10 @@ extern const struct regulator_ops pmbus_regulator_ops;
 		.owner = THIS_MODULE,				\
 		.n_voltages = _voltages,			\
 		.uV_step = _step,				\
+		.min_uV = _min_uV,				\
 	}
 
-#define PMBUS_REGULATOR(_name, _id)	PMBUS_REGULATOR_STEP(_name, _id, 0, 0)
+#define PMBUS_REGULATOR(_name, _id)   PMBUS_REGULATOR_STEP(_name, _id, 0, 0, 0)
 
 /* Function declarations */
 
-- 
2.34.1

[PATCH v2 2/4] dt-bindings: hwmon/pmbus: Add mps,mpq7932 power-management IC

[Saravanan Sekar]

Document mpq7932 power-management IC

Signed-off-by: Saravanan Sekar <saravanan@linumiz.com>
---
 .../bindings/hwmon/pmbus/mps,mpq7932.yaml     | 63 +++++++++++++++++++
 1 file changed, 63 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/pmbus/mps,mpq7932.yaml

diff --git a/Documentation/devicetree/bindings/hwmon/pmbus/mps,mpq7932.yaml b/Documentation/devicetree/bindings/hwmon/pmbus/mps,mpq7932.yaml
new file mode 100644
index 000000000000..5f20c59eb7ff
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/pmbus/mps,mpq7932.yaml
@@ -0,0 +1,63 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/hwmon/pmbus/mps,mpq7932.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Monolithic Power System MPQ7932 PMIC
+
+maintainers:
+  - Saravanan Sekar <saravanan@linumiz.com>
+
+properties:
+  compatible:
+    enum:
+      - mps,mpq7932
+
+  reg:
+    maxItems: 1
+
+  regulators:
+    type: object
+
+    description: |
+      list of regulators provided by this controller, must be named
+      after their hardware counterparts BUCK[1-6]
+
+      "^buck[1-6]$":
+        type: object
+        $ref: regulator.yaml#
+        unevaluatedProperties: false
+
+    additionalProperties: false
+
+required:
+  - compatible
+  - reg
+  - regulators
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        pmic@3 {
+            compatible = "mps,mpq7932";
+            reg = <0x3>;
+
+            regulators {
+                buck1 {
+                    regulator-name = "buck1";
+                    regulator-min-microvolt = <400000>;
+                    regulator-max-microvolt = <3587500>;
+                    regulator-min-microamp  = <460000>;
+                    regulator-max-microamp  = <7600000>;
+                    regulator-boot-on;
+                };
+            };
+        };
+    };
+...
-- 
2.34.1

[PATCH v2 3/4] hwmon: (pmbus/mpq7932) Add a support for mpq7932 Power Management IC

[Saravanan Sekar]

The MPQ7932 is a power management IC designed to operate from 5V buses to
power a variety of Advanced driver-assistance system SOCs. Six integrated
buck converters with hardware monitoring capability powers a variety of
target rails configurable over PMBus interface.

Signed-off-by: Saravanan Sekar <saravanan@linumiz.com>
---
 drivers/hwmon/pmbus/Kconfig   |  10 +++
 drivers/hwmon/pmbus/Makefile  |   1 +
 drivers/hwmon/pmbus/mpq7932.c | 144 ++++++++++++++++++++++++++++++++++
 3 files changed, 155 insertions(+)
 create mode 100644 drivers/hwmon/pmbus/mpq7932.c

diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
index 89668af67206..4a1538949a73 100644
--- a/drivers/hwmon/pmbus/Kconfig
+++ b/drivers/hwmon/pmbus/Kconfig
@@ -317,6 +317,16 @@ config SENSORS_MP5023
 	  This driver can also be built as a module. If so, the module will
 	  be called mp5023.
 
+config SENSORS_MPQ7932
+	tristate "MPS MPQ7932"
+	help
+	  If you say yes here you get six integrated buck converter regulator
+	  with hardware monitoring functionality support for power management
+	  IC MPS MPQ7932.
+
+	  This driver can also be built as a module. If so, the module will
+	  be called mpq7932.
+
 config SENSORS_PIM4328
 	tristate "Flex PIM4328 and compatibles"
 	help
diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile
index 0002dbe22d52..28a534629cc3 100644
--- a/drivers/hwmon/pmbus/Makefile
+++ b/drivers/hwmon/pmbus/Makefile
@@ -34,6 +34,7 @@ obj-$(CONFIG_SENSORS_MAX8688)	+= max8688.o
 obj-$(CONFIG_SENSORS_MP2888)	+= mp2888.o
 obj-$(CONFIG_SENSORS_MP2975)	+= mp2975.o
 obj-$(CONFIG_SENSORS_MP5023)	+= mp5023.o
+obj-$(CONFIG_SENSORS_MPQ7932_REGULATOR) += mpq7932.o
 obj-$(CONFIG_SENSORS_PLI1209BC)	+= pli1209bc.o
 obj-$(CONFIG_SENSORS_PM6764TR)	+= pm6764tr.o
 obj-$(CONFIG_SENSORS_PXE1610)	+= pxe1610.o
diff --git a/drivers/hwmon/pmbus/mpq7932.c b/drivers/hwmon/pmbus/mpq7932.c
new file mode 100644
index 000000000000..3747d7862afd
--- /dev/null
+++ b/drivers/hwmon/pmbus/mpq7932.c
@@ -0,0 +1,144 @@
+/*
+ * SPDX-License-Identifier: GPL-2.0+
+ *
+ * mpq7932.c  - regulator driver for mps mpq7932
+ * Copyright 2022 Monolithic Power Systems, Inc
+ *
+ * Author: Saravanan Sekar <saravanan@linumiz.com>
+ */
+
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/pmbus.h>
+#include "pmbus.h"
+
+#define MPQ7932_BUCK_UV_MIN		206250
+#define MPQ7932_UV_STEP			6250
+#define MPQ7932_N_VOLTAGES		0xFF
+#define MPQ7932_NUM_PAGES		6
+
+#define MPQ7932_TON_DELAY		0x60
+#define MPQ7932_VOUT_STARTUP_SLEW	0xA3
+#define MPQ7932_VOUT_SHUTDOWN_SLEW	0xA5
+#define MPQ7932_VOUT_SLEW_MASK		GENMASK(1, 0)
+#define MPQ7932_TON_DELAY_MASK		GENMASK(4, 0)
+
+struct mpq7932_data {
+	struct pmbus_driver_info info;
+	struct pmbus_platform_data pdata;
+};
+
+static struct regulator_desc mpq7932_regulators_desc[] = {
+	PMBUS_REGULATOR_STEP("buck", 0, MPQ7932_N_VOLTAGES,
+				MPQ7932_UV_STEP, MPQ7932_BUCK_UV_MIN),
+	PMBUS_REGULATOR_STEP("buck", 1, MPQ7932_N_VOLTAGES,
+				MPQ7932_UV_STEP, MPQ7932_BUCK_UV_MIN),
+	PMBUS_REGULATOR_STEP("buck", 2, MPQ7932_N_VOLTAGES,
+				MPQ7932_UV_STEP, MPQ7932_BUCK_UV_MIN),
+	PMBUS_REGULATOR_STEP("buck", 3, MPQ7932_N_VOLTAGES,
+				MPQ7932_UV_STEP, MPQ7932_BUCK_UV_MIN),
+	PMBUS_REGULATOR_STEP("buck", 4, MPQ7932_N_VOLTAGES,
+				MPQ7932_UV_STEP, MPQ7932_BUCK_UV_MIN),
+	PMBUS_REGULATOR_STEP("buck", 5, MPQ7932_N_VOLTAGES,
+				MPQ7932_UV_STEP, MPQ7932_BUCK_UV_MIN),
+};
+
+static int mpq7932_write_word_data(struct i2c_client *client, int page, int reg,
+			       u16 word)
+{
+
+	switch (reg) {
+	case PMBUS_VOUT_COMMAND:
+		return pmbus_write_byte_data(client, page, reg, (u8)word);
+
+	default:
+		return -ENODATA;
+	}
+}
+
+static int mpq7932_read_word_data(struct i2c_client *client, int page,
+				  int phase, int reg)
+{
+
+	switch (reg) {
+	case PMBUS_MFR_VOUT_MIN:
+		return 0;
+
+	case PMBUS_MFR_VOUT_MAX:
+		return MPQ7932_N_VOLTAGES;
+
+	case PMBUS_READ_VOUT:
+		return pmbus_read_byte_data(client, page, PMBUS_VOUT_COMMAND);
+
+	default:
+		return -ENODATA;
+	}
+}
+
+static int mpq7932_probe(struct i2c_client *client)
+{
+	struct mpq7932_data *data;
+	struct pmbus_driver_info *info;
+	struct device *dev = &client->dev;
+	int i;
+
+	if (!i2c_check_functionality(client->adapter,
+				     I2C_FUNC_SMBUS_READ_WORD_DATA))
+		return -ENODEV;
+
+	data = devm_kzalloc(&client->dev, sizeof(struct mpq7932_data),
+			    GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	info = &data->info;
+	info->pages = MPQ7932_NUM_PAGES;
+	info->num_regulators = ARRAY_SIZE(mpq7932_regulators_desc);
+	info->reg_desc = mpq7932_regulators_desc;
+	info->format[PSC_VOLTAGE_OUT] = direct;
+	info->m[PSC_VOLTAGE_OUT] = 160;
+	info->b[PSC_VOLTAGE_OUT] = -33;
+	for (i = 0; i < info->pages; i++) {
+		info->func[i] = PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT
+				| PMBUS_HAVE_STATUS_TEMP;
+	}
+
+	info->read_word_data = mpq7932_read_word_data;
+	info->write_word_data = mpq7932_write_word_data;
+
+	data->pdata.flags = PMBUS_NO_CAPABILITY;
+	dev->platform_data = &data->pdata;
+
+	return pmbus_do_probe(client, info);
+}
+
+static const struct of_device_id mpq7932_of_match[] = {
+	{ .compatible = "mps,mpq7932"},
+	{},
+};
+MODULE_DEVICE_TABLE(of, mpq7932_of_match);
+
+static const struct i2c_device_id mpq7932_id[] = {
+	{ "mpq7932", },
+	{ },
+};
+MODULE_DEVICE_TABLE(i2c, mpq7932_id);
+
+static struct i2c_driver mpq7932_regulator_driver = {
+	.driver = {
+		.name = "mpq7932",
+		.of_match_table = mpq7932_of_match,
+	},
+	.probe_new = mpq7932_probe,
+	.id_table = mpq7932_id,
+};
+module_i2c_driver(mpq7932_regulator_driver);
+
+MODULE_AUTHOR("Saravanan Sekar <saravanan@linumiz.com>");
+MODULE_DESCRIPTION("MPQ7932 PMIC regulator driver");
+MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS(PMBUS);
-- 
2.34.1

[PATCH v2 4/4] MAINTAINERS: Update the entry for MPQ7932 PMIC driver

[Saravanan Sekar]

Update the MAINTAINERS file to include the path for the MPQ7932 and
MPQ7932 devicetree bindings documentation.

Signed-off-by: Saravanan Sekar <saravanan@linumiz.com>
---
 MAINTAINERS | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 379945f82a64..8e0dbf4c6cf3 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -13924,8 +13924,10 @@ F:	scripts/module*
 MONOLITHIC POWER SYSTEM PMIC DRIVER
 M:	Saravanan Sekar <sravanhome@gmail.com>
 S:	Maintained
+F:	Documentation/devicetree/bindings/hwmon/pmbus/mps,mpq7932.yaml
 F:	Documentation/devicetree/bindings/mfd/mps,mp2629.yaml
 F:	Documentation/devicetree/bindings/regulator/mps,mp*.yaml
+F:	drivers/hwmon/pmbus/mpq7932.c
 F:	drivers/iio/adc/mp2629_adc.c
 F:	drivers/mfd/mp2629.c
 F:	drivers/power/supply/mp2629_charger.c
-- 
2.34.1

Re: [PATCH v2 2/4] dt-bindings: hwmon/pmbus: Add mps,mpq7932 power-management IC

[Krzysztof Kozlowski]

On 01/12/2022 05:46, Saravanan Sekar wrote:
> Document mpq7932 power-management IC
> 
> Signed-off-by: Saravanan Sekar <saravanan@linumiz.com>
> ---

This is a friendly reminder during the review process.

It seems my previous comments were not fully addressed. Maybe my
feedback got lost between the quotes, maybe you just forgot to apply it.
Please go back to the previous discussion and either implement all
requested changes or keep discussing them.

Thank you.

Best regards,
Krzysztof

Re: [PATCH v2 3/4] hwmon: (pmbus/mpq7932) Add a support for mpq7932 Power Management IC

[Krzysztof Kozlowski]

On 01/12/2022 05:46, Saravanan Sekar wrote:
> The MPQ7932 is a power management IC designed to operate from 5V buses to
> power a variety of Advanced driver-assistance system SOCs. Six integrated
> buck converters with hardware monitoring capability powers a variety of
> target rails configurable over PMBus interface.
> 
> Signed-off-by: Saravanan Sekar <saravanan@linumiz.com>
> ---
>  drivers/hwmon/pmbus/Kconfig   |  10 +++
>  drivers/hwmon/pmbus/Makefile  |   1 +
>  drivers/hwmon/pmbus/mpq7932.c | 144 ++++++++++++++++++++++++++++++++++
>  3 files changed, 155 insertions(+)
>  create mode 100644 drivers/hwmon/pmbus/mpq7932.c

This is a friendly reminder during the review process.

It seems my previous comments were not fully addressed. Maybe my
feedback got lost between the quotes, maybe you just forgot to apply it.
Please go back to the previous discussion and either implement all
requested changes or keep discussing them.

Thank you.

Best regards,
Krzysztof

Re: [PATCH v2 2/4] dt-bindings: hwmon/pmbus: Add mps,mpq7932 power-management IC

[Saravanan Sekar]

On 01/12/22 11:26, Krzysztof Kozlowski wrote:
> On 01/12/2022 05:46, Saravanan Sekar wrote:
>> Document mpq7932 power-management IC
>>
>> Signed-off-by: Saravanan Sekar <saravanan@linumiz.com>
>> ---
> 
> This is a friendly reminder during the review process.
> 
> It seems my previous comments were not fully addressed. Maybe my
> feedback got lost between the quotes, maybe you just forgot to apply it.
> Please go back to the previous discussion and either implement all
> requested changes or keep discussing them.
>
Hi Krzysztof,

Thanks for your time to review and feedback.

Here are the summary of comments on V1, I have fixed all according to my 
understanding.


1. Use subject prefixes matching the subsystem (git log --oneline -- ...).

git log --oneline -- Documentation/devicetree/bindings/hwmon/pmbus/
1ccca53618c4 dt-bindings: hwmon/pmbus: Add mps,mpq7932 power-management IC
373c0a77934c dt-bindings: hwmon/pmbus: Add ti,lm25066 power-management IC
7f464532b05d dt-bindings: Add missing 'additionalProperties: false'
8a36e38d8b0f dt-bindings: hwmon/pmbus: Add ti,ucd90320 power sequencer

I have used the same format of 373c0a77934c.

2. Does not look like you tested the bindings. Please run `make
dt_binding_check` (see
Documentation/devicetree/bindings/writing-schema.rst for instructions).

I did run dt_binding_check on V1 but failed to notice warnings. Fixed 
warning on V2 and didn't observed any warnings.

3. Why requiring nodename? Device schemas usually don't do that.
dropped "pattern: "pmic@[0-9a-f]{1,2}""

4. regulators node is a regulator with one more regulator? Drop.
dropped "$ref: regulator.yaml# "

5. Messed indentation. Use same for entire example, e.g. 4-spaces.
Fixed it.


Please help if anything which I missed


> Thank you.
> 
> Best regards,
> Krzysztof
> 

Re: [PATCH v2 3/4] hwmon: (pmbus/mpq7932) Add a support for mpq7932 Power Management IC

[Saravanan Sekar]

On 01/12/22 11:26, Krzysztof Kozlowski wrote:
> On 01/12/2022 05:46, Saravanan Sekar wrote:
>> The MPQ7932 is a power management IC designed to operate from 5V buses to
>> power a variety of Advanced driver-assistance system SOCs. Six integrated
>> buck converters with hardware monitoring capability powers a variety of
>> target rails configurable over PMBus interface.
>>
>> Signed-off-by: Saravanan Sekar <saravanan@linumiz.com>
>> ---
>>   drivers/hwmon/pmbus/Kconfig   |  10 +++
>>   drivers/hwmon/pmbus/Makefile  |   1 +
>>   drivers/hwmon/pmbus/mpq7932.c | 144 ++++++++++++++++++++++++++++++++++
>>   3 files changed, 155 insertions(+)
>>   create mode 100644 drivers/hwmon/pmbus/mpq7932.c
> 
> This is a friendly reminder during the review process.
> 
> It seems my previous comments were not fully addressed. Maybe my
> feedback got lost between the quotes, maybe you just forgot to apply it.
> Please go back to the previous discussion and either implement all
> requested changes or keep discussing them.
> 
Thank you again for your time for review.

I saw two comments from you on V1 which I believe addressed on V2

1. Missing maybe_unused, so drop of_match_ptr.
  ".of_match_table = of_match_ptr(mpq7932_of_match)"

dropped of_match_ptr.

2. It's a regulator, not hwmon.
   "config SENSORS_MPQ7932_REGULATOR
    tristate "MPS MPQ7932 buck regulator" "

It is PMIC chip with hwmon support access over PMBUS.

Please help if anything I missed

> Thank you.
> 
> Best regards,
> Krzysztof
> 

Re: [PATCH v2 2/4] dt-bindings: hwmon/pmbus: Add mps,mpq7932 power-management IC

[Krzysztof Kozlowski]

On 01/12/2022 12:29, Saravanan Sekar wrote:
> On 01/12/22 11:26, Krzysztof Kozlowski wrote:
>> On 01/12/2022 05:46, Saravanan Sekar wrote:
>>> Document mpq7932 power-management IC
>>>
>>> Signed-off-by: Saravanan Sekar <saravanan@linumiz.com>
>>> ---
>>
>> This is a friendly reminder during the review process.
>>
>> It seems my previous comments were not fully addressed. Maybe my
>> feedback got lost between the quotes, maybe you just forgot to apply it.
>> Please go back to the previous discussion and either implement all
>> requested changes or keep discussing them.
>>
> Hi Krzysztof,
> 
> Thanks for your time to review and feedback.
> 
> Here are the summary of comments on V1, I have fixed all according to my 
> understanding.
> 
> 
> 1. Use subject prefixes matching the subsystem (git log --oneline -- ...).
> 
> git log --oneline -- Documentation/devicetree/bindings/hwmon/pmbus/
> 1ccca53618c4 dt-bindings: hwmon/pmbus: Add mps,mpq7932 power-management IC
> 373c0a77934c dt-bindings: hwmon/pmbus: Add ti,lm25066 power-management IC
> 7f464532b05d dt-bindings: Add missing 'additionalProperties: false'
> 8a36e38d8b0f dt-bindings: hwmon/pmbus: Add ti,ucd90320 power sequencer
> 
> I have used the same format of 373c0a77934c.
> 
> 2. Does not look like you tested the bindings. Please run `make
> dt_binding_check` (see
> Documentation/devicetree/bindings/writing-schema.rst for instructions).
> 
> I did run dt_binding_check on V1 but failed to notice warnings. Fixed 
> warning on V2 and didn't observed any warnings.
> 
> 3. Why requiring nodename? Device schemas usually don't do that.
> dropped "pattern: "pmic@[0-9a-f]{1,2}""
> 
> 4. regulators node is a regulator with one more regulator? Drop.
> dropped "$ref: regulator.yaml# "

The comment was - drop entire regulators node.

Plus additional comment for the driver (and related to bindings) was
that this is not hwmon but a regulator driver. Why putting regulator
driver in hwmon?

Best regards,
Krzysztof

Re: [PATCH v2 2/4] dt-bindings: hwmon/pmbus: Add mps,mpq7932 power-management IC

[Rob Herring]

On Thu, 01 Dec 2022 05:46:41 +0100, Saravanan Sekar wrote:
> Document mpq7932 power-management IC
> 
> Signed-off-by: Saravanan Sekar <saravanan@linumiz.com>
> ---
>  .../bindings/hwmon/pmbus/mps,mpq7932.yaml     | 63 +++++++++++++++++++
>  1 file changed, 63 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/hwmon/pmbus/mps,mpq7932.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/hwmon/pmbus/mps,mpq7932.example.dtb: pmic@3: regulators: Additional properties are not allowed ('buck1' was unexpected)
	From schema: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/hwmon/pmbus/mps,mpq7932.yaml

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20221201044643.1150870-3-saravanan@linumiz.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH v2 3/4] hwmon: (pmbus/mpq7932) Add a support for mpq7932 Power Management IC

[Guenter Roeck]

On 11/30/22 20:46, Saravanan Sekar wrote:
> The MPQ7932 is a power management IC designed to operate from 5V buses to
> power a variety of Advanced driver-assistance system SOCs. Six integrated
> buck converters with hardware monitoring capability powers a variety of
> target rails configurable over PMBus interface.
> 
> Signed-off-by: Saravanan Sekar <saravanan@linumiz.com>
> ---
>   drivers/hwmon/pmbus/Kconfig   |  10 +++
>   drivers/hwmon/pmbus/Makefile  |   1 +
>   drivers/hwmon/pmbus/mpq7932.c | 144 ++++++++++++++++++++++++++++++++++
>   3 files changed, 155 insertions(+)
>   create mode 100644 drivers/hwmon/pmbus/mpq7932.c
> 
> diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
> index 89668af67206..4a1538949a73 100644
> --- a/drivers/hwmon/pmbus/Kconfig
> +++ b/drivers/hwmon/pmbus/Kconfig
> @@ -317,6 +317,16 @@ config SENSORS_MP5023
>   	  This driver can also be built as a module. If so, the module will
>   	  be called mp5023.
>   
> +config SENSORS_MPQ7932
> +	tristate "MPS MPQ7932"

As written, a dependency on REGULATOR is missing. However, we want the driver
enabled even if CONFIG_REGULATOR is not enabled. I would suggest to follow the
approach used by other drivers: add a second configuration option
SENSORS_MPQ7932_REGULATOR which depends on SENSORS_MPQ7932 and REGULATOR
and enables regulator functionality, and use that in the driver to
make regulator support optional.

> +	help
> +	  If you say yes here you get six integrated buck converter regulator
> +	  with hardware monitoring functionality support for power management
> +	  IC MPS MPQ7932.

That description is more appropriate for the second configuration option.
Primarily one gets hardware monitoring support for the chip.

> +
> +	  This driver can also be built as a module. If so, the module will
> +	  be called mpq7932.
> +
>   config SENSORS_PIM4328
>   	tristate "Flex PIM4328 and compatibles"
>   	help
> diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile
> index 0002dbe22d52..28a534629cc3 100644
> --- a/drivers/hwmon/pmbus/Makefile
> +++ b/drivers/hwmon/pmbus/Makefile
> @@ -34,6 +34,7 @@ obj-$(CONFIG_SENSORS_MAX8688)	+= max8688.o
>   obj-$(CONFIG_SENSORS_MP2888)	+= mp2888.o
>   obj-$(CONFIG_SENSORS_MP2975)	+= mp2975.o
>   obj-$(CONFIG_SENSORS_MP5023)	+= mp5023.o
> +obj-$(CONFIG_SENSORS_MPQ7932_REGULATOR) += mpq7932.o
>   obj-$(CONFIG_SENSORS_PLI1209BC)	+= pli1209bc.o
>   obj-$(CONFIG_SENSORS_PM6764TR)	+= pm6764tr.o
>   obj-$(CONFIG_SENSORS_PXE1610)	+= pxe1610.o
> diff --git a/drivers/hwmon/pmbus/mpq7932.c b/drivers/hwmon/pmbus/mpq7932.c
> new file mode 100644
> index 000000000000..3747d7862afd
> --- /dev/null
> +++ b/drivers/hwmon/pmbus/mpq7932.c
> @@ -0,0 +1,144 @@
> +/*
> + * SPDX-License-Identifier: GPL-2.0+

The SPDX license must be in the first line and be a C++ style comment.
Please run checkpatch --strict and fix what it reports (including the
various continuation line misalignments and unnecessary empty lines).

> + *
> + * mpq7932.c  - regulator driver for mps mpq7932
> + * Copyright 2022 Monolithic Power Systems, Inc

This is a hwmon driver with optional regulator functionality.

> + *
> + * Author: Saravanan Sekar <saravanan@linumiz.com>
> + */
> +
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/pmbus.h>
> +#include "pmbus.h"
> +
> +#define MPQ7932_BUCK_UV_MIN		206250
> +#define MPQ7932_UV_STEP			6250
> +#define MPQ7932_N_VOLTAGES		0xFF
> +#define MPQ7932_NUM_PAGES		6
> +
> +#define MPQ7932_TON_DELAY		0x60
> +#define MPQ7932_VOUT_STARTUP_SLEW	0xA3
> +#define MPQ7932_VOUT_SHUTDOWN_SLEW	0xA5
> +#define MPQ7932_VOUT_SLEW_MASK		GENMASK(1, 0)
> +#define MPQ7932_TON_DELAY_MASK		GENMASK(4, 0)

Please include the appropriate include file defining GENMASK.

> +
> +struct mpq7932_data {
> +	struct pmbus_driver_info info;
> +	struct pmbus_platform_data pdata;
> +};
> +
> +static struct regulator_desc mpq7932_regulators_desc[] = {
> +	PMBUS_REGULATOR_STEP("buck", 0, MPQ7932_N_VOLTAGES,
> +				MPQ7932_UV_STEP, MPQ7932_BUCK_UV_MIN),
> +	PMBUS_REGULATOR_STEP("buck", 1, MPQ7932_N_VOLTAGES,
> +				MPQ7932_UV_STEP, MPQ7932_BUCK_UV_MIN),
> +	PMBUS_REGULATOR_STEP("buck", 2, MPQ7932_N_VOLTAGES,
> +				MPQ7932_UV_STEP, MPQ7932_BUCK_UV_MIN),
> +	PMBUS_REGULATOR_STEP("buck", 3, MPQ7932_N_VOLTAGES,
> +				MPQ7932_UV_STEP, MPQ7932_BUCK_UV_MIN),
> +	PMBUS_REGULATOR_STEP("buck", 4, MPQ7932_N_VOLTAGES,
> +				MPQ7932_UV_STEP, MPQ7932_BUCK_UV_MIN),
> +	PMBUS_REGULATOR_STEP("buck", 5, MPQ7932_N_VOLTAGES,
> +				MPQ7932_UV_STEP, MPQ7932_BUCK_UV_MIN),
> +};
> +
> +static int mpq7932_write_word_data(struct i2c_client *client, int page, int reg,
> +			       u16 word)
> +{
> +
> +	switch (reg) {
> +	case PMBUS_VOUT_COMMAND:

This needs a comment explaining why it is needed.

> +		return pmbus_write_byte_data(client, page, reg, (u8)word);

word should be clamped to [0, 255], not cut off.

> +
> +	default:
> +		return -ENODATA;
> +	}
> +}
> +
> +static int mpq7932_read_word_data(struct i2c_client *client, int page,
> +				  int phase, int reg)
> +{
> +
> +	switch (reg) {
> +	case PMBUS_MFR_VOUT_MIN:
> +		return 0;
> +
> +	case PMBUS_MFR_VOUT_MAX:
> +		return MPQ7932_N_VOLTAGES;

The above need comments. Also, MPQ7932_N_VOLTAGES is inappropriate. This is not the
number of voltages, it is the maximum voltage. Even if the values happen to be the
same, the content is different.

Also, with PMBUS_MFR_VOUT_MIN=0 and PMBUS_MFR_VOUT_MAX=0xff, the number of voltages
would actually be 256, not 255.

> +
> +	case PMBUS_READ_VOUT:
> +		return pmbus_read_byte_data(client, page, PMBUS_VOUT_COMMAND);
> +
Needs same comment as above.

> +	default:
> +		return -ENODATA;
> +	}
> +}
> +
> +static int mpq7932_probe(struct i2c_client *client)
> +{
> +	struct mpq7932_data *data;
> +	struct pmbus_driver_info *info;
> +	struct device *dev = &client->dev;
> +	int i;
> +
> +	if (!i2c_check_functionality(client->adapter,
> +				     I2C_FUNC_SMBUS_READ_WORD_DATA))

Unnecessary check. This code doesn't use it, and pmbus_do_probe()
does its own check.

> +		return -ENODEV;
> +
> +	data = devm_kzalloc(&client->dev, sizeof(struct mpq7932_data),

Use dev.

> +			    GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	info = &data->info;
> +	info->pages = MPQ7932_NUM_PAGES;
> +	info->num_regulators = ARRAY_SIZE(mpq7932_regulators_desc);
> +	info->reg_desc = mpq7932_regulators_desc;
> +	info->format[PSC_VOLTAGE_OUT] = direct;
> +	info->m[PSC_VOLTAGE_OUT] = 160;
> +	info->b[PSC_VOLTAGE_OUT] = -33;
> +	for (i = 0; i < info->pages; i++) {
> +		info->func[i] = PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT
> +				| PMBUS_HAVE_STATUS_TEMP;

I think I already asked: Is this really all telemetry supported by the chip ?
I keep asking because that would be highly unusual.

> +	}
> +
> +	info->read_word_data = mpq7932_read_word_data;
> +	info->write_word_data = mpq7932_write_word_data;
> +
> +	data->pdata.flags = PMBUS_NO_CAPABILITY;
> +	dev->platform_data = &data->pdata;
> +
> +	return pmbus_do_probe(client, info);
> +}
> +
> +static const struct of_device_id mpq7932_of_match[] = {
> +	{ .compatible = "mps,mpq7932"},
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, mpq7932_of_match);
> +
> +static const struct i2c_device_id mpq7932_id[] = {
> +	{ "mpq7932", },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(i2c, mpq7932_id);
> +
> +static struct i2c_driver mpq7932_regulator_driver = {
> +	.driver = {
> +		.name = "mpq7932",
> +		.of_match_table = mpq7932_of_match,
> +	},
> +	.probe_new = mpq7932_probe,
> +	.id_table = mpq7932_id,
> +};
> +module_i2c_driver(mpq7932_regulator_driver);
> +
> +MODULE_AUTHOR("Saravanan Sekar <saravanan@linumiz.com>");
> +MODULE_DESCRIPTION("MPQ7932 PMIC regulator driver");
> +MODULE_LICENSE("GPL");
> +MODULE_IMPORT_NS(PMBUS);

Re: [PATCH v2 2/4] dt-bindings: hwmon/pmbus: Add mps,mpq7932 power-management IC

[Guenter Roeck]

On 12/1/22 03:38, Krzysztof Kozlowski wrote:
> On 01/12/2022 12:29, Saravanan Sekar wrote:
>> On 01/12/22 11:26, Krzysztof Kozlowski wrote:
>>> On 01/12/2022 05:46, Saravanan Sekar wrote:
>>>> Document mpq7932 power-management IC
>>>>
>>>> Signed-off-by: Saravanan Sekar <saravanan@linumiz.com>
>>>> ---
>>>
>>> This is a friendly reminder during the review process.
>>>
>>> It seems my previous comments were not fully addressed. Maybe my
>>> feedback got lost between the quotes, maybe you just forgot to apply it.
>>> Please go back to the previous discussion and either implement all
>>> requested changes or keep discussing them.
>>>
>> Hi Krzysztof,
>>
>> Thanks for your time to review and feedback.
>>
>> Here are the summary of comments on V1, I have fixed all according to my
>> understanding.
>>
>>
>> 1. Use subject prefixes matching the subsystem (git log --oneline -- ...).
>>
>> git log --oneline -- Documentation/devicetree/bindings/hwmon/pmbus/
>> 1ccca53618c4 dt-bindings: hwmon/pmbus: Add mps,mpq7932 power-management IC
>> 373c0a77934c dt-bindings: hwmon/pmbus: Add ti,lm25066 power-management IC
>> 7f464532b05d dt-bindings: Add missing 'additionalProperties: false'
>> 8a36e38d8b0f dt-bindings: hwmon/pmbus: Add ti,ucd90320 power sequencer
>>
>> I have used the same format of 373c0a77934c.
>>
>> 2. Does not look like you tested the bindings. Please run `make
>> dt_binding_check` (see
>> Documentation/devicetree/bindings/writing-schema.rst for instructions).
>>
>> I did run dt_binding_check on V1 but failed to notice warnings. Fixed
>> warning on V2 and didn't observed any warnings.
>>
>> 3. Why requiring nodename? Device schemas usually don't do that.
>> dropped "pattern: "pmic@[0-9a-f]{1,2}""
>>
>> 4. regulators node is a regulator with one more regulator? Drop.
>> dropped "$ref: regulator.yaml# "
> 
> The comment was - drop entire regulators node.
> 
> Plus additional comment for the driver (and related to bindings) was
> that this is not hwmon but a regulator driver. Why putting regulator
> driver in hwmon?
> 

Turns out this is primarily a hardware monitoring driver, like the drivers
for all other PMBus chips. Regulator support is actually optional; the driver
works perfectly well with CONFIG_REGULATOR=n (except that it needs some
#ifdefs to address that situation).

Guenter

Re: [PATCH v2 2/4] dt-bindings: hwmon/pmbus: Add mps,mpq7932 power-management IC

[Saravanan Sekar]

On 01/12/22 16:37, Guenter Roeck wrote:
> On 12/1/22 03:38, Krzysztof Kozlowski wrote:
>> On 01/12/2022 12:29, Saravanan Sekar wrote:
>>> On 01/12/22 11:26, Krzysztof Kozlowski wrote:
>>>> On 01/12/2022 05:46, Saravanan Sekar wrote:
>>>>> Document mpq7932 power-management IC
>>>>>
>>>>> Signed-off-by: Saravanan Sekar <saravanan@linumiz.com>
>>>>> ---
>>>>
>>>> This is a friendly reminder during the review process.
>>>>
>>>> It seems my previous comments were not fully addressed. Maybe my
>>>> feedback got lost between the quotes, maybe you just forgot to apply 
>>>> it.
>>>> Please go back to the previous discussion and either implement all
>>>> requested changes or keep discussing them.
>>>>
>>> Hi Krzysztof,
>>>
>>> Thanks for your time to review and feedback.
>>>
>>> Here are the summary of comments on V1, I have fixed all according to my
>>> understanding.
>>>
>>>
>>> 1. Use subject prefixes matching the subsystem (git log --oneline -- 
>>> ...).
>>>
>>> git log --oneline -- Documentation/devicetree/bindings/hwmon/pmbus/
>>> 1ccca53618c4 dt-bindings: hwmon/pmbus: Add mps,mpq7932 
>>> power-management IC
>>> 373c0a77934c dt-bindings: hwmon/pmbus: Add ti,lm25066 
>>> power-management IC
>>> 7f464532b05d dt-bindings: Add missing 'additionalProperties: false'
>>> 8a36e38d8b0f dt-bindings: hwmon/pmbus: Add ti,ucd90320 power sequencer
>>>
>>> I have used the same format of 373c0a77934c.
>>>
>>> 2. Does not look like you tested the bindings. Please run `make
>>> dt_binding_check` (see
>>> Documentation/devicetree/bindings/writing-schema.rst for instructions).
>>>
>>> I did run dt_binding_check on V1 but failed to notice warnings. Fixed
>>> warning on V2 and didn't observed any warnings.
>>>
>>> 3. Why requiring nodename? Device schemas usually don't do that.
>>> dropped "pattern: "pmic@[0-9a-f]{1,2}""
>>>
>>> 4. regulators node is a regulator with one more regulator? Drop.
>>> dropped "$ref: regulator.yaml# "
>>
>> The comment was - drop entire regulators node.
>>


PMBUS_REGULATOR_STEP helper macro has
                 .regulators_node = of_match_ptr("regulators"),  \

regulator subsystem parse (regulator_of_get_init_node) based on 
regulators_node. I think it is common all the regulator/pmic dts has 
regulators node.

>> Plus additional comment for the driver (and related to bindings) was
>> that this is not hwmon but a regulator driver. Why putting regulator
>> driver in hwmon?
>>
>
> Turns out this is primarily a hardware monitoring driver, like the drivers
> for all other PMBus chips. Regulator support is actually optional; the 
> driver
> works perfectly well with CONFIG_REGULATOR=n (except that it needs some
> #ifdefs to address that situation).
> 

Here is my view, communication to MPQ7932 PMIC chip is based on pmbus 
specification.

Now the conflict is that we have pmbus directory under hwmon subsystem, 
if pmbus spec implementation would have been separate like i2c-smbus 
then we can implement chip driver in regulator subsystem which access pmbus.

pmbus_core does supports regulator framework PMBUS_REGUALTOR and I 
believe it is valid MPQ7932 driver implantation under pmbus directory.

Kindly suggest to remove pmbus dependency on hwmon and proceed further.

Thanks,
Saravanan

Re: [PATCH v2 2/4] dt-bindings: hwmon/pmbus: Add mps,mpq7932 power-management IC

[Krzysztof Kozlowski]

On 01/12/2022 16:37, Guenter Roeck wrote:
> On 12/1/22 03:38, Krzysztof Kozlowski wrote:
>> On 01/12/2022 12:29, Saravanan Sekar wrote:
>>> On 01/12/22 11:26, Krzysztof Kozlowski wrote:
>>>> On 01/12/2022 05:46, Saravanan Sekar wrote:
>>>>> Document mpq7932 power-management IC
>>>>>
>>>>> Signed-off-by: Saravanan Sekar <saravanan@linumiz.com>
>>>>> ---
>>>>
>>>> This is a friendly reminder during the review process.
>>>>
>>>> It seems my previous comments were not fully addressed. Maybe my
>>>> feedback got lost between the quotes, maybe you just forgot to apply it.
>>>> Please go back to the previous discussion and either implement all
>>>> requested changes or keep discussing them.
>>>>
>>> Hi Krzysztof,
>>>
>>> Thanks for your time to review and feedback.
>>>
>>> Here are the summary of comments on V1, I have fixed all according to my
>>> understanding.
>>>
>>>
>>> 1. Use subject prefixes matching the subsystem (git log --oneline -- ...).
>>>
>>> git log --oneline -- Documentation/devicetree/bindings/hwmon/pmbus/
>>> 1ccca53618c4 dt-bindings: hwmon/pmbus: Add mps,mpq7932 power-management IC
>>> 373c0a77934c dt-bindings: hwmon/pmbus: Add ti,lm25066 power-management IC
>>> 7f464532b05d dt-bindings: Add missing 'additionalProperties: false'
>>> 8a36e38d8b0f dt-bindings: hwmon/pmbus: Add ti,ucd90320 power sequencer
>>>
>>> I have used the same format of 373c0a77934c.
>>>
>>> 2. Does not look like you tested the bindings. Please run `make
>>> dt_binding_check` (see
>>> Documentation/devicetree/bindings/writing-schema.rst for instructions).
>>>
>>> I did run dt_binding_check on V1 but failed to notice warnings. Fixed
>>> warning on V2 and didn't observed any warnings.
>>>
>>> 3. Why requiring nodename? Device schemas usually don't do that.
>>> dropped "pattern: "pmic@[0-9a-f]{1,2}""
>>>
>>> 4. regulators node is a regulator with one more regulator? Drop.
>>> dropped "$ref: regulator.yaml# "
>>
>> The comment was - drop entire regulators node.
>>
>> Plus additional comment for the driver (and related to bindings) was
>> that this is not hwmon but a regulator driver. Why putting regulator
>> driver in hwmon?
>>
> 
> Turns out this is primarily a hardware monitoring driver, like the drivers
> for all other PMBus chips. Regulator support is actually optional; the driver
> works perfectly well with CONFIG_REGULATOR=n (except that it needs some
> #ifdefs to address that situation).

OK, this would explain location  of the driver. However the bindings are
saying:
"Monolithic Power System MPQ7932 PMIC"
and PMIC is not mainly a hwmon device, even if it has such capabilities.
It might be missing description and proper title... or might be misplaced.

Best regards,
Krzysztof

Re: [PATCH v2 2/4] dt-bindings: hwmon/pmbus: Add mps,mpq7932 power-management IC

[Krzysztof Kozlowski]

On 01/12/2022 05:46, Saravanan Sekar wrote:
> Document mpq7932 power-management IC
> 
> Signed-off-by: Saravanan Sekar <saravanan@linumiz.com>
> ---
>  .../bindings/hwmon/pmbus/mps,mpq7932.yaml     | 63 +++++++++++++++++++
>  1 file changed, 63 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/hwmon/pmbus/mps,mpq7932.yaml
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/pmbus/mps,mpq7932.yaml b/Documentation/devicetree/bindings/hwmon/pmbus/mps,mpq7932.yaml
> new file mode 100644
> index 000000000000..5f20c59eb7ff
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/pmbus/mps,mpq7932.yaml
> @@ -0,0 +1,63 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/hwmon/pmbus/mps,mpq7932.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Monolithic Power System MPQ7932 PMIC
> +
> +maintainers:
> +  - Saravanan Sekar <saravanan@linumiz.com>
> +
> +properties:
> +  compatible:
> +    enum:
> +      - mps,mpq7932
> +
> +  reg:
> +    maxItems: 1
> +
> +  regulators:
> +    type: object

The comment was - drop regulators node if you have just one regulator...
but it turns out you have more regulators? Then you need to follow
regulator bindings. Open few and take a look.

> +
> +    description: |
> +      list of regulators provided by this controller, must be named
> +      after their hardware counterparts BUCK[1-6]
> +

This misses patternProperties.

> +      "^buck[1-6]$":
> +        type: object
> +        $ref: regulator.yaml#
> +        unevaluatedProperties: false
> +
> +    additionalProperties: false
> +
> +required:
> +  - compatible
> +  - reg
> +  - regulators
> +
> +additionalProperties: false


Best regards,
Krzysztof

Re: [PATCH v2 2/4] dt-bindings: hwmon/pmbus: Add mps,mpq7932 power-management IC

[Saravanan Sekar]

On 01/12/22 17:15, Krzysztof Kozlowski wrote:
> On 01/12/2022 16:37, Guenter Roeck wrote:
>> On 12/1/22 03:38, Krzysztof Kozlowski wrote:
>>> On 01/12/2022 12:29, Saravanan Sekar wrote:
>>>> On 01/12/22 11:26, Krzysztof Kozlowski wrote:
>>>>> On 01/12/2022 05:46, Saravanan Sekar wrote:
>>>>>> Document mpq7932 power-management IC
>>>>>>
>>>>>> Signed-off-by: Saravanan Sekar <saravanan@linumiz.com>
>>>>>> ---
>>>>>
>>>>> This is a friendly reminder during the review process.
>>>>>
>>>>> It seems my previous comments were not fully addressed. Maybe my
>>>>> feedback got lost between the quotes, maybe you just forgot to apply it.
>>>>> Please go back to the previous discussion and either implement all
>>>>> requested changes or keep discussing them.
>>>>>
>>>> Hi Krzysztof,
>>>>
>>>> Thanks for your time to review and feedback.
>>>>
>>>> Here are the summary of comments on V1, I have fixed all according to my
>>>> understanding.
>>>>
>>>>
>>>> 1. Use subject prefixes matching the subsystem (git log --oneline -- ...).
>>>>
>>>> git log --oneline -- Documentation/devicetree/bindings/hwmon/pmbus/
>>>> 1ccca53618c4 dt-bindings: hwmon/pmbus: Add mps,mpq7932 power-management IC
>>>> 373c0a77934c dt-bindings: hwmon/pmbus: Add ti,lm25066 power-management IC
>>>> 7f464532b05d dt-bindings: Add missing 'additionalProperties: false'
>>>> 8a36e38d8b0f dt-bindings: hwmon/pmbus: Add ti,ucd90320 power sequencer
>>>>
>>>> I have used the same format of 373c0a77934c.
>>>>
>>>> 2. Does not look like you tested the bindings. Please run `make
>>>> dt_binding_check` (see
>>>> Documentation/devicetree/bindings/writing-schema.rst for instructions).
>>>>
>>>> I did run dt_binding_check on V1 but failed to notice warnings. Fixed
>>>> warning on V2 and didn't observed any warnings.
>>>>
>>>> 3. Why requiring nodename? Device schemas usually don't do that.
>>>> dropped "pattern: "pmic@[0-9a-f]{1,2}""
>>>>
>>>> 4. regulators node is a regulator with one more regulator? Drop.
>>>> dropped "$ref: regulator.yaml# "
>>>
>>> The comment was - drop entire regulators node.
>>>
>>> Plus additional comment for the driver (and related to bindings) was
>>> that this is not hwmon but a regulator driver. Why putting regulator
>>> driver in hwmon?
>>>
>>
>> Turns out this is primarily a hardware monitoring driver, like the drivers
>> for all other PMBus chips. Regulator support is actually optional; the driver
>> works perfectly well with CONFIG_REGULATOR=n (except that it needs some
>> #ifdefs to address that situation).
> 
> OK, this would explain location  of the driver. However the bindings are
> saying:
> "Monolithic Power System MPQ7932 PMIC"
> and PMIC is not mainly a hwmon device, even if it has such capabilities.
> It might be missing description and proper title... or might be misplaced.
> 

Indeed it is PMIC chip. I think this is not the first and not sure title 
has to be changed for hwmon subsystem.

bindings/hwmon/pmbus/ti,lm25066.yaml
title: National Semiconductor/Texas Instruments LM250x6/LM506x 
power-management ICs

Thanks,
Saravanan

Re: [PATCH v2 2/4] dt-bindings: hwmon/pmbus: Add mps,mpq7932 power-management IC

[Krzysztof Kozlowski]

On 01/12/2022 17:38, Saravanan Sekar wrote:
>>>> Plus additional comment for the driver (and related to bindings) was
>>>> that this is not hwmon but a regulator driver. Why putting regulator
>>>> driver in hwmon?
>>>>
>>>
>>> Turns out this is primarily a hardware monitoring driver, like the drivers
>>> for all other PMBus chips. Regulator support is actually optional; the driver
>>> works perfectly well with CONFIG_REGULATOR=n (except that it needs some
>>> #ifdefs to address that situation).
>>
>> OK, this would explain location  of the driver. However the bindings are
>> saying:
>> "Monolithic Power System MPQ7932 PMIC"
>> and PMIC is not mainly a hwmon device, even if it has such capabilities.
>> It might be missing description and proper title... or might be misplaced.
>>
> 
> Indeed it is PMIC chip. I think this is not the first and not sure title 
> has to be changed for hwmon subsystem.
> 
> bindings/hwmon/pmbus/ti,lm25066.yaml
> title: National Semiconductor/Texas Instruments LM250x6/LM506x 
> power-management ICs

Then I propose to put it in regulator directory.

Best regards,
Krzysztof

Re: [PATCH v2 2/4] dt-bindings: hwmon/pmbus: Add mps,mpq7932 power-management IC

[Saravanan Sekar]

On 01/12/22 17:43, Krzysztof Kozlowski wrote:
> On 01/12/2022 17:38, Saravanan Sekar wrote:
>>>>> Plus additional comment for the driver (and related to bindings) was
>>>>> that this is not hwmon but a regulator driver. Why putting regulator
>>>>> driver in hwmon?
>>>>>
>>>>
>>>> Turns out this is primarily a hardware monitoring driver, like the drivers
>>>> for all other PMBus chips. Regulator support is actually optional; the driver
>>>> works perfectly well with CONFIG_REGULATOR=n (except that it needs some
>>>> #ifdefs to address that situation).
>>>
>>> OK, this would explain location  of the driver. However the bindings are
>>> saying:
>>> "Monolithic Power System MPQ7932 PMIC"
>>> and PMIC is not mainly a hwmon device, even if it has such capabilities.
>>> It might be missing description and proper title... or might be misplaced.
>>>
>>
>> Indeed it is PMIC chip. I think this is not the first and not sure title
>> has to be changed for hwmon subsystem.
>>
>> bindings/hwmon/pmbus/ti,lm25066.yaml
>> title: National Semiconductor/Texas Instruments LM250x6/LM506x
>> power-management ICs
> 
> Then I propose to put it in regulator directory.
>

Just for clarification, should bindings put in regulator directory?

Thanks,
Saravanan

Re: [PATCH v2 2/4] dt-bindings: hwmon/pmbus: Add mps,mpq7932 power-management IC

[Krzysztof Kozlowski]

On 01/12/2022 17:52, Saravanan Sekar wrote:

>>>
>>> Indeed it is PMIC chip. I think this is not the first and not sure title
>>> has to be changed for hwmon subsystem.
>>>
>>> bindings/hwmon/pmbus/ti,lm25066.yaml
>>> title: National Semiconductor/Texas Instruments LM250x6/LM506x
>>> power-management ICs
>>
>> Then I propose to put it in regulator directory.
>>
> 
> Just for clarification, should bindings put in regulator directory?

Yes, only the bindings. Assuming of course that the PMIC is the main
role of this device and it's not a MFD (then usually we put them into mfd).

Best regards,
Krzysztof

Re: [PATCH v2 3/4] hwmon: (pmbus/mpq7932) Add a support for mpq7932 Power Management IC

[Saravanan Sekar]

On 01/12/22 16:34, Guenter Roeck wrote:
> On 11/30/22 20:46, Saravanan Sekar wrote:
>> The MPQ7932 is a power management IC designed to operate from 5V buses to
>> power a variety of Advanced driver-assistance system SOCs. Six integrated
>> buck converters with hardware monitoring capability powers a variety of
>> target rails configurable over PMBus interface.
>>
>> Signed-off-by: Saravanan Sekar <saravanan@linumiz.com>
>> ---
>>   drivers/hwmon/pmbus/Kconfig   |  10 +++
>>   drivers/hwmon/pmbus/Makefile  |   1 +
>>   drivers/hwmon/pmbus/mpq7932.c | 144 ++++++++++++++++++++++++++++++++++
>>   3 files changed, 155 insertions(+)
>>   create mode 100644 drivers/hwmon/pmbus/mpq7932.c
>>
>> diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
>> index 89668af67206..4a1538949a73 100644
>> --- a/drivers/hwmon/pmbus/Kconfig
>> +++ b/drivers/hwmon/pmbus/Kconfig
>> @@ -317,6 +317,16 @@ config SENSORS_MP5023
>>         This driver can also be built as a module. If so, the module will
>>         be called mp5023.
>> +config SENSORS_MPQ7932
>> +    tristate "MPS MPQ7932"
> 
> As written, a dependency on REGULATOR is missing. However, we want the 
> driver
> enabled even if CONFIG_REGULATOR is not enabled. I would suggest to 
> follow the
> approach used by other drivers: add a second configuration option
> SENSORS_MPQ7932_REGULATOR which depends on SENSORS_MPQ7932 and REGULATOR
> and enables regulator functionality, and use that in the driver to
> make regulator support optional.
> 

Hello Guenter,

I need clarification or confirmation from you before V3.

Here is my view, communication to MPQ7932 PMIC chip is based on pmbus 
specification.

Now the conflict is that we have pmbus directory under hwmon subsystem, 
if pmbus spec implementation would have been separate like i2c-smbus 
then we can implement chip driver in regulator subsystem which access pmbus.

pmbus_core does supports regulator framework PMBUS_REGUALTOR and I 
believe it is valid MPQ7932 driver implantation under pmbus directory.

Kindly suggest to remove pmbus dependency on hwmon and proceed further.


>> +    help
>> +      If you say yes here you get six integrated buck converter 
>> regulator
>> +      with hardware monitoring functionality support for power 
>> management
>> +      IC MPS MPQ7932.
> 
> That description is more appropriate for the second configuration option.
> Primarily one gets hardware monitoring support for the chip.
> 
>> +
>> +      This driver can also be built as a module. If so, the module will
>> +      be called mpq7932.
>> +
>>   config SENSORS_PIM4328
>>       tristate "Flex PIM4328 and compatibles"
>>       help
>> diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile
>> index 0002dbe22d52..28a534629cc3 100644
>> --- a/drivers/hwmon/pmbus/Makefile
>> +++ b/drivers/hwmon/pmbus/Makefile
>> @@ -34,6 +34,7 @@ obj-$(CONFIG_SENSORS_MAX8688)    += max8688.o
>>   obj-$(CONFIG_SENSORS_MP2888)    += mp2888.o
>>   obj-$(CONFIG_SENSORS_MP2975)    += mp2975.o
>>   obj-$(CONFIG_SENSORS_MP5023)    += mp5023.o
>> +obj-$(CONFIG_SENSORS_MPQ7932_REGULATOR) += mpq7932.o
>>   obj-$(CONFIG_SENSORS_PLI1209BC)    += pli1209bc.o
>>   obj-$(CONFIG_SENSORS_PM6764TR)    += pm6764tr.o
>>   obj-$(CONFIG_SENSORS_PXE1610)    += pxe1610.o
>> diff --git a/drivers/hwmon/pmbus/mpq7932.c 
>> b/drivers/hwmon/pmbus/mpq7932.c
>> new file mode 100644
>> index 000000000000..3747d7862afd
>> --- /dev/null
>> +++ b/drivers/hwmon/pmbus/mpq7932.c
>> @@ -0,0 +1,144 @@
>> +/*
>> + * SPDX-License-Identifier: GPL-2.0+
> 
> The SPDX license must be in the first line and be a C++ style comment.
> Please run checkpatch --strict and fix what it reports (including the
> various continuation line misalignments and unnecessary empty lines).
> 
>> + *
>> + * mpq7932.c  - regulator driver for mps mpq7932
>> + * Copyright 2022 Monolithic Power Systems, Inc
> 
> This is a hwmon driver with optional regulator functionality.
> 
>> + *
>> + * Author: Saravanan Sekar <saravanan@linumiz.com>
>> + */
>> +
>> +#include <linux/err.h>
>> +#include <linux/i2c.h>
>> +#include <linux/init.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/of_device.h>
>> +#include <linux/pmbus.h>
>> +#include "pmbus.h"
>> +
>> +#define MPQ7932_BUCK_UV_MIN        206250
>> +#define MPQ7932_UV_STEP            6250
>> +#define MPQ7932_N_VOLTAGES        0xFF
>> +#define MPQ7932_NUM_PAGES        6
>> +
>> +#define MPQ7932_TON_DELAY        0x60
>> +#define MPQ7932_VOUT_STARTUP_SLEW    0xA3
>> +#define MPQ7932_VOUT_SHUTDOWN_SLEW    0xA5
>> +#define MPQ7932_VOUT_SLEW_MASK        GENMASK(1, 0)
>> +#define MPQ7932_TON_DELAY_MASK        GENMASK(4, 0)
> 
> Please include the appropriate include file defining GENMASK.
> 
>> +
>> +struct mpq7932_data {
>> +    struct pmbus_driver_info info;
>> +    struct pmbus_platform_data pdata;
>> +};
>> +
>> +static struct regulator_desc mpq7932_regulators_desc[] = {
>> +    PMBUS_REGULATOR_STEP("buck", 0, MPQ7932_N_VOLTAGES,
>> +                MPQ7932_UV_STEP, MPQ7932_BUCK_UV_MIN),
>> +    PMBUS_REGULATOR_STEP("buck", 1, MPQ7932_N_VOLTAGES,
>> +                MPQ7932_UV_STEP, MPQ7932_BUCK_UV_MIN),
>> +    PMBUS_REGULATOR_STEP("buck", 2, MPQ7932_N_VOLTAGES,
>> +                MPQ7932_UV_STEP, MPQ7932_BUCK_UV_MIN),
>> +    PMBUS_REGULATOR_STEP("buck", 3, MPQ7932_N_VOLTAGES,
>> +                MPQ7932_UV_STEP, MPQ7932_BUCK_UV_MIN),
>> +    PMBUS_REGULATOR_STEP("buck", 4, MPQ7932_N_VOLTAGES,
>> +                MPQ7932_UV_STEP, MPQ7932_BUCK_UV_MIN),
>> +    PMBUS_REGULATOR_STEP("buck", 5, MPQ7932_N_VOLTAGES,
>> +                MPQ7932_UV_STEP, MPQ7932_BUCK_UV_MIN),
>> +};
>> +
>> +static int mpq7932_write_word_data(struct i2c_client *client, int 
>> page, int reg,
>> +                   u16 word)
>> +{
>> +
>> +    switch (reg) {
>> +    case PMBUS_VOUT_COMMAND:
> 
> This needs a comment explaining why it is needed.
> 
>> +        return pmbus_write_byte_data(client, page, reg, (u8)word);
> 
> word should be clamped to [0, 255], not cut off.
> 
>> +
>> +    default:
>> +        return -ENODATA;
>> +    }
>> +}
>> +
>> +static int mpq7932_read_word_data(struct i2c_client *client, int page,
>> +                  int phase, int reg)
>> +{
>> +
>> +    switch (reg) {
>> +    case PMBUS_MFR_VOUT_MIN:
>> +        return 0;
>> +
>> +    case PMBUS_MFR_VOUT_MAX:
>> +        return MPQ7932_N_VOLTAGES;
> 
> The above need comments. Also, MPQ7932_N_VOLTAGES is inappropriate. This 
> is not the
> number of voltages, it is the maximum voltage. Even if the values happen 
> to be the
> same, the content is different.
> 
> Also, with PMBUS_MFR_VOUT_MIN=0 and PMBUS_MFR_VOUT_MAX=0xff, the number 
> of voltages
> would actually be 256, not 255.
> 
>> +
>> +    case PMBUS_READ_VOUT:
>> +        return pmbus_read_byte_data(client, page, PMBUS_VOUT_COMMAND);
>> +
> Needs same comment as above.
> 
>> +    default:
>> +        return -ENODATA;
>> +    }
>> +}
>> +
>> +static int mpq7932_probe(struct i2c_client *client)
>> +{
>> +    struct mpq7932_data *data;
>> +    struct pmbus_driver_info *info;
>> +    struct device *dev = &client->dev;
>> +    int i;
>> +
>> +    if (!i2c_check_functionality(client->adapter,
>> +                     I2C_FUNC_SMBUS_READ_WORD_DATA))
> 
> Unnecessary check. This code doesn't use it, and pmbus_do_probe()
> does its own check.
> 
>> +        return -ENODEV;
>> +
>> +    data = devm_kzalloc(&client->dev, sizeof(struct mpq7932_data),
> 
> Use dev.
> 
>> +                GFP_KERNEL);
>> +    if (!data)
>> +        return -ENOMEM;
>> +
>> +    info = &data->info;
>> +    info->pages = MPQ7932_NUM_PAGES;
>> +    info->num_regulators = ARRAY_SIZE(mpq7932_regulators_desc);
>> +    info->reg_desc = mpq7932_regulators_desc;
>> +    info->format[PSC_VOLTAGE_OUT] = direct;
>> +    info->m[PSC_VOLTAGE_OUT] = 160;
>> +    info->b[PSC_VOLTAGE_OUT] = -33;
>> +    for (i = 0; i < info->pages; i++) {
>> +        info->func[i] = PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT
>> +                | PMBUS_HAVE_STATUS_TEMP;
> 
> I think I already asked: Is this really all telemetry supported by the 
> chip ?
> I keep asking because that would be highly unusual.
> 

Yes, only the above.

Thanks,
Saravanan

>> +    }
>> +
>> +    info->read_word_data = mpq7932_read_word_data;
>> +    info->write_word_data = mpq7932_write_word_data;
>> +
>> +    data->pdata.flags = PMBUS_NO_CAPABILITY;
>> +    dev->platform_data = &data->pdata;
>> +
>> +    return pmbus_do_probe(client, info);
>> +}
>> +
>> +static const struct of_device_id mpq7932_of_match[] = {
>> +    { .compatible = "mps,mpq7932"},
>> +    {},
>> +};
>> +MODULE_DEVICE_TABLE(of, mpq7932_of_match);
>> +
>> +static const struct i2c_device_id mpq7932_id[] = {
>> +    { "mpq7932", },
>> +    { },
>> +};
>> +MODULE_DEVICE_TABLE(i2c, mpq7932_id);
>> +
>> +static struct i2c_driver mpq7932_regulator_driver = {
>> +    .driver = {
>> +        .name = "mpq7932",
>> +        .of_match_table = mpq7932_of_match,
>> +    },
>> +    .probe_new = mpq7932_probe,
>> +    .id_table = mpq7932_id,
>> +};
>> +module_i2c_driver(mpq7932_regulator_driver);
>> +
>> +MODULE_AUTHOR("Saravanan Sekar <saravanan@linumiz.com>");
>> +MODULE_DESCRIPTION("MPQ7932 PMIC regulator driver");
>> +MODULE_LICENSE("GPL");
>> +MODULE_IMPORT_NS(PMBUS);
> 

Re: [PATCH v2 3/4] hwmon: (pmbus/mpq7932) Add a support for mpq7932 Power Management IC

[Guenter Roeck]

On 12/6/22 19:51, Saravanan Sekar wrote:
> On 01/12/22 16:34, Guenter Roeck wrote:
>> On 11/30/22 20:46, Saravanan Sekar wrote:
>>> The MPQ7932 is a power management IC designed to operate from 5V buses to
>>> power a variety of Advanced driver-assistance system SOCs. Six integrated
>>> buck converters with hardware monitoring capability powers a variety of
>>> target rails configurable over PMBus interface.
>>>
>>> Signed-off-by: Saravanan Sekar <saravanan@linumiz.com>
>>> ---
>>>   drivers/hwmon/pmbus/Kconfig   |  10 +++
>>>   drivers/hwmon/pmbus/Makefile  |   1 +
>>>   drivers/hwmon/pmbus/mpq7932.c | 144 ++++++++++++++++++++++++++++++++++
>>>   3 files changed, 155 insertions(+)
>>>   create mode 100644 drivers/hwmon/pmbus/mpq7932.c
>>>
>>> diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
>>> index 89668af67206..4a1538949a73 100644
>>> --- a/drivers/hwmon/pmbus/Kconfig
>>> +++ b/drivers/hwmon/pmbus/Kconfig
>>> @@ -317,6 +317,16 @@ config SENSORS_MP5023
>>>         This driver can also be built as a module. If so, the module will
>>>         be called mp5023.
>>> +config SENSORS_MPQ7932
>>> +    tristate "MPS MPQ7932"
>>
>> As written, a dependency on REGULATOR is missing. However, we want the driver
>> enabled even if CONFIG_REGULATOR is not enabled. I would suggest to follow the
>> approach used by other drivers: add a second configuration option
>> SENSORS_MPQ7932_REGULATOR which depends on SENSORS_MPQ7932 and REGULATOR
>> and enables regulator functionality, and use that in the driver to
>> make regulator support optional.
>>
> 
> Hello Guenter,
> 
> I need clarification or confirmation from you before V3.
> 
> Here is my view, communication to MPQ7932 PMIC chip is based on pmbus specification.
> 
> Now the conflict is that we have pmbus directory under hwmon subsystem, if pmbus spec implementation would have been separate like i2c-smbus then we can implement chip driver in regulator subsystem which access pmbus.
> 
> pmbus_core does supports regulator framework PMBUS_REGUALTOR and I believe it is valid MPQ7932 driver implantation under pmbus directory.
> 
> Kindly suggest to remove pmbus dependency on hwmon and proceed further.
> 

Every chip supporting PMBus has hwmon functionality. The PMBus core
as implemented primarily supports hardware monitoring. Some can
act as regulators; that is why regulator support was added to
the PMBus core. Trying to extract it would make no sense.

If you want to implement a driver without hardware monitoring
support for this chip, you won't need the PMBus core. I would not
agree with such an approach, but there is nothing that prevents you
from implementing a regulator-only driver for this chip in the
regulator subsystem.

Guenter