Re: [PATCH 6/7] i2c: img-scb: add handle for stop detected interrupt

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

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

On Wed, Jul 29, 2015 at 06:06:24PM +0100, Sifan Naeem wrote:
> Hi James,
> 
> > -----Original Message-----
> > From: James Hogan
> > Sent: 29 July 2015 16:34
> > To: Sifan Naeem; Wolfram Sang; linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > Subject: Re: [PATCH 6/7] i2c: img-scb: add handle for stop detected interrupt
> > 
> > On 27/07/15 12:56, Sifan Naeem wrote:
> > > Stop Detected interrupt is triggered when a Stop bit is detected on
> > > the bus, which indicates the end of the current transfer.
> > >
> > > When the end of a transfer is indicated by the Stop bit interrupt,
> > > drain the FIFO and signal completion for the transaction. But if the
> > > interrupt was triggered before all data is written to the fifo or with
> > > more data expected return error with transfer complete signal.
> > >
> > > Halting the bus is no longer necessary after a stop bit is detected on
> > > the bus, as there cannot be a repeated start transfer when the stop
> > > bit has been issued, hence remove the transaction halt bit.
> > >
> > > Signed-off-by: Sifan Naeem <sifan.naeem-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
> > > ---
> > >  drivers/i2c/busses/i2c-img-scb.c |   28 +++++++++++++++++++++++++++-
> > >  1 file changed, 27 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/i2c/busses/i2c-img-scb.c
> > > b/drivers/i2c/busses/i2c-img-scb.c
> > > index 10141a9..90faf48 100644
> > > --- a/drivers/i2c/busses/i2c-img-scb.c
> > > +++ b/drivers/i2c/busses/i2c-img-scb.c
> > > @@ -152,6 +152,7 @@
> > >  #define INT_TRANSACTION_DONE		BIT(15)
> > >  #define INT_SLAVE_EVENT			BIT(16)
> > >  #define INT_TIMING			BIT(18)
> > > +#define INT_STOP_DETECTED		BIT(19)
> > >
> > >  #define INT_FIFO_FULL_FILLING	(INT_FIFO_FULL  |
> > INT_FIFO_FILLING)
> > >
> > > @@ -175,7 +176,8 @@
> > >  					 INT_WRITE_ACK_ERR    | \
> > >  					 INT_FIFO_FULL        | \
> > >  					 INT_FIFO_FILLING     | \
> > > -					 INT_FIFO_EMPTY)
> > > +					 INT_FIFO_EMPTY       | \
> > > +					 INT_STOP_DETECTED)
> > >
> > >  #define INT_ENABLE_MASK_WAITSTOP	(INT_SLAVE_EVENT      | \
> > >  					 INT_ADDR_ACK_ERR     | \
> > > @@ -907,6 +909,18 @@ static unsigned int img_i2c_auto(struct img_i2c
> > *i2c,
> > >  				return ISR_COMPLETE(0);
> > >  			}
> > >  		}
> > > +		if (int_status & INT_STOP_DETECTED) {
> > > +			int ret;
> > > +			/*
> > > +			 * Stop bit indicates the end of the transfer, it means
> > > +			 * we should read all the data (or drain the FIFO). We
> > > +			 * must signal completion for this transaction.
> > > +			 */
> > > +			img_i2c_transaction_halt(i2c, false);
> > 
> > to get a stop bit detected, wouldn't transaction halt already have to be off?
> 
> Yes, but in case we were too slow re-enabling the master halt and a stop was detected simultaneously to when master halt was set.

Okay, I already checked to make sure the t_halt thing is being done with
the spinlock held in both places, and irqs disabled in non-irq context,
since you wouldn't want a preemption or irq in between to delay t_halt
getting set anyway, in case it reached the stop bit without halting.

So you should be okay. On UP you'll never handle an irq in between as
irqs are disabled, and on SMP the spinlock will ensure the bulk of the
irq handler and t_halt thing in user context can't run concurrently on
different CPUs.

Thanks
James

> > 
> > > +			img_i2c_read_fifo(i2c);
> > > +			ret = (i2c->msg.len == 0) ? 0 : EIO;
> > 
> > If it wasn't fully transferred, wouldn't that already imply an
> > INT_WRITE_ACK_ERR or INT_ADDR_ACK_ERR, which should've already been
> > handled?
> > 
> This was added as a safety check, yes I guess it's impossible to get a stop bit before all data is transferred without an error condition.
> >
> > Could it then be as simple as this? (untested):
> > 
> Yes, this would do.
> 
> Thanks,
> Sifan
> > diff --git a/drivers/i2c/busses/i2c-img-scb.c b/drivers/i2c/busses/i2c-img-
> > scb.c
> > index 00ffd6613680..f694b47dcf74 100644
> > --- a/drivers/i2c/busses/i2c-img-scb.c
> > +++ b/drivers/i2c/busses/i2c-img-scb.c
> > @@ -867,6 +867,13 @@ static unsigned int img_i2c_auto(struct img_i2c *i2c,
> > 
> >  	mod_timer(&i2c->check_timer, jiffies + msecs_to_jiffies(1));
> > 
> > +	if (int_status & INT_STOP_DETECTED) {
> > +		/* Drain remaining data in FIFO and complete transaction */
> > +		if (i2c->msg.flags & I2C_M_RD)
> > +			img_i2c_read_fifo(i2c);
> > +		return ISR_COMPLETE(0);
> > +	}
> > +
> >  	if (i2c->msg.flags & I2C_M_RD) {
> >  		if (int_status & INT_FIFO_FULL_FILLING) {
> >  			img_i2c_read_fifo(i2c);
> > 
> > Cheers
> > James
> > 
> > > +			return ISR_COMPLETE(ret);
> > > +		}
> > >  	} else {
> > >  		if (int_status & INT_FIFO_EMPTY) {
> > >  			if (i2c->msg.len == 0) {
> > > @@ -916,6 +930,18 @@ static unsigned int img_i2c_auto(struct img_i2c
> > *i2c,
> > >  			}
> > >  			img_i2c_write_fifo(i2c);
> > >  		}
> > > +		if (int_status & INT_STOP_DETECTED) {
> > > +			int ret;
> > > +
> > > +			img_i2c_transaction_halt(i2c, false);
> > > +			/*
> > > +			 * Stop bit indicates the end of a transfer and if the
> > > +			 * transfer has ended before all data is written to the
> > > +			 * fifo, return an error with transfer complete signal.
> > > +			 */
> > > +			ret = (i2c->msg.len == 0) ? 0 : EIO;
> > > +			return ISR_COMPLETE(ret);
> > > +		}
> > >  	}
> > >
> > >  	return 0;
> > >
> 

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