On 2012-11-26 11:07, Steffen Trumtrar wrote:

+/*
+ * Subsystem independent description of a videomode.
+ * Can be generated from struct display_timing.
+ */
+struct videomode {
+	u32 pixelclock;		/* pixelclock in Hz */

I don't know if this is of any importance, but the linux clock framework
manages clock rates with unsigned long. Would it be better to use the
same type here?

+	u32 hactive;
+	u32 hfront_porch;
+	u32 hback_porch;
+	u32 hsync_len;
+
+	u32 vactive;
+	u32 vfront_porch;
+	u32 vback_porch;
+	u32 vsync_len;
+
+	u32 hah;		/* hsync active high */
+	u32 vah;		/* vsync active high */
+	u32 de;			/* data enable */
+	u32 pixelclk_pol;

What values will these 4 have? Aren't these booleans?

The data enable comment should be "data enable active high".

The next patch says in the devtree binding documentation that the values
may be on/off/ignored. Is that represented here somehow, i.e. values are
0/1/2 or so? If not, what does it mean if the value is left out from
devtree data, meaning "not used on hardware"?

There's also a "doubleclk" value mentioned in the devtree bindings doc,
but not shown here.

I think this videomode struct is the one that display drivers are going
to use (?), in addition to the display_timing, so it would be good to
document it well.

 Tomi