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

Pekka Enberg
2005-06-22T19:44:06Z

Hi,

Some comments below. Please note that I am more familiar with 2.6 so
some of these might not apply.

Pekka

On 6/22/05, Bouchard, Sebastien <Sebastien.Bouchard@ca.kontron.com> wrote:
> --- 2.4.31-ori/drivers/char/tlclk.c Wed Dec 31 19:00:00 1969
> +++ 2.4.31-mod/drivers/char/tlclk.c Wed Jun 22 09:43:10 2005
> +/* 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

Please use enums instead.

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

Please use kerneldoc format.

> +static struct file_operations tlclk_fops = {
> + read:tlclk_read,
> + write:tlclk_write,
> + ioctl:tlclk_ioctl,
> + open:tlclk_open,
> + release:tlclk_release,

Please use C99 struct initializers.

> +
> +};
> +/*
> +* Function : Module Initialisation
> +* Description : Called at module loading,
> +* all the OS registering stuff is her
> +*/
> +static int __init
> +tlclk_init(void)
> +{
> + alarm_events = kmalloc(sizeof (struct tlclk_alarms), GFP_KERNEL);
> +
> + if(!alarm_events)
> + return -EBUSY;
> +
> + memset(alarm_events, 0, sizeof (struct tlclk_alarms));
> +
> +/* Read telecom clock IRQ number (Set by BIOS) */
> +
> + telclk_interrupt = (inb(TLCLK_REG7) & 0x0f);
> +
> + printk(KERN_WARNING "telclock: Reserving I/O region...\n");
> +
> + if (check_region(TLCLK_BASE, 8)) {
> + printk(KERN_ERR
> + "telclock: I/O region already used by another
> driver!\n");
> + return -EBUSY;

You're leaking alarm_events here.

> + } else {
> + request_region(TLCLK_BASE, 8, "telclock");

Please put nominal case first (i.e. make this the first block).

> + }
> +
> +/* DEVFS or NOT? */
> +
> +#ifdef CONFIG_DEVFS_FS
> + devfs_handle = devfs_register(NULL, "telclock",
> + DEVFS_FL_AUTO_DEVNUM, TLCLK_MAJOR,
> + 0,
> + S_IFCHR | S_IRUGO | S_IWUGO,
> + &tlclk_fops, NULL);
> + if (!devfs_handle)
> + return -EBUSY;

You're leaking alarm_events and reserved region here. (Use gotos for
error handling, btw.)

> +#else
> + tlclk_major = register_chrdev(tlclk_major, "telclock", &tlclk_fops);
> +
> + if (tlclk_major < 0) {
> + printk(KERN_ERR "telclock: can't get major! %d\n",
> tlclk_major);
> + return tlclk_major;

Same as before plus leaking devfs handle.

> + }
> +#endif
> +
> + init_timer(&switchover_timer);
> + switchover_timer.function = switchover_timeout;
> + switchover_timer.data = 0;
> +
> + return 0;
> +}

> diff -ruN 2.4.31-ori/drivers/char/tlclk.h 2.4.31-mod/drivers/char/tlclk.h
> --- 2.4.31-ori/drivers/char/tlclk.h Wed Dec 31 19:00:00 1969
> +++ 2.4.31-mod/drivers/char/tlclk.h Wed Jun 22 09:43:10 2005
> +/* 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

Please use enums instead (applies to other parts of this file too).