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

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=93243

--- 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:
-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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/openchrome-devel/attachments/20151211/d8bd4514/attachment.html>