Re: [PATCH v3 6/7] i2c: designware: Implement I2C_M_STOP support

[Andy Shevchenko]

On Wed, Nov 19, 2025 at 04:05:35PM +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.
> 
> 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, 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.

...

>  	ret = -EIO;
>  
> +done:
> +	return ret;

I don't see the point of keeping this label anymore. Is there a followup
that addresses this?

> +static inline bool
> +i2c_dw_msg_is_valid(struct dw_i2c_dev *dev, const struct i2c_msg *msgs, size_t idx)
> +{
> +	/* The first message of a transaction is valid */
> +	if (!idx)
> +		return true;
> +
> +	/*
> +	 * We cannot change the target address during a transaction, so make
> +	 * sure the address is identical to the one of the previous message.
> +	 */
> +	if (msgs[idx - 1].addr != msgs[idx].addr) {
> +		dev_err(dev->dev, "invalid target address\n");
> +		return false;
> +	}
> +
> +	return true;
> +}

Hmm... I would do it in the nested conditionals if nothing comes between
the lines in the future.

static inline bool
i2c_dw_msg_is_valid(struct dw_i2c_dev *dev, const struct i2c_msg *msgs, size_t idx)
{
	/*
	 * We cannot change the target address during a transaction, so make
	 * sure the address of the sequential messages is identical to the one
	 * of the previous message.
	 */
	if (idx && (msgs[idx - 1].addr != msgs[idx].addr) {
		dev_err(dev->dev, "invalid target address\n");
		return false;
	}

	/*
	 * Addresses match. For the first message return true as it sets the
	 * address for the whole transaction.
	 */
	return true;
}

...

> +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);
> +
> +	ACQUIRE(pm_runtime_active_auto_try, pm)(dev->dev);
> +	if (ACQUIRE_ERR(pm_runtime_active_auto_try, &pm))
> +		return -ENXIO;
> +
> +	ret = i2c_dw_acquire_lock(dev);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * 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++) {
> +			if (!i2c_dw_msg_is_valid(dev, msgs_part, cnt - 1)) {
> +				ret = -EINVAL;
> +				goto done;

What about

				break;

> +			}
> +
> +			if ((msgs_part[cnt - 1].flags & I2C_M_STOP) ||
> +			    (msgs_part + cnt == msgs + num))
> +				break;
> +		}
		if (ret < 0)
			break;

?

The motivation is to avoid mixing auto ptr with goto style
(it's not recommended as per cleanup.h).

> +		/* 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);
>  
> -	return ret;
> +	if (ret < 0)
> +		return ret;
> +	return num;
>  }

-- 
With Best Regards,
Andy Shevchenko