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

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

On Thu, 16 Nov 2023 12:04:38 +0100, 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
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Documentation/devicetree/bindings/media/rockchip,px30-vip.example.dtb: /example-0/parent/i2c/video-decoder@44: failed to match any schema with compatible: ['techwell,tw9900']

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/a0af1d30e79fb1f2567297c951781996836d6db6.1700132457.git.mehdi.djait@bootlin.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

On Thu, Nov 16, 2023 at 06:26:53AM -0600, Rob Herring wrote:
> 
> On Thu, 16 Nov 2023 12:04:38 +0100, 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
> > 
> 
> My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
> on your patch (DT_CHECKER_FLAGS is new in v5.13):
> 
> yamllint warnings/errors:
> 
> dtschema/dtc warnings/errors:
> Documentation/devicetree/bindings/media/rockchip,px30-vip.example.dtb: /example-0/parent/i2c/video-decoder@44: failed to match any schema with compatible: ['techwell,tw9900']

There's a reviewed binding for this on the list at present:
https://lore.kernel.org/all/169947001607.2754020.4176816227714592571.robh@kernel.org/

Perhaps a poor choice however for an example in a another (unrelated?)
series.

Cheers,
Conor.

On Thu, Nov 16, 2023 at 12:04:38PM +0100, Mehdi Djait wrote:
> Add a documentation for the Rockchip Camera Interface binding.
> 
> Signed-off-by: Mehdi Djait <mehdi.djait@bootlin.com>

Provided the undocumented compatible in the example is resolved either
by sequencing, or by swapping out for another device:
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

Cheers,
Conor.

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