[PATCH v5] of: add display helper (was: of: add videomode helper)

[PATCH v5] of: add display helper (was: of: add videomode helper)

[Steffen Trumtrar]

Hi!

After the feedback I got on v4, I thought about the current state of the 
videomode helper and decided to reengineer it. The most confusion seems to
stem from the fact, that a videomode does not have ranges. And that is correct.
Therefore the description of the display now uses a list of timings.
These timings support ranges, as they do in datasheets (min/typ/max).

A device driver may choose to grep a display-description from the devicetree and
directly work with that (matching parameters according to their range etc.).
Or one can use the former struct videomode to convert from the timings to a videomode
(at the moment it just grabs the typical-value from every timing-parameter).
This videomode on the other hand, can then be converted to a mode the backend wants
(drm_mode_info, fb_videomode,...).
As of now, this intermediate step is a bit, well, unnecessary. But it provides a way
to have a generic videomode and functions to possibly convert back-and-forth.
In the end, this version does the same as of_videomode, but I hope makes the whole
thing a little clearer.

Thanks to everybody who reviewed the previous versions.
Feedback is always welcome.

Regards,
Steffen


----------------------------------------------------------------
Steffen Trumtrar (2):
      of: add helper to parse display specs
      video: add generic videomode description

 Documentation/devicetree/bindings/video/display |  208 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/of/Kconfig                              |    5 +++
 drivers/of/Makefile                             |    1 +
 drivers/of/of_display.c                         |  157 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/video/Makefile                          |    1 +
 drivers/video/videomode.c                       |  146 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/display.h                         |   85 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/of_display.h                      |   15 ++++++++
 include/linux/videomode.h                       |   38 +++++++++++++++++++
 9 files changed, 656 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/video/display
 create mode 100644 drivers/of/of_display.c
 create mode 100644 drivers/video/videomode.c
 create mode 100644 include/linux/display.h
 create mode 100644 include/linux/of_display.h
 create mode 100644 include/linux/videomode.h

[PATCH 1/2] of: add helper to parse display specs

[Steffen Trumtrar]

Parse a display-node with timings and hardware-specs from devictree.

Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
---
 Documentation/devicetree/bindings/video/display |  208 +++++++++++++++++++++++
 drivers/of/Kconfig                              |    5 +
 drivers/of/Makefile                             |    1 +
 drivers/of/of_display.c                         |  157 +++++++++++++++++
 include/linux/display.h                         |   85 +++++++++
 include/linux/of_display.h                      |   15 ++
 6 files changed, 471 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/video/display
 create mode 100644 drivers/of/of_display.c
 create mode 100644 include/linux/display.h
 create mode 100644 include/linux/of_display.h

