Messages in this thread | | | From | Arnd Bergmann <> | Subject | Re: [Patch 1/1 v2] via-sdmmc: VIA MSP card reader driver support | Date | Tue, 10 Feb 2009 13:18:08 +0100 |
| |
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 <><
| |