Messages in this thread | | | Date | Wed, 26 Jan 2005 14:42:31 -0800 | From | Greg KH <> | Subject | Re: [PATCH][I2C] Marvell mv64xxx i2c driver |
| |
On Wed, Jan 26, 2005 at 02:56:55PM -0700, Mark A. Greer wrote: > +#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"
Please put <asm/ after <linux/
> +static inline 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);
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.
> +static inline void > +mv64xxx_i2c_prepare_for_io(struct mv64xxx_i2c_data *drv_data, > + struct i2c_msg *msg)
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.
> diff -Nru a/drivers/i2c/busses/i2c-mv64xxx.h b/drivers/i2c/busses/i2c-mv64xxx.h > --- /dev/null Wed Dec 31 16:00:00 196900 > +++ b/drivers/i2c/busses/i2c-mv64xxx.h 2005-01-26 14:49:22 -07:00
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.
thanks,
greg k-h - 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/
| |