Hi,

On 2012-11-20 17:54, Steffen Trumtrar wrote:

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

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

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

Sorry for reviewing this so late.

One thing I'd like to see is some explanation of the structs involved. For example, in this patch you present structs videomode, display_timing and display_timings without giving any hint what they represent.

I'm not asking for you to write a long documentation, but perhaps the header files could include a few lines of comments above the structs, explaining the idea.

+void display_timings_release(struct display_timings *disp) +{

• if (disp->timings) {

• unsigned int i;
  

• for (i = 0; i < disp->num_timings; i++)
  

• 	kfree(disp->timings[i]);
  

• kfree(disp->timings);
  

• }
• kfree(disp);

+} +EXPORT_SYMBOL_GPL(display_timings_release);

Perhaps this will become clearer after reading the following patches, but it feels a bit odd to add a release function, without anything in this patch that would actually allocate the timings.

diff --git a/drivers/video/videomode.c b/drivers/video/videomode.c new file mode 100644 index 0000000..e24f879 --- /dev/null +++ b/drivers/video/videomode.c @@ -0,0 +1,46 @@ +/*

• • generic display timing functions
• • Copyright (c) 2012 Steffen Trumtrar s.trumtrar@pengutronix.de, Pengutronix
• • This file is released under the GPLv2
• */

+#include <linux/export.h> +#include <linux/errno.h> +#include <linux/display_timing.h> +#include <linux/kernel.h> +#include <linux/videomode.h>

+int videomode_from_timing(const struct display_timings *disp,

• 	  struct videomode *vm, unsigned int index)
  

+{

• struct display_timing *dt;
• dt = display_timings_get(disp, index);
• if (!dt)

• return -EINVAL;
  

• vm->pixelclock = display_timing_get_value(&dt->pixelclock, 0);
• vm->hactive = display_timing_get_value(&dt->hactive, 0);
• vm->hfront_porch = display_timing_get_value(&dt->hfront_porch, 0);
• vm->hback_porch = display_timing_get_value(&dt->hback_porch, 0);
• vm->hsync_len = display_timing_get_value(&dt->hsync_len, 0);
• vm->vactive = display_timing_get_value(&dt->vactive, 0);
• vm->vfront_porch = display_timing_get_value(&dt->vfront_porch, 0);
• vm->vback_porch = display_timing_get_value(&dt->vback_porch, 0);
• vm->vsync_len = display_timing_get_value(&dt->vsync_len, 0);

Shouldn't all these calls get the typical value, with index 1?

• vm->vah = dt->vsync_pol_active;
• vm->hah = dt->hsync_pol_active;
• vm->de = dt->de_pol_active;
• vm->pixelclk_pol = dt->pixelclk_pol;
• vm->interlaced = dt->interlaced;
• vm->doublescan = dt->doublescan;
• return 0;

+}

+EXPORT_SYMBOL_GPL(videomode_from_timing); diff --git a/include/linux/display_timing.h b/include/linux/display_timing.h new file mode 100644 index 0000000..d5bf03f --- /dev/null +++ b/include/linux/display_timing.h @@ -0,0 +1,70 @@ +/*

• • Copyright 2012 Steffen Trumtrar s.trumtrar@pengutronix.de
• • description of display timings
• • This file is released under the GPLv2
• */

+#ifndef __LINUX_DISPLAY_TIMINGS_H +#define __LINUX_DISPLAY_TIMINGS_H

+#include <linux/types.h>

What is this needed for? u32? I don't see it defined in types.h

+struct timing_entry {

• u32 min;
• u32 typ;
• u32 max;

+};

+struct display_timing {

• struct timing_entry pixelclock;
• struct timing_entry hactive;
• struct timing_entry hfront_porch;
• struct timing_entry hback_porch;
• struct timing_entry hsync_len;
• struct timing_entry vactive;
• struct timing_entry vfront_porch;
• struct timing_entry vback_porch;
• struct timing_entry vsync_len;
• unsigned int vsync_pol_active;
• unsigned int hsync_pol_active;
• unsigned int de_pol_active;
• unsigned int pixelclk_pol;
• bool interlaced;
• bool doublescan;

+};

+struct display_timings {

• unsigned int num_timings;
• unsigned int native_mode;
• struct display_timing **timings;

+};

+/*

• • placeholder function until ranges are really needed
• • the index parameter should then be used to select one of [min typ max]
• */

+static inline u32 display_timing_get_value(const struct timing_entry *te,

• 			   unsigned int index)
  

+{

• return te->typ;

+}

Why did you opt for a placeholder here? It feels trivial to me to have support to get the min/typ/max value properly.

+static inline struct display_timing *display_timings_get(const struct

• 					 display_timings *disp,
  

• 					 unsigned int index)
  

+{

• if (disp->num_timings > index)

• return disp->timings[index];
  

• else

• return NULL;
  

+}

+void display_timings_release(struct display_timings *disp);

+#endif diff --git a/include/linux/videomode.h b/include/linux/videomode.h new file mode 100644 index 0000000..5d3e796 --- /dev/null +++ b/include/linux/videomode.h @@ -0,0 +1,40 @@ +/*

• • Copyright 2012 Steffen Trumtrar s.trumtrar@pengutronix.de
• • generic videomode description
• • This file is released under the GPLv2
• */

+#ifndef __LINUX_VIDEOMODE_H +#define __LINUX_VIDEOMODE_H

+#include <linux/display_timing.h>

This is not needed, just add:

struct display_timings;

+struct videomode {

• u32 pixelclock;
• u32 refreshrate;
• u32 hactive;
• u32 hfront_porch;
• u32 hback_porch;
• u32 hsync_len;
• u32 vactive;
• u32 vfront_porch;
• u32 vback_porch;
• u32 vsync_len;
• u32 hah;
• u32 vah;
• u32 de;
• u32 pixelclk_pol;
• bool interlaced;
• bool doublescan;

+};

