Hi Dan, sorry if I seem a bit reluctant here. I just spent 3 days testing the patches. Keep the feedback coming. Hopefully, I'll have a bit more energy in a few days for testing the next version of the patches. On Tue, Aug 23, 2011 at 02:35:55PM +0300, Dan Carpenter wrote: > On Tue, Aug 23, 2011 at 04:41:32PM +0800, Ali Bahar wrote: > > You need to realize that there were 165 files which showed changes! > > Patches 1 and 2 are good. Then there are only 43 files left. And those 43 took only 3 weeks! :-) > Run git citool. Highlight all the copyright notices. Right click > and select "Stage Lines for commit". That brings you down to 35 files. Yes, this will handle the copyright stuff. (Commit 1) > Since we already discussed it, the _netdev_priv() to netdev_priv() is > simple. Make that a single patch. The os_free_netdev() is the same > and so are the _usb_alloc_urb() and _usb_submit_urb() patches. Yes, this, too, will compile w/o a headache. (Commit 2) > Changing #includes can break the build but in this case it's fine to > just merge all the include changes in one commit. I tested it, and > it compiles ok. OK. (Commit 3) > The rest is probably spaghetti changes like you said. There are some > one liners here and there. > > LedControl871x() removes an unneeded NULL check. (The address of a > stack variable is never NULL). Yes, there are a number of such things in the code. Frankly, amidst all the whitespace and trivial/cosmetic changes sucking-up time, I chose not to dwell on them. There are numerous optimizations which can be done here. But, the plan for this driver includes eventual rewrite. > There was an unused macro deleted in osdep_service.h > -#define MSECS(t) (HZ * ((t) / 1000) + (HZ * ((t) % 1000)) / 1000) Realtek had moved it to another file. But, since it was not used, I removed it altogther. > The changes in rtl8712_cmd.c are purely whitespace changes. It looks > like the intent was to fix a bug, but the patch doesn't actually do > anything. The cpu_to_le32 call was moved a few lines down, after the if-else. > The casting changes in r8712_wep_encrypt() are wrong: > - *((u32 *)crc) = cpu_to_le32(getcrc32( > + *((unsigned long *)crc) = cpu_to_le32(getcrc32( > payload, length)); > crc is 32 bits so casting it to long will just cause memory > corruption on 64 bit systems. I'm not sure what was intended. Same > in r8712_wep_decrypt(). Ouch! Good catch! This was one of _Realtek's_ changes, so I checked only the lvalue for pointer-size. Thanks. I'll sift thru the code, again, for this sort of thing. > That still leaves a huge 1000 line patch... But yeah we may not have > a lot of options. That's the thing. The sum total of the commits will not change, nor will the commit logs improve for most of the code; and it's already a lot prettier than it was (in the development branch), and turned out far better than it was likely to have. thanks, ali > regards, > dan carpenter > _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
