Re: Question about vt82c596 SMBus driver (Via I2c Bus driver)

Re: Question about vt82c596 SMBus driver (Via I2c Bus driver)

[Jean Delvare]

Hi Prakash,

Removing Kyosti and Mark, who have stepped back long ago, and adding
the i2c mailing list instead, where people may have insight about your
problem.

On Thu, 25 Sep 2008 10:25:50 -0400, Mortha, Prakash wrote:
> I am working with I2c bus driver in Via Ex mother board.
> 
> I am trying to communicate with Micronas chip (MAP5401) on SMBus.
> 
> I need to send 1 sub command and 2 bytes of register address and then
> have to read 2 bytes of data. For this to work I am trying to implement
> I2C_SMBUS_PROC_CALL in function vt596_access() in the file
> drivers/i2c/busses/i2c-viapro.c
> 
> One of the requirement here is I need to send REPEATED START after
> sending 1 command byte and 2 register address bytes before reading 2
> data bytes. But when I use the logic analyzer I could see that a STOP
> being sent when I expect a REPEATED START.
> 
> How to I implement SMBus Process Call functionality. Please guide me.

Well, these VIA chips aren't I2C chips where you have full control of
the clock and data lines, but SMBus master chips with a limited set of
commands available. The SMBus Process Call is one of these commands, it
is supposed to be supported by the VIA chips (protocol = 0100b). Adding
support to the i2c-viapro driver should be trivial.

However, if you did that and the logic analyzer reveals that the VIA
chip doesn't implement the command properly, then this is a hardware
bug and unfortunately there's nothing we can do. This makes me a bit
curious... did you test the other commands with your logic analyzer to
make sure they are correct?

Can you show us the patch you wrote?

What exact VIA chip are you using?

-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: Question about vt82c596 SMBus driver (Via I2c Bus driver)

[Mortha, Prakash]

