<html> <head> <base href="https://bugs.freedesktop.org/" /> </head> <body> <div> <a class="bz_bug_link bz_status_NEW " title="NEW - [Patch] VIARestore function improvement" href="https://bugs.freedesktop.org/show_bug.cgi?id=93243#c4">Comment # 4</a> on <a class="bz_bug_link bz_status_NEW " title="NEW - [Patch] VIARestore function improvement" href="https://bugs.freedesktop.org/show_bug.cgi?id=93243">bug 93243</a> from <a class="email" href="mailto:kevinbrace@gmx.com" title="Kevin Brace <kevinbrace@gmx.com>"> Kevin Brace</a> <pre>(In reply to Benno Schulenberg from <a href="show_bug.cgi?id=93243#c1">comment #1</a>) Hi Benno, > 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. > There are no resources that I know of (Although I did not particularly look for it other than followed some guidelines outlined by Xavier when I sent him my work in September.) that instructs me how to write release notes regarding a patch. I used whatever common sense I think I have when I prepared the comment section of the patch. I could improve this in the future if needed. I will assume the 50 character thing has something to do with when the patch file is generated, the patch file name gets truncated. To be honest, it is hard to summarize the change to less than 50 characters, although I can try this in the future. > * 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 don't get it. I have done C / C++ language coding for years (although I do not really specialized in software), and I do not know any rational reason why the function declaration has to be done the way OpenChrome has done it. I do not know any textbook on C language that does this in the first place. What rational reason is there for doing it like the way you want me to do? Other Linux source code I have seen does the way I am doing. > * 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. > I was told not to use C++ style comment ("//") by Xavier if I remember it correctly (September e-mail reply). I rather use "//," of course. The particular line you are referring to will be removed in the one the newer patch that will temporarily fix LVDS-based DFP screen getting lost when the computer resumes from ACPI S3 State. I have not posted this patch yet, but I may do so soon. I have not posted the patch yet due to secondary display HI (Hardware Icon) getting lost when the computer resumes from ACPI S3 State. HI is used for screen mouse cursor. Despite my tries, I still have not figured out why secondary display (i.e., LVDS-based DFP) HI disappears. There is a temporary workaround for this issue (i.e., somehow turning on IGA1 simultaneously with IGA2 solves the issue), but I am still holding out for a complete fix. > +/* Sequencer Extended Registers */ > +/* Extended Register Unlock (0x3c5.0x10[0]) */ > > Too verbose. Unneeded. > The code itself contains too few comments to begin with where it is very difficult for a non-expert to even start tinkering with the code. It is often very unclear what the developer's intention is, so I decided to put more comments explaining what I am accessing certain registers for what reason. It took me tremendous personal effort (i.e., spending several days straight on trying to figure out what OpenChrome is doing) to figure out why LVDS-based DFP was getting lost when it resumes from ACPI S3 State. I did figure most reasons behind the issue eventually (Hardware Icon issue for secondary display is still unresolved.), but it certainly would have been easier if past developers put more effort in documenting what the code is doing. If the developer does not say what the register does, it takes a lot of researching the hardware programming documentation to understand what is going on. Frankly, OpenChrome has become a Frankenstein code project, and I was trying to make it more maintainable for other people in the future. While UMS code base is slated for eventual discontinuation, that is all that sort of works at this point (I am not trying to insult anyone. 2D rendering is perfect, but display control section is very low reliability at this point.), so I put a lot of effort in trying to clean up the code. > +/* 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. > That is just a personal taste issue. As a person, I tend to be very verbose (i.e., I tend to write long e-mails as other people may have already noticed.). If someone else wanted to know what is going on, those comments are necessary, or they will be confused as to what is going on. For someone who is seeing the code for the first time or even second time, it is very difficult to discern what is going on. Regarding the specifics of the comment I left, I am trying to distinguish between IBM VGA registers and VIA Technologies extended VGA registers. That is why it ended up being somewhat verbose. > * 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. > Just to let you know, C language generally assumes the use of lower case as default. This is why I converted upper case numbers to lower case. I suppose if there is one good reason to stick to upper characters, it is when copying register number from manufacturer hardware programming guide. If one converts it manually to lower case, one can screw up. I am very careful on the upper case to lower case conversion. > * 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. > I can follow this guideline in the future. That commenting style is what I usually do for my personal projects. > * 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 I believe this was one of the first patch I posted (<a class="bz_bug_link bz_status_NEW " title="NEW - [Patch] VM800 / VN800 / P4M800 Pro Correction" href="show_bug.cgi?id=92861">Bug 92861</a>). You are probably correct that I should have split the VM800 removal and replacement, and Xorg.0.log device support identification message improvement into 2 separate patches. The language related to Xorg.0.log device support identification white space insertion was in this portion of the patch comment. ". . . Made an improvement in supported device message recorded by Xorg.0.log. Other than that, there is no change in terms of the device driver functionality. . . ." Probably the message is too cryptic. It could have been written better. If you want me to, I can reword that portion of the patch comment. Regards, Kevin Brace</pre> </div> <hr> You are receiving this mail because: <ul> <li>You are the assignee for the bug.</li> </ul> </body> </html>