Messages in this thread | | | Date | Wed, 6 Jul 2005 15:57:10 -0700 | From | Greg KH <> | Subject | Re: 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/
| |