Thomas Winischhofer wrote:
Randy.Dunlap wrote:
OK, allright then. I'll do it. Anything else, before I go through it
once again?
OK, I'll look over it (1.0.2). Some of these comments could
be repeats of others, I'm not checking the previous reviews.
2. The MAINTAINERS entry should include
L: linux-usb-devel@lists.sourceforge.net
Just because I wrote this driver doesn't mean I am going to read this
list. So I won't see any bug reports sent there. What's the point? There
is a website (with a forum for all my SiS-related drivers), and an email
address.
Look, I don't intend to hand this driver over to the kernel and thereby
neglecting my maintainance duties. If there is a problem with this
driver, I will fix it (unless somebody else jumps in first). That's why
I added me to the MAINTAINERS in the first place.
I see.
3. Looks to me like struct sisusb_packet could use some padding
after the unsigned short header, if you want to make it __packed__.
Why is it __packed__? Is this a data struct that the device
hardware/firmware specifies? If not, it shouldn't be packed.
At any rate, it would be better if address & data were u32-aligned.
Come on. You can rest assured I didn't make it __packed__ just for fun.
The firmware expects it in this format.
That's good. It still seems inefficient on the host side, but I
guess there's nothing to be done about that.
19. Do you know enough about the hardware interface to get rid of
some of these magic numbers? If so, please do. E.g. (but occurs
in many places; wow, MANY places):
Can't, won't. That touches the graphics core, that is entirely my
department. I have no docs, I have no "official" bit-names. Dealing with
SiS chips for almost 5 years made me remember bit numbers and register
indices much better than some self-invented, long defined pseudonames.
And comparing this with my sources of information (read: disassembler
output) is much easier than having to look up a defined alias at every
line.
I see. You are brave. :)
The quantity of magic numbers and magic code is amazing....
Where did they come from?
You know. Did I mention that I have no docs...?
>
Allright, version 1.0.3 of the patch attached, officially named the
"how-to-lose-an-enthusiastic-driver-programmer-in-10-days"-edition.
Well, that certainly wasn't the intent.
I hate the code as it looks now. Especially the missing "{"/"}" drive me
crazy when it comes to consecutive if-iterations. Ugly beyond (my)
belief, as error prone as it can be. But if you like it, be my guest ;)
Thomas, looking forward to actually PROGRAM again after 3 days of
looking for extraneous whitespaces and the like. Good night, Seattle. We
love you :)
Thanks for the changes that you _did_ make.
--
~Randy
-------------------------------------------------------
This SF.Net email is sponsored by: IntelliVIEW -- Interactive Reporting
Tool for open source databases. Create drag-&-drop reports. Save time
by over 75%! Publish reports on the web. Export to DOC, XLS, RTF, etc.
Download a FREE copy at http://www.intelliview.com/go/osdn_nl
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
