2023-04-18 15:33:46

by Hawkins, Nick

[permalink] [raw]
Subject: [PATCH v1 5/9] dt-bindings: gpio: Add HPE GXP GPIO

From: Nick Hawkins <[email protected]>

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 <[email protected]>
---
.../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
+
+maintainers:
+ - Nick Hawkins <[email protected]>
+
+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", "", "", "", "",
+ "", "", "", "", "", "", "", "", "", "";
+ };
--
2.17.1


2023-04-18 17:14:02

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH v1 5/9] dt-bindings: gpio: Add HPE GXP GPIO

On 18/04/2023 17:28, [email protected] wrote:
> From: Nick Hawkins <[email protected]>
>
> 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 <[email protected]>
> ---
> .../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 <[email protected]>
> +
> +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