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

Greg KH
2005-07-06T22:59:37Z

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