| Message ID | 20241227115154.56154-9-prabhakar.mahadev-lad.rj@bp.renesas.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | i2c: riic: driver cleanup and improvements | expand |
Fri, Dec 27, 2024 at 11:51:54AM +0000, Prabhakar kirjoitti: > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > Introduce a new `riic_bus_barrier()` function to verify bus availability > before initiating an I2C transfer. This function enhances the bus > arbitration check by ensuring that the SDA and SCL lines are not held low, > in addition to checking the BBSY flag using `readb_poll_timeout()`. > > Previously, only the BBSY flag was checked to determine bus availability. > However, it is possible for the SDA line to remain low even when BBSY = 0. > This new implementation performs an additional check on the SDA and SCL > lines to avoid potential bus contention issues. ... > + /* > + * The SDA line can still be low even when BBSY = 0. Therefore, after checking > + * the BBSY flag, also verify that the SDA and SCL lines are not being held low. > + */ > + ret = readb_poll_timeout(riic->base + riic->info->regs[RIIC_ICCR2], val, > + !(val & ICCR2_BBSY), 10, riic->adapter.timeout); > + if (ret) > + return -EBUSY; Why the return code is shadowed? Is it requirement by ABI? > + if ((riic_readb(riic, RIIC_ICCR1) & (ICCR1_SDAI | ICCR1_SCLI)) != > + (ICCR1_SDAI | ICCR1_SCLI)) > + return -EBUSY; > + > + return 0; > +}
Hi Andy, Thank you for the review. On Sat, Dec 28, 2024 at 11:40 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > Fri, Dec 27, 2024 at 11:51:54AM +0000, Prabhakar kirjoitti: > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > Introduce a new `riic_bus_barrier()` function to verify bus availability > > before initiating an I2C transfer. This function enhances the bus > > arbitration check by ensuring that the SDA and SCL lines are not held low, > > in addition to checking the BBSY flag using `readb_poll_timeout()`. > > > > Previously, only the BBSY flag was checked to determine bus availability. > > However, it is possible for the SDA line to remain low even when BBSY = 0. > > This new implementation performs an additional check on the SDA and SCL > > lines to avoid potential bus contention issues. > > ... > > > + /* > > + * The SDA line can still be low even when BBSY = 0. Therefore, after checking > > + * the BBSY flag, also verify that the SDA and SCL lines are not being held low. > > + */ > > + ret = readb_poll_timeout(riic->base + riic->info->regs[RIIC_ICCR2], val, > > + !(val & ICCR2_BBSY), 10, riic->adapter.timeout); > > + if (ret) > > + return -EBUSY; > > Why the return code is shadowed? Is it requirement by ABI? > I will propagate the error instead of shadowing it. Cheers, Prabhakar
diff --git a/drivers/i2c/busses/i2c-riic.c b/drivers/i2c/busses/i2c-riic.c index a2d0cde5ac54..cf20e75da044 100644 --- a/drivers/i2c/busses/i2c-riic.c +++ b/drivers/i2c/busses/i2c-riic.c @@ -40,6 +40,7 @@ #include <linux/i2c.h> #include <linux/interrupt.h> #include <linux/io.h> +#include <linux/iopoll.h> #include <linux/module.h> #include <linux/of.h> #include <linux/platform_device.h> @@ -50,6 +51,8 @@ #define ICCR1_ICE BIT(7) #define ICCR1_IICRST BIT(6) #define ICCR1_SOWP BIT(4) +#define ICCR1_SCLI BIT(1) +#define ICCR1_SDAI BIT(0) #define ICCR2_BBSY BIT(7) #define ICCR2_SP BIT(3) @@ -135,6 +138,27 @@ static inline void riic_clear_set_bit(struct riic_dev *riic, u8 clear, u8 set, u riic_writeb(riic, (riic_readb(riic, reg) & ~clear) | set, reg); } +static int riic_bus_barrier(struct riic_dev *riic) +{ + int ret; + u8 val; + + /* + * The SDA line can still be low even when BBSY = 0. Therefore, after checking + * the BBSY flag, also verify that the SDA and SCL lines are not being held low. + */ + ret = readb_poll_timeout(riic->base + riic->info->regs[RIIC_ICCR2], val, + !(val & ICCR2_BBSY), 10, riic->adapter.timeout); + if (ret) + return -EBUSY; + + if ((riic_readb(riic, RIIC_ICCR1) & (ICCR1_SDAI | ICCR1_SCLI)) != + (ICCR1_SDAI | ICCR1_SCLI)) + return -EBUSY; + + return 0; +} + static int riic_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num) { struct riic_dev *riic = i2c_get_adapdata(adap); @@ -147,13 +171,11 @@ static int riic_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num) if (ret) return ret; - if (riic_readb(riic, RIIC_ICCR2) & ICCR2_BBSY) { - riic->err = -EBUSY; + riic->err = riic_bus_barrier(riic); + if (riic->err) goto out; - } reinit_completion(&riic->msg_done); - riic->err = 0; riic_writeb(riic, 0, RIIC_ICSR2);