From: Gao Pan <[email protected]>
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 <[email protected]>
Signed-off-by: Carlos Song <[email protected]>
---
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)
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
Hi Carlos Song,
> Fixes: a55fa9d0e42e ("i2c: imx-lpi2c: add low power i2c bus driver")
> Signed-off-by: Gao Pan <[email protected]>
> Signed-off-by: Carlos Song <[email protected]>
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, [email protected] wrote:
> From: Gao Pan <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Carlos Song <[email protected]>
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
>
Hi, Andy
> -----Original Message-----
> From: Andi Shyti <[email protected]>
> Sent: Wednesday, June 14, 2023 8:09 AM
> To: Carlos Song <[email protected]>
> Cc: Aisheng Dong <[email protected]>; [email protected];
> [email protected]; [email protected]; [email protected]; Clark
> Wang <[email protected]>; Bough Chen <[email protected]>;
> dl-linux-imx <[email protected]>; [email protected];
> [email protected]; [email protected]
> Subject: [EXT] Re: [PATCH] i2c: imx-lpi2c: directly return ISR when detect a NACK
>
> Caution: This is an external email. Please take care when clicking links or
> opening attachments. When in doubt, report the message using the 'Report this
> email' button
>
>
> Hi,
>
> On Wed, May 24, 2023 at 10:40:02AM +0800, [email protected] wrote:
> > From: Gao Pan <[email protected]>
> >
> > 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 <[email protected]>
> > Signed-off-by: Carlos Song <[email protected]>
>
> 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.
>
Carlos: I will use if else and remove goto in V2 patch.
> With the above I can give you a conditional r-b: are you able to pull Dong into
> this review as Wolfram asked?
>
Carlos: I have notified Aisheng. He will add review by.
> 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
> >