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

[Heiko Schocher]

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

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
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany