Bug 93243

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

Benno, unless a style guidelines' doc says so, I'd much rather have the C89 style comments (although one-liners should probably be done in a one-liner way), since a C89 compiler is all that's needed to build both the Linux kernel and X.org, at least ideally.

When they are real comments, yes, please use only comments of the form '/* */'.  But this isn't an actual comment, it is the disabling of code.  First, don't disable code in a patch; throw it out.  But when you /must/ disable it, use '//' so it is easy to recognize /and/ could be sedded out automatically.

(In reply to Benno Schulenberg from comment #1)

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 (Bug 92861).
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

I'm with you about the comments, both in style (C89 comments, since just a little comfort in the comments is not a good reason to drop standard C89 support) and content (we are lacking a lot of info in the driver).
On the return type style, while I consider it ugly, there are a few things about it that tend to justify keeping it.
First, several system-level projects use that style (sections of the Linux kernel do, for example, as most of X.org AFAIR), so the argument that it is not common isn't really that strong.
Second, consistence matter. A good style is twice as good if it is consistent, but a bad style is twice as bad if it is not. This may be a bad style, but unless we first do a separate (so we don't mask actual functional changes) commit fixing it, we'll make it worse if we use the "right" style here and there, and the "wrong" style everywhere else. Your mind will automate how to interpret the structure of text if it is consistent. This automation will be broken if it isn't.
Third, it allows for longer, more descriptive return types without hurting readability too much. Again, your eyes get used to the fact function signatures are at least two lines long, but it won't get used to the fact that randomly sometimes you need to scroll right and sometimes you don't.
Fourth, the more you highlight the important concepts, the more clear the meaning of the code is, and certainly the return type of a function is important, and certainly having its own line highlights it.
Aesthetically, I hate that style. But it does have some advantages that should be weighted out.
Lastly, I suggest we keep conceptually different patches separate from each other. You can make them depend on each other, but a single huge patch is always a future problem. We should make stylistic changes in one patchset, organized by style issue cleaned or by file touched (not both at the same time), bugfixes one per patch, features on their own patchset, one per patch.
That's what I believe to be common sense.

(In reply to Mario Rugiero from comment #5)

Hi Mario,

If I have to comply with some kind of a coding style (someone should define this somewhere), I could.
The problem is, I had to create 10 patches before someone stopped me and told me to do things differently.
Someone should have stopped me around second or third patch, but no one will answer my e-mails or comment on my patches, so I had to keep going.
I do not have repository access, and if I had, it would have made my life easier (I would have done the commits myself.).
If I were to redo some of the patches, please do repository commits very quickly (i.e., within a few days of the submission or public posting), so that I can update my local hard drive's (Epic 1314 laptop I use for my OpenChrome development) Git repository.
Right now, I have about 13 to 14 patches in the aforementioned computer.
Many of them are small changes, but I do fix some big issues like laptop back light turn on / off and temporary fix for ACPI S3 State resume.
I will like to see the patches incorporated in the Version 0.3.4 release since LVDS-based DFP screen getting completely lost after ACPI S3 State resume is a major bug.

Regards,

Kevin Brace

Hi, Kevin.
I believe there is some misunderstanding.
I'm no authorithy here, but just as you, I'm someone with a bit of background in programming. I mostly gave my opinions. I mentioned before that I don't know of a proper style guide for Openchrome.
Just as you, I lack repository access, and I didn't comment on the patches mainly because I don't think I would have provided any useful insight aside from possible style issues. I kind of assumed you were more of a regular commiter.
Regards,
Mario.

(In reply to Kevin Brace from comment #6)
> If I have to comply with some kind of a coding style (someone should define
> this somewhere), I could.

http://xorg.freedesktop.org/wiki/CodingStyle/
(the third bullet especially)

http://stackoverflow.com/questions/2290016/git-commit-messages-50-72-formatting

https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/SubmittingPatches?id=HEAD#l104
(paragraphs 2) and 3))

Or look at the commit messages from some other project, for example grep:

http://git.savannah.gnu.org/cgit/grep.git

You will see that the first line (the "50" line) is used as a summary line in the overview.  When you click on a commit, this line is even put in bold, and the rest of the message (after the blank line) is show after it, as a self-contained piece of text, not as a continuation of the summary line.

> The problem is, I had to create 10 patches before someone stopped me and
> told me to do things differently.

Well, you posted them very rapidly.  But there is no problem whatsoever: first do a 'git format-patch origin' in your current branch.  Then checkout the master branch (I supposde you have been making your commits in a branch, if not, you will have to do a git reset --hard HEAD, or make a fresh clone from master elsewhere), and create a new branch.  Then apply the 0001* patch, correct the style of the changes, run 'git commit -a', paste in the commit message from the 0001* patch and edit it to be in the summary + blankline + explanation format, and save.  Repeat until all patches have been reapplied and recommitted.  This is standard procedure for developers.  Patches are seldom fully correct the first time around.

(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

(In reply to Benno Schulenberg from comment #9)

> Then delete it in this patch instead.  There is no reason to first comment
> something out and then later delete it.  Delete it rightaway.
> 

The particular one you are complaining about was really not meant to be in there.
I do not mind remedying this later, and in fact, it was eliminated in the later patch I have not posted yet.


> > > +/* 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.
> 

That is simply your opinion.
Personally, I do get confused between sequence registers and CRT controller extended registers sometimes.
So, in this case, I decided to put some comments so that I know which register I was dealing with.
If someone who was looking at the source code for the first time or second time, I would imagine they will appreciate what the code is doing.
It also make it easier (at least for me) when I have to look up the bit definition of the register in the VIA Technologies documents.
I am not changing my style on this one.
The developers who worked on OpenChrome put way too few comments in the code, and in some cases, they are not commented very well to begin with.
At least, I try to explain what I am doing in many cases.


> > > +/* 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.
> 

You are totally wrong on that.
The previous developers put very little comments in the code, so I had to spend tremendous amounts of time trying to figure out what they are doing in the code.
    To be honest, OpenChrome is broken.
I am personally surprised that some of the stuff even work in the first place.
Although this particular discussion is specific to ACPI S3 State resume bug I have been working on, the code itself has several design flaws that it took me many days of struggle to figure out why the DFP screen was getting lost after ACPI S3 State resume.
Because there are very few explanations on what the code is doing, it made my life much harder.
There were several faulty assumptions about what state the hardware is in when it comes out of ACPI S3 State.
For example, when the hardware comes out of ACPI S3 State, most VGA registers are basically undefined.
Someone was assuming in the code that saved VGA registers can be trusted, but unfortunately, this was one of the reason why DFP was getting lost (i.e., LVDS pins were being turned off because of this faulty assumption, hence, one sees nothing on the laptop LCD.).
Because no one was explaining what is going on in the code, it took me several days where the only thing I did besides eating and sleeping was analyzing what is going on with OpenChrome regarding DFP.
    At least, I am trying to explain what I am doing when I am accessing certain extended VGA registers.
Someone needs to explain what they are doing in the code, or every time someone works on fixing something in the code, they have to all go through what I went through.
Most such people will quit fairly quickly trying to fix the code.
I happened to survive this process better than most people do.
Your philosophy on code maintenance completely ignores software reuse and maintainability ideas. 


> > 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

That is ridiculous.
The patch comment section explains what I did in the code.
Most of them, except the first patch I posted, I believe are proper in terms of length and content.
You seems to have your own views on how things should be, but I think that what most open source projects could use is developers spending a little more time explaining what is going on.
That is all I did.

Regards,

Kevin Brace

I opened this one long time ago, and it is no longer relevant.