[U-Boot] [PATCH v2 6/7] ARMV7: OMAP: I2C driver: Write more than 1 byte at a time in i2c_write

[Michael Jones]

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 at 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

> 
>> +	writew(alen+len, &i2c_base->cnt);
> 
> please change to "alen + len"

OK. I thought checkpatch.pl would've found that.

> 
>> +	/* 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.

[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?

-Michael

MATRIX VISION GmbH, Talstrasse 16, DE-71570 Oppenweiler
Registergericht: Amtsgericht Stuttgart, HRB 271090
Geschaeftsfuehrer: Gerhard Thullner, Werner Armingeon, Uwe Furtner, Erhard Meier