Re: [PATCH 1/2] Maple Bus support for SEGA Dreamcast

[Adrian McMenamin <adrian@newgolddream.dyndns.info>]

On Tue, 2007-09-11 at 13:27 +0900, Paul Mundt wrote:
> (Adding GregKH to the CC, he needs to Ack this before I can merge it).
> 

> > diff --git a/drivers/sh/maple/maplebus.c b/drivers/sh/maple/maplebus.c
> > new file mode 100644
> > index 0000000..4662463
> > --- /dev/null
> > +++ b/drivers/sh/maple/maplebus.c
> > @@ -0,0 +1,747 @@
> > +/* maplebus.c
> > + * Core maple bus functionality
> > + * Original 2.4 code used here copyright
> > + * YAEGASHI Takeshi, Paul Mundt, M. R. Brown and others
> > + * Porting to 2.6 Copyright Adrian McMenamin, 2007
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + *
> I don't believe the 'or any later version' thing was intended by any of
> the original authors, this should probably be dropped. The original file
> lacked explicit terms, so the default behaviour there is to fall back to
> what the top-level COPYING says, which has the 'or any later version'
> garbage removed.

On 30th of August you wrote this in an email to the linuxsh list:

"Both Marcus and I intended all of our code to be v2 only, so that
should probably be statically defined here. However, I believe
Yaegashi-san wrote a lot of this initial version, and I'm sure there
were others that hacked on it as well. So we may simply have to leave it
as GPLv2 with the unfortunate + any later version garbage."

I think you were right the first time.



> 
> > +static DEFINE_MUTEX(maple_list_lock);
> > +
> > +DEFINE_MUTEX(maple_dma_lock);
> > +EXPORT_SYMBOL_GPL(maple_dma_lock);
> > +
> It would be better to have accessors for this rather than exporting the
> mutex globally.


Actually the mutex is accessed in the aica code so it, or any wrappers
need to be exported
> 


> 
> > +/* use init call to ensure bus is registered ahead of devices */
> > +fs_initcall(maple_bus_init);
> 
> Please use subsys_initcall() or something like that, this isn't a file
> system. The comment is also pointless, it's obvious what the initcall is
> for if you use the proper one.
> 

Except performance of the driver is very poor when I use subsys_initcall
- detecting devices on a phase-of-the-noon related basis. Not an issue
when I use the later call.


> > index 0000000..8a37c7e
> > --- /dev/null
> > +++ b/include/linux/maple.h
> > @@ -0,0 +1,126 @@
> > +/**
> > + * maple.h


> 
> > +void maple_setup_port_rescan(struct maple_device *mdev);
> > +
> > +void maplebus_init_hardware(void);
> > +
> Why would these ever be accessible to drivers?
> 
You are right about the rescan - a relic of an earlier iteration but the
maplebus_init_hardware is used to control the DMA in the aica code