[PATCH v3 0/3] Add support for mpq2286 PMIC IC

[PATCH v3 0/3] Add support for mpq2286 PMIC IC

[Saravanan Sekar]

changes in v3:
 - dt-binding commit message updated the reason for 'buck0' is used instead of 'buck'

changes in v2:
 - fix dt check warnings
 - fix compiler warning on cast device_get_match_data and lexicological order of compatible



The MPQ2286 is a programmable, high frequency synchronous buck regulator with
integrated internal high side and low side power MOSFET. Application in
Automotive compenents such as ADAS, Infotainment, SOC System core, DDR memory.


Saravanan Sekar (3):
  hwmon: (pmbus/mpq7932) Get page count based on chip info
  regulator: dt-bindings: Add mps,mpq2286 power-management IC
  hwmon: (pmbus/mpq2286) Add a support for mpq2286 Power Management IC

 .../bindings/regulator/mps,mpq2286.yaml       | 59 +++++++++++++++++++
 drivers/hwmon/pmbus/mpq7932.c                 |  9 ++-
 2 files changed, 65 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/regulator/mps,mpq2286.yaml

-- 
2.34.1

[PATCH v3 1/3] hwmon: (pmbus/mpq7932) Get page count based on chip info

[Saravanan Sekar]

Get page count using compatible match to support the series of chipsets
which differs in number of regualator/page.

Signed-off-by: Saravanan Sekar <saravanan@linumiz.com>
---
 drivers/hwmon/pmbus/mpq7932.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/hwmon/pmbus/mpq7932.c b/drivers/hwmon/pmbus/mpq7932.c
index 6c62f01da7c6..723c314a57a2 100644
--- a/drivers/hwmon/pmbus/mpq7932.c
+++ b/drivers/hwmon/pmbus/mpq7932.c
@@ -105,7 +105,7 @@ static int mpq7932_probe(struct i2c_client *client)
 		return -ENOMEM;
 
 	info = &data->info;
-	info->pages = MPQ7932_NUM_PAGES;
+	info->pages = (int)(unsigned long)device_get_match_data(&client->dev);
 	info->format[PSC_VOLTAGE_OUT] = direct;
 	info->m[PSC_VOLTAGE_OUT] = 160;
 	info->b[PSC_VOLTAGE_OUT] = -33;
@@ -115,7 +115,7 @@ static int mpq7932_probe(struct i2c_client *client)
 	}
 
 #if IS_ENABLED(CONFIG_SENSORS_MPQ7932_REGULATOR)
-	info->num_regulators = ARRAY_SIZE(mpq7932_regulators_desc);
+	info->num_regulators = info->pages;
 	info->reg_desc = mpq7932_regulators_desc;
 #endif
 
@@ -129,7 +129,7 @@ static int mpq7932_probe(struct i2c_client *client)
 }
 
 static const struct of_device_id mpq7932_of_match[] = {
-	{ .compatible = "mps,mpq7932"},
+	{ .compatible = "mps,mpq7932", .data = (void *)MPQ7932_NUM_PAGES },
 	{},
 };
 MODULE_DEVICE_TABLE(of, mpq7932_of_match);
-- 
2.34.1

[PATCH v3 2/3] regulator: dt-bindings: Add mps,mpq2286 power-management IC

[Saravanan Sekar]

Document mpq2286 power-management IC. Instead of simple 'buck', 'buck0' is
used to keep the driver common which handles multiple regulators.

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

diff --git a/Documentation/devicetree/bindings/regulator/mps,mpq2286.yaml b/Documentation/devicetree/bindings/regulator/mps,mpq2286.yaml
new file mode 100644
index 000000000000..594b929fe4b8
--- /dev/null
+++ b/Documentation/devicetree/bindings/regulator/mps,mpq2286.yaml
@@ -0,0 +1,59 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/regulator/mps,mpq2286.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Monolithic Power System MPQ2286 PMIC
+
+maintainers:
+  - Saravanan Sekar <saravanan@linumiz.com>
+
+properties:
+  compatible:
+    enum:
+      - mps,mpq2286
+
+  reg:
+    maxItems: 1
+
+  regulators:
+    type: object
+
+    properties:
+      buck0:
+        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,mpq2286";
+            reg = <0x3>;
+
+            regulators {
+                buck0 {
+                    regulator-name = "buck0";
+                    regulator-min-microvolt = <1600000>;
+                    regulator-max-microvolt = <1800000>;
+                    regulator-boot-on;
+                };
+            };
+        };
+    };
+...
-- 
2.34.1

