Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.31 as permitted sender) client-ip=23.128.96.31;
MIME-Version: 1.0
References: <20230817094520.21286-1-Huangzheng.Lai@unisoc.com>
 <20230817094520.21286-4-Huangzheng.Lai@unisoc.com> <20230902210513.3xelrcdtynz45p4o@zenone.zhora.eu>
In-Reply-To: <20230902210513.3xelrcdtynz45p4o@zenone.zhora.eu>
From:   huangzheng lai <laihuangzheng@gmail.com>
Date:   Wed, 13 Sep 2023 20:28:11 +0800
Message-ID: <CAAA1NbZOctfNqtnRRMVwk_ZONPnExVum4+R2xhFN_ezsfpzk0A@mail.gmail.com>
Subject: Re: [PATCH 3/8] i2c: sprd: Use global variables to record IIC
 ack/nack status instead of local variables
To:     Andi Shyti <andi.shyti@kernel.org>
Cc:     Huangzheng Lai <Huangzheng.Lai@unisoc.com>,
        Orson Zhai <orsonzhai@gmail.com>,
        Baolin Wang <baolin.wang@linux.alibaba.com>,
        Chunyan Zhang <zhang.lyra@gmail.com>,
        linux-i2c@vger.kernel.org, linux-kernel@vger.kernel.org,
        Xiongpeng Wu <xiongpeng.wu@unisoc.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi Andi,

On Sun, Sep 3, 2023 at 5:05=E2=80=AFAM Andi Shyti <andi.shyti@kernel.org> w=
rote:
>
> Hi Huangzheng,
>
> On Thu, Aug 17, 2023 at 05:45:15PM +0800, Huangzheng Lai wrote:
> > We found that when the interrupt bit of the IIC controller is cleared,
> > the ack/nack bit is also cleared at the same time. After clearing the
> > interrupt bit in sprd_i2c_isr(), incorrect ack/nack information will be
> > obtained in sprd_i2c_isr_thread(), resulting in incorrect communication
> > when nack cannot be recognized. To solve this problem, we used a global
> > variable to record ack/nack information before clearing the interrupt
> > bit instead of a local variable.
> >
> > Signed-off-by: Huangzheng Lai <Huangzheng.Lai@unisoc.com>
>
> Is this a fix? Then please consider adding
>
> Fixes: 8b9ec0719834 ("i2c: Add Spreadtrum I2C controller driver")
> Cc: <stable@vger.kernel.org> # v4.14+

Thank you for your prompt. In the next version of the patch, I will
add the fixes tag.

>
> > ---
> >  drivers/i2c/busses/i2c-sprd.c | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-sprd.c b/drivers/i2c/busses/i2c-spr=
d.c
> > index 066b3a9c30c8..549b60dd3273 100644
> > --- a/drivers/i2c/busses/i2c-sprd.c
> > +++ b/drivers/i2c/busses/i2c-sprd.c
> > @@ -85,6 +85,7 @@ struct sprd_i2c {
> >       struct clk *clk;
> >       u32 src_clk;
> >       u32 bus_freq;
> > +     bool ack_flag;
>
> smells a bit racy... however we are in the same interrupt cycle.
>
> Do you think we might need a spinlock around here?

The fifo empty and full interrupt enable will be turned off in
sprd_i2c_isr(), and will not be reset until sprd_i2c_isr_thread()
finishes processing, depending on the situation. Apart from these two
interrupt types, there are only two types left: transmission
completion and transmission failure. Both interrupts need to be re
initiated for transmission to occur, and transmission will not be re
initiated until the current data processing is completed.

Thanks,
Huangzheng