[PATCH 0/5]

[PATCH 0/5]

[minyard]

This is a small set of patches for the i801 I2C driver that are
mostly cleanups and consolidation.  No real behaviour change, except
not enabling PEC when it's not needed.

I have tested these pretty extensively on qemu with various configurations
and errors (I've hacked up qemu a bit to do this) and it seems to be ok.

Not sure if you are interested, but thought I would offer.

-corey

[PATCH 1/5] i2c-i801: hwpec and check_pre cleanups

[minyard]

From: Corey Minyard <cminyard@mvista.com>

Add an "xact_extra" variable and use it to carry the PEC enable and
interrupt flags .

Also move i801_check_pre() to i801_access()  That consolidates it to
one call, and there's no point in doing all the work if the hardware
isn't ready.

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

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index f62d697..62cf1e5 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -230,6 +230,7 @@ struct i801_priv {
 	/* isr processing */
 	wait_queue_head_t waitq;
 	u8 status;
+	u8 xact_extra; /* Used to set INTREN if irqs enabled, and HWPEC */
 
 	/* Command state used by isr for byte-by-byte block transactions */
 	u8 cmd;
@@ -401,13 +402,13 @@ 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;
+	/*
+	 * the current contents of SMBHSTCNT can be overwritten, since PEC,
+	 * SMBSCMD are passed in xact
+	 */
+	outb_p(xact | priv->xact_extra | SMBHSTCNT_START,  SMBHSTCNT(priv));
 
 	if (priv->features & FEATURE_IRQ) {
-		outb_p(xact | SMBHSTCNT_INTREN | SMBHSTCNT_START,
-		       SMBHSTCNT(priv));
 		result = wait_event_timeout(priv->waitq,
 					    (status = priv->status),
 					    adap->timeout);
@@ -420,17 +421,13 @@ static int i801_transaction(struct i801_priv *priv, int xact)
 		return i801_check_post(priv, 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);
 }
 
 static int i801_block_transaction_by_block(struct i801_priv *priv,
 					   union i2c_smbus_data *data,
-					   char read_write, int hwpec)
+					   char read_write)
 {
 	int i, len;
 	int status;
@@ -445,8 +442,7 @@ static int i801_block_transaction_by_block(struct i801_priv *priv,
 			outb_p(data->block[i+1], SMBBLKDAT(priv));
 	}
 
-	status = i801_transaction(priv, I801_BLOCK_DATA |
-				  (hwpec ? SMBHSTCNT_PEC_EN : 0));
+	status = i801_transaction(priv, I801_BLOCK_DATA);
 	if (status)
 		return status;
 
@@ -553,8 +549,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;
@@ -562,10 +557,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) {
@@ -583,7 +574,7 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
 		priv->is_read = (read_write == I2C_SMBUS_READ);
 		if (len == 1 && priv->is_read)
 			smbcmd |= SMBHSTCNT_LAST_BYTE;
-		priv->cmd = smbcmd | SMBHSTCNT_INTREN;
+		priv->cmd = smbcmd | priv->xact_extra;
 		priv->len = len;
 		priv->count = 0;
 		priv->data = &data->block[1];
@@ -691,13 +682,15 @@ static int i801_block_transaction(struct i801_priv *priv,
 	   doesn't mention this limitation. */
 	if ((priv->features & FEATURE_BLOCK_BUFFER)
 	 && command != I2C_SMBUS_I2C_BLOCK_DATA
-	 && i801_set_block_buffer_mode(priv) == 0)
+	 && i801_set_block_buffer_mode(priv) == 0) {
+		if (hwpec)
+			priv->xact_extra |= SMBHSTCNT_PEC_EN;
 		result = i801_block_transaction_by_block(priv, data,
-							 read_write, hwpec);
-	else
+							 read_write);
+	} 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) {
@@ -716,6 +709,7 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
 	int block = 0;
 	int ret, xact = 0;
 	struct i801_priv *priv = i2c_get_adapdata(adap);
+	int result;
 
 	hwpec = (priv->features & FEATURE_SMBUS_PEC) && (flags & I2C_CLIENT_PEC)
 		&& size != I2C_SMBUS_QUICK
@@ -776,11 +770,17 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
 		return -EOPNOTSUPP;
 	}
 