diff --git a/Documentation/devicetree/bindings/video/display b/Documentation/devicetree/bindings/video/display
new file mode 100644
index 0000000..722766a
--- /dev/null
+++ b/Documentation/devicetree/bindings/video/display
@@ -0,0 +1,208 @@
+display bindings
+=========
+
+display-node
+------------
+
+required properties:
+ - none
+
+optional properties:
+ - default-timing: the default timing value
+ - width-mm, height-mm: Display dimensions in mm
+ - hsync-active-high (bool): Hsync pulse is active high
+ - vsync-active-high (bool): Vsync pulse is active high
+ - de-active-high (bool): Data-Enable pulse is active high
+ - pixelclk-inverted (bool): pixelclock is inverted
+ - pixel-per-clk
+ - link-width: number of channels (e.g. LVDS)
+ - bpp: bits-per-pixel
+
+timings-subnode
+---------------
+
+required properties:
+subnodes that specify
+ - hactive, vactive: Display resolution
+ - hfront-porch, hback-porch, hsync-len: Horizontal Display timing parameters
+   in pixels
+   vfront-porch, vback-porch, vsync-len: Vertical display timing parameters in
+   lines
+ - clock: displayclock in Hz
+
+There are different ways of describing a display and its capabilities. The devicetree
+representation corresponds to the one commonly found in datasheets for displays.
+The description of the display and its timing is split in two parts: first the display
+properties like size in mm and (optionally) multiple subnodes with the supported timings.
+If a display supports multiple signal timings, the default-timing can be specified.
+
+Example:
+
+	display@0 {
+		width-mm = <800>;
+		height-mm = <480>;
+		default-timing = <&timing0>;
+		timings {
+			timing0: timing@0 {
+				/* 1920x1080p24 */
+				clock = <52000000>;
+				hactive = <1920>;
+				vactive = <1080>;
+				hfront-porch = <25>;
+				hback-porch = <25>;
+				hsync-len = <25>;
+				vback-porch = <2>;
+				vfront-porch = <2>;
+				vsync-len = <2>;
+				hsync-active-high;
+			};
+		};
+	};
+
+Every property also supports the use of ranges, so the commonly used datasheet
+description with <min typ max>-tuples can be used.
+
+Example:
+
+	timing1: timing@1 {
+		/* 1920x1080p24 */
+		clock = <148500000>;
+		hactive = <1920>;
+		vactive = <1080>;
+		hsync-len = <0 44 60>;
+		hfront-porch = <80 88 95>;
+		hback-porch = <100 148 160>;
+		vfront-porch = <0 4 6>;
+		vback-porch = <0 36 50>;
+		vsync-len = <0 5 6>;
+	};
+
+The "display"-property in a connector-node (e.g. hdmi, ldb,...) is used to connect
+the display to that driver. 
+of_display expects a phandle, that specifies the display-node, in that property.
+
+Example:
+
+	hdmi@00120000 {
+		status = "okay";
+		display = <&acme>;
+	};
+
+Usage in backend
+========
+
+A backend driver may choose to use the display directly and convert the timing
+ranges to a suitable mode. Or it may just use the conversion of the display timings
+to the required mode via the generic videomode struct.
+
+					dtb
+					 |
+					 |  of_get_display
+					 ↓
+				   struct display
+					 |
+					 |  videomode_from_timings
+					 ↓
+			    ---  struct videomode ---
+			    |			    |
+ videomode_to_displaymode   |			    |   videomode_to_fb_videomode
+		            ↓			    ↓
+		     drm_display_mode         fb_videomode
+
+
+Conversion to videomode
+===========+
+As device drivers normally work with some kind of video mode, the timings can be
+converted (may be just a simple copying of the typical value) to a generic videomode
+structure which then can be converted to the according mode used by the backend.
+
+Supported modes
+=======+
+The generic videomode read in by the driver can be converted to the following
+modes with the following parameters
+
+struct fb_videomode
+=========+
+  +----------+---------------------------------------------+----------+-------+
+  |          |                ↑                            |          |       |
+  |          |                |upper_margin                |          |       |
+  |          |                ↓                            |          |       |
+  +----------###############################################----------+-------+
+  |          #                ↑                            #          |       |
+  |          #                |                            #          |       |
+  |          #                |                            #          |       |
+  |          #                |                            #          |       |
+  |   left   #                |                            #  right   | hsync |
+  |  margin  #                |       xres                 #  margin  |  len  |
+  |<-------->#<---------------+--------------------------->#<-------->|<----->|
+  |          #                |                            #          |       |
+  |          #                |                            #          |       |
+  |          #                |                            #          |       |
+  |          #                |yres                        #          |       |
+  |          #                |                            #          |       |
+  |          #                |                            #          |       |
+  |          #                |                            #          |       |
+  |          #                |                            #          |       |
+  |          #                |                            #          |       |
+  |          #                |                            #          |       |
+  |          #                |                            #          |       |
+  |          #                |                            #          |       |
+  |          #                ↓                            #          |       |
+  +----------###############################################----------+-------+
+  |          |                ↑                            |          |       |
+  |          |                |lower_margin                |          |       |
+  |          |                ↓                            |          |       |
+  +----------+---------------------------------------------+----------+-------+
+  |          |                ↑                            |          |       |
+  |          |                |vsync_len                   |          |       |
+  |          |                ↓                            |          |       |
+  +----------+---------------------------------------------+----------+-------+
+
+clock in nanoseconds
+
+struct drm_display_mode
+===========+
+  +----------+---------------------------------------------+----------+-------+
+  |          |                                             |          |       |  ↑
+  |          |                                             |          |       |  |
+  |          |                                             |          |       |  |
+  +----------###############################################----------+-------+  |
+  |          #   ↑         ↑          ↑                    #          |       |  |
+  |          #   |         |          |                    #          |       |  |
+  |          #   |         |          |                    #          |       |  |
+  |          #   |         |          |                    #          |       |  |
+  |          #   |         |          |                    #          |       |  |
+  |          #   |         |          |       hdisplay     #          |       |  |
+  |          #<--+--------------------+------------------->#          |       |  |
+  |          #   |         |          |                    #          |       |  |
+  |          #   |vsync_start         |                    #          |       |  |
+  |          #   |         |          |                    #          |       |  |
+  |          #   |         |vsync_end |                    #          |       |  |
+  |          #   |         |          |vdisplay            #          |       |  |
+  |          #   |         |          |                    #          |       |  |
+  |          #   |         |          |                    #          |       |  |
+  |          #   |         |          |                    #          |       |  | vtotal
+  |          #   |         |          |                    #          |       |  |
+  |          #   |         |          |     hsync_start    #          |       |  |
+  |          #<--+---------+----------+------------------------------>|       |  |
+  |          #   |         |          |                    #          |       |  |
+  |          #   |         |          |     hsync_end      #          |       |  |
+  |          #<--+---------+----------+-------------------------------------->|  |
+  |          #   |         |          ↓                    #          |       |  |
+  +----------####|#########|################################----------+-------+  |
+  |          |   |         |                               |          |       |  |
+  |          |   |         |                               |          |       |  |
+  |          |   ↓         |                               |          |       |  |
+  +----------+-------------+-------------------------------+----------+-------+  |
+  |          |             |                               |          |       |  |
+  |          |             |                               |          |       |  |
+  |          |             ↓                               |          |       |  ↓
+  +----------+---------------------------------------------+----------+-------+
+                                   htotal
+   <------------------------------------------------------------------------->
+
+clock in kilohertz
diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
index dfba3e6..a4e3074 100644
--- a/drivers/of/Kconfig
+++ b/drivers/of/Kconfig
@@ -83,4 +83,9 @@ config OF_MTD
 	depends on MTD
 	def_bool y
 
+config OF_DISPLAY
+	def_bool y
+	help
+	  helper to parse displays from the devicetree
+
 endmenu # OF
diff --git a/drivers/of/Makefile b/drivers/of/Makefile
index e027f44..0756bee 100644
--- a/drivers/of/Makefile
+++ b/drivers/of/Makefile
@@ -11,3 +11,4 @@ obj-$(CONFIG_OF_MDIO)	+= of_mdio.o
 obj-$(CONFIG_OF_PCI)	+= of_pci.o
 obj-$(CONFIG_OF_PCI_IRQ)  += of_pci_irq.o
 obj-$(CONFIG_OF_MTD)	+= of_mtd.o
