Re: [PATCH 2/3] i2c: designware: Extend check for mixed slave addresses

[Jean Delvare <jdelvare@suse.de>]

On Thu, 18 Sep 2025 16:10:54 +0200, Jean Delvare wrote:
> The i2c-designware driver only supports transfers where all messages
> use the same slave address. This condition is currently tested in
> i2c_dw_xfer_msg(), with 2 limitations:
> * The code only checks the address value, not the 10-bit address
>   flag, so it could miss an address change.
> * For the AMD Navi GPU devices, the driver uses a dedicated function
>   instead of i2c_dw_xfer_msg(), so the check is not performed.
> 
> Move the check to the common code path, and add the 10-bit address
> flag comparison, to catch and report early if a given transfer is not
> supported.
> 
> Signed-off-by: Jean Delvare <jdelvare@suse.de>
> ---
>  drivers/i2c/busses/i2c-designware-master.c |   28 ++++++++++++++--------------
>  1 file changed, 14 insertions(+), 14 deletions(-)
> 
> --- linux-6.16.orig/drivers/i2c/busses/i2c-designware-master.c
> +++ linux-6.16/drivers/i2c/busses/i2c-designware-master.c
> @@ -429,7 +429,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;
> @@ -440,18 +439,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;
> @@ -806,10 +793,23 @@ static int
>  i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
>  {
>  	struct dw_i2c_dev *dev = i2c_get_adapdata(adap);
> -	int ret;
> +	int ret, i;
>  
>  	dev_dbg(dev->dev, "%s: msgs: %d\n", __func__, num);
>  
> +	/*
> +	 * This driver only supports I2C transfers where all the messages
> +	 * use the same address.
> +	 */
> +	for (i = 1; i < num; i++) {
> +		if (msgs[i].addr != msgs[0].addr ||

Note, I found meanwhile that some drivers (i2c-amd-mp2, i2c-mlxcpld)
use unlikely() to limit the performance penalty incurred by these
tests. Sounds like a good idea?

> +		    (msgs[i].flags & I2C_M_TEN) != (msgs[0].flags & I2C_M_TEN)) {
> +			dev_err(dev->dev,
> +				"Mixed slave addresses not supported\n");
> +			return -EOPNOTSUPP;
> +		}
> +	}
> +
>  	pm_runtime_get_sync(dev->dev);
>  
>  	switch (dev->flags & MODEL_MASK) {
> 


-- 
Jean Delvare
SUSE L3 Support