[PATCH 0/2 v6] of: add display helper

[PATCH 0/2 v6] of: add display helper

[Steffen Trumtrar]

Hi!

In accordance with Stepehn Warren, I downsized the binding.
Now, just the display-timing is described, as I think, it is way easier to agree
on those and have a complete binding.

Regards,
Steffen

Steffen Trumtrar (2):
  of: add helper to parse display timings
  of: add generic videomode description

 .../devicetree/bindings/video/display-timings.txt  |  222 ++++++++++++++++++++
 drivers/of/Kconfig                                 |   10 +
 drivers/of/Makefile                                |    2 +
 drivers/of/of_display_timings.c                    |  183 ++++++++++++++++
 drivers/of/of_videomode.c                          |  212 +++++++++++++++++++
 include/linux/of_display_timings.h                 |   85 ++++++++
 include/linux/of_videomode.h                       |   41 ++++
 7 files changed, 755 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/video/display-timings.txt
 create mode 100644 drivers/of/of_display_timings.c
 create mode 100644 drivers/of/of_videomode.c
 create mode 100644 include/linux/of_display_timings.h
 create mode 100644 include/linux/of_videomode.h

-- 
1.7.10.4

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

[Steffen Trumtrar]

Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
---
 .../devicetree/bindings/video/display-timings.txt  |  222 ++++++++++++++++++++
 drivers/of/Kconfig                                 |    5 +
 drivers/of/Makefile                                |    1 +
 drivers/of/of_display_timings.c                    |  183 ++++++++++++++++
 include/linux/of_display_timings.h                 |   85 ++++++++
 5 files changed, 496 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/video/display-timings.txt
 create mode 100644 drivers/of/of_display_timings.c
 create mode 100644 include/linux/of_display_timings.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..45e39bd