+obj-$(CONFIG_OF_DISPLAY) += of_display.o
diff --git a/drivers/of/of_display.c b/drivers/of/of_display.c
new file mode 100644
index 0000000..632a351
--- /dev/null
+++ b/drivers/of/of_display.c
@@ -0,0 +1,157 @@
+/*
+ * OF helpers for parsing display timings
+ * 
+ * Copyright (c) 2012 Steffen Trumtrar <s.trumtrar@pengutronix.de>, Pengutronix
+ * 
+ * based on of_videomode.c by Sascha Hauer <s.hauer@pengutronix.de>
+ *
+ * This file is released under the GPLv2
+ */
+#include <linux/of.h>
+#include <linux/slab.h>
+#include <linux/display.h>
+#include <linux/of_display.h>
+#include <linux/fb.h>
+
+/* every signal_timing can be specified with either
+ * just the typical value or a range consisting of
+ * min/typ/max.
+ * This function helps handling this
+ */
+static int of_display_parse_property(struct device_node *np, char *name,
+				struct timing_entry *result)
+{
+	struct property *prop;
+	int length;
+	int cells;
+	int ret;
+
+	prop = of_find_property(np, name, &length);
+	if (!prop) {
+		pr_err("%s: could not find property %s\n", __func__,
+			name);
+		return -EINVAL;
+	}
+
+	cells = length / sizeof(u32);
+
+	if (cells = 1)
+		ret = of_property_read_u32_array(np, name, &result->typ, cells);
+	else if (cells = 3)
+		ret = of_property_read_u32_array(np, name, &result->min, cells);
+	else {
+		pr_err("%s: illegal timing specification in %s\n", __func__,
+			name);
+		return -EINVAL;
+	}
+
+	return ret;
+}
+
+struct display *of_get_display(struct device_node *np)
+{
+	struct device_node *display_np;
+	struct device_node *timing_np;
+	struct device_node *timings;
+	struct display *disp;
+	char *default_timing;
+
+	if (!np) {
+		pr_err("%s: no devicenode given\n", __func__);
+		return NULL;
+	}
+
+	display_np = of_parse_phandle(np, "display", 0);
+
+	if (!display_np) {
+		pr_err("%s: could not find display node\n", __func__);
+		return NULL;
+	}
+
+	disp = kmalloc(sizeof(struct display *), GFP_KERNEL);
+
+	memset(disp, 0, sizeof(struct display *));
+
+	of_property_read_u32(display_np, "width-mm", &disp->width_mm);
+	of_property_read_u32(display_np, "height-mm", &disp->height_mm);
+
+	timing_np = of_parse_phandle(display_np, "default-timing", 0);
+
+	if (!timing_np) {
+		pr_info("%s: no default-timing specified\n", __func__);
+		timing_np = of_find_node_by_name(np, "timing");
+	}
+
+	if (!timing_np) {
+		pr_info("%s: no timing specifications given\n", __func__);
+		return disp;
+	}
+
+	default_timing = (char *)timing_np->full_name;
+
+	timings = of_find_node_by_name(np, "timings");
+
+	disp->num_timings = 0;
+
+	disp->timings = kmalloc(sizeof(struct signal_timing *), GFP_KERNEL);
+
+	for_each_child_of_node(timings, timing_np) {
+		struct signal_timing *st;
+		int ret;
+
+		st = kmalloc(sizeof(struct signal_timing *), GFP_KERNEL);
+		disp->timings[disp->num_timings] = kmalloc(sizeof(struct signal_timing *), GFP_KERNEL);
+
+		ret |= of_display_parse_property(timing_np, "hback-porch", &st->hback_porch);
+		ret |= of_display_parse_property(timing_np, "hfront-porch", &st->hfront_porch);
+		ret |= of_display_parse_property(timing_np, "hactive", &st->hactive);
+		ret |= of_display_parse_property(timing_np, "hsync-len", &st->hsync_len);
+		ret |= of_display_parse_property(timing_np, "vback-porch", &st->vback_porch);
+		ret |= of_display_parse_property(timing_np, "vfront-porch", &st->vfront_porch);
+		ret |= of_display_parse_property(timing_np, "vactive", &st->vactive);
+		ret |= of_display_parse_property(timing_np, "vsync-len", &st->vsync_len);
+		ret |= of_display_parse_property(timing_np, "clock", &st->pixelclock);
+
+		if (strcmp(default_timing, timing_np->full_name) = 0)
+			disp->default_timing = disp->num_timings;
+
+		disp->timings[disp->num_timings] = st;
+		disp->num_timings++;
+	}
+
+	disp->vsync_pol_active_high = of_property_read_bool(display_np, "vsync-active-high");
+	disp->hsync_pol_active_high = of_property_read_bool(display_np, "hsync-active-high");
+	disp->de_pol_active_high = of_property_read_bool(display_np, "de-active-high");
+	disp->pixelclk_pol_inverted = of_property_read_bool(display_np, "pixelclk-inverted");
+	of_property_read_u32(display_np, "pixel-per-clk", &disp->if_pixel_per_clk);
+	of_property_read_u32(display_np, "link-width", &disp->if_link_width);
+	of_property_read_u32(display_np, "bpp", &disp->if_bpp);
+
+
+	pr_info("%s: got %d timings. Using #%d as default\n", __func__, disp->num_timings, disp->default_timing + 1);
+
+	return disp;
+}
+EXPORT_SYMBOL(of_get_display);
+
+int of_display_exists(struct device_node *np)
+{
+	struct device_node *display_np;
+	struct device_node *timing_np;
+
+	if (!np)
+		return -EINVAL;
+
+	display_np = of_parse_phandle(np, "display", 0);
+
+	if (!display_np)
+		return -EINVAL;
+
+	timing_np = of_parse_phandle(np, "default-timing", 0);
+
+	if (timing_np)
+		return 0;
+
+	return -EINVAL;
+}
+EXPORT_SYMBOL_GPL(of_display_exists);
diff --git a/include/linux/display.h b/include/linux/display.h
new file mode 100644
index 0000000..bb84ed9
--- /dev/null
+++ b/include/linux/display.h
@@ -0,0 +1,85 @@
+/*
+ * Copyright 2012 Steffen Trumtrar <s.trumtrar@pengutronix.de>
+ *
+ * Hardware-description of a display
+ *
+ * This file is released under the GPLv2
+ */
+
+#ifndef __LINUX_DISPLAY_H
+#define __LINUX_DISPLAY_H
+
+#include <linux/list.h>
+
+#define OF_DEFAULT_TIMING -1
+
+struct display {
+	u32 width_mm;
+	u32 height_mm;
+	unsigned int num_timings;
+	unsigned int default_timing;
+
+	struct signal_timing **timings;
+
+	bool vsync_pol_active_high;
+	bool hsync_pol_active_high;
+	bool de_pol_active_high;
+	bool pixelclk_pol_inverted;
+
+	u32 if_bpp;
+	u32 if_link_width;
+	u32 if_pixel_per_clk;
+};
+
+struct timing_entry {
+	u32 min;
+	u32 typ;
+	u32 max;
+};
+
+struct signal_timing {
+	struct timing_entry pixelclock;
+
+	struct timing_entry hactive;
+	struct timing_entry hfront_porch;
+	struct timing_entry hback_porch;
+	struct timing_entry hsync_len;
+
+	struct timing_entry vactive;
+	struct timing_entry vfront_porch;
+	struct timing_entry vback_porch;
+	struct timing_entry vsync_len;
+};
+
+/* placeholder function until ranges are really needed */
+static inline u32 signal_timing_get_value(struct timing_entry *te, int index)
+{
+	return te->typ;
+}
+
+static inline void timings_release(struct display *disp)
+{
+	int i;
+	for (i = 0; i < disp->num_timings; i++)
+		kfree(disp->timings[i]);
+}
+
+static inline void display_release(struct display *disp)
+{
+	timings_release(disp);
+	kfree(disp->timings);
+}
+
+static inline struct signal_timing *display_get_timing(struct display *disp, int index)
+{
+	if (disp->num_timings >= index)
+		return disp->timings[index];
+	else
+		return NULL;
+}
+
+
+int of_display_exists(struct device_node *np);
+struct display *of_get_display(struct device_node *np);
+
+#endif /* __LINUX_DISPLAY_H */
diff --git a/include/linux/of_display.h b/include/linux/of_display.h
new file mode 100644
index 0000000..500ff94
--- /dev/null
+++ b/include/linux/of_display.h
@@ -0,0 +1,15 @@
+/*
+ * Copyright 2012 Steffen Trumtrar <s.trumtrar@pengutronix.de>
+ *
+ * This file is released under the GPLv2
+ */
+
+#ifndef __LINUX_OF_DISPLAY_H
+#define __LINUX_OF_DISPALY_H
+
+#include <linux/fb.h>
+
+struct display *of_get_display(struct device_node *np);
+int of_display_exists(struct device_node *np);
+
+#endif
-- 
1.7.10.4

