lkml.org 
[lkml]   [2008]   [Jun]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] uio_pdrv: Unique IRQ Mode
    On Thu, Jun 5, 2008 at 8:27 PM, Hans J. Koch <hjk@linutronix.de> 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@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?
    >
    > 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


    \
     
     \ /
      Last update: 2008-06-08 12:23    [W:0.038 / U:0.556 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site