lkml.org 
[lkml]   [2004]   [Sep]   [16]   [last100]   RSS Feed
Views: [more markup]  [less markup]  [headers]  [forward] 
 
Messages in this thread
/
DateWed, 15 Sep 2004 21:43:01 -0700
From"Randy.Dunlap" <>
SubjectRe: review MPSC driver
| http://www.uwsg.iu.edu/hypermail/linux/kernel/0408.3/1549.html

Hi Mark,

1.  Do you realize that a version of the driver is already in the -mm
patchset?

2.  + depends on PPC32 && MV64X60

Where is MV64X60 defined?

3.  + select SERIAL_CORE
    + select SERIAL_CORE_CONSOLE

Please don't use "select".  Use "depends on" instead.

4.  + * Author: Mark A. Greer <mgreer@xxxxxxxxxx>
Put a real email address or remove it.

5.  +#include <asm/mv64x60.h>

Where is this file?  Does this driver build?

6.  style:
+	if (pi->brg_can_tune) {
+		MPSC_MOD_FIELD_M(pi, brg, BRG_BCR, 1, 25, 0);
+	}
Has unneeded braces (in several places).
7.  style:
+	return;
+}
Lots of void functions with "return" that is not needed.
8.  Why use 'volatile' here?  Have you read the Linus volatile rant?

+static inline void
+mpsc_sdma_set_tx_ring(struct mpsc_port_info *pi,
+		      volatile struct mpsc_tx_desc *txre_p)
+{
9.  put in mpsc.h:
+	static void mpsc_free_ring_mem(struct mpsc_port_info *pi);
+	static void mpsc_start_rx(struct mpsc_port_info *pi);

10. in the interrupt handler, if rx happened, tx intr not checked.
Is that intentional?

+	if (mpsc_rx_intr(pi, regs) || mpsc_tx_intr(pi))
+		rc = IRQ_HANDLED;
11. In mpsc_verify_port(), if -EINVAL is ever set, the others
are wasted checks.

12. What's the rationale for having both mpsc_console_init() and
mpsc_late_console_init() ?

13. register_serial() and unregister_serial():  names are a bit too
generic -- they sound like serial subsystem functions.
and why are they exported?  what else uses them?

14. mpsc.h:  don't define MIN(), #include <linux/kernel.h> and use
its min() macro.

15. Run it thru sparse for warnings.

--
~Randy
-
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/

\
 
 \ /
  Last update: 2005-03-22 14:06    [from the cache]
©2003-2008