Hi Steffen,

On Monday 08 October 2012 14:48:01 Steffen Trumtrar wrote:

On Mon, Oct 08, 2012 at 02:13:50PM +0200, Laurent Pinchart wrote:

quoted

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

quoted

Get videomode from devicetree in a format appropriate for the
backend. drm_display_mode and fb_videomode are supported atm.
Uses the display signal timings from of_display_timings

Signed-off-by: Steffen Trumtrar <redacted>
---

 drivers/of/Kconfig           |    5 +
 drivers/of/Makefile          |    1 +
 drivers/of/of_videomode.c    |  212 +++++++++++++++++++++++++++++++++++
 include/linux/of_videomode.h |   41 ++++++++
 4 files changed, 259 insertions(+)
 create mode 100644 drivers/of/of_videomode.c
 create mode 100644 include/linux/of_videomode.h

[snip]

quoted

quoted

diff --git a/drivers/of/of_videomode.c b/drivers/of/of_videomode.c
new file mode 100644
index 0000000..76ac16e
--- /dev/null
+++ b/drivers/of/of_videomode.c

[snip]

quoted

quoted

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

The caller must provide the struct videomode, yes. Wouldn't the kernel hang
itself with a NULL pointer exception, if I just work with it ?

The kernel would oops, clearly showing the caller that a non-null vm is needed 
:-)

quoted

quoted

+	st = display_timings_get(disp, index);
+

You can remove the blank line.

quoted

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

The intent was to let the driver decide. That way all the other overhead may
be skipped.

My point is that driver writers might just call of_get_videomode() in a loop, 
not knowing that it's expensive. I want to avoid that. We need to at least add 
kerneldoc to the function stating that this shouldn't be done.

quoted

quoted

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

No need to assign ret to 0 here.

Ah, yes. Unneeded in this case.

quoted

quoted

+
+	disp = of_get_display_timing_list(np);
+

You can remove the blank line.

quoted

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

quoted

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

Okay.

quoted

quoted

+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(of_get_videomode);

-- 
Regards,

Laurent Pinchart