On Mon, Feb 10, 2014 at 10:54:00PM -0800, Surendra Patil wrote: > Fixed MACRO and Extern Coding style warnings and errors - listed only few > bpctl_mod.c:120: WARNING: externs should be avoided in .c files > bpctl_mod.c:121: WARNING: externs should be avoided in .c files > bpctl_mod.c:122: WARNING: externs should be avoided in .c files > bpctl_mod.c:124: WARNING: externs should be avoided in .c files > bpctl_mod.c:125: WARNING: externs should be avoided in .c files > bp_ioctl.h:54: ERROR: Macros with complex values should be enclosed in parenthesis > bp_ioctl.h:159: ERROR: Macros with complex values should be enclosed in parenthesis > This isn't the right way to do things... Summary: 1) This patch tries to do too many things at one time. 2) The changelog is not right. 3) It breaks the build and then fixes it in a later patch. The rule on patches is that it should do only one thing at a time. But deciding what "one thing" means is complicated. You have said that one thing is "Fix coding style". That works if the changes are very minor, but in this case they are complicated. You have only listed a few of the things you have done because there are so many. That means it is complicated. The change log is not right. The explanation of how you fixed the "externs should be avoided in .c files" needs to be explained like this: Checkpatch warns about: WARNING: externs should be avoided in .c files because we have function declarations in a .c file. These functions are not used anywhere else so I have made them static. Changelogs should tell a story. Finally, the patch breaks the build and then fixes it in [patch 2/2]. That's not ok. We need to be able to bisect the patch so it should be included with the changes to the .c file. So break it up like this: [patch 1/2] Staging: silicom: make some functions static [patch 2/2] Staging: silicom: minor checkpatch.pl fixes regards, dan carpenter _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
