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

Hi Steffen,

On Monday 08 October 2012 09:57:41 Steffen Trumtrar wrote:

On Mon, Oct 08, 2012 at 10:21:53AM +0300, Tomi Valkeinen wrote:

quoted

On Thu, 2012-10-04 at 19:59 +0200, Steffen Trumtrar wrote:

[snip]

quoted

quoted

diff --git a/include/linux/of_videomode.h b/include/linux/of_videomode.h
new file mode 100644
index 0000000..96efe01
--- /dev/null
+++ b/include/linux/of_videomode.h

@@ -0,0 +1,41 @@
+/*
+ * Copyright 2012 Steffen Trumtrar <s.trumtrar@pengutronix.de>
+ *
+ * generic videomode description
+ *
+ * This file is released under the GPLv2
+ */
+
+#ifndef __LINUX_VIDEOMODE_H
+#define __LINUX_VIDEOMODE_H
+
+#include <drm/drmP.h>

You don't need to include this.

That is a fix to my liking. Easily done ;-)

quoted

quoted

+struct videomode {
+	u32 pixelclock;
+	u32 refreshrate;
+
+	u32 hactive;
+	u32 hfront_porch;
+	u32 hback_porch;
+	u32 hsync_len;
+
+	u32 vactive;
+	u32 vfront_porch;
+	u32 vback_porch;
+	u32 vsync_len;
+
+	bool hah;
+	bool vah;
+	bool interlaced;
+	bool doublescan;
+
+};

This is not really of related. And actually, neither is the struct
signal_timing in the previous patch. It would be nice to have these in a
common header that fb, drm, and others could use instead of each having
their own timing structs.

But that's probably out of scope for this series =). Did you check the
timing structs from the video related frameworks in the kernel to see if
your structs contain all the info the others have, so that, at least in
theory, everybody could use these common structs?

 Tomi

Yes. Stephen and Laurent already suggested to split it up.
No, all info is not contained. That starts with drm, which has width-mm,..
If time permits, I will go over that.

Just to make sure we won't forget it, the V4L2 version of the timings 
structure is struct v4l2_bt_timings in include/linux/videodev2.h.

-- 
Regards,

Laurent Pinchart