RE: [PATCH v3 1/8] dt-bindings: display: xlnx: Add bindings for Xilinx display pipeline

[Hyun Kwon <hyunk@xxxxxxxxxx>]

Hi Rob,

Thanks for the review.

> -----Original Message-----
> From: Rob Herring [mailto:robh@xxxxxxxxxx]
> Sent: Friday, January 19, 2018 3:33 PM
> To: Hyun Kwon <hyunk@xxxxxxxxxx>
> Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; Michal
> Simek <michal.simek@xxxxxxxxxx>; Daniel Vetter <daniel.vetter@xxxxxxxx>;
> Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> Subject: Re: [PATCH v3 1/8] dt-bindings: display: xlnx: Add bindings for
> Xilinx display pipeline
> 
> On Mon, Jan 15, 2018 at 05:57:04PM -0800, Hyun Kwon wrote:
> > The dt binding for Xilinx display pipeline. The pipeline can be
> > composed with multiple and different types of sub-devices. This node
> > is to represent the entire pipeline as a single entity.
> >
> > Signed-off-by: Hyun Kwon <hyun.kwon@xxxxxxxxxx>
> > ---
> > v2
> > - Remove linux specific terms
> > - Elaborate details, ex regarding port binding
> > - Rename xlnx,kms to xlnx,display
> > - Rename the file name to xlnx,display.txt
> > - Add examples of hardware blocks
> > ---
> > ---
> >  .../bindings/display/xlnx/xlnx,display.txt         | 68
> ++++++++++++++++++++++
> >  1 file changed, 68 insertions(+)
> >  create mode 100644
> Documentation/devicetree/bindings/display/xlnx/xlnx,display.txt
> >
> > diff --git
> a/Documentation/devicetree/bindings/display/xlnx/xlnx,display.txt
> b/Documentation/devicetree/bindings/display/xlnx/xlnx,display.txt
> > new file mode 100644
> > index 0000000..fde1a35
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/display/xlnx/xlnx,display.txt
> > @@ -0,0 +1,68 @@
> > +Xilinx Display Pipeline
> > +-----------------------
> > +
> > +Xilinx display pipeline can be designed with various types of multiple IPs:
> > +IPs hardened on chip, ob board IPs, and soft IPs in programmable logic.
> 
> s/ob board/on-board/
> 

Will fix it.

> > +While each component would need its own node, this node represents
> > +a whole display pipeline as a single entity by integrating individual
> subdevice
> > +with glue logics.
> > +
> > +The following illustrates some examples of topology:
> > +
> > +A linear pipeline with multiple blocks:
> > +
> > +       SoC DMA -> SoC display controller -> SoC display enc
> > +or,
> > +       FPGA DMA -> FPGA display controller -> FPGA display enc
> > +
> > +A pipeline with branches:
> > +
> > +       SoC DMA -> SoC display controller -> SoC display enc
> > +               |
> > +       FPGA DMA->
> > +or,
> > +       SoC DMA -> SoC display controller -> SoC display enc
> > +                                         |
> > +                                         -> FPAG display enc
> 
> s/FPAG/FPGA/
> 

Will fix.

> > +
> > +or,
> > +
> > +                       SoC DMA -> SoC display controller -> SoC display enc
> > +                               |                         |
> > +       FPGA display controller ->                        -> FPGA display enc
> > +
> > +Required properties:
> > +
> > +- compatible: Must be "xlnx,display".
> > +
> > +- ports: phandles for ports of display controller subdevice.
> > +  In the display controller port nodes, topology for entire pipeline
> > +  should be described using the DT bindings defined in
> > +  Documentation/devicetree/bindings/graph.txt.
> > +
> > +Example:
> > +
> > +       xlnx_display {
> > +               compatible = "xlnx,display";
> > +               ports = <&display_controller_port>;
> 
> I still don't think you need this node. Match the DRM driver with the
> display controller node.
> 

With that approach, it becomes tricky as multiple devices can form a single
DRM device (single pipeline). For example, the SoC display controller would
match the DRM driver when it's used alone. There can be a standalone FPGA
display controller which will match the DRM driver. But there can also be
a design where both controllers are connected together and share some block,
ex DisplayPort transmitter. There it's not clear which one to match
the DRM driver. Thus this logical node helps to represent fragmented devices
as a single device display pipeline. If this is not acceptable, I'll try to
find other options.

> > +       };
> > +
> > +       display_controller {
> 
> display-controller@???
> 
> Please show at least the compatible and reg in the example.
> 

Sure will do.

> > +               ...
> > +               display_controller_port: port@0 {
> 
> Unit address is not needed here. If you have a unit address, then you
> should have a reg property (and #size-cells and #address-cells in the
> parent).
> 

Will fix.

Thanks,
-hyun

��.n��������+%������w��{.n����z�{��ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f