Heiko Schocher <h...@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.tjernl...@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 mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
