Re: [PATCH v2 4/5] i2c: designware: Implement I2C_M_STOP support

[Andy Shevchenko <andriy.shevchenko@linux.intel.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