[U-Boot] [PATCH 3/3] fsl_i2c: Impl. AN2819, rev 5 to calculate FDR/DFSR

[Heiko Schocher <hs@denx.de>]

Hello Joakim,

Joakim Tjernlund wrote:
> Heiko Schocher <hs@denx.de> wrote on 17/09/2009 08:00:34:
> 
>> Hello Joakim,
> 
> Hi Heiko
> 
>> Joakim Tjernlund wrote:
>>> The latest AN2819 has changed the way FDR/DFSR should be calculated.
>>> Update the driver according to spec. However, Condition 2
>>> is not accounted for as it is not clear how to do so.
>> Thanks for your work, just some minor Codingstyle comments:
>>
>>> Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
>>> ---
>>>  drivers/i2c/fsl_i2c.c |   88 +++++++++++++++++++++++++++++-------------------
>>>  1 files changed, 53 insertions(+), 35 deletions(-)
>>>
>>> diff --git a/drivers/i2c/fsl_i2c.c b/drivers/i2c/fsl_i2c.c
>>> index 0c5f6be..0491a69 100644
>>> --- a/drivers/i2c/fsl_i2c.c
>>> +++ b/drivers/i2c/fsl_i2c.c
>>> @@ -100,29 +100,9 @@ static const struct fsl_i2c *i2c_dev[2] = {
> 
>>>  #ifdef __PPC__
>>> -         u8 dfsr;
>>> +   u8 dfsr, fdr = 0x31; /* Default if no FDR found */
>>> +   unsigned short A, B, Ga, Gb;
>> Please do not use mixed-case variables, thanks.
> 
> A and B are from the AN2819 spec and I used the same names to ease
> identify with the spec. I rather keep them.

I am fine with that, just please do not mix upper and
lower case in one variable name. So please use "ga" and
"gb" ...

>>> +   unsigned long c_div, est_div;
>>> +
>>>  #ifdef CONFIG_FSL_I2C_CUSTOM_DFSR
>>> -         dfsr = CONFIG_FSL_I2C_CUSTOM_DFSR;
>>> +   dfsr = CONFIG_FSL_I2C_CUSTOM_DFSR;
>>>  #else
>>> -         dfsr = fsl_i2c_speed_map[i].dfsr;
>>> -#endif
>>> -         writeb(dfsr, &dev->dfsrr);   /* set default filter */
>>> +   /* Condition 1: dfsr <= 50/T */
>>> +   dfsr = (5*(i2c_clk/1000))/(100000);
>> Please use one space around (on each side of) most binary
>> and ternary operators.
> 
> Like so?
> dfsr = (5 * (i2c_clk / 1000)) / 100000);

Yep.

>>>  #endif
>>>  #ifdef CONFIG_FSL_I2C_CUSTOM_FDR
>>> -         fdr = CONFIG_FSL_I2C_CUSTOM_FDR;
>>> -         speed = i2c_clk / divider; /* Fake something */
>>> +   fdr = CONFIG_FSL_I2C_CUSTOM_FDR;
>>> +   speed = i2c_clk / divider; /* Fake something */
>>>  #else
>>> +   debug("Requested speed:%d, i2c_clk:%d\n", speed, i2c_clk);
>>> +   if (!dfsr)
>>> +      dfsr = 1;
>>> +
>>> +   est_div = ~0;
>>> +   for(Ga=0x4, A=10; A<=30; Ga++, A+=2) {
>> spaces her too.
> Like so?
> for(Ga = 0x4, A = 10; A <= 30; Ga++, A += 2) {

for (Ga = 0x4, A = 10; A <= 30; Ga++, A += 2) {

>>> +      for (Gb=0; Gb<8; Gb++) {
>> and here too. Please check the whole patch.
>>
>>> +         B = 16 << Gb;
>>> +         c_div = B * (A + ((3*dfsr)/B)*2);
>>> +         if (c_div > divider && c_div < est_div) {
>> Can we make
>>
>> if ((c_div > divider) && (c_div < est_div)) {
> 
> Sure.

Thanks!

bye
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany