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

[Sakari Ailus <sakari.ailus@linux.intel.com>]

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@wolfvision.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@wolfvision.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