[PATCH V2 0/4] Add support for VIP

[PATCH V2 0/4] Add support for VIP

[Yemike Abhilash Chandra]

This patch series add support for the TI VIP video capture engine.
VIP stands for Video Input Port, it can be found on devices such as
DRA7xx and provides a parallel interface to a video source such as
a sensor or TV decoder. 

Each VIP can support two inputs (slices) and a SoC can be configured
with a variable number of VIP's. Each slice can support two ports
each connected to its own sub-device.

The first patch in this series updates the outdated MAINTAINERS entry
for the TI VPE and CAL drivers. The subsequent three patches introduce
support for the TI VIP (Video Input Port) driver.

Link for v1: https://lore.kernel.org/all/20200522225412.29440-1-bparrot@ti.com/
The v1 patch series was posted in the year 2020. This v2 series resumes the
effort to upstream VIP support by addressing all previous review comments

Changelog:
Changes in v2:
- Remove array and just use hsync: true in bindings (Patch 3/5)
- Remove array and use enum for bus width in bindings (Patch 3/5)
- Use pattern properties since properties across ports are same (Patch 3/5)
- Remove vip_dbg, vip_info, vip_err aliases and just use v4l2_dbg, v4l2_info
  and v4l2_err instead (Patch 4/5)
- Remove color space information from vip_formats struct (Patch 4/5)
- Use g_std instead of g_std_output (Patch 4/5)
- Do not touch pix.priv (Patch 4/5)
- Remove all comments with just register values (Patch 4/5)
- Remove support for vidioc_default ioctl (Patch 4/5)
- In case of any error while streaming, push all pending buffers to vb2 (Patch 4/5)
- Address some minor comments made by Hans throughout the driver (Patch 4/5)
- Update copyright year at various places

v4l2-compliance output: https://gist.github.com/Yemike-Abhilash-Chandra/b0791cb465fadc11d4c995197cb22f29

v4l2-compliance cropping and composing tests are failing likely
due to OV10635 sensor supporting several discrete frame sizes,
fail: v4l2-test-formats.cpp(1560): node->frmsizes_count[pixfmt] > 1

Test logs: https://gist.github.com/Yemike-Abhilash-Chandra/98504ab56416aef38b851036aef5eeb1

Dale Farnsworth (2):
  dt-bindings: media: ti: vpe: Add bindings for Video Input Port
  media: ti-vpe: Add the VIP driver

Yemike Abhilash Chandra (2):
  MAINTAINERS: Update maintainers of TI VPE and CAL
  Revert "media: platform: ti: Remove unused vpdma_update_dma_addr"

 .../devicetree/bindings/media/ti,vip.yaml     |  211 +
 MAINTAINERS                                   |    3 +-
 drivers/media/platform/ti/Kconfig             |   13 +
 drivers/media/platform/ti/vpe/Makefile        |    2 +
 drivers/media/platform/ti/vpe/vip.c           | 3824 +++++++++++++++++
 drivers/media/platform/ti/vpe/vip.h           |  719 ++++
 drivers/media/platform/ti/vpe/vpdma.c         |   32 +
 drivers/media/platform/ti/vpe/vpdma.h         |    3 +
 8 files changed, 4806 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/media/ti,vip.yaml
 create mode 100644 drivers/media/platform/ti/vpe/vip.c
 create mode 100644 drivers/media/platform/ti/vpe/vip.h

-- 
2.34.1

[PATCH V2 1/4] MAINTAINERS: Update maintainers of TI VPE and CAL

[Yemike Abhilash Chandra]

Benoit has moved to work in other technical areas, and Yemike Abhilash will
maintain these modules instead of him.

Signed-off-by: Yemike Abhilash Chandra <y-abhilashchandra@ti.com>
---
 MAINTAINERS | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 7b6b13c01ba7..ea4b29c8b6b5 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -25205,7 +25205,7 @@ S:	Maintained
 F:	sound/soc/codecs/twl4030*
 
 TI VPE/CAL DRIVERS
-M:	Benoit Parrot <bparrot@ti.com>
+M:	Yemike Abhilash Chandra <y-abhilashchandra@ti.com>
 L:	linux-media@vger.kernel.org
 S:	Maintained
 W:	http://linuxtv.org/
-- 
2.34.1

[PATCH V2 2/4] Revert "media: platform: ti: Remove unused vpdma_update_dma_addr"

[Yemike Abhilash Chandra]

This reverts commit 9314891df119442a6ec1518b3d872c330e2bf1a1.

We're adding support for TI VIP driver, so this is no longer unused.

Signed-off-by: Yemike Abhilash Chandra <y-abhilashchandra@ti.com>
---
 drivers/media/platform/ti/vpe/vpdma.c | 32 +++++++++++++++++++++++++++
 drivers/media/platform/ti/vpe/vpdma.h |  3 +++
 2 files changed, 35 insertions(+)

diff --git a/drivers/media/platform/ti/vpe/vpdma.c b/drivers/media/platform/ti/vpe/vpdma.c
index bb8a8bd7980c..da90d7f03f82 100644
--- a/drivers/media/platform/ti/vpe/vpdma.c
+++ b/drivers/media/platform/ti/vpe/vpdma.c
@@ -552,6 +552,38 @@ EXPORT_SYMBOL(vpdma_submit_descs);
 
 static void dump_dtd(struct vpdma_dtd *dtd);
 
