From: Paul M. <le...@li...> - 2007-08-30 07:20:22
|
On Thu, Aug 30, 2007 at 08:09:21AM +0100, Adrian McMenamin wrote: > On Thu, 2007-08-30 at 15:39 +0900, Paul Mundt wrote: > > In the long run these patches will have to be submitted through different > > maintainers, so the earlier they are split logically, the better. > > I will have a stab at doing this. More generally, a lot of your comments > are directed at code I inherited and I'll look at fixing them too. One > or two other things though... > The origin of the code in question has little to do with what needs to be fixed. A lot of it was crap from the beginning, that doesn't mean it doesn't have to be fixed up before it can be merged. > > > + .connect = dc_kbd_connect, > > > + .disconnect = dc_kbd_disconnect, > > > + .drv = { > > > + .name = "Dreamcast_keyboard", > > > + .bus = &maple_bus_type, > > > + .probe = probe_maple_kbd,}, > > > +}; > > > + > > I'm a bit confused as to why you don't have a registration function for a > > struct maple_driver that automatically sets the bus type? This is the > > convention most of the busses use. > > > > I'm not sure I follow what you mean - are you saying I shouldn't > initialise it with the bus type set? > Yes, you should have a maple registration function that sets the bus type for you. Most of the other busses do this. > > > --- /dev/null > > > +++ b/drivers/sh/maple/Makefile > > > @@ -0,0 +1,3 @@ > > > +#Makefile for Maple Bus > > > + > > > +obj-y := maplebus.o > > > > No module goodness? > > I can do it as a module I suppose... > Well, it can be a tristate, so obj-y is artificially limiting. > > > +/* maplebus.c > > > + * Core maple bus functionality > > > + * Core code copyright > > > + * Paul Mundt, M. R. Brown and others > > > + * Porting to 2.6 Copyright Adrian McMenamin, 2007 > > > + * Licence: GNU GPL version 2 > > > + * http://www.gnu.org > > > + */ > > > + > > 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. > > Yes, and none of you left any copyright messages. Very poor show! > I suppose no one wanted to be associated too closely with that particular bit of code.. at least that's the excuse I'm sticking with. ;-) This is probably something you can grovel through the CVS history for, though. > > > +void maple_add_packet(struct mapleq *mq) > > > +{ > > > + unsigned long flags; > > > + > > > + local_irq_save(flags); > > > + list_add((struct list_head *) mq, &maple_waitq); > > > + local_irq_restore(flags); > > > +} > > > + > > This can use some proper locking, and the cast is pointless. mq should > > have a list_head in it somewhere that gets passed in rather than > > expecting the cast to work out. > > Actually, I'm not sure the locking is needed anymore at all - I > inherited that. But assuming a kernel thread will not be scheduled > against itself (am I safe to do that?), I cannot see how this could > executed concurrently with other code that manipulates the queue. > Yes, it should be safe to drop the irq disabling around this. Remember you also have safe iterators if you are concerned about something disappearing from the list while you are looping over it. > > > + > > > +static void maple_bus_release(struct device *dev) > > > +{ > > > + printk(KERN_INFO "Releasing maple bus device\n"); > > > +} > > > + > > Kill this. > > > > I thought busses where required to have them ... that's what my book > says. Oh well. > The release function, yes, not the printk(). > > > +/* Maple Bus command and response codes */ > > > +/* Function codes */ > > > > These can use enums and opaque types. > > These are real numbers though. > As are enums. enums are also generally more readable for these sorts of things, too. |