-	if (hwpec)	/* enable/disable hardware PEC */
+	result = i801_check_pre(priv);
+	if (result < 0)
+		return result;
+
+	if (hwpec) {	/* enable/disable hardware PEC */
 		outb_p(inb_p(SMBAUXCTL(priv)) | SMBAUXCTL_CRC, SMBAUXCTL(priv));
-	else
+	} else {
 		outb_p(inb_p(SMBAUXCTL(priv)) & (~SMBAUXCTL_CRC),
 		       SMBAUXCTL(priv));
+		priv->xact_extra &= ~SMBHSTCNT_PEC_EN;
+	}
 
 	if (block)
 		ret = i801_block_transaction(priv, data, read_write, size,
@@ -1381,6 +1381,7 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
 	}
 
 	if (priv->features & FEATURE_IRQ) {
+		priv->xact_extra |= SMBHSTCNT_INTREN;
 		init_waitqueue_head(&priv->waitq);
 
 		err = devm_request_irq(&dev->dev, dev->irq, i801_isr,
-- 
2.5.0

Re: [PATCH 1/5] i2c-i801: hwpec and check_pre cleanups

[Jean Delvare]

Hi Corey,

On Tue, 19 Jan 2016 11:09:33 -0600, minyard@acm.org wrote:
> From: Corey Minyard <cminyard@mvista.com>
> 
> Add an "xact_extra" variable and use it to carry the PEC enable and
> interrupt flags .

Which is a good thing, because...? This needs a justification. At this
point I just see an implementation change, with no reason to prefer one
implementation to the other. In such cases I prefer to leave things as
they are, to avoid introducing bugs.

Stray space in description above.

Is it just me or PEC handling is quite broken in this driver,
regardless of your patch? If I read the code correctly:
* hwpec isn't used for non-block transactions.
* i801_block_transaction_byte_by_byte() takes hwpec as a parameter but
  never uses it.
Which basically means that PEC is only really implemented for one-shot
block transactions, right?

> Also move i801_check_pre() to i801_access()  That consolidates it to
> one call, and there's no point in doing all the work if the hardware
> isn't ready.

Why do both changes in a single patch? They don't seem to be related.
These changes would be easier to review and integrate as separate
patches.

> 
> Signed-off-by: Corey Minyard <cminyard@mvista.com>
> ---
>  drivers/i2c/busses/i2c-i801.c | 51 ++++++++++++++++++++++---------------------
>  1 file changed, 26 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index f62d697..62cf1e5 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -230,6 +230,7 @@ struct i801_priv {
>  	/* isr processing */
>  	wait_queue_head_t waitq;
>  	u8 status;
> +	u8 xact_extra; /* Used to set INTREN if irqs enabled, and HWPEC */
>  
>  	/* Command state used by isr for byte-by-byte block transactions */
>  	u8 cmd;
> @@ -401,13 +402,13 @@ 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;
> +	/*
> +	 * the current contents of SMBHSTCNT can be overwritten, since PEC,
> +	 * SMBSCMD are passed in xact

This is no longer true, PEC is in priv->xact_extra after your changes,
not xact.

> +	 */
> +	outb_p(xact | priv->xact_extra | SMBHSTCNT_START,  SMBHSTCNT(priv));
>  
>  	if (priv->features & FEATURE_IRQ) {
> -		outb_p(xact | SMBHSTCNT_INTREN | SMBHSTCNT_START,
> -		       SMBHSTCNT(priv));
>  		result = wait_event_timeout(priv->waitq,
>  					    (status = priv->status),
>  					    adap->timeout);
> @@ -420,17 +421,13 @@ static int i801_transaction(struct i801_priv *priv, int xact)
>  		return i801_check_post(priv, 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);
>  }
>  
>  static int i801_block_transaction_by_block(struct i801_priv *priv,
>  					   union i2c_smbus_data *data,
> -					   char read_write, int hwpec)
> +					   char read_write)
>  {
>  	int i, len;
>  	int status;
> @@ -445,8 +442,7 @@ static int i801_block_transaction_by_block(struct i801_priv *priv,
>  			outb_p(data->block[i+1], SMBBLKDAT(priv));
>  	}
>  
> -	status = i801_transaction(priv, I801_BLOCK_DATA |
> -				  (hwpec ? SMBHSTCNT_PEC_EN : 0));
> +	status = i801_transaction(priv, I801_BLOCK_DATA);
>  	if (status)
>  		return status;
>  
> @@ -553,8 +549,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;
> @@ -562,10 +557,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) {
> @@ -583,7 +574,7 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
>  		priv->is_read = (read_write == I2C_SMBUS_READ);
>  		if (len == 1 && priv->is_read)
>  			smbcmd |= SMBHSTCNT_LAST_BYTE;
> -		priv->cmd = smbcmd | SMBHSTCNT_INTREN;
> +		priv->cmd = smbcmd | priv->xact_extra;
>  		priv->len = len;
>  		priv->count = 0;
>  		priv->data = &data->block[1];
> @@ -691,13 +682,15 @@ static int i801_block_transaction(struct i801_priv *priv,
>  	   doesn't mention this limitation. */
>  	if ((priv->features & FEATURE_BLOCK_BUFFER)
>  	 && command != I2C_SMBUS_I2C_BLOCK_DATA
> -	 && i801_set_block_buffer_mode(priv) == 0)
> +	 && i801_set_block_buffer_mode(priv) == 0) {
> +		if (hwpec)
> +			priv->xact_extra |= SMBHSTCNT_PEC_EN;
>  		result = i801_block_transaction_by_block(priv, data,
> -							 read_write, hwpec);
> -	else
> +							 read_write);
> +	} 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) {
> @@ -716,6 +709,7 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
>  	int block = 0;
>  	int ret, xact = 0;
>  	struct i801_priv *priv = i2c_get_adapdata(adap);
> +	int result;

You already have ret here which serves the same purpose, no need to
introduce another variable.

>  
>  	hwpec = (priv->features & FEATURE_SMBUS_PEC) && (flags & I2C_CLIENT_PEC)
>  		&& size != I2C_SMBUS_QUICK
> @@ -776,11 +770,17 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
>  		return -EOPNOTSUPP;
>  	}
>  
> -	if (hwpec)	/* enable/disable hardware PEC */
> +	result = i801_check_pre(priv);
> +	if (result < 0)
> +		return result;

At first I was worried that this move is breaking the symmetry with
i2c_check_post(), but now I see you do the same cleanup for
i2c_check_post() later in the series. Good.

So I have no objection to moving the call to i801_check_pre() earlier,
in the common path. But shouldn't it be even earlier? As you said,
there's no point in doing all the work if the controller is not ready.
Shouldn't i801_check_pre() be called first thing in i801_access()?

> +
> +	if (hwpec) {	/* enable/disable hardware PEC */
>  		outb_p(inb_p(SMBAUXCTL(priv)) | SMBAUXCTL_CRC, SMBAUXCTL(priv));
> -	else
> +	} else {
>  		outb_p(inb_p(SMBAUXCTL(priv)) & (~SMBAUXCTL_CRC),
>  		       SMBAUXCTL(priv));
> +		priv->xact_extra &= ~SMBHSTCNT_PEC_EN;
> +	}
>  
>  	if (block)
>  		ret = i801_block_transaction(priv, data, read_write, size,
> @@ -1381,6 +1381,7 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
>  	}
>  
>  	if (priv->features & FEATURE_IRQ) {
> +		priv->xact_extra |= SMBHSTCNT_INTREN;

A few lines below, we may disable FEATURE_IRQ if we failed to allocate
the irq. If this happens, your change leaves the driver in an
inconsistent state where priv->xact_extra contains SMBHSTCNT_INTREN but
interrupts should not be used.

>  		init_waitqueue_head(&priv->waitq);
>  
>  		err = devm_request_irq(&dev->dev, dev->irq, i801_isr,


-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 1/5] i2c-i801: hwpec and check_pre cleanups

[Jean Delvare]

Sorry, I accidentally sent the previous reply while I wasn't done with
the review yet.

On Tue, 19 Jan 2016 11:09:33 -0600, minyard@acm.org wrote:
> @@ -691,13 +682,15 @@ static int i801_block_transaction(struct i801_priv *priv,
>  	   doesn't mention this limitation. */
>  	if ((priv->features & FEATURE_BLOCK_BUFFER)
>  	 && command != I2C_SMBUS_I2C_BLOCK_DATA
> -	 && i801_set_block_buffer_mode(priv) == 0)
> +	 && i801_set_block_buffer_mode(priv) == 0) {
> +		if (hwpec)
> +			priv->xact_extra |= SMBHSTCNT_PEC_EN;
>  		result = i801_block_transaction_by_block(priv, data,
> -							 read_write, hwpec);
> -	else
> +							 read_write);
> +	} else
>  		result = i801_block_transaction_byte_by_byte(priv, data,
>  							     read_write,
> -							     command, hwpec);
> +							     command);
>  
> (...)
> @@ -776,11 +770,17 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
>  		return -EOPNOTSUPP;
>  	}
>  
> -	if (hwpec)	/* enable/disable hardware PEC */
> +	result = i801_check_pre(priv);
> +	if (result < 0)
> +		return result;
> +
> +	if (hwpec) {	/* enable/disable hardware PEC */
>  		outb_p(inb_p(SMBAUXCTL(priv)) | SMBAUXCTL_CRC, SMBAUXCTL(priv));
> -	else
> +	} else {
>  		outb_p(inb_p(SMBAUXCTL(priv)) & (~SMBAUXCTL_CRC),
>  		       SMBAUXCTL(priv));
> +		priv->xact_extra &= ~SMBHSTCNT_PEC_EN;
> +	}

I'm confused by this asymmetry. You clear the PEC flag here, but you
set it in i801_block_transaction() above. Originally the flag was set
in i801_block_transaction_by_block() (and didn't have to be cleared, as
it was temporary.)

With your implementation, PEC may or may not be enabled if a driver
asks for a non-block transaction with PEC. If this is the first
transaction then it will not be enabled (bug already existed before
your patch.) But if the previous transaction was a block transaction
with PEC then the flag will still be present, so PEC will still be
enabled. The previous implementation was wrong but at least it was
consistently so.

This makes me believe we should rather fix the bugs first, and then
look into cleaning up this part of the code.

If you start storing transaction-dependent information in struct
i801_priv, you must make sure that nothing will leak from one
transaction to the next. I still have to review the rest of your patch
series, but I don't think it makes sense to carry the PEC flag that way
if the rest of the transaction information is still passed as function
parameters.

-- 
Jean Delvare
SUSE L3 Support

[PATCH 2/5] 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.

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

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 62cf1e5..9143fcf 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -652,20 +652,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) {
@@ -692,11 +678,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;
 }
 
@@ -710,6 +691,7 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
 	int ret, xact = 0;
 	struct i801_priv *priv = i2c_get_adapdata(adap);
 	int result;
+	int hostc = -1;
 
 	hwpec = (priv->features & FEATURE_SMBUS_PEC) && (flags & I2C_CLIENT_PEC)
 		&& size != I2C_SMBUS_QUICK
@@ -757,9 +739,19 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
 		 * bit should be cleared here, even when reading */
 		outb_p((addr & 0x7f) << 1, SMBHSTADD(priv));
 		if (read_write == I2C_SMBUS_READ) {
+			unsigned char thostc;
 			/* NB: page 240 of ICH5 datasheet also shows
 			 * that DATA1 is the cmd field when reading */
 			outb_p(command, SMBHSTDAT1(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");
+			return -EOPNOTSUPP;
 		} else
 			outb_p(command, SMBHSTCMD(priv));
 		block = 1;
@@ -795,6 +787,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)
 		return ret;
 	if (ret)
-- 
2.5.0

Re: [PATCH 2/5] i2c-i801: Move hostcfg set/reset to i801_access()

[Jean Delvare]

On Tue, 19 Jan 2016 11:09:34 -0600, minyard@acm.org 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.
> 
> Signed-off-by: Corey Minyard <cminyard@mvista.com>
> ---
>  drivers/i2c/busses/i2c-i801.c | 33 ++++++++++++++-------------------
>  1 file changed, 14 insertions(+), 19 deletions(-)

Did you test this patch?

> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index 62cf1e5..9143fcf 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -652,20 +652,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) {

This is the write case...

> -			/* 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)) {

... and this is the read case.

> -			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) {
> @@ -692,11 +678,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;
>  }
>  
> @@ -710,6 +691,7 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
>  	int ret, xact = 0;
>  	struct i801_priv *priv = i2c_get_adapdata(adap);
>  	int result;
> +	int hostc = -1;
>  
>  	hwpec = (priv->features & FEATURE_SMBUS_PEC) && (flags & I2C_CLIENT_PEC)
>  		&& size != I2C_SMBUS_QUICK
> @@ -757,9 +739,19 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
>  		 * bit should be cleared here, even when reading */
>  		outb_p((addr & 0x7f) << 1, SMBHSTADD(priv));
>  		if (read_write == I2C_SMBUS_READ) {

However after your changes, this is the READ case...

> +			unsigned char thostc;
>  			/* NB: page 240 of ICH5 datasheet also shows
>  			 * that DATA1 is the cmd field when reading */
>  			outb_p(command, SMBHSTDAT1(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)) {

... and this is the WRITE case. Looks very wrong.

> +			dev_err(&priv->pci_dev->dev,
> +				"I2C block read is unsupported!\n");
> +			return -EOPNOTSUPP;
>  		} else
>  			outb_p(command, SMBHSTCMD(priv));
>  		block = 1;
> @@ -795,6 +787,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)
>  		return ret;
>  	if (ret)

More generally, I'm only half convinced that the change is beneficial.
Yes, you avoid duplicate tests in the block case. However this is at
the expense of an initialization and a test in the common path.

I do agree that testing for unsupported functionality should be moved
back into i801_access() for consistency, as this is where the general
test for unsupported transaction types is.

-- 
Jean Delvare
SUSE L3 Support

[PATCH 3/5] i2c-i801: Move PEC handling into i2c_block_transaction()

[minyard]

From: Corey Minyard <cminyard@mvista.com>

PEC is only used on real block transactions, moving it into the block
transaction code allows removal of some if statements and the proper
setting of SMBAUXCTL.  PEC was being set in the byte-by-byte block
transaction, though it wasn't valid in that situation.

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

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 9143fcf..7567a96 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -669,14 +669,31 @@ static int i801_block_transaction(struct i801_priv *priv,
 	if ((priv->features & FEATURE_BLOCK_BUFFER)
 	 && command != I2C_SMBUS_I2C_BLOCK_DATA
 	 && i801_set_block_buffer_mode(priv) == 0) {
-		if (hwpec)
+		if (hwpec) {	/* enable/disable hardware PEC */
+			outb_p(inb_p(SMBAUXCTL(priv)) | SMBAUXCTL_CRC,
+			       SMBAUXCTL(priv));
 			priv->xact_extra |= SMBHSTCNT_PEC_EN;
+		} else
+			outb_p(inb_p(SMBAUXCTL(priv)) & (~SMBAUXCTL_CRC),
+			       SMBAUXCTL(priv));
+
 		result = i801_block_transaction_by_block(priv, data,
 							 read_write);
-	} else
+	} else {
+		outb_p(inb_p(SMBAUXCTL(priv)) & (~SMBAUXCTL_CRC),
+		       SMBAUXCTL(priv));
 		result = i801_block_transaction_byte_by_byte(priv, data,
 							     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.
+	 */
+	outb_p(inb_p(SMBAUXCTL(priv)) &
+	       ~(SMBAUXCTL_CRC | SMBAUXCTL_E32B), SMBAUXCTL(priv));
 
 	return result;
 }
@@ -686,17 +703,12 @@ 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, xact = 0;
 	struct i801_priv *priv = i2c_get_adapdata(adap);
 	int result;
 	int hostc = -1;
 
-	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),
@@ -766,26 +778,20 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
 	if (result < 0)
 		return result;
 
-	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));
-		priv->xact_extra &= ~SMBHSTCNT_PEC_EN;
-	}
+	priv->xact_extra &= ~SMBHSTCNT_PEC_EN;
+	if (block) {
+		int hwpec = (priv->features & FEATURE_SMBUS_PEC) &&
+			(flags & I2C_CLIENT_PEC)
+			&& size != I2C_SMBUS_QUICK
+			&& size != I2C_SMBUS_I2C_BLOCK_DATA;
 
-	if (block)
 		ret = i801_block_transaction(priv, data, read_write, size,
 					     hwpec);
-	else
+	} else {
+		outb_p(inb_p(SMBAUXCTL(priv)) & (~SMBAUXCTL_CRC),
+		       SMBAUXCTL(priv));
 		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.5.0

Re: [PATCH 3/5] i2c-i801: Move PEC handling into i2c_block_transaction()

[Jean Delvare]

Hi Corey,

On Tue, 19 Jan 2016 11:09:35 -0600, minyard@acm.org wrote:
> From: Corey Minyard <cminyard@mvista.com>
> 
> PEC is only used on real block transactions, moving it into the block
> transaction code allows removal of some if statements and the proper
> setting of SMBAUXCTL.  PEC was being set in the byte-by-byte block
> transaction, though it wasn't valid in that situation.

First thing to do would be check what the hardware actually supports.
The driver is indeed only handling PEC properly for one-shot block
transactions. That does not mean the hardware itself doesn't support
PEC in other cases. I'm almost certain that PEC is supported for
non-block transactions (except SMBus Quick Command.) Not sure about
byte-by-byte block transactions, but this should be tested first.

Cleaning up broken code makes little sense, as we may have to undo some
of the cleanups to get the driver to work properly.

And no, I don't think "PEC was being set in the byte-by-byte block
transaction". What was being set is the "automatically append CRC" bit
in AUX_CTL, but my understanding is that this bit has no effect when
SMBHSTCNT_PEC_EN itself isn't set.

-- 
Jean Delvare
SUSE L3 Support

[PATCH 4/5] i2c-i801: clean up block transaction

[minyard]

From: Corey Minyard <cminyard@mvista.com>

The block transaction code looked a little messy, pull out the
code to copy to/from the buffer into separate functions to make
it a little neater.

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

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 7567a96..840656c 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -425,37 +425,51 @@ static int i801_transaction(struct i801_priv *priv, int xact)
 	return i801_check_post(priv, status);
 }
 
+static void i801_write_block(struct i801_priv *priv, union i2c_smbus_data *data)
+{
+	int i, len;
+
+	len = data->block[0];
+	outb_p(len, SMBHSTDAT0(priv));
+	for (i = 0; i < len; i++)
+		outb_p(data->block[i+1], SMBBLKDAT(priv));
+}
+
+static int i801_read_block(struct i801_priv *priv, union i2c_smbus_data *data)
+{
+	int i, len;
+
+	len = inb_p(SMBHSTDAT0(priv));
+	if (len < 1 || len > I2C_SMBUS_BLOCK_MAX)
+		return -EPROTO;
+
+	data->block[0] = len;
+	for (i = 0; i < len; i++)
+		data->block[i + 1] = inb_p(SMBBLKDAT(priv));
+
+	return 0;
+}
+
 static int i801_block_transaction_by_block(struct i801_priv *priv,
 					   union i2c_smbus_data *data,
 					   char read_write)
 {
-	int i, len;
-	int status;
+	int result;
 
 	inb_p(SMBHSTCNT(priv)); /* reset the data buffer index */
 
 	/* Use 32-byte buffer to process this transaction */
-	if (read_write == I2C_SMBUS_WRITE) {
-		len = data->block[0];
-		outb_p(len, SMBHSTDAT0(priv));
-		for (i = 0; i < len; i++)
-			outb_p(data->block[i+1], SMBBLKDAT(priv));
-	}
+	if (read_write == I2C_SMBUS_WRITE)
+		i801_write_block(priv, data);
 
-	status = i801_transaction(priv, I801_BLOCK_DATA);
-	if (status)
-		return status;
+	result = i801_transaction(priv, I801_BLOCK_DATA);
+	if (result)
+		return result;
 
-	if (read_write == I2C_SMBUS_READ) {
-		len = inb_p(SMBHSTDAT0(priv));
-		if (len < 1 || len > I2C_SMBUS_BLOCK_MAX)
-			return -EPROTO;
+	if (read_write == I2C_SMBUS_READ)
+		result = i801_read_block(priv, data);
 
-		data->block[0] = len;
-		for (i = 0; i < len; i++)
-			data->block[i + 1] = inb_p(SMBBLKDAT(priv));
-	}
-	return 0;
+	return result;
 }
 
 static void i801_isr_byte_done(struct i801_priv *priv)
-- 
2.5.0

Re: [PATCH 4/5] i2c-i801: clean up block transaction

[Jean Delvare]

