Re: [PATCH] drivers: staging: Add driver for Amplicon PCIe215

[Piotr Gregor]

On Fri, Sep 01, 2017 at 08:57:50AM +0200, Greg Kroah-Hartman wrote:
> On Thu, Aug 31, 2017 at 05:54:58PM +0100, Piotr Gregor wrote:
> > This is a small and simple driver for handling of external
> > interrupt signal asserted on pins of Amplicon's PCIe215 board.
> > There is already a Comedi driver subsystem in kernel which handles
> > that (and more) board, but that framework while offering more
> > flexibility brings also additional complexity at the cost of being generic.
> > 
> > In some cases the simpler, more compact solution may be preferred.
> 
> Note, having different drivers for the same hardware platform in the
> kernel tree is a mess, and generally discouraged.  I've handled this in
> the past and we hated every minute of it.

Agree.

> 
> What is wrong with the comedi interface instead?

For a user who needs only the interruption, this driver is preferred.
The reason is that using Comedi framework requires knowledge of Comedi
framework. There is no reason user should spend time learning additional
software if what he needs can be achieved in a simpler way.
Comedi allows you to do many things, but this means you must tell Comedi what
is what you want. If you want to use Comedi for interrupt you end up learning
how to configure Comedi so it can setup the driver, so that the driver will
setup the board correctly. Once you know this, you have plenty of Comedi
related code in your program and you are dependent on Comedi drivers
and Comedi interface to them. It also means that after handling
interrupt in kernel, the code control path visits plenty of Comedi
related logic, before it uderstands what it is configured to do now and
returns to your user space program.
Using this driver means insmod followed by 2 ioctl to enable
interrupt generation and set up enabled pins. I tried to make it return
to user space as quickly as possible after the ISR is entered.

> 
> And custom ioctls for a single hardware device is horrid, what's wrong
> with using the uio interface for something like this?
> 

You are right. This would be something TODO.

> And finally, why staging?  While I know I wouldn't accept merging this
> into the main portion of the kernel, why do you feel that adding this to
> drivers/staging/ is ok?  What's so wrong with it (well, becides the
> previous questions), that dumping it here is the properly location?

There is nothing wrong, probably it should be submitted there.
I thought it would be good to know your thoughts on this
and improve things like ioctl interface if needed.

> 
> > The purpose of this driver is therefore to handle interrupt
> > feature of the PCIe215, while being small, simple and reliable.
> 
> Why isn't comedi reliable?

It isn't small and simple. This driver implements interrupt handling
simpler and more efficiently. There is ftrace support in it, that's
because I tested latency of wake up introduced by this driver
and compared to Comedi framework. The result was 19.31 us on average
for Comedi, and it was few microsecods less for this driver.
It was also just recently when we found a bug on rt kernel 4.4.70 in Comedi
code related to sleep/wake up. If it didn't aim to give all that flexibility
this bug would not be there. Nothing bad happened, bug was fixed, but it
shows how you are dependent on unrelated things you shouldn't have
to worry about.

> 
> thanks,
> 
> greg k-h

Thank you,
Piotr