marker issue for i2c burst API concerns

[pabigot]

This serves as an issue that can be referenced summarizing that the as-designed API simply cannot be implemented on all platforms.

This feels wrong. I know we've had issues w/the i2c api and burst_write, do we have any way of this not being NRFX specific?

ie can we some how detect that i2c_burst_write isn't supported and fall back?

Yes, we can add a common prefix to the device data structure with capability flags, similar to what's done in the new GPIO driver with active-low flags. To my knowledge only Nordic TWIM is unable to implement the API, so that driver would have to set a flag that all other drivers keep cleared. However, that would involve a runtime rather than compile-time check. (Also nobody's done a rigorous survey to confirm that only Nordic TWIM fails to support this; the closest we've come is #11612 (comment) which may be out of date, and suggests that NXP MCUX may also be incorrect.)

Is the non-burst write code NRFX specific or would it work on any controller that didn't support burst write?

The non-burst code is generic, though it's slightly fragile in that it assumes a maximum transfer size. Changes to the driver could require changes to the generic write function.

Even if we do this, we should probably reference an issue related to needing to fix the API.

We've tried to fix the API. The conclusion of #11612 is that we can't implement the designed functionality--I2C single-transaction gather writes--on all platforms. However, gather writes are extremely useful: #19972 is a specific example where we will not be able to use a workaround based on a constant-sized stack-allocated buffer, but we certainly don't want to penalize the platforms that are capable of doing such a write.

What I've proposed is the minimum change necessary to make the code work on the platform on which I need it to work. A more complete fix would be nice, but it's not easy to provide. Do we leave it broken, or do we make it slightly better?

Originally posted by @pabigot in #19901

[pabigot]

Below is an example of a workaround for burst writes when the maximum size of the total transfer can be determined. There is no known workaround for burst writes of arbitrary size (true gather writes).

 #elif defined(DT_ST_LIS2DH_BUS_I2C)
+       /* NRF TWIM is default and does not support burst write.  We
+        * can't detect whether the I2C master uses TWI or TWIM, so
+        * use a substitute implementation unconditionally on Nordic.
+        */
+       if (IS_ENABLED(CONFIG_I2C_NRFX)) {
+               u8_t buffer[8];
+
+               /* Largest num_bytes used is 6 */
+               __ASSERT((1U + num_bytes) <= sizeof(buffer),
+                        "burst buffer too small");
+               buffer[0] = start_addr | LIS2DH_AUTOINCREMENT_ADDR;
+               memmove(buffer + 1, buf, num_bytes);
+               return i2c_write(lis2dh->bus, buffer, 1 + num_bytes,
+                                LIS2DH_BUS_ADDRESS);
+       }
        return i2c_burst_write(lis2dh->bus, LIS2DH_BUS_ADDRESS,
                               start_addr | LIS2DH_AUTOINCREMENT_ADDR,
                               buf, num_bytes);

[ravikh19]

Hi,
I was reading through this post & corresponding tagged post for i2c burst API. I wanted to summarize the known issues, propose potential solution & get feedback
Current issues:

• All the I2C master Controller does not support Multiple/Gather writes.
• The current i2c_burst_write/i2c_burst_read API accommodates only 8 bit internal address.

Based on my understanding of the problems, I wanted to propose:

• Deprecating i2c_burst_write/i2c_burst_read API, and replace it with i2c_write_write & i2c_write_read APIs to accommodate for wider internal address register. (16 bit or more), as shown below.
• Expose the i2c_write_write/i2c_read_write API only when a specific Kconfig is enabled by user when the controller supports gather write/repeating start condition.

`

static inline int i2c_write_write(const struct device *dev, uint16_t addr, 
                        const void *write_buf0, size_t num_write0, const void *write_buf1, size_t num_write1)  {

    struct i2c_msg msg[2];
    msg[0].buf = (uint8_t *)write_buf0;
    msg[0].len = num_write0;
    msg[0].flags = I2C_MSG_WRITE;

    msg[1].buf = (uint8_t *)write_buf1;
    msg[1].len = num_write1;
    msg[1].flags = I2C_MSG_WRITE | I2C_MSG_STOP;

    return i2c_transfer(dev, msg, 2, addr);
}

`

Please let me know your thoughts.

[zephyrbot]

Hi @teburd,

This issue, marked as an Enhancement, was opened a while ago and did not get any traction. It was just assigned to you based on the labels. If you don't consider yourself the right person to address this issue, please re-assing it to the right person.

Please take a moment to review if the issue is still relevant to the project. If it is, please provide feedback and direction on how to move forward. If it is not, has already been addressed, is a duplicate, or is no longer relevant, please close it with a short comment explaining the reason.

@pabigot you are also encouraged to help moving this issue forward by providing additional information and confirming this request/issue is still relevant to you.

Thanks!