Re: [Patch 1/1 v2] via-sdmmc: VIA MSP card reader driver support

[Arnd Bergmann <arnd@arndb.de>]

On Tuesday 10 February 2009, JosephChan@via.com.tw wrote:
> The following patches provides VIA MSP SD/MMC card reader driver support.
> And these patches are based on kernel 2.6.29-rc4.
> Also, thanks for Arnd's previous suggestions.

Looks mostly good, just two more comments:

> --- a/drivers/mmc/host/via-sdmmc.h	1970-01-01 08:00:00.000000000 +0800
> +++ b/drivers/mmc/host/via-sdmmc.h	2009-02-10 21:34:18.000000000 +0800

The header file is used in only one place, which means that you could
move everything from there into via-sdmmc.c. This would make reading the
code easier later on.

> +
> +	if (data->flags & MMC_DATA_WRITE) {
> +		for (i = 0; i < len; i++) {
> +			tmpbuf = kmap_atomic(sg_page(&sg[i]), KM_BIO_SRC_IRQ);
> +			tmpbuf += sg[i].offset;
> +			memcpy(host->ddmabuf + offset, tmpbuf, sg[i].length);
> +			offset += sg[i].length;
> +			kunmap_atomic(tmpbuf, KM_BIO_SRC_IRQ);
> +		}
> +	}

This loop is broken for sg chaining. You should use for_each_sg() or
sg_next() instead of sg[i] indexing.

> +	if (data->flags & MMC_DATA_READ) {
> +		struct scatterlist *sg = data->sg;
> +		unsigned char *sgbuf;
> +		int i;
> +
> +		for (i = 0; i < data->sg_len; i++) {
> +			sgbuf = kmap_atomic(sg_page(&sg[i]), KM_BIO_SRC_IRQ);
> +			memcpy(sgbuf + sg[i].offset, host->ddmabuf + offset,
> +			       sg[i].length);
> +			offset += sg[i].length;
> +			kunmap_atomic(sgbuf, KM_BIO_SRC_IRQ);
> +		}
> +	}
> +

same here.

	Arnd <><