[PATCH v2 00/10] i2c-i801: Various cleanups

[PATCH v2 00/10] i2c-i801: Various cleanups

[minyard]

This is a new set that is fairly different than the old set, though it
fixes a lot of the same issues, and some new ones I noticed.

I did not do the consolidation of the isr and non-isr byte-by-byte
handling, based on Jean's comments.

I tested this on qemu (including returning a bad number of bytes to
cause a protocol error) with all four combinations of block/byte-by-byte
and interrupts/polled.

I also tested all those combinations on an Intel Chesnee board.

I do not know aobut hwpec, so I assumed it doesn't work on byte-by-byte
transactions.  I'm not quite sure how to test that.

Feel free to say if you like or don't like certain changes, and I
will rework as necessary, of course.

-corey

[PATCH v2 01/10] i2c-i801: Remove hwpec from block byte-by-byte function

[minyard]

From: Corey Minyard <cminyard@mvista.com>

It's not used in the function, so get rid of it.

Signed-off-by: Corey Minyard <cminyard@mvista.com>
---
 drivers/i2c/busses/i2c-i801.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 64b1208b..818c0c8 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -554,8 +554,7 @@ static irqreturn_t i801_isr(int irq, void *dev_id)
  */
 static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
 					       union i2c_smbus_data *data,
-					       char read_write, int command,
-					       int hwpec)
+					       char read_write, int command)
 {
 	int i, len;
 	int smbcmd;
@@ -698,7 +697,7 @@ static int i801_block_transaction(struct i801_priv *priv,
 	else
 		result = i801_block_transaction_byte_by_byte(priv, data,
 							     read_write,
-							     command, hwpec);
+							     command);
 
 	if (command == I2C_SMBUS_I2C_BLOCK_DATA
 	 && read_write == I2C_SMBUS_WRITE) {
-- 
2.7.4

Re: [v2,01/10] i2c-i801: Remove hwpec from block byte-by-byte function

[Benjamin Tissoires]

Hi Corey,

I wanted to send a global rev-by, but it looks like some changes in this
series are required from my point of view, so I'll just send individual
rev-by and change requests as my review progresses.

On May 29 2016 or thereabouts, Corey Minyard wrote:
> From: Corey Minyard <cminyard@mvista.com>
> 
> It's not used in the function, so get rid of it.
> 
> Signed-off-by: Corey Minyard <cminyard@mvista.com>
> ---

I am not entirely sure if this was kept for a reason or not, but the
argument is not used. So:
Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

Cheers,
Benjamin

>  drivers/i2c/busses/i2c-i801.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index 64b1208b..818c0c8 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -554,8 +554,7 @@ static irqreturn_t i801_isr(int irq, void *dev_id)
>   */
>  static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
>  					       union i2c_smbus_data *data,
> -					       char read_write, int command,
> -					       int hwpec)
> +					       char read_write, int command)
>  {
>  	int i, len;
>  	int smbcmd;
> @@ -698,7 +697,7 @@ static int i801_block_transaction(struct i801_priv *priv,
>  	else
>  		result = i801_block_transaction_byte_by_byte(priv, data,
>  							     read_write,
> -							     command, hwpec);
> +							     command);
>  
>  	if (command == I2C_SMBUS_I2C_BLOCK_DATA
>  	 && read_write == I2C_SMBUS_WRITE) {

[PATCH v2 02/10] i2c-i801: Move hostcfg set/reset to i801_access()

[minyard]

From: Corey Minyard <cminyard@mvista.com>

The HSTCFG register save/restore was done in i2c_block_transaction,
but all the checks were already done in i801_access, so move it into
those checks.

This results in a small savings of code, and moves some special
handing for I2C transactions into code that is already handling
special things for I2C transactions.

Signed-off-by: Corey Minyard <cminyard@mvista.com>
---
 drivers/i2c/busses/i2c-i801.c | 41 +++++++++++++++++++----------------------
 1 file changed, 19 insertions(+), 22 deletions(-)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 818c0c8..205f9d0 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -661,20 +661,6 @@ static int i801_block_transaction(struct i801_priv *priv,
 				  int command, int hwpec)
 {
 	int result = 0;
-	unsigned char hostc;
-
-	if (command == I2C_SMBUS_I2C_BLOCK_DATA) {
-		if (read_write == I2C_SMBUS_WRITE) {
-			/* set I2C_EN bit in configuration register */
-			pci_read_config_byte(priv->pci_dev, SMBHSTCFG, &hostc);
-			pci_write_config_byte(priv->pci_dev, SMBHSTCFG,
-					      hostc | SMBHSTCFG_I2C_EN);
-		} else if (!(priv->features & FEATURE_I2C_BLOCK_READ)) {
-			dev_err(&priv->pci_dev->dev,
-				"I2C block read is unsupported!\n");
-			return -EOPNOTSUPP;
-		}
-	}
 
 	if (read_write == I2C_SMBUS_WRITE
 	 || command == I2C_SMBUS_I2C_BLOCK_DATA) {
@@ -699,11 +685,6 @@ static int i801_block_transaction(struct i801_priv *priv,
 							     read_write,
 							     command);
 
-	if (command == I2C_SMBUS_I2C_BLOCK_DATA
-	 && read_write == I2C_SMBUS_WRITE) {
-		/* restore saved configuration register value */
-		pci_write_config_byte(priv->pci_dev, SMBHSTCFG, hostc);
-	}
 	return result;
 }
 
@@ -715,6 +696,7 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
 	int hwpec;
 	int block = 0;
 	int ret = 0, xact = 0;
+	int hostc = -1;
 	struct i801_priv *priv = i2c_get_adapdata(adap);
 
 	pm_runtime_get_sync(&priv->pci_dev->dev);
@@ -764,12 +746,24 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
 		/* NB: page 240 of ICH5 datasheet shows that the R/#W
 		 * bit should be cleared here, even when reading */
 		outb_p((addr & 0x7f) << 1, SMBHSTADD(priv));
-		if (read_write == I2C_SMBUS_READ) {
+		if (read_write == I2C_SMBUS_WRITE) {
+			unsigned char thostc;
+
+			outb_p(command, SMBHSTCMD(priv));
+			/* set I2C_EN bit in configuration register */
+			pci_read_config_byte(priv->pci_dev, SMBHSTCFG, &thostc);
+			pci_write_config_byte(priv->pci_dev, SMBHSTCFG,
+					      thostc | SMBHSTCFG_I2C_EN);
+			hostc = thostc;
+		} else if (!(priv->features & FEATURE_I2C_BLOCK_READ)) {
+			dev_err(&priv->pci_dev->dev,
+				"I2C block read is unsupported!\n");
+			ret = -EOPNOTSUPP;
+			goto out;
+		} else
 			/* NB: page 240 of ICH5 datasheet also shows
 			 * that DATA1 is the cmd field when reading */
 			outb_p(command, SMBHSTDAT1(priv));
-		} else
-			outb_p(command, SMBHSTCMD(priv));
 		block = 1;
 		break;
 	default:
@@ -798,6 +792,9 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
 		outb_p(inb_p(SMBAUXCTL(priv)) &
 		       ~(SMBAUXCTL_CRC | SMBAUXCTL_E32B), SMBAUXCTL(priv));
 
+	if (hostc >= 0)
+		pci_write_config_byte(priv->pci_dev, SMBHSTCFG, hostc);
+
 	if (block)
 		goto out;
 	if (ret)
-- 
2.7.4

Re: [v2,02/10] i2c-i801: Move hostcfg set/reset to i801_access()

[Benjamin Tissoires]

On May 29 2016 or thereabouts, Corey Minyard wrote:
> From: Corey Minyard <cminyard@mvista.com>
> 
> The HSTCFG register save/restore was done in i2c_block_transaction,
> but all the checks were already done in i801_access, so move it into
> those checks.
> 
> This results in a small savings of code, and moves some special
> handing for I2C transactions into code that is already handling
> special things for I2C transactions.
> 
> Signed-off-by: Corey Minyard <cminyard@mvista.com>
> ---
>  drivers/i2c/busses/i2c-i801.c | 41 +++++++++++++++++++----------------------
>  1 file changed, 19 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index 818c0c8..205f9d0 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -661,20 +661,6 @@ static int i801_block_transaction(struct i801_priv *priv,
>  				  int command, int hwpec)
>  {
>  	int result = 0;
> -	unsigned char hostc;
> -
> -	if (command == I2C_SMBUS_I2C_BLOCK_DATA) {
> -		if (read_write == I2C_SMBUS_WRITE) {
> -			/* set I2C_EN bit in configuration register */
> -			pci_read_config_byte(priv->pci_dev, SMBHSTCFG, &hostc);
> -			pci_write_config_byte(priv->pci_dev, SMBHSTCFG,
> -					      hostc | SMBHSTCFG_I2C_EN);
> -		} else if (!(priv->features & FEATURE_I2C_BLOCK_READ)) {
> -			dev_err(&priv->pci_dev->dev,
> -				"I2C block read is unsupported!\n");
> -			return -EOPNOTSUPP;
> -		}
> -	}
>  
>  	if (read_write == I2C_SMBUS_WRITE
>  	 || command == I2C_SMBUS_I2C_BLOCK_DATA) {
> @@ -699,11 +685,6 @@ static int i801_block_transaction(struct i801_priv *priv,
>  							     read_write,
>  							     command);
>  
> -	if (command == I2C_SMBUS_I2C_BLOCK_DATA
> -	 && read_write == I2C_SMBUS_WRITE) {
> -		/* restore saved configuration register value */
> -		pci_write_config_byte(priv->pci_dev, SMBHSTCFG, hostc);
> -	}
>  	return result;
>  }
>  
> @@ -715,6 +696,7 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
>  	int hwpec;
>  	int block = 0;
>  	int ret = 0, xact = 0;
> +	int hostc = -1;
>  	struct i801_priv *priv = i2c_get_adapdata(adap);
>  
>  	pm_runtime_get_sync(&priv->pci_dev->dev);
> @@ -764,12 +746,24 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
>  		/* NB: page 240 of ICH5 datasheet shows that the R/#W
>  		 * bit should be cleared here, even when reading */
>  		outb_p((addr & 0x7f) << 1, SMBHSTADD(priv));
> -		if (read_write == I2C_SMBUS_READ) {
> +		if (read_write == I2C_SMBUS_WRITE) {
> +			unsigned char thostc;
> +
> +			outb_p(command, SMBHSTCMD(priv));
> +			/* set I2C_EN bit in configuration register */
> +			pci_read_config_byte(priv->pci_dev, SMBHSTCFG, &thostc);
> +			pci_write_config_byte(priv->pci_dev, SMBHSTCFG,
> +					      thostc | SMBHSTCFG_I2C_EN);
> +			hostc = thostc;
> +		} else if (!(priv->features & FEATURE_I2C_BLOCK_READ)) {

nitpick: I'd prefer seeing:
if (write) {
  foo
} else {
  if (!FEATURE_I2C_BLOCK_READ) {
    goto out
  }
  bar
}

It's equivalent but I feel like it's easier to understand.

Other than that, no strong opinion on the patch itself, but it's correct
(the code path stays the same):
Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

Cheers,
Benjamin

> +			dev_err(&priv->pci_dev->dev,
> +				"I2C block read is unsupported!\n");
> +			ret = -EOPNOTSUPP;
> +			goto out;
> +		} else
>  			/* NB: page 240 of ICH5 datasheet also shows
>  			 * that DATA1 is the cmd field when reading */
>  			outb_p(command, SMBHSTDAT1(priv));
> -		} else
> -			outb_p(command, SMBHSTCMD(priv));
>  		block = 1;
>  		break;
>  	default:
> @@ -798,6 +792,9 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
>  		outb_p(inb_p(SMBAUXCTL(priv)) &
>  		       ~(SMBAUXCTL_CRC | SMBAUXCTL_E32B), SMBAUXCTL(priv));
>  
> +	if (hostc >= 0)
> +		pci_write_config_byte(priv->pci_dev, SMBHSTCFG, hostc);
> +
>  	if (block)
>  		goto out;
>  	if (ret)

Re: [v2,02/10] i2c-i801: Move hostcfg set/reset to i801_access()

[Corey Minyard]

On 06/09/2016 04:39 AM, Benjamin Tissoires wrote:
> On May 29 2016 or thereabouts, Corey Minyard wrote:
>> From: Corey Minyard <cminyard@mvista.com>
>>
>> The HSTCFG register save/restore was done in i2c_block_transaction,
>> but all the checks were already done in i801_access, so move it into
>> those checks.
>>
>> This results in a small savings of code, and moves some special
>> handing for I2C transactions into code that is already handling
>> special things for I2C transactions.
>>
>> Signed-off-by: Corey Minyard <cminyard@mvista.com>
>> ---
>>   drivers/i2c/busses/i2c-i801.c | 41 +++++++++++++++++++----------------------
>>   1 file changed, 19 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
>> index 818c0c8..205f9d0 100644
>> --- a/drivers/i2c/busses/i2c-i801.c
>> +++ b/drivers/i2c/busses/i2c-i801.c
>> @@ -661,20 +661,6 @@ static int i801_block_transaction(struct i801_priv *priv,
>>   				  int command, int hwpec)
>>   {
>>   	int result = 0;
>> -	unsigned char hostc;
>> -
>> -	if (command == I2C_SMBUS_I2C_BLOCK_DATA) {
>> -		if (read_write == I2C_SMBUS_WRITE) {
>> -			/* set I2C_EN bit in configuration register */
>> -			pci_read_config_byte(priv->pci_dev, SMBHSTCFG, &hostc);
>> -			pci_write_config_byte(priv->pci_dev, SMBHSTCFG,
>> -					      hostc | SMBHSTCFG_I2C_EN);
>> -		} else if (!(priv->features & FEATURE_I2C_BLOCK_READ)) {
>> -			dev_err(&priv->pci_dev->dev,
>> -				"I2C block read is unsupported!\n");
>> -			return -EOPNOTSUPP;
>> -		}
>> -	}
>>   
>>   	if (read_write == I2C_SMBUS_WRITE
>>   	 || command == I2C_SMBUS_I2C_BLOCK_DATA) {
>> @@ -699,11 +685,6 @@ static int i801_block_transaction(struct i801_priv *priv,
>>   							     read_write,
>>   							     command);
>>   
>> -	if (command == I2C_SMBUS_I2C_BLOCK_DATA
>> -	 && read_write == I2C_SMBUS_WRITE) {
>> -		/* restore saved configuration register value */
>> -		pci_write_config_byte(priv->pci_dev, SMBHSTCFG, hostc);
>> -	}
>>   	return result;
>>   }
>>   
>> @@ -715,6 +696,7 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
>>   	int hwpec;
>>   	int block = 0;
>>   	int ret = 0, xact = 0;
>> +	int hostc = -1;
>>   	struct i801_priv *priv = i2c_get_adapdata(adap);
>>   
>>   	pm_runtime_get_sync(&priv->pci_dev->dev);
>> @@ -764,12 +746,24 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
>>   		/* NB: page 240 of ICH5 datasheet shows that the R/#W
>>   		 * bit should be cleared here, even when reading */
>>   		outb_p((addr & 0x7f) << 1, SMBHSTADD(priv));
>> -		if (read_write == I2C_SMBUS_READ) {
>> +		if (read_write == I2C_SMBUS_WRITE) {
>> +			unsigned char thostc;
>> +
>> +			outb_p(command, SMBHSTCMD(priv));
>> +			/* set I2C_EN bit in configuration register */
>> +			pci_read_config_byte(priv->pci_dev, SMBHSTCFG, &thostc);
>> +			pci_write_config_byte(priv->pci_dev, SMBHSTCFG,
>> +					      thostc | SMBHSTCFG_I2C_EN);
>> +			hostc = thostc;
>> +		} else if (!(priv->features & FEATURE_I2C_BLOCK_READ)) {
> nitpick: I'd prefer seeing:
> if (write) {
>    foo
> } else {
>    if (!FEATURE_I2C_BLOCK_READ) {
>      goto out
>    }
>    bar
> }
>
> It's equivalent but I feel like it's easier to understand.

First, thanks for all the reviews.  I really appreciate it.

On this particular change, it's moved from another location unchanged,
and it follows the style of the rest of the file.  I personally think 
it's better
to leave it like it is.

-corey

> Other than that, no strong opinion on the patch itself, but it's correct
> (the code path stays the same):
> Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>
> Cheers,
> Benjamin
>
>> +			dev_err(&priv->pci_dev->dev,
>> +				"I2C block read is unsupported!\n");
>> +			ret = -EOPNOTSUPP;
>> +			goto out;
>> +		} else
>>   			/* NB: page 240 of ICH5 datasheet also shows
>>   			 * that DATA1 is the cmd field when reading */
>>   			outb_p(command, SMBHSTDAT1(priv));
>> -		} else
>> -			outb_p(command, SMBHSTCMD(priv));
>>   		block = 1;
>>   		break;
>>   	default:
>> @@ -798,6 +792,9 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
>>   		outb_p(inb_p(SMBAUXCTL(priv)) &
>>   		       ~(SMBAUXCTL_CRC | SMBAUXCTL_E32B), SMBAUXCTL(priv));
>>   
>> +	if (hostc >= 0)
>> +		pci_write_config_byte(priv->pci_dev, SMBHSTCFG, hostc);
>> +
>>   	if (block)
>>   		goto out;
>>   	if (ret)

[PATCH v2 03/10] i2c-i801: Move hwpec handling into block transaction

[minyard]

From: Corey Minyard <cminyard@mvista.com>

Since hwpec is only used for block transactions, move it out of
i801_access() and into i801_block_transaction().

Signed-off-by: Corey Minyard <cminyard@mvista.com>
---
 drivers/i2c/busses/i2c-i801.c | 47 +++++++++++++++++++++++--------------------
 1 file changed, 25 insertions(+), 22 deletions(-)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 205f9d0..222be9c 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -656,11 +656,22 @@ static int i801_set_block_buffer_mode(struct i801_priv *priv)
 }
 
 /* Block transaction function */
-static int i801_block_transaction(struct i801_priv *priv,
+static int i801_block_transaction(struct i801_priv *priv, unsigned short flags,
 				  union i2c_smbus_data *data, char read_write,
-				  int command, int hwpec)
+				  int command)
 {
 	int result = 0;
+	int hwpec = (priv->features & FEATURE_SMBUS_PEC)
+		&& (flags & I2C_CLIENT_PEC)
+		&& command != I2C_SMBUS_QUICK
+		&& command != I2C_SMBUS_I2C_BLOCK_DATA;
+
+	if (hwpec)	/* enable/disable hardware PEC */
+		outb_p(inb_p(SMBAUXCTL(priv)) | SMBAUXCTL_CRC,
+		       SMBAUXCTL(priv));
+	else
+		outb_p(inb_p(SMBAUXCTL(priv)) & (~SMBAUXCTL_CRC),
+		       SMBAUXCTL(priv));
 
 	if (read_write == I2C_SMBUS_WRITE
 	 || command == I2C_SMBUS_I2C_BLOCK_DATA) {
@@ -685,6 +696,16 @@ static int i801_block_transaction(struct i801_priv *priv,
 							     read_write,
 							     command);
 
+	/*
+	 * Some BIOSes don't like it when PEC is enabled at reboot or
+	 * resume time, so we forcibly disable it after every
+	 * transaction. Turn off E32B for the same reason.
+	 */
+	if (hwpec)
+		outb_p(inb_p(SMBAUXCTL(priv)) &
+		       ~(SMBAUXCTL_CRC | SMBAUXCTL_E32B),
+		       SMBAUXCTL(priv));
+
 	return result;
 }
 
@@ -693,7 +714,6 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
 		       unsigned short flags, char read_write, u8 command,
 		       int size, union i2c_smbus_data *data)
 {
-	int hwpec;
 	int block = 0;
 	int ret = 0, xact = 0;
 	int hostc = -1;
@@ -701,10 +721,6 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
 
 	pm_runtime_get_sync(&priv->pci_dev->dev);
 
-	hwpec = (priv->features & FEATURE_SMBUS_PEC) && (flags & I2C_CLIENT_PEC)
-		&& size != I2C_SMBUS_QUICK
-		&& size != I2C_SMBUS_I2C_BLOCK_DATA;
-
 	switch (size) {
 	case I2C_SMBUS_QUICK:
 		outb_p(((addr & 0x7f) << 1) | (read_write & 0x01),
@@ -773,25 +789,12 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
 		goto out;
 	}
 
-	if (hwpec)	/* enable/disable hardware PEC */
-		outb_p(inb_p(SMBAUXCTL(priv)) | SMBAUXCTL_CRC, SMBAUXCTL(priv));
-	else
-		outb_p(inb_p(SMBAUXCTL(priv)) & (~SMBAUXCTL_CRC),
-		       SMBAUXCTL(priv));
-
 	if (block)
-		ret = i801_block_transaction(priv, data, read_write, size,
-					     hwpec);
+		ret = i801_block_transaction(priv, flags, data, read_write,
+					     size);
 	else
 		ret = i801_transaction(priv, xact);
 
-	/* Some BIOSes don't like it when PEC is enabled at reboot or resume
-	   time, so we forcibly disable it after every transaction. Turn off
-	   E32B for the same reason. */
-	if (hwpec || block)
-		outb_p(inb_p(SMBAUXCTL(priv)) &
-		       ~(SMBAUXCTL_CRC | SMBAUXCTL_E32B), SMBAUXCTL(priv));
-
 	if (hostc >= 0)
 		pci_write_config_byte(priv->pci_dev, SMBHSTCFG, hostc);
 
-- 
2.7.4

Re: [v2,03/10] i2c-i801: Move hwpec handling into block transaction

[Benjamin Tissoires]

On May 29 2016 or thereabouts, Corey Minyard wrote:
> From: Corey Minyard <cminyard@mvista.com>
> 
> Since hwpec is only used for block transactions, move it out of
> i801_access() and into i801_block_transaction().
> 
> Signed-off-by: Corey Minyard <cminyard@mvista.com>
> ---
>  drivers/i2c/busses/i2c-i801.c | 47 +++++++++++++++++++++++--------------------
>  1 file changed, 25 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index 205f9d0..222be9c 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -656,11 +656,22 @@ static int i801_set_block_buffer_mode(struct i801_priv *priv)
>  }
>  
>  /* Block transaction function */
> -static int i801_block_transaction(struct i801_priv *priv,
> +static int i801_block_transaction(struct i801_priv *priv, unsigned short flags,
>  				  union i2c_smbus_data *data, char read_write,
> -				  int command, int hwpec)
> +				  int command)
>  {
>  	int result = 0;
> +	int hwpec = (priv->features & FEATURE_SMBUS_PEC)
> +		&& (flags & I2C_CLIENT_PEC)
> +		&& command != I2C_SMBUS_QUICK
> +		&& command != I2C_SMBUS_I2C_BLOCK_DATA;
> +
> +	if (hwpec)	/* enable/disable hardware PEC */
> +		outb_p(inb_p(SMBAUXCTL(priv)) | SMBAUXCTL_CRC,
> +		       SMBAUXCTL(priv));
> +	else
> +		outb_p(inb_p(SMBAUXCTL(priv)) & (~SMBAUXCTL_CRC),
> +		       SMBAUXCTL(priv));
>  
>  	if (read_write == I2C_SMBUS_WRITE
>  	 || command == I2C_SMBUS_I2C_BLOCK_DATA) {
> @@ -685,6 +696,16 @@ static int i801_block_transaction(struct i801_priv *priv,
>  							     read_write,
>  							     command);
>  
> +	/*
> +	 * Some BIOSes don't like it when PEC is enabled at reboot or
> +	 * resume time, so we forcibly disable it after every
> +	 * transaction. Turn off E32B for the same reason.
> +	 */
> +	if (hwpec)

If I read correctly, there should not be a need for the if, but do the
outb_p() call unconditionally. In i801_access, this is guarded by
(hwpec || block), and we are in the block case anyway.

So now, with this testm you won't disable SMBAUXCTL_E32B after every
transaction, which causes problems according to the comment above.

Cheers,
Benjamin

> +		outb_p(inb_p(SMBAUXCTL(priv)) &
> +		       ~(SMBAUXCTL_CRC | SMBAUXCTL_E32B),
> +		       SMBAUXCTL(priv));
> +
>  	return result;
>  }
>  
> @@ -693,7 +714,6 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
>  		       unsigned short flags, char read_write, u8 command,
>  		       int size, union i2c_smbus_data *data)
>  {
> -	int hwpec;
>  	int block = 0;
>  	int ret = 0, xact = 0;
>  	int hostc = -1;
> @@ -701,10 +721,6 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
>  
>  	pm_runtime_get_sync(&priv->pci_dev->dev);
>  
> -	hwpec = (priv->features & FEATURE_SMBUS_PEC) && (flags & I2C_CLIENT_PEC)
> -		&& size != I2C_SMBUS_QUICK
> -		&& size != I2C_SMBUS_I2C_BLOCK_DATA;
> -
>  	switch (size) {
>  	case I2C_SMBUS_QUICK:
>  		outb_p(((addr & 0x7f) << 1) | (read_write & 0x01),
> @@ -773,25 +789,12 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
>  		goto out;
>  	}
>  
> -	if (hwpec)	/* enable/disable hardware PEC */
> -		outb_p(inb_p(SMBAUXCTL(priv)) | SMBAUXCTL_CRC, SMBAUXCTL(priv));
> -	else
> -		outb_p(inb_p(SMBAUXCTL(priv)) & (~SMBAUXCTL_CRC),
> -		       SMBAUXCTL(priv));
> -
>  	if (block)
> -		ret = i801_block_transaction(priv, data, read_write, size,
> -					     hwpec);
> +		ret = i801_block_transaction(priv, flags, data, read_write,
> +					     size);
>  	else
>  		ret = i801_transaction(priv, xact);
>  
> -	/* Some BIOSes don't like it when PEC is enabled at reboot or resume
> -	   time, so we forcibly disable it after every transaction. Turn off
> -	   E32B for the same reason. */
> -	if (hwpec || block)
> -		outb_p(inb_p(SMBAUXCTL(priv)) &
> -		       ~(SMBAUXCTL_CRC | SMBAUXCTL_E32B), SMBAUXCTL(priv));
> -
>  	if (hostc >= 0)
>  		pci_write_config_byte(priv->pci_dev, SMBHSTCFG, hostc);
>  

[PATCH v2 04/10] i2c-i801: Consolidate calls to i801_check_pre()

[minyard]

From: Corey Minyard <cminyard@mvista.com>

It was always done before starting the transaction, so do it
in common code before the transaction start.

Signed-off-by: Corey Minyard <cminyard@mvista.com>
---
 drivers/i2c/busses/i2c-i801.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 222be9c..8794e70 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -402,10 +402,6 @@ static int i801_transaction(struct i801_priv *priv, int xact)
 	int result;
 	const struct i2c_adapter *adap = &priv->adapter;
 
-	result = i801_check_pre(priv);
-	if (result < 0)
-		return result;
-
 	if (priv->features & FEATURE_IRQ) {
 		outb_p(xact | SMBHSTCNT_INTREN | SMBHSTCNT_START,
 		       SMBHSTCNT(priv));
@@ -562,10 +558,6 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
 	int result;
 	const struct i2c_adapter *adap = &priv->adapter;
 
-	result = i801_check_pre(priv);
-	if (result < 0)
-		return result;
-
 	len = data->block[0];
 
 	if (read_write == I2C_SMBUS_WRITE) {
@@ -789,6 +781,10 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
 		goto out;
 	}
 
+	ret = i801_check_pre(priv);
+	if (ret < 0)
+		goto out;
+
 	if (block)
 		ret = i801_block_transaction(priv, flags, data, read_write,
 					     size);
-- 
2.7.4

Re: [v2,04/10] i2c-i801: Consolidate calls to i801_check_pre()

[Benjamin Tissoires]

On May 29 2016 or thereabouts, Corey Minyard wrote:
> From: Corey Minyard <cminyard@mvista.com>
> 
> It was always done before starting the transaction, so do it
> in common code before the transaction start.
> 
> Signed-off-by: Corey Minyard <cminyard@mvista.com>
> ---
>  drivers/i2c/busses/i2c-i801.c | 12 ++++--------
>  1 file changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index 222be9c..8794e70 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -402,10 +402,6 @@ static int i801_transaction(struct i801_priv *priv, int xact)
>  	int result;
>  	const struct i2c_adapter *adap = &priv->adapter;
>  
> -	result = i801_check_pre(priv);
> -	if (result < 0)
> -		return result;
> -
>  	if (priv->features & FEATURE_IRQ) {
>  		outb_p(xact | SMBHSTCNT_INTREN | SMBHSTCNT_START,
>  		       SMBHSTCNT(priv));
> @@ -562,10 +558,6 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
>  	int result;
>  	const struct i2c_adapter *adap = &priv->adapter;
>  
> -	result = i801_check_pre(priv);
> -	if (result < 0)
> -		return result;
> -
>  	len = data->block[0];
>  
>  	if (read_write == I2C_SMBUS_WRITE) {
> @@ -789,6 +781,10 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
>  		goto out;
>  	}
>  
> +	ret = i801_check_pre(priv);
> +	if (ret < 0)
> +		goto out;
> +

The calls looks identical (it gets called before each transaction), but
given that this is a status register, I wonder if this should not be
checked once the setup has been done, and not at the very early
beginning.

Cheers,
Benjamin

>  	if (block)
>  		ret = i801_block_transaction(priv, flags, data, read_write,
>  					     size);

Re: [v2,04/10] i2c-i801: Consolidate calls to i801_check_pre()

[Corey Minyard]

On 06/09/2016 04:44 AM, Benjamin Tissoires wrote:
> On May 29 2016 or thereabouts, Corey Minyard wrote:
>> From: Corey Minyard <cminyard@mvista.com>
>>
>> It was always done before starting the transaction, so do it
>> in common code before the transaction start.
>>
>> Signed-off-by: Corey Minyard <cminyard@mvista.com>
>> ---
>>   drivers/i2c/busses/i2c-i801.c | 12 ++++--------
>>   1 file changed, 4 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
>> index 222be9c..8794e70 100644
>> --- a/drivers/i2c/busses/i2c-i801.c
>> +++ b/drivers/i2c/busses/i2c-i801.c
>> @@ -402,10 +402,6 @@ static int i801_transaction(struct i801_priv *priv, int xact)
>>   	int result;
>>   	const struct i2c_adapter *adap = &priv->adapter;
>>   
>> -	result = i801_check_pre(priv);
>> -	if (result < 0)
>> -		return result;
>> -
>>   	if (priv->features & FEATURE_IRQ) {
>>   		outb_p(xact | SMBHSTCNT_INTREN | SMBHSTCNT_START,
>>   		       SMBHSTCNT(priv));
>> @@ -562,10 +558,6 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
>>   	int result;
>>   	const struct i2c_adapter *adap = &priv->adapter;
>>   
>> -	result = i801_check_pre(priv);
>> -	if (result < 0)
>> -		return result;
>> -
>>   	len = data->block[0];
>>   
>>   	if (read_write == I2C_SMBUS_WRITE) {
>> @@ -789,6 +781,10 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
>>   		goto out;
>>   	}
>>   
>> +	ret = i801_check_pre(priv);
>> +	if (ret < 0)
>> +		goto out;
>> +
> The calls looks identical (it gets called before each transaction), but
> given that this is a status register, I wonder if this should not be
> checked once the setup has been done, and not at the very early
> beginning.

What I did is not a functional change from what was before, but I
think you may be right, it might be best to move this check this
before the switch statement above.  I'm not sure, though.

Jean, do you have any thoughts on this?

-corey

> Cheers,
> Benjamin
>
>>   	if (block)
>>   		ret = i801_block_transaction(priv, flags, data, read_write,
>>   					     size);

[PATCH v2 05/10] i2c-i801: Consolidate calls to i801_check_post

[minyard]

From: Corey Minyard <cminyard@mvista.com>

This was almost always called at the end of the transaction.  The
only time it wasn't called was when a protocol violation occurred,
and the error reporting was inconsistent there.

So have the transaction functions return positive status or a
negative error if they detect an issue.  If a protocol violation
does occur, always do a kill operation on the bus instead of just
trying to finish the operation.  If there was a hardware issue,
the code that was there to terminate the transaction could loop
forever, and the kill seems more appropriate if something goes
grossly wrong.

Signed-off-by: Corey Minyard <cminyard@mvista.com>
---
 drivers/i2c/busses/i2c-i801.c | 51 +++++++++++++++++++++----------------------
 1 file changed, 25 insertions(+), 26 deletions(-)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 8794e70..56db310 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -317,7 +317,13 @@ static int i801_check_post(struct i801_priv *priv, int status)
 	 * DEV_ERR.
 	 */
 	if (unlikely(status < 0)) {
-		dev_err(&priv->pci_dev->dev, "Transaction timeout\n");
+		result = status;
+		if (status == -EPROTO)
+			dev_err(&priv->pci_dev->dev,
+				"Illegal SMBus block read size %d\n",
+				priv->len);
+		else
+			dev_err(&priv->pci_dev->dev, "Transaction timeout\n");
 		/* try to stop the current command */
 		dev_dbg(&priv->pci_dev->dev, "Terminating the current operation\n");
 		outb_p(inb_p(SMBHSTCNT(priv)) | SMBHSTCNT_KILL,
@@ -333,7 +339,7 @@ static int i801_check_post(struct i801_priv *priv, int status)
 			dev_err(&priv->pci_dev->dev,
 				"Failed terminating the transaction\n");
 		outb_p(STATUS_FLAGS, SMBHSTSTS(priv));
-		return -ETIMEDOUT;
+		return result;
 	}
 
 	if (status & SMBHSTSTS_FAILED) {
@@ -414,15 +420,14 @@ static int i801_transaction(struct i801_priv *priv, int xact)
 				 "Timeout waiting for interrupt!\n");
 		}
 		priv->status = 0;
-		return i801_check_post(priv, status);
+		return status;
 	}
 
 	/* the current contents of SMBHSTCNT can be overwritten, since PEC,
 	 * SMBSCMD are passed in xact */
 	outb_p(xact | SMBHSTCNT_START, SMBHSTCNT(priv));
 
-	status = i801_wait_intr(priv);
-	return i801_check_post(priv, status);
+	return i801_wait_intr(priv);
 }
 
 static int i801_block_transaction_by_block(struct i801_priv *priv,
@@ -444,11 +449,11 @@ static int i801_block_transaction_by_block(struct i801_priv *priv,
 
 	status = i801_transaction(priv, I801_BLOCK_DATA |
 				  (hwpec ? SMBHSTCNT_PEC_EN : 0));
-	if (status)
+	if (status < 0 || status & STATUS_ERROR_FLAGS)
 		return status;
 
 	if (read_write == I2C_SMBUS_READ) {
-		len = inb_p(SMBHSTDAT0(priv));
+		len = priv->len = inb_p(SMBHSTDAT0(priv));
 		if (len < 1 || len > I2C_SMBUS_BLOCK_MAX)
 			return -EPROTO;
 
@@ -456,7 +461,7 @@ static int i801_block_transaction_by_block(struct i801_priv *priv,
 		for (i = 0; i < len; i++)
 			data->block[i + 1] = inb_p(SMBBLKDAT(priv));
 	}
-	return 0;
+	return status;
 }
 
 static void i801_isr_byte_done(struct i801_priv *priv)
@@ -590,7 +595,7 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
 				 "Timeout waiting for interrupt!\n");
 		}
 		priv->status = 0;
-		return i801_check_post(priv, status);
+		return status;
 	}
 
 	for (i = 1; i <= len; i++) {
@@ -604,24 +609,14 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
 
 		status = i801_wait_byte_done(priv);
 		if (status)
-			goto exit;
+			return status;
 
 		if (i == 1 && read_write == I2C_SMBUS_READ
 		 && command != I2C_SMBUS_I2C_BLOCK_DATA) {
-			len = inb_p(SMBHSTDAT0(priv));
-			if (len < 1 || len > I2C_SMBUS_BLOCK_MAX) {
-				dev_err(&priv->pci_dev->dev,
-					"Illegal SMBus block read size %d\n",
-					len);
-				/* Recover */
-				while (inb_p(SMBHSTSTS(priv)) &
-				       SMBHSTSTS_HOST_BUSY)
-					outb_p(SMBHSTSTS_BYTE_DONE,
-					       SMBHSTSTS(priv));
-				outb_p(SMBHSTSTS_INTR, SMBHSTSTS(priv));
+			priv->len = inb_p(SMBHSTDAT0(priv));
+			if (priv->len < 1 || priv->len > I2C_SMBUS_BLOCK_MAX)
 				return -EPROTO;
-			}
-			data->block[0] = len;
+			data->block[0] = len = priv->len;
 		}
 
 		/* Retrieve/store value in SMBBLKDAT */
@@ -634,9 +629,7 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
 		outb_p(SMBHSTSTS_BYTE_DONE, SMBHSTSTS(priv));
 	}
 
-	status = i801_wait_intr(priv);
-exit:
-	return i801_check_post(priv, status);
+	return i801_wait_intr(priv);
 }
 
 static int i801_set_block_buffer_mode(struct i801_priv *priv)
@@ -791,6 +784,12 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
 	else
 		ret = i801_transaction(priv, xact);
 
+	/*
+	 * ret can be status if positive or an error if negative.  Let
+	 * the post check handle it.
+	 */
+	ret = i801_check_post(priv, ret);
+
 	if (hostc >= 0)
 		pci_write_config_byte(priv->pci_dev, SMBHSTCFG, hostc);
 
-- 
2.7.4

Re: [v2,05/10] i2c-i801: Consolidate calls to i801_check_post

[Benjamin Tissoires]

On May 29 2016 or thereabouts, Corey Minyard wrote:
> From: Corey Minyard <cminyard@mvista.com>
> 
> This was almost always called at the end of the transaction.  The
> only time it wasn't called was when a protocol violation occurred,
> and the error reporting was inconsistent there.
> 
> So have the transaction functions return positive status or a
> negative error if they detect an issue.  If a protocol violation
> does occur, always do a kill operation on the bus instead of just
> trying to finish the operation.  If there was a hardware issue,
> the code that was there to terminate the transaction could loop
> forever, and the kill seems more appropriate if something goes
> grossly wrong.
> 
> Signed-off-by: Corey Minyard <cminyard@mvista.com>
> ---

I *think* the patch is OK. I have a few comments.

>  drivers/i2c/busses/i2c-i801.c | 51 +++++++++++++++++++++----------------------
>  1 file changed, 25 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index 8794e70..56db310 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -317,7 +317,13 @@ static int i801_check_post(struct i801_priv *priv, int status)
>  	 * DEV_ERR.
>  	 */
>  	if (unlikely(status < 0)) {
> -		dev_err(&priv->pci_dev->dev, "Transaction timeout\n");
> +		result = status;
> +		if (status == -EPROTO)
> +			dev_err(&priv->pci_dev->dev,
> +				"Illegal SMBus block read size %d\n",
> +				priv->len);

Not sure this is a good idea to map -EPROTO to "Illegal SMBus block read
size". True, it looks like it can only happens this way, but I would
rather see it mapped to "Protocol Error". Given that Protocol Error is
not very interesting in itself, I'd rather keep the error messages in
the callers only (except for ETIMEOUT where you can output an error
message).

> +		else
> +			dev_err(&priv->pci_dev->dev, "Transaction timeout\n");
>  		/* try to stop the current command */
>  		dev_dbg(&priv->pci_dev->dev, "Terminating the current operation\n");
>  		outb_p(inb_p(SMBHSTCNT(priv)) | SMBHSTCNT_KILL,
> @@ -333,7 +339,7 @@ static int i801_check_post(struct i801_priv *priv, int status)
>  			dev_err(&priv->pci_dev->dev,
>  				"Failed terminating the transaction\n");
>  		outb_p(STATUS_FLAGS, SMBHSTSTS(priv));
> -		return -ETIMEDOUT;
> +		return result;
>  	}
>  
>  	if (status & SMBHSTSTS_FAILED) {
> @@ -414,15 +420,14 @@ static int i801_transaction(struct i801_priv *priv, int xact)
>  				 "Timeout waiting for interrupt!\n");

Here, the code already dev_warn the timeout and later i801_check_post
will dev_err the timeout too. That's one too much IMO :)

>  		}
>  		priv->status = 0;
> -		return i801_check_post(priv, status);
> +		return status;
>  	}
>  
>  	/* the current contents of SMBHSTCNT can be overwritten, since PEC,
>  	 * SMBSCMD are passed in xact */
>  	outb_p(xact | SMBHSTCNT_START, SMBHSTCNT(priv));
>  
> -	status = i801_wait_intr(priv);
> -	return i801_check_post(priv, status);
> +	return i801_wait_intr(priv);
>  }
>  
>  static int i801_block_transaction_by_block(struct i801_priv *priv,
> @@ -444,11 +449,11 @@ static int i801_block_transaction_by_block(struct i801_priv *priv,
>  
>  	status = i801_transaction(priv, I801_BLOCK_DATA |
>  				  (hwpec ? SMBHSTCNT_PEC_EN : 0));
> -	if (status)
> +	if (status < 0 || status & STATUS_ERROR_FLAGS)
>  		return status;
>  
>  	if (read_write == I2C_SMBUS_READ) {
> -		len = inb_p(SMBHSTDAT0(priv));
> +		len = priv->len = inb_p(SMBHSTDAT0(priv));
>  		if (len < 1 || len > I2C_SMBUS_BLOCK_MAX)
>  			return -EPROTO;
>  
> @@ -456,7 +461,7 @@ static int i801_block_transaction_by_block(struct i801_priv *priv,
>  		for (i = 0; i < len; i++)
>  			data->block[i + 1] = inb_p(SMBBLKDAT(priv));
>  	}
> -	return 0;
> +	return status;
>  }
>  
>  static void i801_isr_byte_done(struct i801_priv *priv)
> @@ -590,7 +595,7 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
>  				 "Timeout waiting for interrupt!\n");
>  		}
>  		priv->status = 0;
> -		return i801_check_post(priv, status);
> +		return status;
>  	}
>  
>  	for (i = 1; i <= len; i++) {
> @@ -604,24 +609,14 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
>  
>  		status = i801_wait_byte_done(priv);
>  		if (status)
> -			goto exit;
> +			return status;
>  
>  		if (i == 1 && read_write == I2C_SMBUS_READ
>  		 && command != I2C_SMBUS_I2C_BLOCK_DATA) {
> -			len = inb_p(SMBHSTDAT0(priv));
> -			if (len < 1 || len > I2C_SMBUS_BLOCK_MAX) {
> -				dev_err(&priv->pci_dev->dev,
> -					"Illegal SMBus block read size %d\n",
> -					len);
> -				/* Recover */
> -				while (inb_p(SMBHSTSTS(priv)) &
> -				       SMBHSTSTS_HOST_BUSY)
> -					outb_p(SMBHSTSTS_BYTE_DONE,
> -					       SMBHSTSTS(priv));
> -				outb_p(SMBHSTSTS_INTR, SMBHSTSTS(priv));
> +			priv->len = inb_p(SMBHSTDAT0(priv));
> +			if (priv->len < 1 || priv->len > I2C_SMBUS_BLOCK_MAX)
>  				return -EPROTO;
> -			}
> -			data->block[0] = len;
> +			data->block[0] = len = priv->len;

Not entirely sure why the priv->len changes are interleaved in this
patch. I might be missing something, but it looks like it should not be
there or in a separate patch.

Cheers,
Benjamin

>  		}
>  
>  		/* Retrieve/store value in SMBBLKDAT */
> @@ -634,9 +629,7 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
>  		outb_p(SMBHSTSTS_BYTE_DONE, SMBHSTSTS(priv));
>  	}
>  
> -	status = i801_wait_intr(priv);
> -exit:
> -	return i801_check_post(priv, status);
> +	return i801_wait_intr(priv);
>  }
>  
>  static int i801_set_block_buffer_mode(struct i801_priv *priv)
> @@ -791,6 +784,12 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
>  	else
>  		ret = i801_transaction(priv, xact);
>  
> +	/*
> +	 * ret can be status if positive or an error if negative.  Let
> +	 * the post check handle it.
> +	 */
> +	ret = i801_check_post(priv, ret);
> +
>  	if (hostc >= 0)
>  		pci_write_config_byte(priv->pci_dev, SMBHSTCFG, hostc);
>  

Re: [v2,05/10] i2c-i801: Consolidate calls to i801_check_post

[Corey Minyard]

On 06/09/2016 05:03 AM, Benjamin Tissoires wrote:
> On May 29 2016 or thereabouts, Corey Minyard wrote:
>> From: Corey Minyard <cminyard@mvista.com>
>>
>> This was almost always called at the end of the transaction.  The
>> only time it wasn't called was when a protocol violation occurred,
>> and the error reporting was inconsistent there.
>>
>> So have the transaction functions return positive status or a
>> negative error if they detect an issue.  If a protocol violation
>> does occur, always do a kill operation on the bus instead of just
>> trying to finish the operation.  If there was a hardware issue,
>> the code that was there to terminate the transaction could loop
>> forever, and the kill seems more appropriate if something goes
>> grossly wrong.
>>
>> Signed-off-by: Corey Minyard <cminyard@mvista.com>
>> ---
> I *think* the patch is OK. I have a few comments.
>
>>   drivers/i2c/busses/i2c-i801.c | 51 +++++++++++++++++++++----------------------
>>   1 file changed, 25 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
>> index 8794e70..56db310 100644
>> --- a/drivers/i2c/busses/i2c-i801.c
>> +++ b/drivers/i2c/busses/i2c-i801.c
>> @@ -317,7 +317,13 @@ static int i801_check_post(struct i801_priv *priv, int status)
>>   	 * DEV_ERR.
>>   	 */
>>   	if (unlikely(status < 0)) {
>> -		dev_err(&priv->pci_dev->dev, "Transaction timeout\n");
>> +		result = status;
>> +		if (status == -EPROTO)
>> +			dev_err(&priv->pci_dev->dev,
>> +				"Illegal SMBus block read size %d\n",
>> +				priv->len);
> Not sure this is a good idea to map -EPROTO to "Illegal SMBus block read
> size". True, it looks like it can only happens this way, but I would
> rather see it mapped to "Protocol Error". Given that Protocol Error is
> not very interesting in itself, I'd rather keep the error messages in
> the callers only (except for ETIMEOUT where you can output an error
> message).

That would mean you would have exactly the same error print in
two (well, three if you count a future patch) different locations in the
code.  I moved it here to avoid that issue.  I agree that "Protocol
Error" is not that specific, and I don't see a clean way to communicate
this information.  So I think you are right.

>> +		else
>> +			dev_err(&priv->pci_dev->dev, "Transaction timeout\n");
>>   		/* try to stop the current command */
>>   		dev_dbg(&priv->pci_dev->dev, "Terminating the current operation\n");
>>   		outb_p(inb_p(SMBHSTCNT(priv)) | SMBHSTCNT_KILL,
>> @@ -333,7 +339,7 @@ static int i801_check_post(struct i801_priv *priv, int status)
>>   			dev_err(&priv->pci_dev->dev,
>>   				"Failed terminating the transaction\n");
>>   		outb_p(STATUS_FLAGS, SMBHSTSTS(priv));
>> -		return -ETIMEDOUT;
>> +		return result;
>>   	}
>>   
>>   	if (status & SMBHSTSTS_FAILED) {
>> @@ -414,15 +420,14 @@ static int i801_transaction(struct i801_priv *priv, int xact)
>>   				 "Timeout waiting for interrupt!\n");
> Here, the code already dev_warn the timeout and later i801_check_post
> will dev_err the timeout too. That's one too much IMO :)

Indeed, I didn't notice that.

>
>>   		}
>>   		priv->status = 0;
>> -		return i801_check_post(priv, status);
>> +		return status;
>>   	}
>>   
>>   	/* the current contents of SMBHSTCNT can be overwritten, since PEC,
>>   	 * SMBSCMD are passed in xact */
>>   	outb_p(xact | SMBHSTCNT_START, SMBHSTCNT(priv));
>>   
>> -	status = i801_wait_intr(priv);
>> -	return i801_check_post(priv, status);
>> +	return i801_wait_intr(priv);
>>   }
>>   
>>   static int i801_block_transaction_by_block(struct i801_priv *priv,
>> @@ -444,11 +449,11 @@ static int i801_block_transaction_by_block(struct i801_priv *priv,
>>   
>>   	status = i801_transaction(priv, I801_BLOCK_DATA |
>>   				  (hwpec ? SMBHSTCNT_PEC_EN : 0));
>> -	if (status)
>> +	if (status < 0 || status & STATUS_ERROR_FLAGS)
>>   		return status;
>>   
>>   	if (read_write == I2C_SMBUS_READ) {
>> -		len = inb_p(SMBHSTDAT0(priv));
>> +		len = priv->len = inb_p(SMBHSTDAT0(priv));
>>   		if (len < 1 || len > I2C_SMBUS_BLOCK_MAX)
>>   			return -EPROTO;
>>   
>> @@ -456,7 +461,7 @@ static int i801_block_transaction_by_block(struct i801_priv *priv,
>>   		for (i = 0; i < len; i++)
>>   			data->block[i + 1] = inb_p(SMBBLKDAT(priv));
>>   	}
>> -	return 0;
>> +	return status;
>>   }
>>   
>>   static void i801_isr_byte_done(struct i801_priv *priv)
>> @@ -590,7 +595,7 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
>>   				 "Timeout waiting for interrupt!\n");
>>   		}
>>   		priv->status = 0;
>> -		return i801_check_post(priv, status);
>> +		return status;
>>   	}
>>   
>>   	for (i = 1; i <= len; i++) {
>> @@ -604,24 +609,14 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
>>   
>>   		status = i801_wait_byte_done(priv);
>>   		if (status)
>> -			goto exit;
>> +			return status;
>>   
>>   		if (i == 1 && read_write == I2C_SMBUS_READ
>>   		 && command != I2C_SMBUS_I2C_BLOCK_DATA) {
>> -			len = inb_p(SMBHSTDAT0(priv));
>> -			if (len < 1 || len > I2C_SMBUS_BLOCK_MAX) {
>> -				dev_err(&priv->pci_dev->dev,
>> -					"Illegal SMBus block read size %d\n",
>> -					len);
>> -				/* Recover */
>> -				while (inb_p(SMBHSTSTS(priv)) &
>> -				       SMBHSTSTS_HOST_BUSY)
>> -					outb_p(SMBHSTSTS_BYTE_DONE,
>> -					       SMBHSTSTS(priv));
>> -				outb_p(SMBHSTSTS_INTR, SMBHSTSTS(priv));
>> +			priv->len = inb_p(SMBHSTDAT0(priv));
>> +			if (priv->len < 1 || priv->len > I2C_SMBUS_BLOCK_MAX)
>>   				return -EPROTO;
>> -			}
>> -			data->block[0] = len;
>> +			data->block[0] = len = priv->len;
> Not entirely sure why the priv->len changes are interleaved in this
> patch. I might be missing something, but it looks like it should not be
> there or in a separate patch.

This was so the code reporting the error in the check_post function could
print out the length value.  If I move the logs to where the error is 
detected,
the priv->len setting here will no longer be necessary.

Thanks,

-corey

> Cheers,
> Benjamin
>
>>   		}
>>   
>>   		/* Retrieve/store value in SMBBLKDAT */
>> @@ -634,9 +629,7 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
>>   		outb_p(SMBHSTSTS_BYTE_DONE, SMBHSTSTS(priv));
>>   	}
>>   
>> -	status = i801_wait_intr(priv);
>> -exit:
>> -	return i801_check_post(priv, status);
>> +	return i801_wait_intr(priv);
>>   }
>>   
>>   static int i801_set_block_buffer_mode(struct i801_priv *priv)
>> @@ -791,6 +784,12 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
>>   	else
>>   		ret = i801_transaction(priv, xact);
>>   
>> +	/*
>> +	 * ret can be status if positive or an error if negative.  Let
>> +	 * the post check handle it.
>> +	 */
>> +	ret = i801_check_post(priv, ret);
>> +
>>   	if (hostc >= 0)
>>   		pci_write_config_byte(priv->pci_dev, SMBHSTCFG, hostc);
>>   