On Tue, 19 Jan 2016 11:09:36 -0600, minyard@acm.org wrote:
> From: Corey Minyard <cminyard@mvista.com>
> 
> The block transaction code looked a little messy, pull out the
> code to copy to/from the buffer into separate functions to make
> it a little neater.

A matter of taste I suppose. I don't see anything messy in the code as
it exists. A 32-line function doing a single thing is just fine with me.

> 
> Signed-off-by: Corey Minyard <cminyard@mvista.com>
> ---
>  drivers/i2c/busses/i2c-i801.c | 54 +++++++++++++++++++++++++++----------------
>  1 file changed, 34 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index 7567a96..840656c 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -425,37 +425,51 @@ static int i801_transaction(struct i801_priv *priv, int xact)
>  	return i801_check_post(priv, status);
>  }
>  
> +static void i801_write_block(struct i801_priv *priv, union i2c_smbus_data *data)
> +{
> +	int i, len;
> +
> +	len = data->block[0];
> +	outb_p(len, SMBHSTDAT0(priv));
> +	for (i = 0; i < len; i++)
> +		outb_p(data->block[i+1], SMBBLKDAT(priv));
> +}
> +
> +static int i801_read_block(struct i801_priv *priv, union i2c_smbus_data *data)
> +{
> +	int i, len;
> +
> +	len = inb_p(SMBHSTDAT0(priv));
> +	if (len < 1 || len > I2C_SMBUS_BLOCK_MAX)
> +		return -EPROTO;
> +
> +	data->block[0] = len;
> +	for (i = 0; i < len; i++)
> +		data->block[i + 1] = inb_p(SMBBLKDAT(priv));
> +
> +	return 0;
> +}
> +
>  static int i801_block_transaction_by_block(struct i801_priv *priv,
>  					   union i2c_smbus_data *data,
>  					   char read_write)
>  {
> -	int i, len;
> -	int status;
> +	int result;
>  
>  	inb_p(SMBHSTCNT(priv)); /* reset the data buffer index */
>  
>  	/* Use 32-byte buffer to process this transaction */
> -	if (read_write == I2C_SMBUS_WRITE) {
> -		len = data->block[0];
> -		outb_p(len, SMBHSTDAT0(priv));
> -		for (i = 0; i < len; i++)
> -			outb_p(data->block[i+1], SMBBLKDAT(priv));
> -	}
> +	if (read_write == I2C_SMBUS_WRITE)
> +		i801_write_block(priv, data);
>  
> -	status = i801_transaction(priv, I801_BLOCK_DATA);
> -	if (status)
> -		return status;
> +	result = i801_transaction(priv, I801_BLOCK_DATA);
> +	if (result)
> +		return result;
>  
> -	if (read_write == I2C_SMBUS_READ) {
> -		len = inb_p(SMBHSTDAT0(priv));
> -		if (len < 1 || len > I2C_SMBUS_BLOCK_MAX)
> -			return -EPROTO;
> +	if (read_write == I2C_SMBUS_READ)
> +		result = i801_read_block(priv, data);
>  
> -		data->block[0] = len;
> -		for (i = 0; i < len; i++)
> -			data->block[i + 1] = inb_p(SMBBLKDAT(priv));
> -	}
> -	return 0;
> +	return result;
>  }
>  
>  static void i801_isr_byte_done(struct i801_priv *priv)


-- 
Jean Delvare
SUSE L3 Support

[PATCH 5/5] i2c-i801: Remove redundant code and event-drive

[minyard]

From: Corey Minyard <cminyard@mvista.com>

There was a lot of redundant code in the driver, basically one copy
for the polled interface and one for the interrupt-driven interface.
Only use the interrupt-driven interface and use a timer to check
the interface periodically and time things out.

This also adds timeouts when using interrupt, so a failed operation
won't hang forever.

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

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 840656c..ed528e2 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -100,6 +100,8 @@
 #include <linux/gpio.h>
 #include <linux/i2c-mux-gpio.h>
 #endif
+#include <linux/spinlock.h>
+#include <linux/hrtimer.h>
 
 /* I801 SMBus address offsets */
 #define SMBHSTSTS(p)	(0 + (p)->smba)
@@ -148,6 +150,8 @@
 #define SMBAUXCTL_E32B		2
 
 /* Other settings */
+#define I801_TIMEOUT_NS		250000
+#define I801_TIMEOUT_DELTA_NS	250000
 #define MAX_RETRIES		400
 
 /* I801 command constants */
