| 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
On Wed, Sep 15, 2004 at 09:43:01PM -0700, Randy.Dunlap wrote:
>
> | http://www.uwsg.iu.edu/hypermail/linux/kernel/0408.3/1549.html
>
> Hi Mark,
>...
> 3. + select SERIAL_CORE
> + select SERIAL_CORE_CONSOLE
>
> Please don't use "select". Use "depends on" instead.
>...
That's a silly suggestion since none of these options are user visible.
> ~Randy
cu
Adrian
--
"Is there not promise of rain?" Ling Tan asked suddenly out
of the darkness. There had been need of rain for many days.
"Only a promise," Lao Er said.
Pearl S. Buck - Dragon Seed
On Wed, Sep 15, 2004 at 09:43:01PM -0700, Randy.Dunlap wrote:
> 3. + select SERIAL_CORE
> + select SERIAL_CORE_CONSOLE
>
> Please don't use "select". Use "depends on" instead.
This is actually (one of the few) correct uses of select. These two
symbols are *not* user visible, and are derived from the configuration
settings of the hardware drivers.
--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of: 2.6 PCMCIA - http://pcmcia.arm.linux.org.uk/
2.6 Serial core
On Thu, 16 Sep 2004 10:10:17 +0200 Adrian Bunk wrote:
| On Wed, Sep 15, 2004 at 09:43:01PM -0700, Randy.Dunlap wrote:
| >
| > | http://www.uwsg.iu.edu/hypermail/linux/kernel/0408.3/1549.html
| >
| > Hi Mark,
| >...
| > 3. + select SERIAL_CORE
| > + select SERIAL_CORE_CONSOLE
| >
| > Please don't use "select". Use "depends on" instead.
| >...
|
| That's a silly suggestion since none of these options are user visible.
and Russell King wrote about the same text:
| This is actually (one of the few) correct uses of select. These two
| symbols are *not* user visible, and are derived from the configuration
| settings of the hardware drivers.
Thanks for your comments. Any other comments on the driver?
--
~Randy
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
<[email protected]>, 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