[PATCH v4 2/6] i2c: npcm: use a software flag to indicate a BER condition

[Andi Shyti]

Hi Tyrone,

patch looks good, but...

On Fri, Sep 20, 2024 at 06:18:16PM GMT, warp5tw at gmail.com wrote:
> From: Tyrone Ting <kfting at nuvoton.com>
> 
> From: Tyrone Ting <kfting at nuvoton.com>
> 
> If not clearing the BB (bus busy) condition in the BER (bus error)
> interrupt, the driver causes a timeout and hence the i2c core
> doesn't do the i2c transfer retry but returns the driver's return
> value to the upper layer instead.
> 
> Clear the BB condition in the BER interrupt and a software flag is
> used. The driver does an i2c recovery without causing the timeout
> if the flag is set.
> 
> Signed-off-by: Tyrone Ting <kfting at nuvoton.com>
> Reviewed-by: Tali Perry <tali.perry1 at gmail.com>
> ---
>  drivers/i2c/busses/i2c-npcm7xx.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/busses/i2c-npcm7xx.c b/drivers/i2c/busses/i2c-npcm7xx.c
> index 2b76dbfba438..2d034503d8bc 100644
> --- a/drivers/i2c/busses/i2c-npcm7xx.c
> +++ b/drivers/i2c/busses/i2c-npcm7xx.c
> @@ -334,6 +334,7 @@ struct npcm_i2c {
>  	u64 nack_cnt;
>  	u64 timeout_cnt;
>  	u64 tx_complete_cnt;
> +	bool ber_state;

I need some comment here, what is this ber state? You described
it in the commit log, but people won't be browsing much of the
commit log rather than the code itself.

You could perhaps mention the meaning as:

	bool ber_state; /* brief description */

and...

>  };
>  
>  static inline void npcm_i2c_select_bank(struct npcm_i2c *bus,
> @@ -1521,6 +1522,7 @@ static void npcm_i2c_irq_handle_ber(struct npcm_i2c *bus)
>  	if (npcm_i2c_is_master(bus)) {
>  		npcm_i2c_master_abort(bus);
>  	} else {
> +		bus->ber_state = true;
>  		npcm_i2c_clear_master_status(bus);
>  
>  		/* Clear BB (BUS BUSY) bit */
> @@ -1699,6 +1701,7 @@ static int npcm_i2c_recovery_tgclk(struct i2c_adapter *_adap)
>  		dev_dbg(bus->dev, "bus%d-0x%x recovery skipped, bus not stuck",
>  			bus->num, bus->dest_addr);
>  		npcm_i2c_reset(bus);
> +		bus->ber_state = false;
>  		return 0;
>  	}
>  
> @@ -1763,6 +1766,7 @@ static int npcm_i2c_recovery_tgclk(struct i2c_adapter *_adap)
>  		if (bus->rec_succ_cnt < ULLONG_MAX)
>  			bus->rec_succ_cnt++;
>  	}
> +	bus->ber_state = false;
>  	return status;
>  }
>  
> @@ -2158,7 +2162,7 @@ static int npcm_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
>  
>  	} while (time_is_after_jiffies(time_left) && bus_busy);
>  
> -	if (bus_busy) {
> +	if (bus_busy || bus->ber_state) {

/*
 * Check the BER (bus error) state, if it's true means that blah
 * blah blah, while when it's false blah blah blah and we should
 * or should not do blah blah blah
 */
if (bus_busy || bus->ber_state) {
	...
}

Thanks,
Andi

>  		iowrite8(NPCM_I2CCST_BB, bus->reg + NPCM_I2CCST);
>  		npcm_i2c_reset(bus);
>  		i2c_recover_bus(adap);
> -- 
> 2.34.1
>