diff mbox series

[v2,11/14] dt-bindings: mfd: pm8008: rework binding

Message ID 20240529162958.18081-12-johan+linaro@kernel.org
State New
Headers show
Series [v2,01/14] dt-bindings: mfd: pm8008: add reset gpio | expand

Commit Message

Johan Hovold May 29, 2024, 4:29 p.m. UTC
Rework the pm8008 binding by dropping internal details like register
offsets and interrupts and by adding the missing regulator and
temperature alarm properties.

Note that child nodes are still used for pinctrl and regulator
configuration.

Also note that the pinctrl state definition will be extended later and
could eventually also be shared with other PMICs (e.g. by breaking out
bits of qcom,pmic-gpio.yaml).

Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 .../devicetree/bindings/mfd/qcom,pm8008.yaml  | 149 +++++++++++-------
 1 file changed, 90 insertions(+), 59 deletions(-)

Comments

Rob Herring June 4, 2024, 2:43 p.m. UTC | #1
On Wed, May 29, 2024 at 06:29:55PM +0200, Johan Hovold wrote:
> Rework the pm8008 binding by dropping internal details like register
> offsets and interrupts and by adding the missing regulator and
> temperature alarm properties.
> 
> Note that child nodes are still used for pinctrl and regulator
> configuration.
> 
> Also note that the pinctrl state definition will be extended later and
> could eventually also be shared with other PMICs (e.g. by breaking out
> bits of qcom,pmic-gpio.yaml).

This commit message should also state there are no users.

> 
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>  .../devicetree/bindings/mfd/qcom,pm8008.yaml  | 149 +++++++++++-------
>  1 file changed, 90 insertions(+), 59 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml b/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml
> index d71657f488db..ccf472e7f552 100644
> --- a/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml
> +++ b/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml
> @@ -27,103 +27,134 @@ properties:
>    reset-gpios:
>      maxItems: 1
>  
> -  "#interrupt-cells":
> +  vdd-l1-l2-supply: true
> +  vdd-l3-l4-supply: true
> +  vdd-l5-supply: true
> +  vdd-l6-supply: true
> +  vdd-l7-supply: true
> +
> +  gpio-controller: true
> +
> +  "#gpio-cells":
>      const: 2
>  
> -    description: |
> -      The first cell is the IRQ number, the second cell is the IRQ trigger
> -      flag. All interrupts are listed in include/dt-bindings/mfd/qcom-pm8008.h.
> +  gpio-ranges:
> +    maxItems: 1
>  
>    interrupt-controller: true
>  
> -  "#address-cells":
> -    const: 1
> +  "#interrupt-cells":
> +    const: 2
>  
> -  "#size-cells":
> +  "#thermal-sensor-cells":
>      const: 0
>  
> -patternProperties:
> -  "^gpio@[0-9a-f]+$":
> +  pinctrl:
>      type: object
> +    additionalProperties: false
> +    patternProperties:
> +      "-state$":
> +        type: object
> +        $ref: "#/$defs/qcom-pm8008-pinctrl-state"

There's only 1 reference to this, so just move the $def contents here.

Rob
Dmitry Baryshkov June 5, 2024, 8:43 a.m. UTC | #2
On Wed, May 29, 2024 at 06:29:55PM +0200, Johan Hovold wrote:
> Rework the pm8008 binding by dropping internal details like register
> offsets and interrupts and by adding the missing regulator and
> temperature alarm properties.
> 
> Note that child nodes are still used for pinctrl and regulator
> configuration.
> 
> Also note that the pinctrl state definition will be extended later and
> could eventually also be shared with other PMICs (e.g. by breaking out
> bits of qcom,pmic-gpio.yaml).

Obviously we want to adapt this style of bindings for the other PMICs
too. My main concern here are PMICs which have two kinds of controlled
pins: GPIOs and MPPs. With the existing bindings style those are
declared as two subdevices. What would be your suggested way to support
MPPs with the proposed kind of bindings?

> 
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>  .../devicetree/bindings/mfd/qcom,pm8008.yaml  | 149 +++++++++++-------
>  1 file changed, 90 insertions(+), 59 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml b/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml
> index d71657f488db..ccf472e7f552 100644
> --- a/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml
> +++ b/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml
> @@ -27,103 +27,134 @@ properties:
>    reset-gpios:
>      maxItems: 1
>  
> -  "#interrupt-cells":
> +  vdd-l1-l2-supply: true
> +  vdd-l3-l4-supply: true
> +  vdd-l5-supply: true
> +  vdd-l6-supply: true
> +  vdd-l7-supply: true
> +
> +  gpio-controller: true
> +
> +  "#gpio-cells":
>      const: 2
>  
> -    description: |
> -      The first cell is the IRQ number, the second cell is the IRQ trigger
> -      flag. All interrupts are listed in include/dt-bindings/mfd/qcom-pm8008.h.
> +  gpio-ranges:
> +    maxItems: 1
>  
>    interrupt-controller: true
>  
> -  "#address-cells":
> -    const: 1
> +  "#interrupt-cells":
> +    const: 2
>  
> -  "#size-cells":
> +  "#thermal-sensor-cells":
>      const: 0
>  
> -patternProperties:
> -  "^gpio@[0-9a-f]+$":
> +  pinctrl:
>      type: object
> +    additionalProperties: false
> +    patternProperties:
> +      "-state$":
> +        type: object
> +        $ref: "#/$defs/qcom-pm8008-pinctrl-state"
> +        unevaluatedProperties: false
>  
> -    description: |
> -      The GPIO peripheral. This node may be specified twice, one for each GPIO.
> -
> -    properties:
> -      compatible:
> -        items:
> -          - const: qcom,pm8008-gpio
> -          - const: qcom,spmi-gpio
> +  regulators:
> +    type: object
> +    additionalProperties: false
> +    patternProperties:
> +      "^ldo[1-7]$":
> +        type: object
> +        $ref: /schemas/regulator/regulator.yaml#
> +        unevaluatedProperties: false
>  
> -      reg:
> -        description: Peripheral address of one of the two GPIO peripherals.
> -        maxItems: 1
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - vdd-l1-l2-supply
> +  - vdd-l3-l4-supply
> +  - vdd-l5-supply
> +  - vdd-l6-supply
> +  - vdd-l7-supply
> +  - gpio-controller
> +  - "#gpio-cells"
> +  - gpio-ranges
> +  - interrupt-controller
> +  - "#interrupt-cells"
> +  - "#thermal-sensor-cells"
>  
> -      gpio-controller: true
> +additionalProperties: false
>  
> -      gpio-ranges:
> -        maxItems: 1
> +$defs:
> +  qcom-pm8008-pinctrl-state:
> +    type: object
>  
> -      interrupt-controller: true
> +    allOf:
> +      - $ref: /schemas/pinctrl/pinmux-node.yaml
> +      - $ref: /schemas/pinctrl/pincfg-node.yaml
>  
> -      "#interrupt-cells":
> -        const: 2
> +    properties:
> +      pins:
> +        items:
> +          pattern: "^gpio[12]$"
>  
> -      "#gpio-cells":
> -        const: 2
> +      function:
> +        items:
> +          - enum:
> +              - normal
>  
>      required:
> -      - compatible
> -      - reg
> -      - gpio-controller
> -      - interrupt-controller
> -      - "#gpio-cells"
> -      - gpio-ranges
> -      - "#interrupt-cells"
> +      - pins
> +      - function
>  
>      additionalProperties: false
>  
> -required:
> -  - compatible
> -  - reg
> -  - interrupts
> -  - "#address-cells"
> -  - "#size-cells"
> -  - "#interrupt-cells"
> -
> -additionalProperties: false
> -
>  examples:
>    - |
>      #include <dt-bindings/gpio/gpio.h>
> -    #include <dt-bindings/mfd/qcom-pm8008.h>
>      #include <dt-bindings/interrupt-controller/irq.h>
>  
>      i2c {
>        #address-cells = <1>;
>        #size-cells = <0>;
>  
> -      pmic@8 {
> +      pm8008: pmic@8 {
>          compatible = "qcom,pm8008";
>          reg = <0x8>;
> -        #address-cells = <1>;
> -        #size-cells = <0>;
> -        interrupt-controller;
> -        #interrupt-cells = <2>;
>  
>          interrupt-parent = <&tlmm>;
>          interrupts = <32 IRQ_TYPE_EDGE_RISING>;
>  
>          reset-gpios = <&tlmm 42 GPIO_ACTIVE_LOW>;
>  
> -        pm8008_gpios: gpio@c000 {
> -          compatible = "qcom,pm8008-gpio", "qcom,spmi-gpio";
> -          reg = <0xc000>;
> -          gpio-controller;
> -          gpio-ranges = <&pm8008_gpios 0 0 2>;
> -          #gpio-cells = <2>;
> -          interrupt-controller;
> -          #interrupt-cells = <2>;
> +        vdd-l1-l2-supply = <&vreg_s8b_1p2>;
> +        vdd-l3-l4-supply = <&vreg_s1b_1p8>;
> +        vdd-l5-supply = <&vreg_bob>;
> +        vdd-l6-supply = <&vreg_bob>;
> +        vdd-l7-supply = <&vreg_bob>;
> +
> +        gpio-controller;
> +        #gpio-cells = <2>;
> +        gpio-ranges = <&pm8008 0 0 2>;
> +
> +        interrupt-controller;
> +        #interrupt-cells = <2>;
> +
> +        #thermal-sensor-cells = <0>;
> +
> +        pinctrl {
> +          gpio-keys-state {
> +            pins = "gpio1";
> +            function = "normal";
> +          };
> +        };
> +
> +        regulators {
> +          ldo1 {
> +            regulator-name = "vreg_l1";
> +            regulator-min-microvolt = <950000>;
> +            regulator-max-microvolt = <1300000>;
> +          };
>          };
>        };
>      };
> -- 
> 2.44.1
>
Johan Hovold June 8, 2024, 3:36 p.m. UTC | #3
On Wed, Jun 05, 2024 at 11:43:16AM +0300, Dmitry Baryshkov wrote:
> On Wed, May 29, 2024 at 06:29:55PM +0200, Johan Hovold wrote:
> > Rework the pm8008 binding by dropping internal details like register
> > offsets and interrupts and by adding the missing regulator and
> > temperature alarm properties.
> > 
> > Note that child nodes are still used for pinctrl and regulator
> > configuration.
> > 
> > Also note that the pinctrl state definition will be extended later and
> > could eventually also be shared with other PMICs (e.g. by breaking out
> > bits of qcom,pmic-gpio.yaml).
> 
> Obviously we want to adapt this style of bindings for the other PMICs
> too. My main concern here are PMICs which have two kinds of controlled
> pins: GPIOs and MPPs. With the existing bindings style those are
> declared as two subdevices. What would be your suggested way to support
> MPPs with the proposed kind of bindings?

As far as I understand newer PMICs do not have MPP blocks and we do not
necessarily want to convert the existing bindings.

That said, if there is ever a need to describe two separate gpio blocks
this can, for example, be done using subnodes on those PMICs.

Johan
Dmitry Baryshkov June 10, 2024, 6:12 p.m. UTC | #4
On Sat, Jun 08, 2024 at 05:36:36PM +0200, Johan Hovold wrote:
> On Wed, Jun 05, 2024 at 11:43:16AM +0300, Dmitry Baryshkov wrote:
> > On Wed, May 29, 2024 at 06:29:55PM +0200, Johan Hovold wrote:
> > > Rework the pm8008 binding by dropping internal details like register
> > > offsets and interrupts and by adding the missing regulator and
> > > temperature alarm properties.
> > > 
> > > Note that child nodes are still used for pinctrl and regulator
> > > configuration.
> > > 
> > > Also note that the pinctrl state definition will be extended later and
> > > could eventually also be shared with other PMICs (e.g. by breaking out
> > > bits of qcom,pmic-gpio.yaml).
> > 
> > Obviously we want to adapt this style of bindings for the other PMICs
> > too. My main concern here are PMICs which have two kinds of controlled
> > pins: GPIOs and MPPs. With the existing bindings style those are
> > declared as two subdevices. What would be your suggested way to support
> > MPPs with the proposed kind of bindings?
> 
> As far as I understand newer PMICs do not have MPP blocks and we do not
> necessarily want to convert the existing bindings.

Well, I definitely want to do so.

> That said, if there is ever a need to describe two separate gpio blocks
> this can, for example, be done using subnodes on those PMICs.

This creates an asymmetry between older and newer PMICs. Wouldn't it be
better to always use gpios subnode for GPIO pins? This way older PMICS
will use the same approach _plus_ mpps {} subnode instead of having
either nothing or two subnodes.

The same approach probably applies to some other subdevices: temp-alarm
vs adc-tm, etc.
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml b/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml
index d71657f488db..ccf472e7f552 100644
--- a/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml
+++ b/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml
@@ -27,103 +27,134 @@  properties:
   reset-gpios:
     maxItems: 1
 
-  "#interrupt-cells":
+  vdd-l1-l2-supply: true
+  vdd-l3-l4-supply: true
+  vdd-l5-supply: true
+  vdd-l6-supply: true
+  vdd-l7-supply: true
+
+  gpio-controller: true
+
+  "#gpio-cells":
     const: 2
 
-    description: |
-      The first cell is the IRQ number, the second cell is the IRQ trigger
-      flag. All interrupts are listed in include/dt-bindings/mfd/qcom-pm8008.h.
+  gpio-ranges:
+    maxItems: 1
 
   interrupt-controller: true
 
-  "#address-cells":
-    const: 1
+  "#interrupt-cells":
+    const: 2
 
-  "#size-cells":
+  "#thermal-sensor-cells":
     const: 0
 
-patternProperties:
-  "^gpio@[0-9a-f]+$":
+  pinctrl:
     type: object
+    additionalProperties: false
+    patternProperties:
+      "-state$":
+        type: object
+        $ref: "#/$defs/qcom-pm8008-pinctrl-state"
+        unevaluatedProperties: false
 
-    description: |
-      The GPIO peripheral. This node may be specified twice, one for each GPIO.
-
-    properties:
-      compatible:
-        items:
-          - const: qcom,pm8008-gpio
-          - const: qcom,spmi-gpio
+  regulators:
+    type: object
+    additionalProperties: false
+    patternProperties:
+      "^ldo[1-7]$":
+        type: object
+        $ref: /schemas/regulator/regulator.yaml#
+        unevaluatedProperties: false
 
-      reg:
-        description: Peripheral address of one of the two GPIO peripherals.
-        maxItems: 1
+required:
+  - compatible
+  - reg
+  - interrupts
+  - vdd-l1-l2-supply
+  - vdd-l3-l4-supply
+  - vdd-l5-supply
+  - vdd-l6-supply
+  - vdd-l7-supply
+  - gpio-controller
+  - "#gpio-cells"
+  - gpio-ranges
+  - interrupt-controller
+  - "#interrupt-cells"
+  - "#thermal-sensor-cells"
 
-      gpio-controller: true
+additionalProperties: false
 
-      gpio-ranges:
-        maxItems: 1
+$defs:
+  qcom-pm8008-pinctrl-state:
+    type: object
 
-      interrupt-controller: true
+    allOf:
+      - $ref: /schemas/pinctrl/pinmux-node.yaml
+      - $ref: /schemas/pinctrl/pincfg-node.yaml
 
-      "#interrupt-cells":
-        const: 2
+    properties:
+      pins:
+        items:
+          pattern: "^gpio[12]$"
 
-      "#gpio-cells":
-        const: 2
+      function:
+        items:
+          - enum:
+              - normal
 
     required:
-      - compatible
-      - reg
-      - gpio-controller
-      - interrupt-controller
-      - "#gpio-cells"
-      - gpio-ranges
-      - "#interrupt-cells"
+      - pins
+      - function
 
     additionalProperties: false
 
-required:
-  - compatible
-  - reg
-  - interrupts
-  - "#address-cells"
-  - "#size-cells"
-  - "#interrupt-cells"
-
-additionalProperties: false
-
 examples:
   - |
     #include <dt-bindings/gpio/gpio.h>
-    #include <dt-bindings/mfd/qcom-pm8008.h>
     #include <dt-bindings/interrupt-controller/irq.h>
 
     i2c {
       #address-cells = <1>;
       #size-cells = <0>;
 
-      pmic@8 {
+      pm8008: pmic@8 {
         compatible = "qcom,pm8008";
         reg = <0x8>;
-        #address-cells = <1>;
-        #size-cells = <0>;
-        interrupt-controller;
-        #interrupt-cells = <2>;
 
         interrupt-parent = <&tlmm>;
         interrupts = <32 IRQ_TYPE_EDGE_RISING>;
 
         reset-gpios = <&tlmm 42 GPIO_ACTIVE_LOW>;
 
-        pm8008_gpios: gpio@c000 {
-          compatible = "qcom,pm8008-gpio", "qcom,spmi-gpio";
-          reg = <0xc000>;
-          gpio-controller;
-          gpio-ranges = <&pm8008_gpios 0 0 2>;
-          #gpio-cells = <2>;
-          interrupt-controller;
-          #interrupt-cells = <2>;
+        vdd-l1-l2-supply = <&vreg_s8b_1p2>;
+        vdd-l3-l4-supply = <&vreg_s1b_1p8>;
+        vdd-l5-supply = <&vreg_bob>;
+        vdd-l6-supply = <&vreg_bob>;
+        vdd-l7-supply = <&vreg_bob>;
+
+        gpio-controller;
+        #gpio-cells = <2>;
+        gpio-ranges = <&pm8008 0 0 2>;
+
+        interrupt-controller;
+        #interrupt-cells = <2>;
+
+        #thermal-sensor-cells = <0>;
+
+        pinctrl {
+          gpio-keys-state {
+            pins = "gpio1";
+            function = "normal";
+          };
+        };
+
+        regulators {
+          ldo1 {
+            regulator-name = "vreg_l1";
+            regulator-min-microvolt = <950000>;
+            regulator-max-microvolt = <1300000>;
+          };
         };
       };
     };