Re: [PATCH] staging: octeon: fixed few coding style warnings

[Arnd Bergmann]

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/