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

From: Philipp Zabel <p.zabel@pengutronix.de>
To: Tomasz Figa <tfiga@chromium.org>
Cc: Mark Rutland <mark.rutland@arm.com>,
	devicetree@vger.kernel.org, Paul Bolle <pebolle@tiscali.nl>,
	Jitao Shi <jitao.shi@mediatek.com>,
	Pawel Moll <pawel.moll@arm.com>, Jie Qiu <jie.qiu@mediatek.com>,
	Ian Campbell <ijc+devicetree@hellion.org.uk>,
	Cawa Cheng <cawa.cheng@mediatek.com>,
	dri-devel <dri-devel@lists.freedesktop.org>,
	Rob Herring <robh+dt@kernel.org>,
	linux-mediatek@lists.infradead.org,
	Kumar Gala <galak@codeaurora.org>, YT Shen <yt.shen@mediatek.com>,
	Sasha Hauer <kernel@pengutronix.de>,
	Matthias Brugger <matthias.bgg@gmail.com>
Subject: Re: [PATCH v6 01/12] dt-bindings: drm/mediatek: Add Mediatek display subsystem dts binding
Date: Tue, 24 Nov 2015 11:11:27 +0100	[thread overview]
Message-ID: <1448359887.3346.32.camel@pengutronix.de> (raw)
In-Reply-To: <CAAFQd5BPSWGDUJTihZyWpy3T6kpmdqtn0v9g_7g-YzMgpgiaJQ@mail.gmail.com>

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@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

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