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

[Michael Riesch]

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 at 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 at 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 at 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 at 0 and port at 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 at 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 at 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 at 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>;
>