On Fri, Aug 19, 2011 at 1:59 PM, Simon Horman <horms@xxxxxxxxxxxx> wrote:
> 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.
Sure, I agree.
> 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.
Sounds good, I agree that this should be handled separately if needed.
Thanks,
/ magnus
--
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
