[PATCH v6 01/12] dt-bindings: drm/mediatek: Add Mediatek display subsystem dts binding

[Tomasz Figa]

Hi Philipp, CK,

Please see my comments inline.

On Thu, Nov 19, 2015 at 2:34 AM, Philipp Zabel <p.zabel at pengutronix.de> wrote:
> From: CK Hu <ck.hu at mediatek.com>
>
> Add device tree binding documentation for the display subsystem in
> Mediatek MT8173 SoCs.
>
> Signed-off-by: CK Hu <ck.hu at mediatek.com>
> Signed-off-by: Philipp Zabel <p.zabel at pengutronix.de>
> Acked-by: Rob Herring <robh at kernel.org>
> ---
> Changes since v5:
>  - Updated DISP_MUTEX description
>  - Fixed DSI and DPI documentation path
> ---
>  .../bindings/display/mediatek/mediatek,disp.txt    | 183 +++++++++++++++++++++
>  .../bindings/display/mediatek/mediatek,dpi.txt     |  35 ++++
>  .../bindings/display/mediatek/mediatek,dsi.txt     |  53 ++++++
>  3 files changed, 271 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt
>  create mode 100644 Documentation/devicetree/bindings/display/mediatek/mediatek,dpi.txt
>  create mode 100644 Documentation/devicetree/bindings/display/mediatek/mediatek,dsi.txt
>
> diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt b/Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt
> new file mode 100644
> index 0000000..a311019
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt
> @@ -0,0 +1,183 @@
> +Mediatek display subsystem
> +==========================
> +
> +The Mediatek display subsystem consists of various DISP function blocks in the
> +MMSYS register space. The connections between them can be configured by output
> +and input selectors in the MMSYS_CONFIG register space. Pixel clock and start
> +of frame signal are distributed to the other function blocks by a DISP_MUTEX
> +function block.
> +
> +All DISP device tree nodes must be siblings to the central MMSYS_CONFIG node.
> +For a description of the MMSYS_CONFIG binding, see
> +Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys.txt.
> +
> +DISP function blocks
> +====================
> +
> +A display stream starts at a source function block that reads pixel data from
> +memory and ends with a sink function block that drives pixels on a display
> +interface, or writes pixels back to memory. All DISP function blocks have
> +their own register space, interrupt, and clock gate. The blocks that can
> +access memory additionally have to list the IOMMU and local arbiter they are
> +connected to.
> +
> +For a description of the display interface sink function blocks, see
> +Documentation/devicetree/bindings/display/mediatek/mediatek,dsi.txt and
> +Documentation/devicetree/bindings/display/mediatek/mediatek,dpi.txt.
> +
> +Required properties (all function blocks):
> +- compatible: "mediatek,<chip>-disp-<function>", one of
> +       "mediatek,<chip>-disp-ovl"   - overlay (4 layers, blending, csc)
> +       "mediatek,<chip>-disp-rdma"  - read DMA / line buffer
> +       "mediatek,<chip>-disp-wdma"  - write DMA
> +       "mediatek,<chip>-disp-color" - color processor
> +       "mediatek,<chip>-disp-aal"   - adaptive ambient light controller
> +       "mediatek,<chip>-disp-gamma" - gamma correction
> +       "mediatek,<chip>-disp-ufoe"  - data compression engine
> +       "mediatek,<chip>-dsi"        - DSI controller, see mediatek,dsi.txt
> +       "mediatek,<chip>-dpi"        - DPI controller, see mediatek,dpi.txt
> +       "mediatek,<chip>-disp-mutex" - display mutex
> +       "mediatek,<chip>-disp-od"    - overdrive
> +- reg: Physical base address and length of the function block register space
> +- interrupts: The interrupt signal from the function block.

Do all the blocks have only one interrupt signal?

> +- clocks: device clocks
> +  See Documentation/devicetree/bindings/clock/clock-bindings.txt for details.

What about clock-names? Also all required clocks for particular
compatible strings should be listed in documentation,

> +- compatible: "mediatek,<chip>-ddp"

Is this a rebase error? compatible property was already described few
lines above and also missing description.

> +
> +Required properties (DMA function blocks):
> +- compatible: Should be one of
> +       "mediatek,<chip>-disp-ovl"
> +       "mediatek,<chip>-disp-rdma"
> +       "mediatek,<chip>-disp-wdma"

Perhaps these 3 could also have some short description like the ones
listed above?

> +- larb: Should contain a phandle pointing to the local arbiter device as defined
> +  in Documentation/devicetree/bindings/soc/mediatek/mediatek,smi-larb.txt
> +- iommus: required a iommu node

Maybe rewrite to: "Should point to respective IOMMU block as defined
in <path to IOMMU binding documentation>"?

Best regards,
Tomasz