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