<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#c9">Comment # 9</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:bensberg@justemail.net" title="Benno Schulenberg <bensberg@justemail.net>"> Benno Schulenberg</a> <pre>(In reply to Kevin Brace from <a href="show_bug.cgi?id=93243#c4">comment #4</a>) > 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 [...] Then delete it in this patch instead. There is no reason to first comment something out and then later delete it. Delete it rightaway. > > +/* 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. Okay. But the two comment lines above add almost nothing to the four lines below. That's why I said it's unneeded, too verbose. > > +/* 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); > 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.). Yes. Try to trim it down when describing patches and commenting on code. The bare essentials will do. In fact, bare essentials will do *better* than long texts, because those long texts I can't even read, I lack the patience. > 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. You may change upper to lower, but... in a *separate* patch. Case changes are a matter of style, secondary. Value changes are functional changes, primary, they are the meat. If I were the maintainer, I would then accept the patch with the functional change, and would discard the style change, because unneeded. (You may then ask: did I then make that change for nothing? The answer is: yes. Try to resist making style changes. Concentrate on the functional changes.) > 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. . . ." About describing the content of the patch, don't use an implied "I", don't refer to yourself. Do as mister Torvalds says: use the imperative, describe the changes as if giving orders to the code. > If you want me to, I can reword that portion of the patch comment. Yes, all patch descriptions could use rewording, and trimming down. Benno</pre> </div> <hr> You are receiving this mail because: <ul> <li>You are the assignee for the bug.</li> </ul> </body> </html>