Re: [PATCH v2 2/2] i2c: amlogic: Add Amlogic A9 I2C controller driver

[Xianwei Zhao <xianwei.zhao@amlogic.com>]

Hi Andi,
    Thanks for your review.

On 2026/10/26 23:22, Andi Shyti wrote:
> Hi Junyi,
> 
> Sashiko's comments look legit to me, can you please take a look
> at those?
> 

I'm currently looking at this part. Some of them don't have relatively 
good solutions yet, so I haven't replied.

> Besides...
> 
>> +static irqreturn_t aml_i2c_irq(int irqno, void *dev_id)
>> +{
>> +     struct aml_i2c *i2c = dev_id;
>> +     u8 *buf = NULL;
>> +     int ret;
>> +     u32 rd_len, i;
>> +     u32 rd_val;
>> +
>> +     spin_lock(&i2c->lock);
>> +     ret = controller_is_error(i2c);
>> +     if (ret) {
>> +             i2c->error = ret;
>> +             goto complete_irq_out;
>> +     }
>> +
>> +     switch (i2c->state) {
>> +     case A9_STATE_READ:
>> +             buf = i2c->msg->buf;
>> +             rd_len = min(i2c->msg->len - i2c->pos, A9_I2C_FIFO_DEPTH);
>> +             for (i = 0; i < rd_len; i++) {
>> +                     rd_val = readl(i2c->regs + REG_CFG_RX);
>> +                     buf[i2c->pos++] = rd_val & TX_RX_DATA;
>> +             }
>> +             /* Rx must clear thresh after read fifo */
>> +             clear_irq_status(i2c, A9_RX_THRESH_READ);
>> +             break;
>> +     case A9_STATE_WRITE:
>> +             aml_i2c_prepare_xfer(i2c, A9_FIFO_FILL_HALF);
>> +             if ((i2c->pos + A9_I2C_HALF_FIFO) >= i2c->msg->len)
>> +                     aml_i2c_set_mask(i2c, REG_CFG_IRQ_ENABLE, A9_TX_THRESH_WRITE, 0);
>> +             i2c->pos += i2c->count;
>> +             /* Tx must clear thresh after write fifo */
>> +             clear_irq_status(i2c, A9_TX_THRESH_WRITE);
>> +             break;
>> +     case A9_STATE_IDLE:
>> +             dev_warn(i2c->dev, "i2c irq triggered abnormally, irq_state:0x%x,irq_en:0x%x\n",
>> +                      readl(i2c->regs + REG_CFG_IRQ_STATE),
>> +                      readl(i2c->regs + REG_CFG_IRQ_ENABLE));
>> +             break;
>> +     default:
>> +             break;
>> +     }
>> +
>> +     if (!controller_is_done(i2c)) {
>> +             goto irq_trans_out;
>> +     } else {
>> +             if (i2c->state == A9_STATE_READ && (i2c->msg->len - i2c->pos))
>> +                     goto irq_trans_out;
>> +     }
> No need for brackets here:
> 
> if (...)
>          goto ...;
> else if (...)
>          goto ...;
> 

Will do.

> But perhaps the two can be even merged.

I will try to merged them.

> (i2c->msg->len - i2c->pos) is equal to (i2c->pos < i2c->msg->len)
> 

Will do.

>> +
>> +complete_irq_out:
>> +     reset_fifo_pos(i2c);
>> +     complete(&i2c->done);
>> +     i2c->state = A9_STATE_IDLE;
>> +     aml_i2c_set_mask(i2c, REG_CFG_IRQ_ENABLE, A9_RX_THRESH_READ | A9_TX_THRESH_WRITE, 0);
>> +     clear_irq_status(i2c, IRQ_ALL_MASK);
>> +irq_trans_out:
>> +     spin_unlock(&i2c->lock);
> ...
> 
>> +     i2c->state = (msg->flags & I2C_M_RD) ? A9_STATE_READ : A9_STATE_WRITE;
>> +     spin_lock_irqsave(&i2c->lock, flags);
>> +     aml_i2c_prepare_xfer(i2c, A9_FIFO_FILL_FULL);
>> +     i2c->pos += i2c->count;
>> +     spin_unlock_irqrestore(&i2c->lock, flags);
>> +     reinit_completion(&i2c->done);
>> +
>> +     if (last)
>> +             aml_i2c_set_mask(i2c, REG_CFG_BUS, BUS_NO_STOP, 0);
>> +     else
>> +             aml_i2c_set_mask(i2c, REG_CFG_BUS, BUS_NO_STOP, BUS_NO_STOP);
>> +     /* Start the transfer */
>> +     aml_i2c_do_start(i2c, msg);
> I think this should also be under spinlock in order to avoid that
> the interrupt handler accesses a partially initialized state.
> 
I will lock it.
>> +
>> +     time_left = msecs_to_jiffies(I2C_TIMEOUT_MS);
>> +     time_left = wait_for_completion_timeout(&i2c->done, time_left);
>> +
>> +     /*
>> +      * Protect access to i2c struct and registers from interrupt
>> +      * handlers triggered by a transfer terminated after the
>> +      * timeout period
>> +      */
>> +
>> +     spin_lock_irqsave(&i2c->lock, flags);
>> +     if (!time_left) {
>> +             i2c->state = A9_STATE_IDLE;
>> +             clear_irq_status(i2c, IRQ_ALL_MASK);
>> +             reset_fifo_pos(i2c);
>> +             ret = -ETIMEDOUT;
> the next instruction overwrites this error. Perhaps you could
> use a goto to spin_unlock and return.
> 
> Andi
>

Will do.

>> +     }
>> +
>> +     if (i2c->error)
>> +             ret = i2c->error;
>> +     spin_unlock_irqrestore(&i2c->lock, flags);
>> +
>> +     return ret;
>> +}

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic