Re: [PATCH] video: fbdev: add Marvell PXA framebuffer binding

Philipp Zabel [off-list ref] writes:

On Sat, Oct 3, 2015 at 6:11 PM, Robert Jarzmik [off-list ref] wrote:

quoted

Add documentation for the PXA frambuffer devicetree binding.

Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
Cc: Jean-Christophe Plagniol-Villard <redacted>
Cc: Tomi Valkeinen <redacted>
Cc: linux-fbdev@vger.kernel.org

---
 .../devicetree/bindings/video/marvell,pxafb.txt    | 75 ++++++++++++++++++++++
 1 file changed, 75 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/video/marvell,pxafb.txt

diff --git a/Documentation/devicetree/bindings/video/marvell,pxafb.txt b/Documentation/devicetree/bindings/video/marvell,pxafb.txt
new file mode 100644
index 000000000000..489055bf3c57
--- /dev/null
+++ b/Documentation/devicetree/bindings/video/marvell,pxafb.txt

@@ -0,0 +1,75 @@
+PXA LCDC Framebuffer
+-----------------------------------------------------
+
+Required properties:
+- compatible :
+       "marvell,pxa2xx-fb",

Should be "marvell,pxa2xx-lcd-controller", "marvell,pxa2xx-lcdc" or
something like this.

Whichever you see fit.

quoted

+- reg : Should contain 1 register ranges(address and length).
+       Can contain an additional register range(address and length)
+       for fixed framebuffer memory. Useful for dedicated memories.
+- interrupts : framebuffer controller interrupt
+- display: a phandle pointing to the display node
+
+Required nodes:
+- display: a display node is required to initialize the lcd panel
+          This should be in the board dts.

I'd prefer to use an of-graph link to a panel node with a proper
compatible value for the panel, instead of this custom display
property.
That way, if somebody ever decides convert the fbdev driver to a drm
driver, we don't have to change the device tree and can directly use
drm_panel.

Ok, if you give me an example it would be easier for me.

quoted

+- default-mode: a videomode within the display with timing parameters
+               as specified below.
+- bits-per-pixel: pixel data bus width of the LCD panel

Would bus-width be better here?

bus-width yes, but I think I should remove this property, and only keep the one
in the panel/display.

quoted

+Optional properties:
+- lcd-supply: Regulator for LCD supply voltage.

How does this differ from the regulator below?

Ah yes, good point. In the end I couldn't decide which one was the correct one
... My feeling is that it's the display's one, as hardware wise the power is
necessary for the display, not the framebuffer.

quoted

+- enable-transparency-bit: if framebuffer colorspace reserves a bit for
+                          transparency

That doesn't belong in the device tree.

quoted

+- enable-greyscale-cmap: true if palette is a grayscale based instead of color

I suspect this doesn't belong in the device tree either. Does this
specify the pixel format of the memory framebuffer?

Yes, both these values specify the pixel format. I was thinking this was a
hardware capability of the IP, I was wrong, just cross-checked. I'll remove
these 2 properties.

quoted

+               enable-transparency-bit = <0>;
+               enable-greyscale-cmap = <0>;
+               #address-cells = <1>;
+               #size-cells = <1>;

What are the #address/size-cells needed for?

Copy-paste from another binding, atmel's I think. Poor leftover obviously.

quoted

+       };
+
+PXA LCDC Display
+-----------------------------------------------------
+Required properties (as per of_videomode_helper):
+ - lcd-type: either "mono-stn", "mono-dstn", "color-stn", "color-dstn",
+                   "color-tft", "smart-panel"
+ - bits-per-pixel: LCD data bus width

This is already found in the lcd controller node above.

I think the bus-width should be here. It represents the number of data lines
between the SoC and the panel.

Cheers.

-- 
Robert