On Mon, Aug 15, 2011 at 10:17:28AM +0200, Guennadi Liakhovetski wrote:
> Hi Simon
>
> On Mon, 15 Aug 2011, Simon Horman wrote:
>
> > The SDHI driver already supports making use of up to three interrupt
> > sources.
> >
> > This series breaks up the existing interrupt handler into three handlers,
> > one for card access, one for card detect interrupts, and one for SDIO
> > interrupts. A cover-all handler, which makes use of these new broken-out
> > handlers is provided for for the case where there is only one interrupt
> > source.
>
> The idea is good, thanks for the patches. Only I'm not sure I find the way
> you split the patches extremely intuitive;-) How about:
>
> [PATCH 1/x] cache IRQ masks
> * in this patch I'd propose to cache SD-card and SDIO IRQ masks in struct
> tmio_mmc_host, instead of reading them every time from the hardware
Sure, that sounds reasonable - though it seems somewhat orthogonal to my series.
I'll check over the code, but it seems that you are implying that
the masks never change.
> [PATCH 2/x] split the ISR
> * in this patch you split the IRQ handler directly into the final form as
> after the first your 3 patches, without intermediate steps, also adding
> them to the header
The current split was intended to make bite-size patches that
are easy to review. I'm happy to combine the patches as you
suggest if that is what you prefer.
> [PATCH 3/x] SDHI: use specialized ISRs when available
> * you know what to do here:-) Also, I'd
> #define SH_MOBILE_SDHI_IRQ_SDCARD 0
> #define SH_MOBILE_SDHI_IRQ_CARD_DETECT 1
> #define SH_MOBILE_SDHI_IRQ_SDIO 2
>
> and use these defines both in platforms
>
> }, [1 + SH_MOBILE_SDHI_IRQ_SDCARD] = {
> ...
I think I see what you are getting at there.
I will try and make it so.
> and in sh_mobile_sdhi.c, instead of going "case 2:" Please, also consider
> unfolding the loop over platform IRQs in probing, it might look better
> flat.
I agree the current loop isn't entirely clean.
I'll unroll it and see how things look.
> "card_access" in function names I would replace with "io" or "data,"
> "card_irq" with "sdcard_irq" because I believe, that "SD card" is a proper
> identification to pure storage card in SD format, as opposed to SDIO
> cards.
Sure, if you would prefer that naming scheme.
> Also, maybe you can double-check, whether you really need all those
> functions with names, beginning with a double underscore, and whether
> better names wouldn't be possible for them.
The motivation behind that aspect of the implementation is
to allow re-use of code while avoiding extra register reads.
I believe the two sdio functions could be collapsed into a single function
while only losing (probably unused) debugging information. I will do that,
though I decided against that option previously for the sake of consistency.
For the card_irq() functions I think it is a bit difficult to collapse things
because the access and detect handlers actually need to read the same
register(s).
--
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
