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

[bugzilla-daemon at freedesktop.org]

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

--- Comment #9 from Benno Schulenberg <bensberg at justemail.net> ---
(In reply to Kevin Brace from comment #4)
>     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

-- 
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/20151216/9a970589/attachment.html>