![]() | |||||||||||
Messages in this thread |
| 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 | |||||||||||