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