linux-kernel - Re: [PATCH] staging: kpc2000: whitespace and line length cleanup

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: <5f8d2f42d770a11e1331605fb3f22df44c74574f.camel@perches.com>
Date:   Mon, 15 Jul 2019 15:21:18 -0700
From:   Joe Perches <joe@...ches.com>
To:     john.hubbard@...il.com, Dan Carpenter <dan.carpenter@...cle.com>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        John Hubbard <jhubbard@...dia.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Simon Sandström <simon@...anor.nu>,
        Geordan Neukum <gneukum1@...il.com>,
        Jeremy Sowden <jeremy@...zel.net>,
        Vandana BN <bnvandana@...il.com>, devel@...verdev.osuosl.org,
        Bharath Vedartham <linux.bhar@...il.com>
Subject: Re: [PATCH] staging: kpc2000: whitespace and line length cleanup

On Mon, 2019-07-15 at 14:21 -0700, john.hubbard@...il.com wrote:
> From: John Hubbard <jhubbard@...dia.com>
> 
> This commit was created by running indent(1):
>     `indent -linux`
> 
> ...and then applying some manual corrections and
> cleanup afterward, to keep it sane. No functional changes
> were made.

I don't find many of these whitespace changes "better".

Sometimes, it's just fine to have > 80 char lines.

Alignment formatting was OK before this and now has
many odd uses that make reading for a human harder
rather than simpler or easier.

> diff --git a/drivers/staging/kpc2000/kpc2000_i2c.c b/drivers/staging/kpc2000/kpc2000_i2c.c
[]
> @@ -33,9 +33,9 @@ MODULE_LICENSE("GPL");
>  MODULE_AUTHOR("Matt.Sickler@...tronics.com");
>  
>  struct i2c_device {
> -	unsigned long           smba;
> -	struct i2c_adapter      adapter;
> -	unsigned int            features;
> +	unsigned long		smba;
> +	struct i2c_adapter	adapter;
> +	unsigned int		features;

Here the spaces before the identifier are converted to tab aligned 

>  };
>  
>  /*****************************
> @@ -52,9 +52,9 @@ struct i2c_device {
>  #define SMBHSTDAT0(p)   ((5  * REG_SIZE) + (p)->smba)
>  #define SMBHSTDAT1(p)   ((6  * REG_SIZE) + (p)->smba)
>  #define SMBBLKDAT(p)    ((7  * REG_SIZE) + (p)->smba)
> -#define SMBPEC(p)       ((8  * REG_SIZE) + (p)->smba)   /* ICH3 and later */
> -#define SMBAUXSTS(p)    ((12 * REG_SIZE) + (p)->smba)   /* ICH4 and later */
> -#define SMBAUXCTL(p)    ((13 * REG_SIZE) + (p)->smba)   /* ICH4 and later */
> +#define SMBPEC(p)       ((8  * REG_SIZE) + (p)->smba)	/* ICH3 and later */
> +#define SMBAUXSTS(p)    ((12 * REG_SIZE) + (p)->smba)	/* ICH4 and later */
> +#define SMBAUXCTL(p)    ((13 * REG_SIZE) + (p)->smba)	/* ICH4 and later */

But here the #define value still has spaces but the comment uses tabs.
Why tab align the comments but not the #define value?

> @@ -136,17 +138,21 @@ static int i801_check_pre(struct i2c_device *priv)
[]
>  	status &= STATUS_FLAGS;
>  	if (status) {
> -		//dev_dbg(&priv->adapter.dev, "Clearing status flags (%02x)\n", status);
> +		//dev_dbg(&priv->adapter.dev,
> +		//"Clearing status flags (%02x)\n", status);

This was better before.

An improvement might be to add more macros like:

#define i2c_err(priv, fmt, ...)
	dev_err(&(priv)->adapter.dev, fmt, ##__VA_ARGS__)
#define i2c_dbg(priv, fmt, ...)
	dev_dbg(&(priv)->adapter.dev, fmt, ##__VA_ARGS__)

So all these uses of dev_<level>(&priv->adapter.dev, ...)
become much shorter visually and the line wrapping becomes
rather better.

>  		outb_p(status, SMBHSTSTS(priv));
>  		status = inb_p(SMBHSTSTS(priv)) & STATUS_FLAGS;
>  		if (status) {
> -			dev_err(&priv->adapter.dev, "Failed clearing status flags (%02x)\n", status);
> +			dev_err(&priv->adapter.dev,
> +				"Failed clearing status flags (%02x)\n",
> +				status);

e.g.:
			i2c_err(priv, "Failed clearing status flags (%02x)\n",
				status);

etc...


[]

> @@ -301,7 +322,8 @@ static int i801_block_transaction_byte_by_byte(struct i2c_device *priv, union i2
>  			else
>  				smbcmd = I801_BLOCK_LAST;
>  		} else {
> -			if (command == I2C_SMBUS_I2C_BLOCK_DATA && read_write == I2C_SMBUS_READ)
> +			if (command == I2C_SMBUS_I2C_BLOCK_DATA
> +			    && read_write == I2C_SMBUS_READ)

logic continuations should be at EOL.

			if (command == I2C_SMBUS_I2C_BLOCK_DATA &&
			    read_write == I2C_SMBUS_READ)

[]
> @@ -558,13 +614,14 @@ static u32 i801_func(struct i2c_adapter *adapter)
>  		I2C_FUNC_SMBUS_WORD_DATA         | /* _READ_WORD_DATA  _WRITE_WORD_DATA */
>  		I2C_FUNC_SMBUS_BLOCK_DATA        | /* _READ_BLOCK_DATA  _WRITE_BLOCK_DATA */
>  		!I2C_FUNC_SMBUS_I2C_BLOCK        | /* _READ_I2C_BLOCK  _WRITE_I2C_BLOCK */
> -		!I2C_FUNC_SMBUS_EMUL;              /* _QUICK  _BYTE  _BYTE_DATA  _WORD_DATA  _PROC_CALL  _WRITE_BLOCK_DATA  _I2C_BLOCK _PEC */
> +		/* _QUICK  _BYTE  _BYTE_DATA  _WORD_DATA  _PROC_CALL  _WRITE_BLOCK_DATA  _I2C_BLOCK _PEC : */
> +		!I2C_FUNC_SMBUS_EMUL;
>  	return f;
>  }
>  
>  static const struct i2c_algorithm smbus_algorithm = {
> -	.smbus_xfer     = i801_access,
> -	.functionality  = i801_func,
> +	.smbus_xfer = i801_access,
> +	.functionality = i801_func,

Many people prefer the aligned function names.

etc...