Re: [PATCH] ALCHEMY: Add SD support to AU1200 MMC/SD driver

[Pierre Ossman <drzeus@drzeus.cx>]

Jordan Crouse wrote:
> Add SD support to the AU1200 MMC driver.  This can
> be added post 2.6.15, I'm just sending them out today so the various
> maintainers can get them queued up. 
> 
> Signed-off-by: Jordan Crouse <jordan.crouse@amd.com>
> ---
> 
>  drivers/mmc/au1xmmc.c |  124 ++++++++++++++++++++++++++++---------------------
>  1 files changed, 71 insertions(+), 53 deletions(-)
> 

Russell is still the maintainer of the MMC layer. :)

I still have some comments though.

> @@ -124,8 +132,8 @@ static inline void IRQ_OFF(struct au1xmm
>  static inline void SEND_STOP(struct au1xmmc_host *host) 
>  {
>  
> -	/* We know the value of CONFIG2, so avoid a read we don't need */
> -	u32 mask = SD_CONFIG2_EN;
> +	/* Penalty box for Jordan - NEVER ASSUME! */
> +	u32 mask = au_readl(HOST_CONFIG2(host));
>  
>  	WARN_ON(host->status != HOST_S_DATA);
>  	host->status = HOST_S_STOP;

This comment will be terribly confusing to anyone reading your code.

> @@ -196,7 +207,11 @@ static int au1xmmc_send_command(struct a
>  
>  	switch(cmd->flags) {
>  	case MMC_RSP_R1:
> -		mmccmd |= SD_CMD_RT_1;
> +		if (cmd->opcode == 0x03 && host->mmc->mode == MMC_MODE_SD)
> +			mmccmd |= SD_CMD_RT_6;
> +		else
> +			mmccmd |= SD_CMD_RT_1;
> +
>  		break;
>  	case MMC_RSP_R1B:
>  		mmccmd |= SD_CMD_RT_1B;

No, no, no! Even if this wasn't already fixed in the current kernel you
never hack around bugs in other parts of the kernel, you fix them!

> @@ -504,8 +519,8 @@ static void au1xmmc_cmd_complete(struct 
>  		r[3] = au_readl(host->iobase + SD_RESP0);
>  		
>  		/* The CRC is omitted from the response, so really we only got
> -		 * 120 bytes, but the engine expects 128 bits, so we have to shift
> -		 * things up 
> +		 * 120 bytes, but the engine expects 128 bits, so we have to 
> +		 * shift things up 
>  		 */
>  		
>  		for(i = 0; i < 4; i++) {

s/bytes/bits/

> @@ -611,7 +635,7 @@ au1xmmc_prepare_data(struct au1xmmc_host
>  			
>  			int len = (datalen > sg_len) ? sg_len : datalen;
>  
> -			if (i == host->dma.len - 1)
> +		if (i == (host->dma.len - 1))
>  				flags = DDMA_FLAGS_IE;
>  
>      			if (host->flags & HOST_F_XMIT){

broken indentation.

> @@ -627,23 +651,11 @@ au1xmmc_prepare_data(struct au1xmmc_host
>  					len, flags);
>  			}
>  
> -    			if (!ret) 
> +    		if (ret == 0) 
>  				goto dataerr;
>  
>  			datalen -= len;
>  		}
> -	}
> -	else {
> -		host->pio.index = 0;
> -		host->pio.offset = 0;
> -		host->pio.len = datalen;
> -		
> -		if (host->flags & HOST_F_XMIT)
> -			IRQ_ON(host, SD_CONFIG_TH);
> -		else 
> -			IRQ_ON(host, SD_CONFIG_NE);
> -			//IRQ_ON(host, SD_CONFIG_RA|SD_CONFIG_RF);
> -	}
>  
>  	return MMC_ERR_NONE;
>  

Here aswell. And it seems the block above will get the wrong indentation.

Rgds
Pierre