On Mon, 31 Jan 2011 15:00:23 +0800, "Lan, Hai" <hai....@intel.com> wrote:
> Add test case for dumping display into, test all mode(including clone/twin
> mode) and support to test forced mode in testdisplay
> usage: ./testdisplay [-hiaf:]
> -i dump info
> -a test all modes
> -f <clock
> MHz>,<hdisp>,<hsync-start>,<hsync-end>,<htotal>,<vdisp>,<vsync-start>,<vsync-end>,<vtotal>
> test force mode
> Default is to test the preferred mode.
This was unnecessarily tricky to review as the patch was attached as an
octet-stream rather than text/plain. (Hence also why I've not inlined my
comments.)
I did see a few points worth mentioning.
The patch summary should ideally be less than 80 characters, so that it
doesn't wrap in condensed views (shortlogs and the like).
Even for a seemingly trivial patch such as this, it is useful to explain
why we need it in the commit log. Especially how this complements
libdrm/tests/modeprint.
And it a useful habit to get into is to sign-off on all your changes. The
sign-off says that you know the work is genuine and have a legitimate right
to use the code in this project.
Please no my_*. Definitely not as globals.
Lots of non-static functions used in static context. (This is not C++ ;-)
An entirely superfluous function:
static void drm_props() { return; // don't show props }
in a patch adding command line options to select what to show.
Instances of bad whitespace: if(force_mode){
sleep(5)!
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
