On Thu, 22 Jun 2006 09:20:28 -0400
"Straub, Michael" <[email protected]> wrote:
> Adds Equinox multi-port serial (SST) driver.
>
> Part 9: new source file: drivers/char/eqnx/sst_tty.c. Provides the
> standard
> TTY interface routines for the SST driver.
>
> ...
>
> +/* defer call to write_wakeup */
> +int eqnx_write_wakeup_deferred = 0;
Please don't initilise static storage to zero - the C runtime environment
does that. <entire patchset>
> +#define MY_GROUP() (process_group(current))
Remove this, open-code it at the callsite.
> +extern int SSTMINOR(unsigned int, unsigned int);
> +/**********************************************************************
> *****/
> +/* external variable and routines
> */
> +/**********************************************************************
> *****/
> +
> +extern int eqnx_nssps;
> +extern struct mpdev eqnx_dev[MAXSSP];
> +extern struct mpchan *eqnx_chan;
> +extern struct datascope eqnx_dscope[2];
> +extern char *eqnx_tmpwritebuf;
> +extern struct semaphore eqnx_tmpwritesem;
> +
> +extern void eqnx_megaparam(int);
> +extern int eqnx_modem(int, int);
> +extern void eqnx_frame_wait(struct mpchan *, int);
> +extern void eqnx_chnl_sync(struct mpchan *);
> +extern int SSTMINOR(unsigned int, unsigned int);
> +extern u32 eqnx_get_modem_info(struct mpchan *);
> +extern void eqnx_set_modem_info(struct mpchan *, unsigned int, unsigned
> int,
> + struct tty_struct *);
In all patches, move all extern declarations to header files which are
shared by both the declaration site and by all users.
> +
> + return (-ENODEV);
I'd second previous comments wrt overparenthesization.
> + mpc->refcount++;
WHat's the locking for this field?
> + mpc->openwaitcnt++;
Ditto.
> + if (!page)
> + return (-ENOMEM);
Please try to make all non-trivial functions have a single `return'
statement. This driver like to sprinkle returns all over the place, and
that's an invitation to missed unlocks, resource leaks and duplicated
error-recovery code. We use gotos for this.
> + mpc->pgrp = MY_GROUP();
Why does this driver need to know about process groups?
> +void eqnx_close(struct tty_struct *tty, struct file *filp)
> +{
> + struct mpchan *mpc;
> + struct mpdev *mpd;
> + int nchan, d;
> + unsigned long flags;
> + u16 attn;
> +
> + /* channel validity checks */
> + if (tty == (struct tty_struct *)NULL)
> + return;
> + mpc = (struct mpchan *)tty->driver_data;
> + if (mpc == (struct mpchan *)NULL)
> + return;
Please review the whole driver for unneeded typecasts of void*, remove them.
> + (unsigned int)SSTMINOR(mpc->mpc_major, mpc->mpc_minor),
> count);
Remove SSTMINOR, open-code it.
> +/*
> + * delay_jiffies(len)
> + *
> + * Delay by the specified number of jiffies.
> + *
> + * len = jiffies to delay.
> + */
> +static void delay_jiffies(int len)
> +{
> + if (len > 0) {
> + msleep(jiffies_to_msecs(len));
> + }
> +}
It's unusual for a driver to want to sleep for a number of jiffies.
Because HZ jiffies can vary by an order of magnitude. msleep() is a better
interface to use.
This function is an obfuscated version of schedule_timeout_uninterruptible().
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
