This is the mail archive of the mailing list for the eCos project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

[Bug 1000763] I2C Driver for at91sam7x

vibi <> changed:

           What    |Removed                     |Added
                 CC|                            |

--- Comment #4 from vibi <>  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:
> 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 &

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

Configure bugmail:
------- You are receiving this mail because: -------
You are the QA contact for the bug.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]