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

[Daniel Kurtz]

Hi Philipp,

Two more comments below...

On Tue, Feb 2, 2016 at 4:10 PM, Daniel Kurtz <djkurtz at chromium.org> wrote:
> On Tue, Jan 5, 2016 at 1:36 AM, Philipp Zabel <p.zabel at pengutronix.de> wrote:
>> From: CK Hu <ck.hu at mediatek.com>
>>
>> This patch adds the device nodes for the DISP function blocks
>> comprising the display subsystem.
>>
>> Signed-off-by: CK Hu <ck.hu at mediatek.com>
>> Signed-off-by: Cawa Cheng <cawa.cheng at mediatek.com>
>> Signed-off-by: Jie Qiu <jie.qiu at mediatek.com>
>> Signed-off-by: Daniel Kurtz <djkurtz at chromium.org>
>> Signed-off-by: Philipp Zabel <p.zabel at pengutronix.de>

>> +
>> +               dsi0: dsi at 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.


>> +               };
>> +
>> +               dsi1: dsi at 1401c000 {
>> +                       compatible = "mediatek,mt8173-dsi";
>> +                       reg = <0 0x1401c000 0 0x1000>;
>> +                       interrupts = <GIC_SPI 193 IRQ_TYPE_LEVEL_LOW>;
>> +                       power-domains = <&scpsys MT8173_POWER_DOMAIN_MM>;
>> +                       clocks = <&mmsys CLK_MM_DSI1_ENGINE>,
>> +                                <&mmsys CLK_MM_DSI1_DIGITAL>,
>> +                                <&mipi_tx1>;
>> +                       clock-names = "engine", "digital", "hs";
>> +                       phy = <&mipi_tx1>;
>> +                       phy-names = "dphy";
>> +                       status = "disabled";
>> +               };
>> +
>> +               dpi0: dpi at 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.

>
>> +                               };
>> +                       };
>>                 };