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

[Tomi Valkeinen <tomi.valkeinen@ti.com>]

[-- Attachment #1: Type: text/plain, Size: 1378 bytes --]

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



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 899 bytes --]