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

[Jean Delvare <khali@linux-fr.org>]

Hi Derek,

My comments on your code:

> +    Changes:
> +    v0.1	26 March 2005
> +		Initial Release - developed on uClinux with 2.6.9 kernel
> +    v0.2	11 April 2005
> +		Coding style changes
> +

Changelogs are not welcome in kernel code.

> +#include <linux/config.h>
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/errno.h>
> +#include <linux/i2c.h>
> +#include <linux/delay.h>
> +#include <linux/string.h>
> +#include <asm/coldfire.h>
> +#include <asm/m528xsim.h>
> +#include <asm/types.h>
> +#include "i2c-mcf5282.h"

I can't see that you need config.h, errno.h nor string.h.

> +	/* printk(KERN_DEBUG "%s I2DR is %.2x \n", __FUNCTION__, *rxData); */

Please don't include dead code.

> +	timeout = 500;

You use a timeout of 500 in various places. I wonder if you shouldn't
have a define for it - in case it needs to be altered later.

> +	while (timeout-- && !(*MCF5282_I2C_I2SR & MCF5282_I2C_I2SR_IIF))
> +		udelay(1);
> +	if (timeout <= 0)
> +		printk(KERN_WARNING "%s - I2C IIF never set \n", __FUNCTION__);

These constructs are error-prone. I personally prefer:
	while(--timeout && ...)
		...
	if (!timeout)
		...

> +			rc += mcf5282_write_data(data->word & 0x00FF);
> +			rc += mcf5282_write_data((data->word & 0x00FF) >> 8);

Looks like a bug to me.

> +static s32 mcf5282_i2c_access(struct i2c_adapter *adap, u16 addr,
> +			      unsigned short flags, char read_write,
> +			      u8 command, int size, union i2c_smbus_data *data)
> (...)
> +		printk(KERN_WARNING "Not support I2C_SMBUS_PROC_CALL yet \n");

Don't use printk when you can user dev_{dbg,warn,info,err}. Also, please
no space before newline.

> +static u32 mcf5282_func(struct i2c_adapter *adapter)
> +{
> +	return I2C_FUNC_SMBUS_QUICK |
> +	       I2C_FUNC_SMBUS_BYTE |
> +	       I2C_FUNC_SMBUS_PROC_CALL |
> +	       I2C_FUNC_SMBUS_BYTE_DATA |
> +	       I2C_FUNC_SMBUS_WORD_DATA |
> +	       I2C_FUNC_SMBUS_BLOCK_DATA;
> +};

Don't say you support I2C_FUNC_SMBUS_PROC_CALL and
I2C_FUNC_SMBUS_BLOCK_DATA when you in fact don't. Not supporting some
functions is no problem, just leave them out for now. You can always
submit patches later.

> --- linux-2.6.11.6/drivers/i2c/busses/i2c-mcf5282.h	1969-12-31 19:00:00.000000000 -0500
> +++ linux_dev/drivers/i2c/busses/i2c-mcf5282.h	2005-04-12 20:45:00.000000000 -0400

I see no reason to have a separate .h file for this.

> --- linux-2.6.11.6/drivers/i2c/busses/Kconfig	2005-03-25 22:28:29.000000000 -0500
> +++ linux_dev/drivers/i2c/busses/Kconfig	2005-04-12 20:45:24.000000000 -0400
> @@ -39,6 +39,16 @@ config I2C_ALI15X3
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called i2c-ali15x3.
>  
> +config I2C_MCF5282

Please keep the entries in somewhat alphabetical order.

> --- linux-2.6.11.6/drivers/i2c/busses/Makefile	2005-03-25 22:28:39.000000000 -0500
> +++ linux_dev/drivers/i2c/busses/Makefile	2005-04-13 18:52:03.000000000 -0400
> @@ -40,6 +40,7 @@ obj-$(CONFIG_I2C_VIAPRO)	+= i2c-viapro.o
>  obj-$(CONFIG_I2C_VOODOO3)	+= i2c-voodoo3.o
>  obj-$(CONFIG_SCx200_ACB)	+= scx200_acb.o
>  obj-$(CONFIG_SCx200_I2C)	+= scx200_i2c.o
> +obj-$(CONFIG_I2C_MCF5282)	+= i2c-mcf5282.o

Ditto.

> +#define MCF5282_I2C_I2CR_IEN    (0x80)	/* I2C enable */
> +#define MCF5282_I2C_I2CR_IIEN   (0x40)  /* interrupt enable */
> +#define MCF5282_I2C_I2CR_MSTA   (0x20)  /* master/slave mode */
> +#define MCF5282_I2C_I2CR_MTX    (0x10)  /* transmit/receive mode */
> +#define MCF5282_I2C_I2CR_TXAK   (0x08)  /* transmit acknowledge enable */
> +#define MCF5282_I2C_I2CR_RSTA   (0x04)  /* repeat start */
> +
> +#define MCF5282_I2C_I2SR_ICF    (0x80)  /* data transfer bit */
> +#define MCF5282_I2C_I2SR_IAAS   (0x40)  /* I2C addressed as a slave */
> +#define MCF5282_I2C_I2SR_IBB    (0x20)  /* I2C bus busy */
> +#define MCF5282_I2C_I2SR_IAL    (0x10)  /* aribitration lost */
> +#define MCF5282_I2C_I2SR_SRW    (0x04)  /* slave read/write */
> +#define MCF5282_I2C_I2SR_IIF    (0x02)  /* I2C interrupt */
> +#define MCF5282_I2C_I2SR_RXAK   (0x01)  /* received acknowledge */

Parentheses are not needed here.

Thanks,
-- 
Jean Delvare
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/