| 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
| ||
|
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
Powered by blists - more mailing lists