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 | 169 +++++++++++++++++++++
MAINTAINERS | 1 +
2 files changed, 170 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..0e61f02a9508
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/rockchip,rk3568-vicap.yaml
@@ -0,0 +1,169 @@
+# 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. Delay is zero by default and can be adjusted
+ optionally.
+
+ 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#/properties/port
+ description: Internal port connected to a MIPI CSI-2 host.
+
+ properties:
+ endpoint:
+ $ref: video-interfaces.yaml#
+ unevaluatedProperties: false
+
+required:
+ - compatible
+ - reg
+ - interrupts
+ - clocks
+ - ports
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/clock/rk3568-cru.h>
+ #include <dt-bindings/interrupt-controller/arm-gic.h>
+ #include <dt-bindings/interrupt-controller/irq.h>
+ #include <dt-bindings/power/rk3568-power.h>
+ #include <dt-bindings/media/video-interfaces.h>
+
+ parent {
+ #address-cells = <2>;
+ #size-cells = <2>;
+
+ vicap: video-capture@fdfe0000 {
+ compatible = "rockchip,rk3568-vicap";
+ reg = <0x0 0xfdfe0000 0x0 0x200>;
+ interrupts = <GIC_SPI 146 IRQ_TYPE_LEVEL_HIGH>;
+ assigned-clocks = <&cru DCLK_VICAP>;
+ assigned-clock-rates = <300000000>;
+ clocks = <&cru ACLK_VICAP>, <&cru HCLK_VICAP>,
+ <&cru DCLK_VICAP>, <&cru ICLK_VICAP_G>;
+ clock-names = "aclk", "hclk", "dclk", "iclk";
+ iommus = <&vicap_mmu>;
+ power-domains = <&power RK3568_PD_VI>;
+ resets = <&cru SRST_A_VICAP>, <&cru SRST_H_VICAP>,
+ <&cru SRST_D_VICAP>, <&cru SRST_P_VICAP>,
+ <&cru SRST_I_VICAP>;
+ reset-names = "arst", "hrst", "drst", "prst", "irst";
+ rockchip,grf = <&grf>;
+
+ ports {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ vicap_dvp: port@0 {
+ reg = <0>;
+
+ vicap_dvp_input: endpoint {
+ bus-type = <MEDIA_BUS_TYPE_BT656>;
+ bus-width = <16>;
+ pclk-sample = <MEDIA_PCLK_SAMPLE_DUAL_EDGE>;
+ remote-endpoint = <&it6801_output>;
+ };
+ };
+
+ vicap_mipi: port@1 {
+ reg = <1>;
+
+ vicap_mipi_input: endpoint {
+ remote-endpoint = <&csi_output>;
+ };
+ };
+ };
+ };
+ };
diff --git a/MAINTAINERS b/MAINTAINERS
index bbfaf35d50c6..cd8fa1afe5eb 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -20407,6 +20407,7 @@ M: Michael Riesch <michael.riesch@wolfvision.net>
L: linux-media@vger.kernel.org
S: Maintained
F: Documentation/devicetree/bindings/media/rockchip,px30-vip.yaml
+F: Documentation/devicetree/bindings/media/rockchip,rk3568-vicap.yaml
ROCKCHIP CRYPTO DRIVERS
M: Corentin Labbe <clabbe@baylibre.com>
--
2.34.1On Thu, Mar 06, 2025 at 05:56:04PM +0100, Michael Riesch wrote:
> Add documentation for the Rockchip RK3568 Video Capture (VICAP) unit.
>
> Signed-off-by: Michael Riesch <michael.riesch@wolfvision.net>
subject: only one media prefix, the first
A nit, subject: drop second/last, redundant "bindings". The
"dt-bindings" prefix is already stating that these are bindings.
See also:
https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18
> ---
> .../bindings/media/rockchip,rk3568-vicap.yaml | 169 +++++++++++++++++++++
> MAINTAINERS | 1 +
> 2 files changed, 170 insertions(+)
>
...
> + 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. Delay is zero by default and can be adjusted
> + optionally.
default: 0
> +
> + 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#/properties/port
> + description: Internal port connected to a MIPI CSI-2 host.
> +
> + properties:
> + endpoint:
> + $ref: video-interfaces.yaml#
> + unevaluatedProperties: false
Hm, does it actually work? graph/port does not allow any other
properties. You should use graph/port-base and probably still narrow
lanes for both of port@0 and port@1.
> +
> +required:
> + - compatible
> + - reg
> + - interrupts
> + - clocks
> + - ports
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/clock/rk3568-cru.h>
> + #include <dt-bindings/interrupt-controller/arm-gic.h>
> + #include <dt-bindings/interrupt-controller/irq.h>
> + #include <dt-bindings/power/rk3568-power.h>
> + #include <dt-bindings/media/video-interfaces.h>
> +
> + parent {
soc {
> + #address-cells = <2>;
> + #size-cells = <2>;
Best regards,
Krzysztof
Hi Krzysztof, Michael,
On Fri, Mar 07, 2025 at 08:51:54AM +0100, Krzysztof Kozlowski wrote:
> On Thu, Mar 06, 2025 at 05:56:04PM +0100, Michael Riesch wrote:
> > Add documentation for the Rockchip RK3568 Video Capture (VICAP) unit.
> >
> > Signed-off-by: Michael Riesch <michael.riesch@wolfvision.net>
>
> subject: only one media prefix, the first
>
> A nit, subject: drop second/last, redundant "bindings". The
> "dt-bindings" prefix is already stating that these are bindings.
> See also:
> https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18
>
> > ---
> > .../bindings/media/rockchip,rk3568-vicap.yaml | 169 +++++++++++++++++++++
> > MAINTAINERS | 1 +
> > 2 files changed, 170 insertions(+)
> >
>
> ...
>
> > + 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. Delay is zero by default and can be adjusted
> > + optionally.
>
> default: 0
And this is technically specific to the DVP port (0). Should (or could?) it
be located there?
>
> > +
> > + 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#/properties/port
> > + description: Internal port connected to a MIPI CSI-2 host.
> > +
> > + properties:
> > + endpoint:
> > + $ref: video-interfaces.yaml#
> > + unevaluatedProperties: false
>
> Hm, does it actually work? graph/port does not allow any other
> properties. You should use graph/port-base and probably still narrow
> lanes for both of port@0 and port@1.
I'd list the relevant properties for both DVP and CSI-2, either as
mandatory or with defaults (could be reasonable for DVP signal polarities
but not e.g. on number of CSI-2 lanes).
>
>
> > +
> > +required:
> > + - compatible
> > + - reg
> > + - interrupts
> > + - clocks
> > + - ports
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > + - |
> > + #include <dt-bindings/clock/rk3568-cru.h>
> > + #include <dt-bindings/interrupt-controller/arm-gic.h>
> > + #include <dt-bindings/interrupt-controller/irq.h>
> > + #include <dt-bindings/power/rk3568-power.h>
> > + #include <dt-bindings/media/video-interfaces.h>
> > +
> > + parent {
>
> soc {
>
> > + #address-cells = <2>;
> > + #size-cells = <2>;
>
> Best regards,
> Krzysztof
>
--
Kind regards,
Sakari Ailus
Hi Krzysztof, Sakari,
Thanks for your feedback! Also, sorry for the delayed response, but as
the e-mail address indicates, there has been a job change in between
that kept me busy :-)
On 3/7/25 10:49, Sakari Ailus wrote:
> Hi Krzysztof, Michael,
>
> On Fri, Mar 07, 2025 at 08:51:54AM +0100, Krzysztof Kozlowski wrote:
>> On Thu, Mar 06, 2025 at 05:56:04PM +0100, Michael Riesch wrote:
>>> Add documentation for the Rockchip RK3568 Video Capture (VICAP) unit.
>>>
>>> Signed-off-by: Michael Riesch <michael.riesch@wolfvision.net>
>>
>> subject: only one media prefix, the first
>>
>> A nit, subject: drop second/last, redundant "bindings". The
>> "dt-bindings" prefix is already stating that these are bindings.
>> See also:
>> https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18
Ack. Plain "media: dt-bindings: add rockchip rk3568 vicap" it is, then.
>>
>>> ---
>>> .../bindings/media/rockchip,rk3568-vicap.yaml | 169 +++++++++++++++++++++
>>> MAINTAINERS | 1 +
>>> 2 files changed, 170 insertions(+)
>>>
>>
>> ...
>>
>>> + 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. Delay is zero by default and can be adjusted
>>> + optionally.
>>
>> default: 0
Ack.
>
> And this is technically specific to the DVP port (0). Should (or could?) it
> be located there?
"Should"? Yes, makes sense to me.
"Could"? I guess, as we are referencing port-base here it should be
feasible. Not an expert opinion, mind you.
>
>>
>>> +
>>> + 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#/properties/port
>>> + description: Internal port connected to a MIPI CSI-2 host.
>>> +
>>> + properties:
>>> + endpoint:
>>> + $ref: video-interfaces.yaml#
>>> + unevaluatedProperties: false
>>
>> Hm, does it actually work? graph/port does not allow any other
>> properties. You should use graph/port-base and probably still narrow
>> lanes for both of port@0 and port@1.
>
> I'd list the relevant properties for both DVP and CSI-2, either as
> mandatory or with defaults (could be reasonable for DVP signal polarities
> but not e.g. on number of CSI-2 lanes).
Not sure whether we are on the same page here. As pointed out in the
last round of feedback
(https://lore.kernel.org/all/0b19c544-f773-435e-9829-aaaa1c6daf7a@wolfvision.net/),
port@1 is not MIPI CSI, but some internal interface.
I tried to clarify this by changing the description of this port to
"Internal port connected to a MIPI CSI-2 host." The host (see
rockchip,rk3568-mipi-csi.yaml) has a port that is actually MIPI CSI and
one port that is the other end of port@1 here.
As to port@1 here, I am not aware of any properties that can be set. Not
even very peculiar ones similar to rockchip,cif-clk-delaynum. Should I
have overlooked something, I think we can relax the constraints, but we
should start strict, right?
>
>>
>>
>>> +
>>> +required:
>>> + - compatible
>>> + - reg
>>> + - interrupts
>>> + - clocks
>>> + - ports
>>> +
>>> +additionalProperties: false
>>> +
>>> +examples:
>>> + - |
>>> + #include <dt-bindings/clock/rk3568-cru.h>
>>> + #include <dt-bindings/interrupt-controller/arm-gic.h>
>>> + #include <dt-bindings/interrupt-controller/irq.h>
>>> + #include <dt-bindings/power/rk3568-power.h>
>>> + #include <dt-bindings/media/video-interfaces.h>
>>> +
>>> + parent {
>>
>> soc {
Ack.
Best regards,
Michael
>>
>>> + #address-cells = <2>;
>>> + #size-cells = <2>;
>>
>> Best regards,
>> Krzysztof
>>
>
On Mon, Apr 28, 2025 at 10:11:51AM +0200, Michael Riesch wrote:
> Hi Krzysztof, Sakari,
>
> Thanks for your feedback! Also, sorry for the delayed response, but as
> the e-mail address indicates, there has been a job change in between
> that kept me busy :-)
>
> On 3/7/25 10:49, Sakari Ailus wrote:
> > Hi Krzysztof, Michael,
> >
> > On Fri, Mar 07, 2025 at 08:51:54AM +0100, Krzysztof Kozlowski wrote:
> >> On Thu, Mar 06, 2025 at 05:56:04PM +0100, Michael Riesch wrote:
> >>> Add documentation for the Rockchip RK3568 Video Capture (VICAP) unit.
> >>>
> >>> Signed-off-by: Michael Riesch <michael.riesch@wolfvision.net>
> >>
> >> subject: only one media prefix, the first
> >>
> >> A nit, subject: drop second/last, redundant "bindings". The
> >> "dt-bindings" prefix is already stating that these are bindings.
> >> See also:
> >> https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18
>
> Ack. Plain "media: dt-bindings: add rockchip rk3568 vicap" it is, then.
>
> >>
> >>> ---
> >>> .../bindings/media/rockchip,rk3568-vicap.yaml | 169 +++++++++++++++++++++
> >>> MAINTAINERS | 1 +
> >>> 2 files changed, 170 insertions(+)
> >>>
> >>
> >> ...
> >>
> >>> + 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. Delay is zero by default and can be adjusted
> >>> + optionally.
> >>
> >> default: 0
>
> Ack.
>
> >
> > And this is technically specific to the DVP port (0). Should (or could?) it
> > be located there?
>
> "Should"? Yes, makes sense to me.
> "Could"? I guess, as we are referencing port-base here it should be
> feasible. Not an expert opinion, mind you.
>
> >
> >>
> >>> +
> >>> + 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#/properties/port
> >>> + description: Internal port connected to a MIPI CSI-2 host.
> >>> +
> >>> + properties:
> >>> + endpoint:
> >>> + $ref: video-interfaces.yaml#
> >>> + unevaluatedProperties: false
> >>
> >> Hm, does it actually work? graph/port does not allow any other
> >> properties. You should use graph/port-base and probably still narrow
> >> lanes for both of port@0 and port@1.
> >
> > I'd list the relevant properties for both DVP and CSI-2, either as
> > mandatory or with defaults (could be reasonable for DVP signal polarities
> > but not e.g. on number of CSI-2 lanes).
>
> Not sure whether we are on the same page here. As pointed out in the
> last round of feedback
> (https://lore.kernel.org/all/0b19c544-f773-435e-9829-aaaa1c6daf7a@wolfvision.net/),
> port@1 is not MIPI CSI, but some internal interface.
>
> I tried to clarify this by changing the description of this port to
> "Internal port connected to a MIPI CSI-2 host." The host (see
> rockchip,rk3568-mipi-csi.yaml) has a port that is actually MIPI CSI and
> one port that is the other end of port@1 here.
I'd write "Port connected to the MIPI CSI-2 receiver output". We use
"receiver" instead of "host".
> As to port@1 here, I am not aware of any properties that can be set. Not
> even very peculiar ones similar to rockchip,cif-clk-delaynum. Should I
> have overlooked something, I think we can relax the constraints, but we
> should start strict, right?
>
> >>> +
> >>> +required:
> >>> + - compatible
> >>> + - reg
> >>> + - interrupts
> >>> + - clocks
> >>> + - ports
> >>> +
> >>> +additionalProperties: false
> >>> +
> >>> +examples:
> >>> + - |
> >>> + #include <dt-bindings/clock/rk3568-cru.h>
> >>> + #include <dt-bindings/interrupt-controller/arm-gic.h>
> >>> + #include <dt-bindings/interrupt-controller/irq.h>
> >>> + #include <dt-bindings/power/rk3568-power.h>
> >>> + #include <dt-bindings/media/video-interfaces.h>
> >>> +
> >>> + parent {
> >>
> >> soc {
>
> Ack.
>
> >>> + #address-cells = <2>;
> >>> + #size-cells = <2>;
--
Regards,
Laurent Pinchart
Hi Laurent,
On 4/28/25 11:22, Laurent Pinchart wrote:
> On Mon, Apr 28, 2025 at 10:11:51AM +0200, Michael Riesch wrote:
>> Hi Krzysztof, Sakari,
>>
>> Thanks for your feedback! Also, sorry for the delayed response, but as
>> the e-mail address indicates, there has been a job change in between
>> that kept me busy :-)
>>
>> On 3/7/25 10:49, Sakari Ailus wrote:
>>> Hi Krzysztof, Michael,
>>>
>>> On Fri, Mar 07, 2025 at 08:51:54AM +0100, Krzysztof Kozlowski wrote:
>>>> On Thu, Mar 06, 2025 at 05:56:04PM +0100, Michael Riesch wrote:
>>>>> Add documentation for the Rockchip RK3568 Video Capture (VICAP) unit.
>>>>>
>>>>> Signed-off-by: Michael Riesch <michael.riesch@wolfvision.net>
>>>>
>>>> subject: only one media prefix, the first
>>>>
>>>> A nit, subject: drop second/last, redundant "bindings". The
>>>> "dt-bindings" prefix is already stating that these are bindings.
>>>> See also:
>>>> https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18
>>
>> Ack. Plain "media: dt-bindings: add rockchip rk3568 vicap" it is, then.
>>
>>>>
>>>>> ---
>>>>> .../bindings/media/rockchip,rk3568-vicap.yaml | 169 +++++++++++++++++++++
>>>>> MAINTAINERS | 1 +
>>>>> 2 files changed, 170 insertions(+)
>>>>>
>>>>
>>>> ...
>>>>
>>>>> + 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. Delay is zero by default and can be adjusted
>>>>> + optionally.
>>>>
>>>> default: 0
>>
>> Ack.
>>
>>>
>>> And this is technically specific to the DVP port (0). Should (or could?) it
>>> be located there?
>>
>> "Should"? Yes, makes sense to me.
>> "Could"? I guess, as we are referencing port-base here it should be
>> feasible. Not an expert opinion, mind you.
>>
>>>
>>>>
>>>>> +
>>>>> + 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#/properties/port
>>>>> + description: Internal port connected to a MIPI CSI-2 host.
>>>>> +
>>>>> + properties:
>>>>> + endpoint:
>>>>> + $ref: video-interfaces.yaml#
>>>>> + unevaluatedProperties: false
>>>>
>>>> Hm, does it actually work? graph/port does not allow any other
>>>> properties. You should use graph/port-base and probably still narrow
>>>> lanes for both of port@0 and port@1.
>>>
>>> I'd list the relevant properties for both DVP and CSI-2, either as
>>> mandatory or with defaults (could be reasonable for DVP signal polarities
>>> but not e.g. on number of CSI-2 lanes).
>>
>> Not sure whether we are on the same page here. As pointed out in the
>> last round of feedback
>> (https://lore.kernel.org/all/0b19c544-f773-435e-9829-aaaa1c6daf7a@wolfvision.net/),
>> port@1 is not MIPI CSI, but some internal interface.
>>
>> I tried to clarify this by changing the description of this port to
>> "Internal port connected to a MIPI CSI-2 host." The host (see
>> rockchip,rk3568-mipi-csi.yaml) has a port that is actually MIPI CSI and
>> one port that is the other end of port@1 here.
>
> I'd write "Port connected to the MIPI CSI-2 receiver output". We use
> "receiver" instead of "host".
Ack. I'll adjust the "host" -> "receiver" wording change in all the
other places as well.
Regards,
Michael
>
>> As to port@1 here, I am not aware of any properties that can be set. Not
>> even very peculiar ones similar to rockchip,cif-clk-delaynum. Should I
>> have overlooked something, I think we can relax the constraints, but we
>> should start strict, right?
>>
>>>>> +
>>>>> +required:
>>>>> + - compatible
>>>>> + - reg
>>>>> + - interrupts
>>>>> + - clocks
>>>>> + - ports
>>>>> +
>>>>> +additionalProperties: false
>>>>> +
>>>>> +examples:
>>>>> + - |
>>>>> + #include <dt-bindings/clock/rk3568-cru.h>
>>>>> + #include <dt-bindings/interrupt-controller/arm-gic.h>
>>>>> + #include <dt-bindings/interrupt-controller/irq.h>
>>>>> + #include <dt-bindings/power/rk3568-power.h>
>>>>> + #include <dt-bindings/media/video-interfaces.h>
>>>>> +
>>>>> + parent {
>>>>
>>>> soc {
>>
>> Ack.
>>
>>>>> + #address-cells = <2>;
>>>>> + #size-cells = <2>;
>
On Mon, Apr 28, 2025 at 11:48:57AM +0200, Michael Riesch wrote:
> On 4/28/25 11:22, Laurent Pinchart wrote:
> > On Mon, Apr 28, 2025 at 10:11:51AM +0200, Michael Riesch wrote:
> >> Hi Krzysztof, Sakari,
> >>
> >> Thanks for your feedback! Also, sorry for the delayed response, but as
> >> the e-mail address indicates, there has been a job change in between
> >> that kept me busy :-)
> >>
> >> On 3/7/25 10:49, Sakari Ailus wrote:
> >>> Hi Krzysztof, Michael,
> >>>
> >>> On Fri, Mar 07, 2025 at 08:51:54AM +0100, Krzysztof Kozlowski wrote:
> >>>> On Thu, Mar 06, 2025 at 05:56:04PM +0100, Michael Riesch wrote:
> >>>>> Add documentation for the Rockchip RK3568 Video Capture (VICAP) unit.
> >>>>>
> >>>>> Signed-off-by: Michael Riesch <michael.riesch@wolfvision.net>
> >>>>
> >>>> subject: only one media prefix, the first
> >>>>
> >>>> A nit, subject: drop second/last, redundant "bindings". The
> >>>> "dt-bindings" prefix is already stating that these are bindings.
> >>>> See also:
> >>>> https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18
> >>
> >> Ack. Plain "media: dt-bindings: add rockchip rk3568 vicap" it is, then.
> >>
> >>>>
> >>>>> ---
> >>>>> .../bindings/media/rockchip,rk3568-vicap.yaml | 169 +++++++++++++++++++++
> >>>>> MAINTAINERS | 1 +
> >>>>> 2 files changed, 170 insertions(+)
> >>>>>
> >>>>
> >>>> ...
> >>>>
> >>>>> + 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. Delay is zero by default and can be adjusted
> >>>>> + optionally.
> >>>>
> >>>> default: 0
> >>
> >> Ack.
> >>
> >>>
> >>> And this is technically specific to the DVP port (0). Should (or could?) it
> >>> be located there?
> >>
> >> "Should"? Yes, makes sense to me.
> >> "Could"? I guess, as we are referencing port-base here it should be
> >> feasible. Not an expert opinion, mind you.
> >>
> >>>
> >>>>
> >>>>> +
> >>>>> + 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#/properties/port
> >>>>> + description: Internal port connected to a MIPI CSI-2 host.
> >>>>> +
> >>>>> + properties:
> >>>>> + endpoint:
> >>>>> + $ref: video-interfaces.yaml#
> >>>>> + unevaluatedProperties: false
> >>>>
> >>>> Hm, does it actually work? graph/port does not allow any other
> >>>> properties. You should use graph/port-base and probably still narrow
> >>>> lanes for both of port@0 and port@1.
> >>>
> >>> I'd list the relevant properties for both DVP and CSI-2, either as
> >>> mandatory or with defaults (could be reasonable for DVP signal polarities
> >>> but not e.g. on number of CSI-2 lanes).
> >>
> >> Not sure whether we are on the same page here. As pointed out in the
> >> last round of feedback
> >> (https://lore.kernel.org/all/0b19c544-f773-435e-9829-aaaa1c6daf7a@wolfvision.net/),
> >> port@1 is not MIPI CSI, but some internal interface.
> >>
> >> I tried to clarify this by changing the description of this port to
> >> "Internal port connected to a MIPI CSI-2 host." The host (see
> >> rockchip,rk3568-mipi-csi.yaml) has a port that is actually MIPI CSI and
> >> one port that is the other end of port@1 here.
> >
> > I'd write "Port connected to the MIPI CSI-2 receiver output". We use
> > "receiver" instead of "host".
>
> Ack. I'll adjust the "host" -> "receiver" wording change in all the
> other places as well.
You can keep "host" when you quote documentation if it uses that
vocabulary, but for generic usage, "receiver" is better.
> >> As to port@1 here, I am not aware of any properties that can be set. Not
> >> even very peculiar ones similar to rockchip,cif-clk-delaynum. Should I
> >> have overlooked something, I think we can relax the constraints, but we
> >> should start strict, right?
> >>
> >>>>> +
> >>>>> +required:
> >>>>> + - compatible
> >>>>> + - reg
> >>>>> + - interrupts
> >>>>> + - clocks
> >>>>> + - ports
> >>>>> +
> >>>>> +additionalProperties: false
> >>>>> +
> >>>>> +examples:
> >>>>> + - |
> >>>>> + #include <dt-bindings/clock/rk3568-cru.h>
> >>>>> + #include <dt-bindings/interrupt-controller/arm-gic.h>
> >>>>> + #include <dt-bindings/interrupt-controller/irq.h>
> >>>>> + #include <dt-bindings/power/rk3568-power.h>
> >>>>> + #include <dt-bindings/media/video-interfaces.h>
> >>>>> +
> >>>>> + parent {
> >>>>
> >>>> soc {
> >>
> >> Ack.
> >>
> >>>>> + #address-cells = <2>;
> >>>>> + #size-cells = <2>;
--
Regards,
Laurent Pinchart
© 2016 - 2026 Red Hat, Inc.