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

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

[Felipe Balbi]

On Tue, Nov 08, 2011 at 11:49:46AM +0100, Nikolaus Voss wrote:
> This driver has the following properties compared to the old driver:
> 1. Support for multiple interfaces.
> 2. Interrupt driven I/O as opposed to polling/busy waiting.
> 3. Support for _one_ repeated start (Sr) condition, which is enough
>    for most real-world applications including all SMBus transfer types.
>    (The hardware does not support issuing arbitrary Sr conditions on the
>     bus.)
> 
> Tested on Atmel G45 with BQ20Z80 battery SMBus client.
> 
> Signed-off-by: Nikolaus Voss <n.voss@weinmann.de>

FWIW:

Reviewed-by: Felipe Balbi <balbi@ti.com>

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20111110/fdf1b885/attachment.sig>

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

[Voss, Nikolaus]

Hi,

Carsten Behling wrote on 2011-11-22:
> +static void at91_twi_read_next_byte(struct at91_twi_dev *dev)
> +{
> +       *dev->buf = at91_twi_read(dev, AT91_TWI_RHR) & 0xff;
> +
> +       /* send stop if second but last byte has been read */
> +       if (--dev->buf_len == 1)
> +               at91_twi_write(dev, AT91_TWI_CR, AT91_TWI_STOP);
> 
> 
> 
> If dev->buf_len =1 at the beginning of a read transfer, a stop condition will
> never be send.

this case is already catched in at91_do_twi_transfer():

+       if (dev->msg->flags & I2C_M_RD) {
+               unsigned start_flags = AT91_TWI_START;
+
+               /* if only one byte is to be read, immediately stop transfer */
+               if (dev->buf_len <= 1 && !(dev->msg->flags & I2C_M_RECV_LEN))
+                       start_flags |= AT91_TWI_STOP;
+               at91_twi_write(dev, AT91_TWI_CR, start_flags);

Thanks for reviewing,
Niko

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

[Carsten Behling]

> this case is already catched in at91_do_twi_transfer():

Sorry, I did not found this code in your patch.
(http://www.mail-archive.com/linux-i2c at vger.kernel.org/msg06556.html):

> +       if (is_read) {
> +               if (!dev->buf_len)
> +                       at91_twi_write(dev, AT91_TWI_CR,
> +                                      AT91_TWI_START | AT91_TWI_STOP);
> +               else
> +                       at91_twi_write(dev, AT91_TWI_CR, AT91_TWI_START);
> +               at91_twi_write(dev, AT91_TWI_IER,
> +                              AT91_TWI_TXCOMP | AT91_TWI_RXRDY);
> +       } else {
> +               at91_twi_write_next_byte(dev);
> +               at91_twi_write(dev, AT91_TWI_IER,
> +                              AT91_TWI_TXCOMP | AT91_TWI_TXRDY);
> +       }

Is there a more recent version ?

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 at weinmann.de] 
Gesendet: Dienstag, 22. November 2011 17:26
An: Carsten Behling
Cc: 'linux-i2c at vger.kernel.org'; 'linux-arm-kernel at lists.infradead.org'; 'linux-kernel at vger.kernel.org'
Betreff: RE: [PATCH v5 3/4] drivers/i2c/busses/i2c-at91.c: add new driver

Hi,

Carsten Behling wrote on 2011-11-22:
> +static void at91_twi_read_next_byte(struct at91_twi_dev *dev)
> +{
> +       *dev->buf = at91_twi_read(dev, AT91_TWI_RHR) & 0xff;
> +
> +       /* send stop if second but last byte has been read */
> +       if (--dev->buf_len == 1)
> +               at91_twi_write(dev, AT91_TWI_CR, AT91_TWI_STOP);
> 
> 
> 
> If dev->buf_len =1 at the beginning of a read transfer, a stop condition will
> never be send.

this case is already catched in at91_do_twi_transfer():

+       if (dev->msg->flags & I2C_M_RD) {
+               unsigned start_flags = AT91_TWI_START;
+
+               /* if only one byte is to be read, immediately stop transfer */
+               if (dev->buf_len <= 1 && !(dev->msg->flags & I2C_M_RECV_LEN))
+                       start_flags |= AT91_TWI_STOP;
+               at91_twi_write(dev, AT91_TWI_CR, start_flags);

Thanks for reviewing,
Niko

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

[Voss, Nikolaus]

Hi,

Carsten Behling wrote on 2011-11-23:

>> this case is already catched in at91_do_twi_transfer():
> 
> Sorry, I did not found this code in your patch.
> (http://www.mail-archive.com/linux-i2c at vger.kernel.org/msg06556.html):
> 
>> +       if (is_read) {
>> +               if (!dev->buf_len)

yes, this won't work for buf_len == 1. It is corrected in
https://lkml.org/lkml/2011/11/18/223 which I held back for some time
as it should have been just a feature extension. I was not aware it
also fixed the buf_len = 1 bug. Sorry for that...

(Explanation: in the first implementation I immediately decremented
buf_len, so buf_len == 1 could not occur. Later I removed that but
did not fully fold it into the base patch.)

Thanks,
Niko

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

[Carsten Behling]

Hi,

I try to use the at24 eeprom driver on top of this driver.

This EEPROM (24c32) works with two address bytes.

Writing results in a call to at91_twi_xfer() with num=1.
In this case the internal address register is not used and the address is sent out within the buffer.

Reading results in a call to at91_twi_xfer() with num=2.
In this case the internal address register is used.

However the MSB of the internal address resides in msg->buf[0] and the LSB resides in msg->buf[1] of the first message.

As a result the code:

+		for (i = 0; i < msg->len; ++i) {
+			internal_address |= ((unsigned)msg->buf[i]) << (8 * i);
+			int_addr_flag += AT91_TWI_IADRSZ_1;
+		}
+		at91_twi_write(dev, AT91_TWI_IADR, internal_address);

constructs an internal address in a wrong byte order.

Example: Try to read from address 0x100:

msg[0]->buf[0] = 0x1; 
msg[0]->buf[1] = 0x0;

results in

internal_address = 0x1;

I think it must be:

+		for (i = 0; i < msg->len; ++i) {
+			internal_address |= ((unsigned)msg->buf[msg->len-1-i]) << (8 * i);
+			int_addr_flag += AT91_TWI_IADRSZ_1;
+		}
+		at91_twi_write(dev, AT91_TWI_IADR, internal_address);

... or the at24 eeprom driver constructs the wrong internal address ...

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 at weinmann.de] 
Gesendet: Mittwoch, 23. November 2011 11:29
An: Carsten Behling
Cc: 'linux-i2c at vger.kernel.org'; 'linux-arm-kernel at lists.infradead.org'; 'linux-kernel@vger.kernel.org'
Betreff: RE: [PATCH v5 3/4] drivers/i2c/busses/i2c-at91.c: add new driver

Hi,

Carsten Behling wrote on 2011-11-23:

>> this case is already catched in at91_do_twi_transfer():
> 
> Sorry, I did not found this code in your patch.
> (http://www.mail-archive.com/linux-i2c at vger.kernel.org/msg06556.html):
> 
>> +       if (is_read) {
>> +               if (!dev->buf_len)

yes, this won't work for buf_len == 1. It is corrected in
https://lkml.org/lkml/2011/11/18/223 which I held back for some time
as it should have been just a feature extension. I was not aware it
also fixed the buf_len = 1 bug. Sorry for that...

(Explanation: in the first implementation I immediately decremented
buf_len, so buf_len == 1 could not occur. Later I removed that but
did not fully fold it into the base patch.)

Thanks,
Niko

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

[Voss, Nikolaus]

Hi Carsten,

Carsten Behling wrote on 2011-11-23:
> I think it must be:
> 
> +		for (i = 0; i < msg->len; ++i) {
> +			internal_address |= ((unsigned)msg->buf[msg->len-1-i]) << (8 * i);
> +			int_addr_flag += AT91_TWI_IADRSZ_1;
> +		}
> +		at91_twi_write(dev, AT91_TWI_IADR, internal_address);

yes, I think you're right. I tested only with 8 bit addresses
(msg->len = 1), so I haven't seen this error.

I will modify the patch and post it later today.

Thanks for this feedback,
Niko