Skip to content

Commit 2476211

Browse files
teburdnashif
teburd
authored and
nashif
committed
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 <thomas.burdick@intel.com>
1 parent fab01c0 commit 2476211

File tree

2 files changed

+87
-43
lines changed

2 files changed

+87
-43
lines changed

‎drivers/i2c/Kconfig‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -97,6 +97,20 @@ config I2C_RTIO_CQ_SIZE
9797
is going to be 4 given the device address, register address, and a value
9898
to be read or written.
9999

100+
config I2C_RTIO_FALLBACK_MSGS
101+
int "Number of available i2c_msg structs for the default handler to use"
102+
default 4
103+
help
104+
When RTIO is used with a driver that does not yet implement the submit API
105+
natively the submissions are converted back to struct i2c_msg values that
106+
are given to i2c_transfer. This requires some number of msgs be available to convert
107+
the submissions into on the stack. MISRA rules dictate we must know this in
108+
advance.
109+
110+
In all likelihood 4 is going to work for everyone, but in case you do end up with
111+
an issue where you are using RTIO, your driver does not implement submit natively,
112+
and get an error relating to not enough i2c msgs this is the Kconfig to manipulate.
113+
100114
endif # I2C_RTIO
101115

102116

‎drivers/i2c/i2c_rtio_default.c‎

Lines changed: 73 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -12,97 +12,127 @@
1212
#include <zephyr/logging/log.h>
1313
LOG_MODULE_DECLARE(i2c_rtio, CONFIG_I2C_LOG_LEVEL);
1414

15-
static int i2c_iodev_submit_rx(struct rtio_iodev_sqe *iodev_sqe, struct i2c_msg msgs[2],
16-
uint8_t *num_msgs)
15+
static inline void i2c_msg_from_rx(const struct rtio_iodev_sqe *iodev_sqe, struct i2c_msg *msg)
1716
{
1817
__ASSERT_NO_MSG(iodev_sqe->sqe.op == RTIO_OP_RX);
1918

20-
msgs[0].buf = iodev_sqe->sqe.rx.buf;
21-
msgs[0].len = iodev_sqe->sqe.rx.buf_len;
22-
msgs[0].flags =
19+
msg->buf = iodev_sqe->sqe.rx.buf;
20+
msg->len = iodev_sqe->sqe.rx.buf_len;
21+
msg->flags =
2322
((iodev_sqe->sqe.iodev_flags & RTIO_IODEV_I2C_STOP) ? I2C_MSG_STOP : 0) |
2423
((iodev_sqe->sqe.iodev_flags & RTIO_IODEV_I2C_RESTART) ? I2C_MSG_RESTART : 0) |
2524
((iodev_sqe->sqe.iodev_flags & RTIO_IODEV_I2C_10_BITS) ? I2C_MSG_ADDR_10_BITS : 0) |
2625
I2C_MSG_READ;
27-
*num_msgs = 1;
28-
return 0;
2926
}
3027

31-
static int i2c_iodev_submit_tx(struct rtio_iodev_sqe *iodev_sqe, struct i2c_msg msgs[2],
32-
uint8_t *num_msgs)
28+
static inline void i2c_msg_from_tx(const struct rtio_iodev_sqe *iodev_sqe, struct i2c_msg *msg)
3329
{
3430
__ASSERT_NO_MSG(iodev_sqe->sqe.op == RTIO_OP_TX);
3531

36-
msgs[0].buf = (uint8_t *)iodev_sqe->sqe.tx.buf;
37-
msgs[0].len = iodev_sqe->sqe.tx.buf_len;
38-
msgs[0].flags =
32+
msg->buf = (uint8_t *)iodev_sqe->sqe.tx.buf;
33+
msg->len = iodev_sqe->sqe.tx.buf_len;
34+
msg->flags =
3935
((iodev_sqe->sqe.iodev_flags & RTIO_IODEV_I2C_STOP) ? I2C_MSG_STOP : 0) |
4036
((iodev_sqe->sqe.iodev_flags & RTIO_IODEV_I2C_RESTART) ? I2C_MSG_RESTART : 0) |
4137
((iodev_sqe->sqe.iodev_flags & RTIO_IODEV_I2C_10_BITS) ? I2C_MSG_ADDR_10_BITS : 0) |
4238
I2C_MSG_WRITE;
43-
*num_msgs = 1;
44-
return 0;
4539
}
4640

47-
static int i2c_iodev_submit_tiny_tx(struct rtio_iodev_sqe *iodev_sqe, struct i2c_msg msgs[2],
48-
uint8_t *num_msgs)
41+
static inline void i2c_msg_from_tiny_tx(const struct rtio_iodev_sqe *iodev_sqe, struct i2c_msg *msg)
4942
{
5043
__ASSERT_NO_MSG(iodev_sqe->sqe.op == RTIO_OP_TINY_TX);
5144

52-
msgs[0].buf = (uint8_t *)iodev_sqe->sqe.tiny_tx.buf;
53-
msgs[0].len = iodev_sqe->sqe.tiny_tx.buf_len;
54-
msgs[0].flags =
45+
msg->buf = (uint8_t *)iodev_sqe->sqe.tiny_tx.buf;
46+
msg->len = iodev_sqe->sqe.tiny_tx.buf_len;
47+
msg->flags =
5548
((iodev_sqe->sqe.iodev_flags & RTIO_IODEV_I2C_STOP) ? I2C_MSG_STOP : 0) |
5649
((iodev_sqe->sqe.iodev_flags & RTIO_IODEV_I2C_RESTART) ? I2C_MSG_RESTART : 0) |
5750
((iodev_sqe->sqe.iodev_flags & RTIO_IODEV_I2C_10_BITS) ? I2C_MSG_ADDR_10_BITS : 0) |
5851
I2C_MSG_WRITE;
59-
*num_msgs = 1;
60-
return 0;
6152
}
6253

