i2c: Bug fixes for default RTIO handler

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
 
 

drivers/i2c/i2c_rtio_default.c

@@ -12,121 +12,127 @@
 #include <zephyr/logging/log.h>
 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;
 }
 
-static int i2c_iodev_submit_txrx(struct rtio_iodev_sqe *iodev_sqe, struct i2c_msg msgs[2],
-				 uint8_t *num_msgs)
+void i2c_iodev_submit_work_handler(struct rtio_iodev_sqe *txn_first)
 {
-	__ASSERT_NO_MSG(iodev_sqe->sqe.op == RTIO_OP_TXRX);
-
-	msgs[0].buf = (uint8_t *)iodev_sqe->sqe.txrx.tx_buf;
-	msgs[0].len = iodev_sqe->sqe.txrx.buf_len;
-	msgs[0].flags =
-		((iodev_sqe->sqe.iodev_flags & RTIO_IODEV_I2C_10_BITS) ? I2C_MSG_ADDR_10_BITS : 0) |
-		I2C_MSG_WRITE;
-	msgs[1].buf = iodev_sqe->sqe.txrx.rx_buf;
-	msgs[1].len = iodev_sqe->sqe.txrx.buf_len;
-	msgs[1].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 = 2;
-	return 0;
-}
-
-void i2c_iodev_submit_work_handler(struct rtio_iodev_sqe *iodev_sqe)
-{
-	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:
-			rc = i2c_iodev_submit_rx(transaction_current, msgs, &num_msgs);
+		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:
+			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);
-			break;
-		case RTIO_OP_TXRX:
-			rc = i2c_iodev_submit_txrx(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);
 	}
 }
 

tests/subsys/rtio/rtio_i2c/src/main.cpp

@@ -117,42 +117,6 @@ ZTEST(rtio_i2c, test_fallback_submit_tiny_tx)
 	rtio_cqe_release(&test_rtio_ctx, cqe);
 }
 
-ZTEST(rtio_i2c, test_fallback_submit_txrx)
-{
-	uint8_t tx_data[] = {0x01, 0x02, 0x03};
-	uint8_t rx_data[ARRAY_SIZE(tx_data)] = {0};
-	struct rtio_sqe *sqe = rtio_sqe_acquire(&test_rtio_ctx);
-
-	blocking_emul_i2c_transfer_fake.custom_fake =
-		[&tx_data](const struct emul *, struct i2c_msg *msgs, int msg_count, int) {
-			zassert_equal(2, msg_count);
-			// First message should be a 'tx'
-			zassert_equal(ARRAY_SIZE(tx_data), msgs[0].len);
-			zassert_mem_equal(tx_data, msgs[0].buf, msgs[0].len);
-			zassert_equal(I2C_MSG_WRITE, msgs[0].flags);
-			// Second message should be an 'rx'
-			zassert_equal(ARRAY_SIZE(tx_data), msgs[1].len);
-			zassert_equal(I2C_MSG_READ | I2C_MSG_STOP, msgs[1].flags);
-			for (uint8_t i = 0; i < msgs[1].len; ++i) {
-				msgs[1].buf[i] = msgs[0].buf[i];
-			}
-			return 0;
-		};
-
-	zassert_not_null(sqe);
-	rtio_sqe_prep_transceive(sqe, &blocking_emul_iodev, RTIO_PRIO_NORM, tx_data, rx_data,
-				 ARRAY_SIZE(tx_data), NULL);
-	zassert_ok(rtio_submit(&test_rtio_ctx, 1));
-	zassert_equal(1, blocking_emul_i2c_transfer_fake.call_count);
-
-	struct rtio_cqe *cqe = rtio_cqe_consume_block(&test_rtio_ctx);
-
-	zassert_ok(cqe->result);
-	zassert_mem_equal(tx_data, rx_data, ARRAY_SIZE(tx_data));
-
-	rtio_cqe_release(&test_rtio_ctx, cqe);
-}
-
 ZTEST(rtio_i2c, test_fallback_submit_rx)
 {
 	uint8_t expected_buffer[] = {0x00, 0x01, 0x02};
@@ -237,7 +201,7 @@ ZTEST(rtio_i2c, test_fallback_transaction)
 	phase1->flags |= RTIO_SQE_TRANSACTION;
 
 	zassert_ok(rtio_submit(&test_rtio_ctx, 2));
-	zassert_equal(2, blocking_emul_i2c_transfer_fake.call_count);
+	zassert_equal(1, blocking_emul_i2c_transfer_fake.call_count);
 
 	struct rtio_cqe *cqe;