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

Michael Riesch
2025-04-28T09:51:17Z

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>;
>