In article <01033116451801.00335@pinky.paradox.demon.co.uk> you write:
There's one thing I want to check up on though, I'm not happy with the way 
>I've had to do the irq handler attach:
>The patch wanted to do:
>! 	npsc->sc_ih.ih_func = dp8390_intr;
>! 	npsc->sc_ih.ih_arg = dsc;
>! 	npsc->sc_ih.ih_level = IPL_NET;
>! 	npsc->sc_ih.ih_name = "if_ne";
>! 	npsc->sc_ih.ih_maskaddr = npsc->sc_podule->irq_addr;
>! 	npsc->sc_ih.ih_maskbits = npsc->sc_podule->irq_mask;
>! 
>! 	if (irq_claim(npsc->sc_podule->interrupt,&(npsc->sc_ih)) < 0) {
>
>The code submitted, does:
>	/* Install an interrupt handler */
>	evcnt_attach_dynamic(&npsc->sc_intrcnt, EVCNT_TYPE_INTR, NULL,
>	    self->dv_xname, "intr");
>	npsc->sc_ih = podulebus_irq_establish(pa->pa_ih, IPL_NET, dp8390_intr,
>	    dsc, &npsc->sc_intrcnt);
>	if (npsc->sc_ih == NULL)
>		panic("%s: Cannot install interrupt handler",
>		   dsc->sc_dev.dv_xname);
>+	/* this feels wrong to do this here */
>+	npsc->sc_ih->ih_maskaddr = npsc->sc_podule->irq_addr;
>+	npsc->sc_ih->ih_maskbits = npsc->sc_podule->irq_mask;
>
>It works, but it just feels wrong to update the maskaddr and maskbits 
>after the handler was established.  What is the correct thing to do?  add 
>extra params to irq_establish for irq_addr and irq_mask (irq_establish is 
>really just a wrapper around intr_claim (which wrappers irq_claim))?

Last time I checked, the IOMD IRQ handler completely ignored those fields,
which is why podulebus_irq_establish() doesn't bother filling them in.  My
recommendation would thus be to do the same unless you have a good reason to
do otherwise (in which case, I want to know about it).

I don't think the possible performance gain from using them on machines
with many podules is worth the extra complexity -- most drivers only need to
read one register to know whether an interrupt's for them anyway.

-- 
Ben Harris                                                   <bjh21@netbsd.org>
Portmaster, NetBSD/arm26               <URL:http://www.netbsd.org/Ports/arm26/>