Re: [PATCH v7 4/4] i2c: qcom-geni: Support multi-owner controllers in GPI mode

[Mukesh Savaliya]

Thanks for the detailed review, Bjorn.

On 5/22/2026 8:45 AM, Bjorn Andersson wrote:
> On Thu, Apr 23, 2026 at 08:25:51PM +0530, Mukesh Kumar Savaliya wrote:
> > Some platforms use a QUP-based I2C controller in a configuration where the
> > controller is shared with another system processor. In this setup the
> > operating system must not assume exclusive ownership of the controller or
> > its associated pins.
> >
> > Add support for enabling multi-owner operation when DeviceTree specifies
> > qcom,qup-multi-owner. When enabled, mark the underlying serial engine as
> > shared so the common GENI resource handling avoids selecting the "sleep"
> > pinctrl state, which could disrupt transfers initiated by the other
> > processor.
> >
> > For GPI mode transfers, request lock/unlock TRE sequencing from the GPI
>
> "For GPI mode transfers" is there any other form?

Good point. The lock/unlock sequencing is only relevant when the
controller operates in GPI DMA mode. In FIFO mode the transfer path
does not go through the GPI engine, so this mechanism does not apply.

Let me clarify this wording in the commit message to make it explicit.

> > driver by setting a single lock_action selector per message, emitting lock
> > before the first message and unlock after the last message (handling the
> > single-message case as well). This serializes access to the shared
> > controller without requiring message-position flags to be passed into the
> > DMA engine layer.
> >
> > Signed-off-by: Mukesh Kumar Savaliya <mukesh.savaliya@xxxxxxxxxxxxxxxx>
> > ---
> >   drivers/i2c/busses/i2c-qcom-geni.c | 22 +++++++++++++++++++++-
> >   1 file changed, 21 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
> > index ae609bdd2ec4..a396ddc7d8f4 100644
> > --- a/drivers/i2c/busses/i2c-qcom-geni.c
> > +++ b/drivers/i2c/busses/i2c-qcom-geni.c
> > @@ -815,6 +815,14 @@ static int geni_i2c_gpi_xfer(struct geni_i2c_dev *gi2c, struct i2c_msg msgs[], i
> >   		if (i < num - 1)
> >   			peripheral.stretch = 1;
> >   
> > +		peripheral.lock_action = GPI_LOCK_NONE;
> > +		if (gi2c->se.multi_owner) {
> > +			if (i == 0)
> > +				peripheral.lock_action = GPI_LOCK_ACQUIRE;
> > +			else if (i == num - 1)
> > +				peripheral.lock_action = GPI_LOCK_RELEASE;
>
> You say above that single-messages case is handled, but if num == 1 then
> we will hit i == 0, set the acquire, we will not hit else, and then we
> will exit the loop. What am I missing?

You are right, the current implementation does not handle the
single-message case correctly. In that case we need both acquire
and release for the same message.

I will fix this by explicitly handling the num == 1 case and setting
both lock and unlock for that transfer.

> > +		}
> > +
> >   		peripheral.addr = msgs[i].addr;
> >   		if (i > 0 && (!(msgs[i].flags & I2C_M_RD)))
> >   			peripheral.multi_msg = false;
> > @@ -1014,6 +1022,11 @@ static int geni_i2c_probe(struct platform_device *pdev)
> >   		gi2c->clk_freq_out = I2C_MAX_STANDARD_MODE_FREQ;
> >   	}
> >   
> > +	if (of_property_read_bool(pdev->dev.of_node, "qcom,qup-multi-owner")) {
> > +		gi2c->se.multi_owner = true;
>
> gi2c->se.multi_owner = of_property_read_bool(pdev->dev.of_node, "qcom,qup-multi-owner");

Agreed, this can be simplified. I will update accordingly.

> > +		dev_dbg(&pdev->dev, "I2C controller is shared with another system processor\n");
> > +	}
> > +
> >   	if (has_acpi_companion(dev))
> >   		ACPI_COMPANION_SET(&gi2c->adap.dev, ACPI_COMPANION(dev));
> >   
> > @@ -1089,7 +1102,9 @@ static int geni_i2c_probe(struct platform_device *pdev)
> >   	}
> >   
> >   	if (fifo_disable) {
> > -		/* FIFO is disabled, so we can only use GPI DMA */
> > +		/* FIFO is disabled, so we can only use GPI DMA.
>
> That's not how we format comments outside the network subsystem.

Ack, I will fix the comment formatting.

> Regards,
> Bjorn
>
> > +		 * SE can be shared in GSI mode between subsystems, each SS owns a GPII.
> > +		 */
> >   		gi2c->gpi_mode = true;
> >   		ret = setup_gpi_dma(gi2c);
> >   		if (ret)
> > @@ -1098,6 +1113,11 @@ static int geni_i2c_probe(struct platform_device *pdev)
> >   		dev_dbg(dev, "Using GPI DMA mode for I2C\n");
> >   	} else {
> >   		gi2c->gpi_mode = false;
> > +
> > +		if (gi2c->se.multi_owner)
> > +			return dev_err_probe(dev, -EINVAL,
> > +					     "I2C sharing not supported in non GSI mode\n");
> > +
> >   		tx_depth = geni_se_get_tx_fifo_depth(&gi2c->se);
> >   
> >   		/* I2C Master Hub Serial Elements doesn't have the HW_PARAM_0 register */
> > --
> > 2.43.0