linux-kernel - Re: [PATCH 6/6] staging: comedi: ni_at

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20160825154708.a55hsffnfdz2zqac@acer>
Date:   Thu, 25 Aug 2016 18:47:08 +0300
From:   Andrey Utkin <andrey_utkin@...tmail.com>
To:     Anson Jacob <ansonjacob.aj@...il.com>
Cc:     gregkh@...uxfoundation.org, abbotti@....co.uk,
        devel@...verdev.osuosl.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 6/6] staging: comedi: ni_at_a2150.c: Fix checkpatch
 warning

On Thu, Aug 25, 2016 at 11:16:13AM -0400, Anson Jacob wrote:
> Fix checkpatch.pl warning 'line over 80 characters'
> 
> Signed-off-by: Anson Jacob <ansonjacob.aj@...il.com>
> ---
>  drivers/staging/comedi/drivers/ni_at_a2150.c | 82 ++++++++++++++++------------
>  1 file changed, 46 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/staging/comedi/drivers/ni_at_a2150.c b/drivers/staging/comedi/drivers/ni_at_a2150.c
> index 957fb9f..3c00de1 100644
> --- a/drivers/staging/comedi/drivers/ni_at_a2150.c
> +++ b/drivers/staging/comedi/drivers/ni_at_a2150.c
> @@ -58,41 +58,49 @@
>  
>  /* Registers and bits */
>  #define CONFIG_REG		0x0
> -#define   CHANNEL_BITS(x)		((x) & 0x7)
> -#define   CHANNEL_MASK		0x7
> -#define   CLOCK_SELECT_BITS(x)		(((x) & 0x3) << 3)

You have lost this treeish look which carried information about thing
being a register or a field.

> +#define CHANNEL_BITS(x)		((x) & 0x7)
> +#define CHANNEL_MASK		0x7

No uniform alignment. Please get everything in a row.
If it's hard or this part of driver is expected to have a lot of changes
in near future, then I'd remove any whitespace over single space between
name and value.

> +#define ENABLE0_BIT		0x80	/* enable (don't internally ground)
> +					 * channels 0 and 1
> +					 */
> +#define ENABLE1_BIT		0x100	/* enable (don't internally ground)
> +					 * channels 2 and 3
> +					 */

This commenting style is discouraged (Linus has even stated explicitly
that he dislikes it, even for pieces of code which historically had this
style everywhere). Opening "/*" should not be followed by text.

Also, In my personal experience, it is more stable to have these
comments on previous line, not at end of the line. This way you always
have enough space for both a comment and a macro.