Re: review MPSC driver

[Randy.Dunlap]

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