Re: hd64465_ss & hd64461_ss

[Kristoffer E. <kri...@gm...>]

On Wed, 14 Nov 2007 14:08:41 +0900
Paul Mundt <le...@li...> wrote:

> On Tue, Nov 13, 2007 at 11:52:38PM +0100, Kristoffer Ericson wrote:
> > Just starting looking at pcmcia code again. When compairing to hd64465
> > code I found this segment:
> > 
> Both hd64461 and hd64465 do totally bogus IRQ handling, most of which
> should be moved over to arch/sh/cchips and corrected to work with the new
> API.
Agreed.

I would ask these questions on the pcmcia mailinglist, but they seem quite inactive. And despite trying to
get membership to their mailinglist, I only get 'your mail needs to be looked at by administrator..' and then nothing.

The hd64461 approach to pcmcia is to add an demux to make us detect where an interrupt
is coming from (pcmcia IRQ or slot IRQ). I haven't seen any examples of s/w demuxes anywhere else.
Other drivers tend to usually use polling interrupts.

Im finding it hard to think out the best solution though. I have a theory why we are getting this bug:
---------------------------------------------------------------------------
We set chip data for our io_irq (to get proper ack/disable/mask) and expect 
the pcmcia driver to aquire it without a problem. 
But by setting the chip data we make it alot harder (marked as already used?) for the pcmcia 
driver to get the irq. Since we don't have any handler we will generate a bad irq every time
IRQ is triggerd.

A possible solution would be to simply add the pcmcia disable/enable/ack code to 
the standard IRQ chip but I don't think thats the correct solution since we 
effectivly add code that only affect 1 / 16 IRQ's.

Another approach would be to have it first run the pcmcia chip data and after that the hd64461 data. That way
we would get IRQ_generated -> ack_mask by pcmcia chip -> ack_mask by pcmcia chip.

I can't say I fully understand how the pcmcia code works, but I know enough to see that
its the IRQ handling thats causing the issue.
> 
> > static void hs_mask_and_ack_irq(unsigned int irq)
> > {
> > 	hs_socket_disable_ireq(hs_mapped_irq[irq].sock);
> >         if (hs_mapped_irq[irq].old_handler != &no_irq_type)
> > 	    hs_mapped_irq[irq].chip->ack(irq);
> > }
> > 
> Again, this is simply crap. You are better off deleting everything that
> mentions 'irq' in the hd6446x pcmcia drivers and rewriting it completely.
Thanks for confirming that the code is bad.

> 
> We have proper APIs in place for chaining the handlers, demux, etc. all
> of which hd6446x tries to side-step. Surprisingly that doesn't work very
> well. Look at the other PCMCIA drivers (ie, not hd6446x) for examples on
> how to implement a less fucked up driver.
I've spent the day looking through the current IRQ API so I've started to clean it up.

Been googling and checking kernel source to find out what exactly chaining IRQ means. 
To me it looks like it basicly locks the IRQ, so that it cannot be aquired. That is desired
for example hd64461 IRQ since in itself its not an IRQ, its only an representation of different devices (through demux).

> 
> Also, your keyboard was blessed with an enter key, please consider using
> it when writing email.
Point taken :)