Hi Steffen,

Thanks for the patch.

On Thursday 04 October 2012 19:59:20 Steffen Trumtrar wrote:

That's going to be pretty difficult to read :-) Would it make sense to group 
several attributes logically (for instance using %ux%u for m->xres, m->yres) ?

+                m->refresh, m->xres, m->yres, m->pixclock, m->left_margin,
+                m->right_margin, m->upper_margin, m->lower_margin, +      
         m->hsync_len, m->vsync_len, m->sync, m->vmode, m->flag);
+}

Shouldn't this (and the other non exported functions below) be static ?

+void dump_drm_displaymode(struct drm_display_mode *m)
+{
+        pr_debug("drm_displaymode = %d %d %d %d %d %d %d %d %d\n",
+                m->hdisplay, m->hsync_start, m->hsync_end, m->htotal,
+                m->vdisplay, m->vsync_start, m->vsync_end, m->vtotal,
+                m->clock);
+}
+
+int videomode_from_timing(struct display_timings *disp, struct videomode
*vm,
+			int index)
+{
+	struct signal_timing *st = NULL;
+
+	if (!vm)
+		return -EINVAL;
+

What about making vm a mandatory argument ? It looks to me like a caller bug 
if vm is NULL.

+	st = display_timings_get(disp, index);
+

You can remove the blank line.

+	if (!st) {
+		pr_err("%s: no signal timings found\n", __func__);
+		return -EINVAL;
+	}
+
+	vm->pixelclock = signal_timing_get_value(&st->pixelclock, 0);
+	vm->hactive = signal_timing_get_value(&st->hactive, 0);
+	vm->hfront_porch = signal_timing_get_value(&st->hfront_porch, 0);
+	vm->hback_porch = signal_timing_get_value(&st->hback_porch, 0);
+	vm->hsync_len = signal_timing_get_value(&st->hsync_len, 0);
+
+	vm->vactive = signal_timing_get_value(&st->vactive, 0);
+	vm->vfront_porch = signal_timing_get_value(&st->vfront_porch, 0);
+	vm->vback_porch = signal_timing_get_value(&st->vback_porch, 0);
+	vm->vsync_len = signal_timing_get_value(&st->vsync_len, 0);
+
+	vm->vah = st->vsync_pol_active_high;
+	vm->hah = st->hsync_pol_active_high;
+	vm->interlaced = st->interlaced;
+	vm->doublescan = st->doublescan;
+
+	return 0;
+}
+
+int of_get_videomode(struct device_node *np, struct videomode *vm, int
index)

I wonder how to avoid abuse of this functions. It's a useful helper for 
drivers that need to get a video mode once only, but would result in lower 
performances if a driver calls it for every mode. Drivers must call 
of_get_display_timing_list instead in that case and case the display timings. 
I'm wondering whether we should really expose of_get_videomode.

+{
+	struct display_timings *disp;
+	int ret = 0;

No need to assign ret to 0 here.

+
+	disp = of_get_display_timing_list(np);
+

You can remove the blank line.

+	if (!disp) {
+		pr_err("%s: no timings specified\n", __func__);
+		return -EINVAL;
+	}
+
+	if (index == OF_DEFAULT_TIMING)
+		index = disp->default_timing;
+
+	ret = videomode_from_timing(disp, vm, index);
+

No need for a blank line.

+	if (ret)
+		return ret;
+
+	display_timings_release(disp);
+
+	if (!vm) {
+		pr_err("%s: could not get videomode %d\n", __func__, index);
+		return -EINVAL;
+	}

This can't happen. If vm is NULL the videomode_from_timing call above will 
return -EINVAL, and this function will then return immediately without 
reaching this code block.

+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(of_get_videomode);
+
+#if defined(CONFIG_DRM)
+int videomode_to_display_mode(struct videomode *vm, struct drm_display_mode
*dmode)
+{
+	memset(dmode, 0, sizeof(*dmode));
+
+	dmode->hdisplay = vm->hactive;
+	dmode->hsync_start = dmode->hdisplay + vm->hfront_porch;
+	dmode->hsync_end = dmode->hsync_start + vm->hsync_len;
+	dmode->htotal = dmode->hsync_end + vm->hback_porch;
+
+	dmode->vdisplay = vm->vactive;
+	dmode->vsync_start = dmode->vdisplay + vm->vfront_porch;
+	dmode->vsync_end = dmode->vsync_start + vm->vsync_len;
+	dmode->vtotal = dmode->vsync_end + vm->vback_porch;
+
+	dmode->clock = vm->pixelclock / 1000;
+
+	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 of_get_drm_display_mode(struct device_node *np, struct drm_display_mode
*dmode,
+			int index)

Same as above, do we really need to expose this helper function ? If so we 
should at least clearly document (using kerneldoc for instance) that drivers 
should only use it if they need to get a single mode once.

+{
+	struct videomode vm;
+	int ret;
+
+	ret = of_get_videomode(np, &vm, index);
+
+	if (ret)
+		return ret;
+
+	videomode_to_display_mode(&vm, dmode);
+
+	pr_info("%s: got %dx%d display mode from %s\n", __func__, vm.hactive,
+		vm.vactive, np->name);
+	dump_drm_displaymode(dmode);
+
+	return 0;
+
+}
+EXPORT_SYMBOL_GPL(of_get_drm_display_mode);
+#else
+int videomode_to_display_mode(struct videomode *vm, struct drm_display_mode
*dmode)
+{
+	return 0;
+}
+
+int of_get_drm_display_mode(struct device_node *np, struct drm_display_mode
*dmode,
+			int index)
+{
+	return 0;
+}

What about not defining those if CONFIG_DRM is not set ? No driver should call 
these functions in that case. If we really need those stubs they should return 
an error.

-- 
Regards,

Laurent Pinchart