Re: [PATCH v7 2/2] i2c: spacemit: add support for SpacemiT K1 SoC

[Andi Shyti]

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

...