diff mbox

[v6] i2c: Adding support for Intel iSMT SMBus 2.0 host controller

Message ID 20130204181902.06c247ad@endymion.delvare
State Not Applicable
Headers show

Commit Message

Jean Delvare Feb. 4, 2013, 5:19 p.m. UTC 
On Mon, 4 Feb 2013 10:47:44 +0100, Jean Delvare wrote:
> I have backported your code to kernel 3.0 and I'll get back to you when
> I finally get a chance to test it.

OK, I am done with the backport and testing. The test system I have
access to appears to have a single slave on the second iSMT channel, at
address 0x54. I have no idea what this device is so my testing is
fairly limited, in particular I can't test writes. But at least I could
test the quick command and read byte transactions. I believe the
following fix is needed on top of your patch to make non-quick,
non-block transactions right:

---
 i2c-ismt.c |   35 +++++++++++++++++++++++++----------
 1 file changed, 25 insertions(+), 10 deletions(-)


Maybe that's what Seth was missing. Seth, can you please try this patch
of mine and let us know if it helps? Warning: I was only able to test
the quick, receive byte and read byte transactions.

Comments

Neil Horman Feb. 4, 2013, 5:28 p.m. UTC | #1 
On Mon, Feb 04, 2013 at 06:19:02PM +0100, Jean Delvare wrote:
> On Mon, 4 Feb 2013 10:47:44 +0100, Jean Delvare wrote:
> > I have backported your code to kernel 3.0 and I'll get back to you when
> > I finally get a chance to test it.
> 
> OK, I am done with the backport and testing. The test system I have
> access to appears to have a single slave on the second iSMT channel, at
> address 0x54. I have no idea what this device is so my testing is
> fairly limited, in particular I can't test writes. But at least I could
> test the quick command and read byte transactions. I believe the
> following fix is needed on top of your patch to make non-quick,
> non-block transactions right:
> 
I've got the same device on my system here.  Seth, can you elaborate on what it
is.  FWIW, I took a risk, read a block from that device and wrote the same block
back to it with my v6 driver, with dyamic debug enabled, and the reported
printk's indicated it succeded.  I'll test out this patch in a bit here, make
the other requested corrections and repost shortly.
Neil

