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

From: Joe Perches
Date: Mon Jul 15 2019 - 18:30:22 EST


On Mon, 2019-07-15 at 14:21 -0700, john.hubbard@xxxxxxxxx wrote:
> From: John Hubbard <jhubbard@xxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxx");
>
> 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...