Re: [PATCH] Fix sd/sdio/mmc initialization frequency retries

[Chris Ball <cjb@laptop.org>]

Hi Andy,

This patch doesn't apply against mmc-next, which makes it hard to review
properly; could you rebase/resend?

I like the look of these changes, just a few comments:

On Tue, Dec 28, 2010 at 09:15:05AM -0800, Andy Ross wrote:
> @@ -1450,19 +1440,12 @@ void mmc_rescan(struct work_struct *work)
>  	if ((host->bus_ops != NULL) && host->bus_ops->detect && !host->bus_dead)
>  		host->bus_ops->detect(host);
> 
> -	mmc_bus_put(host);
> -
> -
> -	mmc_bus_get(host);
> -
>  	/* if there still is a card present, stop here */
>  	if (host->bus_ops != NULL) {
>  		mmc_bus_put(host);
>  		goto out;
>  	}

Let's leave these in -- I think we're depending on the side-effect of 
mmc_bus_put() clearing out host->bus_ops.  I'll check and submit a
separate patch adding a comment there explaining what's going on.

> +		/* Try SDIO, then SD, then MMC */
> +		if(mmc_attach_sdio(host) == 0)
> +			break;
> +		else if(mmc_attach_sd(host) == 0)
> +			break;
> +		else if(mmc_attach_mmc(host) == 0)
> +			break;

Space after if, and perhaps let's use !mmc_attach_*() rather than == 0
for brevity.  I guess we could also consider:

		/* Try SDIO, then SD, then MMC */
		if (mmc_attach_sdio(host) && mmc_attach_sd(host) &&
		    mmc_attach_mmc(host))
			mmc_power_off(host);

	mmc_release_host(host);

which would save quite a few lines, but perhaps it's less clear (or not
equivalent somehow).  What do you think?

> -out_fail:
> -		mmc_release_host(host);
> -		mmc_power_off(host);
> +              mmc_power_off(host);

This looks misaligned.

> + 	if((err = mmc_send_op_cond(host, 0, &ocr)))
> +		return err;

Redundant parens, and space after if.

> +	if((err =mmc_send_io_op_cond(host, 0, &ocr)))

As above, plus space after =.

Thanks!  You might also consider splitting this up into two separate
patches, but I'll leave that up to you.

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child