[RFC] Documentation: devicetree: bindings: drm: Xylon binding

[Davor Joja]

Hi Mark,

> On Mon, Jan 27, 2014 at 03:47:42PM +0000, Davor Joja wrote:
> > Hi,
> 
> Hi,
> 
> > 
> > Can I please get comments about adding new vendor prefix "xylon", and on 
> > following devicetree binding for Xylon configurable video controller (logiCVC).
> > Shown node is prepared for Xilinx Linux kernel dts file.
> 
> Does this device have any publicly-accessible documentation?

Yes it has, but it does not explain the details mentioned in binding.
http://www.logicbricks.com/Documentation/Datasheets/IP/logiCVC-ML_hds.pdf

> 
> It would be helpful if you could Cc this to some graphics related
> mailing lists. Not everyone on devicetree at vger.kernel.org is a graphics
> expert, and you'll get much better feedback with the relevant people on
> Cc.
> 

Ok, CC'ed.

> It would also be nice to see some code with the binding, and for both
> the code and binding to be sent as patches. That makes it _far_ easier
> to review as it's far easier to compare with existing bindings if in a
> standard format.
> 

Currently I do not have it. I only have some old binding which I want to get 
rid off. That is why I want to change binding (officially) and then rewrite the 
driver code for that exact binding.

> > 
> > 
> > logicvc_0: logicvc at 40030000 {
> > 	compatible = "xylon,logicvc-4.00.a";
> > 	reg = <0x40030000 0x6000>;
> > 	interrupt-parent = <&ps7_scugic_0>;
> > 	interrupts = <0 59 4>;
> > 	background-layer-bits-per-pixel = <32>;
> > 	display-interface = <0>;
> > 	display-color-space = <1>;
> > 	is-readable-regs = <1>;
> > 	is-size-position = <1>;
> > 	layer-width = <2048>;
> > 	num-layers = <4>;
> > 	layer_0 {
> > 		address = <0>;
> > 		alpha-mode = <0>;
> > 		data-width = <16>;
> > 		type = <0>;
> > 	} ;
> > 	layer_1 {
> > 		address = <0>;
> > 		alpha-mode = <0>;
> > 		data-width = <32>;
> > 		type = <0>;
> > 	} ;
> > 	layer_2 {
> > 		address = <0>;
> > 		alpha-mode = <1>;
> > 		data-width = <32>;
> > 		type = <0>;
> > 	} ;
> > 	layer_3 {
> > 		address = <0>;
> > 		alpha-mode = <0>;
> > 		data-width = <16>;
> > 		type = <1>;
> > 	} ;
> > } ;
> > 
> > 
> > Required properties for configuring logiCVC device:
> >  - compatible: value must be "xylon,logicvc-4.00.a"
> >  - reg: base address and size of the logiCVC IP
> 
> Presumably the address and size of the MMIO region the IP has?

Yes, MMIO region address where IP resides and size of IP registers area.

> 
> Does it only have a single bank of registers?
> 

Yes.

> >  - interrupts-parent: the phandle for interrupt controller
> >  - interrupts: the interrupt number
> 
> Does the device have only a single interrupt?

Yes, in this case connected to ARM GIC.

> 
> >  - background-layer-bits-per-pixel: background layer color format (0, 16, 32)
> >       if "0" last available layer is standard layer
> 
> Why is 0 quoted, and what is a "standard layer"?

Thought that this is simple way for saying "not used".
Maybe have / not to have property?

> 
> >       if 16 or 32, last available layer is background layer implemented in
> >       hw register and containing specified bits per pixel color value
> >  - display-interface: logiCVC to display physical interface
> >       (0=Parallel, 1=ITU656)
> >  - display-color-space: logiCVC to display physical color space
> >       (0=RGB, 1=YCbCr 4:2:2, 2=YCbCr 4:4:4)
> 
> These sound like they should be properties of the display this unit is
> attached to.

To be more exact, this is output interface to whatever (LCD, encoder, 
converter, ...), but it is IP property selectable when configuring.
Maybe better name for property should be "interface" and "color-space".

> 
> >  - is-readable-regs: all hw registers are readable by sw
> 
> Which registers aren't always accessible?

IP core can be configured to disable read registers access to all except 
interrupt status power control and interupt status.

> 
> >  - is-size-position: hw changing of layer size and position
> 
> These look like booleans, but have values above.

Yes, it is boolean.
Should it be
"readable-regs;" instead "is-readable-regs = <1>;"
"size-position;" instead "is-size-position = <1>;"

> 
> >  - layer-width: layer width in pixels, common for all layers
> >  - num-layers: supported number of layers (1-5)
> 
> If you require a node for each layer, you don't need this proeprty --
> you can simply count the layer nodes.

True, I do not know what is practice in this case.

> 
> >       if "background-layer-bits-per-pixel != 0", "num-layers" property value is
> >       decreased by 1
> 
> Does that mean the author of the dt subtracts one, or this is done by
> the kernel?
> 

In given example it is substracted by author, and I would like to have it like 
that.
This comment should be just info for user, and maybe it is confusing.

> Why?
> 
> >  - layer_N
> 
> Where N is?

0-4

> 
> >     - address: layer address hardcoded in hw (0=Unused, 0x...)
> 
> The example gives all layers 0 / unused. What exactly is this address
> space?

This property is set while configuring IP, and if it is set to "0" then driver 
knows that there is no dedicated address for video memory and uses its own.

> 
> >     - alpha-mode: layer transparency mode (0=Layer, 1=Pixel)
> >          layer alpha mode contains single alpha value for all layer pixels
> >          pixel alpha mode contains alpha value per pixel in video memory
> >          pixel alpha mode can increase physical size of pixel in memory
> >          (8 bits per pixel in pixel alpha mode uses 16 bits per pixel in 
> > memory)
> 
> This looks like a runtime decision rather than a property of the device.
> 
> >     - data-width: layer bits per pixel color format (16, 32)
> >     - type: layer type (0=RGB, 1=YCbCr)
> 
> Likewise why is this static?

What exactly do you mean with "runtime decision"?
All layer properties are configured in IP, and driver needs to know what they 
are to properly handle pixel memory access on specific layer.

Thank you,
Davor

> 
> Thanks,
> Mark.