Re: [PATCH v5 1/6] dt-bindings: media: platform: visconti: Add Toshiba Visconti Video Input Interface bindings
From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Date: 2023-01-17 15:26:59
Also in:
linux-devicetree, linux-media, lkml
Hi Yuji, Thank you for the patch. On Wed, Jan 11, 2023 at 11:24:28AM +0900, Yuji Ishikawa wrote:
quoted hunk ↗ jump to hunk
Adds the Device Tree binding documentation that allows to describe the Video Input Interface found in Toshiba Visconti SoCs. Signed-off-by: Yuji Ishikawa <redacted> Reviewed-by: Nobuhiro Iwamatsu <redacted> --- Changelog v2: - no change Changelog v3: - no change Changelog v4: - fix style problems at the v3 patch - remove "index" member - update example Changelog v5: - no change --- .../bindings/media/toshiba,visconti-viif.yaml | 98 +++++++++++++++++++ 1 file changed, 98 insertions(+) create mode 100644 Documentation/devicetree/bindings/media/toshiba,visconti-viif.yamldiff --git a/Documentation/devicetree/bindings/media/toshiba,visconti-viif.yaml b/Documentation/devicetree/bindings/media/toshiba,visconti-viif.yaml new file mode 100644 index 00000000000..71442724d1a --- /dev/null +++ b/Documentation/devicetree/bindings/media/toshiba,visconti-viif.yaml@@ -0,0 +1,98 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/media/toshiba,visconti-viif.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Toshiba Visconti5 SoC Video Input Interface Device Tree Bindings + +maintainers: + - Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp> + +description: + Toshiba Visconti5 SoC Video Input Interface (VIIF) + receives MIPI CSI2 video stream, + processes the stream with embedded image signal processor (L1ISP, L2ISP), + then stores pictures to main memory. + +properties: + compatible: + const: toshiba,visconti-viif + + reg: + items: + - description: registers for capture control + - description: registers for CSI2 receiver control
Nitpicking, s/registers/Registers/ in the two lines above as you capitalize the descriptions below.
+ + interrupts: + items: + - description: Sync Interrupt + - description: Status (Error) Interrupt + - description: CSI2 Receiver Interrupt + - description: L1ISP Interrupt + + port: + $ref: /schemas/graph.yaml#/$defs/port-base + unevaluatedProperties: false + description: Input port, single endpoint describing the CSI-2 transmitter.
I would write
description:
CSI-2 input port, with a single endpoint connected to the CSI-2
transmitter.
+ + properties: + endpoint: + $ref: video-interfaces.yaml# + unevaluatedProperties: false + + properties: + data-lanes: + description: VIIF supports 2 or 4 data lines
s/lines/lanes/
+ $ref: /schemas/types.yaml#/definitions/uint32-array
You can drop this line, it's already handled by video-interfaces.yaml.
+ minItems: 1 + maxItems: 4
If only 2 or 4 data lanes are supported, shouldn't minItems be 2 ?
+ items: + minimum: 1 + maximum: 4
Can the CSI-2 receiver reorder the data lanes ? If not, I think you can
write
items:
- const: 1
- const: 2
- const: 3
- const: 4
+ + clock-lanes: + description: VIIF supports 1 clock line
s/line/lane/
+ const: 0
I would also add
clock-noncontinuous: true
link-frequencies: true
to indicate that the above two properties are used by this device.
Also, mark the properties that are required:
required:
- data-lanes
- clock-lanes
I'm wondering, though, if clock-lanes shouldn't be simply omitted. If
the hardware doesn't support any other option than using lane 0 for the
clock lane (as in, no lane remapping), then you can drop the clock-lanes
property completely.
+
+required:
+ - compatible
+ - reg
+ - interrupts
+ - port
+
+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>;
+
+ viif@1c000000 {
+ compatible = "toshiba,visconti-viif";
+ reg = <0 0x1c000000 0 0x6000>,
+ <0 0x1c008000 0 0x400>;
+ interrupts = <GIC_SPI 64 IRQ_TYPE_LEVEL_HIGH>,
+ <GIC_SPI 67 IRQ_TYPE_LEVEL_HIGH>,
+ <GIC_SPI 73 IRQ_TYPE_LEVEL_HIGH>,
+ <GIC_SPI 76 IRQ_TYPE_LEVEL_HIGH>;
+
+ port {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ csi_in0: endpoint {
+ remote-endpoint = <&imx219_out0>;
+ bus-type = <4>;Does the hardware support any other bus type ? If not, you can drop the bus-type. If it does, bus-type should be added to the binding, with the value set to "const: 4".
+ data-lanes = <1 2>; + clock-lanes = <0>; + clock-noncontinuous; + link-frequencies = /bits/ 64 <456000000>; + }; + }; + }; + };
-- Regards, Laurent Pinchart _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel