Re: [PATCH 5/5] omap mmc: Add new omap hsmmc controller for 2430and 34xx

[Tony Lindgren]

 * Tony Lindgren <tony@xxxxxxxxxxx> [081229 18:56]:
> Hi,
> 
> * Pierre Ossman <drzeus-mmc@xxxxxxxxx> [081221 18:17]:
> > On Sun, 07 Dec 2008 13:51:39 -0800
> > Tony Lindgren <tony@xxxxxxxxxxx> wrote:
> > 
> > > +	/*
> > > +	 * Unlike OMAP1 controller, the cmdtype does not seem to be based on
> > > +	 * ac, bc, adtc, bcr. Only CMD12 needs a val of 0x3, rest 0x0.
> > > +	 */
> > > +	if (cmd->opcode == 12)
> > > +		cmdtype = 0x3;
> > 
> > Isn't it more likely that it needs 0x3 for any commands that ends an
> > open ended transfer? I.e. every time cmd == mrq.stop.
> 
> OK, changed and still works :)
> 
> > > +	host->dbclk = clk_get(&pdev->dev, "mmchsdb_fck");
> > > +	/*
> > > +	 * MMC can still work without debounce clock.
> > > +	 */
> > > +	if (IS_ERR(host->dbclk))
> > > +		dev_dbg(mmc_dev(host->mmc), "Failed to get debounce clock\n");
> > 
> > Perhaps a warning should be printed though, as there could be some
> > problems caused by trying to init a card that is still bouncing.
> 
> OK
> 
> > > +static int omap_mmc_remove(struct platform_device *pdev)
> > > +{
> > > +	struct mmc_omap_host *host = platform_get_drvdata(pdev);
> > > +	struct resource *res;
> > > +	u16 vdd = 0;
> > > +
> > > +	if (!(OMAP_HSMMC_READ(host->base, HCTL) & SDVSDET)) {
> > > +	/*
> > > +	 * Set the vdd back to 3V,
> > > +	 * applicable for dual volt support.
> > > +	 */
> > > +		vdd = fls(host->mmc->ocr_avail) - 1;
> > > +		if (omap_mmc_switch_opcond(host, vdd) != 0)
> > > +			host->mmc->ios.vdd = vdd;
> > > +	}
> > > +
> > > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > +	if (res)
> > > +		release_mem_region(res->start, res->end - res->start + 1);
> > > +
> > > +	platform_set_drvdata(pdev, NULL);
> > > +	if (host) {
> > > +		mmc_remove_host(host->mmc);
> > 
> > You can't go fiddling with the voltage and removing the MMIO until
> > you've made sure that there is no more activity on the slot.
> 
> Changed in the attached patch. I've changed the code to reset
> the controller voltage setting after powering down the socket
> instead of trying to do it in two earlier places..
> 
> Do you have any better suggestions for this? Basically it looks
> like the controller may fail to detect the next card after unplugging
> a 1.8V card unless the controller is reset to 3V.
> 
> > Fix the above issues and you can let it go via Russell with a:
> > 
> > Acked-by: Pierre Ossman <drzeus@xxxxxxxxx>
> 
> Thank you for your comments. Care to take a one more look at the
> attached patch before I add your Ack? I also fixed the remaining
> checkpatch warnings.

Here's one more version that leaves out the unnecessary TWL4030
dependency in the Kconfig as pointed out by David Brownell.

> Regards,
> 
> Tony