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

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

On Wed, August 29, 2007 2:36 am, Mike Frysinger wrote:
> On Tuesday 28 August 2007, Adrian McMenamin wrote:
> general note ... looks like your e-mail client word wrapped things and you
> took the "80 cols" rule a little too seriously (or you just ran Lindent
> and
> didnt check the results) ... imo, breaking across things like pointers to
> structures is excessive:
> kbd->
> old[i]
>
I was a bit OTT on that, it's true. And I'll post the refactored patch as
an attachment next time.

> be nice if more funcs had overview comments, but i'd make that lower
> priority ;)
>

Will add some. A couple of points that clarification would be useful on...


>> --- /dev/null
>> +++ b/drivers/input/keyboard/maple_keyb.c
>> @@ -0,0 +1,210 @@
>> +/*
>> + *	SEGA Dreamcast keyboard driver
>> + *	Based on drivers/usb/usbkbd.c
>> + *	Copyright YAEGASHI Takeshi, 2001
>> + *	Porting to 2.6 Copyright Adrian McMenamin, 2007
>> + *	Licensed under the GPL
>
> you mean GPL-2 [or later] ?
>
Actually, I don't know. The original just said "GPL" and as it was
submitted to the kernel we can assume it's at least v2 but there was no
additional specification given. Shall I just leave it as is?

>> +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,},
>
> that last "}," shouldnt be cuddled like that
>

Really? It's what i've used before and it has been accepted into the kernel.

Should it be
              .probe =      probe_maple_kbd,
              },

then?







>> +/* use init call to ensure bus is registered ahead of devices */
>> +fs_initcall(maple_bus_init);
>
> thought subsys_initcall() was what you wanted ?
>

I thought it would be better behaved to use a later call and as this is fs
related (sys) it seemed appropriate. If this a faux pas I can change it...