[PATCH v3 3/3] hwmon: (pmbus/mpq2286) Add a support for mpq2286 Power Management IC

[Saravanan Sekar]

The MPQ2286 is a programmable, high frequency synchronous buck regulator
designed to power a variety of Automotive system peripherals. Single buck
converters with hardware monitoring capability is configurable over PMBus
interface.

Signed-off-by: Saravanan Sekar <saravanan@linumiz.com>
---
 drivers/hwmon/pmbus/mpq7932.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/hwmon/pmbus/mpq7932.c b/drivers/hwmon/pmbus/mpq7932.c
index 723c314a57a2..60d7b8aec05a 100644
--- a/drivers/hwmon/pmbus/mpq7932.c
+++ b/drivers/hwmon/pmbus/mpq7932.c
@@ -21,6 +21,7 @@
 #define MPQ7932_N_VOLTAGES		256
 #define MPQ7932_VOUT_MAX		0xFF
 #define MPQ7932_NUM_PAGES		6
+#define MPQ2286_NUM_PAGES		1
 
 #define MPQ7932_TON_DELAY		0x60
 #define MPQ7932_VOUT_STARTUP_SLEW	0xA3
@@ -129,12 +130,14 @@ static int mpq7932_probe(struct i2c_client *client)
 }
 
 static const struct of_device_id mpq7932_of_match[] = {
+	{ .compatible = "mps,mpq2286", .data = (void *)MPQ2286_NUM_PAGES },
 	{ .compatible = "mps,mpq7932", .data = (void *)MPQ7932_NUM_PAGES },
 	{},
 };
 MODULE_DEVICE_TABLE(of, mpq7932_of_match);
 
 static const struct i2c_device_id mpq7932_id[] = {
+	{ "mpq2286", },
 	{ "mpq7932", },
 	{ },
 };
-- 
2.34.1

Re: [PATCH v3 2/3] regulator: dt-bindings: Add mps,mpq2286 power-management IC

[Guenter Roeck]

On Sat, Oct 07, 2023 at 10:28:02PM +0530, Saravanan Sekar wrote:
> Document mpq2286 power-management IC. Instead of simple 'buck', 'buck0' is
> used to keep the driver common which handles multiple regulators.

Sorry for the maybe dumb question, but where can I find the driver
depencency on buck naming ?

Thanks,
Guenter

> 
> Signed-off-by: Saravanan Sekar <saravanan@linumiz.com>
> ---
>  .../bindings/regulator/mps,mpq2286.yaml       | 59 +++++++++++++++++++
>  1 file changed, 59 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/regulator/mps,mpq2286.yaml
> 
> diff --git a/Documentation/devicetree/bindings/regulator/mps,mpq2286.yaml b/Documentation/devicetree/bindings/regulator/mps,mpq2286.yaml
> new file mode 100644
> index 000000000000..594b929fe4b8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/regulator/mps,mpq2286.yaml
> @@ -0,0 +1,59 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/regulator/mps,mpq2286.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Monolithic Power System MPQ2286 PMIC
> +
> +maintainers:
> +  - Saravanan Sekar <saravanan@linumiz.com>
> +
> +properties:
> +  compatible:
> +    enum:
> +      - mps,mpq2286
> +
> +  reg:
> +    maxItems: 1
> +
> +  regulators:
> +    type: object
> +
> +    properties:
> +      buck0:
> +        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,mpq2286";
> +            reg = <0x3>;
> +
> +            regulators {
> +                buck0 {
> +                    regulator-name = "buck0";
> +                    regulator-min-microvolt = <1600000>;
> +                    regulator-max-microvolt = <1800000>;
> +                    regulator-boot-on;
> +                };
> +            };
> +        };
> +    };
> +...
> -- 
> 2.34.1
> 

Re: [PATCH v3 2/3] regulator: dt-bindings: Add mps,mpq2286 power-management IC

[Krzysztof Kozlowski]