--- /dev/null
+++ b/Documentation/devicetree/bindings/video/display-timings.txt
@@ -0,0 +1,222 @@
+display-timings bindings
+=========
+
+display-timings-node
+------------
+
+required properties:
+ - none
+
+optional properties:
+ - default-timing: the default timing value
+
+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: displayclock in Hz
+
+optional properties:
+ - 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
+ - interlaced (bool)
+ - doublescan (bool)
+
+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 default-timing can be specified.
+
+The parameters are defined as
+
+struct signal_timing
+=========+
+  +----------+---------------------------------------------+----------+-------+
+  |          |                ↑                            |          |       |
+  |          |                |vback_porch                 |          |       |
+  |          |                ↓                            |          |       |
+  +----------###############################################----------+-------+
+  |          #                ↑                            #          |       |
+  |          #                |                            #          |       |
+  |  hback   #                |                            #  hfront  | hsync |
+  |   porch  #                |       hactive              #  porch   |  len  |
+  |<-------->#<---------------+--------------------------->#<-------->|<----->|
+  |          #                |                            #          |       |
+  |          #                |vactive                     #          |       |
+  |          #                |                            #          |       |
+  |          #                ↓                            #          |       |
+  +----------###############################################----------+-------+
+  |          |                ↑                            |          |       |
+  |          |                |vfront_porch                |          |       |
+  |          |                ↓                            |          |       |
+  +----------+---------------------------------------------+----------+-------+
+  |          |                ↑                            |          |       |
+  |          |                |vsync_len                   |          |       |
+  |          |                ↓                            |          |       |
+  +----------+---------------------------------------------+----------+-------+
+
+
+Example:
+
+	display-timings {
+		default-timing = <&timing0>;
+		timing0: 1920p24 {
+			/* 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 {
+		/* 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>;
+	};
+
+Usage in backend
+========
+
+A backend driver may choose to use the display-timings 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_timing_list
+					 ↓
+			      struct display_timings
+					 |
+					 |  videomode_from_timing
+					 ↓
+			    ---  struct videomode ---
+			    |			    |
+ videomode_to_displaymode   |			    |   videomode_to_fb_videomode
+		            ↓			    ↓
+		     drm_display_mode         fb_videomode
+
+The functions of_get_fb_videomode and of_get_display_mode are provided
+to conveniently get the respective mode representation from the devicetree.
+
+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..646deb0 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_TIMINGS
+	def_bool y
+	help
+	  helper to parse display timings from the devicetree
+
 endmenu # OF
diff --git a/drivers/of/Makefile b/drivers/of/Makefile
index e027f44..c8e9603 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_TIMINGS) += of_display_timings.o
diff --git a/drivers/of/of_display_timings.c b/drivers/of/of_display_timings.c
new file mode 100644
index 0000000..e47bc63
--- /dev/null
+++ b/drivers/of/of_display_timings.c
@@ -0,0 +1,183 @@
+/*
+ * 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>
+
+/* 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 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 signal_timing *of_get_display_timing(struct device_node *np)
+{
+	struct signal_timing *st;
+	int ret = 0;
+
+	st = kzalloc(sizeof(*st), GFP_KERNEL);
+
+	if (!st) {
+		pr_err("%s: could not allocate signal_timing struct\n", __func__);
+		return NULL;
+	}
+
+	ret |= parse_property(np, "hback-porch", &st->hback_porch);
+	ret |= parse_property(np, "hfront-porch", &st->hfront_porch);
+	ret |= parse_property(np, "hactive", &st->hactive);
+	ret |= parse_property(np, "hsync-len", &st->hsync_len);
+	ret |= parse_property(np, "vback-porch", &st->vback_porch);
+	ret |= parse_property(np, "vfront-porch", &st->vfront_porch);
+	ret |= parse_property(np, "vactive", &st->vactive);
+	ret |= parse_property(np, "vsync-len", &st->vsync_len);
+	ret |= parse_property(np, "clock", &st->pixelclock);
+
+	st->vsync_pol_active_high = of_property_read_bool(np, "vsync-active-high");
+	st->hsync_pol_active_high = of_property_read_bool(np, "hsync-active-high");
+	st->de_pol_active_high = of_property_read_bool(np, "de-active-high");
+	st->pixelclk_pol_inverted = of_property_read_bool(np, "pixelclk-inverted");
+	st->interlaced = of_property_read_bool(np, "interlaced");
+	st->doublescan = of_property_read_bool(np, "doublescan");
+
+	if (ret) {
+		pr_err("%s: error reading timing properties\n", __func__);
+		return NULL;
+	}
+
+	return st;
+}
+EXPORT_SYMBOL_GPL(of_get_display_timing);
+
+struct display_timings *of_get_display_timing_list(struct device_node *np)
+{
+	struct device_node *timings_np;
+	struct device_node *entry;
+	struct display_timings *disp;
+	char *default_timing;
+
+	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);
+
+	entry = of_parse_phandle(timings_np, "default-timing", 0);
+
+	if (!entry) {
+		pr_info("%s: no default-timing specified\n", __func__);
+		entry = of_find_node_by_name(np, "timing");
+	}
+
+	if (!entry) {
+		pr_info("%s: no timing specifications given\n", __func__);
+		return disp;
+	}
+
+	pr_info("%s: using %s as default timing\n", __func__, entry->name);
+
+	default_timing = (char *)entry->full_name;
+
+	disp->num_timings = 0;
+
+	for_each_child_of_node(timings_np, entry) {
+		disp->num_timings++;
+	}
+
+	disp->timings = kzalloc(sizeof(struct signal_timing *)*disp->num_timings,
+				GFP_KERNEL);
+
+	disp->num_timings = 0;
+
+	for_each_child_of_node(timings_np, entry) {
+		struct signal_timing *st;
+
+		st = of_get_display_timing(entry);
+
+		if (!st)
+			continue;
+
+		if (strcmp(default_timing, entry->full_name) = 0)
+			disp->default_timing = disp->num_timings;
+
+		disp->timings[disp->num_timings] = st;
+		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->default_timing + 1);
+	else
+		pr_info("%s: no timings specified\n", __func__);
+
+	return disp;
+}
+EXPORT_SYMBOL_GPL(of_get_display_timing_list);
+
+int of_display_timings_exists(struct device_node *np)
+{
+	struct device_node *timings_np;
+	struct device_node *default_np;
+
+	if (!np)
+		return -EINVAL;
+
+	timings_np = of_parse_phandle(np, "display-timings", 0);
+
+	if (!timings_np)
+		return -EINVAL;
+
+	default_np = of_parse_phandle(np, "default-timing", 0);
+
+	if (default_np)
+		return 0;
+
+	return -EINVAL;
+}
+EXPORT_SYMBOL_GPL(of_display_timings_exists);
diff --git a/include/linux/of_display_timings.h b/include/linux/of_display_timings.h
new file mode 100644
index 0000000..1ad719e
--- /dev/null
+++ b/include/linux/of_display_timings.h
@@ -0,0 +1,85 @@
+/*
+ * Copyright 2012 Steffen Trumtrar <s.trumtrar@pengutronix.de>
+ *
+ * description of display timings
+ *
+ * This file is released under the GPLv2
+ */
+
+#ifndef __LINUX_OF_DISPLAY_TIMINGS_H
+#define __LINUX_OF_DISPLAY_TIMINGS_H
+
+#define OF_DEFAULT_TIMING -1
+
+struct display_timings {
+	unsigned int num_timings;
+	unsigned int default_timing;
+
+	struct signal_timing **timings;
+};
+
+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;
+
+	bool vsync_pol_active_high;
+	bool hsync_pol_active_high;
+	bool de_pol_active_high;
+	bool pixelclk_pol_inverted;
+	bool interlaced;
+	bool doublescan;
+};
+
+struct display_timings *of_get_display_timing_list(struct device_node *np);
+struct signal_timing *of_get_display_timing(struct device_node *np);
+int of_display_timings_exists(struct device_node *np);
+
+/* 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_timings *disp)
+{
+	int i;
+
+	for (i = 0; i < disp->num_timings; i++)
+		kfree(disp->timings[i]);
+}
+
+static inline void display_timings_release(struct display_timings *disp)
+{
+	timings_release(disp);
+	kfree(disp->timings);
+}
+
+static inline struct signal_timing *display_timings_get(struct display_timings *disp,
+							 int index)
+{
+	struct signal_timing *st;
+
+	if (disp->num_timings > index) {
+		st = disp->timings[index];
+		return st;
+	}
+	else
+		return NULL;
+}
+
+#endif
-- 
1.7.10.4

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

[Stephen Warren]

On 10/04/2012 11:59 AM, Steffen Trumtrar wrote:

A patch description would be useful for something like this.

> diff --git a/Documentation/devicetree/bindings/video/display-timings.txt b/Documentation/devicetree/bindings/video/display-timings.txt
> new file mode 100644
...
> +Usage in backend
> +========

Everything before that point in the file looks fine to me.

Everything after this point in the file seems to be Linux-specific
implementation details. Does it really belong in the DT binding
documentation, rather than some Linux-specific documentation file?

> +struct drm_display_mode
> +===========> +
> +  +----------+---------------------------------------------+----------+-------+
> +  |          |                                             |          |       |  ↑
> +  |          |                                             |          |       |  |
> +  |          |                                             |          |       |  |
> +  +----------###############################################----------+-------+  |

I suspect the entire horizontal box above (and the entire vertical box
all the way down the left-hand side) should be on the bottom/right
instead of top/left. The reason I think this is because all of
vsync_start, vsync_end, vdisplay have to be referenced to some known
point, which is usually zero or the start of the timing definition, /or/
there would be some value indicating the size of the top marging/porch
in order to say where those other values are referenced to.

> +  |          #   ↑         ↑          ↑                    #          |       |  |
> +  |          #   |         |          |                    #          |       |  |
> +  |          #   |         |          |                    #          |       |  |
> +  |          #   |         |          |                    #          |       |  |
> +  |          #   |         |          |                    #          |       |  |
> +  |          #   |         |          |       hdisplay     #          |       |  |
> +  |          #<--+--------------------+------------------->#          |       |  |
> +  |          #   |         |          |                    #          |       |  |
> +  |          #   |vsync_start         |                    #          |       |  |
> +  |          #   |         |          |                    #          |       |  |
> +  |          #   |         |vsync_end |                    #          |       |  |
> +  |          #   |         |          |vdisplay            #          |       |  |
> +  |          #   |         |          |                    #          |       |  |
> +  |          #   |         |          |                    #          |       |  |
> +  |          #   |         |          |                    #          |       |  | vtotal
> +  |          #   |         |          |                    #          |       |  |
> +  |          #   |         |          |     hsync_start    #          |       |  |
> +  |          #<--+---------+----------+------------------------------>|       |  |
> +  |          #   |         |          |                    #          |       |  |
> +  |          #   |         |          |     hsync_end      #          |       |  |
> +  |          #<--+---------+----------+-------------------------------------->|  |
> +  |          #   |         |          ↓                    #          |       |  |
> +  +----------####|#########|################################----------+-------+  |
> +  |          |   |         |                               |          |       |  |
> +  |          |   |         |                               |          |       |  |
> +  |          |   ↓         |                               |          |       |  |
> +  +----------+-------------+-------------------------------+----------+-------+  |
> +  |          |             |                               |          |       |  |
> +  |          |             |                               |          |       |  |
> +  |          |             ↓                               |          |       |  ↓
> +  +----------+---------------------------------------------+----------+-------+
> +                                   htotal
> +   <------------------------------------------------------------------------->

> diff --git a/drivers/of/of_display_timings.c b/drivers/of/of_display_timings.c

> +static int parse_property(struct device_node *np, char *name,
> +				struct timing_entry *result)

> +	if (cells = 1)
> +		ret = of_property_read_u32_array(np, name, &result->typ, cells);

Should that branch not just set result->min/max to typ as well?
Presumably it'd prevent any code that interprets struct timing_entry
from having to check if those values were 0 or not?

> +	else if (cells = 3)
> +		ret = of_property_read_u32_array(np, name, &result->min, cells);

> +struct display_timings *of_get_display_timing_list(struct device_node *np)

> +	entry = of_parse_phandle(timings_np, "default-timing", 0);
> +
> +	if (!entry) {
> +		pr_info("%s: no default-timing specified\n", __func__);
> +		entry = of_find_node_by_name(np, "timing");

I don't think you want to require the node have an explicit name; I
don't recall the DT binding documentation making that a requirement.
Instead, can't you either just leave the default unset, or pick the
first DT child node, irrespective of name?

> +	if (!entry) {
> +		pr_info("%s: no timing specifications given\n", __func__);
> +		return disp;
> +	}

The DT bindings don't state that it's mandatory to have some timing
specified, although I agree that it makes sense in practice.

> +	for_each_child_of_node(timings_np, entry) {
> +		struct signal_timing *st;
> +
> +		st = of_get_display_timing(entry);
> +
> +		if (!st)
> +			continue;

I wonder if that shouldn't be an error?

> +		if (strcmp(default_timing, entry->full_name) = 0)
> +			disp->default_timing = disp->num_timings;

Hmm. Why not compare the node pointers rather than the name? Also, if
the parsing failed, then this can lead to default_timing being
uninitialized anyway...

> +		disp->timings[disp->num_timings] = st;
> +		disp->num_timings++;
> +	}

> +	if (disp->num_timings >= 0)
> +		pr_info("%s: got %d timings. Using timing #%d as default\n", __func__,
> +			disp->num_timings , disp->default_timing + 1);
> +	else
> +		pr_info("%s: no timings specified\n", __func__);

The message in the else clause is not necessarily true; there may have
been some specified, but they just couldn't be parsed.

> +int of_display_timings_exists(struct device_node *np)
> +{
> +	struct device_node *timings_np;
> +	struct device_node *default_np;
> +
> +	if (!np)
> +		return -EINVAL;
> +
> +	timings_np = of_parse_phandle(np, "display-timings", 0);
> +
> +	if (!timings_np)
> +		return -EINVAL;
> +
> +	default_np = of_parse_phandle(np, "default-timing", 0);
> +
> +	if (default_np)
> +		return 0;

If this function checks that a default-timing property exists, shouldn't
the function be named of_display_default_timing_exists()?

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

[Steffen Trumtrar]

On Thu, Oct 04, 2012 at 12:47:16PM -0600, Stephen Warren wrote:
> On 10/04/2012 11:59 AM, Steffen Trumtrar wrote:
> 
> A patch description would be useful for something like this.
> 

Yes. Shame on me.

> > diff --git a/Documentation/devicetree/bindings/video/display-timings.txt b/Documentation/devicetree/bindings/video/display-timings.txt
> > new file mode 100644
> ...
> > +Usage in backend
> > +========
> 
> Everything before that point in the file looks fine to me.
> 

\o/

> Everything after this point in the file seems to be Linux-specific
> implementation details. Does it really belong in the DT binding
> documentation, rather than some Linux-specific documentation file?
> 

I guess you are right about that.

> > +struct drm_display_mode
> > +===========> > +
> > +  +----------+---------------------------------------------+----------+-------+
> > +  |          |                                             |          |       |  ↑
> > +  |          |                                             |          |       |  |
> > +  |          |                                             |          |       |  |
> > +  +----------###############################################----------+-------+  |
> 
> I suspect the entire horizontal box above (and the entire vertical box
> all the way down the left-hand side) should be on the bottom/right
> instead of top/left. The reason I think this is because all of
> vsync_start, vsync_end, vdisplay have to be referenced to some known
> point, which is usually zero or the start of the timing definition, /or/
> there would be some value indicating the size of the top marging/porch
> in order to say where those other values are referenced to.
> 
> > +  |          #   ↑         ↑          ↑                    #          |       |  |
> > +  |          #   |         |          |                    #          |       |  |
> > +  |          #   |         |          |                    #          |       |  |
> > +  |          #   |         |          |                    #          |       |  |
> > +  |          #   |         |          |                    #          |       |  |
> > +  |          #   |         |          |       hdisplay     #          |       |  |
> > +  |          #<--+--------------------+------------------->#          |       |  |
> > +  |          #   |         |          |                    #          |       |  |
> > +  |          #   |vsync_start         |                    #          |       |  |
> > +  |          #   |         |          |                    #          |       |  |
> > +  |          #   |         |vsync_end |                    #          |       |  |
> > +  |          #   |         |          |vdisplay            #          |       |  |
> > +  |          #   |         |          |                    #          |       |  |
> > +  |          #   |         |          |                    #          |       |  |
> > +  |          #   |         |          |                    #          |       |  | vtotal
> > +  |          #   |         |          |                    #          |       |  |
> > +  |          #   |         |          |     hsync_start    #          |       |  |
> > +  |          #<--+---------+----------+------------------------------>|       |  |
> > +  |          #   |         |          |                    #          |       |  |
> > +  |          #   |         |          |     hsync_end      #          |       |  |
> > +  |          #<--+---------+----------+-------------------------------------->|  |
> > +  |          #   |         |          ↓                    #          |       |  |
> > +  +----------####|#########|################################----------+-------+  |
> > +  |          |   |         |                               |          |       |  |
> > +  |          |   |         |                               |          |       |  |
> > +  |          |   ↓         |                               |          |       |  |
> > +  +----------+-------------+-------------------------------+----------+-------+  |
> > +  |          |             |                               |          |       |  |
> > +  |          |             |                               |          |       |  |
> > +  |          |             ↓                               |          |       |  ↓
> > +  +----------+---------------------------------------------+----------+-------+
> > +                                   htotal
> > +   <------------------------------------------------------------------------->
> 
> > diff --git a/drivers/of/of_display_timings.c b/drivers/of/of_display_timings.c
> 
> > +static int parse_property(struct device_node *np, char *name,
> > +				struct timing_entry *result)
> 
> > +	if (cells = 1)
> > +		ret = of_property_read_u32_array(np, name, &result->typ, cells);
> 
> Should that branch not just set result->min/max to typ as well?
> Presumably it'd prevent any code that interprets struct timing_entry
> from having to check if those values were 0 or not?
> 

Yes, okay.

> > +	else if (cells = 3)
> > +		ret = of_property_read_u32_array(np, name, &result->min, cells);
> 
> > +struct display_timings *of_get_display_timing_list(struct device_node *np)
> 
> > +	entry = of_parse_phandle(timings_np, "default-timing", 0);
> > +
> > +	if (!entry) {
> > +		pr_info("%s: no default-timing specified\n", __func__);
> > +		entry = of_find_node_by_name(np, "timing");
> 
> I don't think you want to require the node have an explicit name; I
> don't recall the DT binding documentation making that a requirement.
> Instead, can't you either just leave the default unset, or pick the
> first DT child node, irrespective of name?
> 
Ah, yes. I will set the first child then.

> > +	if (!entry) {
> > +		pr_info("%s: no timing specifications given\n", __func__);
> > +		return disp;
> > +	}
> 
> The DT bindings don't state that it's mandatory to have some timing
> specified, although I agree that it makes sense in practice.
> 

The definition of timings in dt doesn't make much sense, when there
are no timings defined, does it ? Maybe I should state the obvious
in the binding, just in case.

> > +	for_each_child_of_node(timings_np, entry) {
> > +		struct signal_timing *st;
> > +
> > +		st = of_get_display_timing(entry);
> > +
> > +		if (!st)
> > +			continue;
> 
> I wonder if that shouldn't be an error?
> 

In the sense of a pr_err not a -EINVAL I presume?! It is a little bit quiet in
case of a faulty spec, that is right.

> > +		if (strcmp(default_timing, entry->full_name) = 0)
> > +			disp->default_timing = disp->num_timings;
> 
> Hmm. Why not compare the node pointers rather than the name? Also, if
> the parsing failed, then this can lead to default_timing being
> uninitialized anyway...
> 
.. and we don't want that. Will fix.

> > +		disp->timings[disp->num_timings] = st;
> > +		disp->num_timings++;
> > +	}
> 
> > +	if (disp->num_timings >= 0)
> > +		pr_info("%s: got %d timings. Using timing #%d as default\n", __func__,
> > +			disp->num_timings , disp->default_timing + 1);
> > +	else
> > +		pr_info("%s: no timings specified\n", __func__);
> 
> The message in the else clause is not necessarily true; there may have
> been some specified, but they just couldn't be parsed.
> 
Right.

> > +int of_display_timings_exists(struct device_node *np)
> > +{
> > +	struct device_node *timings_np;
> > +	struct device_node *default_np;
> > +
> > +	if (!np)
> > +		return -EINVAL;
> > +
> > +	timings_np = of_parse_phandle(np, "display-timings", 0);
> > +
> > +	if (!timings_np)
> > +		return -EINVAL;
> > +
> > +	default_np = of_parse_phandle(np, "default-timing", 0);
> > +
> > +	if (default_np)
> > +		return 0;
> 
> If this function checks that a default-timing property exists, shouldn't
> the function be named of_display_default_timing_exists()?
> 

Maybe I should split this in two functions.

Thanks for your feedback.

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 1/2 v6] of: add helper to parse display timings

[Stephen Warren]

On 10/05/2012 10:16 AM, Steffen Trumtrar wrote:
> On Thu, Oct 04, 2012 at 12:47:16PM -0600, Stephen Warren wrote:
>> On 10/04/2012 11:59 AM, Steffen Trumtrar wrote:
...
>>> +	for_each_child_of_node(timings_np, entry) {
>>> +		struct signal_timing *st;
>>> +
>>> +		st = of_get_display_timing(entry);
>>> +
>>> +		if (!st)
>>> +			continue;
>>
>> I wonder if that shouldn't be an error?
> 
> In the sense of a pr_err not a -EINVAL I presume?! It is a little bit quiet in
> case of a faulty spec, that is right.

I did mean return an error; if we try to parse something and can't,
shouldn't we return an error?

I suppose it may be possible to limp on and use whatever subset of modes
could be parsed and drop the others, which is what this code does, but
the code after the loop would definitely return an error if zero timings
were parseable.

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

[Steffen Trumtrar]

On Fri, Oct 05, 2012 at 10:21:37AM -0600, Stephen Warren wrote:
> On 10/05/2012 10:16 AM, Steffen Trumtrar wrote:
> > On Thu, Oct 04, 2012 at 12:47:16PM -0600, Stephen Warren wrote:
> >> On 10/04/2012 11:59 AM, Steffen Trumtrar wrote:
> ...
> >>> +	for_each_child_of_node(timings_np, entry) {
> >>> +		struct signal_timing *st;
> >>> +
> >>> +		st = of_get_display_timing(entry);
> >>> +
> >>> +		if (!st)
> >>> +			continue;
> >>
> >> I wonder if that shouldn't be an error?
> > 
> > In the sense of a pr_err not a -EINVAL I presume?! It is a little bit quiet in
> > case of a faulty spec, that is right.
> 
> I did mean return an error; if we try to parse something and can't,
> shouldn't we return an error?
> 
> I suppose it may be possible to limp on and use whatever subset of modes
> could be parsed and drop the others, which is what this code does, but
> the code after the loop would definitely return an error if zero timings
> were parseable.

If a display supports multiple modes, I think it is better to have a working
mode (even if it is not the preferred one) than have none at all.
If there is no mode at all, that should be an error, 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 1/2 v6] of: add helper to parse display timings

[Laurent Pinchart]

Hi Steffen,

On Friday 05 October 2012 18:38:58 Steffen Trumtrar wrote:
> On Fri, Oct 05, 2012 at 10:21:37AM -0600, Stephen Warren wrote:
> > On 10/05/2012 10:16 AM, Steffen Trumtrar wrote:
> > > On Thu, Oct 04, 2012 at 12:47:16PM -0600, Stephen Warren wrote:
> > >> On 10/04/2012 11:59 AM, Steffen Trumtrar wrote:
> > ...
> > 
> > >>> +	for_each_child_of_node(timings_np, entry) {
> > >>> +		struct signal_timing *st;
> > >>> +
> > >>> +		st = of_get_display_timing(entry);
> > >>> +
> > >>> +		if (!st)
> > >>> +			continue;
> > >> 
> > >> I wonder if that shouldn't be an error?
> > > 
> > > In the sense of a pr_err not a -EINVAL I presume?! It is a little bit
> > > quiet in case of a faulty spec, that is right.
> > 
> > I did mean return an error; if we try to parse something and can't,
> > shouldn't we return an error?
> > 
> > I suppose it may be possible to limp on and use whatever subset of modes
> > could be parsed and drop the others, which is what this code does, but
> > the code after the loop would definitely return an error if zero timings
> > were parseable.
> 
> If a display supports multiple modes, I think it is better to have a working
> mode (even if it is not the preferred one) than have none at all.
> If there is no mode at all, that should be an error, right.

If we fail completely in case of an error, DT writers will notice their bugs. 
If we ignore errors silently they won't, and we'll end up with buggy DTs (or, 
to be accurate, even more buggy DTs :-)). I'd rather fail completely in the 
first implementation and add workarounds later only if we need to.

-- 
Regards,

Laurent Pinchart

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

[Steffen Trumtrar]

On Sun, Oct 07, 2012 at 03:38:25PM +0200, Laurent Pinchart wrote:
> Hi Steffen,
> 
> On Friday 05 October 2012 18:38:58 Steffen Trumtrar wrote:
> > On Fri, Oct 05, 2012 at 10:21:37AM -0600, Stephen Warren wrote:
> > > On 10/05/2012 10:16 AM, Steffen Trumtrar wrote:
> > > > On Thu, Oct 04, 2012 at 12:47:16PM -0600, Stephen Warren wrote:
> > > >> On 10/04/2012 11:59 AM, Steffen Trumtrar wrote:
> > > ...
> > > 
> > > >>> +	for_each_child_of_node(timings_np, entry) {
> > > >>> +		struct signal_timing *st;
> > > >>> +
> > > >>> +		st = of_get_display_timing(entry);
> > > >>> +
> > > >>> +		if (!st)
> > > >>> +			continue;
> > > >> 
> > > >> I wonder if that shouldn't be an error?
> > > > 
> > > > In the sense of a pr_err not a -EINVAL I presume?! It is a little bit
> > > > quiet in case of a faulty spec, that is right.
> > > 
> > > I did mean return an error; if we try to parse something and can't,
> > > shouldn't we return an error?
> > > 
> > > I suppose it may be possible to limp on and use whatever subset of modes
> > > could be parsed and drop the others, which is what this code does, but
> > > the code after the loop would definitely return an error if zero timings
> > > were parseable.
> > 
> > If a display supports multiple modes, I think it is better to have a working
> > mode (even if it is not the preferred one) than have none at all.
> > If there is no mode at all, that should be an error, right.
> 
> If we fail completely in case of an error, DT writers will notice their bugs. 
> If we ignore errors silently they won't, and we'll end up with buggy DTs (or, 
> to be accurate, even more buggy DTs :-)). I'd rather fail completely in the 
> first implementation and add workarounds later only if we need to.
> 

Okay, that is two against one. And if you say it like this, Stephen and you are
right I guess. Fail completely it is then.

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 1/2 v6] of: add helper to parse display timings

[Guennadi Liakhovetski]

Hi Steffen

Sorry for chiming in so late in the game, but I've long been wanting to 
have a look at this and compare with what we do for V4L2, so, this seems a 
great opportunity to me:-)

On Thu, 4 Oct 2012, Steffen Trumtrar wrote:

> Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
> ---
>  .../devicetree/bindings/video/display-timings.txt  |  222 ++++++++++++++++++++
>  drivers/of/Kconfig                                 |    5 +
>  drivers/of/Makefile                                |    1 +
>  drivers/of/of_display_timings.c                    |  183 ++++++++++++++++
>  include/linux/of_display_timings.h                 |   85 ++++++++
>  5 files changed, 496 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/video/display-timings.txt
>  create mode 100644 drivers/of/of_display_timings.c
>  create mode 100644 include/linux/of_display_timings.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..45e39bd
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/video/display-timings.txt
> @@ -0,0 +1,222 @@
> +display-timings bindings
> +=========
> +
> +display-timings-node
> +------------
> +
> +required properties:
> + - none
> +
> +optional properties:
> + - default-timing: the default timing value
> +
> +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: displayclock in Hz

You're going to hate me for this, but eventually we want to actually 
reference clock objects in our DT bindings. For now, even if you don't 
want to actually add clock phandles and stuff here, I think, using the 
standard "clock-frequency" property would be much better!

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

For the above two we also considered using bool properties but eventually 
settled down with integer ones:

- hsync-active = <1>

for active-high and 0 for active low. This has the added advantage of 
being able to omit this property in the .dts, which then doesn't mean, 
that the polarity is active low, but rather, that the hsync line is not 
used on this hardware. So, maybe it would be good to use the same binding 
here too?

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

We don't (yet) have a de-active property in V4L, don't know whether we'll 
ever have to distingsuish between what some datasheets call "HREF" and 
HSYNC in DT, but maybe similarly to the above an integer would be 
preferred. As for pixclk, we call the property "pclk-sample" and it's also 
an integer.

> + - interlaced (bool)

Is "interlaced" a property of the hardware, i.e. of the board? Can the 
same display controller on one board require interlaced data and on 
another board - progressive? BTW, I'm not very familiar with display 
interfaces, but for interlaced you probably sometimes use a field signal, 
whose polarity you also want to specify here? We use a "field-even-active" 
integer property for it.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

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

[Robert Schwebel]

On Thu, Oct 04, 2012 at 11:35:35PM +0200, Guennadi Liakhovetski wrote:
> > +optional properties:
> > + - hsync-active-high (bool): Hsync pulse is active high
> > + - vsync-active-high (bool): Vsync pulse is active high
>
> For the above two we also considered using bool properties but eventually
> settled down with integer ones:
>
> - hsync-active = <1>
>
> for active-high and 0 for active low. This has the added advantage of
> being able to omit this property in the .dts, which then doesn't mean,
> that the polarity is active low, but rather, that the hsync line is not
> used on this hardware. So, maybe it would be good to use the same binding
> here too?

Philipp, this is the same argumentation as we discussed yesterday for
the dual-link LVDS option, so that one could be modelled in a similar
way.

rsc
-- 
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 1/2 v6] of: add helper to parse display timings

[Stephen Warren]

On 10/04/2012 03:35 PM, Guennadi Liakhovetski wrote:
> Hi Steffen
> 
> Sorry for chiming in so late in the game, but I've long been wanting to 
> have a look at this and compare with what we do for V4L2, so, this seems a 
> great opportunity to me:-)
> 
> On Thu, 4 Oct 2012, Steffen Trumtrar wrote:

>> diff --git a/Documentation/devicetree/bindings/video/display-timings.txt b/Documentation/devicetree/bindings/video/display-timings.txt

>> +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: displayclock in Hz
> 
> You're going to hate me for this, but eventually we want to actually 
> reference clock objects in our DT bindings. For now, even if you don't 
> want to actually add clock phandles and stuff here, I think, using the 
> standard "clock-frequency" property would be much better!

In a definition of a display timing, we will never need to use the clock
binding; the clock binding would be used by the HW module that is
generating a timing, not by the timing definition itself.

That said, your comment about renaming the property to avoid any kind of
conceptual conflict is still quite valid. This is bike-shedding, but
"pixel-clock" might be more in line with typical video mode terminology,
although there's certainly preference in DT for using the generic term
clock-frequency that you proposed. Either is fine by me.

>> +optional properties:
>> + - hsync-active-high (bool): Hsync pulse is active high
>> + - vsync-active-high (bool): Vsync pulse is active high
> 
> For the above two we also considered using bool properties but eventually 
> settled down with integer ones:
> 
> - hsync-active = <1>
> 
> for active-high and 0 for active low. This has the added advantage of 
> being able to omit this property in the .dts, which then doesn't mean, 
> that the polarity is active low, but rather, that the hsync line is not 
> used on this hardware. So, maybe it would be good to use the same binding 
> here too?

I agree. This also covers the case where analog display connectors often
use polarity to differentiate similar modes, yet digital connectors
often always use a fixed polarity since the receiving device can
"measure" the signal in more complete ways.

If the board HW inverts these lines, the same property names can exist
in the display controller itself, and the two values XORd together to
yield the final output polarity.

>> + - de-active-high (bool): Data-Enable pulse is active high
>> + - pixelclk-inverted (bool): pixelclock is inverted
> 
> We don't (yet) have a de-active property in V4L, don't know whether we'll 
> ever have to distingsuish between what some datasheets call "HREF" and 
> HSYNC in DT, but maybe similarly to the above an integer would be 
> preferred. As for pixclk, we call the property "pclk-sample" and it's also 
> an integer.

Thinking about this more: de-active-high is likely to be a
board-specific property and hence something in the display controller,
not in the mode definition?

>> + - interlaced (bool)
> 
> Is "interlaced" a property of the hardware, i.e. of the board? Can the 
> same display controller on one board require interlaced data and on 
> another board - progressive?

Interlace is a property of a display mode. It's quite possible for a
particular display controller to switch between interlace and
progressive output at run-time. For example, reconfiguring the output
between 480i, 720p, 1080i, 1080p modes. Admittedly, if you're talking to
a built-in LCD display, you're probably always going to be driving the
single mode required by the panel, and that mode will likely always be
progressive. However, since this binding attempts to describe any
display timing, I think we still need this property per mode.

> BTW, I'm not very familiar with display 
> interfaces, but for interlaced you probably sometimes use a field signal, 
> whose polarity you also want to specify here? We use a "field-even-active" 
> integer property for it.

I think that's a property of the display controller itself, rather than
an individual mode, although I'm not 100% certain. My assertion is that
the physical interface that the display controller is driving will
determine whether embedded or separate sync is used, and in the separate
sync case, how the field signal is defined, and that all interlace modes
driven over that interface will use the same field signal definition.

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

[Guennadi Liakhovetski]

On Fri, 5 Oct 2012, Stephen Warren wrote:

> On 10/04/2012 03:35 PM, Guennadi Liakhovetski wrote:
> > Hi Steffen
> > 
> > Sorry for chiming in so late in the game, but I've long been wanting to 
> > have a look at this and compare with what we do for V4L2, so, this seems a 
> > great opportunity to me:-)
> > 
> > On Thu, 4 Oct 2012, Steffen Trumtrar wrote:
> 
> >> diff --git a/Documentation/devicetree/bindings/video/display-timings.txt b/Documentation/devicetree/bindings/video/display-timings.txt
> 
> >> +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: displayclock in Hz
> > 
> > You're going to hate me for this, but eventually we want to actually 
> > reference clock objects in our DT bindings. For now, even if you don't 
> > want to actually add clock phandles and stuff here, I think, using the 
> > standard "clock-frequency" property would be much better!
> 
> In a definition of a display timing, we will never need to use the clock
> binding; the clock binding would be used by the HW module that is
> generating a timing, not by the timing definition itself.

You mean clock consumer bindings will be in the display device DT node? 
And the display-timings node will be its child?

> That said, your comment about renaming the property to avoid any kind of
> conceptual conflict is still quite valid. This is bike-shedding, but
> "pixel-clock" might be more in line with typical video mode terminology,
> although there's certainly preference in DT for using the generic term
> clock-frequency that you proposed. Either is fine by me.
> 
> >> +optional properties:
> >> + - hsync-active-high (bool): Hsync pulse is active high
> >> + - vsync-active-high (bool): Vsync pulse is active high
> > 
> > For the above two we also considered using bool properties but eventually 
> > settled down with integer ones:
> > 
> > - hsync-active = <1>
> > 
> > for active-high and 0 for active low. This has the added advantage of 
> > being able to omit this property in the .dts, which then doesn't mean, 
> > that the polarity is active low, but rather, that the hsync line is not 
> > used on this hardware. So, maybe it would be good to use the same binding 
> > here too?
> 
> I agree. This also covers the case where analog display connectors often
> use polarity to differentiate similar modes, yet digital connectors
> often always use a fixed polarity since the receiving device can
> "measure" the signal in more complete ways.
> 
> If the board HW inverts these lines, the same property names can exist
> in the display controller itself, and the two values XORd together to
> yield the final output polarity.
> 
> >> + - de-active-high (bool): Data-Enable pulse is active high
> >> + - pixelclk-inverted (bool): pixelclock is inverted
> > 
> > We don't (yet) have a de-active property in V4L, don't know whether we'll 
> > ever have to distingsuish between what some datasheets call "HREF" and 
> > HSYNC in DT, but maybe similarly to the above an integer would be 
> > preferred. As for pixclk, we call the property "pclk-sample" and it's also 
> > an integer.
> 
> Thinking about this more: de-active-high is likely to be a
> board-specific property and hence something in the display controller,
> not in the mode definition?
> 
> >> + - interlaced (bool)
> > 
> > Is "interlaced" a property of the hardware, i.e. of the board? Can the 
> > same display controller on one board require interlaced data and on 
> > another board - progressive?
> 
> Interlace is a property of a display mode. It's quite possible for a
> particular display controller to switch between interlace and
> progressive output at run-time. For example, reconfiguring the output
> between 480i, 720p, 1080i, 1080p modes. Admittedly, if you're talking to
> a built-in LCD display, you're probably always going to be driving the
> single mode required by the panel, and that mode will likely always be
> progressive. However, since this binding attempts to describe any
> display timing, I think we still need this property per mode.

But why do you need this in the DT then at all? If it's fixed, as required 
per display controller, then its driver will know it. If it's runtime 
configurable, then it's a purely software parameter and doesn't depend on 
the board?

> > BTW, I'm not very familiar with display 
> > interfaces, but for interlaced you probably sometimes use a field signal, 
> > whose polarity you also want to specify here? We use a "field-even-active" 
> > integer property for it.
> 
> I think that's a property of the display controller itself, rather than
> an individual mode, although I'm not 100% certain. My assertion is that
> the physical interface that the display controller is driving will
> determine whether embedded or separate sync is used, and in the separate
> sync case, how the field signal is defined, and that all interlace modes
> driven over that interface will use the same field signal definition.

In general, I might be misunderstanding something, but don't we have to 
distinguish between 2 types of information about display timings: (1) is 
defined by the display controller requirements, is known to the display 
driver and doesn't need to be present in timings DT. We did have some of 
these parameters in board data previously, because we didn't have proper 
display controller drivers... (2) is board specific configuration, and is 
such it has to be present in DT.

In that way, doesn't "interlaced" belong to type (1) and thus doesn't need 
to be present in DT?

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

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

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 1406 bytes --]

On Mon, 2012-10-08 at 10:25 +0200, Guennadi Liakhovetski wrote:

> In general, I might be misunderstanding something, but don't we have to 
> distinguish between 2 types of information about display timings: (1) is 
> defined by the display controller requirements, is known to the display 
> driver and doesn't need to be present in timings DT. We did have some of 
> these parameters in board data previously, because we didn't have proper 
> display controller drivers... (2) is board specific configuration, and is 
> such it has to be present in DT.
> 
> In that way, doesn't "interlaced" belong to type (1) and thus doesn't need 
> to be present in DT?

As I see it, this DT data is about the display (most commonly LCD
panel), i.e. what video mode(s) the panel supports. If things were done
my way, the panel's supported timings would be defined in the driver for
the panel, and DT would be left to describe board specific data, but
this approach has its benefits.

Thus, if you connect an interlaced panel to your board, you need to tell
the display controller that this panel requires interlace signal. Also,
pixel clock source doesn't make sense in this context, as this doesn't
describe the actual used configuration, but only what the panel
supports.

Of course, if this is about describing the hardware, the default-mode
property doesn't really fit in...

 Tomi


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Laurent Pinchart]

Hi Tomi,

On Monday 08 October 2012 12:01:18 Tomi Valkeinen wrote:
> On Mon, 2012-10-08 at 10:25 +0200, Guennadi Liakhovetski wrote:
> > In general, I might be misunderstanding something, but don't we have to
> > distinguish between 2 types of information about display timings: (1) is
> > defined by the display controller requirements, is known to the display
> > driver and doesn't need to be present in timings DT. We did have some of
> > these parameters in board data previously, because we didn't have proper
> > display controller drivers... (2) is board specific configuration, and is
> > such it has to be present in DT.
> > 
> > In that way, doesn't "interlaced" belong to type (1) and thus doesn't need
> > to be present in DT?
> 
> As I see it, this DT data is about the display (most commonly LCD
> panel), i.e. what video mode(s) the panel supports. If things were done
> my way, the panel's supported timings would be defined in the driver for
> the panel, and DT would be left to describe board specific data, but
> this approach has its benefits.

What about dumb DPI panels ? They will all be supported by a single driver, 
would you have the driver contain information about all known DPI panels ? DT 
seems a good solution to me in this case.

For complex panels where the driver will support a single (or very few) model 
I agree that specifying the timings in DT isn't needed.

> Thus, if you connect an interlaced panel to your board, you need to tell
> the display controller that this panel requires interlace signal. Also,
> pixel clock source doesn't make sense in this context, as this doesn't
> describe the actual used configuration, but only what the panel
> supports.
> 
> Of course, if this is about describing the hardware, the default-mode
> property doesn't really fit in...

Maybe we should rename it to native-mode then ?

-- 
Regards,

Laurent Pinchart

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

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 2558 bytes --]

On Mon, 2012-10-08 at 14:04 +0200, Laurent Pinchart wrote:
> Hi Tomi,
> 
> On Monday 08 October 2012 12:01:18 Tomi Valkeinen wrote:
> > On Mon, 2012-10-08 at 10:25 +0200, Guennadi Liakhovetski wrote:
> > > In general, I might be misunderstanding something, but don't we have to
> > > distinguish between 2 types of information about display timings: (1) is
> > > defined by the display controller requirements, is known to the display
> > > driver and doesn't need to be present in timings DT. We did have some of
> > > these parameters in board data previously, because we didn't have proper
> > > display controller drivers... (2) is board specific configuration, and is
> > > such it has to be present in DT.
> > > 
> > > In that way, doesn't "interlaced" belong to type (1) and thus doesn't need
> > > to be present in DT?
> > 
> > As I see it, this DT data is about the display (most commonly LCD
> > panel), i.e. what video mode(s) the panel supports. If things were done
> > my way, the panel's supported timings would be defined in the driver for
> > the panel, and DT would be left to describe board specific data, but
> > this approach has its benefits.
> 
> What about dumb DPI panels ? They will all be supported by a single driver, 
> would you have the driver contain information about all known DPI panels ? DT 
> seems a good solution to me in this case.

Yes, I would have a table in the driver for all the devices it supports,
which would describe the device specific parameters.

But I don't have a problem with DT solution. Both methods have their
pros and cons, and perhaps DT based solution is more practical.

> For complex panels where the driver will support a single (or very few) model 
> I agree that specifying the timings in DT isn't needed.
> 
> > Thus, if you connect an interlaced panel to your board, you need to tell
> > the display controller that this panel requires interlace signal. Also,
> > pixel clock source doesn't make sense in this context, as this doesn't
> > describe the actual used configuration, but only what the panel
> > supports.
> > 
> > Of course, if this is about describing the hardware, the default-mode
> > property doesn't really fit in...
> 
> Maybe we should rename it to native-mode then ?

Hmm, right, if it means native mode, then it is describing the hardware.
But would it make sense to require that the native mode is the first
mode in the list, then? This would make the separate
default-mode/native-mode property not needed.

 Tomi


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Stephen Warren]

On 10/08/2012 06:20 AM, Tomi Valkeinen wrote:
> On Mon, 2012-10-08 at 14:04 +0200, Laurent Pinchart wrote:
>> On Monday 08 October 2012 12:01:18 Tomi Valkeinen wrote:
>>> On Mon, 2012-10-08 at 10:25 +0200, Guennadi Liakhovetski
>>> wrote:
...
>>> Of course, if this is about describing the hardware, the
>>> default-mode property doesn't really fit in...
>> 
>> Maybe we should rename it to native-mode then ?
> 
> Hmm, right, if it means native mode, then it is describing the
> hardware. But would it make sense to require that the native mode
> is the first mode in the list, then? This would make the separate 
> default-mode/native-mode property not needed.

I'm not sure if device-tree guarantees that the nodes enumerate in a
specific order. If it does, then that may be a reasonable solution.

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

[Mitch Bradley]

On 10/7/2012 11:01 PM, Tomi Valkeinen wrote:
> On Mon, 2012-10-08 at 10:25 +0200, Guennadi Liakhovetski wrote:
> 
>> In general, I might be misunderstanding something, but don't we have to 
>> distinguish between 2 types of information about display timings: (1) is 
>> defined by the display controller requirements, is known to the display 
>> driver and doesn't need to be present in timings DT. We did have some of 
>> these parameters in board data previously, because we didn't have proper 
>> display controller drivers... (2) is board specific configuration, and is 
>> such it has to be present in DT.
>>
>> In that way, doesn't "interlaced" belong to type (1) and thus doesn't need 
>> to be present in DT?
> 
> As I see it, this DT data is about the display (most commonly LCD
> panel), i.e. what video mode(s) the panel supports. If things were done
> my way, the panel's supported timings would be defined in the driver for
> the panel, and DT would be left to describe board specific data, but
> this approach has its benefits.
> 
> Thus, if you connect an interlaced panel to your board,


Do interlaced panels exist?  I have never seen one.


 you need to tell
> the display controller that this panel requires interlace signal. Also,
> pixel clock source doesn't make sense in this context, as this doesn't
> describe the actual used configuration, but only what the panel
> supports.
> 
> Of course, if this is about describing the hardware, the default-mode
> property doesn't really fit in...
> 
>  Tomi
> 
> 
> 
> _______________________________________________
> devicetree-discuss mailing list
> devicetree-discuss@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss
> 

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

[Stephen Warren]

On 10/08/2012 02:25 AM, Guennadi Liakhovetski wrote:
> On Fri, 5 Oct 2012, Stephen Warren wrote:
> 
>> On 10/04/2012 03:35 PM, Guennadi Liakhovetski wrote:
>>> Hi Steffen
>>>
>>> Sorry for chiming in so late in the game, but I've long been wanting to 
>>> have a look at this and compare with what we do for V4L2, so, this seems a 
>>> great opportunity to me:-)
>>>
>>> On Thu, 4 Oct 2012, Steffen Trumtrar wrote:
>>
>>>> diff --git a/Documentation/devicetree/bindings/video/display-timings.txt b/Documentation/devicetree/bindings/video/display-timings.txt
>>
>>>> +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: displayclock in Hz
>>>
>>> You're going to hate me for this, but eventually we want to actually 
>>> reference clock objects in our DT bindings. For now, even if you don't 
>>> want to actually add clock phandles and stuff here, I think, using the 
>>> standard "clock-frequency" property would be much better!
>>
>> In a definition of a display timing, we will never need to use the clock
>> binding; the clock binding would be used by the HW module that is
>> generating a timing, not by the timing definition itself.
> 
> You mean clock consumer bindings will be in the display device DT node? 
> And the display-timings node will be its child?

Yes

...
>>>> + - interlaced (bool)
>>>
>>> Is "interlaced" a property of the hardware, i.e. of the board? Can the 
>>> same display controller on one board require interlaced data and on 
>>> another board - progressive?
>>
>> Interlace is a property of a display mode. It's quite possible for a
>> particular display controller to switch between interlace and
>> progressive output at run-time. For example, reconfiguring the output
>> between 480i, 720p, 1080i, 1080p modes. Admittedly, if you're talking to
>> a built-in LCD display, you're probably always going to be driving the
>> single mode required by the panel, and that mode will likely always be
>> progressive. However, since this binding attempts to describe any
>> display timing, I think we still need this property per mode.
> 
> But why do you need this in the DT then at all?

Because the driver for the display controller has no idea what display
or panel will be connected to it.

> If it's fixed, as required 
> per display controller, then its driver will know it. If it's runtime 
> configurable, then it's a purely software parameter and doesn't depend on 
> the board?

interlace-vs-progressive isn't "fixed, as required per display
controller", but is a property of the mode being sent by the display
controller, and the requirements for that mode are driven by the
panel/display connected to the display controller, not the display
controller, in general.

...
>>> BTW, I'm not very familiar with display 
>>> interfaces, but for interlaced you probably sometimes use a field signal, 
>>> whose polarity you also want to specify here? We use a "field-even-active" 
>>> integer property for it.
>>
>> I think that's a property of the display controller itself, rather than
>> an individual mode, although I'm not 100% certain. My assertion is that
>> the physical interface that the display controller is driving will
>> determine whether embedded or separate sync is used, and in the separate
>> sync case, how the field signal is defined, and that all interlace modes
>> driven over that interface will use the same field signal definition.
> 
> In general, I might be misunderstanding something, but don't we have to 
> distinguish between 2 types of information about display timings: (1) is 
> defined by the display controller requirements, is known to the display 
> driver and doesn't need to be present in timings DT. We did have some of 
> these parameters in board data previously, because we didn't have proper 
> display controller drivers...

Yes, there probably is data of that kind, but the display mode timings
binding is only address standardized display timings information, not
controller-specific information, and hence doesn't cover this case.

> (2) is board specific configuration, and is
> such it has to be present in DT.

Certainly, yes.

> In that way, doesn't "interlaced" belong to type (1) and thus doesn't need 
> to be present in DT?

I don't believe so.

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

[Laurent Pinchart]

Hi Stephen,

On Monday 08 October 2012 10:10:31 Stephen Warren wrote:
> On 10/08/2012 02:25 AM, Guennadi Liakhovetski wrote:
> > On Fri, 5 Oct 2012, Stephen Warren wrote:
> >> On 10/04/2012 03:35 PM, Guennadi Liakhovetski wrote:
> >>> Hi Steffen
> >>> 
> >>> Sorry for chiming in so late in the game, but I've long been wanting to
> >>> have a look at this and compare with what we do for V4L2, so, this seems
> >>> a great opportunity to me:-)
> >>> 
> >>> On Thu, 4 Oct 2012, Steffen Trumtrar wrote:
> >>>> diff --git
> >>>> a/Documentation/devicetree/bindings/video/display-timings.txt
> >>>> b/Documentation/devicetree/bindings/video/display-timings.txt
> >>>> 
> >>>> +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: displayclock in Hz
> >>> 
> >>> You're going to hate me for this, but eventually we want to actually
> >>> reference clock objects in our DT bindings. For now, even if you don't
> >>> want to actually add clock phandles and stuff here, I think, using the
> >>> standard "clock-frequency" property would be much better!
> >> 
> >> In a definition of a display timing, we will never need to use the clock
> >> binding; the clock binding would be used by the HW module that is
> >> generating a timing, not by the timing definition itself.
> > 
> > You mean clock consumer bindings will be in the display device DT node?
> > And the display-timings node will be its child?
> 
> Yes
> 
> ...
> 
> >>>> + - interlaced (bool)
> >>> 
> >>> Is "interlaced" a property of the hardware, i.e. of the board? Can the
> >>> same display controller on one board require interlaced data and on
> >>> another board - progressive?
> >> 
> >> Interlace is a property of a display mode. It's quite possible for a
> >> particular display controller to switch between interlace and
> >> progressive output at run-time. For example, reconfiguring the output
> >> between 480i, 720p, 1080i, 1080p modes. Admittedly, if you're talking to
> >> a built-in LCD display, you're probably always going to be driving the
> >> single mode required by the panel, and that mode will likely always be
> >> progressive. However, since this binding attempts to describe any
> >> display timing, I think we still need this property per mode.
> > 
> > But why do you need this in the DT then at all?
> 
> Because the driver for the display controller has no idea what display
> or panel will be connected to it.

That's right, but with the current common panel framework (WIP, I plan to 
resume working on it this week) the display controller will be able to query 
the panel for bus configuration parameters (or the other way around, the panel 
driver will set the display controller bus configuration). The panel driver 
will still need to receive timing information from DT, but it will be possible 
to communicate display bus configuration parameters between the panel driver 
and the display controller driver.

This being said, I agree that interlaced is a property of the display mode, 
not of the display bus (even though the display bus will then be configured 
for interlaced data).

> > If it's fixed, as required per display controller, then its driver will
> > know it. If it's runtime configurable, then it's a purely software
> > parameter and doesn't depend on the board?
> 
> interlace-vs-progressive isn't "fixed, as required per display
> controller", but is a property of the mode being sent by the display
> controller, and the requirements for that mode are driven by the
> panel/display connected to the display controller, not the display
> controller, in general.
> 
> ...
> 
> >>> BTW, I'm not very familiar with display interfaces, but for interlaced
> >>> you probably sometimes use a field signal, whose polarity you also want
> >>> to specify here? We use a "field-even-active" integer property for it.
> >> 
> >> I think that's a property of the display controller itself, rather than
> >> an individual mode, although I'm not 100% certain. My assertion is that
> >> the physical interface that the display controller is driving will
> >> determine whether embedded or separate sync is used, and in the separate
> >> sync case, how the field signal is defined, and that all interlace modes
> >> driven over that interface will use the same field signal definition.
> > 
> > In general, I might be misunderstanding something, but don't we have to
> > distinguish between 2 types of information about display timings: (1) is
> > defined by the display controller requirements, is known to the display
> > driver and doesn't need to be present in timings DT. We did have some of
> > these parameters in board data previously, because we didn't have proper
> > display controller drivers...
> 
> Yes, there probably is data of that kind, but the display mode timings
> binding is only address standardized display timings information, not
> controller-specific information, and hence doesn't cover this case.
> 
> > (2) is board specific configuration, and is
> > such it has to be present in DT.
> 
> Certainly, yes.
> 
> > In that way, doesn't "interlaced" belong to type (1) and thus doesn't need
> > to be present in DT?
> 
> I don't believe so.

-- 
Regards,

Laurent Pinchart

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

[Steffen Trumtrar]

On Thu, Oct 04, 2012 at 11:35:35PM +0200, Guennadi Liakhovetski wrote:
> Hi Steffen
> 
> Sorry for chiming in so late in the game, but I've long been wanting to 
> have a look at this and compare with what we do for V4L2, so, this seems a 
> great opportunity to me:-)
> 
> On Thu, 4 Oct 2012, Steffen Trumtrar wrote:
> 
> > Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
> > ---
> >  .../devicetree/bindings/video/display-timings.txt  |  222 ++++++++++++++++++++
> >  drivers/of/Kconfig                                 |    5 +
> >  drivers/of/Makefile                                |    1 +
> >  drivers/of/of_display_timings.c                    |  183 ++++++++++++++++
> >  include/linux/of_display_timings.h                 |   85 ++++++++
> >  5 files changed, 496 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/video/display-timings.txt
> >  create mode 100644 drivers/of/of_display_timings.c
> >  create mode 100644 include/linux/of_display_timings.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..45e39bd
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/video/display-timings.txt
> > @@ -0,0 +1,222 @@
> > +display-timings bindings
> > +=========
> > +
> > +display-timings-node
> > +------------
> > +
> > +required properties:
> > + - none
> > +
> > +optional properties:
> > + - default-timing: the default timing value
> > +
> > +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: displayclock in Hz
> 
> You're going to hate me for this, but eventually we want to actually 
> reference clock objects in our DT bindings. For now, even if you don't 
> want to actually add clock phandles and stuff here, I think, using the 
> standard "clock-frequency" property would be much better!
> 

Well, that shouldn't be a big deal, the "clock-frequency" property I mean :-)

> > +
> > +optional properties:
> > + - hsync-active-high (bool): Hsync pulse is active high
> > + - vsync-active-high (bool): Vsync pulse is active high
> 
> For the above two we also considered using bool properties but eventually 
> settled down with integer ones:
> 
> - hsync-active = <1>
> 
> for active-high and 0 for active low. This has the added advantage of 
> being able to omit this property in the .dts, which then doesn't mean, 
> that the polarity is active low, but rather, that the hsync line is not 
> used on this hardware. So, maybe it would be good to use the same binding 
> here too?
> 

Never really thought about it that way. But the argument sounds convincing.

> > + - de-active-high (bool): Data-Enable pulse is active high
> > + - pixelclk-inverted (bool): pixelclock is inverted
> 
> We don't (yet) have a de-active property in V4L, don't know whether we'll 
> ever have to distingsuish between what some datasheets call "HREF" and 
> HSYNC in DT, but maybe similarly to the above an integer would be 
> preferred. As for pixclk, we call the property "pclk-sample" and it's also 
> an integer.
> 
> > + - interlaced (bool)
> 
> Is "interlaced" a property of the hardware, i.e. of the board? Can the 
> same display controller on one board require interlaced data and on 
> another board - progressive? BTW, I'm not very familiar with display 
> interfaces, but for interlaced you probably sometimes use a field signal, 
> whose polarity you also want to specify here? We use a "field-even-active" 
> integer property for it.
> 

I don't really know about that; have to collect some info first.

> Thanks
> Guennadi

Thank you.

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 1/2 v6] of: add helper to parse display timings

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 22858 bytes --]

Hi,

On Thu, 2012-10-04 at 19:59 +0200, Steffen Trumtrar wrote:
> Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
> ---
>  .../devicetree/bindings/video/display-timings.txt  |  222 ++++++++++++++++++++
>  drivers/of/Kconfig                                 |    5 +
>  drivers/of/Makefile                                |    1 +
>  drivers/of/of_display_timings.c                    |  183 ++++++++++++++++
>  include/linux/of_display_timings.h                 |   85 ++++++++
>  5 files changed, 496 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/video/display-timings.txt
>  create mode 100644 drivers/of/of_display_timings.c
>  create mode 100644 include/linux/of_display_timings.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..45e39bd
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/video/display-timings.txt
> @@ -0,0 +1,222 @@
> +display-timings bindings
> +==================
> +
> +display-timings-node
> +------------
> +
> +required properties:
> + - none
> +
> +optional properties:
> + - default-timing: the default timing value
> +
> +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: displayclock in Hz
> +
> +optional properties:
> + - 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
> + - interlaced (bool)
> + - doublescan (bool)

I think bool should be generally used for things that are on/off, like
interlace. For hsync-active-high & others I'd rather have 0/1 values as
others already suggested.

> +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 default-timing can be specified.
> +
> +The parameters are defined as
> +
> +struct signal_timing
> +===================
> +
> +  +----------+---------------------------------------------+----------+-------+
> +  |          |                ↑                            |          |       |
> +  |          |                |vback_porch                 |          |       |
> +  |          |                ↓                            |          |       |
> +  +----------###############################################----------+-------+
> +  |          #                ↑                            #          |       |
> +  |          #                |                            #          |       |
> +  |  hback   #                |                            #  hfront  | hsync |
> +  |   porch  #                |       hactive              #  porch   |  len  |
> +  |<-------->#<---------------+--------------------------->#<-------->|<----->|
> +  |          #                |                            #          |       |
> +  |          #                |vactive                     #          |       |
> +  |          #                |                            #          |       |
> +  |          #                ↓                            #          |       |
> +  +----------###############################################----------+-------+
> +  |          |                ↑                            |          |       |
> +  |          |                |vfront_porch                |          |       |
> +  |          |                ↓                            |          |       |
> +  +----------+---------------------------------------------+----------+-------+
> +  |          |                ↑                            |          |       |
> +  |          |                |vsync_len                   |          |       |
> +  |          |                ↓                            |          |       |
> +  +----------+---------------------------------------------+----------+-------+
> +
> +
> +Example:
> +
> +	display-timings {
> +		default-timing = <&timing0>;
> +		timing0: 1920p24 {
> +			/* 1920x1080p24 */

I think this is commonly called 1080p24.

> +			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 {
> +		/* 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>;
> +	};
> +
> +Usage in backend
> +================
> +
> +A backend driver may choose to use the display-timings 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_timing_list
> +					 ↓
> +			      struct display_timings
> +					 |
> +					 |  videomode_from_timing
> +					 ↓
> +			    ---  struct videomode ---
> +			    |			    |
> + videomode_to_displaymode   |			    |   videomode_to_fb_videomode
> +		            ↓			    ↓
> +		     drm_display_mode         fb_videomode
> +
> +The functions of_get_fb_videomode and of_get_display_mode are provided
> +to conveniently get the respective mode representation from the devicetree.
> +
> +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..646deb0 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_TIMINGS
> +	def_bool y
> +	help
> +	  helper to parse display timings from the devicetree
> +
>  endmenu # OF
> diff --git a/drivers/of/Makefile b/drivers/of/Makefile
> index e027f44..c8e9603 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_TIMINGS) += of_display_timings.o
> diff --git a/drivers/of/of_display_timings.c b/drivers/of/of_display_timings.c
> new file mode 100644
> index 0000000..e47bc63
> --- /dev/null
> +++ b/drivers/of/of_display_timings.c
> @@ -0,0 +1,183 @@
> +/*
> + * 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>
> +
> +/* 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
> + */

The comment is not according to kernel coding style. And I'd start the
sentence with a capital letter =).

> +static int 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 signal_timing *of_get_display_timing(struct device_node *np)
> +{
> +	struct signal_timing *st;
> +	int ret = 0;
> +
> +	st = kzalloc(sizeof(*st), GFP_KERNEL);
> +
> +	if (!st) {
> +		pr_err("%s: could not allocate signal_timing struct\n", __func__);
> +		return NULL;
> +	}
> +
> +	ret |= parse_property(np, "hback-porch", &st->hback_porch);
> +	ret |= parse_property(np, "hfront-porch", &st->hfront_porch);
> +	ret |= parse_property(np, "hactive", &st->hactive);
> +	ret |= parse_property(np, "hsync-len", &st->hsync_len);
> +	ret |= parse_property(np, "vback-porch", &st->vback_porch);
> +	ret |= parse_property(np, "vfront-porch", &st->vfront_porch);
> +	ret |= parse_property(np, "vactive", &st->vactive);
> +	ret |= parse_property(np, "vsync-len", &st->vsync_len);
> +	ret |= parse_property(np, "clock", &st->pixelclock);
> +
> +	st->vsync_pol_active_high = of_property_read_bool(np, "vsync-active-high");
> +	st->hsync_pol_active_high = of_property_read_bool(np, "hsync-active-high");
> +	st->de_pol_active_high = of_property_read_bool(np, "de-active-high");
> +	st->pixelclk_pol_inverted = of_property_read_bool(np, "pixelclk-inverted");
> +	st->interlaced = of_property_read_bool(np, "interlaced");
> +	st->doublescan = of_property_read_bool(np, "doublescan");
> +
> +	if (ret) {
> +		pr_err("%s: error reading timing properties\n", __func__);
> +		return NULL;
> +	}
> +
> +	return st;
> +}
> +EXPORT_SYMBOL_GPL(of_get_display_timing);
> +
> +struct display_timings *of_get_display_timing_list(struct device_node *np)
> +{
> +	struct device_node *timings_np;
> +	struct device_node *entry;
> +	struct display_timings *disp;
> +	char *default_timing;
> +
> +	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);
> +
> +	entry = of_parse_phandle(timings_np, "default-timing", 0);
> +
> +	if (!entry) {
> +		pr_info("%s: no default-timing specified\n", __func__);
> +		entry = of_find_node_by_name(np, "timing");
> +	}

If "default-timing" property is optional, I don't see any need for the
pr_info above, as it should be business as usual if the property doesn't
exist.

If the default-timing property doesn't exist, wouldn't it be simpler to
get the first subnode, instead of looking one with "timing" name?

> +
> +	if (!entry) {
> +		pr_info("%s: no timing specifications given\n", __func__);
> +		return disp;
> +	}

Again, I don't think the pr_info is needed if this is a normal case.
Then again, perhaps this could be an error? Why would there be a display
node without any timings?

> +
> +	pr_info("%s: using %s as default timing\n", __func__, entry->name);
> +
> +	default_timing = (char *)entry->full_name;

const char *?

> +
> +	disp->num_timings = 0;
> +
> +	for_each_child_of_node(timings_np, entry) {
> +		disp->num_timings++;
> +	}

No need for { }

> +	disp->timings = kzalloc(sizeof(struct signal_timing *)*disp->num_timings,
> +				GFP_KERNEL);
> +
> +	disp->num_timings = 0;
> +
> +	for_each_child_of_node(timings_np, entry) {
> +		struct signal_timing *st;
> +
> +		st = of_get_display_timing(entry);
> +
> +		if (!st)
> +			continue;
> +
> +		if (strcmp(default_timing, entry->full_name) == 0)
> +			disp->default_timing = disp->num_timings;

I don't see you setting disp->default_timing to OF_DEFAULT_TIMING in
case there's no default_timing found.

Or, at least I presume OF_DEFAULT_TIMING is meant to mark non-existing
default timing. The name OF_DEFAULT_TIMING is not very descriptive to
me.

Would it make more sense to have the disp->default_timing as a pointer
to the timing, instead of index? Then a NULL value would mark a
non-existing default timing.

> +		disp->timings[disp->num_timings] = st;
> +		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->default_timing + 1);
> +	else
> +		pr_info("%s: no timings specified\n", __func__);
> +
> +	return disp;
> +}
> +EXPORT_SYMBOL_GPL(of_get_display_timing_list);
> +
> +int of_display_timings_exists(struct device_node *np)
> +{
> +	struct device_node *timings_np;
> +	struct device_node *default_np;
> +
> +	if (!np)
> +		return -EINVAL;
> +
> +	timings_np = of_parse_phandle(np, "display-timings", 0);
> +
> +	if (!timings_np)
> +		return -EINVAL;
> +
> +	default_np = of_parse_phandle(np, "default-timing", 0);
> +
> +	if (default_np)
> +		return 0;
> +
> +	return -EINVAL;
> +}
> +EXPORT_SYMBOL_GPL(of_display_timings_exists);
> diff --git a/include/linux/of_display_timings.h b/include/linux/of_display_timings.h
> new file mode 100644
> index 0000000..1ad719e
> --- /dev/null
> +++ b/include/linux/of_display_timings.h
> @@ -0,0 +1,85 @@
> +/*
> + * Copyright 2012 Steffen Trumtrar <s.trumtrar@pengutronix.de>
> + *
> + * description of display timings
> + *
> + * This file is released under the GPLv2
> + */
> +
> +#ifndef __LINUX_OF_DISPLAY_TIMINGS_H
> +#define __LINUX_OF_DISPLAY_TIMINGS_H
> +
> +#define OF_DEFAULT_TIMING -1
> +
> +struct display_timings {
> +	unsigned int num_timings;
> +	unsigned int default_timing;
> +
> +	struct signal_timing **timings;

Should this be a pointer to a const array of const data? Is there ever
need to change them after they've been read from DT?

> +};
> +
> +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;
> +
> +	bool vsync_pol_active_high;
> +	bool hsync_pol_active_high;
> +	bool de_pol_active_high;
> +	bool pixelclk_pol_inverted;
> +	bool interlaced;
> +	bool doublescan;
> +};
> +
> +struct display_timings *of_get_display_timing_list(struct device_node *np);
> +struct signal_timing *of_get_display_timing(struct device_node *np);
> +int of_display_timings_exists(struct device_node *np);
> +
> +/* 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_timings *disp)
> +{
> +	int i;
> +
> +	for (i = 0; i < disp->num_timings; i++)
> +		kfree(disp->timings[i]);
> +}
> +
> +static inline void display_timings_release(struct display_timings *disp)
> +{
> +	timings_release(disp);
> +	kfree(disp->timings);
> +}
> +
> +static inline struct signal_timing *display_timings_get(struct display_timings *disp,
> +							 int index)
> +{
> +	struct signal_timing *st;
> +
> +	if (disp->num_timings > index) {
> +		st = disp->timings[index];
> +		return st;
> +	}
> +	else
> +		return NULL;
> +}

Why do you have these functions in the header file?

 Tomi


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 638 bytes --]

On Mon, 2012-10-08 at 10:07 +0300, Tomi Valkeinen wrote:
> Hi,

> I don't see you setting disp->default_timing to OF_DEFAULT_TIMING in
> case there's no default_timing found.
> 
> Or, at least I presume OF_DEFAULT_TIMING is meant to mark non-existing
> default timing. The name OF_DEFAULT_TIMING is not very descriptive to
> me.

Ah, I see now from the second patch how this is meant to be used. So if
there's no default timing in DT data, disp->default_timing is 0, meaning
the first entry. And the caller of of_get_videomode() will use
OF_DEFAULT_TIMING as index to get the default mode.

So I think it's ok.

 Tomi


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Steffen Trumtrar]

Hi,

On Mon, Oct 08, 2012 at 10:07:45AM +0300, Tomi Valkeinen wrote:
> Hi,
> 
> On Thu, 2012-10-04 at 19:59 +0200, Steffen Trumtrar wrote:
> > Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
> > ---
> >  .../devicetree/bindings/video/display-timings.txt  |  222 ++++++++++++++++++++
> >  drivers/of/Kconfig                                 |    5 +
> >  drivers/of/Makefile                                |    1 +
> >  drivers/of/of_display_timings.c                    |  183 ++++++++++++++++
> >  include/linux/of_display_timings.h                 |   85 ++++++++
> >  5 files changed, 496 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/video/display-timings.txt
> >  create mode 100644 drivers/of/of_display_timings.c
> >  create mode 100644 include/linux/of_display_timings.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..45e39bd
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/video/display-timings.txt
> > @@ -0,0 +1,222 @@
> > +display-timings bindings
> > +=========
> > +
> > +display-timings-node
> > +------------
> > +
> > +required properties:
> > + - none
> > +
> > +optional properties:
> > + - default-timing: the default timing value
> > +
> > +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: displayclock in Hz
> > +
> > +optional properties:
> > + - 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
> > + - interlaced (bool)
> > + - doublescan (bool)
> 
> I think bool should be generally used for things that are on/off, like
> interlace. For hsync-active-high & others I'd rather have 0/1 values as
> others already suggested.
> 
> > +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 default-timing can be specified.
> > +
> > +The parameters are defined as
> > +
> > +struct signal_timing
> > +=========> > +
> > +  +----------+---------------------------------------------+----------+-------+
> > +  |          |                ↑                            |          |       |
> > +  |          |                |vback_porch                 |          |       |
> > +  |          |                ↓                            |          |       |
> > +  +----------###############################################----------+-------+
> > +  |          #                ↑                            #          |       |
> > +  |          #                |                            #          |       |
> > +  |  hback   #                |                            #  hfront  | hsync |
> > +  |   porch  #                |       hactive              #  porch   |  len  |
> > +  |<-------->#<---------------+--------------------------->#<-------->|<----->|
> > +  |          #                |                            #          |       |
> > +  |          #                |vactive                     #          |       |
> > +  |          #                |                            #          |       |
> > +  |          #                ↓                            #          |       |
> > +  +----------###############################################----------+-------+
> > +  |          |                ↑                            |          |       |
> > +  |          |                |vfront_porch                |          |       |
> > +  |          |                ↓                            |          |       |
> > +  +----------+---------------------------------------------+----------+-------+
> > +  |          |                ↑                            |          |       |
> > +  |          |                |vsync_len                   |          |       |
> > +  |          |                ↓                            |          |       |
> > +  +----------+---------------------------------------------+----------+-------+
> > +
> > +
> > +Example:
> > +
> > +	display-timings {
> > +		default-timing = <&timing0>;
> > +		timing0: 1920p24 {
> > +			/* 1920x1080p24 */
> 
> I think this is commonly called 1080p24.
> 

