Re: [PATCH v2 5/5] i2c: img-scb: support repeated starts on IP v3.3

[James Hogan <james.hogan-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 4622 bytes --]

On 14/08/15 16:50, Sifan Naeem wrote:
> In version 3.3 of the IP when transaction halt is set, an interrupt
> will be generated after each byte of a transfer instead of after
> every transfer but before the stop bit.
> Due to this behaviour we have to be careful that every time we
> release the transaction halt we have to re-enable it straight away
> so that we only process a single byte, not doing so will result in
> all remaining bytes been processed and a stop bit being issued,
> which will prevent us having a repeated start.
> 
> This change will have no effect on earlier versions of the IP.
> 
> Signed-off-by: Sifan Naeem <sifan.naeem-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
> Reviewed-by: James Hartley <james.hartley-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/i2c/busses/i2c-img-scb.c |   45 ++++++++++++++++++++++++++++++--------
>  1 file changed, 36 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-img-scb.c b/drivers/i2c/busses/i2c-img-scb.c
> index 837a73a43a6d..7a51f39528d7 100644
> --- a/drivers/i2c/busses/i2c-img-scb.c
> +++ b/drivers/i2c/busses/i2c-img-scb.c
> @@ -513,7 +513,17 @@ static void img_i2c_soft_reset(struct img_i2c *i2c)
>  		       SCB_CONTROL_CLK_ENABLE | SCB_CONTROL_SOFT_RESET);
>  }
>  
> -/* enable or release transaction halt for control of repeated starts */
> +/*
> + * Enable or release transaction halt for control of repeated starts.
> + * In version 3.3 of the IP when transaction halt is set, an interrupt
> + * will be generated after each byte of a transfer instead of after
> + * every transfer but before the stop bit.
> + * Due to this behaviour we have to be careful that every time we
> + * release the transaction halt we have to re-enable it straight away
> + * so that we only process a single byte, not doing so will result in
> + * all remaining bytes been processed and a stop bit being issued,
> + * which will prevent us having a repeated start.
> + */
>  static void img_i2c_transaction_halt(struct img_i2c *i2c, bool t_halt)
>  {
>  	u32 val;
> @@ -582,7 +592,6 @@ static void img_i2c_read(struct img_i2c *i2c)
>  	img_i2c_writel(i2c, SCB_READ_ADDR_REG, i2c->msg.addr);
>  	img_i2c_writel(i2c, SCB_READ_COUNT_REG, i2c->msg.len);
>  
> -	img_i2c_transaction_halt(i2c, false);
>  	mod_timer(&i2c->check_timer, jiffies + msecs_to_jiffies(1));
>  }
>  
> @@ -596,7 +605,6 @@ static void img_i2c_write(struct img_i2c *i2c)
>  	img_i2c_writel(i2c, SCB_WRITE_ADDR_REG, i2c->msg.addr);
>  	img_i2c_writel(i2c, SCB_WRITE_COUNT_REG, i2c->msg.len);
>  
> -	img_i2c_transaction_halt(i2c, false);
>  	mod_timer(&i2c->check_timer, jiffies + msecs_to_jiffies(1));
>  	img_i2c_write_fifo(i2c);
>  
> @@ -863,7 +871,7 @@ static unsigned int img_i2c_auto(struct img_i2c *i2c,
>  	/* Enable transaction halt on start bit */
>  	if (line_status & LINESTAT_START_BIT_DET) {
>  		if (!i2c->last_msg) {
> -			img_i2c_transaction_halt(i2c, true);
> +			img_i2c_transaction_halt(i2c, !i2c->last_msg);

This hunk seems pointless given the "if" conditional it resides in.

Otherwise
Acked-by: James Hogan <james.hogan-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>

Cheers
James

>  			/* we're no longer interested in the slave event */
>  			i2c->int_enable &= ~INT_SLAVE_EVENT;
>  		}
> @@ -1093,12 +1101,31 @@ static int img_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
>  		img_i2c_writel(i2c, SCB_INT_CLEAR_REG, ~0);
>  		img_i2c_writel(i2c, SCB_CLEAR_REG, ~0);
>  
> -		if (atomic)
> +		if (atomic) {
>  			img_i2c_atomic_start(i2c);
> -		else if (msg->flags & I2C_M_RD)
> -			img_i2c_read(i2c);
> -		else
> -			img_i2c_write(i2c);
> +		} else {
> +			/*
> +			 * Enable transaction halt if not the last message in
> +			 * the queue so that we can control repeated starts.
> +			 */
> +			img_i2c_transaction_halt(i2c, !i2c->last_msg);
> +
> +			if (msg->flags & I2C_M_RD)
> +				img_i2c_read(i2c);
> +			else
> +				img_i2c_write(i2c);
> +
> +			/*
> +			 * Release and then enable transaction halt, to
> +			 * allow only a single byte to proceed.
> +			 * This doesn't have an effect on the initial transfer
> +			 * but will allow the following transfers to start
> +			 * processing if the previous transfer was marked as
> +			 * complete while the i2c block was halted.
> +			 */
> +			img_i2c_transaction_halt(i2c, false);
> +			img_i2c_transaction_halt(i2c, !i2c->last_msg);
> +		}
>  		spin_unlock_irqrestore(&i2c->lock, flags);
>  
>  		time_left = wait_for_completion_timeout(&i2c->msg_complete,
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]