RE: [PATCH v12 1/8] dt-bindings: media: platform: visconti: Add Toshiba Visconti MIPI CSI-2 Receiver

Hello Laurent

Thank you for your review comments.

-----Original Message-----
From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Sent: Thursday, January 2, 2025 6:29 PM
To: ishikawa yuji(石川 悠司 ○ＲＤＣ□ＡＩＴＣ○ＥＡ開)
[off-list ref]
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>; Rob Herring
[off-list ref]; Krzysztof Kozlowski [off-list ref]; Conor Dooley
[off-list ref]; Sakari Ailus [off-list ref]; Hans
Verkuil [off-list ref]; iwamatsu nobuhiro(岩松 信洋 ○ＤＩＴＣ
□ＤＩＴ○ＯＳＴ) [off-list ref];
linux-media@vger.kernel.org; linux-kernel@vger.kernel.org;
linux-arm-kernel@lists.infradead.org; devicetree@vger.kernel.org
Subject: Re: [PATCH v12 1/8] dt-bindings: media: platform: visconti: Add
Toshiba Visconti MIPI CSI-2 Receiver

Hello Ishikawa-san,

Thank you for the patch.

On Mon, Nov 25, 2024 at 06:21:39PM +0900, Yuji Ishikawa wrote:

quoted

Adds the Device Tree binding documentation that allows to describe the
MIPI CSI-2 Receiver found in Toshiba Visconti SoCs.

Signed-off-by: Yuji Ishikawa <redacted>
Reviewed-by: Nobuhiro Iwamatsu <redacted>
---

Changelog v12:
- Newly add bindings for CSI2RX driver

 .../media/toshiba,visconti5-csi2rx.yaml       | 104

++++++++++++++++++

quoted

 1 file changed, 104 insertions(+)
 create mode 100644
Documentation/devicetree/bindings/media/toshiba,visconti5-csi2rx.yaml

diff --git
a/Documentation/devicetree/bindings/media/toshiba,visconti5-csi2rx.yam
l
b/Documentation/devicetree/bindings/media/toshiba,visconti5-csi2rx.yam
l
new file mode 100644
index 000000000000..5488072bc82a

--- /dev/null
+++ b/Documentation/devicetree/bindings/media/toshiba,visconti5-csi2rx
+++ .yaml

@@ -0,0 +1,104 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) %YAML 1.2
+---
+$id:
+http://devicetree.org/schemas/media/toshiba,visconti5-csi2rx.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Toshiba Visconti5 SoC MIPI CSI-2 receiver
+
+maintainers:
+  - Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp>
+
+description: |-

As Krzysztof mentioned, '|-' isn't needed. See https://yaml-multiline.info/ for
more information. The literal block style indicator ('|') is only needed when line
breaks need to be preserved, e.g. when the description contains ASCII art.

I'll remove "|-".

quoted

+  Toshiba Visconti5 SoC MIPI CSI-2 receiver device receives MIPI
+ CSI-2 video  stream. Use with VIIF device. T.B.D

T.B.D ?

This should have been removed.
I'll remove it.

quoted

+
+properties:
+  compatible:
+    const: toshiba,visconti5-csi2rx
+
+  reg:
+    items:
+      - description: Registers for CSI2 receiver control
+
+  interrupts:
+    items:
+      - description: CSI2 Receiver Interrupt
+
+  ports:
+    $ref: /schemas/graph.yaml#/properties/ports
+
+    properties:
+      port@0:
+        $ref: /schemas/graph.yaml#/$defs/port-base
+        unevaluatedProperties: false
+        description:
+          Input port node, single endpoint describing the CSI-2

transmitter.

quoted

+
+        properties:
+          endpoint:
+            $ref: video-interfaces.yaml#

Please use a full path for the ref:

            $ref: /schemas/media/video-interfaces.yaml#

I'll use a full path description

quoted

+            unevaluatedProperties: false
+
+            properties:
+              data-lanes:
+                description: CSI2 receiver supports 1, 2, 3 or 4 data
+ lanes

You can drop the description. The video-interfaces.yaml schema has a more
complete description, and the fact that the receiver supports between 1 and 4
lanes is conveyed by minItems and items below.

I'll drop the description.

quoted

+                minItems: 1
+                items:
+                  - const: 1
+                  - const: 2
+                  - const: 3
+                  - const: 4
+            required:
+              - data-lanes
+
+      port@1:
+        $ref: /schemas/graph.yaml#/properties/port
+        description:
+          Output port node, single endpoint describing the Visconti VIIF.
+
+    required:
+      - port@0
+      - port@1
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - ports
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    soc {
+        #address-cells = <2>;
+        #size-cells = <2>;
+
+        csi2rx@1c008000 {

Node names should describe the function of the node, not the precise model of
the device. "csi2" would be a more appropriate name.

I'll use "csi2" for the node's name.

quoted

+            compatible = "toshiba,visconti5-csi2rx";
+            reg = <0 0x1c008000 0 0x400>;
+            interrupts = <GIC_SPI 73 IRQ_TYPE_LEVEL_HIGH>;
+
+            ports {
+                #address-cells = <1>;
+                #size-cells = <0>;
+                port@0 {
+                    reg = <0>;
+                    csi2rx_in0: endpoint {
+                        data-lanes = <1 2>;
+                        remote-endpoint = <&imx219_out0>;
+                    };
+                };
+                port@1 {
+                    reg = <1>;
+                    csi2rx_out0: endpoint {
+                        remote-endpoint = <&csi_in0>;
+                    };
+                };
+            };
+        };
+    };

--
Regards,

Laurent Pinchart

Regards,

Yuji Ishikawa