[U-Boot] [PATCH 1/3] fsl_i2c: Wait for STOP condition to propagate

[U-Boot] [PATCH 1/3] fsl_i2c: Wait for STOP condition to propagate

[Joakim Tjernlund]

After issuing a STOP one must wait until the STOP has completed
on the bus before doing something new to the controller.

Also add an extra read of SR as the manual mentions doing that
is a good idea.

Remove surplus write of CR just before a write, isn't required and
could potentially disturb the I2C bus.

Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
---
 drivers/i2c/fsl_i2c.c |   12 ++++++++----
 1 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/fsl_i2c.c b/drivers/i2c/fsl_i2c.c
index 47bbf79..56f9680 100644
--- a/drivers/i2c/fsl_i2c.c
+++ b/drivers/i2c/fsl_i2c.c
@@ -223,7 +223,7 @@ i2c_init(int speed, int slaveadd)
 #endif
 }
 
-static __inline__ int
+static int
 i2c_wait4bus(void)
 {
 	unsigned long long timeval = get_ticks();
@@ -248,6 +248,8 @@ i2c_wait(int write)
 		csr = readb(&i2c_dev[i2c_bus_num]->sr);
 		if (!(csr & I2C_SR_MIF))
 			continue;
+		/* Read again to allow register to stabilise */
+		csr = readb(&i2c_dev[i2c_bus_num]->sr);
 
 		writeb(0x0, &i2c_dev[i2c_bus_num]->sr);
 
@@ -293,9 +295,6 @@ __i2c_write(u8 *data, int length)
 {
 	int i;
 
-	writeb(I2C_CR_MEN | I2C_CR_MSTA | I2C_CR_MTX,
-	       &i2c_dev[i2c_bus_num]->cr);
-
 	for (i = 0; i < length; i++) {
 		writeb(data[i], &i2c_dev[i2c_bus_num]->dr);
 
@@ -351,6 +350,9 @@ i2c_read(u8 dev, uint addr, int alen, u8 *data, int length)
 	    && i2c_write_addr(dev, I2C_READ_BIT, 1) != 0)
 		i = __i2c_read(data, length);
 
+	if (length && i2c_wait4bus()) /* Wait until STOP */
+		debug("i2c_read: wait4bus timed out\n");
+
 	writeb(I2C_CR_MEN, &i2c_dev[i2c_bus_num]->cr);
 
 	if (i == length)
@@ -372,6 +374,8 @@ i2c_write(u8 dev, uint addr, int alen, u8 *data, int length)
 	}
 
 	writeb(I2C_CR_MEN, &i2c_dev[i2c_bus_num]->cr);
+	if (i2c_wait4bus()) /* Wait until STOP */
+		debug("i2c_write: wait4bus timed out\n");
 
 	if (i == length)
 	    return 0;
-- 
1.6.4.2

[U-Boot] [PATCH 2/3] fsl_i2c: Add CONFIG_FSL_I2C_CUSTOM_{DFSR/DFR}

[Joakim Tjernlund]

Some boards need a higher DFSR value than the spec currently
recommends so give these boards the means to define there own.

For completeness, add CONFIG_FSL_I2C_CUSTOM_DFR too.

Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
---
 drivers/i2c/fsl_i2c.c |   14 +++++++++++---
 1 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/fsl_i2c.c b/drivers/i2c/fsl_i2c.c
index 56f9680..0c5f6be 100644
--- a/drivers/i2c/fsl_i2c.c
+++ b/drivers/i2c/fsl_i2c.c
@@ -172,14 +172,22 @@ static unsigned int set_i2c_bus_speed(const struct fsl_i2c *dev,
 			u8 fdr;
 #ifdef __PPC__
 			u8 dfsr;
+#ifdef 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 */
+#endif
+#ifdef CONFIG_FSL_I2C_CUSTOM_FDR
+			fdr = CONFIG_FSL_I2C_CUSTOM_FDR;
+			speed = i2c_clk / divider; /* Fake something */
+#else
 			fdr = fsl_i2c_speed_map[i].fdr;
 			speed = i2c_clk / fsl_i2c_speed_map[i].divider;
-			writeb(fdr, &dev->fdr);		/* set bus speed */
-#ifdef __PPC__
-			writeb(dfsr, &dev->dfsrr);	/* set default filter */
 #endif
+			writeb(fdr, &dev->fdr);		/* set bus speed */
+
 			break;
 		}
 
-- 
1.6.4.2

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

[Joakim Tjernlund]

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.

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] = {
  */
 static const struct {
 	unsigned short divider;
-#ifdef __PPC__
-	u8 dfsr;
-#endif
 	u8 fdr;
 } fsl_i2c_speed_map[] = {
-#ifdef __PPC__
-	{160, 1, 32}, {192, 1, 33}, {224, 1, 34}, {256, 1, 35},
-	{288, 1, 0}, {320, 1, 1}, {352, 6, 1}, {384, 1, 2}, {416, 6, 2},
-	{448, 1, 38}, {480, 1, 3}, {512, 1, 39}, {544, 11, 3}, {576, 1, 4},
-	{608, 22, 3}, {640, 1, 5}, {672, 32, 3}, {704, 11, 5}, {736, 43, 3},
-	{768, 1, 6}, {800, 54, 3}, {832, 11, 6}, {896, 1, 42}, {960, 1, 7},
-	{1024, 1, 43}, {1088, 22, 7}, {1152, 1, 8}, {1216, 43, 7}, {1280, 1, 9},
-	{1408, 22, 9}, {1536, 1, 10}, {1664, 22, 10}, {1792, 1, 46},
-	{1920, 1, 11}, {2048, 1, 47}, {2176, 43, 11}, {2304, 1, 12},
-	{2560, 1, 13}, {2816, 43, 13}, {3072, 1, 14}, {3328, 43, 14},
-	{3584, 1, 50}, {3840, 1, 15}, {4096, 1, 51}, {4608, 1, 16},
-	{5120, 1, 17}, {6144, 1, 18}, {7168, 1, 54}, {7680, 1, 19},
-	{8192, 1, 55}, {9216, 1, 20}, {10240, 1, 21}, {12288, 1, 22},
-	{14336, 1, 58}, {15360, 1, 23}, {16384, 1, 59}, {18432, 1, 24},
-	{20480, 1, 25}, {24576, 1, 26}, {28672, 1, 62}, {30720, 1, 27},
-	{32768, 1, 63}, {36864, 1, 28}, {40960, 1, 29}, {49152, 1, 30},
-	{61440, 1, 31}, {-1, 1, 31}
-#elif defined(__M68K__)
+#ifdef __M68K__
 	{20, 32}, {22, 33}, {24, 34}, {26, 35},
 	{28, 0}, {28, 36}, {30, 1}, {32, 37},
 	{34, 2}, {36, 38}, {40, 3}, {40, 39},
@@ -158,7 +138,6 @@ static unsigned int set_i2c_bus_speed(const struct fsl_i2c *dev,
 	unsigned int i2c_clk, unsigned int speed)
 {
 	unsigned short divider = min(i2c_clk / speed, (unsigned short) -1);
-	unsigned int i;
 
 	/*
 	 * We want to choose an FDR/DFSR that generates an I2C bus speed that
@@ -166,31 +145,70 @@ static unsigned int set_i2c_bus_speed(const struct fsl_i2c *dev,
 	 * want the first divider that is equal to or greater than the
 	 * calculated divider.
 	 */
-
-	for (i = 0; i < ARRAY_SIZE(fsl_i2c_speed_map); i++)
-		if (fsl_i2c_speed_map[i].divider >= divider) {
-			u8 fdr;
 #ifdef __PPC__
-			u8 dfsr;
+	u8 dfsr, fdr = 0x31; /* Default if no FDR found */
+	unsigned short A, B, Ga, 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);
 #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) {
+		for (Gb=0; Gb<8; Gb++) {
+			B = 16 << Gb;
+			c_div = B * (A + ((3*dfsr)/B)*2);
+			if (c_div > divider && c_div < est_div) {
+				unsigned short bin_Gb, bin_Ga;
+
+				est_div = c_div;
+				bin_Gb = Gb << 2;
+				bin_Ga = (Ga & 0x3) | ((Ga & 0x4) << 3);
+				fdr = bin_Gb | bin_Ga;
+				speed = i2c_clk/est_div;
+				debug("FDR:0x%.2x, div:%ld, Ga:0x%x, Gb:0x%x, "
+				      "A:%d, B:%d, speed:%d\n",
+				      fdr, est_div, Ga, Gb, A, B, speed);
+				/* Condition 2 not accounted for */
+				debug("Tr <= %d ns\n", 
+				      (B-3*dfsr)* 1000000/(i2c_clk/1000));
+			}
+		}
+		if (A == 20)
+			A+=2;
+		if (A == 24)
+			A+=4;
+	}
+	debug("divider:%d, est_div:%ld, DFSR:%d\n", divider, est_div, dfsr);
+	debug("FDR:0x%.2x, speed:%d\n", fdr, speed);
+#endif
+	writeb(dfsr, &dev->dfsrr);	/* set default filter */
+	writeb(fdr, &dev->fdr);		/* set bus speed */
+#else
+	unsigned int i;
+
+	for (i = 0; i < ARRAY_SIZE(fsl_i2c_speed_map); i++)
+		if (fsl_i2c_speed_map[i].divider >= divider) {
+			u8 fdr;
+
 			fdr = fsl_i2c_speed_map[i].fdr;
 			speed = i2c_clk / fsl_i2c_speed_map[i].divider;
-#endif
 			writeb(fdr, &dev->fdr);		/* set bus speed */
 
 			break;
 		}
-
+#endif
 	return speed;
 }
 
-- 
1.6.4.2

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

[Heiko Schocher]

Hello Joakim,

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] = {
>   */
>  static const struct {
>  	unsigned short divider;
> -#ifdef __PPC__
> -	u8 dfsr;
> -#endif
>  	u8 fdr;
>  } fsl_i2c_speed_map[] = {
> -#ifdef __PPC__
> -	{160, 1, 32}, {192, 1, 33}, {224, 1, 34}, {256, 1, 35},
> -	{288, 1, 0}, {320, 1, 1}, {352, 6, 1}, {384, 1, 2}, {416, 6, 2},
> -	{448, 1, 38}, {480, 1, 3}, {512, 1, 39}, {544, 11, 3}, {576, 1, 4},
> -	{608, 22, 3}, {640, 1, 5}, {672, 32, 3}, {704, 11, 5}, {736, 43, 3},
> -	{768, 1, 6}, {800, 54, 3}, {832, 11, 6}, {896, 1, 42}, {960, 1, 7},
> -	{1024, 1, 43}, {1088, 22, 7}, {1152, 1, 8}, {1216, 43, 7}, {1280, 1, 9},
> -	{1408, 22, 9}, {1536, 1, 10}, {1664, 22, 10}, {1792, 1, 46},
> -	{1920, 1, 11}, {2048, 1, 47}, {2176, 43, 11}, {2304, 1, 12},
> -	{2560, 1, 13}, {2816, 43, 13}, {3072, 1, 14}, {3328, 43, 14},
> -	{3584, 1, 50}, {3840, 1, 15}, {4096, 1, 51}, {4608, 1, 16},
> -	{5120, 1, 17}, {6144, 1, 18}, {7168, 1, 54}, {7680, 1, 19},
> -	{8192, 1, 55}, {9216, 1, 20}, {10240, 1, 21}, {12288, 1, 22},
> -	{14336, 1, 58}, {15360, 1, 23}, {16384, 1, 59}, {18432, 1, 24},
> -	{20480, 1, 25}, {24576, 1, 26}, {28672, 1, 62}, {30720, 1, 27},
> -	{32768, 1, 63}, {36864, 1, 28}, {40960, 1, 29}, {49152, 1, 30},
> -	{61440, 1, 31}, {-1, 1, 31}
> -#elif defined(__M68K__)
> +#ifdef __M68K__
>  	{20, 32}, {22, 33}, {24, 34}, {26, 35},
>  	{28, 0}, {28, 36}, {30, 1}, {32, 37},
>  	{34, 2}, {36, 38}, {40, 3}, {40, 39},
> @@ -158,7 +138,6 @@ static unsigned int set_i2c_bus_speed(const struct fsl_i2c *dev,
>  	unsigned int i2c_clk, unsigned int speed)
>  {
>  	unsigned short divider = min(i2c_clk / speed, (unsigned short) -1);
> -	unsigned int i;
>  
>  	/*
>  	 * We want to choose an FDR/DFSR that generates an I2C bus speed that
> @@ -166,31 +145,70 @@ static unsigned int set_i2c_bus_speed(const struct fsl_i2c *dev,
>  	 * want the first divider that is equal to or greater than the
>  	 * calculated divider.
>  	 */
> -
> -	for (i = 0; i < ARRAY_SIZE(fsl_i2c_speed_map); i++)
> -		if (fsl_i2c_speed_map[i].divider >= divider) {
> -			u8 fdr;
>  #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.

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

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

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


> +				unsigned short bin_Gb, bin_Ga;

here too, please no mixed-case vars.

> +
> +				est_div = c_div;
> +				bin_Gb = Gb << 2;
> +				bin_Ga = (Ga & 0x3) | ((Ga & 0x4) << 3);
> +				fdr = bin_Gb | bin_Ga;
> +				speed = i2c_clk/est_div;
> +				debug("FDR:0x%.2x, div:%ld, Ga:0x%x, Gb:0x%x, "
> +				      "A:%d, B:%d, speed:%d\n",
> +				      fdr, est_div, Ga, Gb, A, B, speed);
> +				/* Condition 2 not accounted for */
> +				debug("Tr <= %d ns\n", 
> +				      (B-3*dfsr)* 1000000/(i2c_clk/1000));
> +			}
> +		}
> +		if (A == 20)
> +			A+=2;
> +		if (A == 24)
> +			A+=4;
> +	}
> +	debug("divider:%d, est_div:%ld, DFSR:%d\n", divider, est_div, dfsr);
> +	debug("FDR:0x%.2x, speed:%d\n", fdr, speed);
> +#endif
> +	writeb(dfsr, &dev->dfsrr);	/* set default filter */
> +	writeb(fdr, &dev->fdr);		/* set bus speed */
> +#else
> +	unsigned int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(fsl_i2c_speed_map); i++)
> +		if (fsl_i2c_speed_map[i].divider >= divider) {
> +			u8 fdr;
> +
>  			fdr = fsl_i2c_speed_map[i].fdr;
>  			speed = i2c_clk / fsl_i2c_speed_map[i].divider;
> -#endif
>  			writeb(fdr, &dev->fdr);		/* set bus speed */
>  
>  			break;
>  		}
> -
> +#endif
>  	return speed;
>  }

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

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

[Joakim Tjernlund]

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.

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

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

[Wolfgang Denk]

Dear Joakim Tjernlund,

In message <OF0E2B8150.47DDCD99-ONC1257634.0023E2E7-C1257634.002481BA@transmode.se> you wrote:
> 
> > > +   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.

Feel free to add a comment to explain the relation, but please use
only lower-case identifiers.

...
> Like so?
> for(Ga = 0x4, A = 10; A <= 30; Ga++, A += 2) {

Please also here:

	for (...)


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
It's certainly  convenient  the  way  the  crime  (or  condition)  of
stupidity   carries   with   it  its  own  punishment,  automatically
admisistered without remorse, pity, or prejudice. :-)
         -- Tom Christiansen in <559seq$ag1$1@csnews.cs.colorado.edu>

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

[Heiko Schocher]

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