Re: [PATCH V2 3/4] dt-bindings: media: ti: vpe: Add bindings for Video Input Port

On 16/07/2025 13:19, Yemike Abhilash Chandra wrote:

From: Dale Farnsworth <redacted>

Add device tree bindings for the Video Input Port. Video Input Port (VIP)
can be found on devices such as DRA7xx and provides a parallel interface
to a video source such as a sensor or TV decoder.

Signed-off-by: Dale Farnsworth <redacted>
Signed-off-by: Benoit Parrot <redacted>
Signed-off-by: Sukrut Bellary <redacted>
Signed-off-by: Yemike Abhilash Chandra <y-abhilashchandra@ti.com>
---
Changelog:
Changes in v2:
- Remove array and just use hsync: true in bindings
- Remove array and use enum for bus width in bindings
- Use pattern properties since properties across ports are same
- Update copyright year

This fails testing so limited review follows.

Why do you need it? How many instances are there per SoC?

+
+  reg:
+    items:
+      - description: The VIP main register region
+      - description: Video Data Parser (PARSER) register region for Slice0
+      - description: Color Space Conversion (CSC) register region for Slice0
+      - description: Scaler (SC) register region for Slice0
+      - description: Video Data Parser (PARSER) register region for Slice1
+      - description: Color Space Conversion (CSC) register region for Slice1
+      - description: Scaler (SC) register region for Slice1
+      - description: Video Port Direct Memory Access (VPDMA) register region
+
+  reg-names:
+    items:
+      - const: vip
+      - const: parser0
+      - const: csc0
+      - const: sc0
+      - const: parser1
+      - const: csc1
+      - const: sc1
+      - const: vpdma
+
+  interrupts:
+    minItems: 2

Which makes 10 interrupts valid :/

+    description:
+      IRQ index 0 is used for Slice0 interrupts
+      IRQ index 1 is used for Slice1 interrupts

No, list the items with description, just like most bindings.

+
+  ti,vip-clk-polarity:
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+    description:
+      phandle to the device control module. The 1st argument should

Polarity is not a phandle... The name is confusing, especially that
nothing in description matches it. Explain what is the purpose of this
in THIS device. For what this device needs it?

+      contain the register offset to the CTRL_CORE_SMA_SW_1 register.
+      2nd argument contains the bit field to slice 0 port A,
+      3rd argument contains the bit field to slice 0 port B,
+      4th argument contains the bit field to slice 1 port A,
+      5th argument contains the bit field to slice 1 port B.

Don't open code schema in any case. Look at other examples how such this
is encoded.

+
+patternProperties:
+  '^port@[0-3]$':
+    type: object
+    additionalProperties: false

Missing ref. From which binding did you take port without ref?

What are these ports? Look below:

+
+    properties:
+      "#address-cells":
+        const: 1
+
+      "#size-cells":
+        const: 0
+      reg:
+        description: The port number

You add redundant, obvious descriptions, but you do not add description
what each port is.

+        maxItems: 1

Order this according to DTS coding style. See other bindings.

+
+      label:
+        description: Port name. Usually the pin group name

Why do you have labels everywhere?

+
+      endpoint:
+        $ref: /schemas/graph.yaml#/properties/endpoint
+        type: object
+        additionalProperties: false
+
+        properties:
+          hsync-active: true
+          vsync-active: true
+          pclk-sample: true

Where are definitions of above?

+          bus-width:
+            enum: [8, 24]
+            default: 8
+
+          remote-endpoint: true
+
+    required:
+      - "#address-cells"
+      - "#size-cells"
+      - port@0
+      - reg
+      - label

So port@0 is next to label, yes? Let's go to your example

+
+required:
+  - compatible
+  - label
+  - reg
+  - reg-names
+  - interrupts
+  - ti,vip-clk-polarity
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+    vip1: vip@48970000 {

Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

+        compatible = "ti,dra7-vip1";
+        label = "vip1";
+        reg = <0x48970000 0x114>,
+              <0x48975500 0xD8>,
+              <0x48975700 0x18>,
+              <0x48975800 0x80>,
+              <0x48975a00 0xD8>,
+              <0x48975c00 0x18>,
+              <0x48975d00 0x80>,
+              <0x4897d000 0x400>;
+        reg-names = "vip",
+                    "parser0",
+                    "csc0",
+                    "sc0",
+                    "parser1",
+                    "csc1",
+                    "sc1",
+                    "vpdma";
+        interrupts = <GIC_SPI 351 IRQ_TYPE_LEVEL_HIGH>,
+                     <GIC_SPI 392 IRQ_TYPE_LEVEL_HIGH>;
+        ti,vip-clk-polarity = <&scm_conf 0x534 0x1 0x4 0x2 0x8>;
+
+        ports {
+              #address-cells = <1>;
+              #size-cells = <0>;
+

Where is the label? It is supposed to be required just like port@0 is.

+              vin1a: port@0 {

+                    reg = <0>;
+                    label = "vin1a";
+
+                    vin1a_ep: endpoint {
+                           remote-endpoint = <&camera1>;
+                           hsync-active = <1>;
+                           vsync-active = <1>;
+                           pclk-sample = <0>;
+                           bus-width = <8>;
+                    };
+              };
+              vin1b: port@1 {
+                    reg = <1>;
+                    label = "vin1b";
+              };
+              vin2a: port@2 {
+                    reg = <2>;
+                    label = "vin2a";
+              };
+              vin2b: port@3 {
+                    reg = <3>;
+                    label = "vin2b";
+              };
+         };
+    };
+
+    i2c {

Drop entire node, not relevant.


Best regards,
Krzysztof