Hi!

On Wed, Feb 27, 2013 at 06:13:49PM +0200, Tomi Valkeinen wrote:

On 2013-02-27 18:05, Steffen Trumtrar wrote:

quoted

Ah, sorry. Forgot to answer this.

On Wed, Feb 27, 2013 at 05:45:31PM +0200, Tomi Valkeinen wrote:

quoted

Ping.

On 2013-02-18 16:09, Tomi Valkeinen wrote:

quoted

Hi Steffen,

On 2013-01-25 11:01, Steffen Trumtrar wrote:

quoted

+/* VESA display monitor timing parameters */
+#define VESA_DMT_HSYNC_LOW		BIT(0)
+#define VESA_DMT_HSYNC_HIGH		BIT(1)
+#define VESA_DMT_VSYNC_LOW		BIT(2)
+#define VESA_DMT_VSYNC_HIGH		BIT(3)
+
+/* display specific flags */
+#define DISPLAY_FLAGS_DE_LOW		BIT(0)	/* data enable flag */
+#define DISPLAY_FLAGS_DE_HIGH		BIT(1)
+#define DISPLAY_FLAGS_PIXDATA_POSEDGE	BIT(2)	/* drive data on pos. edge */
+#define DISPLAY_FLAGS_PIXDATA_NEGEDGE	BIT(3)	/* drive data on neg. edge */
+#define DISPLAY_FLAGS_INTERLACED	BIT(4)
+#define DISPLAY_FLAGS_DOUBLESCAN	BIT(5)

<snip>

quoted

+	unsigned int dmt_flags;	/* VESA DMT flags */
+	unsigned int data_flags; /* video data flags */

Why did you go for this approach? To be able to represent
true/false/not-specified?

We decided somewhere between v3 and v8 (I think), that those flags can be
high/low/ignored.

Okay. Why aren't they enums, though? That always makes more clear which
defines are to be used with which fields.

Hm...

quoted

quoted

quoted

Would it be simpler to just have "flags" field? What does it give us to
have those two separately?

I decided to split them, so it is clear that some flags are VESA defined and
the others are "invented" for the display-timings framework and may be
extended.

Hmm... Okay. Is it relevant that they are VESA defined? It just feels to
complicate handling the flags =).

quoted

quoted

quoted

Should the above say raising edge/falling edge instead of positive
edge/negative edge?

Hm, I used posedge/negedge because it is shorter (and because of my Verilog past
pretty natural to me :-) ). I don't know what others are thinking though.

I guess it's quite clear, but it's still different terms than used
elsewhere, e.g. documentation for videomodes.

Another thing I noticed while using the new videomode, display_timings.h
has a few names that are quite short and generic. Like "TE_MIN", which
is now a global define. And "timing_entry". Either name could be well
used internally in some .c file, and could easily clash.

Yes. You are correct.
Everyone using this is welcome to send patches now :-)

Regards,
Steffen

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |