| Message ID | 20240920101820.44850-3-kfting@nuvoton.com |
|---|---|
| State | New |
| Headers | show |
| Series | i2c: npcm: read/write operation, checkpatch | expand |
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 >
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
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);