# Re: \[PATCHv15 2/7\] video: add display\_timing and videomode

- From: Tomi Valkeinen \<hidden\>
- Date: 2012-11-26 12:37:26 
- List: linux-fbdev
- Page: https://ratatoskr.run/linux-fbdev/2012/11/8066093
- Thread: https://ratatoskr.run/linux-fbdev/2012/11/8066080/t.md

```text
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


```

## Attachments

- [signature.asc](https://ratatoskr.run/linux-fbdev/2012/11/8066093/attachment/0) (application/pgp-signature, 899 bytes)