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

[Russell King - ARM Linux <linux@arm.linux.org.uk>]

On Wed, Feb 19, 2014 at 03:39:35PM +0530, Viresh Kumar wrote:
> On 19 February 2014 15:22, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > If you don't want to use a regulator, then the right way to do this is to
> > use the set_ios callback and check the power field - anything which is not
> > MMC_POWER_OFF should result in power to the card being turned on.
> 
> 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

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

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

> 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?

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".