[PATCH v1 1/3] dt-bindings: hwmon/pmbus: Add mps,mpq7932 power-management IC

[PATCH v1 1/3] dt-bindings: hwmon/pmbus: Add mps,mpq7932 power-management IC

[Saravanan Sekar]

Add bindings for mps,mpq7932 power-management IC

Signed-off-by: Saravanan Sekar <saravanan@linumiz.com>
---
 .../bindings/hwmon/pmbus/mps,mpq7932.yaml     | 69 +++++++++++++++++++
 1 file changed, 69 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..6ec072c287a3
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/pmbus/mps,mpq7932.yaml
@@ -0,0 +1,69 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/regulator/mps,mpq7932.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Monolithic Power System MPQ7932 PMIC
+
+maintainers:
+  - Saravanan Sekar <saravanan@linumiz.com>
+
+properties:
+  $nodename:
+    pattern: "pmic@[0-9a-f]{1,2}"
+  compatible:
+    enum:
+      - mps,mpq7932
+
+  reg:
+    maxItems: 1
+
+  regulators:
+    type: object
+    $ref: regulator.yaml#
+
+    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@69 {
+          compatible = "mps,mpq7932";
+          reg = <0x69>;
+
+          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 v1 2/3] 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 ADAS SOCs. Six integrated buck converters power a
variety of target rails configurable over PMBus interface.

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

diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
index 89668af67206..5e938768bd77 100644
--- a/drivers/hwmon/pmbus/Kconfig
+++ b/drivers/hwmon/pmbus/Kconfig
@@ -317,6 +317,15 @@ config SENSORS_MP5023
 	  This driver can also be built as a module. If so, the module will
 	  be called mp5023.
 
