From 77186ea0f901560a83f6847f5767b1873d4da3bd Mon Sep 17 00:00:00 2001 From: Tom Burdick Date: Fri, 18 Oct 2024 09:55:25 -0500 Subject: [PATCH] i2c: Fix default RTIO handler transactions Transactions from RTIO should result in single calls to i2c_transfer. This corrects the default handler to first count the number of submissions in the transaction, allocate on the stack, and then copy over each submission to an equivalent i2c_msg. It also cleans up the helper functions to be infallible, taking only the submission and msg to copy to. Signed-off-by: Tom Burdick --- drivers/i2c/Kconfig | 14 ++++ drivers/i2c/i2c_rtio_default.c | 115 +++++++++++++++++++++------------ 2 files changed, 86 insertions(+), 43 deletions(-) diff --git a/drivers/i2c/Kconfig b/drivers/i2c/Kconfig index 3651b4b09ee719a..86e7165bf080c91 100644 --- a/drivers/i2c/Kconfig +++ b/drivers/i2c/Kconfig @@ -97,6 +97,20 @@ config I2C_RTIO_CQ_SIZE is going to be 4 given the device address, register address, and a value to be read or written. +config I2C_RTIO_FALLBACK_MSGS + int "Number of available i2c_msg structs for the default handler to use" + default 4 + help + When RTIO is used with a driver that does not yet implement the submit API + natively the submissions are converted back to struct i2c_msg values that + are given to i2c_transfer. This requires some number of msgs be available to convert + the submissions into on the stack. MISRA rules dictate we must know this in + advance. + + In all likelihood 4 is going to work for everyone, but in case you do end up with + an issue where you are using RTIO, your driver does not implement submit natively, + and get an error relating to not enough i2c msgs this is the Kconfig to manipulate. + endif # I2C_RTIO diff --git a/drivers/i2c/i2c_rtio_default.c b/drivers/i2c/i2c_rtio_default.c index 048b5b42d79a59f..1464e7855fed975 100644 --- a/drivers/i2c/i2c_rtio_default.c +++ b/drivers/i2c/i2c_rtio_default.c @@ -12,97 +12,126 @@ #include LOG_MODULE_DECLARE(i2c_rtio, CONFIG_I2C_LOG_LEVEL); -static int i2c_iodev_submit_rx(struct rtio_iodev_sqe *iodev_sqe, struct i2c_msg msgs[2], - uint8_t *num_msgs) +static inline void i2c_msg_from_rx(const struct rtio_iodev_sqe *iodev_sqe, struct i2c_msg *msg) { __ASSERT_NO_MSG(iodev_sqe->sqe.op == RTIO_OP_RX); - msgs[0].buf = iodev_sqe->sqe.rx.buf; - msgs[0].len = iodev_sqe->sqe.rx.buf_len; - msgs[0].flags = + msg->buf = iodev_sqe->sqe.rx.buf; + msg->len = iodev_sqe->sqe.rx.buf_len; + msg->flags = ((iodev_sqe->sqe.iodev_flags & RTIO_IODEV_I2C_STOP) ? I2C_MSG_STOP : 0) | ((iodev_sqe->sqe.iodev_flags & RTIO_IODEV_I2C_RESTART) ? I2C_MSG_RESTART : 0) | ((iodev_sqe->sqe.iodev_flags & RTIO_IODEV_I2C_10_BITS) ? I2C_MSG_ADDR_10_BITS : 0) | I2C_MSG_READ; - *num_msgs = 1; - return 0; } -static int i2c_iodev_submit_tx(struct rtio_iodev_sqe *iodev_sqe, struct i2c_msg msgs[2], - uint8_t *num_msgs) +static inline void i2c_msg_from_tx(const struct rtio_iodev_sqe *iodev_sqe, struct i2c_msg *msg) { __ASSERT_NO_MSG(iodev_sqe->sqe.op == RTIO_OP_TX); - msgs[0].buf = (uint8_t *)iodev_sqe->sqe.tx.buf; - msgs[0].len = iodev_sqe->sqe.tx.buf_len; - msgs[0].flags = + msg->buf = (uint8_t *)iodev_sqe->sqe.tx.buf; + msg->len = iodev_sqe->sqe.tx.buf_len; + msg->flags = ((iodev_sqe->sqe.iodev_flags & RTIO_IODEV_I2C_STOP) ? I2C_MSG_STOP : 0) | ((iodev_sqe->sqe.iodev_flags & RTIO_IODEV_I2C_RESTART) ? I2C_MSG_RESTART : 0) | ((iodev_sqe->sqe.iodev_flags & RTIO_IODEV_I2C_10_BITS) ? I2C_MSG_ADDR_10_BITS : 0) | I2C_MSG_WRITE; - *num_msgs = 1; - return 0; } -static int i2c_iodev_submit_tiny_tx(struct rtio_iodev_sqe *iodev_sqe, struct i2c_msg msgs[2], - uint8_t *num_msgs) +static inline void i2c_msg_from_tiny_tx(const struct rtio_iodev_sqe *iodev_sqe, struct i2c_msg *msg) { __ASSERT_NO_MSG(iodev_sqe->sqe.op == RTIO_OP_TINY_TX); - msgs[0].buf = (uint8_t *)iodev_sqe->sqe.tiny_tx.buf; - msgs[0].len = iodev_sqe->sqe.tiny_tx.buf_len; - msgs[0].flags = + msg->buf = (uint8_t *)iodev_sqe->sqe.tiny_tx.buf; + msg->len = iodev_sqe->sqe.tiny_tx.buf_len; + msg->flags = ((iodev_sqe->sqe.iodev_flags & RTIO_IODEV_I2C_STOP) ? I2C_MSG_STOP : 0) | ((iodev_sqe->sqe.iodev_flags & RTIO_IODEV_I2C_RESTART) ? I2C_MSG_RESTART : 0) | ((iodev_sqe->sqe.iodev_flags & RTIO_IODEV_I2C_10_BITS) ? I2C_MSG_ADDR_10_BITS : 0) | I2C_MSG_WRITE; - *num_msgs = 1; - return 0; } -void i2c_iodev_submit_work_handler(struct rtio_iodev_sqe *iodev_sqe) +void i2c_iodev_submit_work_handler(struct rtio_iodev_sqe *txn_first) { - const struct i2c_dt_spec *dt_spec = (const struct i2c_dt_spec *)iodev_sqe->sqe.iodev->data; + const struct i2c_dt_spec *dt_spec = (const struct i2c_dt_spec *)txn_first->sqe.iodev->data; const struct device *dev = dt_spec->bus; - LOG_DBG("Sync RTIO work item for: %p", (void *)iodev_sqe); - - struct rtio_iodev_sqe *transaction_current = iodev_sqe; - struct i2c_msg msgs[2]; - uint8_t num_msgs; + LOG_DBG("Sync RTIO work item for: %p", (void *)txn_first); + uint32_t num_msgs = 0; int rc = 0; + struct rtio_iodev_sqe *txn_last = txn_first; + /* We allocate the i2c_msg's on the stack, to do so + * the count of messages needs to be determined to + * ensure we don't go over the statically sized array. + */ do { - /* Convert the iodev_sqe back to an i2c_msg */ - switch (transaction_current->sqe.op) { + switch (txn_last->sqe.op) { + case RTIO_OP_RX: + case RTIO_OP_TX: + case RTIO_OP_TINY_TX: + num_msgs++; + break; + default: + LOG_ERR("Invalid op code %d for submission %p", txn_last->sqe.op, + (void *)&txn_last->sqe); + rc = -EIO; + break; + } + txn_last = rtio_txn_next(txn_last); + } while (rc == 0 && txn_last != NULL); + + if (rc != 0) { + rtio_iodev_sqe_err(txn_first, rc); + return; + } + + /* Allocate msgs on the stack, MISRA doesn't like VLAs so we need a statically + * sized array here. It's pretty unlikely we have more than 4 i2c messages + * in a transaction as we typically would only have 2, one to write a + * register address, and another to read/write the register into an array + */ + if (num_msgs > CONFIG_I2C_RTIO_FALLBACK_MSGS) { + LOG_ERR("At most " CONFIG_I2C_RTIO_FALLBACK_MSGS " submissions in a transaction are " + "allowed in the default handler"); + rtio_iodev_sqe_err(txn_first, -ENOMEM); + return; + } + struct i2c_msg msgs[CONFIG_I2C_RTIO_FALLBACK_MSGS]; + + rc = 0; + txn_last = txn_first; + + /* Copy the transaction into the stack allocated msgs */ + for (int i = 0; i < num_msgs; i++) { + switch (txn_last->sqe.op) { case RTIO_OP_RX: - rc = i2c_iodev_submit_rx(transaction_current, msgs, &num_msgs); + i2c_msg_from_rx(txn_last, &msgs[i]); break; case RTIO_OP_TX: - rc = i2c_iodev_submit_tx(transaction_current, msgs, &num_msgs); + i2c_msg_from_tx(txn_last, &msgs[i]); break; case RTIO_OP_TINY_TX: - rc = i2c_iodev_submit_tiny_tx(transaction_current, msgs, &num_msgs); + i2c_msg_from_tiny_tx(txn_last, &msgs[i]); break; default: - LOG_ERR("Invalid op code %d for submission %p", transaction_current->sqe.op, - (void *)&transaction_current->sqe); rc = -EIO; break; } - if (rc == 0) { - __ASSERT_NO_MSG(num_msgs > 0); + txn_last = rtio_txn_next(txn_last); + } - rc = i2c_transfer(dev, msgs, num_msgs, dt_spec->addr); - transaction_current = rtio_txn_next(transaction_current); - } - } while (rc == 0 && transaction_current != NULL); + if (rc == 0) { + __ASSERT_NO_MSG(num_msgs > 0); + + rc = i2c_transfer(dev, msgs, num_msgs, dt_spec->addr); + } if (rc != 0) { - rtio_iodev_sqe_err(iodev_sqe, rc); + rtio_iodev_sqe_err(txn_first, rc); } else { - rtio_iodev_sqe_ok(iodev_sqe, 0); + rtio_iodev_sqe_ok(txn_first, 0); } }