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: Sat, 16 May 2009 11:56:54 +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
vibi <vibi_sreenivasan@cms.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
CC| |vibi_sreenivasan@cms.com
--- Comment #4 from vibi <vibi_sreenivasan@cms.com> 2009-05-16 11:56:52 ---
Hi Andrew,
Thanks alot for looking in to my code.
> 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.
Added in the latest version send as attatchment
>
> 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.
Replaced AT91_GPIO_PA10 & AT91_GPIO_PA11 with AT91_PIO_PSR_TWD &
AT91_PIO_PSR_TWCK
>
> 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.
>
removed volatile
> 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.
Sorry I didnt add a README for this.
AT91SAM7X has a facility to access locations inside I2C devices like EEPROM,
FRAM etc.
If someone wants to use that facility they can use it by setting
"int_addr_sz" variable in "cyg_i2c_at91sam7x_dev_t" to the number of internal
address bytes.
It can be disabled by setting it to zero.
If it is set , driver will expect first "int_addr_sz" bytes of regular I2C API
to be internal address.
here "tmp" will contain internal address.
>
> 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.
Changed to
// Wait for txr completion flag || until COUNT_DOWN ends
for ( success = COUNT_DOWN,stat_reg = 0;
success && !(stat_reg & sr_mask); success-- )
I2C_R32 (AT91_TWI_SR,stat_reg);
// error condition ,ie count down ended before txr complete flag is
//set.return how much data was transferred
if (!success)
break;
Is this more clear.
Shall i break down the for loop?
>
> 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.
>
now there is a break
> 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
>
Thanks Again for your comments & spending your valuable time.
THanks & Regards
vibi
--
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.