On Fri, Mar 27, 2020 at 01:28:20PM -0600, Rob Herring wrote:
> On Tue, Mar 17, 2020 at 10:39:15AM +0100, Lubomir Rintel wrote:
> > This converts the mrvl-gpio binding to DT schema format using json-schema.
> >
> > Various fixes were done during the conversion, such as adding more
> > properties that are in fact mandatory or extending the examples to
> > include child nodes with extra GPIO blocks.
>
> Ugg, not how I would have done this. Differences in register layout
> should be implied by the compatible strings. But I guess we have to live
> with it now.
>
> > The compatible strings are a mess. It is not clear why so many of them
> > are needed; the driver doesn't really seem to differentiate between the
> > models. Some of them, like marvell,pxa93x-gpio and marvell,pxa1928-gpio
> > are not used at all, so it's not known how many interrupts they utilize.
> > On the other hand, mrvl,pxa-gpio has been seen in the tree, but it
> > doesn't end up in any actual DTB file.
>
> I added pxa1928 and then work on it ended. I think it is safe to remove,
> but I dug up dts file and it is 1 irq.
>
> > In any case -- the schema merely copies whatever was in the original
> > binding document, so it's hopefully no more wrong that the original.
> >
> > Signed-off-by: Lubomir Rintel <lkundrak@xxxxx>
> > ---
> > .../devicetree/bindings/gpio/mrvl-gpio.txt | 48 -----
> > .../devicetree/bindings/gpio/mrvl-gpio.yaml | 173 ++++++++++++++++++
> > 2 files changed, 173 insertions(+), 48 deletions(-)
> > delete mode 100644 Documentation/devicetree/bindings/gpio/mrvl-gpio.txt
> > create mode 100644 Documentation/devicetree/bindings/gpio/mrvl-gpio.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/gpio/mrvl-gpio.txt b/Documentation/devicetree/bindings/gpio/mrvl-gpio.txt
> > deleted file mode 100644
> > index 30fd2201b3d4c..0000000000000
> > --- a/Documentation/devicetree/bindings/gpio/mrvl-gpio.txt
> > +++ /dev/null
> > @@ -1,48 +0,0 @@
> > -* Marvell PXA GPIO controller
> > -
> > -Required properties:
> > -- compatible : Should be "intel,pxa25x-gpio", "intel,pxa26x-gpio",
> > - "intel,pxa27x-gpio", "intel,pxa3xx-gpio",
> > - "marvell,pxa93x-gpio", "marvell,mmp-gpio",
> > - "marvell,mmp2-gpio" or marvell,pxa1928-gpio.
> > -- reg : Address and length of the register set for the device
> > -- interrupts : Should be the port interrupt shared by all gpio pins.
> > - There're three gpio interrupts in arch-pxa, and they're gpio0,
> > - gpio1 and gpio_mux. There're only one gpio interrupt in arch-mmp,
> > - gpio_mux.
> > -- interrupt-names : Should be the names of irq resources. Each interrupt
> > - uses its own interrupt name, so there should be as many interrupt names
> > - as referenced interrupts.
> > -- interrupt-controller : Identifies the node as an interrupt controller.
> > -- #interrupt-cells: Specifies the number of cells needed to encode an
> > - interrupt source.
> > -- gpio-controller : Marks the device node as a gpio controller.
> > -- #gpio-cells : Should be two. The first cell is the pin number and
> > - the second cell is used to specify flags. See gpio.txt for possible
> > - values.
> > -
> > -Example for a MMP platform:
> > -
> > - gpio: gpio@d4019000 {
> > - compatible = "marvell,mmp-gpio";
> > - reg = <0xd4019000 0x1000>;
> > - interrupts = <49>;
> > - interrupt-names = "gpio_mux";
> > - gpio-controller;
> > - #gpio-cells = <2>;
> > - interrupt-controller;
> > - #interrupt-cells = <1>;
> > - };
> > -
> > -Example for a PXA3xx platform:
> > -
> > - gpio: gpio@40e00000 {
> > - compatible = "intel,pxa3xx-gpio";
> > - reg = <0x40e00000 0x10000>;
> > - interrupt-names = "gpio0", "gpio1", "gpio_mux";
> > - interrupts = <8 9 10>;
> > - gpio-controller;
> > - #gpio-cells = <0x2>;
> > - interrupt-controller;
> > - #interrupt-cells = <0x2>;
> > - };
> > diff --git a/Documentation/devicetree/bindings/gpio/mrvl-gpio.yaml b/Documentation/devicetree/bindings/gpio/mrvl-gpio.yaml
> > new file mode 100644
> > index 0000000000000..5c713bf59b06d
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/gpio/mrvl-gpio.yaml
> > @@ -0,0 +1,173 @@
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/gpio/mrvl-gpio.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Marvell PXA GPIO controller
> > +
> > +maintainers:
> > + - devicetree@xxxxxxxxxxxxxxx
>
> Needs to be a real person that cares about this h/w.
As I've said in the cover letter -- it's not clear who would that be.
> > +
> > +allOf:
> > + - if:
> > + properties:
> > + compatible:
> > + contains:
> > + enum:
> > + - intel,pxa25x-gpio
> > + - intel,pxa26x-gpio
> > + - intel,pxa27x-gpio
> > + - intel,pxa3xx-gpio
> > + then:
> > + properties:
> > + interrupts:
> > + minItems: 3
> > + maxItems: 3
> > + interrupt-names:
> > + items:
> > + - const: gpio0
> > + - const: gpio1
> > + - const: gpio_mux
> > + - if:
> > + properties:
> > + compatible:
> > + contains:
> > + enum:
> > + - marvell,mmp-gpio
> > + - marvell,mmp2-gpio
>
> I'd make this an else clause.
This would imply that marvell,pxa93x-gpio has one interrupt and it is
not know whether that is actually the case.
> > + then:
> > + properties:
> > + interrupts:
> > + maxItems: 1
> > + interrupt-names:
> > + items:
> > + - const: gpio_mux
> > +
> > +properties:
> > + $nodename:
> > + pattern: '^gpio@[0-9a-f]+$'
> > +
> > + compatible:
> > + enum:
> > + - intel,pxa25x-gpio
> > + - intel,pxa26x-gpio
> > + - intel,pxa27x-gpio
> > + - intel,pxa3xx-gpio
> > + - marvell,mmp-gpio
> > + - marvell,mmp2-gpio
> > + - marvell,pxa93x-gpio
> > + - marvell,pxa1928-gpio
> > +
> > + reg:
> > + maxItems: 1
> > +
> > + clocks:
> > + maxItems: 1
> > +
> > + resets:
> > + maxItems: 1
> > +
> > + ranges: true
> > +
> > + '#address-cells':
> > + const: 1
> > +
> > + '#size-cells':
> > + const: 1
> > +
> > + gpio-controller: true
> > +
> > + '#gpio-cells':
> > + const: 2
> > +
> > + gpio-ranges:
> > + maxItems: 1
>
> My pxa1928 dts has this in the child nodes. Sure this is right?
I actually don't know anything about pxa1928. pxa27x.dtsi and
pxa3xx.dtsi specify gpio-ranges on main gpio nodes and have no gcb
child nodes.
> > +
> > + interrupts: true
> > +
> > + interrupt-names: true
> > +
> > + interrupt-controller: true
> > +
> > + '#interrupt-cells':
> > + const: 2
> > +
> > +patternProperties:
> > + '^gpio@[0-9a-f]*$':
> > + type: object
> > + properties:
> > + reg:
> > + maxItems: 1
> > +
> > + required:
> > + - reg
> > +
> > + additionalProperties: false
> > +
> > +required:
> > + - compatible
> > + - '#address-cells'
> > + - '#size-cells'
> > + - reg
> > + - gpio-controller
> > + - '#gpio-cells'
> > + - interrupts
> > + - interrupt-names
> > + - interrupt-controller
> > + - '#interrupt-cells'
> > + - ranges
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > + - |
> > + #include <dt-bindings/clock/pxa-clock.h>
> > + gpio@40e00000 {
> > + compatible = "intel,pxa3xx-gpio";
> > + #address-cells = <1>;
> > + #size-cells = <1>;
> > + reg = <0x40e00000 0x10000>;
> > + gpio-controller;
> > + #gpio-cells = <2>;
> > + interrupts = <8>, <9>, <10>;
> > + interrupt-names = "gpio0", "gpio1", "gpio_mux";
> > + clocks = <&clks CLK_GPIO>;
> > + interrupt-controller;
> > + #interrupt-cells = <2>;
> > + ranges;
>
> No child, so there should be no ranges here.
Will fix in next version.
> > + };
> > + - |
> > + #include <dt-bindings/clock/marvell,pxa910.h>
> > + gpio@d4019000 {
> > + compatible = "marvell,mmp-gpio";
> > + #address-cells = <1>;
> > + #size-cells = <1>;
> > + reg = <0xd4019000 0x1000>;
> > + gpio-controller;
> > + #gpio-cells = <2>;
> > + interrupts = <49>;
> > + interrupt-names = "gpio_mux";
> > + clocks = <&soc_clocks PXA910_CLK_GPIO>;
> > + resets = <&soc_clocks PXA910_CLK_GPIO>;
> > + interrupt-controller;
> > + #interrupt-cells = <2>;
> > + ranges;
> > +
> > + gpio@d4019000 {
> > + reg = <0xd4019000 0x4>;
> > + };
> > +
> > + gpio@d4019004 {
> > + reg = <0xd4019004 0x4>;
> > + };
> > +
> > + gpio@d4019008 {
> > + reg = <0xd4019008 0x4>;
> > + };
> > +
> > + gpio@d4019100 {
> > + reg = <0xd4019100 0x4>;
> > + };
> > + };
> > +
> > +...
> > --
> > 2.25.1
> >
