Re: [PATCH] input: add support for SEGA Dreamcast controller as joystick

On Mon, May 19, 2008 at 10:09:28AM +0900, Paul Mundt wrote:

On Sun, May 18, 2008 at 08:18:42PM +0100, Adrian McMenamin wrote:

quoted

+MODULE_AUTHOR("YAEGASHI Takeshi [off-list ref]");
+MODULE_DESCRIPTION("SEGA Dreamcast controller driver");
+MODULE_LICENSE("GPL");
+

MODULE_AUTHOR() is a bit ambiguous here. As this is user-visible by way
of modinfo, it really doesn't make any sense to list someone who has
nothing to do with the driver in its present form here. Especially since
it's been 7 years since their last contribution or even basic list
activity, I think it's safe to say that you are going to be the only one
that users wish to contact about this.

In this case, the copyright should suffice. You may wish to add a bit of
a note there indicating where the driver came from, but there's not much
point in going beyond that.

Note that if you aren't comfortable with being in MODULE_AUTHOR(), you
can also make this more generic and attribute it to the LinuxDC or
Linux/SH teams respectively, as that's a bit more true to form as far as
all copyright holders are concerned, and gives people a pointer to a list
to go to with problems.

It's probably more reasonable to treat MODULE_AUTHOR() as
MODULE_MAINTAINER() rather than the literal original author, as the
latter is of zero interest or relevance to users that are looking at
this information on the other side of the fence.

quoted

+struct dc_pad {
+	struct input_dev *dev;
+	struct maple_device *mdev;
+};
+

This is pretty ugly, you should really fix up your private device
pointer layering to get around having to use this structure at all.

quoted

+static struct maple_driver dc_pad_driver = {
+	.function =	MAPLE_FUNC_CONTROLLER,
+	.connect =	dc_pad_connect,
+	.disconnect =	dc_pad_disconnect,
+	.drv = {
+		.name = "Dreamcast_controller",

Is there some particular reason why the driver name can't reflect the
module name? There's no reason to have such sickeningly verbose driver
names.

quoted

+		.probe = probe_maple_controller,
+		.remove = remove_maple_controller,
+	},
+};
+

These should be __devinit/__devexit at least, with the latter being
__devexit_p() wrapped..

In addition to Pauls comments - is there a reson why maple drivers
need to brovide both connect/disconnect and probe/remove functions?
Normally the layer has generic probe/remove defined on bus level and
individual drivers provide connect/disconnect (or their equivalent)
which is called from the bus's probe/remove.

-- 
Dmitry