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 <linux@xxxxxxxxxxxxxxxx> wrote: > On Wed, Feb 19, 2014 at 03:39:35PM +0530, Viresh Kumar wrote: >> 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.. >> 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.. >> 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.. -- 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
