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

[Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>]

[-- 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 --]