linux-kernel - Re: [PATCH v4 03/11] media: dt-bindings: media: add bindings for rockchip rk3568 vicap

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <Z7xpU8yKNKBReage@kekkonen.localdomain>
Date: Mon, 24 Feb 2025 12:42:59 +0000
From: Sakari Ailus <sakari.ailus@...ux.intel.com>
To: Michael Riesch <michael.riesch@...fvision.net>
Cc: Mehdi Djait <mehdi.djait@...ux.intel.com>,
	Maxime Chevallier <maxime.chevallier@...tlin.com>,
	Théo Lebrun <theo.lebrun@...tlin.com>,
	Gerald Loacker <gerald.loacker@...fvision.net>,
	Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
	Laurent Pinchart <laurent.pinchart@...asonboard.com>,
	Mauro Carvalho Chehab <mchehab@...nel.org>,
	Rob Herring <robh+dt@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Heiko Stuebner <heiko@...ech.de>,
	Kever Yang <kever.yang@...k-chips.com>,
	Nicolas Dufresne <nicolas.dufresne@...labora.com>,
	Sebastian Fricke <sebastian.fricke@...labora.com>,
	Sebastian Reichel <sebastian.reichel@...labora.com>,
	Paul Kocialkowski <paulk@...-base.io>,
	Alexander Shiyan <eagle.alexander923@...il.com>,
	Val Packett <val@...kett.cool>, Rob Herring <robh@...nel.org>,
	Philipp Zabel <p.zabel@...gutronix.de>, linux-media@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org,
	linux-rockchip@...ts.infradead.org
Subject: Re: [PATCH v4 03/11] media: dt-bindings: media: add bindings for
 rockchip rk3568 vicap

Hi Michael,

On Mon, Feb 24, 2025 at 11:21:41AM +0100, Michael Riesch wrote:
> Hi Sakari,
> 
> Thanks for the review.
> 
> On 2/21/25 15:10, Sakari Ailus wrote:
> > Hi Michael,
> > 
> > Thanks for the update.
> > 
> > On Wed, Feb 19, 2025 at 11:16:34AM +0100, Michael Riesch wrote:
> >> Add documentation for the Rockchip RK3568 Video Capture (VICAP) unit.
> >>
> >> Signed-off-by: Michael Riesch <michael.riesch@...fvision.net>
> >> ---
> >>  .../bindings/media/rockchip,rk3568-vicap.yaml      | 168 +++++++++++++++++++++
> >>  MAINTAINERS                                        |   1 +
> >>  2 files changed, 169 insertions(+)
> >>
> >> diff --git a/Documentation/devicetree/bindings/media/rockchip,rk3568-vicap.yaml b/Documentation/devicetree/bindings/media/rockchip,rk3568-vicap.yaml
> >> new file mode 100644
> >> index 000000000000..3dc15efeb32e
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/media/rockchip,rk3568-vicap.yaml
> >> @@ -0,0 +1,168 @@
> >> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> >> +%YAML 1.2
> >> +---
> >> +$id: http://devicetree.org/schemas/media/rockchip,rk3568-vicap.yaml#
> >> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >> +
> >> +title: Rockchip RK3568 Video Capture (VICAP)
> >> +
> >> +maintainers:
> >> +  - Michael Riesch <michael.riesch@...fvision.net>
> >> +
> >> +description:
> >> +  The Rockchip RK3568 Video Capture (VICAP) block features a digital video
> >> +  port (DVP, a parallel video interface) and a MIPI CSI-2 port. It receives
> >> +  the data from camera sensors, video decoders, or other companion ICs and
> >> +  transfers it into system main memory by AXI bus.
> >> +
> >> +properties:
> >> +  compatible:
> >> +    const: rockchip,rk3568-vicap
> >> +
> >> +  reg:
> >> +    maxItems: 1
> >> +
> >> +  interrupts:
> >> +    maxItems: 1
> >> +
> >> +  clocks:
> >> +    items:
> >> +      - description: ACLK
> >> +      - description: HCLK
> >> +      - description: DCLK
> >> +      - description: ICLK
> >> +
> >> +  clock-names:
> >> +    items:
> >> +      - const: aclk
> >> +      - const: hclk
> >> +      - const: dclk
> >> +      - const: iclk
> >> +
> >> +  rockchip,cif-clk-delaynum:
> >> +    $ref: /schemas/types.yaml#/definitions/uint32
> >> +    minimum: 0
> >> +    maximum: 127
> >> +    description:
> >> +      Delay the DVP path clock input to align the sampling phase, only valid
> >> +      in dual edge sampling mode.
> > 
> > I suppose there's further documentation on this somewhere else? A reference
> > would be nice.
> 
> I like your optimism :-) No, I am afraid this single sentence is all the
> the RK3568 TRM has to say about it. I can add a reference to the TRM
> page, but everyone who actually follows this reference will be
> disappointed...

:-(

Is this something that needs to be set? Is there a default, for instance?

If there's documentation available, it'd be nice to refer to that from
somewhere, I'm not sure if the driver or DT bindings would be better.
Probably the driver.

> 
> >> +
> >> +  iommus:
> >> +    maxItems: 1
> >> +
> >> +  resets:
> >> +    items:
> >> +      - description: ARST
> >> +      - description: HRST
> >> +      - description: DRST
> >> +      - description: PRST
> >> +      - description: IRST
> >> +
> >> +  reset-names:
> >> +    items:
> >> +      - const: arst
> >> +      - const: hrst
> >> +      - const: drst
> >> +      - const: prst
> >> +      - const: irst
> >> +
> >> +  rockchip,grf:
> >> +    $ref: /schemas/types.yaml#/definitions/phandle
> >> +    description:
> >> +      Phandle to general register file used for video input block control.
> >> +
> >> +  power-domains:
> >> +    maxItems: 1
> >> +
> >> +  ports:
> >> +    $ref: /schemas/graph.yaml#/properties/ports
> >> +
> >> +    properties:
> >> +      port@0:
> >> +        $ref: /schemas/graph.yaml#/$defs/port-base
> >> +        unevaluatedProperties: false
> >> +        description:
> >> +          The digital video port (DVP, a parallel video interface).
> >> +
> >> +        properties:
> >> +          endpoint:
> >> +            $ref: video-interfaces.yaml#
> >> +            unevaluatedProperties: false
> >> +
> >> +            properties:
> >> +              bus-type:
> >> +                enum: [5, 6]
> >> +
> >> +            required:
> >> +              - bus-type
> >> +
> >> +      port@1:
> >> +        $ref: /schemas/graph.yaml#/$defs/port-base
> >> +        unevaluatedProperties: false
> >> +        description: The MIPI CSI-2 port.
> >> +
> >> +        properties:
> >> +          endpoint:
> >> +            $ref: video-interfaces.yaml#
> >> +            unevaluatedProperties: false
> > 
> > Don't you need things like data-lanes here? Or is this a single lane
> > receiver?
> 
> This may be a bit confusing, and I probably should extend the
> description a bit. This port/endpoint faces the MIPI CSI Host, which has
> its own driver provided in patch 6. The connection in between is a link
> with some internal format. Hence, no properties required.
> 
> This is the same issue as the one discussed in the other thread, since
> the other end of this connection is discussed there. I'll fix the issue
> on both ends using Rob's suggestion.

If this is some custom interface, you should not say it's MIPI CSI-2
(even though MIPI CSI-2 data could be transported on top).

> >> +                vicap_mipi: port@1 {
> >> +                    reg = <1>;
> > 
> > Where is the endpoint?
> 
> I'll add the endpoint in the example.

Thank you.

-- 
Kind regards,

Sakari Ailus