Re: [PATCH 1/2] of: add helper to parse display specs

[Stephen Warren]

On 09/24/2012 09:35 AM, Steffen Trumtrar wrote:
> Parse a display-node with timings and hardware-specs from devictree.

> diff --git a/Documentation/devicetree/bindings/video/display b/Documentation/devicetree/bindings/video/display
> new file mode 100644
> index 0000000..722766a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/video/display

This should be display.txt.

> @@ -0,0 +1,208 @@
> +display bindings
> +=========
> +
> +display-node
> +------------

I'm not personally convinced about the direction this is going. While I
think it's reasonable to define DT bindings for displays, and DT
bindings for display modes, I'm not sure that it's reasonable to couple
them together into a single binding.

I think creating a well-defined timing binding first will be much
simpler than doing so within the context of a display binding; the
scope/content of a general display binding seems much less well-defined
to me at least, for reasons I mentioned before.

> +required properties:
> + - none
> +
> +optional properties:
> + - default-timing: the default timing value
> + - width-mm, height-mm: Display dimensions in mm

> + - hsync-active-high (bool): Hsync pulse is active high
> + - vsync-active-high (bool): Vsync pulse is active high

At least those two properties should exist in the display timing instead
(or perhaps as well). There are certainly cases where different similar
display modes are differentiated by hsync/vsync polarity more than
anything else. This is probably more likely with analog display
connectors than digital, but I see no reason why a DT binding for
display timing shouldn't cover both.

> + - de-active-high (bool): Data-Enable pulse is active high
> + - pixelclk-inverted (bool): pixelclock is inverted

> + - pixel-per-clk

pixel-per-clk is probably something that should either be part of the
timing definition, or something computed internally to the display
driver based on rules for the signal type, rather than something
represented in DT.

The above comment assumes this property is intended to represent DVI's
requirement for pixel clock doubling for low-pixel-clock-rate modes. If
it's something to do with e.g. a single-data-rate vs. double-data-rate
property of the underlying physical connection, that's most likely
something that should be defined in a binding specific to e.g. LVDS,
rather than something generic.

