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.jo...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
