[PATCH v7 0/8] of: add display helper

[PATCH v7 0/8] of: add display helper

[Steffen Trumtrar]

Hi!

Finally, v7 of the series.

Changes since v6:
	- get rid of some empty lines etc.
	- move functions to their subsystems
	- split of_ from non-of_ functions
	- add at least some kerneldoc to some functions

Regards,
Steffen


Steffen Trumtrar (8):
  video: add display_timing struct and helpers
  of: add helper to parse display timings
  of: add generic videomode description
  video: add videomode helpers
  fbmon: add videomode helpers
  fbmon: add of_videomode helpers
  drm_modes: add videomode helpers
  drm_modes: add of_videomode helpers

 .../devicetree/bindings/video/display-timings.txt  |  139 +++++++++++++++
 drivers/gpu/drm/drm_modes.c                        |   78 +++++++++
 drivers/of/Kconfig                                 |   12 ++
 drivers/of/Makefile                                |    2 +
 drivers/of/of_display_timings.c                    |  185 ++++++++++++++++++++
 drivers/of/of_videomode.c                          |   47 +++++
 drivers/video/Kconfig                              |   11 ++
 drivers/video/Makefile                             |    2 +
 drivers/video/display_timing.c                     |   24 +++
 drivers/video/fbmon.c                              |   76 ++++++++
 drivers/video/videomode.c                          |   44 +++++
 include/drm/drmP.h                                 |    8 +
 include/linux/display_timing.h                     |   69 ++++++++
 include/linux/fb.h                                 |    5 +
 include/linux/of_display_timings.h                 |   20 +++
 include/linux/of_videomode.h                       |   15 ++
 include/linux/videomode.h                          |   36 ++++
 17 files changed, 773 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 drivers/video/display_timing.c
 create mode 100644 drivers/video/videomode.c
 create mode 100644 include/linux/display_timing.h
 create mode 100644 include/linux/of_display_timings.h
 create mode 100644 include/linux/of_videomode.h
 create mode 100644 include/linux/videomode.h

-- 
1.7.10.4

[PATCH v7 1/8] video: add display_timing struct and helpers

[Steffen Trumtrar]

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

Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
---
 drivers/video/Kconfig          |    5 +++
 drivers/video/Makefile         |    1 +
 drivers/video/display_timing.c |   24 ++++++++++++++
 include/linux/display_timing.h |   69 ++++++++++++++++++++++++++++++++++++++++
 4 files changed, 99 insertions(+)
 create mode 100644 drivers/video/display_timing.c
 create mode 100644 include/linux/display_timing.h

diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
index d08d799..1421fc8 100644
--- a/drivers/video/Kconfig
+++ b/drivers/video/Kconfig
@@ -33,6 +33,11 @@ config VIDEO_OUTPUT_CONTROL
 	  This framework adds support for low-level control of the video 
 	  output switch.
 
+config DISPLAY_TIMING
+       bool "Enable display timings helpers"
+       help
+         Say Y here, to use the display timing helpers.
+
 menuconfig FB
 	tristate "Support for frame buffer devices"
 	---help---
diff --git a/drivers/video/Makefile b/drivers/video/Makefile
index 23e948e..552c045 100644
--- a/drivers/video/Makefile
+++ b/drivers/video/Makefile
@@ -167,3 +167,4 @@ 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
diff --git a/drivers/video/display_timing.c b/drivers/video/display_timing.c
new file mode 100644
index 0000000..9ccfdb3
--- /dev/null
+++ b/drivers/video/display_timing.c
@@ -0,0 +1,24 @@
+/*
+ * generic display timing functions
+ *
+ * Copyright (c) 2012 Steffen Trumtrar <s.trumtrar@pengutronix.de>, Pengutronix
+ *
+ * This file is released under the GPLv2
+ */
+
+#include <linux/slab.h>
+#include <linux/display_timing.h>
+
+void timings_release(struct display_timings *disp)
+{
+	int i;
+
+	for (i = 0; i < disp->num_timings; i++)
+		kfree(disp->timings[i]);
+}
+
+void display_timings_release(struct display_timings *disp)
+{
+	timings_release(disp);
+	kfree(disp->timings);
+}
diff --git a/include/linux/display_timing.h b/include/linux/display_timing.h
new file mode 100644
index 0000000..aa02a12
--- /dev/null
+++ b/include/linux/display_timing.h
@@ -0,0 +1,69 @@
+/*
+ * Copyright 2012 Steffen Trumtrar <s.trumtrar@pengutronix.de>
+ *
+ * description of display timings
+ *
+ * This file is released under the GPLv2
+ */
+
+#ifndef __LINUX_DISPLAY_TIMINGS_H
+#define __LINUX_DISPLAY_TIMINGS_H
+
+#include <linux/types.h>
+
+struct timing_entry {
+	u32 min;
+	u32 typ;
+	u32 max;
+};
+
+struct display_timing {
+	struct timing_entry pixelclock;
+
+	struct timing_entry hactive;
+	struct timing_entry hfront_porch;
+	struct timing_entry hback_porch;
+	struct timing_entry hsync_len;
+
+	struct timing_entry vactive;
+	struct timing_entry vfront_porch;
+	struct timing_entry vback_porch;
+	struct timing_entry vsync_len;
+
+	unsigned int vsync_pol_active;
+	unsigned int hsync_pol_active;
+	unsigned int de_pol_active;
+	unsigned int pixelclk_pol;
+	bool interlaced;
+	bool doublescan;
+};
+
+struct display_timings {
+	unsigned int num_timings;
+	unsigned int native_mode;
+
+	struct display_timing **timings;
+};
+
+/* placeholder function until ranges are really needed */
+static inline u32 display_timing_get_value(struct timing_entry *te, int index)
+{
+	return te->typ;
+}
+
+static inline struct display_timing *display_timings_get(struct display_timings *disp,
+							 int index)
+{
+	struct display_timing *dt;
+
+	if (disp->num_timings > index) {
+		dt = disp->timings[index];
+		return dt;
+	} else
+		return NULL;
+}
+
+void timings_release(struct display_timings *disp);
+void display_timings_release(struct display_timings *disp);
+
+#endif
-- 
1.7.10.4

Re: [PATCH v7 1/8] video: add display_timing struct and helpers

[Laurent Pinchart]

Hi Steffen,

Thanks for the patch.

As we'll need a v8 anyway due to the comment on patch 5/8, here are a couple 
of other small comments.

On Wednesday 31 October 2012 10:28:01 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>.
> 
> Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
> ---
>  drivers/video/Kconfig          |    5 +++
>  drivers/video/Makefile         |    1 +
>  drivers/video/display_timing.c |   24 ++++++++++++++
>  include/linux/display_timing.h |   69 +++++++++++++++++++++++++++++++++++++
>  4 files changed, 99 insertions(+)
>  create mode 100644 drivers/video/display_timing.c
>  create mode 100644 include/linux/display_timing.h
> 
> diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
> index d08d799..1421fc8 100644
> --- a/drivers/video/Kconfig
> +++ b/drivers/video/Kconfig
> @@ -33,6 +33,11 @@ config VIDEO_OUTPUT_CONTROL
>  	  This framework adds support for low-level control of the video
>  	  output switch.
> 
> +config DISPLAY_TIMING
> +       bool "Enable display timings helpers"
> +       help
> +         Say Y here, to use the display timing helpers.
> +
>  menuconfig FB
>  	tristate "Support for frame buffer devices"
>  	---help---
> diff --git a/drivers/video/Makefile b/drivers/video/Makefile
> index 23e948e..552c045 100644
> --- a/drivers/video/Makefile
> +++ b/drivers/video/Makefile
> @@ -167,3 +167,4 @@ 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
> diff --git a/drivers/video/display_timing.c b/drivers/video/display_timing.c
> new file mode 100644
> index 0000000..9ccfdb3
> --- /dev/null
> +++ b/drivers/video/display_timing.c
> @@ -0,0 +1,24 @@
> +/*
> + * generic display timing functions
> + *
> + * Copyright (c) 2012 Steffen Trumtrar <s.trumtrar@pengutronix.de>,
> Pengutronix + *
> + * This file is released under the GPLv2
> + */
> +
> +#include <linux/slab.h>
> +#include <linux/display_timing.h>

I try to keep #include's sorted alphabetically, but I won't push for that.

> +void timings_release(struct display_timings *disp)
> +{
> +	int i;
> +
> +	for (i = 0; i < disp->num_timings; i++)

disp->num_timings is an unsigned int, i should be an unsigned int as well to 
avoid signed vs. unsigned comparisons.

> +		kfree(disp->timings[i]);
> +}
> +
> +void display_timings_release(struct display_timings *disp)
> +{
> +	timings_release(disp);
> +	kfree(disp->timings);
> +}
> diff --git a/include/linux/display_timing.h b/include/linux/display_timing.h
> new file mode 100644
> index 0000000..aa02a12
> --- /dev/null
> +++ b/include/linux/display_timing.h
> @@ -0,0 +1,69 @@
> +/*
> + * Copyright 2012 Steffen Trumtrar <s.trumtrar@pengutronix.de>
> + *
> + * description of display timings
> + *
> + * This file is released under the GPLv2
> + */
> +
> +#ifndef __LINUX_DISPLAY_TIMINGS_H
> +#define __LINUX_DISPLAY_TIMINGS_H
> +
> +#include <linux/types.h>
> +
> +struct timing_entry {
> +	u32 min;
> +	u32 typ;
> +	u32 max;
> +};
> +
> +struct display_timing {
> +	struct timing_entry pixelclock;
> +
> +	struct timing_entry hactive;
> +	struct timing_entry hfront_porch;
> +	struct timing_entry hback_porch;
> +	struct timing_entry hsync_len;
> +
> +	struct timing_entry vactive;
> +	struct timing_entry vfront_porch;
> +	struct timing_entry vback_porch;
> +	struct timing_entry vsync_len;
> +
> +	unsigned int vsync_pol_active;
> +	unsigned int hsync_pol_active;
> +	unsigned int de_pol_active;
> +	unsigned int pixelclk_pol;
> +	bool interlaced;
> +	bool doublescan;
> +};
> +
> +struct display_timings {
> +	unsigned int num_timings;
> +	unsigned int native_mode;
> +
> +	struct display_timing **timings;
> +};
> +
> +/* placeholder function until ranges are really needed */
> +static inline u32 display_timing_get_value(struct timing_entry *te, int
> index)

What is the index parameter for ?

> +{
> +	return te->typ;
> +}
> +
> +static inline struct display_timing *display_timings_get(struct
> display_timings *disp,
> +							 int index)
> +{
> +	struct display_timing *dt;
> +
> +	if (disp->num_timings > index) {

index should be an unsigned int for the same reason as above.

> +		dt = disp->timings[index];
> +		return dt;

Maybe just

	return disp->timings[index];

?

> +	} else
> +		return NULL;
> +}
> +
> +void timings_release(struct display_timings *disp);
> +void display_timings_release(struct display_timings *disp);
> +
> +#endif
-- 
Regards,

Laurent Pinchart

Re: [PATCH v7 1/8] video: add display_timing struct and helpers

[Laurent Pinchart]

Hi Steffen,

One more comment.

On Wednesday 31 October 2012 10:28:01 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>.
> 
> Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
> ---
>  drivers/video/Kconfig          |    5 +++
>  drivers/video/Makefile         |    1 +
>  drivers/video/display_timing.c |   24 ++++++++++++++
>  include/linux/display_timing.h |   69 +++++++++++++++++++++++++++++++++++++
>  4 files changed, 99 insertions(+)
>  create mode 100644 drivers/video/display_timing.c
>  create mode 100644 include/linux/display_timing.h
> 
> diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
> index d08d799..1421fc8 100644
> --- a/drivers/video/Kconfig
> +++ b/drivers/video/Kconfig
> @@ -33,6 +33,11 @@ config VIDEO_OUTPUT_CONTROL
>  	  This framework adds support for low-level control of the video
>  	  output switch.
> 
> +config DISPLAY_TIMING
> +       bool "Enable display timings helpers"
> +       help
> +         Say Y here, to use the display timing helpers.
> +
>  menuconfig FB
>  	tristate "Support for frame buffer devices"
>  	---help---
> diff --git a/drivers/video/Makefile b/drivers/video/Makefile
> index 23e948e..552c045 100644
> --- a/drivers/video/Makefile
> +++ b/drivers/video/Makefile
> @@ -167,3 +167,4 @@ 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
> diff --git a/drivers/video/display_timing.c b/drivers/video/display_timing.c
> new file mode 100644
> index 0000000..9ccfdb3
> --- /dev/null
> +++ b/drivers/video/display_timing.c
> @@ -0,0 +1,24 @@
> +/*
> + * generic display timing functions
> + *
> + * Copyright (c) 2012 Steffen Trumtrar <s.trumtrar@pengutronix.de>,
> Pengutronix + *
> + * This file is released under the GPLv2
> + */
> +
> +#include <linux/slab.h>
> +#include <linux/display_timing.h>
> +
> +void timings_release(struct display_timings *disp)
> +{
> +	int i;
> +
> +	for (i = 0; i < disp->num_timings; i++)
> +		kfree(disp->timings[i]);
> +}

This function doesn't seem to be called externally, you can make it static.

> +void display_timings_release(struct display_timings *disp)
> +{
> +	timings_release(disp);
> +	kfree(disp->timings);
> +}
> diff --git a/include/linux/display_timing.h b/include/linux/display_timing.h
> new file mode 100644
> index 0000000..aa02a12
> --- /dev/null
> +++ b/include/linux/display_timing.h
> @@ -0,0 +1,69 @@
> +/*
> + * Copyright 2012 Steffen Trumtrar <s.trumtrar@pengutronix.de>
> + *
> + * description of display timings
> + *
> + * This file is released under the GPLv2
> + */
> +
> +#ifndef __LINUX_DISPLAY_TIMINGS_H
> +#define __LINUX_DISPLAY_TIMINGS_H
> +
> +#include <linux/types.h>
> +
> +struct timing_entry {
> +	u32 min;
> +	u32 typ;
> +	u32 max;
> +};
> +
> +struct display_timing {
> +	struct timing_entry pixelclock;
> +
> +	struct timing_entry hactive;
> +	struct timing_entry hfront_porch;
> +	struct timing_entry hback_porch;
> +	struct timing_entry hsync_len;
> +
> +	struct timing_entry vactive;
> +	struct timing_entry vfront_porch;
> +	struct timing_entry vback_porch;
> +	struct timing_entry vsync_len;
> +
> +	unsigned int vsync_pol_active;
> +	unsigned int hsync_pol_active;
> +	unsigned int de_pol_active;
> +	unsigned int pixelclk_pol;
> +	bool interlaced;
> +	bool doublescan;
> +};
> +
> +struct display_timings {
> +	unsigned int num_timings;
> +	unsigned int native_mode;
> +
> +	struct display_timing **timings;
> +};
> +
> +/* placeholder function until ranges are really needed */
> +static inline u32 display_timing_get_value(struct timing_entry *te, int
> index) +{
> +	return te->typ;
> +}
> +
> +static inline struct display_timing *display_timings_get(struct
> display_timings *disp, +							 int index)
> +{
> +	struct display_timing *dt;
> +
> +	if (disp->num_timings > index) {
> +		dt = disp->timings[index];
> +		return dt;
> +	} else
> +		return NULL;
> +}
> +void timings_release(struct display_timings *disp);
> +void display_timings_release(struct display_timings *disp);
> +
> +#endif

-- 
Regards,

Laurent Pinchart

Re: [PATCH v7 1/8] video: add display_timing struct and helpers

[Thierry Reding]

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

On Wed, Oct 31, 2012 at 10:28:01AM +0100, Steffen Trumtrar wrote:
[...]
> +void timings_release(struct display_timings *disp)
> +{
> +	int i;
> +
> +	for (i = 0; i < disp->num_timings; i++)
> +		kfree(disp->timings[i]);
> +}
> +
> +void display_timings_release(struct display_timings *disp)
> +{
> +	timings_release(disp);
> +	kfree(disp->timings);
> +}

I'm not quite sure I understand how these are supposed to be used. The
only use-case where a struct display_timings is dynamically allocated is
for the OF helpers. In that case, wouldn't it be more useful to have a
function that frees the complete structure, including the struct
display_timings itself? Something like this, which has all of the above
rolled into one:

	void display_timings_free(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);
	}

Is there a use-case where a struct display_timings is not dynamically
allocated? The only one I can think of is where it is defined as
platform data, but in that case you don't want to be calling
display_timing_release() on it anyway.

Thierry

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

Re: [PATCH v7 1/8] video: add display_timing struct and helpers

[Steffen Trumtrar]

On Thu, Nov 01, 2012 at 09:08:42PM +0100, Thierry Reding wrote:
> On Wed, Oct 31, 2012 at 10:28:01AM +0100, Steffen Trumtrar wrote:
> [...]
> > +void timings_release(struct display_timings *disp)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < disp->num_timings; i++)
> > +		kfree(disp->timings[i]);
> > +}
> > +
> > +void display_timings_release(struct display_timings *disp)
> > +{
> > +	timings_release(disp);
> > +	kfree(disp->timings);
> > +}
> 
> I'm not quite sure I understand how these are supposed to be used. The
> only use-case where a struct display_timings is dynamically allocated is
> for the OF helpers. In that case, wouldn't it be more useful to have a
> function that frees the complete structure, including the struct
> display_timings itself? Something like this, which has all of the above
> rolled into one:
> 
> 	void display_timings_free(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);
> 	}
> 

Well, you are right. They can be rolled into one function.
The extra function call is useless and as it seems confusing.

Regards,
Steffen

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


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

[Steffen Trumtrar]

Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
---
 .../devicetree/bindings/video/display-timings.txt  |  139 +++++++++++++++
 drivers/of/Kconfig                                 |    6 +
 drivers/of/Makefile                                |    1 +
 drivers/of/of_display_timings.c                    |  185 ++++++++++++++++++++
 include/linux/of_display_timings.h                 |   20 +++
 5 files changed, 351 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..04c94a3
