Re: [PATCH 2/2 v6] of: add generic videomode description

On Tue, Oct 09, 2012 at 09:26:08AM +0200, Steffen Trumtrar wrote:

Hi Laurent,

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

quoted

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

quoted

quoted

quoted

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

In that case we should export videomode_from_timing(). For Tegra DRM I
wrote a small utility function that takes a struct display_timings and
converts every timing to a struct videomode which is then converted to
a struct drm_display_mode and added to the DRM connector. The code is
really generic and could be part of the DRM core.

Thierry