Re: [PATCH v8 03/14] dt-bindings: display: bridge: Add i.MX8qm/qxp pixel combiner binding

Hi Laurent,

On Thu, 2022-06-09 at 11:24 +0300, Laurent Pinchart wrote:

Hi Liu,

Thank you for the patch.

Thank you for the review.

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: https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevicetree.... +$schema: https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevicetree....

+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.

graph.yaml mentions 'ports' node is optional and 'port' node can be a direct property of a device. So, it's legit to use 'port' node without 'ports' and Rob has already provided his R-b tag to this patch.

Regarding generic helper, the DPU KMS driver and the PXL2DPI driver(patch 8/14) call of_graph_get_remote_port_parent() to find the remote pixel combiner channel and ldb channel. So, it looks ok.

This comment applies to all bindings in this series.

Besides this pixel combiner binding, the ldb binding(patch 10/14) is the only one this comment applies to. The i.MX8qxp reference manual mentions both ldb and pixel combiner support 2 channels. That's why the bindings use 'channel' nodes to better reflect HW design. Using 'ports' node in 'channel' node would bring more tabs for indentation, so I prefer not to use it.

Regards, Liu Ying

...

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

•            };
  

•        };
  

•    };
  

• };