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

[Krzysztof Kozlowski <krzk@kernel.org>]

On 16/07/2025 13:19, Yemike Abhilash Chandra wrote:
> From: Dale Farnsworth <dale@farnsworth.org>
> 
> 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 <dale@farnsworth.org>
> Signed-off-by: Benoit Parrot <bparrot@ti.com>
> Signed-off-by: Sukrut Bellary <sbellary@baylibre.com>
> 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.

>  .../devicetree/bindings/media/ti,vip.yaml     | 211 ++++++++++++++++++
>  MAINTAINERS                                   |   1 +
>  2 files changed, 212 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/ti,vip.yaml
> 
> diff --git a/Documentation/devicetree/bindings/media/ti,vip.yaml b/Documentation/devicetree/bindings/media/ti,vip.yaml
> new file mode 100644
> index 000000000000..44091c15a537
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/ti,vip.yaml
> @@ -0,0 +1,211 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +# Copyright (C) 2025 Texas Instruments Incorporated -  http://www.ti.com/
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/media/ti,vip.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Texas Instruments DRA7x VIDEO INPUT PORT (VIP).
> +
> +maintainers:
> +  - Yemike Abhilash Chandra <y-abhilashchandra@ti.com>
> +
> +description: |-
> +  The Video Input Port (VIP) is a key component for image capture
> +  applications. The capture module provides the system interface and the
> +  processing capability to connect parallel image-sensor as well as
> +  BT.656/1120 capable encoder chip to DRA7x device.
> +
> +  Each VIP instance supports 2 independently configurable external video
> +  input capture slices (Slice 0 and Slice 1) each providing up to two video
> +  input ports (Port A and Port B) where Port A can be configured as
> +  24/16/8-bit port and Port B is fixed as 8-bit port.
> +  Here these ports a represented as follows
> +    port@0 -> Slice 0 Port A
> +    port@1 -> Slice 0 Port B
> +    port@2 -> Slice 1 Port A
> +    port@3 -> Slice 1 Port B
> +
> +  Each camera port nodes should contain a 'port' child node with child
> +  'endpoint' node. Please refer to the bindings defined in
> +  Documentation/devicetree/bindings/media/video-interfaces.yaml.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - ti,dra7-vip
> +
> +  label:
> +    description: Instance name

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