+int videomode_from_timing(const struct display_timings *disp,

• 	  struct videomode *vm, unsigned int index);
  

Are this and the few other functions above meant to be used from drivers? If so, some explanation of the parameters here would be nice. If they are just framework internal, they don't probably need that.

Tomi

Hi Steffen,

On Tue, Nov 20, 2012 at 21:24:52, Steffen Trumtrar wrote:

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

Signed-off-by: Steffen Trumtrar s.trumtrar@pengutronix.de Signed-off-by: Philipp Zabel p.zabel@pengutronix.de Acked-by: Stephen Warren swarren@nvidia.com Reviewed-by: Thierry Reding thierry.reding@avionic-design.de Acked-by: Thierry Reding thierry.reding@avionic-design.de Tested-by: Thierry Reding thierry.reding@avionic-design.de Tested-by: Philipp Zabel p.zabel@pengutronix.de Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com

---

.../devicetree/bindings/video/display-timings.txt | 107 ++++++++++ drivers/video/Kconfig | 13 ++ drivers/video/Makefile | 2 + drivers/video/of_display_timing.c | 216 ++++++++++++++++++++ drivers/video/of_videomode.c | 48 +++++ include/linux/of_display_timings.h | 20 ++ include/linux/of_videomode.h | 18 ++ 7 files changed, 424 insertions(+) create mode 100644 Documentation/devicetree/bindings/video/display-timings.txt create mode 100644 drivers/video/of_display_timing.c create mode 100644 drivers/video/of_videomode.c create mode 100644 include/linux/of_display_timings.h create mode 100644 include/linux/of_videomode.h

diff --git a/Documentation/devicetree/bindings/video/display-timings.txt b/Documentation/devicetree/bindings/video/display-timings.txt new file mode 100644 index 0000000..a05cade --- /dev/null +++ b/Documentation/devicetree/bindings/video/display-timings.txt @@ -0,0 +1,107 @@ +display-timings bindings +========================

+display-timings node +--------------------

+required properties:

• • none

+optional properties:

• • native-mode: The native mode for the display, in case multiple modes are

• provided. When omitted, assume the first node is the native.
  

+timings subnode +---------------

+required properties:

• • hactive, vactive: Display resolution
• • hfront-porch, hback-porch, hsync-len: Horizontal Display timing parameters
• in pixels
• vfront-porch, vback-porch, vsync-len: Vertical display timing parameters in
• lines
• • clock-frequency: display clock in Hz

+optional properties:

• • hsync-active: Hsync pulse is active low/high/ignored
• • vsync-active: Vsync pulse is active low/high/ignored
• • de-active: Data-Enable pulse is active low/high/ignored
• • pixelclk-inverted: pixelclock is inverted/non-inverted/ignored
• • interlaced (bool)
• • doublescan (bool)

+All the optional properties that are not bool follow the following logic:

• <1>: high active
• <0>: low active
• omitted: not used on hardware

+There are different ways of describing the capabilities of a display. The devicetree +representation corresponds to the one commonly found in datasheets for displays. +If a display supports multiple signal timings, the native-mode can be specified.

+The parameters are defined as

+struct display_timing +=====================

• +----------+---------------------------------------------+----------+-------+
• | | ↑ | | |
• | | |vback_porch | | |
• | | ↓ | | |
• +----------###############################################----------+-------+
• | # ↑ # | |
• | # | # | |
• | hback # | # hfront | hsync |
• | porch # | hactive # porch | len |
• |<-------->#<---------------+--------------------------->#<-------->|<----->|
• | # | # | |
• | # |vactive # | |
• | # | # | |
• | # ↓ # | |
• +----------###############################################----------+-------+
• | | ↑ | | |
• | | |vfront_porch | | |
• | | ↓ | | |
• +----------+---------------------------------------------+----------+-------+
• | | ↑ | | |
• | | |vsync_len | | |
• | | ↓ | | |
• +----------+---------------------------------------------+----------+-------+

+Example:

• display-timings {

• native-mode = <&timing0>;
  

• timing0: 1920p24 {
  

• 	/* 1920x1080p24 */
  

• 	clock-frequency = <52000000>;
  

• 	hactive = <1920>;
  

• 	vactive = <1080>;
  

• 	hfront-porch = <25>;
  

• 	hback-porch = <25>;
  

• 	hsync-len = <25>;
  

• 	vback-porch = <2>;
  

• 	vfront-porch = <2>;
  

• 	vsync-len = <2>;
  

• 	hsync-active = <1>;
  

• };
  

• };

+Every required property also supports the use of ranges, so the commonly used +datasheet description with <min typ max>-tuples can be used.

+Example:

• timing1: timing {

• /* 1920x1080p24 */
  

• clock-frequency = <148500000>;
  

• hactive = <1920>;
  

• vactive = <1080>;
  

• hsync-len = <0 44 60>;
  

• hfront-porch = <80 88 95>;
  

• hback-porch = <100 148 160>;
  

• vfront-porch = <0 4 6>;
  

• vback-porch = <0 36 50>;
  

• vsync-len = <0 5 6>;
  

• };

diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig index 2a23b18..807fedd 100644 --- a/drivers/video/Kconfig +++ b/drivers/video/Kconfig @@ -39,6 +39,19 @@ config DISPLAY_TIMING config VIDEOMODE bool

+config OF_DISPLAY_TIMING

• bool "Enable OF display timing support"
• select DISPLAY_TIMING
• help

•  helper to parse display timings from the devicetree
  

+config OF_VIDEOMODE

• bool "Enable OF videomode support"
• select VIDEOMODE
• select OF_DISPLAY_TIMING
• help

•  helper to get videomodes from the devicetree
  

