Michael Gao <[EMAIL PROTECTED]> writes: > 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. ;-)
Understood, any patches are helpful and will make it easier for anyone else to finish the work. >> 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. IIRC, I was looking at the patches from your git tree, not the patches sent to the mailing list. I assumed they were the same. If some of these issues were fixed in the mailed patches, I apologize for the confusion. Kevin _______________________________________________ Davinci-linux-open-source mailing list [email protected] http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
