lkml.org 
[lkml]   [2005]   [Jun]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: 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/

    \
     
     \ /
      Last update: 2005-06-22 21:48    [W:0.046 / U:1.796 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site