63-
void i2c_iodev_submit_work_handler(struct rtio_iodev_sqe *iodev_sqe)
54+
void i2c_iodev_submit_work_handler(struct rtio_iodev_sqe *txn_first)
6455
{
65-
const struct i2c_dt_spec *dt_spec = (const struct i2c_dt_spec *)iodev_sqe->sqe.iodev->data;
56+
const struct i2c_dt_spec *dt_spec = (const struct i2c_dt_spec *)txn_first->sqe.iodev->data;
6657
const struct device *dev = dt_spec->bus;
6758

68-
LOG_DBG("Sync RTIO work item for: %p", (void *)iodev_sqe);
69-
70-
struct rtio_iodev_sqe *transaction_current = iodev_sqe;
71-
struct i2c_msg msgs[2];
72-
uint8_t num_msgs;
59+
LOG_DBG("Sync RTIO work item for: %p", (void *)txn_first);
60+
uint32_t num_msgs = 0;
7361
int rc = 0;
62+
struct rtio_iodev_sqe *txn_last = txn_first;
7463

64+
/* We allocate the i2c_msg's on the stack, to do so
65+
* the count of messages needs to be determined to
66+
* ensure we don't go over the statically sized array.
67+
*/
7568
do {
76-
/* Convert the iodev_sqe back to an i2c_msg */
77-
switch (transaction_current->sqe.op) {
69+
switch (txn_last->sqe.op) {
70+
case RTIO_OP_RX:
71+
case RTIO_OP_TX:
72+
case RTIO_OP_TINY_TX:
73+
num_msgs++;
74+
break;
75+
default:
76+
LOG_ERR("Invalid op code %d for submission %p", txn_last->sqe.op,
77+
(void *)&txn_last->sqe);
78+
rc = -EIO;
79+
break;
80+
}
81+
txn_last = rtio_txn_next(txn_last);
82+
} while (rc == 0 && txn_last != NULL);
83+
84+
if (rc != 0) {
85+
rtio_iodev_sqe_err(txn_first, rc);
86+
return;
87+
}
88+
89+
/* Allocate msgs on the stack, MISRA doesn't like VLAs so we need a statically
90+
* sized array here. It's pretty unlikely we have more than 4 i2c messages
91+
* in a transaction as we typically would only have 2, one to write a
92+
* register address, and another to read/write the register into an array
93+
*/
94+
if (num_msgs > CONFIG_I2C_RTIO_FALLBACK_MSGS) {
95+
LOG_ERR("At most CONFIG_I2C_RTIO_FALLBACK_MSGS"
96+
" submissions in a transaction are"
97+
" allowed in the default handler");
98+
rtio_iodev_sqe_err(txn_first, -ENOMEM);
99+
return;
100+
}
101+
struct i2c_msg msgs[CONFIG_I2C_RTIO_FALLBACK_MSGS];
102+
103+
rc = 0;
104+
txn_last = txn_first;
105+
106+
/* Copy the transaction into the stack allocated msgs */
107+
for (int i = 0; i < num_msgs; i++) {
108+
switch (txn_last->sqe.op) {
78109
case RTIO_OP_RX:
79-
rc = i2c_iodev_submit_rx(transaction_current, msgs, &num_msgs);
110+
i2c_msg_from_rx(txn_last, &msgs[i]);
80111
break;
81112
case RTIO_OP_TX:
82-
rc = i2c_iodev_submit_tx(transaction_current, msgs, &num_msgs);
113+
i2c_msg_from_tx(txn_last, &msgs[i]);
83114
break;
84115
case RTIO_OP_TINY_TX:
85-
rc = i2c_iodev_submit_tiny_tx(transaction_current, msgs, &num_msgs);
116+
i2c_msg_from_tiny_tx(txn_last, &msgs[i]);
86117
break;
87118
default:
88-
LOG_ERR("Invalid op code %d for submission %p", transaction_current->sqe.op,
89-
(void *)&transaction_current->sqe);
90119
rc = -EIO;
91120
break;
92121
}
93122

94-
if (rc == 0) {
95-
__ASSERT_NO_MSG(num_msgs > 0);
123+
txn_last = rtio_txn_next(txn_last);
124+
}
96125

97-
rc = i2c_transfer(dev, msgs, num_msgs, dt_spec->addr);
98-
transaction_current = rtio_txn_next(transaction_current);
99-
}
100-
} while (rc == 0 && transaction_current != NULL);
126+
if (rc == 0) {
127+
__ASSERT_NO_MSG(num_msgs > 0);
128+
129+
rc = i2c_transfer(dev, msgs, num_msgs, dt_spec->addr);
130+
}
101131

102132
if (rc != 0) {
103-
rtio_iodev_sqe_err(iodev_sqe, rc);
133+
rtio_iodev_sqe_err(txn_first, rc);
104134
} else {
105-
rtio_iodev_sqe_ok(iodev_sqe, 0);
135+
rtio_iodev_sqe_ok(txn_first, 0);
106136
}
107137
}
108138

0 commit comments

Comments
 (0)