lkml.org 
[lkml]   [2005]   [Jan]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH][I2C] Marvell mv64xxx i2c driver
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
--
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 */
\
 
 \ /
  Last update: 2005-03-22 14:09    [W:0.072 / U:0.972 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site