http://bugs.ecos.sourceware.org/show_bug.cgi?id=1000763
vibi <[email protected]> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |[email protected] --- Comment #4 from vibi <[email protected]> 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.
