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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <3d92b1b1-d081-05ce-e3c0-d53b402ac1dd@nvidia.com>
Date:   Mon, 15 Jul 2019 15:26:22 -0700
From:   John Hubbard <jhubbard@...dia.com>
To:     Joe Perches <joe@...ches.com>, <john.hubbard@...il.com>,
        Dan Carpenter <dan.carpenter@...cle.com>
CC:     LKML <linux-kernel@...r.kernel.org>,
        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 7/15/19 3:21 PM, Joe Perches wrote:
> 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.

Definitely agree! :)

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

OK, I'll accept that. I attempted to pick something that fit
on the screen [much!] better, without making it less readable, but if
people feel that it is a net "worse", then let's just drop
the patch, no problem.

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

Sure. Opinions differ on that, I was guessing that the `indent -linux` option
was the majority, but that could be wrong.


thanks,
-- 
John Hubbard
NVIDIA