1. Patchew
  2. linux
  3. [PATCH v4 0/6] i2c: npcm: read/write operation, checkpatch
  4. View patch

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

warp5tw@gmail.com posted 6 patches 2 months, 1 week ago
There is a newer version of this series
[PATCH v4 2/6] i2c: npcm: use a software flag to indicate a BER condition
Posted by warp5tw@gmail.com 2 months, 1 week ago 
From: Tyrone Ting <kfting@nuvoton.com>

From: Tyrone Ting <kfting@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@nuvoton.com>
Reviewed-by: Tali Perry <tali.perry1@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;
 };
 
 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) {
 		iowrite8(NPCM_I2CCST_BB, bus->reg + NPCM_I2CCST);
 		npcm_i2c_reset(bus);
 		i2c_recover_bus(adap);
-- 
2.34.1
Re: [PATCH v4 2/6] i2c: npcm: use a software flag to indicate a BER condition
Posted by Andi Shyti 2 months ago 
Hi Tyrone,

patch looks good, but...

On Fri, Sep 20, 2024 at 06:18:16PM GMT, warp5tw@gmail.com wrote:
> From: Tyrone Ting <kfting@nuvoton.com>
> 
> From: Tyrone Ting <kfting@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@nuvoton.com>
> Reviewed-by: Tali Perry <tali.perry1@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
>
Re: [PATCH v4 2/6] i2c: npcm: use a software flag to indicate a BER condition
Posted by Tyrone Ting 2 months ago 
Hi Andi:

Thank you for your feedback.

Andi Shyti <andi.shyti@kernel.org> 於 2024年9月26日 週四 上午4:22寫道：
>
> Hi Tyrone,
>
> patch looks good, but...
>
> On Fri, Sep 20, 2024 at 06:18:16PM GMT, warp5tw@gmail.com wrote:
> > From: Tyrone Ting <kfting@nuvoton.com>
> >
> > From: Tyrone Ting <kfting@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@nuvoton.com>
> > Reviewed-by: Tali Perry <tali.perry1@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 */
>

Understood. I'll add some descriptions.

> 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) {
>         ...
> }
>

Understood. Your comments will be addressed.

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

Thank you.

Regards,
Tyrone

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.