Oops. Yes, you are right.

> > +			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 {
> > +		/* 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>;
> > +	};
> > +
> > +Usage in backend
> > +========
> > +
> > +A backend driver may choose to use the display-timings 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_timing_list
> > +					 ↓
> > +			      struct display_timings
> > +					 |
> > +					 |  videomode_from_timing
> > +					 ↓
> > +			    ---  struct videomode ---
> > +			    |			    |
> > + videomode_to_displaymode   |			    |   videomode_to_fb_videomode
> > +		            ↓			    ↓
> > +		     drm_display_mode         fb_videomode
> > +
> > +The functions of_get_fb_videomode and of_get_display_mode are provided
> > +to conveniently get the respective mode representation from the devicetree.
> > +
> > +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..646deb0 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_TIMINGS
> > +	def_bool y
> > +	help
> > +	  helper to parse display timings from the devicetree
> > +
> >  endmenu # OF
> > diff --git a/drivers/of/Makefile b/drivers/of/Makefile
> > index e027f44..c8e9603 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_TIMINGS) += of_display_timings.o
> > diff --git a/drivers/of/of_display_timings.c b/drivers/of/of_display_timings.c
> > new file mode 100644
> > index 0000000..e47bc63
> > --- /dev/null
> > +++ b/drivers/of/of_display_timings.c
> > @@ -0,0 +1,183 @@
> > +/*
> > + * 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>
> > +
> > +/* 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
> > + */
> 
> The comment is not according to kernel coding style. And I'd start the
> sentence with a capital letter =).
> 
> > +static int 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 signal_timing *of_get_display_timing(struct device_node *np)
> > +{
> > +	struct signal_timing *st;
> > +	int ret = 0;
> > +
> > +	st = kzalloc(sizeof(*st), GFP_KERNEL);
> > +
> > +	if (!st) {
> > +		pr_err("%s: could not allocate signal_timing struct\n", __func__);
> > +		return NULL;
> > +	}
> > +
> > +	ret |= parse_property(np, "hback-porch", &st->hback_porch);
> > +	ret |= parse_property(np, "hfront-porch", &st->hfront_porch);
> > +	ret |= parse_property(np, "hactive", &st->hactive);
> > +	ret |= parse_property(np, "hsync-len", &st->hsync_len);
> > +	ret |= parse_property(np, "vback-porch", &st->vback_porch);
> > +	ret |= parse_property(np, "vfront-porch", &st->vfront_porch);
> > +	ret |= parse_property(np, "vactive", &st->vactive);
> > +	ret |= parse_property(np, "vsync-len", &st->vsync_len);
> > +	ret |= parse_property(np, "clock", &st->pixelclock);
> > +
> > +	st->vsync_pol_active_high = of_property_read_bool(np, "vsync-active-high");
> > +	st->hsync_pol_active_high = of_property_read_bool(np, "hsync-active-high");
> > +	st->de_pol_active_high = of_property_read_bool(np, "de-active-high");
> > +	st->pixelclk_pol_inverted = of_property_read_bool(np, "pixelclk-inverted");
> > +	st->interlaced = of_property_read_bool(np, "interlaced");
> > +	st->doublescan = of_property_read_bool(np, "doublescan");
> > +
> > +	if (ret) {
> > +		pr_err("%s: error reading timing properties\n", __func__);
> > +		return NULL;
> > +	}
> > +
> > +	return st;
> > +}
> > +EXPORT_SYMBOL_GPL(of_get_display_timing);
> > +
> > +struct display_timings *of_get_display_timing_list(struct device_node *np)
> > +{
> > +	struct device_node *timings_np;
> > +	struct device_node *entry;
> > +	struct display_timings *disp;
> > +	char *default_timing;
> > +
> > +	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);
> > +
> > +	entry = of_parse_phandle(timings_np, "default-timing", 0);
> > +
> > +	if (!entry) {
> > +		pr_info("%s: no default-timing specified\n", __func__);
> > +		entry = of_find_node_by_name(np, "timing");
> > +	}
> 
> If "default-timing" property is optional, I don't see any need for the
> pr_info above, as it should be business as usual if the property doesn't
> exist.
> 
> If the default-timing property doesn't exist, wouldn't it be simpler to
> get the first subnode, instead of looking one with "timing" name?
> 