menuconfig FB tristate "Support for frame buffer devices" ---help--- diff --git a/drivers/video/Makefile b/drivers/video/Makefile index fc30439..b936b00 100644 --- a/drivers/video/Makefile +++ b/drivers/video/Makefile @@ -168,4 +168,6 @@ obj-$(CONFIG_FB_VIRTUAL) += vfb.o #video output switch sysfs driver obj-$(CONFIG_VIDEO_OUTPUT_CONTROL) += output.o obj-$(CONFIG_DISPLAY_TIMING) += display_timing.o +obj-$(CONFIG_OF_DISPLAY_TIMING) += of_display_timing.o obj-$(CONFIG_VIDEOMODE) += videomode.o +obj-$(CONFIG_OF_VIDEOMODE) += of_videomode.o diff --git a/drivers/video/of_display_timing.c b/drivers/video/of_display_timing.c new file mode 100644 index 0000000..3eb731f --- /dev/null +++ b/drivers/video/of_display_timing.c @@ -0,0 +1,216 @@ +/*

• • OF helpers for parsing display timings
• • Copyright (c) 2012 Steffen Trumtrar s.trumtrar@pengutronix.de, Pengutronix
• • based on of_videomode.c by Sascha Hauer s.hauer@pengutronix.de
• • This file is released under the GPLv2
• */

+#include <linux/of.h> +#include <linux/slab.h> +#include <linux/export.h> +#include <linux/of_display_timings.h>

+/**

• • parse_property - parse timing_entry from device_node
• • @np: device_node with the property
• • @name: name of the property
• • @result: will be set to the return value
• • DESCRIPTION:
• • Every display_timing can be specified with either just the typical value or
• • a range consisting of min/typ/max. This function helps handling this
• **/

+static int parse_property(const struct device_node *np, const char *name,

• 	  struct timing_entry *result)
  

+{

• struct property *prop;
• int length, cells, ret;
• prop = of_find_property(np, name, &length);
• if (!prop) {

• pr_err("%s: could not find property %s\n", __func__, name);
  

• return -EINVAL;
  

• }
• cells = length / sizeof(u32);
• if (cells == 1) {

• ret = of_property_read_u32(np, name, &result->typ);
  

• result->min = result->typ;
  

• result->max = result->typ;
  

• } else if (cells == 3) {

• ret = of_property_read_u32_array(np, name, &result->min, cells);
  

• } else {

• pr_err("%s: illegal timing specification in %s\n", __func__, name);
  

• return -EINVAL;
  

• }
• return ret;

+}

+/**

• • of_get_display_timing - parse display_timing entry from device_node
• • @np: device_node with the properties
• **/

+static struct display_timing *of_get_display_timing(const struct device_node *np) +{

• struct display_timing *dt;
• int ret = 0;
• dt = kzalloc(sizeof(*dt), GFP_KERNEL);
• if (!dt) {

• pr_err("%s: could not allocate display_timing struct\n", __func__);
  

• return NULL;
  

• }
• ret |= parse_property(np, "hback-porch", &dt->hback_porch);
• ret |= parse_property(np, "hfront-porch", &dt->hfront_porch);
• ret |= parse_property(np, "hactive", &dt->hactive);
• ret |= parse_property(np, "hsync-len", &dt->hsync_len);
• ret |= parse_property(np, "vback-porch", &dt->vback_porch);
• ret |= parse_property(np, "vfront-porch", &dt->vfront_porch);
• ret |= parse_property(np, "vactive", &dt->vactive);
• ret |= parse_property(np, "vsync-len", &dt->vsync_len);
• ret |= parse_property(np, "clock-frequency", &dt->pixelclock);
• of_property_read_u32(np, "vsync-active", &dt->vsync_pol_active);
• of_property_read_u32(np, "hsync-active", &dt->hsync_pol_active);
• of_property_read_u32(np, "de-active", &dt->de_pol_active);
• of_property_read_u32(np, "pixelclk-inverted", &dt->pixelclk_pol);
• dt->interlaced = of_property_read_bool(np, "interlaced");
• dt->doublescan = of_property_read_bool(np, "doublescan");
• if (ret) {

• pr_err("%s: error reading timing properties\n", __func__);
  

• kfree(dt);
  

• return NULL;
  

• }
• return dt;

+}

+/**

• • of_get_display_timings - parse all display_timing entries from a device_node
• • @np: device_node with the subnodes
• **/

+struct display_timings *of_get_display_timings(const struct device_node *np) +{

• struct device_node *timings_np;
• struct device_node *entry;
• struct device_node *native_mode;
• struct display_timings *disp;
• if (!np) {

• pr_err("%s: no devicenode given\n", __func__);
  

• return NULL;
  

• }
• timings_np = of_find_node_by_name(np, "display-timings");

I get below build warnings on this line drivers/video/of_display_timing.c: In function 'of_get_display_timings': drivers/video/of_display_timing.c:109:2: warning: passing argument 1 of 'of_find_node_by_name' discards qualifiers from pointer target type include/linux/of.h:167:28: note: expected 'struct device_node *' but argument is of type 'const struct device_node *'

• if (!timings_np) {

• pr_err("%s: could not find display-timings node\n", __func__);
  

• return NULL;
  

• }
• disp = kzalloc(sizeof(*disp), GFP_KERNEL);
• if (!disp) {

• pr_err("%s: could not allocate struct disp'\n", __func__);
  

• goto dispfail;
  

• }
• entry = of_parse_phandle(timings_np, "native-mode", 0);
• /* assume first child as native mode if none provided */
• if (!entry)

• entry = of_get_next_child(np, NULL);
  

• /* if there is no child, it is useless to go on */
• if (!entry) {

• pr_err("%s: no timing specifications given\n", __func__);
  

• goto entryfail;
  

• }
• pr_info("%s: using %s as default timing\n", __func__, entry->name);
• native_mode = entry;
• disp->num_timings = of_get_child_count(timings_np);
• if (disp->num_timings == 0) {

• /* should never happen, as entry was already found above */
  

• pr_err("%s: no timings specified\n", __func__);
  

• goto entryfail;
  

• }
• disp->timings = kzalloc(sizeof(struct display_timing *) * disp->num_timings,

• 		GFP_KERNEL);
  

• if (!disp->timings) {

• pr_err("%s: could not allocate timings array\n", __func__);
  

• goto entryfail;
  

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

• struct display_timing *dt;
  

• dt = of_get_display_timing(entry);
  

• if (!dt) {
  

• 	/*
  

• 	 * to not encourage wrong devicetrees, fail in case of
  

• 	 * an error
  

• 	 */
  

• 	pr_err("%s: error in timing %d\n", __func__,
  

• 	       disp->num_timings + 1);
  

• 	goto timingfail;
  

• }
  

• if (native_mode == entry)
  

• 	disp->native_mode = disp->num_timings;
  

• disp->timings[disp->num_timings] = dt;
  

• disp->num_timings++;
  

• }
• of_node_put(timings_np);
• of_node_put(native_mode);
• if (disp->num_timings > 0)

• pr_info("%s: got %d timings. Using timing #%d as default\n",
  

• 	__func__, disp->num_timings, disp->native_mode + 1);
  

• else {

• pr_err("%s: no valid timings specified\n", __func__);
  

• display_timings_release(disp);
  

• return NULL;
  

• }
• return disp;

+timingfail:

• if (native_mode)

• of_node_put(native_mode);
  

• display_timings_release(disp);

+entryfail:

• if (disp)

• kfree(disp);
  

+dispfail:

• of_node_put(timings_np);
• return NULL;

+} +EXPORT_SYMBOL_GPL(of_get_display_timings);

+/**

• • of_display_timings_exists - check if a display-timings node is provided
• • @np: device_node with the timing
• **/

+int of_display_timings_exists(const struct device_node *np) +{

• struct device_node *timings_np;
• if (!np)

• return -EINVAL;
  

• timings_np = of_parse_phandle(np, "display-timings", 0);

Also here: drivers/video/of_display_timing.c: In function 'of_display_timings_exists': drivers/video/of_display_timing.c:209:2: warning: passing argument 1 of 'of_parse_phandle' discards qualifiers from pointer target type include/linux/of.h:258:28: note: expected 'struct device_node *' but argument is of type 'const struct device_node *'

Thanks, Prakash

• if (!timings_np)

• return -EINVAL;
  

• of_node_put(timings_np);
• return 1;

+} +EXPORT_SYMBOL_GPL(of_display_timings_exists); diff --git a/drivers/video/of_videomode.c b/drivers/video/of_videomode.c new file mode 100644 index 0000000..c573f92 --- /dev/null +++ b/drivers/video/of_videomode.c @@ -0,0 +1,48 @@ +/*

• • generic videomode helper
• • Copyright (c) 2012 Steffen Trumtrar s.trumtrar@pengutronix.de, Pengutronix
• • This file is released under the GPLv2
• */

+#include <linux/of.h> +#include <linux/of_display_timings.h> +#include <linux/of_videomode.h> +#include <linux/export.h>

+/**

• • of_get_videomode - get the videomode #<index> from devicetree
• • @np - devicenode with the display_timings
• • @vm - set to return value
• • @index - index into list of display_timings
• • DESCRIPTION:
• • Get a list of all display timings and put the one
• • specified by index into *vm. This function should only be used, if
• • only one videomode is to be retrieved. A driver that needs to work
• • with multiple/all videomodes should work with
• • of_get_display_timings instead.
• **/

+int of_get_videomode(const struct device_node *np, struct videomode *vm,

•      int index)
  

+{

• struct display_timings *disp;
• int ret;
• disp = of_get_display_timings(np);
• if (!disp) {

• pr_err("%s: no timings specified\n", __func__);
  

• return -EINVAL;
  

• }
• if (index == OF_USE_NATIVE_MODE)

• index = disp->native_mode;
  

• ret = videomode_from_timing(disp, vm, index);
• if (ret)

• return ret;
  

• display_timings_release(disp);
• return 0;

+} +EXPORT_SYMBOL_GPL(of_get_videomode); diff --git a/include/linux/of_display_timings.h b/include/linux/of_display_timings.h new file mode 100644 index 0000000..2b4fa0a --- /dev/null +++ b/include/linux/of_display_timings.h @@ -0,0 +1,20 @@ +/*

• • Copyright 2012 Steffen Trumtrar s.trumtrar@pengutronix.de
• • display timings of helpers
• • This file is released under the GPLv2
• */

+#ifndef __LINUX_OF_DISPLAY_TIMINGS_H +#define __LINUX_OF_DISPLAY_TIMINGS_H

+#include <linux/display_timing.h> +#include <linux/of.h>

+#define OF_USE_NATIVE_MODE -1

+struct display_timings *of_get_display_timings(const struct device_node *np); +int of_display_timings_exists(const struct device_node *np);

+#endif diff --git a/include/linux/of_videomode.h b/include/linux/of_videomode.h new file mode 100644 index 0000000..4de5fcc --- /dev/null +++ b/include/linux/of_videomode.h @@ -0,0 +1,18 @@ +/*

• • Copyright 2012 Steffen Trumtrar s.trumtrar@pengutronix.de
• • videomode of-helpers
• • This file is released under the GPLv2
• */

+#ifndef __LINUX_OF_VIDEOMODE_H +#define __LINUX_OF_VIDEOMODE_H

+#include <linux/videomode.h> +#include <linux/of.h>

