From: Rob Herring <robh@kernel.org>
To: Jacopo Mondi <jacopo@jmondi.org>
Cc: "Niklas Söderlund" <niklas.soderlund+renesas@ragnatech.se>,
devicetree@vger.kernel.org, linux-media@vger.kernel.org,
linux-renesas-soc@vger.kernel.org
Subject: Re: [PATCH 3/3] dt-bindings: rcar-vin: Convert bindings to json-schema
Date: Thu, 14 Nov 2019 08:15:05 -0600 [thread overview]
Message-ID: <20191114141505.GA22574@bogus> (raw)
In-Reply-To: <20191112094231.ssi2qaksvn7djxmq@uno.localdomain>
On Tue, Nov 12, 2019 at 10:42:31AM +0100, Jacopo Mondi wrote:
> Hi Niklas,
> thanks for the patch
>
> On Fri, Nov 08, 2019 at 03:16:09AM +0100, Niklas Söderlund wrote:
> > Convert Renesas R-Car VIN bindings documentation to json-schema.
> >
> > As the examples in the bindings now can be tested add a new one which
> > describes how the both a parallel and a CSI-2 source can be connected on
> > Gen3 SoCs.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > ---
> > .../devicetree/bindings/media/renesas,vin.txt | 217 ----------
> > .../bindings/media/renesas,vin.yaml | 409 ++++++++++++++++++
> > 2 files changed, 409 insertions(+), 217 deletions(-)
> > delete mode 100644 Documentation/devicetree/bindings/media/renesas,vin.txt
> > create mode 100644 Documentation/devicetree/bindings/media/renesas,vin.yaml
> > diff --git a/Documentation/devicetree/bindings/media/renesas,vin.yaml b/Documentation/devicetree/bindings/media/renesas,vin.yaml
> > new file mode 100644
> > index 0000000000000000..be261d9a98d62dcd
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/renesas,vin.yaml
> > @@ -0,0 +1,409 @@
> > +# SPDX-License-Identifier: GPL-2.0
>
> I think you should use GPL-2.0-only
>
> > +# Copyright (C) 2019 Renesas Electronics Corp.
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/media/renesas,vin.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Renesas R-Car Video Input (VIN)
> > +
> > +maintainers:
> > + - Niklas Söderlund <niklas.soderlund@ragnatech.se>
>
> I also put the mailing lists here, not sure if that's desired.
Not really. A Renesas list is fine, but no need for linux-media as the
file path will pick that up.
> > +
> > +description: |-
> > + The R-Car Video Input (VIN) device provides video input capabilities for the
> > + Renesas R-Car family of devices.
> > +
> > + Each VIN instance has a single parallel input that supports RGB and YUV video,
> > + with both external synchronization and BT.656 synchronization for the latter.
> > + Depending on the instance the VIN input is connected to external SoC pins, or
> > + on Gen3 and RZ/G2 platforms to a CSI-2 receiver.
> > +
> > +properties:
> > + compatible:
> > + oneOf:
> > + - items:
> > + - enum:
> > + - renesas,vin-r8a7743 # RZ/G1M
> > + - renesas,vin-r8a7744 # RZ/G1N
> > + - renesas,vin-r8a7745 # RZ/G1E
> > + - renesas,vin-r8a77470 # RZ/G1C
> > + - renesas,vin-r8a7790 # R-Car H2
> > + - renesas,vin-r8a7791 # R-Car M2-W
> > + - renesas,vin-r8a7792 # R-Car V2H
> > + - renesas,vin-r8a7793 # R-Car M2-N
> > + - renesas,vin-r8a7794 # R-Car E2
> > + - const: renesas,rcar-gen2-vin # Generic R-Car Gen2 or RZ/G1
> > +
> > + - items:
> > + - enum:
> > + - renesas,vin-r8a774a1 # RZ/G2M
> > + - renesas,vin-r8a774b1 # RZ/G2N
> > + - renesas,vin-r8a774c0 # RZ/G2E
> > + - renesas,vin-r8a7778 # R-Car M1
> > + - renesas,vin-r8a7779 # R-Car H1
> > + - renesas,vin-r8a7795 # R-Car H3
> > + - renesas,vin-r8a7796 # R-Car M3-W
> > + - renesas,vin-r8a77965 # R-Car M3-N
> > + - renesas,vin-r8a77970 # R-Car V3M
> > + - renesas,vin-r8a77980 # R-Car V3H
> > + - renesas,vin-r8a77990 # R-Car E3
> > + - renesas,vin-r8a77995 # R-Car D3
> > +
> > + reg:
> > + maxItems: 1
> > +
> > + interrupts:
> > + maxItems: 1
> > +
> > + clocks:
> > + maxItems: 1
> > +
> > + power-domains:
> > + maxItems: 1
> > +
> > + resets:
> > + maxItems: 1
> > +
> > + #The per-board settings for Gen2 and RZ/G1 platforms:
> > + port:
> > + type: object
> > + description: |-
> > + A node containing a parallel input with a singel endpoint definitions as
> > + documented in
> > + Documentation/devicetree/bindings/media/video-interfaces.txt
> > +
> > + properties:
> > + reg:
> > + const: 1
>
> Do you need reg for the single 'port' ?
>
> > +
> > + endpoint:
> > + type: object
> > +
> > + properties:
> > + hsync-active:
> > + description: |-
> > + Default is active high. If both HSYNC and VSYNC polarities are not
> > + specified, embedded synchronization is selected.
> > + enum: [ 0, 1 ]
> > +
> > + vsync-active:
> > + description: |-
> > + Default is active high. If both HSYNC and VSYNC polarities are not
> > + specified, embedded synchronization is selected.
> > + enum: [ 0, 1 ]
> > +
> > + field-active-even:
> > + description: Default is active high.
> > + enum: [ 0, 1 ]
> > +
> > + bus-width:
> > + enum: [ 8, 10, 12, 16, 24, 32 ]
> > +
> > + data-shift:
> > + enum: [ 0, 8 ]
> > +
> > + data-enable-active:
> > + description: Polarity of CLKENB signal, default is active high.
> > + enum: [ 0, 1 ]
> > +
> > + pclk-sample:
> > + enum: [ 0, 1 ]
> > +
> > + data-active:
> > + enum: [ 0, 1 ]
> > +
> > + remote-endpoint:
> > + maxItems: 1
> > +
> > + required:
> > + - remote-endpoint
> > +
> > + additionalProperties: false
> > +
> > + additionalProperties: false
> > +
> > + #The per-board settings for Gen3 and RZ/G2 platforms:
> > + renesas,id:
> > + description: VIN channel number
> > + allOf:
> > + - $ref: /schemas/types.yaml#/definitions/uint32
> > + - enum: [ 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15 ]
> > +
> > + ports:
> > + type: object
> > + description: |-
> > + A node containing input nodes with endpoint definitions as documented in
> > + Documentation/devicetree/bindings/media/video-interfaces.txt
> > +
> > + properties:
> > + port@0:
> > + type: object
> > + description: |-
> > + Input port node, single endpoint describing a parallel input source.
> > +
> > + properties:
> > + reg:
> > + const: 0
> > +
> > + endpoint:
> > + type: object
> > +
> > + properties:
> > + hsync-active:
> > + description: |-
Drop the literal block (|-).
> > + Default is active high. If both HSYNC and VSYNC polarities are not
> > + specified, embedded synchronization is selected.
Express defaults as a schema:
default: 1
> > + enum: [ 0, 1 ]
Unless a subset of possible values apply, assume this is defined
elsewhere (hopefully someone converts video-interfaces.txt to schema
soon).
> > +
> > + vsync-active:
> > + description: |-
> > + Default is active high. If both HSYNC and VSYNC polarities are not
> > + specified, embedded synchronization is selected.
> > + enum: [ 0, 1 ]
> > +
> > + field-active-even:
> > + description: Default is active high.
> > + enum: [ 0, 1 ]
> > +
> > + bus-width:
> > + enum: [ 8, 10, 12, 16, 24, 32 ]
> > +
> > + data-shift:
> > + enum: [ 0, 8 ]
>
> Why are those two not described ? I feel like or you either refrain
> from describing all poperties and point the reader to
> video-interfaces.txt, or you describe all of them, at least to provide
> an indication of the default if the property is not specified.
>
> > +
> > + data-enable-active:
> > + description: Polarity of CLKENB signal, default is active high.
> > + enum: [ 0, 1 ]
> > +
> > + pclk-sample:
> > + enum: [ 0, 1 ]
> > +
> > + data-active:
> > + enum: [ 0, 1 ]
> > +
> > + remote-endpoint:
> > + maxItems: 1
Just 'true' is enough here. Assume it's defined as a phandle elsewhere.
> > +
> > + required:
> > + - remote-endpoint
> > +
> > + additionalProperties: false
> > +
> > + additionalProperties: false
> > +
> > + port@1:
> > + type: object
> > + description: |-
> > + Input port node, multiple endpoints describing all the R-Car CSI-2
> > + modules connected the VIN.
> > +
> > + properties:
> > + '#address-cells':
> > + const: 1
> > +
> > + '#size-cells':
> > + const: 0
> > +
> > + reg:
> > + const: 1
> > +
> > + endpoint@0:
> > + type: object
> > + description: Endpoint connected to CSI20.
> > +
> > + properties:
> > + reg:
> > + const: 0
> > +
> > + remote-endpoint:
> > + maxItems: 1
> > +
> > + required:
> > + - reg
> > + - remote-endpoint
> > +
> > + additionalProperties: false
> > +
> > + endpoint@1:
> > + type: object
> > + description: Endpoint connected to CSI21.
> > +
> > + properties:
> > + reg:
> > + const: 1
> > +
> > + remote-endpoint:
> > + maxItems: 1
> > +
> > + required:
> > + - reg
> > + - remote-endpoint
> > +
> > + additionalProperties: false
> > +
> > + endpoint@2:
> > + type: object
> > + description: Endpoint connected to CSI40.
> > +
> > + properties:
> > + reg:
> > + const: 2
> > +
> > + remote-endpoint:
> > + maxItems: 1
> > +
> > + required:
> > + - reg
> > + - remote-endpoint
> > +
> > + additionalProperties: false
> > +
> > + endpoint@3:
> > + type: object
> > + description: Endpoint connected to CSI41.
> > +
> > + properties:
> > + reg:
> > + const: 3
> > +
> > + remote-endpoint:
> > + maxItems: 1
> > +
> > + required:
> > + - reg
> > + - remote-endpoint
>
> Here and in the other endpoints of port@1, do you need required
> properties ? As I read in writing-schema.rts:
> "Unless noted otherwise, all properties are required."
That is referring to schema properties ($id, properties, title, etc.),
not binding properties. That's also just the top-level
properties/keywords. If that's confusing, try reading the meta-schemas
which has schema keywords as both properties and schema.
> > +
> > + additionalProperties: false
> > +
> > + additionalProperties: false
> > +
> > +required:
> > + - compatible
> > + - reg
> > + - interrupts
> > + - clocks
> > + - power-domains
> > + - resets
> > +
> > +if:
> > + properties:
> > + compatible:
> > + contains:
> > + anyOf:
> > + - const: renesas,vin-r8a7778
> > + - const: renesas,vin-r8a7779
> > + - const: renesas,rcar-gen2-vin
> > +then:
> > + required:
> > + - port
> > +else:
> > + required:
> > + - renesas,id
> > + - ports
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > + # Device node example for Gen2 platform
> > + - |
> > + #include <dt-bindings/clock/r8a7790-cpg-mssr.h>
> > + #include <dt-bindings/interrupt-controller/arm-gic.h>
> > + #include <dt-bindings/power/r8a7790-sysc.h>
> > +
> > + vin1: vin@e6ef1000 {
> > + compatible = "renesas,vin-r8a7790",
> > + "renesas,rcar-gen2-vin";
> > + reg = <0 0xe6ef1000 0 0x1000>;
> > + interrupts = <GIC_SPI 189 IRQ_TYPE_LEVEL_HIGH>;
> > + clocks = <&cpg CPG_MOD 810>;
> > + power-domains = <&sysc R8A7790_PD_ALWAYS_ON>;
> > + resets = <&cpg 810>;
> > +
> > + port {
> > + vin1ep0: endpoint {
> > + remote-endpoint = <&adv7180>;
> > + bus-width = <8>;
> > + };
> > + };
> > + };
> > +
> > + # Device node example for Gen3 platform with only CSI-2
> > + - |
> > + #include <dt-bindings/clock/r8a7795-cpg-mssr.h>
> > + #include <dt-bindings/interrupt-controller/arm-gic.h>
> > + #include <dt-bindings/power/r8a7795-sysc.h>
> > +
> > + vin0: video@e6ef0000 {
> > + compatible = "renesas,vin-r8a7795";
> > + reg = <0 0xe6ef0000 0 0x1000>;
> > + interrupts = <GIC_SPI 188 IRQ_TYPE_LEVEL_HIGH>;
> > + clocks = <&cpg CPG_MOD 811>;
> > + power-domains = <&sysc R8A7795_PD_ALWAYS_ON>;
> > + resets = <&cpg 811>;
> > + renesas,id = <0>;
> > +
> > + ports {
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + port@1 {
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + reg = <1>;
> > +
> > + vin0csi20: endpoint@0 {
> > + reg = <0>;
> > + remote-endpoint= <&csi20vin0>;
> > + };
> > + vin0csi40: endpoint@2 {
> > + reg = <2>;
> > + remote-endpoint= <&csi40vin0>;
> > + };
> > + };
> > + };
> > + };
> > +
> > + # Device node example for Gen3 platform with CSI-2 and parallel
> > + - |
> > + #include <dt-bindings/clock/r8a77970-cpg-mssr.h>
> > + #include <dt-bindings/interrupt-controller/arm-gic.h>
> > + #include <dt-bindings/power/r8a77970-sysc.h>
> > +
> > + vin2: video@e6ef2000 {
> > + compatible = "renesas,vin-r8a77970";
> > + reg = <0 0xe6ef2000 0 0x1000>;
> > + interrupts = <GIC_SPI 190 IRQ_TYPE_LEVEL_HIGH>;
> > + clocks = <&cpg CPG_MOD 809>;
> > + power-domains = <&sysc R8A77970_PD_ALWAYS_ON>;
> > + resets = <&cpg 809>;
> > + renesas,id = <2>;
> > +
> > + ports {
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + port@0 {
> > + reg = <0>;
> > +
> > + vin2_in: endpoint {
> > + remote-endpoint = <&adv7612_out>;
> > + hsync-active = <0>;
> > + vsync-active = <0>;
> > + };
> > + };
> > +
> > + port@1 {
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + reg = <1>;
> > +
> > + vin2csi40: endpoint@2 {
> > + reg = <2>;
> > + remote-endpoint = <&csi40vin2>;
> > + };
> > + };
> > + };
> > + };
>
> I can't really comment on the json-schema most complex parts, but from
> a device point of view, minors apart, it seems sane to me.
>
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
>
> Thanks
> j
>
> > --
> > 2.23.0
> >
prev parent reply other threads:[~2019-11-14 14:15 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-11-08 2:16 [PATCH 0/3] dt-bindings: rcar-vin: Convert bindings to json-schema Niklas Söderlund
2019-11-08 2:16 ` [PATCH 1/3] dt-bindings: rcar-vin: Remove paragraph about aliases Niklas Söderlund
2019-11-08 9:26 ` Geert Uytterhoeven
2019-11-08 2:16 ` [PATCH 2/3] dt-bindings: rcar-vin: Document compatibility string for R8A77470 Niklas Söderlund
2019-11-08 9:33 ` Geert Uytterhoeven
2019-11-08 2:16 ` [PATCH 3/3] dt-bindings: rcar-vin: Convert bindings to json-schema Niklas Söderlund
2019-11-10 10:46 ` Hans Verkuil
2019-11-12 9:42 ` Jacopo Mondi
2019-11-14 14:15 ` Rob Herring [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20191114141505.GA22574@bogus \
--to=robh@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=jacopo@jmondi.org \
--cc=linux-media@vger.kernel.org \
--cc=linux-renesas-soc@vger.kernel.org \
--cc=niklas.soderlund+renesas@ragnatech.se \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.