[Openchrome-devel] [Bug 93243] [Patch] VIARestore function improvement

bugzilla-daemon at freedesktop.org bugzilla-daemon at freedesktop.org
Fri Dec 11 12:40:03 PST 2015


--- Comment #1 from Benno Schulenberg <bensberg at 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:
-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

* 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



You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/openchrome-devel/attachments/20151211/d8bd4514/attachment.html>

More information about the Openchrome-devel mailing list