Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 9 Jun 2022 11:24:11 +0300
From:   Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To:     Liu Ying <victor.liu@nxp.com>
Cc:     dri-devel@lists.freedesktop.org, devicetree@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        linux-media@vger.kernel.org, andrzej.hajda@intel.com,
        narmstrong@baylibre.com, robert.foss@linaro.org, jonas@kwiboo.se,
        jernej.skrabec@gmail.com, airlied@linux.ie, daniel@ffwll.ch,
        robh+dt@kernel.org, krzysztof.kozlowski+dt@linaro.org,
        shawnguo@kernel.org, s.hauer@pengutronix.de, kernel@pengutronix.de,
        festevam@gmail.com, linux-imx@nxp.com, lee.jones@linaro.org,
        mchehab@kernel.org, marcel.ziswiler@toradex.com,
        Rob Herring <robh@kernel.org>
Subject: Re: [PATCH v8 03/14] dt-bindings: display: bridge: Add i.MX8qm/qxp
 pixel combiner binding
Message-ID: <YqGuK9g0g2XsJV+x@pendragon.ideasonboard.com>
References: <20220609064931.3068601-1-victor.liu@nxp.com>
 <20220609064931.3068601-4-victor.liu@nxp.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20220609064931.3068601-4-victor.liu@nxp.com>
Precedence: bulk

Hi Liu,

Thank you for the patch.

On Thu, Jun 09, 2022 at 02:49:20PM +0800, Liu Ying wrote:
> This patch adds bindings for i.MX8qm/qxp pixel combiner.
> 
> Reviewed-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Liu Ying <victor.liu@nxp.com>
> ---
> v7->v8:
> * No change.
> 
> v6->v7:
> * No change.
> 
> v5->v6:
> * No change.
> 
> v4->v5:
> * No change.
> 
> v3->v4:
> * No change.
> 
> v2->v3:
> * Add Rob's R-b tag.
> 
> v1->v2:
> * Use graph schema. (Laurent)
> * Use enum instead of oneOf + const for the reg property of pixel combiner
>   channels. (Rob)
> 
>  .../bridge/fsl,imx8qxp-pixel-combiner.yaml    | 144 ++++++++++++++++++
>  1 file changed, 144 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/bridge/fsl,imx8qxp-pixel-combiner.yaml
> 
> diff --git a/Documentation/devicetree/bindings/display/bridge/fsl,imx8qxp-pixel-combiner.yaml b/Documentation/devicetree/bindings/display/bridge/fsl,imx8qxp-pixel-combiner.yaml
> new file mode 100644
> index 000000000000..50bae2122183
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/bridge/fsl,imx8qxp-pixel-combiner.yaml
> @@ -0,0 +1,144 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/bridge/fsl,imx8qxp-pixel-combiner.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Freescale i.MX8qm/qxp Pixel Combiner
> +
> +maintainers:
> +  - Liu Ying <victor.liu@nxp.com>
> +
> +description: |
> +  The Freescale i.MX8qm/qxp Pixel Combiner takes two output streams from a
> +  single display controller and manipulates the two streams to support a number
> +  of modes(bypass, pixel combine, YUV444 to YUV422, split_RGB) configured as
> +  either one screen, two screens, or virtual screens.  The pixel combiner is
> +  also responsible for generating some of the control signals for the pixel link
> +  output channel.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - fsl,imx8qm-pixel-combiner
> +      - fsl,imx8qxp-pixel-combiner
> +
> +  "#address-cells":
> +    const: 1
> +
> +  "#size-cells":
> +    const: 0
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +  clock-names:
> +    const: apb
> +
> +  power-domains:
> +    maxItems: 1
> +
> +patternProperties:
> +  "^channel@[0-1]$":
> +    type: object
> +    description: Represents a display stream of pixel combiner.
> +
> +    properties:
> +      "#address-cells":
> +        const: 1
> +
> +      "#size-cells":
> +        const: 0
> +
> +      reg:
> +        description: The display stream index.
> +        enum: [ 0, 1 ]
> +
> +      port@0:
> +        $ref: /schemas/graph.yaml#/properties/port
> +        description: Input endpoint of the display stream.
> +
> +      port@1:
> +        $ref: /schemas/graph.yaml#/properties/port
> +        description: Output endpoint of the display stream.

When multiple ports are present, they are usually grouped in a "ports"
node. Not doing say may work from a schema point of view but makes
implementation of generic helpers more difficult. Unless Rob thinks
"ports" is really not needed here, I'd add it.

This comment applies to all bindings in this series.

> +
> +    required:
> +      - "#address-cells"
> +      - "#size-cells"
> +      - reg
> +      - port@0
> +      - port@1
> +
> +    additionalProperties: false
> +
> +required:
> +  - compatible
> +  - "#address-cells"
> +  - "#size-cells"
> +  - reg
> +  - clocks
> +  - clock-names
> +  - power-domains
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/imx8-lpcg.h>
> +    #include <dt-bindings/firmware/imx/rsrc.h>
> +    pixel-combiner@56020000 {
> +        compatible = "fsl,imx8qxp-pixel-combiner";
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +        reg = <0x56020000 0x10000>;
> +        clocks = <&dc0_pixel_combiner_lpcg IMX_LPCG_CLK_4>;
> +        clock-names = "apb";
> +        power-domains = <&pd IMX_SC_R_DC_0>;
> +
> +        channel@0 {
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +            reg = <0>;
> +
> +            port@0 {
> +                reg = <0>;
> +
> +                dc0_pixel_combiner_ch0_dc0_dpu_disp0: endpoint {
> +                    remote-endpoint = <&dc0_dpu_disp0_dc0_pixel_combiner_ch0>;
> +                };
> +            };
> +
> +            port@1 {
> +                reg = <1>;
> +
> +                dc0_pixel_combiner_ch0_dc0_pixel_link0: endpoint {
> +                    remote-endpoint = <&dc0_pixel_link0_dc0_pixel_combiner_ch0>;
> +                };
> +            };
> +        };
> +
> +        channel@1 {
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +            reg = <1>;
> +
> +            port@0 {
> +                reg = <0>;
> +
> +                dc0_pixel_combiner_ch1_dc0_dpu_disp1: endpoint {
> +                    remote-endpoint = <&dc0_dpu_disp1_dc0_pixel_combiner_ch1>;
> +                };
> +            };
> +
> +            port@1 {
> +                reg = <1>;
> +
> +                dc0_pixel_combiner_ch1_dc0_pixel_link1: endpoint {
> +                    remote-endpoint = <&dc0_pixel_link1_dc0_pixel_combiner_ch1>;
> +                };
> +            };
> +        };
> +    };

-- 
Regards,

Laurent Pinchart