lkml.org 
[lkml]   [2004]   [Sep]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: review MPSC driver
Randy.Dunlap wrote:

>| http://www.uwsg.iu.edu/hypermail/linux/kernel/0408.3/1549.html
>
>Hi Mark,
>

Hi Randy. Thanks for putting time into reviewing and commenting on this.

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

Ahh, no... Hmm, I guess someone did notice the patch I sent after all. :)

>2. + depends on PPC32 && MV64X60
>
>Where is MV64X60 defined?
>

There is a separate patch that provides "core" support for the
gt/mv64x60 host bridges (which the MPSC is on). I haven't provided the
patch yet b/c I keep finding little bugs in it. For modularity and to
keep patch sizes smaller, I'm submitting separate patches for the core
and for the mpsc driver. I had to draw the line somewhere between the
driver and the core. It so happened that this file fell on the other
side of that line. So, no, it won't compile until the core patch is
submitted and accepted. However, if no one selects that driver--which
they probably won't since the core support isn't there anyway--no harm
done. I sent the mpsc driver patch before the core patch because it was
more-or-less ready and so I could get the ball rolling WRT getting it
accepted.

>
>3. + select SERIAL_CORE
> + select SERIAL_CORE_CONSOLE
>
>Please don't use "select". Use "depends on" instead.
>

Already discussed.

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

Odd... It looks like the archive tried to do me a favor and "x-out" my
domain so I don't get spam (I guess). The original patch has
<mgreer@mvista.com>, and that's reflected in the -mm patch and in other
archives (e.g., http://lkml.org/lkml/2004/8/27/305). I should have
pointed you to the lkml.org link instead.

>5. +#include <asm/mv64x60.h>
>
>Where is this file? Does this driver build?
>

See comments on 2.

>6. style:
>+ if (pi->brg_can_tune) {
>+ MPSC_MOD_FIELD_M(pi, brg, BRG_BCR, 1, 25, 0);
>+ }
>Has unneeded braces (in several places).
>

I will fix.

>7. style:
>+ return;
>+}
>Lots of void functions with "return" that is not needed.
>

Well, I didn't notice anything in Documentation/CodingStyle on this so I
assumed it was up to me. If there is a policy on this, I'll remove
them. If its up to me, I'd prefer that they be left in--redundant, yes,
but somehow I like the clarity.

>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)
>+{
>

I have heard about it but I've not actually read it. I will read it and
change whatever needs to be.

>
>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);
>

I didn't put them in mpsc.h because they're never referenced outside of
mpsc.c. I will move them unless someone else objects.

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

Good point. I should have caught that one.

>11. In mpsc_verify_port(), if -EINVAL is ever set, the others
>are wasted checks.
>

Another good point.

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

I modeled that after the 8250 driver. I am not intimately familiar with
all of the char/tty/serial infrastructure so I erred on the safe side
and did what was done in the 8250 driver. I will look deeper to see if
I can find a reason.

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

AFAICT, they are standard interfaces for "add-on" files to call into
"core" files like 8250_pci.c calls 8250.c's register_serial(). Since
the mpsc doesn't really have any add-on files, I believe that I can get
rid of them.

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

Yep, already pointed out to me by Russell. Will be fixed.

>15. Run it thru sparse for warnings.
>
>

Will do.

BTW, I'm waiting on a minor number (for major 204) from lanana so I
won't post a new patch until I get that (or I get no response in which
case, I guess I'm forced to pick one).

Thanks again,

Mark

-
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    [W:3.644 / U:0.008 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site