[i2c] Question about vt82c596 SMBus driver (Via I2c Bus driver)

Mortha, Prakash pmortha at escient.com
Thu Oct 2 02:09:15 CEST 2008


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 at linux-fr.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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: i2c-core.patch
Type: application/octet-stream
Size: 682 bytes
Desc: i2c-core.patch
Url : http://lists.lm-sensors.org/pipermail/i2c/attachments/20081001/e34a1849/attachment.obj 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: i2c-viapro.patch
Type: application/octet-stream
Size: 1553 bytes
Desc: i2c-viapro.patch
Url : http://lists.lm-sensors.org/pipermail/i2c/attachments/20081001/e34a1849/attachment-0001.obj 


More information about the i2c mailing list