Message-ID: <6db35313-5dcd-c9f2-724c-d157a835c9fc@linaro.org>
Date:   Tue, 10 May 2022 15:40:29 +0200
From:   Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To:     Chris Packham <chris.packham@...iedtelesis.co.nz>,
        linus.walleij@...aro.org, brgl@...ev.pl, robh+dt@...nel.org,
        krzysztof.kozlowski+dt@...aro.org, thierry.reding@...il.com,
        u.kleine-koenig@...gutronix.de, lee.jones@...aro.org
Cc:     linux-gpio@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-pwm@...r.kernel.org
Subject: Re: [PATCH] dt-bindings: gpio: gpio-mvebu: convert txt binding to
 YAML

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@...iedtelesis.co.nz>
> ---
>  .../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@...18100 {
> -			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@...40 {
> -			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@...il.com>
> +  - Lee Jones <lee.jones@...aro.org>
> +
> +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

Powered by blists - more mailing lists