[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.dudhat at 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: wd at denx.de
Your own mileage may vary.