Re: HP Jornada 6xx PCMCIA driver

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

On Sun, 18 Nov 2007 19:03:37 +0100
Dominik Brodowski <li...@do...> wrote:

> Hi,
> 
> On Fri, Nov 16, 2007 at 12:36:52AM +0100, Kristoffer Ericson wrote:
> > Im in the process of both bugtracking and rewriting the hp6xx pcmcia
> > driver. It stopped working approx 2.6.17/2.6.18 but was in bad shape
> > before then so it sure needs a rewrite. Need to fix the bug first though.
> > 
> > Anyhow. When inserting a card I get unexpected IRQ at vector blabla. Im not
> > surprised by this but more in how its suppose to be done.
> > Please note that my issues are mainly related to IRQ, so thats why Im not
> > adding major code examples.
> > 
> > HP Jornada 6xx has an hd64461 chipset (arch/sh/cchips/hd6446x/hd64461.c)
> > where we have IRQ 36 that symbolizes 16 devices.
> > We use an demuxer to mask out where the interrupt is actually coming from
> > and then passes it to the correct interrupt handler.
> > 
> > Now PCMCIA has IRQ 78 (demuxed from IRQ 36) usually. Since we want to
> > divide what comes from the slot and pcmcia_card we create a software
> > demuxer. IRQ 78 -> demuxer (if PCMCIA then IRQ=79 otherwise cardstatus
> > IRQ=78).
> 
> Actually, why do you bother? The PCMCIA subsystem is capable of working with
> shared IRQs...
> 
So I should have same interrupt for both? Isn't it important to know where the IRQ came from? Thats what puzzles me.

> > static void hd64461_enable_irq(unsigned int irq)
> > {
> > 	hd64461_enable_int(irq);
> > }
> > 
> > static void hd64461_disable_irq(unsigned int irq)
> > {
> > 	hd64461_disable_int(irq);
> > }
> > 
> > static unsigned int hd64461_startup_irq(unsigned int irq)
> > {
> > 	hd64461_enable_irq(irq);
> > 	return 0;
> > }
> > 
> > static void hd64461_shutdown_irq(unsigned int irq)
> > {
> > 	hd64461_disable_irq(irq);
> > }
> > 
> > static void hd64461_mask_and_ack_irq(unsigned int irq)
> > {
> > 	hd64461_disable_irq(irq);
> > }
> > 
> > static void hd64461_end_irq(unsigned int irq)
> > {
> > 	hd64461_enable_irq(irq);
> > }
> 
> Since these functions seem to be (at least almost) the same, could you unify
> this?
>

Yeah, good point.

> > static int hd64461_pcmcia_irq_demux(int irq, void *dev)
> > {
> > 	hd64461_socket_t *sp = (hd64461_socket_t *) dev;
> > 	unsigned char cscr;
> > 	unsigned cscr_reg = HD64461_PCC0CSCR;
> > 
> > 	/* irq should be 78 here */
> > 	printk(KERN_INFO "hd64461_pcmcia_irq_demux(irq= %d)\n", irq);
> > 
> > 	/* If interrupt was due to pcmcia interrupt, then change it to 81 */
> 
> 81? I thought 79
> 
Just a bad comment, I've tried using all kinds of irqs. Currently its set to 78 main and demux 79.

> > int hd64461_init_socket(int sock, int irq, int io_irq, unsigned long mem_base,unsigned long io_offset)
> > {
> > 	sp->irq = irq;
> ...
> > 	sp->socket.pci_irq = io_irq;
> ...
> > 	if ((request_irq(irq, hd64461_interrupt, IRQF_DISABLED, "hd64461_ss-irq", sp)) < 0) {
> 
> I think this should be io_irq here -- the "socket" IRQ which is demuxed to 79
> is io_irq; and the "card" one stays at 78. (Better use these terms than
> "pcmcia" interrupt -- which is ambiguous).
>
Oki, so basicly IRQ 78 for socket IRQ? I'll changed the IRQF_DISABLED -> IRQF_SHARED then.

> > 		printk(KERN_INFO "hd64461_init: request for irq %d: failed\n", sp->irq);
> > 		return -1;
> > 	}
> > 	
> > 	/* setup io_irq chip data and move it through demux */
> > 	set_irq_chip(io_irq, &hd64461_ss_irq_chip);
> > 	hd64461_register_irq_demux(sp->irq, hd64461_pcmcia_irq_demux,sp);
> 
> Regarding the demux', see my comment above.
> 
If shared IRQ's work nicely, I would much rather use that obviously. That would get rid of the entire demux.

> > 	/* HD64461_IRQ_PCC0 = 64 + 14 = 78         */
> > 	/* HD64461_IRQ_PCC0 + 3 = 64 + 14 + 1 = 79 */
> 	                    + 1
> 
> Hope this helps a bit,
It does, thx! Btw, do you have any good suggestion on which drivers to look at for good reference?

> 
> 	Dominik


-- 
Kristoffer Ericson <Kri...@Gm...>