On Mon, Aug 15, 2011 at 10:17:28AM +0200, Guennadi Liakhovetski wrote:

Hi Simon

On Mon, 15 Aug 2011, Simon Horman wrote:

quoted

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).