[Bug 1000763] New: I2C Driver for at91sam7x

[Bug 1000763] New: I2C Driver for at91sam7x

[bugzilla-daemon]

http://bugs.ecos.sourceware.org/show_bug.cgi?id=1000763

           Summary: I2C Driver for at91sam7x
           Product: eCos
           Version: CVS
          Platform: at91sam7xek_256 (AT91SAM7X-EK board with AT91SAM7X256)
        OS/Version: ARM
            Status: UNCONFIRMED
          Severity: normal
          Priority: normal
         Component: Patches and contributions
        AssignedTo: jifl@ecoscentric.com
        ReportedBy: vibi_sreenivasan@cms.com
         QAContact: ecos-patches@ecos.sourceware.org
             Class: ---


Attached with this is a .bz2 compressed folder which includes I2C driver for
at91sam7x architecture.
tested on at91sam7x256ek


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

[Bug 1000763] I2C Driver for at91sam7x

[bugzilla-daemon]

http://bugs.ecos.sourceware.org/show_bug.cgi?id=1000763





--- Comment #1 from vibi <vibi_sreenivasan@cms.com>  2009-05-13 08:33:53 ---
Created an attachment (id=717)
 --> (http://bugs.ecos.sourceware.org/attachment.cgi?id=717)
I2C Driver for at91sam7x


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

[Bug 1000763] I2C Driver for at91sam7x

[bugzilla-daemon]

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.

[Bug 1000763] I2C Driver for at91sam7x

[bugzilla-daemon]

http://bugs.ecos.sourceware.org/show_bug.cgi?id=1000763





--- Comment #3 from vibi <vibi_sreenivasan@cms.com>  2009-05-16 11:42:27 ---
Created an attachment (id=725)
 --> (http://bugs.ecos.sourceware.org/attachment.cgi?id=725)
Second version of I2C Driver for at91sam7x


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

[Bug 1000763] I2C Driver for at91sam7x

[bugzilla-daemon]

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.

[Bug 1000763] I2C Driver for at91sam7x

[bugzilla-daemon]

http://bugs.ecos.sourceware.org/show_bug.cgi?id=1000763





--- Comment #5 from vibi <vibi_sreenivasan@cms.com>  2009-05-19 07:41:17 ---
hi,
   Any more comments?

Looking forwward for reply.

Thanks & Regards
vibi sreenivasan


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

[Bug 1000763] I2C Driver for at91sam7x

[bugzilla-daemon]

http://bugs.ecos.sourceware.org/show_bug.cgi?id=1000763


Andrew Lunn <andrew.lunn@ascom.ch> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         AssignedTo|jifl@ecoscentric.com        |andrew.lunn@ascom.ch




--- Comment #6 from Andrew Lunn <andrew.lunn@ascom.ch>  2009-05-19 08:20:16 ---
We don't seem to have a copyright assignment on file for you. Do you have one?

If not, please take a look at the first part of:

http://ecos.sourceware.org/assign.html

Without a copyright assignment the maintainers cannot commit the driver to
anoncvs.


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

[Bug 1000763] I2C Driver for at91sam7x

[bugzilla-daemon]

http://bugs.ecos.sourceware.org/show_bug.cgi?id=1000763





--- Comment #7 from Andrew Lunn <andrew.lunn@ascom.ch>  2009-05-19 08:53:42 ---
The code looks better. Thanks for the changes. I have a few more minor
requests/questions.

    // enable multi-drain
    HAL_WRITE_UINT32((AT91_PIOA + AT91_PIO_MDER),(AT91_PIO_PSR_TWD
        |AT91_PIO_PSR_TWCK));

To make the code more portable to other AT91 platforms, it would be better to
add a HAL_ARM_AT91_GPIO_MULTIDRAIN(__pin__, __enable__) to var_io.h following
the HAL_ARM_AT91_GPIO_CFG_PULLUP macro.

HAL_ARM_AT91_GPIO_CFG_PULLUP() takes an AT91_PIN macro, not AT91_PIO_PSR_*
macro.

These macros are important when the pin is actually on PIOB.

As you suggested, breaking up the for loop for waiting for completion would be
nice. I find the following much more readable (note: Untested):

    for (success = COUNT_DOWN, success, success--) {
        IC2_R32(AT91_TWI_SR,stat_reg);
        if (stat_req & sr_mask)
            break;
    }
    // error condition ,ie count down ended before txr complete flag is set
    // return how much data was transferred
    if (!success)
        break;


Before commiting to anoncvs your cyg_i2c_at91_fram_dev needs removing from both
the .c and .h file.

Apart from these comments the code looks good.


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

[Bug 1000763] I2C Driver for at91sam7x

[bugzilla-daemon]

http://bugs.ecos.sourceware.org/show_bug.cgi?id=1000763





--- Comment #8 from vibi <vibi_sreenivasan@cms.com>  2009-05-19 13:00:30 ---
hi,
   Lot of thanks for going through this code & giving suggestions.

(In reply to comment #7)
> The code looks better. Thanks for the changes. I have a few more minor
> requests/questions.
> 
>     // enable multi-drain
>     HAL_WRITE_UINT32((AT91_PIOA + AT91_PIO_MDER),(AT91_PIO_PSR_TWD
>         |AT91_PIO_PSR_TWCK));
> To make the code more portable to other AT91 platforms, it would be better to
> add a HAL_ARM_AT91_GPIO_MULTIDRAIN(__pin__, __enable__) to var_io.h following
> the HAL_ARM_AT91_GPIO_CFG_PULLUP macro.

Added & i have send that as separate patch.

> HAL_ARM_AT91_GPIO_CFG_PULLUP() takes an AT91_PIN macro, not AT91_PIO_PSR_*
> macro.
changed to 
    HAL_ARM_AT91_GPIO_CFG_PULLUP(AT91_TWI_TWD,AT91_PIN_PULLUP_DISABLE);
    HAL_ARM_AT91_GPIO_CFG_PULLUP(AT91_TWI_TWCK,AT91_PIN_PULLUP_DISABLE);

> 
> These macros are important when the pin is actually on PIOB.
> 
> As you suggested, breaking up the for loop for waiting for completion would be
> nice. I find the following much more readable (note: Untested):
> 
>     for (success = COUNT_DOWN, success, success--) {
>         IC2_R32(AT91_TWI_SR,stat_reg);
>         if (stat_req & sr_mask)
>             break;
>     }
>     // error condition ,ie count down ended before txr complete flag is set
>     // return how much data was transferred
>     if (!success)
>         break;
> 
Changed accordingly
> 
> Before commiting to anoncvs your cyg_i2c_at91_fram_dev needs removing from both
> the .c and .h file.
> 
:) that was for testing..REMOVED

> Apart from these comments the code looks good.
> 

Thanks again for spending your valuable time on helping me.
Thanks & Regards
vibi sreenivasan


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

[Bug 1000763] I2C Driver for at91sam7x

[bugzilla-daemon]

http://bugs.ecos.sourceware.org/show_bug.cgi?id=1000763


vibi <vibi_sreenivasan@cms.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #717 is|0                           |1
           obsolete|                            |
 Attachment #725 is|0                           |1
           obsolete|                            |




--- Comment #9 from vibi <vibi_sreenivasan@cms.com>  2009-05-19 13:02:09 ---
Created an attachment (id=729)
 --> (http://bugs.ecos.sourceware.org/attachment.cgi?id=729)
third version of  I2C Driver for at91sam7x


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