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

[Joakim Tjernlund]

Heiko Schocher <hs at 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 at 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.

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

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