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 <redacted>
Cc: Jean-Christophe Plagniol-Villard <plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org>
Cc: Tomi Valkeinen <redacted>
Cc: linux-fbdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Am Samstag, den 03.10.2015, 19:23 +0200 schrieb Robert Jarzmik:
[...]

a/Documentation/devicetree/bindings/video/marvell,pxafb.txt

quoted

quoted

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.

Personally, I like the lcdc one better, even though it is an arbitrary
abbreviation of the text found in the manual.

quoted

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.

Have a look at the of-graph connection between capture interface and
sensor (QCI and MT9M111) in your example below. The connection between
LCD controller and panel should look similar:

	pxabus {
		lcd-controller@40500000 {
			compatible = "marvell,pxa2xx-lcdc";
			/* ... */

			port {
				lcdc_out: endpoint {
					remote-endpoint = <&panel_in>;

					bus-width = <16>;
				};
			};
		};
	};

	panel {
		compatible = "toshiba,ltm0305a776";
		lcd-type = "color-tft";
		power-supply = <&lcd_supply>;
		backlight = <&lcd_backlight>;

		port {
			panel_in: endpoint {
				remote-endpoint = <&lcdc_out>;
			};
		};
	}

The bus-width could be made a property of the lcdc_out endpoint for
symmetry with the QCI binding, and as documented in
Documentation/devicetree/bindings/media/video-interfaces.txt

If you later bind a drm_panel driver to the panel node, it can look up
that information (and the timings) just from the compatible string.

[...]

quoted

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.

Then I'd suggest a power-supply property in the panel node, as is
already documented for simple panels:
Documentation/devicetree/bindings/panel/simple-panel.txt

quoted

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.

With the of-graph, it can be argued that this is a property of both
endpoints of the bus (imagine an 18-bit panel driven by a 16-bit LCD
controller with some funny wiring).

regards
Philipp