[PATCH v8 0/6] of: add display helper

STALE5026d

Revision v8 of 8 in this series.

24 messages, 5 authors, 2012-11-14 · open the first message on its own page

[PATCH v8 0/6] of: add display helper

From: Steffen Trumtrar <hidden>
Date: 2012-11-12 15:37:00

Hi!

This is v8 of the display helper series

Changes since v7:
	- move of_xxx to drivers/video
	- remove non-binding documentation from display-timings.txt
	- squash display_timings and videomode in one patch
	- misc minor fixes

Regards,
Steffen


Steffen Trumtrar (6):
  video: add display_timing and videomode
  video: add of helper for videomode
  fbmon: add videomode helpers
  fbmon: add of_videomode helpers
  drm_modes: add videomode helpers
  drm_modes: add of_videomode helpers

 .../devicetree/bindings/video/display-timings.txt  |  107 +++++++++++
 drivers/gpu/drm/drm_modes.c                        |   77 ++++++++
 drivers/video/Kconfig                              |   19 ++
 drivers/video/Makefile                             |    4 +
 drivers/video/display_timing.c                     |   22 +++
 drivers/video/fbmon.c                              |   77 ++++++++
 drivers/video/of_display_timing.c                  |  186 ++++++++++++++++++++
 drivers/video/of_videomode.c                       |   47 +++++
 drivers/video/videomode.c                          |   45 +++++
 include/drm/drmP.h                                 |    8 +
 include/linux/display_timing.h                     |   69 ++++++++
 include/linux/fb.h                                 |    5 +
 include/linux/of_display_timings.h                 |   19 ++
 include/linux/of_videomode.h                       |   15 ++
 include/linux/videomode.h                          |   39 ++++
 15 files changed, 739 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/video/display-timings.txt
 create mode 100644 drivers/video/display_timing.c
 create mode 100644 drivers/video/of_display_timing.c
 create mode 100644 drivers/video/of_videomode.c
 create mode 100644 drivers/video/videomode.c
 create mode 100644 include/linux/display_timing.h
 create mode 100644 include/linux/of_display_timings.h
 create mode 100644 include/linux/of_videomode.h
 create mode 100644 include/linux/videomode.h

-- 
1.7.10.4

[PATCH v8 1/6] video: add display_timing and videomode

From: Steffen Trumtrar <hidden>
Date: 2012-11-12 15:37:01

Add display_timing structure and the according helper functions. This allows
the description of a display via its supported timing parameters.

Every timing parameter can be specified as a single value or a range
<min typ max>.

Also, add helper functions to convert from display timings to a generic videomode
structure. This videomode can then be converted to the corresponding subsystem
mode representation (e.g. fb_videomode).

Signed-off-by: Steffen Trumtrar <redacted>
---
 drivers/video/Kconfig          |    6 ++++
 drivers/video/Makefile         |    2 ++
 drivers/video/display_timing.c |   22 +++++++++++++
 drivers/video/videomode.c      |   45 ++++++++++++++++++++++++++
 include/linux/display_timing.h |   69 ++++++++++++++++++++++++++++++++++++++++
 include/linux/videomode.h      |   39 +++++++++++++++++++++++
 6 files changed, 183 insertions(+)
 create mode 100644 drivers/video/display_timing.c
 create mode 100644 drivers/video/videomode.c
 create mode 100644 include/linux/display_timing.h
 create mode 100644 include/linux/videomode.h
diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
index d08d799..2a23b18 100644
--- a/drivers/video/Kconfig
+++ b/drivers/video/Kconfig
@@ -33,6 +33,12 @@ config VIDEO_OUTPUT_CONTROL
 	  This framework adds support for low-level control of the video 
 	  output switch.
 
+config DISPLAY_TIMING
+       bool
+
+config VIDEOMODE
+       bool
+
 menuconfig FB
 	tristate "Support for frame buffer devices"
 	---help---
diff --git a/drivers/video/Makefile b/drivers/video/Makefile
index 23e948e..fc30439 100644
--- a/drivers/video/Makefile
+++ b/drivers/video/Makefile
@@ -167,3 +167,5 @@ obj-$(CONFIG_FB_VIRTUAL)          += vfb.o
 
 #video output switch sysfs driver
 obj-$(CONFIG_VIDEO_OUTPUT_CONTROL) += output.o
+obj-$(CONFIG_DISPLAY_TIMING) += display_timing.o
+obj-$(CONFIG_VIDEOMODE) += videomode.o
diff --git a/drivers/video/display_timing.c b/drivers/video/display_timing.c
new file mode 100644
index 0000000..04b7b69
--- /dev/null
+++ b/drivers/video/display_timing.c
@@ -0,0 +1,22 @@
+/*
+ * generic display timing functions
+ *
+ * Copyright (c) 2012 Steffen Trumtrar <s.trumtrar@pengutronix.de>, Pengutronix
+ *
+ * This file is released under the GPLv2
+ */
+
+#include <linux/display_timing.h>
+#include <linux/slab.h>
+
+void display_timings_release(struct display_timings *disp)
+{
+	if (disp->timings) {
+		unsigned int i;
+
+		for (i = 0; i < disp->num_timings; i++)
+			kfree(disp->timings[i]);
+		kfree(disp->timings);
+	}
+	kfree(disp);
+}
diff --git a/drivers/video/videomode.c b/drivers/video/videomode.c
new file mode 100644
index 0000000..087374a
--- /dev/null
+++ b/drivers/video/videomode.c
@@ -0,0 +1,45 @@
+/*
+ * generic display timing functions
+ *
+ * Copyright (c) 2012 Steffen Trumtrar <s.trumtrar@pengutronix.de>, Pengutronix
+ *
+ * This file is released under the GPLv2
+ */
+
+#include <linux/export.h>
+#include <linux/errno.h>
+#include <linux/display_timing.h>
+#include <linux/kernel.h>
+#include <linux/videomode.h>
+
+int videomode_from_timing(struct display_timings *disp, struct videomode *vm,
+			  unsigned int index)
+{
+	struct display_timing *dt;
+
+	dt = display_timings_get(disp, index);
+	if (!dt)
+		return -EINVAL;
+
+	vm->pixelclock = display_timing_get_value(&dt->pixelclock, 0);
+	vm->hactive = display_timing_get_value(&dt->hactive, 0);
+	vm->hfront_porch = display_timing_get_value(&dt->hfront_porch, 0);
+	vm->hback_porch = display_timing_get_value(&dt->hback_porch, 0);
+	vm->hsync_len = display_timing_get_value(&dt->hsync_len, 0);
+
+	vm->vactive = display_timing_get_value(&dt->vactive, 0);
+	vm->vfront_porch = display_timing_get_value(&dt->vfront_porch, 0);
+	vm->vback_porch = display_timing_get_value(&dt->vback_porch, 0);
+	vm->vsync_len = display_timing_get_value(&dt->vsync_len, 0);
+
+	vm->vah = dt->vsync_pol_active;
+	vm->hah = dt->hsync_pol_active;
+	vm->de = dt->de_pol_active;
+	vm->pixelclk_pol = dt->pixelclk_pol;
+
+	vm->interlaced = dt->interlaced;
+	vm->doublescan = dt->doublescan;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(videomode_from_timing);
diff --git a/include/linux/display_timing.h b/include/linux/display_timing.h
new file mode 100644
index 0000000..0ed2a1e
--- /dev/null
+++ b/include/linux/display_timing.h
@@ -0,0 +1,69 @@
+/*
+ * Copyright 2012 Steffen Trumtrar <s.trumtrar@pengutronix.de>
+ *
+ * description of display timings
+ *
+ * This file is released under the GPLv2
+ */
+
+#ifndef __LINUX_DISPLAY_TIMINGS_H
+#define __LINUX_DISPLAY_TIMINGS_H
+
+#include <linux/types.h>
+
+struct timing_entry {
+	u32 min;
+	u32 typ;
+	u32 max;
+};
+
+struct display_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;
+
+	unsigned int vsync_pol_active;
+	unsigned int hsync_pol_active;
+	unsigned int de_pol_active;
+	unsigned int pixelclk_pol;
+	bool interlaced;
+	bool doublescan;
+};
+
+struct display_timings {
+	unsigned int num_timings;
+	unsigned int native_mode;
+
+	struct display_timing **timings;
+};
+
+/* placeholder function until ranges are really needed 
+ * the index parameter should then be used to select one of [min typ max]
+ */
+static inline u32 display_timing_get_value(struct timing_entry *te,
+					   unsigned int index)
+{
+	return te->typ;
+}
+
+static inline struct display_timing *display_timings_get(struct display_timings *disp,
+							 unsigned int index)
+{
+	if (disp->num_timings > index)
+		return disp->timings[index];
+	else
+		return NULL;
+}
+
+void timings_release(struct display_timings *disp);
+void display_timings_release(struct display_timings *disp);
+
+#endif
diff --git a/include/linux/videomode.h b/include/linux/videomode.h
new file mode 100644
index 0000000..0b87fbb
--- /dev/null
+++ b/include/linux/videomode.h
@@ -0,0 +1,39 @@
+/*
+ * 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 <linux/display_timing.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;
+
+	u32 hah;
+	u32 vah;
+	u32 de;
+	u32 pixelclk_pol;
+
+	bool interlaced;
+	bool doublescan;
+};
+
+int videomode_from_timing(struct display_timings *disp, struct videomode *vm,
+			  unsigned int index);
+#endif
-- 
1.7.10.4

[PATCH v8 2/6] video: add of helper for videomode

From: Steffen Trumtrar <hidden>
Date: 2012-11-12 15:37:02

This adds support for reading display timings from DT or/and convert one of those
timings to a videomode.
The of_display_timing implementation supports multiple children where each
property can have up to 3 values. All children are read into an array, that
can be queried.
of_get_videomode converts exactly one of that timings to a struct videomode.

Signed-off-by: Steffen Trumtrar <redacted>
Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 .../devicetree/bindings/video/display-timings.txt  |  107 +++++++++++
 drivers/video/Kconfig                              |   13 ++
 drivers/video/Makefile                             |    2 +
 drivers/video/of_display_timing.c                  |  186 ++++++++++++++++++++
 drivers/video/of_videomode.c                       |   47 +++++
 include/linux/of_display_timings.h                 |   19 ++
 include/linux/of_videomode.h                       |   15 ++
 7 files changed, 389 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/video/display-timings.txt
 create mode 100644 drivers/video/of_display_timing.c
 create mode 100644 drivers/video/of_videomode.c
 create mode 100644 include/linux/of_display_timings.h
 create mode 100644 include/linux/of_videomode.h
diff --git a/Documentation/devicetree/bindings/video/display-timings.txt b/Documentation/devicetree/bindings/video/display-timings.txt
new file mode 100644
index 0000000..e22e2fd
--- /dev/null
+++ b/Documentation/devicetree/bindings/video/display-timings.txt
@@ -0,0 +1,107 @@
+display-timings bindings
+============
+
+display-timings-node
+--------------------
+
+required properties:
+ - none
+
+optional properties:
+ - native-mode: the native mode for the display, in case multiple modes are
+		provided. When omitted, assume the first node is the native.
+
+timings-subnode
+---------------
+
+required properties:
+ - 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-frequency: displayclock in Hz
+
+optional properties:
+ - hsync-active : Hsync pulse is active low/high/ignored
+ - vsync-active : Vsync pulse is active low/high/ignored
+ - de-active : Data-Enable pulse is active low/high/ignored
+ - pixelclk-inverted : pixelclock is inverted/non-inverted/ignored
+ - interlaced (bool)
+ - doublescan (bool)
+
+All the optional properties that are not bool follow the following logic:
+    <1> : high active
+    <0> : low active
+    omitted : not used on hardware
+
+There are different ways of describing the capabilities of a display. The devicetree
+representation corresponds to the one commonly found in datasheets for displays.
+If a display supports multiple signal timings, the native-mode can be specified.
+
+The parameters are defined as
+
+struct display_timing
+==========+
+  +----------+---------------------------------------------+----------+-------+
+  |          |                ↑                            |          |       |
+  |          |                |vback_porch                 |          |       |
+  |          |                ↓                            |          |       |
+  +----------###############################################----------+-------+
+  |          #                ↑                            #          |       |
+  |          #                |                            #          |       |
+  |  hback   #                |                            #  hfront  | hsync |
+  |   porch  #                |       hactive              #  porch   |  len  |
+  |<-------->#<---------------+--------------------------->#<-------->|<----->|
+  |          #                |                            #          |       |
+  |          #                |vactive                     #          |       |
+  |          #                |                            #          |       |
+  |          #                ↓                            #          |       |
+  +----------###############################################----------+-------+
+  |          |                ↑                            |          |       |
+  |          |                |vfront_porch                |          |       |
+  |          |                ↓                            |          |       |
+  +----------+---------------------------------------------+----------+-------+
+  |          |                ↑                            |          |       |
+  |          |                |vsync_len                   |          |       |
+  |          |                ↓                            |          |       |
+  +----------+---------------------------------------------+----------+-------+
+
+
+Example:
+
+	display-timings {
+		native-mode = <&timing0>;
+		timing0: 1920p24 {
+			/* 1920x1080p24 */
+			clock-frequency = <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 = <1>;
+		};
+	};
+
+Every required property also supports the use of ranges, so the commonly used
+datasheet description with <min typ max>-tuples can be used.
+
+Example:
+
+	timing1: timing {
+		/* 1920x1080p24 */
+		clock-frequency = <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>;
+	};
diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
index 2a23b18..a324457 100644
--- a/drivers/video/Kconfig
+++ b/drivers/video/Kconfig
@@ -39,6 +39,19 @@ config DISPLAY_TIMING
 config VIDEOMODE
        bool
 
