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

[Andi Shyti]

Hi Junyi,

Sashiko's comments look legit to me, can you please take a look
at those?

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

But perhaps the two can be even merged.

(i2c->msg->len - i2c->pos) is equal to (i2c->pos < i2c->msg->len)

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

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

> +	}
> +
> +	if (i2c->error)
> +		ret = i2c->error;
> +	spin_unlock_irqrestore(&i2c->lock, flags);
> +
> +	return ret;
> +}