+int of_get_videomode(const struct device_node *np, struct videomode *vm,

•      int index);
  

+#endif /* __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

On 2012-11-20 17:54, Steffen Trumtrar wrote:

+timings subnode +---------------

+required properties:

• • hactive, vactive: Display resolution
• • hfront-porch, hback-porch, hsync-len: Horizontal Display timing parameters
• in pixels
• vfront-porch, vback-porch, vsync-len: Vertical display timing parameters in
• lines
• • clock-frequency: display clock in Hz

+optional properties:

• • hsync-active: Hsync pulse is active low/high/ignored
• • vsync-active: Vsync pulse is active low/high/ignored
• • de-active: Data-Enable pulse is active low/high/ignored
• • pixelclk-inverted: pixelclock is inverted/non-inverted/ignored

Inverted related to what? And isn't this a bool? Pixel clock is either normal (whatever that is), or inverted. It can't be "not used".

I guess normal case is "pixel data is driven on the rising edge of pixel clock"? If that's common knowledge, I guess it doesn't need to be mentioned. But I always have to verify from the documentation what "normal" means on this particular panel/soc =).

• • interlaced (bool)
• • doublescan (bool)

+All the optional properties that are not bool follow the following logic:

• <1>: high active
• <0>: low active
• omitted: not used on hardware

Perhaps it's obvious, but no harm being explicit: mention that the bool properties are off is omitted.

And I didn't read the rest of the patches yet, so perhaps this is already correct, but as I think this framework is usable without DT also, the meanings of the fields in the structs should be explained in the header files also in case they are not obvious.

+Example:

• display-timings {

• native-mode = <&timing0>;
  

• timing0: 1920p24 {
  

This should still be 1080p24, not 1920p24 =).

• 	/* 1920x1080p24 */
  

• 	clock-frequency = <52000000>;
  

• 	hactive = <1920>;
  

• 	vactive = <1080>;
  

• 	hfront-porch = <25>;
  

• 	hback-porch = <25>;
  

• 	hsync-len = <25>;
  

• 	vback-porch = <2>;
  

• 	vfront-porch = <2>;
  

• 	vsync-len = <2>;
  

• 	hsync-active = <1>;
  

• };
  

• };

diff --git a/include/linux/of_display_timings.h b/include/linux/of_display_timings.h new file mode 100644 index 0000000..2b4fa0a --- /dev/null +++ b/include/linux/of_display_timings.h @@ -0,0 +1,20 @@ +/*

• • Copyright 2012 Steffen Trumtrar s.trumtrar@pengutronix.de
• • display timings of helpers
• • This file is released under the GPLv2
• */

+#ifndef __LINUX_OF_DISPLAY_TIMINGS_H +#define __LINUX_OF_DISPLAY_TIMINGS_H

+#include <linux/display_timing.h> +#include <linux/of.h>

No need to include these, just add "struct ...;".

+#define OF_USE_NATIVE_MODE -1

+struct display_timings *of_get_display_timings(const struct device_node *np); +int of_display_timings_exists(const struct device_node *np);

+#endif diff --git a/include/linux/of_videomode.h b/include/linux/of_videomode.h new file mode 100644 index 0000000..4de5fcc --- /dev/null +++ b/include/linux/of_videomode.h @@ -0,0 +1,18 @@ +/*

• • Copyright 2012 Steffen Trumtrar s.trumtrar@pengutronix.de
• • videomode of-helpers
• • This file is released under the GPLv2
• */

+#ifndef __LINUX_OF_VIDEOMODE_H +#define __LINUX_OF_VIDEOMODE_H

+#include <linux/videomode.h> +#include <linux/of.h>

Same here.

+int of_get_videomode(const struct device_node *np, struct videomode *vm,

•      int index);
  

+#endif /* __LINUX_OF_VIDEOMODE_H */

Tomi

Hi Steffen,

I am trying to add DT support for da8xx-fb driver on top of your patches. Encountered below build error. Sorry for reporting it late.

On Tue, Nov 20, 2012 at 21:24:53, Steffen Trumtrar wrote:

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

Signed-off-by: Steffen Trumtrar s.trumtrar@pengutronix.de Reviewed-by: Thierry Reding thierry.reding@avionic-design.de Acked-by: Thierry Reding thierry.reding@avionic-design.de Tested-by: Thierry Reding thierry.reding@avionic-design.de Tested-by: Philipp Zabel p.zabel@pengutronix.de Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com

---

drivers/video/fbmon.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++ include/linux/fb.h | 6 ++++++ 2 files changed, 52 insertions(+)

diff --git a/drivers/video/fbmon.c b/drivers/video/fbmon.c index cef6557..c1939a6 100644 --- a/drivers/video/fbmon.c +++ b/drivers/video/fbmon.c @@ -31,6 +31,7 @@ #include <linux/pci.h> #include <linux/slab.h> #include <video/edid.h> +#include <linux/videomode.h> #ifdef CONFIG_PPC_OF #include <asm/prom.h> #include <asm/pci-bridge.h> @@ -1373,6 +1374,51 @@ int fb_get_mode(int flags, u32 val, struct fb_var_screeninfo *var, struct fb_inf kfree(timings); return err; }

+#if IS_ENABLED(CONFIG_VIDEOMODE) +int fb_videomode_from_videomode(const struct videomode *vm,

• 		struct fb_videomode *fbmode)
  

+{

• unsigned int htotal, vtotal;
• fbmode->xres = vm->hactive;
• fbmode->left_margin = vm->hback_porch;
• fbmode->right_margin = vm->hfront_porch;
• fbmode->hsync_len = vm->hsync_len;
• fbmode->yres = vm->vactive;
• fbmode->upper_margin = vm->vback_porch;
• fbmode->lower_margin = vm->vfront_porch;
• fbmode->vsync_len = vm->vsync_len;
• fbmode->pixclock = KHZ2PICOS(vm->pixelclock / 1000);
• fbmode->sync = 0;
• fbmode->vmode = 0;
• if (vm->hah)

• fbmode->sync |= FB_SYNC_HOR_HIGH_ACT;
  

• if (vm->vah)

• fbmode->sync |= FB_SYNC_VERT_HIGH_ACT;
  

• if (vm->interlaced)

• fbmode->vmode |= FB_VMODE_INTERLACED;
  

• if (vm->doublescan)

• fbmode->vmode |= FB_VMODE_DOUBLE;
  

• if (vm->de)

• fbmode->sync |= FB_SYNC_DATA_ENABLE_HIGH_ACT;
  

"FB_SYNC_DATA_ENABLE_HIGH_ACT" seems to be mxsfb specific flag, I am getting build error on this. Please let me know if I am missing something.

Thanks, Prakash

• fbmode->flag = 0;
• htotal = vm->hactive + vm->hfront_porch + vm->hback_porch +

•  vm->hsync_len;
  

• vtotal = vm->vactive + vm->vfront_porch + vm->vback_porch +

•  vm->vsync_len;
  

• fbmode->refresh = (vm->pixelclock * 1000) / (htotal * vtotal);
• return 0;

+} +EXPORT_SYMBOL_GPL(fb_videomode_from_videomode); +#endif

#else int fb_parse_edid(unsigned char *edid, struct fb_var_screeninfo *var) { diff --git a/include/linux/fb.h b/include/linux/fb.h index c7a9571..920cbe3 100644 --- a/include/linux/fb.h +++ b/include/linux/fb.h @@ -14,6 +14,7 @@ #include <linux/backlight.h> #include <linux/slab.h> #include <asm/io.h> +#include <linux/videomode.h>

struct vm_area_struct; struct fb_info; @@ -714,6 +715,11 @@ extern void fb_destroy_modedb(struct fb_videomode *modedb); extern int fb_find_mode_cvt(struct fb_videomode *mode, int margins, int rb); extern unsigned char *fb_ddc_read(struct i2c_adapter *adapter);

+#if IS_ENABLED(CONFIG_VIDEOMODE) +extern int fb_videomode_from_videomode(const struct videomode *vm,

• 		       struct fb_videomode *fbmode);
  

+#endif

/* drivers/video/modedb.c */ #define VESA_MODEDB_SIZE 34 extern void fb_var_to_videomode(struct fb_videomode *mode, -- 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

Hi!

On Wed, Nov 21, 2012 at 04:39:01PM +0530, Leela Krishna Amudala wrote:

Yes, Even I got the same build error. later I fixed it by including "#include <linux/mxsfb.h>"

Best Wishes, Leela Krishna.

On Wed, Nov 21, 2012 at 3:39 PM, Manjunathappa, Prakash prakash.pm@ti.com wrote:

...

Hi Steffen,

I am trying to add DT support for da8xx-fb driver on top of your patches. Encountered below build error. Sorry for reporting it late.

On Tue, Nov 20, 2012 at 21:24:53, Steffen Trumtrar wrote:

...

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

Signed-off-by: Steffen Trumtrar s.trumtrar@pengutronix.de Reviewed-by: Thierry Reding thierry.reding@avionic-design.de Acked-by: Thierry Reding thierry.reding@avionic-design.de Tested-by: Thierry Reding thierry.reding@avionic-design.de Tested-by: Philipp Zabel p.zabel@pengutronix.de Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com

---

drivers/video/fbmon.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++ include/linux/fb.h | 6 ++++++ 2 files changed, 52 insertions(+)

diff --git a/drivers/video/fbmon.c b/drivers/video/fbmon.c index cef6557..c1939a6 100644 --- a/drivers/video/fbmon.c +++ b/drivers/video/fbmon.c @@ -31,6 +31,7 @@ #include <linux/pci.h> #include <linux/slab.h> #include <video/edid.h> +#include <linux/videomode.h> #ifdef CONFIG_PPC_OF #include <asm/prom.h> #include <asm/pci-bridge.h> @@ -1373,6 +1374,51 @@ int fb_get_mode(int flags, u32 val, struct fb_var_screeninfo *var, struct fb_inf kfree(timings); return err; }

+#if IS_ENABLED(CONFIG_VIDEOMODE) +int fb_videomode_from_videomode(const struct videomode *vm,

•                         struct fb_videomode *fbmode)
  

