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


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.


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