| Message ID | 20230524024002.1790405-1-carlos.song@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | i2c: imx-lpi2c: directly return ISR when detect a NACK | expand |
Hi Carlos Song, > Fixes: a55fa9d0e42e ("i2c: imx-lpi2c: add low power i2c bus driver") > Signed-off-by: Gao Pan <pandy.gao@nxp.com> > Signed-off-by: Carlos Song <carlos.song@nxp.com> You sent quite some fixes for the lpi2c driver, thanks for that! Much appreciated. Current maintainer of this driver is Dong Aisheng. Since you are in the same company, maybe you can notify him more directly than I can? Happy hacking, Wolfram
Hi, On Wed, May 24, 2023 at 10:40:02AM +0800, carlos.song@nxp.com wrote: > From: Gao Pan <pandy.gao@nxp.com> > > A NACK flag in ISR means i2c bus error. In such codition, > there is no need to do read/write operation. It's better > to return ISR directly and then stop i2c transfer. > > Fixes: a55fa9d0e42e ("i2c: imx-lpi2c: add low power i2c bus driver") > Signed-off-by: Gao Pan <pandy.gao@nxp.com> > Signed-off-by: Carlos Song <carlos.song@nxp.com> looks good to me, just a little note. > --- > drivers/i2c/busses/i2c-imx-lpi2c.c | 12 +++++++----- > 1 file changed, 7 insertions(+), 5 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c b/drivers/i2c/busses/i2c-imx-lpi2c.c > index 1af0a637d7f1..11240bf8e8e2 100644 > --- a/drivers/i2c/busses/i2c-imx-lpi2c.c > +++ b/drivers/i2c/busses/i2c-imx-lpi2c.c > @@ -514,15 +514,17 @@ static irqreturn_t lpi2c_imx_isr(int irq, void *dev_id) > temp = readl(lpi2c_imx->base + LPI2C_MSR); > temp &= enabled; > > + if (temp & MSR_NDF) { > + complete(&lpi2c_imx->complete); > + goto ret; > + } > + > if (temp & MSR_RDF) else if () and remove goto and brackets. You have used the "else if" below and we can keep it consistent. This way the commit log should be a bit different as we decide to check exclusively using else if's for the active bits. This way we also stop processing the MSR register if a NACK is received. With the above I can give you a conditional r-b: are you able to pull Dong into this review as Wolfram asked? Thank you, Andi > lpi2c_imx_read_rxfifo(lpi2c_imx); > - > - if (temp & MSR_TDF) > + else if (temp & MSR_TDF) > lpi2c_imx_write_txfifo(lpi2c_imx); > > - if (temp & MSR_NDF) > - complete(&lpi2c_imx->complete); > - > +ret: > return IRQ_HANDLED; > } > > -- > 2.34.1 >
diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c b/drivers/i2c/busses/i2c-imx-lpi2c.c index 1af0a637d7f1..11240bf8e8e2 100644 --- a/drivers/i2c/busses/i2c-imx-lpi2c.c +++ b/drivers/i2c/busses/i2c-imx-lpi2c.c @@ -514,15 +514,17 @@ static irqreturn_t lpi2c_imx_isr(int irq, void *dev_id) temp = readl(lpi2c_imx->base + LPI2C_MSR); temp &= enabled; + if (temp & MSR_NDF) { + complete(&lpi2c_imx->complete); + goto ret; + } + if (temp & MSR_RDF) lpi2c_imx_read_rxfifo(lpi2c_imx); - - if (temp & MSR_TDF) + else if (temp & MSR_TDF) lpi2c_imx_write_txfifo(lpi2c_imx); - if (temp & MSR_NDF) - complete(&lpi2c_imx->complete); - +ret: return IRQ_HANDLED; }