linux-kernel - Re: [PATCH] i2c-algo-pca: fix all coding style issues in i2c-algo-pca.c

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: <20100422075937.GN24384@pengutronix.de>
Date:	Thu, 22 Apr 2010 09:59:37 +0200
From:	Wolfram Sang <w.sang@...gutronix.de>
To:	Jean Delvare <khali@...ux-fr.org>
Cc:	Farid Hammane <farid.hammane@...il.com>, ben-linux@...ff.org,
	Enrik.Berkhan@...com, linux-i2c@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] i2c-algo-pca: fix all coding style issues in
	i2c-algo-pca.c

On Thu, Apr 22, 2010 at 09:25:24AM +0200, Jean Delvare wrote:
> On Thu, 22 Apr 2010 01:57:38 +0200, Wolfram Sang wrote:
> > Hi Farid,
> > 
> > thanks for this approach. Have you checked that the binary is the same
> > before/after your patch? If so, please mention in your patch description.
> > 
> > Also, always keep in mind that checkpatch helps to make code readable. Some of
> > your changes should keep readability in mind not just fixing the warnings.
> > 
> > On Wed, Apr 21, 2010 at 09:11:37PM +0200, Farid Hammane wrote:
> > > This patch fixes all coding style issues found by checkpatch.pl.
> > 
> > > 
> > > Signed-off-by: Farid Hammane <farid.hammane@...il.com>
> > > ---
> > >  drivers/i2c/algos/i2c-algo-pca.c |   74 ++++++++++++++++++++++---------------
> > >  1 files changed, 44 insertions(+), 30 deletions(-)
> > > 
> > > diff --git a/drivers/i2c/algos/i2c-algo-pca.c b/drivers/i2c/algos/i2c-algo-pca.c
> > > (...)
> > > @@ -178,12 +178,12 @@ static int pca_rx_ack(struct i2c_algo_pca_data *adap,
> > >  }
> > >  
> > >  static int pca_xfer(struct i2c_adapter *i2c_adap,
> > > -                    struct i2c_msg *msgs,
> > > -                    int num)
> > > +		struct i2c_msg *msgs,
> > > +		int num)
> > 
> > One more tab maybe?
> 
> Better use tab + spaces and align on the opening parenthesis. What
> checkpatch.pl complains about here isn't the alignment, it's the use of
> more than 8 consecutive spaces.
> 

OK

> > > (...)
> > > @@ -241,8 +244,10 @@ static int pca_xfer(struct i2c_adapter *i2c_adap,
> > >  			completed = pca_address(adap, msg);
> > >  			break;
> > >  
> > > -		case 0x18: /* SLA+W has been transmitted; ACK has been received */
> > > -		case 0x28: /* Data byte in I2CDAT has been transmitted; ACK has been received */
> > > +		case 0x18: /* SLA+W has been transmitted;
> > > +			      ACK has been received */
> > > +		case 0x28: /* Data byte in I2CDAT has been transmitted;
> > > +			      ACK has been received */
> > 
> > First, check CodingStyle for how multiline comments should look like. For
> > readability, I'd like to keep them single line, though. I think this could
> > be done by rewording. Same for all following comments.
> 
> Please keep in mind that Farid doesn't know the code. He is "only"
> helping with formatting cleanups. Asking him to reword the comments

I think s/has been//g will help already. And getting accoustomed to the code
you are modifying is not that bad ;)

> doesn't seem wise. And there's nothing wrong with two-line comments as
> above. The "preferred comment format" in CodingStyle is often
> unrealistic IMHO, I'm not always following it in my own code.

I disagree here. I'd rather break the 80 columns limit than having multiline
comments not really standing out.

Also, I feel a bit uneasy as changing the comments spoils the history for
git-blame in case you want to know when/if the state-machine was changed. Then
again, I like properly formatted code.

Summa summarum, just send V2, it's not really worth fighting over it :)

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Download attachment "signature.asc" of type "application/pgp-signature" (198 bytes)