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

[Troy Mitchell]

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