Re: review MPSC driver

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

Re: review MPSC driver

[Adrian Bunk]

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

Re: review MPSC driver

[Randy.Dunlap]

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

Re: review MPSC driver

[Russell King]

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

Re: review MPSC driver

[Mark A. Greer]

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