Message-ID: <20080605090916.GA3198@local>
Date:	Thu, 5 Jun 2008 11:09:16 +0200
From:	"Hans J. Koch" <hjk@...utronix.de>
To:	Magnus Damm <magnus.damm@...il.com>
Cc:	"Hans J. Koch" <hjk@...utronix.de>, linux-kernel@...r.kernel.org,
	Uwe.Kleine-Koenig@...i.com, gregkh@...e.de,
	akpm@...ux-foundation.org, lethal@...ux-sh.org, tglx@...utronix.de
Subject: Re: [PATCH] uio_pdrv: Unique IRQ Mode

On Thu, Jun 05, 2008 at 10:25:27AM +0900, Magnus Damm wrote:
> On Wed, Jun 4, 2008 at 7:11 PM, Hans J. Koch <hjk@...utronix.de> wrote:
> > On Wed, Jun 04, 2008 at 03:08:26PM +0900, Magnus Damm wrote:
> >> From: Magnus Damm <damm@...l.co.jp>
> >>
> >> This patch adds a "Unique IRQ Mode" to the uio_pdrv UIO platform driver.
> >> In this mode the user space driver is responsible for acknowledging and
> >> re-enabling the interrupt. Shared interrupts are not supported.
> >
> > I still don't see any gain in this. This only works for embedded
> > devices, so a user has to setup hardware specific code in his board
> > support anyway.
> 
> Exactly what in my patch makes this platform driver only suitable for
> embedded devices?

You assume the interrupt is not shared. You can never do that on a
normal x86 PC, for example. E.g. for a PCI card you don't know which irq
it'll get and if it is shared or not.

> 
> > With your code, we would have to add something like this
> > to the docs:
> >
> > IF you define an irq AND ommit the irq handler THEN we silently add a
> > handler that blindly assumes the irq is not shared...
> 
> I'm not sure if silently and blindly are the first words that pop into
> my mind, but sure. Documentation is a good idea. Just let me know
> which uio_pdrv document you want me to modify.

Stuff that is useful for other driver writers and not only for userspace
people should be added to Documentation/Docbook/uio_howto.tmpl.
Unfortunately, I forgot to ask Uwe to do this, so I'll probably have to
do it myself one day ;-)

> 
> > In my opinion, this is confusing, and all it does is saving the need for a
> > three-lines irq handler in the board support.
> 
> You propose that I put the callbacks in my board support code instead
> of modifying the driver.

That's what uio_pdrv does.

> I don't think the board support level is the
> proper place for this code.

You have to write code there anyway, e.g. code that configures your GPIO
as input, makes it generate interrupts and so on. And of course, you
have to setup your platform device as well. If you simply add the irq
handler, you can use uio_pdrv as-is. And if you _know_ that on your
platform the irq is not shared, this might really be a one-liner that
simply calls irq_disable. That's OK in board specific code, but not in a
generic driver.

> The patch contains no board specific code,
> and it is independent of both architecture and cpu model.

Every platform device driver depends on board support.

> 
> > So, NAK to this until somebody convinces me that I completely missed the
> > point.
> 
> We can reuse this driver for _many_ different SuperH processor models.
> Most of these processor models even have more than one hardware block
> that can be exported to user space using this uio_pdrv driver in
> "Unique IRQ Mode". There is nothing board specific with this at all,
> so yes, I think you are missing the point.

First, I won't accept anything that changes the current UIO behaviour.
If uio_info->irq is not set, then uio_register_device will fail. That's
it. Your patch redefines the meaning of irq-not-set if uio_pdrv is used.

Second, if we decide to introduce new UIO capabilities, there must be an
obvious advantage. E.g., uio_pdrv saves the trouble of writing almost
identical UIO platform drivers over and over again. A programmer only
needs to touch the board support file he has to set up anyway which
makes his board support patches simpler and easier to maintain. Your
patch adds code that is not obvious, just to save a few lines of board
support code. It doesn't add new possibilities, everything can be done
with uio_pdrv alone.

> 
> Maybe you prefer that I repost my "Reusable UIO Platform Driver"
> instead of modifying uio_pdrv?

If you have an idea that adds new possibilities, feel free to post it.
If you come up with something that just adds confusion but no
advantages, I'll never accept it.

Thanks,
Hans

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists