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

[Andi Shyti <andi.shyti@kernel.org>]

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@gmail.com>
> 

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.

> Signed-off-by: Thilo Zirnstein <thi.zirnstein@gmail.com>

I'd need here an ack from Michal.

> ---
>  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?

> +/* 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.

>  /* 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.

> +
> +		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?

> +			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.

Andi

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