Re: [EXT] Re: [PATCH v6 2/8] dt-bindings: display: bridge: Add Cadence MHDP8501 HDMI and DP
On 21/06/2023 04:23, Sandor Yu wrote: >>> + >>> +properties: >>> + compatible: >>> +enum: >>> + - cdns,mhdp8501-dp >>> + - cdns,mhdp8501-hdmi >>> + - fsl,imx8mq-mhdp8501-dp >>> + - fsl,imx8mq-mhdp8501-hdmi >> >> Is DP vs. HDMI fixed for a particular SoC implementation or it's a board >> level >> decision. In the latter case, the type of connector should determine the >> mode, >> not compatible. > DP or HDMI is bord level decision. Then it's a connector, not compatible. > Because DP and HDMI have different initialize process and less functions > could be reuse, so they have different drivers. How do you organize drivers is independent of bindings. > Please check it in patch > [PATCH v6 3/8] drm: bridge: Cadence: Add MHDP8501 DP driver > [PATCH v6 5/8] drm: bridge: Cadence: Add MHDP8501 HDMI driver > > If use the type of connector to determine the mode, hdmi and DP driver have > to combine into one driver. > So the compatible may the better choice. Why? Because one driver implementation tells you to do that? Bindings are for hardware, not for driver, so whatever you have to do in drivers is not convincing argument. Best regards, Krzysztof
RE: [EXT] Re: [PATCH v6 2/8] dt-bindings: display: bridge: Add Cadence MHDP8501 HDMI and DP
Hi Rob, Thanks for your comments, > -Original Message- > From: Rob Herring > Sent: 2023年6月20日 23:49 > To: Sandor Yu > Cc: andrzej.ha...@intel.com; neil.armstr...@linaro.org; > robert.f...@linaro.org; laurent.pinch...@ideasonboard.com; > jo...@kwiboo.se; jernej.skra...@gmail.com; airl...@gmail.com; > dan...@ffwll.ch; krzysztof.kozlowski...@linaro.org; shawn...@kernel.org; > s.ha...@pengutronix.de; feste...@gmail.com; vk...@kernel.org; > dri-devel@lists.freedesktop.org; devicet...@vger.kernel.org; > linux-arm-ker...@lists.infradead.org; linux-ker...@vger.kernel.org; > linux-...@lists.infradead.org; ker...@pengutronix.de; dl-linux-imx > ; Oliver Brown > Subject: [EXT] Re: [PATCH v6 2/8] dt-bindings: display: bridge: Add Cadence > MHDP8501 HDMI and DP > > Caution: This is an external email. Please take care when clicking links or > opening attachments. When in doubt, report the message using the 'Report > this email' button > > > On Thu, Jun 15, 2023 at 09:38:12AM +0800, Sandor Yu wrote: > > Add bindings for Cadence MHDP8501 DisplayPort and HDMI driver. > > Bindings are for h/w, not a driver. OK, I will change it in the next version. > > > > > Signed-off-by: Sandor Yu > > --- > > .../display/bridge/cdns,mhdp8501.yaml | 105 > ++ > > 1 file changed, 105 insertions(+) > > create mode 100644 > > Documentation/devicetree/bindings/display/bridge/cdns,mhdp8501.yaml > > > > diff --git > > a/Documentation/devicetree/bindings/display/bridge/cdns,mhdp8501.yaml > > b/Documentation/devicetree/bindings/display/bridge/cdns,mhdp8501.yaml > > new file mode 100644 > > index ..a54756815e6f > > --- /dev/null > > +++ > b/Documentation/devicetree/bindings/display/bridge/cdns,mhdp8501.y > > +++ aml > > @@ -0,0 +1,105 @@ > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) %YAML 1.2 > > +--- > > +$id: > > +https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevi > > > +cetree.org%2Fschemas%2Fdisplay%2Fbridge%2Fcdns%2Cmhdp8501.yaml% > 23&dat > > > +a=05%7C01%7CSandor.yu%40nxp.com%7C4d4e118d60d744b5dba708db71 > a5de79%7C > > > +686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C63822872943965530 > 2%7CUnkno > > > +wn%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1 > haWwi > > > +LCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=UEsMdkZBmfD7tM1wzJ71 > DHQoi4zVOkpT > > +A9TNE7Rxn%2B8%3D&reserved=0 > > +$schema: > > +https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevi > > > +cetree.org%2Fmeta-schemas%2Fcore.yaml%23&data=05%7C01%7CSandor. > yu%40n > > > +xp.com%7C4d4e118d60d744b5dba708db71a5de79%7C686ea1d3bc2b4c6fa > 92cd99c5 > > > +c301635%7C0%7C0%7C638228729439655302%7CUnknown%7CTWFpbGZs > b3d8eyJWIjoi > > > +MC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3 > 000%7C% > > > +7C%7C&sdata=Zu3v0yG2BXWXvTWV5oLiGvdu3O3PhK%2FrYNJIS2zHwpI%3 > D&reserved > > +=0 > > + > > +title: Cadence MHDP8501 Displayport bridge > > + > > +maintainers: > > + - Sandor Yu > > + > > +description: > > + The Cadence MHDP8501 Displayport/HDMI TX interface. > > + > > +properties: > > + compatible: > > +enum: > > + - cdns,mhdp8501-dp > > + - cdns,mhdp8501-hdmi > > + - fsl,imx8mq-mhdp8501-dp > > + - fsl,imx8mq-mhdp8501-hdmi > > Is DP vs. HDMI fixed for a particular SoC implementation or it's a board level > decision. In the latter case, the type of connector should determine the mode, > not compatible. DP or HDMI is bord level decision. Because DP and HDMI have different initialize process and less functions could be reuse, so they have different drivers. Please check it in patch [PATCH v6 3/8] drm: bridge: Cadence: Add MHDP8501 DP driver [PATCH v6 5/8] drm: bridge: Cadence: Add MHDP8501 HDMI driver If use the type of connector to determine the mode, hdmi and DP driver have to combine into one driver. So the compatible may the better choice. > > > + > > + reg: > > +maxItems: 1 > > + > > + clocks: > > +maxItems: 1 > > +description: MHDP8501 DP/HDMI APB clock. > > Seems odd there's no clock tied to the pixel/serdes clock. MHDP8501 for i.MX8MQ use the pixel clock from PHY PLL not from external CCM. The pixel clock will be set in function phy_configure B.R Sandor > > > + > > + phys: > > +maxItems: 1 > > + > > + interrupts: > > +items: > > + - description: Hotplug cable plugin. > > + - description: Hotplug cable plugout. > > + > > + interrupt-names: > > +items: > > + - const: plug_in > > + - const: plug_out > > + > > + ports: > > +$ref: /schemas/graph.yaml#/properties/ports > > + > > +properties: > > + port@0: > > +$ref: /schemas/graph.yaml#/properties/port > > +description: > > + Input port from display controller output. > > + port@1: > > +$ref: /schemas/graph.yaml#/properties/port > > +description: > > + Output port to DP/HDMI connector. > > + > > +required: > > + - port@0 > > + - port@1 > > + > > +require
RE: [EXT] Re: [PATCH v6 2/8] dt-bindings: display: bridge: Add Cadence MHDP8501 HDMI and DP
Hi Alexander,
Thanks for your comments,
> -Original Message-
> From: Alexander Stein
> Sent: 2023年6月16日 17:32
> To: andrzej.ha...@intel.com; neil.armstr...@linaro.org;
> robert.f...@linaro.org; laurent.pinch...@ideasonboard.com;
> jo...@kwiboo.se; jernej.skra...@gmail.com; airl...@gmail.com;
> dan...@ffwll.ch; robh...@kernel.org; krzysztof.kozlowski...@linaro.org;
> shawn...@kernel.org; s.ha...@pengutronix.de; feste...@gmail.com;
> vk...@kernel.org; dri-devel@lists.freedesktop.org;
> devicet...@vger.kernel.org; linux-arm-ker...@lists.infradead.org;
> linux-ker...@vger.kernel.org; linux-...@lists.infradead.org
> Cc: Oliver Brown ; Sandor Yu ;
> dl-linux-imx ; ker...@pengutronix.de; Sandor Yu
>
> Subject: [EXT] Re: [PATCH v6 2/8] dt-bindings: display: bridge: Add Cadence
> MHDP8501 HDMI and DP
>
> Caution: This is an external email. Please take care when clicking links or
> opening attachments. When in doubt, report the message using the 'Report
> this email' button
>
>
> Hi Sandor,
>
> Am Donnerstag, 15. Juni 2023, 03:38:12 CEST schrieb Sandor Yu:
> > Add bindings for Cadence MHDP8501 DisplayPort and HDMI driver.
> >
> > Signed-off-by: Sandor Yu
> > ---
> > .../display/bridge/cdns,mhdp8501.yaml | 105
> ++
> > 1 file changed, 105 insertions(+)
> > create mode 100644
> > Documentation/devicetree/bindings/display/bridge/cdns,mhdp8501.yaml
> >
> > diff --git
> > a/Documentation/devicetree/bindings/display/bridge/cdns,mhdp8501.yaml
> > b/Documentation/devicetree/bindings/display/bridge/cdns,mhdp8501.yaml
> > new file mode 100644 index ..a54756815e6f
> > --- /dev/null
> > +++
> b/Documentation/devicetree/bindings/display/bridge/cdns,mhdp8501.y
> > +++ aml
> > @@ -0,0 +1,105 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) %YAML 1.2
> > +---
> > +$id:
> > +http://devi/
> >
> +cetree.org%2Fschemas%2Fdisplay%2Fbridge%2Fcdns%2Cmhdp8501.yaml%
> 23&dat
> >
> +a=05%7C01%7CSandor.yu%40nxp.com%7C6ef2c732b3674cb2896c08db6e4
> c827b%7C
> >
> +686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C63822504710561225
> 8%7CUnkno
> >
> +wn%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1
> haWwi
> >
> +LCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=DGYYt2LQ%2FhlNBVd2m0s
> aXTm9IoKKwn
> > +X7CTTplhbLxcI%3D&reserved=0
> > +$schema:
> > +http://devi/
> >
> +cetree.org%2Fmeta-schemas%2Fcore.yaml%23&data=05%7C01%7CSandor.
> yu%40n
> >
> +xp.com%7C6ef2c732b3674cb2896c08db6e4c827b%7C686ea1d3bc2b4c6fa9
> 2cd99c5
> >
> +c301635%7C0%7C0%7C638225047105612258%7CUnknown%7CTWFpbGZs
> b3d8eyJWIjoi
> >
> +MC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3
> 000%7C%
> >
> +7C%7C&sdata=SkTFYM7HHgJmFUkyo3Ftf%2B8FdGqlnty0Ch6ggwSPeLY%3D
> &reserved
> > +=0
> > +
> > +title: Cadence MHDP8501 Displayport bridge
> > +
> > +maintainers:
> > + - Sandor Yu
> > +
> > +description:
> > + The Cadence MHDP8501 Displayport/HDMI TX interface.
> > +
> > +properties:
> > + compatible:
> > +enum:
> > + - cdns,mhdp8501-dp
> > + - cdns,mhdp8501-hdmi
> > + - fsl,imx8mq-mhdp8501-dp
> > + - fsl,imx8mq-mhdp8501-hdmi
> > +
> > + reg:
> > +maxItems: 1
> > +
> > + clocks:
> > +maxItems: 1
> > +description: MHDP8501 DP/HDMI APB clock.
> > +
> > + phys:
> > +maxItems: 1
> > +
> > + interrupts:
> > +items:
> > + - description: Hotplug cable plugin.
> > + - description: Hotplug cable plugout.
> > +
> > + interrupt-names:
> > +items:
> > + - const: plug_in
> > + - const: plug_out
> > +
> > + ports:
> > +$ref: /schemas/graph.yaml#/properties/ports
> > +
> > +properties:
> > + port@0:
> > +$ref: /schemas/graph.yaml#/properties/port
> > +description:
> > + Input port from display controller output.
> > + port@1:
> > +$ref: /schemas/graph.yaml#/properties/port
> > +description:
> > + Output port to DP/HDMI connector.
> > +
> > +required:
> > + - port@0
> > + - port@1
>
> You mark these ports as required, but apparently the drivers do not use them,
> AFAICT. E.g. missing port@1 is not resulting in an error, at lease for HDMI
> one.
>
Yes, port@1 is not really needed, I add it just to follow HDMI/DP framework
that same as other platforms in community code.
B.R
Sandor
> Best regards,
> Alexander
>
> > +
> > +required:
> > + - compatible
> > + - reg
> > + - clocks
> > + - interrupts
> > + - interrupt-names
> > + - phys
> > + - ports
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > + - |
> > +#include
> > +#include
> > +
> > +mhdp_dp: dp-bridge@32c0 {
> > +compatible = "fsl,imx8mq-mhdp8501-dp";
> > +reg = <0x32c0 0x10>;
> > +interrupts = ,
> > + ;
> > +interrupt-names = "plug_in", "plug_out";
> > +clocks = <&clk IMX8MQ_CLK_DISP_APB_ROOT>;
> > +phys = <&dp_phy>;
> > +
> > +ports {
> > +#address-cells = <1>;
> > +#s
