Konbanwa Hideki, hi all,
> Implementing read_byte_pec using read_byte_data is impossible.
>
> SMBus read byte with PEC:
> S Addr Rd [A] [Data] A [PEC] NA P
>
> SMBus read byte data:
> S Addr Wr [A] Comm [A] S Addr Rd [A] [Data] NA P
>
> Index: i2c/kernel/i2c.h
> ===================================================================
> RCS file: /home/cvs/i2c/kernel/i2c.h,v
> retrieving revision 1.95
> diff -u -r1.95 i2c.h
> --- i2c/kernel/i2c.h 6 Sep 2005 20:08:39 -0000 1.95
> +++ i2c/kernel/i2c.h 24 Sep 2005 02:36:03 -0000
> @@ -397,6 +397,7 @@
> #define I2C_FUNC_10BIT_ADDR 0x00000002
> #define I2C_FUNC_PROTOCOL_MANGLING 0x00000004 /* I2C_M_{REV_DIR_ADDR,NOSTART,..} */
> #define I2C_FUNC_SMBUS_HWPEC_CALC 0x00000008 /* SMBus 2.0 */
> +#define I2C_FUNC_SMBUS_READ_BYTE_PEC 0x00000400 /* SMBus 2.0 */
> #define I2C_FUNC_SMBUS_READ_WORD_DATA_PEC 0x00000800 /* SMBus 2.0 */
> #define I2C_FUNC_SMBUS_WRITE_WORD_DATA_PEC 0x00001000 /* SMBus 2.0 */
> #define I2C_FUNC_SMBUS_PROC_CALL_PEC 0x00002000 /* SMBus 2.0 */
> @@ -435,12 +436,12 @@
> I2C_FUNC_SMBUS_WRITE_BLOCK_DATA_PEC)
> #define I2C_FUNC_SMBUS_WORD_DATA_PEC (I2C_FUNC_SMBUS_READ_WORD_DATA_PEC | \
> I2C_FUNC_SMBUS_WRITE_WORD_DATA_PEC)
> +#define I2C_FUNC_SMBUS_BYTE_PEC (I2C_FUNC_SMBUS_READ_BYTE_PEC | \
> + I2C_FUNC_SMBUS_WRITE_BYTE_PEC)
>
> -#define I2C_FUNC_SMBUS_READ_BYTE_PEC I2C_FUNC_SMBUS_READ_BYTE_DATA
> #define I2C_FUNC_SMBUS_WRITE_BYTE_PEC I2C_FUNC_SMBUS_WRITE_BYTE_DATA
> #define I2C_FUNC_SMBUS_READ_BYTE_DATA_PEC I2C_FUNC_SMBUS_READ_WORD_DATA
> #define I2C_FUNC_SMBUS_WRITE_BYTE_DATA_PEC I2C_FUNC_SMBUS_WRITE_WORD_DATA
> -#define I2C_FUNC_SMBUS_BYTE_PEC I2C_FUNC_SMBUS_BYTE_DATA
> #define I2C_FUNC_SMBUS_BYTE_DATA_PEC I2C_FUNC_SMBUS_WORD_DATA
>
> #define I2C_FUNC_SMBUS_EMUL (I2C_FUNC_SMBUS_QUICK | \
> Index: lm_sensors2/kernel/include/i2c-dev.h
> ===================================================================
> RCS file: /home/cvs/lm_sensors2/kernel/include/i2c-dev.h,v
> retrieving revision 1.7
> diff -u -r1.7 i2c-dev.h
> --- lm_sensors2/kernel/include/i2c-dev.h 21 Jan 2003 20:01:26 -0000 1.7
> +++ lm_sensors2/kernel/include/i2c-dev.h 24 Sep 2005 02:36:03 -0000
> @@ -55,6 +55,7 @@
> #define I2C_FUNC_10BIT_ADDR 0x00000002
> #define I2C_FUNC_PROTOCOL_MANGLING 0x00000004 /* I2C_M_{REV_DIR_ADDR,NOSTART,..} */
> #define I2C_FUNC_SMBUS_HWPEC_CALC 0x00000008 /* SMBus 2.0 */
> +#define I2C_FUNC_SMBUS_READ_BYTE_PEC 0x00000400 /* SMBus 2.0 */
> #define I2C_FUNC_SMBUS_READ_WORD_DATA_PEC 0x00000800 /* SMBus 2.0 */
> #define I2C_FUNC_SMBUS_WRITE_WORD_DATA_PEC 0x00001000 /* SMBus 2.0 */
> #define I2C_FUNC_SMBUS_PROC_CALL_PEC 0x00002000 /* SMBus 2.0 */
> @@ -93,12 +94,12 @@
> I2C_FUNC_SMBUS_WRITE_BLOCK_DATA_PEC
> #define I2C_FUNC_SMBUS_WORD_DATA_PEC I2C_FUNC_SMBUS_READ_WORD_DATA_PEC | \
> I2C_FUNC_SMBUS_WRITE_WORD_DATA_PEC
> +#define I2C_FUNC_SMBUS_BYTE_PEC I2C_FUNC_SMBUS_READ_BYTE_PEC | \
> + I2C_FUNC_SMBUS_WRITE_BYTE_PEC
>
> -#define I2C_FUNC_SMBUS_READ_BYTE_PEC I2C_FUNC_SMBUS_READ_BYTE_DATA
> #define I2C_FUNC_SMBUS_WRITE_BYTE_PEC I2C_FUNC_SMBUS_WRITE_BYTE_DATA
> #define I2C_FUNC_SMBUS_READ_BYTE_DATA_PEC I2C_FUNC_SMBUS_READ_WORD_DATA
> #define I2C_FUNC_SMBUS_WRITE_BYTE_DATA_PEC I2C_FUNC_SMBUS_WRITE_WORD_DATA
> -#define I2C_FUNC_SMBUS_BYTE_PEC I2C_FUNC_SMBUS_BYTE_DATA
> #define I2C_FUNC_SMBUS_BYTE_DATA_PEC I2C_FUNC_SMBUS_WORD_DATA
>
> #define I2C_FUNC_SMBUS_EMUL I2C_FUNC_SMBUS_QUICK | \
As a consequence of the opinion I exposed yesterday, I would like to
propose a different fix: I think we should simply drop all
I2C_FUNC_SMBUS_*_PEC constants. i2c clients are not supposed to check
whether an adapter can do PEC on a specific SMBus transaction. Instead,
they should declare themselves PEC-capable (flags |= I2C_CLIENT_PEC)
and ask for the regular (non-PEC) transaction, leaving it to the bus
driver to do PEC or not.
Attached are three patches doing that: one for i2c CVS, one for
lm_sensors2 CVS and one for linux-2.6. Note that the latter should be
applied after the i2c-i801 patch I posted on this list earlier today.
In details, here's what these patches do:
i2c-func-pec-i2c-CVS.diff:
* i2c-algo-bit.c: No need to explicitely declare the ability to do
Block Process Call and Block Read with PEC.
* i2c.h: Drop I2C_FUNC_SMBUS_*_PEC constants.
i2c-func-pec-lm_sensors2-CVS.diff:
* i2c-i801.c: Rely on I2C_CLIENT_PEC to detect the PEC support in core.
No need to explicitely declare the ability to do Block Read/Write with
PEC.
* smbus-arp.c: Use a different way to detect SW PEC capabilities. It's
probably not a definitive fix, but that will do for now.
* i2c-dev.h: Drop I2C_FUNC_SMBUS_*_PEC constants.
* i2cdump.c: Do not check explictely for individual PEC transactions
support. Check for global PEC support instead. It's not reliable at
this point (which is why I made it a warning rather than an error) so
the fix isn't definitive either.
i2c-func-pec-linux-2.6.diff:
* i2c.h: Drop I2C_FUNC_SMBUS_*_PEC constants.
* i2c-i801.c: Rely on I2C_CLIENT_PEC to detect the PEC support in core.
No need to explicitely declare the ability to do Block Read/Write with
PEC.
Are there objections to this approach? If not, I'll just apply these
patches.
Note that the bug Hideki found here (read_byte_pec cannot be emulated
with read_byte_data) seems to be a proof that the design of the current
PEC implementation is broken. This implementation (or at least the
software PEC part) was relying on the fact that one transaction
"size" (really more a transaction type) would always lead to another,
unique "size" when adding PEC. Now we see that this isn't true anymore
for size = byte: the new size is byte_data for writes, and something
different (no #define yet) for reads. This means that we will need to
rewrite a part of the code if we want to support SW PEC on byte reads.
I'll work on this. My idea is that we shouldn't be considering PEC
transactions as new or reused "sizes", but as an independant flag. I'll
see what code changes are needed to do this.
Arigato/thanks,
--
Jean Delvare
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: i2c-func-pec-i2c-CVS.diff
Url: http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20050926/62498170/attachment.pl
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: i2c-func-pec-lm_sensors2-CVS.diff
Url: http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20050926/62498170/attachment-0001.pl
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: i2c-func-pec-linux-2.6.diff
Url: http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20050926/62498170/attachment-0002.pl
