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

[Dmitry Torokhov]

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:
> > +MODULE_AUTHOR("YAEGASHI Takeshi <t@xxxxxxxxx>");
> > +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.
> 
> > +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.
> 
> > +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.
> 
> > +		.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/