On Mon, Mar 23, 2020 at 8:38 AM Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote: > > Hi Rob, > > Thanks a lot for your comments! > > On Thu, Mar 12, 2020 at 9:05 PM Rob Herring <robh@xxxxxxxxxx> wrote: > > On Fri, Mar 06, 2020 at 10:07:20AM +0100, Geert Uytterhoeven wrote: > > > Convert the Renesas Serial Communication Interface ((H)SCI(F)(A|B)) > > > Device Tree binding documentation to json-schema. > > > > > > Split the bindings in 5 files, one per major type, to ease expressing > > > constraints. > > > > > > Signed-off-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx> > > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/serial/renesas,sci.yaml > > > > +properties: > > > + compatible: > > > + oneOf: > > > + - items: > > > + - const: renesas,sci > > > > Do you plan to add to this? It can be simplified to just > > Not really. I just used the same construct for consistency with the other > SCI variants. > > > 'const: renesas,sci'. > > OK. I guess no other H8 and SuperH variants will pop up anytime soon. > Oops, RZ/A1 has SCI, and RZ/A2 has SCIg (they keep on inventing new > names, to be seen how compatible). > > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/serial/renesas,scif.yaml > > > @@ -0,0 +1,168 @@ > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > > +%YAML 1.2 > > > +--- > > > +$id: "http://devicetree.org/schemas/serial/renesas,scif.yaml#"; > > > +$schema: "http://devicetree.org/meta-schemas/core.yaml#"; > > > + > > > +title: Renesas Serial Communication Interface with FIFO (SCIF) > > > + > > > +maintainers: > > > + - Geert Uytterhoeven <geert+renesas@xxxxxxxxx> > > > + > > > +description: > > > + Each enabled UART may have an optional "serialN" alias in the "aliases" node, > > > + where N is the port number (non-negative decimal integer) as printed on the > > > + label next to the physical port. > > > > That's every serial port... > > So you suggest to just remove this paragraph from all files? Yes. > Shall I add it to serial.yaml instead? Sure. I'd really like to get to having a registry of alias names we can check. > > > > + interrupts: > > > + description: | > > > + Must contain one or more interrupt-specifiers for the serial interface. > > > + If a single interrupt is expressed, then all events are > > > + multiplexed into this single interrupt. > > > + > > > + If multiple interrupts are provided by the hardware, the order > > > + in which the interrupts are listed must match order below. Note > > > + that some HW interrupt events may be muxed together resulting > > > + in duplicate entries. > > > + minItems: 1 > > > + maxItems: 6 > > > > This allows 2, 3, 4, or 5 interrupts. Is that valid? If not, then you > > 1, 4, and 6 are valid. > > > should do something like this: > > > > oneOf: > > - items: > > description: A combined interrupt > > - items: > > - description: Error interrupt > > - ... > > So I tried: > > interrupts: > oneOf: > - items: > description: A combined interrupt You're missing a '-' here. > - items: > - description: Error interrupt > - description: Receive buffer full interrupt > - description: Transmit buffer empty interrupt > - description: Transmit End interrupt > - items: > - description: Error interrupt > - description: Receive buffer full interrupt > - description: Transmit buffer empty interrupt > - description: Break interrupt > - description: Data Ready interrupt > - description: Transmit End interrupt > > That fails for devices with 4 or 6 interrupts, e.g.: > > arch/arm/boot/dts/r7s9210-rza2mevb.dt.yaml: serial@e8007000: > interrupts: [[0, 265, 4], [0, 266, 4], [0, 267, 4], [0, 265, 4], [0, > 268, 4], [0, 268, 4]] is valid under each of {'additionalItems': > False, 'items': [{}, {}, {}, {}, {}, {}], 'maxItems': 6, 'minItems': > 6, 'type': 'array'}, {'items': {}, 'type': 'array'} > > Note that initially I forgot to cater for the 4-interrupt case used in > arch/arm/boot/dts/r7s72100.dtsi, and "make dtbs_check" did not complain. > > > > + > > > + interrupt-names: > > > + minItems: 1 > > > + maxItems: 6 > > > + items: > > > + enum: > > > + - eri # Error > > > + - rxi # Receive buffer full > > > + - txi # Transmit buffer empty > > > + - bri # Break > > > + - dri # Data Ready > > > + - tei # Transmit End > > > > Based on above, you probably want 'items' to be a list, not a > > dict(schema). > > Like > > interrupt-names: > oneOf: > - items: > - const: eri > - const: rxi > - const: txi > - const: tei > - items: > - const: eri > - const: rxi > - const: txi > - const: bri > - const: dri > - const: tei > > ? > Seems to work, but needs the duplication as the 4-interrupt case is not > just the 4 first entries of the 6-interrupt case (tei is always last). Yes, that looks right. No name for the single irq case? > > > > + > > > + clocks: > > > + minItems: 1 > > > + maxItems: 4 > > > + > > > + clock-names: > > > + minItems: 1 > > > + maxItems: 4 > > > + items: > > > + enum: > > > + - fck # UART functional clock > > > + - sck # optional external clock input > > > + - brg_int # optional internal clock source for BRG frequency divider > > > + - scif_clk # optional external clock source for BRG frequency divider > > > > Same issue again... > > The order is not fixed for the three optional clocks, as they may or > may not be wired (for sck and scif_clk), or the BRG may not be present. > Hence unlike for interrupts, I cannot drop the "enum", IIUIC? Well, you could list out every possible combination, but the above is fine I guess. Rob
