Message-ID: <20080605112744.GB3198@local>
Date:	Thu, 5 Jun 2008 13:27:44 +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 06:46:35PM +0900, Magnus Damm wrote:
> On Thu, Jun 5, 2008 at 6:09 PM, Hans J. Koch <hjk@...utronix.de> wrote:
> > 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.
> 
> So your main objection against this patch is that you cannot use it
> with shared interrupts?

I think I've explained my objections detailed enough.

> 
> >> > 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 ;-)
> 
> Right.
> 
> >> > 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.
> 
> Yes.
> 
> >> 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.
> 
> Ever heard about system on chip?

ATM, I work with iMX31 and AT91SAM9263, before that I had a PXA270,
can't remember what was before that...
So yes, I've heard of SoC.

> Not all platform devices need board
> specific setup.

If it's a device within the SoC, you won't use UIO for that. If you did,
your platform would depend on certain userspace software which is simply
crap. And devices outside the SoC are board specific.

> 
> >> 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.
> 
> Is that so? I suggest that you have a look at the mfd drivers and think again.

All I said about board support also applies to platform support files
like at91sam9263_devices.c, I'm simply talking about the file where you
define your struct platform_device.

> 
> >> > 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.
> 
> How is this changing the UIO behavior? I'm modifying the uio_pdrv
> driver, which is a driver that you didn't even write yourself.

uio_pdrv is a generic driver, so I consider it part of the UIO
framework. It adds new possibilities for authors of UIO platform device
drivers (which are the vast majority of all UIO drivers). It is not just
another UIO driver, but part of the system. It'll appear in UIO
documentation, I'll explain it in future UIO presentations, and so on.

And I consider it my job to make sure that such generic code is clean,
obvious, and consistent.

> And yet
> you seem to have very strong feelings against this patch.

I explained why. My reasons are purely technical, please don't take this
as a personal offense.

> 
> > 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.
> 
> Again, it's not board support code. And it's pretty obvious.
> 
> >> 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.
> 
> You don't have to accept it. I'll just repost my original driver and
> let someone else merge it then.

Unfortunately, I'm one of the two UIO maintainers, so I feel obliged to
review your patch and give my opinion. That doesn't mean I'm
the big boss who makes the final decision. I can be critized and
overridden. If Greg loves your patch and merges it, fine. Try 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