diff mbox series

[v1,5/9] dt-bindings: gpio: Add HPE GXP GPIO

Message ID 20230418152824.110823-6-nick.hawkins@hpe.com (mailing list archive)
State Changes Requested
Headers show
Series ARM: Add GPIO and PSU Support | expand

Commit Message

Hawkins, Nick April 18, 2023, 3:28 p.m. UTC 
From: Nick Hawkins <nick.hawkins@hpe.com>

Provide access to the registers and interrupts for GPIO. The GPIO
will have two driver instances: One for host, the other for CPLD.

Signed-off-by: Nick Hawkins <nick.hawkins@hpe.com>
---
 .../bindings/gpio/hpe,gxp-gpio.yaml           | 137 ++++++++++++++++++
 1 file changed, 137 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/hpe,gxp-gpio.yaml

Comments

Krzysztof Kozlowski April 18, 2023, 5:08 p.m. UTC | #1 
On 18/04/2023 17:28, nick.hawkins@hpe.com wrote:
> From: Nick Hawkins <nick.hawkins@hpe.com>
> 
> Provide access to the registers and interrupts for GPIO. The GPIO
> will have two driver instances: One for host, the other for CPLD.

Are these different devices? What does it mean here "instance"? What are
the differences?

> 
> Signed-off-by: Nick Hawkins <nick.hawkins@hpe.com>
> ---
>  .../bindings/gpio/hpe,gxp-gpio.yaml           | 137 ++++++++++++++++++
>  1 file changed, 137 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/gpio/hpe,gxp-gpio.yaml
> 
> diff --git a/Documentation/devicetree/bindings/gpio/hpe,gxp-gpio.yaml b/Documentation/devicetree/bindings/gpio/hpe,gxp-gpio.yaml
> new file mode 100644
> index 000000000000..1cf4cff26d5f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/hpe,gxp-gpio.yaml
> @@ -0,0 +1,137 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/gpio/hpe,gxp-gpio.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: HPE GXP gpio controllers

s/gpio/GPIO/

> +
> +maintainers:
> +  - Nick Hawkins <nick.hawkins@hpe.com>
> +
> +description:
> +  Interruptable GPIO drivers for the HPE GXP that covers multiple interfaces.
> +
> +properties:
> +  compatible:
> +    oneOf:

Drop oneOf.

> +      - items:

And items. You do not have here multiple choices and items.

> +          - enum:
> +              - hpe,gxp-gpio
> +              - hpe,gxp-gpio-pl
> +
> +  reg:
> +    minItems: 3
> +    maxItems: 6
> +
> +  reg-names:
> +    minItems: 3
> +    maxItems: 6
> +
> +  gpio-controller: true
> +
> +  '#gpio-cells':
> +    const: 2
> +
> +  gpio-line-names:
> +    minItems: 1
> +    maxItems: 300

Hm, shouldn't line-names match all GPIOs? If someone provides just one
name, how do you know for which GPIO is it?

> +
> +  interrupts:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - reg-names
> +  - gpio-controller
> +  - "#gpio-cells"

Use consistent quotes. Either ' or "

> +
> +additionalProperties: false

Put it after allOf: block.

> +
> +allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - hpe,gxp-gpio
> +    then:
> +      properties:
> +        reg:
> +          items:
> +            - description: CSM
> +            - description: fn2 virtual button
> +            - description: fn2 system status
> +            - description: vuhc status
> +            - description: external virtual button

I have doubts you describe actual one GPIO controller...

