On Fri, Aug 19, 2011 at 01:27:41PM +0900, Magnus Damm wrote: > On Fri, Aug 19, 2011 at 12:30 PM, Simon Horman <horms@xxxxxxxxxxxx> wrote: > > On Fri, Aug 19, 2011 at 12:09:50PM +0900, Magnus Damm wrote: > >> On Fri, Aug 19, 2011 at 10:10 AM, Simon Horman <horms@xxxxxxxxxxxx> wrote: > >> > Provide separate interrupt handlers which may be used by platforms where > >> > SDHI has three interrupt sources. > >> > > >> > This patch also removes the commented-out handling of CRC and other errors. > >> > > >> > Cc: Guennadi Liakhovetski <g.liakhovetski@xxxxxx> > >> > Cc: Magnus Damm <magnus.damm@xxxxxxxxx> > >> > Signed-off-by: Simon Horman <horms@xxxxxxxxxxxx> > >> > > >> > --- > >> > >> > +irqreturn_t tmio_mmc_irq(int irq, void *devid) > >> > +{ > >> > + struct tmio_mmc_host *host = devid; > >> > + unsigned int ireg, status; > >> > + > >> > + pr_debug("MMC IRQ begin\n"); > >> > + > >> > + tmio_mmc_card_irq_status(host, &ireg, &status); > >> > + if (__tmio_mmc_card_detect_irq(host, ireg, status)) > >> > + return IRQ_HANDLED; > >> > + if (__tmio_mmc_sdcard_irq(host, ireg, status)) > >> > + return IRQ_HANDLED; > >> > + > >> > + tmio_mmc_sdio_irq(irq, devid); > >> > > >> > -out: > >> > return IRQ_HANDLED; > >> > } > >> > EXPORT_SYMBOL(tmio_mmc_irq); > >> > >> Is there any particular reason for returning early in this interrupt > >> handler? By returning early I mean the "if ... return IRQ_HANDLED" > >> cases above. > >> > >> I realize the old ISR code in the driver does just this, so if the > >> goal is to stay compatible then I guess we should keep this behavior. > >> >From my point of view it usually makes more sense to try to handle all > >> events that may be associated with the IRQ. > > > > My original post had the behaviour that you suggest but Guennadi > > indicted that he would be much more comfortable with keeping the original > > behaviour as it is know to work on a wide range of hardware. > > I see, thanks for your patience... > > So I may remember this wrong, but for the 3 different interrupt > sources I believe that the SDIO IRQ code was added by Arnd for one of > the Renesas SDHI platforms. Back then Ian disliked supporting more > than a single interrupt source, so for that reason the SDIO IRQ code > was added on top of the common interrupt handler. We had no > documentation either, so it was added in a rather random way. I recall > the SDIO IRQ being handled before the other interrupt types in the > common handler not last, but that's not very important. Anyway, with > the fact that SDIO IRQ support was added for SDHI platforms in mind > then we can assume that other platforms won't need it. Not sure if > this fact will improve our situation or not. > > As for the two remaining interrupts, I believe they share hardware > registers somehow. I guess I'm OK keeping the original behavior > somehow, but I still believe it's incorrect. It may not matter very > much though since it's rather unlikely that hotplug insertion or eject > coincides with the data IRQs. Hi Magnus, to be honest I'm unsure if the current behaviour is correct or not. But it does appear to work on a wide range of hardware. And as tmio_mmc_irq() is an implementation of the single-source ISR intended for legacy purposes it does seem reasonable to maintain the existing behaviour. If there is a need to change the behaviour of tmio_mmc_irq() in some way then I think that is best dealt with in a separate patch (series) subsequent to the current series that is intended to introduce broken-out handlers. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html