> ---
>  i2c-ismt.c |   35 +++++++++++++++++++++++++----------
>  1 file changed, 25 insertions(+), 10 deletions(-)
> 
> --- i2c-ismt.orig/i2c-ismt.c
> +++ i2c-ismt/i2c-ismt.c
> @@ -329,10 +329,23 @@ static int ismt_process_desc(const struc
>  	__ismt_desc_dump(&priv->pci_dev->dev, desc);
>  
>  	if (desc->status & ISMT_DESC_SCS) {
> -		if ((size != I2C_SMBUS_QUICK) &&
> -		    (read_write == I2C_SMBUS_READ)) {
> +		if (read_write == I2C_SMBUS_WRITE &&
> +		    size != I2C_SMBUS_PROC_CALL)
> +			return 0;
> +
> +		switch (size) {
> +		case I2C_SMBUS_BYTE:
> +		case I2C_SMBUS_BYTE_DATA:
> +			data->byte = dma_buffer[0];
> +			break;
> +		case I2C_SMBUS_WORD_DATA:
> +		case I2C_SMBUS_PROC_CALL:
> +			data->word = dma_buffer[0] | (dma_buffer[1] << 8);
> +			break;
> +		case I2C_SMBUS_BLOCK_DATA:
>  			memcpy(&data->block[1], dma_buffer, desc->rxbytes);
>  			data->block[0] = desc->rxbytes;
> +			break;
>  		}
>  		return 0;
>  	}
> @@ -426,6 +439,8 @@ static int ismt_access(struct i2c_adapte
>  			desc->wr_len_cmd = 2;
>  			dma_size = 2;
>  			dma_direction = DMA_TO_DEVICE;
> +			priv->dma_buffer[0] = command;
> +			priv->dma_buffer[1] = data->byte;
>  		} else {
>  			/* Read Byte */
>  			dev_dbg(dev, "I2C_SMBUS_BYTE_DATA:  READ\n");
> @@ -444,6 +459,9 @@ static int ismt_access(struct i2c_adapte
>  			desc->wr_len_cmd = 3;
>  			dma_size = 3;
>  			dma_direction = DMA_TO_DEVICE;
> +			priv->dma_buffer[0] = command;
> +			priv->dma_buffer[1] = data->word & 0xff;
> +			priv->dma_buffer[2] = data->word >> 8;
>  		} else {
>  			/* Read Word */
>  			dev_dbg(dev, "I2C_SMBUS_WORD_DATA:  READ\n");
> @@ -461,6 +479,9 @@ static int ismt_access(struct i2c_adapte
>  		desc->rd_len = 2;
>  		dma_size = 3;
>  		dma_direction = DMA_BIDIRECTIONAL;
> +		priv->dma_buffer[0] = command;
> +		priv->dma_buffer[1] = data->word & 0xff;
> +		priv->dma_buffer[2] = data->word >> 8;
>  		break;
>  
>  	case I2C_SMBUS_BLOCK_DATA:
> @@ -471,6 +492,8 @@ static int ismt_access(struct i2c_adapte
>  			dma_direction = DMA_TO_DEVICE;
>  			desc->wr_len_cmd = dma_size;
>  			desc->control |= ISMT_DESC_BLK;
> +			priv->dma_buffer[0] = command;
> +			memcpy(&priv->dma_buffer[1], &data->block[1], dma_size);
>  		} else {
>  			/* Block Read */
>  			dev_dbg(dev, "I2C_SMBUS_BLOCK_DATA:  READ\n");
> @@ -488,14 +511,6 @@ static int ismt_access(struct i2c_adapte
>  		return -EOPNOTSUPP;
>  	}
>  
> -	/* Create a temporary buffer for the DMA transaction */
> -	/* and insert the command at the beginning of the buffer */
> -	if ((read_write == I2C_SMBUS_WRITE) &&
> -	    (size > I2C_SMBUS_BYTE)) {
> -		memcpy(&priv->dma_buffer[1], &data->block[1], dma_size);
> -		priv->dma_buffer[0] = command;
> -	}
> -
>  	/* map the data buffer */
>  	if (dma_size != 0) {
>  		dev_dbg(dev, " dev=%p\n", dev);
> 
> Maybe that's what Seth was missing. Seth, can you please try this patch
> of mine and let us know if it helps? Warning: I was only able to test
> the quick, receive byte and read byte transactions.
> 
> -- 
> Jean Delvare
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jean Delvare Feb. 5, 2013, 7:21 a.m. UTC | #2 
On Mon, 4 Feb 2013 12:28:51 -0500, Neil Horman wrote:
> I've got the same device on my system here.  Seth, can you elaborate on what it
> is.  FWIW, I took a risk, read a block from that device and wrote the same block
> back to it with my v6 driver, with dyamic debug enabled, and the reported
> printk's indicated it succeded.

That doesn't mean a thing, unfortunately. It is possible that the write
did not work, if you attempted to write the same data as was already
present, you can't tell the difference between success and failure.
Some slave SMBus devices will ack all transactions regardless of
whether they support these transactions or not. As long as we don't
know what is this slave device at 0x54, it's impossible to tell if the
driver really works. I am glad Seth could test it with a different,
known slave device.

>  I'll test out this patch in a bit here, make
> the other requested corrections and repost shortly.
diff mbox

Patch

--- i2c-ismt.orig/i2c-ismt.c
+++ i2c-ismt/i2c-ismt.c
@@ -329,10 +329,23 @@  static int ismt_process_desc(const struc
 	__ismt_desc_dump(&priv->pci_dev->dev, desc);
 
 	if (desc->status & ISMT_DESC_SCS) {
-		if ((size != I2C_SMBUS_QUICK) &&
-		    (read_write == I2C_SMBUS_READ)) {
+		if (read_write == I2C_SMBUS_WRITE &&
+		    size != I2C_SMBUS_PROC_CALL)
+			return 0;
+
+		switch (size) {
+		case I2C_SMBUS_BYTE:
+		case I2C_SMBUS_BYTE_DATA:
+			data->byte = dma_buffer[0];
+			break;
+		case I2C_SMBUS_WORD_DATA:
+		case I2C_SMBUS_PROC_CALL:
+			data->word = dma_buffer[0] | (dma_buffer[1] << 8);
+			break;
+		case I2C_SMBUS_BLOCK_DATA:
 			memcpy(&data->block[1], dma_buffer, desc->rxbytes);
 			data->block[0] = desc->rxbytes;
+			break;
 		}
 		return 0;
 	}
@@ -426,6 +439,8 @@  static int ismt_access(struct i2c_adapte
 			desc->wr_len_cmd = 2;
 			dma_size = 2;
 			dma_direction = DMA_TO_DEVICE;
+			priv->dma_buffer[0] = command;
+			priv->dma_buffer[1] = data->byte;
 		} else {
 			/* Read Byte */
 			dev_dbg(dev, "I2C_SMBUS_BYTE_DATA:  READ\n");
@@ -444,6 +459,9 @@  static int ismt_access(struct i2c_adapte
 			desc->wr_len_cmd = 3;
 			dma_size = 3;
 			dma_direction = DMA_TO_DEVICE;
+			priv->dma_buffer[0] = command;
+			priv->dma_buffer[1] = data->word & 0xff;
+			priv->dma_buffer[2] = data->word >> 8;
 		} else {
 			/* Read Word */
 			dev_dbg(dev, "I2C_SMBUS_WORD_DATA:  READ\n");
@@ -461,6 +479,9 @@  static int ismt_access(struct i2c_adapte
 		desc->rd_len = 2;
 		dma_size = 3;
 		dma_direction = DMA_BIDIRECTIONAL;
+		priv->dma_buffer[0] = command;
+		priv->dma_buffer[1] = data->word & 0xff;
+		priv->dma_buffer[2] = data->word >> 8;
 		break;
 
 	case I2C_SMBUS_BLOCK_DATA:
@@ -471,6 +492,8 @@  static int ismt_access(struct i2c_adapte
 			dma_direction = DMA_TO_DEVICE;
 			desc->wr_len_cmd = dma_size;
 			desc->control |= ISMT_DESC_BLK;
+			priv->dma_buffer[0] = command;
+			memcpy(&priv->dma_buffer[1], &data->block[1], dma_size);
 		} else {
 			/* Block Read */
 			dev_dbg(dev, "I2C_SMBUS_BLOCK_DATA:  READ\n");
@@ -488,14 +511,6 @@  static int ismt_access(struct i2c_adapte
 		return -EOPNOTSUPP;
 	}
 
-	/* Create a temporary buffer for the DMA transaction */
-	/* and insert the command at the beginning of the buffer */
-	if ((read_write == I2C_SMBUS_WRITE) &&
-	    (size > I2C_SMBUS_BYTE)) {
-		memcpy(&priv->dma_buffer[1], &data->block[1], dma_size);
-		priv->dma_buffer[0] = command;
-	}
-
 	/* map the data buffer */
 	if (dma_size != 0) {
 		dev_dbg(dev, " dev=%p\n", dev);