> + - link-width: number of channels (e.g. LVDS)
> + - bpp: bits-per-pixel
> +
> +timings-subnode
> +---------------
> +
> +required properties:
> +subnodes that specify
> + - hactive, vactive: Display resolution
> + - hfront-porch, hback-porch, hsync-len: Horizontal Display timing parameters
> +   in pixels
> +   vfront-porch, vback-porch, vsync-len: Vertical display timing parameters in
> +   lines
> + - clock: displayclock in Hz
> +
> +There are different ways of describing a display and its capabilities. The devicetree
> +representation corresponds to the one commonly found in datasheets for displays.
> +The description of the display and its timing is split in two parts: first the display
> +properties like size in mm and (optionally) multiple subnodes with the supported timings.
> +If a display supports multiple signal timings, the default-timing can be specified.
> +
> +Example:
> +
> +	display@0 {
> +		width-mm = <800>;
> +		height-mm = <480>;
> +		default-timing = <&timing0>;
> +		timings {
> +			timing0: timing@0 {

If you're going to use a unit address ("@0") to ensure that node names
are unique (which is not mandatory), then each node also needs a reg
property with matching value, and #address-cells/#size-cells in the
parent. Instead, you could name the nodes something unique based on the
mode name to avoid this, e.g. 1080p24 { ... }.

Re: [PATCH 1/2] of: add helper to parse display specs

[Mitch Bradley]

On 10/1/2012 6:53 AM, Stephen Warren wrote:
> On 09/24/2012 09:35 AM, Steffen Trumtrar wrote:
>> Parse a display-node with timings and hardware-specs from devictree.
> 
>> diff --git a/Documentation/devicetree/bindings/video/display b/Documentation/devicetree/bindings/video/display
>> new file mode 100644
>> index 0000000..722766a
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/video/display
> 
> This should be display.txt.
> 
>> @@ -0,0 +1,208 @@
>> +display bindings
>> +=========
>> +
>> +display-node
>> +------------
> 
> I'm not personally convinced about the direction this is going. While I
> think it's reasonable to define DT bindings for displays, and DT
> bindings for display modes, I'm not sure that it's reasonable to couple
> them together into a single binding.
> 
> I think creating a well-defined timing binding first will be much
> simpler than doing so within the context of a display binding; the
> scope/content of a general display binding seems much less well-defined
> to me at least, for reasons I mentioned before.
> 
>> +required properties:
>> + - none
>> +
>> +optional properties:
>> + - default-timing: the default timing value
>> + - width-mm, height-mm: Display dimensions in mm
> 
>> + - hsync-active-high (bool): Hsync pulse is active high
>> + - vsync-active-high (bool): Vsync pulse is active high
> 
> At least those two properties should exist in the display timing instead
> (or perhaps as well). There are certainly cases where different similar
> display modes are differentiated by hsync/vsync polarity more than
> anything else. This is probably more likely with analog display
> connectors than digital, but I see no reason why a DT binding for
> display timing shouldn't cover both.
> 
>> + - de-active-high (bool): Data-Enable pulse is active high
>> + - pixelclk-inverted (bool): pixelclock is inverted
> 
>> + - pixel-per-clk
> 
> pixel-per-clk is probably something that should either be part of the
> timing definition, or something computed internally to the display
> driver based on rules for the signal type, rather than something
> represented in DT.
> 
> The above comment assumes this property is intended to represent DVI's
> requirement for pixel clock doubling for low-pixel-clock-rate modes. If
> it's something to do with e.g. a single-data-rate vs. double-data-rate
> property of the underlying physical connection, that's most likely
> something that should be defined in a binding specific to e.g. LVDS,
> rather than something generic.
> 
>> + - link-width: number of channels (e.g. LVDS)
>> + - bpp: bits-per-pixel
>> +
>> +timings-subnode
>> +---------------
>> +
>> +required properties:
>> +subnodes that specify
>> + - hactive, vactive: Display resolution
>> + - hfront-porch, hback-porch, hsync-len: Horizontal Display timing parameters
>> +   in pixels
>> +   vfront-porch, vback-porch, vsync-len: Vertical display timing parameters in
>> +   lines
>> + - clock: displayclock in Hz
>> +
>> +There are different ways of describing a display and its capabilities. The devicetree
>> +representation corresponds to the one commonly found in datasheets for displays.
>> +The description of the display and its timing is split in two parts: first the display
>> +properties like size in mm and (optionally) multiple subnodes with the supported timings.
>> +If a display supports multiple signal timings, the default-timing can be specified.
>> +
>> +Example:
>> +
>> +	display@0 {
>> +		width-mm = <800>;
>> +		height-mm = <480>;
>> +		default-timing = <&timing0>;
>> +		timings {
>> +			timing0: timing@0 {
> 
> If you're going to use a unit address ("@0") to ensure that node names
> are unique (which is not mandatory), then each node also needs a reg
> property with matching value, and #address-cells/#size-cells in the
> parent. Instead, you could name the nodes something unique based on the
> mode name to avoid this, e.g. 1080p24 { ... }.


I'm concerned that numbered nodes are being misused as arrays.

It's easy to make real arrays by including multiple cells in the value
of each timing parameter, and easy to choose a cell by saying the array
index instead of using the phandle.



> 
> _______________________________________________
> devicetree-discuss mailing list
> devicetree-discuss@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss
> 

Re: [PATCH 1/2] of: add helper to parse display specs

[Stephen Warren]

On 10/01/2012 01:16 PM, Mitch Bradley wrote:
> On 10/1/2012 6:53 AM, Stephen Warren wrote:
>> On 09/24/2012 09:35 AM, Steffen Trumtrar wrote:
>>> Parse a display-node with timings and hardware-specs from devictree.
>>
>>> diff --git a/Documentation/devicetree/bindings/video/display b/Documentation/devicetree/bindings/video/display
>>> new file mode 100644
>>> index 0000000..722766a
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/video/display
>>
>> This should be display.txt.
>>
>>> @@ -0,0 +1,208 @@
>>> +display bindings
>>> +=========
>>> +
>>> +display-node
>>> +------------
>>
>> I'm not personally convinced about the direction this is going. While I
>> think it's reasonable to define DT bindings for displays, and DT
>> bindings for display modes, I'm not sure that it's reasonable to couple
>> them together into a single binding.
>>
>> I think creating a well-defined timing binding first will be much
>> simpler than doing so within the context of a display binding; the
>> scope/content of a general display binding seems much less well-defined
>> to me at least, for reasons I mentioned before.
>>
>>> +required properties:
>>> + - none
>>> +
>>> +optional properties:
>>> + - default-timing: the default timing value
>>> + - width-mm, height-mm: Display dimensions in mm
>>
>>> + - hsync-active-high (bool): Hsync pulse is active high
>>> + - vsync-active-high (bool): Vsync pulse is active high
>>
>> At least those two properties should exist in the display timing instead
>> (or perhaps as well). There are certainly cases where different similar
>> display modes are differentiated by hsync/vsync polarity more than
>> anything else. This is probably more likely with analog display
>> connectors than digital, but I see no reason why a DT binding for
>> display timing shouldn't cover both.
>>
>>> + - de-active-high (bool): Data-Enable pulse is active high
>>> + - pixelclk-inverted (bool): pixelclock is inverted
>>
>>> + - pixel-per-clk
>>
>> pixel-per-clk is probably something that should either be part of the
>> timing definition, or something computed internally to the display
>> driver based on rules for the signal type, rather than something
>> represented in DT.
>>
>> The above comment assumes this property is intended to represent DVI's
>> requirement for pixel clock doubling for low-pixel-clock-rate modes. If
>> it's something to do with e.g. a single-data-rate vs. double-data-rate
>> property of the underlying physical connection, that's most likely
>> something that should be defined in a binding specific to e.g. LVDS,
>> rather than something generic.
>>
>>> + - link-width: number of channels (e.g. LVDS)
>>> + - bpp: bits-per-pixel
>>> +
>>> +timings-subnode
>>> +---------------
>>> +
>>> +required properties:
>>> +subnodes that specify
>>> + - hactive, vactive: Display resolution
>>> + - hfront-porch, hback-porch, hsync-len: Horizontal Display timing parameters
>>> +   in pixels
>>> +   vfront-porch, vback-porch, vsync-len: Vertical display timing parameters in
>>> +   lines
>>> + - clock: displayclock in Hz
>>> +
>>> +There are different ways of describing a display and its capabilities. The devicetree
>>> +representation corresponds to the one commonly found in datasheets for displays.
>>> +The description of the display and its timing is split in two parts: first the display
>>> +properties like size in mm and (optionally) multiple subnodes with the supported timings.
>>> +If a display supports multiple signal timings, the default-timing can be specified.
>>> +
>>> +Example:
>>> +
>>> +	display@0 {
>>> +		width-mm = <800>;
>>> +		height-mm = <480>;
>>> +		default-timing = <&timing0>;
>>> +		timings {
>>> +			timing0: timing@0 {
>>
>> If you're going to use a unit address ("@0") to ensure that node names
>> are unique (which is not mandatory), then each node also needs a reg
>> property with matching value, and #address-cells/#size-cells in the
>> parent. Instead, you could name the nodes something unique based on the
>> mode name to avoid this, e.g. 1080p24 { ... }.
> 
> 
> I'm concerned that numbered nodes are being misused as arrays.
> 
> It's easy to make real arrays by including multiple cells in the value
> of each timing parameter, and easy to choose a cell by saying the array
> index instead of using the phandle.

In this case though, arrays don't work out so well in my opinion:

We want to describe a set of unrelated display modes that the display
can handle. These are logically separate entities. I don't think
combining the values that represent say 5 different modes into a single
set of properties really makes sense here; a separate node or property
per display mode really does make sense because they're separate objects.

Related, each display timing parameter (e.g. hsync length, position,
...) has a range, so min/typical/max values. These are already
represented as a 3-cell property as I believe you're proposing.
Combining that with a cell that represents n different modes in a single
array seems like it'd end up with something rather hard to read, at
least for humans even if it would be admittedly trivial for a CPU.

Re: [PATCH 1/2] of: add helper to parse display specs

[Mitch Bradley]

On 10/1/2012 10:25 AM, Stephen Warren wrote:
> On 10/01/2012 01:16 PM, Mitch Bradley wrote:
>> On 10/1/2012 6:53 AM, Stephen Warren wrote:
>>> On 09/24/2012 09:35 AM, Steffen Trumtrar wrote:
>>>> Parse a display-node with timings and hardware-specs from devictree.
>>>
>>>> diff --git a/Documentation/devicetree/bindings/video/display b/Documentation/devicetree/bindings/video/display
>>>> new file mode 100644
>>>> index 0000000..722766a
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/video/display
>>>
>>> This should be display.txt.
>>>
>>>> @@ -0,0 +1,208 @@
>>>> +display bindings
>>>> +=========
>>>> +
>>>> +display-node
>>>> +------------
>>>
>>> I'm not personally convinced about the direction this is going. While I
>>> think it's reasonable to define DT bindings for displays, and DT
>>> bindings for display modes, I'm not sure that it's reasonable to couple
>>> them together into a single binding.
>>>
>>> I think creating a well-defined timing binding first will be much
>>> simpler than doing so within the context of a display binding; the
>>> scope/content of a general display binding seems much less well-defined
>>> to me at least, for reasons I mentioned before.
>>>
>>>> +required properties:
>>>> + - none
>>>> +
>>>> +optional properties:
>>>> + - default-timing: the default timing value
>>>> + - width-mm, height-mm: Display dimensions in mm
>>>
>>>> + - hsync-active-high (bool): Hsync pulse is active high
>>>> + - vsync-active-high (bool): Vsync pulse is active high
>>>
>>> At least those two properties should exist in the display timing instead
>>> (or perhaps as well). There are certainly cases where different similar
>>> display modes are differentiated by hsync/vsync polarity more than
>>> anything else. This is probably more likely with analog display
>>> connectors than digital, but I see no reason why a DT binding for
>>> display timing shouldn't cover both.
>>>
>>>> + - de-active-high (bool): Data-Enable pulse is active high
>>>> + - pixelclk-inverted (bool): pixelclock is inverted
>>>
>>>> + - pixel-per-clk
>>>
>>> pixel-per-clk is probably something that should either be part of the
>>> timing definition, or something computed internally to the display
>>> driver based on rules for the signal type, rather than something
>>> represented in DT.
>>>
>>> The above comment assumes this property is intended to represent DVI's
>>> requirement for pixel clock doubling for low-pixel-clock-rate modes. If
>>> it's something to do with e.g. a single-data-rate vs. double-data-rate
>>> property of the underlying physical connection, that's most likely
>>> something that should be defined in a binding specific to e.g. LVDS,
>>> rather than something generic.
>>>
>>>> + - link-width: number of channels (e.g. LVDS)
>>>> + - bpp: bits-per-pixel
>>>> +
>>>> +timings-subnode
>>>> +---------------
>>>> +
>>>> +required properties:
>>>> +subnodes that specify
>>>> + - hactive, vactive: Display resolution
>>>> + - hfront-porch, hback-porch, hsync-len: Horizontal Display timing parameters
>>>> +   in pixels
>>>> +   vfront-porch, vback-porch, vsync-len: Vertical display timing parameters in
>>>> +   lines
>>>> + - clock: displayclock in Hz
>>>> +
>>>> +There are different ways of describing a display and its capabilities. The devicetree
>>>> +representation corresponds to the one commonly found in datasheets for displays.
>>>> +The description of the display and its timing is split in two parts: first the display
>>>> +properties like size in mm and (optionally) multiple subnodes with the supported timings.
>>>> +If a display supports multiple signal timings, the default-timing can be specified.
>>>> +
>>>> +Example:
>>>> +
>>>> +	display@0 {
>>>> +		width-mm = <800>;
>>>> +		height-mm = <480>;
>>>> +		default-timing = <&timing0>;
>>>> +		timings {
>>>> +			timing0: timing@0 {
>>>
>>> If you're going to use a unit address ("@0") to ensure that node names
>>> are unique (which is not mandatory), then each node also needs a reg
>>> property with matching value, and #address-cells/#size-cells in the
>>> parent. Instead, you could name the nodes something unique based on the
>>> mode name to avoid this, e.g. 1080p24 { ... }.
>>
>>
>> I'm concerned that numbered nodes are being misused as arrays.
>>
>> It's easy to make real arrays by including multiple cells in the value
>> of each timing parameter, and easy to choose a cell by saying the array
>> index instead of using the phandle.
> 
> In this case though, arrays don't work out so well in my opinion:
> 
> We want to describe a set of unrelated display modes that the display
> can handle. These are logically separate entities. I don't think
> combining the values that represent say 5 different modes into a single
> set of properties really makes sense here; a separate node or property
> per display mode really does make sense because they're separate objects.

That argument seems pretty dependent on how you choose to look at things.


> 
> Related, each display timing parameter (e.g. hsync length, position,
> ...) has a range, so min/typical/max values. These are already
> represented as a 3-cell property as I believe you're proposing.
> Combining that with a cell that represents n different modes in a single
> array seems like it'd end up with something rather hard to read, at
> least for humans even if it would be admittedly trivial for a CPU.


That argument is better.

> 

Re: [PATCH 1/2] of: add helper to parse display specs

[Steffen Trumtrar]

On Mon, Oct 01, 2012 at 10:53:08AM -0600, Stephen Warren wrote:
> On 09/24/2012 09:35 AM, Steffen Trumtrar wrote:
> > Parse a display-node with timings and hardware-specs from devictree.
> 
> > diff --git a/Documentation/devicetree/bindings/video/display b/Documentation/devicetree/bindings/video/display
> > new file mode 100644
> > index 0000000..722766a
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/video/display
> 
> This should be display.txt.
> 
Okay

> > @@ -0,0 +1,208 @@
> > +display bindings
> > +=========
> > +
> > +display-node
> > +------------
> 
> I'm not personally convinced about the direction this is going. While I
> think it's reasonable to define DT bindings for displays, and DT
> bindings for display modes, I'm not sure that it's reasonable to couple
> them together into a single binding.
> 
> I think creating a well-defined timing binding first will be much
> simpler than doing so within the context of a display binding; the
> scope/content of a general display binding seems much less well-defined
> to me at least, for reasons I mentioned before.
> 

Yes, you are right. I'm in the middle of moving things around a little.
It seems best, to have bindings only for the timings at the moment and
get people to agree on those and use them, instead of all the adhoc solutions
based on of_videomode v2.

Then, the of_get_display_timings and the conversion via videomode to fb_videomode
etc can be combined with Laurent Pincharts panel proposal.

> > +required properties:
> > + - none
> > +
> > +optional properties:
> > + - default-timing: the default timing value
> > + - width-mm, height-mm: Display dimensions in mm
> 
> > + - hsync-active-high (bool): Hsync pulse is active high
> > + - vsync-active-high (bool): Vsync pulse is active high
> 
> At least those two properties should exist in the display timing instead
> (or perhaps as well). There are certainly cases where different similar
> display modes are differentiated by hsync/vsync polarity more than
> anything else. This is probably more likely with analog display
> connectors than digital, but I see no reason why a DT binding for
> display timing shouldn't cover both.
> 

Yes. Will do.

> > + - de-active-high (bool): Data-Enable pulse is active high
> > + - pixelclk-inverted (bool): pixelclock is inverted
> 
> > + - pixel-per-clk
> 
> pixel-per-clk is probably something that should either be part of the
> timing definition, or something computed internally to the display
> driver based on rules for the signal type, rather than something
> represented in DT.
> 
> The above comment assumes this property is intended to represent DVI's
> requirement for pixel clock doubling for low-pixel-clock-rate modes. If
> it's something to do with e.g. a single-data-rate vs. double-data-rate
> property of the underlying physical connection, that's most likely
> something that should be defined in a binding specific to e.g. LVDS,
> rather than something generic.
> 
> > + - link-width: number of channels (e.g. LVDS)
> > + - bpp: bits-per-pixel
> > +
> > +timings-subnode
> > +---------------
> > +
> > +required properties:
> > +subnodes that specify
> > + - hactive, vactive: Display resolution
> > + - hfront-porch, hback-porch, hsync-len: Horizontal Display timing parameters
> > +   in pixels
> > +   vfront-porch, vback-porch, vsync-len: Vertical display timing parameters in
> > +   lines
> > + - clock: displayclock in Hz
> > +
> > +There are different ways of describing a display and its capabilities. The devicetree
> > +representation corresponds to the one commonly found in datasheets for displays.
> > +The description of the display and its timing is split in two parts: first the display
> > +properties like size in mm and (optionally) multiple subnodes with the supported timings.
> > +If a display supports multiple signal timings, the default-timing can be specified.
> > +
> > +Example:
> > +
> > +	display@0 {
> > +		width-mm = <800>;
> > +		height-mm = <480>;
> > +		default-timing = <&timing0>;
> > +		timings {
> > +			timing0: timing@0 {
> 
> If you're going to use a unit address ("@0") to ensure that node names
> are unique (which is not mandatory), then each node also needs a reg
> property with matching value, and #address-cells/#size-cells in the
> parent. Instead, you could name the nodes something unique based on the
> mode name to avoid this, e.g. 1080p24 { ... }.
> 

Ah, okay. Wasn't sure that was valid. I prefer to not use unit addresses.

 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH 2/2] video: add generic videomode description

[Steffen Trumtrar]

Backend-independent videomode. At the moment this is not very different
from fb_mode or drm_mode.
It is supposed to be a generic description of videomodes and conversion
step to the desired backend video mode representation.
At the moment, only really makes sense when used with of_display

Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
---
 drivers/video/Makefile    |    1 +
 drivers/video/videomode.c |  146 +++++++++++++++++++++++++++++++++++++++++++++
 include/linux/videomode.h |   38 ++++++++++++
 3 files changed, 185 insertions(+)
 create mode 100644 drivers/video/videomode.c
 create mode 100644 include/linux/videomode.h

diff --git a/drivers/video/Makefile b/drivers/video/Makefile
index ee8dafb..6a02fe0 100644
--- a/drivers/video/Makefile
+++ b/drivers/video/Makefile
@@ -170,3 +170,4 @@ obj-$(CONFIG_FB_VIRTUAL)          += vfb.o
 
 #video output switch sysfs driver
 obj-$(CONFIG_VIDEO_OUTPUT_CONTROL) += output.o
+obj-y				   += videomode.o
diff --git a/drivers/video/videomode.c b/drivers/video/videomode.c
new file mode 100644
index 0000000..b118d00
--- /dev/null
+++ b/drivers/video/videomode.c
@@ -0,0 +1,146 @@
+/*
+ * generic videomode helper
+ *
+ * Copyright (c) 2012 Steffen Trumtrar <s.trumtrar@pengutronix.de>, Pengutronix
+ *
+ * This file is released under the GPLv2
+ */
+#include <linux/of.h>
+#include <linux/fb.h>
+#include <linux/slab.h>
+#include <drm/drm_mode.h>
+#include <linux/display.h>
+#include <linux/videomode.h>
+
+struct videomode *videomode_from_display(struct display *disp, int index)
+{
+	struct videomode *vm;
+	struct signal_timing *st;
+
+	vm = kmalloc(sizeof(struct videomode *), GFP_KERNEL);
+
+	if (!vm)
+		return NULL;
+
+	st = display_get_timing(disp, index);
+
+	vm->pixelclock = signal_timing_get_value(&st->pixelclock, 0);
+	vm->hactive = signal_timing_get_value(&st->hactive, 0);
+	vm->hfront_porch = signal_timing_get_value(&st->hfront_porch, 0);
+	vm->hback_porch = signal_timing_get_value(&st->hback_porch, 0);
+	vm->hsync_len = signal_timing_get_value(&st->hsync_len, 0);
+
+	vm->vactive = signal_timing_get_value(&st->vactive, 0);
+	vm->vfront_porch = signal_timing_get_value(&st->vfront_porch, 0);
+	vm->vback_porch = signal_timing_get_value(&st->vback_porch, 0);
+	vm->vsync_len = signal_timing_get_value(&st->vsync_len, 0);
+
+	vm->vah = disp->vsync_pol_active_high;
+	vm->hah = disp->hsync_pol_active_high;
+
+	return vm;
+}
+
+#if defined(CONFIG_DRM)
+int videomode_to_display_mode(struct videomode *vm, struct drm_display_mode *dmode)
+{
+	memset(dmode, 0, sizeof(*dmode));
+
+	dmode->hdisplay = vm->hactive;
+	dmode->hsync_start = dmode->hdisplay + vm->hfront_porch;
+	dmode->hsync_end = dmode->hsync_start + vm->hsync_len;
+	dmode->htotal = dmode->hsync_end + vm->hback_porch;
+
+	dmode->vdisplay = vm->vactive;
+	dmode->vsync_start = dmode->vdisplay + vm->vfront_porch;
+	dmode->vsync_end = dmode->vsync_start + vm->vsync_len;
+	dmode->vtotal = dmode->vtotal + vm->vback_porch;
+
+	dmode->clock = vm->pixelclock / 1000;
+
+	if (vm->hah)
+		dmode->flags |= DRM_MODE_FLAG_PHSYNC;
+	else
+		dmode->flags |= DRM_MODE_FLAG_NHSYNC;
+	if (vm->vah)
+		dmode->flags |= DRM_MODE_FLAG_PVSYNC;
+	else
+		dmode->flags |= DRM_MODE_FLAG_NVSYNC;
+	if (vm->interlaced)
+		dmode->flags |= DRM_MODE_FLAG_INTERLACE;
+	if (vm->doublescan)
+		dmode->flags |= DRM_MODE_FLAG_DBLSCAN;
+	drm_mode_set_name(dmode);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(videomode_to_display_mode);
+#else
+int videomode_to_displaymode(struct videomode *vm, struct drm_display_mode *dmode)
+{
+	return 0;
+}
+#endif
+
+int videomode_to_fb_videomode(struct videomode *vm, struct fb_videomode *fbmode)
+{
+	memset(fbmode, 0, sizeof(*fbmode));
+
+	fbmode->xres = vm->hactive;
+	fbmode->left_margin = vm->hback_porch;
+	fbmode->right_margin = vm->hfront_porch;
+	fbmode->hsync_len = vm->hsync_len;
+
+	fbmode->yres = vm->vactive;
+	fbmode->upper_margin = vm->vback_porch;
+	fbmode->lower_margin = vm->vfront_porch;
+	fbmode->vsync_len = vm->vsync_len;
+
+	fbmode->pixclock = KHZ2PICOS(vm->pixelclock) / 1000;
+
+	if (vm->hah)
+		fbmode->sync |= FB_SYNC_HOR_HIGH_ACT;
+	if (vm->vah)
+		fbmode->sync |= FB_SYNC_VERT_HIGH_ACT;
+	if (vm->interlaced)
+		fbmode->vmode |= FB_VMODE_INTERLACED;
+	if (vm->doublescan)
+		fbmode->vmode |= FB_VMODE_DOUBLE;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(videomode_to_fb_videomode);
+
+int of_get_display_mode(struct device_node *np, struct drm_display_mode *dmode, int index)
+{
+	struct videomode *vm;
+	struct display *disp;
+
+	disp = of_get_display(np);
+
+	if (!disp) {
+		pr_err("%s: no display specified\n", __func__);
+		return -EINVAL;
+	}
+
+	if (index = OF_DEFAULT_TIMING)
+		index = disp->default_timing;
+
+	vm = videomode_from_display(disp, index);
+
+	if (!vm) {
+		pr_err("%s: could not get videomode %d\n", __func__, index);
+		return -EINVAL;
+	}
+
+	videomode_to_display_mode(vm, dmode);
+
+	pr_info("%s: got %dx%d display mode from %s\n", __func__, vm->hactive, vm->vactive, np->name);
+
+	display_release(disp);
+	kfree(vm);
+
+	return 0;
+
+}
+EXPORT_SYMBOL_GPL(of_get_display_mode);
diff --git a/include/linux/videomode.h b/include/linux/videomode.h
new file mode 100644
index 0000000..100c88b
--- /dev/null
+++ b/include/linux/videomode.h
@@ -0,0 +1,38 @@
+/*
+ * Copyright 2012 Steffen Trumtrar <s.trumtrar@pengutronix.de>
+ *
+ * generic videomode description
+ *
+ * This file is released under the GPLv2
+ */
+
+#ifndef __LINUX_VIDEOMODE_H
+#define __LINUX_VIDEOMODE_H
+
+#include <drm/drmP.h>
+
+struct videomode {
+	u32 pixelclock;
+	u32 refreshrate;
+
+	u32 hactive;
+	u32 hfront_porch;
+	u32 hback_porch;
+	u32 hsync_len;
+
+	u32 vactive;
+	u32 vfront_porch;
+	u32 vback_porch;
+	u32 vsync_len;
+
+	bool hah;
+	bool vah;
+	bool interlaced;
+	bool doublescan;
+
+};
+
+int videomode_to_display_mode(struct videomode *vm, struct drm_display_mode *dmode);
+int videomode_to_fb_videomode(struct videomode *vm, struct fb_videomode *fbmode);
+int of_get_display_mode(struct device_node *np, struct drm_display_mode *dmode, int index);
+#endif /* __LINUX_VIDEOMODE_H */
-- 
1.7.10.4