Add the support of the I2C_M_STOP flag in i2c_msg by splitting
i2c_dw_xfer() in two: __i2c_dw_xfer_unlocked() for the core transfer logic
and i2c_dw_xfer() for handling the high-level transaction management.

In detail __i2c_dw_xfer_unlocked() 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_unlocked() for each part of the messages up to a STOP or
the end of the messages array.

i2c_dw_xfer() holds the device lock while calling __i2c_dw_xfer_unlocked(),
this allows to group multiple accesses to device that support a STOP in
a transaction when done via i2c_dev I2C_RDWR ioctl, in a single-master
configuration.

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. 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, thus a STOP on the i2c bus.

The I2C_FUNC_PROTOCOL_MANGLING flag is added to the list of
functionalities supported by the adapter, 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 | 103 ++++++++++++++++++++---------
 1 file changed, 70 insertions(+), 33 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c
index ec4fc2708d03..da1963d25def 100644
--- a/drivers/i2c/busses/i2c-designware-master.c
+++ b/drivers/i2c/busses/i2c-designware-master.c
@@ -431,7 +431,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;
@@ -442,18 +441,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;
@@ -801,26 +788,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.
+ * Caller must hold the device lock.
  */
 static int
-i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
+__i2c_dw_xfer_unlocked(struct dw_i2c_dev *dev, struct i2c_msg msgs[], int 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);
-
-	switch (dev->flags & MODEL_MASK) {
-	case MODEL_AMD_NAVI_GPU:
-		ret = amd_i2c_dw_xfer_quirk(adap, msgs, num);
-		goto done_nolock;
-	default:
-		break;
-	}
-
 	reinit_completion(&dev->cmd_complete);
 	dev->msgs = msgs;
 	dev->msgs_num = num;
@@ -832,10 +808,6 @@ 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;
@@ -896,13 +868,75 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 
 	ret = -EIO;
 
+done:
+	return ret;
+}
+
+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 *msg;
+	int ret, cnt;
+
+	dev_dbg(dev->dev, "%s: msgs: %d\n", __func__, num);
+
+	pm_runtime_get_sync(dev->dev);
+
+	switch (dev->flags & MODEL_MASK) {
+	case MODEL_AMD_NAVI_GPU:
+		ret = amd_i2c_dw_xfer_quirk(adap, msgs, num);
+		goto done_nolock;
+	default:
+		break;
+	}
+
+	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 (msg = msgs; msg < msgs + num; msg += cnt) {
+		u16 addr = msg->addr;
+
+		/*
+		 * Count the messages in a transaction, up to a STOP
+		 * or the end of the msgs.
+		 */
+		for (cnt = 1; ; cnt++) {
+			/*
+			 * We cannot change the target address during
+			 * a transaction, so make sure the address stays
+			 * the same.
+			 */
+			if (msg[cnt - 1].addr != addr) {
+				dev_err(dev->dev, "invalid target address\n");
+				ret = -EINVAL;
+				goto done;
+			}
+
+			if ((msg[cnt - 1].flags & I2C_M_STOP) ||
+			    (msg + cnt == msgs + num))
+				break;
+		}
+
+		ret = __i2c_dw_xfer_unlocked(dev, msg, cnt);
+		if (ret < 0)
+			goto done;
+	}
+
 done:
 	i2c_dw_release_lock(dev);
 
 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 = {
@@ -920,6 +954,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.51.1

On Fri, Oct 31, 2025 at 03:35:42PM +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_unlocked() for the core transfer logic
> and i2c_dw_xfer() for handling the high-level transaction management.
> 
> In detail __i2c_dw_xfer_unlocked() 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_unlocked() for each part of the messages up to a STOP or
> the end of the messages array.
> 
> i2c_dw_xfer() holds the device lock while calling __i2c_dw_xfer_unlocked(),
> this allows to group multiple accesses to device that support a STOP in
> a transaction when done via i2c_dev I2C_RDWR ioctl, in a single-master
> configuration.
> 
> 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. 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, thus a STOP on the i2c bus.
> 
> The I2C_FUNC_PROTOCOL_MANGLING flag is added to the list of
> functionalities supported by the adapter, except for the AMD NAVI i2c

I2C controller

> 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 | 103 ++++++++++++++++++++---------
>  1 file changed, 70 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c
> index ec4fc2708d03..da1963d25def 100644
> --- a/drivers/i2c/busses/i2c-designware-master.c
> +++ b/drivers/i2c/busses/i2c-designware-master.c
> @@ -431,7 +431,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;
> @@ -442,18 +441,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;
> @@ -801,26 +788,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.
> + * Caller must hold the device lock.
>   */
>  static int
> -i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
> +__i2c_dw_xfer_unlocked(struct dw_i2c_dev *dev, struct i2c_msg msgs[], int 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);
> -
> -	switch (dev->flags & MODEL_MASK) {
> -	case MODEL_AMD_NAVI_GPU:
> -		ret = amd_i2c_dw_xfer_quirk(adap, msgs, num);
> -		goto done_nolock;
> -	default:
> -		break;
> -	}
> -
>  	reinit_completion(&dev->cmd_complete);
>  	dev->msgs = msgs;
>  	dev->msgs_num = num;
> @@ -832,10 +808,6 @@ 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;
> @@ -896,13 +868,75 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
>  
>  	ret = -EIO;
>  
> +done:
> +	return ret;
> +}
> +
> +static int
> +i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)

