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

[bugzilla-daemon at freedesktop.org]

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

--- Comment #10 from Kevin Brace <kevinbrace at gmx.com> ---
(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

-- 
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/20151217/2e034ba9/attachment.html>