Re: Patch of a new driver for kernel 2.4.x that need review

[Greg KH <greg@kroah.com>]

On Wed, Jul 06, 2005 at 02:14:44PM -0700, Mark Gross wrote:
> +#ifndef __KERNEL__
> +#  define __KERNEL__
> +#endif

Not needed.

> +
> +#include <linux/config.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/sched.h>
> +#include <linux/kernel.h>	/* printk() */
> +#include <linux/fs.h>		/* everything... */
> +#include <linux/errno.h>	/* error codes */
> +#include <linux/delay.h>	/* udelay */
> +#include <asm/uaccess.h>
> +#include <linux/slab.h>
> +#include <linux/ioport.h>
> +#include <linux/devfs_fs_kernel.h>	/* Devfs support */
> +#include <linux/interrupt.h>
> +#include <linux/spinlock.h>
> +#include <linux/timer.h>
> +#include <asm/io.h>		/* inb/outb */

Put asm/ at the end.

> +
> +#include "tlclk.h"	/* TELECOM IOCTL DEFINE */

No new ioctls please.  Use sysfs or something else.

> +
> +MODULE_AUTHOR("Sebastien Bouchard <sebastien.bouchard@ca.kontron.com>");
> +MODULE_LICENSE("GPL");
> +
> +/* Telecom clock I/O register definition */
> +#define TLCLK_BASE 0xa08            
> +#define TLCLK_REG0 TLCLK_BASE
> +#define TLCLK_REG1 (TLCLK_BASE+1)
> +#define TLCLK_REG2 (TLCLK_BASE+2)
> +#define TLCLK_REG3 (TLCLK_BASE+3)
> +#define TLCLK_REG4 (TLCLK_BASE+4)
> +#define TLCLK_REG5 (TLCLK_BASE+5)
> +#define TLCLK_REG6 (TLCLK_BASE+6)
> +#define TLCLK_REG7 (TLCLK_BASE+7)
> +
> +#define SET_PORT_BITS(port, mask, val) outb(((inb(port) & mask) | val), port)
> +
> +/* 0 = Dynamic allocation of the major device number */
> +#define TLCLK_MAJOR 252

Is this a reserved major?

> +/* DEVFS support or not */
> +#ifdef CONFIG_DEVFS_FS

DEVFS is obsolete and will be removed very soon now.  You don't need to
add support for it in your driver.

> +/*
> +*  Function : Module Opening
> +*  Description : Called when a program open the 
> +*  /dev/telclock file related to the module.
> +*/

Function comments like this are not needed at all.

> +struct tlclk_alarms {
> +	unsigned int lost_clocks;
> +	unsigned int lost_primary_clock;
> +	unsigned int lost_secondary_clock;
> +	unsigned int primary_clock_back;
> +	unsigned int secondary_clock_back;
> +	unsigned int switchover_primary;
> +	unsigned int switchover_secondary;
> +	unsigned int pll_holdover;
> +	unsigned int pll_end_holdover;
> +	unsigned int pll_lost_sync;
> +	unsigned int pll_sync;
> +};

If you are going to pass a structure accross the kernel/userspace
boundry, you need to explicitly mark the variable sizes.  Use __u8,
__u16, or __u32 here (unless they are signed, then use __s8, etc.)

All of those values look like they should just be exported as individual
sysfs files, right?

thanks,

greg k-h