Re: [PATCH v2 1/4] net: mvneta: driver for Marvell Armada 370/XP network unit

On Fri, Oct 12, 2012 at 04:59:19PM +0200, Thomas Petazzoni wrote:

Jason,

On Fri, 12 Oct 2012 10:31:31 -0400, Jason Cooper wrote:

quoted

I agree with Francois on most of these.  I prefer readability over
hard 80 column limits.

Sure, but checkpatch.pl is warning on every line exceeding the 80
columns. Not that I think that all checkpatch.pl warnings should
necessarily be religiously respected, but if you have gazillions of
warnings regarding line exceeding 80 columns, it is very likely that
you will miss more important warnings.

./scripts/checkpatch.pl --ignore LONG_LINE ...

Will yield the 'more important' warnings/errors.  After those are
cleared, you can run without --ignore to check for over-indentation,
etc.

quoted

Although, 80 columns is still sound
guidance.  For example, a majority of the broken lines are due to
long macro and constant names.  I did a 'git grep NETA' and didn't
see anything alarming.  So, above could become

			val |= rx_filled << NETA_RXQ_ADD_NONOCC_SHIFT;

I don't mind, but then I would like to keep things consistent:

 * The driver file would be neta.c

 * All functions and data structure would be prefixed neta_ and not
   mvneta_

 * The Kconfig option would become CONFIG_NETA. Do we really want such
   a "simple" Kconfig option name for a driver?

Well, you could do mv_neta.c and CONFIG_MV_NETA, but at the end of the
day, we were both trying to put lipstick on a pig.  Your last paragraph
is the most important.

Maybe the fact that those long macros are making long lines is also due
to the code having sometimes a too deep indentation, and I need to fix
that by using more auxiliary functions or something like that?

This is the intent of the 80 column warning.  Please review the patch
for over-indentation, and consider shortening the macros, eg
MVNETA_RXQ_ADD_NONOCC_SHFT.

thx,

Jason.