Re: [U-Boot] [PATCH v1] ppc/85xx: PIO Support for FSL eSDHC Controller Driver

[Wolfgang Denk]

Dear Dipen Dudhat,

In message <1252488547-26676-1-git-send-email-dipen.dud...@freescale.com> you 
wrote:
> PIO Mode Support for eSDHC Controller Driver on Freescale SoCs.

Please do not repeat the Subject: in the commit message.

You may want to add a little more of explanation, though.


...
> +     if (data->flags & MMC_DATA_READ) {
> +             blocks = data->blocks;
> +             buffer = data->dest;
> +             while (blocks) {
> +                     size = data->blocksize;
> +                     while (size && (!(irqstat & IRQSTAT_TC))) {
> +                             udelay(1000);
> +                             irqstat = in_be32(&regs->irqstat);

Do you really need to wait for one millisecond here? This looks a bit
long to me. 

> +                             databuf = in_le32(&regs->datport);
> +                             *((uint *)buffer) = databuf;
> +                             buffer += 4;
> +                             size -= 4;

Um... and I do not see any error checking or error handling here?

> +     } else {
> +             blocks = data->blocks;
> +             buffer = data->src;
> +             while (blocks) {
> +                     timeout = MAX_TIMEOUT;
> +                     size = data->blocksize;
> +                     irqstat = in_be32(&regs->irqstat);
> +                     while (!(in_be32(&regs->prsstat) & PRSSTAT_BWEN) && 
> --timeout);

Please at least move the ';' to a new line.

> +                     if (timeout <= 0) {
> +                             printf("\nData Write Failed in PIO Mode.");
> +                             return TIMEOUT;
> +                     } else {

Omit this "else", and unindent the following block.

> +                             while (size && (!(irqstat & IRQSTAT_TC))) {
> +                                     databuf = *((uint *)buffer);
> +                                     buffer += 4;
> +                                     size -= 4;
> +                                     udelay(1000);
> +                                     irqstat = in_be32(&regs->irqstat);
> +                                     out_le32(&regs->datport, databuf);

Again - is 1 millisecond ok, and don;t we need any error checking
here?

>  
> +#ifdef CONFIG_SYS_FSL_ESDHC_USE_PIO
> +     if (!(data->flags & MMC_DATA_READ)) {
> +             if ((in_be32(&regs->prsstat) & PRSSTAT_WPSPL) == 0) {
> +                     printf("\nThe SD card is locked. "
> +                             "Can not write to a locked card.\n\n");
> +                     return TIMEOUT;
> +                     }

Indentation incorrect.

> +                     out_be32(&regs->dsaddr, (u32)data->src);
> +             } else
> +                     out_be32(&regs->dsaddr, (u32)data->dest);

Indentation incorrect.

> @@ -201,6 +272,7 @@ esdhc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, 
> struct mmc_data *data)
>               return TIMEOUT;
>  
>       /* Copy the response to the response buffer */
> +     udelay(1000);

Why is this needed?


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de
Your own mileage may vary.
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot