Re: Re: [PATCH v3 5/5] arm: dts: mt7623: add display subsystem related device nodes

[Chun-Kuang Hu <chunkuang.hu@kernel.org>]

Hi, Frank:

Frank Wunderlich <frank-w@public-files.de> 於 2020年8月4日 週二 下午11:23寫道：
>
> Hi
>
> Except mmsys (i added in Patch #1) all mt7623-compatibles are not defined in code and fallback (mt2701-x/mt8173-x) is used. If i add it in dt-binding, it should be added in code too, right? or should i remove mt7623 compatibles and only add documentation for new mmsys?

You should add mt7623 compatibles in binding document and device node
(like mmsys node). The driver is optional to have it. If driver does
not implement mt7623, it would fallback to mt2701.

Regards,
Chun-Kuang.

>
> regards Frank
>
>
> > Gesendet: Dienstag, 04. August 2020 um 17:00 Uhr
> > Von: "Chun-Kuang Hu" <chunkuang.hu@kernel.org>
> > An: "Frank Wunderlich" <linux@fw-web.de>
> > > +       mipi_tx0: mipi-dphy@10010000 {
> > > +               compatible = "mediatek,mt7623-mipi-tx",
> >
> > In mediatek,dsi.txt [1], "mediatek,mt7623-mipi-tx" is undefined.
> >
> > [1] https://www.kernel.org/doc/Documentation/devicetree/bindings/display/mediatek/mediatek%2Cdsi.txt
>
> in drivers/gpu/drm/mediatek/mtk_mipi_tx.c only the compatible for mt2701 is defined (which is fallback in dts). So should i remove mt7623 compatible here and in the other occurences?
>
> >
> > > +                            "mediatek,mt2701-mipi-tx";
>
> > > +
> > > +       cec: cec@10012000 {
> > > +               compatible = "mediatek,mt7623-cec",
> >
> > Please explicitly define "mediatek,mt7623-cec" in mediatek,hdmi.txt [2].
> >
> > [2] https://www.kernel.org/doc/Documentation/devicetree/bindings/display/mediatek/mediatek%2Chdmi.txt
> >
> > > +                            "mediatek,mt8173-cec";
>
> same here...only mt8173-cec is defined in drivers/gpu/drm/mediatek/mtk_cec.c
>
> > >         cir: cir@10013000 {
> > >                 compatible = "mediatek,mt7623-cir";
> > >                 reg = <0 0x10013000 0 0x1000>;
> > > @@ -369,6 +393,18 @@ apmixedsys: syscon@10209000 {
> > >                 #clock-cells = <1>;
> > >         };
> > >
> > > +       hdmi_phy: phy@10209100 {
> > > +               compatible = "mediatek,mt7623-hdmi-phy",
> >
> > Ditto.
> >
> > > +                            "mediatek,mt2701-hdmi-phy";
> same as above (./drivers/gpu/drm/mediatek/mtk_hdmi_phy.c)
>
> > > +       hdmiddc0: i2c@11013000 {
> > > +               compatible = "mediatek,mt7623-hdmi-ddc",
> >
> > Ditto.
> >
> > > +                            "mediatek,mt8173-hdmi-ddc";
> > > +               interrupts = <GIC_SPI 81 IRQ_TYPE_LEVEL_LOW>;
> > > +               reg = <0 0x11013000 0 0x1C>;
> > > +               clocks = <&pericfg CLK_PERI_I2C3>;
> > > +               clock-names = "ddc-i2c";
> > > +               status = "disabled";
> > > +       };
> > > +
> > >         nor_flash: spi@11014000 {
> > >                 compatible = "mediatek,mt7623-nor",
> > >                              "mediatek,mt8173-nor";
> > > @@ -766,6 +812,84 @@ mmsys: syscon@14000000 {
> > >                 #clock-cells = <1>;
> > >         };
> > >
> > > +       display_components: dispsys@14000000 {
> > > +               compatible = "mediatek,mt7623-mmsys",
> > > +                            "mediatek,mt2701-mmsys";
> >
> > In mediatek,mmsys.txt [3], this should be:
> >
> > mmsys: syscon@14000000 {
> >         compatible = "mediatek,mt7623-mmsys", "mediatek,mt2701-mmsys", "syscon"
> >
> > [3] https://www.kernel.org/doc/Documentation/devicetree/bindings/arm/mediatek/mediatek%2Cmmsys.txt
>
> as i added this in driver, i add this to documentation (and maybe remove the fallback because it results in wrong routing?)
>
> >
> > > +               reg = <0 0x14000000 0 0x1000>;
> > > +               power-domains = <&scpsys MT2701_POWER_DOMAIN_DISP>;
> > > +       };
> > > +
> > > +       ovl@14007000 {
> > > +               compatible = "mediatek,mt7623-disp-ovl",
> >
> > This is not defined in mediatek,disp.txt [4].
> >
> > [4] https://www.kernel.org/doc/Documentation/devicetree/bindings/display/mediatek/mediatek%2Cdisp.txt
> also not defined in drivers/gpu/drm/mediatek/mtk_drm_drv.c so again fallback used
>
> >
> > > +                            "mediatek,mt2701-disp-ovl";
>
> > > +               reg = <0 0x14007000 0 0x1000>;
> > > +               interrupts = <GIC_SPI 153 IRQ_TYPE_LEVEL_LOW>;
> > > +               clocks = <&mmsys CLK_MM_DISP_OVL>;
> > > +               iommus = <&iommu MT2701_M4U_PORT_DISP_OVL_0>;
> > > +               mediatek,larb = <&larb0>;
> > > +       };
> > > +
> > > +       rdma0: rdma@14008000 {
> > > +               compatible = "mediatek,mt7623-disp-rdma",
> > > +                            "mediatek,mt2701-disp-rdma";
> > > +               reg = <0 0x14008000 0 0x1000>;
> > > +               interrupts = <GIC_SPI 152 IRQ_TYPE_LEVEL_LOW>;
> > > +               clocks = <&mmsys CLK_MM_DISP_RDMA>;
> > > +               iommus = <&iommu MT2701_M4U_PORT_DISP_RDMA>;
> > > +               mediatek,larb = <&larb0>;
> > > +       };
> > > +
> > > +       wdma@14009000 {
> > > +               compatible = "mediatek,mt7623-disp-wdma",
> > > +                            "mediatek,mt2701-disp-wdma";
> > > +               reg = <0 0x14009000 0 0x1000>;
> > > +               interrupts = <GIC_SPI 154 IRQ_TYPE_LEVEL_LOW>;
> > > +               clocks = <&mmsys CLK_MM_DISP_WDMA>;
> > > +               iommus = <&iommu MT2701_M4U_PORT_DISP_WDMA>;
> > > +               mediatek,larb = <&larb0>;
> > > +       };
> > > +
> > > +       bls: pwm@1400a000 {
> > > +               compatible = "mediatek,mt7623-disp-pwm",
> > > +                            "mediatek,mt2701-disp-pwm";
> > > +               reg = <0 0x1400a000 0 0x1000>;
> > > +               #pwm-cells = <2>;
> > > +               clocks = <&mmsys CLK_MM_MDP_BLS_26M>,
> > > +                        <&mmsys CLK_MM_DISP_BLS>;
> > > +               clock-names = "main", "mm";
> > > +               status = "disabled";
> > > +       };
> > > +
> > > +       color@1400b000 {
> > > +               compatible = "mediatek,mt7623-disp-color",
> > > +                            "mediatek,mt2701-disp-color";
> > > +               reg = <0 0x1400b000 0 0x1000>;
> > > +               interrupts = <GIC_SPI 156 IRQ_TYPE_LEVEL_LOW>;
> > > +               clocks = <&mmsys CLK_MM_DISP_COLOR>;
> > > +       };
> > > +
> > > +       dsi: dsi@1400c000 {
> > > +               compatible = "mediatek,mt7623-dsi",
> >
> > This is not defined in mediatek,dsi.txt [1].
> >
> > > +                            "mediatek,mt2701-dsi";
> also fallback used (drivers/gpu/drm/mediatek/mtk_drm_drv.c)
>
>
> > > +       dpi0: dpi@14014000 {
> > > +               compatible = "mediatek,mt7623-dpi",
> >
> > This is not defined in mediatek,dpi.txt [5].
> >
> > [5] https://www.kernel.org/doc/Documentation/devicetree/bindings/display/mediatek/mediatek%2Cdpi.txt
> >
> > > +                            "mediatek,mt2701-dpi";
>