Yes. I will fix that.

> > +
> > +	if (!entry) {
> > +		pr_info("%s: no timing specifications given\n", __func__);
> > +		return disp;
> > +	}
> 
> Again, I don't think the pr_info is needed if this is a normal case.
> Then again, perhaps this could be an error? Why would there be a display
> node without any timings?
> 
This should be an error. It is a relict from trying to define the display instead
of just the timings. I need to rework the whole no/wrong timings thing.

> > +
> > +	pr_info("%s: using %s as default timing\n", __func__, entry->name);
> > +
> > +	default_timing = (char *)entry->full_name;
> 
> const char *?
> 

> > +
> > +	disp->num_timings = 0;
> > +
> > +	for_each_child_of_node(timings_np, entry) {
> > +		disp->num_timings++;
> > +	}
> 
> No need for { }
> 

Okay.

> > +	disp->timings = kzalloc(sizeof(struct signal_timing *)*disp->num_timings,
> > +				GFP_KERNEL);
> > +
> > +	disp->num_timings = 0;
> > +
> > +	for_each_child_of_node(timings_np, entry) {
> > +		struct signal_timing *st;
> > +
> > +		st = of_get_display_timing(entry);
> > +
> > +		if (!st)
> > +			continue;
> > +
> > +		if (strcmp(default_timing, entry->full_name) = 0)
> > +			disp->default_timing = disp->num_timings;
> 
> I don't see you setting disp->default_timing to OF_DEFAULT_TIMING in
> case there's no default_timing found.
> 
> Or, at least I presume OF_DEFAULT_TIMING is meant to mark non-existing
> default timing. The name OF_DEFAULT_TIMING is not very descriptive to
> me.
> 
> Would it make more sense to have the disp->default_timing as a pointer
> to the timing, instead of index? Then a NULL value would mark a
> non-existing default timing.
> 
> > +		disp->timings[disp->num_timings] = st;
> > +		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->default_timing + 1);
> > +	else
> > +		pr_info("%s: no timings specified\n", __func__);
> > +
> > +	return disp;
> > +}
> > +EXPORT_SYMBOL_GPL(of_get_display_timing_list);
> > +
> > +int of_display_timings_exists(struct device_node *np)
> > +{
> > +	struct device_node *timings_np;
> > +	struct device_node *default_np;
> > +
> > +	if (!np)
> > +		return -EINVAL;
> > +
> > +	timings_np = of_parse_phandle(np, "display-timings", 0);
> > +
> > +	if (!timings_np)
> > +		return -EINVAL;
> > +
> > +	default_np = of_parse_phandle(np, "default-timing", 0);
> > +
> > +	if (default_np)
> > +		return 0;
> > +
> > +	return -EINVAL;
> > +}
> > +EXPORT_SYMBOL_GPL(of_display_timings_exists);
> > diff --git a/include/linux/of_display_timings.h b/include/linux/of_display_timings.h
> > new file mode 100644
> > index 0000000..1ad719e
> > --- /dev/null
> > +++ b/include/linux/of_display_timings.h
> > @@ -0,0 +1,85 @@
> > +/*
> > + * Copyright 2012 Steffen Trumtrar <s.trumtrar@pengutronix.de>
> > + *
> > + * description of display timings
> > + *
> > + * This file is released under the GPLv2
> > + */
> > +
> > +#ifndef __LINUX_OF_DISPLAY_TIMINGS_H
> > +#define __LINUX_OF_DISPLAY_TIMINGS_H
> > +
> > +#define OF_DEFAULT_TIMING -1
> > +
> > +struct display_timings {
> > +	unsigned int num_timings;
> > +	unsigned int default_timing;
> > +
> > +	struct signal_timing **timings;
> 
> Should this be a pointer to a const array of const data? Is there ever
> need to change them after they've been read from DT?
> 

No, there isn't. Will change to const.

> > +};
> > +
> > +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;
> > +
> > +	bool vsync_pol_active_high;
> > +	bool hsync_pol_active_high;
> > +	bool de_pol_active_high;
> > +	bool pixelclk_pol_inverted;
> > +	bool interlaced;
> > +	bool doublescan;
> > +};
> > +
> > +struct display_timings *of_get_display_timing_list(struct device_node *np);
> > +struct signal_timing *of_get_display_timing(struct device_node *np);
> > +int of_display_timings_exists(struct device_node *np);
> > +
> > +/* 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_timings *disp)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < disp->num_timings; i++)
> > +		kfree(disp->timings[i]);
> > +}
> > +
> > +static inline void display_timings_release(struct display_timings *disp)
> > +{
> > +	timings_release(disp);
> > +	kfree(disp->timings);
> > +}
> > +
> > +static inline struct signal_timing *display_timings_get(struct display_timings *disp,
> > +							 int index)
> > +{
> > +	struct signal_timing *st;
> > +
> > +	if (disp->num_timings > index) {
> > +		st = disp->timings[index];
> > +		return st;
> > +	}
> > +	else
> > +		return NULL;
> > +}
> 
> Why do you have these functions in the header file?
> 

Well, ... there actually isn't any reason any longer. I will move them.


Thanks and 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 1/2 v6] of: add helper to parse display timings

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 454 bytes --]

On Mon, Oct 08, 2012 at 09:49:21AM +0200, Steffen Trumtrar wrote:
> On Mon, Oct 08, 2012 at 10:07:45AM +0300, Tomi Valkeinen wrote:
> > On Thu, 2012-10-04 at 19:59 +0200, Steffen Trumtrar wrote:
[...]
> > > +
> > > +	disp->num_timings = 0;
> > > +
> > > +	for_each_child_of_node(timings_np, entry) {
> > > +		disp->num_timings++;
> > > +	}
> > 
> > No need for { }
> > 
> 
> Okay.

Or you could just use of_get_child_count().

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

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

[Steffen Trumtrar]

On Thu, Oct 11, 2012 at 09:31:18PM +0200, Thierry Reding wrote:
> On Mon, Oct 08, 2012 at 09:49:21AM +0200, Steffen Trumtrar wrote:
> > On Mon, Oct 08, 2012 at 10:07:45AM +0300, Tomi Valkeinen wrote:
> > > On Thu, 2012-10-04 at 19:59 +0200, Steffen Trumtrar wrote:
> [...]
> > > > +
> > > > +	disp->num_timings = 0;
> > > > +
> > > > +	for_each_child_of_node(timings_np, entry) {
> > > > +		disp->num_timings++;
> > > > +	}
> > > 
> > > No need for { }
> > > 
> > 
> > Okay.
> 
> Or you could just use of_get_child_count().
> 
> Thierry

Ah, very nice. That's definitely better. Didn't know about that function.

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 1/2 v6] of: add helper to parse display timings

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 905 bytes --]

On Thu, Oct 04, 2012 at 07:59:19PM +0200, Steffen Trumtrar wrote:
[...]
> diff --git a/include/linux/of_display_timings.h b/include/linux/of_display_timings.h
> new file mode 100644
> index 0000000..1ad719e
> --- /dev/null
> +++ b/include/linux/of_display_timings.h
> @@ -0,0 +1,85 @@
> +/*
> + * Copyright 2012 Steffen Trumtrar <s.trumtrar@pengutronix.de>
> + *
> + * description of display timings
> + *
> + * This file is released under the GPLv2
> + */
> +
> +#ifndef __LINUX_OF_DISPLAY_TIMINGS_H
> +#define __LINUX_OF_DISPLAY_TIMINGS_H

This file needs to include linux/slab.h because it uses kfree() in the
inline functions. Alternatively I think I'd rather see the inline
functions moved out of the header, with the exception of the
signal_timing_get_value() function perhaps.

Moreover there should be a forward declaration of struct display_node
to avoid the need to include linux/of.h.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

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

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 581 bytes --]

On Thu, Oct 04, 2012 at 07:59:19PM +0200, Steffen Trumtrar wrote:
[...]
> diff --git a/include/linux/of_display_timings.h b/include/linux/of_display_timings.h
[...]
> +struct display_timings {
> +	unsigned int num_timings;
> +	unsigned int default_timing;
> +
> +	struct signal_timing **timings;
> +};
> +
> +struct timing_entry {
> +	u32 min;
> +	u32 typ;
> +	u32 max;
> +};
> +
> +struct signal_timing {

I'm slightly confused by the naming here. signal_timing seems overly
generic in this context. Is there any reason why this isn't called
display_timing or even display_mode?


[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

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

[Steffen Trumtrar]

On Sat, Oct 20, 2012 at 09:59:51PM +0200, Thierry Reding wrote:
> On Thu, Oct 04, 2012 at 07:59:19PM +0200, Steffen Trumtrar wrote:
> [...]
> > diff --git a/include/linux/of_display_timings.h b/include/linux/of_display_timings.h
> [...]
> > +struct display_timings {
> > +	unsigned int num_timings;
> > +	unsigned int default_timing;
> > +
> > +	struct signal_timing **timings;
> > +};
> > +
> > +struct timing_entry {
> > +	u32 min;
> > +	u32 typ;
> > +	u32 max;
> > +};
> > +
> > +struct signal_timing {
> 
> I'm slightly confused by the naming here. signal_timing seems overly
> generic in this context. Is there any reason why this isn't called
> display_timing or even display_mode?
> 

You are right. I actually already changed that, for the same reasons.
It will be called display_timing in the next version, as I think that's what it really
is.

-- 
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 v6] of: add generic videomode description

[Steffen Trumtrar]

Get videomode from devicetree in a format appropriate for the
backend. drm_display_mode and fb_videomode are supported atm.
Uses the display signal timings from of_display_timings

Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
---
 drivers/of/Kconfig           |    5 +
 drivers/of/Makefile          |    1 +
 drivers/of/of_videomode.c    |  212 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/of_videomode.h |   41 ++++++++
 4 files changed, 259 insertions(+)
 create mode 100644 drivers/of/of_videomode.c
 create mode 100644 include/linux/of_videomode.h

diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
index 646deb0..74282e2 100644
--- a/drivers/of/Kconfig
+++ b/drivers/of/Kconfig
@@ -88,4 +88,9 @@ config OF_DISPLAY_TIMINGS
 	help
 	  helper to parse display timings from the devicetree
 
+config OF_VIDEOMODE
+	def_bool y
+	help
+	  helper to get videomodes from the devicetree
+
 endmenu # OF
diff --git a/drivers/of/Makefile b/drivers/of/Makefile
index c8e9603..09d556f 100644
--- a/drivers/of/Makefile
+++ b/drivers/of/Makefile
@@ -12,3 +12,4 @@ 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_TIMINGS) += of_display_timings.o
+obj-$(CONFIG_OF_VIDEOMODE) += of_videomode.o
diff --git a/drivers/of/of_videomode.c b/drivers/of/of_videomode.c
new file mode 100644
index 0000000..76ac16e
--- /dev/null
+++ b/drivers/of/of_videomode.c
@@ -0,0 +1,212 @@
+/*
+ * 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/of_display_timings.h>
+#include <linux/of_videomode.h>
+
+void dump_fb_videomode(struct fb_videomode *m)
+{
+        pr_debug("fb_videomode = %d %d %d %d %d %d %d %d %d %d %d %d %d\n",
+                m->refresh, m->xres, m->yres, 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);
+}
+
+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);
+}
+
+int videomode_from_timing(struct display_timings *disp, struct videomode *vm,
+			int index)
+{
+	struct signal_timing *st = NULL;
+
+	if (!vm)
+		return -EINVAL;
+
+	st = display_timings_get(disp, index);
+
+	if (!st) {
+		pr_err("%s: no signal timings found\n", __func__);
+		return -EINVAL;
+	}
+
+	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 = st->vsync_pol_active_high;
+	vm->hah = st->hsync_pol_active_high;
+	vm->interlaced = st->interlaced;
+	vm->doublescan = st->doublescan;
+
+	return 0;
+}
+
+int of_get_videomode(struct device_node *np, struct videomode *vm, int index)
+{
+	struct display_timings *disp;
+	int ret = 0;
+
+	disp = of_get_display_timing_list(np);
+
+	if (!disp) {
+		pr_err("%s: no timings specified\n", __func__);
+		return -EINVAL;
+	}
+
+	if (index = OF_DEFAULT_TIMING)
+		index = disp->default_timing;
+
+	ret = videomode_from_timing(disp, vm, index);
+
+	if (ret)
+		return ret;
+
+	display_timings_release(disp);
+
+	if (!vm) {
+		pr_err("%s: could not get videomode %d\n", __func__, index);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(of_get_videomode);
+
+#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->vsync_end + 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);
+
+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);
+#else
+int videomode_to_display_mode(struct videomode *vm, struct drm_display_mode *dmode)
+{
+	return 0;
+}
+
+int of_get_drm_display_mode(struct device_node *np, struct drm_display_mode *dmode,
+			int index)
+{
+	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_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_drm_display_mode);
diff --git a/include/linux/of_videomode.h b/include/linux/of_videomode.h
new file mode 100644
index 0000000..96efe01
--- /dev/null
+++ b/include/linux/of_videomode.h
@@ -0,0 +1,41 @@
+/*
+ * 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_videomode(struct device_node *np, struct videomode *vm, int index);
+int of_get_drm_display_mode(struct device_node *np, struct drm_display_mode *dmode,
+			int index);
+int of_get_fb_videomode(struct device_node *np, struct fb_videomode *fb, int index);
+#endif /* __LINUX_VIDEOMODE_H */
-- 
1.7.10.4

Re: [PATCH 2/2 v6] of: add generic videomode description

[Stephen Warren]

On 10/04/2012 11:59 AM, Steffen Trumtrar wrote:
> Get videomode from devicetree in a format appropriate for the
> backend. drm_display_mode and fb_videomode are supported atm.
> Uses the display signal timings from of_display_timings

> +++ b/drivers/of/of_videomode.c

> +int videomode_from_timing(struct display_timings *disp, struct videomode *vm,

> +	st = display_timings_get(disp, index);
> +
> +	if (!st) {

It's a little odd to leave a blank line between those two lines.

Only half of the code in this file seems OF-related; the routines to
convert a timing to a videomode or drm display mode seem like they'd be
useful outside device tree, so I wonder if putting them into
of_videomode.c is the correct thing to do. Still, it's probably not a
big deal.

Re: [PATCH 2/2 v6] of: add generic videomode description

[Steffen Trumtrar]

On Thu, Oct 04, 2012 at 12:51:00PM -0600, Stephen Warren wrote:
> On 10/04/2012 11:59 AM, Steffen Trumtrar wrote:
> > Get videomode from devicetree in a format appropriate for the
> > backend. drm_display_mode and fb_videomode are supported atm.
> > Uses the display signal timings from of_display_timings
> 
> > +++ b/drivers/of/of_videomode.c
> 
> > +int videomode_from_timing(struct display_timings *disp, struct videomode *vm,
> 
> > +	st = display_timings_get(disp, index);
> > +
> > +	if (!st) {
> 
> It's a little odd to leave a blank line between those two lines.

Hm, well okay. That can be remedied

> 
> Only half of the code in this file seems OF-related; the routines to
> convert a timing to a videomode or drm display mode seem like they'd be
> useful outside device tree, so I wonder if putting them into
> of_videomode.c is the correct thing to do. Still, it's probably not a
> big deal.
> 

I am not sure, what the appropriate way to do this is. I can split it up (again).


-- 
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 2/2 v6] of: add generic videomode description

[Laurent Pinchart]

Hi Steffen,

On Friday 05 October 2012 17:51:21 Steffen Trumtrar wrote:
> On Thu, Oct 04, 2012 at 12:51:00PM -0600, Stephen Warren wrote:
> > On 10/04/2012 11:59 AM, Steffen Trumtrar wrote:
> > > Get videomode from devicetree in a format appropriate for the
> > > backend. drm_display_mode and fb_videomode are supported atm.
> > > Uses the display signal timings from of_display_timings
> > > 
> > > +++ b/drivers/of/of_videomode.c
> > > 
> > > +int videomode_from_timing(struct display_timings *disp, struct
> > > videomode *vm,
> > > 
> > > +	st = display_timings_get(disp, index);
> > > +
> > > +	if (!st) {
> > 
> > It's a little odd to leave a blank line between those two lines.
> 
> Hm, well okay. That can be remedied
> 
> > Only half of the code in this file seems OF-related; the routines to
> > convert a timing to a videomode or drm display mode seem like they'd be
> > useful outside device tree, so I wonder if putting them into
> > of_videomode.c is the correct thing to do. Still, it's probably not a
> > big deal.
> 
> I am not sure, what the appropriate way to do this is. I can split it up
> (again).

I think it would make sense to move them to their respective subsystems.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/2 v6] of: add generic videomode description

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1921 bytes --]

On Sun, Oct 07, 2012 at 03:38:33PM +0200, Laurent Pinchart wrote:
> Hi Steffen,
> 
> On Friday 05 October 2012 17:51:21 Steffen Trumtrar wrote:
> > On Thu, Oct 04, 2012 at 12:51:00PM -0600, Stephen Warren wrote:
> > > On 10/04/2012 11:59 AM, Steffen Trumtrar wrote:
> > > > Get videomode from devicetree in a format appropriate for the
> > > > backend. drm_display_mode and fb_videomode are supported atm.
> > > > Uses the display signal timings from of_display_timings
> > > > 
> > > > +++ b/drivers/of/of_videomode.c
> > > > 
> > > > +int videomode_from_timing(struct display_timings *disp, struct
> > > > videomode *vm,
> > > > 
> > > > +	st = display_timings_get(disp, index);
> > > > +
> > > > +	if (!st) {
> > > 
> > > It's a little odd to leave a blank line between those two lines.
> > 
> > Hm, well okay. That can be remedied
> > 
> > > Only half of the code in this file seems OF-related; the routines to
> > > convert a timing to a videomode or drm display mode seem like they'd be
> > > useful outside device tree, so I wonder if putting them into
> > > of_videomode.c is the correct thing to do. Still, it's probably not a
> > > big deal.
> > 
> > I am not sure, what the appropriate way to do this is. I can split it up
> > (again).
> 
> I think it would make sense to move them to their respective subsystems.

I agree. While looking at integrating this for Tegra DRM, I came across
the issue that if I build DRM as a module, linking with this code will
fail. The reason for that was that it was that the code, itself builtin,
uses drm_mode_set_name(), which would be exported by the drm module. So
I had to modifiy the Kconfig entries to be "def_tristate DRM". That
obviously isn't very nice since the code can also be used without DRM.

Moving the subsystem specific conversion routines to the respective
subsystems should solve any of these issues.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/2 v6] of: add generic videomode description

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 9251 bytes --]

On Thu, 2012-10-04 at 19:59 +0200, Steffen Trumtrar wrote:
> Get videomode from devicetree in a format appropriate for the
> backend. drm_display_mode and fb_videomode are supported atm.
> Uses the display signal timings from of_display_timings
> 
> Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
> ---
>  drivers/of/Kconfig           |    5 +
>  drivers/of/Makefile          |    1 +
>  drivers/of/of_videomode.c    |  212 ++++++++++++++++++++++++++++++++++++++++++
>  include/linux/of_videomode.h |   41 ++++++++
>  4 files changed, 259 insertions(+)
>  create mode 100644 drivers/of/of_videomode.c
>  create mode 100644 include/linux/of_videomode.h
> 
> diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
> index 646deb0..74282e2 100644
> --- a/drivers/of/Kconfig
> +++ b/drivers/of/Kconfig
> @@ -88,4 +88,9 @@ config OF_DISPLAY_TIMINGS
>  	help
>  	  helper to parse display timings from the devicetree
>  
> +config OF_VIDEOMODE
> +	def_bool y
> +	help
> +	  helper to get videomodes from the devicetree
> +
>  endmenu # OF
> diff --git a/drivers/of/Makefile b/drivers/of/Makefile
> index c8e9603..09d556f 100644
> --- a/drivers/of/Makefile
> +++ b/drivers/of/Makefile
> @@ -12,3 +12,4 @@ 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_TIMINGS) += of_display_timings.o
> +obj-$(CONFIG_OF_VIDEOMODE) += of_videomode.o
> diff --git a/drivers/of/of_videomode.c b/drivers/of/of_videomode.c
> new file mode 100644
> index 0000000..76ac16e
> --- /dev/null
> +++ b/drivers/of/of_videomode.c
> @@ -0,0 +1,212 @@
> +/*
> + * 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/of_display_timings.h>
> +#include <linux/of_videomode.h>
> +
> +void dump_fb_videomode(struct fb_videomode *m)
> +{
> +        pr_debug("fb_videomode = %d %d %d %d %d %d %d %d %d %d %d %d %d\n",
> +                m->refresh, m->xres, m->yres, 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);
> +}
> +
> +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);
> +}
> +
> +int videomode_from_timing(struct display_timings *disp, struct videomode *vm,
> +			int index)
> +{
> +	struct signal_timing *st = NULL;
> +
> +	if (!vm)
> +		return -EINVAL;
> +
> +	st = display_timings_get(disp, index);
> +
> +	if (!st) {
> +		pr_err("%s: no signal timings found\n", __func__);
> +		return -EINVAL;
> +	}
> +
> +	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 = st->vsync_pol_active_high;
> +	vm->hah = st->hsync_pol_active_high;
> +	vm->interlaced = st->interlaced;
> +	vm->doublescan = st->doublescan;
> +
> +	return 0;
> +}
> +
> +int of_get_videomode(struct device_node *np, struct videomode *vm, int index)
> +{
> +	struct display_timings *disp;
> +	int ret = 0;
> +
> +	disp = of_get_display_timing_list(np);
> +
> +	if (!disp) {
> +		pr_err("%s: no timings specified\n", __func__);
> +		return -EINVAL;
> +	}
> +
> +	if (index == OF_DEFAULT_TIMING)
> +		index = disp->default_timing;
> +
> +	ret = videomode_from_timing(disp, vm, index);
> +
> +	if (ret)
> +		return ret;
> +
> +	display_timings_release(disp);
> +
> +	if (!vm) {
> +		pr_err("%s: could not get videomode %d\n", __func__, index);
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(of_get_videomode);
> +
> +#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->vsync_end + 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);
> +
> +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);
> +#else
> +int videomode_to_display_mode(struct videomode *vm, struct drm_display_mode *dmode)
> +{
> +	return 0;
> +}
> +
> +int of_get_drm_display_mode(struct device_node *np, struct drm_display_mode *dmode,
> +			int index)
> +{
> +	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_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_drm_display_mode);
> diff --git a/include/linux/of_videomode.h b/include/linux/of_videomode.h
> new file mode 100644
> index 0000000..96efe01
> --- /dev/null
> +++ b/include/linux/of_videomode.h
> @@ -0,0 +1,41 @@
> +/*
> + * 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>

You don't need to include this.

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

This is not really of related. And actually, neither is the struct
signal_timing in the previous patch. It would be nice to have these in a
common header that fb, drm, and others could use instead of each having
their own timing structs. 

But that's probably out of scope for this series =). Did you check the
timing structs from the video related frameworks in the kernel to see if
your structs contain all the info the others have, so that, at least in
theory, everybody could use these common structs?

 Tomi


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/2 v6] of: add generic videomode description

[Steffen Trumtrar]

On Mon, Oct 08, 2012 at 10:21:53AM +0300, Tomi Valkeinen wrote:
> On Thu, 2012-10-04 at 19:59 +0200, Steffen Trumtrar wrote:
> > Get videomode from devicetree in a format appropriate for the
> > backend. drm_display_mode and fb_videomode are supported atm.
> > Uses the display signal timings from of_display_timings
> > 
> > Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
> > ---
> >  drivers/of/Kconfig           |    5 +
> >  drivers/of/Makefile          |    1 +
> >  drivers/of/of_videomode.c    |  212 ++++++++++++++++++++++++++++++++++++++++++
> >  include/linux/of_videomode.h |   41 ++++++++
> >  4 files changed, 259 insertions(+)
> >  create mode 100644 drivers/of/of_videomode.c
> >  create mode 100644 include/linux/of_videomode.h
> > 
> > diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
> > index 646deb0..74282e2 100644
> > --- a/drivers/of/Kconfig
> > +++ b/drivers/of/Kconfig
> > @@ -88,4 +88,9 @@ config OF_DISPLAY_TIMINGS
> >  	help
> >  	  helper to parse display timings from the devicetree
> >  
> > +config OF_VIDEOMODE
> > +	def_bool y
> > +	help
> > +	  helper to get videomodes from the devicetree
> > +
> >  endmenu # OF
> > diff --git a/drivers/of/Makefile b/drivers/of/Makefile
> > index c8e9603..09d556f 100644
> > --- a/drivers/of/Makefile
> > +++ b/drivers/of/Makefile
> > @@ -12,3 +12,4 @@ 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_TIMINGS) += of_display_timings.o
> > +obj-$(CONFIG_OF_VIDEOMODE) += of_videomode.o
> > diff --git a/drivers/of/of_videomode.c b/drivers/of/of_videomode.c
> > new file mode 100644
> > index 0000000..76ac16e
> > --- /dev/null
> > +++ b/drivers/of/of_videomode.c
> > @@ -0,0 +1,212 @@
> > +/*
> > + * 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/of_display_timings.h>
> > +#include <linux/of_videomode.h>
> > +
> > +void dump_fb_videomode(struct fb_videomode *m)
> > +{
> > +        pr_debug("fb_videomode = %d %d %d %d %d %d %d %d %d %d %d %d %d\n",
> > +                m->refresh, m->xres, m->yres, 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);
> > +}
> > +
> > +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);
> > +}
> > +
> > +int videomode_from_timing(struct display_timings *disp, struct videomode *vm,
> > +			int index)
> > +{
> > +	struct signal_timing *st = NULL;
> > +
> > +	if (!vm)
> > +		return -EINVAL;
> > +
> > +	st = display_timings_get(disp, index);
> > +
> > +	if (!st) {
> > +		pr_err("%s: no signal timings found\n", __func__);
> > +		return -EINVAL;
> > +	}
> > +
> > +	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 = st->vsync_pol_active_high;
> > +	vm->hah = st->hsync_pol_active_high;
> > +	vm->interlaced = st->interlaced;
> > +	vm->doublescan = st->doublescan;
> > +
> > +	return 0;
> > +}
> > +
> > +int of_get_videomode(struct device_node *np, struct videomode *vm, int index)
> > +{
> > +	struct display_timings *disp;
> > +	int ret = 0;
> > +
> > +	disp = of_get_display_timing_list(np);
> > +
> > +	if (!disp) {
> > +		pr_err("%s: no timings specified\n", __func__);
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (index = OF_DEFAULT_TIMING)
> > +		index = disp->default_timing;
> > +
> > +	ret = videomode_from_timing(disp, vm, index);
> > +
> > +	if (ret)
> > +		return ret;
> > +
> > +	display_timings_release(disp);
> > +
> > +	if (!vm) {
> > +		pr_err("%s: could not get videomode %d\n", __func__, index);
> > +		return -EINVAL;
> > +	}
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(of_get_videomode);
> > +
> > +#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->vsync_end + 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);
> > +
> > +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);
> > +#else
> > +int videomode_to_display_mode(struct videomode *vm, struct drm_display_mode *dmode)
> > +{
> > +	return 0;
> > +}
> > +
> > +int of_get_drm_display_mode(struct device_node *np, struct drm_display_mode *dmode,
> > +			int index)
> > +{
> > +	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_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_drm_display_mode);
> > diff --git a/include/linux/of_videomode.h b/include/linux/of_videomode.h
> > new file mode 100644
> > index 0000000..96efe01
> > --- /dev/null
> > +++ b/include/linux/of_videomode.h
> > @@ -0,0 +1,41 @@
> > +/*
> > + * 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>
> 
> You don't need to include this.
> 

That is a fix to my liking. Easily done ;-)

> > +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;
> > +
> > +};
> 
> This is not really of related. And actually, neither is the struct
> signal_timing in the previous patch. It would be nice to have these in a
> common header that fb, drm, and others could use instead of each having
> their own timing structs. 
> 
> But that's probably out of scope for this series =). Did you check the
> timing structs from the video related frameworks in the kernel to see if
> your structs contain all the info the others have, so that, at least in
> theory, everybody could use these common structs?
> 
>  Tomi
> 

Yes. Stephen and Laurent already suggested to split it up.
No, all info is not contained. That starts with drm, which has width-mm,..
If time permits, I will go over that.

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 2/2 v6] of: add generic videomode description

[Laurent Pinchart]

Hi Steffen,

On Monday 08 October 2012 09:57:41 Steffen Trumtrar wrote:
> On Mon, Oct 08, 2012 at 10:21:53AM +0300, Tomi Valkeinen wrote:
> > On Thu, 2012-10-04 at 19:59 +0200, Steffen Trumtrar wrote:

[snip]

> > > diff --git a/include/linux/of_videomode.h b/include/linux/of_videomode.h
> > > new file mode 100644
> > > index 0000000..96efe01
> > > --- /dev/null
> > > +++ b/include/linux/of_videomode.h
> > > @@ -0,0 +1,41 @@
> > > +/*
> > > + * 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>
> > 
> > You don't need to include this.
> 
> That is a fix to my liking. Easily done ;-)
> 
> > > +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;
> > > +
> > > +};
> > 
> > This is not really of related. And actually, neither is the struct
> > signal_timing in the previous patch. It would be nice to have these in a
> > common header that fb, drm, and others could use instead of each having
> > their own timing structs.
> > 
> > But that's probably out of scope for this series =). Did you check the
> > timing structs from the video related frameworks in the kernel to see if
> > your structs contain all the info the others have, so that, at least in
> > theory, everybody could use these common structs?
> > 
> >  Tomi
> 
> Yes. Stephen and Laurent already suggested to split it up.
> No, all info is not contained. That starts with drm, which has width-mm,..
> If time permits, I will go over that.

Just to make sure we won't forget it, the V4L2 version of the timings 
structure is struct v4l2_bt_timings in include/linux/videodev2.h.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/2 v6] of: add generic videomode description

[Laurent Pinchart]

Hi Steffen,

Thanks for the patch.

On Thursday 04 October 2012 19:59:20 Steffen Trumtrar wrote:
> Get videomode from devicetree in a format appropriate for the
> backend. drm_display_mode and fb_videomode are supported atm.
> Uses the display signal timings from of_display_timings
> 
> Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
> ---
>  drivers/of/Kconfig           |    5 +
>  drivers/of/Makefile          |    1 +
>  drivers/of/of_videomode.c    |  212 +++++++++++++++++++++++++++++++++++++++
>  include/linux/of_videomode.h |   41 ++++++++
>  4 files changed, 259 insertions(+)
>  create mode 100644 drivers/of/of_videomode.c
>  create mode 100644 include/linux/of_videomode.h
> 
> diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
> index 646deb0..74282e2 100644
> --- a/drivers/of/Kconfig
> +++ b/drivers/of/Kconfig
> @@ -88,4 +88,9 @@ config OF_DISPLAY_TIMINGS
>  	help
>  	  helper to parse display timings from the devicetree
> 
> +config OF_VIDEOMODE
> +	def_bool y
> +	help
> +	  helper to get videomodes from the devicetree
> +
>  endmenu # OF
> diff --git a/drivers/of/Makefile b/drivers/of/Makefile
> index c8e9603..09d556f 100644
> --- a/drivers/of/Makefile
> +++ b/drivers/of/Makefile
> @@ -12,3 +12,4 @@ 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_TIMINGS) += of_display_timings.o
> +obj-$(CONFIG_OF_VIDEOMODE) += of_videomode.o
> diff --git a/drivers/of/of_videomode.c b/drivers/of/of_videomode.c
> new file mode 100644
> index 0000000..76ac16e
> --- /dev/null
> +++ b/drivers/of/of_videomode.c
> @@ -0,0 +1,212 @@
> +/*
> + * 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/of_display_timings.h>
> +#include <linux/of_videomode.h>
> +
> +void dump_fb_videomode(struct fb_videomode *m)
> +{
> +        pr_debug("fb_videomode = %d %d %d %d %d %d %d %d %d %d %d %d %d\n",

That's going to be pretty difficult to read :-) Would it make sense to group 
several attributes logically (for instance using %ux%u for m->xres, m->yres) ?

> +                m->refresh, m->xres, m->yres, 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);
> +}

Shouldn't this (and the other non exported functions below) be 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);
> +}
> +
> +int videomode_from_timing(struct display_timings *disp, struct videomode
> *vm,
> +			int index)
> +{
> +	struct signal_timing *st = NULL;
> +
> +	if (!vm)
> +		return -EINVAL;
> +

What about making vm a mandatory argument ? It looks to me like a caller bug 
if vm is NULL.

> +	st = display_timings_get(disp, index);
> +

You can remove the blank line.

> +	if (!st) {
> +		pr_err("%s: no signal timings found\n", __func__);
> +		return -EINVAL;
> +	}
> +
> +	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 = st->vsync_pol_active_high;
> +	vm->hah = st->hsync_pol_active_high;
> +	vm->interlaced = st->interlaced;
> +	vm->doublescan = st->doublescan;
> +
> +	return 0;
> +}
> +
> +int of_get_videomode(struct device_node *np, struct videomode *vm, int
> index)

I wonder how to avoid abuse of this functions. It's a useful helper for 
drivers that need to get a video mode once only, but would result in lower 
performances if a driver calls it for every mode. Drivers must call 
of_get_display_timing_list instead in that case and case the display timings. 
I'm wondering whether we should really expose of_get_videomode.

> +{
> +	struct display_timings *disp;
> +	int ret = 0;

No need to assign ret to 0 here.

> +
> +	disp = of_get_display_timing_list(np);
> +

You can remove the blank line.

> +	if (!disp) {
> +		pr_err("%s: no timings specified\n", __func__);
> +		return -EINVAL;
> +	}
> +
> +	if (index = OF_DEFAULT_TIMING)
> +		index = disp->default_timing;
> +
> +	ret = videomode_from_timing(disp, vm, index);
> +

No need for a blank line.

> +	if (ret)
> +		return ret;
> +
> +	display_timings_release(disp);
> +
> +	if (!vm) {
> +		pr_err("%s: could not get videomode %d\n", __func__, index);
> +		return -EINVAL;
> +	}

This can't happen. If vm is NULL the videomode_from_timing call above will 
return -EINVAL, and this function will then return immediately without 
reaching this code block.

> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(of_get_videomode);
> +
> +#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->vsync_end + 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);
> +
> +int of_get_drm_display_mode(struct device_node *np, struct drm_display_mode
> *dmode,
> +			int index)

Same as above, do we really need to expose this helper function ? If so we 
should at least clearly document (using kerneldoc for instance) that drivers 
should only use it if they need to get a single mode once.

> +{
> +	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);
> +#else
> +int videomode_to_display_mode(struct videomode *vm, struct drm_display_mode
> *dmode)
> +{
> +	return 0;
> +}
> +
> +int of_get_drm_display_mode(struct device_node *np, struct drm_display_mode
> *dmode,
> +			int index)
> +{
> +	return 0;
> +}

What about not defining those if CONFIG_DRM is not set ? No driver should call 
these functions in that case. If we really need those stubs they should return 
an error.

> +#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_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_drm_display_mode);
> diff --git a/include/linux/of_videomode.h b/include/linux/of_videomode.h
> new file mode 100644
> index 0000000..96efe01
> --- /dev/null
> +++ b/include/linux/of_videomode.h
> @@ -0,0 +1,41 @@
> +/*
> + * 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_videomode(struct device_node *np, struct
> videomode *vm, int index); +int of_get_drm_display_mode(struct device_node
> *np, struct drm_display_mode *dmode, +			int index);
> +int of_get_fb_videomode(struct device_node *np, struct fb_videomode *fb,
> int index); +#endif /* __LINUX_VIDEOMODE_H */
-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/2 v6] of: add generic videomode description

[Steffen Trumtrar]

On Mon, Oct 08, 2012 at 02:13:50PM +0200, Laurent Pinchart wrote:
> Hi Steffen,
> 
> Thanks for the patch.
> 
> On Thursday 04 October 2012 19:59:20 Steffen Trumtrar wrote:
> > Get videomode from devicetree in a format appropriate for the
> > backend. drm_display_mode and fb_videomode are supported atm.
> > Uses the display signal timings from of_display_timings
> > 
> > Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
> > ---
> >  drivers/of/Kconfig           |    5 +
> >  drivers/of/Makefile          |    1 +
> >  drivers/of/of_videomode.c    |  212 +++++++++++++++++++++++++++++++++++++++
> >  include/linux/of_videomode.h |   41 ++++++++
> >  4 files changed, 259 insertions(+)
> >  create mode 100644 drivers/of/of_videomode.c
> >  create mode 100644 include/linux/of_videomode.h
> > 
> > diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
> > index 646deb0..74282e2 100644
> > --- a/drivers/of/Kconfig
> > +++ b/drivers/of/Kconfig
> > @@ -88,4 +88,9 @@ config OF_DISPLAY_TIMINGS
> >  	help
> >  	  helper to parse display timings from the devicetree
> > 
> > +config OF_VIDEOMODE
> > +	def_bool y
> > +	help
> > +	  helper to get videomodes from the devicetree
> > +
> >  endmenu # OF
> > diff --git a/drivers/of/Makefile b/drivers/of/Makefile
> > index c8e9603..09d556f 100644
> > --- a/drivers/of/Makefile
> > +++ b/drivers/of/Makefile
> > @@ -12,3 +12,4 @@ 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_TIMINGS) += of_display_timings.o
> > +obj-$(CONFIG_OF_VIDEOMODE) += of_videomode.o
> > diff --git a/drivers/of/of_videomode.c b/drivers/of/of_videomode.c
> > new file mode 100644
> > index 0000000..76ac16e
> > --- /dev/null
> > +++ b/drivers/of/of_videomode.c
> > @@ -0,0 +1,212 @@
> > +/*
> > + * 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/of_display_timings.h>
> > +#include <linux/of_videomode.h>
> > +
> > +void dump_fb_videomode(struct fb_videomode *m)
> > +{
> > +        pr_debug("fb_videomode = %d %d %d %d %d %d %d %d %d %d %d %d %d\n",
> 
> That's going to be pretty difficult to read :-) Would it make sense to group 
> several attributes logically (for instance using %ux%u for m->xres, m->yres) ?
> 

No problem. That can be changed.

> > +                m->refresh, m->xres, m->yres, 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);
> > +}
> 
> Shouldn't this (and the other non exported functions below) be static ?
> 

Yes.

> > +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);
> > +}
> > +
> > +int videomode_from_timing(struct display_timings *disp, struct videomode
> > *vm,
> > +			int index)
> > +{
> > +	struct signal_timing *st = NULL;
> > +
> > +	if (!vm)
> > +		return -EINVAL;
> > +
> 
> What about making vm a mandatory argument ? It looks to me like a caller bug 
> if vm is NULL.
> 

The caller must provide the struct videomode, yes. Wouldn't the kernel hang itself
with a NULL pointer exception, if I just work with it ?

> > +	st = display_timings_get(disp, index);
> > +
> 
> You can remove the blank line.
> 
> > +	if (!st) {
> > +		pr_err("%s: no signal timings found\n", __func__);
> > +		return -EINVAL;
> > +	}
> > +
> > +	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 = st->vsync_pol_active_high;
> > +	vm->hah = st->hsync_pol_active_high;
> > +	vm->interlaced = st->interlaced;
> > +	vm->doublescan = st->doublescan;
> > +
> > +	return 0;
> > +}
> > +
> > +int of_get_videomode(struct device_node *np, struct videomode *vm, int
> > index)
> 
> I wonder how to avoid abuse of this functions. It's a useful helper for 
> drivers that need to get a video mode once only, but would result in lower 
> performances if a driver calls it for every mode. Drivers must call 
> of_get_display_timing_list instead in that case and case the display timings. 
> I'm wondering whether we should really expose of_get_videomode.
> 

The intent was to let the driver decide. That way all the other overhead may
be skipped.

> > +{
> > +	struct display_timings *disp;
> > +	int ret = 0;
> 
> No need to assign ret to 0 here.
> 

Ah, yes. Unneeded in this case.

> > +
> > +	disp = of_get_display_timing_list(np);
> > +
> 
> You can remove the blank line.
> 
> > +	if (!disp) {
> > +		pr_err("%s: no timings specified\n", __func__);
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (index = OF_DEFAULT_TIMING)
> > +		index = disp->default_timing;
> > +
> > +	ret = videomode_from_timing(disp, vm, index);
> > +
> 
> No need for a blank line.
> 
> > +	if (ret)
> > +		return ret;
> > +
> > +	display_timings_release(disp);
> > +
> > +	if (!vm) {
> > +		pr_err("%s: could not get videomode %d\n", __func__, index);
> > +		return -EINVAL;
> > +	}
> 
> This can't happen. If vm is NULL the videomode_from_timing call above will 
> return -EINVAL, and this function will then return immediately without 
> reaching this code block.
> 

Okay.

> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(of_get_videomode);
> > +
> > +#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->vsync_end + 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);
> > +
> > +int of_get_drm_display_mode(struct device_node *np, struct drm_display_mode
> > *dmode,
> > +			int index)
> 
> Same as above, do we really need to expose this helper function ? If so we 
> should at least clearly document (using kerneldoc for instance) that drivers 
> should only use it if they need to get a single mode once.
> 
> > +{
> > +	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);
> > +#else
> > +int videomode_to_display_mode(struct videomode *vm, struct drm_display_mode
> > *dmode)
> > +{
> > +	return 0;
> > +}
> > +
> > +int of_get_drm_display_mode(struct device_node *np, struct drm_display_mode
> > *dmode,
> > +			int index)
> > +{
> > +	return 0;
> > +}
> 
> What about not defining those if CONFIG_DRM is not set ? No driver should call 
> these functions in that case. If we really need those stubs they should return 
> an error.
> 

Okay. I will remove them.

> > +#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_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_drm_display_mode);
> > diff --git a/include/linux/of_videomode.h b/include/linux/of_videomode.h
> > new file mode 100644
> > index 0000000..96efe01
> > --- /dev/null
> > +++ b/include/linux/of_videomode.h
> > @@ -0,0 +1,41 @@
> > +/*
> > + * 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_videomode(struct device_node *np, struct
> > videomode *vm, int index); +int of_get_drm_display_mode(struct device_node
> > *np, struct drm_display_mode *dmode, +			int index);
> > +int of_get_fb_videomode(struct device_node *np, struct fb_videomode *fb,
> > int index); +#endif /* __LINUX_VIDEOMODE_H */
> -- 
> Regards,
> 
> Laurent Pinchart
> 


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 2/2 v6] of: add generic videomode description

