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

["Pekka J Enberg" <penberg@cs.helsinki.fi>]

Mark Gross writes:
> diff -urN -X dontdiff_osdl vanilla/linux-2.4.31/drivers/char/tlclk.c linux-2.4/drivers/char/tlclk.c
> --- vanilla/linux-2.4.31/drivers/char/tlclk.c	1969-12-31 16:00:00.000000000 -0800
> +++ linux-2.4/drivers/char/tlclk.c	2005-07-06 13:21:24.000000000 -0700
> @@ -0,0 +1,459 @@
> +
> +/* 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)

Enums are preferred. 

> +
> +#define SET_PORT_BITS(port, mask, val) outb(((inb(port) & mask) | val), port)

Static inline functions are preferred. 

> +
> +/* 0 = Dynamic allocation of the major device number */
> +#define TLCLK_MAJOR 252

Enums, please. 

> +spinlock_t event_lock = SPIN_LOCK_UNLOCKED;

For 2.6, DEFINE_SPINLOCK is preferred. 

> +
> +/* DEVFS support or not */
> +#ifdef CONFIG_DEVFS_FS
> +devfs_handle_t devfs_handle;
> +#else
> +static int tlclk_major = TLCLK_MAJOR;
> +#endif
> +
> +static void switchover_timeout(unsigned long data);
> +void tlclk_interrupt(int irq, void *dev_id, struct pt_regs *regs);

Please try to avoid forward declarations as much as possible. The function 
definitions can be moved further up. 

> +
> +DECLARE_WAIT_QUEUE_HEAD(wq);
> +/*
> +*  Function : Module I/O functions
> +*  Description : Almost all the control stuff is done here, check I/O dn for
> +*  help.
> +*/

Use kerneldoc format instead of inventing your own. (Appears in other places 
as well.) 

> +
> +/*
> +*  Function : Module Opening
> +*  Description : Called when a program open the 
> +*  /dev/telclock file related to the module.
> +*/
> +static int
> +tlclk_open(struct inode *inode, struct file *filp)
> +{
> +	int result;
> +#ifdef MODULE
> +	if (!MOD_IN_USE) {
> +		MOD_INC_USE_COUNT;
> +#endif
> +		/* Make sure there is no interrupt pending will 
> +		   *  initialising interrupt handler */
> +		inb(TLCLK_REG6);
> +
> +		result = request_irq(telclk_interrupt, &tlclk_interrupt,
> +					SA_SHIRQ, "telclock", tlclk_interrupt);
> +		printk("\ntelclock: Reserving IRQ%d...\n", telclk_interrupt);
> +		if (result == -EBUSY) {

request_irq() can return other error codes too. It returns zero when it 
succeeds so check only for that. 

> +			printk(KERN_ERR
> +				"telclock: Interrupt can't be reserved!\n");
> +			return -EBUSY;

It's better to propagate the error code here. 

> +/*
> +*  Function : Interrupt Handler 
> +*  Description :
> +*  Handling of alarms interrupt.
> +*  
> +*/
> +void
> +tlclk_interrupt(int irq, void *dev_id, struct pt_regs *regs)
> +{

Why isn't this static? 

> diff -urN -X dontdiff_osdl vanilla/linux-2.4.31/drivers/char/tlclk.h linux-2.4/drivers/char/tlclk.h
> --- vanilla/linux-2.4.31/drivers/char/tlclk.h	1969-12-31 16:00:00.000000000 -0800
> +++ linux-2.4/drivers/char/tlclk.h	2005-07-06 12:30:28.000000000 -0700
> @@ -0,0 +1,167 @@
> +/* Ioctl definitions  */
> +
> +/* Use 0xA1 as magic number */
> +#define TLCLK_IOC_MAGIC 0xA1
> +
> +/*Hardware Reset of the PLL */
> +
> +#define RESET_ON 0x00
> +#define RESET_OFF 0x01
> +#define IOCTL_RESET _IO(TLCLK_IOC_MAGIC,  1)
> +
> +#define IOCTL_REFALIGN _IO(TLCLK_IOC_MAGIC,  2)
> +
> +/* MODE SELECT */
> +
> +#define NORMAL_MODE 0x00
> +#define HOLDOVER_MODE 0x10
> +#define FREERUN_MODE 0x20
> +

[snip, snip] 

Enums, please.