Re: [PATCH] kernel 2.6.11.6 - I2C adaptor for ColdFire 5282 CPU

["Randy.Dunlap" <rddunlap@osdl.org>]

On Sun, 10 Apr 2005 12:47:42 -0400 Derek Cheung wrote:

| Enclosed please find the updated patch that incorporates changes for all
| the comments I received.

(yes, almost all)

| The volatile declaration in the m528xsim.h is needed because the
| declaration refers to the ColdFire 5282 register mapping. The volatile
| declaration is actually not needed in my I2C driver but someone may
| include the m528xsim.h file in his/her applications and we need to force
| the compiler not to do any optimization on the register mapping.

Did you also send it to the I2C mailing list like Greg asked you to do?

More comments:

diffstat summary, like so, would be nice:
 drivers/i2c/busses/Kconfig       |   10
 drivers/i2c/busses/Makefile      |    2
 drivers/i2c/busses/i2c-mcf5282.c |  419 +++++++++++++++++++++++++++++++++++++++
 drivers/i2c/busses/i2c-mcf5282.h |   46 ++++
 include/asm-m68knommu/m528xsim.h |   42 +++
 5 files changed, 519 insertions(+)


+	int i, len, rc = 0;
+        u8 rxData, tempRxData[2];
Use tabs, not spaces.  Happens other places too.

+        switch (size) {
+                case I2C_SMBUS_QUICK:
Don't need to indent case statements... (repeating myself).

+                case I2C_SMBUS_PROC_CALL:
+			/* dev_info(&adap->dev, "size = 
+				I2C_SMBUS_PROC_CALL \n"); */
No break needed here ?
+        	case I2C_SMBUS_WORD_DATA:

+	if (rc < 0) 
+		return -1;
+	else
+        	return 0;
There are several of these.  Why not just return rc ?


Kconfig says that the module (if selected) will be called
i2c-mcf5282lite, but Makefile builds
+obj-$(CONFIG_I2C_MCF5282LITE)   += i2c-mcf5282.o
(i.e., no "lite").


---
~Randy