Re: [PATCH] Add support for generic maple bus and for maple bus keyboard

[Adrian M. <ad...@ne...>]

On Thu, 2007-08-30 at 15:39 +0900, Paul Mundt wrote:
> On Tue, Aug 28, 2007 at 11:14:00PM +0100, Adrian McMenamin wrote:
> > Please find this patch below - they said it would never be done!
> > 
> > Anyway, this adds new maplebus support to the kernel and a maple
> > keyboard driver is also attached. There a couple of minor and
> > consequential patches to other kernel subsystems attached that are
> > required to get this to work (eg make the PVR2 VBLANK interrupt
> > shareable).
> > 
> It would be nice if you could split this up at least, so there's the PVR2
> thing by itself, the patch adding the bus, and then the input driver
> afterwards.
> 
> 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...


> > +static struct maple_driver dc_kbd_driver = {
> > +	.function = 	MAPLE_FUNC_KEYBOARD,
> > +	.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?


> > diff --git a/drivers/sh/maple/Makefile b/drivers/sh/maple/Makefile
> > new file mode 100644
> > index 0000000..f8c39f2
> > --- /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...

> > diff --git a/drivers/sh/maple/maplebus.c b/drivers/sh/maple/maplebus.c
> > new file mode 100644
> > index 0000000..fc27543
> > --- /dev/null
> > +++ b/drivers/sh/maple/maplebus.c
> > @@ -0,0 +1,641 @@
> > +/* 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!



> > +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.



> > +	function = be32_to_cpu(dev->devinfo.function);
> > +
> What exactly is the point of this? The function is big endian, really?
> 

Err, yes.


> to 1/0.
> 
> > +static int maple_bus_uevent(struct device *dev, char **envp,
> > +			    int num_envp, char *buffer, int buffer_size)
> > +{
> > +	return 0;
> > +}
> 
> How are you planning on notifying userspace when a device is
> plugged/unplugged?
> 

Device model takes care of that ... the above is pretty redundant and
should go.


> > +
> > +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.



> 
> > diff --git a/include/linux/input.h b/include/linux/input.h
> > index e02c6a6..2e65c88 100644
> > --- a/include/linux/input.h
> > +++ b/include/linux/input.h
> > @@ -688,6 +688,7 @@ struct input_absinfo {
> >  #define BUS_HOST		0x19
> >  #define BUS_GSC			0x1A
> >  #define BUS_ATARI		0x1B
> > +#define BUS_MAPLE		0x1C
> >  
> Do we really need to still do this these days?
> 

I believe so, yes.

> > +/* Maple Bus command and response codes */
> > +/* Function codes */
> 
> These can use enums and opaque types.
> 

These are real numbers though.


> > +struct mapleq {

> > +struct maple_devinfo {
> [snip]
> 
> > +	char product_licence[60];
> 
> British variable names, if there's nothing in CodingStyle about that,
> there should be :-)

Not being British, what ever my passport says, I wouldn't know :)