Re: [PATCH v10 3/4] drivers/i2c/busses/i2c-at91.c: add new driver

Re: [PATCH v10 3/4] drivers/i2c/busses/i2c-at91.c: add new driver

[Nikolaus Voss]

Hi Carsten,

On Wed, 25 Apr 2012, Carsten Behling wrote:
> I cannot find the fix for the RXRDY overrun bug in this patch:

The fix refers to the bug when RXRDY flag was masked out by TXCOMP,
reported by Hubert. I hope this is fixed with v10.

[...]
> here you only report the status flags after a complete transfer, but 
> an overrun can happen after every byte. After reading the status 
> register after an overrun for the next byte this info will be lost !

Right, I didn't consider this. Would you mind to test if the attached 
patch on top of v10 fixes your problem?

Thanks,
Niko

P.S. Are you using a RM9200? Seems that this SOC has some problems...

diff --git a/drivers/i2c/busses/i2c-at91.c b/drivers/i2c/busses/i2c-at91.c
index 97f4365..df0f6ed 100644
--- a/drivers/i2c/busses/i2c-at91.c
+++ b/drivers/i2c/busses/i2c-at91.c
@@ -186,9 +186,11 @@ static irqreturn_t atmel_twi_interrupt(int irq, void *dev_id)
  	else if (irqstatus & AT91_TWI_TXRDY)
  		at91_twi_write_next_byte(dev);

+	/* catch error flags */
+	dev->transfer_status |= status;
+
  	if (irqstatus & AT91_TWI_TXCOMP) {
  		at91_disable_twi_interrupts(dev);
-		dev->transfer_status = status;
  		complete(&dev->cmd_complete);
  	}

@@ -203,6 +205,7 @@ static int at91_do_twi_transfer(struct at91_twi_dev *dev)
  		(dev->msg->flags & I2C_M_RD) ? "read" : "write", dev->buf_len);

  	INIT_COMPLETION(dev->cmd_complete);
+	dev->transfer_status = 0;
  	if (dev->msg->flags & I2C_M_RD) {
  		unsigned start_flags = AT91_TWI_START;

RE: [PATCH v10 3/4] drivers/i2c/busses/i2c-at91.c: add new driver

[Voss, Nikolaus]

Hi Carsten,

Carsten Behling wrote on 2012-04-27:
>>  	INIT_COMPLETION(dev->cmd_complete);
>> +	dev->transfer_status = 0;
>>  	if (dev->msg->flags & I2C_M_RD) {
>>  		unsigned start_flags = AT91_TWI_START;
> 
> this patch will not work, because you reset 'dev->transfer_status'
> before it is evaluated on errors (AT91_TWI_NACK, AT91_TWI_OVRE). 

it should, because transfer_status is updated by the ISR between
wait_for_completion_interruptible_timeout() and the error evaluation.
Please try it out.

> > P.S. Are you using a RM9200? Seems that this SOC has some problems...
> 
> Yes, our ECO920 uses the RM9200, but we support it with an very old kernel
> (2.6.21). I remember there were many problems with I2C.

Ok, but good to hear the driver basically works with the RM9200.

Niko

AW: [PATCH v10 3/4] drivers/i2c/busses/i2c-at91.c: add new driver

[Carsten Behling]

Hi Niko,

you a right. Driver works with that patch.

Mit freundlichen Grüßen / Best regards
Carsten Behling

Development Engineer
Garz & Fricke GmbH
Tempowerkring 2, 21079 Hamburg - Germany
Amtsgericht Hamburg HRB 60514
Geschäftsführer: Manfred Garz, Matthias Fricke
Phone: +49 (0) 40 791 899 - 56
Fax:    +49 40 / 791 899 - 39
www.garz-fricke.com 

 


-----Ursprüngliche Nachricht-----
Von: Voss, Nikolaus [mailto:N.Voss@weinmann.de] 
Gesendet: Freitag, 27. April 2012 10:05
An: Carsten Behling
Cc: Marc-Oliver Westerburg; 'devel@ayanes.com'; 'Wolfram Sang'; 'linux-arm-kernel@lists.infradead.org'; 'linux-kernel@vger.kernel.org'; 'linux-i2c@vger.kernel.org'; 'Hubert Feurstein'
Betreff: RE: [PATCH v10 3/4] drivers/i2c/busses/i2c-at91.c: add new driver

Hi Carsten,

Carsten Behling wrote on 2012-04-27:
>>  	INIT_COMPLETION(dev->cmd_complete);
>> +	dev->transfer_status = 0;
>>  	if (dev->msg->flags & I2C_M_RD) {
>>  		unsigned start_flags = AT91_TWI_START;
> 
> this patch will not work, because you reset 'dev->transfer_status'
> before it is evaluated on errors (AT91_TWI_NACK, AT91_TWI_OVRE). 

it should, because transfer_status is updated by the ISR between
wait_for_completion_interruptible_timeout() and the error evaluation.
Please try it out.

> > P.S. Are you using a RM9200? Seems that this SOC has some problems...
> 
> Yes, our ECO920 uses the RM9200, but we support it with an very old kernel
> (2.6.21). I remember there were many problems with I2C.

Ok, but good to hear the driver basically works with the RM9200.

Niko

Re: AW: [PATCH v10 3/4] drivers/i2c/busses/i2c-at91.c: add new driver

[Adrian Yanes]

>> Yes, our ECO920 uses the RM9200, but we support it with an very old kernel
>> (2.6.21). I remember there were many problems with I2C.

I really doubt this, as I indicated in a previous email[1], the RM9200
has a hardware "bug". So either you performed the testing with less than
2 bytes of data and in a really low frequency (<50kHz) or we are missing
something.

Can you provide more details about which kind of testing you performed?

Thanks

1 - http://article.gmane.org/gmane.linux.kernel/1287445/match=