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

Andy Shevchenko
2025-11-19T20:03:52Z

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