[PATCH RFC 07/31] mmc: sdhci: push card_tasklet into threaded irq handler

Thanks for removing my ST email id from this mail, I was about to do that
this time :)

On 19 February 2014 16:20, Russell King - ARM Linux
[off-list ref] wrote:

On Wed, Feb 19, 2014 at 03:39:35PM +0530, Viresh Kumar wrote:

quoted

I didn't get that completely here.. sorry completely out of touch on
this driver.
The power GPIO here is controlling the power to card socket. Once the card
is inserted, we need to enable power to the socket so that controller can
read back information from card.

Yes, and how is this different from any other implementation where power
to the socket is controlled?

This is what happens:
- You detect a card insertion/removal event
- You tell the mmc core about it via mmc_detect_change()
- The mmc core looks at the current state, and if the socket is powered down,
  it first goes through the power up sequence:
  - Apply power to the socket
    (a call to set_ios with MMC_POWER_UP and zero clock)
  - Apply clock to the socket
    (a call to set_ios with MMC_POWER_ON and non-zero clock)
  - Wait 74 clock cycles (as required by the spec)
- Start the detection by sending commands to the cards
- If no cards are detected, power down the socket

Okay, that's all we need then..

quoted

And this set_ios callback is not available to drivers above sdhci.c, i.e.
glues like sdhci-pltfm or spear.. so, how we can we get that back in
sdhci-spear.c?

This is the crux of the issue which this patch set tries to address.  sdhci
has become - as I've now described it several times - "a patchwork quilt of
hacks all cooperating to give the impression of a working driver".

Being honest, I haven't had a in depth look of your patchset. Was really busy
with some other stuff..

This is just another such hack, because rather than fixing sdhci.c properly,
you've hacked around it, thereby making sdhci.c worse.

What you could have done is turned the power control code in sdhci.c into
a library function, and sdhci_do_set_ios() calls a method in the sdhci_ops
to control the power.  What this means is that:

- if you use the standard sdhci power control support, then you can just
  hook the standard function in there.
- if you need to do something extra, you can place your function there
  to do your own stuff, and call the standard function to do the standard
  bits.
- if you don't need the standard functionality, then you can handle the
  power control entirely within your driver.

Not only does this result in a much cleaner implementation, it means that
going forward, any other such quirky stuff can be handled in the same way
by the "glues" rather than having to augment the horrid quirk/quirk2 stuff,
or coming up with hacks like sdhci-spear.c

If you have a look through this patch set, I've already done that with a
number of these methods - set_clock, set_uhs_signaling and reset.

Moreover, you don't end up fiddling with sdhci internals (such as the
card_tasklet) which then impact on attempts to clean that code up, which
is exactly why we're discussing this. :)

I agree to all above stuff..

quoted

Again, I can't test any modifications to this code and we need help from
Pratyush/Mohit on this..

The important thing is we understand why the code is the way it is today,
and work out some way of cleaning this up.  Do you think the above will
work?

Absolutely, it will work.

Thanks for your efforts in cleaning up this stuff..