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

[Philipp Zabel]

Hi Tomasz,

Am Dienstag, den 24.11.2015, 14:32 +0900 schrieb 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:
[...]
> > +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?

As far as I am aware, yes. They all have their own multiplexer, with
single output to the GIC each.

> > +- 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,

All the internal blocks just have a single clock gate. The exceptions
are the DSI and DPI controllers, which are documented separately. I'll
add a note.

> > +- compatible: "mediatek,<chip>-ddp"
> 
> Is this a rebase error? compatible property was already described few
> lines above and also missing description.

Yes, I'll drop this line.

> > +
> > +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?

These three are already included in the list above. Should I make this
"Additional required properties (DMA function blocks):" to clarify?

> > +- 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>"?

I'll change this.

thank you
Philipp