On 10/05/2022 11:44, Chris Packham wrote:
> Convert the existing device tree binding to YAML format.
>
> The old binding listed the interrupt-controller and related properties
> as required but there are sufficiently many existing usages without it
> that the YAML binding does not make the interrupt properties required.
>
> The offset and marvell,pwm-offset properties weren't in the old binding
> and are added to the YAML binding. The offset property is required when
> the marvell,armada-8k-gpio compatible is used.
>
> Signed-off-by: Chris Packham <chris.packham@xxxxxxxxxxxxxxxxxxx>
> ---
> .../devicetree/bindings/gpio/gpio-mvebu.txt | 93 -----------
> .../devicetree/bindings/gpio/gpio-mvebu.yaml | 147 ++++++++++++++++++
> MAINTAINERS | 2 +-
> 3 files changed, 148 insertions(+), 94 deletions(-)
> delete mode 100644 Documentation/devicetree/bindings/gpio/gpio-mvebu.txt
> create mode 100644 Documentation/devicetree/bindings/gpio/gpio-mvebu.yaml
>
> diff --git a/Documentation/devicetree/bindings/gpio/gpio-mvebu.txt b/Documentation/devicetree/bindings/gpio/gpio-mvebu.txt
> deleted file mode 100644
> index 0fc6700ed800..000000000000
> --- a/Documentation/devicetree/bindings/gpio/gpio-mvebu.txt
> +++ /dev/null
> @@ -1,93 +0,0 @@
> -* Marvell EBU GPIO controller
> -
> -Required properties:
> -
> -- compatible : Should be "marvell,orion-gpio", "marvell,mv78200-gpio",
> - "marvell,armadaxp-gpio" or "marvell,armada-8k-gpio".
> -
> - "marvell,orion-gpio" should be used for Orion, Kirkwood, Dove,
> - Discovery (except MV78200) and Armada 370. "marvell,mv78200-gpio"
> - should be used for the Discovery MV78200.
> -
> - "marvel,armadaxp-gpio" should be used for all Armada XP SoCs
> - (MV78230, MV78260, MV78460).
> -
> - "marvell,armada-8k-gpio" should be used for the Armada 7K and 8K
> - SoCs (either from AP or CP), see
> - Documentation/devicetree/bindings/arm/marvell/ap80x-system-controller.txt
> - for specific details about the offset property.
> -
> -- reg: Address and length of the register set for the device. Only one
> - entry is expected, except for the "marvell,armadaxp-gpio" variant
> - for which two entries are expected: one for the general registers,
> - one for the per-cpu registers. Not used for marvell,armada-8k-gpio.
> -
> -- interrupts: The list of interrupts that are used for all the pins
> - managed by this GPIO bank. There can be more than one interrupt
> - (example: 1 interrupt per 8 pins on Armada XP, which means 4
> - interrupts per bank of 32 GPIOs).
> -
> -- interrupt-controller: identifies the node as an interrupt controller
> -
> -- #interrupt-cells: specifies the number of cells needed to encode an
> - interrupt source. Should be two.
> - The first cell is the GPIO number.
> - The second cell is used to specify flags:
> - bits[3:0] trigger type and level flags:
> - 1 = low-to-high edge triggered.
> - 2 = high-to-low edge triggered.
> - 4 = active high level-sensitive.
> - 8 = active low level-sensitive.
> -
> -- gpio-controller: marks the device node as a gpio controller
> -
> -- ngpios: number of GPIOs this controller has
> -
> -- #gpio-cells: Should be two. The first cell is the pin number. The
> - second cell is reserved for flags, unused at the moment.
> -
> -Optional properties:
> -
> -In order to use the GPIO lines in PWM mode, some additional optional
> -properties are required.
> -
> -- compatible: Must contain "marvell,armada-370-gpio"
> -
> -- reg: an additional register set is needed, for the GPIO Blink
> - Counter on/off registers.
> -
> -- reg-names: Must contain an entry "pwm" corresponding to the
> - additional register range needed for PWM operation.
> -
> -- #pwm-cells: Should be two. The first cell is the GPIO line number. The
> - second cell is the period in nanoseconds.
> -
> -- clocks: Must be a phandle to the clock for the GPIO controller.
> -
> -Example:
> -
> - gpio0: gpio@d0018100 {
> - compatible = "marvell,armadaxp-gpio";
> - reg = <0xd0018100 0x40>,
> - <0xd0018800 0x30>;
> - ngpios = <32>;
> - gpio-controller;
> - #gpio-cells = <2>;
> - interrupt-controller;
> - #interrupt-cells = <2>;
> - interrupts = <16>, <17>, <18>, <19>;
> - };
> -
> - gpio1: gpio@18140 {
> - compatible = "marvell,armada-370-gpio";
> - reg = <0x18140 0x40>, <0x181c8 0x08>;
> - reg-names = "gpio", "pwm";
> - ngpios = <17>;
> - gpio-controller;
> - #gpio-cells = <2>;
> - #pwm-cells = <2>;
> - interrupt-controller;
> - #interrupt-cells = <2>;
> - interrupts = <87>, <88>, <89>;
> - clocks = <&coreclk 0>;
> - };
> diff --git a/Documentation/devicetree/bindings/gpio/gpio-mvebu.yaml b/Documentation/devicetree/bindings/gpio/gpio-mvebu.yaml
> new file mode 100644
> index 000000000000..84b72e506526
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/gpio-mvebu.yaml
> @@ -0,0 +1,147 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/gpio/gpio-mvebu.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Marvell EBU GPIO controller
> +
> +maintainers:
> + - Thierry Reding <thierry.reding@xxxxxxxxx>
> + - Lee Jones <lee.jones@xxxxxxxxxx>
> +
> +properties:
> + compatible:
> + oneOf:
> + - enum:
> + - marvell,orion-gpio
> + - marvell,mv78200-gpio
> + - marvell,armada-370-gpio
It's expected to have orion fallback, so this does not look correct.
> + - marvell,armadaxp-gpio
> + - marvell,armada-8k-gpio
> + - items:
> + - const: marvell,armada-370-gpio
> + - const: marvell,orion-gpio
> +
> + description: |
> + "marvell,orion-gpio" should be used for Orion, Kirkwood, Dove, Discovery
> + (except MV78200) and Armada 370. "marvell,mv78200-gpio" should be used
> + for the Discovery MV78200.
> +
> + "marvel,armadaxp-gpio" should be used for all Armada XP SoCs (MV78230,
> + MV78260, MV78460).
> +
> + "marvell,armada-8k-gpio" should be used for the Armada 7K and 8K SoCs
> + (either from AP or CP), see
> + Documentation/devicetree/bindings/arm/marvell/ap80x-system-controller.txt
> + for specific details about the offset property.
Why having the description? The usage should be obvious from the schema,
so what is so special here?
> +
> + reg:
> + description: |
> + Address and length of the register set for the device. Only one entry
> + is expected, except for the "marvell,armadaxp-gpio" variant for which
> + two entries are expected: one for the general registers, one for the
> + per-cpu registers.
This needs also entry in allOf with per-variant constraints.
> Not used for marvell,armada-8k-gpio.
> +
> + An additional register set is needed, for the GPIO Blink
> + Counter on/off registers.
> + minItems: 1
> + maxItems: 2
PWM? the "reg" above was saying about per-cpu registers, so this is
confusing. I understand old bindings wrote it like that, so maybe it
should be fixed now.
Anyway, you need to describe the items and then apply constraints in allOf.
> +
> + reg-names:
> + description:
> + Must contain an entry "pwm" corresponding to the
> + additional register range needed for PWM operation.
> + minItems: 1
> + maxItems: 2
> +
> + offset:
> + $ref: /schemas/types.yaml#/definitions/uint32
> + description: Offset in the register map for the gpio registers (in bytes)
> +
> + interrupts:
> + description: |
> + The list of interrupts that are used for all the pins managed by this
> + GPIO bank. There can be more than one interrupt (example: 1 interrupt
> + per 8 pins on Armada XP, which means 4 interrupts per bank of 32
> + GPIOs).
> + minItems: 1
> + maxItems: 4
> +
> + interrupt-controller: true
> +
> + "#interrupt-cells":
> + const: 2
> +
> + gpio-controller: true
> +
> + ngpios:
> + description:
> + number of GPIOs this controller has
Skip description, it's obvious from generic bindings.
> + minimum: 1
> + maximum: 32
> +
> + "#gpio-cells":
> + const: 2
> +
> + marvell,pwm-offset:
> + $ref: /schemas/types.yaml#/definitions/uint32
> + description: Offset in the register map for the pwm registers (in bytes)
It's the same as offset. Why allowing both? Isn't one deprecated?
> +
> + "#pwm-cells":
> + description:
> + The first cell is the GPIO line number. The second cell is the period
> + in nanoseconds.
> + const: 2
> +
> + clocks:
> + minItems: 1
> + maxItems: 2
This should be strictly defined, either here or per variant.
> +
> +required:
> + - compatible
> + - gpio-controller
> + - ngpios
> + - "#gpio-cells"
> +
> +if:
Within allOf please.
Best regards,
Krzysztof
