Re: [PATCH v8 08/13] arm64: dts: mt8173: Add display subsystem related nodes

[Philipp Zabel <p.zabel@pengutronix.de>]

Hi Daniel,

Am Mittwoch, den 03.02.2016, 00:24 +0800 schrieb Daniel Kurtz:
> Hi Philipp,
> 
> Two more comments below...
> 
> On Tue, Feb 2, 2016 at 4:10 PM, Daniel Kurtz <djkurtz@chromium.org> wrote:
> > On Tue, Jan 5, 2016 at 1:36 AM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
[...]
> >> +
> >> +               dsi0: dsi@1401b000 {
> >> +                       compatible = "mediatek,mt8173-dsi";
> >> +                       reg = <0 0x1401b000 0 0x1000>;
> >> +                       interrupts = <GIC_SPI 192 IRQ_TYPE_LEVEL_LOW>;
> >> +                       power-domains = <&scpsys MT8173_POWER_DOMAIN_MM>;
> >> +                       clocks = <&mmsys CLK_MM_DSI0_ENGINE>,
> >> +                                <&mmsys CLK_MM_DSI0_DIGITAL>,
> >> +                                <&mipi_tx0>;
> >> +                       clock-names = "engine", "digital", "hs";
> >> +                       phys = <&mipi_tx0>;
> >> +                       phy-names = "dphy";
> 
> I think it might work better if this was also default
> status="disabled", and require the board-specific .dts to enable the
> dsi*.
> This would be useful, for example, boards that use only the MIPI/DSI
> or only HDMI.
> IMHO, it is more clear to have such boards explicit mark the supported
> ports as 'status="okay"' in their .dts, rather than having to mark all
> of the unused ones as disabled.

I'm ok with that, the same should be done for the phys then. To enable
MIPI DSI output on dsi0, the board DT would have to:

	&mipi_tx0 {
		status = "okay";
	};
	&dsi0 {
		status = "okay";
		/* output port here */
	};

To enable HDMI output via dpi0:

	&cec {
		status = "okay";
	};
	&hdmi_phy {
		status = "okay";
	};
	&dpi0 {
		status = "okay";
	};
	&hdmi0 {
		status = "okay";
		/* output port here */
	};

[...]
> >> +               dpi0: dpi@1401d000 {
> >> +                       compatible = "mediatek,mt8173-dpi";
> >> +                       reg = <0 0x1401d000 0 0x1000>;
> >> +                       interrupts = <GIC_SPI 194 IRQ_TYPE_LEVEL_LOW>;
> >> +                       power-domains = <&scpsys MT8173_POWER_DOMAIN_MM>;
> >> +                       clocks = <&mmsys CLK_MM_DPI_PIXEL>,
> >> +                                <&mmsys CLK_MM_DPI_ENGINE>,
> >> +                                <&apmixedsys CLK_APMIXED_TVDPLL>;
> >> +                       clock-names = "pixel", "engine", "pll";
> >> +
> >> +                       port {
> >> +                               dpi0_out: endpoint {
> >> +                                       remote-endpoint = <&hdmi0_in>;
> >
> > nit: At this point in the patch series, you haven't defined hdmi0_in yet.
> > Move this port to "Add HDMI related nodes".
> 
> And, let's mark dpi0 as status="disabled"; by default, and require an
> enabling in the board specific .dts.

Will do.

regards
Philipp


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel