This is the mail archive of the
ecos-patches@sourceware.org
mailing list for the eCos project.
[Bug 1000763] I2C Driver for at91sam7x
- From: bugzilla-daemon at ecoscentric dot com
- To: ecos-patches at ecos dot sourceware dot org
- Date: Fri, 15 May 2009 20:01:37 +0100
- Subject: [Bug 1000763] I2C Driver for at91sam7x
- References: <bug-1000763-104@http.bugs.ecos.sourceware.org/>
http://bugs.ecos.sourceware.org/show_bug.cgi?id=1000763
Andrew Lunn <andrew.lunn@ascom.ch> changed:
What |Removed |Added
----------------------------------------------------------------------------
CC| |andrew.lunn@ascom.ch
--- Comment #2 from Andrew Lunn <andrew.lunn@ascom.ch> 2009-05-15 20:01:35 ---
Hi Vibi
It is good to see you don't attempt to implement an interrupt driver I2C driver
on the AT91 platforms. That is know not to work.
The copyright headers need updating to the latest version.
In i2c_at91sam7x_init() you have:
HAL_ARM_AT91_GPIO_CFG_PULLUP(AT91_GPIO_PA10,AT91_PIN_PULLUP_DISABLE);
HAL_ARM_AT91_GPIO_CFG_PULLUP(AT91_GPIO_PA11,AT91_PIN_PULLUP_DISABLE);
You should not have hard coded references to PA10 and PA11. These will not be
valid for all AT91SAM devices.
volatile cyg_uint32 stat_reg;
volatile should not be needed. The macro HAL_READ_UINT32() ensures the value
will be read from the register every time.
Could you explain this in more detail:
// calculate internal address
while (i--)
tmp |= *tx_data++ << (i << 3);
I've no idea what it is doing.
The following is not the most readable code:
for ( timeout = TIMEOUT,stat_reg = 0;
timeout && !(stat_reg & tmp); timeout-- )
I2C_R32 (AT91_TWI_SR,stat_reg);
// error condition , return how much data was transferred
if (!timeout)
return (count - i2c_count);
Maybe tmp should be called sr_mask?
timeout is not a timeout, it more of a count down. This makes it easy to
misread
if (!timeout). Most people would think !timeout is being the good case, when in
fact it is an error! I would prefer to see this section of code re-written to
make it easier to understand.
Consider:
} while (--i2c_count);
return (count - i2c_count);
}
There is no break in the do {} while loop. So if you got out of the loop it
means i2c_count must be 0. So that makes the subtraction for the return value
does nothing.
There are some indentation issues it would also be nice to clean up, but lets
get the other problems addressed first.
Some AT91 device have more than one TWI bus, eg the AT91SAM9X devices. Maybe
you could think how to handle this? However it is not too important.
Andrew
--
Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.