@@ -227,17 +231,25 @@ struct i801_priv {
 	struct pci_dev *pci_dev;
 	unsigned int features;
 
-	/* isr processing */
+	/* isr/timer processing */
 	wait_queue_head_t waitq;
 	u8 status;
 	u8 xact_extra; /* Used to set INTREN if irqs enabled, and HWPEC */
+	struct hrtimer timer;
+	long timeout;
+	int retries;
+	int done;
+	spinlock_t lock;
 
-	/* Command state used by isr for byte-by-byte block transactions */
+	/* Command state */
 	u8 cmd;
+	u8 xact;
 	bool is_read;
+	int byte_by_byte;
+	int block;
 	int count;
+	int hostc;
 	int len;
-	u8 *data;
 
 #if (defined CONFIG_I2C_MUX_GPIO || defined CONFIG_I2C_MUX_GPIO_MODULE) && \
 		defined CONFIG_DMI
@@ -245,6 +257,7 @@ struct i801_priv {
 	struct platform_device *mux_pdev;
 #endif
 	struct platform_device *tco_pdev;
+	union i2c_smbus_data *data;
 };
 
 #define FEATURE_SMBUS_PEC	(1 << 0)
@@ -272,16 +285,76 @@ MODULE_PARM_DESC(disable_features, "Disable selected driver features:\n"
 	"\t\t  0x08  disable the I2C block read functionality\n"
 	"\t\t  0x10  don't use interrupts ");
 
+static void i801_op_done(struct i801_priv *priv);
+
+static void i801_write_block(struct i801_priv *priv, union i2c_smbus_data *data)
+{
+	int i, len;
+
+	len = data->block[0];
+	outb_p(len, SMBHSTDAT0(priv));
+	for (i = 0; i < len; i++)
+		outb_p(data->block[i+1], SMBBLKDAT(priv));
+}
+
+static int i801_read_block(struct i801_priv *priv, union i2c_smbus_data *data)
+{
+	int i, len;
+
+	len = inb_p(SMBHSTDAT0(priv));
+	if (len < 1 || len > I2C_SMBUS_BLOCK_MAX)
+		return -EPROTO;
+
+	data->block[0] = len;
+	for (i = 0; i < len; i++)
+		data->block[i + 1] = inb_p(SMBBLKDAT(priv));
+
+	return 0;
+}
+
+static int i801_terminate_transaction(struct i801_priv *priv)
+{
+	int status, ret = 0, i;
+
+	dev_dbg(&priv->pci_dev->dev, "Terminating the current operation\n");
+
+	/* Flush the input buffer */
+	for (i = 0; i < 32; i++)
+		inb_p(SMBBLKDAT(priv));
+
+	status = inb_p(SMBHSTSTS(priv));
+	if (!(status & SMBHSTSTS_HOST_BUSY))
+		return 0;
+
+	outb_p(inb_p(SMBHSTCNT(priv)) | SMBHSTCNT_KILL, SMBHSTCNT(priv));
+	usleep_range(1000, 2000);
+	outb_p(inb_p(SMBHSTCNT(priv)) & (~SMBHSTCNT_KILL), SMBHSTCNT(priv));
+
+	/* Check if it worked */
+	status = inb_p(SMBHSTSTS(priv));
+	if ((status & SMBHSTSTS_HOST_BUSY) || !(status & SMBHSTSTS_FAILED)) {
+		dev_err(&priv->pci_dev->dev,
+			"Failed terminating the transaction\n");
+		ret = -EAGAIN;
+	}
+	outb_p(STATUS_FLAGS, SMBHSTSTS(priv));
+	return ret;
+}
+
 /* Make sure the SMBus host is ready to start transmitting.
    Return 0 if it is, -EBUSY if it is not. */
 static int i801_check_pre(struct i801_priv *priv)
 {
-	int status;
+	int status, ret;
 
 	status = inb_p(SMBHSTSTS(priv));
 	if (status & SMBHSTSTS_HOST_BUSY) {
-		dev_err(&priv->pci_dev->dev, "SMBus is busy, can't use it!\n");
-		return -EBUSY;
+		ret = i801_terminate_transaction(priv);
+		if (ret) {
+			dev_err(&priv->pci_dev->dev,
+				"SMBus is busy, can't use it!\n");
+			return -EBUSY;
+		}
 	}
 
 	status &= STATUS_FLAGS;
@@ -306,174 +379,101 @@ static int i801_check_pre(struct i801_priv *priv)
  * Note that status only contains the bits we want to clear, not the
  * actual register value.
  */
-static int i801_check_post(struct i801_priv *priv, int status)
+static int i801_check_post(struct i801_priv *priv)
 {
 	int result = 0;
 
+	hrtimer_cancel(&priv->timer);
+
 	/*
 	 * If the SMBus is still busy, we give up
 	 * Note: This timeout condition only happens when using polling
 	 * transactions.  For interrupt operation, NAK/timeout is indicated by
 	 * DEV_ERR.
 	 */
-	if (unlikely(status < 0)) {
+	if (unlikely(priv->status < 0)) {
 		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,
-		       SMBHSTCNT(priv));
-		usleep_range(1000, 2000);
-		outb_p(inb_p(SMBHSTCNT(priv)) & (~SMBHSTCNT_KILL),
-		       SMBHSTCNT(priv));
-
-		/* Check if it worked */
-		status = inb_p(SMBHSTSTS(priv));
-		if ((status & SMBHSTSTS_HOST_BUSY) ||
-		    !(status & SMBHSTSTS_FAILED))
-			dev_err(&priv->pci_dev->dev,
-				"Failed terminating the transaction\n");
-		outb_p(STATUS_FLAGS, SMBHSTSTS(priv));
-		return -ETIMEDOUT;
+		i801_terminate_transaction(priv);
+		result = -ETIMEDOUT;
+		goto out;
 	}
 
-	if (status & SMBHSTSTS_FAILED) {
+	if (priv->status & SMBHSTSTS_FAILED) {
 		result = -EIO;
 		dev_err(&priv->pci_dev->dev, "Transaction failed\n");
 	}
-	if (status & SMBHSTSTS_DEV_ERR) {
+	if (priv->status & SMBHSTSTS_DEV_ERR) {
 		result = -ENXIO;
 		dev_dbg(&priv->pci_dev->dev, "No response\n");
 	}
-	if (status & SMBHSTSTS_BUS_ERR) {
+	if (priv->status & SMBHSTSTS_BUS_ERR) {
 		result = -EAGAIN;
 		dev_dbg(&priv->pci_dev->dev, "Lost arbitration\n");
 	}
 
 	/* Clear status flags except BYTE_DONE, to be cleared by caller */
-	outb_p(status, SMBHSTSTS(priv));
+	outb_p(priv->status, SMBHSTSTS(priv));
 
-	return result;
-}
+	if (!result && priv->block && priv->is_read && !priv->byte_by_byte)
+		result = i801_read_block(priv, priv->data);
 
-/* Wait for BUSY being cleared and either INTR or an error flag being set */
-static int i801_wait_intr(struct i801_priv *priv)
-{
-	int timeout = 0;
-	int status;
-
-	/* We will always wait for a fraction of a second! */
-	do {
-		usleep_range(250, 500);
-		status = inb_p(SMBHSTSTS(priv));
-	} while (((status & SMBHSTSTS_HOST_BUSY) ||
-		  !(status & (STATUS_ERROR_FLAGS | SMBHSTSTS_INTR))) &&
-		 (timeout++ < MAX_RETRIES));
-
-	if (timeout > MAX_RETRIES) {
-		dev_dbg(&priv->pci_dev->dev, "INTR Timeout!\n");
-		return -ETIMEDOUT;
-	}
-	return status & (STATUS_ERROR_FLAGS | SMBHSTSTS_INTR);
-}
+	/*
+	 * 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.
+	 */
+	outb_p(inb_p(SMBAUXCTL(priv)) &
+	       ~(SMBAUXCTL_CRC | SMBAUXCTL_E32B), SMBAUXCTL(priv));
 
-/* Wait for either BYTE_DONE or an error flag being set */
-static int i801_wait_byte_done(struct i801_priv *priv)
-{
-	int timeout = 0;
-	int status;
-
-	/* We will always wait for a fraction of a second! */
-	do {
-		usleep_range(250, 500);
-		status = inb_p(SMBHSTSTS(priv));
-	} while (!(status & (STATUS_ERROR_FLAGS | SMBHSTSTS_BYTE_DONE)) &&
-		 (timeout++ < MAX_RETRIES));
-
-	if (timeout > MAX_RETRIES) {
-		dev_dbg(&priv->pci_dev->dev, "BYTE_DONE Timeout!\n");
-		return -ETIMEDOUT;
+	if (priv->hostc >= 0)
+		pci_write_config_byte(priv->pci_dev, SMBHSTCFG, priv->hostc);
+
+	if (priv->block)
+		goto out;
+	if (result)
+		goto out;
+	if (!priv->is_read || (priv->xact == I801_QUICK))
+		goto out;
+
+	switch (priv->xact & 0x7f) {
+	case I801_BYTE:	/* Result put in SMBHSTDAT0 */
+	case I801_BYTE_DATA:
+		priv->data->byte = inb_p(SMBHSTDAT0(priv));
+		break;
+	case I801_WORD_DATA:
+		priv->data->word = inb_p(SMBHSTDAT0(priv)) +
+			     (inb_p(SMBHSTDAT1(priv)) << 8);
+		break;
 	}
-	return status & STATUS_ERROR_FLAGS;
+out:
+	return result;
 }
 
-static int i801_transaction(struct i801_priv *priv, int xact)
+static void i801_transaction(struct i801_priv *priv, int xact)
 {
-	int status;
-	int result;
-	const struct i2c_adapter *adap = &priv->adapter;
-
 	/*
 	 * the current contents of SMBHSTCNT can be overwritten, since PEC,
 	 * SMBSCMD are passed in xact
 	 */
 	outb_p(xact | priv->xact_extra | SMBHSTCNT_START,  SMBHSTCNT(priv));
-
-	if (priv->features & FEATURE_IRQ) {
-		result = wait_event_timeout(priv->waitq,
-					    (status = priv->status),
-					    adap->timeout);
-		if (!result) {
-			status = -ETIMEDOUT;
-			dev_warn(&priv->pci_dev->dev,
-				 "Timeout waiting for interrupt!\n");
-		}
-		priv->status = 0;
-		return i801_check_post(priv, status);
-	}
-
-	status = i801_wait_intr(priv);
-	return i801_check_post(priv, status);
 }
 
-static void i801_write_block(struct i801_priv *priv, union i2c_smbus_data *data)
+static void i801_block_transaction_by_block(struct i801_priv *priv)
 {
-	int i, len;
-
-	len = data->block[0];
-	outb_p(len, SMBHSTDAT0(priv));
-	for (i = 0; i < len; i++)
-		outb_p(data->block[i+1], SMBBLKDAT(priv));
-}
-
-static int i801_read_block(struct i801_priv *priv, union i2c_smbus_data *data)
-{
-	int i, len;
-
-	len = inb_p(SMBHSTDAT0(priv));
-	if (len < 1 || len > I2C_SMBUS_BLOCK_MAX)
-		return -EPROTO;
-
-	data->block[0] = len;
-	for (i = 0; i < len; i++)
-		data->block[i + 1] = inb_p(SMBBLKDAT(priv));
-
-	return 0;
-}
-
-static int i801_block_transaction_by_block(struct i801_priv *priv,
-					   union i2c_smbus_data *data,
-					   char read_write)
-{
-	int result;
-
 	inb_p(SMBHSTCNT(priv)); /* reset the data buffer index */
 
 	/* Use 32-byte buffer to process this transaction */
-	if (read_write == I2C_SMBUS_WRITE)
-		i801_write_block(priv, data);
-
-	result = i801_transaction(priv, I801_BLOCK_DATA);
-	if (result)
-		return result;
+	if (!priv->is_read)
+		i801_write_block(priv, priv->data);
 
-	if (read_write == I2C_SMBUS_READ)
-		result = i801_read_block(priv, data);
-
-	return result;
+	i801_transaction(priv, I801_BLOCK_DATA);
 }
 
-static void i801_isr_byte_done(struct i801_priv *priv)
+static void i801_byte_done(struct i801_priv *priv)
 {
+	u8 *data = &priv->data->block[1];
+
 	if (priv->is_read) {
 		/* For SMBus block reads, length is received with first byte */
 		if (((priv->cmd & 0x1c) == I801_BLOCK_DATA) &&
@@ -490,12 +490,12 @@ static void i801_isr_byte_done(struct i801_priv *priv)
 					"SMBus block read size is %d\n",
 					priv->len);
 			}
-			priv->data[-1] = priv->len;
+			data[-1] = priv->len;
 		}
 
 		/* Read next byte */
 		if (priv->count < priv->len)
-			priv->data[priv->count++] = inb(SMBBLKDAT(priv));
+			data[priv->count++] = inb(SMBBLKDAT(priv));
 		else
 			dev_dbg(&priv->pci_dev->dev,
 				"Discarding extra byte on block read\n");
@@ -506,13 +506,58 @@ static void i801_isr_byte_done(struct i801_priv *priv)
 			       SMBHSTCNT(priv));
 	} else if (priv->count < priv->len - 1) {
 		/* Write next byte, except for IRQ after last byte */
-		outb_p(priv->data[++priv->count], SMBBLKDAT(priv));
+		outb_p(data[++priv->count], SMBBLKDAT(priv));
 	}
 
 	/* Clear BYTE_DONE to continue with next byte */
 	outb_p(SMBHSTSTS_BYTE_DONE, SMBHSTSTS(priv));
 }
 
+static int i801_check(struct i801_priv *priv, u8 status)
+{
+	if (status & SMBHSTSTS_BYTE_DONE)
+		i801_byte_done(priv);
+
+	/*
+	 * Clear irq sources and report transaction result.
+	 * ->status must be cleared before the next transaction is started.
+	 */
+	status &= SMBHSTSTS_INTR | STATUS_ERROR_FLAGS;
+	if (status) {
+		outb_p(status, SMBHSTSTS(priv));
+		priv->status |= status;
+		i801_op_done(priv);
+		return 1;
+	}
+	return 0;
+}
+
+static enum hrtimer_restart i801_timeout(struct hrtimer *hrtimer)
+{
+	struct i801_priv *priv = container_of(hrtimer, struct i801_priv, timer);
+	u8 status = inb_p(SMBHSTSTS(priv));
+	enum hrtimer_restart ret = HRTIMER_NORESTART;
+	unsigned long flags;
+
+	spin_lock_irqsave(&priv->lock, flags);
+	if (!priv->done && !i801_check(priv, status)) {
+		priv->retries--;
+		if (priv->retries > 0) {
+			hrtimer_set_expires_range(&priv->timer,
+				ktime_add_ns(ktime_get(), priv->timeout),
+				ns_to_ktime(I801_TIMEOUT_DELTA_NS));
+			ret = HRTIMER_RESTART;
+		} else {
+			dev_dbg(&priv->pci_dev->dev, "timeout, status = %02x\n",
+				status);
+			priv->status = -1;
+			i801_op_done(priv);
+		}
+	}
+	spin_unlock_irqrestore(&priv->lock, flags);
+	return ret;
+}
+
 /*
  * There are two kinds of interrupts:
  *
@@ -539,19 +584,9 @@ static irqreturn_t i801_isr(int irq, void *dev_id)
 		return IRQ_NONE;
 
 	status = inb_p(SMBHSTSTS(priv));
-	if (status & SMBHSTSTS_BYTE_DONE)
-		i801_isr_byte_done(priv);
-
-	/*
-	 * Clear irq sources and report transaction result.
-	 * ->status must be cleared before the next transaction is started.
-	 */
-	status &= SMBHSTSTS_INTR | STATUS_ERROR_FLAGS;
-	if (status) {
-		outb_p(status, SMBHSTSTS(priv));
-		priv->status |= status;
-		wake_up(&priv->waitq);
-	}
+	spin_lock(&priv->lock);
+	i801_check(priv, status);
+	spin_unlock(&priv->lock);
 
 	return IRQ_HANDLED;
 }
@@ -561,95 +596,31 @@ static irqreturn_t i801_isr(int irq, void *dev_id)
  *   I2C write uses cmd=I801_BLOCK_DATA, I2C_EN=1
  *   I2C read uses cmd=I801_I2C_BLOCK_DATA
  */
-static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
-					       union i2c_smbus_data *data,
-					       char read_write, int command)
+static void i801_block_transaction_byte_by_byte(struct i801_priv *priv,
+						int command)
 {
-	int i, len;
+	int len;
 	int smbcmd;
-	int status;
-	int result;
-	const struct i2c_adapter *adap = &priv->adapter;
 
-	len = data->block[0];
+	len = priv->data->block[0];
 
-	if (read_write == I2C_SMBUS_WRITE) {
+	if (!priv->is_read) {
 		outb_p(len, SMBHSTDAT0(priv));
-		outb_p(data->block[1], SMBBLKDAT(priv));
+		outb_p(priv->data->block[1], SMBBLKDAT(priv));
 	}
 
-	if (command == I2C_SMBUS_I2C_BLOCK_DATA &&
-	    read_write == I2C_SMBUS_READ)
+	if (command == I2C_SMBUS_I2C_BLOCK_DATA && priv->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)
-			smbcmd |= SMBHSTCNT_LAST_BYTE;
-		priv->cmd = smbcmd | priv->xact_extra;
-		priv->len = len;
-		priv->count = 0;
-		priv->data = &data->block[1];
-
-		outb_p(priv->cmd | SMBHSTCNT_START, SMBHSTCNT(priv));
-		result = wait_event_timeout(priv->waitq,
-					    (status = priv->status),
-					    adap->timeout);
-		if (!result) {
-			status = -ETIMEDOUT;
-			dev_warn(&priv->pci_dev->dev,
-				 "Timeout waiting for interrupt!\n");
-		}
-		priv->status = 0;
-		return i801_check_post(priv, status);
-	}
-
-	for (i = 1; i <= len; i++) {
-		if (i == len && read_write == I2C_SMBUS_READ)
-			smbcmd |= SMBHSTCNT_LAST_BYTE;
-		outb_p(smbcmd, SMBHSTCNT(priv));
-
-		if (i == 1)
-			outb_p(inb(SMBHSTCNT(priv)) | SMBHSTCNT_START,
-			       SMBHSTCNT(priv));
-
-		status = i801_wait_byte_done(priv);
-		if (status)
-			goto exit;
-
-		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));
-				return -EPROTO;
-			}
-			data->block[0] = len;
-		}
-
-		/* Retrieve/store value in SMBBLKDAT */
-		if (read_write == I2C_SMBUS_READ)
-			data->block[i] = inb_p(SMBBLKDAT(priv));
-		if (read_write == I2C_SMBUS_WRITE && i+1 <= len)
-			outb_p(data->block[i+1], SMBBLKDAT(priv));
-
-		/* signals SMBBLKDAT ready */
-		outb_p(SMBHSTSTS_BYTE_DONE, SMBHSTSTS(priv));
-	}
+	if (len == 1 && priv->is_read)
+		smbcmd |= SMBHSTCNT_LAST_BYTE;
+	priv->cmd = smbcmd | priv->xact_extra;
+	priv->len = len;
+	priv->count = 0;
 
