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@xxxxxxxxxx> > > Signed-off-by: Liu Ying <victor.liu@xxxxxxx> > > --- > > 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.org%2Fschemas%2Fdisplay%2Fbridge%2Ffsl%2Cimx8qxp-pixel-combiner.yaml%23&data=05%7C01%7Cvictor.liu%40nxp.com%7Ca94c4502ac5748e2357408da49f17304%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637903598635803241%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=RIF8RJtIGU1gCGwSD%2FMsNx%2Bo7Kd9cxkInm310B1npq4%3D&reserved=0 > > +$schema: > > https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevicetree.org%2Fmeta-schemas%2Fcore.yaml%23&data=05%7C01%7Cvictor.liu%40nxp.com%7Ca94c4502ac5748e2357408da49f17304%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637903598635803241%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=bQy5KKStqN17%2FnK8vGM%2F1nGhlkxdF%2FfYVDwGReWsbes%3D&reserved=0 > > + > > +title: Freescale i.MX8qm/qxp Pixel Combiner > > + > > +maintainers: > > + - Liu Ying <victor.liu@xxxxxxx> > > + > > +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>; > > + }; > > + }; > > + }; > > + }; > >
