[PATCHv15 2/7] video: add display_timing and videomode

[Tomi Valkeinen]

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


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 899 bytes
Desc: OpenPGP digital signature
URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20121126/1cdf4e51/attachment-0001.pgp>