[i2c] [PATCH] AT91 bus driver loses data

[Ronny Nilsson]

Hi
Here's an update of my AT91 patch with interrupt support. Quite some 
changes has been made. It's both smaller and cleaner now I think. 
Please also read my previous reply to Marks comments:
http://lists.lm-sensors.org/pipermail/i2c/2007-April/001030.html


> > > Yeah, and I don't think this is a good tradeoff.  Please just use
> > > one ISR.
> >
> > Ok, changed. A small common ISR now calls either of a rx/tx
> > handler. The compiler optimises it to one single large ISR though.

It's even smaller now. Duplicate code has been removed.


> > +#include <asm/semaphore.h>
>
> There is a mutex primitive now (for some time actually).
> Use <linux/mutex.h> instead.

Fixed.


> > -	return 0;
> > +	if (unlikely(status & AT91_TWI_TXCOMP)) {
> > +		twi_transfer.status |= status;
>
> If I understand you... the OR here is to propogate a possible error
> status from an earlier run through the ISR.  IMHO it would be cleaner
> to clear IER (mask all interrupts) and just poll the SR to completion
> in case of an error. Then you could do something like this...
>
> > +		at91_twi_write(AT91_TWI_IDR, 0xffffffff);
> > +		twi_transfer.isFinished = 1;
> > +		wake_up(&twi_transfer.sleepQ);
>
> 		xchg(&twi_transfer.status, status);
> 		wake_up_interruptible(&twi_transfer.sleepq);
>
> ... getting rid of isFinished altogether.
>

Fixed.


> Now you can handle the status below.  But you also need to watch the
> return value of wait_event_interruptible_timeout().  It will be < 0
> if it was cut short by a signal and 0 in case of a timeout.  Your
> second test below will therefore do xfer_abort() for a second time.

Fixed.


> >
> > +		at91_twi_hwinit();		/* Reset every transfer */
>
> This seems wasteful.  Whatever part of that function that needs to
> happen here, factor it out into a separate function.  E.g. I can't
> see any need to reinit the clock every transfer.

Fixed.


> Well, I think this code needs a lot of work yet; so my review was not
> 100% thorough.  Let's get the major stuff out of the way and then
> I'll start looking for more spelling errors, etc. ;)

Thanks for your help and suggestions.
Regards
/Ronny Nilsson


-------------- next part --------------
A non-text attachment was scrubbed...
Name: linux-2.6.21-rc7-at91-i2c-irq-support.patch
Type: text/x-diff
Size: 16052 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/i2c/attachments/20070418/3083d22d/attachment-0001.bin