Re: [PATCH v2 1/1] i2c: i2c-xiic: fix clearing of bnb irq

[Thilo Zirnstein]

Hi Andi, 

Thanks for your review and feedback.

On Fri, Oct 16, 2026 at 01:51:35AM +0200, Andi Shyti wrote:
> Hi Thilo,
> 
> On Thu, Oct 15, 2026 at 10:11:27AM +0200, Thilo Zirnstein wrote:
> > Add a function for clearing the bnb bit,
> > It waits for all preconditions before clearing the bnb bit or times out.
> > 
> > Tested on Zynqmp dev board with some custom logic.
> > Tested-by: Thilo Zirnstein <thi.zirnstein@xxxxxxxxx>
> > 
> 
> Please, don't leave blank lines between tags. Moreover in this
> case I don't need your Tested-by, I believe you tested your patch
> before sending.

Ack

> 
> > Signed-off-by: Thilo Zirnstein <thi.zirnstein@xxxxxxxxx>
> 
> I'd need here an ack from Michal.

Ok, but this is nothing i can steer somehow - right ? 

> 
> > ---
> >  drivers/i2c/busses/i2c-xiic.c | 42 +++++++++++++++++++++++++++--------
> >  1 file changed, 33 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
> > index 5e397a7e63f6..da9cedb86f94 100644
> > --- a/drivers/i2c/busses/i2c-xiic.c
> > +++ b/drivers/i2c/busses/i2c-xiic.c
> > @@ -230,7 +230,9 @@ static const struct timing_regs timing_reg_values[] = {
> >  
> >  #define XIIC_RESET_MASK             0xAUL
> >  
> > -#define XIIC_PM_TIMEOUT		1000	/* ms */
> > +#define XIIC_PM_TIMEOUT 1000 /* ms */
> 
> Why this change?

Unintentionally, :/ 

> 
> > +/* timeout waiting for bus busy */
> > +#define XIIC_BB_TIMEOUT		(msecs_to_jiffies(1))
> 
> I don't see much benefit from this define as it's used only once.
> It just makes it more difficult to read.

I think it is a question of personal preference,  
I prefer to read a meaningfull name instead of a "magical" number. 
The driver code has some other defines which are just used one time
( XIIC_PM_TIMEOUT, XIIC_XFER_TIMEOUT_US ), so it fits to the code base too. 

If you not insist, i keep it.

> 
> >  /* timeout waiting for the controller to respond */
> >  #define XIIC_I2C_TIMEOUT	(msecs_to_jiffies(1000))
> >  /* timeout waiting for the controller finish transfers */
> > @@ -392,6 +394,30 @@ static int xiic_wait_tx_empty(struct xiic_i2c *i2c)
> >  	return 0;
> >  }
> >  
> > +static int xiic_clear_en_bnb(struct xiic_i2c *i2c)
> 
> Why do you need to return an error value if you ignore it
> everywhere?
> 
> > +{
> > +	u8 sr = 0, isr = 0;
> > +	unsigned long timeout;
> > +
> > +	timeout = jiffies + XIIC_BB_TIMEOUT;
> > +	for (isr = xiic_getreg8(i2c, XIIC_IISR_OFFSET);
> > +		(isr & XIIC_INTR_BNB_MASK);
> > +		isr = xiic_getreg8(i2c, XIIC_IISR_OFFSET)) {
> 
> this for loop is quite ugly. I believe that with a do...while()
> you can write a clearer code.

Ack, I can get rid of this whole loop if using 
read_poll_timeout_atomic()
A readback after clearing the bit is not needed.

> 
> > +
> > +		sr = xiic_getreg8(i2c, XIIC_SR_REG_OFFSET);
> > +		if (sr & XIIC_SR_BUS_BUSY_MASK)
> > +			xiic_setreg8(i2c, XIIC_IISR_OFFSET, XIIC_INTR_BNB_MASK);
> > +
> > +		if (time_after(jiffies, timeout)) {
> 
> If this is called in a context where interrupts are disabled,
> jiffies might not be incremented and you end up looping forever.
> How about using read_poll_timeout_atomic() instead?

Ack, good catch, i will fix that.

> 
> > +			dev_err(i2c->dev, "Failed to clear bnb\n");
> > +			return -ETIMEDOUT;
> > +		}
> > +	}
> > +
> > +	xiic_irq_en(i2c, XIIC_INTR_BNB_MASK);
> > +	return 0;
> > +}
> > +
> >  /**
> >   * xiic_setclk - Sets the configured clock rate
> >   * @i2c: Pointer to the xiic device structure
> 
> ...
> 
> > @@ -1225,8 +1248,9 @@ static void xiic_start_send(struct xiic_i2c *i2c)
> >  
> >  		/* Clear any pending Tx empty, Tx Error and then enable them */
> >  		xiic_irq_clr_en(i2c, XIIC_INTR_TX_EMPTY_MASK |
> > -				XIIC_INTR_TX_ERROR_MASK |
> > -				XIIC_INTR_BNB_MASK);
> > +						XIIC_INTR_TX_ERROR_MASK);
> > +
> 
> this needs to be aligned properly.

Ack

> 
> Andi
> 
> > +		xiic_clear_en_bnb(i2c);
> >  	}
> >  
> >  	i2c->prev_msg_tx = true;
> > -- 
> > 2.43.0
> >

Kind regards,

Thilo