Kevin, Please see comments below.
On Tue, 2008-04-01 at 16:45 -0700, Kevin Hilman wrote: > In addition to Dirk's comments, I might add that patches against the > master branch of the DaVinci git tree are preferred. With the newly created git-tree and such, I think Steven can fix the patches according to comments from both Dirk and you, except for this one. It is an unfortunate fact that Neuros must stick a certain kernel version (2.6.23 ATM) for well-known reasons, I am afraid Steven will not have the opportunity to go further to make sure the patch apply to master branch cleanly. I take your 'might' above as an ok to this. ;-) > Also, I understand this is a forward port of a driver from 2.6.10, so > most of the errors/warnings come from the original TI driver, and not > from your work. > > However, for me to merge this code, at a minimum all the checkpatch > errors/warnings should be fixed, and there are a handful of other > whitespace issues to be cleaned up for readability as well. Yes, they should all be fixed. > That being said, I did compare the original driver from the TI/MV > 2.6.10 kernel to your forward port, and you seem to have made lots of > changes that are not scrictly forward port changes. When doing this, > I prefer to see separate patches for these kinds of changes. In other > words, I'd prefer to see one patch which is just a forward port, > followed by cleanup patches with detailed descriptions. I believe this was the case already with the patches Steven provided, first there was a strict forward porting, then a patch following up to remove the devfs, I'll let Steven speak for himself in the up-comping new patch series. > In addition to this, you've done several changes to the debug prints, > as well as added braces around single statement while/if blocks, which > is not needed, and is frowned upon by upstream maintainers. It was me who brutally removed some of the debug printfs due to compiling error if keeping them, I didn't know how to fix it and I still don't. :-( Steven, can you help bring them back and post the error messages here to ask for help if you can't fix them yourself? > Also, you explicitly add a NULL assignment to static variables, which > is redundant as these end up in the .bss section which is explicitly > zeroed. (specifically: static struct class_simple *rsz_class = NULL;) I thought Steven has fixed this, but Steven can speak better for himself. Thanks, -- Michael Gao Neuros Technology-Open Consumer Electronics http://developer.neurostechnology.com _______________________________________________ Davinci-linux-open-source mailing list [email protected] http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
