Re: [PATCH][I2C] Marvell mv64xxx i2c driver

["Mark A. Greer" <mgreer@mvista.com>]

[-- Attachment #1: Type: text/plain, Size: 767 bytes --]

Greg KH wrote:

>Please put <asm/ after <linux/
>

Done.

>You have a lot of pr_debug and other printk() for stuff in this driver.
>Please use dev_dbg(), dev_err() and friends instead.  That way you get a
>consistant message, that points to the exact device that is causing the
>message.
>

Cool.  Done.

>You have some big inline functions here.  Should they really be inline?
>We aren't really worried about speed here, right?  Size is probably a
>bigger issue.
>

No, no, and done.

>
>Is this header file really needed?  Does any other file other than this
>single driver ever include it?  If not, please just put it into the
>driver itself.
>  
>

No, no, and done.

Included is an *incremental* patch that I hope addresses your concerns.

Thanks Greg.

Mark
--

[-- Attachment #2: i2c_3.patch --]
[-- Type: text/plain, Size: 10496 bytes --]

diff -Nru a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
--- a/drivers/i2c/busses/i2c-mv64xxx.c	2005-01-26 16:52:56 -07:00
+++ b/drivers/i2c/busses/i2c-mv64xxx.c	2005-01-26 16:52:56 -07:00
@@ -11,21 +11,94 @@
  * is licensed "as is" without any warranty of any kind, whether express
  * or implied.
  */
-#include <linux/config.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
-#include <linux/sched.h>
-#include <linux/init.h>
-#include <linux/pci.h>
-#include <linux/wait.h>
 #include <linux/spinlock.h>
-#include <asm/io.h>
-#include <asm/ocp.h>
 #include <linux/i2c.h>
 #include <linux/interrupt.h>
-#include <linux/delay.h>
 #include <linux/mv643xx.h>
-#include "i2c-mv64xxx.h"
+#include <asm/io.h>
+
+/* Register defines */
+#define	MV64XXX_I2C_REG_SLAVE_ADDR			0x00
+#define	MV64XXX_I2C_REG_DATA				0x04
+#define	MV64XXX_I2C_REG_CONTROL				0x08
+#define	MV64XXX_I2C_REG_STATUS				0x0c
+#define	MV64XXX_I2C_REG_BAUD				0x0c
+#define	MV64XXX_I2C_REG_EXT_SLAVE_ADDR			0x10
+#define	MV64XXX_I2C_REG_SOFT_RESET			0x1c
+
+#define	MV64XXX_I2C_REG_CONTROL_ACK			0x00000004
+#define	MV64XXX_I2C_REG_CONTROL_IFLG			0x00000008
+#define	MV64XXX_I2C_REG_CONTROL_STOP			0x00000010
+#define	MV64XXX_I2C_REG_CONTROL_START			0x00000020
+#define	MV64XXX_I2C_REG_CONTROL_TWSIEN			0x00000040
+#define	MV64XXX_I2C_REG_CONTROL_INTEN			0x00000080
+
+/* Ctlr status values */
+#define	MV64XXX_I2C_STATUS_BUS_ERR			0x00
+#define	MV64XXX_I2C_STATUS_MAST_START			0x08
+#define	MV64XXX_I2C_STATUS_MAST_REPEAT_START		0x10
+#define	MV64XXX_I2C_STATUS_MAST_WR_ADDR_ACK		0x18
+#define	MV64XXX_I2C_STATUS_MAST_WR_ADDR_NO_ACK		0x20
+#define	MV64XXX_I2C_STATUS_MAST_WR_ACK			0x28
+#define	MV64XXX_I2C_STATUS_MAST_WR_NO_ACK		0x30
+#define	MV64XXX_I2C_STATUS_MAST_LOST_ARB		0x38
+#define	MV64XXX_I2C_STATUS_MAST_RD_ADDR_ACK		0x40
+#define	MV64XXX_I2C_STATUS_MAST_RD_ADDR_NO_ACK		0x48
+#define	MV64XXX_I2C_STATUS_MAST_RD_DATA_ACK		0x50
+#define	MV64XXX_I2C_STATUS_MAST_RD_DATA_NO_ACK		0x58
+#define	MV64XXX_I2C_STATUS_MAST_WR_ADDR_2_ACK		0xd0
+#define	MV64XXX_I2C_STATUS_MAST_WR_ADDR_2_NO_ACK	0xd8
+#define	MV64XXX_I2C_STATUS_MAST_RD_ADDR_2_ACK		0xe0
+#define	MV64XXX_I2C_STATUS_MAST_RD_ADDR_2_NO_ACK	0xe8
+#define	MV64XXX_I2C_STATUS_NO_STATUS			0xf8
+
+/* Driver states */
+enum {
+	MV64XXX_I2C_STATE_INVALID,
+	MV64XXX_I2C_STATE_IDLE,
+	MV64XXX_I2C_STATE_WAITING_FOR_START_COND,
+	MV64XXX_I2C_STATE_WAITING_FOR_ADDR_1_ACK,
+	MV64XXX_I2C_STATE_WAITING_FOR_ADDR_2_ACK,
+	MV64XXX_I2C_STATE_WAITING_FOR_SLAVE_ACK,
+	MV64XXX_I2C_STATE_WAITING_FOR_SLAVE_DATA,
+	MV64XXX_I2C_STATE_ABORTING,
+};
+
+/* Driver actions */
+enum {
+	MV64XXX_I2C_ACTION_INVALID,
+	MV64XXX_I2C_ACTION_CONTINUE,
+	MV64XXX_I2C_ACTION_SEND_START,
+	MV64XXX_I2C_ACTION_SEND_ADDR_1,
+	MV64XXX_I2C_ACTION_SEND_ADDR_2,
+	MV64XXX_I2C_ACTION_SEND_DATA,
+	MV64XXX_I2C_ACTION_RCV_DATA,
+	MV64XXX_I2C_ACTION_RCV_DATA_STOP,
+	MV64XXX_I2C_ACTION_SEND_STOP,
+};
+
+struct mv64xxx_i2c_data {
+	int			irq;
+	uint			state;
+	uint			action;
+	u32			cntl_bits;
+	void			*reg_base;
+	ulong			reg_base_p;
+	u32			addr1;
+	u32			addr2;
+	uint			bytes_left;
+	uint			byte_posn;
+	uint			block;
+	int			rc;
+	u32			freq_m;
+	u32			freq_n;
+	wait_queue_head_t	waitq;
+	spinlock_t		lock;
+	struct i2c_msg		*msg;
+	struct i2c_adapter	adapter;
+};
 
 /*
  *****************************************************************************
@@ -34,12 +107,9 @@
  *
  *****************************************************************************
  */
-static inline void
+static void
 mv64xxx_i2c_fsm(struct mv64xxx_i2c_data *drv_data, u32 status)
 {
-	pr_debug("mv64xxx_i2c_fsm: ENTER--state: %d, status: 0x%x\n",
-		drv_data->state, status);
-
 	/*
 	 * If state is idle, then this is likely the remnants of an old
 	 * operation that driver has given up on or the user has killed.
@@ -48,14 +118,12 @@
 	if (drv_data->state == MV64XXX_I2C_STATE_IDLE) {
 		drv_data->action = MV64XXX_I2C_ACTION_SEND_STOP;
 		drv_data->state = MV64XXX_I2C_STATE_IDLE;
-		pr_debug("mv64xxx_i2c_fsm: EXIT--Entered when in IDLE state\n");
 		return;
 	}
 
 	if (drv_data->state == MV64XXX_I2C_STATE_ABORTING) {
 		drv_data->action = MV64XXX_I2C_ACTION_SEND_STOP;
 		drv_data->state = MV64XXX_I2C_STATE_IDLE;
-		pr_debug("mv64xxx_i2c_fsm: EXIT--Aborting\n");
 		return;
 	}
 
@@ -135,27 +203,22 @@
 		break;
 
 	default:
-		printk(KERN_ERR "mv64xxx_i2c_fsm: Ctlr Error -- "
-			"state: 0x%x, status: 0x%x\n", drv_data->state, status);
-		printk(KERN_INFO "addr: 0x%x, flags: 0x%x\n",
-			drv_data->msg->addr, drv_data->msg->flags);
+		dev_err(&drv_data->adapter.dev,
+			"mv64xxx_i2c_fsm: Ctlr Error -- state: 0x%x, "
+			"status: 0x%x, addr: 0x%x, flags: 0x%x\n",
+			 drv_data->state, status, drv_data->msg->addr,
+			 drv_data->msg->flags);
 		drv_data->action = MV64XXX_I2C_ACTION_SEND_STOP;
 		drv_data->state = MV64XXX_I2C_STATE_IDLE;
 		drv_data->rc = -EIO;
 	}
 
-	pr_debug("mv64xxx_i2c_fsm: EXIT--action: %d, state: %d, rc: 0x%x\n",
-		drv_data->action, drv_data->state, drv_data->rc);
 	return;
 }
 
 static void
 mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
 {
-	pr_debug("mv64xxx_i2c_do_action: ENTER--action: %d, state: %d, "
-		"cntl: 0x%x\n", drv_data->action, drv_data->state,
-		drv_data->cntl_bits);
-
 	switch(drv_data->action) {
 	case MV64XXX_I2C_ACTION_CONTINUE:
 		writel(drv_data->cntl_bits,
@@ -207,7 +270,8 @@
 
 	case MV64XXX_I2C_ACTION_INVALID:
 	default:
-		printk(KERN_ERR "mv64xxx_i2c_do_action: Invalid action: %d\n",
+		dev_err(&drv_data->adapter.dev,
+			"mv64xxx_i2c_do_action: Invalid action: %d\n",
 			drv_data->action);
 		drv_data->rc = -EIO;
 		/* FALLTHRU */
@@ -220,7 +284,6 @@
 		break;
 	}
 
-	pr_debug("mv64xxx_i2c_do_action: EXIT\n");
 	return;
 }
 
@@ -252,7 +315,7 @@
  *
  *****************************************************************************
  */
-static inline void
+static void
 mv64xxx_i2c_prepare_for_io(struct mv64xxx_i2c_data *drv_data,
 	struct i2c_msg *msg)
 {
@@ -283,7 +346,7 @@
 	return;
 }
 
-static inline void
+static void
 mv64xxx_i2c_wait_for_completion(struct mv64xxx_i2c_data *drv_data)
 {
 	long	flags, time_left;
@@ -312,7 +375,8 @@
 
 		if (!time_left <= 0) {
 			drv_data->state = MV64XXX_I2C_STATE_IDLE;
-			printk(KERN_WARNING "mv64xxx: I2C bus locked\n");
+			dev_err(&drv_data->adapter.dev,
+				"mv64xxx: I2C bus locked\n");
 		}
 	}
 	else
@@ -321,7 +385,7 @@
 	return;
 }
 
-static inline int
+static int
 mv64xxx_i2c_execute_msg(struct mv64xxx_i2c_data *drv_data, struct i2c_msg *msg)
 {
 	long	flags;
@@ -484,7 +548,7 @@
 		if (request_irq(drv_data->irq, mv64xxx_i2c_intr, 0,
 			MV64XXX_I2C_CTLR_NAME, drv_data)) {
 
-			printk(KERN_ERR "mv64xxx: Can't register intr handler "
+			dev_err(dev, "mv64xxx: Can't register intr handler "
 				"irq: %d\\n", drv_data->irq);
 
 			mv64xxx_i2c_unmap_regs(drv_data);
@@ -492,8 +556,8 @@
 			return -EINVAL;
 		}
 		else if ((rc = i2c_add_adapter(&drv_data->adapter)) != 0) {
-			printk(KERN_WARNING "mv64xxx: Can't add i2c adapter "
-				"rc: %d\n", -rc);
+			dev_err(dev, "mv64xxx: Can't add i2c adapter, rc: %d\n",
+				-rc);
 			free_irq(drv_data->irq, drv_data);
 			mv64xxx_i2c_unmap_regs(drv_data);
 			kfree(drv_data);
diff -Nru a/drivers/i2c/busses/i2c-mv64xxx.h b/drivers/i2c/busses/i2c-mv64xxx.h
--- a/drivers/i2c/busses/i2c-mv64xxx.h	2005-01-26 16:52:56 -07:00
+++ /dev/null	Wed Dec 31 16:00:00 196900
@@ -1,99 +0,0 @@
-/*
- * drivers/i2c/busses/i2c-mv64xxx.h
- * 
- * Driver for the i2c controller on the Marvell line of host bridges for MIPS
- * and PPC (e.g, gt642[46]0, mv643[46]0, mv644[46]0).
- *
- * Author: Mark A. Greer <mgreer@mvista.com>
- *
- * 2005 (c) MontaVista, Software, Inc.  This file is licensed under
- * the terms of the GNU General Public License version 2.  This program
- * is licensed "as is" without any warranty of any kind, whether express
- * or implied.
- */
-
-#ifndef I2C_MV64XXX_H
-#define I2C_MV64XXX_H
-
-/* Register defines */
-#define	MV64XXX_I2C_REG_SLAVE_ADDR			0x00
-#define	MV64XXX_I2C_REG_DATA				0x04
-#define	MV64XXX_I2C_REG_CONTROL				0x08
-#define	MV64XXX_I2C_REG_STATUS				0x0c
-#define	MV64XXX_I2C_REG_BAUD				0x0c
-#define	MV64XXX_I2C_REG_EXT_SLAVE_ADDR			0x10
-#define	MV64XXX_I2C_REG_SOFT_RESET			0x1c
-
-#define	MV64XXX_I2C_REG_CONTROL_ACK			0x00000004
-#define	MV64XXX_I2C_REG_CONTROL_IFLG			0x00000008
-#define	MV64XXX_I2C_REG_CONTROL_STOP			0x00000010
-#define	MV64XXX_I2C_REG_CONTROL_START			0x00000020
-#define	MV64XXX_I2C_REG_CONTROL_TWSIEN			0x00000040
-#define	MV64XXX_I2C_REG_CONTROL_INTEN			0x00000080
-
-/* Ctlr status values */
-#define	MV64XXX_I2C_STATUS_BUS_ERR			0x00
-#define	MV64XXX_I2C_STATUS_MAST_START			0x08
-#define	MV64XXX_I2C_STATUS_MAST_REPEAT_START		0x10
-#define	MV64XXX_I2C_STATUS_MAST_WR_ADDR_ACK		0x18
-#define	MV64XXX_I2C_STATUS_MAST_WR_ADDR_NO_ACK		0x20
-#define	MV64XXX_I2C_STATUS_MAST_WR_ACK			0x28
-#define	MV64XXX_I2C_STATUS_MAST_WR_NO_ACK		0x30
-#define	MV64XXX_I2C_STATUS_MAST_LOST_ARB		0x38
-#define	MV64XXX_I2C_STATUS_MAST_RD_ADDR_ACK		0x40
-#define	MV64XXX_I2C_STATUS_MAST_RD_ADDR_NO_ACK		0x48
-#define	MV64XXX_I2C_STATUS_MAST_RD_DATA_ACK		0x50
-#define	MV64XXX_I2C_STATUS_MAST_RD_DATA_NO_ACK		0x58
-#define	MV64XXX_I2C_STATUS_MAST_WR_ADDR_2_ACK		0xd0
-#define	MV64XXX_I2C_STATUS_MAST_WR_ADDR_2_NO_ACK	0xd8
-#define	MV64XXX_I2C_STATUS_MAST_RD_ADDR_2_ACK		0xe0
-#define	MV64XXX_I2C_STATUS_MAST_RD_ADDR_2_NO_ACK	0xe8
-#define	MV64XXX_I2C_STATUS_NO_STATUS			0xf8
-
-/* Driver states */
-enum {
-	MV64XXX_I2C_STATE_INVALID,
-	MV64XXX_I2C_STATE_IDLE,
-	MV64XXX_I2C_STATE_WAITING_FOR_START_COND,
-	MV64XXX_I2C_STATE_WAITING_FOR_ADDR_1_ACK,
-	MV64XXX_I2C_STATE_WAITING_FOR_ADDR_2_ACK,
-	MV64XXX_I2C_STATE_WAITING_FOR_SLAVE_ACK,
-	MV64XXX_I2C_STATE_WAITING_FOR_SLAVE_DATA,
-	MV64XXX_I2C_STATE_ABORTING,
-};
-
-/* Driver actions */
-enum {
-	MV64XXX_I2C_ACTION_INVALID,
-	MV64XXX_I2C_ACTION_CONTINUE,
-	MV64XXX_I2C_ACTION_SEND_START,
-	MV64XXX_I2C_ACTION_SEND_ADDR_1,
-	MV64XXX_I2C_ACTION_SEND_ADDR_2,
-	MV64XXX_I2C_ACTION_SEND_DATA,
-	MV64XXX_I2C_ACTION_RCV_DATA,
-	MV64XXX_I2C_ACTION_RCV_DATA_STOP,
-	MV64XXX_I2C_ACTION_SEND_STOP,
-};
-
-struct mv64xxx_i2c_data {
-	int			irq;
-	uint			state;
-	uint			action;
-	u32			cntl_bits;
-	void			*reg_base;
-	ulong			reg_base_p;
-	u32			addr1;
-	u32			addr2;
-	uint			bytes_left;
-	uint			byte_posn;
-	uint			block;
-	int			rc;
-	u32			freq_m;
-	u32			freq_n;
-	wait_queue_head_t	waitq;
-	spinlock_t		lock;
-	struct i2c_msg		*msg;
-	struct i2c_adapter	adapter;
-};
-
-#endif /* I2C_MV64XXX_H */