Messages in this thread | | | Date | Tue, 18 Feb 2003 10:45:47 +0000 | From | Christoph Hellwig <> | Subject | Re: [PATCHSET] PC-9800 subarch. support for 2.5.61 (5/26) char device |
| |
> +static struct lp_struct lp = { > + /* Following `TAG: INITIALIZER' notations are GNU CC extension. */
This comment doesn't make sense anymore :)
> + .flags = LP_EXIST | LP_ABORTOPEN, > + .chars = LP_INIT_CHAR, > + .time = LP_INIT_TIME, > + .wait = LP_INIT_WAIT, > +}; > + > +static int dc1_check = 0; > +static spinlock_t lp_old98_lock = SPIN_LOCK_UNLOCKED; > + > + > +#undef LP_OLD98_DEBUG > + > +#ifdef CONFIG_PC9800_OLDLP_CONSOLE > +static struct console lp_old98_console; /* defined later */ > +static __typeof__(lp_old98_console.flags) saved_console_flags;
Please directly use the actual type here.
> +#endif > + > +static DECLARE_WAIT_QUEUE_HEAD (lp_old98_waitq); > + > +static void lp_old98_timer_function(unsigned long data);
This prototype is superflous.
> + __const_udelay(lp.wait * 4);
Why do you use __const_udelay instead of udelay?
> +#if LINUX_VERSION_CODE < 0x20200 > +static long lp_old98_write(struct inode * inode, struct file * file, > + const char * buf, unsigned long count) > +#else > +static ssize_t lp_old98_write(struct file * file, > + const char * buf, size_t count, > + loff_t *dummy) > +#endif
Do you really need that compat code? I don't think it makes much sense to keep 2.0 code around in 2.5.
> +static int lp_old98_open(struct inode * inode, struct file * file) > +{ > + if (minor(inode->i_rdev) != 0) > + return -ENXIO; > + > + if (!try_module_get(THIS_MODULE)) > + return -EBUSY;
This is broken - the upper layer does this for you swhen you set the owner fild of struct file_operations.
> + module_put(THIS_MODULE);
Dito.
> +static struct file_operations lp_old98_fops = { > + .owner = THIS_MODULE,
See, you already set it..
> + .llseek = no_llseek, > + .read = NULL,
Remove this line.
> + if (request_region(LP_PORT_DATA, 1, "lp_old98")) { > + if (request_region(LP_PORT_STATUS, 1, "lp_old98")) { > + if (request_region(LP_PORT_STROBE, 1, "lp_old98")) { > + if (request_region(LP_PORT_EXTMODE, 1, "lp_old98")) { > + if (register_chrdev(LP_MAJOR, "lp", &lp_old98_fops)) {
Using gotos for error handling here might make the code quite a bit more readable :)
- 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/
| |