[Laurent Pinchart]

Hi Steffen,

On Monday 08 October 2012 14:48:01 Steffen Trumtrar wrote:
> On Mon, Oct 08, 2012 at 02:13:50PM +0200, Laurent Pinchart wrote:
> > On Thursday 04 October 2012 19:59:20 Steffen Trumtrar wrote:
> > > Get videomode from devicetree in a format appropriate for the
> > > backend. drm_display_mode and fb_videomode are supported atm.
> > > Uses the display signal timings from of_display_timings
> > > 
> > > Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
> > > ---
> > > 
> > >  drivers/of/Kconfig           |    5 +
> > >  drivers/of/Makefile          |    1 +
> > >  drivers/of/of_videomode.c    |  212 +++++++++++++++++++++++++++++++++++
> > >  include/linux/of_videomode.h |   41 ++++++++
> > >  4 files changed, 259 insertions(+)
> > >  create mode 100644 drivers/of/of_videomode.c
> > >  create mode 100644 include/linux/of_videomode.h

[snip]

> > > diff --git a/drivers/of/of_videomode.c b/drivers/of/of_videomode.c
> > > new file mode 100644
> > > index 0000000..76ac16e
> > > --- /dev/null
> > > +++ b/drivers/of/of_videomode.c

[snip]

> > > +int videomode_from_timing(struct display_timings *disp, struct
> > > videomode *vm,
> > > +			int index)
> > > +{
> > > +	struct signal_timing *st = NULL;
> > > +
> > > +	if (!vm)
> > > +		return -EINVAL;
> > > +
> > 
> > What about making vm a mandatory argument ? It looks to me like a caller
> > bug if vm is NULL.
> 
> The caller must provide the struct videomode, yes. Wouldn't the kernel hang
> itself with a NULL pointer exception, if I just work with it ?

The kernel would oops, clearly showing the caller that a non-null vm is needed 
:-)

