Re: [PATCH 3/3] i2c: cadence: Add atomic transfer support for controller version 1.4

[Andi Shyti]

Hi Manikata,

Sorry for the delay in reviewing this patch. Looks good, just a
few notes below.

...

> +static bool cdns_i2c_error_check(struct cdns_i2c *id)
> +{
> +	unsigned int isr_status;
> +
> +	id->err_status = 0;
> +
> +	isr_status = cdns_i2c_readreg(CDNS_I2C_ISR_OFFSET);
> +	cdns_i2c_writereg(isr_status & CDNS_I2C_IXR_ERR_INTR_MASK, CDNS_I2C_ISR_OFFSET);
> +
> +	id->err_status = isr_status & CDNS_I2C_IXR_ERR_INTR_MASK;
> +	if (id->err_status)
> +		return true;
> +
> +	return false;

return !!id->err_status;

> +}
> +
> +static void cdns_i2c_mrecv_atomic(struct cdns_i2c *id)
> +{
> +	bool updatetx;

Please move the udatex declaration inside the while loop.

> +	while (id->recv_count > 0) {
> +		/*
> +		 * Check if transfer size register needs to be updated again for a
> +		 * large data receive operation.
> +		 */
> +		updatetx = id->recv_count > id->curr_recv_count;
> +
> +		while (id->curr_recv_count > 0) {
> +			if (cdns_i2c_readreg(CDNS_I2C_SR_OFFSET) & CDNS_I2C_SR_RXDV) {
> +				*id->p_recv_buf++ = cdns_i2c_readreg(CDNS_I2C_DATA_OFFSET);

Can you please expand this operation to be a bit more clearer,
without asking people to check on operation precedence?

> +				id->recv_count--;
> +				id->curr_recv_count--;
> +
> +				/*
> +				 * Clear hold bit that was set for FIFO control
> +				 * if RX data left is less than or equal to
> +				 * FIFO DEPTH unless repeated start is selected

mmhhh... the lack of punctuation makes this comment difficult to
understand.

> +				 */
> +				if (id->recv_count <= id->fifo_depth && !id->bus_hold_flag)
> +					cdns_i2c_clear_bus_hold(id);
> +			}
> +			if (cdns_i2c_error_check(id))
> +				return;
> +			if (cdns_is_holdquirk(id, updatetx))
> +				break;
> +		}
> +
> +		/*
> +		 * The controller sends NACK to the slave when transfer size

/slave/target/

> +		 * register reaches zero without considering the HOLD bit.
> +		 * This workaround is implemented for large data transfers to
> +		 * maintain transfer size non-zero while performing a large
> +		 * receive operation.
> +		 */
> +		if (cdns_is_holdquirk(id, updatetx)) {
> +			/* wait while fifo is full */
> +			while (cdns_i2c_readreg(CDNS_I2C_XFER_SIZE_OFFSET) !=
> +			       (id->curr_recv_count - id->fifo_depth))
> +				;
> +
> +			/*
> +			 * Check number of bytes to be received against maximum
> +			 * transfer size and update register accordingly.
> +			 */
> +			if (((int)(id->recv_count) - id->fifo_depth) >

The cast is not needed here.

> +			    id->transfer_size) {
> +				cdns_i2c_writereg(id->transfer_size,
> +						  CDNS_I2C_XFER_SIZE_OFFSET);
> +				id->curr_recv_count = id->transfer_size +
> +						      id->fifo_depth;
> +			} else {
> +				cdns_i2c_writereg(id->recv_count -
> +						  id->fifo_depth,
> +						  CDNS_I2C_XFER_SIZE_OFFSET);
> +				id->curr_recv_count = id->recv_count;
> +			}
> +		}
> +	}
> +
> +	/* Clear hold (if not repeated start) */
> +	if (!id->recv_count && !id->bus_hold_flag)
> +		cdns_i2c_clear_bus_hold(id);
> +}
> +
>  /**
>   * cdns_i2c_mrecv - Prepare and start a master receive operation
>   * @id:		pointer to the i2c device structure
> @@ -715,7 +804,34 @@ static void cdns_i2c_mrecv(struct cdns_i2c *id)
>  		cdns_i2c_writereg(addr, CDNS_I2C_ADDR_OFFSET);
>  	}
>  
> -	cdns_i2c_writereg(CDNS_I2C_ENABLED_INTR_MASK, CDNS_I2C_IER_OFFSET);
> +	if (!id->atomic)
> +		cdns_i2c_writereg(CDNS_I2C_ENABLED_INTR_MASK, CDNS_I2C_IER_OFFSET);
> +	else
> +		cdns_i2c_mrecv_atomic(id);
> +}
> +
> +static void cdns_i2c_msend_rem_atomic(struct cdns_i2c *id)
> +{
> +	unsigned int avail_bytes;
> +	unsigned int bytes_to_send;

Please move these inside the while.

> +
> +	while (id->send_count) {
> +		avail_bytes = id->fifo_depth - cdns_i2c_readreg(CDNS_I2C_XFER_SIZE_OFFSET);
> +		if (id->send_count > avail_bytes)
> +			bytes_to_send = avail_bytes;
> +		else
> +			bytes_to_send = id->send_count;
> +
> +		while (bytes_to_send--) {
> +			cdns_i2c_writereg((*id->p_send_buf++), CDNS_I2C_DATA_OFFSET);
> +			id->send_count--;
> +		}
> +		if (cdns_i2c_error_check(id))
> +			return;
> +	}
> +
> +	if (!id->send_count && !id->bus_hold_flag)
> +		cdns_i2c_clear_bus_hold(id);
>  }
>  
>  /**
> @@ -778,7 +894,12 @@ static void cdns_i2c_msend(struct cdns_i2c *id)
>  	cdns_i2c_writereg(id->p_msg->addr & CDNS_I2C_ADDR_MASK,
>  						CDNS_I2C_ADDR_OFFSET);
>  
> -	cdns_i2c_writereg(CDNS_I2C_ENABLED_INTR_MASK, CDNS_I2C_IER_OFFSET);
> +	if (!id->atomic) {
> +		cdns_i2c_writereg(CDNS_I2C_ENABLED_INTR_MASK, CDNS_I2C_IER_OFFSET);
> +	} else {
> +		if (id->send_count > 0)

If you do:

	} else if (id->send_count > 0) {

we save a level of indentation.

> +			cdns_i2c_msend_rem_atomic(id);
> +	}
>  }
>  
>  /**
> @@ -818,7 +939,8 @@ static int cdns_i2c_process_msg(struct cdns_i2c *id, struct i2c_msg *msg,
>  
>  	id->p_msg = msg;
>  	id->err_status = 0;
> -	reinit_completion(&id->xfer_done);
> +	if (!id->atomic)
> +		reinit_completion(&id->xfer_done);
>  
>  	/* Check for the TEN Bit mode on each msg */
>  	reg = cdns_i2c_readreg(CDNS_I2C_CR_OFFSET);
> @@ -841,13 +963,24 @@ static int cdns_i2c_process_msg(struct cdns_i2c *id, struct i2c_msg *msg,
>  	/* Minimal time to execute this message */
>  	msg_timeout = msecs_to_jiffies((1000 * msg->len * BITS_PER_BYTE) / id->i2c_clk);
>  	/* Plus some wiggle room */
> -	msg_timeout += msecs_to_jiffies(500);
> +	if (!id->atomic)
> +		msg_timeout += msecs_to_jiffies(500);
> +	else
> +		msg_timeout += msecs_to_jiffies(2000);

You explained this in the commit log, can you add it in a
comment, as well?

>  
>  	if (msg_timeout < adap->timeout)
>  		msg_timeout = adap->timeout;
>  
> -	/* Wait for the signal of completion */
> -	time_left = wait_for_completion_timeout(&id->xfer_done, msg_timeout);
> +	if (!id->atomic) {
> +		/* Wait for the signal of completion */
> +		time_left = wait_for_completion_timeout(&id->xfer_done, msg_timeout);
> +	} else {
> +		/* 0 is success, -ETIMEDOUT is error */
> +		time_left = !readl_poll_timeout_atomic(id->membase + CDNS_I2C_ISR_OFFSET,
> +						       reg, (reg & CDNS_I2C_IXR_COMP),
> +						       CDNS_I2C_POLL_US_ATOMIC, msg_timeout);
> +	}

You can merge this if/else with the one above, to save some code.

> +

Thanks,
Andi