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

Jason Cooper
2012-10-12T16:03:41Z

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:
>
> > 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.

> > 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.