> > > +	st = display_timings_get(disp, index);
> > > +
> > 
> > You can remove the blank line.
> > 
> > > +	if (!st) {
> > > +		pr_err("%s: no signal timings found\n", __func__);
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	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 = st->vsync_pol_active_high;
> > > +	vm->hah = st->hsync_pol_active_high;
> > > +	vm->interlaced = st->interlaced;
> > > +	vm->doublescan = st->doublescan;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +int of_get_videomode(struct device_node *np, struct videomode *vm, int
> > > index)
> > 
> > I wonder how to avoid abuse of this functions. It's a useful helper for
> > drivers that need to get a video mode once only, but would result in lower
> > performances if a driver calls it for every mode. Drivers must call
> > of_get_display_timing_list instead in that case and case the display
> > timings. I'm wondering whether we should really expose of_get_videomode.
> 
> The intent was to let the driver decide. That way all the other overhead may
> be skipped.

My point is that driver writers might just call of_get_videomode() in a loop, 
not knowing that it's expensive. I want to avoid that. We need to at least add 
kerneldoc to the function stating that this shouldn't be done.

> > > +{
> > > +	struct display_timings *disp;
> > > +	int ret = 0;
> > 
> > No need to assign ret to 0 here.
> 
> Ah, yes. Unneeded in this case.
> 
> > > +
> > > +	disp = of_get_display_timing_list(np);
> > > +
> > 
> > You can remove the blank line.
> > 
> > > +	if (!disp) {
> > > +		pr_err("%s: no timings specified\n", __func__);
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	if (index = OF_DEFAULT_TIMING)
> > > +		index = disp->default_timing;
> > > +
> > > +	ret = videomode_from_timing(disp, vm, index);
> > > +
> > 
> > No need for a blank line.
> > 
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	display_timings_release(disp);
> > > +
> > > +	if (!vm) {
> > > +		pr_err("%s: could not get videomode %d\n", __func__, index);
> > > +		return -EINVAL;
> > > +	}
> > 
> > This can't happen. If vm is NULL the videomode_from_timing call above will
> > return -EINVAL, and this function will then return immediately without
> > reaching this code block.
> 
> Okay.
> 
> > > +
> > > +	return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(of_get_videomode);

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/2 v6] of: add generic videomode description

[Steffen Trumtrar]

Hi Laurent,

On Mon, Oct 08, 2012 at 10:52:04PM +0200, Laurent Pinchart wrote:
> Hi Steffen,
> 
> On Monday 08 October 2012 14:48:01 Steffen Trumtrar wrote:
> > On Mon, Oct 08, 2012 at 02:13:50PM +0200, Laurent Pinchart wrote:
> > > On Thursday 04 October 2012 19:59:20 Steffen Trumtrar wrote:
> > > > Get videomode from devicetree in a format appropriate for the
> > > > backend. drm_display_mode and fb_videomode are supported atm.
> > > > Uses the display signal timings from of_display_timings
> > > > 
> > > > Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
> > > > ---
> > > > 
> > > >  drivers/of/Kconfig           |    5 +
> > > >  drivers/of/Makefile          |    1 +
> > > >  drivers/of/of_videomode.c    |  212 +++++++++++++++++++++++++++++++++++
> > > >  include/linux/of_videomode.h |   41 ++++++++
> > > >  4 files changed, 259 insertions(+)
> > > >  create mode 100644 drivers/of/of_videomode.c
> > > >  create mode 100644 include/linux/of_videomode.h
> 
> [snip]
> 
> > > > diff --git a/drivers/of/of_videomode.c b/drivers/of/of_videomode.c
> > > > new file mode 100644
> > > > index 0000000..76ac16e
> > > > --- /dev/null
> > > > +++ b/drivers/of/of_videomode.c
> 
> [snip]
> 
> > > > +int videomode_from_timing(struct display_timings *disp, struct
> > > > videomode *vm,
> > > > +			int index)
> > > > +{
> > > > +	struct signal_timing *st = NULL;
> > > > +
> > > > +	if (!vm)
> > > > +		return -EINVAL;
> > > > +
> > > 
> > > What about making vm a mandatory argument ? It looks to me like a caller
> > > bug if vm is NULL.
> > 
> > The caller must provide the struct videomode, yes. Wouldn't the kernel hang
> > itself with a NULL pointer exception, if I just work with it ?
> 
> The kernel would oops, clearly showing the caller that a non-null vm is needed 
> :-)
> 

Okay. No error checking it is then.

> > > > +	st = display_timings_get(disp, index);
> > > > +
> > > 
> > > You can remove the blank line.
> > > 
> > > > +	if (!st) {
> > > > +		pr_err("%s: no signal timings found\n", __func__);
> > > > +		return -EINVAL;
> > > > +	}
> > > > +
> > > > +	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 = st->vsync_pol_active_high;
> > > > +	vm->hah = st->hsync_pol_active_high;
> > > > +	vm->interlaced = st->interlaced;
> > > > +	vm->doublescan = st->doublescan;
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +int of_get_videomode(struct device_node *np, struct videomode *vm, int
> > > > index)
> > > 
> > > I wonder how to avoid abuse of this functions. It's a useful helper for
> > > drivers that need to get a video mode once only, but would result in lower
> > > performances if a driver calls it for every mode. Drivers must call
> > > of_get_display_timing_list instead in that case and case the display
> > > timings. I'm wondering whether we should really expose of_get_videomode.
> > 
> > The intent was to let the driver decide. That way all the other overhead may
> > be skipped.
> 
> My point is that driver writers might just call of_get_videomode() in a loop, 
> not knowing that it's expensive. I want to avoid that. We need to at least add 
> kerneldoc to the function stating that this shouldn't be done.
> 

You're right. That should be made clear in the code.

> > > > +{
> > > > +	struct display_timings *disp;
> > > > +	int ret = 0;
> > > 
> > > No need to assign ret to 0 here.
> > 
> > Ah, yes. Unneeded in this case.
> > 
> > > > +
> > > > +	disp = of_get_display_timing_list(np);
> > > > +
> > > 
> > > You can remove the blank line.
> > > 
> > > > +	if (!disp) {
> > > > +		pr_err("%s: no timings specified\n", __func__);
> > > > +		return -EINVAL;
> > > > +	}
> > > > +
> > > > +	if (index = OF_DEFAULT_TIMING)
> > > > +		index = disp->default_timing;
> > > > +
> > > > +	ret = videomode_from_timing(disp, vm, index);
> > > > +
> > > 
> > > No need for a blank line.
> > > 
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	display_timings_release(disp);
> > > > +
> > > > +	if (!vm) {
> > > > +		pr_err("%s: could not get videomode %d\n", __func__, index);
> > > > +		return -EINVAL;
> > > > +	}
> > > 
> > > This can't happen. If vm is NULL the videomode_from_timing call above will
> > > return -EINVAL, and this function will then return immediately without
> > > reaching this code block.
> > 
> > Okay.
> > 
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(of_get_videomode);
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> 

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 2/2 v6] of: add generic videomode description

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1676 bytes --]

On Tue, Oct 09, 2012 at 09:26:08AM +0200, Steffen Trumtrar wrote:
> Hi Laurent,
> 
> On Mon, Oct 08, 2012 at 10:52:04PM +0200, Laurent Pinchart wrote:
> > Hi Steffen,
> > 
> > On Monday 08 October 2012 14:48:01 Steffen Trumtrar wrote:
> > > On Mon, Oct 08, 2012 at 02:13:50PM +0200, Laurent Pinchart wrote:
> > > > On Thursday 04 October 2012 19:59:20 Steffen Trumtrar wrote:
[...]
> > > > > +int of_get_videomode(struct device_node *np, struct videomode *vm, int
> > > > > index)
> > > > 
> > > > I wonder how to avoid abuse of this functions. It's a useful helper for
> > > > drivers that need to get a video mode once only, but would result in lower
> > > > performances if a driver calls it for every mode. Drivers must call
> > > > of_get_display_timing_list instead in that case and case the display
> > > > timings. I'm wondering whether we should really expose of_get_videomode.
> > > 
> > > The intent was to let the driver decide. That way all the other overhead may
> > > be skipped.
> > 
> > My point is that driver writers might just call of_get_videomode() in a loop, 
> > not knowing that it's expensive. I want to avoid that. We need to at least add 
> > kerneldoc to the function stating that this shouldn't be done.
> > 
> 
> You're right. That should be made clear in the code.

In that case we should export videomode_from_timing(). For Tegra DRM I
wrote a small utility function that takes a struct display_timings and
converts every timing to a struct videomode which is then converted to
a struct drm_display_mode and added to the DRM connector. The code is
really generic and could be part of the DRM core.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/2 v6] of: add generic videomode description

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1160 bytes --]

