linux-mips
[Top] [All Lists]

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

To: Jordan Crouse <jordan.crouse@amd.com>
Subject: Re: [PATCH] ALCHEMY: Add SD support to AU1200 MMC/SD driver
From: Pierre Ossman <drzeus@drzeus.cx>
Date: Fri, 02 Dec 2005 20:42:02 +0100
Cc: linux-mips@linux-mips.org, ralf@linux-mips.org, Russell King <rmk+lkml@arm.linux.org.uk>
In-reply-to: <20051202190108.GF28227@cosmic.amd.com>
Original-recipient: rfc822;linux-mips@linux-mips.org
References: <20051202190108.GF28227@cosmic.amd.com>
Sender: linux-mips-bounce@linux-mips.org
User-agent: Mail/News 1.5 (X11/20051129)
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


<Prev in Thread] Current Thread [Next in Thread>