RE: [PATCH v7 0/5] AT91: replace broken TWI driver i2c-at91.c
Hi, Carsten Behling wrote on 2011-12-28: I've tested this driver with the pca953x GPIO expander driver with a PCA9554. In the case of 8 GPIO pins (my case) i2c_smbus_read_byte_data(...) [...] I observed that reading out the GPIO status is one read delayed. The first read to a register from the pca953x driver does not result in a RXRDY interrupt and at91_twi_read_next_byte(...) is not called. Only the completion routine is triggered with a TXCOMP interrupt. Which SOC are you using? This is probably a hardware bug since on my at91sam9g45 i2c_smbus_read_byte_data() works reliably without any problems. Please check the errata and see if there is a useable workaround. If not, the driver should be disabled for your SOC. Additionally, I took a look at the status flags just after the control flags are set (in at91_do_twi_transfer(...), AT91_TWI_START|AT91_TWI_STOP for the one byte read). Surprisingly, the AT91_TWI_RXRDY flag is zero before the first register read and one for the following reads. According to the manual this flag should be zero after a read on AT91_TWI_RHR. Reading the AT91_TWI_RHR before the control flags are set to be sure that the AT91_TWI_RXRDY is cleared leads to a never occurring RXRDY interrupt. Again, this behavior does not conform to the datasheet, I suspect documented or undocumented errata... At91rm9200 has at least one erratum for which I imported a workaround from the old driver (which does not use interrupts). Niko -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
AW: [PATCH v7 0/5] AT91: replace broken TWI driver i2c-at91.c
Hi, I've tested this driver with the pca953x GPIO expander driver with a PCA9554. In the case of 8 GPIO pins (my case) i2c_smbus_read_byte_data(...) is called to read the registers of the GPIO expander. This results in a at91_twi_xfer with two messages. The first message is to put the register address into the IADR and the second is to read the one byte content of the register. At the end we have a one byte read with a repeated start condition. I observed that reading out the GPIO status is one read delayed. The first read to a register from the pca953x driver does not result in a RXRDY interrupt and at91_twi_read_next_byte(...) is not called. Only the completion routine is triggered with a TXCOMP interrupt. Additionally, I took a look at the status flags just after the control flags are set (in at91_do_twi_transfer(...), AT91_TWI_START|AT91_TWI_STOP for the one byte read). Surprisingly, the AT91_TWI_RXRDY flag is zero before the first register read and one for the following reads. According to the manual this flag should be zero after a read on AT91_TWI_RHR. Reading the AT91_TWI_RHR before the control flags are set to be sure that the AT91_TWI_RXRDY is cleared leads to a never occurring RXRDY interrupt. This looks very strange to me. Can anyone help? 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: Hubert Feurstein [mailto:h.feurst...@gmail.com] Gesendet: Freitag, 25. November 2011 16:42 An: Nikolaus Voss Cc: linux-i2c@vger.kernel.org; linux-arm-ker...@lists.infradead.org; linux-ker...@vger.kernel.org; ben-li...@fluff.org; Carsten Behling Betreff: Re: [PATCH v7 0/5] AT91: replace broken TWI driver i2c-at91.c Hi, I've tested this driver on a 2.6.38 kernel with several i2c clients (temp-sensor, audio-codec, touchscreen-controller, w1-bridge, io-expanders) and works without problems. SoC: at91sam9g45 Because of the 2.6.38 kernel, I had to skip [PATCH v7 2/5] Replace clk_lookup.con_id with clk_lookup.dev_id entries for twi clk and used instead: at91_clock_associate(twi0_clk, at91sam9g45_twi0_device.dev, twi_clk); Best Regards Hubert On 2011-11-23 16:35, Nikolaus Voss wrote: The old driver has two main deficencies: i) No repeated start (Sr) condiction is possible, this makes it unusable e.g. for most SMBus transfers. ii) I/O was done with polling/busy waiting what caused over-/underruns even at light system loads and clock speeds. The new driver overcomes these deficencies and in addition allows for more than one TWI interface. Tested-by: Hubert Feurstein h.feurst...@gmail.com -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 0/5] AT91: replace broken TWI driver i2c-at91.c
Hi, I've tested this driver on a 2.6.38 kernel with several i2c clients (temp-sensor, audio-codec, touchscreen-controller, w1-bridge, io-expanders) and works without problems. SoC: at91sam9g45 Because of the 2.6.38 kernel, I had to skip [PATCH v7 2/5] Replace clk_lookup.con_id with clk_lookup.dev_id entries for twi clk and used instead: at91_clock_associate(twi0_clk, at91sam9g45_twi0_device.dev, twi_clk); Best Regards Hubert On 2011-11-23 16:35, Nikolaus Voss wrote: The old driver has two main deficencies: i) No repeated start (Sr) condiction is possible, this makes it unusable e.g. for most SMBus transfers. ii) I/O was done with polling/busy waiting what caused over-/underruns even at light system loads and clock speeds. The new driver overcomes these deficencies and in addition allows for more than one TWI interface. Tested-by: Hubert Feurstein h.feurst...@gmail.com -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 0/5] AT91: replace broken TWI driver i2c-at91.c
On Wed, Nov 23, 2011 at 04:35:55PM +0100, Nikolaus Voss wrote: The old driver has two main deficencies: i) No repeated start (Sr) condiction is possible, this makes it unusable e.g. for most SMBus transfers. ii) I/O was done with polling/busy waiting what caused over-/underruns even at light system loads and clock speeds. The new driver overcomes these deficencies and in addition allows for more than one TWI interface. A remaining limitation is the fact, that only one repeated start is possible (two concatenated messages). This limitation is imposed by the hardware. However, this should not be a problem as all common i2c-client communication does not rely on more than one repeated start. v7: Patch 4/5: i) fix bug if internal address 1 byte ii) send stop when len == 1 (both reported by Carsten Behling) v6: Patch 5/5: support for I2C_SMBUS_BLOCK_DATA transfers. Better use of clk_(un)prepare(). More sensible transfer timeout. v5: Another round of review comments from Ryan Mallon, Felipe Balbi and Russell King: convert twi clk to use .dev_id, cleanups v4: Integrated more review comments from Ryan Mallon and Felipe Balbi: Moved register include file to local include, code cleanups v3: Integrated review comments from Ryan Mallon and Felipe Balbi v2: Fixed whitespace issue Nikolaus Voss (5): drivers/i2c/busses/i2c-at91.c: remove broken driver Replace clk_lookup.con_id with clk_lookup.dev_id entries for twi clk drivers/i2c/busses/i2c-at91.c: add new driver G45 TWI: remove open drain setting for twi function gpios i2c-at91.c: add SMBUS_READ_BLOCK_DATA functionality Is the original driver so broken that the two could not co-exist, or are we making so many changes that there's no point in keeping the original one? arch/arm/mach-at91/at91cap9.c |1 + arch/arm/mach-at91/at91rm9200.c|1 + arch/arm/mach-at91/at91sam9260.c |1 + arch/arm/mach-at91/at91sam9261.c |1 + arch/arm/mach-at91/at91sam9263.c |1 + arch/arm/mach-at91/at91sam9g45.c |2 + arch/arm/mach-at91/at91sam9g45_devices.c |6 - arch/arm/mach-at91/at91sam9rl.c|2 + arch/arm/mach-at91/include/mach/at91_twi.h | 68 drivers/i2c/busses/Kconfig | 11 +- drivers/i2c/busses/i2c-at91.c | 529 +--- drivers/i2c/busses/i2c-at91.h | 80 + 12 files changed, 408 insertions(+), 295 deletions(-) delete mode 100644 arch/arm/mach-at91/include/mach/at91_twi.h create mode 100644 drivers/i2c/busses/i2c-at91.h -- 1.7.5.4 -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- Ben Dooks, b...@fluff.org, http://www.fluff.org/ben/ Large Hadron Colada: A large Pina Colada that makes the universe disappear. -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v7 0/5] AT91: replace broken TWI driver i2c-at91.c
Hi, Ben Dooks wrote on 2011-11-24: On Wed, Nov 23, 2011 at 04:35:55PM +0100, Nikolaus Voss wrote: The old driver has two main deficencies: i) No repeated start (Sr) condiction is possible, this makes it unusable e.g. for most SMBus transfers. ii) I/O was done with polling/busy waiting what caused over-/underruns even at light system loads and clock speeds. The new driver overcomes these deficencies and in addition allows for more than one TWI interface. A remaining limitation is the fact, that only one repeated start is possible (two concatenated messages). This limitation is imposed by the hardware. However, this should not be a problem as all common i2c-client communication does not rely on more than one repeated start. v7: Patch 4/5: i) fix bug if internal address 1 byte ii) send stop when len == 1 (both reported by Carsten Behling) v6: Patch 5/5: support for I2C_SMBUS_BLOCK_DATA transfers. Better use of clk_(un)prepare(). More sensible transfer timeout. v5: Another round of review comments from Ryan Mallon, Felipe Balbi and Russell King: convert twi clk to use .dev_id, cleanups v4: Integrated more review comments from Ryan Mallon and Felipe Balbi: Moved register include file to local include, code cleanups v3: Integrated review comments from Ryan Mallon and Felipe Balbi v2: Fixed whitespace issue Nikolaus Voss (5): drivers/i2c/busses/i2c-at91.c: remove broken driver Replace clk_lookup.con_id with clk_lookup.dev_id entries for twi clk drivers/i2c/busses/i2c-at91.c: add new driver G45 TWI: remove open drain setting for twi function gpios i2c-at91.c: add SMBUS_READ_BLOCK_DATA functionality Is the original driver so broken that the two could not co-exist, or are we making so many changes that there's no point in keeping the original one? The old driver was marked as broken for the above reasons and I can hardly imagine any setup in which it would be preferable to i2c-gpio. So it does not make any sense to keep the old driver alive. Though inspired by the old driver, the new one is almost a rewrite from scratch, so for better reviewing, I removed the old instead of doing a diff. Niko -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
