Re: [PATCH v4] of: Add videomode helper

On Wed, 2012-09-19 at 10:20 +0200, Steffen Trumtrar wrote:

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.

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