https://bugs.freedesktop.org/show_bug.cgi?id=93243
--- Comment #1 from Benno Schulenberg <bensb...@justemail.net> ---
Hi Kevin,
Some general comments about your patches.
* The subject line of a patch should summarize the patch in 50 characters.
Then there should be a blank line, and then can follow a longer explanation, of
which the lines may be 72/74 characters long.
* You make changes like this:
-void
-ViaCRTCInit(ScrnInfoPtr pScrn)
+void ViaCRTCInit(ScrnInfoPtr pScrn)
Please don't do that. The style is to have return type of a function on a
separate line. It may be ugly, but that is the style. Please leave it so.
* I see for example this
+/*
hwp->writeSeq(hwp, 0x10, 0x01);
+*/
Please simply delete things instead of commenting them out.
Or /if/ you want comment them out, use // instead -- it's much shorter.
+/* Sequencer Extended Registers */
+/* Extended Register Unlock (0x3c5.0x10[0]) */
Too verbose. Unneeded.
+/* Set Extended Register Unlock register bit [0] to 1 in order to */
+/* enable access to VIA Technologies graphics extended graphics */
+/* features. */
+ ViaSeqMask(hwp, 0x10, 0x01, 0x01);
"VIA Technologies graphics" is unneeded; of course openchrome is about VIA
Technologies graphics.
* The following hides the actual change that you made.
- ViaSeqMask(hwp, 0x16, 0x08, 0xBF);
- ViaSeqMask(hwp, 0x17, 0x1F, 0xFF);
- ViaSeqMask(hwp, 0x18, 0x4E, 0xFF);
- ViaSeqMask(hwp, 0x1A, 0x08, 0xFD);
+ ViaSeqMask(hwp, 0x16, 0x08, 0xbf);
+ ViaSeqMask(hwp, 0x17, 0x1f, 0xff);
+ ViaSeqMask(hwp, 0x18, 0x4e, 0xff);
+ ViaSeqMask(hwp, 0x1a, 0x08, 0xf9);
So please keep the uppercase and only change the actual bit that needs
changing.
* You seem to start comments always in column zero. But the existing code
indents comments as much as the code it precedes. Please follow that style.
* Don't change whitespace when not /absolutely/ needed. If you want to
beautify things, make a separate, subsequent patch that changes only the
whitespace. Because in the following it is nearly impossible to see what you
actually changed:
- {VIA_CLE266, "CLE266"},
- {VIA_KM400, "KM400/KN400"},
- {VIA_K8M800, "K8M800/K8N800"},
- {VIA_PM800, "PM800/PM880/CN400"},
- {VIA_VM800, "VM800/P4M800Pro/VN800/CN700"},
- {VIA_CX700, "CX700/VX700"},
- {VIA_K8M890, "K8M890/K8N890"},
- {VIA_P4M890, "P4M890"},
- {VIA_P4M900, "P4M900/VN896/CN896"},
- {VIA_VX800, "VX800/VX820"},
- {VIA_VX855, "VX855/VX875"},
- {VIA_VX900, "VX900"},
- {-1, NULL }
+ {VIA_CLE266, "CLE266"},
+ {VIA_KM400, "KM400 / KM400A / KN400 / P4M800"},
+ {VIA_K8M800, "K8M800 / K8N800"},
+ {VIA_PM800, "PM800 / PN800 / PM880 / CN400"},
+ {VIA_P4M800PRO, "P4M800 Pro / VN800 / CN700"},
+ {VIA_CX700, "CX700 / VX700"},
+ {VIA_P4M890, "P4M890 / CN800"},
+ {VIA_K8M890, "K8M890 / K8N890"},
+ {VIA_P4M900, "P4M900 / VN896 / CN896"},
+ {VIA_VX800, "VX800 / VX820"},
+ {VIA_VX855, "VX855 / VX875"},
+ {VIA_VX900, "VX900"},
+ {-1, NULL }
But better, while making many functional changing, leave the whitespace as much
as possible alone. Later, when things are working, this can be cleaned up and
neatened.
Regards,
Benno
--
You are receiving this mail because:
You are the assignee for the bug.
_______________________________________________
Openchrome-devel mailing list
Openchrome-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/openchrome-devel
