From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dmitry Torokhov Subject: Re: [PATCH] input: add support for SEGA Dreamcast controller as joystick Date: Mon, 19 May 2008 11:47:08 -0400 Message-ID: <20080519114515.ZZRA012@mailhub.coreip.homeip.net> References: <1211138322.6354.5.camel@localhost.localdomain> <20080519010928.GA8797@linux-sh.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Received: from py-out-1112.google.com ([64.233.166.176]:50986 "EHLO py-out-1112.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1761989AbYESPrz (ORCPT ); Mon, 19 May 2008 11:47:55 -0400 Received: by py-out-1112.google.com with SMTP id u52so1942234pyb.10 for ; Mon, 19 May 2008 08:47:54 -0700 (PDT) Content-Disposition: inline In-Reply-To: <20080519010928.GA8797@linux-sh.org> Sender: linux-input-owner@vger.kernel.org List-Id: linux-input@vger.kernel.org To: Paul Mundt , Adrian McMenamin , LKML , Andrew Morton , linux-sh 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 "); > > +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