> +        reg-names:
> +          items:
> +            - const: csm
> +            - const: fn2-vbtn
> +            - const: fn2-stat
> +            - const: vuhc
> +            - const: vbtn
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - hpe,gxp-gpio-pl
> +    then:
> +      properties:
> +        reg:
> +          items:
> +            - description: Programmable logic led
> +            - description: Programmable logic health led
> +            - description: Programmable logic interrupt interface
> +        reg-names:
> +          items:
> +            - const: pl-led
> +            - const: pl-health
> +            - const: pl-int
> +
> +examples:
> +  - |
> +        gpio@0 {

Weird indentation. Use 4 spaces for example indentation.

> +          compatible = "hpe,gxp-gpio";
> +          reg = <0x0 0x400>, <0x200046 0x1>, <0x200070 0x08>, <0x400064 0x80>, <0x5100030f 0x1>;
> +          reg-names = "csm", "fn2-vbtn", "fn2-stat", "vuhc", "vbtn";
> +          gpio-controller;
> +          #gpio-cells = <2>;
> +          interrupt-parent = <&vic0>;
> +          interrupts = <10>;
> +          gpio-line-names =
> +          "IOP_LED1", "IOP_LED2", "IOP_LED3", "IOP_LED4", "IOP_LED5", "IOP_LED6", "IOP_LED7", "IOP_LED8",

Broken indentation and unnecessary line break before.

> +          "FAN1_INST", "FAN2_INST", "FAN3_INST", "FAN4_INST", "FAN5_INST", "FAN6_INST", "FAN7_INST",
> +          "FAN8_INST", "FAN1_FAIL", "FAN2_FAIL", "FAN3_FAIL", "FAN4_FAIL", "FAN5_FAIL", "FAN6_FAIL",
> +          "FAN7_FAIL", "FAN8_FAIL", "FAN1_ID", "FAN2_ID", "FAN3_ID", "FAN4_ID", "FAN5_ID", "FAN6_ID",
> +          "FAN7_ID", "FAN8_ID", "IDENTIFY", "HEALTH_RED", "HEALTH_AMBER", "POWER_BUTTON", "UID_PRESS",
> +          "SLP", "NMI_BUTTON", "RESET_BUTTON", "SIO_S5", "SO_ON_CONTROL", "PSU1_INST", "PSU2_INST",
> +          "PSU3_INST", "PSU4_INST", "PSU5_INST", "PSU6_INST", "PSU7_INST", "PSU8_INST", "PSU1_AC",
> +          "PSU2_AC", "PSU3_AC", "PSU4_AC", "PSU5_AC", "PSU6_AC", "PSU7_AC", "PSU8_AC", "PSU1_DC",
> +          "PSU2_DC", "PSU3_DC", "PSU4_DC", "PSU5_DC", "PSU6_DC", "PSU7_DC", "PSU8_DC", "", "", "", "",
> +          "", "", "", "", "", "", "", "", "", "";
> +        };
> +


