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

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

[Farid Hammane]

This patch fixes all coding style issues found by checkpatch.pl.

Signed-off-by: Farid Hammane <farid.hammane-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 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
index dcdaf8e..4cb9ece 100644
--- a/drivers/i2c/algos/i2c-algo-pca.c
+++ b/drivers/i2c/algos/i2c-algo-pca.c
@@ -37,15 +37,15 @@
 
 static int i2c_debug;
 
-#define pca_outw(adap, reg, val) adap->write_byte(adap->data, reg, val)
-#define pca_inw(adap, reg) adap->read_byte(adap->data, reg)
+#define pca_outw(adap, reg, val) (adap->write_byte(adap->data, reg, val))
+#define pca_inw(adap, reg) (adap->read_byte(adap->data, reg))
 
 #define pca_status(adap) pca_inw(adap, I2C_PCA_STA)
-#define pca_clock(adap) adap->i2c_clock
+#define pca_clock(adap) (adap->i2c_clock)
 #define pca_set_con(adap, val) pca_outw(adap, I2C_PCA_CON, val)
 #define pca_get_con(adap) pca_inw(adap, I2C_PCA_CON)
-#define pca_wait(adap) adap->wait_for_completion(adap->data)
-#define pca_reset(adap) adap->reset_chip(adap->data)
+#define pca_wait(adap) (adap->wait_for_completion(adap->data))
+#define pca_reset(adap) (adap->reset_chip(adap->data))
 
 static void pca9665_reset(void *pd)
 {
@@ -114,8 +114,8 @@ static int pca_address(struct i2c_algo_pca_data *adap,
 	int sta = pca_get_con(adap);
 	int addr;
 
-	addr = ( (0x7f & msg->addr) << 1 );
-	if (msg->flags & I2C_M_RD )
+	addr = ((0x7f & msg->addr) << 1);
+	if (msg->flags & I2C_M_RD)
 		addr |= 1;
 	DEB2("=== SLAVE ADDRESS %#04x+%c=%#04x\n",
 	     msg->addr, msg->flags & I2C_M_RD ? 'R' : 'W', addr);
@@ -170,7 +170,7 @@ static int pca_rx_ack(struct i2c_algo_pca_data *adap,
 
 	sta &= ~(I2C_PCA_CON_STO|I2C_PCA_CON_STA|I2C_PCA_CON_SI|I2C_PCA_CON_AA);
 
-	if ( ack )
+	if (ack)
 		sta |= I2C_PCA_CON_AA;
 
 	pca_set_con(adap, sta);
@@ -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)
 {
-        struct i2c_algo_pca_data *adap = i2c_adap->algo_data;
-        struct i2c_msg *msg = NULL;
-        int curmsg;
+	struct i2c_algo_pca_data *adap = i2c_adap->algo_data;
+	struct i2c_msg *msg = NULL;
+	int curmsg;
 	int numbytes = 0;
 	int state;
 	int ret;
@@ -202,22 +202,25 @@ static int pca_xfer(struct i2c_adapter *i2c_adap,
 
 	DEB1("{{{ XFER %d messages\n", num);
 
-	if (i2c_debug>=2) {
+	if (i2c_debug >= 2) {
 		for (curmsg = 0; curmsg < num; curmsg++) {
 			int addr, i;
 			msg = &msgs[curmsg];
 
 			addr = (0x7f & msg->addr) ;
 
-			if (msg->flags & I2C_M_RD )
+			if (msg->flags & I2C_M_RD)
 				printk(KERN_INFO "    [%02d] RD %d bytes from %#02x [%#02x, ...]\n",
 				       curmsg, msg->len, addr, (addr<<1) | 1);
 			else {
 				printk(KERN_INFO "    [%02d] WR %d bytes to %#02x [%#02x%s",
 				       curmsg, msg->len, addr, addr<<1,
 				       msg->len == 0 ? "" : ", ");
-				for(i=0; i < msg->len; i++)
-					printk("%#04x%s", msg->buf[i], i == msg->len - 1 ? "" : ", ");
+				for (i = 0; i < msg->len; i++)
+					printk("%#04x%s",
+							msg->buf[i],
+							i == msg->len - 1 ?
+							"" : ", ");
 				printk("]\n");
 			}
 		}
@@ -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 */
 			if (numbytes < msg->len) {
 				completed = pca_tx_byte(adap,
 							msg->buf[numbytes]);
@@ -256,16 +261,19 @@ static int pca_xfer(struct i2c_adapter *i2c_adap,
 				completed = pca_repeated_start(adap);
 			break;
 
-		case 0x20: /* SLA+W has been transmitted; NOT ACK has been received */
+		case 0x20: /* SLA+W has been transmitted;
+			      NOT ACK has been received */
 			DEB2("NOT ACK received after SLA+W\n");
 			pca_stop(adap);
 			goto out;
 
-		case 0x40: /* SLA+R has been transmitted; ACK has been received */
+		case 0x40: /* SLA+R has been transmitted;
+			      ACK has been received */
 			completed = pca_rx_ack(adap, msg->len > 1);
 			break;
 
-		case 0x50: /* Data bytes has been received; ACK has been returned */
+		case 0x50: /* Data bytes has been received;
+			      ACK has been returned */
 			if (numbytes < msg->len) {
 				pca_rx_byte(adap, &msg->buf[numbytes], 1);
 				numbytes++;
@@ -280,17 +288,20 @@ static int pca_xfer(struct i2c_adapter *i2c_adap,
 				completed = pca_repeated_start(adap);
 			break;
 
-		case 0x48: /* SLA+R has been transmitted; NOT ACK has been received */
+		case 0x48: /* SLA+R has been transmitted;
+			      NOT ACK has been received */
 			DEB2("NOT ACK received after SLA+R\n");
 			pca_stop(adap);
 			goto out;
 
-		case 0x30: /* Data byte in I2CDAT has been transmitted; NOT ACK has been received */
+		case 0x30: /* Data byte in I2CDAT has been transmitted;
+			      NOT ACK has been received */
 			DEB2("NOT ACK received after data byte\n");
 			pca_stop(adap);
 			goto out;
 
-		case 0x38: /* Arbitration lost during SLA+W, SLA+R or data bytes */
+		case 0x38: /* Arbitration lost during SLA+W, SLA+R or
+			      data bytes */
 			DEB2("Arbitration lost\n");
 			/*
 			 * The PCA9564 data sheet (2006-09-01) says "A
@@ -304,8 +315,9 @@ static int pca_xfer(struct i2c_adapter *i2c_adap,
 			pca_start(adap);
 			goto out;
 
-		case 0x58: /* Data byte has been received; NOT ACK has been returned */
-			if ( numbytes == msg->len - 1 ) {
+		case 0x58: /* Data byte has been received;
+			      NOT ACK has been returned */
+			if (numbytes == msg->len - 1) {
 				pca_rx_byte(adap, &msg->buf[numbytes], 0);
 				curmsg++; numbytes = 0;
 				if (curmsg == num)
@@ -328,12 +340,14 @@ static int pca_xfer(struct i2c_adapter *i2c_adap,
 			DEB2("BUS ERROR - SCL Stuck low\n");
 			pca_reset(adap);
 			goto out;
-		case 0x00: /* Bus error during master or slave mode due to illegal START or STOP condition */
+		case 0x00: /* Bus error during master or slave mode
+			      due to illegal START or STOP condition */
 			DEB2("BUS ERROR - Illegal START or STOP\n");
 			pca_reset(adap);
 			goto out;
 		default:
-			dev_err(&i2c_adap->dev, "unhandled SIO state 0x%02x\n", state);
+			dev_err(&i2c_adap->dev, "unhandled SIO state 0x%02x\n",
+					state);
 			break;
 		}
 
@@ -352,7 +366,7 @@ static int pca_xfer(struct i2c_adapter *i2c_adap,
 
 static u32 pca_func(struct i2c_adapter *adap)
 {
-        return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
+	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
 }
 
 static const struct i2c_algorithm pca_algo = {
-- 
1.7.0

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 8752 bytes --]

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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  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
> index dcdaf8e..4cb9ece 100644
> --- a/drivers/i2c/algos/i2c-algo-pca.c
> +++ b/drivers/i2c/algos/i2c-algo-pca.c
> @@ -37,15 +37,15 @@
>  
>  static int i2c_debug;
>  
> -#define pca_outw(adap, reg, val) adap->write_byte(adap->data, reg, val)
> -#define pca_inw(adap, reg) adap->read_byte(adap->data, reg)
> +#define pca_outw(adap, reg, val) (adap->write_byte(adap->data, reg, val))
> +#define pca_inw(adap, reg) (adap->read_byte(adap->data, reg))
>  
>  #define pca_status(adap) pca_inw(adap, I2C_PCA_STA)
> -#define pca_clock(adap) adap->i2c_clock
> +#define pca_clock(adap) (adap->i2c_clock)
>  #define pca_set_con(adap, val) pca_outw(adap, I2C_PCA_CON, val)
>  #define pca_get_con(adap) pca_inw(adap, I2C_PCA_CON)
> -#define pca_wait(adap) adap->wait_for_completion(adap->data)
> -#define pca_reset(adap) adap->reset_chip(adap->data)
> +#define pca_wait(adap) (adap->wait_for_completion(adap->data))
> +#define pca_reset(adap) (adap->reset_chip(adap->data))

OK

>  
>  static void pca9665_reset(void *pd)
>  {
> @@ -114,8 +114,8 @@ static int pca_address(struct i2c_algo_pca_data *adap,
>  	int sta = pca_get_con(adap);
>  	int addr;
>  
> -	addr = ( (0x7f & msg->addr) << 1 );
> -	if (msg->flags & I2C_M_RD )
> +	addr = ((0x7f & msg->addr) << 1);
> +	if (msg->flags & I2C_M_RD)
> 

OK

> 		addr |= 1;
>  	DEB2("=== SLAVE ADDRESS %#04x+%c=%#04x\n",
>  	     msg->addr, msg->flags & I2C_M_RD ? 'R' : 'W', addr);
> @@ -170,7 +170,7 @@ static int pca_rx_ack(struct i2c_algo_pca_data *adap,
>  
>  	sta &= ~(I2C_PCA_CON_STO|I2C_PCA_CON_STA|I2C_PCA_CON_SI|I2C_PCA_CON_AA);
>  
> -	if ( ack )
> +	if (ack)

OK

>  		sta |= I2C_PCA_CON_AA;
>  
>  	pca_set_con(adap, sta);
> @@ -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?

>  {
> -        struct i2c_algo_pca_data *adap = i2c_adap->algo_data;
> -        struct i2c_msg *msg = NULL;
> -        int curmsg;
> +	struct i2c_algo_pca_data *adap = i2c_adap->algo_data;
> +	struct i2c_msg *msg = NULL;
> +	int curmsg;

OK

>  	int numbytes = 0;
>  	int state;
>  	int ret;
> @@ -202,22 +202,25 @@ static int pca_xfer(struct i2c_adapter *i2c_adap,
>  
>  	DEB1("{{{ XFER %d messages\n", num);
>  
> -	if (i2c_debug>=2) {
> +	if (i2c_debug >= 2) {

OK

>  		for (curmsg = 0; curmsg < num; curmsg++) {
>  			int addr, i;
>  			msg = &msgs[curmsg];
>  
>  			addr = (0x7f & msg->addr) ;
>  
> -			if (msg->flags & I2C_M_RD )
> +			if (msg->flags & I2C_M_RD)

OK

>  				printk(KERN_INFO "    [%02d] RD %d bytes from %#02x [%#02x, ...]\n",
>  				       curmsg, msg->len, addr, (addr<<1) | 1);
>  			else {
>  				printk(KERN_INFO "    [%02d] WR %d bytes to %#02x [%#02x%s",
>  				       curmsg, msg->len, addr, addr<<1,
>  				       msg->len == 0 ? "" : ", ");

You missed some spaces areound operators here. Please check for more.

> -				for(i=0; i < msg->len; i++)
> -					printk("%#04x%s", msg->buf[i], i == msg->len - 1 ? "" : ", ");
> +				for (i = 0; i < msg->len; i++)
> +					printk("%#04x%s",
> +							msg->buf[i],
> +							i == msg->len - 1 ?
> +							"" : ", ");

NACK. This is not readable. Two lines should do.

>  				printk("]\n");
>  			}
>  		}
> @@ -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.

>  			if (numbytes < msg->len) {
>  				completed = pca_tx_byte(adap,
>  							msg->buf[numbytes]);
> @@ -256,16 +261,19 @@ static int pca_xfer(struct i2c_adapter *i2c_adap,
>  				completed = pca_repeated_start(adap);
>  			break;
>  
> -		case 0x20: /* SLA+W has been transmitted; NOT ACK has been received */
> +		case 0x20: /* SLA+W has been transmitted;
> +			      NOT ACK has been received */
>  			DEB2("NOT ACK received after SLA+W\n");
>  			pca_stop(adap);
>  			goto out;
>  
> -		case 0x40: /* SLA+R has been transmitted; ACK has been received */
> +		case 0x40: /* SLA+R has been transmitted;
> +			      ACK has been received */
>  			completed = pca_rx_ack(adap, msg->len > 1);
>  			break;
>  
> -		case 0x50: /* Data bytes has been received; ACK has been returned */
> +		case 0x50: /* Data bytes has been received;
> +			      ACK has been returned */
>  			if (numbytes < msg->len) {
>  				pca_rx_byte(adap, &msg->buf[numbytes], 1);
>  				numbytes++;
> @@ -280,17 +288,20 @@ static int pca_xfer(struct i2c_adapter *i2c_adap,
>  				completed = pca_repeated_start(adap);
>  			break;
>  
> -		case 0x48: /* SLA+R has been transmitted; NOT ACK has been received */
> +		case 0x48: /* SLA+R has been transmitted;
> +			      NOT ACK has been received */
>  			DEB2("NOT ACK received after SLA+R\n");
>  			pca_stop(adap);
>  			goto out;
>  
> -		case 0x30: /* Data byte in I2CDAT has been transmitted; NOT ACK has been received */
> +		case 0x30: /* Data byte in I2CDAT has been transmitted;
> +			      NOT ACK has been received */
>  			DEB2("NOT ACK received after data byte\n");
>  			pca_stop(adap);
>  			goto out;
>  
> -		case 0x38: /* Arbitration lost during SLA+W, SLA+R or data bytes */
> +		case 0x38: /* Arbitration lost during SLA+W, SLA+R or
> +			      data bytes */
>  			DEB2("Arbitration lost\n");
>  			/*
>  			 * The PCA9564 data sheet (2006-09-01) says "A
> @@ -304,8 +315,9 @@ static int pca_xfer(struct i2c_adapter *i2c_adap,
>  			pca_start(adap);
>  			goto out;
>  
> -		case 0x58: /* Data byte has been received; NOT ACK has been returned */
> -			if ( numbytes == msg->len - 1 ) {
> +		case 0x58: /* Data byte has been received;
> +			      NOT ACK has been returned */
> +			if (numbytes == msg->len - 1) {
>  				pca_rx_byte(adap, &msg->buf[numbytes], 0);
>  				curmsg++; numbytes = 0;
>  				if (curmsg == num)
> @@ -328,12 +340,14 @@ static int pca_xfer(struct i2c_adapter *i2c_adap,
>  			DEB2("BUS ERROR - SCL Stuck low\n");
>  			pca_reset(adap);
>  			goto out;
> -		case 0x00: /* Bus error during master or slave mode due to illegal START or STOP condition */
> +		case 0x00: /* Bus error during master or slave mode
> +			      due to illegal START or STOP condition */
>  			DEB2("BUS ERROR - Illegal START or STOP\n");
>  			pca_reset(adap);
>  			goto out;
>  		default:
> -			dev_err(&i2c_adap->dev, "unhandled SIO state 0x%02x\n", state);
> +			dev_err(&i2c_adap->dev, "unhandled SIO state 0x%02x\n",
> +					state);

Oh well, so be it.

>  			break;
>  		}
>  
> @@ -352,7 +366,7 @@ static int pca_xfer(struct i2c_adapter *i2c_adap,
>  
>  static u32 pca_func(struct i2c_adapter *adap)
>  {
> -        return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
> +	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;

OK

>  }
>  
>  static const struct i2c_algorithm pca_algo = {
> -- 
> 1.7.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

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

[Jean Delvare]

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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > ---
> >  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.

> > (...)
> > @@ -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
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 agree with all other comments.

-- 
Jean Delvare

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 3281 bytes --]

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@gmail.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/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

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

[Farid Hammane]

Hi Jean and Wolfram,

On Thursday 22 April 2010 09:59:37 you wrote:
> 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.

Ok, I think that nothing can be broken with this kind of patch. I just built 
i2c-algo-pca.c and everything was ok.

For further patchs I will mention that it has been tested. Thanks for the 
advice !

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

Ok

> > >
> > > 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > > > ---
> > > >  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

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

Done ;)

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

To keep readability, I broke every line after the semicolon.
I understand both your points of view and I prefer not to change these lines.

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

Patch V2 will come with the next email.

Thanks,
I'll keep in mind your advices.

Regards,

Farid

> 
> Regards,
> 
>    Wolfram
> 

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

[Jean Delvare]

Hi Farid,

On Thu, 22 Apr 2010 23:30:34 +0200, Farid Hammane wrote:
> Hi Jean and Wolfram,
> 
> On Thursday 22 April 2010 09:59:37 you wrote:
> > 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.
> 
> Ok, I think that nothing can be broken with this kind of patch. I just built 
> i2c-algo-pca.c and everything was ok.
> 
> For further patchs I will mention that it has been tested. Thanks for the 
> advice !

It's not just a matter of building it. When you only fix coding style
issue, the binary object before and after your patch should be exactly
the same. It's a good idea to verify this, especially for drivers you
aren't using yourself.

-- 
Jean Delvare