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

[Xianwei Zhao]

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;
> > +}