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.
1. someone else mentioned this, I believe, but for a driver
with just a .c file, a .h file, and a Kconfig entry, having a
separate subdirectory is overkill. Just put the Kconfig
and Makefile changes into usb/misc/Kconfig and Makefile.
2. The MAINTAINERS entry should include
L: linux-usb-devel@lists.sourceforge.net
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.
4. + SETIREGAND(SISGR,0x05,0xbf);
Use spaces after commas (multiple places).
5. +#ifndef SISUSB_ID
+#define SISUSB_ID 0x53495355
Why conditional?
6. +#define CLEARPACKET(packet) memset(packet, 0, 10);
Don't end with semi-colon. Add them where it's used.
7. +#define SISUSB_CORRECT_ENDIANNESS_PACKET(p) \
+ p->header = (p->header << 8) | (p->header >> 8);
Isn't that
p->header = cpu_to_le16(p->header);
if not, please explain.
if so, please use kernel-supplied functions for this.
8. +#define SISUSB_CORRECT_ENDIANNESS_U32(v) \
+ v = (v >> 24) | ((v >> 8) & 0xff00) | ((v << 8) & 0xff0000) | (v << 24);
Almost same as above:
use v = cpu_to_le32(v);
if that's what you want/need there....
9. A few lines are longer than the normal 80-column limit.
10. + sisusb->mmiosize = 128 * 1024;
Use a #define for that value, please.
11. down() and up() on semaphores don't need the extra parens.
12. in sisusb_bulkout_msg(), there's no need to clear transfer_flags
and then OR them, just set them one time.
13. same as #12 for sisusb_bulkin_msg()
14. + * If async is nz, URBs will be sent without waiting for completion
Change "nz" to something that other people will understand.
(Yes, I know.)
15. sisusb_recv_bulk_msg(): change part of comment to:
One of the pointers MUST be set.
16. (sisusb_send_packet(), for example:) what is the point of this:
ret |= sisusb_send_bulk_msg(...);
when just using ret =
is sufficient?
(similar in sisusb_send_bridge_packet() and several other functions)
17. check for trailing spaces and tabs, i saw a few.
18. check for using spaces instead of tabs, i saw at least one case of
8 spaces used.
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):
+ ret |= GETIREG(SISSR, 0x16, &tmp8);
+ if (ramtype <= 1) {
+ tmp8 &= 0x3f;
+ ret |= SETIREG(SISSR, 0x16, tmp8);
+ tmp8 |= 0x80;
+ ret |= SETIREG(SISSR, 0x16, tmp8);
20. If sisusb_set_default_mode() can be called after init (not
likely ?), then the const arrays inside it could be made static
so that they aren't initialized at run-time. OTOH, that would
consume some fixed data area, but as is it consumes some
code size always.
21. Same comment as #20 for sisusb_init_gfxcore().
22. + case 2: ramtypetext1 = "asymeric";
is that "asymmetric" ??
The quantity of magic numbers and magic code is amazing....
Where did they come from?
--
~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
