On 2025/3/19 06:18, Andi Shyti wrote:
> Hi Troy,
>
> the driver looks good, I just have a few neetpicks to point out,
> that, along with Alex last comments, makes the v8 a good
> candidate to be sent.
>
> We still have time, to get this merged in this release cycle.
Hi Andi,
I accept all of your and Alex's suggestions and will send v8 today.
Thank you both for your patience!
>
> ...
>
>> +enum spacemit_i2c_state {
>> + STATE_IDLE,
>> + STATE_START,
>> + STATE_READ,
>> + STATE_WRITE,
>
> Can you please use the SPACEMIT prefix for these enume?
>
>> +};
>
> ...
>
>> +static void spacemit_i2c_init(struct spacemit_i2c_dev *i2c)
>> +{
>> + u32 val;
>> +
>> + /*
>> + * Unmask interrupt bits for all xfer mode:
>> + * bus error, arbitration loss detected.
>> + * For transaction complete signal, we use master stop
>> + * interrupt, so we don't need to unmask SPACEMIT_CR_TXDONEIE.
>> + */
>> + val = SPACEMIT_CR_BEIE | SPACEMIT_CR_ALDIE;
>> +
>> + /*
>> + * Unmask interrupt bits for interrupt xfer mode:
>> + * DBR RX full.
>> + * For tx empty interrupt SPACEMIT_CR_DTEIE, we only
>> + * need to enable when trigger byte transfer to start
>> + * data sending.
>
> Can you please rephrase this sentence to something more
> understandable?
>
>> + */
>> + val |= SPACEMIT_CR_DRFIE;
>> +
>> + if (i2c->clock_freq == SPACEMIT_I2C_MAX_FAST_MODE_FREQ)
>> + val |= SPACEMIT_CR_MODE_FAST;
>> +
>> + /* disable response to general call */
>> + val |= SPACEMIT_CR_GCD;
>> +
>> + /* enable SCL clock output */
>> + val |= SPACEMIT_CR_SCLE;
>> +
>> + /* enable master stop detected */
>> + val |= SPACEMIT_CR_MSDE | SPACEMIT_CR_MSDIE;
>> +
>> + writel(val, i2c->base + SPACEMIT_ICR);
>> +}
>> +
>> +static inline void
>> +spacemit_i2c_clear_int_status(struct spacemit_i2c_dev *i2c, u32 mask)
>> +{
>> + writel(mask & SPACEMIT_I2C_INT_STATUS_MASK, i2c->base + SPACEMIT_ISR);
>> +}
>> +
>> +static void spacemit_i2c_start(struct spacemit_i2c_dev *i2c)
>> +{
>> + u32 slave_addr_rw, val;
>
> please don't use the word "slave_*", use "target_*, instead,
> that's the new standard. Unless it aligns with the datasheets,
> but that's not the case.
>
>> + struct i2c_msg *cur_msg = i2c->msgs + i2c->msg_idx;
>> +
>> + i2c->read = !!(cur_msg->flags & I2C_M_RD);
>
> ...
>
>> +static int spacemit_i2c_xfer_msg(struct spacemit_i2c_dev *i2c)
>> +{
>> + unsigned long time_left;
>
> you can move this declaration inside the for loop.
>
>> + for (i2c->msg_idx = 0; i2c->msg_idx < i2c->msg_num; i2c->msg_idx++) {
>> + struct i2c_msg *msg = &i2c->msgs[i2c->msg_idx];
>> +
>
> ...
>
>> +static void spacemit_i2c_err_check(struct spacemit_i2c_dev *i2c)
>> +{
>> + u32 val;
>> +
>> + /*
>> + * send transaction complete signal:
>
> nit: /send/Send/
>
>> + * error happens, detect master stop
>> + */
>> + if (!(i2c->status & (SPACEMIT_SR_ERR | SPACEMIT_SR_MSD)))
>> + return;
>
> ...
>
>> +static void spacemit_i2c_calc_timeout(struct spacemit_i2c_dev *i2c)
>> +{
>> + unsigned long timeout;
>> + int idx = 0, cnt = 0;
>> +
>> + while (idx < i2c->msg_num) {
>> + cnt += (i2c->msgs + idx)->len + 1;
>> + idx++;
>> + }
>
> nit: with a for loop you would save the brackets and the idx
> initialization.
>
>> +
>> + /*
>> + * multiply by 9 because each byte in I2C transmission requires
>
> nit: /multiply/Multiply/
>
>> + * 9 clock cycles: 8 bits of data plus 1 ACK/NACK bit.
>> + */
>> + timeout = cnt * 9 * USEC_PER_SEC / i2c->clock_freq;
>> +
>> + i2c->adapt.timeout = usecs_to_jiffies(timeout + USEC_PER_SEC / 10) / i2c->msg_num;
>> +}
>> +
>> +static int spacemit_i2c_xfer(struct i2c_adapter *adapt, struct i2c_msg *msgs, int num)
>> +{
>> + struct spacemit_i2c_dev *i2c = i2c_get_adapdata(adapt);
>> + int ret;
>> +
>> + i2c->msgs = msgs;
>> + i2c->msg_num = num;
>> +
>> + spacemit_i2c_calc_timeout(i2c);
>> +
>> + spacemit_i2c_init(i2c);
>> +
>> + spacemit_i2c_enable(i2c);
>> +
>> + ret = spacemit_i2c_wait_bus_idle(i2c);
>> + if (!ret)
>> + spacemit_i2c_xfer_msg(i2c);
>> +
>> + if (ret < 0)
>> + dev_dbg(i2c->dev, "i2c transfer error: %d\n", ret);
>> + else if (ret)
>> + spacemit_i2c_check_bus_release(i2c);
>
> Nit: this can all be:
>
> if (!ret)
> ...
> else if (ret < 0)
> ...
> else
> ...
>
>> +
>> + spacemit_i2c_disable(i2c);
>> +
>> + if (ret == -ETIMEDOUT || ret == -EAGAIN)
>> + dev_alert(i2c->dev, "i2c transfer failed, ret %d err 0x%lx\n",
>> + ret, i2c->status & SPACEMIT_SR_ERR);
>
> dev_alert? Is it that bad? Let's use dev_err() instead.
>
>> +
>> + return ret < 0 ? ret : num;
>> +}
>
> ...
--
Troy Mitchell