Typically if you have

i2c_dw_xfer()
i2c_dw_xfer_unlocked()

The only difference is that the former call the latter with lock held.
However, this is not the case here which is confusing.

I suggest move the lookup to be part of the _unlocked() variant.

While there, can we use size_t with the num parameter.

> +{
> +	struct dw_i2c_dev *dev = i2c_get_adapdata(adap);
> +	struct i2c_msg *msg;
> +	int ret, cnt;
> +
> +	dev_dbg(dev->dev, "%s: msgs: %d\n", __func__, num);
> +
> +	pm_runtime_get_sync(dev->dev);
> +
> +	switch (dev->flags & MODEL_MASK) {
> +	case MODEL_AMD_NAVI_GPU:
> +		ret = amd_i2c_dw_xfer_quirk(adap, msgs, num);
> +		goto done_nolock;

Not really problem of this patch but I'm sure there is cleaner way to deal
with this. I mean first of all we don't wan't to differentiate here what
model this is. Instead we can look for a "quirks" based on dev->flags.

Secondly goto inside switch is confusing too.

This can be better written like:

if ((dev->flags & MODEL_MASK) == MODEL_AMD_NAVI_GPU) {
	...
}

> +	default:
> +		break;
> +	}
> +
> +	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 (msg = msgs; msg < msgs + num; msg += cnt) {
> +		u16 addr = msg->addr;
> +
> +		/*
> +		 * Count the messages in a transaction, up to a STOP
> +		 * or the end of the msgs.
> +		 */
> +		for (cnt = 1; ; cnt++) {
> +			/*
> +			 * We cannot change the target address during
> +			 * a transaction, so make sure the address stays
> +			 * the same.
> +			 */
> +			if (msg[cnt - 1].addr != addr) {
> +				dev_err(dev->dev, "invalid target address\n");
> +				ret = -EINVAL;
> +				goto done;
> +			}
> +
> +			if ((msg[cnt - 1].flags & I2C_M_STOP) ||
> +			    (msg + cnt == msgs + num))
> +				break;
> +		}
> +
> +		ret = __i2c_dw_xfer_unlocked(dev, msg, cnt);
> +		if (ret < 0)
> +			goto done;

This can be

	break;

> +	}
> +
>  done:
>  	i2c_dw_release_lock(dev);
>  
>  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 = {
> @@ -920,6 +954,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.51.1

Hi Mika,

On Monday, 3 November 2025 at 11:39:08 CET, Mika Westerberg wrote:
> On Fri, Oct 31, 2025 at 03:35:42PM +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_unlocked() for the core transfer logic
> > and i2c_dw_xfer() for handling the high-level transaction management.
> > 
> > In detail __i2c_dw_xfer_unlocked() 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_unlocked() for each part of the messages up to a STOP or
> > the end of the messages array.
> > 
> > i2c_dw_xfer() holds the device lock while calling __i2c_dw_xfer_unlocked(),
> > this allows to group multiple accesses to device that support a STOP in
> > a transaction when done via i2c_dev I2C_RDWR ioctl, in a single-master
> > configuration.
> > 
> > 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. 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, thus a STOP on the i2c bus.
> > 
> > The I2C_FUNC_PROTOCOL_MANGLING flag is added to the list of
> > functionalities supported by the adapter, except for the AMD NAVI i2c
> 
> I2C controller
> 
> > 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 | 103 ++++++++++++++++++++---------
> >  1 file changed, 70 insertions(+), 33 deletions(-)
> > 
> > diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c
> > index ec4fc2708d03..da1963d25def 100644
> > --- a/drivers/i2c/busses/i2c-designware-master.c
> > +++ b/drivers/i2c/busses/i2c-designware-master.c
> > @@ -431,7 +431,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;
> > @@ -442,18 +441,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;
> > @@ -801,26 +788,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.
> > + * Caller must hold the device lock.
> >   */
> >  static int
> > -i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
> > +__i2c_dw_xfer_unlocked(struct dw_i2c_dev *dev, struct i2c_msg msgs[], int 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);
> > -
> > -	switch (dev->flags & MODEL_MASK) {
> > -	case MODEL_AMD_NAVI_GPU:
> > -		ret = amd_i2c_dw_xfer_quirk(adap, msgs, num);
> > -		goto done_nolock;
> > -	default:
> > -		break;
> > -	}
> > -
> >  	reinit_completion(&dev->cmd_complete);
> >  	dev->msgs = msgs;
> >  	dev->msgs_num = num;
> > @@ -832,10 +808,6 @@ 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;
> > @@ -896,13 +868,75 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
> >  
> >  	ret = -EIO;
> >  
> > +done:
> > +	return ret;
> > +}
> > +
> > +static int
> > +i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
> 
> Typically if you have
> 
> i2c_dw_xfer()
> i2c_dw_xfer_unlocked()
> 
> The only difference is that the former call the latter with lock held.
> However, this is not the case here which is confusing.
> 
> I suggest move the lookup to be part of the _unlocked() variant.
> 
> While there, can we use size_t with the num parameter.
> 
Maybe __i2c_dw_xfer_unlocked() is the wrong name, as it is meant to be
called potentially multiple times from i2c_dw_xfer(), depending on the
number of messages flagged with I2C_M_STOP. So it is not an unlocked
version of i2c_dw_xfer(). I'll try to find a better name.

> > +{
> > +	struct dw_i2c_dev *dev = i2c_get_adapdata(adap);
> > +	struct i2c_msg *msg;
> > +	int ret, cnt;
> > +
> > +	dev_dbg(dev->dev, "%s: msgs: %d\n", __func__, num);
> > +
> > +	pm_runtime_get_sync(dev->dev);
> > +
> > +	switch (dev->flags & MODEL_MASK) {
> > +	case MODEL_AMD_NAVI_GPU:
> > +		ret = amd_i2c_dw_xfer_quirk(adap, msgs, num);
> > +		goto done_nolock;
> 
> Not really problem of this patch but I'm sure there is cleaner way to deal
> with this. I mean first of all we don't wan't to differentiate here what
> model this is. Instead we can look for a "quirks" based on dev->flags.
> 
> Secondly goto inside switch is confusing too.
> 
> This can be better written like:
> 
> if ((dev->flags & MODEL_MASK) == MODEL_AMD_NAVI_GPU) {
> 	...
> }
> 
If we also add the ACQUIRE() for pm_runtime suggested by Andy to
amd_i2c_dw_xfer_quirk(), we could then create a struct i2c_algorithm
dedicated to this hardware and set it in i2c_dw_probe_master(). This way
we can skip all this code, no need to check the model for every transfer.

> > +	default:
> > +		break;
> > +	}
> > +
> > +	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 (msg = msgs; msg < msgs + num; msg += cnt) {
> > +		u16 addr = msg->addr;
> > +
> > +		/*
> > +		 * Count the messages in a transaction, up to a STOP
> > +		 * or the end of the msgs.
> > +		 */
> > +		for (cnt = 1; ; cnt++) {
> > +			/*
> > +			 * We cannot change the target address during
> > +			 * a transaction, so make sure the address stays
> > +			 * the same.
> > +			 */
> > +			if (msg[cnt - 1].addr != addr) {
> > +				dev_err(dev->dev, "invalid target address\n");
> > +				ret = -EINVAL;
> > +				goto done;
> > +			}
> > +
> > +			if ((msg[cnt - 1].flags & I2C_M_STOP) ||
> > +			    (msg + cnt == msgs + num))
> > +				break;
> > +		}
> > +
> > +		ret = __i2c_dw_xfer_unlocked(dev, msg, cnt);
> > +		if (ret < 0)
> > +			goto done;
> 
> This can be
> 
> 	break;
> 
> > +	}
> > +
> >  done:
> >  	i2c_dw_release_lock(dev);
> >  
> >  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 = {
> > @@ -920,6 +954,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;
> >  
> > 
> 


Best regards,
-- 
Benoît Monin, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

On Fri, Oct 31, 2025 at 03:35:42PM +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_unlocked() for the core transfer logic
> and i2c_dw_xfer() for handling the high-level transaction management.
> 
> In detail __i2c_dw_xfer_unlocked() 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_unlocked() for each part of the messages up to a STOP or
> the end of the messages array.
> 
> i2c_dw_xfer() holds the device lock while calling __i2c_dw_xfer_unlocked(),
> this allows to group multiple accesses to device that support a STOP in
> a transaction when done via i2c_dev I2C_RDWR ioctl, in a single-master
> configuration.
> 
> 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. 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, thus a STOP on the i2c bus.
> 
> The I2C_FUNC_PROTOCOL_MANGLING flag is added to the list of
> functionalities supported by the adapter, except for the AMD NAVI i2c
> controller which uses its own xfer() function and is left untouched.

...

>  /*
> - * 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.
> + * Caller must hold the device lock.

Comment is good, but having a lockdep annotation in addition to is even better!

>   */

...

> +done:
> +	return ret;

Drop now unneeded label, return directly.

...

> +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 *msg;
> +	int ret, cnt;
> +
> +	dev_dbg(dev->dev, "%s: msgs: %d\n", __func__, num);

__func__ can be turned on or off at runtime (with Dynamic Debug help).

> +	pm_runtime_get_sync(dev->dev);

Can you switch to use ACQUIRE() ?

> +	switch (dev->flags & MODEL_MASK) {
> +	case MODEL_AMD_NAVI_GPU:
> +		ret = amd_i2c_dw_xfer_quirk(adap, msgs, num);
> +		goto done_nolock;
> +	default:
> +		break;
> +	}
> +
> +	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 (msg = msgs; msg < msgs + num; msg += cnt) {
> +		u16 addr = msg->addr;
> +
> +		/*
> +		 * Count the messages in a transaction, up to a STOP
> +		 * or the end of the msgs.
> +		 */
> +		for (cnt = 1; ; cnt++) {
> +			/*
> +			 * We cannot change the target address during
> +			 * a transaction, so make sure the address stays
> +			 * the same.
> +			 */
> +			if (msg[cnt - 1].addr != addr) {
> +				dev_err(dev->dev, "invalid target address\n");
> +				ret = -EINVAL;
> +				goto done;
> +			}
> +
> +			if ((msg[cnt - 1].flags & I2C_M_STOP) ||
> +			    (msg + cnt == msgs + num))
> +				break;
> +		}
> +
> +		ret = __i2c_dw_xfer_unlocked(dev, msg, cnt);
> +		if (ret < 0)
> +			goto done;
> +	}
> +
>  done:
>  	i2c_dw_release_lock(dev);
>  
>  done_nolock:
>  	pm_runtime_put_autosuspend(dev->dev);
>  
> -	return ret;
> +	if (ret < 0)
> +		return ret;
> +	return num;
>  }

-- 
With Best Regards,
Andy Shevchenko