+config SENSORS_MPQ7932_REGULATOR
+	tristate "MPS MPQ7932 buck regulator"
+	help
+	  If you say yes here you get six integrated buck converter
+	  regulator 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..23d3ccdaed1e
--- /dev/null
+++ b/drivers/hwmon/pmbus/mpq7932.c
@@ -0,0 +1,150 @@
+// 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/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/err.h>
+#include <linux/of_device.h>
+#include <linux/pmbus.h>
+#include <linux/i2c.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)
+
+#define MPQ7932BUCK(_id)					\
+	[_id] = {						\
+		.id = _id,					\
+		.name = ("buck" # _id),				\
+		.of_match = of_match_ptr("buck" # _id),		\
+		.regulators_node = "regulators",		\
+		.ops = &pmbus_regulator_ops,			\
+		.type = REGULATOR_VOLTAGE,			\
+		.min_uV = MPQ7932_BUCK_UV_MIN,			\
+		.uV_step = MPQ7932_UV_STEP,			\
+		.n_voltages = MPQ7932_N_VOLTAGES,		\
+	}
+
+struct mpq7932_data {
+	struct pmbus_driver_info info;
+	struct pmbus_platform_data pdata;
+};
+
+static struct regulator_desc mpq7932_regulators_desc[] = {
+	MPQ7932BUCK(0),
+	MPQ7932BUCK(1),
+	MPQ7932BUCK(2),
+	MPQ7932BUCK(3),
+	MPQ7932BUCK(4),
+	MPQ7932BUCK(5),
+};
+
+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 = of_match_ptr(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 v1 3/3] MAINTAINERS: Add entry for mpq7932 PMIC driver

[Saravanan Sekar]

Add MAINTAINERS entry for Monolithic Power Systems mpq7932 PMIC driver.

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

diff --git a/MAINTAINERS b/MAINTAINERS
index 379945f82a64..6727f1b55da8 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,mpq.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 v1 2/3] hwmon: (pmbus/mpq7932) Add a support for mpq7932 Power Management IC

[Guenter Roeck]

On 11/26/22 09:17, Saravanan Sekar wrote:
> The MPQ7932 is a power management IC designed to operate from 5V buses to
> power a variety of ADAS SOCs. Six integrated buck converters power a
> variety of target rails configurable over PMBus interface.
> 
> Signed-off-by: Saravanan Sekar <saravanan@linumiz.com>

I find no reference to this chip anywhere. Can you provide one ?

Also, please refrain from cryptic abbreviations. You may know what
ADAS means, but I have no idea.

> ---
>   drivers/hwmon/pmbus/Kconfig   |   9 ++
>   drivers/hwmon/pmbus/Makefile  |   1 +
>   drivers/hwmon/pmbus/mpq7932.c | 150 ++++++++++++++++++++++++++++++++++
>   3 files changed, 160 insertions(+)
>   create mode 100644 drivers/hwmon/pmbus/mpq7932.c
> 
> diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
> index 89668af67206..5e938768bd77 100644
> --- a/drivers/hwmon/pmbus/Kconfig
> +++ b/drivers/hwmon/pmbus/Kconfig
> @@ -317,6 +317,15 @@ config SENSORS_MP5023
>   	  This driver can also be built as a module. If so, the module will
>   	  be called mp5023.
>   
> +config SENSORS_MPQ7932_REGULATOR

Drop "_REGULATOR".

> +	tristate "MPS MPQ7932 buck regulator"
> +	help
> +	  If you say yes here you get six integrated buck converter
> +	  regulator support for power management IC MPS MPQ7932.
> +

This primarily adds hardware monitoring support. Referring to it
only as regulator is misleading. Please also refer to hardware
monitoring functionality. More on that below.

> +	  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..23d3ccdaed1e
> --- /dev/null
> +++ b/drivers/hwmon/pmbus/mpq7932.c
> @@ -0,0 +1,150 @@
> +// 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>
> +

No C++ comments in hwmon subsystem, please, unless mandated.

> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/err.h>
> +#include <linux/of_device.h>
> +#include <linux/pmbus.h>
> +#include <linux/i2c.h>

Alphabetic include file order, please.

> +#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)
> +
> +#define MPQ7932BUCK(_id)					\
> +	[_id] = {						\
> +		.id = _id,					\
> +		.name = ("buck" # _id),				\
> +		.of_match = of_match_ptr("buck" # _id),		\
> +		.regulators_node = "regulators",		\
> +		.ops = &pmbus_regulator_ops,			\
> +		.type = REGULATOR_VOLTAGE,			\
> +		.min_uV = MPQ7932_BUCK_UV_MIN,			\
> +		.uV_step = MPQ7932_UV_STEP,			\
> +		.n_voltages = MPQ7932_N_VOLTAGES,		\
> +	}

Why not use PMBUS_REGULATOR_STEP ?

> +
> +struct mpq7932_data {
> +	struct pmbus_driver_info info;
> +	struct pmbus_platform_data pdata;
> +};
> +
> +static struct regulator_desc mpq7932_regulators_desc[] = {
> +	MPQ7932BUCK(0),
> +	MPQ7932BUCK(1),
> +	MPQ7932BUCK(2),
> +	MPQ7932BUCK(3),
> +	MPQ7932BUCK(4),
> +	MPQ7932BUCK(5),
> +};
> +
> +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);
> +

This needs explanation. VOUT_COMMAND directly sets a voltage, and the provided
value is in ULINEAR16 format. Just using its lower 8 bits seems, at the very
least, odd.

> +	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;
> +

This is not how this is intended to work. If the chip does not provide
those properties, they should not be faked.

> +	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;
> +	}
> +

Is that really all the chip supports ? No input data, no current
or power measurements ?

Is this even a real PMBus chip, or a fake one which claims to support
PMBus but in reality doesn't ?

> +	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 = {

PMBus drivers are not primarily regulator drivers. They
are hardware monitoring drivers. It is difficult for me to
determine, due to lack of information about this chip,
what it actually supports. If the chip is really a regulator
masquerading as PMBus chip, you might want to consider writing
a regulator driver for it.

Guenter

> +	.driver = {
> +		.name = "mpq7932",
> +		.of_match_table = of_match_ptr(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 v1 1/3] dt-bindings: hwmon/pmbus: Add mps,mpq7932 power-management IC

[Krzysztof Kozlowski]

On 26/11/2022 18:17, Saravanan Sekar wrote:
> Add bindings for mps,mpq7932 power-management IC
> 
> Signed-off-by: Saravanan Sekar <saravanan@linumiz.com>
> ---
>  .../bindings/hwmon/pmbus/mps,mpq7932.yaml     | 69 +++++++++++++++++++

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

>  1 file changed, 69 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..6ec072c287a3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/pmbus/mps,mpq7932.yaml
> @@ -0,0 +1,69 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/regulator/mps,mpq7932.yaml#

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

> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Monolithic Power System MPQ7932 PMIC
> +
> +maintainers:
> +  - Saravanan Sekar <saravanan@linumiz.com>
> +
> +properties:
> +  $nodename:
> +    pattern: "pmic@[0-9a-f]{1,2}"

Why requiring nodename? Device schemas usually don't do that.

> +  compatible:
> +    enum:
> +      - mps,mpq7932
> +
> +  reg:
> +    maxItems: 1
> +
> +  regulators:
> +    type: object
> +    $ref: regulator.yaml#

regulators node is a regulator with one more regulator? Drop.

> +
> +    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#
> +

Drop blank line.

> +        unevaluatedProperties: false
> +
> +    additionalProperties: false
> +
> +required:
> +  - compatible
> +  - reg
> +  - regulators
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        pmic@69 {
> +          compatible = "mps,mpq7932";

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

> +          reg = <0x69>;
> +
> +          regulators {
> +

Drop blank line.

> +            buck1 {
> +             regulator-name = "buck1";
> +             regulator-min-microvolt = <400000>;
> +             regulator-max-microvolt = <3587500>;
> +             regulator-min-microamp  = <460000>;
> +             regulator-max-microamp  = <7600000>;
> +             regulator-boot-on;
> +            };
> +

Why blank line here? Drop it.

> +         };
> +       };
> +     };
> +...

Best regards,
Krzysztof

Re: [PATCH v1 1/3] dt-bindings: hwmon/pmbus: Add mps,mpq7932 power-management IC

[Krzysztof Kozlowski]

On 26/11/2022 18:17, Saravanan Sekar wrote:
> Add bindings for mps,mpq7932 power-management IC
> 
> Signed-off-by: Saravanan Sekar <saravanan@linumiz.com>
> ---

Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC.  It might happen, that command when run on an older
kernel, gives you outdated entries.  Therefore please be sure you base
your patches on recent Linux kernel.

Skipping lists to cc skips the tests. That's a no. Especially that you
did not test it yourself...

Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

On 26/11/2022 18:17, Saravanan Sekar wrote:
> The MPQ7932 is a power management IC designed to operate from 5V buses to
> power a variety of ADAS SOCs. Six integrated buck converters power a
> variety of target rails configurable over PMBus interface.
> 
> Signed-off-by: Saravanan Sekar <saravanan@linumiz.com>
> ---
>  drivers/hwmon/pmbus/Kconfig   |   9 ++
>  drivers/hwmon/pmbus/Makefile  |   1 +
>  drivers/hwmon/pmbus/mpq7932.c | 150 ++++++++++++++++++++++++++++++++++
>  3 files changed, 160 insertions(+)
>  create mode 100644 drivers/hwmon/pmbus/mpq7932.c
> 
> diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
> index 89668af67206..5e938768bd77 100644
> --- a/drivers/hwmon/pmbus/Kconfig
> +++ b/drivers/hwmon/pmbus/Kconfig
> @@ -317,6 +317,15 @@ config SENSORS_MP5023
>  	  This driver can also be built as a module. If so, the module will
>  	  be called mp5023.
>  
> +config SENSORS_MPQ7932_REGULATOR
> +	tristate "MPS MPQ7932 buck regulator"

It's a regulator, not hwmon.

> +	help
> +	  If you say yes here you get six integrated buck converter
> +	  regulator 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..23d3ccdaed1e
> --- /dev/null
> +++ b/drivers/hwmon/pmbus/mpq7932.c
> @@ -0,0 +1,150 @@
> +// 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/kernel.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/err.h>
> +#include <linux/of_device.h>
> +#include <linux/pmbus.h>
> +#include <linux/i2c.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)
> +
> +#define MPQ7932BUCK(_id)					\
> +	[_id] = {						\
> +		.id = _id,					\
> +		.name = ("buck" # _id),				\
> +		.of_match = of_match_ptr("buck" # _id),		\
> +		.regulators_node = "regulators",		\
> +		.ops = &pmbus_regulator_ops,			\
> +		.type = REGULATOR_VOLTAGE,			\
> +		.min_uV = MPQ7932_BUCK_UV_MIN,			\
> +		.uV_step = MPQ7932_UV_STEP,			\
> +		.n_voltages = MPQ7932_N_VOLTAGES,		\
> +	}
> +
> +struct mpq7932_data {
> +	struct pmbus_driver_info info;
> +	struct pmbus_platform_data pdata;
> +};
> +
> +static struct regulator_desc mpq7932_regulators_desc[] = {
> +	MPQ7932BUCK(0),
> +	MPQ7932BUCK(1),
> +	MPQ7932BUCK(2),
> +	MPQ7932BUCK(3),
> +	MPQ7932BUCK(4),
> +	MPQ7932BUCK(5),
> +};
> +
> +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 = of_match_ptr(mpq7932_of_match),

Missing maybe_unused, so drop of_match_ptr.

Best regards,
Krzysztof

Re: [PATCH v1 3/3] MAINTAINERS: Add entry for mpq7932 PMIC driver

[Krzysztof Kozlowski]

On 26/11/2022 18:17, Saravanan Sekar wrote:
> Add MAINTAINERS entry for Monolithic Power Systems mpq7932 PMIC driver.
> 

The entry is already there. Your commit msg and subject do not match at
all what you are doing.


Best regards,
Krzysztof

Re: [PATCH v1 1/3] dt-bindings: hwmon/pmbus: Add mps,mpq7932 power-management IC

[Rob Herring]

On Sat, Nov 26, 2022 at 11:17 AM Saravanan Sekar <saravanan@linumiz.com> wrote:
>
> Add bindings for mps,mpq7932 power-management IC
>
> Signed-off-by: Saravanan Sekar <saravanan@linumiz.com>
> ---
>  .../bindings/hwmon/pmbus/mps,mpq7932.yaml     | 69 +++++++++++++++++++
>  1 file changed, 69 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/hwmon/pmbus/mps,mpq7932.yaml

Use get_maintainers.pl and send patches to the right lists. DT list is
missing so no checks run and it's not in the review queue (Patchwork).

Rob

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

[Saravanan Sekar]

On 26/11/22 18:50, Guenter Roeck wrote:
> On 11/26/22 09:17, Saravanan Sekar wrote:
>> The MPQ7932 is a power management IC designed to operate from 5V buses to
>> power a variety of ADAS SOCs. Six integrated buck converters power a
>> variety of target rails configurable over PMBus interface.
>>
>> Signed-off-by: Saravanan Sekar <saravanan@linumiz.com>
> 
Hello Guenter,

Thanks for your time to review.

> I find no reference to this chip anywhere. Can you provide one ?
> 

I have Preliminary specification under NDA, checking with manufactures 
upload in public accessibility.

> Also, please refrain from cryptic abbreviations. You may know what
> ADAS means, but I have no idea.
> 
Sure
>> ---
>>   drivers/hwmon/pmbus/Kconfig   |   9 ++
>>   drivers/hwmon/pmbus/Makefile  |   1 +
>>   drivers/hwmon/pmbus/mpq7932.c | 150 ++++++++++++++++++++++++++++++++++
>>   3 files changed, 160 insertions(+)
>>   create mode 100644 drivers/hwmon/pmbus/mpq7932.c
>>
>> diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
>> index 89668af67206..5e938768bd77 100644
>> --- a/drivers/hwmon/pmbus/Kconfig
>> +++ b/drivers/hwmon/pmbus/Kconfig
>> @@ -317,6 +317,15 @@ config SENSORS_MP5023
>>         This driver can also be built as a module. If so, the module will
>>         be called mp5023.
>> +config SENSORS_MPQ7932_REGULATOR
> 
> Drop "_REGULATOR".
> 
>> +    tristate "MPS MPQ7932 buck regulator"
>> +    help
>> +      If you say yes here you get six integrated buck converter
>> +      regulator support for power management IC MPS MPQ7932.
>> +
> 
> This primarily adds hardware monitoring support. Referring to it
> only as regulator is misleading. Please also refer to hardware
> monitoring functionality. More on that below.
> 

okay

>> +      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..23d3ccdaed1e
>> --- /dev/null
>> +++ b/drivers/hwmon/pmbus/mpq7932.c
>> @@ -0,0 +1,150 @@
>> +// 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>
>> +
> 
> No C++ comments in hwmon subsystem, please, unless mandated.

okay
> 
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/init.h>
>> +#include <linux/err.h>
>> +#include <linux/of_device.h>
>> +#include <linux/pmbus.h>
>> +#include <linux/i2c.h>
> 
> Alphabetic include file order, please.
> 

okay
>> +#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)
>> +
>> +#define MPQ7932BUCK(_id)                    \
>> +    [_id] = {                        \
>> +        .id = _id,                    \
>> +        .name = ("buck" # _id),                \
>> +        .of_match = of_match_ptr("buck" # _id),        \
>> +        .regulators_node = "regulators",        \
>> +        .ops = &pmbus_regulator_ops,            \
>> +        .type = REGULATOR_VOLTAGE,            \
>> +        .min_uV = MPQ7932_BUCK_UV_MIN,            \
>> +        .uV_step = MPQ7932_UV_STEP,            \
>> +        .n_voltages = MPQ7932_N_VOLTAGES,        \
>> +    }
> 
> Why not use PMBUS_REGULATOR_STEP ?

MPQ7932 chip supports ramp delay and softstart which is not covered
by PMBUS_REGULATOR_STEP, I have incremental patch to address which
requires API's in pmbus_regulator_ops to be exported so chip driver
can extend/modifiy with reuse of pmbus_regulator api. Plan is to submit 
1st set of minimal driver and create RFC.

I can replace PMBUS_REGULATOR_STEP for this series.

> >> +
>> +struct mpq7932_data {
>> +    struct pmbus_driver_info info;
>> +    struct pmbus_platform_data pdata;
>> +};
>> +
>> +static struct regulator_desc mpq7932_regulators_desc[] = {
>> +    MPQ7932BUCK(0),
>> +    MPQ7932BUCK(1),
>> +    MPQ7932BUCK(2),
>> +    MPQ7932BUCK(3),
>> +    MPQ7932BUCK(4),
>> +    MPQ7932BUCK(5),
>> +};
>> +
>> +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);
>> +
> 
> This needs explanation. VOUT_COMMAND directly sets a voltage, and the 
> provided
> value is in ULINEAR16 format. Just using its lower 8 bits seems, at the 
> very
> least, odd.
> 

Indeed, unfortunately word_write results -EREMOTEIO and got clarified 
with manufacture chip support only byte write except STATUS_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;
>> +
> 
> This is not how this is intended to work. If the chip does not provide
> those properties, they should not be faked.
> 

Unfortunately, chip support neither

       PMBUS_VOUT_MARGIN_HIGH          = 0x25,
       PMBUS_VOUT_MARGIN_LOW           = 0x26,

nor

       PMBUS_MFR_VOUT_MIN              = 0xA4,
       PMBUS_MFR_VOUT_MAX              = 0xA5


as a result set voltage fails due to error in 
pmbus_regulator_get_low_margin. I can understand these absolute 
workaround, please suggest alternatives

>> +    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;
>> +    }
>> +
> 
> Is that really all the chip supports ? No input data, no current
> or power measurements ?
> 
> Is this even a real PMBus chip, or a fake one which claims to support
> PMBus but in reality doesn't ?

I would say this is PMIC buck regulator chip over PMBUS, I guess the 
regulator support in pmbus shall be used without hardware monitor 
support (though chip support temperature, error status, cml, status)


> 
>> +    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 = {
> 
> PMBus drivers are not primarily regulator drivers. They
> are hardware monitoring drivers. It is difficult for me to
> determine, due to lack of information about this chip,
> what it actually supports. If the chip is really a regulator
> masquerading as PMBus chip, you might want to consider writing
> a regulator driver for it.
> 

I try to share datasheet once I got permission from manufacturer.
Sure, I consider to write regulator driver over pmbus (addnl helper 
support on regulator subsystem like regmap) if we conclude should not be 
part of hwmon.

> Guenter
> 
>> +    .driver = {
>> +        .name = "mpq7932",
>> +        .of_match_table = of_match_ptr(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);
> 
Thanks,
Saravanan

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

[Guenter Roeck]

On 11/27/22 22:35, Saravanan Sekar wrote:
> On 26/11/22 18:50, Guenter Roeck wrote:
>> On 11/26/22 09:17, Saravanan Sekar wrote:
>>> The MPQ7932 is a power management IC designed to operate from 5V buses to
>>> power a variety of ADAS SOCs. Six integrated buck converters power a
>>> variety of target rails configurable over PMBus interface.
>>>
>>> Signed-off-by: Saravanan Sekar <saravanan@linumiz.com>
>>
> Hello Guenter,
> 
> Thanks for your time to review.
> 
>> I find no reference to this chip anywhere. Can you provide one ?
>>
> 
> I have Preliminary specification under NDA, checking with manufactures upload in public accessibility.
> 
>> Also, please refrain from cryptic abbreviations. You may know what
>> ADAS means, but I have no idea.
>>
> Sure
>>> ---
>>>   drivers/hwmon/pmbus/Kconfig   |   9 ++
>>>   drivers/hwmon/pmbus/Makefile  |   1 +
>>>   drivers/hwmon/pmbus/mpq7932.c | 150 ++++++++++++++++++++++++++++++++++
>>>   3 files changed, 160 insertions(+)
>>>   create mode 100644 drivers/hwmon/pmbus/mpq7932.c
>>>
>>> diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
>>> index 89668af67206..5e938768bd77 100644
>>> --- a/drivers/hwmon/pmbus/Kconfig
>>> +++ b/drivers/hwmon/pmbus/Kconfig
>>> @@ -317,6 +317,15 @@ config SENSORS_MP5023
>>>         This driver can also be built as a module. If so, the module will
>>>         be called mp5023.
>>> +config SENSORS_MPQ7932_REGULATOR
>>
>> Drop "_REGULATOR".
>>
>>> +    tristate "MPS MPQ7932 buck regulator"
>>> +    help
>>> +      If you say yes here you get six integrated buck converter
>>> +      regulator support for power management IC MPS MPQ7932.
>>> +
>>
>> This primarily adds hardware monitoring support. Referring to it
>> only as regulator is misleading. Please also refer to hardware
>> monitoring functionality. More on that below.
>>
> 
> okay
> 
>>> +      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..23d3ccdaed1e
>>> --- /dev/null
>>> +++ b/drivers/hwmon/pmbus/mpq7932.c
>>> @@ -0,0 +1,150 @@
>>> +// 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>
>>> +
>>
>> No C++ comments in hwmon subsystem, please, unless mandated.
> 
> okay
>>
>>> +#include <linux/kernel.h>
>>> +#include <linux/module.h>
>>> +#include <linux/init.h>
>>> +#include <linux/err.h>
>>> +#include <linux/of_device.h>
>>> +#include <linux/pmbus.h>
>>> +#include <linux/i2c.h>
>>
>> Alphabetic include file order, please.
>>
> 
> okay
>>> +#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)
>>> +
>>> +#define MPQ7932BUCK(_id)                    \
>>> +    [_id] = {                        \
>>> +        .id = _id,                    \
>>> +        .name = ("buck" # _id),                \
>>> +        .of_match = of_match_ptr("buck" # _id),        \
>>> +        .regulators_node = "regulators",        \
>>> +        .ops = &pmbus_regulator_ops,            \
>>> +        .type = REGULATOR_VOLTAGE,            \
>>> +        .min_uV = MPQ7932_BUCK_UV_MIN,            \
>>> +        .uV_step = MPQ7932_UV_STEP,            \
>>> +        .n_voltages = MPQ7932_N_VOLTAGES,        \
>>> +    }
>>
>> Why not use PMBUS_REGULATOR_STEP ?
> 
> MPQ7932 chip supports ramp delay and softstart which is not covered
> by PMBUS_REGULATOR_STEP, I have incremental patch to address which
> requires API's in pmbus_regulator_ops to be exported so chip driver
> can extend/modifiy with reuse of pmbus_regulator api. Plan is to submit 1st set of minimal driver and create RFC.
> 
> I can replace PMBUS_REGULATOR_STEP for this series.
> 

The only difference is that the above also sets min_uV.
I'd rather have that added to PMBUS_REGULATOR_STEP instead
of adding more macros.

As for min_uV, is that really needed ? None of the other regulators
seemed to need it.

>> >> +
>>> +struct mpq7932_data {
>>> +    struct pmbus_driver_info info;
>>> +    struct pmbus_platform_data pdata;
>>> +};
>>> +
>>> +static struct regulator_desc mpq7932_regulators_desc[] = {
>>> +    MPQ7932BUCK(0),
>>> +    MPQ7932BUCK(1),
>>> +    MPQ7932BUCK(2),
>>> +    MPQ7932BUCK(3),
>>> +    MPQ7932BUCK(4),
>>> +    MPQ7932BUCK(5),
>>> +};
>>> +
>>> +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);
>>> +
>>
>> This needs explanation. VOUT_COMMAND directly sets a voltage, and the provided
>> value is in ULINEAR16 format. Just using its lower 8 bits seems, at the very
>> least, odd.
>>
> 
> Indeed, unfortunately word_write results -EREMOTEIO and got clarified with manufacture chip support only byte write except STATUS_WORD :(
> 
Question then is what this byte contains. What if an attempt is made
to write a value > 0xff ? Writing 0x00 instead of 0x100 can only result
in severe trouble.

>>> +    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;
>>> +
>>
>> This is not how this is intended to work. If the chip does not provide
>> those properties, they should not be faked.
>>
> 
> Unfortunately, chip support neither
> 
>        PMBUS_VOUT_MARGIN_HIGH          = 0x25,
>        PMBUS_VOUT_MARGIN_LOW           = 0x26,
> 
> nor
> 
>        PMBUS_MFR_VOUT_MIN              = 0xA4,
>        PMBUS_MFR_VOUT_MAX              = 0xA5
> 
> 
> as a result set voltage fails due to error in pmbus_regulator_get_low_margin. I can understand these absolute workaround, please suggest alternatives
> 

Good point. I'd accept that with explanation, but PMBUS_MFR_VOUT_MAX
is a maximum voltage, not the number of voltages. And is 0 for
the minimum voltage really correct ? It seems to contradict
MPQ7932_BUCK_UV_MIN.

>>> +    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;
>>> +    }
>>> +
>>
>> Is that really all the chip supports ? No input data, no current
>> or power measurements ?
>>
>> Is this even a real PMBus chip, or a fake one which claims to support
>> PMBus but in reality doesn't ?
> 
> I would say this is PMIC buck regulator chip over PMBUS, I guess the regulator support in pmbus shall be used without hardware monitor support (though chip support temperature, error status, cml, status)
> 
> 
>>
>>> +    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 = {
>>
>> PMBus drivers are not primarily regulator drivers. They
>> are hardware monitoring drivers. It is difficult for me to
>> determine, due to lack of information about this chip,
>> what it actually supports. If the chip is really a regulator
>> masquerading as PMBus chip, you might want to consider writing
>> a regulator driver for it.
>>
> 
> I try to share datasheet once I got permission from manufacturer.
> Sure, I consider to write regulator driver over pmbus (addnl helper support on regulator subsystem like regmap) if we conclude should not be part of hwmon.
> 

Depends on you. Do you want a hwmon driver with regulator support,
or a regulator driver ? We can't have a regulator driver in
the hwmon subsystem.

Guenter

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

[Saravanan Sekar]

On 28/11/22 09:15, Guenter Roeck wrote:
> On 11/27/22 22:35, Saravanan Sekar wrote:
>> On 26/11/22 18:50, Guenter Roeck wrote:
>>> On 11/26/22 09:17, Saravanan Sekar wrote:
>>>> The MPQ7932 is a power management IC designed to operate from 5V 
>>>> buses to
>>>> power a variety of ADAS SOCs. Six integrated buck converters power a
>>>> variety of target rails configurable over PMBus interface.
>>>>
>>>> Signed-off-by: Saravanan Sekar <saravanan@linumiz.com>
>>>
>> Hello Guenter,
>>
>> Thanks for your time to review.
>>
>>> I find no reference to this chip anywhere. Can you provide one ?
>>>
>>
>> I have Preliminary specification under NDA, checking with manufactures 
>> upload in public accessibility.
>>
>>> Also, please refrain from cryptic abbreviations. You may know what
>>> ADAS means, but I have no idea.
>>>
>> Sure
>>>> ---
>>>>   drivers/hwmon/pmbus/Kconfig   |   9 ++
>>>>   drivers/hwmon/pmbus/Makefile  |   1 +
>>>>   drivers/hwmon/pmbus/mpq7932.c | 150 
>>>> ++++++++++++++++++++++++++++++++++
>>>>   3 files changed, 160 insertions(+)
>>>>   create mode 100644 drivers/hwmon/pmbus/mpq7932.c
>>>>
>>>> diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
>>>> index 89668af67206..5e938768bd77 100644
>>>> --- a/drivers/hwmon/pmbus/Kconfig
>>>> +++ b/drivers/hwmon/pmbus/Kconfig
>>>> @@ -317,6 +317,15 @@ config SENSORS_MP5023
>>>>         This driver can also be built as a module. If so, the module 
>>>> will
>>>>         be called mp5023.
>>>> +config SENSORS_MPQ7932_REGULATOR
>>>
>>> Drop "_REGULATOR".
>>>
>>>> +    tristate "MPS MPQ7932 buck regulator"
>>>> +    help
>>>> +      If you say yes here you get six integrated buck converter
>>>> +      regulator support for power management IC MPS MPQ7932.
>>>> +
>>>
>>> This primarily adds hardware monitoring support. Referring to it
>>> only as regulator is misleading. Please also refer to hardware
>>> monitoring functionality. More on that below.
>>>
>>
>> okay
>>
>>>> +      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..23d3ccdaed1e
>>>> --- /dev/null
>>>> +++ b/drivers/hwmon/pmbus/mpq7932.c
>>>> @@ -0,0 +1,150 @@
>>>> +// 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>
>>>> +
>>>
>>> No C++ comments in hwmon subsystem, please, unless mandated.
>>
>> okay
>>>
>>>> +#include <linux/kernel.h>
>>>> +#include <linux/module.h>
>>>> +#include <linux/init.h>
>>>> +#include <linux/err.h>
>>>> +#include <linux/of_device.h>
>>>> +#include <linux/pmbus.h>
>>>> +#include <linux/i2c.h>
>>>
>>> Alphabetic include file order, please.
>>>
>>
>> okay
>>>> +#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)
>>>> +
>>>> +#define MPQ7932BUCK(_id)                    \
>>>> +    [_id] = {                        \
>>>> +        .id = _id,                    \
>>>> +        .name = ("buck" # _id),                \
>>>> +        .of_match = of_match_ptr("buck" # _id),        \
>>>> +        .regulators_node = "regulators",        \
>>>> +        .ops = &pmbus_regulator_ops,            \
>>>> +        .type = REGULATOR_VOLTAGE,            \
>>>> +        .min_uV = MPQ7932_BUCK_UV_MIN,            \
>>>> +        .uV_step = MPQ7932_UV_STEP,            \
>>>> +        .n_voltages = MPQ7932_N_VOLTAGES,        \
>>>> +    }
>>>
>>> Why not use PMBUS_REGULATOR_STEP ?
>>
>> MPQ7932 chip supports ramp delay and softstart which is not covered
>> by PMBUS_REGULATOR_STEP, I have incremental patch to address which
>> requires API's in pmbus_regulator_ops to be exported so chip driver
>> can extend/modifiy with reuse of pmbus_regulator api. Plan is to 
>> submit 1st set of minimal driver and create RFC.
>>
>> I can replace PMBUS_REGULATOR_STEP for this series.
>>
> 
> The only difference is that the above also sets min_uV.
> I'd rather have that added to PMBUS_REGULATOR_STEP instead
> of adding more macros.
> 
> As for min_uV, is that really needed ? None of the other regulators
> seemed to need it.
> 

Indeed min_uV is needed.
output voltage is in the range from 0.20625V to 1.8V. Calculated based 
as VOUT_CMD [7:0]*6.25mV+206.25mV

min voltage is 206.25mV if register is 0.

It is common to min_uV in regulator driver, e.g mpq7920, mp5416

I add one more patch to this series for min_uV addition in 
PMBUS_REGULATOR_STEP.


>>> >> +
>>>> +struct mpq7932_data {
>>>> +    struct pmbus_driver_info info;
>>>> +    struct pmbus_platform_data pdata;
>>>> +};
>>>> +
>>>> +static struct regulator_desc mpq7932_regulators_desc[] = {
>>>> +    MPQ7932BUCK(0),
>>>> +    MPQ7932BUCK(1),
>>>> +    MPQ7932BUCK(2),
>>>> +    MPQ7932BUCK(3),
>>>> +    MPQ7932BUCK(4),
>>>> +    MPQ7932BUCK(5),
>>>> +};
>>>> +
>>>> +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);
>>>> +
>>>
>>> This needs explanation. VOUT_COMMAND directly sets a voltage, and the 
>>> provided
>>> value is in ULINEAR16 format. Just using its lower 8 bits seems, at 
>>> the very
>>> least, odd.
>>>
>>
>> Indeed, unfortunately word_write results -EREMOTEIO and got clarified 
>> with manufacture chip support only byte write except STATUS_WORD :(
>>
> Question then is what this byte contains. What if an attempt is made
> to write a value > 0xff ? Writing 0x00 instead of 0x100 can only result
> in severe trouble.
> 

As mention above VOUT_CMD is one byte range 0 to 255. we have 
.n_voltages = 0xff, regulator driver forbid the regulator consumer to 
set value beyond 0xff

VOUT_CMD [7:0]*6.25mV+206.25mV


>>>> +    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;
>>>> +
>>>
>>> This is not how this is intended to work. If the chip does not provide
>>> those properties, they should not be faked.
>>>
>>
>> Unfortunately, chip support neither
>>
>>        PMBUS_VOUT_MARGIN_HIGH          = 0x25,
>>        PMBUS_VOUT_MARGIN_LOW           = 0x26,
>>
>> nor
>>
>>        PMBUS_MFR_VOUT_MIN              = 0xA4,
>>        PMBUS_MFR_VOUT_MAX              = 0xA5
>>
>>
>> as a result set voltage fails due to error in 
>> pmbus_regulator_get_low_margin. I can understand these absolute 
>> workaround, please suggest alternatives
>>
> 
> Good point. I'd accept that with explanation, but PMBUS_MFR_VOUT_MAX
> is a maximum voltage, not the number of voltages. And is 0 for
> the minimum voltage really correct ? It seems to contradict
> MPQ7932_BUCK_UV_MIN.
> 

MRF_VOUT_MAX / MIN takes the absolute value of register (0 to 255) and 
pmbus_reg2data does the conversion

         info->m[PSC_VOLTAGE_OUT] = 160;
         info->b[PSC_VOLTAGE_OUT] = -33;

e.g reg_value is 0
         val = div_s64(val - b, m);
         val = div_s64(0 - (-33), 160);
         val = div_s64(33, 160) => 0.20625 (MPQ7932_BUCK_UV_MIN)


which gives same result of
VOUT_CMD [7:0]*6.25mV+206.25mV


>>>> +    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;
>>>> +    }
>>>> +
>>>
>>> Is that really all the chip supports ? No input data, no current
>>> or power measurements ?
>>>
>>> Is this even a real PMBus chip, or a fake one which claims to support
>>> PMBus but in reality doesn't ?
>>
>> I would say this is PMIC buck regulator chip over PMBUS, I guess the 
>> regulator support in pmbus shall be used without hardware monitor 
>> support (though chip support temperature, error status, cml, status)
>>
>>
>>>
>>>> +    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 = {
>>>
>>> PMBus drivers are not primarily regulator drivers. They
>>> are hardware monitoring drivers. It is difficult for me to
>>> determine, due to lack of information about this chip,
>>> what it actually supports. If the chip is really a regulator
>>> masquerading as PMBus chip, you might want to consider writing
>>> a regulator driver for it.
>>>
>>
>> I try to share datasheet once I got permission from manufacturer.
>> Sure, I consider to write regulator driver over pmbus (addnl helper 
>> support on regulator subsystem like regmap) if we conclude should not 
>> be part of hwmon.
>>
> 
> Depends on you. Do you want a hwmon driver with regulator support,
> or a regulator driver ? We can't have a regulator driver in
> the hwmon subsystem.
>

I have firmly believe this series is already a correct one best of my 
knowledge

> Guenter
> 

Thanks,
Saravanan