Re: [PATCH v2 6/6] i2c: spacemit: introduce pio for k1

[Troy Mitchell <troy.mitchell@linux.spacemit.com>]

On Sat, Sep 27, 2025 at 06:56:16PM +0800, Yixun Lan wrote:
> On 10:02 Thu 25 Sep     , Troy Mitchell wrote:
> > This patch introduces I2C PIO functionality for the Spacemit K1 SoC,
> > enabling the use of I2C with interrupts disabled.
> > 
> > Signed-off-by: Troy Mitchell <troy.mitchell@linux.spacemit.com>
> > ---
> >  drivers/i2c/busses/i2c-k1.c | 164 +++++++++++++++++++++++++++++++++++++-------
> >  1 file changed, 140 insertions(+), 24 deletions(-)
> > 
> > diff --git a/drivers/i2c/busses/i2c-k1.c b/drivers/i2c/busses/i2c-k1.c
> > index 6b918770e612e098b8ad17418f420d87c94df166..e403eb7d6f329f4fe5c5242f94cc21094dff105c 100644
> > --- a/drivers/i2c/busses/i2c-k1.c
> > +++ b/drivers/i2c/busses/i2c-k1.c
> > @@ -97,6 +97,9 @@
> >  
> ..
> >  static int spacemit_i2c_xfer_msg(struct spacemit_i2c_dev *i2c)
> >  {
> >  	unsigned long time_left;
> > @@ -310,10 +368,28 @@ static int spacemit_i2c_xfer_msg(struct spacemit_i2c_dev *i2c)
> >  
> >  		reinit_completion(&i2c->complete);
> >  
> > -		spacemit_i2c_start(i2c);
> > +		if (i2c->is_pio) {
> > +			/* We disable the interrupt to avoid unintended spurious triggers. */
> the comment is suspicious, and seems wrong..
> > +			disable_irq(i2c->irq);
> > +
> I guess this doesn't disable interrupt in the hardware layer, it will still
> fire interrupt once enabled, so instead of calling disable_irq(), why not
> dealing with ISR setting of the controller? I mean those "IE bits"(interrupt
> enableing) of ICR REGISTER, disabling them should prevent the interrupt
> triggered?
For example, take MSD (master stop detect).
If we disable this interrupt, even the interrupt status bit will never be triggered.
Then how are we supposed to know when the transfer has completed?
That’s why we disable the global interrupt here, but still keep the pending bit.

> 
> > +			/*
> > +			 * The K1 I2C controller has a quirk:
> > +			 * when doing PIO transfers, the status register must be cleared
> > +			 * of all other bits before issuing a START.
> > +			 * Failing to do so will prevent the transfer from working properly.
> > +			 */
> > +			spacemit_i2c_clear_int_status(i2c, SPACEMIT_I2C_INT_STATUS_MASK);
> this also doesn't seem correct to me, the irq status bit should be cleared once
> interrupt occur,
> not dealing it here before sending msg, this seems a
> wrong procedure
I'll double check it, as I recall that if it’s not cleared here,
the second message will inevitably fail.

                - Troy
> 
> > +
> > +			spacemit_i2c_start(i2c);
> > +			time_left = spacemit_i2c_wait_pio_xfer(i2c);
> > +
> > +			enable_irq(i2c->irq);
> > +		} else {
> > +			spacemit_i2c_start(i2c);
> > +			time_left = wait_for_completion_timeout(&i2c->complete,
> > +								i2c->adapt.timeout);
> > +		}
> >  
> > -		time_left = wait_for_completion_timeout(&i2c->complete,
> > -							i2c->adapt.timeout);
> >  		if (!time_left) {
> >  			dev_err(i2c->dev, "msg completion timeout\n");
> >  			spacemit_i2c_conditionally_reset_bus(i2c);
> > @@ -341,6 +417,9 @@ static bool spacemit_i2c_is_last_msg(struct spacemit_i2c_dev *i2c)
> 
> -- 
> Yixun Lan (dlan)
>