On Thu, Oct 04, 2012 at 07:59:20PM +0200, Steffen Trumtrar wrote:
[...]
> diff --git a/drivers/of/of_videomode.c b/drivers/of/of_videomode.c
[...]
> +#if defined(CONFIG_DRM)

This should be:

	#if IS_ENABLED(CONFIG_DRM)

or the code below won't be included if DRM is built as a module. But see
my other replies as to how we can probably handle this better by moving
this into the DRM subsystem.

> +int videomode_to_display_mode(struct videomode *vm, struct drm_display_mode *dmode)
> +{
> +	memset(dmode, 0, sizeof(*dmode));

It appears the usual method to obtain a drm_display_mode to allocate it
using drm_mode_create(), which will allocate it and associate it with
the struct drm_device.

Now, if you do a memset() on the structure you'll overwrite a number of
fields that have previously been initialized and are actually required
to get everything cleaned up properly later on.

So I think we should remove the call to memset().

> +int of_get_fb_videomode(struct device_node *np, struct fb_videomode *fb,
> +			int index)
> +{
[...]
> +}
> +EXPORT_SYMBOL_GPL(of_get_drm_display_mode);

This should be:

	EXPORT_SYMBOL_GPL(of_get_fb_videomode);

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/2 v6] of: add generic videomode description

[Steffen Trumtrar]

On Sat, Oct 20, 2012 at 01:04:12PM +0200, Thierry Reding wrote:
> On Thu, Oct 04, 2012 at 07:59:20PM +0200, Steffen Trumtrar wrote:
> [...]
> > diff --git a/drivers/of/of_videomode.c b/drivers/of/of_videomode.c
> [...]
> > +#if defined(CONFIG_DRM)
> 
> This should be:
> 
> 	#if IS_ENABLED(CONFIG_DRM)
> 
> or the code below won't be included if DRM is built as a module. But see
> my other replies as to how we can probably handle this better by moving
> this into the DRM subsystem.
> 

I already started with moving...now I only need some time to finish with it.

> > +int videomode_to_display_mode(struct videomode *vm, struct drm_display_mode *dmode)
> > +{
> > +	memset(dmode, 0, sizeof(*dmode));
> 
> It appears the usual method to obtain a drm_display_mode to allocate it
> using drm_mode_create(), which will allocate it and associate it with
> the struct drm_device.
> 
> Now, if you do a memset() on the structure you'll overwrite a number of
> fields that have previously been initialized and are actually required
> to get everything cleaned up properly later on.
> 
> So I think we should remove the call to memset().
> 

I was not aware of that. The memset has to go than, of course.

> > +int of_get_fb_videomode(struct device_node *np, struct fb_videomode *fb,
> > +			int index)
> > +{
> [...]
> > +}
> > +EXPORT_SYMBOL_GPL(of_get_drm_display_mode);
> 
> This should be:
> 
> 	EXPORT_SYMBOL_GPL(of_get_fb_videomode);

Oops.

Regrads,

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 0/2 v6] of: add display helper

[Leela Krishna Amudala]

Hello Steffen,

To which version of the kernel we can expect this patch set to be merged into?
Because I'm waiting for this from long time to add DT support for my
display controller :)

Best Wishes,
Leela Krishna Amudala.

On Thu, Oct 4, 2012 at 11:29 PM, Steffen Trumtrar
<s.trumtrar@pengutronix.de> wrote:
>
> Hi!
>
> In accordance with Stepehn Warren, I downsized the binding.
> Now, just the display-timing is described, as I think, it is way easier to
> agree
> on those and have a complete binding.
>
> Regards,
> Steffen
>
> Steffen Trumtrar (2):
>   of: add helper to parse display timings
>   of: add generic videomode description
>
>  .../devicetree/bindings/video/display-timings.txt  |  222
> ++++++++++++++++++++
>  drivers/of/Kconfig                                 |   10 +
>  drivers/of/Makefile                                |    2 +
>  drivers/of/of_display_timings.c                    |  183
> ++++++++++++++++
>  drivers/of/of_videomode.c                          |  212
> +++++++++++++++++++
>  include/linux/of_display_timings.h                 |   85 ++++++++
>  include/linux/of_videomode.h                       |   41 ++++
>  7 files changed, 755 insertions(+)
>  create mode 100644
> Documentation/devicetree/bindings/video/display-timings.txt
>  create mode 100644 drivers/of/of_display_timings.c
>  create mode 100644 drivers/of/of_videomode.c
>  create mode 100644 include/linux/of_display_timings.h
>  create mode 100644 include/linux/of_videomode.h
>
> --
> 1.7.10.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/2 v6] of: add display helper

[Steffen Trumtrar]

Hi Leela,

On Mon, Oct 15, 2012 at 04:24:43PM +0530, Leela Krishna Amudala wrote:
> Hello Steffen,
> 
> To which version of the kernel we can expect this patch set to be merged into?
> Because I'm waiting for this from long time to add DT support for my
> display controller :)
> 

I have no idea, sorry. It seems like we have almost settled with the binding
(clock-name needs to be changed), but I'm not responsible for any merging/inclusions
in the kernel.

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 0/2 v6] of: add display helper

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 705 bytes --]

On Mon, Oct 15, 2012 at 04:17:51PM +0200, Steffen Trumtrar wrote:
> Hi Leela,
> 
> On Mon, Oct 15, 2012 at 04:24:43PM +0530, Leela Krishna Amudala wrote:
> > Hello Steffen,
> > 
> > To which version of the kernel we can expect this patch set to be merged into?
> > Because I'm waiting for this from long time to add DT support for my
> > display controller :)
> > 
> 
> I have no idea, sorry. It seems like we have almost settled with the binding
> (clock-name needs to be changed), but I'm not responsible for any merging/inclusions
> in the kernel.

I want to use this in the Tegra DRM driver which I hope to get into 3.8.
If you need any help with this, please let me know.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]