[v7,1/5] dt-bindings: media: platform: visconti: Add Toshiba Visconti Video Input Interface
Commit Message
Adds the Device Tree binding documentation that allows to describe
the Video Input Interface found in Toshiba Visconti SoCs.
Signed-off-by: Yuji Ishikawa <yuji2.ishikawa@toshiba.co.jp>
Reviewed-by: Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp>
---
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
Changelog v6:
- add register definition of BUS-IF and MPU
Changelog v7:
- remove trailing "bindings" from commit header message
- remove trailing "Device Tree Bindings" from title
- fix text wrapping of description
- change compatible to visconti5-viif
- explicitly define allowed properties for port::endpoint
.../bindings/media/toshiba,visconti-viif.yaml | 108 ++++++++++++++++++
1 file changed, 108 insertions(+)
create mode 100644 Documentation/devicetree/bindings/media/toshiba,visconti-viif.yaml
Comments
On 14/07/2023 03:50, Yuji Ishikawa wrote:
> Adds the Device Tree binding documentation that allows to describe
> the Video Input Interface found in Toshiba Visconti SoCs.
>
> Signed-off-by: Yuji Ishikawa <yuji2.ishikawa@toshiba.co.jp>
> Reviewed-by: Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp>
> ---
> Changelog v2:
Thank you for your patch. There is something to discuss/improve.
> - no change
>
> Changelog v3:
> - no change
>
> Changelog v4:
> - fix style problems at the v3 patch
> - remove "index" member
> - update example
>
> Changelog v5:
> - no change
>
> Changelog v6:
> - add register definition of BUS-IF and MPU
>
> Changelog v7:
> - remove trailing "bindings" from commit header message
> - remove trailing "Device Tree Bindings" from title
> - fix text wrapping of description
> - change compatible to visconti5-viif
Then the filename should be updated to match it:
toshiba,visconti5-viif.yaml
> - explicitly define allowed properties for port::endpoint
>
> .../bindings/media/toshiba,visconti-viif.yaml | 108 ++++++++++++++++++
> 1 file changed, 108 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/media/toshiba,visconti-viif.yaml
>
...
> + port:
> + $ref: /schemas/graph.yaml#/$defs/port-base
> + unevaluatedProperties: false
> + description: Input port, single endpoint describing the CSI-2 transmitter.
> +
> + properties:
> + endpoint:
> + $ref: video-interfaces.yaml#
> + additionalProperties: false
This should be rather:
unevaluatedProperties: false
> +
> + required: ["bus-type", "clock-noncontinuous", "link-frequencies", "remote-endpoint"]
That's not the syntax we try to keep in the bindings. See
renesas,rzg2l-csi2.yaml.
> +
> + properties:
> + data-lanes:
> + description: VIIF supports 2 or 4 data lanes
> + $ref: /schemas/types.yaml#/definitions/uint32-array
> + minItems: 1
> + maxItems: 4
> + items:
> + minimum: 1
> + maximum: 4
> +
> + clock-lanes:
> + description: VIIF supports 1 clock lane
> + const: 0
Are you sure it must be on position 0?
> +
> + bus-type: true
> + clock-noncontinuous: true
> + link-frequencies: true
> + remote-endpoint: true
Drop all of these "xxx: true", should not be needed after converting to
unevaluatedProperties: false
> +
> +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 {
isp@
(or video or something else matching this type of the device, but this
should be a generic name)
> + compatible = "toshiba,visconti5-viif";
> + reg = <0 0x1c000000 0 0x6000>,
> + <0 0x1c008000 0 0x400>,
> + <0 0x1c00E000 0 0x1000>,
> + <0 0x2417A000 0 0x1000>;
Lowercase hex, please.
Best regards,
Krzysztof
Hello Krzysztof
Thank you for your review comments.
> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Sent: Friday, July 14, 2023 4:50 PM
> To: ishikawa yuji(石川 悠司 ○RDC□AITC○EA開)
> <yuji2.ishikawa@toshiba.co.jp>; Hans Verkuil <hverkuil@xs4all.nl>; Sakari
> Ailus <sakari.ailus@iki.fi>; Laurent Pinchart
> <laurent.pinchart@ideasonboard.com>; Mauro Carvalho Chehab
> <mchehab@kernel.org>; Rob Herring <robh+dt@kernel.org>; Krzysztof
> Kozlowski <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley
> <conor+dt@kernel.org>; iwamatsu nobuhiro(岩松 信洋 ○DITC□DIT○
> OST) <nobuhiro1.iwamatsu@toshiba.co.jp>; Mark Brown
> <broonie@kernel.org>
> Cc: linux-media@vger.kernel.org; devicetree@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v7 1/5] dt-bindings: media: platform: visconti: Add Toshiba
> Visconti Video Input Interface
>
> On 14/07/2023 03:50, Yuji Ishikawa wrote:
> > Adds the Device Tree binding documentation that allows to describe the
> > Video Input Interface found in Toshiba Visconti SoCs.
> >
> > Signed-off-by: Yuji Ishikawa <yuji2.ishikawa@toshiba.co.jp>
> > Reviewed-by: Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp>
> > ---
> > Changelog v2:
>
> Thank you for your patch. There is something to discuss/improve.
>
> > - no change
> >
> > Changelog v3:
> > - no change
> >
> > Changelog v4:
> > - fix style problems at the v3 patch
> > - remove "index" member
> > - update example
> >
> > Changelog v5:
> > - no change
> >
> > Changelog v6:
> > - add register definition of BUS-IF and MPU
> >
> > Changelog v7:
> > - remove trailing "bindings" from commit header message
> > - remove trailing "Device Tree Bindings" from title
> > - fix text wrapping of description
> > - change compatible to visconti5-viif
>
> Then the filename should be updated to match it:
> toshiba,visconti5-viif.yaml
>
I'll change the filename to toshiba,visconti5-viif.yaml
> > - explicitly define allowed properties for port::endpoint
> >
> > .../bindings/media/toshiba,visconti-viif.yaml | 108
> > ++++++++++++++++++
> > 1 file changed, 108 insertions(+)
> > create mode 100644
> > Documentation/devicetree/bindings/media/toshiba,visconti-viif.yaml
> >
>
> ...
>
> > + port:
> > + $ref: /schemas/graph.yaml#/$defs/port-base
> > + unevaluatedProperties: false
> > + description: Input port, single endpoint describing the CSI-2 transmitter.
> > +
> > + properties:
> > + endpoint:
> > + $ref: video-interfaces.yaml#
> > + additionalProperties: false
>
> This should be rather:
> unevaluatedProperties: false
In the previous discussion with Laurent, I believe additionalProperties was preferred in terms of handling properties not explicitly mentioned.
https://lore.kernel.org/all/Y82NtJCtr+CZgS9k@pendragon.ideasonboard.com/
Do you have concern about defaulting properties in video-interface.yaml to forbidden?
If defaulting to optional (like most of other bindings) is better, I'll use unevaluatedProperties.
> > +
> > + required: ["bus-type", "clock-noncontinuous",
> > + "link-frequencies", "remote-endpoint"]
>
> That's not the syntax we try to keep in the bindings. See renesas,rzg2l-csi2.yaml.
>
I'll use a line for every element.
> > +
> > + properties:
> > + data-lanes:
> > + description: VIIF supports 2 or 4 data lanes
> > + $ref: /schemas/types.yaml#/definitions/uint32-array
> > + minItems: 1
> > + maxItems: 4
> > + items:
> > + minimum: 1
> > + maximum: 4
> > +
> > + clock-lanes:
> > + description: VIIF supports 1 clock lane
> > + const: 0
>
> Are you sure it must be on position 0?
>
Because a Visconti5 CSI2RX has only clock-lane0,
there's no need to specify clock-lane with device tree.
I'll drop "clock-lanes".
> > +
> > + bus-type: true
> > + clock-noncontinuous: true
> > + link-frequencies: true
> > + remote-endpoint: true
>
> Drop all of these "xxx: true", should not be needed after converting to
> unevaluatedProperties: false
>
I'll drop "xxx: true" if unevaluatedProperties is chosen instead of additionalProperties.
> > +
> > +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 {
>
> isp@
> (or video or something else matching this type of the device, but this should be a
> generic name)
>
I'll use video@1c000000.
> > + compatible = "toshiba,visconti5-viif";
> > + reg = <0 0x1c000000 0 0x6000>,
> > + <0 0x1c008000 0 0x400>,
> > + <0 0x1c00E000 0 0x1000>,
> > + <0 0x2417A000 0 0x1000>;
>
> Lowercase hex, please.
>
I'll fix them.
> Best regards,
> Krzysztof
Best regards,
Yuji
On 25/07/2023 08:08, yuji2.ishikawa@toshiba.co.jp wrote:
>> unevaluatedProperties: false
>
> In the previous discussion with Laurent, I believe additionalProperties was preferred in terms of handling properties not explicitly mentioned.
>
> https://lore.kernel.org/all/Y82NtJCtr+CZgS9k@pendragon.ideasonboard.com/
OK.
>
> Do you have concern about defaulting properties in video-interface.yaml to forbidden?
> If defaulting to optional (like most of other bindings) is better, I'll use unevaluatedProperties.
>
...
> there's no need to specify clock-lane with device tree.
>
> I'll drop "clock-lanes".
>
>>> +
>>> + bus-type: true
>>> + clock-noncontinuous: true
>>> + link-frequencies: true
>>> + remote-endpoint: true
>>
>> Drop all of these "xxx: true", should not be needed after converting to
>> unevaluatedProperties: false
>>
>
> I'll drop "xxx: true" if unevaluatedProperties is chosen instead of additionalProperties.
Since we keep unevaluated, this should stay.
Best regards,
Krzysztof
new file mode 100644
@@ -0,0 +1,108 @@
+# 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
+
+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 image signal processors (L1ISP, L2ISP),
+ then stores pictures to main memory.
+
+properties:
+ compatible:
+ const: toshiba,visconti5-viif
+
+ reg:
+ items:
+ - description: registers for capture control
+ - description: registers for CSI2 receiver control
+ - description: registers for bus interface unit control
+ - description: registers for Memory Protection Unit
+
+ 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.
+
+ properties:
+ endpoint:
+ $ref: video-interfaces.yaml#
+ additionalProperties: false
+
+ required: ["bus-type", "clock-noncontinuous", "link-frequencies", "remote-endpoint"]
+
+ properties:
+ data-lanes:
+ description: VIIF supports 2 or 4 data lanes
+ $ref: /schemas/types.yaml#/definitions/uint32-array
+ minItems: 1
+ maxItems: 4
+ items:
+ minimum: 1
+ maximum: 4
+
+ clock-lanes:
+ description: VIIF supports 1 clock lane
+ const: 0
+
+ bus-type: true
+ clock-noncontinuous: true
+ link-frequencies: true
+ remote-endpoint: true
+
+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,visconti5-viif";
+ reg = <0 0x1c000000 0 0x6000>,
+ <0 0x1c008000 0 0x400>,
+ <0 0x1c00E000 0 0x1000>,
+ <0 0x2417A000 0 0x1000>;
+ 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>;
+ data-lanes = <1 2>;
+ clock-lanes = <0>;
+ clock-noncontinuous;
+ link-frequencies = /bits/ 64 <456000000>;
+ };
+ };
+ };
+ };