lkml.org 
[lkml]   [2005]   [Jul]   [6]   [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
    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
    -
    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-07-07 01:06    [W:0.027 / U:62.816 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site