Messages in this thread | | | Date | Wed, 22 Jun 2005 22:43:40 +0300 | From | Pekka Enberg <> | Subject | Re: Patch of a new driver for kernel 2.4.x that need review |
| |
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). - 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/
| |