Message-ID: <5791158.IbC2pHGDlb@benoit.monin>
Date: Thu, 06 Nov 2025 15:38:29 +0100
From: Benoît Monin <benoit.monin@...tlin.com>
To: Mika Westerberg <mika.westerberg@...ux.intel.com>
Cc: Andi Shyti <andi.shyti@...nel.org>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
 Jarkko Nikula <jarkko.nikula@...ux.intel.com>,
 Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
 Jan Dabros <jsd@...ihalf.com>,
 Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
 Clark Williams <clrkwllms@...nel.org>, Steven Rostedt <rostedt@...dmis.org>,
 Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
 Gregory CLEMENT <gregory.clement@...tlin.com>,
 Théo Lebrun <theo.lebrun@...tlin.com>,
 Tawfik Bayouk <tawfik.bayouk@...ileye.com>,
 Vladimir Kondratiev <vladimir.kondratiev@...ileye.com>,
 Dmitry Guzman <dmitry.guzman@...ileye.com>, linux-i2c@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-rt-devel@...ts.linux.dev
Subject: Re: [PATCH v2 4/5] i2c: designware: Implement I2C_M_STOP support

Hi Mika,

On Monday, 3 November 2025 at 11:39:08 CET, Mika Westerberg wrote:
> 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
> 
> I2C controller
> 
> > controller which uses its own xfer() function and is left untouched.
> > 
> > Signed-off-by: Benoît Monin <benoit.monin@...tlin.com>
> > ---
> >  drivers/i2c/busses/i2c-designware-master.c | 103 ++++++++++++++++++++---------
> >  1 file changed, 70 insertions(+), 33 deletions(-)
> > 
> > diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c
> > index ec4fc2708d03..da1963d25def 100644
> > --- a/drivers/i2c/busses/i2c-designware-master.c
> > +++ b/drivers/i2c/busses/i2c-designware-master.c
> > @@ -431,7 +431,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;
> > @@ -442,18 +441,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;
> > @@ -801,26 +788,15 @@ static int i2c_dw_wait_transfer(struct dw_i2c_dev *dev)
> >  }
> >  
> >  /*
> > - * 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.
> >   */
> >  static int
> > -i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
> > +__i2c_dw_xfer_unlocked(struct dw_i2c_dev *dev, struct i2c_msg msgs[], int num)
> >  {
> > -	struct dw_i2c_dev *dev = i2c_get_adapdata(adap);
> >  	int ret;
> >  
> > -	dev_dbg(dev->dev, "%s: msgs: %d\n", __func__, num);
> > -
> > -	pm_runtime_get_sync(dev->dev);
> > -
> > -	switch (dev->flags & MODEL_MASK) {
> > -	case MODEL_AMD_NAVI_GPU:
> > -		ret = amd_i2c_dw_xfer_quirk(adap, msgs, num);
> > -		goto done_nolock;
> > -	default:
> > -		break;
> > -	}
> > -
> >  	reinit_completion(&dev->cmd_complete);
> >  	dev->msgs = msgs;
> >  	dev->msgs_num = num;
> > @@ -832,10 +808,6 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
> >  	dev->abort_source = 0;
> >  	dev->rx_outstanding = 0;
> >  
> > -	ret = i2c_dw_acquire_lock(dev);
> > -	if (ret)
> > -		goto done_nolock;
> > -
> >  	ret = i2c_dw_wait_bus_not_busy(dev);
> >  	if (ret < 0)
> >  		goto done;
> > @@ -896,13 +868,75 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
> >  
> >  	ret = -EIO;
> >  
> > +done:
> > +	return ret;
> > +}
> > +
> > +static int
> > +i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
> 
> Typically if you have
> 
> i2c_dw_xfer()
> i2c_dw_xfer_unlocked()
> 
> The only difference is that the former call the latter with lock held.
> However, this is not the case here which is confusing.
> 
> I suggest move the lookup to be part of the _unlocked() variant.
> 
> While there, can we use size_t with the num parameter.
> 
Maybe __i2c_dw_xfer_unlocked() is the wrong name, as it is meant to be
called potentially multiple times from i2c_dw_xfer(), depending on the
number of messages flagged with I2C_M_STOP. So it is not an unlocked
version of i2c_dw_xfer(). I'll try to find a better name.

> > +{
> > +	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);
> > +
> > +	pm_runtime_get_sync(dev->dev);
> > +
> > +	switch (dev->flags & MODEL_MASK) {
> > +	case MODEL_AMD_NAVI_GPU:
> > +		ret = amd_i2c_dw_xfer_quirk(adap, msgs, num);
> > +		goto done_nolock;
> 
> Not really problem of this patch but I'm sure there is cleaner way to deal
> with this. I mean first of all we don't wan't to differentiate here what
> model this is. Instead we can look for a "quirks" based on dev->flags.
> 
> Secondly goto inside switch is confusing too.
> 
> This can be better written like:
> 
> if ((dev->flags & MODEL_MASK) == MODEL_AMD_NAVI_GPU) {
> 	...
> }
> 
If we also add the ACQUIRE() for pm_runtime suggested by Andy to
amd_i2c_dw_xfer_quirk(), we could then create a struct i2c_algorithm
dedicated to this hardware and set it in i2c_dw_probe_master(). This way
we can skip all this code, no need to check the model for every transfer.

> > +	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;
> 
> This can be
> 
> 	break;
> 
> > +	}
> > +
> >  done:
> >  	i2c_dw_release_lock(dev);
> >  
> >  done_nolock:
> >  	pm_runtime_put_autosuspend(dev->dev);
> >  
> > -	return ret;
> > +	if (ret < 0)
> > +		return ret;
> > +	return num;
> >  }
> >  
> >  static const struct i2c_algorithm i2c_dw_algo = {
> > @@ -920,6 +954,9 @@ void i2c_dw_configure_master(struct dw_i2c_dev *dev)
> >  
> >  	dev->functionality = I2C_FUNC_10BIT_ADDR | DW_IC_DEFAULT_FUNCTIONALITY;
> >  
> > +	if ((dev->flags & MODEL_MASK) != MODEL_AMD_NAVI_GPU)
> > +		dev->functionality |= I2C_FUNC_PROTOCOL_MANGLING;
> > +
> >  	dev->master_cfg = DW_IC_CON_MASTER | DW_IC_CON_SLAVE_DISABLE |
> >  			  DW_IC_CON_RESTART_EN;
> >  
> > 
> 


Best regards,
-- 
Benoît Monin, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Powered by blists - more mailing lists