On Tue, Oct 12, 2004 at 10:00:50AM +0100, Ben Dooks wrote:
> + * Changelog
> + * 29-Sep-2004 BJD Initial version
> + * 04-Oct-2004 BJD Fixed ignore ACK on last read
> + * 08-Oct-2004 BJD Fixed stop if zero length message
Changelogs for kernel files are in the bk tree, not in the individual
files. Otherwise, after a number of years, we end up with a huge mess
(look at drivers/usb/serial/usb-serial.c for an example of that...)
Please remove them from the file.
> +#define PFX "s3c-i2c: "
Not needed
> +#define XFER_RETRY (-2000)
> +#define XFER_NAKED (-ECONNREFUSED)
Don't make up new error codes, just use the exiting ones please.
> +static int debug_info = 0;
Don't pre-initialize variables to 0. It wastes space and is
unnecessary.
> +/* Debug */
> +
> +#define DBG(lev, msg...) do {\
> + if (lev < debug_info) \
> + printk(KERN_INFO msg); \
> + if (lev < debug_norm ) \
> + printk(KERN_DEBUG msg); \
> + } while(0)
Please just use the dev_dbg(), dev_err() and friends macros instead of
rolling your own. Also use them instead of printk() to make it more
consistant, and easier to determine the exact device spitting out the
error/message.
Hm, I don't see a MODULE_LICENSE() marking in the file. How have you
been able to load it without tainting the kernel?
> +/* IO read/write functions */
> +
> +static inline void wr_regb(struct s3c24xx_i2c *i2c, int reg, unsigned int val)
> +{
> + writeb(val, i2c->regs + reg);
> +}
> +
> +static inline void wr_regl(struct s3c24xx_i2c *i2c, int reg, unsigned long val)
> +{
> + writel(val, i2c->regs + reg);
> +}
> +
> +static inline unsigned long rd_regb(struct s3c24xx_i2c *i2c, int reg)
> +{
> + return (unsigned long)readb(i2c->regs + reg);
> +}
> +
> +static inline unsigned long rd_regl(struct s3c24xx_i2c *i2c, int reg)
> +{
> + return readl(i2c->regs + reg);
> +}
Just use the real write* and read* functions in your code, don't wrap
them up into something else. Also try building this with the 2.6.9-rc4
kernel, odds are you will get a bunch of warnings about stuff that needs
to be fixed here... (hint void *regs needs to be void __iomem *regs in
the s3c24xx_i2c structure.)
> +#if 1
> + s3c24xx_i2c_showmsgs(msgs, num);
> +#endif
Heh, is this needed?
thanks,
greg k-h
