Re: [PATCH v17 2/7] video: add display_timing and videomode
Hi! On Wed, Feb 27, 2013 at 06:13:49PM +0200, Tomi Valkeinen wrote: > 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_LOWBIT(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_INTERLACEDBIT(4) > +#define DISPLAY_FLAGS_DOUBLESCANBIT(5) > >>> > >>> > >>> > +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... > >>> 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. > 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- | ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v17 2/7] video: add display_timing and videomode
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_LOWBIT(0) +#define VESA_DMT_HSYNC_HIGH BIT(1) +#define VESA_DMT_VSYNC_LOWBIT(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) >>> >>> >>> + 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 signature.asc Description: OpenPGP digital signature ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v17 2/7] video: add display_timing and videomode
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_LOWBIT(0) > >> +#define VESA_DMT_HSYNC_HIGH BIT(1) > >> +#define VESA_DMT_VSYNC_LOWBIT(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) > > > > > > > >> + 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. > > 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. > > 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. 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- | ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v17 2/7] video: add display_timing and videomode
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_LOWBIT(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_INTERLACEDBIT(4) >> +#define DISPLAY_FLAGS_DOUBLESCANBIT(5) > > > >> +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? > > Would it be simpler to just have "flags" field? What does it give us to > have those two separately? > > Should the above say raising edge/falling edge instead of positive > edge/negative edge? > > Tomi > signature.asc Description: OpenPGP digital signature ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v17 2/7] video: add display_timing and videomode
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_HIGHBIT(1) > +#define DISPLAY_FLAGS_PIXDATA_POSEDGEBIT(2) /* drive data on pos. > edge */ > +#define DISPLAY_FLAGS_PIXDATA_NEGEDGEBIT(3) /* drive data on neg. > edge */ > +#define DISPLAY_FLAGS_INTERLACED BIT(4) > +#define DISPLAY_FLAGS_DOUBLESCAN BIT(5) > + 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? Would it be simpler to just have "flags" field? What does it give us to have those two separately? Should the above say raising edge/falling edge instead of positive edge/negative edge? Tomi ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
