Re: [PATCHSET] PC-9800 subarch. support for 2.5.60 (3/34) AC#3

Christoph Hellwig (hch@infradead.org)
Wed, 12 Feb 2003 14:43:07 +0000


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/