Hi, Can I get some comments on below mails? I want to create devicetree node for Xylon logiCVC DRM device driver, and get comments and suggestions from community. At the end I would send driver and devicetree binding to mainline. Thank you, Davor > 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@xxxxxxxxxxxxxxx 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@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. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
