Messages in this thread | | | Date | Thu, 5 Jun 2008 18:46:35 +0900 | From | "Magnus Damm" <> | Subject | Re: [PATCH] uio_pdrv: Unique IRQ Mode |
| |
On Thu, Jun 5, 2008 at 6:09 PM, Hans J. Koch <hjk@linutronix.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@linutronix.de> wrote: >> > On Wed, Jun 04, 2008 at 03:08:26PM +0900, Magnus Damm wrote: >> >> From: Magnus Damm <damm@igel.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?
>> > 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? Not all platform devices need board specific setup.
>> 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.
>> > 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. And yet you seem to have very strong feelings against this patch.
> 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.
/ magnus
| |