Messages in this thread | | | Date | Thu, 21 Feb 2002 04:39:38 -0500 | From | Jeff Garzik <> | Subject | Re: [DRIVER][RFC] SC1200 Watchdog driver |
| |
> /* > * National Semiconductor PC87307/PC97307 (ala SC1200) WDT driver > * (c) Copyright 2002 Zwane Mwaikambo <zwane@commfireservices.com>, > * All Rights Reserved. > * Based on wdt.c and wdt977.c by Alan Cox and Woody Suwalski respectively.
whee, nice and clean driver....
> #include <linux/config.h> > #include <linux/module.h> > #include <linux/version.h> > #include <linux/types.h> > #include <linux/errno.h> > #include <linux/kernel.h> > #include <linux/sched.h> > #include <linux/smp_lock.h> > #include <linux/miscdevice.h> > #include <linux/watchdog.h> > #include <linux/slab.h> > #include <linux/ioport.h> > #include <linux/fcntl.h> > #include <asm/semaphore.h> > #include <asm/io.h> > #include <asm/uaccess.h> > #include <asm/system.h> > #include <linux/notifier.h> > #include <linux/reboot.h> > #include <linux/init.h>
try deleting all includes and rebuild this list from scratch... I'll bet it can be made smaller.
> static int sc1200wdt_status(void)
why not make this 'static inline' too, it's pretty small, and sc1200wdt_read_data is likewise static inline.
> static int sc1200wdt_open(struct inode *inode, struct file *file) > { > unsigned char reg; > > /* allow one at a time */ > if (down_trylock(&open_sem)) > return -EBUSY; > > MOD_INC_USE_COUNT;
No need for MOD_INC_USE_COUNT when you initialize "owner: THIS_MODULE".
Maybe this is a cleanup you copied from another driver, and this change needs to be propagated to other drivers too?
> if (timeout > 255) > timeout = 255;
IMHO use a constant here instead of magic number 255. maybe use max() or max_t(), too.
> case WDIOC_SETTIMEOUT: > if (get_user(new_timeout, (int *)arg)) > return -EFAULT; > if (new_timeout < 0 || new_timeout > 255) > return -EINVAL;
likewise
> static int sc1200wdt_release(struct inode *inode, struct file *file) > { > lock_kernel(); > > /* Disable it on the way out */ > sc1200wdt_write_data(WDTO, 0); > up(&open_sem); > > unlock_kernel(); > > printk(KERN_INFO PFX "Watchdog disabled\n"); > MOD_DEC_USE_COUNT; > > return 0; > }
are you certain we need lock_kernel(), unlock_kernel() here? especially with a semaphore...
> static struct file_operations sc1200wdt_fops = > { > owner: THIS_MODULE, > write: sc1200wdt_write, > ioctl: sc1200wdt_ioctl, > open: sc1200wdt_open, > release: sc1200wdt_release, > };
I noticed wdt_pci.c implements ->read, too, why not here as well?
> static int __init sc1200wdt_probe(int base_io) > { > /* How can we probe for this thing? */ > return 0; > } > > static int __init sc1200wdt_init(void)
Look at how i810_rng does its PCI probe. [I'm guessing] Surely this SC1200 hardware has _some_ sort of identifier, like a list of commonly found PCI host bridges, that is better than the simple request_region() provided.
Overall, looks good... nice, clean driver.
Jeff
-- Jeff Garzik | "Why is it that attractive girls like you Building 1024 | always seem to have a boyfriend?" MandrakeSoft | "Because I'm a nympho that owns a brewery?" | - BBC TV show "Coupling" - 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/
| |