-	status = i801_wait_intr(priv);
-exit:
-	return i801_check_post(priv, status);
+	outb_p(priv->cmd | SMBHSTCNT_START, SMBHSTCNT(priv));
 }
 
 static int i801_set_block_buffer_mode(struct i801_priv *priv)
@@ -662,19 +633,17 @@ static int i801_set_block_buffer_mode(struct i801_priv *priv)
 
 /* Block transaction function */
 static int i801_block_transaction(struct i801_priv *priv,
-				  union i2c_smbus_data *data, char read_write,
 				  int command, int hwpec)
 {
 	int result = 0;
 
-	if (read_write == I2C_SMBUS_WRITE
-	 || command == I2C_SMBUS_I2C_BLOCK_DATA) {
-		if (data->block[0] < 1)
-			data->block[0] = 1;
-		if (data->block[0] > I2C_SMBUS_BLOCK_MAX)
-			data->block[0] = I2C_SMBUS_BLOCK_MAX;
+	if (!priv->is_read || command == I2C_SMBUS_I2C_BLOCK_DATA) {
+		if (priv->data->block[0] < 1)
+			priv->data->block[0] = 1;
+		if (priv->data->block[0] > I2C_SMBUS_BLOCK_MAX)
+			priv->data->block[0] = I2C_SMBUS_BLOCK_MAX;
 	} else {
-		data->block[0] = 32;	/* max for SMBus block reads */
+		priv->data->block[0] = 32;	/* max for SMBus block reads */
 	}
 
 	/* Experience has shown that the block buffer can only be used for
@@ -691,143 +660,154 @@ static int i801_block_transaction(struct i801_priv *priv,
 			outb_p(inb_p(SMBAUXCTL(priv)) & (~SMBAUXCTL_CRC),
 			       SMBAUXCTL(priv));
 
-		result = i801_block_transaction_by_block(priv, data,
-							 read_write);
+		i801_block_transaction_by_block(priv);
 	} else {
 		outb_p(inb_p(SMBAUXCTL(priv)) & (~SMBAUXCTL_CRC),
 		       SMBAUXCTL(priv));
-		result = i801_block_transaction_byte_by_byte(priv, data,
-							     read_write,
-							     command);
+		priv->byte_by_byte = 1;
+		i801_block_transaction_byte_by_byte(priv, 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.
-	 */
-	outb_p(inb_p(SMBAUXCTL(priv)) &
-	       ~(SMBAUXCTL_CRC | SMBAUXCTL_E32B), SMBAUXCTL(priv));
-
 	return result;
 }
 
-/* Return negative errno on error. */
-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)
+static s32 i801_setup(struct i801_priv *priv, u16 addr, u8 command, int size)
 {
-	int block = 0;
-	int ret, xact = 0;
-	struct i801_priv *priv = i2c_get_adapdata(adap);
-	int result;
-	int hostc = -1;
-
 	switch (size) {
 	case I2C_SMBUS_QUICK:
-		outb_p(((addr & 0x7f) << 1) | (read_write & 0x01),
+		outb_p(((addr & 0x7f) << 1) | priv->is_read,
 		       SMBHSTADD(priv));
-		xact = I801_QUICK;
+		priv->xact = I801_QUICK;
 		break;
 	case I2C_SMBUS_BYTE:
-		outb_p(((addr & 0x7f) << 1) | (read_write & 0x01),
+		outb_p(((addr & 0x7f) << 1) | priv->is_read,
 		       SMBHSTADD(priv));
-		if (read_write == I2C_SMBUS_WRITE)
+		if (!priv->is_read)
 			outb_p(command, SMBHSTCMD(priv));
-		xact = I801_BYTE;
+		priv->xact = I801_BYTE;
 		break;
 	case I2C_SMBUS_BYTE_DATA:
-		outb_p(((addr & 0x7f) << 1) | (read_write & 0x01),
+		outb_p(((addr & 0x7f) << 1) | priv->is_read,
 		       SMBHSTADD(priv));
 		outb_p(command, SMBHSTCMD(priv));
-		if (read_write == I2C_SMBUS_WRITE)
-			outb_p(data->byte, SMBHSTDAT0(priv));
-		xact = I801_BYTE_DATA;
+		if (!priv->is_read)
+			outb_p(priv->data->byte, SMBHSTDAT0(priv));
+		priv->xact = I801_BYTE_DATA;
 		break;
 	case I2C_SMBUS_WORD_DATA:
-		outb_p(((addr & 0x7f) << 1) | (read_write & 0x01),
+		outb_p(((addr & 0x7f) << 1) | priv->is_read,
 		       SMBHSTADD(priv));
 		outb_p(command, SMBHSTCMD(priv));
-		if (read_write == I2C_SMBUS_WRITE) {
-			outb_p(data->word & 0xff, SMBHSTDAT0(priv));
-			outb_p((data->word & 0xff00) >> 8, SMBHSTDAT1(priv));
+		if (!priv->is_read) {
+			outb_p(priv->data->word & 0xff, SMBHSTDAT0(priv));
+			outb_p((priv->data->word & 0xff00) >> 8,
+			       SMBHSTDAT1(priv));
 		}
-		xact = I801_WORD_DATA;
+		priv->xact = I801_WORD_DATA;
 		break;
 	case I2C_SMBUS_BLOCK_DATA:
-		outb_p(((addr & 0x7f) << 1) | (read_write & 0x01),
+		outb_p(((addr & 0x7f) << 1) | priv->is_read,
 		       SMBHSTADD(priv));
 		outb_p(command, SMBHSTCMD(priv));
-		block = 1;
+		priv->block = 1;
 		break;
 	case I2C_SMBUS_I2C_BLOCK_DATA:
 		/* 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 (!priv->is_read) {
 			unsigned char thostc;
-			/* NB: page 240 of ICH5 datasheet also shows
-			 * that DATA1 is the cmd field when reading */
-			outb_p(command, SMBHSTDAT1(priv));
+			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;
+			priv->hostc = thostc;
 		} else if (!(priv->features & FEATURE_I2C_BLOCK_READ)) {
 			dev_err(&priv->pci_dev->dev,
 				"I2C block read is unsupported!\n");
 			return -EOPNOTSUPP;
 		} else
-			outb_p(command, SMBHSTCMD(priv));
-		block = 1;
+			/*
+			 * NB: page 240 of ICH5 datasheet also shows
+			 * that DATA1 is the cmd field when reading
+			 */
+			outb_p(command, SMBHSTDAT1(priv));
+		priv->block = 1;
 		break;
 	default:
 		dev_err(&priv->pci_dev->dev, "Unsupported transaction %d\n",
 			size);
 		return -EOPNOTSUPP;
 	}
+	return 0;
+}
+
+static void i801_op_done(struct i801_priv *priv)
+{
+	priv->done = 1;
+	wake_up(&priv->waitq);
+}
+
+/* Return negative errno on error. */
+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)
+{
+	struct i801_priv *priv = i2c_get_adapdata(adap);
+	int result;
+
+	priv->is_read = (read_write == I2C_SMBUS_READ);
+	priv->byte_by_byte = 0;
+	priv->done = 0;
+	priv->block = 0;
+	priv->xact = 0;
+	priv->hostc = -1;
+	priv->data = data;
+	priv->status = 0;
+
+	result = i801_setup(priv, addr, command, size);
+	if (result < 0)
+		return result;
 
 	result = i801_check_pre(priv);
 	if (result < 0)
 		return result;
 
 	priv->xact_extra &= ~SMBHSTCNT_PEC_EN;
-	if (block) {
+	if (priv->block) {
 		int hwpec = (priv->features & FEATURE_SMBUS_PEC) &&
 			(flags & I2C_CLIENT_PEC)
 			&& size != I2C_SMBUS_QUICK
 			&& size != I2C_SMBUS_I2C_BLOCK_DATA;
 
-		ret = i801_block_transaction(priv, data, read_write, size,
-					     hwpec);
+		i801_block_transaction(priv, size, hwpec);
 	} else {
 		outb_p(inb_p(SMBAUXCTL(priv)) & (~SMBAUXCTL_CRC),
 		       SMBAUXCTL(priv));
-		ret = i801_transaction(priv, xact);
+		i801_transaction(priv, priv->xact);
 	}
 
-	if (hostc >= 0)
-		pci_write_config_byte(priv->pci_dev, SMBHSTCFG, hostc);
+	if (priv->features & FEATURE_IRQ) {
+		priv->timeout = I801_TIMEOUT_NS * MAX_RETRIES;
+		priv->retries = 1;
+	} else {
+		priv->timeout = I801_TIMEOUT_NS;
+		priv->retries = MAX_RETRIES;
+	}
 
-	if (block)
-		return ret;
-	if (ret)
-		return ret;
-	if ((read_write == I2C_SMBUS_WRITE) || (xact == I801_QUICK))
-		return 0;
+	hrtimer_start_range_ns(&priv->timer,
+			       ktime_add_ns(ktime_get(), priv->timeout),
+			       I801_TIMEOUT_DELTA_NS,
+			       HRTIMER_MODE_ABS);
 
-	switch (xact & 0x7f) {
-	case I801_BYTE:	/* Result put in SMBHSTDAT0 */
-	case I801_BYTE_DATA:
-		data->byte = inb_p(SMBHSTDAT0(priv));
-		break;
-	case I801_WORD_DATA:
-		data->word = inb_p(SMBHSTDAT0(priv)) +
-			     (inb_p(SMBHSTDAT1(priv)) << 8);
-		break;
+	result = wait_event_timeout(priv->waitq, priv->status, adap->timeout);
+	if (!result) {
+		priv->status = -ETIMEDOUT;
+		dev_warn(&priv->pci_dev->dev,
+			 "Timeout waiting for interrupt!\n");
 	}
-	return 0;
+	return i801_check_post(priv);
 }
 
 
@@ -1281,6 +1261,9 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
 	priv->adapter.dev.parent = &dev->dev;
 	ACPI_COMPANION_SET(&priv->adapter.dev, ACPI_COMPANION(&dev->dev));
 	priv->adapter.retries = 3;
+	spin_lock_init(&priv->lock);
+	hrtimer_init(&priv->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
+	priv->timer.function = i801_timeout;
 
 	priv->pci_dev = dev;
 	switch (dev->device) {
@@ -1376,6 +1359,8 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
 		outb_p(inb_p(SMBAUXCTL(priv)) &
 		       ~(SMBAUXCTL_CRC | SMBAUXCTL_E32B), SMBAUXCTL(priv));
 
+	init_waitqueue_head(&priv->waitq);
+
 	/* Default timeout in interrupt mode: 200 ms */
 	priv->adapter.timeout = HZ / 5;
 
@@ -1397,7 +1382,6 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
 
 	if (priv->features & FEATURE_IRQ) {
 		priv->xact_extra |= SMBHSTCNT_INTREN;
-		init_waitqueue_head(&priv->waitq);
 
 		err = devm_request_irq(&dev->dev, dev->irq, i801_isr,
 				       IRQF_SHARED,
@@ -1440,6 +1424,8 @@ static void i801_remove(struct pci_dev *dev)
 
 	platform_device_unregister(priv->tco_pdev);
 
+	hrtimer_cancel(&priv->timer);
+	kfree(priv);
 	/*
 	 * do not call pci_disable_device(dev) since it can cause hard hangs on
 	 * some systems during power-off (eg. Fujitsu-Siemens Lifebook E8010)
-- 
2.5.0

Re: [PATCH 5/5] i2c-i801: Remove redundant code and event-drive

[Jean Delvare]

Hi Corey,

On Tue, 19 Jan 2016 11:09:37 -0600, minyard@acm.org wrote:
> From: Corey Minyard <cminyard@mvista.com>
> 
> There was a lot of redundant code in the driver, basically one copy
> for the polled interface and one for the interrupt-driven interface.
> Only use the interrupt-driven interface and use a timer to check
> the interface periodically and time things out.

This patch is simply too large and messy for me to consider reviewing
it. It needs to be split into smaller pieces. But honestly I don't see
that much redundancy in the driver, and as a matter of fact your patch
adds more lines than it removes.

This is unfortunate because some of the changes would still be good to
have. In particular having a single call to i801_check_post() would be
a nice clean-up.

> This also adds timeouts when using interrupt, so a failed operation
> won't hang forever.

As far as I can see, the current code already has these timeouts,
although maybe it wasn't the case when you first wrote this patch? This
was added by b3b8df9772 in November 2014.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 5/5] i2c-i801: Remove redundant code and event-drive

[Corey Minyard]

On 05/25/2016 07:52 AM, Jean Delvare wrote:
> Hi Corey,
>
> On Tue, 19 Jan 2016 11:09:37 -0600, minyard@acm.org wrote:
>> From: Corey Minyard <cminyard@mvista.com>
>>
>> There was a lot of redundant code in the driver, basically one copy
>> for the polled interface and one for the interrupt-driven interface.
>> Only use the interrupt-driven interface and use a timer to check
>> the interface periodically and time things out.
> This patch is simply too large and messy for me to consider reviewing
> it. It needs to be split into smaller pieces. But honestly I don't see
> that much redundancy in the driver, and as a matter of fact your patch
> adds more lines than it removes.

Umm:

  drivers/i2c/busses/i2c-i801.c | 582 +++++++++++++++++++++---------------------
  1 file changed, 284 insertions(+), 298 deletions(-)

It's not a lot of lines, but it is a little smaller.

>
> This is unfortunate because some of the changes would still be good to
> have. In particular having a single call to i801_check_post() would be
> a nice clean-up.

Yes, looking over this some more, several of the issues have been
addressed that these patches originally targeted.

This was originally part of a set of patches that allowed access to
I2C devices when the system couldn't schedule, primarily for
storing panic information and handling watchdogs at panic and
in kgdb.  For that you have to event-drive it so you can poll.
I've abandoned those changes, but this small set looked
like it has some useful stuff.

Thanks for the review, I'll look at pulling out some of the good things
(and getting rid of hwpec from i801_block_transaction_byte_by_byte
since it's not used there any more).

-corey

>> This also adds timeouts when using interrupt, so a failed operation
>> won't hang forever.
> As far as I can see, the current code already has these timeouts,
> although maybe it wasn't the case when you first wrote this patch? This
> was added by b3b8df9772 in November 2014.
>

Re: [PATCH 5/5] i2c-i801: Remove redundant code and event-drive

[Jean Delvare]

Hi Corey,

On Wed, 25 May 2016 11:58:29 -0500, Corey Minyard wrote:
> On 05/25/2016 07:52 AM, Jean Delvare wrote:
> > On Tue, 19 Jan 2016 11:09:37 -0600, minyard@acm.org wrote:
> >> From: Corey Minyard <cminyard@mvista.com>
> >>
> >> There was a lot of redundant code in the driver, basically one copy
> >> for the polled interface and one for the interrupt-driven interface.
> >> Only use the interrupt-driven interface and use a timer to check
> >> the interface periodically and time things out.
> > This patch is simply too large and messy for me to consider reviewing
> > it. It needs to be split into smaller pieces. But honestly I don't see
> > that much redundancy in the driver, and as a matter of fact your patch
> > adds more lines than it removes.
> 
> Umm:
> 
>   drivers/i2c/busses/i2c-i801.c | 582 +++++++++++++++++++++---------------------
>   1 file changed, 284 insertions(+), 298 deletions(-)
> 
> It's not a lot of lines, but it is a little smaller.

Oops, my bad, I read the diffstat wrong. My apologies.

> > This is unfortunate because some of the changes would still be good to
> > have. In particular having a single call to i801_check_post() would be
> > a nice clean-up.
> 
> Yes, looking over this some more, several of the issues have been
> addressed that these patches originally targeted.
> 
> This was originally part of a set of patches that allowed access to
> I2C devices when the system couldn't schedule, primarily for
> storing panic information and handling watchdogs at panic and
> in kgdb.  For that you have to event-drive it so you can poll.
> I've abandoned those changes, but this small set looked
> like it has some useful stuff.
> 
> Thanks for the review, I'll look at pulling out some of the good things

Thank you, I'll be happy to review and test the new series, hopefully
faster than I did for this one. Now that the i2c-i801 driver works on
my system again, it should be much easier.

> (and getting rid of hwpec from i801_block_transaction_byte_by_byte
> since it's not used there any more).

Did it ever work? And more importantly, does the hardware support it?
Before cleaning up the code, I'd like to make sure the driver supports
PEC in all cases where the hardware itself supports it. I'll do some
tests.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 5/5] i2c-i801: Remove redundant code and event-drive

[Corey Minyard]

On 05/26/2016 02:17 AM, Jean Delvare wrote:
> Hi Corey,
>
snip
>
>>> This is unfortunate because some of the changes would still be good to
>>> have. In particular having a single call to i801_check_post() would be
>>> a nice clean-up.
>> Yes, looking over this some more, several of the issues have been
>> addressed that these patches originally targeted.
>>
>> This was originally part of a set of patches that allowed access to
>> I2C devices when the system couldn't schedule, primarily for
>> storing panic information and handling watchdogs at panic and
>> in kgdb.  For that you have to event-drive it so you can poll.
>> I've abandoned those changes, but this small set looked
>> like it has some useful stuff.
>>
>> Thanks for the review, I'll look at pulling out some of the good things
> Thank you, I'll be happy to review and test the new series, hopefully
> faster than I did for this one. Now that the i2c-i801 driver works on
> my system again, it should be much easier.

Ok, thanks.

BTW, I have a working i801 device on qemu, if that helps you.  I'm
working to get it into mainline there.  What's in qemu now is only
marginally functional.

>> (and getting rid of hwpec from i801_block_transaction_byte_by_byte
>> since it's not used there any more).
> Did it ever work? And more importantly, does the hardware support it?
> Before cleaning up the code, I'd like to make sure the driver supports
> PEC in all cases where the hardware itself supports it. I'll do some
> tests.
>

The docs are kind of unclear.  I didn't find anything that said it
didn't work, but I got that impression from some place.  I never
tested it out.

-corey