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