Re: [PATCH v11 1/3] media: dt-bindings: media: add bindings for Rockchip CIF

[Michael Riesch <michael.riesch@wolfvision.net>]

Hi Mehdi,

On 11/16/23 12:04, Mehdi Djait wrote:
> Add a documentation for the Rockchip Camera Interface binding.
> 
> Signed-off-by: Mehdi Djait <mehdi.djait@bootlin.com>
> ---
>  .../bindings/media/rockchip,px30-vip.yaml     | 173 ++++++++++++++++++
>  1 file changed, 173 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/rockchip,px30-vip.yaml
> 
> diff --git a/Documentation/devicetree/bindings/media/rockchip,px30-vip.yaml b/Documentation/devicetree/bindings/media/rockchip,px30-vip.yaml
> new file mode 100644
> index 000000000000..580ed654000c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/rockchip,px30-vip.yaml
> @@ -0,0 +1,173 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/media/rockchip,px30-vip.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Rockchip CIF Camera Interface

May I suggest "Rockchip Camera Interface (CIF)"?

> +
> +maintainers:
> +  - Mehdi Djait <mehdi.djait@bootlin.com>
> +
> +description:
> +  CIF is a camera interface present on some rockchip SoCs. It receives the data

s/rockchip/Rockchip

> +  from Camera sensor or CCIR656 encoder and transfers it into system main memory
> +  by AXI bus.
> +
> +properties:
> +  compatible:
> +    const: rockchip,px30-vip
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  clocks:
> +    items:
> +      - description: ACLK
> +      - description: HCLK
> +      - description: PCLK
> +
> +  clock-names:
> +    items:
> +      - const: aclk
> +      - const: hclk
> +      - const: pclk
> +
> +  resets:
> +    items:
> +      - description: AXI
> +      - description: AHB
> +      - description: PCLK IN
> +
> +  reset-names:
> +    items:
> +      - const: axi
> +      - const: ahb
> +      - const: pclkin
> +
> +  power-domains:
> +    maxItems: 1
> +
> +  ports:
> +    $ref: /schemas/graph.yaml#/properties/ports
> +
> +    properties:
> +      port@0:
> +        $ref: /schemas/graph.yaml#/$defs/port-base
> +        unevaluatedProperties: false
> +        description: input port on the parallel interface

In more recent Rockchip SoCs this seems to be described as "DVP
interface (digital parallel input)". Maybe we could use this description
here as well?

> +
> +        properties:
> +          endpoint:
> +            $ref: video-interfaces.yaml#
> +            unevaluatedProperties: false
> +
> +            properties:
> +              bus-type:
> +                enum: [5, 6]

Not sure whether this is possible, but if we could use the
MEDIA_BUS_TYPE_PARALLEL	and MEDIA_BUS_TYPE_BT656 defines here it would
be way more descriptive.

> +
> +            required:
> +              - bus-type
> +
> +    required:
> +      - port@0
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - clocks
> +  - ports
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/px30-cru.h>
> +    #include <dt-bindings/display/sdtv-standards.h>
> +    #include <dt-bindings/gpio/gpio.h>
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/media/video-interfaces.h>
> +    #include <dt-bindings/power/px30-power.h>
> +
> +    parent {
> +        #address-cells = <2>;
> +        #size-cells = <2>;
> +
> +        video-capture@ff490000 {
> +            compatible = "rockchip,px30-vip";
> +            reg = <0x0 0xff490000 0x0 0x200>;
> +            interrupts = <GIC_SPI 69 IRQ_TYPE_LEVEL_HIGH>;
> +            clocks = <&cru ACLK_CIF>, <&cru HCLK_CIF>, <&cru PCLK_CIF>;
> +            clock-names = "aclk", "hclk", "pclk";
> +            resets = <&cru SRST_CIF_A>, <&cru SRST_CIF_H>, <&cru SRST_CIF_PCLKIN>;
> +            reset-names = "axi", "ahb", "pclkin";
> +            power-domains = <&power PX30_PD_VI>;

Sort alphabetically: power-domains before resets.

> +
> +            ports {
> +                #address-cells = <1>;
> +                #size-cells = <0>;
> +
> +                port@0 {
> +                    reg = <0>;
> +
> +                    cif_in: endpoint {
> +                        remote-endpoint = <&tw9900_out>;
> +                        bus-type = <MEDIA_BUS_TYPE_BT656>;
> +                    };
> +                };
> +            };
> +        };
> +
> +        composite_connector {
> +            compatible = "composite-video-connector";
> +            label = "tv";
> +            sdtv-standards = <(SDTV_STD_PAL | SDTV_STD_NTSC)>;
> +
> +            port {
> +                composite_to_tw9900: endpoint {
> +                    remote-endpoint = <&tw9900_to_composite>;
> +                };
> +            };
> +        };
> +
> +        i2c {
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +
> +            video-decoder@44 {
> +                compatible = "techwell,tw9900";
> +                reg = <0x44>;
> +
> +                vdd-supply = <&tw9900_supply>;
> +                reset-gpios = <&gpio2 5 GPIO_ACTIVE_LOW>;

This goes before vdd-supply (alphabetical sorting). No need for blank
lines between the properties.

> +
> +                ports {
> +                    #address-cells = <1>;
> +                    #size-cells = <0>;
> +
> +                    port@0 {
> +                        #address-cells = <1>;
> +                        #size-cells = <0>;
> +
> +                        reg = <0>;

I think reg property goes first, then the others. No blank lines between
properties, but one blank line between properties and nodes.

> +                        tw9900_to_composite: endpoint@0 {
> +                            reg = <0>;
> +                            remote-endpoint = <&composite_to_tw9900>;
> +                        };
> +                    };
> +
> +                    port@1 {
> +                        reg = <1>;

Same here.

> +                        tw9900_out: endpoint {
> +                            remote-endpoint = <&cif_in>;
> +                        };
> +                    };
> +                };
> +            };
> +        };
> +    };
> +...

With the inline comments addressed,

Reviewed-by: Michael Riesch <michael.riesch@wolfvision.net>

Thanks for your efforts!

Best regards,
Michael