Hi Vishal,

Thank you for the patch.

On Mon, Jul 13, 2020 at 12:54:47PM -0600, Rob Herring wrote:

On Thu, Jun 18, 2020 at 11:03:03AM +0530, Vishal Sagar wrote:

quoted

Add bindings documentation for Xilinx UHD-SDI Receiver Subsystem.

The Xilinx UHD-SDI Receiver Subsystem consists of SMPTE UHD-SDI (RX) IP
core, an SDI RX to Video Bridge IP core to convert SDI video to native
video and a Video In to AXI4-Stream IP core to convert native video to
AXI4-Stream.

Signed-off-by: Vishal Sagar <redacted>
---
v3
- bpc instead of bpp
- removed bpc as required property (default to 10 bpc)
- add dt-bindings/media/xilinx-sdi.h
- made line-rate as u32 instead of string
- fixed reg
- fixed s/upto/up to/

v2
- Removed references to xlnx,video*
- Fixed as per Sakari Ailus and Rob Herring's comments
- Converted to yaml format

 .../bindings/media/xilinx/xlnx,sdirxss.yaml   | 132 ++++++++++++++++++
 include/dt-bindings/media/xilinx-sdi.h        |  20 +++
 2 files changed, 152 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/xilinx/xlnx,sdirxss.yaml
 create mode 100644 include/dt-bindings/media/xilinx-sdi.h

diff --git a/Documentation/devicetree/bindings/media/xilinx/xlnx,sdirxss.yaml b/Documentation/devicetree/bindings/media/xilinx/xlnx,sdirxss.yaml
new file mode 100644
index 000000000000..6cfc18ca435f
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/xilinx/xlnx,sdirxss.yaml

@@ -0,0 +1,132 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/media/xilinx/xlnx,sdirxss.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+

I think a single blank line is enough.

quoted

+title: Xilinx SMPTE UHD-SDI Receiver Subsystem
+
+maintainers:
+  - Vishal Sagar [off-list ref]
+
+description: |
+  The SMPTE UHD-SDI Receiver (RX) Subsystem allows you to quickly create systems
+  based on SMPTE SDI protocols. It receives unaligned native SDI streams from
+  the SDI GT PHY and outputs an AXI4-Stream video stream, native video, or
+  native SDI using Xilinx transceivers as the physical layer.
+
+  The subsystem consists of
+  1 - SMPTE UHD-SDI Rx
+  2 - SDI Rx to Native Video Bridge
+  3 - Video In to AXI4-Stream Bridge
+
+  The subsystem can capture SDI streams in up to 12G mode 8 data streams and output
+  a dual pixel per clock RGB/YUV444,422/420 10/12 bits per component AXI4-Stream.
+
+properties:
+  compatible:
+    items:
+      - enum:
+        - xlnx,v-smpte-uhdsdi-rx-ss-2.0

Should be indented 2 more spaces.

Or you could simply use

properties:
  compatible:
    const: xlnx,v-smpte-uhdsdi-rx-ss-2.0

quoted

+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  clocks:
+    description: List of clock specifiers

Drop. That's every 'clocks' property.

quoted

+    items:
+      - description: AXI4-Lite clock
+      - description: SMPTE UHD-SDI Rx core clock
+      - description: Video clock
+
+  clock-names:
+    items:
+      - const: s_axi_aclk
+      - const: sdi_rx_clk
+      - const: video_out_clk
+
+  xlnx,bpc:
+    description: Bits per component supported. Can be 10 or 12 bits per component only.
+    allOf:

You can drop the 'allOf' now.

quoted

+      - $ref: "/schemas/types.yaml#/definitions/uint32"
+      - enum: [10, 12]

Seems like this should be a standard property?

Rob, if my understanding is correct, this tells for how many bits per
component the IP core has been synthesized. I think it qualifies as a
vendor property, as how to express constraints on supported formats (for
IP cores that can be synthesized with different options) is highly
vendor-specific.

Vishal, I think the question I asked in the review of v2 fell through
the cracks. Is the documentation for the new IP core version available ?
Should this property only be allowed for the new version, given that in
v2.0 the BPC is fixed to 10 ?

quoted

+
+  xlnx,line-rate:
+    description: |
+      The maximum mode supported by the design. Possible values are as below
+      0 - XSDI_STD_3G      -  3G mode
+      1 - XSDI_STD_6G      -  6G mode
+      2 - XSDI_STD_12G_8DS - 12G mode with 8 data streams
+    allOf:
+      - $ref: "/schemas/types.yaml#/definitions/uint32"
+      - enum: [0, 1, 2]

Standard?

For this one, I'm not sure. There's little support for SDI in the
kernel, and I'm sure we'll get this wrong the first time. I'd rather try
not to over-standardize properties before we have more examples.

quoted

+
+  xlnx,include-edh:
+    type: boolean
+    description: |
+      This is present when the Error Detection and Handling processor is
+      enabled in design.
+
+  ports:
+    type: object
+    description: |
+      Generally the SDI port is connected to a device like SDI Broadcast camera
+      which is independently controlled. Hence port@0 is a source port which can be
+      connected to downstream IP which can work with AXI4 Stream data.
+    properties:
+      port@0:
+        type: object
+        description: Source port
+        properties:
+          reg:
+            const: 0
+          endpoint:
+            type: object
+            properties:
+              remote-endpoint: true
+            required:
+              - remote-endpoint
+            additionalProperties: false
+        additionalProperties: false

Same here, I explained in the review of v2 that we should have an input
port.

quoted

+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - clocks
+  - clock-names
+  - xlnx,line-rate
+  - ports
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/media/xilinx-sdi.h>
+    uhdsdirxss: v-smpte-uhdsdi-rxss@80000000 {

The label is not used, you can drop it.

quoted

+      compatible = "xlnx,v-smpte-uhdsdi-rx-ss-2.0";
+      interrupt-parent = <&gic>;
+      interrupts = <0 89 4>;
+      reg = <0x80000000 0x10000>;
+      xlnx,include-edh;
+      xlnx,line-rate = <XSDI_STD_12G_8DS>;
+      clocks = <&clk_1>, <&si570_1>, <&clk_2>;
+      clock-names = "s_axi_aclk", "sdi_rx_clk", "video_out_clk";
+      xlnx,bpc = <10>;

I would group the xlnx,* properties after the standard properties.

quoted

+
+      ports {
+        #address-cells = <1>;
+        #size-cells = <0>;
+        port@0 {
+          reg = <0>;
+          sdirx_out: endpoint {
+            remote-endpoint = <&vcap_sdirx_in>;
+          };
+        };
+      };
+    };
+...

diff --git a/include/dt-bindings/media/xilinx-sdi.h b/include/dt-bindings/media/xilinx-sdi.h
new file mode 100644
index 000000000000..11938fade041
--- /dev/null
+++ b/include/dt-bindings/media/xilinx-sdi.h

@@ -0,0 +1,20 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Xilinx SDI device tree bindings
+ *
+ * Copyright (C) 2020 Xilinx, Inc.
+ *
+ * Contacts: Vishal Sagar <vishal.sagar@xilinx.com>
+ */
+
+#ifndef __DT_BINDINGS_MEDIA_XILINX_SDI_H__
+#define __DT_BINDINGS_MEDIA_XILINX_SDI_H__
+
+/*
+ * SDI Configurations
+ */
+#define XSDI_STD_3G		0
+#define XSDI_STD_6G		1
+#define XSDI_STD_12G_8DS	2
+
+#endif /* __DT_BINDINGS_MEDIA_XILINX_SDI_H__ */

-- 
Regards,

Laurent Pinchart

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel