Re: [PATCH v4] of: Add videomode helper

[Tomi Valkeinen <tomi.valkeinen@ti.com>]

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

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 <s.hauer@pengutronix.de>
> Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
> ---
> 
> 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 <s.hauer@pengutronix.de>, Pengutronix
> + * Copyright (c) 2012 Steffen Trumtrar <s.trumtrar@pengutronix.de>, Pengutronix
> + *
> + * This file is released under the GPLv2
> + */
> +#include <linux/of.h>
> +#include <linux/fb.h>
> +#include <linux/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 <s.hauer@pengutronix.de>
> + * Copyright 2012 Steffen Trumtrar <s.trumtrar@pengutronix.de>
> + *
> + * 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


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