+config OF_DISPLAY_TIMING
+	def_bool y
+	select DISPLAY_TIMING
+	help
+	  helper to parse display timings from the devicetree
+
+config OF_VIDEOMODE
+	def_bool y
+	select VIDEOMODE
+	select OF_DISPLAY_TIMING
+	help
+	  helper to get videomodes from the devicetree
+
 menuconfig FB
 	tristate "Support for frame buffer devices"
 	---help---
diff --git a/drivers/video/Makefile b/drivers/video/Makefile
index fc30439..b936b00 100644
--- a/drivers/video/Makefile
+++ b/drivers/video/Makefile
@@ -168,4 +168,6 @@ obj-$(CONFIG_FB_VIRTUAL)          += vfb.o
 #video output switch sysfs driver
 obj-$(CONFIG_VIDEO_OUTPUT_CONTROL) += output.o
 obj-$(CONFIG_DISPLAY_TIMING) += display_timing.o
+obj-$(CONFIG_OF_DISPLAY_TIMING) += of_display_timing.o
 obj-$(CONFIG_VIDEOMODE) += videomode.o
+obj-$(CONFIG_OF_VIDEOMODE) += of_videomode.o
diff --git a/drivers/video/of_display_timing.c b/drivers/video/of_display_timing.c
new file mode 100644
index 0000000..0bd30cc
--- /dev/null
+++ b/drivers/video/of_display_timing.c
@@ -0,0 +1,186 @@
+/*
+ * 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/export.h>
+#include <linux/of_display_timings.h>
+
+/**
+ * parse_property - parse timing_entry from device_node
+ * @np: device_node with the property
+ * @name: name of the property
+ * @result: will be set to the return value
+ *
+ * DESCRIPTION:
+ * Every display_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 parse_property(struct device_node *np, char *name,
+				struct timing_entry *result)
+{
+	struct property *prop;
+	int length, cells, 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(np, name, &result->typ);
+		result->min = result->typ;
+		result->max = result->typ;
+	} 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;
+}
+
+/**
+ * of_get_display_timing - parse display_timing entry from device_node
+ * @np: device_node with the properties
+ **/
+static struct display_timing *of_get_display_timing(struct device_node *np)
+{
+	struct display_timing *dt;
+	int ret = 0;
+
+	dt = kzalloc(sizeof(*dt), GFP_KERNEL);
+	if (!dt) {
+		pr_err("%s: could not allocate display_timing struct\n", __func__);
+		return NULL;
+	}
+
+	ret |= parse_property(np, "hback-porch", &dt->hback_porch);
+	ret |= parse_property(np, "hfront-porch", &dt->hfront_porch);
+	ret |= parse_property(np, "hactive", &dt->hactive);
+	ret |= parse_property(np, "hsync-len", &dt->hsync_len);
+	ret |= parse_property(np, "vback-porch", &dt->vback_porch);
+	ret |= parse_property(np, "vfront-porch", &dt->vfront_porch);
+	ret |= parse_property(np, "vactive", &dt->vactive);
+	ret |= parse_property(np, "vsync-len", &dt->vsync_len);
+	ret |= parse_property(np, "clock-frequency", &dt->pixelclock);
+
+	of_property_read_u32(np, "vsync-active", &dt->vsync_pol_active);
+	of_property_read_u32(np, "hsync-active", &dt->hsync_pol_active);
+	of_property_read_u32(np, "de-active", &dt->de_pol_active);
+	of_property_read_u32(np, "pixelclk-inverted", &dt->pixelclk_pol);
+	dt->interlaced = of_property_read_bool(np, "interlaced");
+	dt->doublescan = of_property_read_bool(np, "doublescan");
+
+	if (ret) {
+		pr_err("%s: error reading timing properties\n", __func__);
+		return NULL;
+	}
+
+	return dt;
+}
+
+/**
+ * of_get_display_timings - parse all display_timing entries from a device_node
+ * @np: device_node with the subnodes
+ **/
+struct display_timings *of_get_display_timings(struct device_node *np)
+{
+	struct device_node *timings_np;
+	struct device_node *entry;
+	struct device_node *native_mode;
+	struct display_timings *disp;
+
+	if (!np) {
+		pr_err("%s: no devicenode given\n", __func__);
+		return NULL;
+	}
+
+	timings_np = of_find_node_by_name(np, "display-timings");
+	if (!timings_np) {
+		pr_err("%s: could not find display-timings node\n", __func__);
+		return NULL;
+	}
+
+	disp = kzalloc(sizeof(*disp), GFP_KERNEL);
+	if (!disp)
+		return -ENOMEM;
+
+	entry = of_parse_phandle(timings_np, "native-mode", 0);
+	/* assume first child as native mode if none provided */
+	if (!entry)
+		entry = of_get_next_child(np, NULL);
+	if (!entry) {
+		pr_err("%s: no timing specifications given\n", __func__);
+		return NULL;
+	}
+
+	pr_info("%s: using %s as default timing\n", __func__, entry->name);
+
+	native_mode = entry;
+
+	disp->num_timings = of_get_child_count(timings_np);
+	disp->timings = kzalloc(sizeof(struct display_timing *)*disp->num_timings,
+				GFP_KERNEL);
+	if (!disp->timings)
+		return -ENOMEM;
+
+	disp->num_timings = 0;
+	disp->native_mode = 0;
+
+	for_each_child_of_node(timings_np, entry) {
+		struct display_timing *dt;
+
+		dt = of_get_display_timing(entry);
+		if (!dt) {
+			/* to not encourage wrong devicetrees, fail in case of an error */
+			pr_err("%s: error in timing %d\n", __func__, disp->num_timings+1);
+			return NULL;
+		}
+
+		if (native_mode = entry)
+			disp->native_mode = disp->num_timings;
+
+		disp->timings[disp->num_timings] = dt;
+		disp->num_timings++;
+	}
+	of_node_put(timings_np);
+
+	if (disp->num_timings > 0)
+		pr_info("%s: got %d timings. Using timing #%d as default\n", __func__,
+			disp->num_timings , disp->native_mode + 1);
+	else {
+		pr_err("%s: no valid timings specified\n", __func__);
+		return NULL;
+	}
+	return disp;
+}
+EXPORT_SYMBOL_GPL(of_get_display_timings);
+
+/**
+ * of_display_timings_exists - check if a display-timings node is provided
+ * @np: device_node with the timing
+ **/
+int of_display_timings_exists(struct device_node *np)
+{
+	struct device_node *timings_np;
+
+	if (!np)
+		return -EINVAL;
+
+	timings_np = of_parse_phandle(np, "display-timings", 0);
+	if (!timings_np)
+		return -EINVAL;
+
+	return 1;
+}
+EXPORT_SYMBOL_GPL(of_display_timings_exists);
diff --git a/drivers/video/of_videomode.c b/drivers/video/of_videomode.c
new file mode 100644
index 0000000..7051701
--- /dev/null
+++ b/drivers/video/of_videomode.c
@@ -0,0 +1,47 @@
+/*
+ * 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/of_display_timings.h>
+#include <linux/of_videomode.h>
+#include <linux/export.h>
+
+/**
+ * of_get_videomode - get the videomode #<index> from devicetree
+ * @np - devicenode with the display_timings
+ * @vm - set to return value
+ * @index - index into list of display_timings
+ * DESCRIPTION:
+ * Get a list of all display timings and put the one
+ * specified by index into *vm. This function should only be used, if
+ * only one videomode is to be retrieved. A driver that needs to work
+ * with multiple/all videomodes should work with
+ * of_get_display_timings instead.
+ **/
+int of_get_videomode(struct device_node *np, struct videomode *vm, int index)
+{
+	struct display_timings *disp;
+	int ret;
+
+	disp = of_get_display_timings(np);
+	if (!disp) {
+		pr_err("%s: no timings specified\n", __func__);
+		return -EINVAL;
+	}
+
+	if (index = OF_USE_NATIVE_MODE)
+		index = disp->native_mode;
+
+	ret = videomode_from_timing(disp, vm, index);
+	if (ret)
+		return ret;
+
+	display_timings_release(disp);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(of_get_videomode);
diff --git a/include/linux/of_display_timings.h b/include/linux/of_display_timings.h
new file mode 100644
index 0000000..ccf6814
--- /dev/null
+++ b/include/linux/of_display_timings.h
@@ -0,0 +1,19 @@
+/*
+ * Copyright 2012 Steffen Trumtrar <s.trumtrar@pengutronix.de>
+ *
+ * display timings of helpers
+ *
+ * This file is released under the GPLv2
+ */
+
+#ifndef __LINUX_OF_DISPLAY_TIMINGS_H
+#define __LINUX_OF_DISPLAY_TIMINGS_H
+
+#include <linux/display_timing.h>
+
+#define OF_USE_NATIVE_MODE -1
+
+struct display_timings *of_get_display_timings(struct device_node *np);
+int of_display_timings_exists(struct device_node *np);
+
+#endif
diff --git a/include/linux/of_videomode.h b/include/linux/of_videomode.h
new file mode 100644
index 0000000..01518e6
--- /dev/null
+++ b/include/linux/of_videomode.h
@@ -0,0 +1,15 @@
+/*
+ * Copyright 2012 Steffen Trumtrar <s.trumtrar@pengutronix.de>
+ *
+ * videomode of-helpers
+ *
+ * This file is released under the GPLv2
+ */
+
+#ifndef __LINUX_OF_VIDEOMODE_H
+#define __LINUX_OF_VIDEOMODE_H
+
+#include <linux/videomode.h>
+
+int of_get_videomode(struct device_node *np, struct videomode *vm, int index);
+#endif /* __LINUX_OF_VIDEOMODE_H */
-- 
1.7.10.4

[PATCH v8 3/6] fbmon: add videomode helpers

From: Steffen Trumtrar <hidden>
Date: 2012-11-12 15:37:03

Add a function to convert from the generic videomode to a fb_videomode.

Signed-off-by: Steffen Trumtrar <redacted>
---
 drivers/video/fbmon.c |   37 +++++++++++++++++++++++++++++++++++++
 include/linux/fb.h    |    2 ++
 2 files changed, 39 insertions(+)
diff --git a/drivers/video/fbmon.c b/drivers/video/fbmon.c
index cef6557..d46ecef 100644
--- a/drivers/video/fbmon.c
+++ b/drivers/video/fbmon.c
@@ -1373,6 +1373,43 @@ int fb_get_mode(int flags, u32 val, struct fb_var_screeninfo *var, struct fb_inf
 	kfree(timings);
 	return err;
 }
+
+#if IS_ENABLED(CONFIG_VIDEOMODE)
+int videomode_to_fb_videomode(struct videomode *vm, struct fb_videomode *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);
+
+	fbmode->sync = 0;
+	fbmode->vmode = 0;
+	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;
+	if (vm->de)
+		fbmode->sync |= FB_SYNC_DATA_ENABLE_HIGH_ACT;
+	fbmode->refresh = 60;
+	fbmode->flag = 0;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(videomode_to_fb_videomode);
+#endif
+
+
 #else
 int fb_parse_edid(unsigned char *edid, struct fb_var_screeninfo *var)
 {
diff --git a/include/linux/fb.h b/include/linux/fb.h
index c7a9571..46c665b 100644
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -714,6 +714,8 @@ extern void fb_destroy_modedb(struct fb_videomode *modedb);
 extern int fb_find_mode_cvt(struct fb_videomode *mode, int margins, int rb);
 extern unsigned char *fb_ddc_read(struct i2c_adapter *adapter);
 
+extern int videomode_to_fb_videomode(struct videomode *vm, struct fb_videomode *fbmode);
+
 /* drivers/video/modedb.c */
 #define VESA_MODEDB_SIZE 34
 extern void fb_var_to_videomode(struct fb_videomode *mode,
-- 
1.7.10.4

[PATCH v8 4/6] fbmon: add of_videomode helpers

From: Steffen Trumtrar <hidden>
Date: 2012-11-12 15:37:04

Add helper to get fb_videomode from devicetree.

Signed-off-by: Steffen Trumtrar <redacted>
---
 drivers/video/fbmon.c |   40 ++++++++++++++++++++++++++++++++++++++++
 include/linux/fb.h    |    3 +++
 2 files changed, 43 insertions(+)
diff --git a/drivers/video/fbmon.c b/drivers/video/fbmon.c
index d46ecef..c95be79 100644
--- a/drivers/video/fbmon.c
+++ b/drivers/video/fbmon.c
@@ -1409,6 +1409,46 @@ int videomode_to_fb_videomode(struct videomode *vm, struct fb_videomode *fbmode)
 EXPORT_SYMBOL_GPL(videomode_to_fb_videomode);
 #endif
 
+#if IS_ENABLED(CONFIG_OF_VIDEOMODE)
+static void dump_fb_videomode(struct fb_videomode *m)
+{
+	pr_debug("fb_videomode = %ux%u@%uHz (%ukHz) %u %u %u %u %u %u %u %u %u\n",
+		 m->xres, m->yres, m->refresh, m->pixclock, m->left_margin,
+		 m->right_margin, m->upper_margin, m->lower_margin,
+		 m->hsync_len, m->vsync_len, m->sync, m->vmode, m->flag);
+}
+
+/**
+ * of_get_fb_videomode - get a fb_videomode from devicetree
+ * @np: device_node with the timing specification
+ * @fb: will be set to the return value
+ * @index: index into the list of display timings in devicetree
+ *
+ * DESCRIPTION:
+ * This function is expensive and should only be used, if only one mode is to be
+ * read from DT. To get multiple modes start with of_get_display_timings ond
+ * work with that instead.
+ */
+int of_get_fb_videomode(struct device_node *np, struct fb_videomode *fb,
+			int index)
+{
+	struct videomode vm;
+	int ret;
+
+	ret = of_get_videomode(np, &vm, index);
+	if (ret)
+		return ret;
+
+	videomode_to_fb_videomode(&vm, fb);
+
+	pr_info("%s: got %dx%d display mode from %s\n", __func__, vm.hactive,
+		vm.vactive, np->name);
+	dump_fb_videomode(fb);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(of_get_fb_videomode);
+#endif
 
 #else
 int fb_parse_edid(unsigned char *edid, struct fb_var_screeninfo *var)
diff --git a/include/linux/fb.h b/include/linux/fb.h
index 46c665b..9892fd6 100644
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -14,6 +14,8 @@
 #include <linux/backlight.h>
 #include <linux/slab.h>
 #include <asm/io.h>
+#include <linux/of.h>
+#include <linux/of_videomode.h>
 
 struct vm_area_struct;
 struct fb_info;
@@ -714,6 +716,7 @@ extern void fb_destroy_modedb(struct fb_videomode *modedb);
 extern int fb_find_mode_cvt(struct fb_videomode *mode, int margins, int rb);
 extern unsigned char *fb_ddc_read(struct i2c_adapter *adapter);
 
+extern int of_get_fb_videomode(struct device_node *np, struct fb_videomode *fb, int index);
 extern int videomode_to_fb_videomode(struct videomode *vm, struct fb_videomode *fbmode);
 
 /* drivers/video/modedb.c */
-- 
1.7.10.4

[PATCH v8 5/6] drm_modes: add videomode helpers

From: Steffen Trumtrar <hidden>
Date: 2012-11-12 15:37:05

Add conversion from videomode to drm_display_mode

Signed-off-by: Steffen Trumtrar <redacted>
---
 drivers/gpu/drm/drm_modes.c |   36 ++++++++++++++++++++++++++++++++++++
 include/drm/drmP.h          |    3 +++
 2 files changed, 39 insertions(+)
diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index 59450f3..049624d 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -35,6 +35,7 @@
 #include <linux/export.h>
 #include <drm/drmP.h>
 #include <drm/drm_crtc.h>
+#include <linux/videomode.h>
 
 /**
  * drm_mode_debug_printmodeline - debug print a mode
@@ -504,6 +505,41 @@ drm_gtf_mode(struct drm_device *dev, int hdisplay, int vdisplay, int vrefresh,
 }
 EXPORT_SYMBOL(drm_gtf_mode);
 
+#if IS_ENABLED(CONFIG_VIDEOMODE)
+int videomode_to_display_mode(struct videomode *vm, struct drm_display_mode *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->vsync_end + vm->vback_porch;
+
+	dmode->clock = vm->pixelclock / 1000;
+
+	dmode->flags = 0;
+	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);
+#endif
+
 /**
  * drm_mode_set_name - set the name on a mode
  * @mode: name will be set in this mode
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 3fd8280..e9fa1e3 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -56,6 +56,7 @@
 #include <linux/cdev.h>
 #include <linux/mutex.h>
 #include <linux/slab.h>
+#include <linux/videomode.h>
 #if defined(__alpha__) || defined(__powerpc__)
 #include <asm/pgtable.h>	/* For pte_wrprotect */
 #endif
@@ -1454,6 +1455,8 @@ extern struct drm_display_mode *
 drm_mode_create_from_cmdline_mode(struct drm_device *dev,
 				  struct drm_cmdline_mode *cmd);
 
+extern int videomode_to_display_mode(struct videomode *vm,
+				     struct drm_display_mode *dmode);
 /* Modesetting support */
 extern void drm_vblank_pre_modeset(struct drm_device *dev, int crtc);
 extern void drm_vblank_post_modeset(struct drm_device *dev, int crtc);
-- 
1.7.10.4

[PATCH v8 6/6] drm_modes: add of_videomode helpers

From: Steffen Trumtrar <hidden>
Date: 2012-11-12 15:37:06

Add helper to get drm_display_mode from devicetree.

Signed-off-by: Steffen Trumtrar <redacted>
---
 drivers/gpu/drm/drm_modes.c |   41 +++++++++++++++++++++++++++++++++++++++++
 include/drm/drmP.h          |    5 +++++
 2 files changed, 46 insertions(+)
diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index 049624d..c77da59 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -35,6 +35,7 @@
 #include <linux/export.h>
 #include <drm/drmP.h>
 #include <drm/drm_crtc.h>
+#include <linux/of.h>
 #include <linux/videomode.h>
 
 /**
@@ -540,6 +541,46 @@ int videomode_to_display_mode(struct videomode *vm, struct drm_display_mode *dmo
 EXPORT_SYMBOL_GPL(videomode_to_display_mode);
 #endif
 
+#if IS_ENABLED(CONFIG_OF_VIDEOMODE)
+static void dump_drm_displaymode(struct drm_display_mode *m)
+{
+	pr_debug("drm_displaymode = %d %d %d %d %d %d %d %d %d\n",
+		 m->hdisplay, m->hsync_start, m->hsync_end, m->htotal,
+		 m->vdisplay, m->vsync_start, m->vsync_end, m->vtotal,
+		 m->clock);
+}
+
+/**
+ * of_get_drm_display_mode - get a drm_display_mode from devicetree
+ * @np: device_node with the timing specification
+ * @dmode: will be set to the return value
+ * @index: index into the list of display timings in devicetree
+ * 
+ * This function is expensive and should only be used, if only one mode is to be
+ * read from DT. To get multiple modes start with of_get_display_timings and
+ * work with that instead.
+ */
+int of_get_drm_display_mode(struct device_node *np, struct drm_display_mode *dmode,
+			int index)
+{
+	struct videomode vm;
+	int ret;
+
+	ret = of_get_videomode(np, &vm, index);
+	if (ret)
+		return ret;
+
+	videomode_to_display_mode(&vm, dmode);
+
+	pr_info("%s: got %dx%d display mode from %s\n", __func__, vm.hactive,
+		vm.vactive, np->name);
+	dump_drm_displaymode(dmode);
+
+	return 0;
+
+}
+EXPORT_SYMBOL_GPL(of_get_drm_display_mode);
+#endif
 /**
  * drm_mode_set_name - set the name on a mode
  * @mode: name will be set in this mode
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index e9fa1e3..4f9c44e 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -56,6 +56,7 @@
 #include <linux/cdev.h>
 #include <linux/mutex.h>
 #include <linux/slab.h>
+#include <linux/of.h>
 #include <linux/videomode.h>
 #if defined(__alpha__) || defined(__powerpc__)
 #include <asm/pgtable.h>	/* For pte_wrprotect */
@@ -1457,6 +1458,10 @@ drm_mode_create_from_cmdline_mode(struct drm_device *dev,
 
 extern int videomode_to_display_mode(struct videomode *vm,
 				     struct drm_display_mode *dmode);
+extern int of_get_drm_display_mode(struct device_node *np,
+				   struct drm_display_mode *dmode,
+				   int index);
+
 /* Modesetting support */
 extern void drm_vblank_pre_modeset(struct drm_device *dev, int crtc);
 extern void drm_vblank_post_modeset(struct drm_device *dev, int crtc);
-- 
1.7.10.4

Re: [PATCH v8 2/6] video: add of helper for videomode

From: Sascha Hauer <s.hauer@pengutronix.de>
Date: 2012-11-12 18:58:40

Hi Steffen,

You lose memory in several places:

On Mon, Nov 12, 2012 at 04:37:02PM +0100, Steffen Trumtrar wrote:
+static struct display_timing *of_get_display_timing(struct device_node *np)
+{
+	struct display_timing *dt;
+	int ret = 0;
+
+	dt = kzalloc(sizeof(*dt), GFP_KERNEL);
+	if (!dt) {
+		pr_err("%s: could not allocate display_timing struct\n", __func__);
+		return NULL;
+	}
+
+	ret |= parse_property(np, "hback-porch", &dt->hback_porch);
+	ret |= parse_property(np, "hfront-porch", &dt->hfront_porch);
+	ret |= parse_property(np, "hactive", &dt->hactive);
+	ret |= parse_property(np, "hsync-len", &dt->hsync_len);
+	ret |= parse_property(np, "vback-porch", &dt->vback_porch);
+	ret |= parse_property(np, "vfront-porch", &dt->vfront_porch);
+	ret |= parse_property(np, "vactive", &dt->vactive);
+	ret |= parse_property(np, "vsync-len", &dt->vsync_len);
+	ret |= parse_property(np, "clock-frequency", &dt->pixelclock);
+
+	of_property_read_u32(np, "vsync-active", &dt->vsync_pol_active);
+	of_property_read_u32(np, "hsync-active", &dt->hsync_pol_active);
+	of_property_read_u32(np, "de-active", &dt->de_pol_active);
+	of_property_read_u32(np, "pixelclk-inverted", &dt->pixelclk_pol);
+	dt->interlaced = of_property_read_bool(np, "interlaced");
+	dt->doublescan = of_property_read_bool(np, "doublescan");
+
+	if (ret) {
+		pr_err("%s: error reading timing properties\n", __func__);
+		return NULL;
Here
+	}
+
+	return dt;
+}
+
+/**
+ * of_get_display_timings - parse all display_timing entries from a device_node
+ * @np: device_node with the subnodes
+ **/
+struct display_timings *of_get_display_timings(struct device_node *np)
+{
+	struct device_node *timings_np;
+	struct device_node *entry;
+	struct device_node *native_mode;
+	struct display_timings *disp;
+
+	if (!np) {
+		pr_err("%s: no devicenode given\n", __func__);
+		return NULL;
+	}
+
+	timings_np = of_find_node_by_name(np, "display-timings");
+	if (!timings_np) {
+		pr_err("%s: could not find display-timings node\n", __func__);
+		return NULL;
+	}
+
+	disp = kzalloc(sizeof(*disp), GFP_KERNEL);
+	if (!disp)
+		return -ENOMEM;
+
+	entry = of_parse_phandle(timings_np, "native-mode", 0);
+	/* assume first child as native mode if none provided */
+	if (!entry)
+		entry = of_get_next_child(np, NULL);
+	if (!entry) {
+		pr_err("%s: no timing specifications given\n", __func__);
+		return NULL;
Here
+	}
+
+	pr_info("%s: using %s as default timing\n", __func__, entry->name);
+
+	native_mode = entry;
+
+	disp->num_timings = of_get_child_count(timings_np);
+	disp->timings = kzalloc(sizeof(struct display_timing *)*disp->num_timings,
+				GFP_KERNEL);
+	if (!disp->timings)
+		return -ENOMEM;
Here
+
+	disp->num_timings = 0;
+	disp->native_mode = 0;
+
+	for_each_child_of_node(timings_np, entry) {
+		struct display_timing *dt;
+
+		dt = of_get_display_timing(entry);
+		if (!dt) {
+			/* to not encourage wrong devicetrees, fail in case of an error */
+			pr_err("%s: error in timing %d\n", __func__, disp->num_timings+1);
+			return NULL;
Here
+		}
+
+		if (native_mode = entry)
+			disp->native_mode = disp->num_timings;
+
+		disp->timings[disp->num_timings] = dt;
+		disp->num_timings++;
+	}
+	of_node_put(timings_np);
+
+	if (disp->num_timings > 0)
+		pr_info("%s: got %d timings. Using timing #%d as default\n", __func__,
+			disp->num_timings , disp->native_mode + 1);
+	else {
+		pr_err("%s: no valid timings specified\n", __func__);
+		return NULL;
and here

Sascha

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

Re: [PATCH v8 2/6] video: add of helper for videomode

From: Alexey Klimov <hidden>
Date: 2012-11-12 19:00:37

Hello Steffen,

On Mon, Nov 12, 2012 at 7:37 PM, Steffen Trumtrar
[off-list ref] wrote:
quoted hunk
This adds support for reading display timings from DT or/and convert one of those
timings to a videomode.
The of_display_timing implementation supports multiple children where each
property can have up to 3 values. All children are read into an array, that
can be queried.
of_get_videomode converts exactly one of that timings to a struct videomode.

Signed-off-by: Steffen Trumtrar <redacted>
Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 .../devicetree/bindings/video/display-timings.txt  |  107 +++++++++++
 drivers/video/Kconfig                              |   13 ++
 drivers/video/Makefile                             |    2 +
 drivers/video/of_display_timing.c                  |  186 ++++++++++++++++++++
 drivers/video/of_videomode.c                       |   47 +++++
 include/linux/of_display_timings.h                 |   19 ++
 include/linux/of_videomode.h                       |   15 ++
 7 files changed, 389 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/video/display-timings.txt
 create mode 100644 drivers/video/of_display_timing.c
 create mode 100644 drivers/video/of_videomode.c
 create mode 100644 include/linux/of_display_timings.h
 create mode 100644 include/linux/of_videomode.h
diff --git a/Documentation/devicetree/bindings/video/display-timings.txt b/Documentation/devicetree/bindings/video/display-timings.txt
new file mode 100644
index 0000000..e22e2fd
--- /dev/null
+++ b/Documentation/devicetree/bindings/video/display-timings.txt
@@ -0,0 +1,107 @@
+display-timings bindings
+============
+
+display-timings-node
+--------------------
+
+required properties:
+ - none
+
+optional properties:
+ - native-mode: the native mode for the display, in case multiple modes are
+               provided. When omitted, assume the first node is the native.
+
+timings-subnode
+---------------
+
+required properties:
+ - 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-frequency: displayclock in Hz
+
+optional properties:
+ - hsync-active : Hsync pulse is active low/high/ignored
+ - vsync-active : Vsync pulse is active low/high/ignored
+ - de-active : Data-Enable pulse is active low/high/ignored
+ - pixelclk-inverted : pixelclock is inverted/non-inverted/ignored
+ - interlaced (bool)
+ - doublescan (bool)
+
+All the optional properties that are not bool follow the following logic:
+    <1> : high active
+    <0> : low active
+    omitted : not used on hardware
+
+There are different ways of describing the capabilities of a display. The devicetree
+representation corresponds to the one commonly found in datasheets for displays.
+If a display supports multiple signal timings, the native-mode can be specified.
+
+The parameters are defined as
+
+struct display_timing
+==========> +
+  +----------+---------------------------------------------+----------+-------+
+  |          |                ↑                            |          |       |
+  |          |                |vback_porch                 |          |       |
+  |          |                ↓                            |          |       |
+  +----------###############################################----------+-------+
+  |          #                ↑                            #          |       |
+  |          #                |                            #          |       |
+  |  hback   #                |                            #  hfront  | hsync |
+  |   porch  #                |       hactive              #  porch   |  len  |
+  |<-------->#<---------------+--------------------------->#<-------->|<----->|
+  |          #                |                            #          |       |
+  |          #                |vactive                     #          |       |
+  |          #                |                            #          |       |
+  |          #                ↓                            #          |       |
+  +----------###############################################----------+-------+
+  |          |                ↑                            |          |       |
+  |          |                |vfront_porch                |          |       |
+  |          |                ↓                            |          |       |
+  +----------+---------------------------------------------+----------+-------+
+  |          |                ↑                            |          |       |
+  |          |                |vsync_len                   |          |       |
+  |          |                ↓                            |          |       |
+  +----------+---------------------------------------------+----------+-------+
+
+
+Example:
+
+       display-timings {
+               native-mode = <&timing0>;
+               timing0: 1920p24 {
+                       /* 1920x1080p24 */
+                       clock-frequency = <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 = <1>;
+               };
+       };
+
+Every required property also supports the use of ranges, so the commonly used
+datasheet description with <min typ max>-tuples can be used.
+
+Example:
+
+       timing1: timing {
+               /* 1920x1080p24 */
+               clock-frequency = <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>;
+       };
diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
index 2a23b18..a324457 100644
--- a/drivers/video/Kconfig
+++ b/drivers/video/Kconfig
@@ -39,6 +39,19 @@ config DISPLAY_TIMING
 config VIDEOMODE
        bool

+config OF_DISPLAY_TIMING
+       def_bool y
+       select DISPLAY_TIMING
+       help
+         helper to parse display timings from the devicetree
+
+config OF_VIDEOMODE
+       def_bool y
+       select VIDEOMODE
+       select OF_DISPLAY_TIMING
+       help
+         helper to get videomodes from the devicetree
+
 menuconfig FB
        tristate "Support for frame buffer devices"
        ---help---
diff --git a/drivers/video/Makefile b/drivers/video/Makefile
index fc30439..b936b00 100644
--- a/drivers/video/Makefile
+++ b/drivers/video/Makefile
@@ -168,4 +168,6 @@ obj-$(CONFIG_FB_VIRTUAL)          += vfb.o
 #video output switch sysfs driver
 obj-$(CONFIG_VIDEO_OUTPUT_CONTROL) += output.o
 obj-$(CONFIG_DISPLAY_TIMING) += display_timing.o
+obj-$(CONFIG_OF_DISPLAY_TIMING) += of_display_timing.o
 obj-$(CONFIG_VIDEOMODE) += videomode.o
+obj-$(CONFIG_OF_VIDEOMODE) += of_videomode.o
diff --git a/drivers/video/of_display_timing.c b/drivers/video/of_display_timing.c
new file mode 100644
index 0000000..0bd30cc
--- /dev/null
+++ b/drivers/video/of_display_timing.c
@@ -0,0 +1,186 @@
+/*
+ * 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/export.h>
+#include <linux/of_display_timings.h>
+
+/**
+ * parse_property - parse timing_entry from device_node
+ * @np: device_node with the property
+ * @name: name of the property
+ * @result: will be set to the return value
+ *
+ * DESCRIPTION:
+ * Every display_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 parse_property(struct device_node *np, char *name,
+                               struct timing_entry *result)
+{
+       struct property *prop;
+       int length, cells, 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(np, name, &result->typ);
+               result->min = result->typ;
+               result->max = result->typ;
+       } 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;
+}
+
+/**
+ * of_get_display_timing - parse display_timing entry from device_node
+ * @np: device_node with the properties
+ **/
+static struct display_timing *of_get_display_timing(struct device_node *np)
+{
+       struct display_timing *dt;
+       int ret = 0;
+
+       dt = kzalloc(sizeof(*dt), GFP_KERNEL);
+       if (!dt) {
+               pr_err("%s: could not allocate display_timing struct\n", __func__);
+               return NULL;
+       }
+
+       ret |= parse_property(np, "hback-porch", &dt->hback_porch);
+       ret |= parse_property(np, "hfront-porch", &dt->hfront_porch);
+       ret |= parse_property(np, "hactive", &dt->hactive);
+       ret |= parse_property(np, "hsync-len", &dt->hsync_len);
+       ret |= parse_property(np, "vback-porch", &dt->vback_porch);
+       ret |= parse_property(np, "vfront-porch", &dt->vfront_porch);
+       ret |= parse_property(np, "vactive", &dt->vactive);
+       ret |= parse_property(np, "vsync-len", &dt->vsync_len);
+       ret |= parse_property(np, "clock-frequency", &dt->pixelclock);
+
+       of_property_read_u32(np, "vsync-active", &dt->vsync_pol_active);
+       of_property_read_u32(np, "hsync-active", &dt->hsync_pol_active);
+       of_property_read_u32(np, "de-active", &dt->de_pol_active);
+       of_property_read_u32(np, "pixelclk-inverted", &dt->pixelclk_pol);
+       dt->interlaced = of_property_read_bool(np, "interlaced");
+       dt->doublescan = of_property_read_bool(np, "doublescan");
+
+       if (ret) {
+               pr_err("%s: error reading timing properties\n", __func__);
+               return NULL;
+       }
+
+       return dt;
+}
+
+/**
+ * of_get_display_timings - parse all display_timing entries from a device_node
+ * @np: device_node with the subnodes
+ **/
+struct display_timings *of_get_display_timings(struct device_node *np)
+{
+       struct device_node *timings_np;
+       struct device_node *entry;
+       struct device_node *native_mode;
+       struct display_timings *disp;
+
+       if (!np) {
+               pr_err("%s: no devicenode given\n", __func__);
+               return NULL;
+       }
+
+       timings_np = of_find_node_by_name(np, "display-timings");
+       if (!timings_np) {
+               pr_err("%s: could not find display-timings node\n", __func__);
+               return NULL;
+       }
+
+       disp = kzalloc(sizeof(*disp), GFP_KERNEL);
+       if (!disp)
+               return -ENOMEM;
+
+       entry = of_parse_phandle(timings_np, "native-mode", 0);
+       /* assume first child as native mode if none provided */
+       if (!entry)
+               entry = of_get_next_child(np, NULL);
+       if (!entry) {
+               pr_err("%s: no timing specifications given\n", __func__);
+               return NULL;
+       }
+
+       pr_info("%s: using %s as default timing\n", __func__, entry->name);
+
+       native_mode = entry;
+
+       disp->num_timings = of_get_child_count(timings_np);
+       disp->timings = kzalloc(sizeof(struct display_timing *)*disp->num_timings,
+                               GFP_KERNEL);
+       if (!disp->timings)
+               return -ENOMEM;
Could you please check return values here ^^^ and above "disp kzalloc(sizeof(*disp), GFP_KERNEL);" ?
May be it's better to return NULL instead of -ENOMEM and put error message?


-- 
Best regards, Klimov Alexey

Re: [PATCH v8 2/6] video: add of helper for videomode

From: Stephen Warren <hidden>
Date: 2012-11-12 20:40:12

On 11/12/2012 08:37 AM, Steffen Trumtrar wrote:
This adds support for reading display timings from DT or/and convert one of those
timings to a videomode.
The of_display_timing implementation supports multiple children where each
property can have up to 3 values. All children are read into an array, that
can be queried.
of_get_videomode converts exactly one of that timings to a struct videomode.
quoted hunk
diff --git a/Documentation/devicetree/bindings/video/display-timings.txt b/Documentation/devicetree/bindings/video/display-timings.txt
The device tree bindings look reasonable to me, so,

Acked-by: Stephen Warren <redacted>

Re: [PATCH v8 2/6] video: add of helper for videomode

From: Steffen Trumtrar <hidden>
Date: 2012-11-13 08:32:30

Hi!

On Mon, Nov 12, 2012 at 07:58:40PM +0100, Sascha Hauer wrote:
Hi Steffen,

You lose memory in several places:

On Mon, Nov 12, 2012 at 04:37:02PM +0100, Steffen Trumtrar wrote:
quoted
+static struct display_timing *of_get_display_timing(struct device_node *np)
+{
+	struct display_timing *dt;
+	int ret = 0;
+
+	dt = kzalloc(sizeof(*dt), GFP_KERNEL);
+	if (!dt) {
+		pr_err("%s: could not allocate display_timing struct\n", __func__);
+		return NULL;
+	}
+
+	ret |= parse_property(np, "hback-porch", &dt->hback_porch);
+	ret |= parse_property(np, "hfront-porch", &dt->hfront_porch);
+	ret |= parse_property(np, "hactive", &dt->hactive);
+	ret |= parse_property(np, "hsync-len", &dt->hsync_len);
+	ret |= parse_property(np, "vback-porch", &dt->vback_porch);
+	ret |= parse_property(np, "vfront-porch", &dt->vfront_porch);
+	ret |= parse_property(np, "vactive", &dt->vactive);
+	ret |= parse_property(np, "vsync-len", &dt->vsync_len);
+	ret |= parse_property(np, "clock-frequency", &dt->pixelclock);
+
+	of_property_read_u32(np, "vsync-active", &dt->vsync_pol_active);
+	of_property_read_u32(np, "hsync-active", &dt->hsync_pol_active);
+	of_property_read_u32(np, "de-active", &dt->de_pol_active);
+	of_property_read_u32(np, "pixelclk-inverted", &dt->pixelclk_pol);
+	dt->interlaced = of_property_read_bool(np, "interlaced");
+	dt->doublescan = of_property_read_bool(np, "doublescan");
+
+	if (ret) {
+		pr_err("%s: error reading timing properties\n", __func__);
+		return NULL;
Here
quoted
+	}
+
+	return dt;
+}
+
+/**
+ * of_get_display_timings - parse all display_timing entries from a device_node
+ * @np: device_node with the subnodes
+ **/
+struct display_timings *of_get_display_timings(struct device_node *np)
+{
+	struct device_node *timings_np;
+	struct device_node *entry;
+	struct device_node *native_mode;
+	struct display_timings *disp;
+
+	if (!np) {
+		pr_err("%s: no devicenode given\n", __func__);
+		return NULL;
+	}
+
+	timings_np = of_find_node_by_name(np, "display-timings");
+	if (!timings_np) {
+		pr_err("%s: could not find display-timings node\n", __func__);
+		return NULL;
+	}
+
+	disp = kzalloc(sizeof(*disp), GFP_KERNEL);
+	if (!disp)
+		return -ENOMEM;
+
+	entry = of_parse_phandle(timings_np, "native-mode", 0);
+	/* assume first child as native mode if none provided */
+	if (!entry)
+		entry = of_get_next_child(np, NULL);
+	if (!entry) {
+		pr_err("%s: no timing specifications given\n", __func__);
+		return NULL;
Here
quoted
+	}
+
+	pr_info("%s: using %s as default timing\n", __func__, entry->name);
+
+	native_mode = entry;
+
+	disp->num_timings = of_get_child_count(timings_np);
+	disp->timings = kzalloc(sizeof(struct display_timing *)*disp->num_timings,
+				GFP_KERNEL);
+	if (!disp->timings)
+		return -ENOMEM;
Here
quoted
+
+	disp->num_timings = 0;
+	disp->native_mode = 0;
+
+	for_each_child_of_node(timings_np, entry) {
+		struct display_timing *dt;
+
+		dt = of_get_display_timing(entry);
+		if (!dt) {
+			/* to not encourage wrong devicetrees, fail in case of an error */
+			pr_err("%s: error in timing %d\n", __func__, disp->num_timings+1);
+			return NULL;
Here
quoted
+		}
+
+		if (native_mode = entry)
+			disp->native_mode = disp->num_timings;
+
+		disp->timings[disp->num_timings] = dt;
+		disp->num_timings++;
+	}
+	of_node_put(timings_np);
+
+	if (disp->num_timings > 0)
+		pr_info("%s: got %d timings. Using timing #%d as default\n", __func__,
+			disp->num_timings , disp->native_mode + 1);
+	else {
+		pr_err("%s: no valid timings specified\n", __func__);
+		return NULL;
and here
Well,... you are right. :-(

Regards,
Steffen

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

Re: [PATCH v8 1/6] video: add display_timing and videomode

From: Thierry Reding <hidden>
Date: 2012-11-13 10:41:59

On Mon, Nov 12, 2012 at 04:37:01PM +0100, Steffen Trumtrar wrote:
[...]
quoted hunk
diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
index d08d799..2a23b18 100644
--- a/drivers/video/Kconfig
+++ b/drivers/video/Kconfig
@@ -33,6 +33,12 @@ config VIDEO_OUTPUT_CONTROL
 	  This framework adds support for low-level control of the video 
 	  output switch.
 
+config DISPLAY_TIMING
DISPLAY_TIMINGS?
 #video output switch sysfs driver
 obj-$(CONFIG_VIDEO_OUTPUT_CONTROL) += output.o
+obj-$(CONFIG_DISPLAY_TIMING) += display_timing.o
display_timings.o?
quoted hunk
+obj-$(CONFIG_VIDEOMODE) += videomode.o
diff --git a/drivers/video/display_timing.c b/drivers/video/display_timing.c
display_timings.c?
+int videomode_from_timing(struct display_timings *disp, struct videomode *vm,
+			  unsigned int index)
I find the indexing API a bit confusing. But that's perhaps just a
matter of personal preference.

Also the ordering of arguments seems a little off. I find it more
natural to have the destination pointer in the first argument, similar
to the memcpy() function, so this would be:

int videomode_from_timing(struct videomode *vm, struct display_timings *disp,
			  unsigned int index);

Actually, when reading videomode_from_timing() I'd expect the argument
list to be:

int videomode_from_timing(struct videomode *vm, struct display_timing *timing);

Am I the only one confused by this?
quoted hunk
diff --git a/include/linux/display_timing.h b/include/linux/display_timing.h
display_timings.h?
+/* placeholder function until ranges are really needed 
The above line has trailing whitespace. Also the block comment should
have the opening /* on a separate line.
+ * the index parameter should then be used to select one of [min typ max]
If index is supposed to select min, typ or max, then maybe an enum would
be a better candidate? Or alternatively provide separate accessors, like
display_timing_get_{minimum,typical,maximum}().
+ */
+static inline u32 display_timing_get_value(struct timing_entry *te,
+					   unsigned int index)
+{
+	return te->typ;
+}
+
+static inline struct display_timing *display_timings_get(struct display_timings *disp,
+							 unsigned int index)
+{
+	if (disp->num_timings > index)
+		return disp->timings[index];
+	else
+		return NULL;
+}
+
+void timings_release(struct display_timings *disp);
This function no longer exists.

Thierry

Re: [PATCH v8 2/6] video: add of helper for videomode

From: Steffen Trumtrar <hidden>
Date: 2012-11-13 10:53:27

On Mon, Nov 12, 2012 at 11:00:37PM +0400, Alexey Klimov wrote:
Hello Steffen,

On Mon, Nov 12, 2012 at 7:37 PM, Steffen Trumtrar
[off-list ref] wrote:
quoted
This adds support for reading display timings from DT or/and convert one of those
timings to a videomode.
The of_display_timing implementation supports multiple children where each
property can have up to 3 values. All children are read into an array, that
can be queried.
of_get_videomode converts exactly one of that timings to a struct videomode.

Signed-off-by: Steffen Trumtrar <redacted>
Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 .../devicetree/bindings/video/display-timings.txt  |  107 +++++++++++
 drivers/video/Kconfig                              |   13 ++
 drivers/video/Makefile                             |    2 +
 drivers/video/of_display_timing.c                  |  186 ++++++++++++++++++++
 drivers/video/of_videomode.c                       |   47 +++++
 include/linux/of_display_timings.h                 |   19 ++
 include/linux/of_videomode.h                       |   15 ++
 7 files changed, 389 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/video/display-timings.txt
 create mode 100644 drivers/video/of_display_timing.c
 create mode 100644 drivers/video/of_videomode.c
 create mode 100644 include/linux/of_display_timings.h
 create mode 100644 include/linux/of_videomode.h
diff --git a/Documentation/devicetree/bindings/video/display-timings.txt b/Documentation/devicetree/bindings/video/display-timings.txt
new file mode 100644
index 0000000..e22e2fd
--- /dev/null
+++ b/Documentation/devicetree/bindings/video/display-timings.txt
@@ -0,0 +1,107 @@
+display-timings bindings
+============
+
+display-timings-node
+--------------------
+
+required properties:
+ - none
+
+optional properties:
+ - native-mode: the native mode for the display, in case multiple modes are
+               provided. When omitted, assume the first node is the native.
+
+timings-subnode
+---------------
+
+required properties:
+ - 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-frequency: displayclock in Hz
+
+optional properties:
+ - hsync-active : Hsync pulse is active low/high/ignored
+ - vsync-active : Vsync pulse is active low/high/ignored
+ - de-active : Data-Enable pulse is active low/high/ignored
+ - pixelclk-inverted : pixelclock is inverted/non-inverted/ignored
+ - interlaced (bool)
+ - doublescan (bool)
+
+All the optional properties that are not bool follow the following logic:
+    <1> : high active
+    <0> : low active
+    omitted : not used on hardware
+
+There are different ways of describing the capabilities of a display. The devicetree
+representation corresponds to the one commonly found in datasheets for displays.
+If a display supports multiple signal timings, the native-mode can be specified.
+
+The parameters are defined as
+
+struct display_timing
+==========> > +
+  +----------+---------------------------------------------+----------+-------+
+  |          |                ↑                            |          |       |
+  |          |                |vback_porch                 |          |       |
+  |          |                ↓                            |          |       |
+  +----------###############################################----------+-------+
+  |          #                ↑                            #          |       |
+  |          #                |                            #          |       |
+  |  hback   #                |                            #  hfront  | hsync |
+  |   porch  #                |       hactive              #  porch   |  len  |
+  |<-------->#<---------------+--------------------------->#<-------->|<----->|
+  |          #                |                            #          |       |
+  |          #                |vactive                     #          |       |
+  |          #                |                            #          |       |
+  |          #                ↓                            #          |       |
+  +----------###############################################----------+-------+
+  |          |                ↑                            |          |       |
+  |          |                |vfront_porch                |          |       |
+  |          |                ↓                            |          |       |
+  +----------+---------------------------------------------+----------+-------+
+  |          |                ↑                            |          |       |
+  |          |                |vsync_len                   |          |       |
+  |          |                ↓                            |          |       |
+  +----------+---------------------------------------------+----------+-------+
+
+
+Example:
+
+       display-timings {
+               native-mode = <&timing0>;
+               timing0: 1920p24 {
+                       /* 1920x1080p24 */
+                       clock-frequency = <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 = <1>;
+               };
+       };
+
+Every required property also supports the use of ranges, so the commonly used
+datasheet description with <min typ max>-tuples can be used.
+
+Example:
+
+       timing1: timing {
+               /* 1920x1080p24 */
+               clock-frequency = <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>;
+       };
diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
index 2a23b18..a324457 100644
--- a/drivers/video/Kconfig
+++ b/drivers/video/Kconfig
@@ -39,6 +39,19 @@ config DISPLAY_TIMING
 config VIDEOMODE
        bool

+config OF_DISPLAY_TIMING
+       def_bool y
+       select DISPLAY_TIMING
+       help
+         helper to parse display timings from the devicetree
+
+config OF_VIDEOMODE
+       def_bool y
+       select VIDEOMODE
+       select OF_DISPLAY_TIMING
+       help
+         helper to get videomodes from the devicetree
+
 menuconfig FB
        tristate "Support for frame buffer devices"
        ---help---
diff --git a/drivers/video/Makefile b/drivers/video/Makefile
index fc30439..b936b00 100644
--- a/drivers/video/Makefile
+++ b/drivers/video/Makefile
@@ -168,4 +168,6 @@ obj-$(CONFIG_FB_VIRTUAL)          += vfb.o
 #video output switch sysfs driver
 obj-$(CONFIG_VIDEO_OUTPUT_CONTROL) += output.o
 obj-$(CONFIG_DISPLAY_TIMING) += display_timing.o
+obj-$(CONFIG_OF_DISPLAY_TIMING) += of_display_timing.o
 obj-$(CONFIG_VIDEOMODE) += videomode.o
+obj-$(CONFIG_OF_VIDEOMODE) += of_videomode.o
diff --git a/drivers/video/of_display_timing.c b/drivers/video/of_display_timing.c
new file mode 100644
index 0000000..0bd30cc
--- /dev/null
+++ b/drivers/video/of_display_timing.c
@@ -0,0 +1,186 @@
+/*
+ * 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/export.h>
+#include <linux/of_display_timings.h>
+
+/**
+ * parse_property - parse timing_entry from device_node
+ * @np: device_node with the property
+ * @name: name of the property
+ * @result: will be set to the return value
+ *
+ * DESCRIPTION:
+ * Every display_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 parse_property(struct device_node *np, char *name,
+                               struct timing_entry *result)
+{
+       struct property *prop;
+       int length, cells, 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(np, name, &result->typ);
+               result->min = result->typ;
+               result->max = result->typ;
+       } 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;
+}
+
+/**
+ * of_get_display_timing - parse display_timing entry from device_node
+ * @np: device_node with the properties
+ **/
+static struct display_timing *of_get_display_timing(struct device_node *np)
+{
+       struct display_timing *dt;
+       int ret = 0;
+
+       dt = kzalloc(sizeof(*dt), GFP_KERNEL);
+       if (!dt) {
+               pr_err("%s: could not allocate display_timing struct\n", __func__);
+               return NULL;
+       }
+
+       ret |= parse_property(np, "hback-porch", &dt->hback_porch);
+       ret |= parse_property(np, "hfront-porch", &dt->hfront_porch);
+       ret |= parse_property(np, "hactive", &dt->hactive);
+       ret |= parse_property(np, "hsync-len", &dt->hsync_len);
+       ret |= parse_property(np, "vback-porch", &dt->vback_porch);
+       ret |= parse_property(np, "vfront-porch", &dt->vfront_porch);
+       ret |= parse_property(np, "vactive", &dt->vactive);
+       ret |= parse_property(np, "vsync-len", &dt->vsync_len);
+       ret |= parse_property(np, "clock-frequency", &dt->pixelclock);
+
+       of_property_read_u32(np, "vsync-active", &dt->vsync_pol_active);
+       of_property_read_u32(np, "hsync-active", &dt->hsync_pol_active);
+       of_property_read_u32(np, "de-active", &dt->de_pol_active);
+       of_property_read_u32(np, "pixelclk-inverted", &dt->pixelclk_pol);
+       dt->interlaced = of_property_read_bool(np, "interlaced");
+       dt->doublescan = of_property_read_bool(np, "doublescan");
+
+       if (ret) {
+               pr_err("%s: error reading timing properties\n", __func__);
+               return NULL;
+       }
+
+       return dt;
+}
+
+/**
+ * of_get_display_timings - parse all display_timing entries from a device_node
+ * @np: device_node with the subnodes
+ **/
+struct display_timings *of_get_display_timings(struct device_node *np)
+{
+       struct device_node *timings_np;
+       struct device_node *entry;
+       struct device_node *native_mode;
+       struct display_timings *disp;
+
+       if (!np) {
+               pr_err("%s: no devicenode given\n", __func__);
+               return NULL;
+       }
+
+       timings_np = of_find_node_by_name(np, "display-timings");
+       if (!timings_np) {
+               pr_err("%s: could not find display-timings node\n", __func__);
+               return NULL;
+       }
+
+       disp = kzalloc(sizeof(*disp), GFP_KERNEL);
+       if (!disp)
+               return -ENOMEM;
+
+       entry = of_parse_phandle(timings_np, "native-mode", 0);
+       /* assume first child as native mode if none provided */
+       if (!entry)
+               entry = of_get_next_child(np, NULL);
+       if (!entry) {
+               pr_err("%s: no timing specifications given\n", __func__);
+               return NULL;
+       }
+
+       pr_info("%s: using %s as default timing\n", __func__, entry->name);
+
+       native_mode = entry;
+
+       disp->num_timings = of_get_child_count(timings_np);
+       disp->timings = kzalloc(sizeof(struct display_timing *)*disp->num_timings,
+                               GFP_KERNEL);
+       if (!disp->timings)
+               return -ENOMEM;
Could you please check return values here ^^^ and above "disp > kzalloc(sizeof(*disp), GFP_KERNEL);" ?
May be it's better to return NULL instead of -ENOMEM and put error message?
I will do that along with the memory leak fixes.

Regards,
Steffen

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

Re: [PATCH v8 2/6] video: add of helper for videomode

From: Thierry Reding <hidden>
Date: 2012-11-13 11:08:37

On Mon, Nov 12, 2012 at 04:37:02PM +0100, Steffen Trumtrar wrote:
quoted hunk
This adds support for reading display timings from DT or/and convert one of those
timings to a videomode.
The of_display_timing implementation supports multiple children where each
property can have up to 3 values. All children are read into an array, that
can be queried.
of_get_videomode converts exactly one of that timings to a struct videomode.

Signed-off-by: Steffen Trumtrar <redacted>
Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 .../devicetree/bindings/video/display-timings.txt  |  107 +++++++++++
 drivers/video/Kconfig                              |   13 ++
 drivers/video/Makefile                             |    2 +
 drivers/video/of_display_timing.c                  |  186 ++++++++++++++++++++
 drivers/video/of_videomode.c                       |   47 +++++
 include/linux/of_display_timings.h                 |   19 ++
 include/linux/of_videomode.h                       |   15 ++
 7 files changed, 389 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/video/display-timings.txt
 create mode 100644 drivers/video/of_display_timing.c
 create mode 100644 drivers/video/of_videomode.c
 create mode 100644 include/linux/of_display_timings.h
 create mode 100644 include/linux/of_videomode.h
diff --git a/Documentation/devicetree/bindings/video/display-timings.txt b/Documentation/devicetree/bindings/video/display-timings.txt
new file mode 100644
index 0000000..e22e2fd
--- /dev/null
+++ b/Documentation/devicetree/bindings/video/display-timings.txt
@@ -0,0 +1,107 @@
+display-timings bindings
+========================
+
+display-timings-node
+--------------------
Maybe leave away the last -, so that it reads "display-timings node"? I
think that makes it more obvious that the node is supposed to be called
"display-timings".
+
+required properties:
+ - none
+
+optional properties:
+ - native-mode: the native mode for the display, in case multiple modes are
+		provided. When omitted, assume the first node is the native.
+
+timings-subnode
+---------------
Same here: "timing subnodes"?
+
+required properties:
+ - hactive, vactive: Display resolution
+ - hfront-porch, hback-porch, hsync-len: Horizontal Display timing parameters
"display"?
+   in pixels
+   vfront-porch, vback-porch, vsync-len: Vertical display timing parameters in
+   lines
+ - clock-frequency: displayclock in Hz
"display clock"?
+
+optional properties:
+ - hsync-active : Hsync pulse is active low/high/ignored
+ - vsync-active : Vsync pulse is active low/high/ignored
+ - de-active : Data-Enable pulse is active low/high/ignored
+ - pixelclk-inverted : pixelclock is inverted/non-inverted/ignored
+ - interlaced (bool)
+ - doublescan (bool)
+
+All the optional properties that are not bool follow the following logic:
+    <1> : high active
+    <0> : low active
+    omitted : not used on hardware
Nitpick: You use space before : in the optional properties, but not in
the required properties above.
+
+There are different ways of describing the capabilities of a display. The devicetree
+representation corresponds to the one commonly found in datasheets for displays.
+If a display supports multiple signal timings, the native-mode can be specified.
+
+The parameters are defined as
+
+struct display_timing
+=====================
struct display_timing has no meaning in device tree documentation. Maybe
this line can just go away?
quoted hunk
diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
index 2a23b18..a324457 100644
--- a/drivers/video/Kconfig
+++ b/drivers/video/Kconfig
@@ -39,6 +39,19 @@ config DISPLAY_TIMING
 config VIDEOMODE
        bool
 
+config OF_DISPLAY_TIMING
OF_DISPLAY_TIMINGS?
quoted hunk
diff --git a/drivers/video/of_display_timing.c b/drivers/video/of_display_timing.c
of_display_timings.c?
+/**
+ * of_get_display_timings - parse all display_timing entries from a device_node
+ * @np: device_node with the subnodes
+ **/
+struct display_timings *of_get_display_timings(struct device_node *np)
[...]
+	for_each_child_of_node(timings_np, entry) {
+		struct display_timing *dt;
+
+		dt = of_get_display_timing(entry);
+		if (!dt) {
+			/* to not encourage wrong devicetrees, fail in case of an error */
+			pr_err("%s: error in timing %d\n", __func__, disp->num_timings+1);
+			return NULL;
+		}
In case of a parsing error, of_get_display_timing() already shows an
error message, so I don't think we need another one here.
+/**
+ * of_display_timings_exists - check if a display-timings node is provided
+ * @np: device_node with the timing
+ **/
+int of_display_timings_exists(struct device_node *np)
+{
+	struct device_node *timings_np;
+
+	if (!np)
+		return -EINVAL;
+
+	timings_np = of_parse_phandle(np, "display-timings", 0);
+	if (!timings_np)
+		return -EINVAL;
+
+	return 1;
I think this is missing of_node_put(timings_np) in both failure and
success cases. Also, maybe this should really return a bool instead?

Also, why do you use of_parse_phandle() for this? Aren't the
display-timings nodes expected to be children of some other node like an
output/display device?
quoted hunk
diff --git a/include/linux/of_videomode.h b/include/linux/of_videomode.h
[...]
+
+int of_get_videomode(struct device_node *np, struct videomode *vm, int index);
+#endif /* __LINUX_OF_VIDEOMODE_H */
There should be a blank line between the last two lines I think.

Thierry

Re: [PATCH v8 3/6] fbmon: add videomode helpers

From: Thierry Reding <hidden>
Date: 2012-11-13 11:22:58

On Mon, Nov 12, 2012 at 04:37:03PM +0100, Steffen Trumtrar wrote:
[...]
+#if IS_ENABLED(CONFIG_VIDEOMODE)
+int videomode_to_fb_videomode(struct videomode *vm, struct fb_videomode *fbmode)
The other helpers are named <destination-type>_from_<source-type>(),
maybe this should follow that example for consistency?
+{
+	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);
+
+	fbmode->sync = 0;
+	fbmode->vmode = 0;
+	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;
+	if (vm->de)
+		fbmode->sync |= FB_SYNC_DATA_ENABLE_HIGH_ACT;
+	fbmode->refresh = 60;
Can the refresh rate not be computed from the pixel clock and the
horizontal and vertical timings?
+	fbmode->flag = 0;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(videomode_to_fb_videomode);
+#endif
+
+
There's a gratuitous blank line here.
quoted hunk
 #else
 int fb_parse_edid(unsigned char *edid, struct fb_var_screeninfo *var)
 {
diff --git a/include/linux/fb.h b/include/linux/fb.h
index c7a9571..46c665b 100644
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -714,6 +714,8 @@ extern void fb_destroy_modedb(struct fb_videomode *modedb);
 extern int fb_find_mode_cvt(struct fb_videomode *mode, int margins, int rb);
 extern unsigned char *fb_ddc_read(struct i2c_adapter *adapter);
 
+extern int videomode_to_fb_videomode(struct videomode *vm, struct fb_videomode *fbmode);
+
Should you provide a dummy in the !CONFIG_VIDEOMODE case?

Thierry

Re: [PATCH v8 4/6] fbmon: add of_videomode helpers

From: Thierry Reding <hidden>
Date: 2012-11-13 11:28:06

On Mon, Nov 12, 2012 at 04:37:04PM +0100, Steffen Trumtrar wrote:
[...]
quoted hunk
diff --git a/include/linux/fb.h b/include/linux/fb.h
[...]
+extern int of_get_fb_videomode(struct device_node *np, struct fb_videomode *fb, int index);
Similarily this should get a dummy for the !CONFIG_OF_VIDEOMODE case,
right? Either that or the prototype should be protected by
CONFIG_OF_VIDEOMODE as well.

Thierry

Re: [PATCH v8 5/6] drm_modes: add videomode helpers

From: Thierry Reding <hidden>
Date: 2012-11-13 11:31:14

On Mon, Nov 12, 2012 at 04:37:05PM +0100, Steffen Trumtrar wrote:
[...]
quoted hunk
diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
[...]
+#if IS_ENABLED(CONFIG_VIDEOMODE)
+int videomode_to_display_mode(struct videomode *vm, struct drm_display_mode *dmode)
This one as well may be more consistently named
drm_display_mode_from_videomode().
quoted hunk
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
[...]
+extern int videomode_to_display_mode(struct videomode *vm,
+				     struct drm_display_mode *dmode);
And this also needs protection or a dummy.

Thierry

Re: [PATCH v8 6/6] drm_modes: add of_videomode helpers

From: Thierry Reding <hidden>
Date: 2012-11-13 11:35:18

On Mon, Nov 12, 2012 at 04:37:06PM +0100, Steffen Trumtrar wrote:
[...]
+#if IS_ENABLED(CONFIG_OF_VIDEOMODE)
+static void dump_drm_displaymode(struct drm_display_mode *m)
+{
+	pr_debug("drm_displaymode = %d %d %d %d %d %d %d %d %d\n",
+		 m->hdisplay, m->hsync_start, m->hsync_end, m->htotal,
+		 m->vdisplay, m->vsync_start, m->vsync_end, m->vtotal,
+		 m->clock);
I seem to remember a comment to an earlier version of this patch
requesting better formatting of this string. Alternatively you might
want to consider replacing it using drm_mode_debug_printmodeline().
quoted hunk
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
[...]
quoted hunk
@@ -1457,6 +1458,10 @@ drm_mode_create_from_cmdline_mode(struct drm_device *dev,
 
 extern int videomode_to_display_mode(struct videomode *vm,
 				     struct drm_display_mode *dmode);
+extern int of_get_drm_display_mode(struct device_node *np,
+				   struct drm_display_mode *dmode,
+				   int index);
Also requires either a dummy or protection.

Thierry

Re: [PATCH v8 2/6] video: add of helper for videomode

From: Steffen Trumtrar <hidden>
Date: 2012-11-13 12:59:27

On Mon, Nov 12, 2012 at 01:40:12PM -0700, Stephen Warren wrote:
On 11/12/2012 08:37 AM, Steffen Trumtrar wrote:
quoted
This adds support for reading display timings from DT or/and convert one of those
timings to a videomode.
The of_display_timing implementation supports multiple children where each
property can have up to 3 values. All children are read into an array, that
can be queried.
of_get_videomode converts exactly one of that timings to a struct videomode.
quoted
diff --git a/Documentation/devicetree/bindings/video/display-timings.txt b/Documentation/devicetree/bindings/video/display-timings.txt
The device tree bindings look reasonable to me, so,

Acked-by: Stephen Warren <redacted>
Thanks,

Steffen

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

Re: [PATCH v8 3/6] fbmon: add videomode helpers

From: Steffen Trumtrar <hidden>
Date: 2012-11-13 13:28:46

On Tue, Nov 13, 2012 at 12:22:58PM +0100, Thierry Reding wrote:
On Mon, Nov 12, 2012 at 04:37:03PM +0100, Steffen Trumtrar wrote:
[...]
quoted
+#if IS_ENABLED(CONFIG_VIDEOMODE)
+int videomode_to_fb_videomode(struct videomode *vm, struct fb_videomode *fbmode)
The other helpers are named <destination-type>_from_<source-type>(),
maybe this should follow that example for consistency?
Hm, not a bad idea.
quoted
+{
+	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);
+
+	fbmode->sync = 0;
+	fbmode->vmode = 0;
+	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;
+	if (vm->de)
+		fbmode->sync |= FB_SYNC_DATA_ENABLE_HIGH_ACT;
+	fbmode->refresh = 60;
Can the refresh rate not be computed from the pixel clock and the
horizontal and vertical timings?
Yes. That totally got lost on the way.
quoted
+	fbmode->flag = 0;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(videomode_to_fb_videomode);
+#endif
+
+
There's a gratuitous blank line here.
quoted
 #else
 int fb_parse_edid(unsigned char *edid, struct fb_var_screeninfo *var)
 {
diff --git a/include/linux/fb.h b/include/linux/fb.h
index c7a9571..46c665b 100644
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -714,6 +714,8 @@ extern void fb_destroy_modedb(struct fb_videomode *modedb);
 extern int fb_find_mode_cvt(struct fb_videomode *mode, int margins, int rb);
 extern unsigned char *fb_ddc_read(struct i2c_adapter *adapter);
 
+extern int videomode_to_fb_videomode(struct videomode *vm, struct fb_videomode *fbmode);
+
Should you provide a dummy in the !CONFIG_VIDEOMODE case?
Okay

Steffen


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

Re: [PATCH v8 6/6] drm_modes: add of_videomode helpers

From: Steffen Trumtrar <hidden>
Date: 2012-11-13 13:30:14

On Tue, Nov 13, 2012 at 12:35:18PM +0100, Thierry Reding wrote:
On Mon, Nov 12, 2012 at 04:37:06PM +0100, Steffen Trumtrar wrote:
[...]
quoted
+#if IS_ENABLED(CONFIG_OF_VIDEOMODE)
+static void dump_drm_displaymode(struct drm_display_mode *m)
+{
+	pr_debug("drm_displaymode = %d %d %d %d %d %d %d %d %d\n",
+		 m->hdisplay, m->hsync_start, m->hsync_end, m->htotal,
+		 m->vdisplay, m->vsync_start, m->vsync_end, m->vtotal,
+		 m->clock);
I seem to remember a comment to an earlier version of this patch
requesting better formatting of this string. Alternatively you might
want to consider replacing it using drm_mode_debug_printmodeline().
Ah, yes. I only did that for fb_videomode and forgot about this one.
But the existing function is even better.
quoted
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
[...]
quoted
@@ -1457,6 +1458,10 @@ drm_mode_create_from_cmdline_mode(struct drm_device *dev,
 
 extern int videomode_to_display_mode(struct videomode *vm,
 				     struct drm_display_mode *dmode);
+extern int of_get_drm_display_mode(struct device_node *np,
+				   struct drm_display_mode *dmode,
+				   int index);
Also requires either a dummy or protection.

Thierry


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

Re: [PATCH v8 2/6] video: add of helper for videomode

From: Stephen Warren <hidden>
Date: 2012-11-13 17:46:53

On 11/13/2012 04:08 AM, Thierry Reding wrote:
On Mon, Nov 12, 2012 at 04:37:02PM +0100, Steffen Trumtrar wrote:
quoted
This adds support for reading display timings from DT or/and
convert one of those timings to a videomode. The
of_display_timing implementation supports multiple children where
each property can have up to 3 values. All children are read into
an array, that can be queried. of_get_videomode converts exactly
one of that timings to a struct videomode.
quoted
diff --git
a/Documentation/devicetree/bindings/video/display-timings.txt
b/Documentation/devicetree/bindings/video/display-timings.txt
quoted
+ - clock-frequency: displayclock in Hz
"display clock"?
I /think/ I had suggested naming this clock-frequency before so that
the property name would be more standardized; other bindings use that
same name. But I'm not too attached to the name I guess.

Re: [PATCH v8 1/6] video: add display_timing and videomode

From: Thierry Reding <hidden>
Date: 2012-11-14 10:56:34

On Mon, Nov 12, 2012 at 04:37:01PM +0100, Steffen Trumtrar wrote:
[...]
quoted hunk
diff --git a/drivers/video/display_timing.c b/drivers/video/display_timing.c
[...]
+void display_timings_release(struct display_timings *disp)
+{
+	if (disp->timings) {
+		unsigned int i;
+
+		for (i = 0; i < disp->num_timings; i++)
+			kfree(disp->timings[i]);
+		kfree(disp->timings);
+	}
+	kfree(disp);
+}
I think this is still missing an EXPORT_SYMBOL_GPL. Otherwise it can't
be used from modules.

Thierry

Re: [PATCH v8 2/6] video: add of helper for videomode

From: Thierry Reding <hidden>
Date: 2012-11-14 10:59:40

On Mon, Nov 12, 2012 at 04:37:02PM +0100, Steffen Trumtrar wrote:
[...]
quoted hunk
diff --git a/include/linux/of_display_timings.h b/include/linux/of_display_timings.h
[...]
+#ifndef __LINUX_OF_DISPLAY_TIMINGS_H
+#define __LINUX_OF_DISPLAY_TIMINGS_H
+
+#include <linux/display_timing.h>
+
+#define OF_USE_NATIVE_MODE -1
+
+struct display_timings *of_get_display_timings(struct device_node *np);
+int of_display_timings_exists(struct device_node *np);
This either needs to include linux/of.h or a forward declaration of
struct device_node. Otherwise this will fail to compile if the file
where this is included from doesn't pull linux/of.h in explicitly.
quoted hunk
diff --git a/include/linux/of_videomode.h b/include/linux/of_videomode.h
[...]
+#ifndef __LINUX_OF_VIDEOMODE_H
+#define __LINUX_OF_VIDEOMODE_H
+
+#include <linux/videomode.h>
+
+int of_get_videomode(struct device_node *np, struct videomode *vm, int index);
Same here.

Thierry
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help