This is the mail archive of the ecos-patches@sourceware.org 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


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.


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