Mark,
On Wed, 22 Jul 2009, Mark Brown wrote:
> On Wed, Jul 22, 2009 at 12:44:01PM +0200, Thomas Gleixner wrote:
> > On Tue, 21 Jul 2009, Mark Brown wrote:
>
> > > I'll need to have a more detailed look at that but it's not immediately
> > > clear to me how a driver (or even machine) should use that code - it
> > > looks more like it's intended to be called from within the IRQ
> > > infrastructure than from random driver code.
>
> > All it needs is to set handle_level_oneshot_irq for the interrupt line
> > of your I2C or whatever devices.
>
> > set_irq_handler(irq, handle_level_oneshot_irq);
>
> Yeah, I know - the issue I was having was that the use of set_irq_handler()
> seemed rather rude for driver code. Grepping around I see that there
> are actually a small number of drivers using it already but at first
> glance most of them appear to be implementing interrupt controllers. It
> was setting off alarm bells about abstraction layer violation like
> set_irq_type() does.
I don't think it belongs into the driver code. It belongs into the
platform code which sets up the system and knows what's on which
interrupt line.
> > > Nothing if the above works, though I guess more documentation wouldn't
> > > hurt (and possibly a more friendly wrapper). From the name and
>
> > Wrapper for what ?
>
> Something to package up the set_irq_handler() and request_threaded_irq()
> (possibly a flag for request_threaded_irq()). This is such a common
> thing and request_threaded_irq() looks so much like it should Just Work
> that I'd expect it'll help usability a lot to have a single function
> which says "this is the idiomatic way to implement this".
Yeah, we might do with a flag, but I'd prefer that the platform code
aka arch/xxx/yourmachine/setup.c does this. That's the canonical place.
> > After that bus_sync_unlock() is called outside the atomic context. Now
> > the chip implementation issues the bus commands, waits for completion
> > and unlocks the interrupt controller bus.
>
> I'll try to find time to implement some use of it and give it a spin -
> it looks good at first glance but I'll need to convert one of my drivers
> to genirq in order to test. Someone working on a chip that already uses
> genirq might get there first.
That'd be great to get some feedback. Both patches need some more
thought, but I think they are a good start to do some testing. One
thing which I definitely want to change is the thread_eoi function. We
probably want to have it customizable for the following reason:
main interrupt
hardirq handler wakes main thread handler
main thread handler
bus magic
subdevice1 "hardirq" handler wakes subdevice1 irq thread
subdevice2 "hardirq" handler wakes subdevice2 irq thread
main thread handler waits for subdevice1/2 handlers to complete
subdevice1 thread handler
bus magic
....
thread_fn returns
signal main thread handler via completion
subdevice2 thread handler
bus magic
....
thread_fn returns
signal main thread handler via completion
main thread handler resumes
bus magic
main thread handler returns from thread_fn
unmask main interrupt
So the thread_eoi function is useful for both the main and the
subdevice handlers. The main handler probably just issues the unmask
while the subdevice handlers probably want to call a completion to
notify the main thread handler that they are done. That would be a
fairly proper design I think and would encourage driver writers to
follow the common scheme instead of doing their own black magic.
Thinking further we should even provide some infrastructure for that
in the common code which would handle the completion and attach it to
the interrupts in question, so the driver authors would not have to
deal with that at all. They just would return from thread_fn and let
the generic code deal with the notification. The notification has to
be set up by the interrupt controller code. That way you can use the
same device driver code w/o knowledge of the interrupt controller
implementation it is attached to.
Thoughts ?
Thanks,
tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
