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) 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. 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- | -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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) 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? 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
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) 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. 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- | -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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) 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 signature.asc Description: OpenPGP digital signature
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) 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? 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 -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