+{

• unsigned int htotal, vtotal;
  

• fbmode->xres = vm->hactive;
  

• fbmode->left_margin = vm->hback_porch;
  

• fbmode->right_margin = vm->hfront_porch;
  

• fbmode->hsync_len = vm->hsync_len;
  

• fbmode->yres = vm->vactive;
  

• fbmode->upper_margin = vm->vback_porch;
  

• fbmode->lower_margin = vm->vfront_porch;
  

• fbmode->vsync_len = vm->vsync_len;
  

• fbmode->pixclock = KHZ2PICOS(vm->pixelclock / 1000);
  

• fbmode->sync = 0;
  

• fbmode->vmode = 0;
  

• if (vm->hah)
  

•         fbmode->sync |= FB_SYNC_HOR_HIGH_ACT;
  

• if (vm->vah)
  

•         fbmode->sync |= FB_SYNC_VERT_HIGH_ACT;
  

• if (vm->interlaced)
  

•         fbmode->vmode |= FB_VMODE_INTERLACED;
  

• if (vm->doublescan)
  

•         fbmode->vmode |= FB_VMODE_DOUBLE;
  

• if (vm->de)
  

•         fbmode->sync |= FB_SYNC_DATA_ENABLE_HIGH_ACT;
  

"FB_SYNC_DATA_ENABLE_HIGH_ACT" seems to be mxsfb specific flag, I am getting build error on this. Please let me know if I am missing something.

Thanks, Prakash

I compile-tested the series and didn't have that error. But obviously I should have. As this is a mxsfs flag, I will throw it out.

Regards, Steffen

Hi Steffen,

Sorry, I've just found another small bug below.

On Tuesday 20 November 2012 16:54:53 Steffen Trumtrar wrote:

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

Signed-off-by: Steffen Trumtrar s.trumtrar@pengutronix.de Reviewed-by: Thierry Reding thierry.reding@avionic-design.de Acked-by: Thierry Reding thierry.reding@avionic-design.de Tested-by: Thierry Reding thierry.reding@avionic-design.de Tested-by: Philipp Zabel p.zabel@pengutronix.de Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com

---

drivers/video/fbmon.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++ include/linux/fb.h | 6 ++++++ 2 files changed, 52 insertions(+)

diff --git a/drivers/video/fbmon.c b/drivers/video/fbmon.c index cef6557..c1939a6 100644 --- a/drivers/video/fbmon.c +++ b/drivers/video/fbmon.c @@ -31,6 +31,7 @@ #include <linux/pci.h> #include <linux/slab.h> #include <video/edid.h> +#include <linux/videomode.h> #ifdef CONFIG_PPC_OF #include <asm/prom.h> #include <asm/pci-bridge.h> @@ -1373,6 +1374,51 @@ int fb_get_mode(int flags, u32 val, struct fb_var_screeninfo *var, struct fb_inf kfree(timings); return err; }

+#if IS_ENABLED(CONFIG_VIDEOMODE) +int fb_videomode_from_videomode(const struct videomode *vm,

• 		struct fb_videomode *fbmode)
  

+{

• unsigned int htotal, vtotal;
• fbmode->xres = vm->hactive;
• fbmode->left_margin = vm->hback_porch;
• fbmode->right_margin = vm->hfront_porch;
• fbmode->hsync_len = vm->hsync_len;
• fbmode->yres = vm->vactive;
• fbmode->upper_margin = vm->vback_porch;
• fbmode->lower_margin = vm->vfront_porch;
• fbmode->vsync_len = vm->vsync_len;
• fbmode->pixclock = KHZ2PICOS(vm->pixelclock / 1000);
• fbmode->sync = 0;
• fbmode->vmode = 0;
• if (vm->hah)

• fbmode->sync |= FB_SYNC_HOR_HIGH_ACT;
  

• if (vm->vah)

• fbmode->sync |= FB_SYNC_VERT_HIGH_ACT;
  

• if (vm->interlaced)

• fbmode->vmode |= FB_VMODE_INTERLACED;
  

• if (vm->doublescan)

• fbmode->vmode |= FB_VMODE_DOUBLE;
  

• if (vm->de)

• fbmode->sync |= FB_SYNC_DATA_ENABLE_HIGH_ACT;
  

• fbmode->flag = 0;
• htotal = vm->hactive + vm->hfront_porch + vm->hback_porch +

•  vm->hsync_len;
  

• vtotal = vm->vactive + vm->vfront_porch + vm->vback_porch +

•  vm->vsync_len;
  

• fbmode->refresh = (vm->pixelclock * 1000) / (htotal * vtotal);

This will fail if vm->pixelclock >= ((1 << 32) / 1000). The easiest solution is probably to use 64-bit computation.

• return 0;

+} +EXPORT_SYMBOL_GPL(fb_videomode_from_videomode); +#endif

#else int fb_parse_edid(unsigned char *edid, struct fb_var_screeninfo *var) { diff --git a/include/linux/fb.h b/include/linux/fb.h index c7a9571..920cbe3 100644 --- a/include/linux/fb.h +++ b/include/linux/fb.h @@ -14,6 +14,7 @@ #include <linux/backlight.h> #include <linux/slab.h> #include <asm/io.h> +#include <linux/videomode.h>

struct vm_area_struct; struct fb_info; @@ -714,6 +715,11 @@ extern void fb_destroy_modedb(struct fb_videomode *modedb); extern int fb_find_mode_cvt(struct fb_videomode *mode, int margins, int rb); extern unsigned char *fb_ddc_read(struct i2c_adapter *adapter);

+#if IS_ENABLED(CONFIG_VIDEOMODE) +extern int fb_videomode_from_videomode(const struct videomode *vm,

• 		       struct fb_videomode *fbmode);
  

+#endif

/* drivers/video/modedb.c */ #define VESA_MODEDB_SIZE 34 extern void fb_var_to_videomode(struct fb_videomode *mode,

Hi Tomi,

On Wednesday 21 November 2012 14:49:30 Tomi Valkeinen wrote:

On 2012-11-20 17:54, Steffen Trumtrar wrote:

...

Add helper to get fb_videomode from devicetree.

Signed-off-by: Steffen Trumtrar s.trumtrar@pengutronix.de Reviewed-by: Thierry Reding thierry.reding@avionic-design.de Acked-by: Thierry Reding thierry.reding@avionic-design.de Tested-by: Thierry Reding thierry.reding@avionic-design.de Tested-by: Philipp Zabel p.zabel@pengutronix.de Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com

---

drivers/video/fbmon.c | 42 +++++++++++++++++++++++++++++++++++++++++- include/linux/fb.h | 7 +++++++ 2 files changed, 48 insertions(+), 1 deletion(-)

diff --git a/include/linux/fb.h b/include/linux/fb.h index 920cbe3..41b5e49 100644 --- a/include/linux/fb.h +++ b/include/linux/fb.h @@ -15,6 +15,8 @@

#include <linux/slab.h> #include <asm/io.h> #include <linux/videomode.h>

+#include <linux/of.h> +#include <linux/of_videomode.h>

Guess what? =)

