Re: review MPSC driver
From: Randy.Dunlap
Date: Wed Sep 15 2004 - 23:47:52 EST
| 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/