On Wed, 13 Sept 2023 at 15:12, huangzheng lai <laihuangzheng@xxxxxxxxx> wrote:
>
> Hi Chunyan，
>
> I don't think it's necessary to clear i2c_dev->ack_flag in sprd_i2c_clear_ack() . Because every time a new interrupt is triggered, it will retrieve the value of i2c_dev->ack_flag in sprd_i2c_isr and then use it in sprd_i2c_isr_thread.
You're assuming that ack_flag is and will be used in sprd_i2c_isr_thread() only.
If ack_flag is used to represent the ack/nack bit of interrupt status
register, I think we should clear it as well when clearing ack/nack
bit.
BTW, please make sure your email is plain-text mode so that people can
receive from the mail list, and do not top-post again.
Thanks,
Chunyan
>
> On Sun, Sep 3, 2023 at 5:05 AM Andi Shyti <andi.shyti@xxxxxxxxxx> wrote:
>>
>> 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@xxxxxxxxxx>
>>
>> Is this a fix? Then please consider adding
>>
>> Fixes: 8b9ec0719834 ("i2c: Add Spreadtrum I2C controller driver")
>> Cc: <stable@xxxxxxxxxxxxxxx> # v4.14+
>>
>> > ---
>> > 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-sprd.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?
>>
>> > struct completion complete;
>> > struct reset_control *rst;
>> > u8 *buf;
>> > @@ -384,7 +385,6 @@ static irqreturn_t sprd_i2c_isr_thread(int irq, void *dev_id)
>> > {
>> > struct sprd_i2c *i2c_dev = dev_id;
>> > struct i2c_msg *msg = i2c_dev->msg;
>> > - bool ack = !(readl(i2c_dev->base + I2C_STATUS) & I2C_RX_ACK);
>> > u32 i2c_tran;
>> >
>> > if (msg->flags & I2C_M_RD)
>> > @@ -400,7 +400,7 @@ static irqreturn_t sprd_i2c_isr_thread(int irq, void *dev_id)
>> > * For reading data, ack is always true, if i2c_tran is not 0 which
>> > * means we still need to contine to read data from slave.
>> > */
>> > - if (i2c_tran && ack) {
>> > + if (i2c_tran && i2c_dev->ack_flag) {
>> > sprd_i2c_data_transfer(i2c_dev);
>> > return IRQ_HANDLED;
>> > }
>> > @@ -411,7 +411,7 @@ static irqreturn_t sprd_i2c_isr_thread(int irq, void *dev_id)
>> > * If we did not get one ACK from slave when writing data, we should
>> > * return -EIO to notify users.
>> > */
>> > - if (!ack)
>> > + if (!i2c_dev->ack_flag)
>> > i2c_dev->err = -EIO;
>> > else if (msg->flags & I2C_M_RD && i2c_dev->count)
>> > sprd_i2c_read_bytes(i2c_dev, i2c_dev->buf, i2c_dev->count);
>> > @@ -428,7 +428,6 @@ static irqreturn_t sprd_i2c_isr(int irq, void *dev_id)
>> > {
>> > struct sprd_i2c *i2c_dev = dev_id;
>> > struct i2c_msg *msg = i2c_dev->msg;
>> > - bool ack = !(readl(i2c_dev->base + I2C_STATUS) & I2C_RX_ACK);
>> > u32 i2c_tran;
>> >
>> > if (msg->flags & I2C_M_RD)
>> > @@ -447,7 +446,8 @@ static irqreturn_t sprd_i2c_isr(int irq, void *dev_id)
>> > * means we can read all data in one time, then we can finish this
>> > * transmission too.
>> > */
>> > - if (!i2c_tran || !ack) {
>> > + i2c_dev->ack_flag = !(readl(i2c_dev->base + I2C_STATUS) & I2C_RX_ACK);
>>
>> there is a question from Chunyan here.
>>
>> I like more
>>
>> val = readl(...);
>> i2c_dev->ack_flag = !(val & I2C_RX_ACK);
>>
>> a matter of taste, your choice.
>>
>> Andi
>>
>> > + if (!i2c_tran || !i2c_dev->ack_flag) {
>> > sprd_i2c_clear_start(i2c_dev);
>> > sprd_i2c_clear_irq(i2c_dev);
>> > }
>> > --
>> > 2.17.1
>> >
