Re: [PATCH] drivers/misc: Altera Cyclone active serial implementation

[Greg KH <greg@kroah.com>]

On Mon, Nov 08, 2010 at 08:57:37AM +0200, Baruch Siach wrote:
> Hi Greg,
> 
> On Sat, Nov 06, 2010 at 11:19:30AM -0700, Greg KH wrote:
> > On Wed, Nov 03, 2010 at 04:21:35PM +0200, Baruch Siach wrote:
> > > From: Alex Gershgorin <agersh@rambler.ru>
> > > 
> > > The active serial protocol can be used to program Altera Cyclone FPGA devices.
> > > This driver uses the kernel gpio interface to implement the active serial
> > > protocol.
> > > 
> > > Signed-off-by: Alex Gershgorin <agersh@rambler.ru>
> > > Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> > > ---
> 
> [snip]
> 
> > > +static struct class *cyclone_as_class;
> > 
> > Please don't create your own class just for a single driver.  Just use
> > the misc class interface instead, as all you really want/need here is a
> > character device node, right?
> 
> Searching for 'mist' under include/linux/ I couldn't find this "misc class 
> interface". Can you enlighten me?
> 
> I did find the "miscdevices" interface in include/linux/miscdevice.h.

Yes, that is the one.

> I also tried this one before going on to create a class of my own.
> However, this interface seems to be limited to singleton devices. Can
> I use MISC_DYNAMIC_MINOR to create multiple device nodes?

I didn't see where your code was handling multiple device nodes, is it
really doing that?

> Is there any example for this?

You can just increment the name for the miscdevice to do this based on a
driver-supplied unique number.

> > And as discussed at the Plumbers conference this past week, we don't
> > want to add any new 'struct class' implementations to the kernel from
> > now on, as it's the overall wrong thing to do.
> > 
> > > --- /dev/null
> > > +++ b/include/linux/cyclone_as.h
> > 
> > Why do you need a .h file at all?
> 
> Look at the content of this file. I need to pass GPIO numbers from the 
> platform code under arch/ to the driver. For this I use a platform_data 
> struct, which must be visible to the platform code. Is there a better way to 
> do this?

{sigh}

I'm getting really tired of these kinds of .h files cluttering up the
kernel for an interface that should be easier to handle some other way.

I'm really tempted to create something like:
	include/platform_crap/
to put all of these into.

Ok, that probably will not make people all that happy, so how about:
	include/platform_drivers/

instead?

thanks,

greg k-h