--- /dev/null
+++ b/Documentation/devicetree/bindings/video/display-timings.txt
@@ -0,0 +1,139 @@
+display-timings bindings
+=========
+
+display-timings-node
+------------
+
+required properties:
+ - none
+
+optional properties:
+ - native-mode: the native mode for the display, in case multiple modes are
+		provided. When omitted, assume the first node is the native.
+
+timings-subnode
+---------------
+
+required properties:
+ - hactive, vactive: Display resolution
+ - hfront-porch, hback-porch, hsync-len: Horizontal Display timing parameters
+   in pixels
+   vfront-porch, vback-porch, vsync-len: Vertical display timing parameters in
+   lines
+ - clock-frequency: displayclock in Hz
+
+optional properties:
+ - hsync-active : Hsync pulse is active low/high/ignored
+ - vsync-active : Vsync pulse is active low/high/ignored
+ - de-active : Data-Enable pulse is active low/high/ignored
+ - pixelclk-inverted : pixelclock is inverted/non-inverted/ignored
+ - interlaced (bool)
+ - doublescan (bool)
+
+All the optional properties that are not bool follow the following logic:
+    <1> : high active
+    <0> : low active
+    omitted : not used on hardware
+
+There are different ways of describing the capabilities of a display. The devicetree
+representation corresponds to the one commonly found in datasheets for displays.
+If a display supports multiple signal timings, the native-mode can be specified.
+
+The parameters are defined as
+
+struct display_timing
+=========+
+  +----------+---------------------------------------------+----------+-------+
+  |          |                ↑                            |          |       |
+  |          |                |vback_porch                 |          |       |
+  |          |                ↓                            |          |       |
+  +----------###############################################----------+-------+
+  |          #                ↑                            #          |       |
+  |          #                |                            #          |       |
+  |  hback   #                |                            #  hfront  | hsync |
+  |   porch  #                |       hactive              #  porch   |  len  |
+  |<-------->#<---------------+--------------------------->#<-------->|<----->|
+  |          #                |                            #          |       |
+  |          #                |vactive                     #          |       |
+  |          #                |                            #          |       |
+  |          #                ↓                            #          |       |
+  +----------###############################################----------+-------+
+  |          |                ↑                            |          |       |
+  |          |                |vfront_porch                |          |       |
+  |          |                ↓                            |          |       |
+  +----------+---------------------------------------------+----------+-------+
+  |          |                ↑                            |          |       |
+  |          |                |vsync_len                   |          |       |
+  |          |                ↓                            |          |       |
+  +----------+---------------------------------------------+----------+-------+
+
+
+Example:
+
+	display-timings {
+		native-mode = <&timing0>;
+		timing0: 1920p24 {
+			/* 1920x1080p24 */
+			clock = <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 = <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.
diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
index dfba3e6..781e773 100644
--- a/drivers/of/Kconfig
+++ b/drivers/of/Kconfig
@@ -83,4 +83,10 @@ config OF_MTD
 	depends on MTD
 	def_bool y
 
+config OF_DISPLAY_TIMINGS
+	def_bool y
+	depends on DISPLAY_TIMING
+	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..388fe4c
--- /dev/null
+++ b/drivers/of/of_display_timings.c
@@ -0,0 +1,185 @@
+/*
+ * OF helpers for parsing display timings
+ *
+ * Copyright (c) 2012 Steffen Trumtrar <s.trumtrar@pengutronix.de>, Pengutronix
+ *
+ * based on of_videomode.c by Sascha Hauer <s.hauer@pengutronix.de>
+ *
+ * This file is released under the GPLv2
+ */
+#include <linux/of.h>
+#include <linux/slab.h>
+#include <linux/export.h>
+#include <linux/of_display_timings.h>
+
+/**
+ * parse_property - parse timing_entry from device_node
+ * @np: device_node with the property
+ * @name: name of the property
+ * @result: will be set to the return value
+ *
+ * DESCRIPTION:
+ * Every display_timing can be specified with either just the typical value or
+ * a range consisting of min/typ/max. This function helps handling this
+ **/
+static int parse_property(struct device_node *np, char *name,
+				struct timing_entry *result)
+{
+	struct property *prop;
+	int length;
+	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);
+		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
+ **/
+struct display_timing *of_get_display_timing(struct device_node *np)
+{
+	struct display_timing *dt;
+	int ret = 0;
+
+	dt = kzalloc(sizeof(*dt), GFP_KERNEL);
+	if (!dt) {
+		pr_err("%s: could not allocate display_timing struct\n", __func__);
+		return NULL;
+	}
+
+	ret |= parse_property(np, "hback-porch", &dt->hback_porch);
+	ret |= parse_property(np, "hfront-porch", &dt->hfront_porch);
+	ret |= parse_property(np, "hactive", &dt->hactive);
+	ret |= parse_property(np, "hsync-len", &dt->hsync_len);
+	ret |= parse_property(np, "vback-porch", &dt->vback_porch);
+	ret |= parse_property(np, "vfront-porch", &dt->vfront_porch);
+	ret |= parse_property(np, "vactive", &dt->vactive);
+	ret |= parse_property(np, "vsync-len", &dt->vsync_len);
+	ret |= parse_property(np, "clock-frequency", &dt->pixelclock);
+
+	of_property_read_u32(np, "vsync-active", &dt->vsync_pol_active);
+	of_property_read_u32(np, "hsync-active", &dt->hsync_pol_active);
+	of_property_read_u32(np, "de-active", &dt->de_pol_active);
+	of_property_read_u32(np, "pixelclk-inverted", &dt->pixelclk_pol);
+	dt->interlaced = of_property_read_bool(np, "interlaced");
+	dt->doublescan = of_property_read_bool(np, "doublescan");
+
+	if (ret) {
+		pr_err("%s: error reading timing properties\n", __func__);
+		return NULL;
+	}
+
+	return dt;
+}
+EXPORT_SYMBOL_GPL(of_get_display_timing);
+
+/**
+ * of_get_display_timing_list - parse all display_timing entries from a device_node
+ * @np: device_node with the subnodes
+ **/
+struct display_timings *of_get_display_timing_list(struct device_node *np)
+{
+	struct device_node *timings_np;
+	struct device_node *entry;
+	struct device_node *native_mode;
+	struct display_timings *disp;
+
+	if (!np) {
+		pr_err("%s: no devicenode given\n", __func__);
+		return NULL;
+	}
+
+	timings_np = of_find_node_by_name(np, "display-timings");
+	if (!timings_np) {
+		pr_err("%s: could not find display-timings node\n", __func__);
+		return NULL;
+	}
+
+	disp = kzalloc(sizeof(*disp), GFP_KERNEL);
+
+	entry = of_parse_phandle(timings_np, "native-mode", 0);
+	/* assume first child as native mode if none provided */
+	if (!entry)
+		entry = of_get_next_child(np, NULL);
+	if (!entry) {
+		pr_err("%s: no timing specifications given\n", __func__);
+		return NULL;
+	}
+
+	pr_info("%s: using %s as default timing\n", __func__, entry->name);
+
+	native_mode = entry;
+
+	disp->num_timings = of_get_child_count(timings_np);
+	disp->timings = kzalloc(sizeof(struct display_timing *)*disp->num_timings,
+				GFP_KERNEL);
+	disp->num_timings = 0;
+	disp->native_mode = 0;
+
+	for_each_child_of_node(timings_np, entry) {
+		struct display_timing *dt;
+
+		dt = of_get_display_timing(entry);
+		if (!dt) {
+			/* to not encourage wrong devicetrees, fail in case of an error */
+			pr_err("%s: error in timing %d\n", __func__, disp->num_timings+1);
+			return NULL;
+		}
+
+		if (native_mode = entry)
+			disp->native_mode = disp->num_timings;
+
+		disp->timings[disp->num_timings] = dt;
+		disp->num_timings++;
+	}
+	of_node_put(timings_np);
+
+	if (disp->num_timings > 0)
+		pr_info("%s: got %d timings. Using timing #%d as default\n", __func__,
+			disp->num_timings , disp->native_mode + 1);
+	else {
+		pr_err("%s: no valid timings specified\n", __func__);
+		return NULL;
+	}
+	return disp;
+}
+EXPORT_SYMBOL_GPL(of_get_display_timing_list);
+
+/**
+ * of_display_timings_exists - check if a display-timings node is provided
+ * @np: device_node with the timing
+ **/
+int of_display_timings_exists(struct device_node *np)
+{
+	struct device_node *timings_np;
+	struct device_node *default_np;
+
+	if (!np)
+		return -EINVAL;
+
+	timings_np = of_parse_phandle(np, "display-timings", 0);
+	if (!timings_np)
+		return -EINVAL;
+
+	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..e4e1f22
--- /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>
+
+#define OF_USE_NATIVE_MODE -1
+
+struct display_timings *of_get_display_timing_list(struct device_node *np);
+struct display_timing *of_get_display_timing(struct device_node *np);
+int of_display_timings_exists(struct device_node *np);
+
+#endif
-- 
1.7.10.4

Re: [PATCH v7 2/8] of: add helper to parse display timings

[Stephen Warren]

On 10/31/2012 03:28 AM, Steffen Trumtrar wrote:

Patch description? The patch defines the DT binding as well, which isn't
mentioned in the patch subject.

> new file mode 100644
> index 0000000..04c94a3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/video/display-timings.txt

> +Usage in backend
> +========

Everything before this point in the binding docs looks reasonable to me.
Everything after this point is Linux-specific/internal implementation
detail, and hence shouldn't be in the binding document.

I only read the DT binding.

Re: [PATCH v7 2/8] of: add helper to parse display timings

[Thierry Reding]

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

On Wed, Oct 31, 2012 at 10:28:02AM +0100, Steffen Trumtrar wrote:
[...]
> diff --git a/Documentation/devicetree/bindings/video/display-timings.txt b/Documentation/devicetree/bindings/video/display-timings.txt
[...]
> @@ -0,0 +1,139 @@
> +display-timings bindings
> +==================
> +
> +display-timings-node
> +------------

Maybe extend the underline to the length of the section and subsection
titles respectively?

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

Same here.

> +config OF_DISPLAY_TIMINGS
> +	def_bool y
> +	depends on DISPLAY_TIMING

Maybe this should be called OF_DISPLAY_TIMING to match DISPLAY_TIMING,
or rename DISPLAY_TIMING to DISPLAY_TIMINGS for the sake of consistency?

> +/**
> + * of_get_display_timing_list - parse all display_timing entries from a device_node
> + * @np: device_node with the subnodes
> + **/
> +struct display_timings *of_get_display_timing_list(struct device_node *np)

Perhaps this would better be named of_get_display_timings() to match the
return type?

> +	disp = kzalloc(sizeof(*disp), GFP_KERNEL);

Shouldn't you be checking this for allocation failures?

> +	disp->timings = kzalloc(sizeof(struct display_timing *)*disp->num_timings,
> +				GFP_KERNEL);

Same here.

Thierry

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

Re: [PATCH v7 2/8] of: add helper to parse display timings

[Steffen Trumtrar]

On Thu, Nov 01, 2012 at 09:15:10PM +0100, Thierry Reding wrote:
> On Wed, Oct 31, 2012 at 10:28:02AM +0100, Steffen Trumtrar wrote:
> [...]
> > diff --git a/Documentation/devicetree/bindings/video/display-timings.txt b/Documentation/devicetree/bindings/video/display-timings.txt
> [...]
> > @@ -0,0 +1,139 @@
> > +display-timings bindings
> > +=========
> > +
> > +display-timings-node
> > +------------
> 
> Maybe extend the underline to the length of the section and subsection
> titles respectively?
> 
> > +struct display_timing
> > +=========> 
> Same here.
> 
> > +config OF_DISPLAY_TIMINGS
> > +	def_bool y
> > +	depends on DISPLAY_TIMING
> 
> Maybe this should be called OF_DISPLAY_TIMING to match DISPLAY_TIMING,
> or rename DISPLAY_TIMING to DISPLAY_TIMINGS for the sake of consistency?
> 

Yes, to all three above.

> > +/**
> > + * of_get_display_timing_list - parse all display_timing entries from a device_node
> > + * @np: device_node with the subnodes
> > + **/
> > +struct display_timings *of_get_display_timing_list(struct device_node *np)
> 
> Perhaps this would better be named of_get_display_timings() to match the
> return type?
> 

Hm, I'm not really sure about that. I found it to error prone, to have a function
of_get_display_timing and of_get_display_timings. That's why I chose
of_get_display_timing_list. But you are correct, that it doesn't match the return
value. Maybe I should just make the first function static and change the name as you
suggested.

> > +	disp = kzalloc(sizeof(*disp), GFP_KERNEL);
> 
> Shouldn't you be checking this for allocation failures?
> 
> > +	disp->timings = kzalloc(sizeof(struct display_timing *)*disp->num_timings,
> > +				GFP_KERNEL);
> 
> Same here.
> 

Yes, to both.

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

[Leela Krishna Amudala]

Hello Steffen,

On Wed, Oct 31, 2012 at 2:58 PM, Steffen Trumtrar
<s.trumtrar@pengutronix.de> wrote:
> Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
> ---
>  .../devicetree/bindings/video/display-timings.txt  |  139 +++++++++++++++
>  drivers/of/Kconfig                                 |    6 +
>  drivers/of/Makefile                                |    1 +
>  drivers/of/of_display_timings.c                    |  185 ++++++++++++++++++++
>  include/linux/of_display_timings.h                 |   20 +++
>  5 files changed, 351 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..04c94a3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/video/display-timings.txt
> @@ -0,0 +1,139 @@
> +display-timings bindings
> +=========
> +
> +display-timings-node
> +------------
> +
> +required properties:
> + - none
> +
> +optional properties:
> + - native-mode: the native mode for the display, in case multiple modes are
> +               provided. When omitted, assume the first node is the native.
> +
> +timings-subnode
> +---------------
> +
> +required properties:
> + - hactive, vactive: Display resolution
> + - hfront-porch, hback-porch, hsync-len: Horizontal Display timing parameters
> +   in pixels
> +   vfront-porch, vback-porch, vsync-len: Vertical display timing parameters in
> +   lines
> + - clock-frequency: displayclock in Hz
> +
> +optional properties:
> + - hsync-active : Hsync pulse is active low/high/ignored
> + - vsync-active : Vsync pulse is active low/high/ignored
> + - de-active : Data-Enable pulse is active low/high/ignored
> + - pixelclk-inverted : pixelclock is inverted/non-inverted/ignored
> + - interlaced (bool)
> + - doublescan (bool)
> +
> +All the optional properties that are not bool follow the following logic:
> +    <1> : high active
> +    <0> : low active
> +    omitted : not used on hardware
> +
> +There are different ways of describing the capabilities of a display. The devicetree
> +representation corresponds to the one commonly found in datasheets for displays.
> +If a display supports multiple signal timings, the native-mode can be specified.
> +
> +The parameters are defined as
> +
> +struct display_timing
> +=========> +
> +  +----------+---------------------------------------------+----------+-------+
> +  |          |                ↑                            |          |       |
> +  |          |                |vback_porch                 |          |       |
> +  |          |                ↓                            |          |       |
> +  +----------###############################################----------+-------+
> +  |          #                ↑                            #          |       |
> +  |          #                |                            #          |       |
> +  |  hback   #                |                            #  hfront  | hsync |
> +  |   porch  #                |       hactive              #  porch   |  len  |
> +  |<-------->#<---------------+--------------------------->#<-------->|<----->|
> +  |          #                |                            #          |       |
> +  |          #                |vactive                     #          |       |
> +  |          #                |                            #          |       |
> +  |          #                ↓                            #          |       |
> +  +----------###############################################----------+-------+
> +  |          |                ↑                            |          |       |
> +  |          |                |vfront_porch                |          |       |
> +  |          |                ↓                            |          |       |
> +  +----------+---------------------------------------------+----------+-------+
> +  |          |                ↑                            |          |       |
> +  |          |                |vsync_len                   |          |       |
> +  |          |                ↓                            |          |       |
> +  +----------+---------------------------------------------+----------+-------+
> +
> +
> +Example:
> +
> +       display-timings {
> +               native-mode = <&timing0>;
> +               timing0: 1920p24 {
> +                       /* 1920x1080p24 */
> +                       clock = <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 = <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.
> diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
> index dfba3e6..781e773 100644
> --- a/drivers/of/Kconfig
> +++ b/drivers/of/Kconfig
> @@ -83,4 +83,10 @@ config OF_MTD
>         depends on MTD
>         def_bool y
>
> +config OF_DISPLAY_TIMINGS
> +       def_bool y
> +       depends on DISPLAY_TIMING
> +       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..388fe4c
> --- /dev/null
> +++ b/drivers/of/of_display_timings.c
> @@ -0,0 +1,185 @@
> +/*
> + * OF helpers for parsing display timings
> + *
> + * Copyright (c) 2012 Steffen Trumtrar <s.trumtrar@pengutronix.de>, Pengutronix
> + *
> + * based on of_videomode.c by Sascha Hauer <s.hauer@pengutronix.de>
> + *
> + * This file is released under the GPLv2
> + */
> +#include <linux/of.h>
> +#include <linux/slab.h>
> +#include <linux/export.h>
> +#include <linux/of_display_timings.h>
> +
> +/**
> + * parse_property - parse timing_entry from device_node
> + * @np: device_node with the property
> + * @name: name of the property
> + * @result: will be set to the return value
> + *
> + * DESCRIPTION:
> + * Every display_timing can be specified with either just the typical value or
> + * a range consisting of min/typ/max. This function helps handling this
> + **/
> +static int parse_property(struct device_node *np, char *name,
> +                               struct timing_entry *result)
> +{
> +       struct property *prop;
> +       int length;
> +       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);

As you are reading only one vaue, you can use of_property_read_u32 instead.

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

You are considering only min element, what about typ and max elements?

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

Here it should return success instead of -EINVAL.

And one query.. are the binding properties names and "display-timings"
node structure template  finalized..?

Best Wishes,
Leela Krishna Amudala.

> +}
> +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..e4e1f22
> --- /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>
> +
> +#define OF_USE_NATIVE_MODE -1
> +
> +struct display_timings *of_get_display_timing_list(struct device_node *np);
> +struct display_timing *of_get_display_timing(struct device_node *np);
> +int of_display_timings_exists(struct device_node *np);
> +
> +#endif
> --
> 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 v7 2/8] of: add helper to parse display timings

[Steffen Trumtrar]

Hi!

On Fri, Nov 02, 2012 at 10:49:47PM +0530, Leela Krishna Amudala wrote:
> Hello Steffen,
> 
> On Wed, Oct 31, 2012 at 2:58 PM, Steffen Trumtrar
> > +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);
> 
> As you are reading only one vaue, you can use of_property_read_u32 instead.
> 

Yes, thats copypasta, no need for _array here.

> > +               result->min = result->typ;
> > +               result->max = result->typ;
> > +       } else if (cells = 3) {
> > +               ret = of_property_read_u32_array(np, name, &result->min, cells);
> 
> You are considering only min element, what about typ and max elements?
> 

I start at the address of result->min and read three u32-values, therefore all
three (min,typ,max) are filled with values.

> > +
> > +/**
> > + * of_display_timings_exists - check if a display-timings node is provided
> > + * @np: device_node with the timing
> > + **/
> > +int of_display_timings_exists(struct device_node *np)
> > +{
> > +       struct device_node *timings_np;
> > +       struct device_node *default_np;
> > +
> > +       if (!np)
> > +               return -EINVAL;
> > +
> > +       timings_np = of_parse_phandle(np, "display-timings", 0);
> > +       if (!timings_np)
> > +               return -EINVAL;
> > +
> > +       return -EINVAL;
> 
> Here it should return success instead of -EINVAL.
> 

Yes.

> And one query.. are the binding properties names and "display-timings"
> node structure template  finalized..?
> 

I sure hope so. There actually is one error in the examples though.
The property clock is called clock-frequency. I included it correctly
at the top of display-timings.txt, but overlooked it in the examples.

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 |

[PATCH v7 3/8] 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: Philipp Zabel <p.zabel@pengutronix.de>
Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
---
 drivers/of/Kconfig           |    6 ++++++
 drivers/of/Makefile          |    1 +
 drivers/of/of_videomode.c    |   47 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/of_videomode.h |   15 ++++++++++++++
 4 files changed, 69 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 781e773..0575ffe 100644
--- a/drivers/of/Kconfig
+++ b/drivers/of/Kconfig
@@ -89,4 +89,10 @@ config OF_DISPLAY_TIMINGS
 	help
 	  helper to parse display timings from the devicetree
 
+config OF_VIDEOMODE
+	def_bool y
+	depends on VIDEOMODE
+	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..91a26fc
--- /dev/null
+++ b/drivers/of/of_videomode.c
@@ -0,0 +1,47 @@
+/*
+ * generic videomode helper
+ *
+ * Copyright (c) 2012 Steffen Trumtrar <s.trumtrar@pengutronix.de>, Pengutronix
+ *
+ * This file is released under the GPLv2
+ */
+#include <linux/of.h>
+#include <linux/of_display_timings.h>
+#include <linux/of_videomode.h>
+#include <linux/export.h>
+
+/**
+ * of_get_videomode - get the videomode #<index> from devicetree
+ * @np - devicenode with the display_timings
+ * @vm - set to return value
+ * @index - index into list of display_timings
+ * DESCRIPTION:
+ * Get a list of all display timings and put the one
+ * specified by index into *vm. This function should only be used, if
+ * only one videomode is to be retrieved. A driver that needs to work
+ * with multiple/all videomodes should work with
+ * of_get_display_timing_list instead.
+ **/
+int of_get_videomode(struct device_node *np, struct videomode *vm, int index)
+{
+	struct display_timings *disp;
+	int ret;
+
+	disp = of_get_display_timing_list(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_videomode.h b/include/linux/of_videomode.h
new file mode 100644
index 0000000..01518e6
--- /dev/null
+++ b/include/linux/of_videomode.h
@@ -0,0 +1,15 @@
+/*
+ * Copyright 2012 Steffen Trumtrar <s.trumtrar@pengutronix.de>
+ *
+ * videomode of-helpers
+ *
+ * This file is released under the GPLv2
+ */
+
+#ifndef __LINUX_OF_VIDEOMODE_H
+#define __LINUX_OF_VIDEOMODE_H
+
+#include <linux/videomode.h>
+
+int of_get_videomode(struct device_node *np, struct videomode *vm, int index);
+#endif /* __LINUX_OF_VIDEOMODE_H */
-- 
1.7.10.4

Re: [PATCH v7 3/8] of: add generic videomode description

[Thierry Reding]

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

On Wed, Oct 31, 2012 at 10:28:03AM +0100, Steffen Trumtrar wrote:
[...]
> +config OF_VIDEOMODE
> +	def_bool y
> +	depends on VIDEOMODE
> +	help
> +	  helper to get videomodes from the devicetree
> +

I think patches 3 and 4 need to be swapped, since patch 4 introduces the
VIDEOMODE Kconfig symbol (as well as the videomode.h helper) that patch
3 uses.

Thierry

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

[PATCH v7 4/8] video: add videomode helpers

[Steffen Trumtrar]

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

Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
---
 drivers/video/Kconfig     |    6 ++++++
 drivers/video/Makefile    |    1 +
 drivers/video/videomode.c |   44 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/videomode.h |   36 ++++++++++++++++++++++++++++++++++++
 4 files changed, 87 insertions(+)
 create mode 100644 drivers/video/videomode.c
 create mode 100644 include/linux/videomode.h

diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
index 1421fc8..45dd393 100644
--- a/drivers/video/Kconfig
+++ b/drivers/video/Kconfig
@@ -38,6 +38,12 @@ config DISPLAY_TIMING
        help
          Say Y here, to use the display timing helpers.
 
+config VIDEOMODE
+       bool "Enable videomode helpers"
+       help
+         Say Y here, to use the generic videomode helpers. This allows
+	 converting from display timings to fb_videomode and drm_display_mode
+
 menuconfig FB
 	tristate "Support for frame buffer devices"
 	---help---
diff --git a/drivers/video/Makefile b/drivers/video/Makefile
index 552c045..fc30439 100644
--- a/drivers/video/Makefile
+++ b/drivers/video/Makefile
@@ -168,3 +168,4 @@ obj-$(CONFIG_FB_VIRTUAL)          += vfb.o
 #video output switch sysfs driver
 obj-$(CONFIG_VIDEO_OUTPUT_CONTROL) += output.o
 obj-$(CONFIG_DISPLAY_TIMING) += display_timing.o
+obj-$(CONFIG_VIDEOMODE) += videomode.o
diff --git a/drivers/video/videomode.c b/drivers/video/videomode.c
new file mode 100644
index 0000000..a9fe010
--- /dev/null
+++ b/drivers/video/videomode.c
@@ -0,0 +1,44 @@
+/*
+ * generic display timing functions
+ *
+ * Copyright (c) 2012 Steffen Trumtrar <s.trumtrar@pengutronix.de>, Pengutronix
+ *
+ * This file is released under the GPLv2
+ */
+
+#include <linux/kernel.h>
+#include <linux/export.h>
+#include <linux/errno.h>
+#include <linux/display_timing.h>
+#include <linux/videomode.h>
+
+int videomode_from_timing(struct display_timings *disp, struct videomode *vm,
+			  int index)
+{
+	struct display_timing *dt = NULL;
+
+	dt = display_timings_get(disp, index);
+	if (!dt) {
+		pr_err("%s: no signal timings found\n", __func__);
+		return -EINVAL;
+	}
+
+	vm->pixelclock = display_timing_get_value(&dt->pixelclock, 0);
+	vm->hactive = display_timing_get_value(&dt->hactive, 0);
+	vm->hfront_porch = display_timing_get_value(&dt->hfront_porch, 0);
+	vm->hback_porch = display_timing_get_value(&dt->hback_porch, 0);
+	vm->hsync_len = display_timing_get_value(&dt->hsync_len, 0);
+
+	vm->vactive = display_timing_get_value(&dt->vactive, 0);
+	vm->vfront_porch = display_timing_get_value(&dt->vfront_porch, 0);
+	vm->vback_porch = display_timing_get_value(&dt->vback_porch, 0);
+	vm->vsync_len = display_timing_get_value(&dt->vsync_len, 0);
+
+	vm->vah = dt->vsync_pol_active;
+	vm->hah = dt->hsync_pol_active;
+	vm->interlaced = dt->interlaced;
+	vm->doublescan = dt->doublescan;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(videomode_from_timing);
diff --git a/include/linux/videomode.h b/include/linux/videomode.h
new file mode 100644
index 0000000..f932147
--- /dev/null
+++ b/include/linux/videomode.h
@@ -0,0 +1,36 @@
+/*
+ * Copyright 2012 Steffen Trumtrar <s.trumtrar@pengutronix.de>
+ *
+ * generic videomode description
+ *
+ * This file is released under the GPLv2
+ */
+
+#ifndef __LINUX_VIDEOMODE_H
+#define __LINUX_VIDEOMODE_H
+
+#include <linux/display_timing.h>
+
+struct videomode {
+	u32 pixelclock;
+	u32 refreshrate;
+
+	u32 hactive;
+	u32 hfront_porch;
+	u32 hback_porch;
+	u32 hsync_len;
+
+	u32 vactive;
+	u32 vfront_porch;
+	u32 vback_porch;
+	u32 vsync_len;
+
+	u32 hah;
+	u32 vah;
+	bool interlaced;
+	bool doublescan;
+};
+
+int videomode_from_timing(struct display_timings *disp, struct videomode *vm,
+			  int index);
+#endif
-- 
1.7.10.4

Re: [PATCH v7 4/8] video: add videomode helpers

[Laurent Pinchart]

Hi Steffen,

Thanks for the patch.

On Wednesday 31 October 2012 10:28:04 Steffen Trumtrar wrote:
> Add helper functions to convert from display timings to a generic videomode
> structure. This videomode can then be converted to the corresponding
> subsystem mode representation (e.g. fb_videomode).
> 
> Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
> ---
>  drivers/video/Kconfig     |    6 ++++++
>  drivers/video/Makefile    |    1 +
>  drivers/video/videomode.c |   44 ++++++++++++++++++++++++++++++++++++++++++
>  include/linux/videomode.h |   36 ++++++++++++++++++++++++++++++++++++
>  4 files changed, 87 insertions(+)
>  create mode 100644 drivers/video/videomode.c
>  create mode 100644 include/linux/videomode.h
> 
> diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
> index 1421fc8..45dd393 100644
> --- a/drivers/video/Kconfig
> +++ b/drivers/video/Kconfig
> @@ -38,6 +38,12 @@ config DISPLAY_TIMING
>         help
>           Say Y here, to use the display timing helpers.
> 
> +config VIDEOMODE
> +       bool "Enable videomode helpers"

Shouldn't this option should be automatically selected through a select 
statement in other options that depend on it instead of manually selected ? 
Same for the DISPLAY_TIMING option in 1/8.

There's so little code here, do you think it would be a good idea to merge 
patches 1/8 and 4/8 and have a single Kconfig option ?

> +       help
> +         Say Y here, to use the generic videomode helpers. This allows
> +	 converting from display timings to fb_videomode and drm_display_mode
> +
>  menuconfig FB
>  	tristate "Support for frame buffer devices"
>  	---help---
> diff --git a/drivers/video/Makefile b/drivers/video/Makefile
> index 552c045..fc30439 100644
> --- a/drivers/video/Makefile
> +++ b/drivers/video/Makefile
> @@ -168,3 +168,4 @@ obj-$(CONFIG_FB_VIRTUAL)          += vfb.o
>  #video output switch sysfs driver
>  obj-$(CONFIG_VIDEO_OUTPUT_CONTROL) += output.o
>  obj-$(CONFIG_DISPLAY_TIMING) += display_timing.o
> +obj-$(CONFIG_VIDEOMODE) += videomode.o
> diff --git a/drivers/video/videomode.c b/drivers/video/videomode.c
> new file mode 100644
> index 0000000..a9fe010
> --- /dev/null
> +++ b/drivers/video/videomode.c
> @@ -0,0 +1,44 @@
> +/*
> + * generic display timing functions
> + *
> + * Copyright (c) 2012 Steffen Trumtrar <s.trumtrar@pengutronix.de>,
> Pengutronix + *
> + * This file is released under the GPLv2
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/export.h>
> +#include <linux/errno.h>
> +#include <linux/display_timing.h>
> +#include <linux/videomode.h>

As in 1/8, I try to keep #include's sorted alphabetically, but I won't push 
for it here either :-)

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

unsigned int for index ?

> +{
> +	struct display_timing *dt = NULL;

No need to initialize dt to NULL.

> +	dt = display_timings_get(disp, index);
> +	if (!dt) {
> +		pr_err("%s: no signal timings found\n", __func__);

I wonder whether this really deserves a pr_err() here. Would this be a caller 
bug, or can there be valid use cases where this function would return an error 
?

> +		return -EINVAL;
> +	}
> +
> +	vm->pixelclock = display_timing_get_value(&dt->pixelclock, 0);
> +	vm->hactive = display_timing_get_value(&dt->hactive, 0);
> +	vm->hfront_porch = display_timing_get_value(&dt->hfront_porch, 0);
> +	vm->hback_porch = display_timing_get_value(&dt->hback_porch, 0);
> +	vm->hsync_len = display_timing_get_value(&dt->hsync_len, 0);
> +
> +	vm->vactive = display_timing_get_value(&dt->vactive, 0);
> +	vm->vfront_porch = display_timing_get_value(&dt->vfront_porch, 0);
> +	vm->vback_porch = display_timing_get_value(&dt->vback_porch, 0);
> +	vm->vsync_len = display_timing_get_value(&dt->vsync_len, 0);
> +
> +	vm->vah = dt->vsync_pol_active;
> +	vm->hah = dt->hsync_pol_active;
> +	vm->interlaced = dt->interlaced;
> +	vm->doublescan = dt->doublescan;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(videomode_from_timing);
> diff --git a/include/linux/videomode.h b/include/linux/videomode.h
> new file mode 100644
> index 0000000..f932147
> --- /dev/null
> +++ b/include/linux/videomode.h
> @@ -0,0 +1,36 @@
> +/*
> + * Copyright 2012 Steffen Trumtrar <s.trumtrar@pengutronix.de>
> + *
> + * generic videomode description
> + *
> + * This file is released under the GPLv2
> + */
> +
> +#ifndef __LINUX_VIDEOMODE_H
> +#define __LINUX_VIDEOMODE_H
> +
> +#include <linux/display_timing.h>
> +
> +struct videomode {
> +	u32 pixelclock;
> +	u32 refreshrate;
> +
> +	u32 hactive;
> +	u32 hfront_porch;
> +	u32 hback_porch;
> +	u32 hsync_len;
> +
> +	u32 vactive;
> +	u32 vfront_porch;
> +	u32 vback_porch;
> +	u32 vsync_len;
> +
> +	u32 hah;
> +	u32 vah;
> +	bool interlaced;
> +	bool doublescan;
> +};
> +
> +int videomode_from_timing(struct display_timings *disp, struct videomode
> *vm,
> +			  int index);
> +#endif
-- 
Regards,

Laurent Pinchart

[PATCH v7 5/8] fbmon: add videomode helpers

[Steffen Trumtrar]

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

Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
---
 drivers/video/fbmon.c |   36 ++++++++++++++++++++++++++++++++++++
 include/linux/fb.h    |    2 ++
 2 files changed, 38 insertions(+)

diff --git a/drivers/video/fbmon.c b/drivers/video/fbmon.c
index cef6557..b9e6ab3 100644
--- a/drivers/video/fbmon.c
+++ b/drivers/video/fbmon.c
@@ -1373,6 +1373,42 @@ int fb_get_mode(int flags, u32 val, struct fb_var_screeninfo *var, struct fb_inf
 	kfree(timings);
 	return err;
 }
+
+#if IS_ENABLED(CONFIG_VIDEOMODE)
+int videomode_to_fb_videomode(struct videomode *vm, struct fb_videomode *fbmode)
+{
+	fbmode->xres = vm->hactive;
+	fbmode->left_margin = vm->hback_porch;
+	fbmode->right_margin = vm->hfront_porch;
+	fbmode->hsync_len = vm->hsync_len;
+
+	fbmode->yres = vm->vactive;
+	fbmode->upper_margin = vm->vback_porch;
+	fbmode->lower_margin = vm->vfront_porch;
+	fbmode->vsync_len = vm->vsync_len;
+
+	fbmode->pixclock = KHZ2PICOS(vm->pixelclock / 1000);
+
+	fbmode->sync = 0;
+	fbmode->vmode = 0;
+	if (vm->hah)
+		fbmode->sync |= FB_SYNC_HOR_HIGH_ACT;
+	if (vm->vah)
+		fbmode->sync |= FB_SYNC_VERT_HIGH_ACT;
+	if (vm->interlaced)
+		fbmode->vmode |= FB_VMODE_INTERLACED;
+	if (vm->doublescan)
+		fbmode->vmode |= FB_VMODE_DOUBLE;
+
+	fbmode->refresh = 60;
+	fbmode->flag = 0;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(videomode_to_fb_videomode);
+#endif
+
+
 #else
 int fb_parse_edid(unsigned char *edid, struct fb_var_screeninfo *var)
 {
diff --git a/include/linux/fb.h b/include/linux/fb.h
index c7a9571..46c665b 100644
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -714,6 +714,8 @@ extern void fb_destroy_modedb(struct fb_videomode *modedb);
 extern int fb_find_mode_cvt(struct fb_videomode *mode, int margins, int rb);
 extern unsigned char *fb_ddc_read(struct i2c_adapter *adapter);
 
+extern int videomode_to_fb_videomode(struct videomode *vm, struct fb_videomode *fbmode);
+
 /* drivers/video/modedb.c */
 #define VESA_MODEDB_SIZE 34
 extern void fb_var_to_videomode(struct fb_videomode *mode,
-- 
1.7.10.4

RE: [PATCH v7 5/8] fbmon: add videomode helpers

[Manjunathappa, Prakash]

Hi Steffen,

On Wed, Oct 31, 2012 at 14:58:05, 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>
> ---
>  drivers/video/fbmon.c |   36 ++++++++++++++++++++++++++++++++++++
>  include/linux/fb.h    |    2 ++
>  2 files changed, 38 insertions(+)
> 
> diff --git a/drivers/video/fbmon.c b/drivers/video/fbmon.c
> index cef6557..b9e6ab3 100644
> --- a/drivers/video/fbmon.c
> +++ b/drivers/video/fbmon.c
> @@ -1373,6 +1373,42 @@ int fb_get_mode(int flags, u32 val, struct fb_var_screeninfo *var, struct fb_inf
>  	kfree(timings);
>  	return err;
>  }
> +
> +#if IS_ENABLED(CONFIG_VIDEOMODE)
> +int videomode_to_fb_videomode(struct videomode *vm, struct fb_videomode *fbmode)
> +{
> +	fbmode->xres = vm->hactive;
> +	fbmode->left_margin = vm->hback_porch;
> +	fbmode->right_margin = vm->hfront_porch;
> +	fbmode->hsync_len = vm->hsync_len;
> +
> +	fbmode->yres = vm->vactive;
> +	fbmode->upper_margin = vm->vback_porch;
> +	fbmode->lower_margin = vm->vfront_porch;
> +	fbmode->vsync_len = vm->vsync_len;
> +
> +	fbmode->pixclock = KHZ2PICOS(vm->pixelclock / 1000);
> +
> +	fbmode->sync = 0;
> +	fbmode->vmode = 0;
> +	if (vm->hah)
> +		fbmode->sync |= FB_SYNC_HOR_HIGH_ACT;
> +	if (vm->vah)
> +		fbmode->sync |= FB_SYNC_VERT_HIGH_ACT;
> +	if (vm->interlaced)
> +		fbmode->vmode |= FB_VMODE_INTERLACED;
> +	if (vm->doublescan)
> +		fbmode->vmode |= FB_VMODE_DOUBLE;
> +

"pixelclk-inverted" property of the panel is not percolated fb_videomode.
Please let me know if I am missing something.

Thanks,
Prakash

> +	fbmode->refresh = 60;
> +	fbmode->flag = 0;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(videomode_to_fb_videomode);
> +#endif
> +
> +
>  #else
>  int fb_parse_edid(unsigned char *edid, struct fb_var_screeninfo *var)
>  {
> diff --git a/include/linux/fb.h b/include/linux/fb.h
> index c7a9571..46c665b 100644
> --- a/include/linux/fb.h
> +++ b/include/linux/fb.h
> @@ -714,6 +714,8 @@ extern void fb_destroy_modedb(struct fb_videomode *modedb);
>  extern int fb_find_mode_cvt(struct fb_videomode *mode, int margins, int rb);
>  extern unsigned char *fb_ddc_read(struct i2c_adapter *adapter);
>  
> +extern int videomode_to_fb_videomode(struct videomode *vm, struct fb_videomode *fbmode);
> +
>  /* drivers/video/modedb.c */
>  #define VESA_MODEDB_SIZE 34
>  extern void fb_var_to_videomode(struct fb_videomode *mode,
> -- 
> 1.7.10.4
> 
> --
> 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 v7 5/8] fbmon: add videomode helpers

[Steffen Trumtrar]

Hi Prakash!

On Wed, Oct 31, 2012 at 03:30:03PM +0000, Manjunathappa, Prakash wrote:
> Hi Steffen,
> 
> On Wed, Oct 31, 2012 at 14:58:05, 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>
> > ---
> >  drivers/video/fbmon.c |   36 ++++++++++++++++++++++++++++++++++++
> >  include/linux/fb.h    |    2 ++
> >  2 files changed, 38 insertions(+)
> > 
> > diff --git a/drivers/video/fbmon.c b/drivers/video/fbmon.c
> > index cef6557..b9e6ab3 100644
> > --- a/drivers/video/fbmon.c
> > +++ b/drivers/video/fbmon.c
> > @@ -1373,6 +1373,42 @@ int fb_get_mode(int flags, u32 val, struct fb_var_screeninfo *var, struct fb_inf
> >  	kfree(timings);
> >  	return err;
> >  }
> > +
> > +#if IS_ENABLED(CONFIG_VIDEOMODE)
> > +int videomode_to_fb_videomode(struct videomode *vm, struct fb_videomode *fbmode)
> > +{
> > +	fbmode->xres = vm->hactive;
> > +	fbmode->left_margin = vm->hback_porch;
> > +	fbmode->right_margin = vm->hfront_porch;
> > +	fbmode->hsync_len = vm->hsync_len;
> > +
> > +	fbmode->yres = vm->vactive;
> > +	fbmode->upper_margin = vm->vback_porch;
> > +	fbmode->lower_margin = vm->vfront_porch;
> > +	fbmode->vsync_len = vm->vsync_len;
> > +
> > +	fbmode->pixclock = KHZ2PICOS(vm->pixelclock / 1000);
> > +
> > +	fbmode->sync = 0;
> > +	fbmode->vmode = 0;
> > +	if (vm->hah)
> > +		fbmode->sync |= FB_SYNC_HOR_HIGH_ACT;
> > +	if (vm->vah)
> > +		fbmode->sync |= FB_SYNC_VERT_HIGH_ACT;
> > +	if (vm->interlaced)
> > +		fbmode->vmode |= FB_VMODE_INTERLACED;
> > +	if (vm->doublescan)
> > +		fbmode->vmode |= FB_VMODE_DOUBLE;
> > +
> 
> "pixelclk-inverted" property of the panel is not percolated fb_videomode.
> Please let me know if I am missing something.
> 

You are right. I forgot 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 v7 5/8] fbmon: add videomode helpers

[Steffen Trumtrar]

Hi!

On Wed, Oct 31, 2012 at 03:30:03PM +0000, Manjunathappa, Prakash wrote:
> Hi Steffen,
> 
> On Wed, Oct 31, 2012 at 14:58:05, Steffen Trumtrar wrote:
> > +#if IS_ENABLED(CONFIG_VIDEOMODE)
> > +int videomode_to_fb_videomode(struct videomode *vm, struct fb_videomode *fbmode)
> > +{
> > +	fbmode->xres = vm->hactive;
> > +	fbmode->left_margin = vm->hback_porch;
> > +	fbmode->right_margin = vm->hfront_porch;
> > +	fbmode->hsync_len = vm->hsync_len;
> > +
> > +	fbmode->yres = vm->vactive;
> > +	fbmode->upper_margin = vm->vback_porch;
> > +	fbmode->lower_margin = vm->vfront_porch;
> > +	fbmode->vsync_len = vm->vsync_len;
> > +
> > +	fbmode->pixclock = KHZ2PICOS(vm->pixelclock / 1000);
> > +
> > +	fbmode->sync = 0;
> > +	fbmode->vmode = 0;
> > +	if (vm->hah)
> > +		fbmode->sync |= FB_SYNC_HOR_HIGH_ACT;
> > +	if (vm->vah)
> > +		fbmode->sync |= FB_SYNC_VERT_HIGH_ACT;
> > +	if (vm->interlaced)
> > +		fbmode->vmode |= FB_VMODE_INTERLACED;
> > +	if (vm->doublescan)
> > +		fbmode->vmode |= FB_VMODE_DOUBLE;
> > +
> 
> "pixelclk-inverted" property of the panel is not percolated fb_videomode.
> Please let me know if I am missing something.
> 

The next version is almost finished. Only thing I'm missing is this.
And I actually do not know which flag would represent an inverted pixelclock
in fb_videomode. Does anybody have any idea what I have to do here?

	if (vm->pixelclk_pol)
		fbmode->sync = ???

That's as far as I have come and I don't see a flag that seems right.
Is this even a valid property of fb_videomode?

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 v7 5/8] fbmon: add videomode helpers

[Manjunathappa, Prakash]

Hi Steffen,

On Fri, Nov 09, 2012 at 02:55:45, Steffen Trumtrar wrote:
> Hi!
> 
> On Wed, Oct 31, 2012 at 03:30:03PM +0000, Manjunathappa, Prakash wrote:
> > Hi Steffen,
> > 
> > On Wed, Oct 31, 2012 at 14:58:05, Steffen Trumtrar wrote:
> > > +#if IS_ENABLED(CONFIG_VIDEOMODE)
> > > +int videomode_to_fb_videomode(struct videomode *vm, struct fb_videomode *fbmode)
> > > +{
> > > +	fbmode->xres = vm->hactive;
> > > +	fbmode->left_margin = vm->hback_porch;
> > > +	fbmode->right_margin = vm->hfront_porch;
> > > +	fbmode->hsync_len = vm->hsync_len;
> > > +
> > > +	fbmode->yres = vm->vactive;
> > > +	fbmode->upper_margin = vm->vback_porch;
> > > +	fbmode->lower_margin = vm->vfront_porch;
> > > +	fbmode->vsync_len = vm->vsync_len;
> > > +
> > > +	fbmode->pixclock = KHZ2PICOS(vm->pixelclock / 1000);
> > > +
> > > +	fbmode->sync = 0;
> > > +	fbmode->vmode = 0;
> > > +	if (vm->hah)
> > > +		fbmode->sync |= FB_SYNC_HOR_HIGH_ACT;
> > > +	if (vm->vah)
> > > +		fbmode->sync |= FB_SYNC_VERT_HIGH_ACT;
> > > +	if (vm->interlaced)
> > > +		fbmode->vmode |= FB_VMODE_INTERLACED;
> > > +	if (vm->doublescan)
> > > +		fbmode->vmode |= FB_VMODE_DOUBLE;
> > > +
> > 
> > "pixelclk-inverted" property of the panel is not percolated fb_videomode.
> > Please let me know if I am missing something.
> > 
> 
> The next version is almost finished. Only thing I'm missing is this.
> And I actually do not know which flag would represent an inverted pixelclock
> in fb_videomode. Does anybody have any idea what I have to do here?
> 
> 	if (vm->pixelclk_pol)
> 		fbmode->sync = ???
> 
> That's as far as I have come and I don't see a flag that seems right.
> Is this even a valid property of fb_videomode?
> 

Thanks for considering it, I see IMX addresses it as proprietary FB_SYNC_ flag.
FB_SYNC_CLK_INVERT: arch/arm/plat-mxc/include/mach/mx3fb.h

Thanks,
Prakash

Re: [PATCH v7 5/8] fbmon: add videomode helpers

[Steffen Trumtrar]

Hi!

On Fri, Nov 09, 2012 at 04:54:16PM +0000, Manjunathappa, Prakash wrote:
> Hi Steffen,
> 
> On Fri, Nov 09, 2012 at 02:55:45, Steffen Trumtrar wrote:
> > Hi!
> > 
> > On Wed, Oct 31, 2012 at 03:30:03PM +0000, Manjunathappa, Prakash wrote:
> > > Hi Steffen,
> > > 
> > > On Wed, Oct 31, 2012 at 14:58:05, Steffen Trumtrar wrote:
> > > > +#if IS_ENABLED(CONFIG_VIDEOMODE)
> > > > +int videomode_to_fb_videomode(struct videomode *vm, struct fb_videomode *fbmode)
> > > > +{
> > > > +	fbmode->xres = vm->hactive;
> > > > +	fbmode->left_margin = vm->hback_porch;
> > > > +	fbmode->right_margin = vm->hfront_porch;
> > > > +	fbmode->hsync_len = vm->hsync_len;
> > > > +
> > > > +	fbmode->yres = vm->vactive;
> > > > +	fbmode->upper_margin = vm->vback_porch;
> > > > +	fbmode->lower_margin = vm->vfront_porch;
> > > > +	fbmode->vsync_len = vm->vsync_len;
> > > > +
> > > > +	fbmode->pixclock = KHZ2PICOS(vm->pixelclock / 1000);
> > > > +
> > > > +	fbmode->sync = 0;
> > > > +	fbmode->vmode = 0;
> > > > +	if (vm->hah)
> > > > +		fbmode->sync |= FB_SYNC_HOR_HIGH_ACT;
> > > > +	if (vm->vah)
> > > > +		fbmode->sync |= FB_SYNC_VERT_HIGH_ACT;
> > > > +	if (vm->interlaced)
> > > > +		fbmode->vmode |= FB_VMODE_INTERLACED;
> > > > +	if (vm->doublescan)
> > > > +		fbmode->vmode |= FB_VMODE_DOUBLE;
> > > > +
> > > 
> > > "pixelclk-inverted" property of the panel is not percolated fb_videomode.
> > > Please let me know if I am missing something.
> > > 
> > 
> > The next version is almost finished. Only thing I'm missing is this.
> > And I actually do not know which flag would represent an inverted pixelclock
> > in fb_videomode. Does anybody have any idea what I have to do here?
> > 
> > 	if (vm->pixelclk_pol)
> > 		fbmode->sync = ???
> > 
> > That's as far as I have come and I don't see a flag that seems right.
> > Is this even a valid property of fb_videomode?
> > 
> 
> Thanks for considering it, I see IMX addresses it as proprietary FB_SYNC_ flag.
> FB_SYNC_CLK_INVERT: arch/arm/plat-mxc/include/mach/mx3fb.h
> 

No problem. So, it seems this flag has to be set in some imx-specific
videomode_to_fb_videomode function. It is included in the
struct videomode, so that should be no problem. But it will not be
part of this series.

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 v7 5/8] fbmon: add videomode helpers

[Laurent Pinchart]

Hi Steffen,

On Friday 09 November 2012 20:31:33 Steffen Trumtrar wrote:
> On Fri, Nov 09, 2012 at 04:54:16PM +0000, Manjunathappa, Prakash wrote:
> > On Fri, Nov 09, 2012 at 02:55:45, Steffen Trumtrar wrote:
> > > On Wed, Oct 31, 2012 at 03:30:03PM +0000, Manjunathappa, Prakash wrote:
> > > > On Wed, Oct 31, 2012 at 14:58:05, Steffen Trumtrar wrote:
> > > > > +#if IS_ENABLED(CONFIG_VIDEOMODE)
> > > > > +int videomode_to_fb_videomode(struct videomode *vm, struct
> > > > > fb_videomode *fbmode) +{
> > > > > +	fbmode->xres = vm->hactive;
> > > > > +	fbmode->left_margin = vm->hback_porch;
> > > > > +	fbmode->right_margin = vm->hfront_porch;
> > > > > +	fbmode->hsync_len = vm->hsync_len;
> > > > > +
> > > > > +	fbmode->yres = vm->vactive;
> > > > > +	fbmode->upper_margin = vm->vback_porch;
> > > > > +	fbmode->lower_margin = vm->vfront_porch;
> > > > > +	fbmode->vsync_len = vm->vsync_len;
> > > > > +
> > > > > +	fbmode->pixclock = KHZ2PICOS(vm->pixelclock / 1000);
> > > > > +
> > > > > +	fbmode->sync = 0;
> > > > > +	fbmode->vmode = 0;
> > > > > +	if (vm->hah)
> > > > > +		fbmode->sync |= FB_SYNC_HOR_HIGH_ACT;
> > > > > +	if (vm->vah)
> > > > > +		fbmode->sync |= FB_SYNC_VERT_HIGH_ACT;
> > > > > +	if (vm->interlaced)
> > > > > +		fbmode->vmode |= FB_VMODE_INTERLACED;
> > > > > +	if (vm->doublescan)
> > > > > +		fbmode->vmode |= FB_VMODE_DOUBLE;
> > > > > +
> > > > 
> > > > "pixelclk-inverted" property of the panel is not percolated
> > > > fb_videomode.
> > > > Please let me know if I am missing something.
> > > 
> > > The next version is almost finished. Only thing I'm missing is this.
> > > And I actually do not know which flag would represent an inverted
> > > pixelclock in fb_videomode. Does anybody have any idea what I have to
> > > do here?> > 
> > > 	if (vm->pixelclk_pol)
> > > 	
> > > 		fbmode->sync = ???
> > > 
> > > That's as far as I have come and I don't see a flag that seems right.
> > > Is this even a valid property of fb_videomode?
> > 
> > Thanks for considering it, I see IMX addresses it as proprietary FB_SYNC_
> > flag. FB_SYNC_CLK_INVERT: arch/arm/plat-mxc/include/mach/mx3fb.h
> 
> No problem. So, it seems this flag has to be set in some imx-specific
> videomode_to_fb_videomode function. It is included in the
> struct videomode, so that should be no problem. But it will not be
> part of this series.

A possible solution would be to standardize the FB_SYNC_CLK_INVERT flag in 
fbdev, but that's in my opinion out of scope of this patch series. iMX3 
developers, feel free to send patches :-)

-- 
Regards,

Laurent Pinchart

[PATCH v7 6/8] fbmon: add of_videomode helpers

[Steffen Trumtrar]

Add helper to get fb_videomode from devicetree.

Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
---
 drivers/video/fbmon.c |   40 ++++++++++++++++++++++++++++++++++++++++
 include/linux/fb.h    |    3 +++
 2 files changed, 43 insertions(+)

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

[PATCH v7 7/8] drm_modes: add videomode helpers

[Steffen Trumtrar]

Add conversion from videomode to drm_display_mode

Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
---
 drivers/gpu/drm/drm_modes.c |   36 ++++++++++++++++++++++++++++++++++++
 include/drm/drmP.h          |    3 +++
 2 files changed, 39 insertions(+)

diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index 59450f3..049624d 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -35,6 +35,7 @@
 #include <linux/export.h>
 #include <drm/drmP.h>
 #include <drm/drm_crtc.h>
+#include <linux/videomode.h>
 
 /**
  * drm_mode_debug_printmodeline - debug print a mode
@@ -504,6 +505,41 @@ drm_gtf_mode(struct drm_device *dev, int hdisplay, int vdisplay, int vrefresh,
 }
 EXPORT_SYMBOL(drm_gtf_mode);
 
+#if IS_ENABLED(CONFIG_VIDEOMODE)
+int videomode_to_display_mode(struct videomode *vm, struct drm_display_mode *dmode)
+{
+	dmode->hdisplay = vm->hactive;
+	dmode->hsync_start = dmode->hdisplay + vm->hfront_porch;
+	dmode->hsync_end = dmode->hsync_start + vm->hsync_len;
+	dmode->htotal = dmode->hsync_end + vm->hback_porch;
+
+	dmode->vdisplay = vm->vactive;
+	dmode->vsync_start = dmode->vdisplay + vm->vfront_porch;
+	dmode->vsync_end = dmode->vsync_start + vm->vsync_len;
+	dmode->vtotal = dmode->vsync_end + vm->vback_porch;
+
+	dmode->clock = vm->pixelclock / 1000;
+
+	dmode->flags = 0;
+	if (vm->hah)
+		dmode->flags |= DRM_MODE_FLAG_PHSYNC;
+	else
+		dmode->flags |= DRM_MODE_FLAG_NHSYNC;
+	if (vm->vah)
+		dmode->flags |= DRM_MODE_FLAG_PVSYNC;
+	else
+		dmode->flags |= DRM_MODE_FLAG_NVSYNC;
+	if (vm->interlaced)
+		dmode->flags |= DRM_MODE_FLAG_INTERLACE;
+	if (vm->doublescan)
+		dmode->flags |= DRM_MODE_FLAG_DBLSCAN;
+	drm_mode_set_name(dmode);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(videomode_to_display_mode);
+#endif
+
 /**
  * drm_mode_set_name - set the name on a mode
  * @mode: name will be set in this mode
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 3fd8280..e9fa1e3 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -56,6 +56,7 @@
 #include <linux/cdev.h>
 #include <linux/mutex.h>
 #include <linux/slab.h>
+#include <linux/videomode.h>
 #if defined(__alpha__) || defined(__powerpc__)
 #include <asm/pgtable.h>	/* For pte_wrprotect */
 #endif
@@ -1454,6 +1455,8 @@ extern struct drm_display_mode *
 drm_mode_create_from_cmdline_mode(struct drm_device *dev,
 				  struct drm_cmdline_mode *cmd);
 
+extern int videomode_to_display_mode(struct videomode *vm,
+				     struct drm_display_mode *dmode);
 /* Modesetting support */
 extern void drm_vblank_pre_modeset(struct drm_device *dev, int crtc);
 extern void drm_vblank_post_modeset(struct drm_device *dev, int crtc);
-- 
1.7.10.4

[PATCH v7 8/8] drm_modes: add of_videomode helpers

[Steffen Trumtrar]

Add helper to get drm_display_mode from devicetree.

Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
---
 drivers/gpu/drm/drm_modes.c |   42 ++++++++++++++++++++++++++++++++++++++++++
 include/drm/drmP.h          |    5 +++++
 2 files changed, 47 insertions(+)

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

Re: [PATCH v7 8/8] drm_modes: add of_videomode helpers

[Thierry Reding]

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

On Wed, Oct 31, 2012 at 10:28:08AM +0100, Steffen Trumtrar wrote:
[...]
> +/**
> + * of_get_drm_display_mode - get a drm_display_mode from devicetree
> + * @np: device_node with the timing specification
> + * @dmode: will be set to the return value
> + * @index: index into the list of display timings in devicetree
> + * 
> + * DESCRIPTION:

I don't think this is necessary.

> + * This function is expensive and should only be used, if only one mode is to be
> + * read from DT. To get multiple modes start with of_get_display_timing_list ond

You probably meant "and" at the end of this line. Also I'm not even sure
that we should be exposing this function, but rather provide a helper
which automatically adds the parsed modes to a DRM connector object.

Thierry

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

Re: [PATCH v7 0/8] of: add display helper

[Rob Herring]

On 10/31/2012 04:28 AM, Steffen Trumtrar wrote:
> Hi!
> 
> Finally, v7 of the series.
> 
> Changes since v6:
> 	- get rid of some empty lines etc.
> 	- move functions to their subsystems
> 	- split of_ from non-of_ functions
> 	- add at least some kerneldoc to some functions
> 
> Regards,
> Steffen
> 
> 
> Steffen Trumtrar (8):
>   video: add display_timing struct and helpers
>   of: add helper to parse display timings
>   of: add generic videomode description
>   video: add videomode helpers
>   fbmon: add videomode helpers
>   fbmon: add of_videomode helpers
>   drm_modes: add videomode helpers
>   drm_modes: add of_videomode helpers
> 
>  .../devicetree/bindings/video/display-timings.txt  |  139 +++++++++++++++
>  drivers/gpu/drm/drm_modes.c                        |   78 +++++++++
>  drivers/of/Kconfig                                 |   12 ++
>  drivers/of/Makefile                                |    2 +
>  drivers/of/of_display_timings.c                    |  185 ++++++++++++++++++++
>  drivers/of/of_videomode.c                          |   47 +++++

Not sure why you moved this, but please move this back to drivers/video.
We're trying to move subsystem specific pieces out of drivers/of.

Rob

>  drivers/video/Kconfig                              |   11 ++
>  drivers/video/Makefile                             |    2 +
>  drivers/video/display_timing.c                     |   24 +++
>  drivers/video/fbmon.c                              |   76 ++++++++
>  drivers/video/videomode.c                          |   44 +++++
>  include/drm/drmP.h                                 |    8 +
>  include/linux/display_timing.h                     |   69 ++++++++
>  include/linux/fb.h                                 |    5 +
>  include/linux/of_display_timings.h                 |   20 +++
>  include/linux/of_videomode.h                       |   15 ++
>  include/linux/videomode.h                          |   36 ++++
>  17 files changed, 773 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 drivers/video/display_timing.c
>  create mode 100644 drivers/video/videomode.c
>  create mode 100644 include/linux/display_timing.h
>  create mode 100644 include/linux/of_display_timings.h
>  create mode 100644 include/linux/of_videomode.h
>  create mode 100644 include/linux/videomode.h
> 

Re: [PATCH v7 0/8] of: add display helper

[Steffen Trumtrar]

On Thu, Nov 08, 2012 at 03:35:47PM -0600, Rob Herring wrote:
> On 10/31/2012 04:28 AM, Steffen Trumtrar wrote:
> > Hi!
> > 
> > Finally, v7 of the series.
> > 
> > Changes since v6:
> > 	- get rid of some empty lines etc.
> > 	- move functions to their subsystems
> > 	- split of_ from non-of_ functions
> > 	- add at least some kerneldoc to some functions
> > 
> > Regards,
> > Steffen
> > 
> > 
> > Steffen Trumtrar (8):
> >   video: add display_timing struct and helpers
> >   of: add helper to parse display timings
> >   of: add generic videomode description
> >   video: add videomode helpers
> >   fbmon: add videomode helpers
> >   fbmon: add of_videomode helpers
> >   drm_modes: add videomode helpers
> >   drm_modes: add of_videomode helpers
> > 
> >  .../devicetree/bindings/video/display-timings.txt  |  139 +++++++++++++++
> >  drivers/gpu/drm/drm_modes.c                        |   78 +++++++++
> >  drivers/of/Kconfig                                 |   12 ++
> >  drivers/of/Makefile                                |    2 +
> >  drivers/of/of_display_timings.c                    |  185 ++++++++++++++++++++
> >  drivers/of/of_videomode.c                          |   47 +++++
> 
> Not sure why you moved this, but please move this back to drivers/video.
> We're trying to move subsystem specific pieces out of drivers/of.
> 
> Rob
> 

Hm, the of_xxx part always was in drivers/of, but I can move that. No problem.

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 |