Re: [PATCH 2/3] i2c: designware: Enable transfer with different target addresses

[Hans Verkuil <hverkuil+cisco@kernel.org>]

Hi Benoît,

On 17/10/2025 16:59, Benoît Monin wrote:
> When i2c_dw_xfer() is called with more than one message, it sets the
> target address according to the first message. If any of the following
> messages have a different target address, the transfer finishes with
> an error.
> 
> Instead, if the next message has a different target address, wait until
> all previous messages are sent and the STOP condition is detected. This
> will complete the current part of the transfer. The next part is then
> handled by looping in i2c_dw_xfer(), calling i2c_dw_xfer_init() and
> i2c_dw_wait_transfer() until all messages of the transfer have been
> processed, or an error is detected.
> 
> The RESTART bit is now set after the first message of each part of the
> transfer, instead of just after the very first message of the whole
> transfer.
> 
> For each address change, i2c_dw_xfer_init() is called, which takes care
> of disabling the adapter before changing the target address register,
> then re-enabling it. Given that we cannot know the value of the
> I2C_DYNAMIC_TAR_UPDATE parameter, this is the only sure way to change
> the target address.

I have the problem described here:

https://lore.kernel.org/linux-i2c/ee6afdd7-3117-43cd-831f-e0ec5ee46f46@kernel.org/

And it looks like this patch is intended to solve that problem (one transaction
with two writes to different target addresses).

I tried this patch, but it doesn't work. Instead I get a time out:

[  111.695238] i2c_designware 1f00074000.i2c: controller timed out

Is it indeed meant to solve the problem I have or is it addressing another
issue?

I'm happy to help test patches.

Regards,

	Hans

> 
> Based on the work of Dmitry Guzman <dmitry.guzman@mobileye.com>
> 
> Signed-off-by: Benoît Monin <benoit.monin@bootlin.com>
> ---
>  drivers/i2c/busses/i2c-designware-master.c | 58 ++++++++++++++++--------------
>  1 file changed, 31 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c
> index c7a72c28786c2..f9a180b145da8 100644
> --- a/drivers/i2c/busses/i2c-designware-master.c
> +++ b/drivers/i2c/busses/i2c-designware-master.c
> @@ -436,6 +436,7 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
>  	u8 *buf = dev->tx_buf;
>  	bool need_restart = false;
>  	unsigned int flr;
> +	int first_idx = dev->msg_write_idx;
>  
>  	intr_mask = DW_IC_INTR_MASTER_MASK;
>  
> @@ -446,11 +447,11 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
>  		 * If target address has changed, we need to
>  		 * reprogram the target address in the I2C
>  		 * adapter when we are done with this transfer.
> +		 * This can be done after STOP_DET IRQ flag is raised.
> +		 * So, disable "TX FIFO empty" interrupt.
>  		 */
>  		if (msgs[dev->msg_write_idx].addr != addr) {
> -			dev_err(dev->dev,
> -				"%s: invalid target address\n", __func__);
> -			dev->msg_err = -EINVAL;
> +			intr_mask &= ~DW_IC_INTR_TX_EMPTY;
>  			break;
>  		}
>  
> @@ -465,7 +466,7 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
>  			 * set restart bit between messages.
>  			 */
>  			if ((dev->master_cfg & DW_IC_CON_RESTART_EN) &&
> -					(dev->msg_write_idx > 0))
> +					(dev->msg_write_idx > first_idx))
>  				need_restart = true;
>  		}
>  
> @@ -822,7 +823,6 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
>  		break;
>  	}
>  
> -	reinit_completion(&dev->cmd_complete);
>  	dev->msgs = msgs;
>  	dev->msgs_num = num;
>  	dev->cmd_err = 0;
> @@ -841,18 +841,33 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
>  	if (ret < 0)
>  		goto done;
>  
> -	/* Start the transfers */
> -	i2c_dw_xfer_init(dev);
> +	do {
> +		reinit_completion(&dev->cmd_complete);
>  
> -	/* Wait for tx to complete */
> -	ret = i2c_dw_wait_transfer(dev);
> -	if (ret) {
> -		dev_err(dev->dev, "controller timed out\n");
> -		/* i2c_dw_init_master() implicitly disables the adapter */
> -		i2c_recover_bus(&dev->adapter);
> -		i2c_dw_init_master(dev);
> -		goto done;
> -	}
> +		/* Start the transfers */
> +		i2c_dw_xfer_init(dev);
> +
> +		/* Wait for tx to complete */
> +		ret = i2c_dw_wait_transfer(dev);
> +		if (ret) {
> +			dev_err(dev->dev, "controller timed out\n");
> +			/* i2c_dw_init_master() implicitly disables the adapter */
> +			i2c_recover_bus(&dev->adapter);
> +			i2c_dw_init_master(dev);
> +			goto done;
> +		}
> +
> +		if (dev->msg_err) {
> +			ret = dev->msg_err;
> +			goto done;
> +		}
> +
> +		/* We have an error */
> +		if (dev->cmd_err == DW_IC_ERR_TX_ABRT) {
> +			ret = i2c_dw_handle_tx_abort(dev);
> +			goto done;
> +		}
> +	} while (dev->msg_write_idx < num);
>  
>  	/*
>  	 * This happens rarely (~1:500) and is hard to reproduce. Debug trace
> @@ -874,23 +889,12 @@ 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;
> -	}
> -
>  	/* No error */
>  	if (likely(!dev->cmd_err && !dev->status)) {
>  		ret = num;
>  		goto done;
>  	}
>  
> -	/* We have an error */
> -	if (dev->cmd_err == DW_IC_ERR_TX_ABRT) {
> -		ret = i2c_dw_handle_tx_abort(dev);
> -		goto done;
> -	}
> -
>  	if (dev->status)
>  		dev_err(dev->dev,
>  			"transfer terminated early - interrupt latency too high?\n");
>