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
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/i2c
