On Wednesday 14 October 2015 08:47:44 David Daney wrote: > On 10/14/2015 07:06 AM, Sakshi Bansal wrote: > > Fixed allignment issues and line over 80 characters > > Use spell checking on 'allignment' > > But that is not the main problem with the patch... > > > You are changing things other than white space and comment formatting, > can you tell us on which platforms the patch was tested to verify that > you didn't break anything? In general a good advice, but for trivial whitespace or comment changes, this is normally not necessary. Compile-testing a patch as you say is normally expected, if only to avoid embarrassing complaints if it does break later. For drivers that are not enabled in the x86 allmodconfig, it sure helps to say something like "Compile-tested using MIPS cross toolchain from https://www.kernel.org/pub/tools/crosstool/";. Even better would be to send a fix to decouple the driver from asm/octeon/*.h to make it build on all architectures, but that is much more work than I'd expect for a trivial patch. > > Signed-off-by: Sakshi Bansal <sakshi.april5@xxxxxxxxx> > > > NAK. Actually all the changes in the patch look reasonable for a staging driver, but the submission form needs to be improved. I'm guessing this was a first-time patch submission (presumably for an outreachy application), which means it's pretty much expected to get a few things wrong and correct them in a follow-up. > > @@ -68,7 +68,7 @@ static void cvm_oct_rgmii_poll(struct net_device *dev) > > struct octeon_ethernet *priv = netdev_priv(dev); > > unsigned long flags = 0; > > cvmx_helper_link_info_t link_info; > > - int use_global_register_lock = (priv->phydev == NULL); > > + int use_global_register_lock = (!priv->phydev); > > Same. > > I could go on, but I think you see the pattern here. > > Your changelog says you are fixing warnings, but none of these are > warning fixes. > > In fact it is perfectly acceptable to compare a pointer to NULL. It is > a common idiom in the kernel. The original author of the code thought > it was more clear this way, and you are causing code churn for no reason. The style she changed it to is the preferred style however. We don't normally send or take patches to change this in regular code, but for drivers/staging/ this is the kind of patch you should expect to see. To improve the patch, I would suggest to split it up into smaller chunks in more logical units. "fixed few coding style warnings" is not a good one-line description for a patch, as it already hints that the patch is doing multiple unrelated changes, and is not unique in the sense that another patch could have the exact same description and do something completely different. Instead, this can be done as a series of three patches: [PATCH 1/3] staging: octeon: fix commenting style [PATCH 2/3] staging: octeon: avoid redundant "== NULL" comparison [PATCH 3/3] staging: octeon: whitespace fixes All three of these are still trivial of course, but this is already made clear by the subject line. In the detailed description, there should be an explanation for each patch why the new version is considered an improvement over the existing code. If you cannot come up with an explanation other than 'checkpatch said this', it's better to drop the patch, but with a good description most arguments about the merits of a change can be avoided. Arnd _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
