Hello Michael,
Michael Jones wrote:
Hi Heiko,
Thanks for the review.
On 07/27/2011 08:07 AM, Heiko Schocher wrote:
Hello Michael,
Sorry for the long delay...
Michael Jones wrote:
This allows the EEPROM layer to send a single i2c write command per page, and wait CONFIG_SYS_EEPROM_PAGE_WRITE_DELAY_MS between i2c write commands.
Signed-off-by: Michael Jones michael.jones@matrix-vision.de
Changes for v2:
- None. Resubmitting to include custodian in cc:
drivers/i2c/omap24xx_i2c.c | 134 ++++++++++++++++++------------------------- 1 files changed, 56 insertions(+), 78 deletions(-)
diff --git a/drivers/i2c/omap24xx_i2c.c b/drivers/i2c/omap24xx_i2c.c index 966ffc4..4ae03bc 100644 --- a/drivers/i2c/omap24xx_i2c.c +++ b/drivers/i2c/omap24xx_i2c.c
[...]
@@ -372,26 +301,75 @@ int i2c_read (uchar chip, uint addr, int alen, uchar * buffer, int len) int i2c_write (uchar chip, uint addr, int alen, uchar * buffer, int len) { int i;
u16 status;
int i2c_error = 0;
if (alen > 1) {
printf ("I2C read: addr len %d not supported\n", alen);
printf("I2C write: addr len %d not supported\n", alen);return 1; }
if (addr + len > 256) {
printf ("I2C read: address out of range\n");
printf("I2C write: address 0x%x + 0x%x out of range\n");return 1; }
/* wait until bus not busy */
wait_for_bb();
/* start address phase - will write regoffset + len bytes data */
/* TODO consider case when !CONFIG_OMAP243X/34XX/44XX */
Do we have this usecase?
I don't know, I assumed so, as there is the following #ifdef in the part I removed:
#if defined(CONFIG_OMAP243X) || defined(CONFIG_OMAP34XX) || \ defined(CONFIG_OMAP44XX) #else /* there is code here that I didn't consider when replacing it. */ #endif
Hmm.. I have to look at this deeper, but your patch shouldn;t break any existing board...
- writew(alen+len, &i2c_base->cnt);
please change to "alen + len"
OK. I thought checkpatch.pl would've found that.
Yes, I also checked your patch with checkpatch, but it didn;t found this ...
- /* set slave address */
- writew(chip, &i2c_base->sa);
- /* stop bit needed here */
- writew(I2C_CON_EN | I2C_CON_MST | I2C_CON_STT | I2C_CON_TRX |
I2C_CON_STP, &i2c_base->con);- /* Send address byte */
- status = wait_for_pin();
- if (status == 0 || status & I2C_STAT_NACK) {
i2c_error = 1;printf("%s:%d error status=0x%x\n", __func__, __LINE__, status);Can you change this printf to output some more info, instead __func__, __LINE__?
OK, I will make these more informative. Do you not want __func__ to be in the output? I originally put __LINE__ in as well because the strings were otherwise identical, so I'm fine with getting rid of that once the messages are unique.
Thanks! I think, we don;t need __func__ and __LINE__, if you make informative printfs ...
[snip]
bye, Heiko
Question about cosmetics: the README says "In sources originating from U-Boot a style corresponding to "Lindent -pcs" (adding spaces before parameters to function calls) is actually used." Currently this is not uniform in this file, because checkpatch.pl doesn't like the spaces between function names and '(' (and neither do I). Are there supposed to be such spaces in U-Boot code? Or can we uniformly remove them in this file?
We should get this "checkpatch compatible". If you do such a cosmetic change, please split this in a seperate patch, thanks!
bye, Heiko