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

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

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

On 2013-02-27 18:05, Steffen Trumtrar wrote:
> Ah, sorry. Forgot to answer this.
> 
> On Wed, Feb 27, 2013 at 05:45:31PM +0200, Tomi Valkeinen wrote:
>> Ping.
>>
>> On 2013-02-18 16:09, Tomi Valkeinen wrote:
>>> Hi Steffen,
>>>
>>> On 2013-01-25 11:01, Steffen Trumtrar wrote:
>>>
>>>> +/* 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>
>>>
>>>> +	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.

>>> 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 =).

>>> 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.

 Tomi



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