[PATCH v2 06/10] i2c-i801: Pass around a boolean read/write variable

[minyard]

From: Corey Minyard <cminyard@mvista.com>

The code was passing around read_write, which required comparison
with a constant, but was effectively a bool.  Pass around an
is_read bool instead.  This also makes it consistent with the
priv->is_read used for interrupt handling.

Signed-off-by: Corey Minyard <cminyard@mvista.com>
---
 drivers/i2c/busses/i2c-i801.c | 51 ++++++++++++++++++++-----------------------
 1 file changed, 24 insertions(+), 27 deletions(-)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 56db310..ae1e60a 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -432,7 +432,7 @@ static int i801_transaction(struct i801_priv *priv, int xact)
 
 static int i801_block_transaction_by_block(struct i801_priv *priv,
 					   union i2c_smbus_data *data,
-					   char read_write, int hwpec)
+					   bool is_read, int hwpec)
 {
 	int i, len;
 	int status;
@@ -440,7 +440,7 @@ static int i801_block_transaction_by_block(struct i801_priv *priv,
 	inb_p(SMBHSTCNT(priv)); /* reset the data buffer index */
 
 	/* Use 32-byte buffer to process this transaction */
-	if (read_write == I2C_SMBUS_WRITE) {
+	if (!is_read) {
 		len = data->block[0];
 		outb_p(len, SMBHSTDAT0(priv));
 		for (i = 0; i < len; i++)
@@ -452,7 +452,7 @@ static int i801_block_transaction_by_block(struct i801_priv *priv,
 	if (status < 0 || status & STATUS_ERROR_FLAGS)
 		return status;
 
-	if (read_write == I2C_SMBUS_READ) {
+	if (is_read) {
 		len = priv->len = inb_p(SMBHSTDAT0(priv));
 		if (len < 1 || len > I2C_SMBUS_BLOCK_MAX)
 			return -EPROTO;
@@ -555,7 +555,7 @@ static irqreturn_t i801_isr(int irq, void *dev_id)
  */
 static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
 					       union i2c_smbus_data *data,
-					       char read_write, int command)
+					       bool is_read, int command)
 {
 	int i, len;
 	int smbcmd;
@@ -565,20 +565,19 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
 
 	len = data->block[0];
 
-	if (read_write == I2C_SMBUS_WRITE) {
+	if (!is_read) {
 		outb_p(len, SMBHSTDAT0(priv));
 		outb_p(data->block[1], SMBBLKDAT(priv));
 	}
 
-	if (command == I2C_SMBUS_I2C_BLOCK_DATA &&
-	    read_write == I2C_SMBUS_READ)
+	if (command == I2C_SMBUS_I2C_BLOCK_DATA && is_read)
 		smbcmd = I801_I2C_BLOCK_DATA;
 	else
 		smbcmd = I801_BLOCK_DATA;
 
 	if (priv->features & FEATURE_IRQ) {
-		priv->is_read = (read_write == I2C_SMBUS_READ);
-		if (len == 1 && priv->is_read)
+		priv->is_read = is_read;
+		if (len == 1 && is_read)
 			smbcmd |= SMBHSTCNT_LAST_BYTE;
 		priv->cmd = smbcmd | SMBHSTCNT_INTREN;
 		priv->len = len;
@@ -599,7 +598,7 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
 	}
 
 	for (i = 1; i <= len; i++) {
-		if (i == len && read_write == I2C_SMBUS_READ)
+		if (i == len && is_read)
 			smbcmd |= SMBHSTCNT_LAST_BYTE;
 		outb_p(smbcmd, SMBHSTCNT(priv));
 
@@ -611,7 +610,7 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
 		if (status)
 			return status;
 
-		if (i == 1 && read_write == I2C_SMBUS_READ
+		if (i == 1 && is_read
 		 && command != I2C_SMBUS_I2C_BLOCK_DATA) {
 			priv->len = inb_p(SMBHSTDAT0(priv));
 			if (priv->len < 1 || priv->len > I2C_SMBUS_BLOCK_MAX)
@@ -620,9 +619,9 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
 		}
 
 		/* Retrieve/store value in SMBBLKDAT */
-		if (read_write == I2C_SMBUS_READ)
+		if (is_read)
 			data->block[i] = inb_p(SMBBLKDAT(priv));
-		if (read_write == I2C_SMBUS_WRITE && i+1 <= len)
+		else if (i+1 <= len)
 			outb_p(data->block[i+1], SMBBLKDAT(priv));
 
 		/* signals SMBBLKDAT ready */
@@ -642,7 +641,7 @@ static int i801_set_block_buffer_mode(struct i801_priv *priv)
 
 /* Block transaction function */
 static int i801_block_transaction(struct i801_priv *priv, unsigned short flags,
-				  union i2c_smbus_data *data, char read_write,
+				  union i2c_smbus_data *data, bool is_read,
 				  int command)
 {
 	int result = 0;
@@ -658,8 +657,7 @@ static int i801_block_transaction(struct i801_priv *priv, unsigned short flags,
 		outb_p(inb_p(SMBAUXCTL(priv)) & (~SMBAUXCTL_CRC),
 		       SMBAUXCTL(priv));
 
-	if (read_write == I2C_SMBUS_WRITE
-	 || command == I2C_SMBUS_I2C_BLOCK_DATA) {
+	if (!is_read || command == I2C_SMBUS_I2C_BLOCK_DATA) {
 		if (data->block[0] < 1)
 			data->block[0] = 1;
 		if (data->block[0] > I2C_SMBUS_BLOCK_MAX)
@@ -674,12 +672,11 @@ static int i801_block_transaction(struct i801_priv *priv, unsigned short flags,
 	if ((priv->features & FEATURE_BLOCK_BUFFER)
 	 && command != I2C_SMBUS_I2C_BLOCK_DATA
 	 && i801_set_block_buffer_mode(priv) == 0)
-		result = i801_block_transaction_by_block(priv, data,
-							 read_write, hwpec);
+		result = i801_block_transaction_by_block(priv, data, is_read,
+							 hwpec);
 	else
 		result = i801_block_transaction_byte_by_byte(priv, data,
-							     read_write,
-							     command);
+							     is_read, command);
 
 	/*
 	 * Some BIOSes don't like it when PEC is enabled at reboot or
@@ -702,6 +699,7 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
 	int block = 0;
 	int ret = 0, xact = 0;
 	int hostc = -1;
+	bool is_read = (read_write == I2C_SMBUS_READ);
 	struct i801_priv *priv = i2c_get_adapdata(adap);
 
 	pm_runtime_get_sync(&priv->pci_dev->dev);
@@ -715,7 +713,7 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
 	case I2C_SMBUS_BYTE:
 		outb_p(((addr & 0x7f) << 1) | (read_write & 0x01),
 		       SMBHSTADD(priv));
-		if (read_write == I2C_SMBUS_WRITE)
+		if (!is_read)
 			outb_p(command, SMBHSTCMD(priv));
 		xact = I801_BYTE;
 		break;
@@ -723,7 +721,7 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
 		outb_p(((addr & 0x7f) << 1) | (read_write & 0x01),
 		       SMBHSTADD(priv));
 		outb_p(command, SMBHSTCMD(priv));
-		if (read_write == I2C_SMBUS_WRITE)
+		if (!is_read)
 			outb_p(data->byte, SMBHSTDAT0(priv));
 		xact = I801_BYTE_DATA;
 		break;
@@ -731,7 +729,7 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
 		outb_p(((addr & 0x7f) << 1) | (read_write & 0x01),
 		       SMBHSTADD(priv));
 		outb_p(command, SMBHSTCMD(priv));
-		if (read_write == I2C_SMBUS_WRITE) {
+		if (!is_read) {
 			outb_p(data->word & 0xff, SMBHSTDAT0(priv));
 			outb_p((data->word & 0xff00) >> 8, SMBHSTDAT1(priv));
 		}
@@ -747,7 +745,7 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
 		/* NB: page 240 of ICH5 datasheet shows that the R/#W
 		 * bit should be cleared here, even when reading */
 		outb_p((addr & 0x7f) << 1, SMBHSTADD(priv));
-		if (read_write == I2C_SMBUS_WRITE) {
+		if (!is_read) {
 			unsigned char thostc;
 
 			outb_p(command, SMBHSTCMD(priv));
@@ -779,8 +777,7 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
 		goto out;
 
 	if (block)
-		ret = i801_block_transaction(priv, flags, data, read_write,
-					     size);
+		ret = i801_block_transaction(priv, flags, data, is_read, size);
 	else
 		ret = i801_transaction(priv, xact);
 
@@ -797,7 +794,7 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
 		goto out;
 	if (ret)
 		goto out;
-	if ((read_write == I2C_SMBUS_WRITE) || (xact == I801_QUICK))
+	if (!is_read || (xact == I801_QUICK))
 		goto out;
 
 	switch (xact & 0x7f) {
-- 
2.7.4

Re: [v2,06/10] i2c-i801: Pass around a boolean read/write variable

[Benjamin Tissoires]

On May 29 2016 or thereabouts, Corey Minyard wrote:
> From: Corey Minyard <cminyard@mvista.com>
> 
> The code was passing around read_write, which required comparison
> with a constant, but was effectively a bool.  Pass around an
> is_read bool instead.  This also makes it consistent with the
> priv->is_read used for interrupt handling.
> 
> Signed-off-by: Corey Minyard <cminyard@mvista.com>
> ---

The rough idea of the patch looks good to me, but I don't know if the
authors of i2c-i801.c did want to keep the char to be closer to the
SMBus protocol and be able to inject this char in the pipe directly.

So no strong opinion on this one.

Cheers,
Benjamin

>  drivers/i2c/busses/i2c-i801.c | 51 ++++++++++++++++++++-----------------------
>  1 file changed, 24 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index 56db310..ae1e60a 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -432,7 +432,7 @@ static int i801_transaction(struct i801_priv *priv, int xact)
>  
>  static int i801_block_transaction_by_block(struct i801_priv *priv,
>  					   union i2c_smbus_data *data,
> -					   char read_write, int hwpec)
> +					   bool is_read, int hwpec)
>  {
>  	int i, len;
>  	int status;
> @@ -440,7 +440,7 @@ static int i801_block_transaction_by_block(struct i801_priv *priv,
>  	inb_p(SMBHSTCNT(priv)); /* reset the data buffer index */
>  
>  	/* Use 32-byte buffer to process this transaction */
> -	if (read_write == I2C_SMBUS_WRITE) {
> +	if (!is_read) {
>  		len = data->block[0];
>  		outb_p(len, SMBHSTDAT0(priv));
>  		for (i = 0; i < len; i++)
> @@ -452,7 +452,7 @@ static int i801_block_transaction_by_block(struct i801_priv *priv,
>  	if (status < 0 || status & STATUS_ERROR_FLAGS)
>  		return status;
>  
> -	if (read_write == I2C_SMBUS_READ) {
> +	if (is_read) {
>  		len = priv->len = inb_p(SMBHSTDAT0(priv));
>  		if (len < 1 || len > I2C_SMBUS_BLOCK_MAX)
>  			return -EPROTO;
> @@ -555,7 +555,7 @@ static irqreturn_t i801_isr(int irq, void *dev_id)
>   */
>  static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
>  					       union i2c_smbus_data *data,
> -					       char read_write, int command)
> +					       bool is_read, int command)
>  {
>  	int i, len;
>  	int smbcmd;
> @@ -565,20 +565,19 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
>  
>  	len = data->block[0];
>  
> -	if (read_write == I2C_SMBUS_WRITE) {
> +	if (!is_read) {
>  		outb_p(len, SMBHSTDAT0(priv));
>  		outb_p(data->block[1], SMBBLKDAT(priv));
>  	}
>  
> -	if (command == I2C_SMBUS_I2C_BLOCK_DATA &&
> -	    read_write == I2C_SMBUS_READ)
> +	if (command == I2C_SMBUS_I2C_BLOCK_DATA && is_read)
>  		smbcmd = I801_I2C_BLOCK_DATA;
>  	else
>  		smbcmd = I801_BLOCK_DATA;
>  
>  	if (priv->features & FEATURE_IRQ) {
> -		priv->is_read = (read_write == I2C_SMBUS_READ);
> -		if (len == 1 && priv->is_read)
> +		priv->is_read = is_read;
> +		if (len == 1 && is_read)
>  			smbcmd |= SMBHSTCNT_LAST_BYTE;
>  		priv->cmd = smbcmd | SMBHSTCNT_INTREN;
>  		priv->len = len;
> @@ -599,7 +598,7 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
>  	}
>  
>  	for (i = 1; i <= len; i++) {
> -		if (i == len && read_write == I2C_SMBUS_READ)
> +		if (i == len && is_read)
>  			smbcmd |= SMBHSTCNT_LAST_BYTE;
>  		outb_p(smbcmd, SMBHSTCNT(priv));
>  
> @@ -611,7 +610,7 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
>  		if (status)
>  			return status;
>  
> -		if (i == 1 && read_write == I2C_SMBUS_READ
> +		if (i == 1 && is_read
>  		 && command != I2C_SMBUS_I2C_BLOCK_DATA) {
>  			priv->len = inb_p(SMBHSTDAT0(priv));
>  			if (priv->len < 1 || priv->len > I2C_SMBUS_BLOCK_MAX)
> @@ -620,9 +619,9 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
>  		}
>  
>  		/* Retrieve/store value in SMBBLKDAT */
> -		if (read_write == I2C_SMBUS_READ)
> +		if (is_read)
>  			data->block[i] = inb_p(SMBBLKDAT(priv));
> -		if (read_write == I2C_SMBUS_WRITE && i+1 <= len)
> +		else if (i+1 <= len)
>  			outb_p(data->block[i+1], SMBBLKDAT(priv));
>  
>  		/* signals SMBBLKDAT ready */
> @@ -642,7 +641,7 @@ static int i801_set_block_buffer_mode(struct i801_priv *priv)
>  
>  /* Block transaction function */
>  static int i801_block_transaction(struct i801_priv *priv, unsigned short flags,
> -				  union i2c_smbus_data *data, char read_write,
> +				  union i2c_smbus_data *data, bool is_read,
>  				  int command)
>  {
>  	int result = 0;
> @@ -658,8 +657,7 @@ static int i801_block_transaction(struct i801_priv *priv, unsigned short flags,
>  		outb_p(inb_p(SMBAUXCTL(priv)) & (~SMBAUXCTL_CRC),
>  		       SMBAUXCTL(priv));
>  
> -	if (read_write == I2C_SMBUS_WRITE
> -	 || command == I2C_SMBUS_I2C_BLOCK_DATA) {
> +	if (!is_read || command == I2C_SMBUS_I2C_BLOCK_DATA) {
>  		if (data->block[0] < 1)
>  			data->block[0] = 1;
>  		if (data->block[0] > I2C_SMBUS_BLOCK_MAX)
> @@ -674,12 +672,11 @@ static int i801_block_transaction(struct i801_priv *priv, unsigned short flags,
>  	if ((priv->features & FEATURE_BLOCK_BUFFER)
>  	 && command != I2C_SMBUS_I2C_BLOCK_DATA
>  	 && i801_set_block_buffer_mode(priv) == 0)
> -		result = i801_block_transaction_by_block(priv, data,
> -							 read_write, hwpec);
> +		result = i801_block_transaction_by_block(priv, data, is_read,
> +							 hwpec);
>  	else
>  		result = i801_block_transaction_byte_by_byte(priv, data,
> -							     read_write,
> -							     command);
> +							     is_read, command);
>  
>  	/*
>  	 * Some BIOSes don't like it when PEC is enabled at reboot or
> @@ -702,6 +699,7 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
>  	int block = 0;
>  	int ret = 0, xact = 0;
>  	int hostc = -1;
> +	bool is_read = (read_write == I2C_SMBUS_READ);
>  	struct i801_priv *priv = i2c_get_adapdata(adap);
>  
>  	pm_runtime_get_sync(&priv->pci_dev->dev);
> @@ -715,7 +713,7 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
>  	case I2C_SMBUS_BYTE:
>  		outb_p(((addr & 0x7f) << 1) | (read_write & 0x01),
>  		       SMBHSTADD(priv));
> -		if (read_write == I2C_SMBUS_WRITE)
> +		if (!is_read)
>  			outb_p(command, SMBHSTCMD(priv));
>  		xact = I801_BYTE;
>  		break;
> @@ -723,7 +721,7 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
>  		outb_p(((addr & 0x7f) << 1) | (read_write & 0x01),
>  		       SMBHSTADD(priv));
>  		outb_p(command, SMBHSTCMD(priv));
> -		if (read_write == I2C_SMBUS_WRITE)
> +		if (!is_read)
>  			outb_p(data->byte, SMBHSTDAT0(priv));
>  		xact = I801_BYTE_DATA;
>  		break;
> @@ -731,7 +729,7 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
>  		outb_p(((addr & 0x7f) << 1) | (read_write & 0x01),
>  		       SMBHSTADD(priv));
>  		outb_p(command, SMBHSTCMD(priv));
> -		if (read_write == I2C_SMBUS_WRITE) {
> +		if (!is_read) {
>  			outb_p(data->word & 0xff, SMBHSTDAT0(priv));
>  			outb_p((data->word & 0xff00) >> 8, SMBHSTDAT1(priv));
>  		}
> @@ -747,7 +745,7 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
>  		/* NB: page 240 of ICH5 datasheet shows that the R/#W
>  		 * bit should be cleared here, even when reading */
>  		outb_p((addr & 0x7f) << 1, SMBHSTADD(priv));
> -		if (read_write == I2C_SMBUS_WRITE) {
> +		if (!is_read) {
>  			unsigned char thostc;
>  
>  			outb_p(command, SMBHSTCMD(priv));
> @@ -779,8 +777,7 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
>  		goto out;
>  
>  	if (block)
> -		ret = i801_block_transaction(priv, flags, data, read_write,
> -					     size);
> +		ret = i801_block_transaction(priv, flags, data, is_read, size);
>  	else
>  		ret = i801_transaction(priv, xact);
>  
> @@ -797,7 +794,7 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
>  		goto out;
>  	if (ret)
>  		goto out;
> -	if ((read_write == I2C_SMBUS_WRITE) || (xact == I801_QUICK))
> +	if (!is_read || (xact == I801_QUICK))
>  		goto out;
>  
>  	switch (xact & 0x7f) {

[PATCH v2 07/10] i2c-i801: Fix some inconsistent variable names

[minyard]

From: Corey Minyard <cminyard@mvista.com>

The priv->cmd is called subcmd elsewhere, and that's a more
appropriate name for it, so rename it.

The "size" parameter passed in to i801_access is passed to other
functions and those name is "command".  This is confusing with the
"command" parameter passed in to i801_access.  Rename it to "size"
everywhere.

Signed-off-by: Corey Minyard <cminyard@mvista.com>
---
 drivers/i2c/busses/i2c-i801.c | 29 ++++++++++++++---------------
 1 file changed, 14 insertions(+), 15 deletions(-)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index ae1e60a..b415948 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -233,7 +233,7 @@ struct i801_priv {
 	u8 status;
 
 	/* Command state used by isr for byte-by-byte block transactions */
-	u8 cmd;
+	u8 subcmd;
 	bool is_read;
 	int count;
 	int len;
@@ -468,7 +468,7 @@ static void i801_isr_byte_done(struct i801_priv *priv)
 {
 	if (priv->is_read) {
 		/* For SMBus block reads, length is received with first byte */
-		if (((priv->cmd & 0x1c) == I801_BLOCK_DATA) &&
+		if (((priv->subcmd & 0x1c) == I801_BLOCK_DATA) &&
 		    (priv->count == 0)) {
 			priv->len = inb_p(SMBHSTDAT0(priv));
 			if (priv->len < 1 || priv->len > I2C_SMBUS_BLOCK_MAX) {
@@ -494,7 +494,7 @@ static void i801_isr_byte_done(struct i801_priv *priv)
 
 		/* Set LAST_BYTE for last byte of read transaction */
 		if (priv->count == priv->len - 1)
-			outb_p(priv->cmd | SMBHSTCNT_LAST_BYTE,
+			outb_p(priv->subcmd | SMBHSTCNT_LAST_BYTE,
 			       SMBHSTCNT(priv));
 	} else if (priv->count < priv->len - 1) {
 		/* Write next byte, except for IRQ after last byte */
@@ -555,7 +555,7 @@ static irqreturn_t i801_isr(int irq, void *dev_id)
  */
 static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
 					       union i2c_smbus_data *data,
-					       bool is_read, int command)
+					       bool is_read, int size)
 {
 	int i, len;
 	int smbcmd;
@@ -570,7 +570,7 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
 		outb_p(data->block[1], SMBBLKDAT(priv));
 	}
 
-	if (command == I2C_SMBUS_I2C_BLOCK_DATA && is_read)
+	if (size == I2C_SMBUS_I2C_BLOCK_DATA && is_read)
 		smbcmd = I801_I2C_BLOCK_DATA;
 	else
 		smbcmd = I801_BLOCK_DATA;
@@ -579,12 +579,12 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
 		priv->is_read = is_read;
 		if (len == 1 && is_read)
 			smbcmd |= SMBHSTCNT_LAST_BYTE;
-		priv->cmd = smbcmd | SMBHSTCNT_INTREN;
+		priv->subcmd = smbcmd | SMBHSTCNT_INTREN;
 		priv->len = len;
 		priv->count = 0;
 		priv->data = &data->block[1];
 
-		outb_p(priv->cmd | SMBHSTCNT_START, SMBHSTCNT(priv));
+		outb_p(priv->subcmd | SMBHSTCNT_START, SMBHSTCNT(priv));
 		result = wait_event_timeout(priv->waitq,
 					    (status = priv->status),
 					    adap->timeout);
@@ -610,8 +610,7 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
 		if (status)
 			return status;
 
-		if (i == 1 && is_read
-		 && command != I2C_SMBUS_I2C_BLOCK_DATA) {
+		if (i == 1 && is_read && size != I2C_SMBUS_I2C_BLOCK_DATA) {
 			priv->len = inb_p(SMBHSTDAT0(priv));
 			if (priv->len < 1 || priv->len > I2C_SMBUS_BLOCK_MAX)
 				return -EPROTO;
@@ -642,13 +641,13 @@ static int i801_set_block_buffer_mode(struct i801_priv *priv)
 /* Block transaction function */
 static int i801_block_transaction(struct i801_priv *priv, unsigned short flags,
 				  union i2c_smbus_data *data, bool is_read,
-				  int command)
+				  int size)
 {
 	int result = 0;
 	int hwpec = (priv->features & FEATURE_SMBUS_PEC)
 		&& (flags & I2C_CLIENT_PEC)
-		&& command != I2C_SMBUS_QUICK
-		&& command != I2C_SMBUS_I2C_BLOCK_DATA;
+		&& size != I2C_SMBUS_QUICK
+		&& size != I2C_SMBUS_I2C_BLOCK_DATA;
 
 	if (hwpec)	/* enable/disable hardware PEC */
 		outb_p(inb_p(SMBAUXCTL(priv)) | SMBAUXCTL_CRC,
@@ -657,7 +656,7 @@ static int i801_block_transaction(struct i801_priv *priv, unsigned short flags,
 		outb_p(inb_p(SMBAUXCTL(priv)) & (~SMBAUXCTL_CRC),
 		       SMBAUXCTL(priv));
 
-	if (!is_read || command == I2C_SMBUS_I2C_BLOCK_DATA) {
+	if (!is_read || size == I2C_SMBUS_I2C_BLOCK_DATA) {
 		if (data->block[0] < 1)
 			data->block[0] = 1;
 		if (data->block[0] > I2C_SMBUS_BLOCK_MAX)
@@ -670,13 +669,13 @@ static int i801_block_transaction(struct i801_priv *priv, unsigned short flags,
 	   SMBus (not I2C) block transactions, even though the datasheet
 	   doesn't mention this limitation. */
 	if ((priv->features & FEATURE_BLOCK_BUFFER)
-	 && command != I2C_SMBUS_I2C_BLOCK_DATA
+	 && size != I2C_SMBUS_I2C_BLOCK_DATA
 	 && i801_set_block_buffer_mode(priv) == 0)
 		result = i801_block_transaction_by_block(priv, data, is_read,
 							 hwpec);
 	else
 		result = i801_block_transaction_byte_by_byte(priv, data,
-							     is_read, command);
+							     is_read, size);
 
 	/*
 	 * Some BIOSes don't like it when PEC is enabled at reboot or
-- 
2.7.4

Re: [v2,07/10] i2c-i801: Fix some inconsistent variable names

[Benjamin Tissoires]

On May 29 2016 or thereabouts, Corey Minyard wrote:
> From: Corey Minyard <cminyard@mvista.com>
> 
> The priv->cmd is called subcmd elsewhere, and that's a more
> appropriate name for it, so rename it.
> 
> The "size" parameter passed in to i801_access is passed to other
> functions and those name is "command".  This is confusing with the
> "command" parameter passed in to i801_access.  Rename it to "size"
> everywhere.

Well, this parameter contains defines that are declared as SMBus
transaction types in i2c.h. So even if they are passed as the size
parameter in i2c_access, they actually are commands. So I am not sure it
is good to rename them as "size".

Cheers,
Benjamin

> 
> Signed-off-by: Corey Minyard <cminyard@mvista.com>
> ---
>  drivers/i2c/busses/i2c-i801.c | 29 ++++++++++++++---------------
>  1 file changed, 14 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index ae1e60a..b415948 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -233,7 +233,7 @@ struct i801_priv {
>  	u8 status;
>  
>  	/* Command state used by isr for byte-by-byte block transactions */
> -	u8 cmd;
> +	u8 subcmd;
>  	bool is_read;
>  	int count;
>  	int len;
> @@ -468,7 +468,7 @@ static void i801_isr_byte_done(struct i801_priv *priv)
>  {
>  	if (priv->is_read) {
>  		/* For SMBus block reads, length is received with first byte */
> -		if (((priv->cmd & 0x1c) == I801_BLOCK_DATA) &&
> +		if (((priv->subcmd & 0x1c) == I801_BLOCK_DATA) &&
>  		    (priv->count == 0)) {
>  			priv->len = inb_p(SMBHSTDAT0(priv));
>  			if (priv->len < 1 || priv->len > I2C_SMBUS_BLOCK_MAX) {
> @@ -494,7 +494,7 @@ static void i801_isr_byte_done(struct i801_priv *priv)
>  
>  		/* Set LAST_BYTE for last byte of read transaction */
>  		if (priv->count == priv->len - 1)
> -			outb_p(priv->cmd | SMBHSTCNT_LAST_BYTE,
> +			outb_p(priv->subcmd | SMBHSTCNT_LAST_BYTE,
>  			       SMBHSTCNT(priv));
>  	} else if (priv->count < priv->len - 1) {
>  		/* Write next byte, except for IRQ after last byte */
> @@ -555,7 +555,7 @@ static irqreturn_t i801_isr(int irq, void *dev_id)
>   */
>  static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
>  					       union i2c_smbus_data *data,
> -					       bool is_read, int command)
> +					       bool is_read, int size)
>  {
>  	int i, len;
>  	int smbcmd;
> @@ -570,7 +570,7 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
>  		outb_p(data->block[1], SMBBLKDAT(priv));
>  	}
>  
> -	if (command == I2C_SMBUS_I2C_BLOCK_DATA && is_read)
> +	if (size == I2C_SMBUS_I2C_BLOCK_DATA && is_read)
>  		smbcmd = I801_I2C_BLOCK_DATA;
>  	else
>  		smbcmd = I801_BLOCK_DATA;
> @@ -579,12 +579,12 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
>  		priv->is_read = is_read;
>  		if (len == 1 && is_read)
>  			smbcmd |= SMBHSTCNT_LAST_BYTE;
> -		priv->cmd = smbcmd | SMBHSTCNT_INTREN;
> +		priv->subcmd = smbcmd | SMBHSTCNT_INTREN;
>  		priv->len = len;
>  		priv->count = 0;
>  		priv->data = &data->block[1];
>  
> -		outb_p(priv->cmd | SMBHSTCNT_START, SMBHSTCNT(priv));
> +		outb_p(priv->subcmd | SMBHSTCNT_START, SMBHSTCNT(priv));
>  		result = wait_event_timeout(priv->waitq,
>  					    (status = priv->status),
>  					    adap->timeout);
> @@ -610,8 +610,7 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
>  		if (status)
>  			return status;
>  
> -		if (i == 1 && is_read
> -		 && command != I2C_SMBUS_I2C_BLOCK_DATA) {
> +		if (i == 1 && is_read && size != I2C_SMBUS_I2C_BLOCK_DATA) {
>  			priv->len = inb_p(SMBHSTDAT0(priv));
>  			if (priv->len < 1 || priv->len > I2C_SMBUS_BLOCK_MAX)
>  				return -EPROTO;
> @@ -642,13 +641,13 @@ static int i801_set_block_buffer_mode(struct i801_priv *priv)
>  /* Block transaction function */
>  static int i801_block_transaction(struct i801_priv *priv, unsigned short flags,
>  				  union i2c_smbus_data *data, bool is_read,
> -				  int command)
> +				  int size)
>  {
>  	int result = 0;
>  	int hwpec = (priv->features & FEATURE_SMBUS_PEC)
>  		&& (flags & I2C_CLIENT_PEC)
> -		&& command != I2C_SMBUS_QUICK
> -		&& command != I2C_SMBUS_I2C_BLOCK_DATA;
> +		&& size != I2C_SMBUS_QUICK
> +		&& size != I2C_SMBUS_I2C_BLOCK_DATA;
>  
>  	if (hwpec)	/* enable/disable hardware PEC */
>  		outb_p(inb_p(SMBAUXCTL(priv)) | SMBAUXCTL_CRC,
> @@ -657,7 +656,7 @@ static int i801_block_transaction(struct i801_priv *priv, unsigned short flags,
>  		outb_p(inb_p(SMBAUXCTL(priv)) & (~SMBAUXCTL_CRC),
>  		       SMBAUXCTL(priv));
>  
> -	if (!is_read || command == I2C_SMBUS_I2C_BLOCK_DATA) {
> +	if (!is_read || size == I2C_SMBUS_I2C_BLOCK_DATA) {
>  		if (data->block[0] < 1)
>  			data->block[0] = 1;
>  		if (data->block[0] > I2C_SMBUS_BLOCK_MAX)
> @@ -670,13 +669,13 @@ static int i801_block_transaction(struct i801_priv *priv, unsigned short flags,
>  	   SMBus (not I2C) block transactions, even though the datasheet
>  	   doesn't mention this limitation. */
>  	if ((priv->features & FEATURE_BLOCK_BUFFER)
> -	 && command != I2C_SMBUS_I2C_BLOCK_DATA
> +	 && size != I2C_SMBUS_I2C_BLOCK_DATA
>  	 && i801_set_block_buffer_mode(priv) == 0)
>  		result = i801_block_transaction_by_block(priv, data, is_read,
>  							 hwpec);
>  	else
>  		result = i801_block_transaction_byte_by_byte(priv, data,
> -							     is_read, command);
> +							     is_read, size);
>  
>  	/*
>  	 * Some BIOSes don't like it when PEC is enabled at reboot or

Re: [v2,07/10] i2c-i801: Fix some inconsistent variable names

[Corey Minyard]

On 06/09/2016 09:01 AM, Benjamin Tissoires wrote:
> On May 29 2016 or thereabouts, Corey Minyard wrote:
>> From: Corey Minyard <cminyard@mvista.com>
>>
>> The priv->cmd is called subcmd elsewhere, and that's a more
>> appropriate name for it, so rename it.
>>
>> The "size" parameter passed in to i801_access is passed to other
>> functions and those name is "command".  This is confusing with the
>> "command" parameter passed in to i801_access.  Rename it to "size"
>> everywhere.
> Well, this parameter contains defines that are declared as SMBus
> transaction types in i2c.h. So even if they are passed as the size
> parameter in i2c_access, they actually are commands. So I am not sure it
> is good to rename them as "size".

I looked in the user interfaces of the I2C core, and it's named "protocol"
there.  That's a much better name than "size", so I'll use it instead.

-corey

> Cheers,
> Benjamin
>
>> Signed-off-by: Corey Minyard <cminyard@mvista.com>
>> ---
>>   drivers/i2c/busses/i2c-i801.c | 29 ++++++++++++++---------------
>>   1 file changed, 14 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
>> index ae1e60a..b415948 100644
>> --- a/drivers/i2c/busses/i2c-i801.c
>> +++ b/drivers/i2c/busses/i2c-i801.c
>> @@ -233,7 +233,7 @@ struct i801_priv {
>>   	u8 status;
>>   
>>   	/* Command state used by isr for byte-by-byte block transactions */
>> -	u8 cmd;
>> +	u8 subcmd;
>>   	bool is_read;
>>   	int count;
>>   	int len;
>> @@ -468,7 +468,7 @@ static void i801_isr_byte_done(struct i801_priv *priv)
>>   {
>>   	if (priv->is_read) {
>>   		/* For SMBus block reads, length is received with first byte */
>> -		if (((priv->cmd & 0x1c) == I801_BLOCK_DATA) &&
>> +		if (((priv->subcmd & 0x1c) == I801_BLOCK_DATA) &&
>>   		    (priv->count == 0)) {
>>   			priv->len = inb_p(SMBHSTDAT0(priv));
>>   			if (priv->len < 1 || priv->len > I2C_SMBUS_BLOCK_MAX) {
>> @@ -494,7 +494,7 @@ static void i801_isr_byte_done(struct i801_priv *priv)
>>   
>>   		/* Set LAST_BYTE for last byte of read transaction */
>>   		if (priv->count == priv->len - 1)
>> -			outb_p(priv->cmd | SMBHSTCNT_LAST_BYTE,
>> +			outb_p(priv->subcmd | SMBHSTCNT_LAST_BYTE,
>>   			       SMBHSTCNT(priv));
>>   	} else if (priv->count < priv->len - 1) {
>>   		/* Write next byte, except for IRQ after last byte */
>> @@ -555,7 +555,7 @@ static irqreturn_t i801_isr(int irq, void *dev_id)
>>    */
>>   static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
>>   					       union i2c_smbus_data *data,
>> -					       bool is_read, int command)
>> +					       bool is_read, int size)
>>   {
>>   	int i, len;
>>   	int smbcmd;
>> @@ -570,7 +570,7 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
>>   		outb_p(data->block[1], SMBBLKDAT(priv));
>>   	}
>>   
>> -	if (command == I2C_SMBUS_I2C_BLOCK_DATA && is_read)
>> +	if (size == I2C_SMBUS_I2C_BLOCK_DATA && is_read)
>>   		smbcmd = I801_I2C_BLOCK_DATA;
>>   	else
>>   		smbcmd = I801_BLOCK_DATA;
>> @@ -579,12 +579,12 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
>>   		priv->is_read = is_read;
>>   		if (len == 1 && is_read)
>>   			smbcmd |= SMBHSTCNT_LAST_BYTE;
>> -		priv->cmd = smbcmd | SMBHSTCNT_INTREN;
>> +		priv->subcmd = smbcmd | SMBHSTCNT_INTREN;
>>   		priv->len = len;
>>   		priv->count = 0;
>>   		priv->data = &data->block[1];
>>   
>> -		outb_p(priv->cmd | SMBHSTCNT_START, SMBHSTCNT(priv));
>> +		outb_p(priv->subcmd | SMBHSTCNT_START, SMBHSTCNT(priv));
>>   		result = wait_event_timeout(priv->waitq,
>>   					    (status = priv->status),
>>   					    adap->timeout);
>> @@ -610,8 +610,7 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
>>   		if (status)
>>   			return status;
>>   
>> -		if (i == 1 && is_read
>> -		 && command != I2C_SMBUS_I2C_BLOCK_DATA) {
>> +		if (i == 1 && is_read && size != I2C_SMBUS_I2C_BLOCK_DATA) {
>>   			priv->len = inb_p(SMBHSTDAT0(priv));
>>   			if (priv->len < 1 || priv->len > I2C_SMBUS_BLOCK_MAX)
>>   				return -EPROTO;
>> @@ -642,13 +641,13 @@ static int i801_set_block_buffer_mode(struct i801_priv *priv)
>>   /* Block transaction function */
>>   static int i801_block_transaction(struct i801_priv *priv, unsigned short flags,
>>   				  union i2c_smbus_data *data, bool is_read,
>> -				  int command)
>> +				  int size)
>>   {
>>   	int result = 0;
>>   	int hwpec = (priv->features & FEATURE_SMBUS_PEC)
>>   		&& (flags & I2C_CLIENT_PEC)
>> -		&& command != I2C_SMBUS_QUICK
>> -		&& command != I2C_SMBUS_I2C_BLOCK_DATA;
>> +		&& size != I2C_SMBUS_QUICK
>> +		&& size != I2C_SMBUS_I2C_BLOCK_DATA;
>>   
>>   	if (hwpec)	/* enable/disable hardware PEC */
>>   		outb_p(inb_p(SMBAUXCTL(priv)) | SMBAUXCTL_CRC,
>> @@ -657,7 +656,7 @@ static int i801_block_transaction(struct i801_priv *priv, unsigned short flags,
>>   		outb_p(inb_p(SMBAUXCTL(priv)) & (~SMBAUXCTL_CRC),
>>   		       SMBAUXCTL(priv));
>>   
>> -	if (!is_read || command == I2C_SMBUS_I2C_BLOCK_DATA) {
>> +	if (!is_read || size == I2C_SMBUS_I2C_BLOCK_DATA) {
>>   		if (data->block[0] < 1)
>>   			data->block[0] = 1;
>>   		if (data->block[0] > I2C_SMBUS_BLOCK_MAX)
>> @@ -670,13 +669,13 @@ static int i801_block_transaction(struct i801_priv *priv, unsigned short flags,
>>   	   SMBus (not I2C) block transactions, even though the datasheet
>>   	   doesn't mention this limitation. */
>>   	if ((priv->features & FEATURE_BLOCK_BUFFER)
>> -	 && command != I2C_SMBUS_I2C_BLOCK_DATA
>> +	 && size != I2C_SMBUS_I2C_BLOCK_DATA
>>   	 && i801_set_block_buffer_mode(priv) == 0)
>>   		result = i801_block_transaction_by_block(priv, data, is_read,
>>   							 hwpec);
>>   	else
>>   		result = i801_block_transaction_byte_by_byte(priv, data,
>> -							     is_read, command);
>> +							     is_read, size);
>>   
>>   	/*
>>   	 * Some BIOSes don't like it when PEC is enabled at reboot or

[PATCH v2 08/10] i2c-i801: Handle a protocol error in byte-by-byte isr

[minyard]

From: Corey Minyard <cminyard@mvista.com>

If a bad number of bytes is read on a transaction, have it
report an error and return -EPROTO for the transaction.

Signed-off-by: Corey Minyard <cminyard@mvista.com>
---
 drivers/i2c/busses/i2c-i801.c | 27 ++++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index b415948..70da60a 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -230,7 +230,7 @@ struct i801_priv {
 
 	/* isr processing */
 	wait_queue_head_t waitq;
-	u8 status;
+	int status;
 
 	/* Command state used by isr for byte-by-byte block transactions */
 	u8 subcmd;
@@ -472,16 +472,18 @@ static void i801_isr_byte_done(struct i801_priv *priv)
 		    (priv->count == 0)) {
 			priv->len = inb_p(SMBHSTDAT0(priv));
 			if (priv->len < 1 || priv->len > I2C_SMBUS_BLOCK_MAX) {
-				dev_err(&priv->pci_dev->dev,
-					"Illegal SMBus block read size %d\n",
-					priv->len);
-				/* FIXME: Recover */
-				priv->len = I2C_SMBUS_BLOCK_MAX;
-			} else {
-				dev_dbg(&priv->pci_dev->dev,
-					"SMBus block read size is %d\n",
-					priv->len);
+				/*
+				 * Terminate the transaction here, to avoid
+				 * getting any more interrupts that might
+				 * hold us in an interrupt loop forever.
+				 */
+				outb_p(inb_p(SMBHSTCNT(priv)) | SMBHSTCNT_KILL,
+				       SMBHSTCNT(priv));
+				priv->status = -EPROTO;
+				return;
 			}
+			dev_dbg(&priv->pci_dev->dev,
+				"SMBus block read size is %d\n", priv->len);
 			priv->data[-1] = priv->len;
 		}
 
@@ -541,7 +543,10 @@ static irqreturn_t i801_isr(int irq, void *dev_id)
 	status &= SMBHSTSTS_INTR | STATUS_ERROR_FLAGS;
 	if (status) {
 		outb_p(status, SMBHSTSTS(priv));
-		priv->status |= status;
+		if (status < 0)
+			priv->status = status;
+		else
+			priv->status |= status;
 		wake_up(&priv->waitq);
 	}
 
-- 
2.7.4

Re: [v2,08/10] i2c-i801: Handle a protocol error in byte-by-byte isr

[Benjamin Tissoires]

On May 29 2016 or thereabouts, Corey Minyard wrote:
> From: Corey Minyard <cminyard@mvista.com>
> 
> If a bad number of bytes is read on a transaction, have it
> report an error and return -EPROTO for the transaction.
> 
> Signed-off-by: Corey Minyard <cminyard@mvista.com>
> ---
>  drivers/i2c/busses/i2c-i801.c | 27 ++++++++++++++++-----------
>  1 file changed, 16 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index b415948..70da60a 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -230,7 +230,7 @@ struct i801_priv {
>  
>  	/* isr processing */
>  	wait_queue_head_t waitq;
> -	u8 status;
> +	int status;
>  
>  	/* Command state used by isr for byte-by-byte block transactions */
>  	u8 subcmd;
> @@ -472,16 +472,18 @@ static void i801_isr_byte_done(struct i801_priv *priv)
>  		    (priv->count == 0)) {
>  			priv->len = inb_p(SMBHSTDAT0(priv));
>  			if (priv->len < 1 || priv->len > I2C_SMBUS_BLOCK_MAX) {
> -				dev_err(&priv->pci_dev->dev,
> -					"Illegal SMBus block read size %d\n",
> -					priv->len);
> -				/* FIXME: Recover */
> -				priv->len = I2C_SMBUS_BLOCK_MAX;
> -			} else {
> -				dev_dbg(&priv->pci_dev->dev,
> -					"SMBus block read size is %d\n",
> -					priv->len);
> +				/*
> +				 * Terminate the transaction here, to avoid
> +				 * getting any more interrupts that might
> +				 * hold us in an interrupt loop forever.
> +				 */
> +				outb_p(inb_p(SMBHSTCNT(priv)) | SMBHSTCNT_KILL,
> +				       SMBHSTCNT(priv));
> +				priv->status = -EPROTO;
> +				return;
>  			}
> +			dev_dbg(&priv->pci_dev->dev,
> +				"SMBus block read size is %d\n", priv->len);
>  			priv->data[-1] = priv->len;
>  		}
>  
> @@ -541,7 +543,10 @@ static irqreturn_t i801_isr(int irq, void *dev_id)
>  	status &= SMBHSTSTS_INTR | STATUS_ERROR_FLAGS;
>  	if (status) {
>  		outb_p(status, SMBHSTSTS(priv));
> -		priv->status |= status;
> +		if (status < 0)
> +			priv->status = status;
> +		else
> +			priv->status |= status;

This hunk conflicts with http://patchwork.ozlabs.org/patch/626051/ and
could be removed if this mentioned patch is applied first.

The rest of the patch looks OK to me.

Cheers,
Benjamin

>  		wake_up(&priv->waitq);
>  	}
>  

[PATCH v2 09/10] i2c-i801: Null isr data buffer when done with it

[minyard]

From: Corey Minyard <cminyard@mvista.com>

Don't leave a pointer to some external buffer lying around.

Signed-off-by: Corey Minyard <cminyard@mvista.com>
---
 drivers/i2c/busses/i2c-i801.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 70da60a..bb15356 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -598,6 +598,7 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
 			dev_warn(&priv->pci_dev->dev,
 				 "Timeout waiting for interrupt!\n");
 		}
+		priv->data = NULL;
 		priv->status = 0;
 		return status;
 	}
-- 
2.7.4

Re: [v2,09/10] i2c-i801: Null isr data buffer when done with it

[Benjamin Tissoires]

On May 29 2016 or thereabouts, Corey Minyard wrote:
> From: Corey Minyard <cminyard@mvista.com>
> 
> Don't leave a pointer to some external buffer lying around.
> 
> Signed-off-by: Corey Minyard <cminyard@mvista.com>
> ---

Looks good to me.

Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

>  drivers/i2c/busses/i2c-i801.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index 70da60a..bb15356 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -598,6 +598,7 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
>  			dev_warn(&priv->pci_dev->dev,
>  				 "Timeout waiting for interrupt!\n");
>  		}
> +		priv->data = NULL;
>  		priv->status = 0;
>  		return status;
>  	}

[PATCH v2 10/10] i2c-i801: Only write the host control reg when necessary

[minyard]

From: Corey Minyard <cminyard@mvista.com>

When doing byte-by-byte mode, the code was writing the host
control register on every byte.  However, this is only necessary
on the first byte and for read transactions the last byte.
So only do it those times.

Signed-off-by: Corey Minyard <cminyard@mvista.com>
---
 drivers/i2c/busses/i2c-i801.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index bb15356..4a96a95 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -606,11 +606,11 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
 	for (i = 1; i <= len; i++) {
 		if (i == len && is_read)
 			smbcmd |= SMBHSTCNT_LAST_BYTE;
-		outb_p(smbcmd, SMBHSTCNT(priv));
 
 		if (i == 1)
-			outb_p(inb(SMBHSTCNT(priv)) | SMBHSTCNT_START,
-			       SMBHSTCNT(priv));
+			outb_p(smbcmd | SMBHSTCNT_START, SMBHSTCNT(priv));
+		else if (i == len && is_read)
+			outb_p(smbcmd, SMBHSTCNT(priv));
 
 		status = i801_wait_byte_done(priv);
 		if (status)
-- 
2.7.4