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

Mortha, Prakash pmortha at escient.com
Tue Oct 7 17:19:46 CEST 2008


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 at linux-fr.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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: series
Type: application/octet-stream
Size: 14 bytes
Desc: series
Url : http://lists.lm-sensors.org/pipermail/i2c/attachments/20081007/3be312fb/attachment.obj 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: patch1
Type: application/octet-stream
Size: 1316 bytes
Desc: patch1
Url : http://lists.lm-sensors.org/pipermail/i2c/attachments/20081007/3be312fb/attachment-0001.obj 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: patch2
Type: application/octet-stream
Size: 1729 bytes
Desc: patch2
Url : http://lists.lm-sensors.org/pipermail/i2c/attachments/20081007/3be312fb/attachment-0002.obj 


More information about the i2c mailing list