Hi Laurent,

On Mon, Oct 08, 2012 at 10:52:04PM +0200, Laurent Pinchart wrote:

Hi Steffen,

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

quoted

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

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

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 
:-)

Okay. No error checking it is then.

quoted

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.

You're right. That should be made clear in the code.

quoted

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

Regards,

Steffen

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |