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 <[email protected]>");
> +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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
| |
 |