Re: [PATCH v5 4/6] i2c: designware: Implement I2C_M_STOP support

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

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