Date:   Tue, 10 May 2022 23:34:00 +0000
From:   Chris Packham <Chris.Packham@...iedtelesis.co.nz>
To:     Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
        "linus.walleij@...aro.org" <linus.walleij@...aro.org>,
        "brgl@...ev.pl" <brgl@...ev.pl>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "krzysztof.kozlowski+dt@...aro.org" 
        <krzysztof.kozlowski+dt@...aro.org>,
        "thierry.reding@...il.com" <thierry.reding@...il.com>,
        "u.kleine-koenig@...gutronix.de" <u.kleine-koenig@...gutronix.de>,
        "lee.jones@...aro.org" <lee.jones@...aro.org>
CC:     "linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-pwm@...r.kernel.org" <linux-pwm@...r.kernel.org>
Subject: Re: [PATCH] dt-bindings: gpio: gpio-mvebu: convert txt binding to
 YAML


On 11/05/22 01:40, Krzysztof Kozlowski wrote:
> 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://scanmail.trustwave.com/?c=20988&d=1ev64paEKF7o3yTVoF_fgiVBkeUi0_Dg3YQi4JPtpA&u=http%3a%2f%2fdevicetree%2eorg%2fschemas%2fgpio%2fgpio-mvebu%2eyaml%23
>> +$schema: http://scanmail.trustwave.com/?c=20988&d=1ev64paEKF7o3yTVoF_fgiVBkeUi0_Dg3YBxtMDuqw&u=http%3a%2f%2fdevicetree%2eorg%2fmeta-schemas%2fcore%2eyaml%23
>> +
>> +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.
Except for marvell,armada-8k-gpio. I'll look in to how to capture that.
>
>> +          - 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?

I found the mapping of SoCs to compatible a bit non-obvious so I kept 
it. The in-tree dts files don't actually follow what's documented 
(armada-xp-*.dtsi don't use "marvel,armadaxp-gpio").

>> +
>> +  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.

I merged two sections from the old bindings. On the ArmadaXP the 2nd 
entry is documented as for the per-CPU registers and appears to be 
mandatory. For the others the 2nd entry is optional and is for the pwm 
function. But as noted above the in-tree dts files don't actually use 
the armadaxp compatible so this might be wrong in the old binding.

>
> 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?
As mentioned separately. This is for the pwm function which is different 
to the other offset.
>> +
>> +  "#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.
Will update
>> +
>> +required:
>> +  - compatible
>> +  - gpio-controller
>> +  - ngpios
>> +  - "#gpio-cells"
>> +
>> +if:
> Within allOf please.
>
> Best regards,
> Krzysztof

Powered by blists - more mailing lists