[-- Attachment #1.1: Type: text/plain, Size: 2999 bytes --]

 

Hi Jean,

 

I found the reason why drivers/i2c/busses/i2c-viapro.c is not supporting
I2C_SMBUS_PROC_CALL. The reason I found is as per the data sheet of Via
Chip set the "SMBus Command Protocol field" should be set as bits 4-2 in
"SMBus Host Control" register. But in the function 

      static int vt596_transaction(u8 size)

we are setting the "SMBus Command Protocol field" as bits 2-0:

outb_p(0x40 | size, SMBHSTCNT); (Line 134 in
drivers/i2c/busses/i2c-viapro.c)

 

When I changed the statement to shift the Protocol field by 2 bits
I2C_SMBUS_PROC_CALL worked and REPEATED START is being generated as per
SMBus Process call specification.

outb_p(0x40 | (size<<2), SMBHSTCNT);

 

Could you please confirm whether the reason I found is valid or not? 

Please find attached the patch I wrote. The Via Motherboard I am using
is EPIA Ex 1500G.  

 

 

Thank you,

Prakash

 

 

 

-----Original Message-----
From: Jean Delvare [mailto:khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org] 
Sent: Friday, September 26, 2008 3:29 AM
To: Mortha, Prakash
Cc: Linux I2C
Subject: Re: Question about vt82c596 SMBus driver (Via I2c Bus driver)

 

Hi Prakash,

 

Removing Kyosti and Mark, who have stepped back long ago, and adding

the i2c mailing list instead, where people may have insight about your

problem.

 

On Thu, 25 Sep 2008 10:25:50 -0400, Mortha, Prakash wrote:

> I am working with I2c bus driver in Via Ex mother board.

> 

> I am trying to communicate with Micronas chip (MAP5401) on SMBus.

> 

> I need to send 1 sub command and 2 bytes of register address and then

> have to read 2 bytes of data. For this to work I am trying to
implement

> I2C_SMBUS_PROC_CALL in function vt596_access() in the file

> drivers/i2c/busses/i2c-viapro.c

> 

> One of the requirement here is I need to send REPEATED START after

> sending 1 command byte and 2 register address bytes before reading 2

> data bytes. But when I use the logic analyzer I could see that a STOP

> being sent when I expect a REPEATED START.

> 

> How to I implement SMBus Process Call functionality. Please guide me.

 

Well, these VIA chips aren't I2C chips where you have full control of

the clock and data lines, but SMBus master chips with a limited set of

commands available. The SMBus Process Call is one of these commands, it

is supposed to be supported by the VIA chips (protocol = 0100b). Adding

support to the i2c-viapro driver should be trivial.

 

However, if you did that and the logic analyzer reveals that the VIA

chip doesn't implement the command properly, then this is a hardware

bug and unfortunately there's nothing we can do. This makes me a bit

curious... did you test the other commands with your logic analyzer to

make sure they are correct?

 

Can you show us the patch you wrote?

 

What exact VIA chip are you using?

 

-- 

Jean Delvare


[-- Attachment #1.2: Type: text/html, Size: 11744 bytes --]

[-- Attachment #2: i2c-viapro.patch --]
[-- Type: application/octet-stream, Size: 2096 bytes --]

*** ./../../../../../../Original_LinuxView/drivers/i2c/busses/i2c-viapro.c	2008-02-25 19:20:20.000000000 -0500
--- i2c-viapro.c	2008-09-29 11:40:31.643869167 -0400
***************
*** 156,162 ****
  	}
  
  	/* Start the transaction by setting bit 6 */
! 	outb_p(0x40 | size, SMBHSTCNT);
  
  	/* We will always wait for a fraction of a second */
  	do {
--- 156,162 ----
  	}
  
  	/* Start the transaction by setting bit 6 */
! 	outb_p(0x40 | (size<<2), SMBHSTCNT);
  
  	/* We will always wait for a fraction of a second */
  	do {
***************
*** 207,213 ****
  		int size, union i2c_smbus_data *data)
  {
  	int i;
! 
  	switch (size) {
  	case I2C_SMBUS_QUICK:
  		size = VT596_QUICK;
--- 207,213 ----
  		int size, union i2c_smbus_data *data)
  {
  	int i;
! 	
  	switch (size) {
  	case I2C_SMBUS_QUICK:
  		size = VT596_QUICK;
***************
*** 223,228 ****
--- 223,234 ----
  			outb_p(data->byte, SMBHSTDAT0);
  		size = VT596_BYTE_DATA;
  		break;
+ 	case I2C_SMBUS_PROC_CALL:
+ 		outb_p(command, SMBHSTCMD);
+ 		outb_p((data->word & 0xff00) >> 8, SMBHSTDAT0);
+ 		outb_p(data->word & 0xff, SMBHSTDAT1);
+ 		read_write = I2C_SMBUS_WRITE;
+ 		break;
  	case I2C_SMBUS_WORD_DATA:
  		outb_p(command, SMBHSTCMD);
  		if (read_write == I2C_SMBUS_WRITE) {
***************
*** 260,265 ****
--- 266,276 ----
  	if (vt596_transaction(size)) /* Error in transaction */
  		return -1;
  
+ 	if(size == I2C_SMBUS_PROC_CALL){
+ 		read_write = I2C_SMBUS_READ;
+ 		size = VT596_WORD_DATA;
+ 		}
+ 
  	if ((read_write == I2C_SMBUS_WRITE) || (size == VT596_QUICK))
  		return 0;
  
***************
*** 269,275 ****
  		data->byte = inb_p(SMBHSTDAT0);
  		break;
  	case VT596_WORD_DATA:
! 		data->word = inb_p(SMBHSTDAT0) + (inb_p(SMBHSTDAT1) << 8);
  		break;
  	case VT596_I2C_BLOCK_DATA:
  	case VT596_BLOCK_DATA:
--- 280,288 ----
  		data->byte = inb_p(SMBHSTDAT0);
  		break;
  	case VT596_WORD_DATA:
! 		data->word = inb_p(SMBHSTDAT0);
! 		data->word = (data->word) << 8;
! 		data->word = data->word + inb_p(SMBHSTDAT1);
  		break;
  	case VT596_I2C_BLOCK_DATA:
  	case VT596_BLOCK_DATA:

[-- Attachment #3: Type: text/plain, Size: 157 bytes --]

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: Question about vt82c596 SMBus driver (Via I2c Bus driver)

[Jean Delvare]

Hi Prakash,

On Mon, 29 Sep 2008 11:49:30 -0400, Mortha, Prakash wrote:
> I found the reason why drivers/i2c/busses/i2c-viapro.c is not supporting
> I2C_SMBUS_PROC_CALL. The reason I found is as per the data sheet of Via
> Chip set the "SMBus Command Protocol field" should be set as bits 4-2 in
> "SMBus Host Control" register. But in the function 
> 
>       static int vt596_transaction(u8 size)
> 
> we are setting the "SMBus Command Protocol field" as bits 2-0:
> 
> outb_p(0x40 | size, SMBHSTCNT); (Line 134 in
> drivers/i2c/busses/i2c-viapro.c)

"size" is supposed to be already shifted at this point. If you look at
the constants:

#define VT596_QUICK		0x00
#define VT596_BYTE		0x04
#define VT596_BYTE_DATA		0x08
#define VT596_WORD_DATA		0x0C
#define VT596_BLOCK_DATA	0x14
#define VT596_I2C_BLOCK_DATA	0x34

They really are (0x0 << 2), (0x1 << 2), (0x2 <<2) etc. Maybe we should
write them that way to make it clearer.

> When I changed the statement to shift the Protocol field by 2 bits
> I2C_SMBUS_PROC_CALL worked and REPEATED START is being generated as per
> SMBus Process call specification.
> 
> outb_p(0x40 | (size<<2), SMBHSTCNT);
> 
>  
> 
> Could you please confirm whether the reason I found is valid or not?

No, it's not. Your change just happens to work by accident in your
specific case. But the original code is correct as it is. Please
remember that this driver has been used by thousands of people for many
years now, so it's rather unlikely that the common code paths have such
a huge bug.

> Please find attached the patch I wrote. The Via Motherboard I am using
> is EPIA Ex 1500G.  

Please use "diff -u" to generate the patch, otherwise it's rather
difficult to read. But your current patch doesn't look correct anyway.
The first thing to do is to introduce a new constant for process call
transactions:

#define VT596_PROC_CALL		0x10

Then in vt596_access() you must set size = VT596_PROC_CALL for the
I2C_SMBUS_PROC_CALL case. This is currently missing in your code (you
do not set "size" at all!) and that's the reason why it didn't work.

Also, you must add I2C_SMBUS_PROC_CALL to vt596_func(), otherwise users
do not know that this protocol is supported.

-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: Question about vt82c596 SMBus driver (Via I2c Bus driver)

[Mortha, Prakash]

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


Hi Jean,

Thank you very much for your corrections/advice.

Please find attached the patch files for i2c-viapro.c and i2c-core.c
files for supporting I2C_SMBUS_PROC_CALL. 

Please provide your comments.

(FYI, in my test environment I had to swap MSB and LSB bytes while
dealing with I2C_SMBUS_WORD_DATA, I2C_SMBUS_PROC_CALL and
VT596_WORD_DATA/VT596_PROC_CALL, but I didn't do the swap while
generating the patch file to keep the earlier design in-tact.)

Thank you,
Prakash



-----Original Message-----
From: Jean Delvare [mailto:khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org] 
Sent: Tuesday, September 30, 2008 3:25 AM
To: Mortha, Prakash
Cc: Linux I2C
Subject: Re: Question about vt82c596 SMBus driver (Via I2c Bus driver)

Hi Prakash,

On Mon, 29 Sep 2008 11:49:30 -0400, Mortha, Prakash wrote:
> I found the reason why drivers/i2c/busses/i2c-viapro.c is not
supporting
> I2C_SMBUS_PROC_CALL. The reason I found is as per the data sheet of
Via
> Chip set the "SMBus Command Protocol field" should be set as bits 4-2
in
> "SMBus Host Control" register. But in the function 
> 
>       static int vt596_transaction(u8 size)
> 
> we are setting the "SMBus Command Protocol field" as bits 2-0:
> 
> outb_p(0x40 | size, SMBHSTCNT); (Line 134 in
> drivers/i2c/busses/i2c-viapro.c)

"size" is supposed to be already shifted at this point. If you look at
the constants:

#define VT596_QUICK		0x00
#define VT596_BYTE		0x04
#define VT596_BYTE_DATA		0x08
#define VT596_WORD_DATA		0x0C
#define VT596_BLOCK_DATA	0x14
#define VT596_I2C_BLOCK_DATA	0x34

They really are (0x0 << 2), (0x1 << 2), (0x2 <<2) etc. Maybe we should
write them that way to make it clearer.

> When I changed the statement to shift the Protocol field by 2 bits
> I2C_SMBUS_PROC_CALL worked and REPEATED START is being generated as
per
> SMBus Process call specification.
> 
> outb_p(0x40 | (size<<2), SMBHSTCNT);
> 
>  
> 
> Could you please confirm whether the reason I found is valid or not?

No, it's not. Your change just happens to work by accident in your
specific case. But the original code is correct as it is. Please
remember that this driver has been used by thousands of people for many
years now, so it's rather unlikely that the common code paths have such
a huge bug.

> Please find attached the patch I wrote. The Via Motherboard I am using
> is EPIA Ex 1500G.  

Please use "diff -u" to generate the patch, otherwise it's rather
difficult to read. But your current patch doesn't look correct anyway.
The first thing to do is to introduce a new constant for process call
transactions:

#define VT596_PROC_CALL		0x10

Then in vt596_access() you must set size = VT596_PROC_CALL for the
I2C_SMBUS_PROC_CALL case. This is currently missing in your code (you
do not set "size" at all!) and that's the reason why it didn't work.

Also, you must add I2C_SMBUS_PROC_CALL to vt596_func(), otherwise users
do not know that this protocol is supported.

-- 
Jean Delvare

[-- Attachment #2: i2c-core.patch --]
[-- Type: application/octet-stream, Size: 682 bytes --]

--- i2c-core.c	2008-02-25 19:20:20.000000000 -0500
+++ ./../../../buildroot/build_i686/linux-2.6.24.3/drivers/i2c/i2c-core.c	2008-10-01 19:48:25.771245369 -0400
@@ -1274,6 +1274,18 @@
 }
 EXPORT_SYMBOL(i2c_smbus_read_word_data);
 
+s32 i2c_smbus_process_call(struct i2c_client *client, u8 command, u16 value)
+{
+	union i2c_smbus_data data;
+	data.word = value;
+	if (i2c_smbus_xfer(client->adapter,client->addr,client->flags,I2C_SMBUS_WRITE,command, I2C_SMBUS_PROC_CALL, &data))
+		return -1;
+	else
+		return data.word;
+}
+EXPORT_SYMBOL(i2c_smbus_process_call);
+
+
 s32 i2c_smbus_write_word_data(struct i2c_client *client, u8 command, u16 value)
 {
 	union i2c_smbus_data data;

[-- Attachment #3: i2c-viapro.patch --]
[-- Type: application/octet-stream, Size: 1553 bytes --]

--- i2c-viapro.c	2008-10-01 12:19:39.578169020 -0400
+++ ./../../../../buildroot/build_i686/linux-2.6.24.3/drivers/i2c/busses/i2c-viapro.c	2008-10-01 19:49:02.460538263 -0400
@@ -82,6 +82,7 @@
 #define VT596_BYTE		0x04
 #define VT596_BYTE_DATA		0x08
 #define VT596_WORD_DATA		0x0C
+#define VT596_PROC_CALL         0x10
 #define VT596_BLOCK_DATA	0x14
 #define VT596_I2C_BLOCK_DATA	0x34
 
@@ -231,6 +232,12 @@
 		}
 		size = VT596_WORD_DATA;
 		break;
+	case I2C_SMBUS_PROC_CALL:
+		outb_p(command, SMBHSTCMD);
+		outb_p((data->word & 0xff00) >> 8, SMBHSTDAT0);
+		outb_p(data->word & 0xff, SMBHSTDAT1);
+		size = VT596_PROC_CALL;
+		break;
 	case I2C_SMBUS_I2C_BLOCK_DATA:
 		if (!(vt596_features & FEATURE_I2CBLOCK))
 			goto exit_unsupported;
@@ -260,7 +267,7 @@
 	if (vt596_transaction(size)) /* Error in transaction */
 		return -1;
 
-	if ((read_write == I2C_SMBUS_WRITE) || (size == VT596_QUICK))
+	if (((size != VT596_PROC_CALL) && (read_write == I2C_SMBUS_WRITE)) || (size == VT596_QUICK))
 		return 0;
 
 	switch (size) {
@@ -269,6 +276,7 @@
 		data->byte = inb_p(SMBHSTDAT0);
 		break;
 	case VT596_WORD_DATA:
+	case VT596_PROC_CALL:
 		data->word = inb_p(SMBHSTDAT0) + (inb_p(SMBHSTDAT1) << 8);
 		break;
 	case VT596_I2C_BLOCK_DATA:
@@ -293,7 +301,7 @@
 {
 	u32 func = I2C_FUNC_SMBUS_QUICK | I2C_FUNC_SMBUS_BYTE |
 	    I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_WORD_DATA |
-	    I2C_FUNC_SMBUS_BLOCK_DATA;
+	    I2C_FUNC_SMBUS_BLOCK_DATA | I2C_SMBUS_PROC_CALL;
 
 	if (vt596_features & FEATURE_I2CBLOCK)
 		func |= I2C_FUNC_SMBUS_I2C_BLOCK;

[-- Attachment #4: Type: text/plain, Size: 157 bytes --]

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: Question about vt82c596 SMBus driver (Via I2c Bus driver)

[Jean Delvare]

Hi Prakash,

On Wed, 1 Oct 2008 20:09:15 -0400, Mortha, Prakash wrote:
> Hi Jean,
> 
> Thank you very much for your corrections/advice.
> 
> Please find attached the patch files for i2c-viapro.c and i2c-core.c
> files for supporting I2C_SMBUS_PROC_CALL. 

For future patches, please make sure that I can apply them with patch
-p1. The header should look like:

--- linux-2.6.24.3/drivers/i2c/i2c-core.c.orig  2008-02-25 19:20:20.000000000 -0500
+++ linux-2.6.24.3/drivers/i2c/i2c-core.c       2008-10-01 19:48:25.771245369 -0400

If you're working with many patches, I can only recommend using
"quilt", so you always get the patch format right.

> 
> Please provide your comments.

Comments on the i2c-core patch:

> --- i2c-core.c	2008-02-25 19:20:20.000000000 -0500
> +++ ./../../../buildroot/build_i686/linux-2.6.24.3/drivers/i2c/i2c-core.c	2008-10-01 19:48:25.771245369 -0400
> @@ -1274,6 +1274,18 @@
>  }
>  EXPORT_SYMBOL(i2c_smbus_read_word_data);
>  
> +s32 i2c_smbus_process_call(struct i2c_client *client, u8 command, u16 value)
> +{
> +	union i2c_smbus_data data;
> +	data.word = value;
> +	if (i2c_smbus_xfer(client->adapter,client->addr,client->flags,I2C_SMBUS_WRITE,command, I2C_SMBUS_PROC_CALL, &data))

Line too long, you'll have to fold it, and there must be a space after
each comma. I recommend that you run scripts/checkpatch.pl on every
patch before you post them, this will catch formatting errors.

> +		return -1;

All these helper functions have been updated recently to transmit the
error code form i2c_smbus_xfer() instead of -1. Please do the same.
Please look at a recent kernel tree to use the same variable names so
that all the helper functions look the same.

> +	else
> +		return data.word;
> +}
> +EXPORT_SYMBOL(i2c_smbus_process_call);
> +

And please insert the new function _after_ i2c_smbus_write_word_data()
so that the two word functions stay next to each other.

> +
>  s32 i2c_smbus_write_word_data(struct i2c_client *client, u8 command, u16 value)
>  {
>  	union i2c_smbus_data data;

Documentation/i2c/smbus-protocol will also have to be updated to
mention this new function.

Comments on the i2c-viapro patch:

> --- i2c-viapro.c	2008-10-01 12:19:39.578169020 -0400
> +++ ./../../../../buildroot/build_i686/linux-2.6.24.3/drivers/i2c/busses/i2c-viapro.c	2008-10-01 19:49:02.460538263 -0400
> @@ -82,6 +82,7 @@
>  #define VT596_BYTE		0x04
>  #define VT596_BYTE_DATA		0x08
>  #define VT596_WORD_DATA		0x0C
> +#define VT596_PROC_CALL         0x10

Please use a tabulation instead of spaces.

>  #define VT596_BLOCK_DATA	0x14
>  #define VT596_I2C_BLOCK_DATA	0x34
>  
> @@ -231,6 +232,12 @@
>  		}
>  		size = VT596_WORD_DATA;
>  		break;
> +	case I2C_SMBUS_PROC_CALL:
> +		outb_p(command, SMBHSTCMD);
> +		outb_p((data->word & 0xff00) >> 8, SMBHSTDAT0);
> +		outb_p(data->word & 0xff, SMBHSTDAT1);

As already discussed, the bytes are in the wrong order. The SMBus
specification says that the LSB is always first.

> +		size = VT596_PROC_CALL;

I would add:

		read_write = I2C_SMBUS_READ;

This way...

> +		break;
>  	case I2C_SMBUS_I2C_BLOCK_DATA:
>  		if (!(vt596_features & FEATURE_I2CBLOCK))
>  			goto exit_unsupported;
> @@ -260,7 +267,7 @@
>  	if (vt596_transaction(size)) /* Error in transaction */
>  		return -1;
>  
> -	if ((read_write == I2C_SMBUS_WRITE) || (size == VT596_QUICK))
> +	if (((size != VT596_PROC_CALL) && (read_write == I2C_SMBUS_WRITE)) || (size == VT596_QUICK))

... you no longer have to change this. This is what i2c-core does for
the software emulation of SMBus over I2C, and some i2c bus drivers as
well.

>  		return 0;
>  
>  	switch (size) {
> @@ -269,6 +276,7 @@
>  		data->byte = inb_p(SMBHSTDAT0);
>  		break;
>  	case VT596_WORD_DATA:
> +	case VT596_PROC_CALL:
>  		data->word = inb_p(SMBHSTDAT0) + (inb_p(SMBHSTDAT1) << 8);
>  		break;
>  	case VT596_I2C_BLOCK_DATA:
> @@ -293,7 +301,7 @@
>  {
>  	u32 func = I2C_FUNC_SMBUS_QUICK | I2C_FUNC_SMBUS_BYTE |
>  	    I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_WORD_DATA |
> -	    I2C_FUNC_SMBUS_BLOCK_DATA;
> +	    I2C_FUNC_SMBUS_BLOCK_DATA | I2C_SMBUS_PROC_CALL;

Nitpicking: I'd add I2C_SMBUS_PROC_CALL before
I2C_FUNC_SMBUS_BLOCK_DATA, because this is where readers will expect it.

>  
>  	if (vt596_features & FEATURE_I2CBLOCK)
>  		func |= I2C_FUNC_SMBUS_I2C_BLOCK;

Also note that, in order to apply your patches upstream, I will need
a proper summary, a description of what the patch is doing and why it
is needed, and your Signed-off-by line. See section 12 (Sign your work)
of Documentation/SubmittingPatches for details.

Thanks,
-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: Question about vt82c596 SMBus driver (Via I2c Bus driver)

[Mortha, Prakash]

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

Hi Jean,

Please find attached the patches I have generated using quilt. Hopefully
this should meet the requirements.  (Please bear with me if this doesn't
meet the requirements, as this is my first time to generate a official
patch).

Thank you once again for your valuable inputs/comments. I tried
implementing all your suggestions, except for the following:

1. I didn't modify the Documentation/i2c/smbus-protocol, as this
document is already talking about the new function that I have worked
on.
2. I had to implicitly have the following lines of code in i2c-viapro.c,
because for process call functionality the bus driver has to be in write
mode initially and then has to switch to Read mode. (With out these
implicit lines of code i2c-viapro bus driver is returning immediately
after write call as per the existing control flow.)
	if ( size == VT596_PROC_CALL)
	              read_write = I2C_SMBUS_READ;
Please provide your suggestion if I could do it in a better way.

Thanks,
Prakash.

-----Original Message-----
From: Jean Delvare [mailto:khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org] 
Sent: Friday, October 03, 2008 9:21 AM
To: Mortha, Prakash
Cc: Linux I2C
Subject: Re: Question about vt82c596 SMBus driver (Via I2c Bus driver)

Hi Prakash,

On Wed, 1 Oct 2008 20:09:15 -0400, Mortha, Prakash wrote:
> Hi Jean,
> 
> Thank you very much for your corrections/advice.
> 
> Please find attached the patch files for i2c-viapro.c and i2c-core.c
> files for supporting I2C_SMBUS_PROC_CALL. 

For future patches, please make sure that I can apply them with patch
-p1. The header should look like:

--- linux-2.6.24.3/drivers/i2c/i2c-core.c.orig  2008-02-25
19:20:20.000000000 -0500
+++ linux-2.6.24.3/drivers/i2c/i2c-core.c       2008-10-01
19:48:25.771245369 -0400

If you're working with many patches, I can only recommend using
"quilt", so you always get the patch format right.

> 
> Please provide your comments.

Comments on the i2c-core patch:

> --- i2c-core.c	2008-02-25 19:20:20.000000000 -0500
> +++
./../../../buildroot/build_i686/linux-2.6.24.3/drivers/i2c/i2c-core.c
2008-10-01 19:48:25.771245369 -0400
> @@ -1274,6 +1274,18 @@
>  }
>  EXPORT_SYMBOL(i2c_smbus_read_word_data);
>  
> +s32 i2c_smbus_process_call(struct i2c_client *client, u8 command, u16
value)
> +{
> +	union i2c_smbus_data data;
> +	data.word = value;
> +	if
(i2c_smbus_xfer(client->adapter,client->addr,client->flags,I2C_SMBUS_WRI
TE,command, I2C_SMBUS_PROC_CALL, &data))

Line too long, you'll have to fold it, and there must be a space after
each comma. I recommend that you run scripts/checkpatch.pl on every
patch before you post them, this will catch formatting errors.

> +		return -1;

All these helper functions have been updated recently to transmit the
error code form i2c_smbus_xfer() instead of -1. Please do the same.
Please look at a recent kernel tree to use the same variable names so
that all the helper functions look the same.

> +	else
> +		return data.word;
> +}
> +EXPORT_SYMBOL(i2c_smbus_process_call);
> +

And please insert the new function _after_ i2c_smbus_write_word_data()
so that the two word functions stay next to each other.

> +
>  s32 i2c_smbus_write_word_data(struct i2c_client *client, u8 command,
u16 value)
>  {
>  	union i2c_smbus_data data;

Documentation/i2c/smbus-protocol will also have to be updated to
mention this new function.

Comments on the i2c-viapro patch:

> --- i2c-viapro.c	2008-10-01 12:19:39.578169020 -0400
> +++
./../../../../buildroot/build_i686/linux-2.6.24.3/drivers/i2c/busses/i2c
-viapro.c	2008-10-01 19:49:02.460538263 -0400
> @@ -82,6 +82,7 @@
>  #define VT596_BYTE		0x04
>  #define VT596_BYTE_DATA		0x08
>  #define VT596_WORD_DATA		0x0C
> +#define VT596_PROC_CALL         0x10

Please use a tabulation instead of spaces.

>  #define VT596_BLOCK_DATA	0x14
>  #define VT596_I2C_BLOCK_DATA	0x34
>  
> @@ -231,6 +232,12 @@
>  		}
>  		size = VT596_WORD_DATA;
>  		break;
> +	case I2C_SMBUS_PROC_CALL:
> +		outb_p(command, SMBHSTCMD);
> +		outb_p((data->word & 0xff00) >> 8, SMBHSTDAT0);
> +		outb_p(data->word & 0xff, SMBHSTDAT1);

As already discussed, the bytes are in the wrong order. The SMBus
specification says that the LSB is always first.

> +		size = VT596_PROC_CALL;

I would add:

		read_write = I2C_SMBUS_READ;

This way...

> +		break;
>  	case I2C_SMBUS_I2C_BLOCK_DATA:
>  		if (!(vt596_features & FEATURE_I2CBLOCK))
>  			goto exit_unsupported;
> @@ -260,7 +267,7 @@
>  	if (vt596_transaction(size)) /* Error in transaction */
>  		return -1;
>  
> -	if ((read_write == I2C_SMBUS_WRITE) || (size == VT596_QUICK))
> +	if (((size != VT596_PROC_CALL) && (read_write ==
I2C_SMBUS_WRITE)) || (size == VT596_QUICK))

... you no longer have to change this. This is what i2c-core does for
the software emulation of SMBus over I2C, and some i2c bus drivers as
well.

>  		return 0;
>  
>  	switch (size) {
> @@ -269,6 +276,7 @@
>  		data->byte = inb_p(SMBHSTDAT0);
>  		break;
>  	case VT596_WORD_DATA:
> +	case VT596_PROC_CALL:
>  		data->word = inb_p(SMBHSTDAT0) + (inb_p(SMBHSTDAT1) <<
8);
>  		break;
>  	case VT596_I2C_BLOCK_DATA:
> @@ -293,7 +301,7 @@
>  {
>  	u32 func = I2C_FUNC_SMBUS_QUICK | I2C_FUNC_SMBUS_BYTE |
>  	    I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_WORD_DATA |
> -	    I2C_FUNC_SMBUS_BLOCK_DATA;
> +	    I2C_FUNC_SMBUS_BLOCK_DATA | I2C_SMBUS_PROC_CALL;

Nitpicking: I'd add I2C_SMBUS_PROC_CALL before
I2C_FUNC_SMBUS_BLOCK_DATA, because this is where readers will expect it.

>  
>  	if (vt596_features & FEATURE_I2CBLOCK)
>  		func |= I2C_FUNC_SMBUS_I2C_BLOCK;

Also note that, in order to apply your patches upstream, I will need
a proper summary, a description of what the patch is doing and why it
is needed, and your Signed-off-by line. See section 12 (Sign your work)
of Documentation/SubmittingPatches for details.

Thanks,
-- 
Jean Delvare

[-- Attachment #2: series --]
[-- Type: application/octet-stream, Size: 14 bytes --]

patch1
patch2

[-- Attachment #3: patch1 --]
[-- Type: application/octet-stream, Size: 1316 bytes --]

Index: linux-2.6.26/drivers/i2c/i2c-core.c
===================================================================
--- linux-2.6.26.orig/drivers/i2c/i2c-core.c	2008-10-07 10:41:22.988208874 -0400
+++ linux-2.6.26/drivers/i2c/i2c-core.c	2008-10-07 10:41:44.781464558 -0400
@@ -1677,6 +1677,28 @@
 EXPORT_SYMBOL(i2c_smbus_write_word_data);
 
 /**
+ * i2c_smbus_process_call - SMBus "process call" protocol
+ * @client: Handle to slave device
+ * @command: Byte interpreted by slave
+ * @value: 16-bit "word" being written
+ *
+ * This executes the SMBus "process call" protocol, returning negative errno
+ * else a 16-bit unsigned "word" received from the device.
+ */
+s32 i2c_smbus_process_call(struct i2c_client *client, u8 command, u16 value)
+{
+	union i2c_smbus_data data;
+	int status;
+	data.word = value;
+
+	status = i2c_smbus_xfer(client->adapter, client->addr, client->flags,
+				I2C_SMBUS_WRITE, command,
+				I2C_SMBUS_PROC_CALL, &data);
+	return (status < 0) ? status : data.word;
+}
+EXPORT_SYMBOL(i2c_smbus_process_call);
+
+/**
  * i2c_smbus_read_block_data - SMBus "block read" protocol
  * @client: Handle to slave device
  * @command: Byte interpreted by slave

  # This patch implements SMBus Process Call functionality in i2c-core module. 
  # Signed-off-by: Prakash Mortha <pmortha@escient.com> 


[-- Attachment #4: patch2 --]
[-- Type: application/octet-stream, Size: 1729 bytes --]

Index: linux-2.6.26/drivers/i2c/busses/i2c-viapro.c
===================================================================
--- linux-2.6.26.orig/drivers/i2c/busses/i2c-viapro.c	2008-10-07 10:42:28.808001280 -0400
+++ linux-2.6.26/drivers/i2c/busses/i2c-viapro.c	2008-10-07 10:42:46.653029475 -0400
@@ -82,6 +82,7 @@
 #define VT596_BYTE		0x04
 #define VT596_BYTE_DATA		0x08
 #define VT596_WORD_DATA		0x0C
+#define VT596_PROC_CALL		0x10
 #define VT596_BLOCK_DATA	0x14
 #define VT596_I2C_BLOCK_DATA	0x34
 
@@ -232,6 +233,12 @@
 		}
 		size = VT596_WORD_DATA;
 		break;
+ 	case I2C_SMBUS_PROC_CALL:
+ 		outb_p(command, SMBHSTCMD);
+ 		outb_p(data->word & 0xff, SMBHSTDAT0);
+ 		outb_p((data->word & 0xff00) >> 8, SMBHSTDAT1);
+ 		size = VT596_PROC_CALL;
+ 		break;
 	case I2C_SMBUS_I2C_BLOCK_DATA:
 		if (!(vt596_features & FEATURE_I2CBLOCK))
 			goto exit_unsupported;
@@ -262,6 +269,9 @@
 	if (status)
 		return status;
 
+ 	if ( size == VT596_PROC_CALL)
+ 		read_write = I2C_SMBUS_READ;
+
 	if ((read_write == I2C_SMBUS_WRITE) || (size == VT596_QUICK))
 		return 0;
 
@@ -271,6 +281,7 @@
 		data->byte = inb_p(SMBHSTDAT0);
 		break;
 	case VT596_WORD_DATA:
+	case VT596_PROC_CALL:
 		data->word = inb_p(SMBHSTDAT0) + (inb_p(SMBHSTDAT1) << 8);
 		break;
 	case VT596_I2C_BLOCK_DATA:
@@ -295,7 +306,7 @@
 {
 	u32 func = I2C_FUNC_SMBUS_QUICK | I2C_FUNC_SMBUS_BYTE |
 	    I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_WORD_DATA |
-	    I2C_FUNC_SMBUS_BLOCK_DATA;
+	    I2C_SMBUS_PROC_CALL | I2C_FUNC_SMBUS_BLOCK_DATA;
 
 	if (vt596_features & FEATURE_I2CBLOCK)
 		func |= I2C_FUNC_SMBUS_I2C_BLOCK;
  # This patch implements SMBus Process Call functionality for VIA i2c bus driver.  
  # Signed-off-by: Prakash Mortha <pmortha@escient.com> 

[-- Attachment #5: Type: text/plain, Size: 157 bytes --]

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: Question about vt82c596 SMBus driver (Via I2c Bus driver)

[Wolfram Sang]

[-- Attachment #1.1: Type: text/plain, Size: 748 bytes --]

Hello Prakash,

On Tue, Oct 07, 2008 at 11:19:46AM -0400, Mortha, Prakash wrote:
> Hi Jean,
> 
> Please find attached the patches I have generated using quilt. Hopefully
> this should meet the requirements.  (Please bear with me if this doesn't
> meet the requirements, as this is my first time to generate a official
> patch).

No problem, we all have started somewhere :) Please do not send patches
as attachments. It is considered good style to send them inline. Just
use the 'quilt mail' command and you won't have problems with that (and
also not with wrapped lines or similar).

All the best,

   Wolfram

-- 
  Dipl.-Ing. Wolfram Sang | http://www.pengutronix.de
 Pengutronix - Linux Solutions for Science and Industry

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

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: Question about vt82c596 SMBus driver (Via I2c Bus driver)

[Jean Delvare]

Hi Prakash,

On Tue, 7 Oct 2008 11:19:46 -0400, Mortha, Prakash wrote:
> Please find attached the patches I have generated using quilt. Hopefully
> this should meet the requirements.  (Please bear with me if this doesn't
> meet the requirements, as this is my first time to generate a official
> patch).

The patches meet the technical requirements, in that I was able to
apply them. However they are missing a proper subject and description,
as well as your Signed-off-by line. Again, please see section 12 (Sign
your work) of Documentation/SubmittingPatches for what it means.

For example, the header of your first patch could be something like:

* * * * *
From: Prakash Mortha <pmortha-UATguYvZg3tBDgjK7y7TUQ@public.gmane.org>
Subject: i2c: Restore i2c_smbus_process_call function

Restore the i2c_smbus_process_call() as one driver (for the
Micronas MAP5401) will need it soon.
* * * * *

And then would come your Signed-off-by line. For your first
contribution, I want to make it easy for you and I'll be adding the
subject and description myself, but I _need_ your Signed-off-by line
before I can push both patches upstream.

> Thank you once again for your valuable inputs/comments. I tried
> implementing all your suggestions, except for the following:
> 
> 1. I didn't modify the Documentation/i2c/smbus-protocol, as this
> document is already talking about the new function that I have worked
> on.

But it doesn't mention the name of the function implementing the
transaction. That's what I wanted you to add. But that's OK, I added it
myself. I also updated Documentation/i2c/writing-clients, which was
claiming that function i2c_smbus_process_call() had been removed from
the kernel.

> 2. I had to implicitly have the following lines of code in i2c-viapro.c,
> because for process call functionality the bus driver has to be in write
> mode initially and then has to switch to Read mode. (With out these
> implicit lines of code i2c-viapro bus driver is returning immediately
> after write call as per the existing control flow.)
> 	if ( size == VT596_PROC_CALL)
> 	              read_write = I2C_SMBUS_READ;
> Please provide your suggestion if I could do it in a better way.

I was wondering about this. The "process call" is special in that it
doesn't have a read variant and a write variant. Instead, it is a
transaction combining write and read. So I was wondering if the
direction bit had any effect on the VIA chip. According to your tests,
it must be handled as a write operation at the device level. So, your
implementation is correct.

I had to fix a number of whitespace issues in the second patch. Next
time, I suggest that you run scripts/checkpatch.pl on your patch, it
will tell you about that kind of minor problems so that you can fix
them before sending the patch.

Thanks,
-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: Question about vt82c596 SMBus driver (Via I2c Bus driver)

[Mortha, Prakash]

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

Hi Jean,

Here is my Signed-off-by line:
Signed-off-by: Prakash Mortha <pmortha-UATguYvZg3tBDgjK7y7TUQ@public.gmane.org>

I tried to include this in the patch files too, but please modify it as
needed.

(This time also I am sending the patches as attachment as quilt send is
not working for me here - needs to configure mail transfer agent and
still trying to set it right).

Thanks a lot,
Prakash

-----Original Message-----
From: Jean Delvare [mailto:khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org] 
Sent: Tuesday, October 07, 2008 4:00 PM
To: Mortha, Prakash
Cc: Linux I2C
Subject: Re: Question about vt82c596 SMBus driver (Via I2c Bus driver)

Hi Prakash,

On Tue, 7 Oct 2008 11:19:46 -0400, Mortha, Prakash wrote:
> Please find attached the patches I have generated using quilt.
Hopefully
> this should meet the requirements.  (Please bear with me if this
doesn't
> meet the requirements, as this is my first time to generate a official
> patch).

The patches meet the technical requirements, in that I was able to
apply them. However they are missing a proper subject and description,
as well as your Signed-off-by line. Again, please see section 12 (Sign
your work) of Documentation/SubmittingPatches for what it means.

For example, the header of your first patch could be something like:

* * * * *
From: Prakash Mortha <pmortha-UATguYvZg3tBDgjK7y7TUQ@public.gmane.org>
Subject: i2c: Restore i2c_smbus_process_call function

Restore the i2c_smbus_process_call() as one driver (for the
Micronas MAP5401) will need it soon.
* * * * *

And then would come your Signed-off-by line. For your first
contribution, I want to make it easy for you and I'll be adding the
subject and description myself, but I _need_ your Signed-off-by line
before I can push both patches upstream.

> Thank you once again for your valuable inputs/comments. I tried
> implementing all your suggestions, except for the following:
> 
> 1. I didn't modify the Documentation/i2c/smbus-protocol, as this
> document is already talking about the new function that I have worked
> on.

But it doesn't mention the name of the function implementing the
transaction. That's what I wanted you to add. But that's OK, I added it
myself. I also updated Documentation/i2c/writing-clients, which was
claiming that function i2c_smbus_process_call() had been removed from
the kernel.

> 2. I had to implicitly have the following lines of code in
i2c-viapro.c,
> because for process call functionality the bus driver has to be in
write
> mode initially and then has to switch to Read mode. (With out these
> implicit lines of code i2c-viapro bus driver is returning immediately
> after write call as per the existing control flow.)
> 	if ( size == VT596_PROC_CALL)
> 	              read_write = I2C_SMBUS_READ;
> Please provide your suggestion if I could do it in a better way.

I was wondering about this. The "process call" is special in that it
doesn't have a read variant and a write variant. Instead, it is a
transaction combining write and read. So I was wondering if the
direction bit had any effect on the VIA chip. According to your tests,
it must be handled as a write operation at the device level. So, your
implementation is correct.

I had to fix a number of whitespace issues in the second patch. Next
time, I suggest that you run scripts/checkpatch.pl on your patch, it
will tell you about that kind of minor problems so that you can fix
them before sending the patch.

Thanks,
-- 
Jean Delvare

[-- Attachment #2: series --]
[-- Type: application/octet-stream, Size: 14 bytes --]

patch1
patch2

[-- Attachment #3: patch1 --]
[-- Type: application/octet-stream, Size: 1445 bytes --]

* * * * *
From: Prakash Mortha <pmortha@escient.com>
Subject: i2c: Restore i2c_smbus_process_call function

Restore the i2c_smbus_process_call() as one driver (for the
Micronas MAP5401) will need it soon.

Signed-off-by: Prakash Mortha <pmortha@escient.com>
* * * * *
Index: linux-2.6.26/drivers/i2c/i2c-core.c
===================================================================
--- linux-2.6.26.orig/drivers/i2c/i2c-core.c	2008-10-07 10:41:22.988208874 -0400
+++ linux-2.6.26/drivers/i2c/i2c-core.c	2008-10-07 10:41:44.781464558 -0400
@@ -1677,6 +1677,28 @@
 EXPORT_SYMBOL(i2c_smbus_write_word_data);
 
 /**
+ * i2c_smbus_process_call - SMBus "process call" protocol
+ * @client: Handle to slave device
+ * @command: Byte interpreted by slave
+ * @value: 16-bit "word" being written
+ *
+ * This executes the SMBus "process call" protocol, returning negative errno
+ * else a 16-bit unsigned "word" received from the device.
+ */
+s32 i2c_smbus_process_call(struct i2c_client *client, u8 command, u16 value)
+{
+	union i2c_smbus_data data;
+	int status;
+	data.word = value;
+
+	status = i2c_smbus_xfer(client->adapter, client->addr, client->flags,
+				I2C_SMBUS_WRITE, command,
+				I2C_SMBUS_PROC_CALL, &data);
+	return (status < 0) ? status : data.word;
+}
+EXPORT_SYMBOL(i2c_smbus_process_call);
+
+/**
  * i2c_smbus_read_block_data - SMBus "block read" protocol
  * @client: Handle to slave device
  * @command: Byte interpreted by slave

[-- Attachment #4: patch2 --]
[-- Type: application/octet-stream, Size: 1850 bytes --]

* * * * *
From: Prakash Mortha <pmortha@escient.com>
Subject: i2c: Restore i2c_smbus_process_call function

Restore the I2C_SMBUS_PROC_CALL as one driver (for the
Micronas MAP5401) will need it soon.

Signed-off-by: Prakash Mortha <pmortha@escient.com>
* * * * *
Index: linux-2.6.26/drivers/i2c/busses/i2c-viapro.c
===================================================================
--- linux-2.6.26.orig/drivers/i2c/busses/i2c-viapro.c	2008-10-07 10:42:28.808001280 -0400
+++ linux-2.6.26/drivers/i2c/busses/i2c-viapro.c	2008-10-07 10:42:46.653029475 -0400
@@ -82,6 +82,7 @@
 #define VT596_BYTE		0x04
 #define VT596_BYTE_DATA		0x08
 #define VT596_WORD_DATA		0x0C
+#define VT596_PROC_CALL		0x10
 #define VT596_BLOCK_DATA	0x14
 #define VT596_I2C_BLOCK_DATA	0x34
 
@@ -232,6 +233,12 @@
 		}
 		size = VT596_WORD_DATA;
 		break;
+ 	case I2C_SMBUS_PROC_CALL:
+ 		outb_p(command, SMBHSTCMD);
+ 		outb_p(data->word & 0xff, SMBHSTDAT0);
+ 		outb_p((data->word & 0xff00) >> 8, SMBHSTDAT1);
+ 		size = VT596_PROC_CALL;
+ 		break;
 	case I2C_SMBUS_I2C_BLOCK_DATA:
 		if (!(vt596_features & FEATURE_I2CBLOCK))
 			goto exit_unsupported;
@@ -262,6 +269,9 @@
 	if (status)
 		return status;
 
+ 	if ( size == VT596_PROC_CALL)
+ 		read_write = I2C_SMBUS_READ;
+
 	if ((read_write == I2C_SMBUS_WRITE) || (size == VT596_QUICK))
 		return 0;
 
@@ -271,6 +281,7 @@
 		data->byte = inb_p(SMBHSTDAT0);
 		break;
 	case VT596_WORD_DATA:
+	case VT596_PROC_CALL:
 		data->word = inb_p(SMBHSTDAT0) + (inb_p(SMBHSTDAT1) << 8);
 		break;
 	case VT596_I2C_BLOCK_DATA:
@@ -295,7 +306,7 @@
 {
 	u32 func = I2C_FUNC_SMBUS_QUICK | I2C_FUNC_SMBUS_BYTE |
 	    I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_WORD_DATA |
-	    I2C_FUNC_SMBUS_BLOCK_DATA;
+	    I2C_SMBUS_PROC_CALL | I2C_FUNC_SMBUS_BLOCK_DATA;
 
 	if (vt596_features & FEATURE_I2CBLOCK)
 		func |= I2C_FUNC_SMBUS_I2C_BLOCK;

[-- Attachment #5: Type: text/plain, Size: 157 bytes --]

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: Question about vt82c596 SMBus driver (Via I2c Bus driver)

[Jean Delvare]

Hi Prakash,

On Tue, 7 Oct 2008 17:02:36 -0400, Mortha, Prakash wrote:
> Hi Jean,
> 
> Here is my Signed-off-by line:
> Signed-off-by: Prakash Mortha <pmortha-UATguYvZg3tBDgjK7y7TUQ@public.gmane.org>

Great, thanks. I've queued up both patches for 2.6.28.

> I tried to include this in the patch files too, but please modify it as
> needed.

Note that the "* * * * *" in my previous mail were for readability
purpose only, you aren't supposed to put these markers in the actual
patch headers!

> (This time also I am sending the patches as attachment as quilt send is
> not working for me here - needs to configure mail transfer agent and
> still trying to set it right).

-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: Question about vt82c596 SMBus driver (Via I2c Bus driver)

[Mortha, Prakash]

Jean,

Great, Thanks a lot for all the support you gave.
I am sort of excited to see my modifications, accepted.

---
Prakash





-----Original Message-----
From: Jean Delvare [mailto:khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org] 
Sent: Wednesday, October 08, 2008 3:08 AM
To: Mortha, Prakash
Cc: Linux I2C
Subject: Re: Question about vt82c596 SMBus driver (Via I2c Bus driver)

Hi Prakash,

On Tue, 7 Oct 2008 17:02:36 -0400, Mortha, Prakash wrote:
> Hi Jean,
> 
> Here is my Signed-off-by line:
> Signed-off-by: Prakash Mortha <pmortha-UATguYvZg3tBDgjK7y7TUQ@public.gmane.org>

Great, thanks. I've queued up both patches for 2.6.28.

> I tried to include this in the patch files too, but please modify it
as
> needed.

Note that the "* * * * *" in my previous mail were for readability
purpose only, you aren't supposed to put these markers in the actual
patch headers!

> (This time also I am sending the patches as attachment as quilt send
is
> not working for me here - needs to configure mail transfer agent and
> still trying to set it right).

-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: Question about vt82c596 SMBus driver (Via I2c Bus driver)

[Mortha, Prakash]

Hi Jean,

In the patch I sent to you, I used the approach where I need to send
MSByte first and then LSByte for I2C_SMBUS_PROC_CALL case.

If we need to keep this in accordance with other cases, We need to
interchange the below lines: ( in i2c-viapro.patch)
		outb_p((data->word & 0xff00) >> 8, SMBHSTDAT0);
		outb_p(data->word & 0xff, SMBHSTDAT1);
Thanks,
Prakash



-----Original Message-----
From: Mortha, Prakash 
Sent: Wednesday, October 01, 2008 8:09 PM
To: 'Jean Delvare'
Cc: Linux I2C
Subject: RE: Question about vt82c596 SMBus driver (Via I2c Bus driver)


Hi Jean,

Thank you very much for your corrections/advice.

Please find attached the patch files for i2c-viapro.c and i2c-core.c
files for supporting I2C_SMBUS_PROC_CALL. 

Please provide your comments.

(FYI, in my test environment I had to swap MSB and LSB bytes while
dealing with I2C_SMBUS_WORD_DATA, I2C_SMBUS_PROC_CALL and
VT596_WORD_DATA/VT596_PROC_CALL, but I didn't do the swap while
generating the patch file to keep the earlier design in-tact.)

Thank you,
Prakash



-----Original Message-----
From: Jean Delvare [mailto:khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org] 
Sent: Tuesday, September 30, 2008 3:25 AM
To: Mortha, Prakash
Cc: Linux I2C
Subject: Re: Question about vt82c596 SMBus driver (Via I2c Bus driver)

Hi Prakash,

On Mon, 29 Sep 2008 11:49:30 -0400, Mortha, Prakash wrote:
> I found the reason why drivers/i2c/busses/i2c-viapro.c is not
supporting
> I2C_SMBUS_PROC_CALL. The reason I found is as per the data sheet of
Via
> Chip set the "SMBus Command Protocol field" should be set as bits 4-2
in
> "SMBus Host Control" register. But in the function 
> 
>       static int vt596_transaction(u8 size)
> 
> we are setting the "SMBus Command Protocol field" as bits 2-0:
> 
> outb_p(0x40 | size, SMBHSTCNT); (Line 134 in
> drivers/i2c/busses/i2c-viapro.c)

"size" is supposed to be already shifted at this point. If you look at
the constants:

#define VT596_QUICK		0x00
#define VT596_BYTE		0x04
#define VT596_BYTE_DATA		0x08
#define VT596_WORD_DATA		0x0C
#define VT596_BLOCK_DATA	0x14
#define VT596_I2C_BLOCK_DATA	0x34

They really are (0x0 << 2), (0x1 << 2), (0x2 <<2) etc. Maybe we should
write them that way to make it clearer.

> When I changed the statement to shift the Protocol field by 2 bits
> I2C_SMBUS_PROC_CALL worked and REPEATED START is being generated as
per
> SMBus Process call specification.
> 
> outb_p(0x40 | (size<<2), SMBHSTCNT);
> 
>  
> 
> Could you please confirm whether the reason I found is valid or not?

No, it's not. Your change just happens to work by accident in your
specific case. But the original code is correct as it is. Please
remember that this driver has been used by thousands of people for many
years now, so it's rather unlikely that the common code paths have such
a huge bug.

> Please find attached the patch I wrote. The Via Motherboard I am using
> is EPIA Ex 1500G.  

Please use "diff -u" to generate the patch, otherwise it's rather
difficult to read. But your current patch doesn't look correct anyway.
The first thing to do is to introduce a new constant for process call
transactions:

#define VT596_PROC_CALL		0x10

Then in vt596_access() you must set size = VT596_PROC_CALL for the
I2C_SMBUS_PROC_CALL case. This is currently missing in your code (you
do not set "size" at all!) and that's the reason why it didn't work.

Also, you must add I2C_SMBUS_PROC_CALL to vt596_func(), otherwise users
do not know that this protocol is supported.

-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: Question about vt82c596 SMBus driver (Via I2c Bus driver)

[Jean Delvare]

Hi Prakash,

On Thu, 2 Oct 2008 09:52:42 -0400, Mortha, Prakash wrote:
> In the patch I sent to you, I used the approach where I need to send
> MSByte first and then LSByte for I2C_SMBUS_PROC_CALL case.
> 
> If we need to keep this in accordance with other cases, We need to
> interchange the below lines: ( in i2c-viapro.patch)
> 		outb_p((data->word & 0xff00) >> 8, SMBHSTDAT0);
> 		outb_p(data->word & 0xff, SMBHSTDAT1);

The SMBus specification says that LSB always goes first when sending or
receiving a 16-byte word. i2c-core and individual i2c bus drivers must
follow this specification, even though most I2C slave devices actually
transmit the MSB first. It is the responsibility of the I2C chip driver
(or user-space in the case of i2c-dev) to swap the bytes if needed. See
for example the lm75 driver.

-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c