| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <Y8a+Hk2jFOjbkIvZ@pendragon.ideasonboard.com>
Date: Tue, 17 Jan 2023 17:26:22 +0200
From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
To: Yuji Ishikawa <yuji2.ishikawa@...hiba.co.jp>
Cc: Hans Verkuil <hverkuil@...all.nl>,
Mauro Carvalho Chehab <mchehab@...nel.org>,
Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@...hiba.co.jp>,
Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
"Rafael J . Wysocki" <rafael.j.wysocki@...el.com>,
Mark Brown <broonie@...nel.org>, linux-media@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
devicetree@...r.kernel.org
Subject: Re: [PATCH v5 1/6] dt-bindings: media: platform: visconti: Add
Toshiba Visconti Video Input Interface bindings
Hi Yuji,
Thank you for the patch.
On Wed, Jan 11, 2023 at 11:24:28AM +0900, 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@...hiba.co.jp>
> Reviewed-by: Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@...hiba.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
> ---
> .../bindings/media/toshiba,visconti-viif.yaml | 98 +++++++++++++++++++
> 1 file changed, 98 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/media/toshiba,visconti-viif.yaml
>
> diff --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@...hiba.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@...00000 {
> + 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
Powered by blists - more mailing lists