On Sunday, February 02, 2014 at 02:52:52 PM, Mark Brown wrote:
Hi, thanks for preparing this patch!
I have just a few very minor nitpicks, ignore if you please.
[...]
+static int spi_map_buf(struct spi_master *master, struct device *dev,
struct sg_table *sgt, void *buf, size_t len,enum dma_data_direction dir)+{
- const bool vmalloced_buf = is_vmalloc_addr(buf);
- const int desc_len = vmalloced_buf ? PAGE_SIZE : master->max_dma_len;
You might want to rename this to "sg_chunk_max_size" or something, "desc_len" doesn't make much sense here. The variable describes the maximum size of one single scatterlist element.
- const int sgs = DIV_ROUND_UP(len, desc_len);
Looking at this, the variables could generally use a more meaningful name. I think it'd be clearer to call this "num_sg_chunks" or so ?
- struct page *vm_page;
- void *sg_buf;
- size_t min;
- int i, ret;
- ret = sg_alloc_table(sgt, sgs, GFP_KERNEL);
- if (ret != 0)
return ret;- for (i = 0; i < sgs; i++) {
min = min_t(size_t, len, desc_len);if (vmalloced_buf) {vm_page = vmalloc_to_page(buf);
Just curious, but shouldn't we check if buf != NULL right at the begining of this function?
[...]
+static void spi_unmap_buf(struct spi_master *master, struct device *dev,
struct sg_table *sgt, enum dma_data_direction dir)+{
- if (sgt->orig_nents) {
I don't want to nag, but why not use if (!sgt->...) return; ? This would cut down one level of indent.
dma_unmap_sg(dev, sgt->sgl, sgt->orig_nents, dir);sg_free_table(sgt);- }
+}
[...]