To be honest, I don't know what the general opinion is about including header files from header files. But I always leave them out if they are not strictly needed.

I agree, I favor structure declaration as well when possible.

...

struct vm_area_struct; struct fb_info;

@@ -715,6 +717,11 @@ extern void fb_destroy_modedb(struct fb_videomode *modedb);> extern int fb_find_mode_cvt(struct fb_videomode *mode, int margins, int rb); extern unsigned char *fb_ddc_read(struct i2c_adapter *adapter);

+#if IS_ENABLED(CONFIG_OF_VIDEOMODE) +extern int of_get_fb_videomode(const struct device_node *np,

• 	       struct fb_videomode *fb,
  

• 	       unsigned int index);
  

+#endif

#if IS_ENABLED(CONFIG_VIDEOMODE) extern int fb_videomode_from_videomode(const struct videomode *vm,

		       struct fb_videomode *fbmode);

Do you really need these #ifs in the header files? They do make it look a bit messy. If somebody uses the functions and CONFIG_VIDEOMODE is not enabled, he'll get a linker error anyway.

On 2012-11-21 18:24, Steffen Trumtrar wrote:

On Wed, Nov 21, 2012 at 02:49:30PM +0200, Tomi Valkeinen wrote:

...

...

@@ -715,6 +717,11 @@ extern void fb_destroy_modedb(struct fb_videomode *modedb); extern int fb_find_mode_cvt(struct fb_videomode *mode, int margins, int rb); extern unsigned char *fb_ddc_read(struct i2c_adapter *adapter);

+#if IS_ENABLED(CONFIG_OF_VIDEOMODE) +extern int of_get_fb_videomode(const struct device_node *np,

• 	       struct fb_videomode *fb,
  

• 	       unsigned int index);
  

+#endif #if IS_ENABLED(CONFIG_VIDEOMODE) extern int fb_videomode_from_videomode(const struct videomode *vm, struct fb_videomode *fbmode);

Do you really need these #ifs in the header files? They do make it look a bit messy. If somebody uses the functions and CONFIG_VIDEOMODE is not enabled, he'll get a linker error anyway.

Well, I don't remember at the moment who requested this, but it was not my idea to put them there. So, this is a matter of style I guess. But maybe I understood that wrong.

Right, one reviewer says this way, and another says that way =).

With the header files I've made I only use #ifs with #else, when I want to give a static inline empty/no-op implementation for the function in case the feature is not compiled into the kernel.

As you said, matter of taste. Up to you.

Tomi

On 2012-11-20 17:54, Steffen Trumtrar wrote:

Add conversion from videomode to drm_display_mode

Signed-off-by: Steffen Trumtrar s.trumtrar@pengutronix.de Reviewed-by: Thierry Reding thierry.reding@avionic-design.de Acked-by: Thierry Reding thierry.reding@avionic-design.de Tested-by: Thierry Reding thierry.reding@avionic-design.de Tested-by: Philipp Zabel p.zabel@pengutronix.de Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com

---

drivers/gpu/drm/drm_modes.c | 37 +++++++++++++++++++++++++++++++++++++ include/drm/drmP.h | 6 ++++++ 2 files changed, 43 insertions(+)

diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 3fd8280..de2f6cf 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -56,6 +56,7 @@ #include <linux/cdev.h> #include <linux/mutex.h> #include <linux/slab.h> +#include <linux/videomode.h> #if defined(__alpha__) || defined(__powerpc__) #include <asm/pgtable.h> /* For pte_wrprotect */ #endif @@ -1454,6 +1455,11 @@ extern struct drm_display_mode * drm_mode_create_from_cmdline_mode(struct drm_device *dev, struct drm_cmdline_mode *cmd);

+#if IS_ENABLED(CONFIG_VIDEOMODE) +extern int drm_display_mode_from_videomode(const struct videomode *vm,

• 			   struct drm_display_mode *dmode);
  

+#endif

/* Modesetting support */ extern void drm_vblank_pre_modeset(struct drm_device *dev, int crtc); extern void drm_vblank_post_modeset(struct drm_device *dev, int crtc);

And the same comments apply to this header file.

Tomi

On 2012-11-20 17:54, Steffen Trumtrar wrote:

Add helper to get drm_display_mode from devicetree.

Signed-off-by: Steffen Trumtrar s.trumtrar@pengutronix.de Reviewed-by: Thierry Reding thierry.reding@avionic-design.de Acked-by: Thierry Reding thierry.reding@avionic-design.de Tested-by: Thierry Reding thierry.reding@avionic-design.de Tested-by: Philipp Zabel p.zabel@pengutronix.de Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com

---

drivers/gpu/drm/drm_modes.c | 35 ++++++++++++++++++++++++++++++++++- include/drm/drmP.h | 6 ++++++ 2 files changed, 40 insertions(+), 1 deletion(-)

diff --git a/include/drm/drmP.h b/include/drm/drmP.h index de2f6cf..377280f 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -56,6 +56,7 @@ #include <linux/cdev.h> #include <linux/mutex.h> #include <linux/slab.h> +#include <linux/of.h> #include <linux/videomode.h> #if defined(__alpha__) || defined(__powerpc__) #include <asm/pgtable.h> /* For pte_wrprotect */ @@ -1459,6 +1460,11 @@ drm_mode_create_from_cmdline_mode(struct drm_device *dev, extern int drm_display_mode_from_videomode(const struct videomode *vm, struct drm_display_mode *dmode); #endif +#if IS_ENABLED(CONFIG_OF_VIDEOMODE) +extern int of_get_drm_display_mode(const struct device_node *np,

• 		   struct drm_display_mode *dmode,
  

• 		   unsigned int index);
  

+#endif

/* Modesetting support */ extern void drm_vblank_pre_modeset(struct drm_device *dev, int crtc);

And the same comments here also.

Tomi