[v4] of: Add videomode helper

Commit Message

Steffen Trumtrar Sept. 19, 2012, 8:20 a.m. UTC

This patch adds a helper function for parsing videomodes from the devicetree.
The videomode can be either converted to a struct drm_display_mode or a
struct fb_videomode.

Signed-off-by: Sascha Hauer <[e-mail]>
Signed-off-by: Steffen Trumtrar <[e-mail]>
---

Hi!

changes since v3:
	- print error messages
	- free alloced memory
	- general cleanup

Regards
Steffen

 .../devicetree/bindings/video/displaymode          |   74 +++++
 drivers/of/Kconfig                                 |    5 +
 drivers/of/Makefile                                |    1 +
 drivers/of/of_videomode.c                          |  283 ++++++++++++++++++++
 include/linux/of_videomode.h                       |   56 ++++
 5 files changed, 419 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/video/displaymode
 create mode 100644 drivers/of/of_videomode.c
 create mode 100644 include/linux/of_videomode.h

Comments

Tomi Valkeinen Sept. 19, 2012, 9:19 a.m. UTC | #1

On Wed, 2012-09-19 at 10:20 +0200, Steffen Trumtrar wrote:
> This patch adds a helper function for parsing videomodes from the devicetree.
> The videomode can be either converted to a struct drm_display_mode or a
> struct fb_videomode.
> 
> Signed-off-by: Sascha Hauer <[e-mail]>
> Signed-off-by: Steffen Trumtrar <[e-mail]>
> ---
> 
> Hi!
> 
> changes since v3:
> 	- print error messages
> 	- free alloced memory
> 	- general cleanup
> 
> Regards
> Steffen
> 
>  .../devicetree/bindings/video/displaymode          |   74 +++++
>  drivers/of/Kconfig                                 |    5 +
>  drivers/of/Makefile                                |    1 +
>  drivers/of/of_videomode.c                          |  283 ++++++++++++++++++++
>  include/linux/of_videomode.h                       |   56 ++++
>  5 files changed, 419 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/video/displaymode
>  create mode 100644 drivers/of/of_videomode.c
>  create mode 100644 include/linux/of_videomode.h
> 
> diff --git a/Documentation/devicetree/bindings/video/displaymode b/Documentation/devicetree/bindings/video/displaymode
> new file mode 100644
> index 0000000..990ca52
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/video/displaymode
> @@ -0,0 +1,74 @@
> +videomode bindings
> +==================
> +
> +Required properties:
> + - hactive, vactive: Display resolution
> + - hfront-porch, hback-porch, hsync-len: Horizontal Display timing parameters
> +   in pixels
> +   vfront-porch, vback-porch, vsync-len: Vertical display timing parameters in
> +   lines
> + - clock: displayclock in Hz
> +
> +Optional properties:
> + - width-mm, height-mm: Display dimensions in mm
> + - hsync-active-high (bool): Hsync pulse is active high
> + - vsync-active-high (bool): Vsync pulse is active high
> + - interlaced (bool): This is an interlaced mode
> + - doublescan (bool): This is a doublescan mode
> +
> +There are different ways of describing a display mode. The devicetree representation
> +corresponds to the one commonly found in datasheets for displays.
> +The description of the display and its mode is split in two parts: first the display
> +properties like size in mm and (optionally) multiple subnodes with the supported modes.
> +
> +Example:
> +
> +	display@0 {
> +		width-mm = <800>;
> +		height-mm = <480>;
> +		modes {
> +			mode0: mode@0 {
> +				/* 1920x1080p24 */
> +				clock = <52000000>;
> +				hactive = <1920>;
> +				vactive = <1080>;
> +				hfront-porch = <25>;
> +				hback-porch = <25>;
> +				hsync-len = <25>;
> +				vback-porch = <2>;
> +				vfront-porch = <2>;
> +				vsync-len = <2>;
> +				hsync-active-high;
> +			};
> +		};
> +	};
> +
> +Every property also supports the use of ranges, so the commonly used datasheet
> +description with <min typ max>-tuples can be used.
> +
> +Example:
> +
> +	mode1: mode@1 {
> +		/* 1920x1080p24 */
> +		clock = <148500000>;
> +		hactive = <1920>;
> +		vactive = <1080>;
> +		hsync-len = <0 44 60>;
> +		hfront-porch = <80 88 95>;
> +		hback-porch = <100 148 160>;
> +		vfront-porch = <0 4 6>;
> +		vback-porch = <0 36 50>;
> +		vsync-len = <0 5 6>;
> +	};
> +
> +The videomode can be linked to a connector via phandles. The connector has to
> +support the display- and default-mode-property to link to and select a mode.
> +
> +Example:
> +
> +	hdmi@00120000 {
> +		status = "okay";
> +		display = <&benq>;
> +		default-mode = <&mode1>;
> +	};
> +
> diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
> index dfba3e6..a3acaa3 100644
> --- a/drivers/of/Kconfig
> +++ b/drivers/of/Kconfig
> @@ -83,4 +83,9 @@ config OF_MTD
>  	depends on MTD
>  	def_bool y
>  
> +config OF_VIDEOMODE
> +	def_bool y
> +	help
> +	  helper to parse videomodes from the devicetree
> +
>  endmenu # OF
> diff --git a/drivers/of/Makefile b/drivers/of/Makefile
> index e027f44..80e6db3 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_VIDEOMODE)	+= of_videomode.o
> diff --git a/drivers/of/of_videomode.c b/drivers/of/of_videomode.c
> new file mode 100644
> index 0000000..52bfc74
> --- /dev/null
> +++ b/drivers/of/of_videomode.c
> @@ -0,0 +1,283 @@
> +/*
> + * OF helpers for parsing display modes
> + *
> + * Copyright (c) 2012 Sascha Hauer <[e-mail]>, Pengutronix
> + * Copyright (c) 2012 Steffen Trumtrar <[e-mail]>, Pengutronix
> + *
> + * This file is released under the GPLv2
> + */
> +#include <linux/of.h>
> +#include <linux/fb.h>
> +#include <linux/export.h>
> +#include <linux/slab.h>
> +#include <drm/drmP.h>
> +#include <drm/drm_crtc.h>
> +#include <linux/of_videomode.h>
> +
> +static u32 of_video_get_value(struct mode_property *prop)
> +{
> +	return (prop->min >= prop->typ) ? prop->min : prop->typ;
> +}

Why is this needed? If the prop->min is higher than prop->typ, isn't
that an error in the DT data, and should be rejected when parsing?

> +
> +/* read property into new mode_property */
> +static int of_video_parse_property(struct device_node *np, char *name,
> +				struct mode_property *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);
> +
> +	memset(result, 0, sizeof(*result));
> +
> +	ret = of_property_read_u32_array(np, name, &result->min, cells);
> +
> +	return ret;

Ah, I guess this is the reason for the of_video_get_value... Wouldn't it
be cleaner to be more explicit here? If there's one cell, it's the
typical value, otherwise if there are 3 cells, they are min/typ/max,
else it's an error. And I think the above also trashes memory if there
happens to be 4+ cells.

> +}
> +
> +static int of_video_free(struct display *disp)
> +{
> +	int i;
> +
> +	for (i=0; i<disp->num_modes; i++)
> +		kfree(disp->modes[i]);
> +	kfree(disp->modes);
> +
> +	return 0;
> +}
> +
> +int videomode_to_display_mode(struct display *disp, struct drm_display_mode *dmode, int index)
> +{
> +	struct videomode *vm;
> +
> +	memset(dmode, 0, sizeof(*dmode));
> +
> +	if (index > disp->num_modes) {
> +		pr_err("%s: wrong index: %d from %d\n", __func__, index, disp->num_modes);
> +		return -EINVAL;
> +	}
> +
> +	vm = disp->modes[index];
> +
> +	dmode->hdisplay = of_video_get_value(&vm->hactive);
> +	dmode->hsync_start = of_video_get_value(&vm->hactive) + of_video_get_value(&vm->hfront_porch);
> +	dmode->hsync_end = of_video_get_value(&vm->hactive) + of_video_get_value(&vm->hfront_porch)
> +			+ of_video_get_value(&vm->hsync_len);
> +	dmode->htotal = of_video_get_value(&vm->hactive) + of_video_get_value(&vm->hfront_porch)
> +			+ of_video_get_value(&vm->hsync_len) + of_video_get_value(&vm->hback_porch);
> +
> +	dmode->vdisplay = of_video_get_value(&vm->vactive);
> +	dmode->vsync_start = of_video_get_value(&vm->vactive) + of_video_get_value(&vm->vfront_porch);
> +	dmode->vsync_end = of_video_get_value(&vm->vactive) + of_video_get_value(&vm->vfront_porch)
> +			+ of_video_get_value(&vm->vsync_len);
> +	dmode->vtotal = of_video_get_value(&vm->vactive) + of_video_get_value(&vm->vfront_porch) +
> +			of_video_get_value(&vm->vsync_len) + of_video_get_value(&vm->vback_porch);
> +
> +	dmode->width_mm = disp->width_mm;
> +	dmode->height_mm = disp->height_mm;
> +
> +	dmode->clock = of_video_get_value(&vm->clock) / 1000;
> +
> +	if (vm->hah)
> +		dmode->flags |= DRM_MODE_FLAG_PHSYNC;
> +	else
> +		dmode->flags |= DRM_MODE_FLAG_NHSYNC;
> +	if (vm->vah)
> +		dmode->flags |= DRM_MODE_FLAG_PVSYNC;
> +	else
> +		dmode->flags |= DRM_MODE_FLAG_NVSYNC;
> +	if (vm->interlaced)
> +		dmode->flags |= DRM_MODE_FLAG_INTERLACE;
> +	if (vm->doublescan)
> +		dmode->flags |= DRM_MODE_FLAG_DBLSCAN;
> +
> +	drm_mode_set_name(dmode);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(videomode_to_display_mode);
> +
> +int videomode_to_fb_mode(struct display *disp, struct fb_videomode *fbmode, int index)
> +{
> +	struct videomode *vm;
> +
> +	memset(fbmode, 0, sizeof(*fbmode));
> +
> +	if (index > disp->num_modes) {
> +		pr_err("%s: wrong index: %d from %d\n", __func__, index, disp->num_modes);
> +		return -EINVAL;
> +	}
> +
> +	vm = disp->modes[index];
> +
> +	fbmode->xres = of_video_get_value(&vm->hactive);
> +	fbmode->left_margin = of_video_get_value(&vm->hback_porch);
> +	fbmode->right_margin = of_video_get_value(&vm->hfront_porch);
> +	fbmode->hsync_len = of_video_get_value(&vm->hsync_len);
> +
> +	fbmode->yres = of_video_get_value(&vm->vactive);
> +	fbmode->upper_margin = of_video_get_value(&vm->vback_porch);
> +	fbmode->lower_margin = of_video_get_value(&vm->vfront_porch);
> +	fbmode->vsync_len = of_video_get_value(&vm->vsync_len);
> +
> +	fbmode->pixclock = KHZ2PICOS(of_video_get_value(&vm->clock) / 1000);
> +
> +	if (vm->hah)
> +		fbmode->sync |= FB_SYNC_HOR_HIGH_ACT;
> +	if (vm->vah)
> +		fbmode->sync |= FB_SYNC_VERT_HIGH_ACT;
> +	if (vm->interlaced)
> +		fbmode->vmode |= FB_VMODE_INTERLACED;
> +	if (vm->doublescan)
> +		fbmode->vmode |= FB_VMODE_DOUBLE;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(videomode_to_fb_mode);
> +
> +int of_get_video_modes(struct device_node *np, struct display *disp)
> +{
> +	struct device_node *display_np;
> +	struct device_node *mode_np;
> +	struct device_node *modes_np;
> +	char *default_mode;
> +
> +	int ret = 0;
> +
> +	memset(disp, 0, sizeof(*disp));
> +	disp->modes = kmalloc(sizeof(struct videomode*), GFP_KERNEL);
> +
> +	if (!np) {
> +		pr_err("%s: no node provided\n", __func__);
> +		return -EINVAL;
> +	}
> +
> +	display_np = of_parse_phandle(np, "display", 0);
> +
> +	if (!display_np) {
> +		pr_err("%s: could not find display node\n", __func__);
> +		return -EINVAL;
> +	}
> +
> +	of_property_read_u32(display_np, "width-mm", &disp->width_mm);
> +	of_property_read_u32(display_np, "height-mm", &disp->height_mm);
> +
> +	mode_np = of_parse_phandle(np, "default-mode", 0);
> +
> +	if (!mode_np) {
> +		pr_info("%s: no default-mode specified.\n", __func__);
> +		mode_np = of_find_node_by_name(np, "mode");
> +	}
> +
> +	if (!mode_np) {
> +		pr_err("%s: could not find any mode specification\n", __func__);
> +		return -EINVAL;
> +	}
> +
> +	default_mode = (char *)mode_np->full_name;
> +
> +	modes_np = of_find_node_by_name(np, "modes");
> +	for_each_child_of_node(modes_np, mode_np) {
> +		struct videomode *vm;
> +
> +		vm = kmalloc(sizeof(struct videomode*), GFP_KERNEL);
> +		disp->modes[disp->num_modes] = kmalloc(sizeof(struct videomode*), GFP_KERNEL);
> +
> +		ret |= of_video_parse_property(mode_np, "hback-porch", &vm->hback_porch);
> +		ret |= of_video_parse_property(mode_np, "hfront-porch", &vm->hfront_porch);
> +		ret |= of_video_parse_property(mode_np, "hactive", &vm->hactive);
> +		ret |= of_video_parse_property(mode_np, "hsync-len", &vm->hsync_len);
> +		ret |= of_video_parse_property(mode_np, "vback-porch", &vm->vback_porch);
> +		ret |= of_video_parse_property(mode_np, "vfront-porch", &vm->vfront_porch);
> +		ret |= of_video_parse_property(mode_np, "vactive", &vm->vactive);
> +		ret |= of_video_parse_property(mode_np, "vsync-len", &vm->vsync_len);
> +		ret |= of_video_parse_property(mode_np, "clock", &vm->clock);
> +
> +		if (ret)
> +			return -EINVAL;
> +
> +		vm->hah = of_property_read_bool(mode_np, "hsync-active-high");
> +		vm->vah = of_property_read_bool(mode_np, "vsync-active-high");
> +		vm->interlaced = of_property_read_bool(mode_np, "interlaced");
> +		vm->doublescan = of_property_read_bool(mode_np, "doublescan");
> +
> +		if (strcmp(default_mode,mode_np->full_name) == 0)
> +			disp->default_mode = disp->num_modes;
> +
> +		disp->modes[disp->num_modes] = vm;
> +		disp->num_modes++;
> +	}
> +	of_node_put(display_np);
> +
> +	pr_info("%s: found %d modelines. Using #%d as default\n", __func__,
> +		disp->num_modes, disp->default_mode + 1);
> +
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(of_get_video_modes);
> +
> +int of_video_mode_exists(struct device_node *np)
> +{
> +	struct device_node *display_np;
> +	struct device_node *mode_np;
> +
> +	if (!np)
> +		return -EINVAL;
> +
> +	display_np = of_parse_phandle(np, "display", 0);
> +
> +	if (!display_np)
> +		return -EINVAL;
> +
> +	mode_np = of_parse_phandle(np, "default-mode", 0);
> +
> +	if (mode_np)
> +		return 0;
> +
> +	return -EINVAL;
> +}
> +EXPORT_SYMBOL_GPL(of_video_mode_exists);
> +
> +int of_get_display_mode(struct device_node *np, struct drm_display_mode *dmode, int index)
> +{
> +	struct display disp;
> +
> +	of_get_video_modes(np, &disp);
> +
> +	if (index == OF_MODE_SELECTION)
> +		index = disp.default_mode;
> +	if (dmode)
> +		videomode_to_display_mode(&disp, dmode, index);
> +
> +	of_video_free(&disp);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(of_get_display_mode);
> +
> +int of_get_fb_videomode(struct device_node *np, struct fb_videomode *fbmode, int index)
> +{
> +	struct display disp;
> +
> +	of_get_video_modes(np, &disp);
> +
> +	if (index == OF_MODE_SELECTION)
> +		index = disp.default_mode;
> +	if (fbmode)
> +		videomode_to_fb_mode(&disp, fbmode, index);
> +
> +	of_video_free(&disp);
> +
> +	return 0;

This and of_get_display_mode() do not handle errors from
of_get_video_modes() nor from videomode_to_xxx_mode(). And I don't see a
reason for the if (fbmode) check (and the same for dmode), as there's no
reason to call these functions except to get the video modes.

> +}
> +EXPORT_SYMBOL_GPL(of_get_fb_videomode);
> diff --git a/include/linux/of_videomode.h b/include/linux/of_videomode.h
> new file mode 100644
> index 0000000..5571ce3
> --- /dev/null
> +++ b/include/linux/of_videomode.h
> @@ -0,0 +1,56 @@
> +/*
> + * Copyright 2012 Sascha Hauer <[e-mail]>
> + * Copyright 2012 Steffen Trumtrar <[e-mail]>
> + *
> + * OF helpers for videomodes.
> + *
> + * This file is released under the GPLv2
> + */
> +
> +#ifndef __LINUX_OF_VIDEOMODE_H
> +#define __LINUX_OF_VIDEOMODE_H
> +
> +#define OF_MODE_SELECTION -1
> +
> +struct mode_property {
> +	u32 min;
> +	u32 typ;
> +	u32 max;
> +};
> +
> +struct display {
> +	u32 width_mm;
> +	u32 height_mm;
> +	struct videomode **modes;
> +	int default_mode;
> +	int num_modes;
> +};
> +
> +/* describe videomode in terms of hardware parameters */
> +struct videomode {
> +	struct mode_property hback_porch;
> +	struct mode_property hfront_porch;
> +	struct mode_property hactive;
> +	struct mode_property hsync_len;
> +
> +	struct mode_property vback_porch;
> +	struct mode_property vfront_porch;
> +	struct mode_property vactive;
> +	struct mode_property vsync_len;
> +
> +	struct mode_property clock;
> +
> +	bool hah;
> +	bool vah;
> +	bool interlaced;
> +	bool doublescan;
> +};

I think the names display and videomode are a bit too generic here, and
could clash with names from kernel drivers. Also, the videomode is not
really a videomode, but a "template" (or something) for videomode. A
real videomode doesn't have min & max values, only the actual value.

Perhaps these should be prefixed with "of_"? Then again, they are not
really of specific either...

 Tomi

Laurent Pinchart Sept. 20, 2012, 9:29 p.m. UTC | #2

Hi,

(CC'ing the linux-media mailing list, as video modes are of interest there as 
well)

On Wednesday 19 September 2012 12:19:22 Tomi Valkeinen wrote:
> On Wed, 2012-09-19 at 10:20 +0200, Steffen Trumtrar wrote:
> > This patch adds a helper function for parsing videomodes from the
> > devicetree. The videomode can be either converted to a struct
> > drm_display_mode or a struct fb_videomode.
> > 
> > Signed-off-by: Sascha Hauer <[e-mail]>
> > Signed-off-by: Steffen Trumtrar <[e-mail]>
> > ---
> > 
> > Hi!
> > 
> > changes since v3:
> > 	- print error messages
> > 	- free alloced memory
> > 	- general cleanup
> > 
> > Regards
> > Steffen
> > 
> >  .../devicetree/bindings/video/displaymode          |   74 +++++
> >  drivers/of/Kconfig                                 |    5 +
> >  drivers/of/Makefile                                |    1 +
> >  drivers/of/of_videomode.c                          |  283 +++++++++++++++
> >  include/linux/of_videomode.h                       |   56 ++++
> >  5 files changed, 419 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/video/displaymode
> >  create mode 100644 drivers/of/of_videomode.c
> >  create mode 100644 include/linux/of_videomode.h
> > 
> > diff --git a/Documentation/devicetree/bindings/video/displaymode
> > b/Documentation/devicetree/bindings/video/displaymode new file mode
> > 100644
> > index 0000000..990ca52
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/video/displaymode
> > @@ -0,0 +1,74 @@
> > +videomode bindings
> > +==================
> > +
> > +Required properties:
> > + - hactive, vactive: Display resolution
> > + - hfront-porch, hback-porch, hsync-len: Horizontal Display timing
> > parameters
> > +   in pixels
> > +   vfront-porch, vback-porch, vsync-len: Vertical display timing
> > parameters in
> > +   lines
> > + - clock: displayclock in Hz
> > +
> > +Optional properties:
> > + - width-mm, height-mm: Display dimensions in mm
> > + - hsync-active-high (bool): Hsync pulse is active high
> > + - vsync-active-high (bool): Vsync pulse is active high
> > + - interlaced (bool): This is an interlaced mode
> > + - doublescan (bool): This is a doublescan mode
> > +
> > +There are different ways of describing a display mode. The devicetree
> > representation
> > +corresponds to the one commonly found in datasheets for displays.
> > +The description of the display and its mode is split in two parts: first
> > the display
> > +properties like size in mm and (optionally) multiple subnodes with the
> > supported modes.
> > +
> > +Example:
> > +
> > +	display@0 {
> > +		width-mm = <800>;
> > +		height-mm = <480>;
> > +		modes {
> > +			mode0: mode@0 {
> > +				/* 1920x1080p24 */
> > +				clock = <52000000>;
> > +				hactive = <1920>;
> > +				vactive = <1080>;
> > +				hfront-porch = <25>;
> > +				hback-porch = <25>;
> > +				hsync-len = <25>;
> > +				vback-porch = <2>;
> > +				vfront-porch = <2>;
> > +				vsync-len = <2>;
> > +				hsync-active-high;
> > +			};
> > +		};
> > +	};
> > +
> > +Every property also supports the use of ranges, so the commonly used
> > datasheet +description with <min typ max>-tuples can be used.
> > +
> > +Example:
> > +
> > +	mode1: mode@1 {
> > +		/* 1920x1080p24 */
> > +		clock = <148500000>;
> > +		hactive = <1920>;
> > +		vactive = <1080>;
> > +		hsync-len = <0 44 60>;
> > +		hfront-porch = <80 88 95>;
> > +		hback-porch = <100 148 160>;
> > +		vfront-porch = <0 4 6>;
> > +		vback-porch = <0 36 50>;
> > +		vsync-len = <0 5 6>;
> > +	};
> > +
> > +The videomode can be linked to a connector via phandles. The connector
> > has to
> > +support the display- and default-mode-property to link to and select a
> > mode.
> > +
> > +Example:
> > +
> > +	hdmi@00120000 {
> > +		status = "okay";
> > +		display = <&benq>;
> > +		default-mode = <&mode1>;
> > +	};
> > +
> > diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
> > index dfba3e6..a3acaa3 100644
> > --- a/drivers/of/Kconfig
> > +++ b/drivers/of/Kconfig
> > @@ -83,4 +83,9 @@ config OF_MTD
> > 
> >  	depends on MTD
> >  	def_bool y
> > 
> > +config OF_VIDEOMODE
> > +	def_bool y
> > +	help
> > +	  helper to parse videomodes from the devicetree
> > +
> > 
> >  endmenu # OF
> > 
> > diff --git a/drivers/of/Makefile b/drivers/of/Makefile
> > index e027f44..80e6db3 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_VIDEOMODE)	+= of_videomode.o
> > diff --git a/drivers/of/of_videomode.c b/drivers/of/of_videomode.c
> > new file mode 100644
> > index 0000000..52bfc74
> > --- /dev/null
> > +++ b/drivers/of/of_videomode.c
> > @@ -0,0 +1,283 @@
> > +/*
> > + * OF helpers for parsing display modes
> > + *
> > + * Copyright (c) 2012 Sascha Hauer <[e-mail]>, Pengutronix
> > + * Copyright (c) 2012 Steffen Trumtrar <[e-mail]>,
> > Pengutronix
> > + *
> > + * This file is released under the GPLv2
> > + */
> > +#include <linux/of.h>
> > +#include <linux/fb.h>
> > +#include <linux/export.h>
> > +#include <linux/slab.h>
> > +#include <drm/drmP.h>
> > +#include <drm/drm_crtc.h>
> > +#include <linux/of_videomode.h>
> > +
> > +static u32 of_video_get_value(struct mode_property *prop)
> > +{
> > +	return (prop->min >= prop->typ) ? prop->min : prop->typ;
> > +}
> 
> Why is this needed? If the prop->min is higher than prop->typ, isn't
> that an error in the DT data, and should be rejected when parsing?
> 
> > +
> > +/* read property into new mode_property */
> > +static int of_video_parse_property(struct device_node *np, char *name,
> > +				struct mode_property *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);
> > +
> > +	memset(result, 0, sizeof(*result));
> > +
> > +	ret = of_property_read_u32_array(np, name, &result->min, cells);
> > +
> > +	return ret;
> 
> Ah, I guess this is the reason for the of_video_get_value... Wouldn't it
> be cleaner to be more explicit here? If there's one cell, it's the
> typical value, otherwise if there are 3 cells, they are min/typ/max,
> else it's an error.

I agree. We should flag errors and fail instead of trying to fix them 
silently. That's the only way to make sure that device tree authors will get 
it right (or at least less wrong). We might need to accept wrong DT data later 
to support buggy devices found in the wild, but we should not start that way.

> And I think the above also trashes memory if there happens to be 4+ cells.
> 
> > +}
> > +
> > +static int of_video_free(struct display *disp)
> > +{
> > +	int i;
> > +
> > +	for (i=0; i<disp->num_modes; i++)

Spaces around = and < please.

> > +		kfree(disp->modes[i]);
> > +	kfree(disp->modes);
> > +
> > +	return 0;
> > +}
> > +
> > +int videomode_to_display_mode(struct display *disp, struct
> > drm_display_mode *dmode, int index)

Shouldn't this use drm_mode_modeinfo instead of drm_display_mode ?

> > +{
> > +	struct videomode *vm;
> > +
> > +	memset(dmode, 0, sizeof(*dmode));
> > +
> > +	if (index > disp->num_modes) {
> > +		pr_err("%s: wrong index: %d from %d\n", __func__, index,
> > disp->num_modes); +		return -EINVAL;
> > +	}
> > +
> > +	vm = disp->modes[index];
> > +
> > +	dmode->hdisplay = of_video_get_value(&vm->hactive);
> > +	dmode->hsync_start = of_video_get_value(&vm->hactive) +
> > of_video_get_value(&vm->hfront_porch);

You could replace of_video_get_value(&vm->hactive) with dmode->hdisplay here, 
and similarly below (hsync_end = hsync_start + of_video_get_value(&vm-
>hsync_len), ...). Beside shortening lines, it would save calls to 
of_video_get_value().

> > +	dmode->hsync_end = of_video_get_value(&vm->hactive) +
> > of_video_get_value(&vm->hfront_porch)
> > +			+ of_video_get_value(&vm->hsync_len);
> > +	dmode->htotal = of_video_get_value(&vm->hactive) +
> > of_video_get_value(&vm->hfront_porch)
> > +			+ of_video_get_value(&vm->hsync_len) +
> > of_video_get_value(&vm->hback_porch);
> > +	dmode->vdisplay = of_video_get_value(&vm->vactive);
> > +	dmode->vsync_start = of_video_get_value(&vm->vactive) +
> > of_video_get_value(&vm->vfront_porch); +	dmode->vsync_end =
> > of_video_get_value(&vm->vactive) + of_video_get_value(&vm->vfront_porch)
> > +			+ of_video_get_value(&vm->vsync_len);
> > +	dmode->vtotal = of_video_get_value(&vm->vactive) +
> > of_video_get_value(&vm->vfront_porch) +
> > +			of_video_get_value(&vm->vsync_len) +
> > of_video_get_value(&vm->vback_porch); +
> > +	dmode->width_mm = disp->width_mm;
> > +	dmode->height_mm = disp->height_mm;
> > +
> > +	dmode->clock = of_video_get_value(&vm->clock) / 1000;
> > +
> > +	if (vm->hah)
> > +		dmode->flags |= DRM_MODE_FLAG_PHSYNC;
> > +	else
> > +		dmode->flags |= DRM_MODE_FLAG_NHSYNC;
> > +	if (vm->vah)
> > +		dmode->flags |= DRM_MODE_FLAG_PVSYNC;
> > +	else
> > +		dmode->flags |= DRM_MODE_FLAG_NVSYNC;
> > +	if (vm->interlaced)
> > +		dmode->flags |= DRM_MODE_FLAG_INTERLACE;
> > +	if (vm->doublescan)
> > +		dmode->flags |= DRM_MODE_FLAG_DBLSCAN;
> > +
> > +	drm_mode_set_name(dmode);
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(videomode_to_display_mode);
> > +
> > +int videomode_to_fb_mode(struct display *disp, struct fb_videomode
> > *fbmode, int index)

disp is only used to retrieve the mode, you could pass a struct videomode 
instead of disp and index.

Thinking about it, I would make videomode_to_display_mode take a struct 
videomode as well. The only reason it needs struct display is to get the 
display physical dimensions. Those are only used in very specific cases in the 
DRM subsystem, I don't think they should be copied to every struct 
drm_mode_modeinfo.

> > +{
> > +	struct videomode *vm;
> > +
> > +	memset(fbmode, 0, sizeof(*fbmode));
> > +
> > +	if (index > disp->num_modes) {
> > +		pr_err("%s: wrong index: %d from %d\n", __func__, index,
> > disp->num_modes);
> > +		return -EINVAL;
> > +	}
> > +
> > +	vm = disp->modes[index];
> > +
> > +	fbmode->xres = of_video_get_value(&vm->hactive);
> > +	fbmode->left_margin = of_video_get_value(&vm->hback_porch);
> > +	fbmode->right_margin = of_video_get_value(&vm->hfront_porch);
> > +	fbmode->hsync_len = of_video_get_value(&vm->hsync_len);
> > +
> > +	fbmode->yres = of_video_get_value(&vm->vactive);
> > +	fbmode->upper_margin = of_video_get_value(&vm->vback_porch);
> > +	fbmode->lower_margin = of_video_get_value(&vm->vfront_porch);
> > +	fbmode->vsync_len = of_video_get_value(&vm->vsync_len);
> > +
> > +	fbmode->pixclock = KHZ2PICOS(of_video_get_value(&vm->clock) / 1000);
> > +
> > +	if (vm->hah)
> > +		fbmode->sync |= FB_SYNC_HOR_HIGH_ACT;
> > +	if (vm->vah)
> > +		fbmode->sync |= FB_SYNC_VERT_HIGH_ACT;
> > +	if (vm->interlaced)
> > +		fbmode->vmode |= FB_VMODE_INTERLACED;
> > +	if (vm->doublescan)
> > +		fbmode->vmode |= FB_VMODE_DOUBLE;
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(videomode_to_fb_mode);
> > +
> > +int of_get_video_modes(struct device_node *np, struct display *disp)
> > +{
> > +	struct device_node *display_np;
> > +	struct device_node *mode_np;
> > +	struct device_node *modes_np;
> > +	char *default_mode;
> > +
> > +	int ret = 0;
> > +
> > +	memset(disp, 0, sizeof(*disp));
> > +	disp->modes = kmalloc(sizeof(struct videomode*), GFP_KERNEL);
> > +
> > +	if (!np) {
> > +		pr_err("%s: no node provided\n", __func__);
> > +		return -EINVAL;
> > +	}
> > +
> > +	display_np = of_parse_phandle(np, "display", 0);
> > +
> > +	if (!display_np) {
> > +		pr_err("%s: could not find display node\n", __func__);
> > +		return -EINVAL;
> > +	}
> > +
> > +	of_property_read_u32(display_np, "width-mm", &disp->width_mm);
> > +	of_property_read_u32(display_np, "height-mm", &disp->height_mm);
> > +
> > +	mode_np = of_parse_phandle(np, "default-mode", 0);
> > +
> > +	if (!mode_np) {
> > +		pr_info("%s: no default-mode specified.\n", __func__);
> > +		mode_np = of_find_node_by_name(np, "mode");
> > +	}
> > +
> > +	if (!mode_np) {
> > +		pr_err("%s: could not find any mode specification\n", __func__);
> > +		return -EINVAL;
> > +	}
> > +
> > +	default_mode = (char *)mode_np->full_name;
> > +
> > +	modes_np = of_find_node_by_name(np, "modes");
> > +	for_each_child_of_node(modes_np, mode_np) {
> > +		struct videomode *vm;
> > +
> > +		vm = kmalloc(sizeof(struct videomode*), GFP_KERNEL);
> > +		disp->modes[disp->num_modes] = kmalloc(sizeof(struct videomode*),
> > GFP_KERNEL);
> > +
> > +		ret |= of_video_parse_property(mode_np, "hback-porch",
> > &vm->hback_porch);
> > +		ret |= of_video_parse_property(mode_np, "hfront-porch",
> > &vm->hfront_porch);
> > +		ret |= of_video_parse_property(mode_np, "hactive", &vm->hactive);
> > +		ret |= of_video_parse_property(mode_np, "hsync-len",
> > &vm->hsync_len);
> > +		ret |= of_video_parse_property(mode_np, "vback-porch",
> > &vm->vback_porch);
> > +		ret |= of_video_parse_property(mode_np, "vfront-porch",
> > &vm->vfront_porch);
> > +		ret |= of_video_parse_property(mode_np, "vactive", &vm->vactive);
> > +		ret |= of_video_parse_property(mode_np, "vsync-len",
> > &vm->vsync_len);
> > +		ret |= of_video_parse_property(mode_np, "clock", &vm->clock);
> > +
> > +		if (ret)
> > +			return -EINVAL;
> > +
> > +		vm->hah = of_property_read_bool(mode_np, "hsync-active-high");
> > +		vm->vah = of_property_read_bool(mode_np, "vsync-active-high");
> > +		vm->interlaced = of_property_read_bool(mode_np, "interlaced");
> > +		vm->doublescan = of_property_read_bool(mode_np, "doublescan");
> > +
> > +		if (strcmp(default_mode,mode_np->full_name) == 0)
> > +			disp->default_mode = disp->num_modes;
> > +
> > +		disp->modes[disp->num_modes] = vm;
> > +		disp->num_modes++;
> > +	}
> > +	of_node_put(display_np);
> > +
> > +	pr_info("%s: found %d modelines. Using #%d as default\n", __func__,
> > +		disp->num_modes, disp->default_mode + 1);
> > +
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(of_get_video_modes);
> > +
> > +int of_video_mode_exists(struct device_node *np)
> > +{
> > +	struct device_node *display_np;
> > +	struct device_node *mode_np;
> > +
> > +	if (!np)
> > +		return -EINVAL;
> > +
> > +	display_np = of_parse_phandle(np, "display", 0);
> > +
> > +	if (!display_np)
> > +		return -EINVAL;
> > +
> > +	mode_np = of_parse_phandle(np, "default-mode", 0);
> > +
> > +	if (mode_np)
> > +		return 0;
> > +
> > +	return -EINVAL;
> > +}
> > +EXPORT_SYMBOL_GPL(of_video_mode_exists);
> > +
> > +int of_get_display_mode(struct device_node *np, struct drm_display_mode
> > *dmode, int index)
> > +{
> > +	struct display disp;
> > +
> > +	of_get_video_modes(np, &disp);
> > +
> > +	if (index == OF_MODE_SELECTION)
> > +		index = disp.default_mode;
> > +	if (dmode)
> > +		videomode_to_display_mode(&disp, dmode, index);
> > +
> > +	of_video_free(&disp);
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(of_get_display_mode);
> > +
> > +int of_get_fb_videomode(struct device_node *np, struct fb_videomode
> > *fbmode, int index) 
> > +{
> > +	struct display disp;
> > +
> > +	of_get_video_modes(np, &disp);
> > +
> > +	if (index == OF_MODE_SELECTION)
> > +		index = disp.default_mode;
> > +	if (fbmode)
> > +		videomode_to_fb_mode(&disp, fbmode, index);
> > +
> > +	of_video_free(&disp);
> > +
> > +	return 0;
> 
> This and of_get_display_mode() do not handle errors from
> of_get_video_modes() nor from videomode_to_xxx_mode(). And I don't see a
> reason for the if (fbmode) check (and the same for dmode), as there's no
> reason to call these functions except to get the video modes.
> 
> > +}
> > +EXPORT_SYMBOL_GPL(of_get_fb_videomode);

What are those two functions used for ? Aren't drivers expected to parse the 
modes into a struct display and then operate on that structure, instead of 
getting individual modes from the DT node ?

> > diff --git a/include/linux/of_videomode.h b/include/linux/of_videomode.h
> > new file mode 100644
> > index 0000000..5571ce3
> > --- /dev/null
> > +++ b/include/linux/of_videomode.h
> > @@ -0,0 +1,56 @@
> > +/*
> > + * Copyright 2012 Sascha Hauer <[e-mail]>
> > + * Copyright 2012 Steffen Trumtrar <[e-mail]>
> > + *
> > + * OF helpers for videomodes.
> > + *
> > + * This file is released under the GPLv2
> > + */
> > +
> > +#ifndef __LINUX_OF_VIDEOMODE_H
> > +#define __LINUX_OF_VIDEOMODE_H
> > +
> > +#define OF_MODE_SELECTION -1
> > +
> > +struct mode_property {
> > +	u32 min;
> > +	u32 typ;
> > +	u32 max;
> > +};
> > +
> > +struct display {
> > +	u32 width_mm;
> > +	u32 height_mm;
> > +	struct videomode **modes;
> > +	int default_mode;
> > +	int num_modes;

default_mode and num_modes are non-negative, what about using an unsigned int 
type for them ?

> > +};
> > +
> > +/* describe videomode in terms of hardware parameters */
> > +struct videomode {
> > +	struct mode_property hback_porch;
> > +	struct mode_property hfront_porch;
> > +	struct mode_property hactive;
> > +	struct mode_property hsync_len;
> > +
> > +	struct mode_property vback_porch;
> > +	struct mode_property vfront_porch;
> > +	struct mode_property vactive;
> > +	struct mode_property vsync_len;
> > +
> > +	struct mode_property clock;
> > +
> > +	bool hah;
> > +	bool vah;
> > +	bool interlaced;
> > +	bool doublescan;
> > +};
> 
> I think the names display and videomode are a bit too generic here, and
> could clash with names from kernel drivers. Also, the videomode is not
> really a videomode, but a "template" (or something) for videomode. A
> real videomode doesn't have min & max values, only the actual value.
> 
> Perhaps these should be prefixed with "of_"? Then again, they are not
> really of specific either...

If feel this is an important topic.

A generic video mode structure is definitely needed, as well as helper 
functions to convert between the new structure and existing video mode 
structures (struct fb_videomode, struct drm_mode_modeinfo and struct 
v4l2_bt_timings). The structure and the helper functions should be generic, as 
the goal is to gradually replace subsystem-specific video mode structures 
where possible (userspace APIs will still need to keep the old structures). We 
thus need to drop the dependency on OF for everything but the DT parsing code. 
For that reason an of_ prefix wouldn't be a good idea. As the goal is to 
create a truly generic video mode structure, a generic name is a good idea (I 
might prefer struct video_mode instead of struct videomode, but that's just 
bikeshedding).

We might need two video mode structures, one to represent a video mode, and 
one to represent a range of video modes. I don't have enough experience with 
video modes to have a really strong opinion on this, having a single structure 
to represent both could be useful as well. This particular topic needs to be 
discussed.

Steffen Trumtrar Sept. 21, 2012, 12:47 p.m. UTC | #3

On Thu, Sep 20, 2012 at 11:29:42PM +0200, Laurent Pinchart wrote:
> Hi,
> 
> (CC'ing the linux-media mailing list, as video modes are of interest there as 
> well)
> 
> On Wednesday 19 September 2012 12:19:22 Tomi Valkeinen wrote:
> > On Wed, 2012-09-19 at 10:20 +0200, Steffen Trumtrar wrote:
> > > This patch adds a helper function for parsing videomodes from the
> > > devicetree. The videomode can be either converted to a struct
> > > drm_display_mode or a struct fb_videomode.
> > > 
> > > Signed-off-by: Sascha Hauer <[e-mail]>
> > > Signed-off-by: Steffen Trumtrar <[e-mail]>
> > > ---
> > > 
> > > Hi!
> > > 
> > > changes since v3:
> > > 	- print error messages
> > > 	- free alloced memory
> > > 	- general cleanup
> > > 
> > > Regards
> > > Steffen
> > > 
> > >  .../devicetree/bindings/video/displaymode          |   74 +++++
> > >  drivers/of/Kconfig                                 |    5 +
> > >  drivers/of/Makefile                                |    1 +
> > >  drivers/of/of_videomode.c                          |  283 +++++++++++++++
> > >  include/linux/of_videomode.h                       |   56 ++++
> > >  5 files changed, 419 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/video/displaymode
> > >  create mode 100644 drivers/of/of_videomode.c
> > >  create mode 100644 include/linux/of_videomode.h
> > > 
> > > diff --git a/Documentation/devicetree/bindings/video/displaymode
> > > b/Documentation/devicetree/bindings/video/displaymode new file mode
> > > 100644
> > > index 0000000..990ca52
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/video/displaymode
> > > @@ -0,0 +1,74 @@
> > > +videomode bindings
> > > +==================
> > > +
> > > +Required properties:
> > > + - hactive, vactive: Display resolution
> > > + - hfront-porch, hback-porch, hsync-len: Horizontal Display timing
> > > parameters
> > > +   in pixels
> > > +   vfront-porch, vback-porch, vsync-len: Vertical display timing
> > > parameters in
> > > +   lines
> > > + - clock: displayclock in Hz
> > > +
> > > +Optional properties:
> > > + - width-mm, height-mm: Display dimensions in mm
> > > + - hsync-active-high (bool): Hsync pulse is active high
> > > + - vsync-active-high (bool): Vsync pulse is active high
> > > + - interlaced (bool): This is an interlaced mode
> > > + - doublescan (bool): This is a doublescan mode
> > > +
> > > +There are different ways of describing a display mode. The devicetree
> > > representation
> > > +corresponds to the one commonly found in datasheets for displays.
> > > +The description of the display and its mode is split in two parts: first
> > > the display
> > > +properties like size in mm and (optionally) multiple subnodes with the
> > > supported modes.
> > > +
> > > +Example:
> > > +
> > > +	display@0 {
> > > +		width-mm = <800>;
> > > +		height-mm = <480>;
> > > +		modes {
> > > +			mode0: mode@0 {
> > > +				/* 1920x1080p24 */
> > > +				clock = <52000000>;
> > > +				hactive = <1920>;
> > > +				vactive = <1080>;
> > > +				hfront-porch = <25>;
> > > +				hback-porch = <25>;
> > > +				hsync-len = <25>;
> > > +				vback-porch = <2>;
> > > +				vfront-porch = <2>;
> > > +				vsync-len = <2>;
> > > +				hsync-active-high;
> > > +			};
> > > +		};
> > > +	};
> > > +
> > > +Every property also supports the use of ranges, so the commonly used
> > > datasheet +description with <min typ max>-tuples can be used.
> > > +
> > > +Example:
> > > +
> > > +	mode1: mode@1 {
> > > +		/* 1920x1080p24 */
> > > +		clock = <148500000>;
> > > +		hactive = <1920>;
> > > +		vactive = <1080>;
> > > +		hsync-len = <0 44 60>;
> > > +		hfront-porch = <80 88 95>;
> > > +		hback-porch = <100 148 160>;
> > > +		vfront-porch = <0 4 6>;
> > > +		vback-porch = <0 36 50>;
> > > +		vsync-len = <0 5 6>;
> > > +	};
> > > +
> > > +The videomode can be linked to a connector via phandles. The connector
> > > has to
> > > +support the display- and default-mode-property to link to and select a
> > > mode.
> > > +
> > > +Example:
> > > +
> > > +	hdmi@00120000 {
> > > +		status = "okay";
> > > +		display = <&benq>;
> > > +		default-mode = <&mode1>;
> > > +	};
> > > +
> > > diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
> > > index dfba3e6..a3acaa3 100644
> > > --- a/drivers/of/Kconfig
> > > +++ b/drivers/of/Kconfig
> > > @@ -83,4 +83,9 @@ config OF_MTD
> > > 
> > >  	depends on MTD
> > >  	def_bool y
> > > 
> > > +config OF_VIDEOMODE
> > > +	def_bool y
> > > +	help
> > > +	  helper to parse videomodes from the devicetree
> > > +
> > > 
> > >  endmenu # OF
> > > 
> > > diff --git a/drivers/of/Makefile b/drivers/of/Makefile
> > > index e027f44..80e6db3 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_VIDEOMODE)	+= of_videomode.o
> > > diff --git a/drivers/of/of_videomode.c b/drivers/of/of_videomode.c
> > > new file mode 100644
> > > index 0000000..52bfc74
> > > --- /dev/null
> > > +++ b/drivers/of/of_videomode.c
> > > @@ -0,0 +1,283 @@
> > > +/*
> > > + * OF helpers for parsing display modes
> > > + *
> > > + * Copyright (c) 2012 Sascha Hauer <[e-mail]>, Pengutronix
> > > + * Copyright (c) 2012 Steffen Trumtrar <[e-mail]>,
> > > Pengutronix
> > > + *
> > > + * This file is released under the GPLv2
> > > + */
> > > +#include <linux/of.h>
> > > +#include <linux/fb.h>
> > > +#include <linux/export.h>
> > > +#include <linux/slab.h>
> > > +#include <drm/drmP.h>
> > > +#include <drm/drm_crtc.h>
> > > +#include <linux/of_videomode.h>
> > > +
> > > +static u32 of_video_get_value(struct mode_property *prop)
> > > +{
> > > +	return (prop->min >= prop->typ) ? prop->min : prop->typ;
> > > +}
> > 
> > Why is this needed? If the prop->min is higher than prop->typ, isn't
> > that an error in the DT data, and should be rejected when parsing?
> > 
> > > +
> > > +/* read property into new mode_property */
> > > +static int of_video_parse_property(struct device_node *np, char *name,
> > > +				struct mode_property *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);
> > > +
> > > +	memset(result, 0, sizeof(*result));
> > > +
> > > +	ret = of_property_read_u32_array(np, name, &result->min, cells);
> > > +
> > > +	return ret;
> > 
> > Ah, I guess this is the reason for the of_video_get_value... Wouldn't it
> > be cleaner to be more explicit here? If there's one cell, it's the
> > typical value, otherwise if there are 3 cells, they are min/typ/max,
> > else it's an error.
> 
> I agree. We should flag errors and fail instead of trying to fix them 
> silently. That's the only way to make sure that device tree authors will get 
> it right (or at least less wrong). We might need to accept wrong DT data later 
> to support buggy devices found in the wild, but we should not start that way.
> 

Well, I can only agree, too ;-) Fixed.

> > And I think the above also trashes memory if there happens to be 4+ cells.
> > 
> > > +}
> > > +
> > > +static int of_video_free(struct display *disp)
> > > +{
> > > +	int i;
> > > +
> > > +	for (i=0; i<disp->num_modes; i++)
> 
> Spaces around = and < please.
> 
> > > +		kfree(disp->modes[i]);
> > > +	kfree(disp->modes);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +int videomode_to_display_mode(struct display *disp, struct
> > > drm_display_mode *dmode, int index)
> 
> Shouldn't this use drm_mode_modeinfo instead of drm_display_mode ?
> 
> > > +{
> > > +	struct videomode *vm;
> > > +
> > > +	memset(dmode, 0, sizeof(*dmode));
> > > +
> > > +	if (index > disp->num_modes) {
> > > +		pr_err("%s: wrong index: %d from %d\n", __func__, index,
> > > disp->num_modes); +		return -EINVAL;
> > > +	}
> > > +
> > > +	vm = disp->modes[index];
> > > +
> > > +	dmode->hdisplay = of_video_get_value(&vm->hactive);
> > > +	dmode->hsync_start = of_video_get_value(&vm->hactive) +
> > > of_video_get_value(&vm->hfront_porch);
> 
> You could replace of_video_get_value(&vm->hactive) with dmode->hdisplay here, 
> and similarly below (hsync_end = hsync_start + of_video_get_value(&vm-
> >hsync_len), ...). Beside shortening lines, it would save calls to 
> of_video_get_value().
> 
> > > +	dmode->hsync_end = of_video_get_value(&vm->hactive) +
> > > of_video_get_value(&vm->hfront_porch)
> > > +			+ of_video_get_value(&vm->hsync_len);
> > > +	dmode->htotal = of_video_get_value(&vm->hactive) +
> > > of_video_get_value(&vm->hfront_porch)
> > > +			+ of_video_get_value(&vm->hsync_len) +
> > > of_video_get_value(&vm->hback_porch);
> > > +	dmode->vdisplay = of_video_get_value(&vm->vactive);
> > > +	dmode->vsync_start = of_video_get_value(&vm->vactive) +
> > > of_video_get_value(&vm->vfront_porch); +	dmode->vsync_end =
> > > of_video_get_value(&vm->vactive) + of_video_get_value(&vm->vfront_porch)
> > > +			+ of_video_get_value(&vm->vsync_len);
> > > +	dmode->vtotal = of_video_get_value(&vm->vactive) +
> > > of_video_get_value(&vm->vfront_porch) +
> > > +			of_video_get_value(&vm->vsync_len) +
> > > of_video_get_value(&vm->vback_porch); +
> > > +	dmode->width_mm = disp->width_mm;
> > > +	dmode->height_mm = disp->height_mm;
> > > +
> > > +	dmode->clock = of_video_get_value(&vm->clock) / 1000;
> > > +
> > > +	if (vm->hah)
> > > +		dmode->flags |= DRM_MODE_FLAG_PHSYNC;
> > > +	else
> > > +		dmode->flags |= DRM_MODE_FLAG_NHSYNC;
> > > +	if (vm->vah)
> > > +		dmode->flags |= DRM_MODE_FLAG_PVSYNC;
> > > +	else
> > > +		dmode->flags |= DRM_MODE_FLAG_NVSYNC;
> > > +	if (vm->interlaced)
> > > +		dmode->flags |= DRM_MODE_FLAG_INTERLACE;
> > > +	if (vm->doublescan)
> > > +		dmode->flags |= DRM_MODE_FLAG_DBLSCAN;
> > > +
> > > +	drm_mode_set_name(dmode);
> > > +
> > > +	return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(videomode_to_display_mode);
> > > +
> > > +int videomode_to_fb_mode(struct display *disp, struct fb_videomode
> > > *fbmode, int index)
> 
> disp is only used to retrieve the mode, you could pass a struct videomode 
> instead of disp and index.
> 
> Thinking about it, I would make videomode_to_display_mode take a struct 
> videomode as well. The only reason it needs struct display is to get the 
> display physical dimensions. Those are only used in very specific cases in the 
> DRM subsystem, I don't think they should be copied to every struct 
> drm_mode_modeinfo.
> 

Sounds reasonable. Will fix this.

> > > +{
> > > +	struct videomode *vm;
> > > +
> > > +	memset(fbmode, 0, sizeof(*fbmode));
> > > +
> > > +	if (index > disp->num_modes) {
> > > +		pr_err("%s: wrong index: %d from %d\n", __func__, index,
> > > disp->num_modes);
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	vm = disp->modes[index];
> > > +
> > > +	fbmode->xres = of_video_get_value(&vm->hactive);
> > > +	fbmode->left_margin = of_video_get_value(&vm->hback_porch);
> > > +	fbmode->right_margin = of_video_get_value(&vm->hfront_porch);
> > > +	fbmode->hsync_len = of_video_get_value(&vm->hsync_len);
> > > +
> > > +	fbmode->yres = of_video_get_value(&vm->vactive);
> > > +	fbmode->upper_margin = of_video_get_value(&vm->vback_porch);
> > > +	fbmode->lower_margin = of_video_get_value(&vm->vfront_porch);
> > > +	fbmode->vsync_len = of_video_get_value(&vm->vsync_len);
> > > +
> > > +	fbmode->pixclock = KHZ2PICOS(of_video_get_value(&vm->clock) / 1000);
> > > +
> > > +	if (vm->hah)
> > > +		fbmode->sync |= FB_SYNC_HOR_HIGH_ACT;
> > > +	if (vm->vah)
> > > +		fbmode->sync |= FB_SYNC_VERT_HIGH_ACT;
> > > +	if (vm->interlaced)
> > > +		fbmode->vmode |= FB_VMODE_INTERLACED;
> > > +	if (vm->doublescan)
> > > +		fbmode->vmode |= FB_VMODE_DOUBLE;
> > > +
> > > +	return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(videomode_to_fb_mode);
> > > +
> > > +int of_get_video_modes(struct device_node *np, struct display *disp)
> > > +{
> > > +	struct device_node *display_np;
> > > +	struct device_node *mode_np;
> > > +	struct device_node *modes_np;
> > > +	char *default_mode;
> > > +
> > > +	int ret = 0;
> > > +
> > > +	memset(disp, 0, sizeof(*disp));
> > > +	disp->modes = kmalloc(sizeof(struct videomode*), GFP_KERNEL);
> > > +
> > > +	if (!np) {
> > > +		pr_err("%s: no node provided\n", __func__);
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	display_np = of_parse_phandle(np, "display", 0);
> > > +
> > > +	if (!display_np) {
> > > +		pr_err("%s: could not find display node\n", __func__);
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	of_property_read_u32(display_np, "width-mm", &disp->width_mm);
> > > +	of_property_read_u32(display_np, "height-mm", &disp->height_mm);
> > > +
> > > +	mode_np = of_parse_phandle(np, "default-mode", 0);
> > > +
> > > +	if (!mode_np) {
> > > +		pr_info("%s: no default-mode specified.\n", __func__);
> > > +		mode_np = of_find_node_by_name(np, "mode");
> > > +	}
> > > +
> > > +	if (!mode_np) {
> > > +		pr_err("%s: could not find any mode specification\n", __func__);
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	default_mode = (char *)mode_np->full_name;
> > > +
> > > +	modes_np = of_find_node_by_name(np, "modes");
> > > +	for_each_child_of_node(modes_np, mode_np) {
> > > +		struct videomode *vm;
> > > +
> > > +		vm = kmalloc(sizeof(struct videomode*), GFP_KERNEL);
> > > +		disp->modes[disp->num_modes] = kmalloc(sizeof(struct videomode*),
> > > GFP_KERNEL);
> > > +
> > > +		ret |= of_video_parse_property(mode_np, "hback-porch",
> > > &vm->hback_porch);
> > > +		ret |= of_video_parse_property(mode_np, "hfront-porch",
> > > &vm->hfront_porch);
> > > +		ret |= of_video_parse_property(mode_np, "hactive", &vm->hactive);
> > > +		ret |= of_video_parse_property(mode_np, "hsync-len",
> > > &vm->hsync_len);
> > > +		ret |= of_video_parse_property(mode_np, "vback-porch",
> > > &vm->vback_porch);
> > > +		ret |= of_video_parse_property(mode_np, "vfront-porch",
> > > &vm->vfront_porch);
> > > +		ret |= of_video_parse_property(mode_np, "vactive", &vm->vactive);
> > > +		ret |= of_video_parse_property(mode_np, "vsync-len",
> > > &vm->vsync_len);
> > > +		ret |= of_video_parse_property(mode_np, "clock", &vm->clock);
> > > +
> > > +		if (ret)
> > > +			return -EINVAL;
> > > +
> > > +		vm->hah = of_property_read_bool(mode_np, "hsync-active-high");
> > > +		vm->vah = of_property_read_bool(mode_np, "vsync-active-high");
> > > +		vm->interlaced = of_property_read_bool(mode_np, "interlaced");
> > > +		vm->doublescan = of_property_read_bool(mode_np, "doublescan");
> > > +
> > > +		if (strcmp(default_mode,mode_np->full_name) == 0)
> > > +			disp->default_mode = disp->num_modes;
> > > +
> > > +		disp->modes[disp->num_modes] = vm;
> > > +		disp->num_modes++;
> > > +	}
> > > +	of_node_put(display_np);
> > > +
> > > +	pr_info("%s: found %d modelines. Using #%d as default\n", __func__,
> > > +		disp->num_modes, disp->default_mode + 1);
> > > +
> > > +
> > > +	return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(of_get_video_modes);
> > > +
> > > +int of_video_mode_exists(struct device_node *np)
> > > +{
> > > +	struct device_node *display_np;
> > > +	struct device_node *mode_np;
> > > +
> > > +	if (!np)
> > > +		return -EINVAL;
> > > +
> > > +	display_np = of_parse_phandle(np, "display", 0);
> > > +
> > > +	if (!display_np)
> > > +		return -EINVAL;
> > > +
> > > +	mode_np = of_parse_phandle(np, "default-mode", 0);
> > > +
> > > +	if (mode_np)
> > > +		return 0;
> > > +
> > > +	return -EINVAL;
> > > +}
> > > +EXPORT_SYMBOL_GPL(of_video_mode_exists);
> > > +
> > > +int of_get_display_mode(struct device_node *np, struct drm_display_mode
> > > *dmode, int index)
> > > +{
> > > +	struct display disp;
> > > +
> > > +	of_get_video_modes(np, &disp);
> > > +
> > > +	if (index == OF_MODE_SELECTION)
> > > +		index = disp.default_mode;
> > > +	if (dmode)
> > > +		videomode_to_display_mode(&disp, dmode, index);
> > > +
> > > +	of_video_free(&disp);
> > > +
> > > +	return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(of_get_display_mode);
> > > +
> > > +int of_get_fb_videomode(struct device_node *np, struct fb_videomode
> > > *fbmode, int index) 
> > > +{
> > > +	struct display disp;
> > > +
> > > +	of_get_video_modes(np, &disp);
> > > +
> > > +	if (index == OF_MODE_SELECTION)
> > > +		index = disp.default_mode;
> > > +	if (fbmode)
> > > +		videomode_to_fb_mode(&disp, fbmode, index);
> > > +
> > > +	of_video_free(&disp);
> > > +
> > > +	return 0;
> > 
> > This and of_get_display_mode() do not handle errors from
> > of_get_video_modes() nor from videomode_to_xxx_mode(). And I don't see a
> > reason for the if (fbmode) check (and the same for dmode), as there's no
> > reason to call these functions except to get the video modes.
> > 
> > > +}
> > > +EXPORT_SYMBOL_GPL(of_get_fb_videomode);
> 
> What are those two functions used for ? Aren't drivers expected to parse the 
> modes into a struct display and then operate on that structure, instead of 
> getting individual modes from the DT node ?
> 
> > > diff --git a/include/linux/of_videomode.h b/include/linux/of_videomode.h
> > > new file mode 100644
> > > index 0000000..5571ce3
> > > --- /dev/null
> > > +++ b/include/linux/of_videomode.h
> > > @@ -0,0 +1,56 @@
> > > +/*
> > > + * Copyright 2012 Sascha Hauer <[e-mail]>
> > > + * Copyright 2012 Steffen Trumtrar <[e-mail]>
> > > + *
> > > + * OF helpers for videomodes.
> > > + *
> > > + * This file is released under the GPLv2
> > > + */
> > > +
> > > +#ifndef __LINUX_OF_VIDEOMODE_H
> > > +#define __LINUX_OF_VIDEOMODE_H
> > > +
> > > +#define OF_MODE_SELECTION -1
> > > +
> > > +struct mode_property {
> > > +	u32 min;
> > > +	u32 typ;
> > > +	u32 max;
> > > +};
> > > +
> > > +struct display {
> > > +	u32 width_mm;
> > > +	u32 height_mm;
> > > +	struct videomode **modes;
> > > +	int default_mode;
> > > +	int num_modes;
> 
> default_mode and num_modes are non-negative, what about using an unsigned int 
> type for them ?
> 
> > > +};
> > > +
> > > +/* describe videomode in terms of hardware parameters */
> > > +struct videomode {
> > > +	struct mode_property hback_porch;
> > > +	struct mode_property hfront_porch;
> > > +	struct mode_property hactive;
> > > +	struct mode_property hsync_len;
> > > +
> > > +	struct mode_property vback_porch;
> > > +	struct mode_property vfront_porch;
> > > +	struct mode_property vactive;
> > > +	struct mode_property vsync_len;
> > > +
> > > +	struct mode_property clock;
> > > +
> > > +	bool hah;
> > > +	bool vah;
> > > +	bool interlaced;
> > > +	bool doublescan;
> > > +};
> > 
> > I think the names display and videomode are a bit too generic here, and
> > could clash with names from kernel drivers. Also, the videomode is not
> > really a videomode, but a "template" (or something) for videomode. A
> > real videomode doesn't have min & max values, only the actual value.
> > 
> > Perhaps these should be prefixed with "of_"? Then again, they are not
> > really of specific either...
> 
> If feel this is an important topic.
> 
> A generic video mode structure is definitely needed, as well as helper 
> functions to convert between the new structure and existing video mode 
> structures (struct fb_videomode, struct drm_mode_modeinfo and struct 
> v4l2_bt_timings). The structure and the helper functions should be generic, as 
> the goal is to gradually replace subsystem-specific video mode structures 
> where possible (userspace APIs will still need to keep the old structures). We 
> thus need to drop the dependency on OF for everything but the DT parsing code. 

Okay. Here I'm in a dilemma. Sounds like I should split up the code as it is
right now: a generic displaymode/videomode-handler and the of-functions to handle it.
Where would I put the displaymode/videomode-stuff ? drivers/media/video ?
I will keep it all in the same location for the next patch version. But I guess,
I will not be finished with that and have to split up the code (apart from any
coding errors I might still have in there ;-))

> For that reason an of_ prefix wouldn't be a good idea. As the goal is to 
> create a truly generic video mode structure, a generic name is a good idea (I 
> might prefer struct video_mode instead of struct videomode, but that's just 
> bikeshedding).
>

An internal meeting (well, me and Sascha) came to the conclusion, that I keep
struct videomode for the time beeing. But I may be persuaded to change this, if
other people chime in and want struct video_mode, too.

> We might need two video mode structures, one to represent a video mode, and 
> one to represent a range of video modes. I don't have enough experience with 
> video modes to have a really strong opinion on this, having a single structure 
> to represent both could be useful as well. This particular topic needs to be 
> discussed.
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> 

Regards,

Steffen

Hans Verkuil Sept. 21, 2012, 1:07 p.m. UTC | #4

On Wed September 19 2012 10:20:43 Steffen Trumtrar wrote:
> This patch adds a helper function for parsing videomodes from the devicetree.
> The videomode can be either converted to a struct drm_display_mode or a
> struct fb_videomode.
> 
> Signed-off-by: Sascha Hauer <[e-mail]>
> Signed-off-by: Steffen Trumtrar <[e-mail]>
> ---
> 
> Hi!
> 
> changes since v3:
> 	- print error messages
> 	- free alloced memory
> 	- general cleanup
> 
> Regards
> Steffen
> 
>  .../devicetree/bindings/video/displaymode          |   74 +++++
>  drivers/of/Kconfig                                 |    5 +
>  drivers/of/Makefile                                |    1 +
>  drivers/of/of_videomode.c                          |  283 ++++++++++++++++++++
>  include/linux/of_videomode.h                       |   56 ++++
>  5 files changed, 419 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/video/displaymode
>  create mode 100644 drivers/of/of_videomode.c
>  create mode 100644 include/linux/of_videomode.h
> 
> diff --git a/Documentation/devicetree/bindings/video/displaymode b/Documentation/devicetree/bindings/video/displaymode
> new file mode 100644
> index 0000000..990ca52
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/video/displaymode
> @@ -0,0 +1,74 @@
> +videomode bindings
> +==================
> +
> +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

In the case of interlaced formats, can the vfront-porch, vback-porch and vsync-len
for the second field always be calculated from these values? Is that fully
standardized or can there be exceptions? struct v4l2_bt_timings has separate fields
for these, but that may have been overkill.

> + - clock: displayclock in Hz

CEA-861 defined the option to slightly lower the clock frequency by 1.000/1.001 to
achieve frequencies like 29.97 or 59.94. In v4l2_bt_timings I made a flag for that.
I'm not sure whether it is better to just set the clock to the correct frequency
(which is a weird value) or mark it with a bool.

> +
> +Optional properties:
> + - width-mm, height-mm: Display dimensions in mm
> + - hsync-active-high (bool): Hsync pulse is active high
> + - vsync-active-high (bool): Vsync pulse is active high
> + - interlaced (bool): This is an interlaced mode
> + - doublescan (bool): This is a doublescan mode
> +
> +There are different ways of describing a display mode. The devicetree representation
> +corresponds to the one commonly found in datasheets for displays.
> +The description of the display and its mode is split in two parts: first the display
> +properties like size in mm and (optionally) multiple subnodes with the supported modes.
> +
> +Example:
> +
> +	display@0 {
> +		width-mm = <800>;
> +		height-mm = <480>;
> +		modes {
> +			mode0: mode@0 {
> +				/* 1920x1080p24 */
> +				clock = <52000000>;
> +				hactive = <1920>;
> +				vactive = <1080>;
> +				hfront-porch = <25>;
> +				hback-porch = <25>;
> +				hsync-len = <25>;
> +				vback-porch = <2>;
> +				vfront-porch = <2>;
> +				vsync-len = <2>;
> +				hsync-active-high;
> +			};
> +		};
> +	};
> +
> +Every property also supports the use of ranges, so the commonly used datasheet
> +description with <min typ max>-tuples can be used.
> +
> +Example:
> +
> +	mode1: mode@1 {
> +		/* 1920x1080p24 */
> +		clock = <148500000>;
> +		hactive = <1920>;
> +		vactive = <1080>;
> +		hsync-len = <0 44 60>;
> +		hfront-porch = <80 88 95>;
> +		hback-porch = <100 148 160>;
> +		vfront-porch = <0 4 6>;
> +		vback-porch = <0 36 50>;
> +		vsync-len = <0 5 6>;
> +	};
> +
> +The videomode can be linked to a connector via phandles. The connector has to
> +support the display- and default-mode-property to link to and select a mode.
> +
> +Example:
> +
> +	hdmi@00120000 {
> +		status = "okay";
> +		display = <&benq>;
> +		default-mode = <&mode1>;
> +	};
> +
> diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
> index dfba3e6..a3acaa3 100644
> --- a/drivers/of/Kconfig
> +++ b/drivers/of/Kconfig
> @@ -83,4 +83,9 @@ config OF_MTD
>  	depends on MTD
>  	def_bool y
>  
> +config OF_VIDEOMODE
> +	def_bool y
> +	help
> +	  helper to parse videomodes from the devicetree
> +
>  endmenu # OF
> diff --git a/drivers/of/Makefile b/drivers/of/Makefile
> index e027f44..80e6db3 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_VIDEOMODE)	+= of_videomode.o
> diff --git a/drivers/of/of_videomode.c b/drivers/of/of_videomode.c
> new file mode 100644
> index 0000000..52bfc74
> --- /dev/null
> +++ b/drivers/of/of_videomode.c
> @@ -0,0 +1,283 @@
> +/*
> + * OF helpers for parsing display modes
> + *
> + * Copyright (c) 2012 Sascha Hauer <[e-mail]>, Pengutronix
> + * Copyright (c) 2012 Steffen Trumtrar <[e-mail]>, Pengutronix
> + *
> + * This file is released under the GPLv2
> + */
> +#include <linux/of.h>
> +#include <linux/fb.h>
> +#include <linux/export.h>
> +#include <linux/slab.h>
> +#include <drm/drmP.h>
> +#include <drm/drm_crtc.h>
> +#include <linux/of_videomode.h>
> +
> +static u32 of_video_get_value(struct mode_property *prop)
> +{
> +	return (prop->min >= prop->typ) ? prop->min : prop->typ;
> +}
> +
> +/* read property into new mode_property */
> +static int of_video_parse_property(struct device_node *np, char *name,
> +				struct mode_property *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);
> +
> +	memset(result, 0, sizeof(*result));
> +
> +	ret = of_property_read_u32_array(np, name, &result->min, cells);
> +
> +	return ret;
> +}
> +
> +static int of_video_free(struct display *disp)
> +{
> +	int i;
> +
> +	for (i=0; i<disp->num_modes; i++)
> +		kfree(disp->modes[i]);
> +	kfree(disp->modes);
> +
> +	return 0;
> +}
> +
> +int videomode_to_display_mode(struct display *disp, struct drm_display_mode *dmode, int index)
> +{
> +	struct videomode *vm;
> +
> +	memset(dmode, 0, sizeof(*dmode));
> +
> +	if (index > disp->num_modes) {
> +		pr_err("%s: wrong index: %d from %d\n", __func__, index, disp->num_modes);
> +		return -EINVAL;
> +	}
> +
> +	vm = disp->modes[index];
> +
> +	dmode->hdisplay = of_video_get_value(&vm->hactive);
> +	dmode->hsync_start = of_video_get_value(&vm->hactive) + of_video_get_value(&vm->hfront_porch);
> +	dmode->hsync_end = of_video_get_value(&vm->hactive) + of_video_get_value(&vm->hfront_porch)
> +			+ of_video_get_value(&vm->hsync_len);
> +	dmode->htotal = of_video_get_value(&vm->hactive) + of_video_get_value(&vm->hfront_porch)
> +			+ of_video_get_value(&vm->hsync_len) + of_video_get_value(&vm->hback_porch);
> +
> +	dmode->vdisplay = of_video_get_value(&vm->vactive);
> +	dmode->vsync_start = of_video_get_value(&vm->vactive) + of_video_get_value(&vm->vfront_porch);
> +	dmode->vsync_end = of_video_get_value(&vm->vactive) + of_video_get_value(&vm->vfront_porch)
> +			+ of_video_get_value(&vm->vsync_len);
> +	dmode->vtotal = of_video_get_value(&vm->vactive) + of_video_get_value(&vm->vfront_porch) +
> +			of_video_get_value(&vm->vsync_len) + of_video_get_value(&vm->vback_porch);
> +
> +	dmode->width_mm = disp->width_mm;
> +	dmode->height_mm = disp->height_mm;
> +
> +	dmode->clock = of_video_get_value(&vm->clock) / 1000;
> +
> +	if (vm->hah)
> +		dmode->flags |= DRM_MODE_FLAG_PHSYNC;
> +	else
> +		dmode->flags |= DRM_MODE_FLAG_NHSYNC;
> +	if (vm->vah)
> +		dmode->flags |= DRM_MODE_FLAG_PVSYNC;
> +	else
> +		dmode->flags |= DRM_MODE_FLAG_NVSYNC;
> +	if (vm->interlaced)
> +		dmode->flags |= DRM_MODE_FLAG_INTERLACE;
> +	if (vm->doublescan)
> +		dmode->flags |= DRM_MODE_FLAG_DBLSCAN;
> +
> +	drm_mode_set_name(dmode);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(videomode_to_display_mode);
> +
> +int videomode_to_fb_mode(struct display *disp, struct fb_videomode *fbmode, int index)
> +{
> +	struct videomode *vm;
> +
> +	memset(fbmode, 0, sizeof(*fbmode));
> +
> +	if (index > disp->num_modes) {
> +		pr_err("%s: wrong index: %d from %d\n", __func__, index, disp->num_modes);
> +		return -EINVAL;
> +	}
> +
> +	vm = disp->modes[index];
> +
> +	fbmode->xres = of_video_get_value(&vm->hactive);
> +	fbmode->left_margin = of_video_get_value(&vm->hback_porch);
> +	fbmode->right_margin = of_video_get_value(&vm->hfront_porch);
> +	fbmode->hsync_len = of_video_get_value(&vm->hsync_len);
> +
> +	fbmode->yres = of_video_get_value(&vm->vactive);
> +	fbmode->upper_margin = of_video_get_value(&vm->vback_porch);
> +	fbmode->lower_margin = of_video_get_value(&vm->vfront_porch);
> +	fbmode->vsync_len = of_video_get_value(&vm->vsync_len);
> +
> +	fbmode->pixclock = KHZ2PICOS(of_video_get_value(&vm->clock) / 1000);
> +
> +	if (vm->hah)
> +		fbmode->sync |= FB_SYNC_HOR_HIGH_ACT;
> +	if (vm->vah)
> +		fbmode->sync |= FB_SYNC_VERT_HIGH_ACT;
> +	if (vm->interlaced)
> +		fbmode->vmode |= FB_VMODE_INTERLACED;
> +	if (vm->doublescan)
> +		fbmode->vmode |= FB_VMODE_DOUBLE;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(videomode_to_fb_mode);
> +
> +int of_get_video_modes(struct device_node *np, struct display *disp)
> +{
> +	struct device_node *display_np;
> +	struct device_node *mode_np;
> +	struct device_node *modes_np;
> +	char *default_mode;
> +
> +	int ret = 0;
> +
> +	memset(disp, 0, sizeof(*disp));
> +	disp->modes = kmalloc(sizeof(struct videomode*), GFP_KERNEL);
> +
> +	if (!np) {
> +		pr_err("%s: no node provided\n", __func__);
> +		return -EINVAL;
> +	}
> +
> +	display_np = of_parse_phandle(np, "display", 0);
> +
> +	if (!display_np) {
> +		pr_err("%s: could not find display node\n", __func__);
> +		return -EINVAL;
> +	}
> +
> +	of_property_read_u32(display_np, "width-mm", &disp->width_mm);
> +	of_property_read_u32(display_np, "height-mm", &disp->height_mm);
> +
> +	mode_np = of_parse_phandle(np, "default-mode", 0);
> +
> +	if (!mode_np) {
> +		pr_info("%s: no default-mode specified.\n", __func__);
> +		mode_np = of_find_node_by_name(np, "mode");
> +	}
> +
> +	if (!mode_np) {
> +		pr_err("%s: could not find any mode specification\n", __func__);
> +		return -EINVAL;
> +	}
> +
> +	default_mode = (char *)mode_np->full_name;
> +
> +	modes_np = of_find_node_by_name(np, "modes");
> +	for_each_child_of_node(modes_np, mode_np) {
> +		struct videomode *vm;
> +
> +		vm = kmalloc(sizeof(struct videomode*), GFP_KERNEL);
> +		disp->modes[disp->num_modes] = kmalloc(sizeof(struct videomode*), GFP_KERNEL);
> +
> +		ret |= of_video_parse_property(mode_np, "hback-porch", &vm->hback_porch);
> +		ret |= of_video_parse_property(mode_np, "hfront-porch", &vm->hfront_porch);
> +		ret |= of_video_parse_property(mode_np, "hactive", &vm->hactive);
> +		ret |= of_video_parse_property(mode_np, "hsync-len", &vm->hsync_len);
> +		ret |= of_video_parse_property(mode_np, "vback-porch", &vm->vback_porch);
> +		ret |= of_video_parse_property(mode_np, "vfront-porch", &vm->vfront_porch);
> +		ret |= of_video_parse_property(mode_np, "vactive", &vm->vactive);
> +		ret |= of_video_parse_property(mode_np, "vsync-len", &vm->vsync_len);
> +		ret |= of_video_parse_property(mode_np, "clock", &vm->clock);
> +
> +		if (ret)
> +			return -EINVAL;
> +
> +		vm->hah = of_property_read_bool(mode_np, "hsync-active-high");
> +		vm->vah = of_property_read_bool(mode_np, "vsync-active-high");
> +		vm->interlaced = of_property_read_bool(mode_np, "interlaced");
> +		vm->doublescan = of_property_read_bool(mode_np, "doublescan");
> +
> +		if (strcmp(default_mode,mode_np->full_name) == 0)
> +			disp->default_mode = disp->num_modes;
> +
> +		disp->modes[disp->num_modes] = vm;
> +		disp->num_modes++;
> +	}
> +	of_node_put(display_np);
> +
> +	pr_info("%s: found %d modelines. Using #%d as default\n", __func__,
> +		disp->num_modes, disp->default_mode + 1);
> +
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(of_get_video_modes);
> +
> +int of_video_mode_exists(struct device_node *np)
> +{
> +	struct device_node *display_np;
> +	struct device_node *mode_np;
> +
> +	if (!np)
> +		return -EINVAL;
> +
> +	display_np = of_parse_phandle(np, "display", 0);
> +
> +	if (!display_np)
> +		return -EINVAL;
> +
> +	mode_np = of_parse_phandle(np, "default-mode", 0);
> +
> +	if (mode_np)
> +		return 0;
> +
> +	return -EINVAL;
> +}
> +EXPORT_SYMBOL_GPL(of_video_mode_exists);
> +
> +int of_get_display_mode(struct device_node *np, struct drm_display_mode *dmode, int index)
> +{
> +	struct display disp;
> +
> +	of_get_video_modes(np, &disp);
> +
> +	if (index == OF_MODE_SELECTION)
> +		index = disp.default_mode;
> +	if (dmode)
> +		videomode_to_display_mode(&disp, dmode, index);
> +
> +	of_video_free(&disp);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(of_get_display_mode);
> +
> +int of_get_fb_videomode(struct device_node *np, struct fb_videomode *fbmode, int index)
> +{
> +	struct display disp;
> +
> +	of_get_video_modes(np, &disp);
> +
> +	if (index == OF_MODE_SELECTION)
> +		index = disp.default_mode;
> +	if (fbmode)
> +		videomode_to_fb_mode(&disp, fbmode, index);
> +
> +	of_video_free(&disp);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(of_get_fb_videomode);
> diff --git a/include/linux/of_videomode.h b/include/linux/of_videomode.h
> new file mode 100644
> index 0000000..5571ce3
> --- /dev/null
> +++ b/include/linux/of_videomode.h
> @@ -0,0 +1,56 @@
> +/*
> + * Copyright 2012 Sascha Hauer <[e-mail]>
> + * Copyright 2012 Steffen Trumtrar <[e-mail]>
> + *
> + * OF helpers for videomodes.
> + *
> + * This file is released under the GPLv2
> + */
> +
> +#ifndef __LINUX_OF_VIDEOMODE_H
> +#define __LINUX_OF_VIDEOMODE_H
> +
> +#define OF_MODE_SELECTION -1
> +
> +struct mode_property {
> +	u32 min;
> +	u32 typ;
> +	u32 max;
> +};

For what purpose are the min and max values added? To specify hardware limitations?
If it is the latter, then I think it would be a better approach to split that off
in its own struct.

For V4L2 we made a separate capability struct:

http://hverkuil.home.xs4all.nl/spec/media.html#vidioc-dv-timings-cap

Regards,

	Hans

> +
> +struct display {
> +	u32 width_mm;
> +	u32 height_mm;
> +	struct videomode **modes;
> +	int default_mode;
> +	int num_modes;
> +};
> +
> +/* describe videomode in terms of hardware parameters */
> +struct videomode {
> +	struct mode_property hback_porch;
> +	struct mode_property hfront_porch;
> +	struct mode_property hactive;
> +	struct mode_property hsync_len;
> +
> +	struct mode_property vback_porch;
> +	struct mode_property vfront_porch;
> +	struct mode_property vactive;
> +	struct mode_property vsync_len;
> +
> +	struct mode_property clock;
> +
> +	bool hah;
> +	bool vah;
> +	bool interlaced;
> +	bool doublescan;
> +};
> +
> +int of_video_mode_exists(struct device_node *np);
> +int videomode_to_display_mode(struct display *disp, struct drm_display_mode *dmode, int index);
> +int videomode_to_fb_mode(struct display *disp, struct fb_videomode *fbmode, int index);
> +int of_get_video_modes(struct device_node *np, struct display *disp);
> +int of_video_mode_exists(struct device_node *np);
> +int of_get_display_mode(struct device_node *np, struct drm_display_mode *dmode, int index);
> +int of_get_fb_videomode(struct device_node *np, struct fb_videomode *fbmode, int index);
> +#endif /* __LINUX_OF_VIDEOMODE_H */
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Rob Herring Sept. 24, 2012, 1:42 p.m. UTC | #5

On 09/19/2012 03:20 AM, Steffen Trumtrar wrote:
> This patch adds a helper function for parsing videomodes from the devicetree.
> The videomode can be either converted to a struct drm_display_mode or a
> struct fb_videomode.
> 
> Signed-off-by: Sascha Hauer <[e-mail]>
> Signed-off-by: Steffen Trumtrar <[e-mail]>
> ---
> 
> Hi!
> 
> changes since v3:
> 	- print error messages
> 	- free alloced memory
> 	- general cleanup
> 
> Regards
> Steffen
> 
>  .../devicetree/bindings/video/displaymode          |   74 +++++
>  drivers/of/Kconfig                                 |    5 +
>  drivers/of/Makefile                                |    1 +
>  drivers/of/of_videomode.c                          |  283 ++++++++++++++++++++
>  include/linux/of_videomode.h                       |   56 ++++
>  5 files changed, 419 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/video/displaymode
>  create mode 100644 drivers/of/of_videomode.c
>  create mode 100644 include/linux/of_videomode.h
> 
> diff --git a/Documentation/devicetree/bindings/video/displaymode b/Documentation/devicetree/bindings/video/displaymode
> new file mode 100644
> index 0000000..990ca52
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/video/displaymode
> @@ -0,0 +1,74 @@
> +videomode bindings
> +==================
> +
> +Required properties:
> + - hactive, vactive: Display resolution
> + - hfront-porch, hback-porch, hsync-len: Horizontal Display timing parameters
> +   in pixels
> +   vfront-porch, vback-porch, vsync-len: Vertical display timing parameters in
> +   lines
> + - clock: displayclock in Hz

A major piece missing is the LCD controller to display interface width
and component ordering.

> +
> +Optional properties:
> + - width-mm, height-mm: Display dimensions in mm
> + - hsync-active-high (bool): Hsync pulse is active high
> + - vsync-active-high (bool): Vsync pulse is active high
> + - interlaced (bool): This is an interlaced mode
> + - doublescan (bool): This is a doublescan mode
> +
> +There are different ways of describing a display mode. The devicetree representation
> +corresponds to the one commonly found in datasheets for displays.
> +The description of the display and its mode is split in two parts: first the display
> +properties like size in mm and (optionally) multiple subnodes with the supported modes.
> +
> +Example:
> +
> +	display@0 {

It would be useful to have a compatible string here. We may not always
know the panel type or have a fixed panel though. We could define
"generic-lcd" or something for cases where the panel type is unknown.

> +		width-mm = <800>;
> +		height-mm = <480>;
> +		modes {
> +			mode0: mode@0 {
> +				/* 1920x1080p24 */
> +				clock = <52000000>;
> +				hactive = <1920>;
> +				vactive = <1080>;
> +				hfront-porch = <25>;
> +				hback-porch = <25>;
> +				hsync-len = <25>;
> +				vback-porch = <2>;
> +				vfront-porch = <2>;
> +				vsync-len = <2>;
> +				hsync-active-high;
> +			};
> +		};
> +	};
> +
> +Every property also supports the use of ranges, so the commonly used datasheet
> +description with <min typ max>-tuples can be used.
> +
> +Example:
> +
> +	mode1: mode@1 {
> +		/* 1920x1080p24 */
> +		clock = <148500000>;
> +		hactive = <1920>;
> +		vactive = <1080>;
> +		hsync-len = <0 44 60>;
> +		hfront-porch = <80 88 95>;
> +		hback-porch = <100 148 160>;
> +		vfront-porch = <0 4 6>;
> +		vback-porch = <0 36 50>;
> +		vsync-len = <0 5 6>;
> +	};
> +
> +The videomode can be linked to a connector via phandles. The connector has to
> +support the display- and default-mode-property to link to and select a mode.

Could also be phandle in the lcd controller node? What are the '-' for?
Is "display-blah" a valid name or something?

"default-mode" is pretty generic. How about display-mode or
display-default-mode?

Rob

> +
> +Example:
> +
> +	hdmi@00120000 {
> +		status = "okay";
> +		display = <&benq>;
> +		default-mode = <&mode1>;
> +	};
> +
> diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
> index dfba3e6..a3acaa3 100644
> --- a/drivers/of/Kconfig
> +++ b/drivers/of/Kconfig
> @@ -83,4 +83,9 @@ config OF_MTD
>  	depends on MTD
>  	def_bool y
>  
> +config OF_VIDEOMODE
> +	def_bool y
> +	help
> +	  helper to parse videomodes from the devicetree
> +
>  endmenu # OF
> diff --git a/drivers/of/Makefile b/drivers/of/Makefile
> index e027f44..80e6db3 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_VIDEOMODE)	+= of_videomode.o
> diff --git a/drivers/of/of_videomode.c b/drivers/of/of_videomode.c
> new file mode 100644
> index 0000000..52bfc74
> --- /dev/null
> +++ b/drivers/of/of_videomode.c
> @@ -0,0 +1,283 @@
> +/*
> + * OF helpers for parsing display modes
> + *
> + * Copyright (c) 2012 Sascha Hauer <[e-mail]>, Pengutronix
> + * Copyright (c) 2012 Steffen Trumtrar <[e-mail]>, Pengutronix
> + *
> + * This file is released under the GPLv2
> + */
> +#include <linux/of.h>
> +#include <linux/fb.h>
> +#include <linux/export.h>
> +#include <linux/slab.h>
> +#include <drm/drmP.h>
> +#include <drm/drm_crtc.h>
> +#include <linux/of_videomode.h>
> +
> +static u32 of_video_get_value(struct mode_property *prop)
> +{
> +	return (prop->min >= prop->typ) ? prop->min : prop->typ;
> +}
> +
> +/* read property into new mode_property */
> +static int of_video_parse_property(struct device_node *np, char *name,
> +				struct mode_property *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);
> +
> +	memset(result, 0, sizeof(*result));
> +
> +	ret = of_property_read_u32_array(np, name, &result->min, cells);
> +
> +	return ret;
> +}
> +
> +static int of_video_free(struct display *disp)
> +{
> +	int i;
> +
> +	for (i=0; i<disp->num_modes; i++)
> +		kfree(disp->modes[i]);
> +	kfree(disp->modes);
> +
> +	return 0;
> +}
> +
> +int videomode_to_display_mode(struct display *disp, struct drm_display_mode *dmode, int index)
> +{
> +	struct videomode *vm;
> +
> +	memset(dmode, 0, sizeof(*dmode));
> +
> +	if (index > disp->num_modes) {
> +		pr_err("%s: wrong index: %d from %d\n", __func__, index, disp->num_modes);
> +		return -EINVAL;
> +	}
> +
> +	vm = disp->modes[index];
> +
> +	dmode->hdisplay = of_video_get_value(&vm->hactive);
> +	dmode->hsync_start = of_video_get_value(&vm->hactive) + of_video_get_value(&vm->hfront_porch);
> +	dmode->hsync_end = of_video_get_value(&vm->hactive) + of_video_get_value(&vm->hfront_porch)
> +			+ of_video_get_value(&vm->hsync_len);
> +	dmode->htotal = of_video_get_value(&vm->hactive) + of_video_get_value(&vm->hfront_porch)
> +			+ of_video_get_value(&vm->hsync_len) + of_video_get_value(&vm->hback_porch);
> +
> +	dmode->vdisplay = of_video_get_value(&vm->vactive);
> +	dmode->vsync_start = of_video_get_value(&vm->vactive) + of_video_get_value(&vm->vfront_porch);
> +	dmode->vsync_end = of_video_get_value(&vm->vactive) + of_video_get_value(&vm->vfront_porch)
> +			+ of_video_get_value(&vm->vsync_len);
> +	dmode->vtotal = of_video_get_value(&vm->vactive) + of_video_get_value(&vm->vfront_porch) +
> +			of_video_get_value(&vm->vsync_len) + of_video_get_value(&vm->vback_porch);
> +
> +	dmode->width_mm = disp->width_mm;
> +	dmode->height_mm = disp->height_mm;
> +
> +	dmode->clock = of_video_get_value(&vm->clock) / 1000;
> +
> +	if (vm->hah)
> +		dmode->flags |= DRM_MODE_FLAG_PHSYNC;
> +	else
> +		dmode->flags |= DRM_MODE_FLAG_NHSYNC;
> +	if (vm->vah)
> +		dmode->flags |= DRM_MODE_FLAG_PVSYNC;
> +	else
> +		dmode->flags |= DRM_MODE_FLAG_NVSYNC;
> +	if (vm->interlaced)
> +		dmode->flags |= DRM_MODE_FLAG_INTERLACE;
> +	if (vm->doublescan)
> +		dmode->flags |= DRM_MODE_FLAG_DBLSCAN;
> +
> +	drm_mode_set_name(dmode);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(videomode_to_display_mode);
> +
> +int videomode_to_fb_mode(struct display *disp, struct fb_videomode *fbmode, int index)
> +{
> +	struct videomode *vm;
> +
> +	memset(fbmode, 0, sizeof(*fbmode));
> +
> +	if (index > disp->num_modes) {
> +		pr_err("%s: wrong index: %d from %d\n", __func__, index, disp->num_modes);
> +		return -EINVAL;
> +	}
> +
> +	vm = disp->modes[index];
> +
> +	fbmode->xres = of_video_get_value(&vm->hactive);
> +	fbmode->left_margin = of_video_get_value(&vm->hback_porch);
> +	fbmode->right_margin = of_video_get_value(&vm->hfront_porch);
> +	fbmode->hsync_len = of_video_get_value(&vm->hsync_len);
> +
> +	fbmode->yres = of_video_get_value(&vm->vactive);
> +	fbmode->upper_margin = of_video_get_value(&vm->vback_porch);
> +	fbmode->lower_margin = of_video_get_value(&vm->vfront_porch);
> +	fbmode->vsync_len = of_video_get_value(&vm->vsync_len);
> +
> +	fbmode->pixclock = KHZ2PICOS(of_video_get_value(&vm->clock) / 1000);
> +
> +	if (vm->hah)
> +		fbmode->sync |= FB_SYNC_HOR_HIGH_ACT;
> +	if (vm->vah)
> +		fbmode->sync |= FB_SYNC_VERT_HIGH_ACT;
> +	if (vm->interlaced)
> +		fbmode->vmode |= FB_VMODE_INTERLACED;
> +	if (vm->doublescan)
> +		fbmode->vmode |= FB_VMODE_DOUBLE;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(videomode_to_fb_mode);
> +
> +int of_get_video_modes(struct device_node *np, struct display *disp)
> +{
> +	struct device_node *display_np;
> +	struct device_node *mode_np;
> +	struct device_node *modes_np;
> +	char *default_mode;
> +
> +	int ret = 0;
> +
> +	memset(disp, 0, sizeof(*disp));
> +	disp->modes = kmalloc(sizeof(struct videomode*), GFP_KERNEL);
> +
> +	if (!np) {
> +		pr_err("%s: no node provided\n", __func__);
> +		return -EINVAL;
> +	}
> +
> +	display_np = of_parse_phandle(np, "display", 0);
> +
> +	if (!display_np) {
> +		pr_err("%s: could not find display node\n", __func__);
> +		return -EINVAL;
> +	}
> +
> +	of_property_read_u32(display_np, "width-mm", &disp->width_mm);
> +	of_property_read_u32(display_np, "height-mm", &disp->height_mm);
> +
> +	mode_np = of_parse_phandle(np, "default-mode", 0);
> +
> +	if (!mode_np) {
> +		pr_info("%s: no default-mode specified.\n", __func__);
> +		mode_np = of_find_node_by_name(np, "mode");
> +	}
> +
> +	if (!mode_np) {
> +		pr_err("%s: could not find any mode specification\n", __func__);
> +		return -EINVAL;
> +	}
> +
> +	default_mode = (char *)mode_np->full_name;
> +
> +	modes_np = of_find_node_by_name(np, "modes");
> +	for_each_child_of_node(modes_np, mode_np) {
> +		struct videomode *vm;
> +
> +		vm = kmalloc(sizeof(struct videomode*), GFP_KERNEL);
> +		disp->modes[disp->num_modes] = kmalloc(sizeof(struct videomode*), GFP_KERNEL);
> +
> +		ret |= of_video_parse_property(mode_np, "hback-porch", &vm->hback_porch);
> +		ret |= of_video_parse_property(mode_np, "hfront-porch", &vm->hfront_porch);
> +		ret |= of_video_parse_property(mode_np, "hactive", &vm->hactive);
> +		ret |= of_video_parse_property(mode_np, "hsync-len", &vm->hsync_len);
> +		ret |= of_video_parse_property(mode_np, "vback-porch", &vm->vback_porch);
> +		ret |= of_video_parse_property(mode_np, "vfront-porch", &vm->vfront_porch);
> +		ret |= of_video_parse_property(mode_np, "vactive", &vm->vactive);
> +		ret |= of_video_parse_property(mode_np, "vsync-len", &vm->vsync_len);
> +		ret |= of_video_parse_property(mode_np, "clock", &vm->clock);
> +
> +		if (ret)
> +			return -EINVAL;
> +
> +		vm->hah = of_property_read_bool(mode_np, "hsync-active-high");
> +		vm->vah = of_property_read_bool(mode_np, "vsync-active-high");
> +		vm->interlaced = of_property_read_bool(mode_np, "interlaced");
> +		vm->doublescan = of_property_read_bool(mode_np, "doublescan");
> +
> +		if (strcmp(default_mode,mode_np->full_name) == 0)
> +			disp->default_mode = disp->num_modes;
> +
> +		disp->modes[disp->num_modes] = vm;
> +		disp->num_modes++;
> +	}
> +	of_node_put(display_np);
> +
> +	pr_info("%s: found %d modelines. Using #%d as default\n", __func__,
> +		disp->num_modes, disp->default_mode + 1);
> +
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(of_get_video_modes);
> +
> +int of_video_mode_exists(struct device_node *np)
> +{
> +	struct device_node *display_np;
> +	struct device_node *mode_np;
> +
> +	if (!np)
> +		return -EINVAL;
> +
> +	display_np = of_parse_phandle(np, "display", 0);
> +
> +	if (!display_np)
> +		return -EINVAL;
> +
> +	mode_np = of_parse_phandle(np, "default-mode", 0);
> +
> +	if (mode_np)
> +		return 0;
> +
> +	return -EINVAL;
> +}
> +EXPORT_SYMBOL_GPL(of_video_mode_exists);
> +
> +int of_get_display_mode(struct device_node *np, struct drm_display_mode *dmode, int index)
> +{
> +	struct display disp;
> +
> +	of_get_video_modes(np, &disp);
> +
> +	if (index == OF_MODE_SELECTION)
> +		index = disp.default_mode;
> +	if (dmode)
> +		videomode_to_display_mode(&disp, dmode, index);
> +
> +	of_video_free(&disp);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(of_get_display_mode);
> +
> +int of_get_fb_videomode(struct device_node *np, struct fb_videomode *fbmode, int index)
> +{
> +	struct display disp;
> +
> +	of_get_video_modes(np, &disp);
> +
> +	if (index == OF_MODE_SELECTION)
> +		index = disp.default_mode;
> +	if (fbmode)
> +		videomode_to_fb_mode(&disp, fbmode, index);
> +
> +	of_video_free(&disp);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(of_get_fb_videomode);
> diff --git a/include/linux/of_videomode.h b/include/linux/of_videomode.h
> new file mode 100644
> index 0000000..5571ce3
> --- /dev/null
> +++ b/include/linux/of_videomode.h
> @@ -0,0 +1,56 @@
> +/*
> + * Copyright 2012 Sascha Hauer <[e-mail]>
> + * Copyright 2012 Steffen Trumtrar <[e-mail]>
> + *
> + * OF helpers for videomodes.
> + *
> + * This file is released under the GPLv2
> + */
> +
> +#ifndef __LINUX_OF_VIDEOMODE_H
> +#define __LINUX_OF_VIDEOMODE_H
> +
> +#define OF_MODE_SELECTION -1
> +
> +struct mode_property {
> +	u32 min;
> +	u32 typ;
> +	u32 max;
> +};
> +
> +struct display {
> +	u32 width_mm;
> +	u32 height_mm;
> +	struct videomode **modes;
> +	int default_mode;
> +	int num_modes;
> +};
> +
> +/* describe videomode in terms of hardware parameters */
> +struct videomode {
> +	struct mode_property hback_porch;
> +	struct mode_property hfront_porch;
> +	struct mode_property hactive;
> +	struct mode_property hsync_len;
> +
> +	struct mode_property vback_porch;
> +	struct mode_property vfront_porch;
> +	struct mode_property vactive;
> +	struct mode_property vsync_len;
> +
> +	struct mode_property clock;
> +
> +	bool hah;
> +	bool vah;
> +	bool interlaced;
> +	bool doublescan;
> +};
> +
> +int of_video_mode_exists(struct device_node *np);
> +int videomode_to_display_mode(struct display *disp, struct drm_display_mode *dmode, int index);
> +int videomode_to_fb_mode(struct display *disp, struct fb_videomode *fbmode, int index);
> +int of_get_video_modes(struct device_node *np, struct display *disp);
> +int of_video_mode_exists(struct device_node *np);
> +int of_get_display_mode(struct device_node *np, struct drm_display_mode *dmode, int index);
> +int of_get_fb_videomode(struct device_node *np, struct fb_videomode *fbmode, int index);
> +#endif /* __LINUX_OF_VIDEOMODE_H */
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Sascha Hauer Sept. 24, 2012, 2:12 p.m. UTC | #6

Hi Rob,

On Mon, Sep 24, 2012 at 08:42:12AM -0500, Rob Herring wrote:
> On 09/19/2012 03:20 AM, Steffen Trumtrar wrote:
> > This patch adds a helper function for parsing videomodes from the devicetree.
> > The videomode can be either converted to a struct drm_display_mode or a
> > struct fb_videomode.
> > 
> > +
> > +Required properties:
> > + - hactive, vactive: Display resolution
> > + - hfront-porch, hback-porch, hsync-len: Horizontal Display timing parameters
> > +   in pixels
> > +   vfront-porch, vback-porch, vsync-len: Vertical display timing parameters in
> > +   lines
> > + - clock: displayclock in Hz
> 
> A major piece missing is the LCD controller to display interface width
> and component ordering.

Psst. We silently skipped this for now...

I think having such a videomode helper is useful without having the
data order part. We are aware that this should be added later, but
I think currently it makes sense to concentrate on the basics.

> 
> > +
> > +Optional properties:
> > + - width-mm, height-mm: Display dimensions in mm
> > + - hsync-active-high (bool): Hsync pulse is active high
> > + - vsync-active-high (bool): Vsync pulse is active high
> > + - interlaced (bool): This is an interlaced mode
> > + - doublescan (bool): This is a doublescan mode
> > +
> > +There are different ways of describing a display mode. The devicetree representation
> > +corresponds to the one commonly found in datasheets for displays.
> > +The description of the display and its mode is split in two parts: first the display
> > +properties like size in mm and (optionally) multiple subnodes with the supported modes.
> > +
> > +Example:
> > +
> > +	display@0 {
> 
> It would be useful to have a compatible string here. We may not always
> know the panel type or have a fixed panel though. We could define
> "generic-lcd" or something for cases where the panel type is unknown.

We expect the display nodes being subnodes of a driver (which of course
has a compatible), or maybe referred to from a driver using phandles. So
I don't see why the display node itself should have a compatible
property. The display information is only ever useful in the context of
a driver.

Sascha

Rob Herring Sept. 24, 2012, 3:18 p.m. UTC | #7

On 09/24/2012 09:12 AM, Sascha Hauer wrote:
> Hi Rob,
> 
> On Mon, Sep 24, 2012 at 08:42:12AM -0500, Rob Herring wrote:
>> On 09/19/2012 03:20 AM, Steffen Trumtrar wrote:
>>> This patch adds a helper function for parsing videomodes from the devicetree.
>>> The videomode can be either converted to a struct drm_display_mode or a
>>> struct fb_videomode.
>>>
>>> +
>>> +Required properties:
>>> + - hactive, vactive: Display resolution
>>> + - hfront-porch, hback-porch, hsync-len: Horizontal Display timing parameters
>>> +   in pixels
>>> +   vfront-porch, vback-porch, vsync-len: Vertical display timing parameters in
>>> +   lines
>>> + - clock: displayclock in Hz
>>
>> A major piece missing is the LCD controller to display interface width
>> and component ordering.
> 
> Psst. We silently skipped this for now...
> 
> I think having such a videomode helper is useful without having the
> data order part. We are aware that this should be added later, but
> I think currently it makes sense to concentrate on the basics.

Evolving bindings is not a good thing. We know this is needed, so we
should address it now. If Linux had a standard way to describe the
interface (it didn't a few years ago and I haven't kept up), then I
would bet it would already be part of this binding. Defining the
bindings in terms of what an OS uses or not is the wrong way around.

>>
>>> +
>>> +Optional properties:
>>> + - width-mm, height-mm: Display dimensions in mm
>>> + - hsync-active-high (bool): Hsync pulse is active high
>>> + - vsync-active-high (bool): Vsync pulse is active high
>>> + - interlaced (bool): This is an interlaced mode
>>> + - doublescan (bool): This is a doublescan mode
>>> +
>>> +There are different ways of describing a display mode. The devicetree representation
>>> +corresponds to the one commonly found in datasheets for displays.
>>> +The description of the display and its mode is split in two parts: first the display
>>> +properties like size in mm and (optionally) multiple subnodes with the supported modes.
>>> +
>>> +Example:
>>> +
>>> +	display@0 {
>>
>> It would be useful to have a compatible string here. We may not always
>> know the panel type or have a fixed panel though. We could define
>> "generic-lcd" or something for cases where the panel type is unknown.
> 
> We expect the display nodes being subnodes of a driver (which of course
> has a compatible), or maybe referred to from a driver using phandles. So
> I don't see why the display node itself should have a compatible
> property. The display information is only ever useful in the context of
> a driver.

A subnode or phandle will describe the h/w connection, but you need a
name to describe what is at each end of the connection.

Where would the model number of an lcd panel be captured then? The
timing parameters are a property of a specific panel, so both should be
described together. You may not have any use for the compatible string
now, but more information is better than less and adding it would not
hurt anything. For pretty much any other device sitting on a board, we
describe the manufacturer and type of device. LCD panels should be no
different.

Rob
--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Stephen Warren Sept. 24, 2012, 3:45 p.m. UTC | #8

On 09/24/2012 07:42 AM, Rob Herring wrote:
> On 09/19/2012 03:20 AM, Steffen Trumtrar wrote:
>> This patch adds a helper function for parsing videomodes from the devicetree.
>> The videomode can be either converted to a struct drm_display_mode or a
>> struct fb_videomode.

>> +++ b/Documentation/devicetree/bindings/video/displaymode
>> @@ -0,0 +1,74 @@
>> +videomode bindings
>> +==================
>> +
>> +Required properties:
>> + - hactive, vactive: Display resolution
>> + - hfront-porch, hback-porch, hsync-len: Horizontal Display timing parameters
>> +   in pixels
>> +   vfront-porch, vback-porch, vsync-len: Vertical display timing parameters in
>> +   lines
>> + - clock: displayclock in Hz
> 
> A major piece missing is the LCD controller to display interface width
> and component ordering.

I thought this binding was solely defining the timing of the video
signal (hence "video mode"). Any definition of the physical interface to
the LCD/display-connector is something entirely orthogonal, so it seems
entirely reasonable to represent that separately.

>> +Example:
>> +
>> +	display@0 {
> 
> It would be useful to have a compatible string here. We may not always
> know the panel type or have a fixed panel though. We could define
> "generic-lcd" or something for cases where the panel type is unknown.
> 
>> +		width-mm = <800>;
>> +		height-mm = <480>;

I would hope that everything in the example above this point is just
that - an example, and this binding only covers the display mode
definition - i.e. that part of the example below.

If that's not the intent, as Rob says, there's a /ton/ of stuff missing.

>> +		modes {
>> +			mode0: mode@0 {
>> +				/* 1920x1080p24 */
>> +				clock = <52000000>;
>> +				hactive = <1920>;
>> +				vactive = <1080>;
>> +				hfront-porch = <25>;
>> +				hback-porch = <25>;
>> +				hsync-len = <25>;
>> +				vback-porch = <2>;
>> +				vfront-porch = <2>;
>> +				vsync-len = <2>;
>> +				hsync-active-high;
>> +			};
>> +		};
>> +	};

--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Rob Herring Sept. 24, 2012, 6:26 p.m. UTC | #9

On 09/24/2012 10:45 AM, Stephen Warren wrote:
> On 09/24/2012 07:42 AM, Rob Herring wrote:
>> On 09/19/2012 03:20 AM, Steffen Trumtrar wrote:
>>> This patch adds a helper function for parsing videomodes from the devicetree.
>>> The videomode can be either converted to a struct drm_display_mode or a
>>> struct fb_videomode.
> 
>>> +++ b/Documentation/devicetree/bindings/video/displaymode
>>> @@ -0,0 +1,74 @@
>>> +videomode bindings
>>> +==================
>>> +
>>> +Required properties:
>>> + - hactive, vactive: Display resolution
>>> + - hfront-porch, hback-porch, hsync-len: Horizontal Display timing parameters
>>> +   in pixels
>>> +   vfront-porch, vback-porch, vsync-len: Vertical display timing parameters in
>>> +   lines
>>> + - clock: displayclock in Hz
>>
>> A major piece missing is the LCD controller to display interface width
>> and component ordering.
> 
> I thought this binding was solely defining the timing of the video
> signal (hence "video mode"). Any definition of the physical interface to
> the LCD/display-connector is something entirely orthogonal, so it seems
> entirely reasonable to represent that separately.

It is not orthogonal because in many cases the LCD panel defines the
mode. It is only cases where you just have a connector like hdmi that
you have multiple modes. Ideally, you would use EDID in those cases, but
obviously there are boards where that is missing or broken.

>>> +Example:
>>> +
>>> +	display@0 {
>>
>> It would be useful to have a compatible string here. We may not always
>> know the panel type or have a fixed panel though. We could define
>> "generic-lcd" or something for cases where the panel type is unknown.
>>
>>> +		width-mm = <800>;
>>> +		height-mm = <480>;
> 
> I would hope that everything in the example above this point is just
> that - an example, and this binding only covers the display mode
> definition - i.e. that part of the example below.
> 

It's fairly clear this binding is being defined based on what Linux
supports vs. what the h/w looks like.

> If that's not the intent, as Rob says, there's a /ton/ of stuff missing.

Assuming not, what all is missing?

Rob

> 
>>> +		modes {
>>> +			mode0: mode@0 {
>>> +				/* 1920x1080p24 */
>>> +				clock = <52000000>;
>>> +				hactive = <1920>;
>>> +				vactive = <1080>;
>>> +				hfront-porch = <25>;
>>> +				hback-porch = <25>;
>>> +				hsync-len = <25>;
>>> +				vback-porch = <2>;
>>> +				vfront-porch = <2>;
>>> +				vsync-len = <2>;
>>> +				hsync-active-high;
>>> +			};
>>> +		};
>>> +	};
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Sascha Hauer Sept. 24, 2012, 7:16 p.m. UTC | #10

On Mon, Sep 24, 2012 at 10:18:39AM -0500, Rob Herring wrote:
> On 09/24/2012 09:12 AM, Sascha Hauer wrote:
> >>
> >> A major piece missing is the LCD controller to display interface width
> >> and component ordering.
> > 
> > Psst. We silently skipped this for now...
> > 
> > I think having such a videomode helper is useful without having the
> > data order part. We are aware that this should be added later, but
> > I think currently it makes sense to concentrate on the basics.
> 
> Evolving bindings is not a good thing. We know this is needed, so we
> should address it now. If Linux had a standard way to describe the
> interface (it didn't a few years ago and I haven't kept up), then I
> would bet it would already be part of this binding. Defining the
> bindings in terms of what an OS uses or not is the wrong way around.

I see your point. I'm just afraid that if we start a discussion about
this now, it will take a long time we get something merged. In the
meantime we will get a lot of ad-hoc bindings like this one
suggested for the exynos:

> +       lcd_fimd0: lcd_panel0 {
> +                       lcd-htiming = <4 4 4 480>;
> +                       lcd-vtiming = <4 4 4 320>;
> +                       supports-mipi-panel;
> +       };

Or this one for the AMBA LCD controller;

> +       panel.mode.refresh      = get_val(node, "refresh");
> +       panel.mode.xres         = get_val(node, "xres");
> +       panel.mode.yres         = get_val(node, "yres");
> +       panel.mode.pixclock     = get_val(node, "pixclock");
> +       panel.mode.left_margin  = get_val(node, "left_margin");
> +       panel.mode.right_margin = get_val(node, "right_margin");
> +       panel.mode.upper_margin = get_val(node, "upper_margin");
> +       panel.mode.lower_margin = get_val(node, "lower_margin");
> +       panel.mode.hsync_len    = get_val(node, "hsync_len");
> +       panel.mode.vsync_len    = get_val(node, "vsync_len");
> +       panel.mode.sync         = get_val(node, "sync");
> +       panel.bpp               = get_val(node, "bpp");
> +       panel.width             = (signed short) get_val(node, "width");
> +       panel.height            = (signed short) get_val(node, "height");

(get_val() is a wrapper around of_property_read_u32)

BTW the SoC-camera guys will need a wire format description for their
cameras aswell.

> > 
> > We expect the display nodes being subnodes of a driver (which of course
> > has a compatible), or maybe referred to from a driver using phandles. So
> > I don't see why the display node itself should have a compatible
> > property. The display information is only ever useful in the context of
> > a driver.
> 
> A subnode or phandle will describe the h/w connection, but you need a
> name to describe what is at each end of the connection.
> 
> Where would the model number of an lcd panel be captured then? The
> timing parameters are a property of a specific panel, so both should be
> described together. You may not have any use for the compatible string
> now, but more information is better than less and adding it would not
> hurt anything. For pretty much any other device sitting on a board, we
> describe the manufacturer and type of device. LCD panels should be no
> different.

You convinced me. Lets add a compatible property, it won't hurt.

Sascha

Stephen Warren Sept. 24, 2012, 11:09 p.m. UTC | #11

On 09/24/2012 12:26 PM, Rob Herring wrote:
> On 09/24/2012 10:45 AM, Stephen Warren wrote:
>> On 09/24/2012 07:42 AM, Rob Herring wrote:
>>> On 09/19/2012 03:20 AM, Steffen Trumtrar wrote:
>>>> This patch adds a helper function for parsing videomodes from the devicetree.
>>>> The videomode can be either converted to a struct drm_display_mode or a
>>>> struct fb_videomode.
>>
>>>> +++ b/Documentation/devicetree/bindings/video/displaymode
>>>> @@ -0,0 +1,74 @@
>>>> +videomode bindings
>>>> +==================
>>>> +
>>>> +Required properties:
>>>> + - hactive, vactive: Display resolution
>>>> + - hfront-porch, hback-porch, hsync-len: Horizontal Display timing parameters
>>>> +   in pixels
>>>> +   vfront-porch, vback-porch, vsync-len: Vertical display timing parameters in
>>>> +   lines
>>>> + - clock: displayclock in Hz
>>>
>>> A major piece missing is the LCD controller to display interface width
>>> and component ordering.
>>
>> I thought this binding was solely defining the timing of the video
>> signal (hence "video mode"). Any definition of the physical interface to
>> the LCD/display-connector is something entirely orthogonal, so it seems
>> entirely reasonable to represent that separately.
> 
> It is not orthogonal because in many cases the LCD panel defines the
> mode.

The LCD panel itself defines both the mode and the physical interface
properties. The mode does not imply anything about the physical
interface, nor does the physical interface imply anything about the
mode. So, they are in fact orthogonal. In other words, just because you
need both sets of information, doesn't make the two pieces of
information correlated.

>>>> +Example:
>>>> +
>>>> +	display@0 {
>>>
>>> It would be useful to have a compatible string here. We may not always
>>> know the panel type or have a fixed panel though. We could define
>>> "generic-lcd" or something for cases where the panel type is unknown.
>>>
>>>> +		width-mm = <800>;
>>>> +		height-mm = <480>;
>>
>> I would hope that everything in the example above this point is just
>> that - an example, and this binding only covers the display mode
>> definition - i.e. that part of the example below.
>>
> 
> It's fairly clear this binding is being defined based on what Linux
> supports vs. what the h/w looks like.
> 
>> If that's not the intent, as Rob says, there's a /ton/ of stuff missing.
> 
> Assuming not, what all is missing?

Everything related to the physical interface:

* For DSI, whatever it needs to be configured.
* For LVDS, e.g. number of lanes of R, G, B.
* Perhaps multi-pumping rates (# of clock signals to send each data
value for, to satisfy any minimum clock rates)
* Built-in displays typically need to be coupled with a backlight and
all the associated control of that.
* Pinctrl interaction.

and probably a bunch of other things I haven't thought about.

--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Steffen Trumtrar Sept. 25, 2012, 7:51 a.m. UTC | #12

On Mon, Sep 24, 2012 at 09:16:28PM +0200, Sascha Hauer wrote:
> On Mon, Sep 24, 2012 at 10:18:39AM -0500, Rob Herring wrote:
> > On 09/24/2012 09:12 AM, Sascha Hauer wrote:
> > >>
> > >> A major piece missing is the LCD controller to display interface width
> > >> and component ordering.
> > > 
> > > Psst. We silently skipped this for now...
> > > 
> > > I think having such a videomode helper is useful without having the
> > > data order part. We are aware that this should be added later, but
> > > I think currently it makes sense to concentrate on the basics.
> > 
> > Evolving bindings is not a good thing. We know this is needed, so we
> > should address it now. If Linux had a standard way to describe the
> > interface (it didn't a few years ago and I haven't kept up), then I
> > would bet it would already be part of this binding. Defining the
> > bindings in terms of what an OS uses or not is the wrong way around.
> 
> I see your point. I'm just afraid that if we start a discussion about
> this now, it will take a long time we get something merged. In the
> meantime we will get a lot of ad-hoc bindings like this one
> suggested for the exynos:
> 
> > +       lcd_fimd0: lcd_panel0 {
> > +                       lcd-htiming = <4 4 4 480>;
> > +                       lcd-vtiming = <4 4 4 320>;
> > +                       supports-mipi-panel;
> > +       };
> 
> Or this one for the AMBA LCD controller;
> 
> > +       panel.mode.refresh      = get_val(node, "refresh");
> > +       panel.mode.xres         = get_val(node, "xres");
> > +       panel.mode.yres         = get_val(node, "yres");
> > +       panel.mode.pixclock     = get_val(node, "pixclock");
> > +       panel.mode.left_margin  = get_val(node, "left_margin");
> > +       panel.mode.right_margin = get_val(node, "right_margin");
> > +       panel.mode.upper_margin = get_val(node, "upper_margin");
> > +       panel.mode.lower_margin = get_val(node, "lower_margin");
> > +       panel.mode.hsync_len    = get_val(node, "hsync_len");
> > +       panel.mode.vsync_len    = get_val(node, "vsync_len");
> > +       panel.mode.sync         = get_val(node, "sync");
> > +       panel.bpp               = get_val(node, "bpp");
> > +       panel.width             = (signed short) get_val(node, "width");
> > +       panel.height            = (signed short) get_val(node, "height");
> 
> (get_val() is a wrapper around of_property_read_u32)
> 
> BTW the SoC-camera guys will need a wire format description for their
> cameras aswell.
> 
> > > 
> > > We expect the display nodes being subnodes of a driver (which of course
> > > has a compatible), or maybe referred to from a driver using phandles. So
> > > I don't see why the display node itself should have a compatible
> > > property. The display information is only ever useful in the context of
> > > a driver.
> > 
> > A subnode or phandle will describe the h/w connection, but you need a
> > name to describe what is at each end of the connection.
> > 
> > Where would the model number of an lcd panel be captured then? The
> > timing parameters are a property of a specific panel, so both should be
> > described together. You may not have any use for the compatible string
> > now, but more information is better than less and adding it would not
> > hurt anything. For pretty much any other device sitting on a board, we
> > describe the manufacturer and type of device. LCD panels should be no
> > different.
> 
> You convinced me. Lets add a compatible property, it won't hurt.

Okay. That doesn't seem to be a big problem. I can add that.

Regards,
Steffen

Steffen Trumtrar Sept. 25, 2012, 8:03 a.m. UTC | #13

On Mon, Sep 24, 2012 at 05:09:30PM -0600, Stephen Warren wrote:
> On 09/24/2012 12:26 PM, Rob Herring wrote:
> > On 09/24/2012 10:45 AM, Stephen Warren wrote:
> >> On 09/24/2012 07:42 AM, Rob Herring wrote:
> >>> On 09/19/2012 03:20 AM, Steffen Trumtrar wrote:
> >>>> This patch adds a helper function for parsing videomodes from the devicetree.
> >>>> The videomode can be either converted to a struct drm_display_mode or a
> >>>> struct fb_videomode.
> >>
> >>>> +++ b/Documentation/devicetree/bindings/video/displaymode
> >>>> @@ -0,0 +1,74 @@
> >>>> +videomode bindings
> >>>> +==================
> >>>> +
> >>>> +Required properties:
> >>>> + - hactive, vactive: Display resolution
> >>>> + - hfront-porch, hback-porch, hsync-len: Horizontal Display timing parameters
> >>>> +   in pixels
> >>>> +   vfront-porch, vback-porch, vsync-len: Vertical display timing parameters in
> >>>> +   lines
> >>>> + - clock: displayclock in Hz
> >>>
> >>> A major piece missing is the LCD controller to display interface width
> >>> and component ordering.
> >>
> >> I thought this binding was solely defining the timing of the video
> >> signal (hence "video mode"). Any definition of the physical interface to
> >> the LCD/display-connector is something entirely orthogonal, so it seems
> >> entirely reasonable to represent that separately.
> > 
> > It is not orthogonal because in many cases the LCD panel defines the
> > mode.
> 
> The LCD panel itself defines both the mode and the physical interface
> properties. The mode does not imply anything about the physical
> interface, nor does the physical interface imply anything about the
> mode. So, they are in fact orthogonal. In other words, just because you
> need both sets of information, doesn't make the two pieces of
> information correlated.
> 
> >>>> +Example:
> >>>> +
> >>>> +	display@0 {
> >>>
> >>> It would be useful to have a compatible string here. We may not always
> >>> know the panel type or have a fixed panel though. We could define
> >>> "generic-lcd" or something for cases where the panel type is unknown.
> >>>
> >>>> +		width-mm = <800>;
> >>>> +		height-mm = <480>;
> >>
> >> I would hope that everything in the example above this point is just
> >> that - an example, and this binding only covers the display mode
> >> definition - i.e. that part of the example below.
> >>
> > 
> > It's fairly clear this binding is being defined based on what Linux
> > supports vs. what the h/w looks like.
> > 
> >> If that's not the intent, as Rob says, there's a /ton/ of stuff missing.
> > 
> > Assuming not, what all is missing?
> 
> Everything related to the physical interface:
> 
> * For DSI, whatever it needs to be configured.
> * For LVDS, e.g. number of lanes of R, G, B.
> * Perhaps multi-pumping rates (# of clock signals to send each data
> value for, to satisfy any minimum clock rates)
> * Built-in displays typically need to be coupled with a backlight and
> all the associated control of that.
> * Pinctrl interaction.
> 
> and probably a bunch of other things I haven't thought about.

I already added some of those things in my v5. For those who missed it
<[e-mail]>
I renamed from "of videomode helper" to "of display helper", seemed to be more
clear what it is supposed to accomplish.

Regards,

Steffen

Laurent Pinchart Sept. 25, 2012, 11:23 a.m. UTC | #14

Hi Rob,

On Monday 24 September 2012 08:42:12 Rob Herring wrote:
> On 09/19/2012 03:20 AM, Steffen Trumtrar wrote:
> > This patch adds a helper function for parsing videomodes from the
> > devicetree. The videomode can be either converted to a struct
> > drm_display_mode or a struct fb_videomode.
> > 
> > Signed-off-by: Sascha Hauer <[e-mail]>
> > Signed-off-by: Steffen Trumtrar <[e-mail]>
> > ---
> > 
> > Hi!
> > 
> > changes since v3:
> > 	- print error messages
> > 	- free alloced memory
> > 	- general cleanup
> > 
> > Regards
> > Steffen
> > 
> >  .../devicetree/bindings/video/displaymode          |   74 +++++
> >  drivers/of/Kconfig                                 |    5 +
> >  drivers/of/Makefile                                |    1 +
> >  drivers/of/of_videomode.c                          |  283
> >  ++++++++++++++++++++ include/linux/of_videomode.h                      
> >  |   56 ++++
> >  5 files changed, 419 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/video/displaymode
> >  create mode 100644 drivers/of/of_videomode.c
> >  create mode 100644 include/linux/of_videomode.h
> > 
> > diff --git a/Documentation/devicetree/bindings/video/displaymode
> > b/Documentation/devicetree/bindings/video/displaymode new file mode
> > 100644
> > index 0000000..990ca52
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/video/displaymode
> > @@ -0,0 +1,74 @@
> > +videomode bindings
> > +==================
> > +
> > +Required properties:
> > + - hactive, vactive: Display resolution
> > + - hfront-porch, hback-porch, hsync-len: Horizontal Display timing
> > parameters +   in pixels
> > +   vfront-porch, vback-porch, vsync-len: Vertical display timing
> > parameters in +   lines
> > + - clock: displayclock in Hz
> 
> A major piece missing is the LCD controller to display interface width
> and component ordering.
> 
> > +
> > +Optional properties:
> > + - width-mm, height-mm: Display dimensions in mm
> > + - hsync-active-high (bool): Hsync pulse is active high
> > + - vsync-active-high (bool): Vsync pulse is active high
> > + - interlaced (bool): This is an interlaced mode
> > + - doublescan (bool): This is a doublescan mode
> > +
> > +There are different ways of describing a display mode. The devicetree
> > representation
> > +corresponds to the one commonly found in datasheets for displays.
> > +The description of the display and its mode is split in two parts: first
> > the display
> > +properties like size in mm and (optionally) multiple subnodes with the
> > supported modes.
> > +
> > +Example:
> > +
> > +	display@0 {
> 
> It would be useful to have a compatible string here. We may not always
> know the panel type or have a fixed panel though. We could define
> "generic-lcd" or something for cases where the panel type is unknown.

I'm working on a generic panel framework (see 
http://lwn.net/Articles/512363/). DT bindings are not there yet, but they're 
certainly a hot topic. A compatible string here will definitely be needed 
here.

The exact properties required in the display node will likely be panel-
dependent. width-mm, height-mm and modes sound like a good baseline to me.

> > +		width-mm = <800>;
> > +		height-mm = <480>;
> > +		modes {
> > +			mode0: mode@0 {
> > +				/* 1920x1080p24 */
> > +				clock = <52000000>;
> > +				hactive = <1920>;
> > +				vactive = <1080>;
> > +				hfront-porch = <25>;
> > +				hback-porch = <25>;
> > +				hsync-len = <25>;
> > +				vback-porch = <2>;
> > +				vfront-porch = <2>;
> > +				vsync-len = <2>;
> > +				hsync-active-high;
> > +			};
> > +		};
> > +	};
> > +
> > +Every property also supports the use of ranges, so the commonly used
> > datasheet +description with <min typ max>-tuples can be used.
> > +
> > +Example:
> > +
> > +	mode1: mode@1 {
> > +		/* 1920x1080p24 */
> > +		clock = <148500000>;
> > +		hactive = <1920>;
> > +		vactive = <1080>;
> > +		hsync-len = <0 44 60>;
> > +		hfront-porch = <80 88 95>;
> > +		hback-porch = <100 148 160>;
> > +		vfront-porch = <0 4 6>;
> > +		vback-porch = <0 36 50>;
> > +		vsync-len = <0 5 6>;
> > +	};
> > +
> > +The videomode can be linked to a connector via phandles. The connector
> > has to
> > +support the display- and default-mode-property to link to and select a
> > mode.
>
> Could also be phandle in the lcd controller node? What are the '-' for?
> Is "display-blah" a valid name or something?
> 
> "default-mode" is pretty generic. How about display-mode or
> display-default-mode?
> 
> Rob
> 
> > +
> > +Example:
> > +
> > +	hdmi@00120000 {
> > +		status = "okay";
> > +		display = <&benq>;
> > +		default-mode = <&mode1>;
> > +	};
> > +

Patch

diff --git a/Documentation/devicetree/bindings/video/displaymode b/Documentation/devicetree/bindings/video/displaymode
new file mode 100644
index 0000000..990ca52
--- /dev/null
+++ b/Documentation/devicetree/bindings/video/displaymode
@@ -0,0 +1,74 @@ 
+videomode bindings
+==================
+
+Required properties:
+ - hactive, vactive: Display resolution
+ - hfront-porch, hback-porch, hsync-len: Horizontal Display timing parameters
+   in pixels
+   vfront-porch, vback-porch, vsync-len: Vertical display timing parameters in
+   lines
+ - clock: displayclock in Hz
+
+Optional properties:
+ - width-mm, height-mm: Display dimensions in mm
+ - hsync-active-high (bool): Hsync pulse is active high
+ - vsync-active-high (bool): Vsync pulse is active high
+ - interlaced (bool): This is an interlaced mode
+ - doublescan (bool): This is a doublescan mode
+
+There are different ways of describing a display mode. The devicetree representation
+corresponds to the one commonly found in datasheets for displays.
+The description of the display and its mode is split in two parts: first the display
+properties like size in mm and (optionally) multiple subnodes with the supported modes.
+
+Example:
+
+	display@0 {
+		width-mm = <800>;
+		height-mm = <480>;
+		modes {
+			mode0: mode@0 {
+				/* 1920x1080p24 */
+				clock = <52000000>;
+				hactive = <1920>;
+				vactive = <1080>;
+				hfront-porch = <25>;
+				hback-porch = <25>;
+				hsync-len = <25>;
+				vback-porch = <2>;
+				vfront-porch = <2>;
+				vsync-len = <2>;
+				hsync-active-high;
+			};
+		};
+	};
+
+Every property also supports the use of ranges, so the commonly used datasheet
+description with <min typ max>-tuples can be used.
+
+Example:
+
+	mode1: mode@1 {
+		/* 1920x1080p24 */
+		clock = <148500000>;
+		hactive = <1920>;
+		vactive = <1080>;
+		hsync-len = <0 44 60>;
+		hfront-porch = <80 88 95>;
+		hback-porch = <100 148 160>;
+		vfront-porch = <0 4 6>;
+		vback-porch = <0 36 50>;
+		vsync-len = <0 5 6>;
+	};
+
+The videomode can be linked to a connector via phandles. The connector has to
+support the display- and default-mode-property to link to and select a mode.
+
+Example:
+
+	hdmi@00120000 {
+		status = "okay";
+		display = <&benq>;
+		default-mode = <&mode1>;
+	};
+
diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
index dfba3e6..a3acaa3 100644
--- a/drivers/of/Kconfig
+++ b/drivers/of/Kconfig
@@ -83,4 +83,9 @@  config OF_MTD
 	depends on MTD
 	def_bool y
 
+config OF_VIDEOMODE
+	def_bool y
+	help
+	  helper to parse videomodes from the devicetree
+
 endmenu # OF
diff --git a/drivers/of/Makefile b/drivers/of/Makefile
index e027f44..80e6db3 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_VIDEOMODE)	+= of_videomode.o
diff --git a/drivers/of/of_videomode.c b/drivers/of/of_videomode.c
new file mode 100644
index 0000000..52bfc74
--- /dev/null
+++ b/drivers/of/of_videomode.c
@@ -0,0 +1,283 @@ 
+/*
+ * OF helpers for parsing display modes
+ *
+ * Copyright (c) 2012 Sascha Hauer <[e-mail]>, Pengutronix
+ * Copyright (c) 2012 Steffen Trumtrar <[e-mail]>, Pengutronix
+ *
+ * This file is released under the GPLv2
+ */
+#include <linux/of.h>
+#include <linux/fb.h>
+#include <linux/export.h>
+#include <linux/slab.h>
+#include <drm/drmP.h>
+#include <drm/drm_crtc.h>
+#include <linux/of_videomode.h>
+
+static u32 of_video_get_value(struct mode_property *prop)
+{
+	return (prop->min >= prop->typ) ? prop->min : prop->typ;
+}
+
+/* read property into new mode_property */
+static int of_video_parse_property(struct device_node *np, char *name,
+				struct mode_property *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);
+
+	memset(result, 0, sizeof(*result));
+
+	ret = of_property_read_u32_array(np, name, &result->min, cells);
+
+	return ret;
+}
+
+static int of_video_free(struct display *disp)
+{
+	int i;
+
+	for (i=0; i<disp->num_modes; i++)
+		kfree(disp->modes[i]);
+	kfree(disp->modes);
+
+	return 0;
+}
+
+int videomode_to_display_mode(struct display *disp, struct drm_display_mode *dmode, int index)
+{
+	struct videomode *vm;
+
+	memset(dmode, 0, sizeof(*dmode));
+
+	if (index > disp->num_modes) {
+		pr_err("%s: wrong index: %d from %d\n", __func__, index, disp->num_modes);
+		return -EINVAL;
+	}
+
+	vm = disp->modes[index];
+
+	dmode->hdisplay = of_video_get_value(&vm->hactive);
+	dmode->hsync_start = of_video_get_value(&vm->hactive) + of_video_get_value(&vm->hfront_porch);
+	dmode->hsync_end = of_video_get_value(&vm->hactive) + of_video_get_value(&vm->hfront_porch)
+			+ of_video_get_value(&vm->hsync_len);
+	dmode->htotal = of_video_get_value(&vm->hactive) + of_video_get_value(&vm->hfront_porch)
+			+ of_video_get_value(&vm->hsync_len) + of_video_get_value(&vm->hback_porch);
+
+	dmode->vdisplay = of_video_get_value(&vm->vactive);
+	dmode->vsync_start = of_video_get_value(&vm->vactive) + of_video_get_value(&vm->vfront_porch);
+	dmode->vsync_end = of_video_get_value(&vm->vactive) + of_video_get_value(&vm->vfront_porch)
+			+ of_video_get_value(&vm->vsync_len);
+	dmode->vtotal = of_video_get_value(&vm->vactive) + of_video_get_value(&vm->vfront_porch) +
+			of_video_get_value(&vm->vsync_len) + of_video_get_value(&vm->vback_porch);
+
+	dmode->width_mm = disp->width_mm;
+	dmode->height_mm = disp->height_mm;
+
+	dmode->clock = of_video_get_value(&vm->clock) / 1000;
+
+	if (vm->hah)
+		dmode->flags |= DRM_MODE_FLAG_PHSYNC;
+	else
+		dmode->flags |= DRM_MODE_FLAG_NHSYNC;
+	if (vm->vah)
+		dmode->flags |= DRM_MODE_FLAG_PVSYNC;
+	else
+		dmode->flags |= DRM_MODE_FLAG_NVSYNC;
+	if (vm->interlaced)
+		dmode->flags |= DRM_MODE_FLAG_INTERLACE;
+	if (vm->doublescan)
+		dmode->flags |= DRM_MODE_FLAG_DBLSCAN;
+
+	drm_mode_set_name(dmode);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(videomode_to_display_mode);
+
+int videomode_to_fb_mode(struct display *disp, struct fb_videomode *fbmode, int index)
+{
+	struct videomode *vm;
+
+	memset(fbmode, 0, sizeof(*fbmode));
+
+	if (index > disp->num_modes) {
+		pr_err("%s: wrong index: %d from %d\n", __func__, index, disp->num_modes);
+		return -EINVAL;
+	}
+
+	vm = disp->modes[index];
+
+	fbmode->xres = of_video_get_value(&vm->hactive);
+	fbmode->left_margin = of_video_get_value(&vm->hback_porch);
+	fbmode->right_margin = of_video_get_value(&vm->hfront_porch);
+	fbmode->hsync_len = of_video_get_value(&vm->hsync_len);
+
+	fbmode->yres = of_video_get_value(&vm->vactive);
+	fbmode->upper_margin = of_video_get_value(&vm->vback_porch);
+	fbmode->lower_margin = of_video_get_value(&vm->vfront_porch);
+	fbmode->vsync_len = of_video_get_value(&vm->vsync_len);
+
+	fbmode->pixclock = KHZ2PICOS(of_video_get_value(&vm->clock) / 1000);
+
+	if (vm->hah)
+		fbmode->sync |= FB_SYNC_HOR_HIGH_ACT;
+	if (vm->vah)
+		fbmode->sync |= FB_SYNC_VERT_HIGH_ACT;
+	if (vm->interlaced)
+		fbmode->vmode |= FB_VMODE_INTERLACED;
+	if (vm->doublescan)
+		fbmode->vmode |= FB_VMODE_DOUBLE;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(videomode_to_fb_mode);
+
+int of_get_video_modes(struct device_node *np, struct display *disp)
+{
+	struct device_node *display_np;
+	struct device_node *mode_np;
+	struct device_node *modes_np;
+	char *default_mode;
+
+	int ret = 0;
+
+	memset(disp, 0, sizeof(*disp));
+	disp->modes = kmalloc(sizeof(struct videomode*), GFP_KERNEL);
+
+	if (!np) {
+		pr_err("%s: no node provided\n", __func__);
+		return -EINVAL;
+	}
+
+	display_np = of_parse_phandle(np, "display", 0);
+
+	if (!display_np) {
+		pr_err("%s: could not find display node\n", __func__);
+		return -EINVAL;
+	}
+
+	of_property_read_u32(display_np, "width-mm", &disp->width_mm);
+	of_property_read_u32(display_np, "height-mm", &disp->height_mm);
+
+	mode_np = of_parse_phandle(np, "default-mode", 0);
+
+	if (!mode_np) {
+		pr_info("%s: no default-mode specified.\n", __func__);
+		mode_np = of_find_node_by_name(np, "mode");
+	}
+
+	if (!mode_np) {
+		pr_err("%s: could not find any mode specification\n", __func__);
+		return -EINVAL;
+	}
+
+	default_mode = (char *)mode_np->full_name;
+
+	modes_np = of_find_node_by_name(np, "modes");
+	for_each_child_of_node(modes_np, mode_np) {
+		struct videomode *vm;
+
+		vm = kmalloc(sizeof(struct videomode*), GFP_KERNEL);
+		disp->modes[disp->num_modes] = kmalloc(sizeof(struct videomode*), GFP_KERNEL);
+
+		ret |= of_video_parse_property(mode_np, "hback-porch", &vm->hback_porch);
+		ret |= of_video_parse_property(mode_np, "hfront-porch", &vm->hfront_porch);
+		ret |= of_video_parse_property(mode_np, "hactive", &vm->hactive);
+		ret |= of_video_parse_property(mode_np, "hsync-len", &vm->hsync_len);
+		ret |= of_video_parse_property(mode_np, "vback-porch", &vm->vback_porch);
+		ret |= of_video_parse_property(mode_np, "vfront-porch", &vm->vfront_porch);
+		ret |= of_video_parse_property(mode_np, "vactive", &vm->vactive);
+		ret |= of_video_parse_property(mode_np, "vsync-len", &vm->vsync_len);
+		ret |= of_video_parse_property(mode_np, "clock", &vm->clock);
+
+		if (ret)
+			return -EINVAL;
+
+		vm->hah = of_property_read_bool(mode_np, "hsync-active-high");
+		vm->vah = of_property_read_bool(mode_np, "vsync-active-high");
+		vm->interlaced = of_property_read_bool(mode_np, "interlaced");
+		vm->doublescan = of_property_read_bool(mode_np, "doublescan");
+
+		if (strcmp(default_mode,mode_np->full_name) == 0)
+			disp->default_mode = disp->num_modes;
+
+		disp->modes[disp->num_modes] = vm;
+		disp->num_modes++;
+	}
+	of_node_put(display_np);
+
+	pr_info("%s: found %d modelines. Using #%d as default\n", __func__,
+		disp->num_modes, disp->default_mode + 1);
+
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(of_get_video_modes);
+
+int of_video_mode_exists(struct device_node *np)
+{
+	struct device_node *display_np;
+	struct device_node *mode_np;
+
+	if (!np)
+		return -EINVAL;
+
+	display_np = of_parse_phandle(np, "display", 0);
+
+	if (!display_np)
+		return -EINVAL;
+
+	mode_np = of_parse_phandle(np, "default-mode", 0);
+
+	if (mode_np)
+		return 0;
+
+	return -EINVAL;
+}
+EXPORT_SYMBOL_GPL(of_video_mode_exists);
+
+int of_get_display_mode(struct device_node *np, struct drm_display_mode *dmode, int index)
+{
+	struct display disp;
+
+	of_get_video_modes(np, &disp);
+
+	if (index == OF_MODE_SELECTION)
+		index = disp.default_mode;
+	if (dmode)
+		videomode_to_display_mode(&disp, dmode, index);
+
+	of_video_free(&disp);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(of_get_display_mode);
+
+int of_get_fb_videomode(struct device_node *np, struct fb_videomode *fbmode, int index)
+{
+	struct display disp;
+
+	of_get_video_modes(np, &disp);
+
+	if (index == OF_MODE_SELECTION)
+		index = disp.default_mode;
+	if (fbmode)
+		videomode_to_fb_mode(&disp, fbmode, index);
+
+	of_video_free(&disp);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(of_get_fb_videomode);
diff --git a/include/linux/of_videomode.h b/include/linux/of_videomode.h
new file mode 100644
index 0000000..5571ce3
--- /dev/null
+++ b/include/linux/of_videomode.h
@@ -0,0 +1,56 @@ 
+/*
+ * Copyright 2012 Sascha Hauer <[e-mail]>
+ * Copyright 2012 Steffen Trumtrar <[e-mail]>
+ *
+ * OF helpers for videomodes.
+ *
+ * This file is released under the GPLv2
+ */
+
+#ifndef __LINUX_OF_VIDEOMODE_H
+#define __LINUX_OF_VIDEOMODE_H
+
+#define OF_MODE_SELECTION -1
+
+struct mode_property {
+	u32 min;
+	u32 typ;
+	u32 max;
+};
+
+struct display {
+	u32 width_mm;
+	u32 height_mm;
+	struct videomode **modes;
+	int default_mode;
+	int num_modes;
+};
+
+/* describe videomode in terms of hardware parameters */
+struct videomode {
+	struct mode_property hback_porch;
+	struct mode_property hfront_porch;
+	struct mode_property hactive;
+	struct mode_property hsync_len;
+
+	struct mode_property vback_porch;
+	struct mode_property vfront_porch;
+	struct mode_property vactive;
+	struct mode_property vsync_len;
+
+	struct mode_property clock;
+
+	bool hah;
+	bool vah;
+	bool interlaced;
+	bool doublescan;
+};
+
+int of_video_mode_exists(struct device_node *np);
+int videomode_to_display_mode(struct display *disp, struct drm_display_mode *dmode, int index);
+int videomode_to_fb_mode(struct display *disp, struct fb_videomode *fbmode, int index);
+int of_get_video_modes(struct device_node *np, struct display *disp);
+int of_video_mode_exists(struct device_node *np);
+int of_get_display_mode(struct device_node *np, struct drm_display_mode *dmode, int index);
+int of_get_fb_videomode(struct device_node *np, struct fb_videomode *fbmode, int index);
+#endif /* __LINUX_OF_VIDEOMODE_H */