On 08/10/2023 03:20, Guenter Roeck wrote:
> On Sat, Oct 07, 2023 at 10:28:02PM +0530, Saravanan Sekar wrote:
>> Document mpq2286 power-management IC. Instead of simple 'buck', 'buck0' is
>> used to keep the driver common which handles multiple regulators.
> 
> Sorry for the maybe dumb question, but where can I find the driver
> depencency on buck naming ?

I guess it is because:
PMBUS_REGULATOR_STEP("buck", 0, MPQ7932_N_VOLTAGES,
creates regulator name as buck+id (so buck0).

However as Rob pointed out, driver can handle buck for this variant. We
want the bindings to be independent from Linux implementation and this
patch here adds such dependency.

Therefore I still believe this should be just "buck", unless the
datasheet says it is "buck0". Which I doubt.


Best regards,
Krzysztof

Re: [PATCH v3 2/3] regulator: dt-bindings: Add mps,mpq2286 power-management IC

[Guenter Roeck]

On Sun, Oct 08, 2023 at 12:40:29PM +0200, Krzysztof Kozlowski wrote:
> On 08/10/2023 03:20, Guenter Roeck wrote:
> > On Sat, Oct 07, 2023 at 10:28:02PM +0530, Saravanan Sekar wrote:
> >> Document mpq2286 power-management IC. Instead of simple 'buck', 'buck0' is
> >> used to keep the driver common which handles multiple regulators.
> > 
> > Sorry for the maybe dumb question, but where can I find the driver
> > depencency on buck naming ?
> 
> I guess it is because:
> PMBUS_REGULATOR_STEP("buck", 0, MPQ7932_N_VOLTAGES,
> creates regulator name as buck+id (so buck0).
> 

Ah, good point. Problem here is that this is already kind of common,
even though the use of "buckX" isn't. Look for "vout0", or
'PMBUS_REGULATOR("vout", 0)'. Apparently so far no one took offence
if a regulator was named "vout0" even if "vout1" didn't exist.

I don't really have a good solution right now, but I guess we'll need
a second set of macros for the single-regulator case, or maybe generate
struct regulator_desc arrays using a function. I'll have to explore
options.

Please let me know how you want the subsystem to handle existing
single-channel regulators with numbered regulator name.

Saravanan - for this driver please just declare a local driver-specific
variant of the PMBUS_REGULATOR_STEP() macro which doesn't use indexing,
use it to initialise a second regulators_desc array, and use that second
array for mpq2286. That is a bit messy, but acceptable for now until
there is a more generic solution (unless of course you have an idea for
one and want to implement it, but that is not a requirement).

Thanks,
Guenter

Re: [PATCH v3 2/3] regulator: dt-bindings: Add mps,mpq2286 power-management IC

[Saravanan Sekar]

On 08/10/23 19:22, Guenter Roeck wrote:
> On Sun, Oct 08, 2023 at 12:40:29PM +0200, Krzysztof Kozlowski wrote:
>> On 08/10/2023 03:20, Guenter Roeck wrote:
>>> On Sat, Oct 07, 2023 at 10:28:02PM +0530, Saravanan Sekar wrote:
>>>> Document mpq2286 power-management IC. Instead of simple 'buck', 'buck0' is
>>>> used to keep the driver common which handles multiple regulators.
>>>
>>> Sorry for the maybe dumb question, but where can I find the driver
>>> depencency on buck naming ?
>>
>> I guess it is because:
>> PMBUS_REGULATOR_STEP("buck", 0, MPQ7932_N_VOLTAGES,
>> creates regulator name as buck+id (so buck0).
>>
> 
> Ah, good point. Problem here is that this is already kind of common,
> even though the use of "buckX" isn't. Look for "vout0", or
> 'PMBUS_REGULATOR("vout", 0)'. Apparently so far no one took offence
> if a regulator was named "vout0" even if "vout1" didn't exist.
> 
> I don't really have a good solution right now, but I guess we'll need
> a second set of macros for the single-regulator case, or maybe generate
> struct regulator_desc arrays using a function. I'll have to explore
> options.
> 
> Please let me know how you want the subsystem to handle existing
> single-channel regulators with numbered regulator name.
> 
> Saravanan - for this driver please just declare a local driver-specific
> variant of the PMBUS_REGULATOR_STEP() macro which doesn't use indexing,
> use it to initialise a second regulators_desc array, and use that second
> array for mpq2286. That is a bit messy, but acceptable for now until
> there is a more generic solution (unless of course you have an idea for
> one and want to implement it, but that is not a requirement).
Hello Guenter,

Thanks for your proposal as intermediate fix local declaration of macro, 
could you please suggest whether below changes is acceptable as workaround?

+++ b/drivers/hwmon/pmbus/mpq7932.c
@@ -118,6 +118,12 @@ static int mpq7932_probe(struct i2c_client *client)
  #if IS_ENABLED(CONFIG_SENSORS_MPQ7932_REGULATOR)
         info->num_regulators = info->pages;
         info->reg_desc = mpq7932_regulators_desc;
+
+       if (info->num_regulators == 1) {
+               mpq7932_regulators_desc->name = "buck";
+               mpq7932_regulators_desc->of_match = of_match_ptr("buck");
+       }
+


> 
> Thanks,
> Guenter

Thanks,
Saravanan

Re: [PATCH v3 2/3] regulator: dt-bindings: Add mps,mpq2286 power-management IC

[Guenter Roeck]

On Wed, Oct 11, 2023 at 12:02:44PM +0530, Saravanan Sekar wrote:
> On 08/10/23 19:22, Guenter Roeck wrote:
> > On Sun, Oct 08, 2023 at 12:40:29PM +0200, Krzysztof Kozlowski wrote:
> > > On 08/10/2023 03:20, Guenter Roeck wrote:
> > > > On Sat, Oct 07, 2023 at 10:28:02PM +0530, Saravanan Sekar wrote:
> > > > > Document mpq2286 power-management IC. Instead of simple 'buck', 'buck0' is
> > > > > used to keep the driver common which handles multiple regulators.
> > > > 
> > > > Sorry for the maybe dumb question, but where can I find the driver
> > > > depencency on buck naming ?
> > > 
> > > I guess it is because:
> > > PMBUS_REGULATOR_STEP("buck", 0, MPQ7932_N_VOLTAGES,
> > > creates regulator name as buck+id (so buck0).
> > > 
> > 
> > Ah, good point. Problem here is that this is already kind of common,
> > even though the use of "buckX" isn't. Look for "vout0", or
> > 'PMBUS_REGULATOR("vout", 0)'. Apparently so far no one took offence
> > if a regulator was named "vout0" even if "vout1" didn't exist.
> > 
> > I don't really have a good solution right now, but I guess we'll need
> > a second set of macros for the single-regulator case, or maybe generate
> > struct regulator_desc arrays using a function. I'll have to explore
> > options.
> > 
> > Please let me know how you want the subsystem to handle existing
> > single-channel regulators with numbered regulator name.
> > 
> > Saravanan - for this driver please just declare a local driver-specific
> > variant of the PMBUS_REGULATOR_STEP() macro which doesn't use indexing,
> > use it to initialise a second regulators_desc array, and use that second
> > array for mpq2286. That is a bit messy, but acceptable for now until
> > there is a more generic solution (unless of course you have an idea for
> > one and want to implement it, but that is not a requirement).
> Hello Guenter,
> 
> Thanks for your proposal as intermediate fix local declaration of macro,
> could you please suggest whether below changes is acceptable as workaround?
> 

No, because that would overwrite a data structure which might be needed
by another mpq7932 in the system. mpq7932_regulators_desc should really
be declared const to clarify that it is not supposed to be changed.

Thinking more about it, the solution is actually quite simple. Please add
a second patch adding PMBUS_REGULATOR_STEP_ONE() and PMBUS_REGULATOR_ONE()
macros to drivers/hwmon/pmbus/pmbus.h and use the new macro in this patch.
That would result in code such as

static const struct regulator_desc mpq7932_regulators_desc_one[] = {
        PMBUS_REGULATOR_STEP_ONE("buck", MPQ7932_N_VOLTAGES,
				 MPQ7932_UV_STEP, MPQ7932_BUCK_UV_MIN),
};

...

        if (info->num_regulators == 1)
                info->reg_desc = mpq7932_regulators_desc_one;
        else
                info->reg_desc = mpq7932_regulators_desc;

We can then use the xxx_ONE macros when adding regulator support to existing
or new drivers, and either keep existing drivers as-is or update them based
on DT maintainer input.

Thanks,
Guenter