diff mbox series

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

Message ID 20231007165803.239718-3-saravanan@linumiz.com (mailing list archive)
State Superseded
Headers show
Series Add support for mpq2286 PMIC IC | expand

Commit Message

Saravanan Sekar Oct. 7, 2023, 4:58 p.m. UTC 
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

Comments

Guenter Roeck Oct. 8, 2023, 1:20 a.m. UTC | #1 
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
>
Krzysztof Kozlowski Oct. 8, 2023, 10:40 a.m. UTC | #2 
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
Guenter Roeck Oct. 8, 2023, 1:52 p.m. UTC | #3 
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
Saravanan Sekar Oct. 11, 2023, 6:32 a.m. UTC | #4 
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
Guenter Roeck Oct. 11, 2023, 2:50 p.m. UTC | #5 
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
diff mbox series

Patch

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