Messages in this thread | | | Date | Wed, 12 Feb 2003 14:43:07 +0000 | From | Christoph Hellwig <> | Subject | Re: [PATCHSET] PC-9800 subarch. support for 2.5.60 (3/34) AC#3 |
| |
On Wed, Feb 12, 2003 at 10:25:49PM +0900, Osamu Tomita wrote: > +ifneq ($(CONFIG_PC9800),y) > obj-$(CONFIG_BLK_DEV_FD) += floppy.o > +else > +obj-$(CONFIG_BLK_DEV_FD) += floppy98.o > +endif
Please use a different config option for your floppy driver, i.e. CONFIG_BLK_DEV_FD98
> +#if !defined(CONFIG_PC9800) && !defined(CONFIG_PC98) > +#error This driver works only for NEC PC-9800 series > +#endif
this shoiuld be handled by the config system..
> + > +#if LINUX_VERSION_CODE < 0x20200 > +# define LP_STATS > +#endif > + > +#if LINUX_VERSION_CODE >= 0x2030b > +# define CONFIG_RESOURCE98 > +#endif
it would be nice if you could get rid of those obsolete version ifdefs for a new port..
> + /* Following `TAG: INITIALIZER' notations are GNU CC extension. */ > + flags: LP_EXIST | LP_ABORTOPEN, > + chars: LP_INIT_CHAR, > + time: LP_INIT_TIME, > + wait: LP_INIT_WAIT, > +};
please use C99-style initializers here.
> +static inline void nanodelay(unsigned long nanosecs) /* Evil ? */
yes.
> +static long long lp_old98_llseek(struct file * file, > + long long offset, int whence) > +{ > + return -ESPIPE; /* cannot seek like pipe */ > +}
use no_llseek instead.
> + unsigned long eflags; > + > + save_flags(eflags); > + cli(); /* interrupts while check is fairly bad */
use proper spinlocking.
> + > + if (!lp_old98_char(DC1)) { > + restore_flags(eflags); > + return -EBUSY;
you don't free an buffer allocated above here
> +static int lp_old98_release(struct inode * inode, struct file * file) > +{ > + kfree(lp.lp_buffer); > + lp.lp_buffer = NULL; > + lp.flags &= ~LP_BUSY; > +#ifdef CONFIG_PC9800_OLDLP_CONSOLE > + lp_old98_console.flags = saved_console_flags; > +#endif > + MOD_DEC_USE_COUNT;
please use fops.owner based refcounting instead.
> +/* > + * Many use of `put_user' macro enlarge code size... > + */ > +static /* not inline */ int lp_old98_put_user(int val, int *addr) > +{ > + return put_user(val, addr); > +}
umm..
> +#ifdef MODULE > +#define lp_old98_init init_module > +#endif > + > +int __init lp_old98_init(void)
use module_init/module_exit instead.
> + if (check_region(LP_PORT_DATA, 1) || check_region(LP_PORT_STATUS, 1) > + || check_region(LP_PORT_STROBE, 1) > +#ifdef PC98_HW_H98 > + || ((pc98_hw_flags & PC98_HW_H98) > + && check_region(LP_PORT_H98MODE, 1)) > +#endif > + || check_region(LP_PORT_EXTMODE, 1)) { > + printk(KERN_ERR > + "lp_old98: I/O ports already occupied, giving up.\n"); > + return -EBUSY; > + }
check_region is obsolete, use the return value from request_region instead.
> +static long long rtc_llseek(struct file *file, loff_t offset, int origin) > +{ > + return -ESPIPE; > +}
again, use no_llseek
> +EXPORT_NO_SYMBOLS;
EXPORT_NO_SYMBOLS is obsolete, don't use it in 2.5.
- 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/
| |