Add the support of the I2C_M_STOP flag in i2c_msg by splitting
i2c_dw_xfer() in two: __i2c_dw_xfer_one_part() for the core transfer logic
and i2c_dw_xfer() for handling the high-level transaction management.
In detail __i2c_dw_xfer_one_part() starts a transaction and wait for its
completion, either with a STOP on the bus or an error. i2c_dw_xfer()
loops over the messages to search for the I2C_M_STOP flag and calls
__i2c_dw_xfer_one_part() for each part of the messages up to a STOP or
the end of the messages array.
i2c_dw_xfer() takes care of runtime PM and holds the hardware lock on
the bus while calling __i2c_dw_xfer_one_part(), this allows grouping
multiple accesses to device that support a STOP in a transaction when
done via i2c_dev I2C_RDWR ioctl.
Also, now that we have a lookup of the messages in i2c_dw_xfer() prior
to each transaction, we use it to make sure the messages are valid for
the transaction, via a new function i2c_dw_msg_is_valid(). We check
that the target address does not change before starting the transaction
instead of aborting the transfer while it is happening, as it was done
in i2c_dw_xfer_msg(). The target address can only be changed after an
I2C_M_STOP flag, i.e after a STOP on the i2c bus.
The I2C_FUNC_PROTOCOL_MANGLING flag is added to the list of
functionalities supported by the controller, except for the AMD NAVI
i2c controller which uses its own xfer() function and is left untouched.
Signed-off-by: Benoît Monin <benoit.monin@bootlin.com>
---
drivers/i2c/busses/i2c-designware-master.c | 130 ++++++++++++++++++++---------
1 file changed, 90 insertions(+), 40 deletions(-)
diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c
index f247cf323207..a0ff0e2db065 100644
--- a/drivers/i2c/busses/i2c-designware-master.c
+++ b/drivers/i2c/busses/i2c-designware-master.c
@@ -442,7 +442,6 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
struct i2c_msg *msgs = dev->msgs;
u32 intr_mask;
int tx_limit, rx_limit;
- u32 addr = msgs[dev->msg_write_idx].addr;
u32 buf_len = dev->tx_buf_len;
u8 *buf = dev->tx_buf;
bool need_restart = false;
@@ -453,18 +452,6 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
for (; dev->msg_write_idx < dev->msgs_num; dev->msg_write_idx++) {
u32 flags = msgs[dev->msg_write_idx].flags;
- /*
- * If target address has changed, we need to
- * reprogram the target address in the I2C
- * adapter when we are done with this transfer.
- */
- if (msgs[dev->msg_write_idx].addr != addr) {
- dev_err(dev->dev,
- "%s: invalid target address\n", __func__);
- dev->msg_err = -EINVAL;
- break;
- }
-
if (!(dev->status & STATUS_WRITE_IN_PROGRESS)) {
/* new i2c_msg */
buf = msgs[dev->msg_write_idx].buf;
@@ -812,18 +799,15 @@ static int i2c_dw_wait_transfer(struct dw_i2c_dev *dev)
}
/*
- * Prepare controller for a transaction and call i2c_dw_xfer_msg.
+ * Prepare controller for a transaction, start the transfer of the msgs
+ * and wait for completion, either a STOP or a error.
+ * Return 0 or a negative error code.
*/
static int
-i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
+__i2c_dw_xfer_one_part(struct dw_i2c_dev *dev, struct i2c_msg *msgs, size_t num)
{
- struct dw_i2c_dev *dev = i2c_get_adapdata(adap);
int ret;
- dev_dbg(dev->dev, "%s: msgs: %d\n", __func__, num);
-
- pm_runtime_get_sync(dev->dev);
-
reinit_completion(&dev->cmd_complete);
dev->msgs = msgs;
dev->msgs_num = num;
@@ -835,13 +819,9 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
dev->abort_source = 0;
dev->rx_outstanding = 0;
- ret = i2c_dw_acquire_lock(dev);
- if (ret)
- goto done_nolock;
-
ret = i2c_dw_wait_bus_not_busy(dev);
if (ret < 0)
- goto done;
+ return ret;
/* Start the transfers */
i2c_dw_xfer_init(dev);
@@ -853,7 +833,7 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
/* i2c_dw_init_master() implicitly disables the adapter */
i2c_recover_bus(&dev->adapter);
i2c_dw_init_master(dev);
- goto done;
+ return ret;
}
/*
@@ -876,28 +856,93 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
*/
__i2c_dw_disable_nowait(dev);
- if (dev->msg_err) {
- ret = dev->msg_err;
- goto done;
- }
+ if (dev->msg_err)
+ return dev->msg_err;
/* No error */
- if (likely(!dev->cmd_err && !dev->status)) {
- ret = num;
- goto done;
- }
+ if (likely(!dev->cmd_err && !dev->status))
+ return 0;
/* We have an error */
- if (dev->cmd_err == DW_IC_ERR_TX_ABRT) {
- ret = i2c_dw_handle_tx_abort(dev);
- goto done;
- }
+ if (dev->cmd_err == DW_IC_ERR_TX_ABRT)
+ return i2c_dw_handle_tx_abort(dev);
if (dev->status)
dev_err(dev->dev,
"transfer terminated early - interrupt latency too high?\n");
- ret = -EIO;
+ return -EIO;
+}
+
+/*
+ * Verify that the message at index @idx can be processed as part
+ * of a single transaction. The @msgs array contains the messages
+ * of the transaction. The message is checked against its predecessor
+ * to ensure that it respects the limitation of the controller.
+ */
+static bool
+i2c_dw_msg_is_valid(struct dw_i2c_dev *dev, const struct i2c_msg *msgs, size_t idx)
+{
+ /*
+ * The first message of a transaction is valid,
+ * no constraint from a previous message.
+ */
+ if (!idx)
+ return true;
+
+ /*
+ * We cannot change the target address during a transaction, so make
+ * sure the address is identical to the one of the previous message.
+ */
+ if (msgs[idx - 1].addr != msgs[idx].addr) {
+ dev_err(dev->dev, "invalid target address\n");
+ return false;
+ }
+
+ return true;
+}
+
+static int
+i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
+{
+ struct dw_i2c_dev *dev = i2c_get_adapdata(adap);
+ struct i2c_msg *msgs_part;
+ size_t cnt;
+ int ret;
+
+ dev_dbg(dev->dev, "msgs: %d\n", num);
+
+ pm_runtime_get_sync(dev->dev);
+
+ ret = i2c_dw_acquire_lock(dev);
+ if (ret)
+ goto done_nolock;
+
+ /*
+ * If the I2C_M_STOP is present in some the messages,
+ * we do one transaction for each part up to the STOP.
+ */
+ for (msgs_part = msgs; msgs_part < msgs + num; msgs_part += cnt) {
+ /*
+ * Count the messages in a transaction, up to a STOP
+ * or the end of the msgs.
+ */
+ for (cnt = 1; ; cnt++) {
+ if (!i2c_dw_msg_is_valid(dev, msgs_part, cnt - 1)) {
+ ret = -EINVAL;
+ goto done;
+ }
+
+ if ((msgs_part[cnt - 1].flags & I2C_M_STOP) ||
+ (msgs_part + cnt == msgs + num))
+ break;
+ }
+
+ /* transfer one part up to a STOP */
+ ret = __i2c_dw_xfer_one_part(dev, msgs_part, cnt);
+ if (ret < 0)
+ break;
+ }
done:
i2c_dw_release_lock(dev);
@@ -905,7 +950,9 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
done_nolock:
pm_runtime_put_autosuspend(dev->dev);
- return ret;
+ if (ret < 0)
+ return ret;
+ return num;
}
static const struct i2c_algorithm i2c_dw_algo = {
@@ -928,6 +975,9 @@ void i2c_dw_configure_master(struct dw_i2c_dev *dev)
dev->functionality = I2C_FUNC_10BIT_ADDR | DW_IC_DEFAULT_FUNCTIONALITY;
+ if ((dev->flags & MODEL_MASK) != MODEL_AMD_NAVI_GPU)
+ dev->functionality |= I2C_FUNC_PROTOCOL_MANGLING;
+
dev->master_cfg = DW_IC_CON_MASTER | DW_IC_CON_SLAVE_DISABLE |
DW_IC_CON_RESTART_EN;
--
2.52.0
On Tue, Jan 20, 2026 at 10:28:04AM +0100, Benoît Monin wrote:
> Add the support of the I2C_M_STOP flag in i2c_msg by splitting
> i2c_dw_xfer() in two: __i2c_dw_xfer_one_part() for the core transfer logic
> and i2c_dw_xfer() for handling the high-level transaction management.
>
> In detail __i2c_dw_xfer_one_part() starts a transaction and wait for its
> completion, either with a STOP on the bus or an error. i2c_dw_xfer()
> loops over the messages to search for the I2C_M_STOP flag and calls
> __i2c_dw_xfer_one_part() for each part of the messages up to a STOP or
> the end of the messages array.
>
> i2c_dw_xfer() takes care of runtime PM and holds the hardware lock on
> the bus while calling __i2c_dw_xfer_one_part(), this allows grouping
> multiple accesses to device that support a STOP in a transaction when
> done via i2c_dev I2C_RDWR ioctl.
Does i2c-tools support this. I.o.w. can you put an example of user space call
to achieve the above?
> Also, now that we have a lookup of the messages in i2c_dw_xfer() prior
> to each transaction, we use it to make sure the messages are valid for
> the transaction, via a new function i2c_dw_msg_is_valid(). We check
> that the target address does not change before starting the transaction
> instead of aborting the transfer while it is happening, as it was done
> in i2c_dw_xfer_msg(). The target address can only be changed after an
> I2C_M_STOP flag, i.e after a STOP on the i2c bus.
>
> The I2C_FUNC_PROTOCOL_MANGLING flag is added to the list of
> functionalities supported by the controller, except for the AMD NAVI
> i2c controller which uses its own xfer() function and is left untouched.
...
While the below is not a kernel-doc, I would still use a couple of things to
make it a bit aligned with that.
> /*
> - * Prepare controller for a transaction and call i2c_dw_xfer_msg.
> + * Prepare controller for a transaction, start the transfer of the msgs
@msgs
> + * and wait for completion, either a STOP or a error.
> + * Return 0 or a negative error code.
Return:
> */
...
> +/*
> + * Verify that the message at index @idx can be processed as part
> + * of a single transaction. The @msgs array contains the messages
> + * of the transaction. The message is checked against its predecessor
> + * to ensure that it respects the limitation of the controller.
Ha, you even used @ notation here!
Perhaps also add Return: line?
> + */
...
> + /*
> + * The first message of a transaction is valid,
> + * no constraint from a previous message.
constraints ?
> + */
...
> +static int
> +i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
> +{
> + struct dw_i2c_dev *dev = i2c_get_adapdata(adap);
> + struct i2c_msg *msgs_part;
> + size_t cnt;
> + int ret;
> + dev_dbg(dev->dev, "msgs: %d\n", num);
> +
> + pm_runtime_get_sync(dev->dev);
> +
> + ret = i2c_dw_acquire_lock(dev);
> + if (ret)
> + goto done_nolock;
A side note: Perhaps make sense to factor the body out to another helper.
> + /*
> + * If the I2C_M_STOP is present in some the messages,
> + * we do one transaction for each part up to the STOP.
> + */
> + for (msgs_part = msgs; msgs_part < msgs + num; msgs_part += cnt) {
> + /*
> + * Count the messages in a transaction, up to a STOP
> + * or the end of the msgs.
> + */
> + for (cnt = 1; ; cnt++) {
Do we have a guarantee that this doesn't become an infinite loop or more
precisely out-of-boundary access?
I hope to see this be explicitly mentioned in the comment on top of for-loop.
> + if (!i2c_dw_msg_is_valid(dev, msgs_part, cnt - 1)) {
> + ret = -EINVAL;
> + goto done;
> + }
> +
> + if ((msgs_part[cnt - 1].flags & I2C_M_STOP) ||
> + (msgs_part + cnt == msgs + num))
> + break;
> + }
> +
> + /* transfer one part up to a STOP */
> + ret = __i2c_dw_xfer_one_part(dev, msgs_part, cnt);
> + if (ret < 0)
> + break;
> + }
>
> done:
> i2c_dw_release_lock(dev);
> done_nolock:
> pm_runtime_put_autosuspend(dev->dev);
>
> - return ret;
> + if (ret < 0)
> + return ret;
> + return num;
> }
...
> + if ((dev->flags & MODEL_MASK) != MODEL_AMD_NAVI_GPU)
> + dev->functionality |= I2C_FUNC_PROTOCOL_MANGLING;
Why do we need this flag? I mean can't we use the fact that the code for that
platform uses custom xfer implementation?
--
With Best Regards,
Andy Shevchenko
On Thursday, 22 January 2026 at 17:25:58 CET, Andy Shevchenko wrote: > On Tue, Jan 20, 2026 at 10:28:04AM +0100, Benoît Monin wrote: > > Add the support of the I2C_M_STOP flag in i2c_msg by splitting > > i2c_dw_xfer() in two: __i2c_dw_xfer_one_part() for the core transfer logic > > and i2c_dw_xfer() for handling the high-level transaction management. > > > > In detail __i2c_dw_xfer_one_part() starts a transaction and wait for its > > completion, either with a STOP on the bus or an error. i2c_dw_xfer() > > loops over the messages to search for the I2C_M_STOP flag and calls > > __i2c_dw_xfer_one_part() for each part of the messages up to a STOP or > > the end of the messages array. > > > > i2c_dw_xfer() takes care of runtime PM and holds the hardware lock on > > the bus while calling __i2c_dw_xfer_one_part(), this allows grouping > > multiple accesses to device that support a STOP in a transaction when > > done via i2c_dev I2C_RDWR ioctl. > > Does i2c-tools support this. I.o.w. can you put an example of user space call > to achieve the above? > I posted a patch series for i2c-tools, adding support for message modifier flags including I2C_M_STOP to i2ctransfer: https://lore.kernel.org/all/20251223-msg-flags-v2-0-8d934a4366e2@bootlin.com/ I am working on version 3, following Jean and Wolfram's reviews. [...] Thanks for the review! -- Benoît Monin, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
On Fri, Jan 23, 2026 at 03:25:24PM +0100, Benoît Monin wrote: > On Thursday, 22 January 2026 at 17:25:58 CET, Andy Shevchenko wrote: > > On Tue, Jan 20, 2026 at 10:28:04AM +0100, Benoît Monin wrote: > > > Add the support of the I2C_M_STOP flag in i2c_msg by splitting > > > i2c_dw_xfer() in two: __i2c_dw_xfer_one_part() for the core transfer logic > > > and i2c_dw_xfer() for handling the high-level transaction management. > > > > > > In detail __i2c_dw_xfer_one_part() starts a transaction and wait for its > > > completion, either with a STOP on the bus or an error. i2c_dw_xfer() > > > loops over the messages to search for the I2C_M_STOP flag and calls > > > __i2c_dw_xfer_one_part() for each part of the messages up to a STOP or > > > the end of the messages array. > > > > > > i2c_dw_xfer() takes care of runtime PM and holds the hardware lock on > > > the bus while calling __i2c_dw_xfer_one_part(), this allows grouping > > > multiple accesses to device that support a STOP in a transaction when > > > done via i2c_dev I2C_RDWR ioctl. > > > > Does i2c-tools support this. I.o.w. can you put an example of user space call > > to achieve the above? > > > I posted a patch series for i2c-tools, adding support for message modifier > flags including I2C_M_STOP to i2ctransfer: > https://lore.kernel.org/all/20251223-msg-flags-v2-0-8d934a4366e2@bootlin.com/ > > I am working on version 3, following Jean and Wolfram's reviews. Thanks, it would be nice to have this being mentioned in the commit message that the user space tools are going to support this in near future. -- With Best Regards, Andy Shevchenko
© 2016 - 2026 Red Hat, Inc.