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

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

[Robert Jarzmik]

Add documentation for the PXA frambuffer devicetree binding.

Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
Cc: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
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",
+- 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.
+- default-mode: a videomode within the display with timing parameters
+		as specified below.
+- bits-per-pixel: pixel data bus width of the LCD panel
+
+Optional properties:
+- lcd-supply: Regulator for LCD supply voltage.
+- enable-transparency-bit: if framebuffer colorspace reserves a bit for
+			   transparency
+- enable-greyscale-cmap: true if palette is a grayscale based instead of color
+
+Example:
+
+	fb0: video@0x44000000 {
+		compatible = "marvell,pxa2xx-fb";
+		reg = <0x44000000 0x10000>;
+		interrupts = <17>;
+		clocks = <&clks CLK_LCD>;
+		interrupts = <23>;
+		display = <&display0>;
+		status = "okay";
+
+		enable-transparency-bit = <0>;
+		enable-greyscale-cmap = <0>;
+		#address-cells = <1>;
+		#size-cells = <1>;
+	};
+
+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
+
+Optional properties (as per of_videomode_helper):
+ - power-regulator: power supply regulator to the LCD to power it on or off
+
+Example:
+	display0: display {
+		lcd-type = "color-tft";
+		bits-per-pixel = <16>;
+
+		display-timings {
+			native-mode = <&timing0>;
+			timing0: 240p {
+				/* 240x320p24 */
+				clock-frequency = <4545000>;
+				hactive = <240>;
+				vactive = <320>;
+				hfront-porch = <4>;
+				hback-porch = <6>;
+				hsync-len = <4>;
+				vback-porch = <5>;
+				vfront-porch = <3>;
+				vsync-len = <2>;
+				pixelclk-active = <0>;
+				de-active = <1>;
+			};
+		};
+	};
-- 
2.1.4

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

[Philipp Zabel]

On Sat, Oct 3, 2015 at 6:11 PM, Robert Jarzmik <robert.jarzmik@free.fr> wrote:
> Add documentation for the PXA frambuffer devicetree binding.
>
> Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
> Cc: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
> Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
> 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.

> +- 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.

> +- 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?

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

How does this differ from the regulator below?

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

That doesn't belong in the device tree.

> +- 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?

> +Example:
> +
> +       fb0: video@0x44000000 {
> +               compatible = "marvell,pxa2xx-fb";
> +               reg = <0x44000000 0x10000>;
> +               interrupts = <17>;
> +               clocks = <&clks CLK_LCD>;
> +               interrupts = <23>;
> +               display = <&display0>;
> +               status = "okay";
> +
> +               enable-transparency-bit = <0>;
> +               enable-greyscale-cmap = <0>;
> +               #address-cells = <1>;
> +               #size-cells = <1>;

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

> +       };
> +
> +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.

> +Optional properties (as per of_videomode_helper):
> + - power-regulator: power supply regulator to the LCD to power it on or off

regards
Philipp

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

[Robert Jarzmik]

Philipp Zabel <philipp.zabel-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> writes:

> On Sat, Oct 3, 2015 at 6:11 PM, Robert Jarzmik <robert.jarzmik-GANU6spQydw@public.gmane.org> wrote:
>> Add documentation for the PXA frambuffer devicetree binding.
>>
>> Signed-off-by: Robert Jarzmik <robert.jarzmik-GANU6spQydw@public.gmane.org>
>> Cc: Jean-Christophe Plagniol-Villard <plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org>
>> Cc: Tomi Valkeinen <tomi.valkeinen-l0cyMroinI0@public.gmane.org>
>> 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.

>
>> +- 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.

>> +- 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.

>> +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.
>
>> +- enable-transparency-bit: if framebuffer colorspace reserves a bit for
>> +                          transparency
>
> That doesn't belong in the device tree.
>
>> +- 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.

>> +               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.

>> +       };
>> +
>> +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

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

[Philipp Zabel]

Am Samstag, den 03.10.2015, 19:23 +0200 schrieb Robert Jarzmik:
[...]
> 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.

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


> > > +- 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.

[...]
> > > +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

> > > +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