Re: [PATCH] uio_pdrv: Unique IRQ Mode

On Thu, Jun 5, 2008 at 8:27 PM, Hans J. Koch <hjk@xxxxxxxxxxxxx> wrote:
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@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx> wrote:
On Wed, Jun 04, 2008 at 03:08:26PM +0900, Magnus Damm wrote:
From: Magnus Damm <damm@xxxxxxxxxx>

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.

It's still unclear to me. Please make a brief summary of your objections.

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.

Why wouldn't we use UIO for device within the Soc? I've been doing
that for quite some time now.

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.

Oh, I see. That's cpu specific code in my mind.

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.

Would you like me to write longer comments? Or some slides?

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.

No offense taken. But I can't really see your technical arguments. If
something in my code is unclear please ask before NAK.

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.

Maybe I will. =)

Thank you.

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

Relevant Pages

  • Re: [PATCH] ne.c fix for hibernate and rmmod oops fix
    ... Notable changes since the last patch: ... code doesn't even need the resources passed by the platform device. ... The ne.c driver already registers the resources it needs and there ... Note the user space ISA PnP doesn't work for suspend to disk, ...
    (Linux-Kernel)
  • Re: [PATCH] uio_pdrv: Unique IRQ Mode
    ... So your main objection against this patch is that you cannot use it ... The objection is that your code offers no advantages. ... Why wouldn't we use UIO for device within the Soc? ... I haven't seen any such driver yet. ...
    (Linux-Kernel)
  • Re: [PATCH 2.6.16-mm1 3/3] rtc: add support for m41t81 & m41t85 chips to m41t00 driver
    ... hardware-specific header file under include/linux. ... The platform-specific code for any platform that uses that rtc will need ... I have a patch for arch/ppc/platforms/katana.c but I haven't ... If no platform data is found, the driver will attempt to ...
    (Linux-Kernel)
  • Re: [PATCH] uio_pdrv: Unique IRQ Mode
    ... This patch adds a "Unique IRQ Mode" to the uio_pdrv UIO platform driver. ...
    (Linux-Kernel)
  • [PATCH 1/3] UIO: dont let UIO_CIF and UIO_SMX depend twice on UIO
    ... where all uio drivers are defined. ... these hunks were part of a patch in my first series but that patch was dropped ... Driver for Hilscher CIF DeviceNet and Profibus cards. ... tristate "SMX cryptengine UIO interface" ...
    (Linux-Kernel)