i2c_burst_write on nrf51 is not a burst write

[pabigot]

Based on bus snooping it appears that on nrf51_ble400 an i2c_burst_write produces two bus transactions: a write of the one byte address, followed by a write of the multi-byte value. On a CCS811 this produces an error due to a write to an invalid register (as the multi-byte value did not start with a register).

Packing the register address and data together with i2c_write produces the correct behavior.

[pabigot]

Reviewing the code it appears the extended I2C API from ef26bf7 is based on a generalization that does not account for all the ways subordinate devices might use the protocol.

An I2C control interface based on request/response operations exposing "registers" of various sizes that are identified by octet (or multi-octet) values is common, but not universal. Fundamentally there are read operations and there are write operations. The relation between them is determined by the subordinate device.

The operation identified as i2c_burst_read is a reasonable abstraction (though IMO badly named: there is no "burst" involved). It initiates a write that tells the device "I'm about to issue a read; here's what I want you to return", followed by a read that collects the response. This works on devices where "what I want you to return" is identified by an 8-bit value. That a distinct i2c_burst_read16 is required for devices that identify content by 16-bit values suggests the true nature of the transaction.

The operation identified as i2c_burst_write is fundamentally wrong. It produces a write that is identical to the first transaction of i2c_burst_read, followed by a second transaction that is intended to continue the first write. For some devices this can be made to work by using repeated start however the implementation of i2c_burst_write does not set that flag.

Even if it did (and the Nordic driver seems to implicitly add it), a device is under no obligation to respect repeated starts. In the case of the CCS811 a write of 0x05 followed by a repeated-start write of environmental data starting with 0x61 produces an error, because while 0x05 is a valid "register" 0x61 is not.

i2c_write_byte works because the implementation generates a single write transaction.

Things to consider for issue #4040 if this is to be addressed:

• i2c_burst_write should add IC2_MSG_REPEAT to the first transaction to clearly indicate the intent of combining the transactions;
• update the documentation for the "burst" write functions to note that they do not generate a unified write transaction and that for it to work the device must respect repeated start for writes;
• consider renaming these functions to something like i2c_reg{8,16}_{read,write} to more accurately reflect their assumption that the subordinate device implements an API based on a register access model.

[pabigot]

After further review, the burst and reg read16/write16 functions should be removed. They're not used anywhere in the tree, and they assume that a device that uses 16-bit addressing expects to receive it in little-endian byte order. The burst and reg read/write_addr functions should go too. The only one used is burst_read_addr once in eth_sam_gmac where the implementation could just as easily use i2c_transfer.

This would remove eight unnecessary functions from the API, simplifying it so people have a better chance of figuring out how to use it properly.

[vikrant8052]

@pabigot Thanks for linking #11720

[pabigot]

I've checked i2c_burst_write on various platforms:

• Nordic TWI (nrf51_ble400) and TWIM (particle_xenon) both fail (this issue). For both drivers two separate bus transactions are generated, which is incorrect.

• MCUX (frdm_k64f) fails (driver: i2c_mcux: unable to perform more than one write transfer like i2c_burst_write #8684). The attempt to initiate the second transfer is rejected because the device is busy. If I2C_MSG_RESTART is added to the flags of the second transfer the transfer proceeds after a 1.75 ms delay, as a distinct bus transaction, which is incorrect.

• ST L4 (nucleo_l476rg) succeeds: the implementation correctly combines the two writes into a single bus transaction. However, if I2C_MSG_RESTART is added to the flags of the second transfer two distinct write transactions are generated, which would be a fail.

• EFR32 (efr32mg_sltb004a) succeeds: the implementation correctly combines the two writes into a single bus transaction. The driver implementation appears to ignore I2C_MSG_RESTART.

Two fail, one succeeds but would fail if the restart flag were added as proposed above, one succeeds but ignores restart.

[galak]

@pabigot will put up a PR for proposed API for dealing with this issue.

[pabigot]

This has been resolved within the constraints of Zephyr process: the problems with the API including that i2c_burst_write is not globally functional have been documented, and a warning placed in the documentation for the functions that don't work. The core functions have not been deprecated because there is no functional replacement. At some point the I2C API will need to be revisited and reworked, possibly replacing i2c_transfer with i2c_write_read and potentially i2c_write_write to handle the existing sample uses where i2c_burst_write is used. The latter will require bypassing vendor HALs when those wrappers don't support gather write operations.