Best regards,
Krzysztof
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/gpio/hpe,gxp-gpio.yaml b/Documentation/devicetree/bindings/gpio/hpe,gxp-gpio.yaml
new file mode 100644
index 000000000000..1cf4cff26d5f
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/hpe,gxp-gpio.yaml
@@ -0,0 +1,137 @@ 
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/gpio/hpe,gxp-gpio.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: HPE GXP gpio controllers
+
+maintainers:
+  - Nick Hawkins <nick.hawkins@hpe.com>
+
+description:
+  Interruptable GPIO drivers for the HPE GXP that covers multiple interfaces.
+
+properties:
+  compatible:
+    oneOf:
+      - items:
+          - enum:
+              - hpe,gxp-gpio
+              - hpe,gxp-gpio-pl
+
+  reg:
+    minItems: 3
+    maxItems: 6
+
+  reg-names:
+    minItems: 3
+    maxItems: 6
+
+  gpio-controller: true
+
+  '#gpio-cells':
+    const: 2
+
+  gpio-line-names:
+    minItems: 1
+    maxItems: 300
+
+  interrupts:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - reg-names
+  - gpio-controller
+  - "#gpio-cells"
+
+additionalProperties: false
+
+allOf:
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - hpe,gxp-gpio
+    then:
+      properties:
+        reg:
+          items:
+            - description: CSM
+            - description: fn2 virtual button
+            - description: fn2 system status
+            - description: vuhc status
+            - description: external virtual button
+        reg-names:
+          items:
+            - const: csm
+            - const: fn2-vbtn
+            - const: fn2-stat
+            - const: vuhc
+            - const: vbtn
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - hpe,gxp-gpio-pl
+    then:
+      properties:
+        reg:
+          items:
+            - description: Programmable logic led
+            - description: Programmable logic health led
+            - description: Programmable logic interrupt interface
+        reg-names:
+          items:
+            - const: pl-led
+            - const: pl-health
+            - const: pl-int
+
+examples:
+  - |
+        gpio@0 {
+          compatible = "hpe,gxp-gpio";
+          reg = <0x0 0x400>, <0x200046 0x1>, <0x200070 0x08>, <0x400064 0x80>, <0x5100030f 0x1>;
+          reg-names = "csm", "fn2-vbtn", "fn2-stat", "vuhc", "vbtn";
+          gpio-controller;
+          #gpio-cells = <2>;
+          interrupt-parent = <&vic0>;
+          interrupts = <10>;
+          gpio-line-names =
+          "IOP_LED1", "IOP_LED2", "IOP_LED3", "IOP_LED4", "IOP_LED5", "IOP_LED6", "IOP_LED7", "IOP_LED8",
+          "FAN1_INST", "FAN2_INST", "FAN3_INST", "FAN4_INST", "FAN5_INST", "FAN6_INST", "FAN7_INST",
+          "FAN8_INST", "FAN1_FAIL", "FAN2_FAIL", "FAN3_FAIL", "FAN4_FAIL", "FAN5_FAIL", "FAN6_FAIL",
+          "FAN7_FAIL", "FAN8_FAIL", "FAN1_ID", "FAN2_ID", "FAN3_ID", "FAN4_ID", "FAN5_ID", "FAN6_ID",
+          "FAN7_ID", "FAN8_ID", "IDENTIFY", "HEALTH_RED", "HEALTH_AMBER", "POWER_BUTTON", "UID_PRESS",
+          "SLP", "NMI_BUTTON", "RESET_BUTTON", "SIO_S5", "SO_ON_CONTROL", "PSU1_INST", "PSU2_INST",
+          "PSU3_INST", "PSU4_INST", "PSU5_INST", "PSU6_INST", "PSU7_INST", "PSU8_INST", "PSU1_AC",
+          "PSU2_AC", "PSU3_AC", "PSU4_AC", "PSU5_AC", "PSU6_AC", "PSU7_AC", "PSU8_AC", "PSU1_DC",
+          "PSU2_DC", "PSU3_DC", "PSU4_DC", "PSU5_DC", "PSU6_DC", "PSU7_DC", "PSU8_DC", "", "", "", "",
+          "", "", "", "", "", "", "", "", "", "";
+        };
+
+  - |
+        gpio@51000304 {
+          compatible = "hpe,gxp-gpio-pl";
+          reg = <0x51000304 0x2>, <0x5100030d 0x01>, <0x51000380 0x7f>;
+          reg-names = "pl-led", "pl-health", "pl-int";
+          gpio-controller;
+          #gpio-cells = <2>;
+          interrupt-parent = <&vic0>;
+          interrupts = <24>;
+          gpio-line-names =
+          "IOP_LED1", "IOP_LED2", "IOP_LED3", "IOP_LED4", "IOP_LED5", "IOP_LED6", "IOP_LED7", "IOP_LED8",
+          "FAN1_INST", "FAN2_INST", "FAN3_INST", "FAN4_INST", "FAN5_INST", "FAN6_INST", "FAN7_INST",
+          "FAN8_INST", "FAN1_FAIL", "FAN2_FAIL", "FAN3_FAIL", "FAN4_FAIL", "FAN5_FAIL", "FAN6_FAIL",
+          "FAN7_FAIL", "FAN8_FAIL", "FAN1_ID", "FAN2_ID", "FAN3_ID", "FAN4_ID", "FAN5_ID", "FAN6_ID",
+          "FAN7_ID", "FAN8_ID", "IDENTIFY", "HEALTH_RED", "HEALTH_AMBER", "POWER_BUTTON", "UID_PRESS",
+          "SLP", "NMI_BUTTON", "RESET_BUTTON", "SIO_S5", "SO_ON_CONTROL", "PSU1_INST", "PSU2_INST",
+          "PSU3_INST", "PSU4_INST", "PSU5_INST", "PSU6_INST", "PSU7_INST", "PSU8_INST", "PSU1_AC",
+          "PSU2_AC", "PSU3_AC", "PSU4_AC", "PSU5_AC", "PSU6_AC", "PSU7_AC", "PSU8_AC", "PSU1_DC",
+          "PSU2_DC", "PSU3_DC", "PSU4_DC", "PSU5_DC", "PSU6_DC", "PSU7_DC", "PSU8_DC", "", "", "", "",
+          "", "", "", "", "", "", "", "", "", "";
+        };