+void vpdma_update_dma_addr(struct vpdma_data *vpdma,
+	struct vpdma_desc_list *list, dma_addr_t dma_addr,
+	void *write_dtd, int drop, int idx)
+{
+	struct vpdma_dtd *dtd = list->buf.addr;
+	dma_addr_t write_desc_addr;
+	int offset;
+
+	dtd += idx;
+	vpdma_unmap_desc_buf(vpdma, &list->buf);
+
+	dtd->start_addr = dma_addr;
+
+	/* Calculate write address from the offset of write_dtd from start
+	 * of the list->buf
+	 */
+	offset = (void *)write_dtd - list->buf.addr;
+	write_desc_addr = list->buf.dma_addr + offset;
+
+	if (drop)
+		dtd->desc_write_addr = dtd_desc_write_addr(write_desc_addr,
+							   1, 1, 0);
+	else
+		dtd->desc_write_addr = dtd_desc_write_addr(write_desc_addr,
+							   1, 0, 0);
+
+	vpdma_map_desc_buf(vpdma, &list->buf);
+
+	dump_dtd(dtd);
+}
+EXPORT_SYMBOL(vpdma_update_dma_addr);
+
 void vpdma_set_max_size(struct vpdma_data *vpdma, int reg_addr,
 			u32 width, u32 height)
 {
diff --git a/drivers/media/platform/ti/vpe/vpdma.h b/drivers/media/platform/ti/vpe/vpdma.h
index e4d7941c6207..393fcbb3cb40 100644
--- a/drivers/media/platform/ti/vpe/vpdma.h
+++ b/drivers/media/platform/ti/vpe/vpdma.h
@@ -222,6 +222,9 @@ void vpdma_free_desc_list(struct vpdma_desc_list *list);
 int vpdma_submit_descs(struct vpdma_data *vpdma, struct vpdma_desc_list *list,
 		       int list_num);
 bool vpdma_list_busy(struct vpdma_data *vpdma, int list_num);
+void vpdma_update_dma_addr(struct vpdma_data *vpdma,
+	struct vpdma_desc_list *list, dma_addr_t dma_addr,
+	void *write_dtd, int drop, int idx);
 
 /* VPDMA hardware list funcs */
 int vpdma_hwlist_alloc(struct vpdma_data *vpdma, void *priv);
-- 
2.34.1

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

[Yemike Abhilash Chandra]

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

 .../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
+
+  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
+    description:
+      IRQ index 0 is used for Slice0 interrupts
+      IRQ index 1 is used for Slice1 interrupts
+
+  ti,vip-clk-polarity:
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+    description:
+      phandle to the device control module. The 1st argument should
+      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.
+
+patternProperties:
+  '^port@[0-3]$':
+    type: object
+    additionalProperties: false
+
+    properties:
+      "#address-cells":
+        const: 1
+
+      "#size-cells":
+        const: 0
+      reg:
+        description: The port number
+        maxItems: 1
+
+      label:
+        description: Port name. Usually the pin group name
+
+      endpoint:
+        $ref: /schemas/graph.yaml#/properties/endpoint
+        type: object
+        additionalProperties: false
+
+        properties:
+          hsync-active: true
+          vsync-active: true
+          pclk-sample: true
+          bus-width:
+            enum: [8, 24]
+            default: 8
+
+          remote-endpoint: true
+
+    required:
+      - "#address-cells"
+      - "#size-cells"
+      - port@0
+      - reg
+      - label
+
+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 {
+        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>;
+
+              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 {
+        clock-frequency = <400000>;
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+         camera@37 {
+              compatible = "ovti,ov10633";
+              reg = <0x37>;
+
+              clocks = <&fixed_clock>;
+              clocks-names = "xvclk";
+
+              port {
+                   camera1: endpoint {
+                           remote-endpoint = <&vin1a_ep>;
+                           hsync-active = <1>;
+                           vsync-active = <1>;
+                           pclk-sample = <0>;
+                           bus-width = <8>;
+                   };
+              };
+         };
+    };
+
+...
diff --git a/MAINTAINERS b/MAINTAINERS
index ea4b29c8b6b5..c5c5f0d1967f 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -25211,6 +25211,7 @@ S:	Maintained
 W:	http://linuxtv.org/
 Q:	http://patchwork.linuxtv.org/project/linux-media/list/
 F:	Documentation/devicetree/bindings/media/ti,cal.yaml
+F:	Documentation/devicetree/bindings/media/ti,vip.yaml
 F:	Documentation/devicetree/bindings/media/ti,vpe.yaml
 F:	drivers/media/platform/ti/cal/
 F:	drivers/media/platform/ti/vpe/
-- 
2.34.1

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

[Rob Herring (Arm)]

On Wed, 16 Jul 2025 16:49:11 +0530, 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
> 
>  .../devicetree/bindings/media/ti,vip.yaml     | 211 ++++++++++++++++++
>  MAINTAINERS                                   |   1 +
>  2 files changed, 212 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/ti,vip.yaml
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Documentation/devicetree/bindings/media/ti,vip.example.dtb: /example-0/vip@48970000: failed to match any schema with compatible: ['ti,dra7-vip1']
Documentation/devicetree/bindings/media/ti,vip.example.dtb: /example-0/i2c/camera@37: failed to match any schema with compatible: ['ovti,ov10633']

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20250716111912.235157-4-y-abhilashchandra@ti.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

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

[Krzysztof Kozlowski]

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

Re: [PATCH V2 4/4] media: ti-vpe: Add the VIP driver

[Krzysztof Kozlowski]

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

> +static int vip_probe_complete(struct platform_device *pdev)
> +{
> +	struct vip_shared *shared = platform_get_drvdata(pdev);
> +	struct regmap *syscon_pol = NULL;
> +	u32 syscon_pol_offset = 0;
> +	struct vip_port *port;
> +	struct vip_dev *dev;
> +	struct device_node *parent = pdev->dev.of_node;
> +	struct fwnode_handle *ep = NULL;
> +	int ret, slice_id, port_id, p;
> +
> +	if (parent && of_property_read_bool(parent, "ti,vip-clk-polarity")) {
> +		syscon_pol = syscon_regmap_lookup_by_phandle(parent,
> +							     "ti,vip-clk-polarity");
> +		if (IS_ERR(syscon_pol)) {
> +			dev_err(&pdev->dev, "failed to get ti,vip-clk-polarity regmap\n");
> +			return PTR_ERR(syscon_pol);

Syntax is return dev_err_probe. If this is not probe path, then this has
to be fixed.

> +		}
> +
> +		if (of_property_read_u32_index(parent, "ti,vip-clk-polarity",
> +					       1, &syscon_pol_offset)) {
> +			dev_err(&pdev->dev, "failed to get ti,vip-clk-polarity offset\n");
> +			return -EINVAL;
> +		}
> +	}
> +
> +	for (p = 0; p < (VIP_NUM_PORTS * VIP_NUM_SLICES); p++) {
> +		ep = fwnode_graph_get_next_endpoint_by_regs(of_fwnode_handle(parent),
> +							    p, 0);
> +		if (!ep)
> +			continue;
> +
> +		switch (p) {
> +		case 0:
> +			slice_id = VIP_SLICE1;	port_id = VIP_PORTA;
> +			break;
> +		case 1:
> +			slice_id = VIP_SLICE2;	port_id = VIP_PORTA;
> +			break;
> +		case 2:
> +			slice_id = VIP_SLICE1;	port_id = VIP_PORTB;
> +			break;
> +		case 3:
> +			slice_id = VIP_SLICE2;	port_id = VIP_PORTB;
> +			break;
> +		default:
> +			dev_err(&pdev->dev, "Unknown port reg=<%d>\n", p);
> +			continue;
> +		}
> +
> +		ret = alloc_port(shared->devs[slice_id], port_id);
> +		if (ret < 0)
> +			continue;
> +
> +		dev = shared->devs[slice_id];
> +		dev->syscon_pol = syscon_pol;
> +		dev->syscon_pol_offset = syscon_pol_offset;
> +		port = dev->ports[port_id];
> +
> +		vip_register_subdev_notif(port, ep);
> +		fwnode_handle_put(ep);
> +	}
> +	return 0;
> +}
> +
> +static int vip_probe_slice(struct platform_device *pdev, int slice, int instance_id)
> +{
> +	struct vip_shared *shared = platform_get_drvdata(pdev);
> +	struct vip_dev *dev;
> +	struct vip_parser_data *parser;
> +	u32 vin_id;
> +	int ret;
> +
> +	dev = devm_kzalloc(&pdev->dev, sizeof(*dev), GFP_KERNEL);
> +	if (!dev)
> +		return -ENOMEM;
> +
> +	dev->instance_id = instance_id;
> +	vin_id = 1 + ((dev->instance_id - 1) * 2) + slice;
> +	snprintf(dev->name, sizeof(dev->name), "vin%d", vin_id);
> +
> +	dev->irq = platform_get_irq(pdev, slice);
> +	if (dev->irq < 0)
> +		return dev->irq;
> +
> +	ret = devm_request_irq(&pdev->dev, dev->irq, vip_irq,
> +			       0, dev->name, dev);
> +	if (ret < 0)
> +		return -ENOMEM;
> +
> +	spin_lock_init(&dev->slock);
> +	mutex_init(&dev->mutex);
> +
> +	dev->slice_id = slice;
> +	dev->pdev = pdev;
> +	dev->res = shared->res;
> +	dev->base = shared->base;
> +	dev->v4l2_dev = &shared->v4l2_dev;
> +
> +	dev->shared = shared;
> +	shared->devs[slice] = dev;
> +
> +	vip_top_reset(dev);
> +	vip_set_slice_path(dev, VIP_MULTI_CHANNEL_DATA_SELECT, 1);
> +
> +	parser = devm_kzalloc(&pdev->dev, sizeof(*dev->parser), GFP_KERNEL);
> +	if (!parser)
> +		return PTR_ERR(parser);
> +
> +	parser->res = platform_get_resource_byname(pdev,
> +						   IORESOURCE_MEM,
> +						   (slice == 0) ?
> +						   "parser0" :
> +						   "parser1");
> +	parser->base = devm_ioremap_resource(&pdev->dev, parser->res);
> +	if (IS_ERR(parser->base))
> +		return PTR_ERR(parser->base);
> +
> +	parser->pdev = pdev;
> +	dev->parser = parser;
> +
> +	dev->sc_assigned = VIP_NOT_ASSIGNED;
> +	dev->sc = sc_create(pdev, (slice == 0) ? "sc0" : "sc1");
> +	if (IS_ERR(dev->sc))
> +		return PTR_ERR(dev->sc);
> +
> +	dev->csc_assigned = VIP_NOT_ASSIGNED;
> +	dev->csc = csc_create(pdev, (slice == 0) ? "csc0" : "csc1");
> +	if (IS_ERR(dev->sc))
> +		return PTR_ERR(dev->sc);
> +
> +	return 0;
> +}
> +
> +static int vip_probe(struct platform_device *pdev)
> +{
> +	struct vip_shared *shared;
> +	struct pinctrl *pinctrl;
> +	int ret, slice = VIP_SLICE1;
> +	int instance_id;
> +	u32 tmp, pid;
> +	const char *label;
> +
> +	if (!of_property_read_string(pdev->dev.of_node, "label", &label)) {
> +		if (strcmp(label, "vip1") == 0)
> +			instance_id = 1;
> +		else if (strcmp(label, "vip2") == 0)
> +			instance_id = 2;
> +		else if (strcmp(label, "vip3") == 0)


Heh, nice try. You cannot encode instance ID as different property (and
instance ID is not allowed, see writing bindings in next).

And how does it work with label called "krzk"? Your binding said that
"krzk" is a perfectly correct label.

You need to think about such cases.


> +			instance_id = 3;

And past here you use uninitialized instance_id, because you did not
consider "krzk".



Best regards,
Krzysztof

Re: [PATCH V2 2/4] Revert "media: platform: ti: Remove unused vpdma_update_dma_addr"

[Krzysztof Kozlowski]

On 16/07/2025 13:19, Yemike Abhilash Chandra wrote:
> This reverts commit 9314891df119442a6ec1518b3d872c330e2bf1a1.
> 
> We're adding support for TI VIP driver, so this is no longer unused.
> 
> Signed-off-by: Yemike Abhilash Chandra <y-abhilashchandra@ti.com>
> ---
>  drivers/media/platform/ti/vpe/vpdma.c | 32 +++++++++++++++++++++++++++
>  drivers/media/platform/ti/vpe/vpdma.h |  3 +++
>  2 files changed, 35 insertions(+)
> 
> diff --git a/drivers/media/platform/ti/vpe/vpdma.c b/drivers/media/platform/ti/vpe/vpdma.c
> index bb8a8bd7980c..da90d7f03f82 100644
> --- a/drivers/media/platform/ti/vpe/vpdma.c
> +++ b/drivers/media/platform/ti/vpe/vpdma.c
> @@ -552,6 +552,38 @@ EXPORT_SYMBOL(vpdma_submit_descs);
>  
>  static void dump_dtd(struct vpdma_dtd *dtd);
>  


Please add kerneldoc.

> +void vpdma_update_dma_addr(struct vpdma_data *vpdma,
> +	struct vpdma_desc_list *list, dma_addr_t dma_addr,
> +	void *write_dtd, int drop, int idx)
> +{
> +	struct vpdma_dtd *dtd = list->buf.addr;
> +	dma_addr_t write_desc_addr;
> +	int offset;
> +
> +	dtd += idx;
> +	vpdma_unmap_desc_buf(vpdma, &list->buf);
> +
> +	dtd->start_addr = dma_addr;
> +
> +	/* Calculate write address from the offset of write_dtd from start
> +	 * of the list->buf
> +	 */
> +	offset = (void *)write_dtd - list->buf.addr;
> +	write_desc_addr = list->buf.dma_addr + offset;
> +
> +	if (drop)
> +		dtd->desc_write_addr = dtd_desc_write_addr(write_desc_addr,
> +							   1, 1, 0);
> +	else
> +		dtd->desc_write_addr = dtd_desc_write_addr(write_desc_addr,
> +							   1, 0, 0);
> +
> +	vpdma_map_desc_buf(vpdma, &list->buf);
> +
> +	dump_dtd(dtd);
> +}
> +EXPORT_SYMBOL(vpdma_update_dma_addr);


This has to be GPL

> +
>  void vpdma_set_max_size(struct vpdma_data *vpdma, int reg_addr,
>  			u32 width, u32 height)

Best regards,
Krzysztof

Re: [PATCH V2 4/4] media: ti-vpe: Add the VIP driver

[kernel test robot]

Hi Yemike,

kernel test robot noticed the following build warnings:

[auto build test WARNING on linuxtv-media-pending/master]
[also build test WARNING on sailus-media-tree/master next-20250716]
[cannot apply to robh/for-next linus/master sailus-media-tree/streams v6.16-rc6]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Yemike-Abhilash-Chandra/MAINTAINERS-Update-maintainers-of-TI-VPE-and-CAL/20250716-192326
base:   https://git.linuxtv.org/media-ci/media-pending.git master
patch link:    https://lore.kernel.org/r/20250716111912.235157-5-y-abhilashchandra%40ti.com
patch subject: [PATCH V2 4/4] media: ti-vpe: Add the VIP driver
config: arm-allyesconfig (https://download.01.org/0day-ci/archive/20250717/202507171820.SxYjRSbE-lkp@intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 15.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250717/202507171820.SxYjRSbE-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202507171820.SxYjRSbE-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/media/platform/ti/vpe/vip.c: In function 'populate_desc_list':
>> drivers/media/platform/ti/vpe/vip.c:828:22: warning: variable 'list_length' set but not used [-Wunused-but-set-variable]
     828 |         unsigned int list_length;
         |                      ^~~~~~~~~~~
   drivers/media/platform/ti/vpe/vip.c: In function 'vip_create_streams':
>> drivers/media/platform/ti/vpe/vip.c:3382:43: warning: variable 'bus' set but not used [-Wunused-but-set-variable]
    3382 |         struct v4l2_mbus_config_parallel *bus;
         |                                           ^~~
   drivers/media/platform/ti/vpe/vip.c: In function 'vip_probe':
>> drivers/media/platform/ti/vpe/vip.c:3677:25: warning: variable 'pinctrl' set but not used [-Wunused-but-set-variable]
    3677 |         struct pinctrl *pinctrl;
         |                         ^~~~~~~


vim +/list_length +828 drivers/media/platform/ti/vpe/vip.c

   823	
   824	static void populate_desc_list(struct vip_stream *stream)
   825	{
   826		struct vip_port *port = stream->port;
   827		struct vip_dev *dev = port->dev;
 > 828		unsigned int list_length;
   829	
   830		stream->desc_next = stream->desc_list.buf.addr;
   831		add_stream_dtds(stream);
   832	
   833		list_length = stream->desc_next - stream->desc_list.buf.addr;
   834		vpdma_map_desc_buf(dev->shared->vpdma, &stream->desc_list.buf);
   835	}
   836	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH V2 0/4] Add support for VIP

[Hans Verkuil]

Hi Yemike,

On 16/07/2025 13:19, Yemike Abhilash Chandra wrote:
> This patch series add support for the TI VIP video capture engine.
> VIP stands for Video Input Port, it can be found on devices such as
> DRA7xx and provides a parallel interface to a video source such as
> a sensor or TV decoder. 
> 
> Each VIP can support two inputs (slices) and a SoC can be configured
> with a variable number of VIP's. Each slice can support two ports
> each connected to its own sub-device.
> 
> The first patch in this series updates the outdated MAINTAINERS entry
> for the TI VPE and CAL drivers. The subsequent three patches introduce
> support for the TI VIP (Video Input Port) driver.

I'll pick up the MAINTAINERS patch, but the others need more work, so
v3 is needed for that.

> 
> Link for v1: https://lore.kernel.org/all/20200522225412.29440-1-bparrot@ti.com/
> The v1 patch series was posted in the year 2020. This v2 series resumes the
> effort to upstream VIP support by addressing all previous review comments
> 
> Changelog:
> Changes in v2:
> - Remove array and just use hsync: true in bindings (Patch 3/5)
> - Remove array and use enum for bus width in bindings (Patch 3/5)
> - Use pattern properties since properties across ports are same (Patch 3/5)
> - Remove vip_dbg, vip_info, vip_err aliases and just use v4l2_dbg, v4l2_info
>   and v4l2_err instead (Patch 4/5)
> - Remove color space information from vip_formats struct (Patch 4/5)
> - Use g_std instead of g_std_output (Patch 4/5)
> - Do not touch pix.priv (Patch 4/5)
> - Remove all comments with just register values (Patch 4/5)
> - Remove support for vidioc_default ioctl (Patch 4/5)
> - In case of any error while streaming, push all pending buffers to vb2 (Patch 4/5)
> - Address some minor comments made by Hans throughout the driver (Patch 4/5)
> - Update copyright year at various places
> 
> v4l2-compliance output: https://gist.github.com/Yemike-Abhilash-Chandra/b0791cb465fadc11d4c995197cb22f29

Also run v4l2-compliance with the -s option to check compliance while streaming.

Regards,

	Hans

> 
> v4l2-compliance cropping and composing tests are failing likely
> due to OV10635 sensor supporting several discrete frame sizes,
> fail: v4l2-test-formats.cpp(1560): node->frmsizes_count[pixfmt] > 1
> 
> Test logs: https://gist.github.com/Yemike-Abhilash-Chandra/98504ab56416aef38b851036aef5eeb1
> 
> Dale Farnsworth (2):
>   dt-bindings: media: ti: vpe: Add bindings for Video Input Port
>   media: ti-vpe: Add the VIP driver
> 
> Yemike Abhilash Chandra (2):
>   MAINTAINERS: Update maintainers of TI VPE and CAL
>   Revert "media: platform: ti: Remove unused vpdma_update_dma_addr"
> 
>  .../devicetree/bindings/media/ti,vip.yaml     |  211 +
>  MAINTAINERS                                   |    3 +-
>  drivers/media/platform/ti/Kconfig             |   13 +
>  drivers/media/platform/ti/vpe/Makefile        |    2 +
>  drivers/media/platform/ti/vpe/vip.c           | 3824 +++++++++++++++++
>  drivers/media/platform/ti/vpe/vip.h           |  719 ++++
>  drivers/media/platform/ti/vpe/vpdma.c         |   32 +
>  drivers/media/platform/ti/vpe/vpdma.h         |    3 +
>  8 files changed, 4806 insertions(+), 1 deletion(-)
>  create mode 100644 Documentation/devicetree/bindings/media/ti,vip.yaml
>  create mode 100644 drivers/media/platform/ti/vpe/vip.c
>  create mode 100644 drivers/media/platform/ti/vpe/vip.h
> 

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

[Yemike Abhilash Chandra]

Hi Krzysztof,
Thanks for the review.

On 16/07/25 19:35, Krzysztof Kozlowski wrote:
> 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?
> 

I now understand that specifying instance id is not allowed.
I will fix this v3.

>> +
>> +  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 :/
> 

Understood, will fix this in v3.

>> +    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.
> 

Understood. will do that way in v3.

>> +
>> +  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?
> 

Thanks, I Will name this better and also provide rationale on why it is
needed in v3.

>> +      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.
> 

Understood, will fix this in v3

>> +
>> +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:
> 
> 

Apologies. I will fix this entire hierarchy using appropriate
refs and description. Thanks.

>> +
>> +    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?
> 

I will remove this in v3.

>> +
>> +      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
> 

Understood, will correct this in v3.

> 
>> +        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.
> 

I will fix this in v3.

> 
>> +              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.
>

I will remove this in v3.

Thanks and Regards
Yemike Abhilash Chandra

> 
> Best regards,
> Krzysztof

Re: [PATCH V2 2/4] Revert "media: platform: ti: Remove unused vpdma_update_dma_addr"

[Yemike Abhilash Chandra]

Hi Krzysztof,
Thanks for the review.

On 16/07/25 19:53, Krzysztof Kozlowski wrote:
> On 16/07/2025 13:19, Yemike Abhilash Chandra wrote:
>> This reverts commit 9314891df119442a6ec1518b3d872c330e2bf1a1.
>>
>> We're adding support for TI VIP driver, so this is no longer unused.
>>
>> Signed-off-by: Yemike Abhilash Chandra <y-abhilashchandra@ti.com>
>> ---
>>   drivers/media/platform/ti/vpe/vpdma.c | 32 +++++++++++++++++++++++++++
>>   drivers/media/platform/ti/vpe/vpdma.h |  3 +++
>>   2 files changed, 35 insertions(+)
>>
>> diff --git a/drivers/media/platform/ti/vpe/vpdma.c b/drivers/media/platform/ti/vpe/vpdma.c
>> index bb8a8bd7980c..da90d7f03f82 100644
>> --- a/drivers/media/platform/ti/vpe/vpdma.c
>> +++ b/drivers/media/platform/ti/vpe/vpdma.c
>> @@ -552,6 +552,38 @@ EXPORT_SYMBOL(vpdma_submit_descs);
>>   
>>   static void dump_dtd(struct vpdma_dtd *dtd);
>>   
> 
> 
> Please add kerneldoc.
> 

I will add kerneldoc in v3. Thanks

>> +void vpdma_update_dma_addr(struct vpdma_data *vpdma,
>> +	struct vpdma_desc_list *list, dma_addr_t dma_addr,
>> +	void *write_dtd, int drop, int idx)
>> +{
>> +	struct vpdma_dtd *dtd = list->buf.addr;
>> +	dma_addr_t write_desc_addr;
>> +	int offset;
>> +
>> +	dtd += idx;
>> +	vpdma_unmap_desc_buf(vpdma, &list->buf);
>> +
>> +	dtd->start_addr = dma_addr;
>> +
>> +	/* Calculate write address from the offset of write_dtd from start
>> +	 * of the list->buf
>> +	 */
>> +	offset = (void *)write_dtd - list->buf.addr;
>> +	write_desc_addr = list->buf.dma_addr + offset;
>> +
>> +	if (drop)
>> +		dtd->desc_write_addr = dtd_desc_write_addr(write_desc_addr,
>> +							   1, 1, 0);
>> +	else
>> +		dtd->desc_write_addr = dtd_desc_write_addr(write_desc_addr,
>> +							   1, 0, 0);
>> +
>> +	vpdma_map_desc_buf(vpdma, &list->buf);
>> +
>> +	dump_dtd(dtd);
>> +}
>> +EXPORT_SYMBOL(vpdma_update_dma_addr);
> 
> 
> This has to be GPL
> 

I will correct this in v3.

Thanks and Regards
Yemike Abhilash Chandra

>> +
>>   void vpdma_set_max_size(struct vpdma_data *vpdma, int reg_addr,
>>   			u32 width, u32 height)
> 
> Best regards,
> Krzysztof

Re: [PATCH V2 4/4] media: ti-vpe: Add the VIP driver

[Yemike Abhilash Chandra]

Hi Krzysztof,
Thanks for the review.

On 16/07/25 19:39, Krzysztof Kozlowski wrote:
> On 16/07/2025 13:19, Yemike Abhilash Chandra wrote:
> 
>> +static int vip_probe_complete(struct platform_device *pdev)
>> +{
>> +	struct vip_shared *shared = platform_get_drvdata(pdev);
>> +	struct regmap *syscon_pol = NULL;
>> +	u32 syscon_pol_offset = 0;
>> +	struct vip_port *port;
>> +	struct vip_dev *dev;
>> +	struct device_node *parent = pdev->dev.of_node;
>> +	struct fwnode_handle *ep = NULL;
>> +	int ret, slice_id, port_id, p;
>> +
>> +	if (parent && of_property_read_bool(parent, "ti,vip-clk-polarity")) {
>> +		syscon_pol = syscon_regmap_lookup_by_phandle(parent,
>> +							     "ti,vip-clk-polarity");
>> +		if (IS_ERR(syscon_pol)) {
>> +			dev_err(&pdev->dev, "failed to get ti,vip-clk-polarity regmap\n");
>> +			return PTR_ERR(syscon_pol);
> 
> Syntax is return dev_err_probe. If this is not probe path, then this has
> to be fixed.
> 

I will fix this in v3.

>> +		}
>> +
>> +		if (of_property_read_u32_index(parent, "ti,vip-clk-polarity",
>> +					       1, &syscon_pol_offset)) {
>> +			dev_err(&pdev->dev, "failed to get ti,vip-clk-polarity offset\n");
>> +			return -EINVAL;
>> +		}
>> +	}
>> +
>> +	for (p = 0; p < (VIP_NUM_PORTS * VIP_NUM_SLICES); p++) {
>> +		ep = fwnode_graph_get_next_endpoint_by_regs(of_fwnode_handle(parent),
>> +							    p, 0);
>> +		if (!ep)
>> +			continue;
>> +
>> +		switch (p) {
>> +		case 0:
>> +			slice_id = VIP_SLICE1;	port_id = VIP_PORTA;
>> +			break;
>> +		case 1:
>> +			slice_id = VIP_SLICE2;	port_id = VIP_PORTA;
>> +			break;
>> +		case 2:
>> +			slice_id = VIP_SLICE1;	port_id = VIP_PORTB;
>> +			break;
>> +		case 3:
>> +			slice_id = VIP_SLICE2;	port_id = VIP_PORTB;
>> +			break;
>> +		default:
>> +			dev_err(&pdev->dev, "Unknown port reg=<%d>\n", p);
>> +			continue;
>> +		}
>> +
>> +		ret = alloc_port(shared->devs[slice_id], port_id);
>> +		if (ret < 0)
>> +			continue;
>> +
>> +		dev = shared->devs[slice_id];
>> +		dev->syscon_pol = syscon_pol;
>> +		dev->syscon_pol_offset = syscon_pol_offset;
>> +		port = dev->ports[port_id];
>> +
>> +		vip_register_subdev_notif(port, ep);
>> +		fwnode_handle_put(ep);
>> +	}
>> +	return 0;
>> +}
>> +
>> +static int vip_probe_slice(struct platform_device *pdev, int slice, int instance_id)
>> +{
>> +	struct vip_shared *shared = platform_get_drvdata(pdev);
>> +	struct vip_dev *dev;
>> +	struct vip_parser_data *parser;
>> +	u32 vin_id;
>> +	int ret;
>> +
>> +	dev = devm_kzalloc(&pdev->dev, sizeof(*dev), GFP_KERNEL);
>> +	if (!dev)
>> +		return -ENOMEM;
>> +
>> +	dev->instance_id = instance_id;
>> +	vin_id = 1 + ((dev->instance_id - 1) * 2) + slice;
>> +	snprintf(dev->name, sizeof(dev->name), "vin%d", vin_id);
>> +
>> +	dev->irq = platform_get_irq(pdev, slice);
>> +	if (dev->irq < 0)
>> +		return dev->irq;
>> +
>> +	ret = devm_request_irq(&pdev->dev, dev->irq, vip_irq,
>> +			       0, dev->name, dev);
>> +	if (ret < 0)
>> +		return -ENOMEM;
>> +
>> +	spin_lock_init(&dev->slock);
>> +	mutex_init(&dev->mutex);
>> +
>> +	dev->slice_id = slice;
>> +	dev->pdev = pdev;
>> +	dev->res = shared->res;
>> +	dev->base = shared->base;
>> +	dev->v4l2_dev = &shared->v4l2_dev;
>> +
>> +	dev->shared = shared;
>> +	shared->devs[slice] = dev;
>> +
>> +	vip_top_reset(dev);
>> +	vip_set_slice_path(dev, VIP_MULTI_CHANNEL_DATA_SELECT, 1);
>> +
>> +	parser = devm_kzalloc(&pdev->dev, sizeof(*dev->parser), GFP_KERNEL);
>> +	if (!parser)
>> +		return PTR_ERR(parser);
>> +
>> +	parser->res = platform_get_resource_byname(pdev,
>> +						   IORESOURCE_MEM,
>> +						   (slice == 0) ?
>> +						   "parser0" :
>> +						   "parser1");
>> +	parser->base = devm_ioremap_resource(&pdev->dev, parser->res);
>> +	if (IS_ERR(parser->base))
>> +		return PTR_ERR(parser->base);
>> +
>> +	parser->pdev = pdev;
>> +	dev->parser = parser;
>> +
>> +	dev->sc_assigned = VIP_NOT_ASSIGNED;
>> +	dev->sc = sc_create(pdev, (slice == 0) ? "sc0" : "sc1");
>> +	if (IS_ERR(dev->sc))
>> +		return PTR_ERR(dev->sc);
>> +
>> +	dev->csc_assigned = VIP_NOT_ASSIGNED;
>> +	dev->csc = csc_create(pdev, (slice == 0) ? "csc0" : "csc1");
>> +	if (IS_ERR(dev->sc))
>> +		return PTR_ERR(dev->sc);
>> +
>> +	return 0;
>> +}
>> +
>> +static int vip_probe(struct platform_device *pdev)
>> +{
>> +	struct vip_shared *shared;
>> +	struct pinctrl *pinctrl;
>> +	int ret, slice = VIP_SLICE1;
>> +	int instance_id;
>> +	u32 tmp, pid;
>> +	const char *label;
>> +
>> +	if (!of_property_read_string(pdev->dev.of_node, "label", &label)) {
>> +		if (strcmp(label, "vip1") == 0)
>> +			instance_id = 1;
>> +		else if (strcmp(label, "vip2") == 0)
>> +			instance_id = 2;
>> +		else if (strcmp(label, "vip3") == 0)
> 
> 
> Heh, nice try. You cannot encode instance ID as different property (and
> instance ID is not allowed, see writing bindings in next).
> 
> And how does it work with label called "krzk"? Your binding said that
> "krzk" is a perfectly correct label.
> 
> You need to think about such cases.
> 
> 
>> +			instance_id = 3;
> 
> And past here you use uninitialized instance_id, because you did not
> consider "krzk".
> 

Understood. I will avoid this in v3

Thanks and Regards,
Yemike Abhilash Chandra

> 
> 
> Best regards,
> Krzysztof

Re: [PATCH V2 0/4] Add support for VIP

[Yemike Abhilash Chandra]

Hi Hans,
Thanks for the review.

On 25/08/25 20:04, Hans Verkuil wrote:
> Hi Yemike,
> 
> On 16/07/2025 13:19, Yemike Abhilash Chandra wrote:
>> This patch series add support for the TI VIP video capture engine.
>> VIP stands for Video Input Port, it can be found on devices such as
>> DRA7xx and provides a parallel interface to a video source such as
>> a sensor or TV decoder.
>>
>> Each VIP can support two inputs (slices) and a SoC can be configured
>> with a variable number of VIP's. Each slice can support two ports
>> each connected to its own sub-device.
>>
>> The first patch in this series updates the outdated MAINTAINERS entry
>> for the TI VPE and CAL drivers. The subsequent three patches introduce
>> support for the TI VIP (Video Input Port) driver.
> 
> I'll pick up the MAINTAINERS patch, but the others need more work, so
> v3 is needed for that.
> 

Thanks, I am working on v3. I plan to incorporate the additional
feedback you provided on the driver before submitting the v3.

>>
>> Link for v1: https://lore.kernel.org/all/20200522225412.29440-1-bparrot@ti.com/
>> The v1 patch series was posted in the year 2020. This v2 series resumes the
>> effort to upstream VIP support by addressing all previous review comments
>>
>> Changelog:
>> Changes in v2:
>> - Remove array and just use hsync: true in bindings (Patch 3/5)
>> - Remove array and use enum for bus width in bindings (Patch 3/5)
>> - Use pattern properties since properties across ports are same (Patch 3/5)
>> - Remove vip_dbg, vip_info, vip_err aliases and just use v4l2_dbg, v4l2_info
>>    and v4l2_err instead (Patch 4/5)
>> - Remove color space information from vip_formats struct (Patch 4/5)
>> - Use g_std instead of g_std_output (Patch 4/5)
>> - Do not touch pix.priv (Patch 4/5)
>> - Remove all comments with just register values (Patch 4/5)
>> - Remove support for vidioc_default ioctl (Patch 4/5)
>> - In case of any error while streaming, push all pending buffers to vb2 (Patch 4/5)
>> - Address some minor comments made by Hans throughout the driver (Patch 4/5)
>> - Update copyright year at various places
>>
>> v4l2-compliance output: https://gist.github.com/Yemike-Abhilash-Chandra/b0791cb465fadc11d4c995197cb22f29
> 
> Also run v4l2-compliance with the -s option to check compliance while streaming.
> 

Thanks for pointing this out. Will run as part of v3.

Thanks and Regards,
Yemike Abhilash Chandra

> Regards,
> 
> 	Hans
> 
>>
>> v4l2-compliance cropping and composing tests are failing likely
>> due to OV10635 sensor supporting several discrete frame sizes,
>> fail: v4l2-test-formats.cpp(1560): node->frmsizes_count[pixfmt] > 1
>>
>> Test logs: https://gist.github.com/Yemike-Abhilash-Chandra/98504ab56416aef38b851036aef5eeb1
>>
>> Dale Farnsworth (2):
>>    dt-bindings: media: ti: vpe: Add bindings for Video Input Port
>>    media: ti-vpe: Add the VIP driver
>>
>> Yemike Abhilash Chandra (2):
>>    MAINTAINERS: Update maintainers of TI VPE and CAL
>>    Revert "media: platform: ti: Remove unused vpdma_update_dma_addr"
>>
>>   .../devicetree/bindings/media/ti,vip.yaml     |  211 +
>>   MAINTAINERS                                   |    3 +-
>>   drivers/media/platform/ti/Kconfig             |   13 +
>>   drivers/media/platform/ti/vpe/Makefile        |    2 +
>>   drivers/media/platform/ti/vpe/vip.c           | 3824 +++++++++++++++++
>>   drivers/media/platform/ti/vpe/vip.h           |  719 ++++
>>   drivers/media/platform/ti/vpe/vpdma.c         |   32 +
>>   drivers/media/platform/ti/vpe/vpdma.h         |    3 +
>>   8 files changed, 4806 insertions(+), 1 deletion(-)
>>   create mode 100644 Documentation/devicetree/bindings/media/ti,vip.yaml
>>   create mode 100644 drivers/media/platform/ti/vpe/vip.c
>>   create mode 100644 drivers/media/platform/ti/vpe/vip.h
>>
>